Bug 80540 – [Chromium] Single buffered canvas layers with the threaded compositor

• Component: New Bugs
• Status: RESOLVED FIXED

[Justin Novosad]

[Chromium] Single buffered canvas layers with the threaded compositor

[Justin Novosad]

Created attachment 130696 [details]
Patch

[WebKit Review Bot]

Attachment 130696 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/ChangeLog:9:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
Source/WebCore/ChangeLog:10:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
Source/WebCore/ChangeLog:11:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
Source/WebCore/ChangeLog:12:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
Source/WebCore/ChangeLog:13:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
Total errors found: 5 in 15 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[James Robinson]

Comment on attachment 130696 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=130696&action=review

> Source/WebCore/platform/graphics/skia/ImageBufferSkia.cpp:90
> +                host->beginLayerWrite();

why is this logic here instead of in Canvas2DLayerChromium? because we might have draws initiated into the canvas that aren't triggered by the compositor?

when exactly (how often) is this function called?

[Justin Novosad]

Comment on attachment 130696 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=130696&action=review

>> Source/WebCore/platform/graphics/skia/ImageBufferSkia.cpp:90
>> +                host->beginLayerWrite();
> 
> why is this logic here instead of in Canvas2DLayerChromium? because we might have draws initiated into the canvas that aren't triggered by the compositor?
> 
> when exactly (how often) is this function called?

That is correct. We can  sometimes have draws that are initiated during Javascript execution. If a readback from the canvas is necessary because of a call to toDataURL or getImageData, then the canvas has to execute deferred draw commands immediately.  With the vast majority of canvas games and demos, those API methods are never used, or they are only used in initialization routines.

A possible future improvement would be to switch the canvas back to double buffered mode on the fly if there is enough available vram and if this method keeps keep getting called at each frame.

[Justin Novosad]

FYI: I am currently working on an incremental patch that will allow single buffering without deferred canvas, which I intend to land separately.

[Justin Novosad]

Created attachment 136505 [details]
Patch

[James Robinson]

How frequently is willDraw() called? Once per draw command, once per force flush, once per invalidation, something else?

Could you attach a trace showing how this works on some simple canvas demo? That might make it easier to reason about the flow.

[Stephen White]

Comment on attachment 136505 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=136505&action=review

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.h:168
> +    enum LayerState {
> +        LAYER_STATE_COMMITTED,
> +        LAYER_STATE_READY,
> +        LAYER_STATE_DIRTY,
> +    };

Since this new state is going to automatically rate limit us to one canvas frame per vsync, I don't think this is really a workable solution, since it will lower the framerate on non-RAF content.

For that reason, I'm hesitant to land this new infrastructure, since I don't think it's going to lead to viable solution long-term (ie., one that doesn't penalize us over our current performance in the single-buffer, non-threaded-compositor case).

[Justin Novosad]

(In reply to comment #7)
> How frequently is willDraw() called? Once per draw command, once per force flush, once per invalidation, something else?
> 

willDraw() normally gets called once per draw command, but id deferred canvas is enable, willDraw is called at playback time.  Playback happens during commit, and it can also happen during JS execution if a readback or a canvas to canvas copy is performed.

> Could you attach a trace showing how this works on some simple canvas demo? That might make it easier to reason about the flow.

Good idea. Will follow-up

[Justin Novosad]

(In reply to comment #8)
> (From update of attachment 136505 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=136505&action=review
> 
> > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.h:168
> > +    enum LayerState {
> > +        LAYER_STATE_COMMITTED,
> > +        LAYER_STATE_READY,
> > +        LAYER_STATE_DIRTY,
> > +    };
> 
> Since this new state is going to automatically rate limit us to one canvas frame per vsync, I don't think this is really a workable solution, since it will lower the framerate on non-RAF content.
> 
> For that reason, I'm hesitant to land this new infrastructure, since I don't think it's going to lead to viable solution long-term (ie., one that doesn't penalize us over our current performance in the single-buffer, non-threaded-compositor case).

Let me just spell-out the constraints we are working with here:
1. We do not want threaded compositing to cause performance regressions in accelerated canvas, with respect to benchmark scores.
2. We want to maintain the benefits of the threaded compositor (fast, non-janky)
3. We want to maintain highest possible "real" frame rate for pages that use canvas 2d. By "real", I mean that frames that are rendered but never displayed don't count.

To meet constraint 1, we have to move away from double buffering.  The extra vram consumption introduces a penalty that is otherwise uncircumventable for pages that bust the vram ceiling.
To meet constraint 2, we must not impede the execution of the compositor thread in any way.
To meet constraint 3, we must avoid blocking the main thread with only one frame pending.

The current patch will only meet constaints 2 and 3 when deferred canvas rendering is enabled and there are no readbacks or canvas to canvas copies. I know that is a big "if". 

Without deferred canvas rendering, the compositor may drop frames if draws are scheduled while js code is drawing to a canvas (LAYER_STATE_DIRTY). If no rate limiting is imposed, the compositor may drop frames indefinitely.  That is why the main thread blocks if a canvas draw is attempted between a commit and a compositor draw.  This has the unfortunate consequence of snapping the frame rate to 30 when it drops below 60, thus breaking constraint 3.

I see two possible solutions, both have caveats.  
Solution 1: Allow compositor draws to be postponed. If a compositor draw is scheduled while layer state is LAYER_STATE_DIRTY, rather than drop a frame, reschedule the compositor to draw immediately after the next commit. Caveat: compositor draws could get phase shifted with respect to vsync, and the phase shift would not be steady for non-RAF pages. So this could introduce a little bit of compositor jank (+/- 1 frame period).

Solution 2: Use deferred canvas rendering and defer rendering to the compositor thread.  With canvas and compositor rendering happening in the same thread synchronizing access to the canvas backing store is no longer an issue.  Caveat: Extra load on the compositor thread. The playback may not have time to complete between commit and draw, which may cause the draw to become out of phase with vsync it could even reduce the compositor's frame rate in extreme cases.

Suggestion are welcome.

[WebKit Review Bot]

Comment on attachment 136505 [details]
Patch

Attachment 136505 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12381574

[Justin Novosad]

Created attachment 136662 [details]
Patch

[Justin Novosad]

(In reply to comment #12)
> Created an attachment (id=136662) [details]
> Patch

Corrected initializer order to fix build

[Justin Novosad]

Created attachment 136739 [details]
Trace graphs showing single buffered rendering behavior with the threaded compositor enabled

Three traces are attached all were produced with this patch and --enable-threaded-compositing

What to look for: in the renderer main thread, calls to CCThreadProxy::beginLayerWrite show where the thread blocks while it waits for the previously committed frame to be drawn by the compositor.

fishie-non-deferred-trace: Running the IE testdrive FishIE demo wit 1000 fish.  With this many fish, the 16.7ms window is exceeded and the frame rate snaps to 30fps by stalling the renderer main thread while wait for the previously committed frame to be drawn by the compositor.

fishie-deferred-trace: Same as previous test, but with --enable-deferred-2d-canvas.  This reduces blockage in the main thread, allowing Javascript execution of rendering instruction to be asynchronous with respect to compositing.  The trace graph shows occasion short blockages of the main thread when attempting to commit a new frame while the previously committed frame has not yet been drawn by the compositor.  It seems to occur every time a compositor draw is skipped (look for CCFrameRateController::onTimerTickButMaxFramesPending) This indicate that performance is bound by the GPU. Running full screen fps is 47-48, same as when rendering with the threaded compositor, without this change.  However, this is down from 52fps without the threaded compositor.  My Opinion: the 10% throughput regression is not that big a deal considering that smoothness is greatly improved.  The performance actually feels like it improved to a human observer.

html5-benchmark.com-trace: Running the demo at htm5-benchmark.com. This is an example of a website that can render at 60fps.  The trace graph shows how beginLayerWrite acts as an implicit rate limiter. deferred rendering has no effect on this demo because it is based on non deferrable draw commands (canvas to canvas copies).

[Justin Novosad]

Looked at the Fireflies IE testdrive demo.  The performance of that demo was greatly affect by the threaded compositor due to canvas double buffering.  Running the demo in a 1000x1000 window showed performance drop from 22fps (without threaded compositing) down to 7fps.  With this patch, threaded composing performs at 22fps again.  Another big win is that the rendering is A LOT less janky than ever before, in particular the initialization sequence.

[Justin Novosad]

Created attachment 136748 [details]
Patch

[Stephen White]

Comment on attachment 136748 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=136748&action=review

> Source/WebCore/platform/graphics/chromium/Canvas2DLayerChromium.h:38
> +#include "ImageBuffer.h"

There's a bit of a cyclic dependency here.  Could be eliminated by Canvas2DLayerChromium having its own DeferralMode, at the cost of some translation from one to the other.

> Source/WebCore/platform/graphics/chromium/LayerChromium.cpp:572
> +void LayerChromium::beginLayerWrite() const
> +{
> +    if (m_layerTreeHost)
> +        m_layerTreeHost->beginLayerWrite();
> +}
> +

Does this need to be in LayerChromium?  It seems to be Canvas2DLayerChromium-specific.  In fact, it looks like you could call layerTreeHost()->beginLayerWrite() directly from Canvas2DLayerChromium::willDraw().

To bikeshed for a bit:  I'm not fond of the beginLayerWrite() name.  What is it writing, exactly?  If it's drawing to the texture, maybe it should be beginDrawTexture() or beginTextureDraw(), or beginContentsDraw() or beginDrawContents() or ...

> Source/WebCore/platform/graphics/skia/ImageBufferSkia.cpp:127
>      } else

You could probably just but an #endif here, and skip the extra clause below.

> Source/WebCore/platform/graphics/skia/ImageBufferSkia.cpp:194
> +#if USE(ACCELERATED_COMPOSITING)
> +    if (m_data.m_platformLayer)
> +        m_data.m_platformLayer->willDraw();
> +#endif

I know we've talked about this before, but I'm still not fond of retrieval of the context as a signal that we're going to draw, although I understand that this was the only easy way to hook into CanvasRenderingContext2D's draws before they happen.

Oh well, I guess the worst thing that can happen is that we block the render thread unnecessarily, if someone later adds a call to context() without actually drawing anything.

[WebKit Review Bot]

Comment on attachment 136748 [details]
Patch

Attachment 136748 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12392026

[James Robinson]

Looks like this needs a rebase.

I think a more idiomatic name for "beginLayerWrite()" would be "willDraw" or something along those lines.

On a higher level, I really feel that if we can't hit 60fps then we really want the compositor framerate to drop to 30FPS.  Rendering at 52FPS isn't helping anyone at all.  If that means eating extra frames in an SkPicture to keep JS spinning because of crappy benchmarks, then so be it, but that's a secondary concern.  The primary concern is having something that looks good to users which in the case where we can't hit 60fps means drawing at 30.

[James Robinson]

Comment on attachment 136748 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=136748&action=review

>> Source/WebCore/platform/graphics/skia/ImageBufferSkia.cpp:194
>> +#endif
> 
> I know we've talked about this before, but I'm still not fond of retrieval of the context as a signal that we're going to draw, although I understand that this was the only easy way to hook into CanvasRenderingContext2D's draws before they happen.
> 
> Oh well, I guess the worst thing that can happen is that we block the render thread unnecessarily, if someone later adds a call to context() without actually drawing anything.

Why do we need a call here and in AcceleratedDeviceContext::prepareForDraw()?

[Nat Duca]

Comment on attachment 136748 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=136748&action=review

Did you give any thought to, as you get this closer to "lets land it" to having one patch that just establishes the beginLayerWrite code and does the layer changes. Then, we can do the proxy changes as another patch? This patch is kinda mega mega.

We do need to bikeshed a name for this flow. Words from synchornization come to mind, like aquire/release ... acquireSingleBufferedLayerWritePermission or something. But I'm not sure that's right either.

> Source/WebCore/platform/graphics/chromium/Canvas2DLayerChromium.cpp:86
> +    m_notifyLayerTreeHostBeforeDrawing = m_deferralMode == NonDeferred && !m_useDoubleBuffering && CCProxy::hasImplThread();

This breaks my face.
Also, do you really need to cache this state? Can't you compute this on the fly, e.g. in willDraw? Or if not there, make it a private getter?

What about always notifying the layer tree host, and the proxy can decide whether to do anything about it?

> Source/WebCore/platform/graphics/chromium/Canvas2DLayerChromium.cpp:154
> +        beginLayerWrite();

I'm a little confused at the semantics of this when there are >1 layers in the tree.

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:118
> +    , m_layerState(LAYER_STATE_READY)

Isnt' this the "singlebuffered layer state"?

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:145
> +bool CCLayerTreeHostImpl::beginLayerWrite()

beginSingleBufferedLayerWrite or something. General theme I think.

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:146
> +{

This feels like proxy and/or scheduler code, when you combine it with the beginLayerWrite and beginLayerWriteOnIMplThread. Why does it live on the impl tree? It seems to me that the beginLayerWrite is more a property of "layerWriteRequested" and scheduledActionAllowLayerWrite that you feed down to the scheduler and schedulerStateMachine.

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:158
> +    if (m_layerState == LAYER_STATE_DIRTY)

Have you thought through putting this in prepareToDraw vs Candraw? What happens in the case of setNeedsForcedRedraw flow?

> Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:592
> +    if (m_layerDrawCompletionEvent) {

This field should be m_layerDrawCompletionEventOnImplThread --- we use the OnImplThread suffix to denote thread-pinnedness

> Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:641
> +    m_layersWritable = true;

I'm a little confused about the overlap of this state with the state on the impl tree. Can there be just one place where this state exists?

> Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:649
> +    if (!m_layerTreeHostImpl || m_layerTreeHostImpl->beginLayerWrite())

I mentioned this before, but I would rather this went through the scheduler, or at least understand how that flow migth look. Maybe I'm wrong on this advice... take a peek at how compositeAndReadback works and see if that makes any sense? Happy to chat offline.

[Stephen White]

(In reply to comment #19)
> Looks like this needs a rebase.
> 
> I think a more idiomatic name for "beginLayerWrite()" would be "willDraw" or something along those lines.

CCLayerTreeHost::willDraw() and CCThreadProxy::willDraw() seem a little generic, though.  I mean, it's sort of implying that the compositor is about to draw, which isn't the case.  Maybe those ones *should* have layer in their name, or something.  (Just for the record, I also hate CCThreadProxy as a name -- it's totally meaningless to me).

OK, I have used up my allotted bikeshedding quota for this comment.

> On a higher level, I really feel that if we can't hit 60fps then we really want the compositor framerate to drop to 30FPS.  Rendering at 52FPS isn't helping anyone at all.  If that means eating extra frames in an SkPicture to keep JS spinning because of crappy benchmarks, then so be it, but that's a secondary concern.  The primary concern is having something that looks good to users which in the case where we can't hit 60fps means drawing at 30.

I think this change actually delivers both:  decent scores on non-RAF content, and a better user experience.  Unlike the single-threaded case with the rate limiter, we're not slamming the GPU with useless commands, so the experience is much less janky and the system remains more responsive throughout.  I encourage you to try out this patch and try some of the GPU-heavy demos -- they're much smoother.  In theory, mouse latency should also be reduced, but I have yet to verify that.

[Justin Novosad]

Comment on attachment 136748 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=136748&action=review

>> Source/WebCore/platform/graphics/chromium/Canvas2DLayerChromium.cpp:86
>> +    m_notifyLayerTreeHostBeforeDrawing = m_deferralMode == NonDeferred && !m_useDoubleBuffering && CCProxy::hasImplThread();
> 
> This breaks my face.
> Also, do you really need to cache this state? Can't you compute this on the fly, e.g. in willDraw? Or if not there, make it a private getter?
> 
> What about always notifying the layer tree host, and the proxy can decide whether to do anything about it?

Sorry about your face.  Yes it can be computed on the fly.  I want to avoid deferring this logic to the proxy (or the layer tree host) because the CC code currently has no notion of deferred rendering, and I think it is cleaner if it stays that way.

>> Source/WebCore/platform/graphics/chromium/Canvas2DLayerChromium.cpp:154
>> +        beginLayerWrite();
> 
> I'm a little confused at the semantics of this when there are >1 layers in the tree.

Good point, as far as the layer tree is concerned, this call is not associated with any particular layer. It refers to all single buffered layers.

>> Source/WebCore/platform/graphics/chromium/LayerChromium.cpp:572
>> +
> 
> Does this need to be in LayerChromium?  It seems to be Canvas2DLayerChromium-specific.  In fact, it looks like you could call layerTreeHost()->beginLayerWrite() directly from Canvas2DLayerChromium::willDraw().
> 
> To bikeshed for a bit:  I'm not fond of the beginLayerWrite() name.  What is it writing, exactly?  If it's drawing to the texture, maybe it should be beginDrawTexture() or beginTextureDraw(), or beginContentsDraw() or beginDrawContents() or ...

Modifying a texture used by the compositor on the main thread is not a 2d canvas-specific notion, although the current use case is 2d canvas-specific.  I am kind of ambivalent on this matter. If you'd rather we keep the base class tidy, I can do that.

>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:146
>> +{
> 
> This feels like proxy and/or scheduler code, when you combine it with the beginLayerWrite and beginLayerWriteOnIMplThread. Why does it live on the impl tree? It seems to me that the beginLayerWrite is more a property of "layerWriteRequested" and scheduledActionAllowLayerWrite that you feed down to the scheduler and schedulerStateMachine.

Putting the LayerState in the impl tree was actually your idea.  Remember? You made me realize that everything the scheduler state machine needed in order to play nice with single buffering could be channeled through CCLayerTreeHostImpl::canDraw().  I am actually a big fan of the idea of not adding complexity to the state transition logic.  I could move all this logic to CCThreadProxy

>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:158
>> +    if (m_layerState == LAYER_STATE_DIRTY)
> 
> Have you thought through putting this in prepareToDraw vs Candraw? What happens in the case of setNeedsForcedRedraw flow?

I am unsure about the safety of this code with setNeedsForcedRedraw.  I believe that a forced redraw can't currently be triggered between a beginLayerWrite and a commit, but I could be wrong.  That is why I added an ASSERT in CCLayerTreeHostImpl::prepareToDraw. If there is a race condition with that, it could cause an incomplete canvas frame to be composited.  Is that a big deal?  Perhaps you can enlighten me here: what exact situations trigger a forced redraw?

>> Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:641
>> +    m_layersWritable = true;
> 
> I'm a little confused about the overlap of this state with the state on the impl tree. Can there be just one place where this state exists?

To avoid race conditions, only the impl thread can access the layerState. m_layerWrtiable allows us to avoid a postTask/signal round trip to the impl thread on each call to beginLayerWrite. After a commit, only the first call to beginLayerWrite needs to propagate to the impl thread. m_layersWritable tells us whether the layer state has already transitioned to DIRTY since the last commit.

>> Source/WebCore/platform/graphics/skia/ImageBufferSkia.cpp:127
>>      } else
> 
> You could probably just but an #endif here, and skip the extra clause below.

Yes, but I thought this way was easier to read.

>>> Source/WebCore/platform/graphics/skia/ImageBufferSkia.cpp:194
>>> +#endif
>> 
>> I know we've talked about this before, but I'm still not fond of retrieval of the context as a signal that we're going to draw, although I understand that this was the only easy way to hook into CanvasRenderingContext2D's draws before they happen.
>> 
>> Oh well, I guess the worst thing that can happen is that we block the render thread unnecessarily, if someone later adds a call to context() without actually drawing anything.
> 
> Why do we need a call here and in AcceleratedDeviceContext::prepareForDraw()?

willDraw() only kicks in for non deferred canvas rendering, while AcceleratedDeviceContext::prepareForDraw handles deferred rendering.

[Justin Novosad]

Comment on attachment 136748 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=136748&action=review

>> Source/WebCore/platform/graphics/chromium/Canvas2DLayerChromium.h:38
>> +#include "ImageBuffer.h"
> 
> There's a bit of a cyclic dependency here.  Could be eliminated by Canvas2DLayerChromium having its own DeferralMode, at the cost of some translation from one to the other.

DeferralMode is not in a class scope, it is in WebCore::.  Perhaps there is a better Header for it?  Maybe GraphicsTypes.h?

>> Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:649
>> +    if (!m_layerTreeHostImpl || m_layerTreeHostImpl->beginLayerWrite())
> 
> I mentioned this before, but I would rather this went through the scheduler, or at least understand how that flow migth look. Maybe I'm wrong on this advice... take a peek at how compositeAndReadback works and see if that makes any sense? Happy to chat offline.

The next patch I will upload will centralize the single buffered layer logic in CCThreadProxy.  I find it feels clean there.  Take a look, and let me know if you agree. Feels unnecessary to me to make the impl layer tree and the scheduler aware single buffering.  The next patch will revert all changes I previously made to CCLayerTreeHostImpl

[Justin Novosad]

Created attachment 137388 [details]
Patch

[Justin Novosad]

(In reply to comment #25)
> Created an attachment (id=137388) [details]
> Patch

Latest patch addresses review comments.  It also fixes a race condition that was causing a lost context+deadlock when resizing an animated canvas.  The changes to Canvas2DLayerChromium::setTextureId and the new method invalidateSingleBufferedLayers on the Proxy+layer tree classes fix that race condition.

[James Robinson]

Comment on attachment 137388 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=137388&action=review

> Source/WebCore/platform/graphics/chromium/Canvas2DLayerChromium.cpp:170
> +void Canvas2DLayerChromium::layerWillDraw() const
> +{
> +    if (m_deferralMode == NonDeferred && !m_useDoubleBuffering && CCProxy::hasImplThread() && layerTreeHost())
> +        layerTreeHost()->singleBufferedLayerWillDraw();
> +}
> +
> +void Canvas2DLayerChromium::layerWillDrawDeferred() const
> +{
> +    // Called before rendering previously recorded draw operations
> +    ASSERT(m_deferralMode == Deferred);
> +    if (!m_useDoubleBuffering && CCProxy::hasImplThread() && layerTreeHost())
> +        layerTreeHost()->singleBufferedLayerWillDraw();
> +}

Why are these different functions? They appear to do exactly the same thing

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.h:157
> +    void invalidateSingleBufferedLayers();

it's strange to see a function called "invalidate..." that does not mark any layers as needing to be painted or drawn.  this function used to mean "I am about to invalidate a single buffered layer, so please block until it is safe to do so" - right?  Seems more like a "will...()" type function.  although if that's the case, how is this different from singleBuffered...WillDraw() ?

> Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.h:96
> +    enum SingleBufferedLayerState {
> +        SINGLE_BUFFERED_LAYER_STATE_COMMITTED,

WebKit style for enums is that values have CamelCaseNames, not MACRO_STYLE_NAMES

[WebKit Review Bot]

Comment on attachment 137388 [details]
Patch

Attachment 137388 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12410834

[Justin Novosad]

(In reply to comment #27)
> (From update of attachment 137388 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=137388&action=review
> 
> > Source/WebCore/platform/graphics/chromium/Canvas2DLayerChromium.cpp:170
> > +void Canvas2DLayerChromium::layerWillDraw() const
> > +{
> > +    if (m_deferralMode == NonDeferred && !m_useDoubleBuffering && CCProxy::hasImplThread() && layerTreeHost())
> > +        layerTreeHost()->singleBufferedLayerWillDraw();
> > +}
> > +
> > +void Canvas2DLayerChromium::layerWillDrawDeferred() const
> > +{
> > +    // Called before rendering previously recorded draw operations
> > +    ASSERT(m_deferralMode == Deferred);
> > +    if (!m_useDoubleBuffering && CCProxy::hasImplThread() && layerTreeHost())
> > +        layerTreeHost()->singleBufferedLayerWillDraw();
> > +}
> 
> Why are these different functions? They appear to do exactly the same thing

The only difference is the  m_deferralMode == NonDeferred condition.  That condition is essential, but I guess I could move it to the call site to make thing cleaner.

> 
> > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.h:157
> > +    void invalidateSingleBufferedLayers();
> 
> it's strange to see a function called "invalidate..." that does not mark any layers as needing to be painted or drawn.  this function used to mean "I am about to invalidate a single buffered layer, so please block until it is safe to do so" - right?  Seems more like a "will...()" type function.  although if that's the case, how is this different from singleBuffered...WillDraw() ?

How about calling it willReleaseSingleBufferedLayerTexture?
The difference with singleBufferedLayerWillDraw is that this new method will not block until the next compositor draw in the case where the compositor has an undrawn committed frame. 

The problem this solves: Deleting the layer's texture between commit and draw was causing a lost context (use of an invalid texture ID).  The solution I found is to send a message to the compositor thread to say "forget about what was committed before and wait until the next commit before drawing again".  Then we are free to delete the layer's texture in the main thread.

[James Robinson]

(In reply to comment #29)
> (In reply to comment #27)
> > (From update of attachment 137388 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=137388&action=review
> > 
> > > Source/WebCore/platform/graphics/chromium/Canvas2DLayerChromium.cpp:170
> > > +void Canvas2DLayerChromium::layerWillDraw() const
> > > +{
> > > +    if (m_deferralMode == NonDeferred && !m_useDoubleBuffering && CCProxy::hasImplThread() && layerTreeHost())
> > > +        layerTreeHost()->singleBufferedLayerWillDraw();
> > > +}
> > > +
> > > +void Canvas2DLayerChromium::layerWillDrawDeferred() const
> > > +{
> > > +    // Called before rendering previously recorded draw operations
> > > +    ASSERT(m_deferralMode == Deferred);
> > > +    if (!m_useDoubleBuffering && CCProxy::hasImplThread() && layerTreeHost())
> > > +        layerTreeHost()->singleBufferedLayerWillDraw();
> > > +}
> > 
> > Why are these different functions? They appear to do exactly the same thing
> 
> The only difference is the  m_deferralMode == NonDeferred condition.  That condition is essential, but I guess I could move it to the call site to make thing cleaner.

The caller has to know the deferral mode anyway to figure out which version to call, right?

> 
> > 
> > > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.h:157
> > > +    void invalidateSingleBufferedLayers();
> > 
> > it's strange to see a function called "invalidate..." that does not mark any layers as needing to be painted or drawn.  this function used to mean "I am about to invalidate a single buffered layer, so please block until it is safe to do so" - right?  Seems more like a "will...()" type function.  although if that's the case, how is this different from singleBuffered...WillDraw() ?
> 
> How about calling it willReleaseSingleBufferedLayerTexture?
> The difference with singleBufferedLayerWillDraw is that this new method will not block until the next compositor draw in the case where the compositor has an undrawn committed frame. 
> 
> The problem this solves: Deleting the layer's texture between commit and draw was causing a lost context (use of an invalid texture ID).  The solution I found is to send a message to the compositor thread to say "forget about what was committed before and wait until the next commit before drawing again".  Then we are free to delete the layer's texture in the main thread.

The texture ID problem seems roughly the same - the main thread is about to do an operation that changes the texture in some way (deleting or modifying its contents) and before it proceeds it needs a promise from the compositor thread that it won't touch it.

Perhaps my confusion is that this mechanism is being used for two things - flow control (blocking the main thread in some cases because it's getting too far ahead) and a exclusive resource management issue (making sure that the main thread doesn't manipulate resources that the compositor thread may be using).  Is that accurate?  If we were handling only the first condition, then it seems like we want something that looks a whole lot like a mutex on the single buffered canvas texture ID.

[Nat Duca]

To jamesr's point, it does beg the question "what is the general form of this?" We need a similar mechanism for some Aura flow control ("dont draw this layer now, the child compositor for this layer is busy updating the layer")

Since we talked about this patch before, we now have CCLTHI::prepareToDraw, which is like canDraw() but does a walk of the tree. Is there a from of this patch that does the mutex-ing on a per-layer level? I wonder if in that form, offscreen canvas would allow us to, e.g. smooth scroll, since the compositor would never "acquire" the drawing right.

Maybe we should do another VC now that you've gotten this patch along to this state. We're out of aura insanity mode, and I worry about you being churned by "we look at this every few days and forget the details each time" that happens with passive reviews.

[Justin Novosad]

> > > > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.h:157
> > > > +    void invalidateSingleBufferedLayers();
> > > 
> > > it's strange to see a function called "invalidate..." that does not mark any layers as needing to be painted or drawn.  this function used to mean "I am about to invalidate a single buffered layer, so please block until it is safe to do so" - right?  Seems more like a "will...()" type function.  although if that's the case, how is this different from singleBuffered...WillDraw() ?
> > 
> > How about calling it willReleaseSingleBufferedLayerTexture?
> > The difference with singleBufferedLayerWillDraw is that this new method will not block until the next compositor draw in the case where the compositor has an undrawn committed frame. 
> > 
> > The problem this solves: Deleting the layer's texture between commit and draw was causing a lost context (use of an invalid texture ID).  The solution I found is to send a message to the compositor thread to say "forget about what was committed before and wait until the next commit before drawing again".  Then we are free to delete the layer's texture in the main thread.
> 
> The texture ID problem seems roughly the same - the main thread is about to do an operation that changes the texture in some way (deleting or modifying its contents) and before it proceeds it needs a promise from the compositor thread that it won't touch it.
> 
Exactly.  The only difference is which thread gets priority when we are between a layer commit and a compositor draw. 

> Perhaps my confusion is that this mechanism is being used for two things - flow control (blocking the main thread in some cases because it's getting too far ahead) and a exclusive resource management issue (making sure that the main thread doesn't manipulate resources that the compositor thread may be using).  Is that accurate?  If we were handling only the first condition, then it seems like we want something that looks a whole lot like a mutex on the single buffered canvas texture ID.

I think having a mutex per texture would be overkill.  We don't want to begin a compositor draw unless we can acquired all the layers we need.  Therefore we'd end up doing a tryAcquire on each the mutexes in one block, and then releasing.  I don't see the benefit of that.  

Having just one mutex for all the single buffered layers would work well.  That is actually what I had done in my first iteration.  Nat convinced me that it was a better idea to use a postTask+completion event to communicate between the threads.  I can see both side to this argument. The current implementation, using a task round trip, puts most of the burden of the synchronisation on the main thread, since the state is local to the compositor thread.

[Justin Novosad]

(In reply to comment #31)
> To jamesr's point, it does beg the question "what is the general form of this?" We need a similar mechanism for some Aura flow control ("dont draw this layer now, the child compositor for this layer is busy updating the layer")

I am not sure I understand. Is there a use case for performing a compositor draw that excludes layers that are currently busy?
> 
> Since we talked about this patch before, we now have CCLTHI::prepareToDraw, which is like canDraw() but does a walk of the tree. Is there a from of this patch that does the mutex-ing on a per-layer level? I wonder if in that form, offscreen canvas would allow us to, e.g. smooth scroll, since the compositor would never "acquire" the drawing right.

We don't need to have per layer mutex-ing to solve that problem: we just need to add logic to CCLTH::singleBufferedLayerWillDraw to do nothing for a layer that is not being composited.  I see that as an incremental improvement that can be landed later.

> 
> Maybe we should do another VC now that you've gotten this patch along to this state. We're out of aura insanity mode, and I worry about you being churned by "we look at this every few days and forget the details each time" that happens with passive reviews.

Yes.  Also, right now, I would just like to get this into a minimal submitable state so that we can land further improvements incrementally, in smaller patches.

[Nat Duca]

(In reply to comment #33)

I dont feel that you're hearing me. Per-layer locks is the simple case. You're prematurely optimizing to make this global state when in fact, it really is a per-layer per-texture state in origination.

[Justin Novosad]

Created attachment 137576 [details]
Patch

[Justin Novosad]

(In reply to comment #35)
> Created an attachment (id=137576) [details]
> Patch

Applied review feedback.  Updated unit testing code to reflect changes

[Justin Novosad]

(In reply to comment #34)
> (In reply to comment #33)
> 
> I dont feel that you're hearing me. Per-layer locks is the simple case. You're prematurely optimizing to make this global state when in fact, it really is a per-layer per-texture state in origination.

Ok, I agree that per layer locks clearly express the problem I am trying to solve (avoid concurrent access to layer textures). However, I disagree that using a global state is a premature optimization.  Implementing per layer locks would be a more complex implementation.  Right now, the only time the impl and non-impl layers talk to each other is during the synchronization of the layer tree at commit time. Per layer locking would require an asynchronous channel between the two.  Possibly a helper class (CCLayerProxy?) that encapsulates the per layer state and synchronization methods.

[James Robinson]

(In reply to comment #32)
> > Perhaps my confusion is that this mechanism is being used for two things - flow control (blocking the main thread in some cases because it's getting too far ahead) and a exclusive resource management issue (making sure that the main thread doesn't manipulate resources that the compositor thread may be using).  Is that accurate?  If we were handling only the first condition, then it seems like we want something that looks a whole lot like a mutex on the single buffered canvas texture ID.
> 
> I think having a mutex per texture would be overkill.  We don't want to begin a compositor draw unless we can acquired all the layers we need.  Therefore we'd end up doing a tryAcquire on each the mutexes in one block, and then releasing.  I don't see the benefit of that.  
> 
> Having just one mutex for all the single buffered layers would work well.  That is actually what I had done in my first iteration.  Nat convinced me that it was a better idea to use a postTask+completion event to communicate between the threads.  I can see both side to this argument. The current implementation, using a task round trip, puts most of the burden of the synchronisation on the main thread, since the state is local to the compositor thread.


I'm talking about what you've logically implemented, not how it's done.  You are defining a set of mutual exclusion around a resource even though you aren't using a Mutex type to do it.

Anyway now that I think I understand it better I think that it might be clearer to express the LayerChromium<->CCLayerTreeHost relationship in terms of what the layer wants (exclusive access to either a layer texture or the whole scene), but I think that mostly boils down to renaming.  I find it pretty hard to follow all the logic and desired relationships in this patch but will give it a shot.

[James Robinson]

Comment on attachment 137576 [details]
Patch

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

[Justin Novosad]

Comment on attachment 137576 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=137576&action=review

>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.h:158
>> +    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

Gotcha.  The current names describe why the methods are called rather than what the methods do (blatant no-no on my part).  I could replace these with a single method with a force argument: acquireSingleBufferedLayerTextures(bool force). The meaning of 'force' would be: if there is a committed frame waiting to be drawn, acquire immediately (possibly drop a frame) rather than wait for the next draw to complete.

>> Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:339
>> +    // 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

The reasoning behind this is based on the code that was already in this method before my change. At line 351, we call m_schedulerOnImplThread->setNeedsRedraw() when visibility is being set to true.  Therefore, there is an assumption that the layer contents are not stale, or that the needsCommit bit is already set if the contents are stale.  I did not dig any deeper to understand the mechanism that enforces this or whether that logic is flawed.  Do you feel it needs to be revisited?

>> 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

Ok, I buy that, will move the state logic to the scheduler. To answer the first question: when the tab becomes invisible, a call to  m_schedulerOnImplThread->setNeedsCommit() is made (line 349).  The commit will result in the main thread releasing its lock on the textures.

>> 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

Agreed. As I suggested above, a single code path with a "force" argument could be used here.

>> 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

Perhaps I could get away with using CommitState for all the single buffering logic.  I think all that is missing is a SINGLE_BUFFERED_FRAME_IN_PROGRESS state (in lieu of SingleBufferedLayerStateDirty).  The semantics of that state would be the same as FRAME_IN_PROGRESS as far as state transition logic is concerned.  The only difference is that compositor draws are not allowed with SINGLE_BUFFERED_FRAME_IN_PROGRESS.  Sounds good?
BTW, how come those enum values are not in CamelCase?

>> Source/WebCore/platform/graphics/skia/ImageBufferSkia.cpp:126
>>          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

The image buffer takes ownership of the SkCanvas and holds a ref on the layer (via ImageBufferData), When the imageBuffer is destroyed, so is the canvas, but not necessarily the layer.  Therefore, it is possible for the layer to outlive the canvas, but not the opposite. The SkDeferredCanvas does take exclusive ownership of deviceContext.  Therefore, this code is safe without the ADC holding a ref on the layer.

You're right that setPlatformLayer layer is unnecessary, it is an artifact of an earlier iteration of the code. I'll get rid of it.

>> Source/WebCore/platform/graphics/skia/ImageBufferSkia.cpp:193
>> +        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

Yes, we are going to draw to the layer even if the canvas is deferred, but not immediately. The idea is to postpone the lock until commit time (where the deferred canvas playback happens).  Now were going in circles. I used to check  whether the the layer was deferred in the layer code, but you didn't like that (comment #27) because it meant having two quasi identical methods (with and without the deferral check).  I consolidated the two methods by moving the deferral check to the call site.  The other call site (in AcceleratedDeviceContext::prepareForDraw) needs to NOT have that check.

[Nat Duca]

(In reply to comment #40)
> Gotcha.  The current names describe why the methods are called rather than what the methods do (blatant no-no on my part).  I could replace these with a single method with a force argument: acquireSingleBufferedLayerTextures(bool force). The meaning of 'force' would be: if there is a committed frame waiting to be drawn, acquire immediately (possibly drop a frame) rather than wait for the next draw to complete.

I'm confused what the purpose of the force argument is. This is on the proxy? I thought we decided that if this code was in the scheduler, it would be able to use the commit state being != waiting_for_first_draw to decide whether to acquire immediately or wait for a draw.

> > this feels dodgy - if you're rapidly switching tabs you still can't draw a bogus frame
> 
> The reasoning behind this is based on the code that was already in this method before my change. At line 351, we call m_schedulerOnImplThread->setNeedsRedraw() when visibility is being set to true.  Therefore, there is an assumption that the layer contents are not stale, or that the needsCommit bit is already set if the contents are stale.  I did not dig any deeper to understand the mechanism that enforces this or whether that logic is flawed.  Do you feel it needs to be revisited?

Probably best to fully understand this. You're messing with the scheduling innards and we need more experts here. :)

> Agreed. As I suggested above, a single code path with a "force" argument could be used here.

My comments before point out my confusion over why force logic isn't sealed inside the scheduler state machine.

[Justin Novosad]

(In reply to comment #41)
> (In reply to comment #40)
> > Gotcha.  The current names describe why the methods are called rather than what the methods do (blatant no-no on my part).  I could replace these with a single method with a force argument: acquireSingleBufferedLayerTextures(bool force). The meaning of 'force' would be: if there is a committed frame waiting to be drawn, acquire immediately (possibly drop a frame) rather than wait for the next draw to complete.
> 
> I'm confused what the purpose of the force argument is. This is on the proxy? I thought we decided that if this code was in the scheduler, it would be able to use the commit state being != waiting_for_first_draw to decide whether to acquire immediately or wait for a draw.
> 

The commit state is not enough. The behavior depends on the reason for which we are acquiring the lock (expressed through 'force'). If waiting_for_first_draw && !force, the main thread blocks until next draw. If waiting_for_first_draw && force, then the state goes immediately to single_buffered_layer_frame_in_progress, and the previously committed frame is dropped.

The purpose of this is to avoid deadlocking when the canvas is resized.
Without the force argument, the state machine has no idea whether the main thread is acquiring the lock in order to draw to the texture or because it intends to delete the texture. There are other ways to resolve the deadlock on resize, but I wanted to avoid introducing an unnecessary frame of latency.

> > > this feels dodgy - if you're rapidly switching tabs you still can't draw a bogus frame
> > 
> > The reasoning behind this is based on the code that was already in this method before my change. At line 351, we call m_schedulerOnImplThread->setNeedsRedraw() when visibility is being set to true.  Therefore, there is an assumption that the layer contents are not stale, or that the needsCommit bit is already set if the contents are stale.  I did not dig any deeper to understand the mechanism that enforces this or whether that logic is flawed.  Do you feel it needs to be revisited?
> 
> Probably best to fully understand this. You're messing with the scheduling innards and we need more experts here. :)

Ok, I will do my homework here...

[Nat Duca]

(In reply to comment #42)
> > I'm confused what the purpose of the force argument is. This is on the proxy? I thought we decided that if this code was in the scheduler, it would be able to use the commit state being != waiting_for_first_draw to decide whether to acquire immediately or wait for a draw.
> > 
> 
> The commit state is not enough. The behavior depends on the reason for which we are acquiring the lock (expressed through 'force'). If waiting_for_first_draw && !force, the main thread blocks until next draw. If waiting_for_first_draw && force, then the state goes immediately to single_buffered_layer_frame_in_progress, and the previously committed frame is dropped.
> 
> The purpose of this is to avoid deadlocking when the canvas is resized.
> Without the force argument, the state machine has no idea whether the main thread is acquiring the lock in order to draw to the texture or because it intends to delete the texture. There are other ways to resolve the deadlock on resize, but I wanted to avoid introducing an unnecessary frame of latency.

This is helpful clarification, thanks.

Is this a deadlock-on-resize, or just "unneded delay on resize?"

We tend to use the word "force" for deadlock breaking. If this is just a "i need this and dont wait for a draw" then maybe just say that as the argument. "urgent" comes to mind, or "preferNotToWaitUntilDraw."

/me wonders out loud if someone could starve the drawing thread by resizing aggressively? Nbd tbqh.

[Justin Novosad]

(In reply to comment #43)

> Is this a deadlock-on-resize, or just "unneded delay on resize?"

The simple fact of acquiring the texture before doing the resize is enough to resolve the deadlock (and lost context).  "force" just to prevent an unnecessary delay.
> 
> We tend to use the word "force" for deadlock breaking. If this is just a "i need this and dont wait for a draw" then maybe just say that as the argument. "urgent" comes to mind, or "preferNotToWaitUntilDraw."

Fair enough. I like preferNotToWaitUntilDraw

> 
> /me wonders out loud if someone could starve the drawing thread by resizing aggressively? Nbd tbqh.

Because the message loop processes event in the order they are received (no shuffling), you can't have more that one additional frame of latency (I think).  For example, if the main thread accumulates a backlog of window resize events, it will be able to batch them without blocking immediately after a compositor draw completes.  We would have a bigger problem if the main thread had a scheduler that gave priority RAF and setTimeout events, becaus that would preempt the batching.

[James Robinson]

(In reply to comment #40)
> (From update of attachment 137576 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=137576&action=review
> 
> >> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.h:158
> >> +    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
> 
> Gotcha.  The current names describe why the methods are called rather than what the methods do (blatant no-no on my part).  I could replace these with a single method with a force argument: acquireSingleBufferedLayerTextures(bool force). The meaning of 'force' would be: if there is a committed frame waiting to be drawn, acquire immediately (possibly drop a frame) rather than wait for the next draw to complete.
> 


You still haven't explained *WHY* you would want different behavior for the deleting vs altering texture case.  Why would you not want to wait for a frame?  If the reason to wait for a frame in the about-to-modify case is to avoid livelocking the compositor thread, why doesn't that apply to deleting a layer as well?  It's not hard at all for me to imagine a page that adds/remove <canvas> elements to the DOM every frame that could easily livelock the compositor if you don't force it to wait.

[James Robinson]

I would like to see a version of this patch with the logic in the scheduler.  I think that if you simply drop the "force" parameter we'll be in good shape to go here.

(In reply to comment #40)
> >> Source/WebCore/platform/graphics/skia/ImageBufferSkia.cpp:193
> >> +        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
> 
> Yes, we are going to draw to the layer even if the canvas is deferred, but not immediately. The idea is to postpone the lock until commit time (where the deferred canvas playback happens).  Now were going in circles. I used to check  whether the the layer was deferred in the layer code, but you didn't like that (comment #27) because it meant having two quasi identical methods (with and without the deferral check).  I consolidated the two methods by moving the deferral check to the call site.  The other call site (in AcceleratedDeviceContext::prepareForDraw) needs to NOT have that check.

I think the two functions and calls should be the same - layerWillDraw().  The layer already knows if it's in deferred mode or not - see Canvas2DLayerChromium::m_deferralMode - so it can do whatever branching it needs to do internally to map to the right compositor functions.  You should not have to create two functions on Canvas2DLayerChromium, that would be rather silly.

[Justin Novosad]

(In reply to comment #45)

> You still haven't explained *WHY* you would want different behavior for the deleting vs altering texture case.  Why would you not want to wait for a frame?  If the reason to wait for a frame in the about-to-modify case is to avoid livelocking the compositor thread, why doesn't that apply to deleting a layer as well?  It's not hard at all for me to imagine a page that adds/remove <canvas> elements to the DOM every frame that could easily livelock the compositor if you don't force it to wait.

I think we don't need to discuss the *WHY* because the argument you just made about livelocking the compositor trumps it. Thanks for that!

[Justin Novosad]

(In reply to comment #46)
> I would like to see a version of this patch with the logic in the scheduler.  I think that if you simply drop the "force" parameter we'll be in good shape to go here.
> 
> (In reply to comment #40)
> > >> Source/WebCore/platform/graphics/skia/ImageBufferSkia.cpp:193
> > >> +        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
> > 
> > Yes, we are going to draw to the layer even if the canvas is deferred, but not immediately. The idea is to postpone the lock until commit time (where the deferred canvas playback happens).  Now were going in circles. I used to check  whether the the layer was deferred in the layer code, but you didn't like that (comment #27) because it meant having two quasi identical methods (with and without the deferral check).  I consolidated the two methods by moving the deferral check to the call site.  The other call site (in AcceleratedDeviceContext::prepareForDraw) needs to NOT have that check.
> 
> I think the two functions and calls should be the same - layerWillDraw().  The layer already knows if it's in deferred mode or not - see Canvas2DLayerChromium::m_deferralMode - so it can do whatever branching it needs to do internally to map to the right compositor functions.  You should not have to create two functions on Canvas2DLayerChromium, that would be rather silly.

No, that doesn't work. One of the call sites is used for acquiring the texture when the canvas is deferred, and the other call site for when the canvas is not deferred.
We want to acquire the texture only when we are about to modify it.  In AcceleratedDeviceContext::prepareForDraw, which is only called on deferred canvases, we know for sure that we are about to modify the texture (canvas is deferred, but we don<t event need to check). 
On the other hand, in ImageBuffer::context() we are only about to modify the texture when rendering is NOT deferred.

[Justin Novosad]

Created attachment 138168 [details]
Patch

[Justin Novosad]

(In reply to comment #49)
> Created an attachment (id=138168) [details]
> Patch

The latest patch is WIP (no ChangeLog, no test code) I think it addresses all the outstanding review comments, but it may raise new ones.  

I put all the state management logic in the scheduler state machine.  This actually made things much clearer, and the resolution of deadlocking races is a lot less esoteric than in previous candidate patches. The deadlocks that happened during tab switching are no longer treated through setVisible. Instead, the state machine explicitly detects deadlocking states and resolved them.  There were two problematic races that were deadlocking: a stale lock by the main thread, and a stale lock by the impl thread, both can be caused by different race conditions when the compositor loses or regains visibility. A stale lock by the impl thread is detected when TEXTURE_STATE_ACQUIRED_BY_IMPL_THREAD and no redraw is currently scheduled.  That condition is detected and resolved in CCSchedulerStateMachine::shouldAcquireTexturesForMainThread(). The other race condition hapened when COMMIT_STATE_WAITING_FOR_FIRST_DRAW and TEXTURE_STATE_ACQUIRED_BY_MAIN_THREAD (which prevents the compositor from drawing). This impass is resolved by scheduling an ACTION_BEGIN_FRAME, which makes the main thread re-commit, and therefore release the textures.

I re-thought the layerWillDraw thing and re-implemented it in a way I think is clearer and still non-redundant.

I am getting warmer aren't I?

[WebKit Review Bot]

Attachment 138168 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 1 in 14 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Nat Duca]

Comment on attachment 138168 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=138168&action=review

Bikeshedding follows. That's a good sign that I'm done being grumpy. :) This is really nice, Justin.

> Source/WebCore/platform/graphics/chromium/cc/CCSchedulerStateMachine.cpp:78
> +bool CCSchedulerStateMachine::shouldAcquireTexturesForMainThread() const

Worries about how similar this looks to shouldDraw(). Any way to reduce duplication? Imagine someone tweaking canDraw rules and them forgetting to tweak this.

Me wonders if we need to take these two functions and break them (plus the hairy bit of code I point at below) it into a few different accessors that combine more obviously.

> Source/WebCore/platform/graphics/chromium/cc/CCSchedulerStateMachine.cpp:138
> +        if ((!m_canDraw || m_textureState == TEXTURE_STATE_ACQUIRED_BY_MAIN_THREAD) && m_needsCommit && (m_visible || m_needsForcedCommit))

This is getting a bit hairy. Might break into a few temp vars to make more gooder.

> Source/WebCore/platform/graphics/chromium/cc/CCSchedulerStateMachine.h:131
> +    void setNeedsAcquireTexturesForMainThread();

setMainThreadNeedsSingleBufferedTextures?

> Source/WebCore/platform/graphics/chromium/cc/CCSingleThreadProxy.h:64
> +    virtual void acquireTextures() OVERRIDE { }

Bikeshedder says this might get easily confused with some of @reveman's texture acquisition logic for SHM copy painting. Might want to keep singleBufferedTextures in the pertinent method names.

[WebKit Review Bot]

Comment on attachment 138168 [details]
Patch

Attachment 138168 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12482288

[Justin Novosad]

(In reply to comment #52)
> Bikeshedder says this might get easily confused with some of @reveman's texture acquisition logic for SHM copy painting. Might want to keep singleBufferedTextures in the pertinent method names.

FWIW, I got rid of the "SingleBufferedLayer" in a lot of places after writing a method named acquireSingleBufferedLayerTexturesForMainThreadOnImplThread, which I thought was a ridiculously verbose name.  Also, actual single buffering behavior is not implemented in core compositor code; so as far as the compositor is concerned, we are just locking it's layers' textures.  The only place where we are explicitly implementing single/double buffering is in Canvas2DLayerChromium, which is probably the only place where we should see those terms. For compositor code, I propose a compromise: how about we use "LayerTextures".  For example: setMainThreadNeedsLayerTextures.

[Nat Duca]

(In reply to comment #54)

What's wrong with acquireSingleBufferedTextures? The only place this felt scary was on the proxy.

[James Robinson]

Comment on attachment 138168 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=138168&action=review

Seems pretty good.  Before landing you need to update the unit test (and corresponding ChangeLog line) and fix up a few things around m_useDoubleBuffering

> Source/WebCore/ChangeLog:8
> +        No new tests. (OOPS!)

this won't blend, and I don't think it is entirely accurate.

you need to update Canvas2DLayerChromiumTests.cpp to cover these new cases, but I suspect you already knew that

> Source/WebCore/platform/graphics/chromium/Canvas2DLayerChromium.cpp:64
> +    setUseDoubleBuffering(false);

did you mean to set useDoubleBuffering to true in some cases (like maybe implThread = true and non-deferred mode)? it looks like it's just dead code here currently.

> Source/WebCore/platform/graphics/chromium/Canvas2DLayerChromium.cpp:74
> +void Canvas2DLayerChromium::setUseDoubleBuffering(bool useDoubleBuffering)

i don't understand why this is a public setter. it appears to only be called from the constructor, if so then it should be folded in to the c'tor

trying to change this at runtime seems difficult

> Source/WebCore/platform/graphics/chromium/Canvas2DLayerChromium.cpp:90
> +        return;

nit: newline after this return

> Source/WebCore/platform/graphics/chromium/Canvas2DLayerChromium.cpp:91
> +    if (m_backTextureId && !m_useDoubleBuffering && CCProxy::hasImplThread() && layerTreeHost()) {

is the hasImplThread() check necessary here? when would m_useDoubleBuffering be true in the single-threaded case?

do we have to worry about lost context here?

> Source/WebCore/platform/graphics/chromium/Canvas2DLayerChromium.cpp:160
> +    if (!m_useDoubleBuffering && CCProxy::hasImplThread() && layerTreeHost()) {

again, when would m_useDoubleBuffering be true and CCProxy::hasImplThread() be false?

> Source/WebCore/platform/graphics/skia/ImageBufferSkia.cpp:55
> +#include "cc/CCLayerTreeHost.h"

what is this #include for? please remove if it's not being used

[Justin Novosad]

Created attachment 138648 [details]
Patch

[Justin Novosad]

Comment on attachment 138648 [details]
Patch

Oops, I missed comment #56. New patch on the way...  abort review.

[James Robinson]

Comment on attachment 138648 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=138648&action=review

> Source/WebKit/chromium/tests/Canvas2DLayerChromiumTest.cpp:147
> +        canvas->setUseDoubleBuffering(doubleBuffered);

if this is going to only be set in tests then just kill it. no need to have (or test) code that we won't use

[Justin Novosad]

(In reply to comment #59)
> (From update of attachment 138648 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=138648&action=review
> 
> > Source/WebKit/chromium/tests/Canvas2DLayerChromiumTest.cpp:147
> > +        canvas->setUseDoubleBuffering(doubleBuffered);
> 
> if this is going to only be set in tests then just kill it. no need to have (or test) code that we won't use

I have plans to use it very soon in a follow-up patch.

[Justin Novosad]

(In reply to comment #56)

> > Source/WebCore/platform/graphics/chromium/Canvas2DLayerChromium.cpp:64
> > +    setUseDoubleBuffering(false);
> 
> did you mean to set useDoubleBuffering to true in some cases 

Yes. That will be the next thing I work on.

> 
> > Source/WebCore/platform/graphics/chromium/Canvas2DLayerChromium.cpp:91
> > +    if (m_backTextureId && !m_useDoubleBuffering && CCProxy::hasImplThread() && layerTreeHost()) {
> 
> is the hasImplThread() check necessary here? when would m_useDoubleBuffering be true in the single-threaded case?
> 
> do we have to worry about lost context here? 

I think you missed the "!" in front of m_useDoubleBuffering. The case I am handling here is single buffered canvas with threaded compositing, so both checks are necessary. Single buffered can be with threaded or non threaded compositing, and threaded compositing can (now) be with single or double buffered canvases.

[Justin Novosad]

Created attachment 138809 [details]
Patch

[James Robinson]

(In reply to comment #60)
> (In reply to comment #59)
> > (From update of attachment 138648 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=138648&action=review
> > 
> > > Source/WebKit/chromium/tests/Canvas2DLayerChromiumTest.cpp:147
> > > +        canvas->setUseDoubleBuffering(doubleBuffered);
> > 
> > if this is going to only be set in tests then just kill it. no need to have (or test) code that we won't use
> 
> I have plans to use it very soon in a follow-up patch.

This is not how we do things.  If you're making code dead in this patch, delete it in this patch.  If you want to add it back in a future patch, add it back then.  Having dead code in the tree is bad practice.  We have version control and bugzilla patches to store old code, the code in the tree should only be the code that runs.

[James Robinson]

Comment on attachment 138809 [details]
Patch

R- for dead code.  I can't tell what is actually going on this patch with logic that will never be hit.

[Justin Novosad]

Created attachment 138849 [details]
Patch

[Justin Novosad]

(In reply to comment #65)
> Created an attachment (id=138849) [details]
> Patch

In the latest patch I am preventing the double buffering implementation from becoming dead code.  Until a better heuristic is implemented, the choice of single vs. double buffering will be driven by whether or not canvas rendering is deferred (as was suggested in comment #56).  I also fixed a build issue with unit test Canvas2DLayerChromiumTest (friend/scope problem).

[WebKit Review Bot]

Comment on attachment 138849 [details]
Patch

Attachment 138849 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12528745

[James Robinson]

Comment on attachment 138849 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=138849&action=review

This looks pretty good so far (modulo inline comments). I'll go through the scheduler changes in more detail tonight and may have more feedback but the shape of it looks right.

> Source/WebCore/ChangeLog:20
> +        Disable double buffering and rate limiting on accelerated canvas
> +        layers when the chromium threaded compositor is used.

i think this is outdated - can you please update to reflect what the current patch does?

> Source/WebCore/platform/graphics/chromium/Canvas2DLayerChromium.cpp:68
> +    setUseDoubleBuffering(deferralMode == NonDeferred);

does this mean we will start double-buffering in single-threaded mode when not using deferred rendering? why - it seems like that would be a memory regression for no benefit.

> Source/WebCore/platform/graphics/chromium/Canvas2DLayerChromium.cpp:78
> +void Canvas2DLayerChromium::setUseDoubleBuffering(bool useDoubleBuffering)

i'd prefer to have the logic in here in the constructor, just to make it crystal clear that we decide on our double buffering state (and thus rate limiting state) at creation time and we don't support trying to change it later down the line. if you do this, you can simplify some of the logic in here as well

> Source/WebCore/platform/graphics/chromium/Canvas2DLayerChromium.cpp:156
> +    if (m_canvas && m_deferralMode == Deferred) {
>          TRACE_EVENT0("cc", "SkCanvas::flush");
>          m_canvas->flush();

is it OK to remove this flush? i thought that even "non-deferred" canvas does some internal buffering

> Source/WebCore/platform/graphics/chromium/Canvas2DLayerChromium.cpp:165
> +    if (!m_useDoubleBuffering && CCProxy::hasImplThread() && layerTreeHost()) {

i think it'd be handy to collapse these set of 3 checks into a private helper (since it's repeated on line 96) - maybe something like "frontBufferInUse" or something of that nature?

[James Robinson]

Also, could you please make the unit tests compile just so we can get a clean EWS run?

[James Robinson]

Comment on attachment 138849 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=138849&action=review

The scheduler changes look good.

R=me but please address the issues I've pointed out in this comment and the ones previous and fix the build before landing.

> Source/WebCore/platform/graphics/chromium/Canvas2DLayerChromium.cpp:74
>      if (m_context && layerTreeHost())

think you want to check for m_useRateLimiter too here, no?

> Source/WebKit/chromium/tests/Canvas2DLayerChromiumTest.cpp:100
> +    void fullLifecycleTest(bool threaded, bool deferred, bool doubleBuffered)

we should test the configurations that we care about - some of the configs here just don't make any sense (like single-threaded and double-buffered). let the layer decide if it wants to double-buffer or not based on the other settings

[Justin Novosad]

(In reply to comment #68)
> > Source/WebCore/platform/graphics/chromium/Canvas2DLayerChromium.cpp:68
> > +    setUseDoubleBuffering(deferralMode == NonDeferred);
> 
> does this mean we will start double-buffering in single-threaded mode when not using deferred rendering? why - it seems like that would be a memory regression for no benefit.

Good catch. This was going trigger an assertion. I'll make sure to try a debug build before submitting another patch.


> > Source/WebCore/platform/graphics/chromium/Canvas2DLayerChromium.cpp:156
> > +    if (m_canvas && m_deferralMode == Deferred) {
> >          TRACE_EVENT0("cc", "SkCanvas::flush");
> >          m_canvas->flush();
> 
> is it OK to remove this flush? i thought that even "non-deferred" canvas does some internal buffering

The internal buffering of non-deffered canvas is flushed When we flush the GraphicsContext3D, which flushes its associated GrContext.  The above flush is still needed for deferred canvases.

[James Robinson]

My understanding (please correct me if I'm wrong) is that in non-deferred mode the GrContext may buffer some commands and not send them to the underlying GL context unless you called flush.  If this is not the case, then why did we have a GrContext flush() call in this code before deferred mode was added?

[Justin Novosad]

Created attachment 139029 [details]
Patch

[Justin Novosad]

(In reply to comment #72)
> My understanding (please correct me if I'm wrong) is that in non-deferred mode the GrContext may buffer some commands and not send them to the underlying GL context unless you called flush.  If this is not the case, then why did we have a GrContext flush() call in this code before deferred mode was added?

There was redundant flushing before. Flushing the the GraphicsContext3D takes care of flushing the GrContext.  Flushing the SkCanvas also flushes the GrContext.  

In the deferred canvas case, we need bot the canvas and the graphics context to be flushed, so there is still redundant flushing of the GrContext in that case.

[Justin Novosad]

(In reply to comment #70)
 
> R=me but please address the issues I've pointed out in this comment and the ones previous and fix the build before landing.

Done.  But I am not yet a committer, so I still have to land through the queue...

[James Robinson]

Comment on attachment 139029 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=139029&action=review

R=me. I'll run through the tests locally and land this by hand (assuming that deleting the ASSERT(0) is correct).

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

I think this is bogus - if the front texture couldn't be reserved, then we want to push a 0 texture here (so we don't generate a quad and attempt to draw).  Is this just for local testing? I can remove before landing.

[James Robinson]

This appears to break several layout tests in platform/chromium/virtual/gpu/fast/canvas/, have you seen this Justin?  The canvas seems to simply not show up.

I'll try to investigate locally.

[Justin Novosad]

(In reply to comment #76)
> (From update of attachment 139029 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=139029&action=review
> 
> R=me. I'll run through the tests locally and land this by hand (assuming that deleting the ASSERT(0) is correct).
> 
> > Source/WebCore/platform/graphics/chromium/Canvas2DLayerChromium.cpp:176
> > +            ASSERT(0);
> 
> I think this is bogus - if the front texture couldn't be reserved, then we want to push a 0 texture here (so we don't generate a quad and attempt to draw).  Is this just for local testing? I can remove before landing.

Yeah, the assert is definitely bogus, my bad.

[Justin Novosad]

(In reply to comment #77)
> This appears to break several layout tests in platform/chromium/virtual/gpu/fast/canvas/, have you seen this Justin?  The canvas seems to simply not show up.
> 
> I'll try to investigate locally.

Does this mean there is a regression with non-threaded compositing?  I will investigate too.

[James Robinson]

As I suspected it's the missing m_canvas->flush() call.  You have to make that call even in non-deferred mode or these tests fail:

platform/chromium/virtual/gpu/fast/canvas/canvas-text-alignment.html
+platform/chromium/virtual/gpu/fast/canvas/canvas-text-baseline.html
+platform/chromium/virtual/gpu/fast/canvas/canvas-transforms-during-path.html
+platform/chromium/virtual/gpu/fast/canvas/fill-stroke-clip-reset-path.html
–platform/chromium/virtual/gpu/fast/canvas/font-update.html

[James Robinson]

Committed r115355: <http://trac.webkit.org/changeset/115355>