Implementation of 2D canvas context lost/restored events

Implementation of 2D canvas context lost/restored events

This change adds the APIs specified in the feature proposal found here:
http://wiki.whatwg.org/wiki/Canvas_Context_Loss_and_Restoration
The API changes are hidden behind the experimental canvas features flag.

This change does not implement any elective canvas evictions. Those will
be added in a future change. Only pre-existing context loss use cases
are handled, such as failure to allocate a backing store, and gpu
failures.

The strategy for recovering from a GPU context lost was modified
substantially in order to accomodate synchronization issues with the
context restored event. Context restoration is now attempted proactively
through a scheduled event. Prior to this change, restoration happened
lazily when trying to use the canvas.

BUG=322335
R=senorblanco@chromium.org

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=170572

[Justin Novosad, 2014-03-25 21:53:47]

PTAL

[Justin Novosad, 2014-03-25 22:03:37]

Intent to implement thread:
https://groups.google.com/a/chromium.org/forum/#!searchin/blink-dev/intent$20...

[Stephen White, 2014-03-25 23:25:04]

Some preliminary comments; I'll take a closer look tomorrow.

https://codereview.chromium.org/211503006/diff/70001/LayoutTests/fast/canvas/...
File LayoutTests/fast/canvas/canvas-lose-restore-googol-size.html (right):

https://codereview.chromium.org/211503006/diff/70001/LayoutTests/fast/canvas/...
LayoutTests/fast/canvas/canvas-lose-restore-googol-size.html:22: // WebIDL
defines width and height as int. 2147483647 is int max.
Nit: but the test is googol-sized, not int max?

https://codereview.chromium.org/211503006/diff/70001/Source/core/html/canvas/...
File Source/core/html/canvas/CanvasRenderingContext2D.h (right):

https://codereview.chromium.org/211503006/diff/70001/Source/core/html/canvas/...
Source/core/html/canvas/CanvasRenderingContext2D.h:295: void
tryRestoreSurfaceEvent(Timer<CanvasRenderingContext2D>*);
Naming nit: does "surface" really have a meaning at this level?

https://codereview.chromium.org/211503006/diff/70001/Source/platform/graphics...
File Source/platform/graphics/Canvas2DLayerBridge.cpp (right):

https://codereview.chromium.org/211503006/diff/70001/Source/platform/graphics...
Source/platform/graphics/Canvas2DLayerBridge.cpp:320: isSurfaceValid(); // To
ensure rate limiter is disabled if context is lost.
Naming nit: this looks like a getter (and has the same name as a member var),
but it does a lot more. Can you come up with a more descriptive name? 
checkValidity()? checkValid()? checkIsValid()? updateValidity()?
updateAndGetValidity()?

https://codereview.chromium.org/211503006/diff/70001/Source/platform/graphics...
File Source/platform/graphics/ImageBufferClient.h (right):

https://codereview.chromium.org/211503006/diff/70001/Source/platform/graphics...
Source/platform/graphics/ImageBufferClient.h:36: class ImageBufferClient {
This is so small, I'd just put it in ImageBuffer.h.

[Justin Novosad, 2014-03-27 17:33:42]

>
https://codereview.chromium.org/211503006/diff/70001/Source/platform/graphics...
> Source/platform/graphics/ImageBufferClient.h:36: class ImageBufferClient {
> This is so small, I'd just put it in ImageBuffer.h.

I'd rather not. Because the definition of class ImageBufferClient is needed in
HTMLCanvasElement.h, putting it in ImageBuffer.h would pull in a massive
unnecessary build dependency chain.  I know because that's what I did at first
and it annoyed me enough that I decided to separate it into its own file to
avoid long incremental builds when touching ImageBuffer.h.

[Justin Novosad, 2014-03-27 17:35:32]

New Patch

[Stephen White, 2014-03-27 17:53:58]

On 2014/03/27 17:33:42, junov wrote:
> >
>
https://codereview.chromium.org/211503006/diff/70001/Source/platform/graphics...
> > Source/platform/graphics/ImageBufferClient.h:36: class ImageBufferClient {
> > This is so small, I'd just put it in ImageBuffer.h.
> 
> I'd rather not. Because the definition of class ImageBufferClient is needed in
> HTMLCanvasElement.h, putting it in ImageBuffer.h would pull in a massive
> unnecessary build dependency chain.  I know because that's what I did at first
> and it annoyed me enough that I decided to separate it into its own file to
> avoid long incremental builds when touching ImageBuffer.h.

Ahh, of course. Missed that.

New patch LGTM.

[Justin Novosad, 2014-03-27 18:32:52]

On 2014/03/27 17:53:58, Stephen White wrote:
> 
> New patch LGTM.

Thanks. I'll wait for the M-35 branch before committing this.

[Justin Novosad, 2014-03-27 19:02:48]

On 2014/03/27 18:32:52, junov wrote:
> On 2014/03/27 17:53:58, Stephen White wrote:
> > 
> > New patch LGTM.
> 
> Thanks. I'll wait for the M-35 branch before committing this.

And I have to fix the bazillion test failures...  Will be back for more
reviewin'

[Justin Novosad, 2014-03-27 19:10:17]

New patch: Added null pointer check in HTMLCanvasElement::setSurfaceSize

[Stephen White, 2014-03-27 19:32:53]

On 2014/03/27 19:10:17, junov wrote:
> New patch: Added null pointer check in HTMLCanvasElement::setSurfaceSize

New patch looks good too.

[Justin Novosad, 2014-03-31 20:27:45]

The CQ bit was checked by junov@chromium.org

[commit-bot: I haz the power, 2014-03-31 20:27:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/junov@chromium.org/211503006/150001

[commit-bot: I haz the power, 2014-03-31 20:28:08]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-31 20:28:09]

Failed to apply patch for Source/platform/graphics/Canvas2DLayerBridge.cpp:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file Source/platform/graphics/Canvas2DLayerBridge.cpp
  Hunk #2 FAILED at 135.
  Hunk #3 succeeded at 208 with fuzz 2 (offset 3 lines).
  Hunk #4 succeeded at 296 (offset 8 lines).
  Hunk #5 FAILED at 315.
  Hunk #6 succeeded at 352 (offset 10 lines).
  Hunk #7 succeeded at 377 (offset 11 lines).
  Hunk #8 succeeded at 503 (offset 12 lines).
  2 out of 8 hunks FAILED -- saving rejects to file
Source/platform/graphics/Canvas2DLayerBridge.cpp.rej

Patch:       Source/platform/graphics/Canvas2DLayerBridge.cpp
Index: Source/platform/graphics/Canvas2DLayerBridge.cpp
diff --git a/Source/platform/graphics/Canvas2DLayerBridge.cpp
b/Source/platform/graphics/Canvas2DLayerBridge.cpp
index
772c369d867c13ce7c789740ae550cb806dbafe5..9e8a41a04497d6772228afdc5f9985c6ddc7bcb8
100644
--- a/Source/platform/graphics/Canvas2DLayerBridge.cpp
+++ b/Source/platform/graphics/Canvas2DLayerBridge.cpp
@@ -83,10 +83,11 @@ PassRefPtr<Canvas2DLayerBridge>
Canvas2DLayerBridge::create(const IntSize& size,

Canvas2DLayerBridge::Canvas2DLayerBridge(PassOwnPtr<blink::WebGraphicsContext3DProvider>
contextProvider, PassOwnPtr<SkDeferredCanvas> canvas, int msaaSampleCount,
OpacityMode opacityMode)
     : m_canvas(canvas)
     , m_contextProvider(contextProvider)
+    , m_imageBuffer(0)
     , m_msaaSampleCount(msaaSampleCount)
     , m_bytesAllocated(0)
     , m_didRecordDrawCommand(false)
-    , m_surfaceIsValid(true)
+    , m_isSurfaceValid(true)
     , m_framesPending(0)
     , m_framesSinceMailboxRelease(0)
     , m_destructionInProgress(false)
@@ -134,6 +135,7 @@ void Canvas2DLayerBridge::beginDestruction()
 {
     ASSERT(!m_destructionInProgress);
     m_destructionInProgress = true;
+    m_imageBuffer = 0;
     freeTransientResources();
     setIsHidden(true);
     GraphicsLayer::unregisterContentsLayer(m_layer->layer());
@@ -204,7 +206,7 @@ void Canvas2DLayerBridge::limitPendingFrames()
 void Canvas2DLayerBridge::prepareForDraw()
 {
     ASSERT(m_layer);
-    if (!surfaceIsValid() && !recoverSurface()) {
+    if (!checkSurfaceValid()) {
         if (m_canvas) {
             // drop pending commands because there is no surface to draw to
             m_canvas->silentFlush();
@@ -287,7 +289,7 @@ bool Canvas2DLayerBridge::hasReleasedMailbox() const
 
 void Canvas2DLayerBridge::freeReleasedMailbox()
 {
-    if (m_contextProvider->context3d()->isContextLost() || !m_surfaceIsValid)
+    if (m_contextProvider->context3d()->isContextLost() || !m_isSurfaceValid)
         return;
     MailboxInfo* mailboxInfo = releasedMailboxInfo();
     if (!mailboxInfo)
@@ -314,22 +316,29 @@ blink::WebGraphicsContext3D*
Canvas2DLayerBridge::context()
 {
     // Check on m_layer is necessary because context() may be called during
     // the destruction of m_layer
-    if (m_layer && !surfaceIsValid()) {
-        recoverSurface(); // To ensure rate limiter is disabled if context is
lost.
-    }
+    if (m_layer)
+        checkSurfaceValid(); // To ensure rate limiter is disabled if context
is lost.
     return m_contextProvider->context3d();
 }
 
-bool Canvas2DLayerBridge::surfaceIsValid()
+bool Canvas2DLayerBridge::checkSurfaceValid()
 {
-    return !m_destructionInProgress &&
!m_contextProvider->context3d()->isContextLost() && m_surfaceIsValid;
+    if (m_destructionInProgress || !m_isSurfaceValid)
+        return false;
+    if (m_contextProvider->context3d()->isContextLost()) {
+        m_isSurfaceValid = false;
+        if (m_imageBuffer)
+            m_imageBuffer->notifySurfaceInvalid();
+        setRateLimitingEnabled(false);
+    }
+    return m_isSurfaceValid;
 }
 
-bool Canvas2DLayerBridge::recoverSurface()
+bool Canvas2DLayerBridge::restoreSurface()
 {
-    ASSERT(m_layer && !surfaceIsValid());
     if (m_destructionInProgress)
         return false;
+    ASSERT(m_layer && !m_isSurfaceValid);
 
     blink::WebGraphicsContext3D* sharedContext = 0;
     // We must clear the mailboxes before calling m_layer->clearTexture() to
prevent
@@ -341,25 +350,17 @@ bool Canvas2DLayerBridge::recoverSurface()
     if (m_contextProvider)
         sharedContext = m_contextProvider->context3d();
 
-    if (!sharedContext || sharedContext->isContextLost()) {
-        m_surfaceIsValid = false;
-    } else {
+    if (sharedContext && !sharedContext->isContextLost()) {
         IntSize size(m_canvas->getTopDevice()->width(),
m_canvas->getTopDevice()->height());
         RefPtr<SkSurface>
surface(createSkSurface(m_contextProvider->grContext(), size,
m_msaaSampleCount));
         if (surface.get()) {
             m_canvas->setSurface(surface.get());
-            m_surfaceIsValid = true;
+            m_isSurfaceValid = true;
             // FIXME: draw sad canvas picture into new buffer crbug.com/243842
-        } else {
-            // Surface allocation failed. Set m_surfaceIsValid to false to
-            // trigger subsequent retry.
-            m_surfaceIsValid = false;
         }
     }
-    if (!m_surfaceIsValid)
-        setRateLimitingEnabled(false);
 
-    return m_surfaceIsValid;
+    return m_isSurfaceValid;
 }
 
 bool Canvas2DLayerBridge::prepareMailbox(blink::WebExternalTextureMailbox*
outMailbox, blink::WebExternalBitmap* bitmap)
@@ -373,7 +374,7 @@ bool
Canvas2DLayerBridge::prepareMailbox(blink::WebExternalTextureMailbox* outMa
         m_lastImageId = 0;
         return false;
     }
-    if (!surfaceIsValid() && !recoverSurface())
+    if (!checkSurfaceValid())
         return false;
 
     blink::WebGraphicsContext3D* webContext = context();
@@ -498,7 +499,7 @@ void Canvas2DLayerBridge::willUse()
 Platform3DObject Canvas2DLayerBridge::getBackingTexture()
 {
     ASSERT(!m_destructionInProgress);
-    if (!surfaceIsValid() && !recoverSurface())
+    if (!checkSurfaceValid())
         return 0;
     willUse();
     m_canvas->flush();

[Justin Novosad, 2014-03-31 20:42:10]

The CQ bit was checked by junov@chromium.org

[commit-bot: I haz the power, 2014-03-31 20:42:34]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/junov@chromium.org/211503006/170001

[commit-bot: I haz the power, 2014-03-31 21:39:41]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-31 21:39:42]

Try jobs failed on following builders:
  tryserver.blink on win_blink_rel

[Justin Novosad, 2014-04-01 14:43:27]

Committed patchset #5 manually as r170572 (presubmit successful).