Bug 206782 - [WebGL2] Implement sub-source texImage2D and texSubImage2D
Summary: [WebGL2] Implement sub-source texImage2D and texSubImage2D
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Justin Fan
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-01-24 16:49 PST by Justin Fan
Modified: 2020-01-29 11:25 PST (History)
11 users (show)

See Also:

Attachments
Patch (72.46 KB, patch)
2020-01-24 17:09 PST, Justin Fan 		no flags Details | Formatted Diff | Diff
Patch for landing (72.52 KB, patch)
2020-01-28 14:21 PST, Justin Fan 		no flags Details | Formatted Diff | Diff
Show Obsolete (1) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Justin Fan 2020-01-24 16:49:53 PST 
[WebGL2] Implement sub-source texImage2D and texSubImage2D
Comment 1 Justin Fan 2020-01-24 16:50:19 PST 
<rdar://problem/58886527>
Comment 2 Justin Fan 2020-01-24 17:09:57 PST 
Created attachment 388746 [details]
Patch
Comment 3 Justin Fan 2020-01-24 17:12:54 PST 
<rdar://problem/58886527>
Comment 4 Dean Jackson 2020-01-28 09:44:53 PST 
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?
Comment 5 Justin Fan 2020-01-28 14:13:15 PST 
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.
Comment 6 Justin Fan 2020-01-28 14:21:36 PST 
Created attachment 389061 [details]
Patch for landing
Comment 7 WebKit Commit Bot 2020-01-28 14:51:02 PST 
Comment on attachment 389061 [details]
Patch for landing

Clearing flags on attachment: 389061

Committed r255316: <https://trac.webkit.org/changeset/255316>
Comment 8 WebKit Commit Bot 2020-01-28 14:51:04 PST 
All reviewed patches have been landed.  Closing bug.