Bug 219139

Summary: Support WEBGL_multi_draw extension
Product: WebKit Reporter: Kenneth Russell <kbr>
Component: WebGLAssignee: Kenneth Russell <kbr>
Status: RESOLVED FIXED    
Severity: Normal CC: annulen, cdumez, changseok, dino, esprehn+autocc, ews-watchlist, graouts, gyuyoung.kim, hi, jdarpinian, kkinnunen, kondapallykalyan, kpiddington, msokalski, ryuan.choi, sergio, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 220753, 219140    
Attachments:
Description Flags
Patch
none
Patch none

Description Kenneth Russell 2020-11-18 18:40:52 PST
https://www.khronos.org/registry/webgl/extensions/WEBGL_multi_draw/ has been moved to Community Approved status. It's shipping in Chromium and well supported by ANGLE.
Comment 1 Radar WebKit Bug Importer 2020-11-25 21:34:21 PST
<rdar://problem/71742593>
Comment 2 Kenneth Russell 2021-01-19 17:27:41 PST
Created attachment 417930 [details]
Patch
Comment 3 Kimmo Kinnunen 2021-01-20 11:08:41 PST
Comment on attachment 417930 [details]
Patch

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

looks consistent

> Source/WebCore/html/canvas/WebGLMultiDraw.cpp:136
> +    GCGLsizei drawcount)

So maybe the line breaking loses its meaning when the below code is anyway relying on user handling long lines? (The arg list here and in the header seems a bit unorthodox?)

> Source/WebCore/platform/graphics/ExtensionsGL.h:353
> +    // GL_ANGLE_multi_draw

I've been trying to move stuff out of the ExtensionsGL. If it fits your structure, these could also perhaps be of form GraphicsContextGL::multiDrawArrays(), etc.
Comment 4 Dean Jackson 2021-01-20 11:55:36 PST
Comment on attachment 417930 [details]
Patch

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

> Source/WebCore/html/canvas/WebGLMultiDraw.cpp:92
> +void WebGLMultiDraw::multiDrawArraysInstancedWEBGL(
> +    GCGLenum mode,
> +    Int32List firstsList,
> +    GCGLuint firstsOffset,
> +    Int32List countsList,
> +    GCGLuint countsOffset,
> +    Int32List instanceCountsList,
> +    GCGLuint instanceCountsOffset,
> +    GCGLsizei drawcount)
> +{

Nit: We do parameters on one line (same for elsewhere in the patch)

> Source/WebCore/html/canvas/WebGLMultiDraw.h:39
> +        using VariantType = Variant<RefPtr<TypedArray>, Vector<DataType>>;

Could we give this a better name?

>> Source/WebCore/platform/graphics/ExtensionsGL.h:353
>> +    // GL_ANGLE_multi_draw
> 
> I've been trying to move stuff out of the ExtensionsGL. If it fits your structure, these could also perhaps be of form GraphicsContextGL::multiDrawArrays(), etc.

+1
Comment 5 Kenneth Russell 2021-01-20 14:25:39 PST
Comment on attachment 417930 [details]
Patch

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

>> Source/WebCore/html/canvas/WebGLMultiDraw.cpp:92
>> +{
> 
> Nit: We do parameters on one line (same for elsewhere in the patch)

Sure thing. (I thought this would be easier to read.) Fixed.

>> Source/WebCore/html/canvas/WebGLMultiDraw.cpp:136
>> +    GCGLsizei drawcount)
> 
> So maybe the line breaking loses its meaning when the below code is anyway relying on user handling long lines? (The arg list here and in the header seems a bit unorthodox?)

Changed to follow WebKit's conventions.

>> Source/WebCore/html/canvas/WebGLMultiDraw.h:39
>> +        using VariantType = Variant<RefPtr<TypedArray>, Vector<DataType>>;
> 
> Could we give this a better name?

This was basically copied from WebGLRenderingContextBase.h (I couldn't think of a good way to share it between the headers) but will call it ListTypeOptions.

>>> Source/WebCore/platform/graphics/ExtensionsGL.h:353
>>> +    // GL_ANGLE_multi_draw
>> 
>> I've been trying to move stuff out of the ExtensionsGL. If it fits your structure, these could also perhaps be of form GraphicsContextGL::multiDrawArrays(), etc.
> 
> +1

Sure thing - done. Currently there still need to be calls to ExtensionsGL::supports and ExtensionsGL::ensureEnabled in WebGLMultiDraw.cpp. We could move those entry points over later.
Comment 6 Kenneth Russell 2021-01-20 14:34:11 PST
Created attachment 417997 [details]
Patch
Comment 7 Kenneth Russell 2021-01-20 17:24:46 PST
EWS is green. CQ'ing.
Comment 8 EWS 2021-01-20 17:42:52 PST
Committed r271679: <https://trac.webkit.org/changeset/271679>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 417997 [details].