-
Notifications
You must be signed in to change notification settings - Fork 54
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: ensure vtt tracks language is set correctly #181
Conversation
src/toM3u8.js
Outdated
@@ -246,10 +246,11 @@ export const organizeAudioPlaylists = (playlists, sidxMapping = {}, isAudioOnly | |||
export const organizeVttPlaylists = (playlists, sidxMapping = {}) => { | |||
return playlists.reduce((a, playlist) => { | |||
const label = playlist.attributes.label || playlist.attributes.lang || 'text'; | |||
const language = playlist.attributes.lang || playlist.attributes.label || 'en'; |
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.
It's more correct to default to und
rather than en
because we don't want to mark something as en
when it isn't.
From dash spec 5.3.3.2 and IETF RFC 5646 section 4.1 step 5 https://datatracker.ietf.org/doc/html/rfc5646#section-4.1
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.
Actually, should we even bother trying to set it to label? We're more likely to have a language set than a label based on mpd files I've seen. So, might be better to do playlist.attributes.lang || 'und'
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.
Thanks for the documentation! Just made that change so we fallback to und
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.
you're welcome!
|
||
if (!a[label]) { | ||
a[label] = { | ||
language: label, |
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.
oof, seems like a huge oversight of us not to have it set to the language, heh. Good catch!
Just thought about it, and the change of the default from |
Totally agree. It seems unlikely people are using a |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #181 +/- ##
=======================================
Coverage 94.68% 94.68%
=======================================
Files 19 19
Lines 846 847 +1
Branches 258 259 +1
=======================================
+ Hits 801 802 +1
Misses 45 45 ☔ View full report in Codecov by Sentry. |
Description
In DASH, the language property we set on VTT tracks should be accurate. As of now, this value is the label when it exists, THEN the language.
This change sets the language to the
lang
property if it exists, then the label, and thenen
by default.