Bug 205823 – [WebGL 2] Implement transform feedback and pass transform feedback conformance tests

• Component: New Bugs
• Status: RESOLVED FIXED

[Justin Fan]

[WebGL 2] Implement transform feedback and pass transform feedback conformance tests

[Justin Fan]

Created attachment 386873 [details]
Patch

[Justin Fan]

Created attachment 387294 [details]
Patch

[Justin Fan]

Created attachment 387304 [details]
Patch

[Justin Fan]

Created attachment 387305 [details]
Patch

[Justin Fan]

Created attachment 387307 [details]
Patch

[Dean Jackson]

Comment on attachment 387307 [details]
Patch

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

You're going to have to rebase, sorry.

> Source/WebCore/html/canvas/WebGL2RenderingContext.cpp:105
> +    // Remove all references to WebGLObjects so if they are the last reference
> +    // they will be freed before the last context is removed from the context group.

I don't think we need this comment.

> Source/WebCore/html/canvas/WebGL2RenderingContext.cpp:331
> -    memcpy(static_cast<char*>(dstData->baseAddress()) + dstData->byteOffset() + dstOffset * elementSize, ptr, copyLength * elementSize);
> +    if (ptr)
> +        memcpy(static_cast<char*>(dstData->baseAddress()) + dstData->byteOffset() + dstOffset * elementSize, ptr, copyLength * elementSize);
> +

If ptr is null, wouldn't we want to throw an error?

> Source/WebCore/html/canvas/WebGL2RenderingContext.cpp:1100
> +    auto addResult = m_activeQueries.add(targetKey, makeRefPtr(&query));
> +
> +    if (!addResult.isNewEntry) {

You can do this in one line - no need for addResult.

> Source/WebCore/html/canvas/WebGL2RenderingContext.cpp:1268
> +            synthesizeGLError(GraphicsContextGL::INVALID_OPERATION, "bindTransformFeedback", "cannot bind a deleted Transform Feedback object");

Nit: Transform Feedback should probably be one word.

> Source/WebCore/html/canvas/WebGL2RenderingContext.cpp:2541
> +    size_t index = m_boundTransformFeedbackBuffers.find(buffer);
> +    if (index < m_boundTransformFeedbackBuffers.size())
> +        m_boundTransformFeedbackBuffers[index] = nullptr;

Why can't you call m_boundTransformFeedbackBuffers.removeAll(buffer) ?

> Source/WebCore/platform/graphics/GraphicsContextGL.h:1068
> +    // WebGL2 Transform Feedback calls
> +    virtual PlatformGLObject createTransformFeedback() = 0;
> +    virtual void deleteTransformFeedback(PlatformGLObject) = 0;
> +    virtual GCGLboolean isTransformFeedback(PlatformGLObject) = 0;
> +    virtual void bindTransformFeedback(GCGLenum, PlatformGLObject) = 0;
> +    virtual void beginTransformFeedback(GCGLenum) = 0;
> +    virtual void endTransformFeedback() = 0;
> +    virtual void transformFeedbackVaryings(PlatformGLObject, const Vector<String>&, GCGLenum) = 0;
> +    virtual void getTransformFeedbackVarying(PlatformGLObject, GCGLuint, ActiveInfo&) = 0;
> +
> +    virtual void bindBufferBase(GCGLenum, GCGLuint, PlatformGLObject) = 0;
> +

I think I landed all these just moments ago.

[Justin Fan]

Comment on attachment 387307 [details]
Patch

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

>> Source/WebCore/html/canvas/WebGL2RenderingContext.cpp:105
>> +    // they will be freed before the last context is removed from the context group.
> 
> I don't think we need this comment.

Ok, it's copied from the WebGLRenderingContextBase dtor.

>> Source/WebCore/html/canvas/WebGL2RenderingContext.cpp:331
>> +
> 
> If ptr is null, wouldn't we want to throw an error?

If ptr is null, ANGLE throws an error here.

>> Source/WebCore/html/canvas/WebGL2RenderingContext.cpp:1100
>> +    if (!addResult.isNewEntry) {
> 
> You can do this in one line - no need for addResult.

ok!

>> Source/WebCore/html/canvas/WebGL2RenderingContext.cpp:1268
>> +            synthesizeGLError(GraphicsContextGL::INVALID_OPERATION, "bindTransformFeedback", "cannot bind a deleted Transform Feedback object");
> 
> Nit: Transform Feedback should probably be one word.

I'll probably keep this, since it follows the convention in the conformance test output.

>> Source/WebCore/html/canvas/WebGL2RenderingContext.cpp:2541
>> +        m_boundTransformFeedbackBuffers[index] = nullptr;
> 
> Why can't you call m_boundTransformFeedbackBuffers.removeAll(buffer) ?

I need to keep the size of the vector the same.

>> Source/WebCore/platform/graphics/GraphicsContextGL.h:1068
>> +
> 
> I think I landed all these just moments ago.

darn. I knew I should've said something >:(

[Jon Lee]

*** Bug 126943 has been marked as a duplicate of this bug. ***

[Jon Lee]

rdar://problem/15002432

[Justin Fan]

needs to land on top of https://bugs.webkit.org/show_bug.cgi?id=206081

[Justin Fan]

Created attachment 387557 [details]
Patch for landing

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 387557 [details]:

imported/w3c/IndexedDB-private-browsing/idbdatabase_close.html bug 206210 (author: [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 387557 [details]
Patch for landing

Clearing flags on attachment: 387557

Committed r254481: <https://trac.webkit.org/changeset/254481>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.