[webkit-reviews] review denied: [Bug 72738] [chromium] Accelerated canvas broken in threaded compositing mode : [Attachment 119028] Moved copyTexImage2D to updateCompositorResources

[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 119028: Moved copyTexImage2D to updateCompositorResources
https://bugs.webkit.org/attachment.cgi?id=119028&action=review

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


> Source/WebCore/platform/graphics/chromium/Canvas2DLayerChromium.cpp:74
> +    // Compositor resources should have been cleaned up on the compositor
thread.
> +    ASSERT(!compositedTextureId());

I'm not sure how you imagine this might work but I don't think it can - the
CCCanvasLayerImpl associated with this canvas will be destroyed _after_ the
Canvas2DLayerChromium is destroyed.  Thus the composited texture has to outlive
the Canvas2DLayerChromium

> Source/WebCore/platform/graphics/chromium/Canvas2DLayerChromium.h:66
> +    unsigned m_backTextureId;
> +    unsigned m_frontTextureId;

why do we have 3 texture IDs for each instance of Canvas2DLayerChromium:

Canvas2DLayerChromium::m_backTextureId
Canvas2DLayerChromium::m_frontTextureId
CanvasLayerChromium::m_compositedTextureId

?

> Source/WebCore/platform/graphics/chromium/Canvas2DLayerChromium.h:69
> +    // Owned by the compositor context, which is where the back -> front
copy happens.
> +    Platform3DObject m_fbo;

This comment's a bit confusing - the framebuffer object is not the interesting
thing to share, it's the texture.  Where the FBO lives or is destroyed is a
function of where the copy happens. The FBO could conceivably live on either
the Canvas2DLayerChromium or the CCCanvasLayerImpl - it's not a huge deal.