Skip to content
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

cli: implement --player-external-http-continuous #4739

Merged

Conversation

bastimeyer
Copy link
Member

Resolves #4738

No tests, because no tests have ever been written for output_stream_http() or the entire streamlink_cli.utils.http_server module.


There are two choices that can be made for implementing the new CLI argument:

  1. The current implementation, with --player-external-http-continuous=bool requiring a boolean value and affecting the logic of the main argument --player-external-http.
    The issue with this is that it's not consistent with the logic of the --player-http and --player-continuous-http arguments, which are individual arguments. It's also not consistent with the naming scheme (which I just realized), so it probably has to be changed to --player-external-continuous-http.
  2. Having two individual CLI arguments, --player-external-http and --player-external-http-non-continuous, similar to --player-http and --player-continuous-http. As you can see, the default logic of --player-external-http already is "continuous", so the second argument name has to be negated, which is not great.

Thoughts?

@nbgibson
Copy link

Of the two paths listed here I'd have to lean towards the former (renamed to be in spec with the rest of the command schema of course), though I'm not too much a fan of either for the reasons listed above. As is I think there is potential for a larger rework of of how the server option tree is handled, but that's beyond the scope of this particular PR.

My reasoning is:

  • As is we already have to pass a separate flag for configuring ports when the "better" flow would be to just optionally tack on a desired target port to --player-external-http, so having another flag or two isn't exactly new. It's not ideal, but it's also not exactly a deviation at this time.
  • The flow here isn't really a deviation from --player-external-http imo as that process flow (from my understanding) is still on the "desktop" side of things albiet with a web based serving method. --player-external-continuous-http and so forth are all off in headless land.

@bastimeyer
Copy link
Member Author

  1. It's also not consistent with the naming scheme (which I just realized), so it probably has to be changed to --player-external-continuous-http

Hm, that's not consistent with --player-external-http-port then... I think I will just leave it as it is. The parameter naming scheme isn't particularly well thought out anyway (eg. --verbose-player), and using natural language for parameter names instead of logical/layered suffixes makes everything an inconsistent mess anyway.

I think there is potential for a larger rework

Yes. After Livestreamer got forked into Streamlink, the CLI was never cleaned up and it's a total mess of historically grown features built on top of each other over several years by lots of different contributors. That's why nobody has tried refactoring or rebuilding the CLI package, and it's still a mess today. And starting from scratch is even more difficult if feature parity needs to be achieved without introducing (many) breaking changes. One feature in regards to the external http server for example was being able to serve multiple clients at once, but implementing this on top of the current code just doesn't make sense from a maintenance perspective.

just optionally tack on a desired target port to --player-external-http

I think this has been avoided intentionally when it was implemented, because optional argument values are a bit weird. The argument could be changed to nargs="?", action="store", which would make the value optional.

@back-to back-to merged commit 8c1430e into streamlink:master Aug 21, 2022
@bastimeyer bastimeyer deleted the cli/player-external-http-continuous branch August 21, 2022 08:32
Billy2011 added a commit to Billy2011/streamlink-27 that referenced this pull request Aug 29, 2022
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.

cli: option for running --player-external-http in "non-continuous" mode
3 participants