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.
Created attachment 414219 [details] Patch
Created attachment 414220 [details] Patch
Created attachment 414223 [details] Patch
Created attachment 414226 [details] Patch
Created attachment 414227 [details] Patch
Created attachment 414228 [details] Patch
Created attachment 414229 [details] Patch
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
(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.
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..
(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;
Created attachment 414269 [details] Patch
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 ?
Created attachment 414307 [details] Patch
(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.
Created attachment 414310 [details] Patch
Created attachment 414311 [details] Patch
Created attachment 414313 [details] Patch
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 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?
(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.
Created attachment 414427 [details] Patch
Created attachment 414428 [details] Patch
Committed r269952: <https://trac.webkit.org/changeset/269952> All reviewed patches have been landed. Closing bug and clearing flags on attachment 414428 [details].
<rdar://problem/71533455>