Skip to content

Conversation

@bastimeyer
Copy link
Member

Since there hasn't been another response in #3170 and I don't want to wait any longer, I'm opening a PR myself with the platform=_ "workaround".

As I've said in the other PR, we've already had added the platform=_ request parameter to the access token API endpoint when embedded ads were first implemented, but we removed it again when it became obsolete after a few days when Twitch made further changes.

This is basically the same situation and I don't expect this "workaround" to keep working indefinitely. We'll see.

Added first in: #2358
Stopped working in: #2368
Got removed in: #2692

Closes #3170

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

codecov bot commented Sep 14, 2020

Codecov Report

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

@@            Coverage Diff             @@
##           master    #3173      +/-   ##
==========================================
- Coverage   52.67%   52.66%   -0.01%     
==========================================
  Files         244      244              
  Lines       15636    15636              
==========================================
- Hits         8236     8235       -1     
- Misses       7400     7401       +1     

@beardypig
Copy link
Member

If it works, it works. The difference between this and the other PR seems to be that this changes it to always send platform=_ and the other PR only sends it when the disable ads option is used?

@bastimeyer
Copy link
Member Author

If it works, it works.

Yes, I was asking for a quick review so that this can be merged, because I'm getting spammed on various channels due to the embedded ads. I won't merge my own PRs without some kind of response or acknowledgement.

difference

See #3170 (comment)

It should be this way, because otherwise it would require to set --twitch-disable-ads when using --stream-url or --player-passthrough=hls, and that's not the point of the disable ads parameter. Since I got no response yesterday and today, I've opened a new PR myself with the previous implementation.

So if there are no objections, please merge, so that this issue can be solved. We may have to publish a new release because of this, not sure. My other PR with the rewrite of the custom Twitch HLS stuff is not that important. It fixes the incorrect log message though.

@gravyboat gravyboat merged commit 5a4a200 into streamlink:master Sep 14, 2020
@gravyboat
Copy link
Member

Looks fine to me, merged. If it only fixes it for a small amount of time oh well. Better that than people constantly creating issues we have to review.

@bastimeyer bastimeyer deleted the plugins/twitch/embedded-ads-workaround branch October 9, 2020 06:52
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.

3 participants