plugins.twitch: always filter out ads #6579
Conversation
|
The docs will need an update before this can be merged. Leaving this open until that's done. Discussions are welcome. |
|
I think this is a good idea in general, but I think it's worth taking another look over the ad detection code before pushing it in. I'm mostly concerned about false positives incorrectly filtering segments, I'm wondering if this might be a little too aggressive. IIRC Twitch also sometimes triggers banner ads through the playlist, while keeping live segments, I wonder if that will also trigger the ad detection for the period of the banner ad? Unfortunately I can't actually seem to get Twitch to serve me any banner ads right now so I haven't been able to confirm one way or another...I can also imagine other kinds of ads that could trigger this kind of check if they implement them in the future, maybe PiP ads or audio ads or something. This also seems to be a bit out of date, there seems to be various titles the ads can have now, besides just "Amazon". I'm seeing "DMC" and also just random numbers as well. For now, as far as I've seen, the |
I have not seen any false positives for years, or maybe ever. If there are false positives, for whatever reason, then this should be fixed separately. I am not concerned at all about the current ad filtering implementation, unless Twitch changes their ad metadata, in which case it needs to be fixed anyway. Here's the plugin's commit history. The last ad-related change was about logging ad durations and a revert of ad detection in prefetch segments which was not working correctly, but which was then fixed. This happened last year. Previous ad-related changes were years ago. As said, I haven't seen any false positives.
This is about dateranges with the twitch-specific Here's the respective test for these attributes: streamlink/tests/plugins/test_twitch.py Lines 336 to 385 in ba6959c
Those are not embedded ads then that replace the actual stream. They can't add segments for separate streams in the same playlist without massively violating the HLS protocol, and I doubt they would do this, even with custom metadata.
That's a fallback check which was implemented in 8105bfc three years ago, in order to avoid more expensive daterange checks if the detection is as simple as checking known segment titles. It's not important to remove this. It's simply a redundancy.
Please show me a false positive, and then we can remove redundant ad checks. The highest priority is filtering all potential ads, not being as efficient as possible with the ad detection. |
I believe the likelihood anyone would actually notice a false positive is low, they would have to
The overlap between all of these is pretty much non-existent, so I would never expect anyone to notice, but that doesn't necessarily mean it couldn't be happening.
Yes, but I'm saying I believe I've seen this
IMO it should be the opposite, the ad filtering should be as lax as possible while still filtering all ads currently know about, which matching Personally speaking, if this went through as is, I would revert it on my local fork because I don't trust the current code to not filter non-ads. |
bastimeyer
force-pushed
the
plugins/twitch/remove-disable-ads
branch
from
dbc1875 to
0090022
Compare
The "while still filtering all ads" is the important part, which was the reason why the current implementation on master exists. There have been cases where we didn't catch all ads, so multiple redundant checks were added in the past. This was also done at a time where the embedded ads were new. As said, I personally have not seen any cases were the current implementation on master made false positive detections. But we can remove the redundant I'd like to see a playlist with contents that would result in a false positive though. So far, this is only a speculative claim based on the ads filtering implementation that checks for the In this recent push to the PR branch I've also added trace log messages for ad dateranges and ad segments. The whole daterange data gets logged and only the relevant parts of the filtered out segment. If there are false positives, then this should log them. The |
I've been trying and failing to get Twitch to serve me any banner ads, at this point I'm assuming they're simply not serving them currently for whatever reason. The best I can do for now is point you to a section of Twitch's Javascript to prove this false positive is possible: If you search for {
...
isVLM: '1' === i['X-TV-TWITCH-AD-VLM'],
...
}where |
Drop `--twitch-disable-ads` and always filter out ads. Suppress the plugin argument, so we don't introduce breaking changes. Suppressed plugin arguments automatically show a deprecation warning. The reason for this change is that Twitch has switched from MPEG-TS to fragmented MPEG-4 streams for their higher quality streams that are currently in beta. This is also the case for any re-encoded lower stream qualities if the input stream is available in a higher quality. This is independent of the used video codec. Unfortunately, stream discontinuities that occur during an ad-transition when not filtering out ads are harder to recover from when the stream uses an fMPEG-4 container, causing more severe playback issues in players compared to MPEG-TS streams when incoherent stream segments of the actual stream and ads are concatenated. This means that we need to always filter out ads and don't let the users decide, to ensure the least amount of playback issues among all players. Since the requested `playerType` has always been "embed" when requesting an access token, only the purple "ads in progress" screen was shown instead of actual ads, so making the plugin always filter out ads (aka "non-stream segments") is the sensible thing to do. Keep the "Will filter out ads" info log message that was previously only shown when the plugin argument was set, so the user is made aware of this change, as gaps in the output will now always occur.
Checking for `X-TV-TWITCH-AD-` attributes in `EXT-X-DATERANGE` tags is unnecessary. It was previously used as a fallback mechanism.
bastimeyer
force-pushed
the
plugins/twitch/remove-disable-ads
branch
from
0090022 to
ce00f5e
Compare
Drop
--twitch-disable-adsand always filter out ads. Suppress the plugin argument, so we don't introduce breaking changes. Suppressed plugin arguments automatically show a deprecation warning.The reason for this change is that Twitch has switched from MPEG-TS to fragmented MPEG-4 streams for their higher quality streams that are currently in beta. This is also the case for any re-encoded lower stream qualities if the input stream is available in a higher quality. This is independent of the used video codec.
Unfortunately, stream discontinuities that occur during an ad-transition when not filtering out ads are harder to recover from when the stream uses an fMPEG-4 container, causing more severe playback issues in players compared to MPEG-TS streams when incoherent stream segments of the actual stream and ads are concatenated. This means that we need to always filter out ads and don't let the users decide, to ensure the least amount of playback issues among all players.
Since the requested
playerTypehas always been "embed" when requesting an access token, only the purple "ads in progress" screen was shown instead of actual ads, so making the plugin always filter out ads (aka "non-stream segments") is the sensible thing to do.Keep the "Will filter out ads" info log message that was previously only shown when the plugin argument was set, so the user is made aware of this change, as gaps in the output will now always occur.