Skip to content

Move plugin options to plugin instances and remove global plugin arguments #5033

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Jul 2, 2023

Conversation

bastimeyer
Copy link
Member

These are breaking changes of the Streamlink session and Plugin APIs.

Opening this as a draft for now, even though it's pretty much final, because I don't think that this has to be merged now. It simply fixes an old design flaw and is not too important and should be included with other breaking changes in the future in one go.


Two separate changes:

1. Moving Plugin.options to plugin instances

As mentioned in #5011, turning Plugin.options from a plugin class attribute into a plugin instance attribute is related to the removal of the Plugin.bind() class method in #4744 / #4768, and ideally should've been part of the 5.0.0 release. While 5.0.0 removed Plugin.bind(), the breaking changes were actually in the Streamlink session class where Plugin.bind() was called internally, and compatibility wrappers were added to the Plugin constructor, with deprecation messages for old-style parameters. This here is another set of breaking changes on the Streamlink session class and Plugin class, but without any deprecations.

Just to recap what I've said in #5011 and what I've included in the commit messages of this PR, the Plugin class previously shared a common Options instance as the Plugin.options class attribute. This class attribute was then overridden by streamlink_cli.main.setup_plugin_args() where plugin arguments were processed and added to the argparser while reading default values of the plugin arguments that were added to a new Options instance.

This design is bad for several reasons. First, this makes streamlink_cli a "soft" dependency of the Plugin and Streamlink classes, as all plugin options would otherwise be shared between all plugins on the same Options instance. Second, multiple instances of the same plugin always have the same set of options set (not really important, but still bad). And third, tests and third party projects not using streamlink_cli need to manually override/clear the Plugin.options class attribute when initializing multiple plugins, which is bad.

Now, after these changes:

  • The Plugin class can receive an optional Options instance as a parameter on its constructor, which streamlink_cli uses for setting up individual plugin options. Having it in the Plugin constructor is important, because plugin subclasses need to be able to read plugin options immediately (see Twitch and TwitchAPI), and not after initialization.
  • Since plugin options are not stored on plugin classes anymore, Streamlink.get_plugin_option() and Streamlink.set_plugin_option() have been removed. These methods were useful for sharing state between different parts of Streamlink, e.g. plugins and streams (see Twitch and TwitchHLSStream), but this can be done in different ways. The PR therefore also adds the keywords parameter to HLSStream.parse_variant_playlist() to be able to pass keywords to the {,Muxed}HLSStream classes. In case of the Twitch plugin, these keywords are disable_ads and low_latency which are read from the plugin options instead of session.get_plugin_option().

2. Removal of Argument.is_global

I've already had this on my radar for a while. Since the changed CLI methods already had to deal with global plugin arguments, I decided to include the removal of global plugin arguments in this PR.

The reason for the removal of global plugin arguments is simple: it adds unnecessary complexity for absolutely no gain. The value which a global plugin argument stores is already stored in the session options and should be read from there.

is_global was added when we removed individual plugin arguments for muxing subtitles, but no other global plugin arguments were ever added. The only benefit it had was being able to include a list of plugins in the docs which were making use of --mux-subtitles, as the global plugin arguments allowed for parsing this data. This additional data in the docs is now gone as well, but only affects four plugins. If we want to have a list of certain features a plugin uses, then this can be done by extending the regular plugin metadata.

What plugin implementors have to do to fix this is very simple, remove any global plugin arguments and change the option lookup, e.g. self.get_option("mux_subtitles") to self.session.get_option("mux-subtitles").

@bastimeyer
Copy link
Member Author

Rebased to master and fixed merge conflict

@bastimeyer bastimeyer force-pushed the plugin/options branch 2 times, most recently from 4ef1049 to ae946fd Compare January 25, 2023 14:30
@bastimeyer
Copy link
Member Author

bastimeyer commented Jan 25, 2023

Rebased to master and fixed merge conflict again. No other changes.

I'm not happy about the keywords keyword arg in HLSStream.parse_variant_playlist though. This is just an ugly hack on an already bad design, just so that HLSStream and MuxedHLSStream are able to receive custom data from plugins, because those can't be retrieved from the session anymore. Ideally, the entire parse_variant_playlist classmethod should be reworked and how the HLS stream instances are returned, but that's very much out of scope of this PR.

@bastimeyer bastimeyer force-pushed the plugin/options branch 2 times, most recently from 1207bcf to 90ad58c Compare February 4, 2023 12:05
@bastimeyer
Copy link
Member Author

Rebased to master and resolved new merge conflicts:

Plugin options are currently shared between all plugin instances via the
`Plugin.options` class attribute, unless it gets overridden by
Streamlink's CLI via `setup_plugin_args()` after reading the plugin
arguments, updating the argument parser and reading the default values.

This allows for accidental pollution of the plugin options when not
overriding the `options` class attribute with a new `Options` instance.
Since this is only done by `streamlink_cli`, this is an issue in tests
and third party python projects using the Streamlink API.

- Remove `Plugin.options` class attribute and store an `Options`
  instance on the `Plugin` instance instead, and allow passing an
  already initialized options instance to the `Plugin` constructor
- Turn `Plugin.{g,s}et_option()` from class methods to regular methods
- Update CLI and initialize the resolved plugin with an options instance
  that gets built by `setup_plugin_options()`, with default values read
  from the CLI arguments
- Remove `Streamlink.{g,s}et_plugin_option()` and fix usages:
  - Session and Options tests
  - Twitch plugin, TwitchAPI, TwitchHLSStream, and their tests
- Add `keywords` mapping to `HLSStream.parse_variant_playlist`,
  to be able to pass custom keywords to the `{,Muxed}HLSStream`
- Move and rewrite CLI plugin args+options integration tests
@bastimeyer bastimeyer marked this pull request as ready for review July 2, 2023 19:49
@bastimeyer
Copy link
Member Author

Rebased to master again and fixed merge conflicts. No other changes.

@bastimeyer bastimeyer merged commit 7f2ea44 into streamlink:master Jul 2, 2023
@bastimeyer bastimeyer deleted the plugin/options branch July 2, 2023 20:00
@vstavrinov
Copy link
Contributor

Please remove set_plugin_option from docs:

https://streamlink.github.io/api.html

and put there explanation and example of how to set plugin options for now

@bastimeyer
Copy link
Member Author

Please remove set_plugin_option from docs:

Have a look at the latest docs.
https://streamlink.github.io/latest/api/session.html#streamlink.session.Streamlink

Plugin.options
self.options.get()
self.options.set()

self.options = Options() if options is None else options

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants