-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Fix unhandled framebuffer error when tab wakes from sleep #7303
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
Open
johanrd
wants to merge
7
commits into
maplibre:main
Choose a base branch
from
johanrd:fix/framebuffer-error-on-tab-wake
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
40f7ff2
Remove `checkFramebufferStatus` from the `Framebuffer` constructor. A…
johanrd b304634
Merge branch 'main' into fix/framebuffer-error-on-tab-wake
johanrd fe80484
Merge branch 'main' into fix/framebuffer-error-on-tab-wake
johanrd 54f6921
Make checkFramebufferStatus() bind its own FBO before checking
johanrd 6bd3c74
remove framebuffer test: marginal value
johanrd c725d1d
Revert "remove framebuffer test: marginal value"
johanrd f8352bb
Merge branch 'main' into fix/framebuffer-error-on-tab-wake
johanrd File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,22 @@ | ||
| import {describe, test, expect, vi} from 'vitest'; | ||
| import {Context} from './context'; | ||
| import {Framebuffer} from './framebuffer'; | ||
|
|
||
| describe('Framebuffer', () => { | ||
| test('constructor does not check framebuffer status before attachments are set', () => { | ||
| const gl = document.createElement('canvas').getContext('webgl'); | ||
| vi.spyOn(gl, 'checkFramebufferStatus').mockReturnValue(0); | ||
| const context = new Context(gl); | ||
|
|
||
| expect(() => new Framebuffer(context, 256, 256, false, false)).not.toThrow(); | ||
| }); | ||
|
|
||
| test('checkFramebufferStatus throws when framebuffer is incomplete', () => { | ||
| const gl = document.createElement('canvas').getContext('webgl'); | ||
| vi.spyOn(gl, 'checkFramebufferStatus').mockReturnValue(0); | ||
| const context = new Context(gl); | ||
| const fbo = new Framebuffer(context, 256, 256, false, false); | ||
|
|
||
| expect(() => fbo.checkFramebufferStatus()).toThrow('Framebuffer is not complete'); | ||
| }); | ||
| }); |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 isn't really checking status, does it?
Uh oh!
There was an error while loading. Please reload this page.
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.
@HarelM Good catch — I think this name is papering over a deeper question about whether the check should exist at all. Some history:
It started as a debug-only assert:
maplibre-gl-js/src/gl/framebuffer.js
Line 26 in 00d7026
#1485 turned it into a production throw, and you flagged exactly this risk on that PR:
That seems to reflect what's showing up in Sentry now.
Then #5266 tightened the
maplibre-gl-js/src/ui/map.ts
Line 3384 in e6d6923
Zooming out
Given all that, I'd suggest deleting ensureFramebufferComplete() and all 5 call sites instead of renaming it. After that, isFramebufferNotCompleteError and the redraw() try/catch guard an error that can no longer be thrown from our code, and can go too.
The alternative is developing an active recovery path (force context loss/restore on failure), but honestly WebGL may handle a transient framebuffer blip fine on its own if we just get out of the way. Happy to restructure the PR either direction.
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.
Given the history you dug, I trust you understand this better than me, so if you think a direction with worth pursuing, feel free to do so.
I would like to get @birkskyum and @ToHold 's input here since @birkskyum recently fixed a long lasting bug with framebuffer misalignment and @ToHold did a lot of work around recovery from context loss.
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.
CC: @mvanhorn who's also added some PRs around context restored.
Is there a sudden interest in making this work well which wasn't a big motivation before?