-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
Set language info for dash audio streams and sort #5094
base: master
Are you sure you want to change the base?
Set language info for dash audio streams and sort #5094
Conversation
- language id - language display name - main/default track Sort audio formats so that main/default is first (for clients not using dash) * Note: this should be a non-breaking change; if audio track info is not availablle, the behavior does not change from current
Hello, thank you for the PR that's really interesting. If this solves #2007, could you add |
# 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 |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
This comment was marked as off-topic.
Sorry, something went wrong.
Well first, you are not commenting in the right section. This is a comment section about a code change in the PR. Second, This PR is about the DASH manifest, not the API that is used by the 3rd party clients. If you are requesting the API using the |
Set audio track info, if present, in dash manifest
Sort audio formats so that main/default is first (for clients not using dash)
This resolves issue of incorrect/undesired audio when automatic translation is enabled by content creator (currently relatively rare, but likely to increase), and allows dash clients to correctly select default language and optionally offer multiple language streams.
closes #2007
Note: this should be a non-breaking change; if audio track info is not available, the behavior does not change from current