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.

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

Canvases should reset their contents when width or height is set #3613

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

greggman
Copy link
Contributor

even if the sizes are the same.

Note: Canvas 2D does this. There are various issues related to requestAnimationFrame and ResizeObserver that this affects. It would probably be best of all the APIs did the same thing so that the workarounds for resizing the canvas are the same across APIs

Safari passes this test, Firefox and Chrome fail

even if the sizes are the same.

Note: Canvas 2D does this. There are various issues related to
requestAnimationFrame and ResizeObserver that this affects. It
would probably be best of all the APIs did the same thing so that
the workarounds for resizing the canvas are the same across APIs

Safari passes this test, Firefox and Chrome fail
@greggman
Copy link
Contributor Author

Probably need a similar test for OffscreenCanvas?

gl.clearColor(0.25, 0.5, 0.75, 1);
gl.clear(gl.COLOR_BUFFER_BIT | gl.DEPTH_BUFFER_BIT);
// Check it's not cleared
checkCanvasContentIs(64, 128, 192, 255);
Copy link
Contributor

Choose a reason for hiding this comment

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

Would be nice to test that stencil and depth are cleared, too. That's of course cumbersome..

@greggman
Copy link
Contributor Author

Probably need a similar test for OffscreenCanvas?

FYI, Offscreen canvas is apparently different. Safari, Firefox, and Chrome don't reset the WebGL canvas when the size is set to the same size.

Copy link
Member

@kenrussell kenrussell left a comment

Choose a reason for hiding this comment

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

Hopefully it'll still be possible for browsers to optimize the reallocation of the back buffer by batching up changes to width and height.

@kenrussell kenrussell requested a review from kdashg January 29, 2024 20:01
@kdashg
Copy link
Contributor

kdashg commented Jan 31, 2024

I believe we have a different test that requires the opposite, or otherwise that there is content that assumes the opposite, because I see this line in Firefox:
https://searchfox.org/mozilla-central/rev/2a867dd1ab015c3ef24b774a57709fb3b3dc4961/dom/canvas/ClientWebGLContext.cpp#797

if (size == curSize) return NS_OK;  // MUST skip no-op resize

@greggman
Copy link
Contributor Author

Safari doesn't do this so any site that is expecting a no-op is not getting a no-op

@MarkCallow
Copy link
Collaborator

What is the purpose of resetting the contents when width or height is set to the same size? I'm pretty sure this kind of choice is behind most of the cases where I see the content I want to read then the screen/window/canvas goes black and the same content is redrawn usually after an irritatingly long time. This is ugly and annoying to the user.

@greggman
Copy link
Contributor Author

greggman commented Feb 1, 2024

It is already true and expected in Canvas 2D. It can't be fixed there because people actually use it as a shortcut to clear the canvas.

The interop between compositing the page and canvas updates has a bunch of issues. See: https://stackoverflow.com/questions/77842752/how-do-i-use-resizeobserver-with-requestanimationframe-correctly.

Because of that, IMO, it would best of all the canvas APIs did the same thing here so that the correct usage for one type of context ("2d", "webgl", "webgl2", "webgpu", "bitmaprender") is the same for all types of contexts. Otherwise you get into this issue where you do things one way, they seem to work, you pass on that "this is how you use canvas" message on the internet. You post examples. People copy the examples. It spreads far and wide. But, it turns out it's wrong because different canvas APIs treat this differently. If they all treat it the same then the "this is how you use canvas" issue has one correct answer.

Further, anyone who was expecting it to be a no-op was wrong. It's never been a no-op on Safari so all iOS users and all MacOS users using Safari, this has never been a no-op

@kdashg
Copy link
Contributor

kdashg commented Feb 1, 2024

I do not think there is enough benefit here to outweigh the backwards-compat hit.

@greggman
Copy link
Contributor Author

greggman commented Feb 1, 2024

What backward compat hit?

It already clears on Safari so you wanted to be compatible for the last 11 years you needed to deal with the fact that it clears.

@kdashg
Copy link
Contributor

kdashg commented Feb 1, 2024

When I accidentally made this change in Firefox ~5years ago, I reverted it for a reason. I regret that I did not write down what it was.
It is not clear to me that this has always been the case in Safari either. It could be a regression, or maybe people have started working around it such that I would not have needed to revert this change when I made it in Firefox years ago.
This is enough uncertainty that I would rather not risk it, because I don't think that aligning behavior that has been out of alignment for a decade is that important.

@kenrussell
Copy link
Member

Per WebGL working group meeting of 2024-02-08: to make progress on this, it's necessary to create a patch (perhaps against Chromium) which removes the early-out for resizing canvases to the same size, and send that patch through the trybots, which will run the entire WebGL CTS. This will help find whether there is a test which contradicts the one being added here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants