Bug 126940 – [WebGL2] Query Objects

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

[Dean Jackson]

Implement the WebGL 2 Query objects.

/* Query Objects */
  WebGLQuery? createQuery();
  void deleteQuery(WebGLQuery? query);
  [WebGLHandlesContextLoss] GLboolean isQuery(WebGLQuery? query);
  void beginQuery(GLenum target, WebGLQuery? query);
  void endQuery(GLenum target);
  /* TODO: document return type */
  any getQuery(GLenum target, GLenum pname);
  /* TODO: document return type */
  any getQueryParameter(WebGLQuery? query, GLenum pname);

[Dean Jackson]

<rdar://problem/15002395>

[David Kilzer (:ddkilzer)]

<rdar://problem/28228150>

[David Kilzer (:ddkilzer)]

<rdar://problem/15002395>

[James Darpinian]

Created attachment 404781 [details]
Patch

[Kenneth Russell]

Comment on attachment 404781 [details]
Patch

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

Looking good overall James - couple of comments.

> Source/WebCore/html/canvas/WebGL2RenderingContext.cpp:1879
> +    if (isContextLostOrPending() || !validateWebGLObject("deleteQuery", query))

Deleting a null query, like other objects, is supposed to be a no-op, so using validateWebGLObject here will generate an incorrect INVALID_VALUE. Is this covered by the WebGL conformance tests?

> Source/WebCore/html/canvas/WebGL2RenderingContext.cpp:1920
>      if (!addResult.isNewEntry) {

It looks like adding, without first testing whether an entry exists, means that if there is an attempt to incorrectly begin two queries of the same type one after the other, m_activeQueries will overwrite the first (actually active) one with the second one, breaking the tracking state. This can be tested by:
 - Attempt to start query A (like TRANSFORM_FEEDBACK_PRIMITIVES_WRITTEN)
 - Attempt to start query B of the same type (leads to INVALID_OPERATION)
 - Attempt to end query A

This will yield INVALID_OPERATION when it shouldn't.

Could you confirm whether this bug exists and if so make sure it's covered by the upstream WebGL conformance tests?

[Kenneth Russell]

Comment on attachment 404781 [details]
Patch

Attempting to remove duplicate comments.

[James Darpinian]

Created attachment 404890 [details]
review feedback

[James Darpinian]

Comment on attachment 404781 [details]
Patch

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

>> Source/WebCore/html/canvas/WebGL2RenderingContext.cpp:1879
>> +    if (isContextLostOrPending() || !validateWebGLObject("deleteQuery", query))
> 
> Deleting a null query, like other objects, is supposed to be a no-op, so using validateWebGLObject here will generate an incorrect INVALID_VALUE. Is this covered by the WebGL conformance tests?

Fixed. Looks like maybe there isn't a test for that, or at least it isn't running in the WebKit layout tests.

>> Source/WebCore/html/canvas/WebGL2RenderingContext.cpp:1920
>>      if (!addResult.isNewEntry) {
> 
> It looks like adding, without first testing whether an entry exists, means that if there is an attempt to incorrectly begin two queries of the same type one after the other, m_activeQueries will overwrite the first (actually active) one with the second one, breaking the tracking state. This can be tested by:
>  - Attempt to start query A (like TRANSFORM_FEEDBACK_PRIMITIVES_WRITTEN)
>  - Attempt to start query B of the same type (leads to INVALID_OPERATION)
>  - Attempt to end query A
> 
> This will yield INVALID_OPERATION when it shouldn't.
> 
> Could you confirm whether this bug exists and if so make sure it's covered by the upstream WebGL conformance tests?

There are tests for this. It turns out WTF::HashMap::add does not overwrite.

[Kenneth Russell]

Comment on attachment 404781 [details]
Patch

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

>>> Source/WebCore/html/canvas/WebGL2RenderingContext.cpp:1879
>>> +    if (isContextLostOrPending() || !validateWebGLObject("deleteQuery", query))
>> 
>> Deleting a null query, like other objects, is supposed to be a no-op, so using validateWebGLObject here will generate an incorrect INVALID_VALUE. Is this covered by the WebGL conformance tests?
> 
> Fixed. Looks like maybe there isn't a test for that, or at least it isn't running in the WebKit layout tests.

Possible to quickly add a test to sdk/tests/conformance2/query/query.html ? It looks like this isn't checked at least by that test.

>>> Source/WebCore/html/canvas/WebGL2RenderingContext.cpp:1920
>>>      if (!addResult.isNewEntry) {
>> 
>> It looks like adding, without first testing whether an entry exists, means that if there is an attempt to incorrectly begin two queries of the same type one after the other, m_activeQueries will overwrite the first (actually active) one with the second one, breaking the tracking state. This can be tested by:
>>  - Attempt to start query A (like TRANSFORM_FEEDBACK_PRIMITIVES_WRITTEN)
>>  - Attempt to start query B of the same type (leads to INVALID_OPERATION)
>>  - Attempt to end query A
>> 
>> This will yield INVALID_OPERATION when it shouldn't.
>> 
>> Could you confirm whether this bug exists and if so make sure it's covered by the upstream WebGL conformance tests?
> 
> There are tests for this. It turns out WTF::HashMap::add does not overwrite.

Ah right, I misread the documentation for HashMap::add. Thanks for checking.

[Kenneth Russell]

Comment on attachment 404890 [details]
review feedback

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

Looks good to me!

> Source/WebCore/html/canvas/WebGL2RenderingContext.cpp:1890
> +    if (isContextLostOrPending() || !query || !query->object() || !validateWebGLObject("isQuery", query))

This will generate INVALID_VALUE if it receives a query from a different context, where Chromium will just return false. Not sure what the better behavior is.

[James Darpinian]

Created attachment 404998 [details]
rebase

[James Darpinian]

Created attachment 405002 [details]
rebaseline layout test expando-loss-2.html

[James Darpinian]

Created attachment 405100 [details]
mark reviewed

[EWS]

Committed r264807: <https://trac.webkit.org/changeset/264807>

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