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

Jump to bottom

fix: ensure vtt tracks language is set correctly #181

Merged
merged 2 commits into from
Oct 9, 2024
Merged

fix: ensure vtt tracks language is set correctly #181

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
816d5db
fix: ensure vtt tracks language is set correctly
wseymour15 Oct 8, 2024
8688dcd
update logic to fallback to und value
wseymour15 Oct 8, 2024
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
3 changes: 2 additions & 1 deletion src/toM3u8.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Copy link
Member

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

Copy link
Member

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'

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 for the documentation! Just made that change so we fallback to und

Copy link
Member

Choose a reason for hiding this comment

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

you're welcome!

adrums86 marked this conversation as resolved.
Show resolved Hide resolved

if (!a[label]) {
a[label] = {
language: label,
Copy link
Member

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!

language,
default: false,
autoselect: false,
playlists: [],
Expand Down
61 changes: 58 additions & 3 deletions test/toM3u8.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -129,7 +129,7 @@ QUnit.test('playlists', function(assert) {
text: {
autoselect: false,
default: false,
language: 'text',
language: 'en',
playlists: [{
attributes: {
BANDWIDTH: 20000,
Expand Down Expand Up @@ -334,6 +334,22 @@ QUnit.test('playlists with content steering and resolvable URLs', function(asser
sourceDuration: 0,
type: 'dyanmic'
}
},
{
attributes: {
bandwidth: 256,
baseUrl: 'https://cdn3.example.com/vtt',
clientOffset: 0,
id: 'fr',
label: 'french',
lang: 'fr',
mimeType: 'text/vtt',
periodStart: 0,
role: {},
serviceLocation: 'beta',
sourceDuration: 0,
type: 'dyanmic'
}
}
];

Expand All @@ -353,6 +369,45 @@ QUnit.test('playlists with content steering and resolvable URLs', function(asser
['CLOSED-CAPTIONS']: {},
SUBTITLES: {
subs: {
french: {
autoselect: false,
default: false,
language: 'fr',
playlists: [
{
attributes: {
BANDWIDTH: 256,
NAME: 'fr',
['PROGRAM-ID']: 1,
serviceLocation: 'beta'
},
discontinuitySequence: 0,
discontinuityStarts: [],
endList: false,
mediaSequence: 0,
resolvedUri: 'https://cdn3.example.com/vtt',
segments: [
{
duration: 0,
number: 0,
resolvedUri: 'https://cdn3.example.com/vtt',
timeline: 0,
uri: 'https://cdn3.example.com/vtt'
}
],
targetDuration: 0,
timeline: 0,
timelineStarts: [
{
start: 0,
timeline: 0
}
],
uri: ''
}
],
uri: ''
},
en: {
autoselect: false,
default: false,
Expand Down Expand Up @@ -1060,7 +1115,7 @@ QUnit.test('playlists with segments', function(assert) {
text: {
autoselect: false,
default: false,
language: 'text',
language: 'en',
playlists: [{
attributes: {
BANDWIDTH: 20000,
Expand Down Expand Up @@ -1893,7 +1948,7 @@ QUnit.test('eventStreams with playlists', function(assert) {
text: {
autoselect: false,
default: false,
language: 'text',
language: 'en',
playlists: [{
attributes: {
BANDWIDTH: 20000,
Expand Down
Loading