[webkit-reviews] review denied: [Bug 72738] [chromium] Accelerated canvas broken in threaded compositing mode : [Attachment 118823] Fixed lost-context check and variable names

[bugzilla-daemon at webkit.org]

James Robinson <jamesr at chromium.org> has denied Iain Merrick
<husky at google.com>'s request for review:
Bug 72738: [chromium] Accelerated canvas broken in threaded compositing mode
https://bugs.webkit.org/show_bug.cgi?id=72738

Attachment 118823: Fixed lost-context check and variable names
https://bugs.webkit.org/attachment.cgi?id=118823&action=review

------- Additional Comments from James Robinson <jamesr at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=118823&action=review


sorry, i missed some threading issues here earlier. see inline comments for
some details. this is definitely close but still needs a bit more work

> Source/WebCore/platform/graphics/chromium/Canvas2DLayerChromium.cpp:65
> +    setTextureId(0);

hmm, this deletes the compositor's texture. unfortunately, the compositor
thread may still try to issue an arbitrary number of bind+draw calls for this
texture ID after the Canvas2DLayerChromium is destroyed but before the trees
are synchronized. i think you need to destroy the compositor's texture on the
impl side somehow

> Source/WebCore/platform/graphics/chromium/Canvas2DLayerChromium.cpp:134
> +    GLC(m_context, m_context->bindTexture(GraphicsContext3D::TEXTURE_2D,
compositedTextureId()));
> +    GLC(m_context, m_context->copyTexImage2D(GraphicsContext3D::TEXTURE_2D,
0, GraphicsContext3D::RGBA, 0, 0, m_size.width(), m_size.height(), 0));

i missed this earlier but this is actually still subtly incorrect - the paint
pass is done while the compositor thread is still free-running, so it may be
issuing draw calls with this texture ID bound while the copy is happening and
pick up the wrong frame. you need to do the actual copy itself in some piece of
code that is synchronized with the compositor thread

i think one option would be to do this in updateCompositorResources() and issue
the copy commands themselves on the compositor's context, not the canvas'

leave the m_context->flush() in here - it's needed for synchronization. you can
be assured that no canvas draw calls will be made between
paintContentsIfDirty() and updateCompositorResources()