Bug 206782 – [WebGL2] Implement sub-source texImage2D and texSubImage2D

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Justin Fan]

[WebGL2] Implement sub-source texImage2D and texSubImage2D

[Justin Fan]

<rdar://problem/58886527>

[Justin Fan]

Created attachment 388746 [details]
Patch

[Justin Fan]

<rdar://problem/58886527>

[Dean Jackson]

Comment on attachment 388746 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=388746&action=review

> Source/WebCore/html/canvas/WebGL2RenderingContext.cpp:158
> +        FOR_EACH_TYPED_ARRAY_TYPE_EXCLUDING_DATA_VIEW(FACTORY_CASE);

Too much indentation here.

> Source/WebCore/html/canvas/WebGL2RenderingContext.cpp:187
> -    Checked<GCGLuint, RecordOverflow> checkedByteLength = checkedlength * checkedElementSize;
> +    Checked<GCGLuint, RecordOverflow> checkedByteLength = length * checkedElementSize;

Does this still preserve the checked arithmetic? If so, then we can remove the checkedLength variable, and do the same for checkedSrcOffset above.

Or maybe use WTF::checkedProduct<GCGLuint> for both?

> Source/WebCore/html/canvas/WebGL2RenderingContext.cpp:711
> +        synthesizeGLError(GraphicsContextGL::INVALID_OPERATION, functionName, "Invalid element offset!");

Remove the !

> Source/WebCore/html/canvas/WebGL2RenderingContext.cpp:769
> +    auto slicedData = sliceTypedArrayBufferView("texSubImage2D", srcData, srcOffset);
> +
> +    WebGLRenderingContextBase::texSubImage2D(target, level, xoffset, yoffset, width, height, format, type, WTFMove(slicedData));

I was wondering if we should skip the call if slicedData is null, but I guess we do output a message from the slice function.

> LayoutTests/webgl/2.0.0/resources/webgl_test_files/conformance2/buffers/buffer-data-and-buffer-sub-data-sub-source.html:55
> -            testFailed("expected data at " + ii + ": " + data[iit] + ", got " + readbackView[ii]);
> +            testFailed("expected data at " + ii + ": " + data[ii] + ", got " + readbackView[ii]);

Has this been fixed in the WebGL2 test suite?

[Justin Fan]

Comment on attachment 388746 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=388746&action=review

>> Source/WebCore/html/canvas/WebGL2RenderingContext.cpp:187
>> +    Checked<GCGLuint, RecordOverflow> checkedByteLength = length * checkedElementSize;
> 
> Does this still preserve the checked arithmetic? If so, then we can remove the checkedLength variable, and do the same for checkedSrcOffset above.
> 
> Or maybe use WTF::checkedProduct<GCGLuint> for both?

I think it does (this section is a bit overkill with the Checked usage methinks) but I don't wanna change the logic here (not the point of this patch + hard to catch regressions due to our bot expectations right now) so I'm going to use checkedLength in the calculation like the original code.

>> Source/WebCore/html/canvas/WebGL2RenderingContext.cpp:711
>> +        synthesizeGLError(GraphicsContextGL::INVALID_OPERATION, functionName, "Invalid element offset!");
> 
> Remove the !

(y)

>> LayoutTests/webgl/2.0.0/resources/webgl_test_files/conformance2/buffers/buffer-data-and-buffer-sub-data-sub-source.html:55
>> +            testFailed("expected data at " + ii + ": " + data[ii] + ", got " + readbackView[ii]);
> 
> Has this been fixed in the WebGL2 test suite?

Appears to be, as of version 2.0.1 beta.

[Justin Fan]

Created attachment 389061 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 389061 [details]
Patch for landing

Clearing flags on attachment: 389061

Committed r255316: <https://trac.webkit.org/changeset/255316>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Jonathan Bedard]

Broke tests on the webgl bot:

https://results.webkit.org/?suite=layout-tests&test=webgl%2F2.0.0%2Fconformance2%2Ftextures%2Fmisc%2Ftex-image-and-sub-image-with-array-buffer-view-sub-source.html