Skip to content

plugins.artetv: only pick the first variant of the stream #3228

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 1 commit into from
Oct 17, 2020

Conversation

beardypig
Copy link
Member

@beardypig beardypig commented Oct 6, 2020

Removes the f4m and http streams from the returned list, and only returns the default variant for the stream. I believe this will match the expected behaviour.

see #3167

@bastimeyer bastimeyer added the plugin issue A Plugin does not work correctly label Oct 7, 2020
@bastimeyer
Copy link
Member

There are still potentially up to 4 additional alternative HLS streams for each stream input which would override each other's stream selections. Just like I've described in #3167 (comment).

If there really is a need for those alt-streams other than the main one, then we should add a plugin parameter for that, which selects those alt-streams.

@bastimeyer bastimeyer changed the title plugins.atretv: simplify the list of streams returned plugins.artetv: simplify the list of streams returned Oct 7, 2020
@bastimeyer
Copy link
Member

Please check the commit message prefix. There's a typo in the plugin name.

@beardypig beardypig force-pushed the arte-simplified branch 3 times, most recently from c5c66ad to ebba262 Compare October 9, 2020 11:41
@codecov

This comment has been minimized.

@beardypig
Copy link
Member Author

There are still potentially up to 4 additional alternative HLS streams for each stream input which would override each other's stream selections. Just like I've described in #3167 (comment).

I'm not sure which ones would come up as alt, I believe the list of codes that are filtered out are only the regular audio tracks. If you could provide a counter case that would be helpful.

If there really is a need for those alt-streams other than the main one, then we should add a plugin parameter for that, which selects those alt-streams.

A plugin parameter could be added to select which stream you wanted, you'd also need an option to list them. Regardless, that should be a separate PR as this is intended to fix the immediate issue highlighted in #3167.

@bastimeyer
Copy link
Member

I'm not sure which ones would come up as alt

I think renaming the yielded streams could just work. Add the code as prefix with lowercase text and underscores instead of dashes. There will be a lot more streams to select, but at least they don't override each other. A plugin parameter would then not be needed.

@beardypig
Copy link
Member Author

After closer inspection I see there is a much better way to do it by selecting the default variant, I will also prepare a PR with a variant selection option that can be review separately, I think it makes more sense than listing 20 streams.

@beardypig beardypig changed the title plugins.artetv: simplify the list of streams returned plugins.artetv: only pick the first variant of the stream Oct 9, 2020
Copy link
Member

@bastimeyer bastimeyer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checked the API responses again and it looks like this is a good enough solution for now.

@beardypig, if you want to make one little refinement, then please move the log = logger.getLogger(__name__) definition up, underneath the imports, and also change the log.error call so that the logger receives the already formatted message string (this way, it can get translated into an f-string later on automatically), thanks.

@beardypig beardypig force-pushed the arte-simplified branch 2 times, most recently from 25ee167 to cdcea57 Compare October 16, 2020 09:33
def _create_stream(self, streams):
variant, variantname = min([(stream["versionProg"], stream["versionLibelle"]) for stream in streams.values()],
key=itemgetter(0))
log.debug("Using the '{0}' stream variant".format(variantname))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

error with py2 because of Français

$ streamlink https://www.arte.tv/fr/direct/ -l info
src/streamlink_cli/main.py:760: PythonDeprecatedWarning: Python 2.7 ...
[cli][info] Found matching plugin artetv for URL https://www.arte.tv/fr/direct/
error: 'ascii' codec can't encode character u'\xe7' in position 11: ordinal not in range(128)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good spot. This should fix it.

Suggested change
log.debug("Using the '{0}' stream variant".format(variantname))
log.debug(u"Using the '{0}' stream variant".format(variantname))

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed it on the error log too.

@back-to back-to added the plugin enhancement A new feature for a working Plugin label Oct 17, 2020
@back-to back-to merged commit a23c3e3 into streamlink:master Oct 17, 2020
@quantenschaum
Copy link

Nice, now selecting the best stream consistently returns the stream with the "right" language. But what, if I want to manually select another stream (different language/subtitles)? The 720_alt or 720p_alt2 are no longer available now.

@beardypig
Copy link
Member Author

@quantenschaum as a compromise there is only the primary language stream available right now, to change the language we need an extra option.

@quantenschaum
Copy link

Yes, please do so. The option --hls-audio-select does not have an effect on the arte plugin. How can I download the stream in a different language? Maybe by changing the URL to point the page page in a different language? Is the order of streams different between the different language versions of the page?

resiproxy pushed a commit to resiproxy/streamlink that referenced this pull request Nov 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
plugin enhancement A new feature for a working Plugin plugin issue A Plugin does not work correctly
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants