[webkit-reviews] review denied: [Bug 80540] [Chromium] Single buffered canvas layers with the threaded compositor : [Attachment 137576] Patch

[bugzilla-daemon at webkit.org]

James Robinson <jamesr at chromium.org> has denied Justin Novosad
<junov at chromium.org>'s request for review:
Bug 80540: [Chromium] Single buffered canvas layers with the threaded
compositor
https://bugs.webkit.org/show_bug.cgi?id=80540

Attachment 137576: Patch
https://bugs.webkit.org/attachment.cgi?id=137576&action=review

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


I think we're all in rough agreement now about the behavior you want, so the
question is just how to implement it. Do you want to sync up on video and talk
through the rest? I feel like we're close and it's just a matter of figuring
out where all the pieces fit.  Comments inline.

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.h:158
> +    void willReleaseSingleBufferedLayerTexture();
> +    void singleBufferedLayerWillDraw();

we've iterated on these names a few times but they still don't make any sense
to me. why is one function prefixed with "willRelease" but the other suffixed
with "WillDraw"? why is releasing different from any other sort of
texture-mutating operation such that it requires a special entry point?

these deserve at a minimum some descriptive comments. i think i would find it
easier to follow if these were expressed in terms of what they were trying to
accomplish - i.e. something about acquiring exclusive access to single buffered
textures or something along those lines - rather than simply describing when
they are called and not what they are for

> Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:339
> +    // When gaining visibility, don't worry about overwriting committed
contents
> +    // from the last time the layer tree was visible.

this feels dodgy - if you're rapidly switching tabs you still can't draw a
bogus frame

> Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:341
> +	   m_singleBufferedLayerStateOnImplThread =
SingleBufferedLayerStateReady;

if the lock is acquired while visible and then the tab becomes invisible,
what's supposed to release the completion? we can't leave the main thread
blocked forever, but we aren't going to draw while invisible so I don't see any
logic that will signal the completion.

It feels like all of this really should be up to the scheduler. IOW, in this
code we should alert the scheduler that we have the
main-thread-single-buffered-texture-lock and make it the scheduler's
responsibility to release it at the Right Time (TM). values for the Right Time
could be:

1.) If we've drawn since the last commit, release it immediately
2.) If we haven't drawn since the last commit yet, hold it until the last draw
opportunity and release it then if we're going to draw, or just release it if
we can't draw for whatever reason

having all scheduling-type decisions in one place makes it significantly easier
to see the logic and tell if there are any holes in the state transitions.
we're also already tracking most of the state you want in the scheduler

> Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:664
> +void CCThreadProxy::singleBufferedLayerWillDraw()

this is the same as "willRelease....()" except for the function it's calling on
the impl side - it feels like we really should have one entry point and
function doing synchronization that takes a parameter indicating whether it
should wait for the next frame on the impl thread or not. although now that I
look more closely I can't figure out why release is so special that it gets a
different codepath

> Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.h:96
> +	   SingleBufferedLayerStateCommitted,

we already track if we're in between a commit and the first draw in
CCSchedulerStateMachine (it's the WAITING_FOR_FIRST_DRAW value on CommitState).
I don't think we should attempt to shadow this state elsewhere - instead we
should ask the scheduler if it's in this state at the appropriate points

> Source/WebCore/platform/graphics/skia/ImageBufferSkia.cpp:126
>	   SkAutoTUnref<AcceleratedDeviceContext> deviceContext(new
AcceleratedDeviceContext(context3D.get()));
> +	   deviceContext.get()->setPlatformLayer(data->m_platformLayer.get());
>	   canvas = new SkDeferredCanvas(device.get(), deviceContext.get());

if AcceleratedDeviceContext is maintaining a weak pointer, what's ensuring that
the ADC does not outlive the layer? does the SkCanvas take exclusive ownership
of the device context? why not have the ADC take a reference?

why is this a layer setter instead of a constructor-time invariant? we appear
to never change the PlatformLayer out later

> Source/WebCore/platform/graphics/skia/ImageBufferSkia.cpp:193
> +    if (m_data.m_platformLayer && !m_data.m_platformLayer->isDeferred())
> +	   m_data.m_platformLayer->layerWillDraw();

it seems odd to check isDeferred here. the layer is going to draw whether it's
deferred or not, isn't it? the layer knows whether it's deferred or not so it
seems like it should be responsible for doing whatever calls on the compositor
are necessary

> Source/WebKit/chromium/tests/CCLayerTreeHostTest.cpp:887
> +// Verify that when resuming visibility, requesting layer write permission
> +// will not deadlock the main thread even though there are not yet any

you also need a test that covers going invisible before the first draw with the
lock held and makes sure that we do release the main thread