refractoring ImageBitmap class

1. Remove m_imageElement in ImageBitmap.
2. Use SkImage internally, which is thread-safe
reference counted, instead of Image
3. Remove class members m_cropRect, m_bitmapRect,
and m_bitmapOffset, for simplification purpose,
which would result in less bug in the future.

Committed: https://crrev.com/0231c3f8988f6c2a9b62c05a4efb1f8705b8553e
Cr-Commit-Position: refs/heads/master@{#357872}

[xidachen, 2015-10-30 12:10:27]

Description was changed from

==========
remove m_imageElement in ImageBitmap, use SkImage internally instead
of Image
==========

to

==========
remove m_imageElement in ImageBitmap, use SkImage internally instead
of Image
==========

[xidachen, 2015-10-30 12:10:27]

xidachen@chromium.org changed reviewers:
+ junov@chromium.org

[xidachen, 2015-10-30 12:11:19]

Hi Justin,

Please review this CL. Thank you.

[Justin Novosad, 2015-10-30 14:35:19]

https://codereview.chromium.org/1413583004/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/ImageBitmap.cpp (right):

https://codereview.chromium.org/1413583004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/ImageBitmap.cpp:163: void
ImageBitmap::notifyImageSourceChanged()
I think this method can be removed now.

https://codereview.chromium.org/1413583004/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/ImageBitmap.h (right):

https://codereview.chromium.org/1413583004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/ImageBitmap.h:36: PassRefPtr<SkImage>
bitmapImage() const;
The name of this method refers to the class BitmapImage (a subclass of Image).
You should rename this method now.

https://codereview.chromium.org/1413583004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/ImageBitmap.h:69: IntRect m_cropRect;
I think we can get rid of this now that we use SkImage::newSubset for cropping?

[xidachen, 2015-10-30 15:32:35]

https://codereview.chromium.org/1413583004/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/ImageBitmap.cpp (right):

https://codereview.chromium.org/1413583004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/ImageBitmap.cpp:163: void
ImageBitmap::notifyImageSourceChanged()
This method is an override of ImageLoaderClient::notifyImageSourceChanged(), so
we cannot delete it. But it should be an empty method now.

https://codereview.chromium.org/1413583004/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/ImageBitmap.h (right):

https://codereview.chromium.org/1413583004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/ImageBitmap.h:36: PassRefPtr<SkImage>
bitmapImage() const;
On 2015/10/30 14:35:19, Justin Novosad wrote:
> The name of this method refers to the class BitmapImage (a subclass of Image).
> You should rename this method now.

Done.

https://codereview.chromium.org/1413583004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/ImageBitmap.h:69: IntRect m_cropRect;
I think this variable is used to store the size of the ImageBitmap.

[Justin Novosad, 2015-10-30 18:57:45]

https://codereview.chromium.org/1413583004/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/ImageBitmap.h (right):

https://codereview.chromium.org/1413583004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/ImageBitmap.h:36: PassRefPtr<SkImage>
bitmapData() const;
Clearer name for this: skImage(). Also, It should just return a SkImage* since
non of the callsites actually need to retain a reference, the just use the image
immediately.

https://codereview.chromium.org/1413583004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/ImageBitmap.h:66: RefPtr<SkImage> m_bitmap;
m_bitmap -> m_image

https://codereview.chromium.org/1413583004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/ImageBitmap.h:69: IntRect m_cropRect;
Why do we still need this?

[Justin Novosad, 2015-11-02 15:53:19]

https://codereview.chromium.org/1413583004/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/ImageBitmap.cpp (right):

https://codereview.chromium.org/1413583004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/ImageBitmap.cpp:37: return
adoptRef(image->newSubset(srcRect));
Not so fast! In the case where the intersection operation clips the crop rect,
you have to add padding to the result image.

Basically in the case where cropRect != srcRect, you need to create an SkSurface
the size of cropRect, fill it with "transparent", the draw the srcRect to it
with an offset of (srcRect.x() - cropRect.x(), srcRect.y() - cropRect.y())

This will allow you to produce results equivalent to the old m_bitmapRect logic.
Not doing this is what is causing the layout test failures you are seeing in the
try jobs.

https://codereview.chromium.org/1413583004/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/ImageBitmap.h (right):

https://codereview.chromium.org/1413583004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/ImageBitmap.h:63: RefPtr<SkImage> m_image;
This is great, you reduced it to just one field. This will the make transferring
logic that much simpler.

[xidachen, 2015-11-03 20:49:08]

Description was changed from

==========
remove m_imageElement in ImageBitmap, use SkImage internally instead
of Image
==========

to

==========
1. Remove m_imageElement in ImageBitmap.
2. Use SkImage internally, which is thread-safe
reference counted, instead of Image
3. Remove class members m_cropRect, m_bitmapRect,
and m_bitmapOffset, for simplification purpose,
which would result in less bug in the future.
==========

[xidachen, 2015-11-03 21:58:18]

Hi Justin, 

Please review this CL. I think there is something wrong with the mac trybot.

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

https://codereview.chromium.org/1413583004/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/ImageBitmap.cpp (right):

https://codereview.chromium.org/1413583004/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/ImageBitmap.cpp:46:
video->paintCurrentFrame(buffer->canvas(), IntRect(dstPoint, srcRect.size()),
nullptr);
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.

https://codereview.chromium.org/1413583004/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/ImageBitmap.cpp:148: unsigned char*
dstPixels = (unsigned char*)malloc(image->width() * image->height() * 4 *
sizeof(unsigned char*));
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.

https://codereview.chromium.org/1413583004/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/ImageBitmap.cpp:161: return
StaticBitmapImage::create(m_image);
Is this what we want to do when m_image is null?

[xidachen, 2015-11-04 12:59:28]

https://codereview.chromium.org/1413583004/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/ImageBitmap.cpp (right):

https://codereview.chromium.org/1413583004/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/ImageBitmap.cpp:46:
video->paintCurrentFrame(buffer->canvas(), IntRect(dstPoint, srcRect.size()),
nullptr);
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.

https://codereview.chromium.org/1413583004/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/ImageBitmap.cpp:148: unsigned char*
dstPixels = (unsigned char*)malloc(image->width() * image->height() * 4 *
sizeof(unsigned char*));
This is awesome. I have fixed the code accordingly.

https://codereview.chromium.org/1413583004/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/ImageBitmap.cpp:161: return
StaticBitmapImage::create(m_image);
On 2015/11/03 22:20:00, Justin Novosad wrote:
> Is this what we want to do when m_image is null?

Done.

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

https://codereview.chromium.org/1413583004/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/ImageBitmap.cpp (right):

https://codereview.chromium.org/1413583004/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/ImageBitmap.cpp:46:
video->paintCurrentFrame(buffer->canvas(), IntRect(dstPoint, srcRect.size()),
nullptr);
On 2015/11/04 12:59:27, xidachen wrote:
> 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.

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.

https://codereview.chromium.org/1413583004/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/ImageBitmap.cpp:148: unsigned char*
dstPixels = (unsigned char*)malloc(image->width() * image->height() * 4 *
sizeof(unsigned char*));
On 2015/11/04 12:59:27, xidachen wrote:
> This is awesome. I have fixed the code accordingly.

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

[Justin Novosad, 2015-11-04 15:13:46]

On 2015/11/04 15:12:37, Justin Novosad wrote:

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

Never mind...  oops

[Justin Novosad, 2015-11-04 15:20:45]

lgtm with nits

https://codereview.chromium.org/1413583004/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/ImageBitmap.cpp (right):

https://codereview.chromium.org/1413583004/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/ImageBitmap.cpp:141: SkSurface* surface =
SkSurface::NewRaster(info);
Nit: you can use the shorthand SkSurface::NewRasterN32Premul

[xidachen, 2015-11-04 15:45:12]

The CQ bit was checked by xidachen@chromium.org

[xidachen, 2015-11-04 15:45:12]

The patchset sent to the CQ was uploaded after l-g-t-m from junov@chromium.org
Link to the patchset: https://codereview.chromium.org/1413583004/#ps180001
(title: "clean up code")

[commit-bot: I haz the power, 2015-11-04 15:45:48]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1413583004/180001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1413583004/180001

[commit-bot: I haz the power, 2015-11-04 17:00:34]

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

[commit-bot: I haz the power, 2015-11-04 17:00:35]

Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Justin Novosad, 2015-11-04 17:43:17]

https://codereview.chromium.org/1413583004/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/ImageBitmap.cpp (right):

https://codereview.chromium.org/1413583004/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/ImageBitmap.cpp:140: SkSurface* surface =
SkSurface::NewRasterN32Premul(cropRect.width(), cropRect.height());
Memory leak:
RefPtr<SkSurface> surface = adoptRef(SkSurface::NewRasterN32Premul...)

[xidachen, 2015-11-04 18:38:56]

The CQ bit was checked by xidachen@chromium.org

[xidachen, 2015-11-04 18:38:56]

The patchset sent to the CQ was uploaded after l-g-t-m from junov@chromium.org
Link to the patchset: https://codereview.chromium.org/1413583004/#ps200001
(title: "fix memory leak issue")

[commit-bot: I haz the power, 2015-11-04 18:39:48]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1413583004/200001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1413583004/200001

[commit-bot: I haz the power, 2015-11-04 19:37:23]

Committed patchset #11 (id:200001)

[commit-bot: I haz the power, 2015-11-04 19:38:48]

Patchset 11 (id:??) landed as
https://crrev.com/0231c3f8988f6c2a9b62c05a4efb1f8705b8553e
Cr-Commit-Position: refs/heads/master@{#357872}