bigscreen-player
bigscreen-player copied to clipboard
Return `undefined` from `getCurrentTime()` when it is unknown
📺 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
setCurrentTimerequest or initialPlaybackTimeorundefined
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.
🛠 How
- Introduce a
SeekStateenum which can beNONE,QUEUEDorIN_FLIGHT. When a seek isQUEUEDorIN_FLIGHTwe now return the time we're seeking to fromgetCurrentTime()instead of calling through to the strategy. - In
msestrategyreturnundefinedinstead of0fromgetCurrentTimewhen the time is unknown. - In
msestrategyreturnundefinedwhen the media elementreadyStateis at leastHAVE_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()returnsundefinedwhilst a stream is loading with noinitialPlaybackTimeusing the mse strategy - [ ]
getCurrentTime()always returns the time it was set to immediately aftersetCurrentTime() - [ ]
getCurrentTime()initially returns the value ofinitialPlaybackTime. - [ ]
getCurrentTime()returns the expected time when the media state is notWAITING
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()?