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

[WebXR] Enable implicit clear for WebXROpaqueFramebuffer #23209

Merged
merged 1 commit into from
Feb 6, 2024

Conversation

djg
Copy link
Contributor

@djg djg commented Jan 25, 2024

153d8bb

[WebXR] Enable implicit clear for WebXROpaqueFramebuffer
https://bugs.webkit.org/show_bug.cgi?id=268042
rdar://117090654

Reviewed by Kimmo Kinnunen.

WebGL's default framebuffer has an implicit clear behavior. The WebXR
specification requires the same behavior for its WebXROpaqueFramebuffer,
therefore WebXR must ensure that it's buffers are always cleared for each rAF of
the session.

Currently we assume content does not do redundant initial clear, as the spec
says the buffer always starts cleared.

* Source/WebCore/Modules/webxr/WebXROpaqueFramebuffer.cpp:
(WebCore::WebXROpaqueFramebuffer::startFrame):
(WebCore::WebXROpaqueFramebuffer::setupFramebuffer):
* Source/WebCore/html/canvas/WebGLRenderingContextBase.h:
* Source/WebCore/html/canvas/WebGLUtilities.h:
(WebCore::ScopedClearColorAndMask::ScopedClearColorAndMask):
(WebCore::ScopedClearColorAndMask::~ScopedClearColorAndMask):
(WebCore::ScopedClearDepthAndMask::ScopedClearDepthAndMask):
(WebCore::ScopedClearDepthAndMask::~ScopedClearDepthAndMask):
(WebCore::ScopedClearStencilAndMask::ScopedClearStencilAndMask):
(WebCore::ScopedClearStencilAndMask::~ScopedClearStencilAndMask):

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

e8de6d2

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 βœ… πŸ›  gtk
  πŸ§ͺ api-ios   πŸ§ͺ mac-wk2   πŸ§ͺ gtk-wk2
  πŸ›  tv   πŸ§ͺ mac-AS-debug-wk2   πŸ§ͺ api-gtk
  πŸ›  tv-sim   πŸ§ͺ mac-wk2-stress
  πŸ›  watch
βœ… πŸ›  πŸ§ͺ unsafe-merge   πŸ›  watch-sim

@djg djg requested review from cdumez and rniwa as code owners January 25, 2024 01:04
@djg djg self-assigned this Jan 25, 2024
@djg djg added the WebXR For bugs in WebXR label Jan 25, 2024
Copy link
Contributor

@kkinnunen-apple kkinnunen-apple left a comment

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 correct.
WebXR framebuffer must be cleared before session rAF, regardless of whether the client draws anything or not.

I think your initial problem to solve is "WebXR does not clear the session framebuffer before rAF".
The solution should be "clear the session framebuffer before rAF".

I think we could focus on optimizing clears if they are problems.
The WebGL clear optimizaiton was a more pressing matter when the preserveDrawingBuffer was default and/or supported. WebXR doesn't have these issues.

class WebGLRenderingContextBase;

// Interface to state related to a set of framebuffer attachments.
class WebGLFramebufferBase {
Copy link
Contributor

Choose a reason for hiding this comment

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

These are only correctly maintained for WebGLDefaultFramebuffer, WebXROpaqueFramebuffer.
These are not correctly maintained for WebGLFramebuffer.

The correct, minimal interface would be:

class WebGLFramebufferBase {
public:
         void clearIfNeeded(...) { }
};

However, I don't think this is needed for WebXR.

}

private:
WebGLRenderingContextBase& m_context;
Copy link
Contributor

Choose a reason for hiding this comment

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

could store the m_context as ptr instead of having m_enabled

}

private:
WebGLRenderingContextBase& m_context;
Copy link
Contributor

Choose a reason for hiding this comment

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

could store the context as ptr instead of having m_enabled

@djg djg added the unsafe-merge-queue Applied to send a pull request to merge-queue, but skip building and testing label Feb 6, 2024
https://bugs.webkit.org/show_bug.cgi?id=268042
rdar://117090654

Reviewed by Kimmo Kinnunen.

WebGL's default framebuffer has an implicit clear behavior. The WebXR
specification requires the same behavior for its WebXROpaqueFramebuffer,
therefore WebXR must ensure that it's buffers are always cleared for each rAF of
the session.

Currently we assume content does not do redundant initial clear, as the spec
says the buffer always starts cleared.

* Source/WebCore/Modules/webxr/WebXROpaqueFramebuffer.cpp:
(WebCore::WebXROpaqueFramebuffer::startFrame):
(WebCore::WebXROpaqueFramebuffer::setupFramebuffer):
* Source/WebCore/html/canvas/WebGLRenderingContextBase.h:
* Source/WebCore/html/canvas/WebGLUtilities.h:
(WebCore::ScopedClearColorAndMask::ScopedClearColorAndMask):
(WebCore::ScopedClearColorAndMask::~ScopedClearColorAndMask):
(WebCore::ScopedClearDepthAndMask::ScopedClearDepthAndMask):
(WebCore::ScopedClearDepthAndMask::~ScopedClearDepthAndMask):
(WebCore::ScopedClearStencilAndMask::ScopedClearStencilAndMask):
(WebCore::ScopedClearStencilAndMask::~ScopedClearStencilAndMask):

Canonical link: https://commits.webkit.org/274121@main
@webkit-commit-queue
Copy link
Collaborator

Committed 274121@main (153d8bb): https://commits.webkit.org/274121@main

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

@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 Feb 6, 2024
@djg djg deleted the eng/webxr/implicit-clear branch February 6, 2024 00:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
WebXR For bugs in WebXR
Projects
None yet
4 participants