Bug 97918 – [Chromium] Smoother animation for non-RAF 2D canvas animations

• Component: New Bugs
• Status: RESOLVED FIXED

[Justin Novosad]

[Chromium] Smoother animation for non-RAF 2D canvas animations

[Justin Novosad]

Created attachment 166277 [details]
Patch

[Stephen White]

Comment on attachment 166277 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/Canvas2DLayerBridge.cpp:139
> +    m_flushNeeded = false;
> +    switch (m_state) {
> +    case FreshState:
> +        if (m_didRecordDrawCommand && !isFreshFrame())
> +            m_state = StaleState;
> +        break;
> +    case StaleState:
> +        if (m_didRecordDrawCommand) {
> +            if (isFreshFrame())
> +                m_state = FreshState;
> +            else {
> +                if (!m_didFlushSinceLastRecordedFrame)
> +                    m_flushNeeded = true;
> +            }
> +        }
> +        break;
> +    }
> +    if (m_didRecordDrawCommand)
> +        m_didFlushSinceLastRecordedFrame = false;
> +    m_didRecordDrawCommand = false;

As discussed, I think this logic would be a lot clearer if we replaced isFreshFrame() with a "skippedPendingCommands()" notification from the deferred canvas.

Then we could simply have a counter on the bridge which counts the frames-since-last-flush, which is reset to zero on flushedPendingCommands() or skippedPendingCommands(), and we don't need this state machine at all.

> Source/WebCore/platform/graphics/chromium/Canvas2DLayerManager.cpp:89
> +        for (Canvas2DLayerBridge* layer = m_layerList.head(); layer; layer = layer->next())
> +            layer->flush();

As discussed, I think we can get away with only flushing the one layer, since the rest of the layers will be taken care of at commit time.

[Justin Novosad]

Created attachment 166746 [details]
Patch

[Stephen White]

Comment on attachment 166746 [details]
Patch

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

Thanks, this seems much cleaner to me.  Just some naming and we're done, I think.

> Source/WebCore/platform/graphics/chromium/Canvas2DLayerManager.cpp:82
> +        layer->didDrawFrame();

<bikeshed> Perhaps didDrawFrame() is misnamed?  It seems to be called regardless of whether that particular canvas drew anything or not. Unless we mean a compositor frame, in which case maybe it should be didDrawCompositorFrame()? </bikeshed>

[Justin Novosad]

(In reply to comment #4)
> (From update of attachment 166746 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=166746&action=review
> 
> Thanks, this seems much cleaner to me.  Just some naming and we're done, I think.
> 
> > Source/WebCore/platform/graphics/chromium/Canvas2DLayerManager.cpp:82
> > +        layer->didDrawFrame();
> 
> <bikeshed> Perhaps didDrawFrame() is misnamed?  It seems to be called regardless of whether that particular canvas drew anything or not. Unless we mean a compositor frame, in which case maybe it should be didDrawCompositorFrame()? </bikeshed>

This does not refer to a compositor frame, but to a frame in the atomic JS execution sense.  Since we are just propagating the task observer signal, we could just use the same name as the observer interface: didProcessTask, The it would be up to the Canvas2DLayerBridge to figure out whether the task drew a new canvas frame.

[Stephen White]

Comment on attachment 166746 [details]
Patch

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

>>> Source/WebCore/platform/graphics/chromium/Canvas2DLayerManager.cpp:82
>>> +        layer->didDrawFrame();
>> 
>> <bikeshed> Perhaps didDrawFrame() is misnamed?  It seems to be called regardless of whether that particular canvas drew anything or not. Unless we mean a compositor frame, in which case maybe it should be didDrawCompositorFrame()? </bikeshed>
> 
> This does not refer to a compositor frame, but to a frame in the atomic JS execution sense.  Since we are just propagating the task observer signal, we could just use the same name as the observer interface: didProcessTask, The it would be up to the Canvas2DLayerBridge to figure out whether the task drew a new canvas frame.

You're right; it's not a compositor frame.    didProcessTask() is a bit generic, though, and I usually prefer to name things by what they do, rather than when they're called.  Maybe flushFrameBacklog()?  rateLimit()?  limitPendingFrames()?  flushExcessiveFrames()?

> Source/WebCore/platform/graphics/chromium/Canvas2DLayerManager.cpp:96
> +        m_deferredFrameStarted = true;

More bikeshedding, just 'cause I'm annoying that way:  m_taskObserverStarted?

[Justin Novosad]

Created attachment 166922 [details]
Patch

[Stephen White]

Comment on attachment 166922 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/Canvas2DLayerBridge.cpp:139
>      storageAllocatedForRecordingChanged(deferredCanvas()->storageAllocatedForRecording());

As discussed, let's mock storageAllocatedForRecordingChanged() itself, instead of creating didFlushPendingCommands() for mocking purposes.

[Justin Novosad]

Created attachment 166952 [details]
Patch

[Stephen White]

Comment on attachment 166952 [details]
Patch

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

> Source/WebKit/chromium/tests/Canvas2DLayerManagerTest.cpp:88
> +    size_t m_fakeStorageAllocatedForRecording;

This member variable seems to be unused.

[Justin Novosad]

Created attachment 167105 [details]
Patch

[Stephen White]

Comment on attachment 167105 [details]
Patch

Looks good.  Looks like the patch has conflicts, but I trust you to fix them.  :)  r=me

[Justin Novosad]

Created attachment 167116 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 167116 [details]
Patch for landing

Rejecting attachment 167116 [details] from commit-queue.

New failing tests:
Canvas2DLayerManagerTest.testDeferredFrame
Full output: http://queues.webkit.org/results/14183038

[Justin Novosad]

Created attachment 168067 [details]
Patch

[Justin Novosad]

Comment on attachment 168067 [details]
Patch

Fixed release build test failure caused by uninitialized m_taskObserverActive in Canvas2DLayerManager

[Stephen White]

Comment on attachment 168067 [details]
Patch

Looks good (bots willing).  r=me

[WebKit Review Bot]

Comment on attachment 168067 [details]
Patch

Clearing flags on attachment: 168067

Committed r130994: <http://trac.webkit.org/changeset/130994>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.