Bug 214211

Summary: [WebGL2] vertexAttribI* and getFragDataLocation
Product: WebKit Reporter: James Darpinian <jdarpinian>
Component: WebGLAssignee: James Darpinian <jdarpinian>
Status: RESOLVED FIXED    
Severity: Normal CC: cdumez, changseok, dino, esprehn+autocc, ews-watchlist, graouts, gyuyoung.kim, justin_fan, kbr, kondapallykalyan, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 126404, 126937, 126938, 209480, 215036    
Attachments:
Description Flags
Patch
none
Patch
none
rebase
none
mark reviewed none

Description James Darpinian 2020-07-10 17:53:13 PDT
[WebGL2] vertexAttribI* and getFragDataLocation
Comment 1 James Darpinian 2020-07-10 17:53:47 PDT
Created attachment 404029 [details]
Patch
Comment 2 James Darpinian 2020-07-13 11:14:34 PDT
Created attachment 404159 [details]
Patch
Comment 3 Dean Jackson 2020-07-13 13:12:50 PDT
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?
Comment 4 Kenneth Russell 2020-07-13 14:38:31 PDT
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.
Comment 5 James Darpinian 2020-07-13 17:49:53 PDT
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.
Comment 6 EWS 2020-07-14 14:52:47 PDT
Tools/Scripts/svn-apply failed to apply attachment 404159 [details] to trunk.
Please resolve the conflicts and upload a new patch.
Comment 7 James Darpinian 2020-07-14 15:22:11 PDT
Created attachment 404291 [details]
rebase
Comment 8 James Darpinian 2020-07-14 15:35:17 PDT
Created attachment 404295 [details]
mark reviewed
Comment 9 Kenneth Russell 2020-07-14 15:37:23 PDT
Comment on attachment 404295 [details]
mark reviewed

Attempting cq+ as a WebKit committer.
Comment 10 EWS 2020-07-14 16:10:16 PDT
Committed r264381: <https://trac.webkit.org/changeset/264381>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 404295 [details].
Comment 11 Radar WebKit Bug Importer 2020-07-14 16:11:17 PDT
<rdar://problem/65570741>