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

Source/core/html/HTMLCanvasElement.cpp

 /*
  * Copyright (C) 2004, 2006, 2007 Apple Inc. All rights reserved.
  * Copyright (C) 2007 Alp Toker <alp@atoker.com>
  * Copyright (C) 2010 Torch Mobile (Beijing) Co. Ltd. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
  * are met:
  * 1. Redistributions of source code must retain the above copyright
  *    notice, this list of conditions and the following disclaimer.
@@ skipping 64 matching lines @@
 
 //In Skia, we will also limit width/height to 32767.
 const int MaxSkiaDim = 32767; // Maximum width/height in CSS pixels.
 
 bool canCreateImageBuffer(const IntSize& deviceSize)
 {
     if (deviceSize.width() * deviceSize.height() > MaxCanvasArea)
         return false;
     if (deviceSize.width() > MaxSkiaDim || deviceSize.height() > MaxSkiaDim)
         return false;
-    if (!deviceSize.width() || !deviceSize.height())
+    if (deviceSize.isEmpty())
         return false;
     return true;
 }
 
 } // namespace
 
 DEFINE_EMPTY_DESTRUCTOR_WILL_BE_REMOVED(CanvasObserver);
 
 inline HTMLCanvasElement::HTMLCanvasElement(Document& document)
     : HTMLElement(canvasTag, document)
@@ skipping 307 matching lines @@
     return lowercaseMimeType;
 }
 
 const AtomicString HTMLCanvasElement::imageSourceURL() const
 {
     return AtomicString(toDataURLInternal("image/png", 0, FrontBuffer));
 }
 
 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);
         if (imageData)
             return ImageEncoder::toDataURL(ImageEncoder::RawImageBytes(imageData->size(), imageData->data()->data()), encodingMimeType, quality);
         m_context->paintRenderingResultsToCanvas(sourceBuffer);
     }
 
     return buffer()->toDataURL(encodingMimeType, quality);
 }
@@ skipping 114 matching lines @@
         if (surface->isValid())
             return surface.release();
         surfaceFactory = createSurfaceFactory(deviceSize, msaaSampleCount); // recreate because old previous one was released
     }
 
     return surfaceFactory->createSurface(deviceSize, opacityMode);
 }
 
 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();
 }
 
 void HTMLCanvasElement::createImageBufferInternal()
 {
     ASSERT(!m_imageBuffer);
     ASSERT(!m_contextStateSaver);
 
     m_didFailToCreateImageBuffer = true;
     m_imageBufferIsClear = true;
@@ skipping 75 matching lines @@
     if (checkedExternallyAllocatedMemory.safeGet(externallyAllocatedMemory) == CheckedState::DidOverflow)
         externallyAllocatedMemory = std::numeric_limits<intptr_t>::max();
 
     // Subtracting two intptr_t that are known to be positive will never underflow.
     v8::Isolate::GetCurrent()->AdjustAmountOfExternalAllocatedMemory(externallyAllocatedMemory - m_externallyAllocatedMemory);
     m_externallyAllocatedMemory = externallyAllocatedMemory;
 }
 
 GraphicsContext* HTMLCanvasElement::drawingContext() const
 {
-    return buffer() ? m_imageBuffer->context() : 0;
+    return buffer() && m_imageBuffer ? m_imageBuffer->context() : 0;
 }
 
 GraphicsContext* HTMLCanvasElement::existingDrawingContext() const
 {
     if (!hasImageBuffer())
         return nullptr;
 
     return drawingContext();
 }
 
 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();
 }
 
 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;
 }
 
 Image* HTMLCanvasElement::copiedImage(SourceDrawingBuffer sourceBuffer) const
 {
     if (!m_copiedImage && buffer()) {

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

Not new in this patch, but there is something wrong here: if m_copiedImage is
non-null, how do we know that is came from the right sourceBuffer? I think this
is a mistake that was not caught in the code review
https://codereview.chromium.org/749653002. Could you fix that in a separate
patch?

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

Good point. I'll fix it in the separate CL. Thank you.
It needs to create new member, m_copiedBackBufferImage, which is not related to
canvas 2d.
It's why I want to move imageBuffer to each CanvasRenderingContext.

         if (m_context && m_context->is3d())
             m_context->paintRenderingResultsToCanvas(sourceBuffer);
         m_copiedImage = buffer()->copyImage(CopyBackingStore, Unscaled);
         updateExternallyAllocatedMemory();
     }
     return m_copiedImage.get();
 }
 
 void HTMLCanvasElement::discardImageBuffer()
 {
@@ skipping 52 matching lines @@
     if (!buffer()) {
         *status = InvalidSourceImageStatus;
         return nullptr;
     }
 
     if (m_context && m_context->is3d()) {
         m_context->paintRenderingResultsToCanvas(BackBuffer);
         *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;
 
         return StaticBitmapImage::create(image.release());
     }
 
 
     *status = InvalidSourceImageStatus;
 
     return nullptr;
 }
 
 bool HTMLCanvasElement::wouldTaintOrigin(SecurityOrigin*) const
 {
     return !originClean();
 }
 
 FloatSize HTMLCanvasElement::sourceSize() const
 {
     return FloatSize(width(), height());
 }
 
 }