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

Pixel pack and unpack state is cached but also asked from the underlying context #14337

Conversation

kkinnunen-apple
Copy link
Contributor

@kkinnunen-apple kkinnunen-apple commented May 25, 2023

cd21603

Pixel pack and unpack state is cached but also asked from the underlying context
https://bugs.webkit.org/show_bug.cgi?id=257314
rdar://109821711

Reviewed by Dan Glastonbury.

WebGLRenderingContextBase, WebGL2RenderingContext caches the
PACK_*, UNPACK_* state. When the WebGL client asks for the state,
the context will ask the underlying GraphicsContextGL for the state,
even though it is available as cached.

Return the cached state directly.
Use WebKit style for the functions and types related to this: use
full names and avoid using get prefix for accesors.

This is work towards being able to not send the state to the underlying
context. Remote variant should always readPixels with ignoring the client
packing in order to minimize transfers. This will be implemented in
subsequent patches. This means the underlying pack state and WebGL
pack state are going to be different, and thus asking the pack state
from the underlying context would need redundant mutation of the
state.

* Source/WebCore/html/canvas/WebGL2RenderingContext.cpp:
(WebCore::WebGL2RenderingContext::initializeNewContext):
(WebCore::WebGL2RenderingContext::getTextureSourceSubRectangle):
(WebCore::WebGL2RenderingContext::pixelStorei):
(WebCore::WebGL2RenderingContext::texImage3D):
(WebCore::WebGL2RenderingContext::texSubImage3D):
(WebCore::WebGL2RenderingContext::getParameter):
(WebCore::WebGL2RenderingContext::resetUnpackParameters): Deleted.
(WebCore::WebGL2RenderingContext::restoreUnpackParameters): Deleted.
(WebCore::WebGL2RenderingContext::getPackPixelStoreParams const): Deleted.
(WebCore::WebGL2RenderingContext::getUnpackPixelStoreParams const): Deleted.
* Source/WebCore/html/canvas/WebGL2RenderingContext.h:
* Source/WebCore/html/canvas/WebGLRenderingContextBase.cpp:
(WebCore::ScopedTightUnpackParameters::ScopedTightUnpackParameters):
(WebCore::ScopedTightUnpackParameters::~ScopedTightUnpackParameters):
(WebCore::ScopedTightUnpackParameters::set):
(WebCore::WebGLRenderingContextBase::initializeNewContext):
(WebCore::WebGLRenderingContextBase::getParameter):
(WebCore::WebGLRenderingContextBase::pixelStorei):
(WebCore::WebGLRenderingContextBase::texImageSource):
(WebCore::WebGLRenderingContextBase::texImageArrayBufferViewHelper):
(WebCore::WebGLRenderingContextBase::texImageImpl):
(WebCore::WebGLRenderingContextBase::validateTexFuncData):
(WebCore::WebGLRenderingContextBase::unpackPixelStoreParameters const):
(WebCore::ScopedUnpackParametersResetRestore::ScopedUnpackParametersResetRestore): Deleted.
(WebCore::ScopedUnpackParametersResetRestore::~ScopedUnpackParametersResetRestore): Deleted.
(WebCore::WebGLRenderingContextBase::resetUnpackParameters): Deleted.
(WebCore::WebGLRenderingContextBase::restoreUnpackParameters): Deleted.
(WebCore::WebGLRenderingContextBase::getPackPixelStoreParams const): Deleted.
(WebCore::WebGLRenderingContextBase::getUnpackPixelStoreParams const): Deleted.
* Source/WebCore/html/canvas/WebGLRenderingContextBase.h:
(WebCore::WebGLRenderingContextBase::pixelStorePackParameters const):
(WebCore::WebGLRenderingContextBase::unpackPixelStoreParameters const):
* Source/WebCore/platform/graphics/GraphicsContextGL.cpp:
(WebCore::GraphicsContextGL::computeImageSizeInBytes):
(WebCore::GraphicsContextGL::packImageData):
(WebCore::GraphicsContextGL::extractPixelBuffer):
(WebCore::GraphicsContextGL::extractTextureData):
* Source/WebCore/platform/graphics/GraphicsContextGL.h:

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

6f8698d

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

@kkinnunen-apple kkinnunen-apple self-assigned this May 25, 2023
@kkinnunen-apple kkinnunen-apple added the WebGL Bugs in WebKit’s implementation of the WebGL standard. label May 25, 2023
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label May 25, 2023
@kkinnunen-apple kkinnunen-apple removed the merging-blocked Applied to prevent a change from being merged label May 25, 2023
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label May 25, 2023
@kkinnunen-apple kkinnunen-apple force-pushed the webgl-pack-parameters-no-read-1 branch from d7763e7 to 1d898f3 Compare May 25, 2023 09:17
@kkinnunen-apple kkinnunen-apple requested review from djg and removed request for rniwa and cdumez May 25, 2023 13:33
@kkinnunen-apple kkinnunen-apple removed the merging-blocked Applied to prevent a change from being merged label May 25, 2023
Copy link
Contributor

@djg djg left a comment

Choose a reason for hiding this comment

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

:shipit:

@@ -3964,7 +3970,7 @@ void WebGLRenderingContextBase::texImageImpl(TexImageFunctionID functionID, GCGL
pixels = std::span<const uint8_t> { data.data(), data.size() };
}

ScopedUnpackParametersResetRestore temporaryResetUnpack(this, true);
ScopedTightUnpackParameters temporaryResetUnpack(*this, true);
Copy link
Contributor

Choose a reason for hiding this comment

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

Previously (Line 3668) this is written as ScopedTightUnpackParameters temporaryResetUnpack(*this). Remove the true or the default parameter to ScopedTightUnpackParameters and be explicit everywhere.

@kkinnunen-apple kkinnunen-apple force-pushed the webgl-pack-parameters-no-read-1 branch from 1d898f3 to 6f8698d Compare May 26, 2023 06:45
@kkinnunen-apple kkinnunen-apple added the merge-queue Applied to send a pull request to merge-queue label May 26, 2023
@webkit-early-warning-system
Copy link
Collaborator

Starting EWS tests for 6f8698d. Live statuses available at the PR page, #14337

…ing context

https://bugs.webkit.org/show_bug.cgi?id=257314
rdar://109821711

Reviewed by Dan Glastonbury.

WebGLRenderingContextBase, WebGL2RenderingContext caches the
PACK_*, UNPACK_* state. When the WebGL client asks for the state,
the context will ask the underlying GraphicsContextGL for the state,
even though it is available as cached.

Return the cached state directly.
Use WebKit style for the functions and types related to this: use
full names and avoid using get prefix for accesors.

This is work towards being able to not send the state to the underlying
context. Remote variant should always readPixels with ignoring the client
packing in order to minimize transfers. This will be implemented in
subsequent patches. This means the underlying pack state and WebGL
pack state are going to be different, and thus asking the pack state
from the underlying context would need redundant mutation of the
state.

* Source/WebCore/html/canvas/WebGL2RenderingContext.cpp:
(WebCore::WebGL2RenderingContext::initializeNewContext):
(WebCore::WebGL2RenderingContext::getTextureSourceSubRectangle):
(WebCore::WebGL2RenderingContext::pixelStorei):
(WebCore::WebGL2RenderingContext::texImage3D):
(WebCore::WebGL2RenderingContext::texSubImage3D):
(WebCore::WebGL2RenderingContext::getParameter):
(WebCore::WebGL2RenderingContext::resetUnpackParameters): Deleted.
(WebCore::WebGL2RenderingContext::restoreUnpackParameters): Deleted.
(WebCore::WebGL2RenderingContext::getPackPixelStoreParams const): Deleted.
(WebCore::WebGL2RenderingContext::getUnpackPixelStoreParams const): Deleted.
* Source/WebCore/html/canvas/WebGL2RenderingContext.h:
* Source/WebCore/html/canvas/WebGLRenderingContextBase.cpp:
(WebCore::ScopedTightUnpackParameters::ScopedTightUnpackParameters):
(WebCore::ScopedTightUnpackParameters::~ScopedTightUnpackParameters):
(WebCore::ScopedTightUnpackParameters::set):
(WebCore::WebGLRenderingContextBase::initializeNewContext):
(WebCore::WebGLRenderingContextBase::getParameter):
(WebCore::WebGLRenderingContextBase::pixelStorei):
(WebCore::WebGLRenderingContextBase::texImageSource):
(WebCore::WebGLRenderingContextBase::texImageArrayBufferViewHelper):
(WebCore::WebGLRenderingContextBase::texImageImpl):
(WebCore::WebGLRenderingContextBase::validateTexFuncData):
(WebCore::WebGLRenderingContextBase::unpackPixelStoreParameters const):
(WebCore::ScopedUnpackParametersResetRestore::ScopedUnpackParametersResetRestore): Deleted.
(WebCore::ScopedUnpackParametersResetRestore::~ScopedUnpackParametersResetRestore): Deleted.
(WebCore::WebGLRenderingContextBase::resetUnpackParameters): Deleted.
(WebCore::WebGLRenderingContextBase::restoreUnpackParameters): Deleted.
(WebCore::WebGLRenderingContextBase::getPackPixelStoreParams const): Deleted.
(WebCore::WebGLRenderingContextBase::getUnpackPixelStoreParams const): Deleted.
* Source/WebCore/html/canvas/WebGLRenderingContextBase.h:
(WebCore::WebGLRenderingContextBase::pixelStorePackParameters const):
(WebCore::WebGLRenderingContextBase::unpackPixelStoreParameters const):
* Source/WebCore/platform/graphics/GraphicsContextGL.cpp:
(WebCore::GraphicsContextGL::computeImageSizeInBytes):
(WebCore::GraphicsContextGL::packImageData):
(WebCore::GraphicsContextGL::extractPixelBuffer):
(WebCore::GraphicsContextGL::extractTextureData):
* Source/WebCore/platform/graphics/GraphicsContextGL.h:

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

Committed 264567@main (cd21603): https://commits.webkit.org/264567@main

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

@webkit-commit-queue webkit-commit-queue merged commit cd21603 into WebKit:main May 26, 2023
@webkit-commit-queue webkit-commit-queue removed the merge-queue Applied to send a pull request to merge-queue label May 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
WebGL Bugs in WebKit’s implementation of the WebGL standard.
Projects
None yet
5 participants