Bug 219139 - Support WEBGL_multi_draw extension
Summary: Support WEBGL_multi_draw extension
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebGL (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Kenneth Russell
URL:
Keywords: InRadar
Depends on:
Blocks: 220753 219140
  Show dependency treegraph
 
Reported: 2020-11-18 18:40 PST by Kenneth Russell
Modified: 2021-01-20 17:42 PST (History)
17 users (show)

See Also:


Attachments
Patch (87.93 KB, patch)
2021-01-19 17:27 PST, Kenneth Russell
no flags Details | Formatted Diff | Diff
Patch (86.13 KB, patch)
2021-01-20 14:34 PST, Kenneth Russell
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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].