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.

Sign up for GitHub

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

Set language info for dash audio streams and sort #5094

Closed
wants to merge 1 commit into from
Closed

Set language info for dash audio streams and sort #5094

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 7 additions & 2 deletions src/invidious/routes/api/manifest.cr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,17 +70,22 @@ module Invidious::Routes::API::Manifest
# OTF streams aren't supported yet (See https://github.com/TeamNewPipe/NewPipe/issues/2415)
next if !(fmt.has_key?("indexRange") && fmt.has_key?("initRange"))

audio_track = fmt["audioTrack"]?.try &.as_h? || {} of String => JSON::Any
lang = audio_track["id"]?.try &.as_s.split('.')[0] || "und"
is_default = audio_track.has_key?("audioIsDefault") ? audio_track["audioIsDefault"].as_bool : i == 0
displayname = audio_track["displayName"]?.try &.as_s || "Unknown"

# Different representations of the same audio should be groupped into one AdaptationSet.
# However, most players don't support auto quality switching, so we have to trick them
# into providing a quality selector.
# See https://github.com/iv-org/invidious/issues/3074 for more details.
xml.element("AdaptationSet", id: i, mimeType: mime_type, startWithSAP: 1, subsegmentAlignment: true, label: fmt["bitrate"].to_s + "k") do
xml.element("AdaptationSet", id: i, mimeType: mime_type, startWithSAP: 1, subsegmentAlignment: true, label: displayname, lang: lang) do
Copy link
Member

Choose a reason for hiding this comment

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

In the case of different representations of the same language you'll end up with two AdaptationSet with the same label and lang. I think that might confuse some downstream clients (and users) if two audio tracks end up having the same label and language.

For example see the dash manifest for this video with this PR https://youtube.comwatch?v=urevinis_PU

Copy link
Contributor Author

@GTechAlpha GTechAlpha Dec 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Hello.

If I am not misunderstanding your statement, yes that could happen if audio streams weren't being filtered for mime type "audio/mp4".

However, since they are being filtered this way, I have only seen one unique language representation per this mime type, from YT.

With this PR and the current state of Inv and YT, I have not experienced the issue you describe. And in the case of your example, no audio stream selection options are provided (by video.js and in my case) because there is no audio track information from YT, so the lang attributes are set to "und" and the client falls back to the default behavior of auto-selecting the first stream. When audio track info is available, video.js provides only one audio option per language in all of my testing.

Thank you for your time and attention.

Copy link
Member

Choose a reason for hiding this comment

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

This isn't just about video.js. There are downstream clients that use other media players which also rely on the dash manifests generated by Invidious.

Copy link
Member

@syeopite syeopite Dec 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I'm also concerned that this could reintroduce issue #3074.

Although the first AdaptationSet should always have the highest bitrate, YouTube's structure is very finicky and subject to change. Even if video.js will always select the first stream it doesn't mean that it'll always have the highest bitrate.

PR #3162 also seemed to introduce a quality selector for audio but it seems to have broke somewhere down the line. If it was ever fixed, this issue will present a UX problem even in Invidious and video.js with the labels being simply "Unknown"

This comment was marked as off-topic.

Sign in to view

Copy link
Contributor

Choose a reason for hiding this comment

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

@syeopite do you think something like this would solve theses issues?

displayname = audio_track["displayName"]?.try &.as_s || "Unknown"
bitrate = fmt["bitrate"]
label = displayname == "Unknown" ? "[#{bitrate}k]" : "#{displayname} [#{bitrate}k]"
xml.element("AdaptationSet", ... label: label)

Copy link
Member

Choose a reason for hiding this comment

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

I believe so but the fallback bitrate label should be #{lang} [#{bitrate}k] instead of just simply a bitrate display.

Copy link
Contributor

Choose a reason for hiding this comment

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

Great! I'm continuing this PR at #5149

codecs = fmt["mimeType"].as_s.split("codecs=")[1].strip('"')
bandwidth = fmt["bitrate"].as_i
itag = fmt["itag"].as_i
url = fmt["url"].as_s

xml.element("Role", schemeIdUri: "urn:mpeg:dash:role:2011", value: i == 0 ? "main" : "alternate")
xml.element("Role", schemeIdUri: "urn:mpeg:dash:role:2011", value: is_default ? "main" : "alternate")

xml.element("Representation", id: fmt["itag"], codecs: codecs, bandwidth: bandwidth) do
xml.element("AudioChannelConfiguration", schemeIdUri: "urn:mpeg:dash:23003:3:audio_channel_configuration:2011",
Expand Down
2 changes: 1 addition & 1 deletion src/invidious/videos.cr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,7 @@ struct Video
if formats = info.dig?("streamingData", "adaptiveFormats")
return formats
.as_a.map(&.as_h)
.sort_by! { |f| f["width"]?.try &.as_i || 0 }
.sort_by! { |f| f["width"]?.try &.as_i || f["audioTrack"]?.try { |a| a["audioIsDefault"]?.try { |v| v.as_bool ? -1 : 0 } } || 0 }
else
return [] of Hash(String, JSON::Any)
end
Expand Down
Loading