Bug 213906 - Fix transform feedback tests
Summary: Fix transform feedback tests
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 209513
  Show dependency treegraph
 
Reported: 2020-07-02 15:55 PDT by James Darpinian
Modified: 2020-07-24 14:42 PDT (History)
16 users (show)

See Also:


Attachments
Patch (24.70 KB, patch)
2020-07-02 16:04 PDT, James Darpinian
no flags Details | Formatted Diff | Diff
rebaseline layout tests and fix non-mac build (47.34 KB, patch)
2020-07-02 17:17 PDT, James Darpinian
no flags Details | Formatted Diff | Diff
address review feedback and fix non-webgl2 builds (49.02 KB, patch)
2020-07-06 12:21 PDT, James Darpinian
no flags Details | Formatted Diff | Diff
more review feedback (48.98 KB, patch)
2020-07-06 14:39 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-02 15:55:41 PDT
Fix transform feedback tests
Comment 1 James Darpinian 2020-07-02 16:04:18 PDT
Created attachment 403413 [details]
Patch
Comment 2 James Darpinian 2020-07-02 17:17:50 PDT
Created attachment 403419 [details]
rebaseline layout tests and fix non-mac build
Comment 3 Darin Adler 2020-07-02 17:34:20 PDT
Comment on attachment 403419 [details]
rebaseline layout tests and fix non-mac build

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

> Source/WebCore/html/canvas/WebGL2RenderingContext.cpp:1836
> +    WebGLTransformFeedback* toBeBound = feedbackObject ? feedbackObject : m_defaultTransformFeedback.get();

auto

> Source/WebCore/html/canvas/WebGL2RenderingContext.h:249
> +    GCGLuint getMaxTransformFeedbackSeparateAttribs();

const?

> Source/WebCore/html/canvas/WebGLProgram.h:77
> +    int getRequiredTransformFeedbackBufferCount()

const

> Source/WebCore/html/canvas/WebGLTransformFeedback.cpp:93
> +    for (auto boundBuffer : m_boundIndexedTransformFeedbackBuffers) {

This will do unnecessary reference count churn. Should just use auto&.

But also, Vector has a find function that I think we can use here.

> Source/WebCore/html/canvas/WebGLTransformFeedback.cpp:105
> +    for (unsigned i = 0; i < m_boundIndexedTransformFeedbackBuffers.size(); i++) {
> +        if (m_boundIndexedTransformFeedbackBuffers[i] == buffer)
> +            m_boundIndexedTransformFeedbackBuffers[i] = nullptr;
> +    }

range-based for loop would be better:

    for (auto& boundBuffer : m_boundIndexedTransformFeedbackBuffers) {
        if (boundBuffer == &buffer)
            boundBuffer = nullptr;
    }

> Source/WebCore/html/canvas/WebGLTransformFeedback.h:58
> +    WebGLProgram* getProgram() const { return m_program.get(); }

WebKit coding style says we don’t name these functions "get".

> Source/WebCore/html/canvas/WebGLTransformFeedback.h:59
> +    void setProgram(WebGLProgram*);

Can this take a reference instead of a pointer?

> Source/WebCore/html/canvas/WebGLTransformFeedback.h:62
> +    bool usesBuffer(WebGLBuffer*);
> +    void unbindBuffer(WebGLBuffer*);

Can these take a reference instead of a pointer?
Comment 4 Justin Fan 2020-07-06 09:45:32 PDT
Comment on attachment 403419 [details]
rebaseline layout tests and fix non-mac build

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

>> Source/WebCore/html/canvas/WebGLProgram.h:77
>> +    int getRequiredTransformFeedbackBufferCount()
> 
> const

This function isn't const, since cacheInfoIfNeeded() is not, but Darin's comment about not calling getters "get" applies here

> Source/WebCore/html/canvas/WebGLTransformFeedback.h:70
> +    bool m_active = false;

Style nit: class member initializer defaults should use { } notation.
Comment 5 Kenneth Russell 2020-07-06 10:55:24 PDT
Comment on attachment 403419 [details]
rebaseline layout tests and fix non-mac build

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

Great work James figuring out all of the dependent fixes needed!

LGTM modulo Darin's and Justin's other feedback.

To fix the build failure on gtk, it might be necessary to change WebGLTransformFeedback.idl to use:
  Conditional=WEBGL2,
  EnabledAtRuntime=WebGL2,
but not 100% sure. Some of the other WebGL 2.0-specific types (WebGLSampler, for example) only use "#if ENABLE(WEBGL)" guards in their headers, which seems wrong.

> Source/WebCore/html/canvas/WebGL2RenderingContext.cpp:1788
> +    if (isContextLostOrPending() || !feedbackObject || feedbackObject->isDeleted() || !validateWebGLObject("deleteTransformFeedback", feedbackObject))

The isDeleted() check differs slightly from Chromium's validation - this version looks more correct. Is there a way to test the difference, and if so could you upstream a WebGL conformance test covering it?
Comment 6 James Darpinian 2020-07-06 12:21:49 PDT
Created attachment 403605 [details]
address review feedback and fix non-webgl2 builds
Comment 7 Dean Jackson 2020-07-06 12:43:48 PDT
Comment on attachment 403419 [details]
rebaseline layout tests and fix non-mac build

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

>> Source/WebCore/html/canvas/WebGL2RenderingContext.h:249
>> +    GCGLuint getMaxTransformFeedbackSeparateAttribs();
> 
> const?

And name should be maxTransformFeedbackSeparateAttribs()

>> Source/WebCore/html/canvas/WebGLTransformFeedback.cpp:93
>> +    for (auto boundBuffer : m_boundIndexedTransformFeedbackBuffers) {
> 
> This will do unnecessary reference count churn. Should just use auto&.
> 
> But also, Vector has a find function that I think we can use here.

I think you can just call m_boundIndexedTransformFeedbackBuffers.contains(buffer)
Comment 8 Dean Jackson 2020-07-06 12:49:53 PDT
Comment on attachment 403605 [details]
address review feedback and fix non-webgl2 builds

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

> Source/WebCore/html/canvas/WebGLTransformFeedback.cpp:64
> +bool WebGLTransformFeedback::setBoundIndexedTransformFeedbackBuffer(GCGLuint index, WebGLBuffer* buffer)

You don't check the return value of this. Should it return void?

> Source/WebCore/html/canvas/WebGLTransformFeedback.cpp:78
> +bool WebGLTransformFeedback::getBoundIndexedTransformFeedbackBuffer(GCGLuint index, WebGLBuffer** outBuffer)
> +{
> +    if (index > m_boundIndexedTransformFeedbackBuffers.size())
> +        return false;
> +    *outBuffer = m_boundIndexedTransformFeedbackBuffers[index].get();
> +    return true;
> +}

Not necessary right now, but this could return an Optional instead.

> Source/WebCore/html/canvas/WebGLTransformFeedback.h:51
> +    bool getBoundIndexedTransformFeedbackBuffer(GCGLuint index, WebGLBuffer** outBuffer);

Nit: Don't use "get"
Comment 9 James Darpinian 2020-07-06 14:39:41 PDT
Created attachment 403622 [details]
more review feedback
Comment 10 James Darpinian 2020-07-06 14:42:41 PDT
Comment on attachment 403419 [details]
rebaseline layout tests and fix non-mac build

Thanks for all the feedback! It should all be addressed now.

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

>> Source/WebCore/html/canvas/WebGL2RenderingContext.cpp:1788
>> +    if (isContextLostOrPending() || !feedbackObject || feedbackObject->isDeleted() || !validateWebGLObject("deleteTransformFeedback", feedbackObject))
> 
> The isDeleted() check differs slightly from Chromium's validation - this version looks more correct. Is there a way to test the difference, and if so could you upstream a WebGL conformance test covering it?

These checks happen in the DeleteObject function in Chromium's implementation, but I don't think the behavior is different.

>> Source/WebCore/html/canvas/WebGL2RenderingContext.cpp:1836
>> +    WebGLTransformFeedback* toBeBound = feedbackObject ? feedbackObject : m_defaultTransformFeedback.get();
> 
> auto

Done.

>>> Source/WebCore/html/canvas/WebGL2RenderingContext.h:249
>>> +    GCGLuint getMaxTransformFeedbackSeparateAttribs();
>> 
>> const?
> 
> And name should be maxTransformFeedbackSeparateAttribs()

Done.

>>> Source/WebCore/html/canvas/WebGLProgram.h:77
>>> +    int getRequiredTransformFeedbackBufferCount()
>> 
>> const
> 
> This function isn't const, since cacheInfoIfNeeded() is not, but Darin's comment about not calling getters "get" applies here

Removed "get".

>>> Source/WebCore/html/canvas/WebGLTransformFeedback.cpp:93
>>> +    for (auto boundBuffer : m_boundIndexedTransformFeedbackBuffers) {
>> 
>> This will do unnecessary reference count churn. Should just use auto&.
>> 
>> But also, Vector has a find function that I think we can use here.
> 
> I think you can just call m_boundIndexedTransformFeedbackBuffers.contains(buffer)

This function was actually not used, so I deleted it.

>> Source/WebCore/html/canvas/WebGLTransformFeedback.cpp:105
>> +    }
> 
> range-based for loop would be better:
> 
>     for (auto& boundBuffer : m_boundIndexedTransformFeedbackBuffers) {
>         if (boundBuffer == &buffer)
>             boundBuffer = nullptr;
>     }

Done.

>> Source/WebCore/html/canvas/WebGLTransformFeedback.h:58
>> +    WebGLProgram* getProgram() const { return m_program.get(); }
> 
> WebKit coding style says we don’t name these functions "get".

Done.

>> Source/WebCore/html/canvas/WebGLTransformFeedback.h:59
>> +    void setProgram(WebGLProgram*);
> 
> Can this take a reference instead of a pointer?

Done.

>> Source/WebCore/html/canvas/WebGLTransformFeedback.h:62
>> +    void unbindBuffer(WebGLBuffer*);
> 
> Can these take a reference instead of a pointer?

Done.

>> Source/WebCore/html/canvas/WebGLTransformFeedback.h:70
>> +    bool m_active = false;
> 
> Style nit: class member initializer defaults should use { } notation.

Done.
Comment 11 James Darpinian 2020-07-06 14:46:09 PDT
Comment on attachment 403605 [details]
address review feedback and fix non-webgl2 builds

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

>> Source/WebCore/html/canvas/WebGLTransformFeedback.cpp:64
>> +bool WebGLTransformFeedback::setBoundIndexedTransformFeedbackBuffer(GCGLuint index, WebGLBuffer* buffer)
> 
> You don't check the return value of this. Should it return void?

Done.

>> Source/WebCore/html/canvas/WebGLTransformFeedback.h:51
>> +    bool getBoundIndexedTransformFeedbackBuffer(GCGLuint index, WebGLBuffer** outBuffer);
> 
> Nit: Don't use "get"

The style guide says to use get for getters that use out parameters.
Comment 12 EWS 2020-07-06 17:01:56 PDT
Committed r263999: <https://trac.webkit.org/changeset/263999>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 403622 [details].
Comment 13 Radar WebKit Bug Importer 2020-07-06 17:02:20 PDT
<rdar://problem/65154525>