videojs / video.js

Video.js - open source HTML5 video player
https://videojs.com
Other
38.11k stars 7.45k forks source link

Audio tracks(and probably text tracks) do not read labels from manifest #6960

Open RomeroDiver opened 3 years ago

RomeroDiver commented 3 years ago

Description

When loading manifest .mpd with the

<AdaptationSet mimeType="audio/mp4" lang="und" segmentAlignment="0">
  <ContentProtection cenc:default_KID="id" schemeIdUri="urn:mpeg:dash:mp4protection:2011" value="cenc"/>
  <ContentProtection cenc:default_KID="id" schemeIdUri="urn:uuid:id" value="2.0">
    <mspr:pro>hash</mspr:pro>
    <cenc:pssh>hash</cenc:pssh>
  </ContentProtection>
  <ContentProtection cenc:default_KID="id" schemeIdUri="urn:uuid:id">
    <cenc:pssh>hash</cenc:pssh>
  </ContentProtection>
  <SegmentTemplate timescale="24000" media="audio_$Number%09d$.mp4" initialization="audio_init.mp4" duration="720720" startNumber="1"/>
  <Label>English</Label>
  <Representation id="9" bandwidth="64000" audioSamplingRate="48000" codecs="mp4a.40.5"/>
</AdaptationSet>

the player.audioTracks() return audio tracks without the label from the above manifest.

Steps to reproduce

Explain in detail the exact steps necessary to reproduce the issue.

  1. Load a player with custom audio tracks
  2. The player will load .mpd manifest with the Label element for the audio tracks

Results

The label in the .audioTracks()[0] is actually from the lang attribute in the manifest

Expected

The label in the .audioTracks()[0] is actually from the Label element in the manifest. I've tried the label attribute on the element AdaptationSet but that didn't work as well.

Actual

The label in the .audioTracks().[0] is actually from the lang attribute in the manifest

Error output

There are no errors in the console, and the actual manifest is correct as I've linted it

Additional Information

Please include any additional information necessary here. Including the following:

versions

videojs

7.8.3

browsers

Both Chrome and Firefox, I suppose the same goes for the rest of them as well

OSes

Tested on Windows 10

plugins

    "videojs-contrib-eme": "^3.7.0",
    "videojs-contrib-quality-levels": "^2.0.9",
    "videojs-hls-quality-selector": "1.1.1",
    "videojs-http-source-selector": "^1.1.6",
    "videojs-markers": "^1.0.1",
    "videojs-seek-buttons": "^1.6.0",
    "videojs-sprite-thumbnails": "^0.5.3"

Probably a duplicate of https://github.com/videojs/http-streaming/issues/645

welcome[bot] commented 3 years ago

👋 Thanks for opening your first issue here! 👋

If you're reporting a 🐞 bug, please make sure you include steps to reproduce it. We get a lot of issues on this repo, so please be patient and we will get back to you as soon as we can. To help make it easier for us to investigate your issue, please follow the contributing guidelines.

brandonocasey commented 3 years ago

Hey @RomeroDiver we don't currently support <Label> in dash manifests as you reported, but we probably should. We might not have time to get to this for awhile. If you are up for making the change I can outline the process how I see it right now. First we would have to add something to know about the <Label> element. I think that would be done here:

https://github.com/videojs/mpd-parser/blob/7bf58e93a9b429107137611220e63d1ced3486c1/src/inheritAttributes.js#L224-L248 and would look something like:

const label = const role = findChildren(adaptationSet, 'Label')[0];

if (label) {
    // add the label to the representation object.
}

Then you will have to prioritze the label property here if it exists. https://github.com/videojs/mpd-parser/blob/7bf58e93a9b429107137611220e63d1ced3486c1/src/toM3u8.js#L134-L144

stale[bot] commented 3 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

RomeroDiver commented 3 years ago

Not outdated - just waiting for a code review :)

gkatsev commented 3 years ago

FYI, we haven't forgotten! Just keep getting distracted by other things!

RomeroDiver commented 3 years ago

Hey, I've seen that the PR has been merged in the mpd-parser package, however what is the flow to push it through other dependencies cause the mpd-parser is not the direct dependency of the videojs but of the other packages? As far as I can see, the http-stream is the one to update.

gkatsev commented 3 years ago

mpd-parser needs to get updated in VHS and then Video.js needs to get that version of VHS. It will likely happen either this week or next week.

RomeroDiver commented 2 years ago

Hey, the issue still exists - now though only for textTracks, the audioTracks seem to be working properly

RomeroDiver commented 2 years ago

Here is the PR for this issue: https://github.com/videojs/mpd-parser/pull/161

ghost commented 2 years ago

@RomeroDiver I had the same problem and opened an issue here: https://github.com/videojs/video.js/issues/7870

As you already pointed out, it's working for Audio-Tracks but not for Subtitle-Tracks like webvtt and such, I can absolutely confirm this. As a hot-fix I set the lang attribute for text tracks like that: lang="Deutsch" where on the other side I use Lable>Deutsch</Label and lang="de" on Audio Tracks. Anyway, I would not take such a hot-fix into production as it will get broken as soon as VideoJS implements Label for Subtitle tracks ...