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(id3): cuechange event not being triggered on audio-only HLS streams #334

Merged
merged 6 commits into from
Jan 10, 2019

Conversation

mparisi76
Copy link
Contributor

@mparisi76 mparisi76 commented Jan 8, 2019

This is a proposed fix for #130. Instead of adding text-track data only to video segments, we are now adding to all segments.

One change bringing the addTextTrackData invocation outside of the conditional which checks for video buffer.

Sources:

With audio only
http://li720-23.members.linode.com/hls/audio.m3u8
With video and audio
http://li720-23.members.linode.com/hls/video.m3u8

Requirements Checklist

  • Feature implemented / Bug fixed
  • If necessary, more likely in a feature request than a bug fix
    • Unit Tests updated or fixed
  • Reviewed by Two Core Contributors

@welcome
Copy link

welcome bot commented Jan 8, 2019

💖 Thanks for opening this pull request! 💖

Things that will help get your PR across the finish line:

  • Run npm run lint -- --errors locally to catch formatting errors earlier.
  • Include tests when adding/changing behavior.
  • Include screenshots and animated GIFs whenever possible.

We get a lot of pull requests on this repo, so please be patient and we will get back to you as soon as we can.

@@ -491,6 +490,9 @@ export default class VirtualSourceBuffer extends videojs.EventTarget {
triggerUpdateend = true;
}

//Add text-track data for all
addTextTrackData(this, sortedSegments.captions, sortedSegments.metadata);
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like the build is failing since this should be indented 2 more spaces (and the comment too)

@ldayananda ldayananda changed the title cuechange event not being triggered on audio only HLS streams #130 fix(id3): cuechange event not being triggered on audio-only HLS streams Jan 10, 2019
@ldayananda
Copy link
Contributor

One last nice to have would be to add a test!

Here's the test for video-only case: https://github.com/videojs/http-streaming/blob/master/test/mse/html.test.js#L1162

You could add another test that is audio-only be copying that test and modifying

let sourceBuffer = mediaSource.addSourceBuffer('video/mp2t');

to be:

let sourceBuffer = mediaSource.addSourceBuffer('video/mp2t; codecs=\"avc1.4d400d, mp4a.40.2\"');

and modifying

sourceBuffer.transmuxer_.onmessage(createDataMessage('video', new Uint8Array(1), {
metadata
}));

to be:

sourceBuffer.transmuxer_.onmessage(createDataMessage('audio', new Uint8Array(1), {
  metadata
}));

Let me know if you need any help! You can run tests locally by running npm run test

ldayananda
ldayananda previously approved these changes Jan 10, 2019
forbesjo
forbesjo previously approved these changes Jan 10, 2019
Copy link
Contributor

@forbesjo forbesjo left a comment

Choose a reason for hiding this comment

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

Looks good to me. I would also suggest a test.

@ldayananda
Copy link
Contributor

@mparisi76 I've made a PR against your branch with the test! Once that's merged into this branch I think we should be able to merge this PR into master

test: adding test for audio-only stream id3 tag parsing
@mparisi76 mparisi76 dismissed stale reviews from forbesjo and ldayananda via 88a0098 January 10, 2019 17:44
@ldayananda ldayananda merged commit bab70fd into videojs:master Jan 10, 2019
@welcome
Copy link

welcome bot commented Jan 10, 2019

Congrats on merging your first pull request! 🎉🎉🎉

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.

3 participants