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

[GTK][WPE] Don't allow depth test and stencil clipping if packed depth stencil is not supported #27682

Conversation

magomez
Copy link
Contributor

@magomez magomez commented Apr 24, 2024

ed28635

[GTK][WPE] Don't allow depth test and stencil clipping if packed depth stencil is not supported
https://bugs.webkit.org/show_bug.cgi?id=273177

Reviewed by Carlos Garcia Campos.

Use a packed depth stencil format when available, which allows depth testing
and stencil clipping at the same time. If packed depth stencil is not
supported, use separate buffers for depth and stencil. In the latter case,
if both buffers are requested at the same time, the stencil buffer won't
be created.

* Source/WebCore/platform/graphics/egl/GLContext.cpp:
(WebCore::GLContext::glExtensions const):
* Source/WebCore/platform/graphics/egl/GLContext.h:
* Source/WebCore/platform/graphics/texmap/BitmapTexture.cpp:
(WebCore::depthBufferFormat):
(WebCore::BitmapTexture::initializeStencil):
(WebCore::BitmapTexture::initializeDepthBuffer):
(WebCore::BitmapTexture::~BitmapTexture):
* Source/WebCore/platform/graphics/texmap/BitmapTexture.h:

Canonical link: https://commits.webkit.org/278159@main

a1b1510

Misc iOS, tvOS & watchOS macOS Linux Windows
βœ… πŸ§ͺ style βœ… πŸ›  ios βœ… πŸ›  mac βœ… πŸ›  wpe βœ… πŸ›  wincairo
βœ… πŸ§ͺ bindings βœ… πŸ›  ios-sim βœ… πŸ›  mac-AS-debug βœ… πŸ§ͺ wpe-wk2
βœ… πŸ§ͺ webkitperl βœ… πŸ§ͺ ios-wk2 βœ… πŸ§ͺ api-mac βœ… πŸ§ͺ api-wpe
βœ… πŸ§ͺ ios-wk2-wpt βœ… πŸ§ͺ mac-wk1 βœ… πŸ›  wpe-skia
βœ… πŸ§ͺ api-ios βœ… πŸ§ͺ mac-wk2 βœ… πŸ›  gtk
βœ… πŸ›  tv βœ… πŸ§ͺ mac-AS-debug-wk2 βœ… πŸ§ͺ gtk-wk2
βœ… πŸ›  tv-sim   πŸ§ͺ mac-wk2-stress βœ… πŸ§ͺ api-gtk
βœ… πŸ›  watch
βœ… πŸ›  πŸ§ͺ unsafe-merge βœ… πŸ›  watch-sim

@magomez magomez self-assigned this Apr 24, 2024
@magomez magomez added the WebKitGTK Bugs related to the Gtk API layer. label Apr 24, 2024
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Apr 24, 2024
@magomez magomez removed the merging-blocked Applied to prevent a change from being merged label Apr 24, 2024
@magomez magomez force-pushed the eng/GTKWPE-Dont-allow-depth-test-and-stencil-clipping-if-packed-depth-stencil-is-not-supported branch from 93c2dd9 to 7cbe359 Compare April 24, 2024 12:15
namespace WebCore {

GLenum depthBufferFormat()
{
static std::once_flag onceFlag;
Copy link
Contributor

Choose a reason for hiding this comment

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

TextureMapper is thread agnostic. Clients have to call all TextureMapper API from a single thread.
I think TextureMapper doesn't need thread synchronization.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I know that TextureMapper is only used from the compositor thread. But I'm using std::call_once to ensure that the initialization of the depth buffer format happens only once. I could use a boolean flag to check whether the value is initialized or not, but std::call_once seems to be commonly used for that.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think it's worth calling this only once, since both the extension and the version are cached.

@fujii
Copy link
Contributor

fujii commented Apr 24, 2024

Do you know which layout test case covers this change?

@magomez
Copy link
Contributor Author

magomez commented Apr 25, 2024

Do you know which layout test case covers this change?

I'm trying to create a test for it, but it's tricky.

@magomez
Copy link
Contributor Author

magomez commented Apr 29, 2024

Do you know which layout test case covers this change?

Creating a simple testcase to reproduce this is being complicated and I can't be stuck with this. The problem is reproducible in the page https://stv.vtmgo.be/vtmgo/prerelease/eos/index.html#/main/storefront?type=main . The size of the rendering area (not the window, as the window decorations reduce the size of the area) must be 1280x720 for it to happen, otherwise a scaling will be applied and then it won't reproduce.

There's a trailer button on the middle right part of the screen with a spinner animation. That's the one causing the problem. The main element of the page has position:fixed, which forces the creation of a stacking context and the rendering of almost all the page happens to a BitmapTexture with a depth buffer. Then the spinner image tries to do an stencil clip, adding the stencil buffer to the BitmapTexture, and this breaks the rendering of the page (at least when using mesa).

@magomez magomez force-pushed the eng/GTKWPE-Dont-allow-depth-test-and-stencil-clipping-if-packed-depth-stencil-is-not-supported branch from 7cbe359 to a1b1510 Compare April 29, 2024 14:56
@magomez magomez added the unsafe-merge-queue Applied to send a pull request to merge-queue, but skip building and testing label Apr 30, 2024
…h stencil is not supported

https://bugs.webkit.org/show_bug.cgi?id=273177

Reviewed by Carlos Garcia Campos.

Use a packed depth stencil format when available, which allows depth testing
and stencil clipping at the same time. If packed depth stencil is not
supported, use separate buffers for depth and stencil. In the latter case,
if both buffers are requested at the same time, the stencil buffer won't
be created.

* Source/WebCore/platform/graphics/egl/GLContext.cpp:
(WebCore::GLContext::glExtensions const):
* Source/WebCore/platform/graphics/egl/GLContext.h:
* Source/WebCore/platform/graphics/texmap/BitmapTexture.cpp:
(WebCore::depthBufferFormat):
(WebCore::BitmapTexture::initializeStencil):
(WebCore::BitmapTexture::initializeDepthBuffer):
(WebCore::BitmapTexture::~BitmapTexture):
* Source/WebCore/platform/graphics/texmap/BitmapTexture.h:

Canonical link: https://commits.webkit.org/278159@main
@webkit-commit-queue webkit-commit-queue force-pushed the eng/GTKWPE-Dont-allow-depth-test-and-stencil-clipping-if-packed-depth-stencil-is-not-supported branch from a1b1510 to ed28635 Compare April 30, 2024 07:35
@webkit-commit-queue
Copy link
Collaborator

Committed 278159@main (ed28635): https://commits.webkit.org/278159@main

Reviewed commits have been landed. Closing PR #27682 and removing active labels.

@webkit-commit-queue webkit-commit-queue merged commit ed28635 into WebKit:main Apr 30, 2024
@webkit-commit-queue webkit-commit-queue removed the unsafe-merge-queue Applied to send a pull request to merge-queue, but skip building and testing label Apr 30, 2024
@aperezdc aperezdc added the GLib Suggested Backport Suggest this merge request be backported to current WPE/GTK stable branch label May 2, 2024
@aperezdc
Copy link
Contributor

aperezdc commented May 2, 2024

Backported into the 2.44 branch as commit bd536df

@aperezdc aperezdc removed the GLib Suggested Backport Suggest this merge request be backported to current WPE/GTK stable branch label May 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
WebKitGTK Bugs related to the Gtk API layer.
Projects
None yet
7 participants