-
Notifications
You must be signed in to change notification settings - Fork 2.7k
fix: handle page restoration from bfcache in Safari #7683
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.
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
base: master
Are you sure you want to change the base?
fix: handle page restoration from bfcache in Safari #7683
Conversation
|
I don't think we should apply this to all instances and browsers. The buffer controller would be the appropriate place to handle premature closing of the MediaSource, with recoverMediaError handling the detach/attach and loading. The source should not be reset in browsers that do not display this issue anywhere the MediaSource remains open. It's also concerning that we're adding a global listener and never removing it. All listeners should be removed when instances are destroyed. |
|
Thanks, @robwalch. Appreciate the feedback on the initial approach. I didn't realize I've removed the global listener, for |
src/controller/buffer-controller.ts
Outdated
| const handler = this._handleSafariMediaSourceClose.bind(this); | ||
| this.safariSourceCloseHandler = handler; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This binding is unnecessary with arrow function properties. private _handleSafariMediaSourceClose = () => will always be involved with the current class instance context for "this". (See other examples in the project like private onMediaSeeked = () =>.)
src/controller/buffer-controller.ts
Outdated
| data: MediaAttachingData, | ||
| ) { | ||
| const media = (this.media = data.media); | ||
| this.needsMediaSourceCloseRecovery = this.detectMediaSourceCloseIssue(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I didn't mean to suggest that this should be behind a user-agent check. I'd prefer to use existing listeners, or only add new event listeners (like "pageshow") as needed to detect that the source closure resulted from backgrounding the page.
We already have a listener for 'sourceclose': _onMediaSourceClose >
- Can we simply use that to reopen when media is still attached and the player is not being destroyed?
- Are there any other conditions that should be considered, like
video.error, hls error, or loading state? - When is the MediaSource closed in relation to the page being backgrounded when the browser bug presents?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, sorry for the misunderstanding! I think we have two solid options here.
- We can use the existing
sourcecloselistener /_onMediaSourceClosehandler. On Safari, the MediaSource is closed when returning to the page via bfcache, so thesourcecloseevent fires immediately when we land back on the page. When this occurs, there's also a media error ("Load was aborted"), but I don't think that error is specific to this issue.
If media is still attached when sourceclose fires, we can trigger recovery. My only concern is understanding all scenarios where sourceclose may fire. Currently, the event also fires during media detachment, but we don't catch it because we remove the listener before the event is captured.
Something like this works:
private _onMediaSourceClose = () => {
if (this.media) {
this.hls.recoverMediaError();
}
};My concern is: what happens if we start catching the sourceclose event during media detachment in the future? I think we would try to trigger a recovery? But then again, this.media would be null, so the condition here probably protects us from this.
- We can add the
pageshowlistener from earlier to specifically account for bfcache restoration:
private _onPageShow = (event: PageTransitionEvent) => {
if (event.persisted && this.mediaSource?.readyState === 'closed' && this.media) {
this.hls.recoverMediaError();
}
};This is more explicit about handling bfcache restoration. On other browsers, the MediaSource readyState remains open when restoring from bfcache, so this check would prevent unnecessary recovery attempts. However, we would be adding a global window listener to all browsers and instances.
I pushed up a change for option 1, but happy to continue discussing.
7931295 to
a7e5d32
Compare
a7e5d32 to
4d10367
Compare
This PR will...
This PR fixes an issue where video playback fails to resume after navigating back and forth between pages in Safari. The problem manifests as buffer append errors when reinitializing the player after a page navigation.
Why is this Pull Request needed?
The issue affects users who navigate away from a page with a playing HLS.js video and then return to it on Safari.
Are there any points in the code the reviewer needs to double check?
Resolves issues:
Fixes #7578 - Unable to resume video playback after navigating back and forth between two pages
Demo of issue:
https://github.com/user-attachments/assets/d87ab24b-9774-474a-bbc1-a096d4d70009
Demo of fix:
Screen.Recording.2026-01-05.at.6.35.50.PM.mov
Checklist