Summary: | [WebGL 2] Implement transform feedback and pass transform feedback conformance tests | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Justin Fan <justin_fan> | ||||||||||||||
Component: | New Bugs | Assignee: | Justin Fan <justin_fan> | ||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||
Severity: | Normal | CC: | benjamin, cdumez, cmarcelo, commit-queue, dbates, dino, esprehn+autocc, ews-watchlist, 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, 209513 | ||||||||||||||||
Attachments: |
|
Description
Justin Fan
2020-01-06 11:58:38 PST
Created attachment 386873 [details]
Patch
Created attachment 387294 [details]
Patch
Created attachment 387304 [details]
Patch
Created attachment 387305 [details]
Patch
Created attachment 387307 [details]
Patch
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 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 >:( *** Bug 126943 has been marked as a duplicate of this bug. *** needs to land on top of https://bugs.webkit.org/show_bug.cgi?id=206081 Created attachment 387557 [details]
Patch for landing
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 on attachment 387557 [details] Patch for landing Clearing flags on attachment: 387557 Committed r254481: <https://trac.webkit.org/changeset/254481> All reviewed patches have been landed. Closing bug. |