Bug 214211 - [WebGL2] vertexAttribI* and getFragDataLocation
Summary: [WebGL2] vertexAttribI* and getFragDataLocation
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebGL (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: James Darpinian
URL:
Keywords: InRadar
Depends on:
Blocks: 126404 126937 126938 209480 215036
  Show dependency treegraph
 
Reported: 2020-07-10 17:53 PDT by James Darpinian
Modified: 2020-07-31 15:14 PDT (History)
11 users (show)

See Also:


Attachments
Patch (13.92 KB, patch)
2020-07-10 17:53 PDT, James Darpinian
no flags Details | Formatted Diff | Diff
Patch (141.24 KB, patch)
2020-07-13 11:14 PDT, James Darpinian
no flags Details | Formatted Diff | Diff
rebase (138.93 KB, patch)
2020-07-14 15:22 PDT, James Darpinian
no flags Details | Formatted Diff | Diff
mark reviewed (138.93 KB, patch)
2020-07-14 15:35 PDT, James Darpinian
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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>