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(toM3u8): set label for vttPlaylist from label instead of lang #161

Merged
merged 1 commit into from Oct 17, 2022

Conversation

RomeroDiver
Copy link
Contributor

Fix the issue when the subtitles' label property was set from lang instead of label

@codecov
Copy link

codecov bot commented Aug 12, 2022

Codecov Report

Merging #161 (419aee1) into main (cd75be1) will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main     #161   +/-   ##
=======================================
  Coverage   94.34%   94.34%           
=======================================
  Files          19       19           
  Lines         778      778           
  Branches      242      242           
=======================================
  Hits          734      734           
  Misses         44       44           
Impacted Files Coverage Δ
src/toM3u8.js 98.75% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@gkatsev
Copy link
Member

gkatsev commented Aug 12, 2022

I believe this isn't the spec-compliant way of labeling text tracks. The expected way is with a Label element.
Also see answer here #153 (comment)

there's some direction on how to do it with a label element here videojs/video.js#6960 (comment)

@RomeroDiver
Copy link
Contributor Author

I agree with your comment, however this fix is exactly to make the Label element work. E.g. without the fix the following is not working properly - the Label element is not read,

<AdaptationSet mimeType="text/vtt" lang="en">
      <Role schemeIdUri="urn:mpeg:dash:role" value="subtitle"/>
      <Label>English</Label>
      <Representation id="12" bandwidth="256">
        <BaseURL>url</BaseURL>
      </Representation>
    </AdaptationSet>

@RomeroDiver
Copy link
Contributor Author

What do you think? I think this change is according to the specs

@gkatsev
Copy link
Member

gkatsev commented Aug 26, 2022

Sorry, haven't had a chance to test it yet.

Copy link
Member

@gkatsev gkatsev left a comment

Choose a reason for hiding this comment

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

Finally got around to this. Confirmed that this does what's expected.

I also found a test source with a labeled text track https://demo.unified-streaming.com/k8s/features/stable/video/tears-of-steel/tears-of-steel-hoh-subs.ism/.mpd (though, VHS won't be able to play it, since VHS doesn't support IMSC currently).

@gkatsev gkatsev merged commit 3fc0486 into videojs:main Oct 17, 2022
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.

None yet

3 participants