plugins.twitch: implement disable-ads parameter #2372
Conversation
- Move tag specific logic and segment/playlist creation logic into individual methods so that the parser can be extended more easily. - Allow load method to set additional keyword arguments on the parser.
- Enable setting custom arguments when reloading the playlist. - Instantiate own class in HLSStream.parse_variant_playlist.
Codecov Report
@@ Coverage Diff @@
## master #2372 +/- ##
==========================================
+ Coverage 52.59% 52.77% +0.18%
==========================================
Files 237 237
Lines 14802 14860 +58
==========================================
+ Hits 7785 7843 +58
Misses 7017 7017 |
Codecov Report
@@ Coverage Diff @@
## master #2372 +/- ##
==========================================
+ Coverage 52.51% 52.71% +0.19%
==========================================
Files 238 238
Lines 14863 14923 +60
==========================================
+ Hits 7806 7867 +61
+ Misses 7057 7056 -1 |
|
Getting this after applying the changes: Never seen it before with PotPlayer and it's allowed through firewall. Does it make sense for this change to add player-specific problems? Should I try MPV? EDIT: Back up and running with MPV; no ads so far. Good stuff. |
|
Yes, please do so. |
|
I'd be more interested in the Since ads can be skipped at the beginning, there's no data for the player to read, which is the reason why you're seeing this. This still needs to be changed. I haven't touched streamlink_cli's code yet. You're also using the HTTP transport method and PotPlayer. Please use a player which is reading from |
bastimeyer
force-pushed
the
plugins/twitch/disable-ads
branch
from
4051790 to
56e9e82
Compare
|
Can someone please apply it to the latest source, compile it, upload it to u.teknik.io and post the link here? |
|
No, this is not how this works. You'll have to wait for this to be merged and then published in the next (nightly) release or you can build it on your own by cloning my Streamlink fork. We will not upload an official Streamlink build to an external site, please stop asking and spamming this PR with questions unrelated to the code changes, thank you. |
bastimeyer
force-pushed
the
plugins/twitch/disable-ads
branch
from
56e9e82 to
ebb3f77
Compare
|
I've made some further changes and removed the worker reference from the parser. Also added support for the |
bastimeyer
force-pushed
the
plugins/twitch/disable-ads
branch
2 times, most recently
from
43a6016 to
66b100c
Compare
|
PR seems to work as advertised (with I've observed that skipping the ads using this flag causes the preroll segments to always be present in the playlist. My guess is that not actually downloading the stream segments containing the adverts, results in Twitch not acknowledging the ad as having been watched. So the PR may be inadvertently be shooting itself in the foot by not downloading those segments. Do you think that always downloading the ad segments in the background would perhaps be a better way to go? Repro 1: Using Executed 4 times sequentially: Resulting in: Invocation 1: Preroll ad in m3u8, have to wait for ~8 ad segments Result is that the m3u8 contains preroll adds on all 4 invocations. Repro 2: Allowing the ad to play Execute 4 times sequentially: Invocation 1: Preroll ad in m3u8, fetched and played fully |
|
@alexzorin thanks for testing this. As I've said, I haven't been getting ads myself the past couple of days, so I can't test this on real stream data, which makes it a bit difficult. All segments, including ads, will now be downloaded, but then filtered out in the I've also improved the logging a bit and it will now log to the info channel when it will begin or stop skipping segments (instead of logging each skipped segment individually in the debug channel). |
|
Thanks, with 71301da , my previous repro no longer happens - now ads are gone after the first Hard to work in such a fuzzy environment! Twitch could change how they maintain ad viewing state at any time (at the moment I think it's bound to the First invocation (single ad in playlist)Subsequent invocation (no ads in playlist) |
bastimeyer
force-pushed
the
plugins/twitch/disable-ads
branch
from
71301da to
70c03e1
Compare
|
Hello, I've tested the changes and it seems to result in (sort of) unseekable streams. It would be usable in live situations, but for watching vods it would be detrimental not being able to seek to any part of the whole video. To elaborate a bit, when using hls-passthrough you get no seeking, when using --player-continuous-http you get seeking on a livestream, however with a local http server you won't get the full vod length when opening a vod. You will only be able to seek the amount you've buffered the stream for. If the changes are made as is and I'm not just missing something obvious, people will lose the ability to seek full vod ranges when this is merged. Please feel free to correct me if it's an easy setting fix. |
|
Streamlink doesn't support any kind of seeking. When using Streamlink, the player will consume a progressive stream from Seeking in the player is only possible if you're using the This further means that when using the passthrough option, you won't be affected by these changes at all and you will continue seeing ads unless the player can filter them out on its own. |
|
I must be missing something then. When using a build without these changes I don't want to make this a troubleshooting thread if it really cannot be the changes made, so I guess I'll just use a portable build without the changes for vods in that case. |
|
No, you're right, something broke and it's not using the passthrough (see with |
- Implement TwitchHLSStream{,Worker,Writer,Reader} and TwitchM3U8Parser
- Skip HLS segments between SCTE35-OUT{,-CONT} and SCTE35-IN tags
- Add --twitch-disable-ads parameter (plugin specific)
- Add tests
bastimeyer
force-pushed
the
plugins/twitch/disable-ads
branch
from
70c03e1 to
94edeb0
Compare
|
Re-running failed AppVeyor task - failed due to not being able to connect to codecov ... |
|
Yep, worst CI service of all time... Btw, there's still the problem of players refusing to work when the initial stream data is missing when it gets filtered out. This would require some changes how the output methods are called in |
|
Im testing this pr at the moment. No software expert here but one thing i noticed is, that since im on the last commit i have higher cpu usage then im used to by streamlink. Running streamlink on manjaro 18.04 and using vlc. cpu is a i7-7500u 2.7Ghzx4 and 16gb ram.Normally streamlink takes about 5 to 10% cpu power when i watch a stream on best, right now its around 15-25% cpu power. |
|
@bastimeyer it seems like @theinfinityproject which options are you using? |
|
@beardypig Both I'm done so far with my changes here (I think), so if I could get a proper review so that it can be merged and we can continue with the changes for the |
@beardypig pretty much the default ones, for example yesterday: streamlink --twitch-disable-ads twitch.tv/wagamamatv best |
|
@theinfinityproject and you don't have anything set in the |
|
There are no changes here which would lead to an increase like this. That sounds more of an aggregation error of the process's CPU usage or something else unrelated. Here's a comparison of Streamlink built from 42105d8 (the commit on the master branch which this PR is based on) and 94edeb0 (the HEAD of this PR) on my ancient system and running it for 300 seconds. |
|
@beardypig no, i didnt alter streamlinkrc. PS: Im not familiar with git since i have a background in humanities and only basic linux knowledge but if u tell me how to revert the commit with git then i could compare it like u did |
|
If there are no further comments or concerns, please somebody go ahead and merge (don't squash). For the moment I don't care if there are players which are having troubles with missing data when it gets filtered out. The twitch-disable-ads parameter is optional and if it's causing problems, people can simply not use it or switch to a different player. As I've said earlier, a fix for that should be submitted in a new pull request. @theinfinityproject I've shown you in my comment earlier that your issue is not related to this pull request and caused by something else. Your own GNU time output also shows a usage of 6% over 300s (with a high system-level load, though) while running the python process, so it's definitely not 25%. |
|
Looks good to me @bastimeyer so I will merge, no squash as requested. Thanks for adding this! |
…able-ads plugins.twitch: implement disable-ads parameter
Resolves #2368, #2357 (deleted, for whatever reason, which is really sad - I've opened a support ticket to get it hopefully restored)
Please see #2369 and my comment here first:
#2369 (comment)
Adds a custom
TwitchHLSStreamimplementation and the--twitch-disable-adsCLI parameter, which will skip HLS segments between#EXT-X-SCTE35-OUTand#EXT-X-SCTE35-INtags.If this PR gets merged, please don't squash the commits.