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

plugins.twitch: block ads using the same method as uBlock #3170

Closed
wants to merge 1 commit into from

Conversation

samfundev
Copy link

Implements uBlock's method of blocking ads on twitch. Which basically involves putting the platform=_ parameter in the access_token URLs. Doing that removes ad segments from the stream.

I removed the tests related to removing ad segments since that isn't done anymore. I'm not sure how I'd test the new way of disabling ads so I didn't add any tests.

Also closes #3120 as passthrough should now be compatible with disabling ads.

@codecov
Copy link

codecov bot commented Sep 12, 2020

Codecov Report

Merging #3170 into master will decrease coverage by 0.00%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##           master    #3170      +/-   ##
==========================================
- Coverage   52.67%   52.66%   -0.01%     
==========================================
  Files         244      244              
  Lines       15636    15637       +1     
==========================================
- Hits         8236     8235       -1     
- Misses       7400     7402       +2     

@bastimeyer
Copy link
Member

bastimeyer commented Sep 12, 2020

We've already had the platform=_ parameter added once and it worked for only a few days until Twitch made further changes.
#2358
#2357 (comment) (edit: wrong thread, we've had multiple thread, but I can't find the other one right now)
#2368
Then we removed it again:
#2692

Even if this seems to be working right now, I doubt that it will keep working.

If we decide to re-add this request parameter, then the only change should be the parameter itself (#2358), and the --twitch-disable-ads logic should not be removed.

@samfundev
Copy link
Author

It does seem like the parameter isn't a very consistent fix, sorry I didn't see that. I've changed it so the only changes are adding the parameter if the user asks for ads to be disabled.

@laichiaheng
Copy link

It really needs to be merged as soon as possible, otherwise the low latency streaming with no ads is unusable.

@bastimeyer
Copy link
Member

bastimeyer commented Sep 13, 2020

adding the parameter if the user asks for ads to be disabled

The twitch-disable-ads parameter shouldn't be checked here, because its purpose is to change the HLS client logic. Acquiring an access token and setting the platform=_ request parameter however should also work when the user is querying the resolved HLS stream URL when setting the --stream-url or --player-passthrough=hls parameters for example, which means the platform=_ request parameter should be applied in all cases (see 1bb3f86). Please fix this, otherwise I'll have to re-apply 1bb3f86, which I don't want to do because you made this suggestion of (re-)adding the request parameter. Thanks.

otherwise the low latency streaming with no ads is unusable.

Not true. The LL streaming still works fine when there are preroll ads.
See #3120 (comment)
I've submitted #3169 yesterday with a fix for that incorrect info log message, but it hasn't been reviewed yet. Just ignore the incorrect info message for now.

@bastimeyer bastimeyer added the plugin issue A Plugin does not work correctly label Sep 13, 2020
@laichiaheng
Copy link

laichiaheng commented Sep 13, 2020

adding the parameter if the user asks for ads to be disabled

The twitch-disable-ads parameter shouldn't be checked here, because its purpose is to change the HLS client logic. Acquiring an access token and setting the platform=_ request parameter however should also work when the user is querying the resolved HLS stream URL when setting the --stream-url or --player-passthrough=hls parameters for example, which means the platform=_ request parameter should be applied in all cases (see 1bb3f86). Please fix this, otherwise I'll have to re-apply 1bb3f86, which I don't want to do because you made this suggestion of (re-)adding the request parameter. Thanks.

otherwise the low latency streaming with no ads is unusable.

Not true. The LL streaming still works fine when there are preroll ads.
See #3120 (comment)
I've submitted #3169 yesterday with a fix for that incorrect info log message, but it hasn't been reviewed yet. Just ignore the incorrect info message for now.

I saw another thread said that it was a player problem, but MPV didn't have this problem with streamlink, why?
Did Twitch change something?

@bastimeyer
Copy link
Member

This is unrelated to this PR and shouldn't be discussed here.
I've also answered this plenty of times.
#3165 (comment)

@samfundev
Copy link
Author

My apologizes, I was busy yesterday but I'm glad this got fixed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
plugin issue A Plugin does not work correctly
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Twitch showing ads/commercials again
3 participants