chore: make mux-subtitles a global CLI argument #3282

beardypig · 2020-10-22T17:22:11Z

removes the plugin specific mux-subtitles arguments in favour a a global --mux-subtitles option.

bastimeyer

I had already made roughly the same changes on my local repo and was testing the subtitle muxing before committing and pushing it, but I ran into an issue with the muxing itself, so I took a break. The issue is unrelated to the parameter changes, but the whole muxing seems to be broken when the subtitle stream ends early and the stream gets shut down. See this input for the pluzz plugin for example (requires a French IP address):
https://www.france.tv/documentaires/voyages/1081133-les-voltigeurs-du-lagon.html

Anyway, I had also added this decorator to the base Plugin class and applied it to the _get_streams methods (or the according sub-calls where subtitles are supported, eg. VODs):

    @classmethod
    def supports_subtitles(cls, func):
        def decorator(self, *args, **kwargs):
            if not self.session.get_option("mux-subtitles"):
                self.logger.info("This plugin supports subtitle muxing via --mux-subtitles")
            return func(self, *args, **kwargs)

        return decorator

If you don't like the info log, then we need to annotate the supported plugin classes and add them to the docs (automatically).

bastimeyer · 2020-10-22T18:00:46Z

src/streamlink_cli/argparser.py

+    output.add_argument(
+        "--mux-subtitles",
+        action="store_true",
+        help="""
+        Automatically mux available subtitles in to the output stream.
+
+        If supported by the plugin.
+        """,
+    )


The parameter belongs into the transport group, I think.

You'll also have to add it to streamlink_cli.main.setup_options and streamlink.plugin.plugin. In streamlink.plugin.plugin, where the self.session.get_option call reads from, the parameter name needs to include a dash and not an underscore character. This needs to be changed in each plugin.

I figured as it affects the output it could go in there, but maybe transport makes sense.

re:decorator, that looks like a nice idea - but maybe it could be applied to the plugin and that information could be picked up in help somewhere. There is perhaps the possibility for something generic here too.

The more I look at it, I think that there is something we could do similar to how PluginArguments works for plugins, but for Streamlink that way other interfaces could interrogate streamlink to find out which options are supported.

bastimeyer · 2020-10-30T18:38:31Z

I believe once the subtitle muxing parameters have been merged into one global parameter, 2.0.0 should be pretty much ready, at least all the mandatory changes. Everything else is very minor and optional, and this here is probably the last remaining thing which makes sense including in a major version bump.

I'm not sure, what exactly you have in mind with a plugin-supported parameter list and how it'll work, but if this is too much effort now, this can be done later on as well, I believe. The only thing that makes these changes critical right now is the removal of the individual CLI parameters.

mkbloke · 2020-11-03T03:13:22Z

While subtitles are being addressed here, is it also worth considering better options for subtitle selection?

The problem at the moment is that not all sites will list subtitles in the standard locale format, i.e. formatted as [language_code]_[country_code]. Some sites may list subtitles in the locale format, but also have multiple versions of those, such as user contributed subs. This leads to the problem that, as far as I know, they cannot currently be selected.

There are perhaps 3 possible options to address this:

Relax the formatting on the --locale parameter so it will accept any string
Introduce another global parameter such as --subtitle-lang or --subtitle-select (this could maybe default to the system locale when it's not explicitly set)
Do neither of the above and implement the subtitle selection via a plugin specific parameter as required for those plugins

Thoughts?

bastimeyer · 2020-11-07T12:30:23Z

@mkbloke Different issue, let's not discuss this here.

@beardypig I've been working on adding global argument references to the plugins, like you've suggested, and I'm running into some issues due to how argparse works. My idea was this:

Add a global_arguments attribute to the Plugin class, similar to arguments, with the same usage of Arguments and Argument classes, but without any argument options, just the names. Then add a simple mux-subtitles "global-argument" to each of those plugins.
Upgrade streamlink_cli.main.setup_plugin_args and interate through the available global_arguments instances of each plugin and compare their dest attributes with the real global arguments. If it matches, then set the provided argument value on the plugin's options dict to make it available there. This makes it compatible with the existing code. Also create a plugins list on the matching argument and add the current plugin name to it, so that it can be used by the docs generator where it lists the available plugins of that argument. For a regular instance of Streamlink, this plugins list doesn't do anything as it's just meant for the docs.
Make docs/ext_argparse.py read that plugins attribute and add the list of supported plugins below the help text.

However, the issue I'm facing with this approach is that the docs extension is using a custom argparse "hook", because accessing the available/registered arguments on an argparse parser requires accessing private interfaces:

streamlink/docs/ext_argparse.py

Lines 37 to 59 in f0b35b6

    
           class ArgumentParser(object): 
        
               def __init__(self, *args, **kwargs): 
        
                   self.args = args 
        
                   self.kwargs = kwargs 
        
                   self.groups = [] 
        
                   self.arguments = [] 
        
               def add_argument(self, *args, **options): 
        
                   if not options.get("help") == argparse.SUPPRESS: 
        
                       self.arguments.append(_Argument(args, options)) 
        
               def add_argument_group(self, *args, **options): 
        
                   group = ArgumentParser(*args, **options) 
        
                   self.groups.append(group) 
        
                   return group 
        
           def get_parser(module_name, attr): 
        
               argparse.ArgumentParser = ArgumentParser 
        
               module = __import__(module_name, globals(), locals(), [attr]) 
        
               argparse.ArgumentParser = _ArgumentParser 
        
               parser = getattr(module, attr) 
        
               return parser if not(callable(parser)) else parser.__call__()

This means that the docs extension needs to be refactored for the private argparse interfaces. This is fairly straightforward though with one exception, and this is causing some issues which I haven't fully figured out yet and it has something to do with how argparse registers/processes the arguments.

bastimeyer · 2020-11-07T13:12:23Z

Nvm, found the issue... I will submit a PR later today or tomorrow.

chore: make mux-subtitles a global CLI argument

00205e0

beardypig added the WIP Work in process label Oct 22, 2020

beardypig mentioned this pull request Oct 22, 2020

chore: remove deprecated CLI arguments #3277

Merged

bastimeyer requested changes Oct 22, 2020

View reviewed changes

back-to added the needs rebase label Oct 31, 2020

bastimeyer mentioned this pull request Nov 8, 2020

plugins: turn mux-subtitles into a global argument #3324

Merged

back-to closed this in #3324 Nov 14, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

chore: make mux-subtitles a global CLI argument #3282

chore: make mux-subtitles a global CLI argument #3282

Uh oh!

beardypig commented Oct 22, 2020

Uh oh!

bastimeyer left a comment

Uh oh!

bastimeyer Oct 22, 2020

Uh oh!

beardypig Oct 22, 2020

Uh oh!

beardypig Oct 22, 2020

Uh oh!

beardypig Oct 23, 2020

Uh oh!

bastimeyer commented Oct 30, 2020

Uh oh!

mkbloke commented Nov 3, 2020

Uh oh!

bastimeyer commented Nov 7, 2020

Uh oh!

bastimeyer commented Nov 7, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Uh oh!

chore: make mux-subtitles a global CLI argument #3282

chore: make mux-subtitles a global CLI argument #3282

Uh oh!

Conversation

beardypig commented Oct 22, 2020

Uh oh!

bastimeyer left a comment

Choose a reason for hiding this comment

Uh oh!

bastimeyer Oct 22, 2020

Choose a reason for hiding this comment

Uh oh!

beardypig Oct 22, 2020

Choose a reason for hiding this comment

Uh oh!

beardypig Oct 22, 2020

Choose a reason for hiding this comment

Uh oh!

beardypig Oct 23, 2020

Choose a reason for hiding this comment

Uh oh!

bastimeyer commented Oct 30, 2020

Uh oh!

mkbloke commented Nov 3, 2020

Uh oh!

bastimeyer commented Nov 7, 2020

Uh oh!

bastimeyer commented Nov 7, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants