Remote Text Tracks Deleted Immediately Because of Asynchronous Nature of `src` #4403

jthomerson · 2017-06-07T21:36:39Z

Description

If I call .src({ src: '...' }) and then call .addRemoteTextTrack({ src: '...' }, false) right after it, my remote text tracks are deleted immediately, and do not appear on the video player. Why? Because the call to .src(...) is actually asynchronous, and passing the false means "auto cleanup remote text tracks the next time the source changes". See the example below for a more detailed flow description.

This did not happen in video.jx 5.x because calls to .src(...) were synchronous. You can verify that by replacing the CSS/JS includes below with:

I believe the source of the problem to be this line of code:

video.js/src/js/tech/middleware.js

Line 19 in c31836c

    
           player.setTimeout(() => setSourceHelper(src, middlewares[src.type], next, player), 1);

One possible fix would be to make the call to addRemoteTextTrack asynchronous as well. But that feels a bit like a hack.

Steps to reproduce

Use this example page, linking the CSS and JS to a 6x version of video.js. (side point: I can't find 6.x on a CDN, which is what stopped me from making this a jsfiddle)

<head>
   <link href="/dist/video-js.css" rel="stylesheet">
</head>

<body>
   <video id="my-video" class="video-js"></video>

   <script src="/dist/video.js"></script>
   <script
      src="https://code.jquery.com/jquery-3.2.1.min.js"
      integrity="sha256-hwg4gsxgFZhOsEEamdOYGBf13FyQuiTwlAQgxVSNgt4="
      crossorigin="anonymous"></script>
   <script>
      $(document).ready(function() {
         var player = videojs('my-video', {
            fluid: true,
            controls: true,
         });

         player.ready(function() {
            // the call to src is asynchronous under the covers
            player.src({ src: 'https://download-a.akamaihd.net/files/media_video/00/wsb_E_r720P.mp4' });
            // this says "delete this text track whenever the source next changes"
            // (in other words manual cleanup is off, auto cleanup is on)
            // but because the call to src(...) was asynchronous, and addRemoteTextTrack is synchronous, the order of
            // operations is:
            //    1. I call src - it sets a timeout for 1ms to set the actual source
            //    2. I call addRemoteTextTrack - it immediately adds the text track
            //    3. I'm done, and now (or 1ms from now), the source gets actually set
            //    4. as part of setting the source, my remote track gets cleaned up - deleted - gone - blown away
            // Nobody sees subtitles and everyone is sad.
            player.addRemoteTextTrack({ src: 'https://download-a.akamaihd.net/files/media_video/9a/wsb_E_r720P.vtt' }, false);

            /*
            // This would work, though:
            // Under the covers, ready must queue functions to happen in a setTimeout - haven't looked to be sure
            player.ready(function() {
               player.addRemoteTextTrack({ src: 'https://download-a.akamaihd.net/files/media_video/9a/wsb_E_r720P.vtt' }, false);
            });
            */

            /*
            // And this works, because our setTimeout function will get invoked after the previously-scheduled one:
            setTimeout(function() {
               player.addRemoteTextTrack({ src: 'https://download-a.akamaihd.net/files/media_video/9a/wsb_E_r720P.vtt' }, false);
            }, 1);
            */
         });
      });
   </script>
</body>

Results

Expected

Please describe what you expected to see.

Subtitles.

Actual

Please describe what actually happened.

No subtitles.

Error output

If there are any errors at all, please include them here.

Not applicable.

Additional Information

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

versions

videojs

what version of videojs does this occur with?

6.x

browsers

what browser are affected?

all

OSes

what platforms (operating systems and devices) are affected?

all

plugins

are any videojs plugins being used on the page? If so, please list them below.

none

The text was updated successfully, but these errors were encountered:

jthomerson · 2017-06-07T21:38:09Z

I haven't read #4315 in its entirety, and @Pinank08 doesn't have a complete test case there, and he doesn't list the version he's using, but the problem he describes is the same thing that I was seeing initially, and is what led me to find this bug. Thus, this may be the problem he's reporting.

gkatsev · 2017-06-07T21:43:13Z

Whoops, that's a total oversight. We should definitely handle this case.

There are some versions of 6.x on the CDN, only patch versions have been uploaded: http://vjs.zencdn.net/6.1.0/video.js. Also, since we release to npm, all the files are available on unpkg.com!

jthomerson · 2017-06-08T12:51:41Z

@gkatsev thanks for the quick response. In thinking through this more, I realized that there are other serious consequences of the call to src being asynchronous. Consider the following example:

app.player.src('your-url');
app.player.addRemoteTextTrack('your-url');

app.emit('HeyGuysThePlayerStateChanged');

In this scenario where you're sending a notification to the rest of your app saying that the state of the player changed, event handlers could potentially be fired before the player actually has the new state.

The naïve way to "fix" this might seem like:

app.player.src('your-url');
app.player.addRemoteTextTrack('your-url');

setTimeout(app.emit.bind(app, 'HeyGuysThePlayerStateChanged'), 2);

But I don't think we can really guarantee that browsers will execute my setTimeout after your setTimeout. So, then you get to something like:

app.player.src('your-url');
app.player.addRemoteTextTrack('your-url');

function checkForChange() {
   if (player.src() === 'your-url' && checkThatRemoteTextTracksMatchExpectedStateAlso()) {
      app.emit('HeyGuysThePlayerStateChanged');
   } else {
      setTimeout(checkForChange, 1);
   }
}
setTimeout(checkForChange, 2);

That should work because you keep waiting until the state has actually changed. But that's horrible to have to write.

It seems like internally, video.js has this same issue, and it is solved by calling player.ready(myFunctionThatShouldRunAfterSourceChanges). For instance, see what happens when player.play() is called:

video.js/src/js/player.js

Lines 1622 to 1630 in c31836c

    
           if (this.changingSrc_) { 
        
             this.ready(function() { 
        
               const retval = this.techGet_('play'); 
        
               // silence errors (unhandled promise from play) 
        
               if (retval !== undefined && typeof retval.then === 'function') { 
        
                 retval.then(null, (e) => {}); 
        
               } 
        
             });

So, the question is: is calling player.ready(myAfterSourceChangesThings) the correct way to emit events or do whatever processing your app needs after the source has actually changed? If so, can the documentation please be clarified to explain the purpose of ready more clearly? I don't see that explained well in either of the two places where I would expect it.

gkatsev · 2017-06-22T17:52:27Z

Hey, sorry to take to long to reply. Yes, I think player.ready() would be the recommended way of going about things.
I'm going to look into the remote text tracks being removed in this scenario now as well.

…to (#4450) We added a feature so that remote text tracks can auto-removed when a source changes. However, in 6.x we changed the source behavior to be asynchronous meaning that some text tracks were accidentally being removed when they weren't supposed to be. For example: ```js var player = videojs('my-player'); player.src({src: 'video.mp4', type: 'video/mp4'}); // set second arg to false so that they get auto-removed on source change player.addRemoteTextTrack({kind: 'captions', src: 'text.vtt', srclang: 'en'}, false); ``` Now when the player loads, this captions track is actually missing because it was removed. Instead of adding auto-removal tracks immediately to the list, wait until we've selected a source before adding them in. Fixes #4403 and #4315.

gkatsev · 2017-07-03T05:28:19Z

This should be fixed in 6.2.1, @jthomerson can you try it out?

jthomerson mentioned this issue Jun 7, 2017

Setting TextTrack to defaults not working when text track and video player are created dynamically. #4315

Closed

This was referenced Jun 22, 2017

fix: auto-removal remote text tracks being removed when not supposed to #4434

Closed

fix: auto-removal remote text tracks being removed when not supposed to #4450

Merged

gkatsev closed this as completed in #4450 Jun 28, 2017

github-actions bot locked as resolved and limited conversation to collaborators May 26, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remote Text Tracks Deleted Immediately Because of Asynchronous Nature of `src` #4403

Remote Text Tracks Deleted Immediately Because of Asynchronous Nature of `src` #4403

jthomerson commented Jun 7, 2017 •

edited

Loading

jthomerson commented Jun 7, 2017

gkatsev commented Jun 7, 2017

jthomerson commented Jun 8, 2017

gkatsev commented Jun 22, 2017

gkatsev commented Jul 3, 2017

Remote Text Tracks Deleted Immediately Because of Asynchronous Nature of src #4403

Remote Text Tracks Deleted Immediately Because of Asynchronous Nature of src #4403

Comments

jthomerson commented Jun 7, 2017 • edited Loading

Description

Steps to reproduce

Results

Expected

Actual

Error output

Additional Information

versions

videojs

browsers

OSes

plugins

jthomerson commented Jun 7, 2017

gkatsev commented Jun 7, 2017

jthomerson commented Jun 8, 2017

gkatsev commented Jun 22, 2017

gkatsev commented Jul 3, 2017

Remote Text Tracks Deleted Immediately Because of Asynchronous Nature of `src` #4403

Remote Text Tracks Deleted Immediately Because of Asynchronous Nature of `src` #4403

jthomerson commented Jun 7, 2017 •

edited

Loading