refractoring ImageBitmap class

third_party/WebKit/Source/core/frame/ImageBitmap.cpp

 // Copyright 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "config.h"
 #include "core/frame/ImageBitmap.h"
 
 #include "core/html/HTMLCanvasElement.h"
 #include "core/html/HTMLVideoElement.h"
 #include "core/html/ImageData.h"
 #include "platform/graphics/GraphicsContext.h"
 #include "platform/graphics/ImageBuffer.h"
 #include "platform/graphics/StaticBitmapImage.h"
 #include "platform/graphics/paint/DrawingRecorder.h"
 #include "platform/graphics/paint/SkPictureBuilder.h"
 #include "wtf/RefPtr.h"
 
 namespace blink {
 
 static inline IntRect normalizeRect(const IntRect& rect)
 {
     return IntRect(std::min(rect.x(), rect.maxX()),
         std::min(rect.y(), rect.maxY()),
         std::max(rect.width(), -rect.width()),
         std::max(rect.height(), -rect.height()));
 }
 
-static inline PassRefPtr<Image> cropImage(PassRefPtr<Image> image, const IntRect& cropRect)
+ImageBitmap::ImageBitmap(HTMLImageElement* image, const IntRect& cropRect)
 {
-    ASSERT(image);
-
-    const SkIRect srcRect = intersection(image->rect(), cropRect);
-    if (srcRect.isEmpty())
-        return nullptr;
-
-    RefPtr<SkImage> skImage = image->imageForCurrentFrame();
-    if (!skImage)
-        return nullptr;
-
-    return StaticBitmapImage::create(adoptRef(skImage->newSubset(srcRect)));
-}
-
-ImageBitmap::ImageBitmap(HTMLImageElement* image, const IntRect& cropRect)
-    : m_imageElement(image)
-    , m_bitmap(nullptr)
-    , m_cropRect(cropRect)
-{
-    IntRect srcRect = intersection(cropRect, IntRect(0, 0, image->width(), image->height()));
-    m_bitmapRect = IntRect(IntPoint(std::max(0, -cropRect.x()), std::max(0, -cropRect.y())), srcRect.size());
-    m_bitmapOffset = srcRect.location();
-
-    if (!srcRect.width() || !srcRect.height())
-        m_imageElement = nullptr;
-    else
-        m_imageElement->addClient(this);
+    m_image = cropImage(image->cachedImage()->image()->imageForCurrentFrame(), cropRect);
 }
 
 ImageBitmap::ImageBitmap(HTMLVideoElement* video, const IntRect& cropRect)
-    : m_imageElement(nullptr)
-    , m_cropRect(cropRect)
-    , m_bitmapOffset(IntPoint())
 {
     IntSize playerSize;
-
     if (video->webMediaPlayer())
         playerSize = video->webMediaPlayer()->naturalSize();
 
     IntRect videoRect = IntRect(IntPoint(), playerSize);
     IntRect srcRect = intersection(cropRect, videoRect);
-    IntRect dstRect(IntPoint(), srcRect.size());
-
-    OwnPtr<ImageBuffer> buffer = ImageBuffer::create(videoRect.size());
+    OwnPtr<ImageBuffer> buffer = ImageBuffer::create(cropRect.size(), NonOpaque, DoNotInitializeImagePixels);
     if (!buffer)
         return;
 
-    buffer->canvas()->clipRect(dstRect);
-    buffer->canvas()->translate(-srcRect.x(), -srcRect.y());
-
-    video->paintCurrentFrame(buffer->canvas(), videoRect, nullptr);
-    m_bitmap = buffer->newImageSnapshot();
-    m_bitmapRect = IntRect(IntPoint(std::max(0, -cropRect.x()), std::max(0, -cropRect.y())), srcRect.size());
+    IntPoint dstPoint = IntPoint(std::max(0, -cropRect.x()), std::max(0, -cropRect.y()));
+    video->paintCurrentFrame(buffer->canvas(), IntRect(dstPoint, srcRect.size()), nullptr);

[Justin Novosad, 2015/11/03 22:19:59]

If the destination rectangle does not no cover the entire buffer, areas may be
left uninitialized.  I think the "DoNotInitializeImagePixels" above should be
conditional based on coverage. Is this use case covered by a layout test that
uses a video as a source?  It should be.

[xidachen, 2015/11/04 12:59:27]

Yes indeed. There is a fast/canvas/canvas-createImageBitmap-drawImage-video.html
does the layout test. I believe it covers all the cases: noCrop, crop,
cropCenter, cropRight, overCrop, overCropRight, negativeCrop and empty.

I hope the following explanation is clear and makes sense.
ImageBuffer::create does some tricky business, it calls the constructor of
"UnacceleratedImageBufferSurface" and adoptRef from it. Now the constructor of
"UnacceleratedImageBufferSurface" has two steps:
1). call SkSurface::NewRaster, which initialize the entire surface to be
NonOpaque black (everything 0).
2). call Skcanvas()::clear(SK_ColorTRANSPARENT) if "DoNotInitializeImagePixels"
is not set.

In my opinion, the second step is an overkill in our case. So the
"DoNotInitializeImagePixels" is set to ignore the second step.

[Justin Novosad, 2015/11/04 15:12:37]

Sounds like there is room for optimization.  Please file a bug for this.
Normally DoNotInitializeImagePixels should do what it says it does, and the
choice between initializing to opaque black (0, 0, 0, 255) or transparent (0, 0,
0, 0) should be driven by the opacityMode parameter. I think the opacity mode
part is correct, except for the fact that we may be doubly initializing the
pixels.

+    m_image = buffer->newSkImageSnapshot(PreferNoAcceleration);
 }
 
 ImageBitmap::ImageBitmap(HTMLCanvasElement* canvas, const IntRect& cropRect)
-    : m_imageElement(nullptr)
-    , m_cropRect(cropRect)
-    , m_bitmapOffset(IntPoint())
 {
-    IntRect srcRect = intersection(cropRect, IntRect(IntPoint(), canvas->size()));
-    m_bitmapRect = IntRect(IntPoint(std::max(0, -cropRect.x()), std::max(0, -cropRect.y())), srcRect.size());
     ASSERT(canvas->isPaintable());
-    m_bitmap = cropImage(canvas->copiedImage(BackBuffer, PreferAcceleration), cropRect);
+    m_image = cropImage(canvas->copiedImage(BackBuffer, PreferAcceleration)->imageForCurrentFrame(), cropRect);
 }
 
 ImageBitmap::ImageBitmap(ImageData* data, const IntRect& cropRect)
-    : m_imageElement(nullptr)
-    , m_cropRect(cropRect)
-    , m_bitmapOffset(IntPoint())
 {
     IntRect srcRect = intersection(cropRect, IntRect(IntPoint(), data->size()));
-    OwnPtr<ImageBuffer> buffer = ImageBuffer::create(data->size(), NonOpaque, DoNotInitializeImagePixels);
-    if (!buffer)
+    if (srcRect.isEmpty()) {
+        OwnPtr<ImageBuffer> buffer = ImageBuffer::create(cropRect.size(), NonOpaque, DoNotInitializeImagePixels);
+        m_image = buffer->newSkImageSnapshot(PreferNoAcceleration);
         return;
-
-    if (srcRect.width() > 0 && srcRect.height() > 0)
-        buffer->putByteArray(Unmultiplied, data->data()->data(), data->size(), srcRect, IntPoint(std::min(0, -cropRect.x()), std::min(0, -cropRect.y())));
-
-    m_bitmap = buffer->newImageSnapshot();
-    m_bitmapRect = IntRect(IntPoint(std::max(0, -cropRect.x()), std::max(0, -cropRect.y())),  srcRect.size());
+    }
+    m_image = cropImageHelper(cropRect, Unmultiplied, data->data()->data(), data->size(), srcRect);
 }
 
 ImageBitmap::ImageBitmap(ImageBitmap* bitmap, const IntRect& cropRect)
-    : m_imageElement(bitmap->imageElement())
-    , m_bitmap(nullptr)
-    , m_cropRect(cropRect)
-    , m_bitmapOffset(IntPoint())
 {
-    IntRect oldBitmapRect = bitmap->bitmapRect();
-    IntRect srcRect = intersection(cropRect, oldBitmapRect);
-    m_bitmapRect = IntRect(IntPoint(std::max(0, oldBitmapRect.x() - cropRect.x()), std::max(0, oldBitmapRect.y() - cropRect.y())), srcRect.size());
-
-    if (m_imageElement) {
-        m_imageElement->addClient(this);
-        m_bitmapOffset = srcRect.location();
-    } else if (bitmap->bitmapImage()) {
-        IntRect adjustedCropRect(IntPoint(cropRect.x() -oldBitmapRect.x(), cropRect.y() - oldBitmapRect.y()), cropRect.size());
-        m_bitmap = cropImage(bitmap->bitmapImage(), adjustedCropRect);
-    }
+    m_image = cropImage(bitmap->skImage(), cropRect);
 }
 
 ImageBitmap::ImageBitmap(Image* image, const IntRect& cropRect)
-    : m_imageElement(nullptr)
-    , m_cropRect(cropRect)
 {
-    IntRect srcRect = intersection(cropRect, image->rect());
-    m_bitmap = cropImage(image, cropRect);
-    m_bitmapRect = IntRect(IntPoint(std::max(0, -cropRect.x()), std::max(0, -cropRect.y())),  srcRect.size());
+    m_image = cropImage(image->imageForCurrentFrame(), cropRect);
 }
 
 ImageBitmap::~ImageBitmap()
 {
-#if !ENABLE(OILPAN)
-    if (m_imageElement)
-        m_imageElement->removeClient(this);
-#endif
 }
 
 PassRefPtrWillBeRawPtr<ImageBitmap> ImageBitmap::create(HTMLImageElement* image, const IntRect& cropRect)
 {
     IntRect normalizedCropRect = normalizeRect(cropRect);
     return adoptRefWillBeNoop(new ImageBitmap(image, normalizedCropRect));
 }
 
 PassRefPtrWillBeRawPtr<ImageBitmap> ImageBitmap::create(HTMLVideoElement* video, const IntRect& cropRect)
 {
@@ skipping 18 matching lines @@
     IntRect normalizedCropRect = normalizeRect(cropRect);
     return adoptRefWillBeNoop(new ImageBitmap(bitmap, normalizedCropRect));
 }
 
 PassRefPtrWillBeRawPtr<ImageBitmap> ImageBitmap::create(Image* image, const IntRect& cropRect)
 {
     IntRect normalizedCropRect = normalizeRect(cropRect);
     return adoptRefWillBeNoop(new ImageBitmap(image, normalizedCropRect));
 }
 
+PassRefPtr<SkImage> ImageBitmap::cropImageHelper(const IntRect& cropRect, Multiply multiplied, const unsigned char* src, const IntSize& srcSize, const IntRect& srcRect)
+{
+    OwnPtr<ImageBuffer> buffer = ImageBuffer::create(cropRect.size(), NonOpaque, DoNotInitializeImagePixels);
+    if (!buffer)
+        return nullptr;
+
+    IntPoint dstPoint = IntPoint(std::min(0, -cropRect.x()), std::min(0, -cropRect.y()));
+    if (cropRect.x() < 0)
+        dstPoint.setX(-cropRect.x());
+    if (cropRect.y() < 0)
+        dstPoint.setY(-cropRect.y());
+    buffer->putByteArray(multiplied, src, srcSize, srcRect, dstPoint);
+    return buffer->newSkImageSnapshot(PreferNoAcceleration);
+}
+
+PassRefPtr<SkImage> ImageBitmap::cropImage(PassRefPtr<SkImage> image, const IntRect& cropRect)
+{
+    ASSERT(image);
+
+    IntRect imgRect = IntRect(IntPoint(), IntSize(image->width(), image->height()));
+    const IntRect srcRect = intersection(imgRect, cropRect);
+
+    if (cropRect == srcRect)
+        return adoptRef(image->newSubset(srcRect));
+
+    if (srcRect.isEmpty()) {
+        OwnPtr<ImageBuffer> buffer = ImageBuffer::create(cropRect.size(), NonOpaque, DoNotInitializeImagePixels);
+        return buffer->newSkImageSnapshot(PreferNoAcceleration);
+    }
+
+    SkImageInfo srcInfo = SkImageInfo::Make(image->width(), image->height(), kRGBA_8888_SkColorType, kPremul_SkAlphaType);
+    unsigned char* dstPixels = (unsigned char*)malloc(image->width() * image->height() * 4 * sizeof(unsigned char*));

[Justin Novosad, 2015/11/03 22:19:59]

This buffer appears to be leaked. Also, it seems unnecessary to go through this
intermediate buffer. You should just use an SkSurface and draw 'image' into it
with surface->getCanvas()->drawImage(), Then snap an SkImage result from the
SkSurface.

[xidachen, 2015/11/04 12:59:27]

This is awesome. I have fixed the code accordingly.

[Justin Novosad, 2015/11/04 15:12:37]

Okay, but you have not re-uploaded the CL.

+    size_t dstRowBytes = image->width() * 4;
+    image->readPixels(srcInfo, dstPixels, dstRowBytes, 0, 0);
+    return cropImageHelper(cropRect, Premultiplied, dstPixels, IntSize(image->width(), image->height()), srcRect);
+}
+
 void ImageBitmap::notifyImageSourceChanged()
 {
-    m_bitmap = cropImage(m_imageElement->cachedImage()->image(), m_cropRect);
-    m_bitmapOffset = IntPoint();
-    m_imageElement = nullptr;
-}
-
-PassRefPtr<Image> ImageBitmap::bitmapImage() const
-{
-    ASSERT((m_imageElement || m_bitmap || !m_bitmapRect.width() || !m_bitmapRect.height()) && (!m_imageElement || !m_bitmap));
-    if (m_imageElement)
-        return m_imageElement->cachedImage()->image();
-    return m_bitmap;
 }
 
 PassRefPtr<Image> ImageBitmap::getSourceImageForCanvas(SourceImageStatus* status, AccelerationHint) const
 {
     *status = NormalSourceImageStatus;
-    return bitmapImage();
+    return StaticBitmapImage::create(m_image);

[Justin Novosad, 2015/11/03 22:20:00]

Is this what we want to do when m_image is null?

[xidachen, 2015/11/04 12:59:27]

Done.

 }
 
 void ImageBitmap::adjustDrawRects(FloatRect* srcRect, FloatRect* dstRect) const
 {
-    FloatRect intersectRect = intersection(m_bitmapRect, *srcRect);
-    FloatRect newSrcRect = intersectRect;
-    newSrcRect.move(m_bitmapOffset - m_bitmapRect.location());
-    FloatRect newDstRect(FloatPoint(intersectRect.location() - srcRect->location()), m_bitmapRect.size());
-    newDstRect.scale(dstRect->width() / srcRect->width() * intersectRect.width() / m_bitmapRect.width(),
-        dstRect->height() / srcRect->height() * intersectRect.height() / m_bitmapRect.height());
-    newDstRect.moveBy(dstRect->location());
-    *srcRect = newSrcRect;
-    *dstRect = newDstRect;
+
 }
 
 FloatSize ImageBitmap::elementSize() const
 {
     return FloatSize(width(), height());
 }
 
 DEFINE_TRACE(ImageBitmap)
 {
-    visitor->trace(m_imageElement);
     ImageLoaderClient::trace(visitor);
 }
 
 } // namespace blink