[webkit-reviews] review denied: [Bug 72738] [chromium] Accelerated canvas broken in threaded compositing mode : [Attachment 119261] Fixed texture allocation

[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 119261: Fixed texture allocation
https://bugs.webkit.org/attachment.cgi?id=119261&action=review

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


We're spiraling in ever and ever closer, like a toilet flushing or some such
analogy. I like moving to a managed texture but have some comments inline about
the usage. I also think we can simplify the triple of texture IDs down a bit
even further. After that, I think we are good to go.

> Source/WebCore/platform/graphics/chromium/Canvas2DLayerChromium.cpp:72
> +    m_rendererTextureId = textureId;

what do you mean by 'renderer' here? that's a novel term for this part of the
code

could we call this the back buffer and the other texture the front buffer?

> Source/WebCore/platform/graphics/chromium/Canvas2DLayerChromium.cpp:115
> +    if (host && layerTreeHost() != host)
> +	   setTextureManager(host->contentsTextureManager());

i think you want to clear the m_managedTexture when transitioning to a nil
CCLayerTreeHost as well

> Source/WebCore/platform/graphics/chromium/Canvas2DLayerChromium.cpp:132
> +    bool success = m_managedTexture->reserve(m_size,
GraphicsContext3D::RGBA);
> +    if (!success)
> +	   return;

i like the idea of using managed textures but there is a bit of subtlety. the
way we do the lifetime management for content textures that i believe you will
want to follow is:

1.) in paintContents(), reserve any textures you may want for this frame. if
the reservation fails, then you're over the memory budget and can't draw this
layer - mark is as skipped and sadface
2.) in updateCompositorResources(), bind the texture you previous reserved and
use it
3.) in unreserveContentsTextures() (which is called at the end of the frame,
after uploads and the commit have occured) unreserve the texture

the reason to do the reserve at paint time is to make sure that you get texture
memory before all of the other layers that come after you in the iteration, and
to make sure that your LRU-ness is properly set. it's fine to reserve without
access to the context, no actual GL calls are made until the bindTexture call.

i think that with this code you will frequently not have any memory left by the
time you try to reserve the canvas, resulting in sadface

> Source/WebCore/platform/graphics/chromium/Canvas2DLayerChromium.cpp:135
> +    setCompositedTextureId(m_managedTexture->textureId());

this seems awkward - you still have 3 variables on Canvas2DLayerChromium that
are responsible for storing 2 texture IDs:

m_rendererTextureId
m_compositorTextureId
m_managedTexture

it seems to me like what you really want to do is remove m_compositedTextureId
from CanvasLayerChromium, make compositedTextureId() virtual and provide
different implementations for Canvas2DLayerChromium and WebGLLayerChromium

> Source/WebCore/platform/graphics/chromium/cc/CCCanvasLayerImpl.h:42
> +
> +    // Deletes the attached texture, if any. To prevent this call
setTextureId(0) first.

not sure i understand this comment - it doesn't look like there is any code
change in this patch and the existing code, as far as i can tell, doesn't
delete any textures

> Source/WebKit/chromium/tests/Canvas2DLayerChromiumTest.cpp:99
> +	   InSequence s;

nit we don't typically abbreviate in WebKit and we never use one letter
variable names except for i/j loop counters and x/y for geometry code