canvas: make a temporary buffer when a context doesn't exist.

Source/core/html/HTMLCanvasElement.cpp

Index: Source/core/html/HTMLCanvasElement.cpp
diff --git a/Source/core/html/HTMLCanvasElement.cpp b/Source/core/html/HTMLCanvasElement.cpp
index 398e2314071092d48ecda73f84c3e49c5c430842..3ec6cb1e4baadd0c25e3f4d3b55d9efe1f64266a 100644
--- a/Source/core/html/HTMLCanvasElement.cpp
+++ b/Source/core/html/HTMLCanvasElement.cpp
@@ -82,7 +82,7 @@ bool canCreateImageBuffer(const IntSize& deviceSize)
         return false;
     if (deviceSize.width() > MaxSkiaDim || deviceSize.height() > MaxSkiaDim)
         return false;
-    if (!deviceSize.width() || !deviceSize.height())
+    if (deviceSize.isEmpty())
         return false;
     return true;
 }
@@ -410,16 +410,11 @@ const AtomicString HTMLCanvasElement::imageSourceURL() const
 
 String HTMLCanvasElement::toDataURLInternal(const String& mimeType, const double* quality, SourceDrawingBuffer sourceBuffer) const
 {
-    if (m_size.isEmpty() || !canCreateImageBuffer(size()))
+    if (!buffer())
         return String("data:,");
 
     String encodingMimeType = toEncodingMimeType(mimeType);
-    if (!m_context) {
-        RefPtrWillBeRawPtr<ImageData> imageData = ImageData::create(m_size);
-        return ImageEncoder::toDataURL(ImageEncoder::RawImageBytes(imageData->size(), imageData->data()->data()), encodingMimeType, quality);
-    }
-
-    if (m_context->is3d()) {
+    if (m_context && m_context->is3d()) {
         // Get non-premultiplied data because of inaccurate premultiplied alpha conversion of buffer()->toDataURL().
         RefPtrWillBeRawPtr<ImageData> imageData =
             toWebGLRenderingContext(m_context.get())->paintRenderingResultsToImageData(sourceBuffer);
@@ -554,8 +549,9 @@ PassOwnPtr<ImageBufferSurface> HTMLCanvasElement::createImageBufferSurface(const
 
 void HTMLCanvasElement::createImageBuffer()
 {
+    ASSERT(m_context);
     createImageBufferInternal();
-    if (m_didFailToCreateImageBuffer && m_context && m_context->is2d())
+    if (m_didFailToCreateImageBuffer && m_context->is2d())
         toCanvasRenderingContext2D(m_context.get())->loseContext();
 }
 
@@ -651,7 +647,7 @@ void HTMLCanvasElement::updateExternallyAllocatedMemory() const
 
 GraphicsContext* HTMLCanvasElement::drawingContext() const
 {
-    return buffer() ? m_imageBuffer->context() : 0;
+    return buffer() && m_imageBuffer ? m_imageBuffer->context() : 0;
 }
 
 GraphicsContext* HTMLCanvasElement::existingDrawingContext() const
@@ -664,6 +660,16 @@ GraphicsContext* HTMLCanvasElement::existingDrawingContext() const
 
 ImageBuffer* HTMLCanvasElement::buffer() const
 {
+    if (!m_context) {
+        if (!canCreateImageBuffer(size()))
+            return nullptr;
+        m_noContextImageBuffer = ImageBuffer::create(size(), NonOpaque);

[Justin Novosad, 2014/11/27 18:58:35]

Please explain how this code helps. It seems wasteful to me to allocate a
persistent buffer in cases where we know the content is blank. Why is this
better than just returning null? The calling code already needs to be able to
handle a null return value since createImageBuffer() is not guaranteed to
succeed.

[Justin Novosad, 2014/11/27 19:04:29]

Ironically, the title of this change is "Don't make a buffer when a context
doesn't exist", which is a good idea, but you are doing the opposite right here.
;-)

[dshwang, 2014/11/27 19:34:28]

That's good question.
Two reason:
1. this method return pointer. ImageBuffer cannot be wrapped by RefPtr
2. All callers except for CanvasRenderingContext expects this method to return
valid buffer if size is acceptable.
For example, HTMLCanvasElement::paint, HTMLCanvasElement::toDataURL,
HTMLCanvasElement::copiedImage, HTMLCanvasElement::getSourceImageForCanvas,
WebGLRenderingContextBase::texImage2D, WebGLRenderingContextBase::texSubImage2D
Those API want to use transparent imageBuffer when context don't exist. So if
returning nullptr here, so many layout tests fail.

The first patch set fix it as those API don't call buffer(), but the problem is
that we don't know if buffer() is really null until we actually call buffer() as
you mentioned.

[Justin Novosad, 2014/11/27 20:18:15]

That is a problem. We cannot guarantee that this method will return a non-null
pointer. The call to ImageBuffer::create will return a null pointer if it is
unable to allocate a buffer of the requested size.

[dshwang, 2014/11/27 20:42:17]

That's true, but in that sense, most of layout tests fail anytime.

Let me explain.
All layout tests expects that ImageBuffer::create success with normal size.

For example,
var canvas = doc.getElementById("canvas");
var context = canvas.getContext("2d");

var canvas2 = doc.getElementById("canvas2");
canvas2.width = 10;
canvas2.height = 10;
// Following 3 lines expect that canvas2 success to create image buffer, no
matter whether canvas2 has a context or not.
context.drawImage(canvas2);
canvas2.toDataURL();
webGL.texImage2D(..., canvas2);

If ImageBuffer::Create of canvas2 fails anytime, we cannot rely on layout test.
If we return nullptr when no context, above kind of tests fail.

The failure of buffer() with normal size is very exceptional case, but we must
handle as-is for release chrome browser.

[dshwang, 2014/11/28 07:49:44]

Let me explain more in the case of context.drawImage(canvas2)

HTMLCanvasElement::getSourceImageForCanvas() is called for
context.drawImage(canvas2).

The patch set 1 deal with it as follows:
if (!canCreateImageBuffer()) {
    *status = InvalidSourceImageStatus;
    return nullptr;
}
if (!m_context) {
    *status = NormalSourceImageStatus;
    return createTransparentImage(size());
}
RefPtr<SkImage> image = buffer()->newImageSnapshot();

Above code is wrong as you mentioned "not safe to assume buffer is non-null"

Patch set 2 deal with it as follows:
if (!buffer()) {
    *status = InvalidSourceImageStatus;
    return nullptr;
}
RefPtr<SkImage> image = buffer()->newImageSnapshot();


If we want buffer() to return nullptr when no context, code need to bloat like
if (!canCreateImageBuffer()) {
    *status = InvalidSourceImageStatus;
    return nullptr;
}
if (!m_context) {
    *status = NormalSourceImageStatus;
    return createTransparentImage(size());
}
if (!buffer()) {
    *status = InvalidSourceImageStatus;
    return nullptr;
}
RefPtr<SkImage> image = buffer()->newImageSnapshot();

It's possible but all callers need double check for canCreateImageBuffer() and
buffer().
What do you think?

+        return m_noContextImageBuffer.get();
+    }
+    if (m_noContextImageBuffer) {
+        m_noContextImageBuffer.clear();
+        const_cast<HTMLCanvasElement*>(this)->clearCopiedImage();
+    }
     if (!hasImageBuffer() && !m_didFailToCreateImageBuffer)
         const_cast<HTMLCanvasElement*>(this)->createImageBuffer();
     return m_imageBuffer.get();
@@ -671,10 +677,11 @@ ImageBuffer* HTMLCanvasElement::buffer() const
 
 void HTMLCanvasElement::ensureUnacceleratedImageBuffer()
 {
+    ASSERT(m_context);
     if ((hasImageBuffer() && !m_imageBuffer->isAccelerated()) || m_didFailToCreateImageBuffer)
         return;
     discardImageBuffer();
-    OpacityMode opacityMode = !m_context || m_context->hasAlpha() ? NonOpaque : Opaque;
+    OpacityMode opacityMode = m_context->hasAlpha() ? NonOpaque : Opaque;
     m_imageBuffer = ImageBuffer::create(size(), opacityMode);
     m_didFailToCreateImageBuffer = !m_imageBuffer;
 }
@@ -754,10 +761,10 @@ PassRefPtr<Image> HTMLCanvasElement::getSourceImageForCanvas(SourceImageMode mod
         *status = ExternalSourceImageStatus;
 
         // can't create SkImage from WebGLImageBufferSurface (contains only SkBitmap)
-        return m_imageBuffer->copyImage(DontCopyBackingStore, Unscaled);
+        return buffer()->copyImage(DontCopyBackingStore, Unscaled);

[Justin Novosad, 2014/11/27 18:58:35]

I think this CL is a good opportunity to fix something else that bugs me. I find
the function name "buffer" could be improved. It looks like an accessor but it
is actually more than that.  In my opinion, buffer() and createImageBuffer()
should be merged into a single function called getOrCreateImageBuffer(), and
there should be a plain accessor called getImageBuffer() which does not attempts
to create.

[dshwang, 2014/11/27 19:34:28]

That's good idea, but many const methods call buffer() currently.
I want to do it in the next CL.

[Justin Novosad, 2014/11/27 20:18:15]

Acknowledged.

     }
 
-    RefPtr<SkImage> image = m_imageBuffer->newImageSnapshot();
+    RefPtr<SkImage> image = buffer()->newImageSnapshot();
     if (image) {
         *status = NormalSourceImageStatus;