Bug 214211 – [WebGL2] vertexAttribI* and getFragDataLocation

• Component: WebGL
• Status: RESOLVED FIXED

[James Darpinian]

[WebGL2] vertexAttribI* and getFragDataLocation

[James Darpinian]

Created attachment 404029 [details]
Patch

[James Darpinian]

Created attachment 404159 [details]
Patch

[Dean Jackson]

Comment on attachment 404159 [details]
Patch

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

> Source/WebCore/html/canvas/WebGL2RenderingContext.cpp:1355
> +    m_context->vertexAttribI4i(index, x, y, z, w);

Do we need to confirm index <= m_maxVertexAttribs?

> Source/WebCore/html/canvas/WebGL2RenderingContext.cpp:1382
> +    if (index >= m_maxVertexAttribs) {
> +        synthesizeGLError(GraphicsContextGL::INVALID_VALUE, "vertexAttribI4iv", "index out of range");
> +        return;

... because you do here.

> Source/WebCore/html/canvas/WebGL2RenderingContext.cpp:1386
> +    memcpy(m_vertexAttribValue[index].iValue, data, sizeof(m_vertexAttribValue[index].iValue));

Do we need to memset(0) the destination values before copying? What if the destination value is larger than the incoming array?

[Kenneth Russell]

Comment on attachment 404159 [details]
Patch

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

Great work James - LGTM overall with Dean's review feedback addressed. One small point.

> Source/WebCore/html/canvas/WebGL2RenderingContext.cpp:1357
> +        m_vertexAttribValue[index].type = GraphicsContextGL::INT;

Would this be a good time to #ifdef out - or remove entirely - the m_vertexAttribValue cache? It was removed from Chromium's WebGL implementation a while ago in favor of calling glGetVertexAttribfv, glGetVertexAttribIiv or glGetVertexAttribIuiv directly. That would indirectly address some of the other review feedback.

[James Darpinian]

Comment on attachment 404159 [details]
Patch

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

>> Source/WebCore/html/canvas/WebGL2RenderingContext.cpp:1355
>> +    m_context->vertexAttribI4i(index, x, y, z, w);
> 
> Do we need to confirm index <= m_maxVertexAttribs?

We could, but we don't need to because ANGLE validates it again. It's only necessary when writing to the m_vertexAttribValue cache below.

>> Source/WebCore/html/canvas/WebGL2RenderingContext.cpp:1357
>> +        m_vertexAttribValue[index].type = GraphicsContextGL::INT;
> 
> Would this be a good time to #ifdef out - or remove entirely - the m_vertexAttribValue cache? It was removed from Chromium's WebGL implementation a while ago in favor of calling glGetVertexAttribfv, glGetVertexAttribIiv or glGetVertexAttribIuiv directly. That would indirectly address some of the other review feedback.

It's tied in with the stuff that emulates vertex attrib 0 and I didn't want to mess around with that in case it's still necessary somewhere.

>> Source/WebCore/html/canvas/WebGL2RenderingContext.cpp:1382
>> +        return;
> 
> ... because you do here.

We do need to check the index before writing to m_vertexAttribValue. This one generates the GL error directly, while the other one lets ANGLE do it. It all works as is, but if you want me to make them consistent let me know which way you prefer.

>> Source/WebCore/html/canvas/WebGL2RenderingContext.cpp:1386
>> +    memcpy(m_vertexAttribValue[index].iValue, data, sizeof(m_vertexAttribValue[index].iValue));
> 
> Do we need to memset(0) the destination values before copying? What if the destination value is larger than the incoming array?

The size of the incoming array is validated above.

[EWS]

Tools/Scripts/svn-apply failed to apply attachment 404159 [details] to trunk.
Please resolve the conflicts and upload a new patch.

[James Darpinian]

Created attachment 404291 [details]
rebase

[James Darpinian]

Created attachment 404295 [details]
mark reviewed

[Kenneth Russell]

Comment on attachment 404295 [details]
mark reviewed

Attempting cq+ as a WebKit committer.

[EWS]

Committed r264381: <https://trac.webkit.org/changeset/264381>

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

[Radar WebKit Bug Importer]

<rdar://problem/65570741>