Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: ensure all timelineregionenter events are fired #6713

Merged
merged 1 commit into from
May 31, 2024
Merged

fix: ensure all timelineregionenter events are fired #6713

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions lib/player.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2405,7 +2405,7 @@ shaka.Player = class extends shaka.util.FakeEventTarget {
const setupPlayhead = (startTime) => {
this.playhead_ = this.createPlayhead(startTime);
this.playheadObservers_ =
this.createPlayheadObserversForMSE_(startTimeOfLoad);
this.createPlayheadObserversForMSE_(startTime);
Copy link
Member

Choose a reason for hiding this comment

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

I could use more explanation in the PR description. Why is startTime correct and startTimeOfLoad wrong? What's the difference?

Might also be worth a comment here in the code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are extensive notes in the associated issue #6711

Copy link
Contributor Author

@littlespex littlespex May 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

We don't understand why startTimeOfLoad is being used, especially for VOD. The startTimeOfLoad argument is used to set the startsPastZero flag in RegionObserver. The comments there seem to indicate that it is playhead related:

shaka-player/lib/media/region_observer.js

Lines 39 to 44 in 30ac8c0

* Whether the asset is expected to start at a time beyond 0 seconds.
* For example, if the asset is a live stream.
* If true, we will not start polling for regions until the playhead has
* moved past 0 seconds, to avoid bad behaviors where the current time is
* briefly 0 before we have enough data to play.
* @private {boolean}

This makes sense. If the user called player.load() with a non-zero start time, the observer should wait until the initial seek has completed. startTimeOfLoad is the wall clock time of when the load operation started. How does that play into how the region observer operates? Before the commit mentioned in the issue, startTimeOfLoad was alway NaN at the time createPlayheadObserversForMSE_, which coincidentally resulted in the correct behavior for VOD.

Copy link
Contributor Author

@littlespex littlespex May 31, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@theodab Can you provide any insight here? The comments in #5361 state:

This change modifiers the region observer so that it does not start examining regions until the time gets past 0 seconds, except in the case of VOD.

Unfortunately, after the @avelad's state engine refactor, startTimeOfLoad is always greater than zero, which means startsPastZero is always true, regardless of live/vod.

This comments also mention:

This does not happen every time; it's likely based on the exact time it takes the player to start playing.

With startTimeOfLoad being a timestamp, it doesn't seem like the correct value to use here. It suggests a delta like Date.now() / 1000 - startTimeOfLoad. That said, when we test on versions <= 4.5.0, we see startTimeOfLoad always comes in as NaN, so really only this.isLive() has any effect on the observer's startsPastZero flag.

@joeyparrish This fact led us to look into what values makes the most sense to use here, and we determined that load()'s startTime was the answer. When playing live, wait for a the playhead to update before dispatching timeline region events. When playing VOD, the player should also wait for any initial seek operations before tracking timeline region changes.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I think the original code using startTimeOfLoad was a mistake. It's supposed to check for if we were loading partway through the presentation, so that we won't fire region events for regions that the player loads in after the end of. The startTime variable would be the actual variable we want for this.

Honestly, we should probably just rename the startTImeOfLoad variable. It's confusingly similar to startTime, despite the two variables having much different meanings and effects in the loading process.


// We need to start the buffer management code near the end because it
// will set the initial buffering state and that depends on other
Expand Down Expand Up @@ -3242,16 +3242,16 @@ shaka.Player = class extends shaka.util.FakeEventTarget {
* Create the observers for MSE playback. These observers are responsible for
* notifying the app and player of specific events during MSE playback.
*
* @param {number} startTimeOfLoad
* @param {number} startTime
* @return {!shaka.media.PlayheadObserverManager}
* @private
*/
createPlayheadObserversForMSE_(startTimeOfLoad) {
createPlayheadObserversForMSE_(startTime) {
goog.asserts.assert(this.manifest_, 'Must have manifest');
goog.asserts.assert(this.regionTimeline_, 'Must have region timeline');
goog.asserts.assert(this.video_, 'Must have video element');

const startsPastZero = this.isLive() || startTimeOfLoad > 0;
const startsPastZero = this.isLive() || startTime > 0;

// Create the region observer. This will allow us to notify the app when we
// move in and out of timeline regions.
Expand Down
Loading