search

videojs / mpd-parser

https://videojs.github.io/mpd-parser/
Other
78 stars 54 forks source link

fix(toM3u8): set label for vttPlaylist from label instead of lang #161

Closed RomeroDiver closed 1 year ago

RomeroDiver commented 2 years ago

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

codecov[bot] commented 2 years ago

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%> (ø)

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

gkatsev commented 2 years ago

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 https://github.com/videojs/mpd-parser/pull/153#issuecomment-971757739

there's some direction on how to do it with a label element here https://github.com/videojs/video.js/issues/6960#issuecomment-733115810

RomeroDiver commented 2 years ago

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 commented 2 years ago

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

gkatsev commented 2 years ago

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