Skip to content

Conversation

kkinnunen-apple
Copy link
Contributor

@kkinnunen-apple kkinnunen-apple commented Mar 14, 2023

23d40f4

WebGLRenderingContextBase texImageSource HTMLCanvasElement implementation unable to share code with OffscreenCanvas
https://bugs.webkit.org/show_bug.cgi?id=253885
rdar://106699408

Reviewed by Matt Woodrow.

The WebGLRenderingContextBase::texImageSourceHelper() would invoke the payload code
directly inside embedded the std::variant visitor. This would make the function overly complex
compared to normally structured member functions. It would also make it harder than needed to
invoke a TexImageSource case which would match to same implementation. This would be needed
in order to match HTMLCanvasElement and OffscreenCanvas implementations with CanvasBase.

Simplify by having a simple generic visitor call overloaded member
function, and let the overloading rules select the correct method based
on the `source` type. This pattern is used in
CanvasRenderingContext2DBase. Removes an rendundant hunk from said
implementation as an example. The hunk is redundant as the generic
implementation is exactly the same as the removed specific one.

The change of structure leaks into other functions:
- validateTexImageSubRectangle was redundantly templated and in the
  header file.
  Instead remove T parameter, add the source size parameter and move to
  .cpp file.
- TexImageFunctionID defines the function name and function type. The
  three variables are redundant: id, name, type.
  Instead of resolving the function name and type in the
  `texImageSourceHelper`
  and passing all three variables down tho the overloads, pass just the `functionID`.
  Propagate the change to all functions that use `functionName`, `functionID` and/or `functionType`:
  pass just the `functionID`. Later patches may remove further use of leaf `functionName` / `functionType`.
- WebCodecsVideoFrame variant of texImageSource was missing texture parameter validation.
  Add the validation. This is an example of case where the code syntax simplification is beneficial, as
  the mistakes are harder to make and easier to spot.

This is needed in order to add missing OffscreenCanvas as texImageSource
in future patches.

* Source/WebCore/html/canvas/CanvasRenderingContext2DBase.cpp:
(WebCore::CanvasRenderingContext2DBase::drawImage):
* Source/WebCore/html/canvas/WebGL2RenderingContext.cpp:
(WebCore::WebGL2RenderingContext::validateTexImageBinding):
(WebCore::WebGL2RenderingContext::texImage2D):
(WebCore::WebGL2RenderingContext::texImage3D):
(WebCore::WebGL2RenderingContext::texSubImage2D):
(WebCore::WebGL2RenderingContext::texSubImage3D):
* Source/WebCore/html/canvas/WebGL2RenderingContext.h:
* Source/WebCore/html/canvas/WebGLRenderingContextBase.cpp:
(WebCore::WebGLRenderingContextBase::safeGetImageSize):
(WebCore::WebGLRenderingContextBase::getImageDataSize):
(WebCore::WebGLRenderingContextBase::texImageSourceHelper):
(WebCore::WebGLRenderingContextBase::texImageSource):
(WebCore::WebGLRenderingContextBase::texImageArrayBufferViewHelper):
(WebCore::WebGLRenderingContextBase::texImageImpl):
(WebCore::WebGLRenderingContextBase::texImageFunctionName):
(WebCore::WebGLRenderingContextBase::texImageFunctionType):
(WebCore::WebGLRenderingContextBase::validateTexImageSubRectangle):
(WebCore::WebGLRenderingContextBase::validateTexFunc):
(WebCore::WebGLRenderingContextBase::validateTexFuncParameters):
(WebCore::WebGLRenderingContextBase::validateTexImageSourceFormatAndType):
(WebCore::WebGLRenderingContextBase::videoFrameToImage):
(WebCore::WebGLRenderingContextBase::validateTexImageBinding):
(WebCore::WebGLRenderingContextBase::validateHTMLImageElement):
(WebCore::WebGLRenderingContextBase::validateHTMLCanvasElement):
(WebCore::WebGLRenderingContextBase::validateHTMLVideoElement):
(WebCore::WebGLRenderingContextBase::validateImageBitmap):
(WebCore::WebGLRenderingContextBase::getTexImageSourceSize): Deleted.
(WebCore::WebGLRenderingContextBase::getTexImageFunctionName): Deleted.
* Source/WebCore/html/canvas/WebGLRenderingContextBase.h:
(WebCore::WebGLRenderingContextBase::getTextureSourceSize): Deleted.
(WebCore::WebGLRenderingContextBase::validateTexImageSubRectangle): Deleted.

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

8608133

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
✅ 🧪 api-ios ✅ 🧪 mac-wk1 ✅ 🧪 gtk-wk2
✅ 🛠 tv ✅ 🧪 mac-wk2 🧪 api-gtk
✅ 🛠 tv-sim 🧪 mac-AS-debug-wk2
✅ 🛠 watch ✅ 🧪 mac-wk2-stress
✅ 🛠 🧪 merge ✅ 🛠 watch-sim

@kkinnunen-apple kkinnunen-apple self-assigned this Mar 14, 2023
@kkinnunen-apple kkinnunen-apple added the WebGL Bugs in WebKit’s implementation of the WebGL standard. label Mar 14, 2023
@kkinnunen-apple kkinnunen-apple requested review from mattwoodrow and removed request for rniwa and cdumez March 14, 2023 14:03
@kkinnunen-apple kkinnunen-apple force-pushed the webgl-teximagesourcehelper-simplify-1 branch from ff83ac5 to 8608133 Compare March 14, 2023 14:30
@kkinnunen-apple kkinnunen-apple added the merge-queue Applied to send a pull request to merge-queue label Mar 15, 2023
…tion unable to share code with OffscreenCanvas

https://bugs.webkit.org/show_bug.cgi?id=253885
rdar://106699408

Reviewed by Matt Woodrow.

The WebGLRenderingContextBase::texImageSourceHelper() would invoke the payload code
directly inside embedded the std::variant visitor. This would make the function overly complex
compared to normally structured member functions. It would also make it harder than needed to
invoke a TexImageSource case which would match to same implementation. This would be needed
in order to match HTMLCanvasElement and OffscreenCanvas implementations with CanvasBase.

Simplify by having a simple generic visitor call overloaded member
function, and let the overloading rules select the correct method based
on the `source` type. This pattern is used in
CanvasRenderingContext2DBase. Removes an rendundant hunk from said
implementation as an example. The hunk is redundant as the generic
implementation is exactly the same as the removed specific one.

The change of structure leaks into other functions:
- validateTexImageSubRectangle was redundantly templated and in the
  header file.
  Instead remove T parameter, add the source size parameter and move to
  .cpp file.
- TexImageFunctionID defines the function name and function type. The
  three variables are redundant: id, name, type.
  Instead of resolving the function name and type in the
  `texImageSourceHelper`
  and passing all three variables down tho the overloads, pass just the `functionID`.
  Propagate the change to all functions that use `functionName`, `functionID` and/or `functionType`:
  pass just the `functionID`. Later patches may remove further use of leaf `functionName` / `functionType`.
- WebCodecsVideoFrame variant of texImageSource was missing texture parameter validation.
  Add the validation. This is an example of case where the code syntax simplification is beneficial, as
  the mistakes are harder to make and easier to spot.

This is needed in order to add missing OffscreenCanvas as texImageSource
in future patches.

* Source/WebCore/html/canvas/CanvasRenderingContext2DBase.cpp:
(WebCore::CanvasRenderingContext2DBase::drawImage):
* Source/WebCore/html/canvas/WebGL2RenderingContext.cpp:
(WebCore::WebGL2RenderingContext::validateTexImageBinding):
(WebCore::WebGL2RenderingContext::texImage2D):
(WebCore::WebGL2RenderingContext::texImage3D):
(WebCore::WebGL2RenderingContext::texSubImage2D):
(WebCore::WebGL2RenderingContext::texSubImage3D):
* Source/WebCore/html/canvas/WebGL2RenderingContext.h:
* Source/WebCore/html/canvas/WebGLRenderingContextBase.cpp:
(WebCore::WebGLRenderingContextBase::safeGetImageSize):
(WebCore::WebGLRenderingContextBase::getImageDataSize):
(WebCore::WebGLRenderingContextBase::texImageSourceHelper):
(WebCore::WebGLRenderingContextBase::texImageSource):
(WebCore::WebGLRenderingContextBase::texImageArrayBufferViewHelper):
(WebCore::WebGLRenderingContextBase::texImageImpl):
(WebCore::WebGLRenderingContextBase::texImageFunctionName):
(WebCore::WebGLRenderingContextBase::texImageFunctionType):
(WebCore::WebGLRenderingContextBase::validateTexImageSubRectangle):
(WebCore::WebGLRenderingContextBase::validateTexFunc):
(WebCore::WebGLRenderingContextBase::validateTexFuncParameters):
(WebCore::WebGLRenderingContextBase::validateTexImageSourceFormatAndType):
(WebCore::WebGLRenderingContextBase::videoFrameToImage):
(WebCore::WebGLRenderingContextBase::validateTexImageBinding):
(WebCore::WebGLRenderingContextBase::validateHTMLImageElement):
(WebCore::WebGLRenderingContextBase::validateHTMLCanvasElement):
(WebCore::WebGLRenderingContextBase::validateHTMLVideoElement):
(WebCore::WebGLRenderingContextBase::validateImageBitmap):
(WebCore::WebGLRenderingContextBase::getTexImageSourceSize): Deleted.
(WebCore::WebGLRenderingContextBase::getTexImageFunctionName): Deleted.
* Source/WebCore/html/canvas/WebGLRenderingContextBase.h:
(WebCore::WebGLRenderingContextBase::getTextureSourceSize): Deleted.
(WebCore::WebGLRenderingContextBase::validateTexImageSubRectangle): Deleted.

Canonical link: https://commits.webkit.org/261678@main
@webkit-commit-queue webkit-commit-queue force-pushed the webgl-teximagesourcehelper-simplify-1 branch from 8608133 to 23d40f4 Compare March 15, 2023 08:35
@webkit-commit-queue
Copy link
Collaborator

Committed 261678@main (23d40f4): https://commits.webkit.org/261678@main

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

@webkit-commit-queue webkit-commit-queue merged commit 23d40f4 into WebKit:main Mar 15, 2023
@webkit-commit-queue webkit-commit-queue removed the merge-queue Applied to send a pull request to merge-queue label Mar 15, 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
Development

Successfully merging this pull request may close these issues.

4 participants