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

Twitch support #301

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Twitch support #301

wants to merge 1 commit into from

Conversation

biolds
Copy link
Contributor

@biolds biolds commented Jan 8, 2023

This PR adds Twitch support and fixes some small bugs

@biolds
Copy link
Contributor Author

biolds commented Jan 8, 2023

Please let me know if you prefer that i split it into multiple PR.

@meeb
Copy link
Owner

meeb commented Jan 10, 2023

This looks pretty good, thanks! I've given it a brief check-over and all looks fine. There is some work I have in a branch to redo how sources are verified but I can easily modify your Twitch patch later so all good. The only modification I might not merge is the change to the default codecs, I picked VP9 and OPUS specifically, not only is VP9 a much better video codec than H264 and has pretty wide support these days it compresses video better as well. VP9 and OPUS are also the only free and open source codecs that match availability.

Have you fully tested Twitch channels as a source? I've had a look at the yt-dlp metadata available for a Twitch channel and video and they seem to be in a compatible format, I basically never use Twitch though so supporting this could be problematic if it differs from YouTube. E.g. codec format changes that require updates to the matching spam functions.

@biolds
Copy link
Contributor Author

biolds commented Jan 10, 2023

Yeah, I have changed the default codecs because Twitch does not provide VP9 and Opus, also it seems some VP9 format can't be played inside <video> tags. Not sure what would be the best solution for that, we could drop them in the Twitch form, or even display the list of available formats returned by yt-dlp to select from a dropdown. What do you think?

I have done some good testings, but it could be great to do a bit more, i'm not sure i have covered everything.
For the metadata, iirc yt-dlp returns a dict with the same keys, though the content is not exactly the same. I had to tinker a bit to make it work as can be seen in utils.parse_twitch_media_format

@meeb
Copy link
Owner

meeb commented Jan 11, 2023

Ah, Twitch not supporting VP9 could be an issue then. VP9 should work fine in HTML5 video tags in all browsers for most of a decade if you have the right codec support. I might just eventually merge these and leave it as VP9 but put a message on it about Twitch. Downloading will still work as the matching will just drop back to AVC1 so it's not terrible user experience if you do pick VP9, just unexpected. Long term I'm pondering a source onboarding wizard that could handle this automatically. I did note your utils.parse_twitch_media_format, I've just zero experience with Twitch so maintaining it if it breaks might be problematic. Looks fine as it is now though if you're confident it works.

@biolds
Copy link
Contributor Author

biolds commented Jan 11, 2023

It may be just me that broke it when testing, but I've not seen the automatic switch occurred when the requested codec is missing, that's why I changed the default. Let's wait a bit before merging this PR, I have seen an issue when using it on more streams, I'll update when I have a fix. Feel to merge other stuff in the mean time if you want to, I'll rebase the PR.

@meeb
Copy link
Owner

meeb commented Jan 12, 2023

If the matching failover to any working codec doesn't work then something likely need tweaking either with the matching code or your format translation wrapper. If you set a source for VP9 and there's no VP9 format available it should still match AVC1 in the closest resolution. Feel free to move everything apart from the Twitch and default codec changes into something I can merge if you would like that merged before debugging the Twitch support.

@biolds biolds changed the title Twitch support, bugfixes Twitch support Jan 15, 2023
@biolds
Copy link
Contributor Author

biolds commented Jan 15, 2023

Yep, i've opened a separate MR.

@biolds
Copy link
Contributor Author

biolds commented Mar 28, 2023

I've just updated the PR, the last bug i noticed is now fixed. Though, it's currently impacted by an ytdl-p bug, which is fixed, but not yet released, see yt-dlp/yt-dlp#6500

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

Successfully merging this pull request may close these issues.

2 participants