Cannot set startTime on Safari-handled HLS content after loading it again

[alexandercerutti]

Have you read the FAQ and checked for duplicate open issues?

Yes, none found

What version of Shaka Player are you using?

3.3.4, 3.3.5

Can you reproduce the issue with our latest release version?

Yes

Can you reproduce the issue with the latest code from main?

Not tested

Are you using the demo app or your own custom app?

Custom App

If custom app, can you reproduce the issue using our demo app?

I guess it cannot be tested (see below for the reason).

What browser and OS are you using?

Safari 15, iOS 15 and MacOS 12.2.1 (see differences below)

For embedded devices (smart TVs, etc.), what model and firmware version are you using?

What are the manifest and license server URIs?

I don't know yet if I can provide one (cause they are owned by our client). I'll provide via mail if needed

What configuration are you using? What is the output of player.getConfiguration()?

What did you do?

• load a (clean for sure) HLS content with startTime not specified or to 0 and leave it go for a few seconds.
• unload the content
• load the content back again with a startTime

What did you expect to happen?

The content should start from the specified point

What actually happened?

Content starts again from 0

---

I've investigated a bit what is happening under the hood and it seems like there's kind of race condition issue that happens on Safari iOS but not on Safari MacOS.

My use case is what follows: we load a content and at a certain point we might have an AD midroll. Hence, we unload the content and give Video node control to our AD Provider's SDK.

Then, we give back Video node control to Shaka, by passing to it the latest currentTime as startTime in .load().

.load call is centralized such as:

await this.player.load(content.src, content.startPosition, content.mimeType);

But when this is executed on Safari iOS, the startTime is ignored. Deep diving inside shaka, I've found there's an utility, that is being used, shaka.media.SrcEqualsPlayhead, which gets created everytime the content is loaded.

This class owns these notable things:

• Instance variable this.started_
• onLoaded(), which gets executed when the media is in ready state and sets this.started_ to true.
• setStartTime(startTime) method, which uses this.started_ to update the startTime

In player.js, onSrcEquals_ executes this:

this.playhead_ = new shaka.media.SrcEqualsPlayhead(mediaElement);

...

if (has.startTime != null) {
  this.playhead_.setStartTime(has.startTime);
}

and expects that onLoaded() in SrcEqualsPlayhead will get called asynchronously but it may happen (especially on iOS, but it happened once also on Mac) that the function is called before setStartTime could get called. Therefore startTime setting is ignored.

I guess that onLoaded() could be moved into a Promise that will always resolve asynchronously but I'm not very into the codebase, so I don't know if it could be the right solution.

I'm available for any clarification if needed.
Thank you!

[joeyparrish]

Often you don't know if an event has already fired before you start listening for it. With media lifecycle events, the readyState attribute tells you what stage you're in, so you can avoid this scenario. To simplify that, we have the utility shaka.util.MediaReadyState.waitForReadyState(). It calls a callback when a certain ready state is reached. If it has already been reached, it calls the callback right away. Otherwise, it waits for the corresponding event.

In the case where the state has already been reached, perhaps the utility should call the callback asynchronously (after a Promise) instead of synchronously before returning. I think that would solve it by deferring that callback until the next interpreter cycle.

An alternate fix could be to change the Playhead interface. We could add a separate method to "start" any async or other actions, instead of those being done in the constructor. Then Player would call the constructor, then setStartTime(), then call "go", or whatever would be an actual good name for the method. That new method would start timers, event listeners, etc., after all other setup methods had been called. This way, the timing would be more explicit.

@alexandercerutti, would you be willing to test patches/branches for me to make sure these proposed fixes work? I don't have an iPhone or Mac available at home to test this.

[joeyparrish]

Here are two possible fixes:

• via Promise: joeyparrish@c6d4ce9
• via new method: joeyparrish@d9acbd5

I expect both to work, but I think I prefer the addition of the new method. If that fix works, I think that is the one I'd prefer to merge.

[alexandercerutti]

Hey @joeyparrish, thanks for your reply and your possible fixes!

I'm going to test them asap. I'm not quite sure if the best way to test them is by changing the compiled code or to recompile it from scratch and replace compiled.debug.js inside npm dependency in node_modules.

I'm not quite sure how to start in case of compilation. Is there any document I can refer to? I was looking in the repo but I wasn't able to find it. May I also propose to create a Github Wiki with documentation?

[joeyparrish]

Documentation is built from the /docs/ folder and lives in https://shaka-player-demo.appspot.com/docs/api/

Compilation is documented briefly in https://shaka-player-demo.appspot.com/docs/api/tutorial-welcome.html

The short version is:

python build/all.py

Output goes to dist/

I hope that helps!

[alexandercerutti]

Thanks @joeyparrish!

I tried to compile both solutions (but yet I have to test them, I'll do as soon as my test iPhone will have enough charge 😝).

Promise solution built correctly, but the other one failed with this error:

[alexandercerutti]

I don't know, I was testing with my work custom application which compiles with Webpack 4. What's weird is that if I try to compile Promise fix, Webpack will output invalid characters:

u
/* WEBPACK VAR INJECTION */}.call(this, __webpack_require__(/*! ./../../webpack/buildin/global.js */ "./node_modules/webpack/buildin/global.js")))ndefinedundefinedundefinedundefinedundefined

I don't actually know the reason.
I'll have to setup a custom project, hoping that this issue won't also happen in the next version with this fix (if you'll decide to use Promise version or if the issue will also happen with the other solution).

EDIT: This issue seems to happen only with Webpack 4 but not with Webpack 5. So, it's okay, cause it might be our "excuse" to update to Webpack 5. Now I have to test the Promise solution.

[alexandercerutti]

Uhm, there's something weird happening with Promise fix. I mean, through some breakpoints I can see that the order issue is fixed and startTime is set on mediaElement when onLoaded is called.

But still, the content starts from 0, and now I have to understand on which side this happens.

I've found this and I think this might be the reason (I have to do some tests - I'll try to change the code and recompile and report it back): https://stackoverflow.com/questions/18266437/html5-video-currenttime-not-setting-properly-on-iphone

I see that in onLoaded, a listener to seeking is added and right after the listener, the time is set.

shaka-player/lib/media/playhead.js

Lines 88 to 94 in b70938c

	this.eventManager_.listenOnce(this.mediaElement_, 'seeking', () => {
	this.started_ = true;
	});
	const currentTime = this.mediaElement_.currentTime;
	// Using the currentTime allows using a negative number in Live HLS
	const newTime = Math.max(0, currentTime + this.startTime_);
	this.mediaElement_.currentTime = newTime;

I guess there might be something else to be done (like what I've proposed above) to create a complete solution... maybe instead of setting the time there, setting it on loadeddata, but this should be tested on all browsers.

EDIT: Yes, that worked.

I've changed playhead.js to this:

         this.eventManager_.listenOnce(this.mediaElement_, 'seeking', () => {
           this.started_ = true;
         });
-        const currentTime = this.mediaElement_.currentTime;
-        // Using the currentTime allows using a negative number in Live HLS
-        const newTime = Math.max(0, currentTime + this.startTime_);
-        this.mediaElement_.currentTime = newTime;
+
+        this.eventManager_.listenOnce(this.mediaElement_, 'loadeddata', () => {
+          const currentTime = this.mediaElement_.currentTime;
+          // Using the currentTime allows using a negative number in Live HLS
+          const newTime = Math.max(0, currentTime + this.startTime_);
+          this.mediaElement_.currentTime = newTime;
+        });

At this point, this solution should be tested everywhere (and I'm going to test it on desktop Browser at least). After building issues have been solved on the second solution and I test that everything is okay, I might create a Pull Request if you want.

EDIT 2: On Chrome and Firefox Desktop, this change seems to work fine as well, but Safari Desktop seems to break... :/

EDIT 3: I've tried a different approach. I've moved the listener to loadeddata outside of onLoaded, cause it probably might be too late. By doing this way, it seems to work perfectly on iOS, Safari Desktop, Firefox Desktop and Chrome Desktop.

@@ -88,10 +88,6 @@ shaka.media.SrcEqualsPlayhead = class {
         this.eventManager_.listenOnce(this.mediaElement_, 'seeking', () => {
           this.started_ = true;
         });
-        const currentTime = this.mediaElement_.currentTime;
-        // Using the currentTime allows using a negative number in Live HLS
-        const newTime = Math.max(0, currentTime + this.startTime_);
-        this.mediaElement_.currentTime = newTime;
       }
     };
 
@@ -100,6 +96,13 @@ shaka.media.SrcEqualsPlayhead = class {
         this.eventManager_, () => {
           onLoaded();
         });
+
+    this.eventManager_.listenOnce(this.mediaElement_, 'loadeddata', () => {
+      const currentTime = this.mediaElement_.currentTime;
+      // Using the currentTime allows using a negative number in Live HLS
+      const newTime = Math.max(0, currentTime + this.startTime_);
+      this.mediaElement_.currentTime = newTime;
+    });

[avelad]

@joeyparrish can you review the last comment?

[alexandercerutti]

@joeyparrish do you have any news about this? This is blocking a more-than-actual production issue (and report) on our product. Should I open a Pull Request? In case, I'd be happy to. If so, against which branch?

[avelad]

Any PR are welcome! Always against main branch! Thanks!

[alexandercerutti]

Oh, wait. Now I'm remembering: I wasn't able to test the second fix possibility. So I can open a PR for Promise fix only. I'm not sure this is actually okay as @joeyparrish expressed preference for the second solution.

[alexandercerutti]

@joeyparrish any news? What should be fixed on the second sample so it can be tested?

[alexandercerutti]

Hey @joeyparrish, were you able to give a look to this?
We have a task open for this issue (even if I'll probably be able to test this again in September, when I'll get back from remote working). So, just to understand if it would be good to proceed with a PR with the Promise solution. Just to remember you, I wasn't able to test the solution you preferred (new method), maybe due to some kind of missing comment that I'm not quite sure how to fix.

Thank you very much!

[avelad]

@joeyparrish Can you review it again? if i remember correctly... you have two branches in your fork about this issue.

[alexandercerutti]

Hey @joeyparrish @avelad, I finally had the chance to put my hands on this again and I tried to make things work. Now I'm of the idea that the "method way" @joeyparrish proposed is the best way.

I had to add a missing .ready() call in onSrcEquals method, but I'm not very sure this is the best place. I thing I'm going to open a PR with the fix to this (which includes what @joeyparrish did).