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

Fix an issue with the HLS manifest check for livestream videos #5189

Merged

Conversation

alexmaras
Copy link
Contributor

@alexmaras alexmaras commented Feb 20, 2025

Originally, the HLS manifest check was essentially a boolean: if the HLS manifest field was present, it was assumed to be a livestream. Some videos include the HLS Manifest but aren't livestreams.

In the case where they are livestreams, the video contains a videoType field with the value "Livestream". In the case that they're normal videos, the videoType is "Video". This is exposed via the video.live_now method.

This commit just checks that video.live_now is true before treating it as a livestream

@alexmaras alexmaras requested a review from a team as a code owner February 20, 2025 15:31
@alexmaras alexmaras requested review from syeopite and removed request for a team February 20, 2025 15:31
@alexmaras
Copy link
Contributor Author

Fixes: iv-org/invidious-companion#35

There's also another place where the simple check for an HLS manifest exists:

if hlsvp = video.hls_manifest_url

Does this also need to be changed?

The alternative approach to this would be to add a boolean property to the video - video.is_livestream? which can resolve to a boolean and centralise that check. Happy to adjust and take that approach.

This change has been tested on my local instance and is working well with:

  • videos that have an hls_manifest_url but aren't a livestream
  • videos that have an hls_manifest_url and are a livestream
  • videos that don't have an hls_manifest_url

Let me know if anything makes sense to change.

@@ -4,7 +4,7 @@
<% if params.autoplay %>autoplay<% end %>
<% if params.video_loop %>loop<% end %>
<% if params.controls %>controls<% end %>>
<% if (hlsvp = video.hls_manifest_url) && !CONFIG.disabled?("livestreams") %>
<% if (hlsvp = video.hls_manifest_url) && hlsvp.includes?("/source/yt_live_broadcast/") && !CONFIG.disabled?("livestreams") %>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should just be able to check whether video.live_now is true instead of the URL

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks - great catch. I'm still digging through the codebase, this does a better job that checking the URL.

I've updated to that in case it's worth merging this in in the meantime as some videos are broken at the moment.

@syeopite
Copy link
Member

If I'm understanding everything correctly this is something that should be fixed by #4118, whenever that is merged.

This part of the PR prevents Invidious's frontend from using the HLS manifest unless the video is a livestream or the user has selected HLS quality

<% if (hlsvp = video.hls_manifest_url) && video.live_now && !CONFIG.disabled?("livestreams") %>
<source src="<%= URI.parse(hlsvp).request_target %><% if params.local %>?local=true<% end %>" type="application/x-mpegURL" label="<%= HTML.escape(translate(locale,"video_quality_livestream_label")) %>">
<% elsif (hlsvp = video.hls_manifest_url) && params.quality == "hls" && !CONFIG.disabled?("hls") %>
<source src="<%= URI.parse(hlsvp).request_target %><% if params.local %>?local=true<% end %>" type="application/x-mpegURL" label="<%= HTML.escape(translate(locale,"video_quality_hls_label")) %>">

Originally, the HLS manifest check was essentially a boolean: if the HLS
manifest field was present, it was assumed to be a livestream. Some
videos include the HLS Manifest but aren't livestreams.

In the case where they are livestreams, the video contains a videoType
field with the value "Livestream". In the case that they're normal
videos, the videoType is "Video". This is exposed via the video.live_now
method.

This commit just checks that video.live_now is true before treating it
as a livestream
@alexmaras alexmaras force-pushed the fix/hls-manifest-livestream-breakage branch from 2cc4444 to 49afbf2 Compare February 21, 2025 08:33
@alexmaras
Copy link
Contributor Author

Thanks @syeopite - yep, looks like the fix in #4118 covers this completely. If that one is still a little while away, this fixes the issue for the moment until that PR gets merged in. I've amended the commit to handle the issue using the video.live_now method instead.

@syeopite syeopite merged commit fe4fa04 into iv-org:master Feb 26, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants