bigscreen-player icon indicating copy to clipboard operation
bigscreen-player copied to clipboard

Return `undefined` from `getCurrentTime()` when it is unknown

Open tjenkinson opened this issue 5 years ago • 1 comments

📺 What

Currently getCurrentTime() has a default value of 0 in several cases. This is not ideal because it makes it impossible to know if it truly is time 0, or if the time is not known yet.

It also sometimes returns the wrong value.

An example where this is an issue is if you start a live stream from the live point. Currently you will get back the incorrect time whilst the stream loads, and then it will jump to the correct value.

With this PR when the strategy is still loading (hasn't changed the state to something other than WAITING) getCurrentTime() will now always return

  • the value of the latest setCurrentTime request or
  • initialPlaybackTime or
  • undefined

I chose to put this logic in 'bigscreenplayer.js' instead of the individual strategies because I think this is something that benefits them all.

This also means a call to getCurrentTime() in the same stack as setCurrentTime() is guaranteed to be what you expect even if the update happens asynchronously in the strategy.

This PR also contains a fix in the msestrategy getCurrentTime() to prevent it returning incorrect values when the media element isn't ready. This change isn't strictly needed given the other change in 'bigscreenplayer.js' prevents it causing an issue, but it's probably good to fix anyway.

IPLAYERTVV1-10907

🛠 How

  • Introduce a SeekState enum which can be NONE, QUEUED or IN_FLIGHT. When a seek is QUEUED or IN_FLIGHT we now return the time we're seeking to from getCurrentTime() instead of calling through to the strategy.
  • In msestrategy return undefined instead of 0 from getCurrentTime when the time is unknown.
  • In msestrategy return undefined when the media element readyState is at least HAVE_METADATA. Before this dashjs has not set the current time to near the live point meaning it would return the wrong value.
  • Update/add tests

✅ Acceptance criteria

  • [ ] getCurrentTime() returns undefined whilst a stream is loading with no initialPlaybackTime using the mse strategy
  • [ ] getCurrentTime() always returns the time it was set to immediately after setCurrentTime()
  • [ ] getCurrentTime() initially returns the value of initialPlaybackTime.
  • [ ] getCurrentTime() returns the expected time when the media state is not WAITING

tjenkinson avatar Apr 07 '20 15:04 tjenkinson

We should be trying to be close to normal web behaviour when providing the current time

Returning undefined instead of 0 is not sticking to that, but I think making getCurrentTime() return the queued seek and be correct immediately after setting it is. The queued seek time looks to me like the default-playback-start-position.

Also I reckon when that spec was first written live streams where there might not be a time 0 weren't really a thing for a native media element, and therefore defaulting to 0 made a lot of sense because if no api was invoked, 0 would be the correct time. If the api was being designed today with live streams and MSE being a thing I wonder if it would be different? ¯\(ツ)

Can we not just check whether or not there is a timeCorrection & time is 0 then return 0

But then if getCurrentTime() returns 0, is it because you are at time 0 in the video/stream, or is it because it's not loaded yet and jumped to near the live point?

Alternatively I'd see something like this spike as more of a long term 'fix' (don't say the player is fully initialised when is isn't) https://github.com/bbc/bigscreen-player/compare/emit-loaded-spike

Delaying the point when the player is considered initialised sounds ok to me, but I still think getCurrentTime() should return the queued seek (ie default-playback-start-position), and it should be possible for the user to call setCurrentTime() before the player is considered loaded (just like it is with a standard media element). If we want to return 0 instead of undefined it needs to be possible on the api to query if the player is loaded yet. Maybe a isInitialised()?

tjenkinson avatar Apr 08 '20 13:04 tjenkinson