Bug 205823

Summary: [WebGL 2] Implement transform feedback and pass transform feedback conformance tests
Product: WebKit Reporter: Justin Fan <justin_fan>
Component: New BugsAssignee: Justin Fan <justin_fan>
Status: RESOLVED FIXED    
Severity: Normal CC: benjamin, cdumez, cmarcelo, commit-queue, dbates, dino, esprehn+autocc, ews, graouts, gyuyoung.kim, jonlee, kondapallykalyan, noam, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 126404    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch for landing none

Description Justin Fan 2020-01-06 11:58:38 PST
[WebGL 2] Implement transform feedback and pass transform feedback conformance tests
Comment 1 Justin Fan 2020-01-06 12:00:06 PST
Created attachment 386873 [details]
Patch
Comment 2 Justin Fan 2020-01-09 17:25:55 PST
Created attachment 387294 [details]
Patch
Comment 3 Justin Fan 2020-01-09 18:40:27 PST
Created attachment 387304 [details]
Patch
Comment 4 Justin Fan 2020-01-09 18:46:29 PST
Created attachment 387305 [details]
Patch
Comment 5 Justin Fan 2020-01-09 18:48:35 PST
Created attachment 387307 [details]
Patch
Comment 6 Dean Jackson 2020-01-10 12:37:08 PST
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.
Comment 7 Justin Fan 2020-01-10 12:44:40 PST
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 >:(
Comment 8 Jon Lee 2020-01-10 12:54:13 PST
*** Bug 126943 has been marked as a duplicate of this bug. ***
Comment 9 Jon Lee 2020-01-10 12:55:13 PST
rdar://problem/15002432
Comment 10 Justin Fan 2020-01-10 17:01:41 PST
needs to land on top of https://bugs.webkit.org/show_bug.cgi?id=206081
Comment 11 Justin Fan 2020-01-13 12:28:40 PST
Created attachment 387557 [details]
Patch for landing
Comment 12 WebKit Commit Bot 2020-01-13 18:34:04 PST
The commit-queue encountered the following flaky tests while processing attachment 387557 [details]:

imported/w3c/IndexedDB-private-browsing/idbdatabase_close.html bug 206210 (author: youennf@gmail.com)
The commit-queue is continuing to process your patch.
Comment 13 WebKit Commit Bot 2020-01-13 18:34:39 PST
Comment on attachment 387557 [details]
Patch for landing

Clearing flags on attachment: 387557

Committed r254481: <https://trac.webkit.org/changeset/254481>
Comment 14 WebKit Commit Bot 2020-01-13 18:34:41 PST
All reviewed patches have been landed.  Closing bug.