fix duplicate caption processing #156

squarebracket · 2017-08-14T18:38:00Z

@mjneil I think this is how I did it, but I don't recall for sure tbh.

mjneil · 2017-08-14T18:56:40Z

I would setup the seeked handler within the HtmlMediaSource sourceopen handler (https://github.com/videojs/videojs-contrib-media-sources/blob/master/src/html-media-source.js#L173) as that is where HtmlMediaSource.player_ is attached which may or may not be available at time of VirtualSourceBuffer construction. You could define a function similar to this.onPlayerMediaChange_ that loops through the sourcebuffers and calling the transmuxer api if it's attached to the source buffer and off the handler in the corresponding sourceclose handler. You'll have to do similar implementations for the FlashMediaSource and FlashTransmuxWorker.

squarebracket · 2017-08-14T21:33:27Z

How's this?

I added the flash stuff, but I'm not sure what I did was appropriate. There's no handler for sourceclose, so I added one. But maybe I should have stuck it in endOfStream?

squarebracket · 2017-08-14T21:35:22Z

Ah, my Chrome keeps "aw, snap!"-ing, so I didn't know the tests were failing. Will look into it.

squarebracket · 2017-08-14T21:38:49Z

Ok, nevermind, apparently all the tests are fine.... 🤔

mjneil · 2017-08-14T22:24:14Z

Here's where I tell you I steered you in the wrong direction because flash is special haha. Since flash is not using real MSE and instead the swf is acting like a media source, I'm not entirely sure if it is actually firing sourceopen and sourceclose events as we expect. For this reason I think at least for the flash side, we can put the code in the FlashSourceBuffer. Specifcally, there is already a listener for seeked here https://github.com/videojs/videojs-contrib-media-sources/blob/master/src/flash-source-buffer.js#L180 that you can add your call to the transmuxer seeked api.
Since this seeked handler is already not being unregistered, you probably don't have to worry about that

mjneil · 2017-08-16T21:53:33Z

src/html-media-source.js

@@ -187,6 +199,7 @@ export default class HtmlMediaSource extends videojs.EventTarget {
      }

      this.player_.on('mediachange', this.onPlayerMediachange_);
+      this.player_.on('seeking', this.onPlayerSeeking_);


Why seeking instead of seeked?

I'm not sure if it matters, but the captions should all be reset before it starts ingesting the video. Don't know if the seeked event will fire and complete before data is pushed through the transmuxer.

Hmmm better safe than sorry I suppose

mjneil · 2017-08-16T21:55:23Z

src/html-media-source.js

+        if (sourceBuffer.transmuxer_) {
+          let range = this.player_.buffered();
+          let current = this.player_.currentTime();
+          if (current >= range.end(0) || current <= range.start(0)) {


You should check range.length before attempting to call start or end because if the buffer is empty you'll get an error

Added range.length && to the if statement.

mjneil · 2017-08-16T23:19:44Z

src/html-media-source.js

+          let range = this.player_.buffered();
+          let current = this.player_.currentTime();
+
+          if (current >= range.end(0) || current <= range.start(0)) {


Since this won't change between buffers, we should do this check first and only do the forEach if it passes.

You'll also want to check range.length here.

I think this comparison will also run into problems if there are gaps in the buffer. (history for context) In the past, contrib-hls had pretty complicated logic surrounding segment loading, and part of that complication was gaps and multiple time ranges after seeks. We've since updated that so contrib-hls appends segments in a sequential order and clears the entire buffer when seeking outside the buffered range. What this gave us was the ability for contrib-hls to have the assumption that there is only ever 1 time range of buffered content, and any gaps within that range are inherit to the source and not because we seeked or skipped a segment. However, buffered() would still reflect those gaps. For example buffered could look like [0 -> 10, 11->20] after appending the first two segments. Seeking to 13 would not reset anything from contrib-hls point of view, but range.end(0) would be 10 so this would cause the captions to be reset.

Specifically, contrib-hls will do its seek reset if this buffered.length === 0. You'll notice the findRange function it calls also uses a fudge factor to account for rounding of the time range values, so you should probably do something similar here as well

Hmm. Think maybe contrib-hls should dispatch the actual message, then? I mean, all this other scaffolding for handling the exact same case is there...

Or would that break things for native HLS?

Having contrib-hls handle the message would simplify this alot. I agree we should come up with a solution that can take advantage of the code and information contrib-hls has.

Historically we've tried to keep this project as a shim for MSE so that contrib-hls would not need knowledge of whether it was working with this project or native media source. (Even though contrib-hls is always using our shim). Eventually we would like to do away with this project and pull functionality into contrib-hls so we don't have to deal with these constraints that we keep running into. However, re-examining how the two projects interact, we are already breaking that principle by calling addSeekableRange_ from contrib-hls. We also communicate mediachange between these projects through an event on the tech that contrib-media-sources listens to. I don't think we should add new apis for contrib-hls to call (even though we are doing it once). I think the event solution is reasonable and keeps with the spirit of being a native shim. If contrib-hls was working with a native mediasource, triggering an event on the tech would not break anything as there would just be nothing listening to it.

So instead of doing the reset on seek events, lets listen for a custom event on the player's tech (I think this should stay just on the tech unlike the mediachange event that is bubbled up to the player since it's not information that the player needs to know about) and do the reset when that event is triggered. Then all that would be needed is for contrib-hls to trigger the event at the right moment

I think this is all that would need to be done from contrib-hls mjneil/videojs-contrib-hls@6cfdb6b

I like that approach!

Is there a "global" place in media-sources where I could receive that event, or will I have to set up the listener for HTML and Flash separately?

No I don't think so. You'd have to do them separately like it is now

Ok I pushed to squarebracket/videojs-contrib-media-sources@20230fa and added your commit to contrib-hls, seems to work. If it looks good to you, I'll force push to this branch.

left a couple comments on that commit otherwise 👍

mjneil · 2017-08-16T23:20:49Z

src/flash-source-buffer.js

@@ -178,6 +178,7 @@ export default class FlashSourceBuffer extends videojs.EventTarget {
    // On a seek we remove all text track data since flash has no concept
    // of a buffered-range and everything else is reset on seek
    this.mediaSource_.player_.on('seeked', () => {
+      this.resetCaptionsIfAppropriate();


Just realized that things are reset on every seek regardless of in buffer or not when Flash is being used, so you should just be able to postMessage here without doing any checks

squarebracket · 2017-08-18T14:59:51Z

Alright, changed things to the event-based method.

mjneil · 2017-08-18T19:19:07Z

can we update the hls-reset-everything event to be just hls-reset (and have contrib-hls changed to fire hls-reset as well)

squarebracket · 2017-08-18T20:40:38Z

Done. I renamed the callbacks as well.

mjneil · 2017-08-18T22:58:55Z

src/flash-source-buffer.js

@@ -182,11 +182,18 @@ export default class FlashSourceBuffer extends videojs.EventTarget {
      removeCuesFromTrack(0, Infinity, this.inbandTextTrack_);
    });

+    this.mediaSource_.player_.tech_.on('hls-reset', this.resetHls);


For some reason when this.resetHls is called after hearing this event, this within resetHls is not being bound correctly.

if you define the function above instead of using the es6 class syntax it works

this.resetHls = () => { this.transmuxer_.postMessage({action: 'resetCaptions'}); }; this.mediaSource_.player.tech_.on('hls-reset', .......

Looks like bind is being used in a couple other places there, maybe it's best to do that to keep the style consistent.

bind would work too. You'll just have to make sure to put it in a local var and use that for both the on and off otherwise the references passed to on and off will be to different functions

let onHlsReset = this.resetHls.bind(this); ...tech_.on('hls-reset', onHlsReset); ...tech_.off('hls-reset', onHlsReset);

mjneil · 2017-08-18T23:13:35Z

src/flash-source-buffer.js

    });
  }

+  resetHls() {


can we update this to onHlsReset_ for naming consistency

mjneil · 2017-08-18T23:13:55Z

src/html-media-source.js

@@ -159,6 +159,14 @@ export default class HtmlMediaSource extends videojs.EventTarget {
      });
    };

+    this.onResetHls_ = () => {


can we also update this to onHlsReset_ as well

squarebracket force-pushed the fix-duplicate-caption-processing branch from b257de6 to 1ed612b Compare August 14, 2017 21:32

mjneil reviewed Aug 16, 2017

View reviewed changes

squarebracket force-pushed the fix-duplicate-caption-processing branch 2 times, most recently from 5bc71e8 to b9fe08d Compare August 16, 2017 22:14

mjneil reviewed Aug 16, 2017

View reviewed changes

squarebracket force-pushed the fix-duplicate-caption-processing branch 3 times, most recently from d4e8e58 to aee6ee2 Compare August 18, 2017 04:09

fix duplicate caption processing

a324adb

squarebracket force-pushed the fix-duplicate-caption-processing branch from aee6ee2 to a324adb Compare August 18, 2017 20:37

mjneil reviewed Aug 18, 2017

View reviewed changes

squarebracket force-pushed the fix-duplicate-caption-processing branch from c3777d9 to ffcde7c Compare August 18, 2017 23:07

mjneil reviewed Aug 18, 2017

View reviewed changes

src/flash-source-buffer.js Outdated

});

}

resetHls() {

Copy link

Contributor

mjneil Aug 18, 2017

Choose a reason for hiding this comment

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

can we update this to onHlsReset_ for naming consistency

mjneil reviewed Aug 18, 2017

View reviewed changes

use bind

63304e9

squarebracket force-pushed the fix-duplicate-caption-processing branch from ffcde7c to 63304e9 Compare August 18, 2017 23:22

mjneil approved these changes Aug 18, 2017

View reviewed changes

mjneil merged commit fa15919 into videojs:master Aug 18, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix duplicate caption processing #156

fix duplicate caption processing #156

squarebracket commented Aug 14, 2017

mjneil commented Aug 14, 2017

squarebracket commented Aug 14, 2017

squarebracket commented Aug 14, 2017

squarebracket commented Aug 14, 2017

mjneil commented Aug 14, 2017

mjneil Aug 16, 2017

squarebracket Aug 16, 2017

mjneil Aug 16, 2017

mjneil Aug 16, 2017

squarebracket Aug 16, 2017

mjneil Aug 16, 2017

squarebracket Aug 16, 2017 •

edited

Loading

squarebracket Aug 16, 2017

mjneil Aug 17, 2017 •

edited

Loading

mjneil Aug 17, 2017

squarebracket Aug 17, 2017

squarebracket Aug 17, 2017

mjneil Aug 17, 2017

squarebracket Aug 17, 2017 •

edited

Loading

mjneil Aug 17, 2017

mjneil Aug 16, 2017

squarebracket commented Aug 18, 2017

mjneil commented Aug 18, 2017

squarebracket commented Aug 18, 2017

mjneil Aug 18, 2017

squarebracket Aug 18, 2017

mjneil Aug 18, 2017

mjneil Aug 18, 2017

mjneil Aug 18, 2017

fix duplicate caption processing #156

fix duplicate caption processing #156

Conversation

squarebracket commented Aug 14, 2017

mjneil commented Aug 14, 2017

squarebracket commented Aug 14, 2017

squarebracket commented Aug 14, 2017

squarebracket commented Aug 14, 2017

mjneil commented Aug 14, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

squarebracket Aug 16, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mjneil Aug 17, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

squarebracket Aug 17, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

squarebracket commented Aug 18, 2017

mjneil commented Aug 18, 2017

squarebracket commented Aug 18, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

squarebracket Aug 16, 2017 •

edited

Loading

mjneil Aug 17, 2017 •

edited

Loading

squarebracket Aug 17, 2017 •

edited

Loading