Bug 218976 - GraphicsContextGL should have robust multivalue getters
Summary: GraphicsContextGL should have robust multivalue getters
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebGL (show other bugs)
Version: WebKit Local Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Kimmo Kinnunen
URL:
Keywords: InRadar
Depends on:
Blocks: webglgpup
  Show dependency treegraph
 
Reported: 2020-11-16 05:11 PST by Kimmo Kinnunen
Modified: 2020-11-18 04:26 PST (History)
15 users (show)

See Also:


Attachments
Patch (128.64 KB, patch)
2020-11-16 05:22 PST, Kimmo Kinnunen
no flags Details | Formatted Diff | Diff
Patch (128.79 KB, patch)
2020-11-16 05:25 PST, Kimmo Kinnunen
no flags Details | Formatted Diff | Diff
Patch (114.05 KB, patch)
2020-11-16 05:31 PST, Kimmo Kinnunen
no flags Details | Formatted Diff | Diff
Patch (115.60 KB, patch)
2020-11-16 05:38 PST, Kimmo Kinnunen
no flags Details | Formatted Diff | Diff
Patch (117.39 KB, patch)
2020-11-16 05:48 PST, Kimmo Kinnunen
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (116.41 KB, patch)
2020-11-16 06:06 PST, Kimmo Kinnunen
no flags Details | Formatted Diff | Diff
Patch (108.85 KB, patch)
2020-11-16 06:09 PST, Kimmo Kinnunen
no flags Details | Formatted Diff | Diff
Patch (113.49 KB, patch)
2020-11-16 12:34 PST, Kimmo Kinnunen
no flags Details | Formatted Diff | Diff
Patch (113.95 KB, patch)
2020-11-16 22:02 PST, Kimmo Kinnunen
no flags Details | Formatted Diff | Diff
Patch (115.06 KB, patch)
2020-11-16 23:12 PST, Kimmo Kinnunen
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (118.17 KB, patch)
2020-11-16 23:25 PST, Kimmo Kinnunen
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (118.04 KB, patch)
2020-11-16 23:37 PST, Kimmo Kinnunen
no flags Details | Formatted Diff | Diff
Patch (118.61 KB, patch)
2020-11-18 00:58 PST, Kimmo Kinnunen
no flags Details | Formatted Diff | Diff
Patch (118.61 KB, patch)
2020-11-18 00:59 PST, Kimmo Kinnunen
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kimmo Kinnunen 2020-11-16 05:11:22 PST
GraphicsContextGL should have robust multivalue getters, otherwise:
1) The GPUP is harder to implement
2) The code contains uninitialised variables and miscalculated receive buffer sizes

The getters such as getUniformiv should contain the info about the amount of data in the receive buffer.
Currently ANGLE_robust_client_memory has this form of API. We should just use this pattern in the main GraphicsContextGL.
Comment 1 Kimmo Kinnunen 2020-11-16 05:22:29 PST
Created attachment 414219 [details]
Patch
Comment 2 Kimmo Kinnunen 2020-11-16 05:25:05 PST
Created attachment 414220 [details]
Patch
Comment 3 Kimmo Kinnunen 2020-11-16 05:31:08 PST
Created attachment 414223 [details]
Patch
Comment 4 Kimmo Kinnunen 2020-11-16 05:38:12 PST
Created attachment 414226 [details]
Patch
Comment 5 Kimmo Kinnunen 2020-11-16 05:48:04 PST
Created attachment 414227 [details]
Patch
Comment 6 Kimmo Kinnunen 2020-11-16 06:06:08 PST
Created attachment 414228 [details]
Patch
Comment 7 Kimmo Kinnunen 2020-11-16 06:09:03 PST
Created attachment 414229 [details]
Patch
Comment 8 Simon Fraser (smfr) 2020-11-16 08:24:46 PST
Comment on attachment 414229 [details]
Patch

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

> Source/WebCore/ChangeLog:10
> +        the value is a normal the return value. This reduces the risk
> +        of having uninitialized receive location.

Some words missing here.

> Source/WebCore/ChangeLog:17
> +        Add GCGLSpan type that is used to communicate pair: ptr, number of

"Span" is a really odd name for this. Does no existing WTF type work here?

> Source/WebCore/platform/graphics/GraphicsContextGL.h:994
> +    virtual void getFloatv(GCGLenum pname, GCGLSpan<GCGLfloat> value) = 0;
> +    virtual void getIntegerv(GCGLenum pname, GCGLSpan<GCGLint> value) = 0;

Can we rely on the return-value optimization to just return GCGLSpan<>?

> Source/WebCore/platform/graphics/GraphicsContextGL.h:998
> +    virtual void getBooleanv(GCGLenum pname, GCGLSpan<GCGLboolean> value) = 0;

Ditto
Comment 9 Kimmo Kinnunen 2020-11-16 11:40:00 PST
(In reply to Simon Fraser (smfr) from comment #8)
> > Source/WebCore/ChangeLog:17
> > +        Add GCGLSpan type that is used to communicate pair: ptr, number of
> 
> "Span" is a really odd name for this. Does no existing WTF type work here?

I've always interpreted it so that a 'span' is a standard name to a view into array memory. Standard as in std::span. As in, if you're going to implement the concept, it's going to be called "span". I couldn't find a corresponding type..

> 
> > Source/WebCore/platform/graphics/GraphicsContextGL.h:994
> > +    virtual void getFloatv(GCGLenum pname, GCGLSpan<GCGLfloat> value) = 0;
> > +    virtual void getIntegerv(GCGLenum pname, GCGLSpan<GCGLint> value) = 0;
> 
> Can we rely on the return-value optimization to just return GCGLSpan<>?

Span doesn't own anything. It's a view into something owned by somebody.
With this and previous comment, do you mean following:

GCGLuint value[4] = {0};
m_context->getUniformuiv(program.object(), location, value);

->

WTF::Vector<GCGLuint> value = m_context->getUniformuiv(program.object(), location, 4);

It's a bit redundant, but if you think that's the way to go, I can do it that way.
Comment 10 Kimmo Kinnunen 2020-11-16 11:51:13 PST
Also, the related but not strictly related to this patch:

Output calls are going to be of form 
  uniform4fv(..., GCGLSpan<GCGLfloat, 4> value)

Having those as WTF::Vectors is a bit redundant
Having those as std::array<GCGLfloat, 4> is copying a bit

Then there are call like
  bufferData(..., GCGLSpan<GCGLvoid>) 
Having this as WTF::Vector might be a quite a bit copying..
Comment 11 Simon Fraser (smfr) 2020-11-16 11:56:11 PST
(In reply to Kimmo Kinnunen from comment #9)
> (In reply to Simon Fraser (smfr) from comment #8)
> > > Source/WebCore/ChangeLog:17
> > > +        Add GCGLSpan type that is used to communicate pair: ptr, number of
> > 
> > "Span" is a really odd name for this. Does no existing WTF type work here?
> 
> I've always interpreted it so that a 'span' is a standard name to a view
> into array memory. Standard as in std::span. As in, if you're going to
> implement the concept, it's going to be called "span". I couldn't find a
> corresponding type..

OK, I wasn't familiar with std::span.

> > > Source/WebCore/platform/graphics/GraphicsContextGL.h:994
> > > +    virtual void getFloatv(GCGLenum pname, GCGLSpan<GCGLfloat> value) = 0;
> > > +    virtual void getIntegerv(GCGLenum pname, GCGLSpan<GCGLint> value) = 0;
> > 
> > Can we rely on the return-value optimization to just return GCGLSpan<>?
> 
> Span doesn't own anything. It's a view into something owned by somebody.
> With this and previous comment, do you mean following:
> 
> GCGLuint value[4] = {0};
> m_context->getUniformuiv(program.object(), location, value);

I meant changing the signatures to:

virtual GCGLSpan<GCGLfloat> getFloatv(GCGLenum pname) = 0;
virtual GCGLSpan<GCGLint> getIntegerv(GCGLenum pname) = 0;
Comment 12 Kimmo Kinnunen 2020-11-16 12:34:44 PST
Created attachment 414269 [details]
Patch
Comment 13 Kenneth Russell 2020-11-16 15:22:27 PST
Comment on attachment 414269 [details]
Patch

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

The failures of gl-object-get-calls and some of the other conformance tests indicate something's going wrong with the multi-value getters - could you investigate those and comment here when this patch is fixed?

> Source/WebCore/html/canvas/WebGL2RenderingContext.cpp:2521
> +        m_context->getActiveUniformBlockiv(program.object(), uniformBlockIndex, pname, params);

As discussed on the Slack channel - good catch on this fix. Did this fix any WebGL conformance tests? If not then could you please file an issue about expanding the conformance tests around UNIFORM_BLOCK_ACTIVE_UNIFORM_INDICES on https://github.com/KhronosGroup/WebGL ?
Comment 14 Kimmo Kinnunen 2020-11-16 22:02:43 PST
Created attachment 414307 [details]
Patch
Comment 15 Kimmo Kinnunen 2020-11-16 22:45:02 PST
(In reply to Kenneth Russell from comment #13)
> > Source/WebCore/html/canvas/WebGL2RenderingContext.cpp:2521
> > +        m_context->getActiveUniformBlockiv(program.object(), uniformBlockIndex, pname, params);
> 
> As discussed on the Slack channel - good catch on this fix. Did this fix any
> WebGL conformance tests? If not then could you please file an issue about
> expanding the conformance tests around UNIFORM_BLOCK_ACTIVE_UNIFORM_INDICES
> on https://github.com/KhronosGroup/WebGL ?

I think it's just a cosmetic fix -- there was no real bug.
So the case was in the spirit of:
 We have one int slot.
 We want a uniform property that fills one in int slot.
 We give the int slot to ANGLE: "Here are int slots, write the property here, the amount of int slots is 4". (<-- bug here, we thought we say "one slot", but actually said "4 slots".).
 ANGLE checks that 4 is bigger than one and writes one int in one slot.

So the case was that we claimed our buffer is bigger than it was, but it does not matter since the value written to the buffer is the size of the buffer.

I'm not sure the bug would be testable from webgl level. 


There is inverse of this bug marked in the patch as FIXME:s : 
ANGLE validates bufSize of Uniform*vRobustANGLE with the logic of bufSize of nUniformEXT. That shouldn't be testable from webgl level either. I'll submit a patch for that if nobody does that before I get around doing it.
Comment 16 Kimmo Kinnunen 2020-11-16 23:12:28 PST
Created attachment 414310 [details]
Patch
Comment 17 Kimmo Kinnunen 2020-11-16 23:25:10 PST
Created attachment 414311 [details]
Patch
Comment 18 Kimmo Kinnunen 2020-11-16 23:37:41 PST
Created attachment 414313 [details]
Patch
Comment 19 Simon Fraser (smfr) 2020-11-17 08:47:36 PST
Comment on attachment 414313 [details]
Patch

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

> Source/WebCore/platform/graphics/GraphicsTypesGL.h:72
> +    T& operator*() {  ASSERT(data); return *data; }

Extra space. Should these be release asserts?

> Source/WebCore/platform/graphics/GraphicsTypesGL.h:92
> +    T& operator[](size_t i) { ASSERT(data); ASSERT(i < bufSize); return data[i]; }
> +    T& operator*() { ASSERT(data); return *data; }

Release asserts?

> Source/WebCore/platform/graphics/angle/GraphicsContextGLANGLE.cpp:1973
> +    // FIXME: Bug in ANGLE bufSize validation for uniforms.

Reference a bug number?

> Source/WebCore/platform/graphics/angle/GraphicsContextGLANGLE.cpp:1982
> +    // FIXME: Bug in ANGLE bufSize validation for uniforms.

Reference a bug number?

> Source/WebCore/platform/graphics/angle/GraphicsContextGLANGLE.cpp:1991
> +    // FIXME: Bug in ANGLE bufSize validation for uniforms.

Reference a bug number?
Comment 20 Darin Adler 2020-11-17 10:30:27 PST
Comment on attachment 414313 [details]
Patch

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

>> Source/WebCore/platform/graphics/GraphicsTypesGL.h:72
>> +    T& operator*() {  ASSERT(data); return *data; }
> 
> Extra space. Should these be release asserts?

Likely valuable to have a buffer overrun assertion be a release assertion. Perhaps less helpful to have the null check be a release assertion?
Comment 21 Kenneth Russell 2020-11-17 14:06:46 PST
(In reply to Kimmo Kinnunen from comment #15)
> (In reply to Kenneth Russell from comment #13)
> > > Source/WebCore/html/canvas/WebGL2RenderingContext.cpp:2521
> > > +        m_context->getActiveUniformBlockiv(program.object(), uniformBlockIndex, pname, params);
> > 
> > As discussed on the Slack channel - good catch on this fix. Did this fix any
> > WebGL conformance tests? If not then could you please file an issue about
> > expanding the conformance tests around UNIFORM_BLOCK_ACTIVE_UNIFORM_INDICES
> > on https://github.com/KhronosGroup/WebGL ?
> 
> I think it's just a cosmetic fix -- there was no real bug.
> So the case was in the spirit of:
>  We have one int slot.
>  We want a uniform property that fills one in int slot.
>  We give the int slot to ANGLE: "Here are int slots, write the property
> here, the amount of int slots is 4". (<-- bug here, we thought we say "one
> slot", but actually said "4 slots".).
>  ANGLE checks that 4 is bigger than one and writes one int in one slot.
> 
> So the case was that we claimed our buffer is bigger than it was, but it
> does not matter since the value written to the buffer is the size of the
> buffer.
> 
> I'm not sure the bug would be testable from webgl level. 
> 
> 
> There is inverse of this bug marked in the patch as FIXME:s : 
> ANGLE validates bufSize of Uniform*vRobustANGLE with the logic of bufSize of
> nUniformEXT. That shouldn't be testable from webgl level either. I'll submit
> a patch for that if nobody does that before I get around doing it.

Thanks for the clarification - I see. Given that, don't worry about new tests.
Comment 22 Kimmo Kinnunen 2020-11-18 00:58:24 PST
Created attachment 414427 [details]
Patch
Comment 23 Kimmo Kinnunen 2020-11-18 00:59:54 PST
Created attachment 414428 [details]
Patch
Comment 24 EWS 2020-11-18 04:26:00 PST
Committed r269952: <https://trac.webkit.org/changeset/269952>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 414428 [details].
Comment 25 Radar WebKit Bug Importer 2020-11-18 04:26:22 PST
<rdar://problem/71533455>