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

Allow to disable twitch reruns #2722

Merged
merged 2 commits into from Dec 2, 2019

Conversation

@skulblakka
Copy link
Contributor

skulblakka commented Dec 1, 2019

This adds the option to disable twitch reruns similar to disabling hosted streams.

I know the check for the stream_type could also be directly done in _get_hls_streams. If you like that better tell me and I can change it :)

Edit: I decided to change it. This should have less changes and I think my previous solution broke the tests for the skip-ads option.

Closes #2721

@codecov

This comment has been minimized.

Copy link

codecov bot commented Dec 2, 2019

Codecov Report

Merging #2722 into master will decrease coverage by 0.01%.
The diff coverage is 20%.

@@            Coverage Diff             @@
##           master    #2722      +/-   ##
==========================================
- Coverage   52.47%   52.46%   -0.02%     
==========================================
  Files         246      246              
  Lines       15431    15436       +5     
==========================================
+ Hits         8098     8099       +1     
- Misses       7333     7337       +4
@gravyboat

This comment has been minimized.

Copy link
Member

gravyboat commented Dec 2, 2019

Thanks @skulblakka, I didn't realize the reruns were flagged by the stream type.

@gravyboat gravyboat merged commit 11370e5 into streamlink:master Dec 2, 2019
3 checks passed
3 checks passed
codecov/project 52.46% (target 30%)
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@EnterGin

This comment has been minimized.

Copy link

EnterGin commented Dec 2, 2019

Thank you!

def _get_hls_streams(self, stream_type="live"):
self.logger.debug("Getting {0} HLS streams for {1}".format(stream_type, self.channel))
self._authenticate()
self._hosted_chain.append(self.channel)

if stream_type == "live":
if self._check_for_rerun() and self.options.get("disable_reruns"):

This comment has been minimized.

Copy link
@bastimeyer

bastimeyer Dec 2, 2019

Member

This will make an API call even if reruns are not disabled, which is unnecessary and will delay launching the stream by a bit.

This comment has been minimized.

Copy link
@gravyboat

gravyboat Dec 3, 2019

Member

Bah my bad @bastimeyer I totally forgot that it would cause an API call which noticeably delayed starting the stream.

@@ -697,12 +703,19 @@ def _check_for_host(self):
self.logger.info("{0} is hosting {1}".format(self.channel, host_info["target_login"]))
return host_info["target_login"]

def _check_for_rerun(self):
return self.api.streams(self.channel_id)["stream"]["stream_type"] == "rerun"

This comment has been minimized.

Copy link
@bastimeyer

bastimeyer Dec 2, 2019

Member

Btw, according to the kraken v5 documentation, supported values are live, playlist and all, but that's not true. There's also a second attribute, broadcast_platform, which isn't documented, but is related to reruns and IIRC, it was used by Twitch before they added the stream_type attribute. Both attributes have changed quite a bit over the past years after reruns were implemented by Twitch. For example, there also was the watch_party value which was used to indicate reruns.

Even if stream_type is the documented property, I am not sure if it's the right property to check. Most attribute names can be used for filtering streams via a GET parameter, like game for example, but stream_type can not be used for filtering whereas broadcast_platform can, which is weird. See here:

curl -s \
  -H "Accept: application/vnd.twitchtv.v5+json" \
  -H "Client-ID: phiay4sq36lfv9zu7cbqwz2ndnesfd8" \
  "https://api.twitch.tv/kraken/streams?stream_type=rerun&limit=1" \
  | jq '.streams[0] | {stream_type: .stream_type, broadcast_platform: .broadcast_platform}'
{
  "stream_type": null,
  "broadcast_platform": null
}
curl -s \
  -H "Accept: application/vnd.twitchtv.v5+json" \
  -H "Client-ID: phiay4sq36lfv9zu7cbqwz2ndnesfd8" \
  "https://api.twitch.tv/kraken/streams?broadcast_platform=rerun&limit=1" \
  | jq '.streams[0] | {stream_type: .stream_type, broadcast_platform: .broadcast_platform}'
{
  "stream_type": "rerun",
  "broadcast_platform": "rerun"
}

Then there's also the problem of broadcasters not setting the rerun flag while streaming a rerun. The Twitch GUI does some additional regex checks of the channel's status message (stream title) in addition to checking both stream_type and broadcast_platform attributes.

This comment has been minimized.

Copy link
@skulblakka

skulblakka Dec 2, 2019

Author Contributor

Btw, according to the kraken v5 documentation, supported values are live, playlist and all

To be honest I totally forgot to check the kraken documentation. I was basically going by what the website was doing. Wasn't the playlist feature replaced by the reruns feature anyway? Maybe they are just no longer updating the docs?

stream_type can not be used for filtering whereas broadcast_platform can, which is weird

Filtering by stream_type was removed on their Helix API a while ago. Maybe the same issues / reasons existed on Kraken too and they decided to remove it there as well?

Then there's also the problem of broadcasters not setting the rerun flag while streaming a rerun.

My intention was to only disable reruns done via Twitch's rerun feature. Are you sure broadcasters have to flag their stream as a rerun when using it?

This comment has been minimized.

Copy link
@bastimeyer

bastimeyer Dec 2, 2019

Member

I totally forgot to check the kraken documentation

The documentation is complete garbage, so forget about that. Checking for rerun as value should be correct, but what I meant is that I'm not sure if it's enough since there's also the broadcast_platform attribute. The entire API is a total mess and you can't be 100% sure unless they document it properly, which they don't. Not sure about the playlist streams and whether they were removed, but there's also the is_playlist attribute. I think they added this after the playlist stream announcement and the documentation is just old and therefore still includes the playlist value.

Filtering by stream_type was removed on their Helix API a while ago.

I haven't really followed the past developments on the helix API, but if you say that stream_type has been there and was removed (it's currently not there), then it's probably the right attribute to check on kraken and they've simply forgot to remove it there as well. The fact that it's been removed doesn't sound good though.

Are you sure broadcasters have to flag their stream as a rerun when using it?

What I meant is that there are many channels which don't set the rerun flag while re-streaming a locally recorded stream and they just set a rerun stream title. See the various ESL channels for example (Dota2, CSGO, etc).

@EnterGin

This comment has been minimized.

Copy link

EnterGin commented Dec 2, 2019

There also a problem occurs when stream is offline
1

@gravyboat

This comment has been minimized.

Copy link
Member

gravyboat commented Dec 3, 2019

@skulblakka Do you have time to try and fix this at some point? Otherwise maybe we should revert since this is kind of hard to test. I only know of a few streamers like followgrubby that regularly have reruns. Seems like the items bastimeyer noted might be biting us in the butt a bit here, might be worth adding some tests as well? I didn't think about how poorly patched together the API is when I merged this.

@skulblakka

This comment has been minimized.

Copy link
Contributor Author

skulblakka commented Dec 3, 2019

I'm sorry for the mess I caused.

@bastimeyer Like I mentioned I assumed the OP of the issue was talking about reruns made via the Rerun-Feature. That's why I didn't consider "live"-streams that broadcasted a video. Getting streams by type was removed March 2018 on Helix (although the field there is only called type) and other values than live were removed from the documentation. Do you think filtering by stream_type is enough or should I also add broadcast_platform?

@gravyboat I have to leave for work now but I will try fixing this in the evening (and will also see about some tests).

@gravyboat

This comment has been minimized.

Copy link
Member

gravyboat commented Dec 3, 2019

@skulblakka There is no need to apologize or any reason to stress about it. Your contribution and desire to implement this is appreciated. There's no release currently planned so we're not in a rush, I just wanted to clarify whether you wanted to work on it or if you were going to be too busy to do so and we should back it out. Don't pressure yourself or inconvenience your personal time.

@bastimeyer

This comment has been minimized.

Copy link
Member

bastimeyer commented Dec 3, 2019

I'll submit a PR with a fix later. Currently adding some tests... No pressure...

bastimeyer added a commit to bastimeyer/streamlink that referenced this pull request Dec 3, 2019
bastimeyer added a commit to bastimeyer/streamlink that referenced this pull request Dec 3, 2019
bastimeyer added a commit to bastimeyer/streamlink that referenced this pull request Dec 3, 2019
bastimeyer added a commit to bastimeyer/streamlink that referenced this pull request Dec 6, 2019
bastimeyer added a commit to bastimeyer/streamlink that referenced this pull request Dec 6, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.