[Chromium] Smoother animation for non-RAF 2D canvas animations
Created attachment 166277 [details] Patch
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.
Created attachment 166746 [details] Patch
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>
(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.
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?
Created attachment 166922 [details] Patch
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.
Created attachment 166952 [details] Patch
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.
Created attachment 167105 [details] Patch
Comment on attachment 167105 [details] Patch Looks good. Looks like the patch has conflicts, but I trust you to fix them. :) r=me
Created attachment 167116 [details] Patch for landing
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
Created attachment 168067 [details] Patch
Comment on attachment 168067 [details] Patch Fixed release build test failure caused by uninitialized m_taskObserverActive in Canvas2DLayerManager
Comment on attachment 168067 [details] Patch Looks good (bots willing). r=me
Comment on attachment 168067 [details] Patch Clearing flags on attachment: 168067 Committed r130994: <http://trac.webkit.org/changeset/130994>
All reviewed patches have been landed. Closing bug.