Codecov Report

Merging #2895 into master will decrease coverage by 0.08%.
The diff coverage is 96.96%.

@@            Coverage Diff             @@
##           master    #2895      +/-   ##
==========================================
- Coverage   52.68%   52.59%   -0.09%     
==========================================
  Files         252      250       -2     
  Lines       15769    15715      -54     
==========================================
- Hits         8308     8266      -42     
+ Misses       7461     7449      -12     

I'm able to get ads every time in a Chrome Incognito window (logged out) but I don't see anything particularly useful past what you're seeing here. What data do you want? I don't know if there is any way to trick Twitch in this sense as it looks to me like they either present an ad, or they present the commercial break notification, there's no stream data to pull.

It seems to me like the big issue with players is being caused by buffering. They're not sending any stream data prior to the commercial break ending, so when it does end that's when you finally get stream data and it causes problems for the player because nothing is cached. It behaves similarly to when I was experimenting with extreme low latency live edge settings. I am fine with making MPV the default suggested player, but the issues brought up by people still have not been addressed in any way by the MPV team. The lack of official builds being by far my biggest concern.

@gravyboat It's possible that ads are not shown to logged in users. Doesn't help Streamlink though, as we can't use any auth data when acquiring the access token (users can still set a custom auth HTTP header I believe).

Regarding MPV, I don't have an answer here. My points from the linked issue still stand, I guess.

Pushed another clean up commit. This should be in a better shape overall now.
If there are preroll ads, another info message will be logged. I think this is better than just saying "Will ignore ad segments".

I like the updated comment @bastimeyer, makes it more clear regarding what is going on.

There might be a problem with the current implementation. I didn't run into non-preroll ads yesterday (pre-roll ads were filtered out just fine), but it's possible that Twitch requires HLS clients to still download the segments, like mentioned last year here:
#2372 (comment)

That would mean that the filtering can't be done in the parser anymore and the removal of the custom StreamWriter has to be reverted.

Unfortunately, this can't be tested right now as the embedded ads have stopped.

Reverted the custom StreamWriter removal. Ad segments will now be always downloaded and then discarded afterwards.

Even if Twitch currently doesn't embed ads, it's safe to assume that this new system will be used in the future. Since this PR doesn't only implement the new ad filtering logic, but also removes the unnecessary restriction that low latency streaming and ads filter can't be used at the same time, I'd like to see this being included in the next release.

Being able to always set the low latency streaming parameter, even for non-LL streams, is a good thing, because it overrides the default playlist refresh timer, which is stupid IMO. The default refresh time is 6 seconds and causes 3 whole segments to be queued each time, meaning that with a default live edge value of 3, a short hickup while refreshing the playlist may cause a hickup in the playback if the player's own cache isn't large enough. And short delays while reloading the playlist happen a lot.

This makes me wonder whether a parameter for manually overriding the refresh time would make sense, maybe with support for both static time values and keywords like min-segment-length or so.

@bastimeyer I agree. There's no reason not to include it and it will save us time going forward from answering repetitive questions. It has no negative impact. I really like the idea of adding an option to manually override the refresh timer, I've run in to this myself where I git a hiccup just because it's waiting for the playlist to update a bit after opening a stream.

Noticed some "incorrect" values in the tests which are now fixed and also patched the HLSStreamWorker's wait method, so that the tests run without unnecessarily waiting between playlist refreshes.