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

Pass any CHARACTERISTICS value of a track with the track object #14

Merged

Conversation

OwenEdwards
Copy link
Member

If an #EXT-X-MEDIA for a track has a CHARACTERISTICS value, include that with the information about the track for use by HLS playlist playback. (possible values of CHARACTERISTICS include public.accessibility.transcribes-spoken-dialog, public.accessibility.describes-music-and-sound, and public.accessibility.describes-video).

src/parser.js Outdated
@@ -251,6 +251,9 @@ export default class Parser extends Stream {
if (entry.attributes['INSTREAM-ID']) {
rendition.instreamId = entry.attributes['INSTREAM-ID'];
}
if (entry.attributes['CHARACTERISTICS']) {
Copy link
Member

Choose a reason for hiding this comment

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

this and below need to be dot-notation.

Copy link
Member Author

Choose a reason for hiding this comment

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

I was just trying to be consistent with the pattern used for INSTREAM-ID already. Are you saying you want me to fix that, as well as what I added?

Copy link
Member

Choose a reason for hiding this comment

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

that's why the build is failing. INSTREAM-ID requires the bracket notation because of the hyphen; if you take a look at other things, they use dot-notation.

@gesinger
Copy link
Contributor

Hey @OwenEdwards , looks nice and simple, but any reason we don't just leave CHARACTERISTICS as part of attributes?

@OwenEdwards
Copy link
Member Author

@gesinger this seems like the only way to pass the characteristics over to the HLS player (videojs-contrib-hls), so that it could process them (see videojs/videojs-contrib-hls#1019). I don't think the attributes get passed over, do they? If you want to pass over all the attributes, that would work, I guess - this was just targeted at adding the specific new feature of videojs-contrib-hls that I was looking for.

@gesinger
Copy link
Contributor

gesinger commented Feb 22, 2017

Sorry @OwenEdwards , you are right, it definitely wasn't passed before this change.

The only thing I'd add to this PR is a test, maybe something like:

QUnit.test('parses characteristics attribute', function() {
  const parser = new Parser();
  const manifest = [
    '#EXTM3U',
    '#EXT-X-MEDIA:TYPE=SUBTITLES,GROUP-ID=subs,CHARACTERISTICS=char,NAME=test',
    '#EXT-X-STREAM-INF:BANDWIDTH=1,CODECS="mp4a.40.2, avc1.4d400d",SUBTITLES="subs"',
    'index.m3u8'
  ].join('\n');

  parser.push(manifest);

  QUnit.equal(parser.manifest.mediaGroups.SUBTITLES.subs.test.characteristics,
              'char',
              'parsed CHARACTERISTICS attribute');
});

EDIT: updated a bug in test (thanks @mjneil )

@OwenEdwards
Copy link
Member Author

@gesinger - I added that test. I think this should do it; thanks for making it so easy!

@gesinger
Copy link
Contributor

No problem @OwenEdwards . Thanks for the contribution!

@mjneil mjneil merged commit 5cb6ae7 into videojs:master Feb 23, 2017
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.

4 participants