Bug 218976 – GraphicsContextGL should have robust multivalue getters

• Status: RESOLVED
• Resolution: FIXED
• Component: WebGL

[Kimmo Kinnunen]

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.

[Kimmo Kinnunen]

Created attachment 414219 [details]
Patch

[Kimmo Kinnunen]

Created attachment 414220 [details]
Patch

[Kimmo Kinnunen]

Created attachment 414223 [details]
Patch

[Kimmo Kinnunen]

Created attachment 414226 [details]
Patch

[Kimmo Kinnunen]

Created attachment 414227 [details]
Patch

[Kimmo Kinnunen]

Created attachment 414228 [details]
Patch

[Kimmo Kinnunen]

Created attachment 414229 [details]
Patch

[Simon Fraser (smfr)]

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

[Kimmo Kinnunen]

(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.

[Kimmo Kinnunen]

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..

[Simon Fraser (smfr)]

(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;

[Kimmo Kinnunen]

Created attachment 414269 [details]
Patch

[Kenneth Russell]

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 ?

[Kimmo Kinnunen]

Created attachment 414307 [details]
Patch

[Kimmo Kinnunen]

(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.

[Kimmo Kinnunen]

Created attachment 414310 [details]
Patch

[Kimmo Kinnunen]

Created attachment 414311 [details]
Patch

[Kimmo Kinnunen]

Created attachment 414313 [details]
Patch

[Simon Fraser (smfr)]

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?

[Darin Adler]

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?

[Kenneth Russell]

(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.

[Kimmo Kinnunen]

Created attachment 414427 [details]
Patch

[Kimmo Kinnunen]

Created attachment 414428 [details]
Patch

[EWS]

Committed r269952: <https://trac.webkit.org/changeset/269952>

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

[Radar WebKit Bug Importer]

<rdar://problem/71533455>