Simplify rate limiter logic in Canvas2DLayerBridge

third_party/WebKit/Source/platform/graphics/Canvas2DLayerBridge.cpp

Index: third_party/WebKit/Source/platform/graphics/Canvas2DLayerBridge.cpp
diff --git a/third_party/WebKit/Source/platform/graphics/Canvas2DLayerBridge.cpp b/third_party/WebKit/Source/platform/graphics/Canvas2DLayerBridge.cpp
index cbd9476bffa47fd08693026f2fe424ea4a0b61f2..aeee334a13ee7af2abde6132aa83e3cafd09f4e9 100644
--- a/third_party/WebKit/Source/platform/graphics/Canvas2DLayerBridge.cpp
+++ b/third_party/WebKit/Source/platform/graphics/Canvas2DLayerBridge.cpp
@@ -141,8 +141,8 @@ Canvas2DLayerBridge::Canvas2DLayerBridge(
       m_filterQuality(kLow_SkFilterQuality),
       m_isHidden(false),
       m_isDeferralEnabled(true),
-      m_isRegisteredTaskObserver(false),
-      m_renderingTaskCompletedForCurrentFrame(false),
+      m_previousFrameWasPresented(

[xlai (Olivia), 2017/02/17 19:39:53]

Will it be better to rename this as "m_nextFrameNeedsToWait" and initialize it
to "false"? Or perhaps other better name.

[Justin Novosad, 2017/02/21 15:49:26]

Acknowledged.

+          true),  // true to prevent rate limiting at startup
       m_softwareRenderingWhileHidden(false),
       m_lastImageId(0),
       m_lastFilter(GL_LINEAR),
@@ -497,7 +497,7 @@ void Canvas2DLayerBridge::hibernate() {
     return;
   }
 
-  TRACE_EVENT0("cc", "Canvas2DLayerBridge::hibernate");
+  TRACE_EVENT0("blink", "Canvas2DLayerBridge::hibernate");
   sk_sp<PaintSurface> tempHibernationSurface =
       PaintSurface::MakeRasterN32Premul(m_size.width(), m_size.height());
   if (!tempHibernationSurface) {
@@ -669,8 +669,6 @@ void Canvas2DLayerBridge::beginDestruction() {
   setIsHidden(true);
   m_surface.reset();
 
-  unregisterTaskObserver();
-
   if (m_layer && m_accelerationMode != DisableAcceleration) {
     GraphicsLayer::unregisterContentsLayer(m_layer->layer());
     m_layer->clearTexture();
@@ -683,13 +681,6 @@ void Canvas2DLayerBridge::beginDestruction() {
   DCHECK(!m_bytesAllocated);
 }
 
-void Canvas2DLayerBridge::unregisterTaskObserver() {
-  if (m_isRegisteredTaskObserver) {
-    Platform::current()->currentThread()->removeTaskObserver(this);
-    m_isRegisteredTaskObserver = false;
-  }
-}
-
 void Canvas2DLayerBridge::setFilterQuality(SkFilterQuality filterQuality) {
   DCHECK(!m_destructionInProgress);
   m_filterQuality = filterQuality;
@@ -772,7 +763,6 @@ void Canvas2DLayerBridge::skipQueuedDrawCommands() {
   }
 
   if (m_isDeferralEnabled) {
-    unregisterTaskObserver();
     if (m_rateLimiter)
       m_rateLimiter->reset();
   }
@@ -900,6 +890,11 @@ bool Canvas2DLayerBridge::PrepareTextureMailbox(
     return false;
   }
 
+  m_previousFrameWasPresented = true;
+  if (m_rateLimiter) {
+    m_rateLimiter->reset();

[xlai (Olivia), 2017/02/17 19:39:53]

Are you sure that this is the only place when the animation is consumed and
committed to the compositor?

[Justin Novosad, 2017/02/21 15:49:26]

For non-offscreen gpu-accelerated 2D canvases, it is.

+  }
+
   // If the context is lost, we don't know if we should be producing GPU or
   // software frames, until we get a new context, since the compositor will
   // be trying to get a new context and may change modes.
@@ -1035,40 +1030,19 @@ void Canvas2DLayerBridge::didDraw(const FloatRect& rect) {
       disableDeferral(DisableDeferralReasonExpensiveOverdrawHeuristic);
     }
   }
-  if (!m_isRegisteredTaskObserver) {
-    Platform::current()->currentThread()->addTaskObserver(this);
-    m_isRegisteredTaskObserver = true;
-  }
   m_didDrawSinceLastFlush = true;
   m_didDrawSinceLastGpuFlush = true;
 }
 
 void Canvas2DLayerBridge::finalizeFrame() {
+  TRACE_EVENT0("blink", "Canvas2DLayerBridge::finalizeFrame");
   DCHECK(!m_destructionInProgress);
 
   // Make sure surface is ready for painting: fix the rendering mode now
   // because it will be too late during the paint invalidation phase.
   getOrCreateSurface(PreferAcceleration);
 
-  if (m_rateLimiter)
-    m_rateLimiter->reset();
-  m_renderingTaskCompletedForCurrentFrame = false;
-}
-
-void Canvas2DLayerBridge::doPaintInvalidation(const FloatRect& dirtyRect) {
-  DCHECK(!m_destructionInProgress);
-  if (m_layer && m_accelerationMode != DisableAcceleration)
-    m_layer->layer()->invalidateRect(enclosingIntRect(dirtyRect));
-}
-
-void Canvas2DLayerBridge::didProcessTask() {
-  TRACE_EVENT0("cc", "Canvas2DLayerBridge::didProcessTask");
-  DCHECK(m_isRegisteredTaskObserver);
-  // If m_renderTaskProcessedForCurrentFrame is already set to true,
-  // it means that rendering tasks are not synchronized with the compositor
-  // (i.e. not using requestAnimationFrame), so we are at risk of posting
-  // a multi-frame backlog to the GPU
-  if (m_renderingTaskCompletedForCurrentFrame) {
+  if (!m_previousFrameWasPresented) {
     if (isAccelerated()) {
       flushGpu();
       if (!m_rateLimiter) {
@@ -1078,18 +1052,19 @@ void Canvas2DLayerBridge::didProcessTask() {
     } else {
       flush();
     }
+  } else {
+    m_previousFrameWasPresented = false;

[xlai (Olivia), 2017/02/17 19:39:53]

If the previous frame was successfully committed (and is true), why calling
another finalizeFrame() will make it false?  Also the initial call to
finalizeFrame() also make it false?

[Justin Novosad, 2017/02/21 15:49:26]

The basic idea is to activate rate limiting when finalize frame gets called more
than once between commits.  I'm going to rename things and add comments to make
this more readable.

   }
 
   if (m_rateLimiter) {
     m_rateLimiter->tick();
   }
-
-  m_renderingTaskCompletedForCurrentFrame = true;
-  unregisterTaskObserver();
 }
 
-void Canvas2DLayerBridge::willProcessTask() {
-  NOTREACHED();
+void Canvas2DLayerBridge::doPaintInvalidation(const FloatRect& dirtyRect) {
+  DCHECK(!m_destructionInProgress);
+  if (m_layer && m_accelerationMode != DisableAcceleration)
+    m_layer->layer()->invalidateRect(enclosingIntRect(dirtyRect));
 }
 
 sk_sp<SkImage> Canvas2DLayerBridge::newImageSnapshot(AccelerationHint hint,