Implement resize option for createImageBitmap from ImageData

Implement resize option for createImageBitmap from ImageData

In the case of ImageData, we use two different code path for the
following two cases:
1). ImageBitmapOptions.premultiplyAlpha = true
2). ImageBitmapOptions.premultiplyAlpha = false

In the first case, we create a SkSurface and draw the original SkImage
onto it for resizing. In the second case, we call scalePixels on the
original SkImage.

BUG=627855
Committed: https://crrev.com/5c5fa614b44d8133ab31280b67fe33638621a192
Cr-Commit-Position: refs/heads/master@{#408402}

[xidachen, 2016-07-27 12:37:23]

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

[xidachen, 2016-07-27 12:37:24]

PTAL

[Justin Novosad, 2016-07-27 17:05:06]

https://codereview.chromium.org/2183323003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/frame/ImageBitmap.cpp (right):

https://codereview.chromium.org/2183323003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/ImageBitmap.cpp:368: SkImageInfo
resizedInfo = SkImageInfo::Make(resizeWidth, resizeHeight,
kBGRA_8888_SkColorType, kUnpremul_SkAlphaType);
Why BGRA instead of N32 (the platform-specific default)?

https://codereview.chromium.org/2183323003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/ImageBitmap.cpp:398: skImage =
fromSkSp(SkImage::MakeRasterCopy(SkPixmap(info, srcAddr, dstPixelBytesPerRow)));
It's a shame to have to make an make a copy here then another one when we
resize. Is there a reasonable way to avoid the double copy?  If it is
complicated, maybe file a bug and leave a comment.

[xidachen, 2016-07-28 13:21:16]

https://codereview.chromium.org/2183323003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/frame/ImageBitmap.cpp (right):

https://codereview.chromium.org/2183323003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/ImageBitmap.cpp:368: SkImageInfo
resizedInfo = SkImageInfo::Make(resizeWidth, resizeHeight,
kBGRA_8888_SkColorType, kUnpremul_SkAlphaType);
On 2016/07/27 17:05:05, Justin Novosad wrote:
> Why BGRA instead of N32 (the platform-specific default)?

Done, changed to N32. Swizzle is applied only when necessary, i.e. N32==BGRA.

https://codereview.chromium.org/2183323003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/ImageBitmap.cpp:398: skImage =
fromSkSp(SkImage::MakeRasterCopy(SkPixmap(info, srcAddr, dstPixelBytesPerRow)));
On 2016/07/27 17:05:05, Justin Novosad wrote:
> It's a shame to have to make an make a copy here then another one when we
> resize. Is there a reasonable way to avoid the double copy?  If it is
> complicated, maybe file a bug and leave a comment.

Not easy. SkSurface doesn't support un-premultiplied backing, so here we have
only one option, which is to create a SkImage directly from a raw ImageData.
Because SkImage cannot take ownership of the input ImageData, we would have to
make a copy here.

One way I can think of is this: we add a scale function to ImageData (not
trivial), which applies scaling to the input ImageData directly and produce a
scaled copy. Then we create a SkImage from that scaled copy, this time SkImage
can take the ownership of the scaled copy. However, I don't think add a scale
function to ImageData is trivial.

[Justin Novosad, 2016-07-28 14:35:42]

On 2016/07/28 13:21:16, xidachen wrote:
>
https://codereview.chromium.org/2183323003/diff/1/third_party/WebKit/Source/c...
> File third_party/WebKit/Source/core/frame/ImageBitmap.cpp (right):
> 
>
https://codereview.chromium.org/2183323003/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/frame/ImageBitmap.cpp:368: SkImageInfo
> resizedInfo = SkImageInfo::Make(resizeWidth, resizeHeight,
> kBGRA_8888_SkColorType, kUnpremul_SkAlphaType);
> On 2016/07/27 17:05:05, Justin Novosad wrote:
> > Why BGRA instead of N32 (the platform-specific default)?
> 
> Done, changed to N32. Swizzle is applied only when necessary, i.e. N32==BGRA.
> 
>
https://codereview.chromium.org/2183323003/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/frame/ImageBitmap.cpp:398: skImage =
> fromSkSp(SkImage::MakeRasterCopy(SkPixmap(info, srcAddr,
dstPixelBytesPerRow)));
> On 2016/07/27 17:05:05, Justin Novosad wrote:
> > It's a shame to have to make an make a copy here then another one when we
> > resize. Is there a reasonable way to avoid the double copy?  If it is
> > complicated, maybe file a bug and leave a comment.
> 
> Not easy. SkSurface doesn't support un-premultiplied backing, so here we have
> only one option, which is to create a SkImage directly from a raw ImageData.
> Because SkImage cannot take ownership of the input ImageData, we would have to
> make a copy here.
> 
> One way I can think of is this: we add a scale function to ImageData (not
> trivial), which applies scaling to the input ImageData directly and produce a
> scaled copy. Then we create a SkImage from that scaled copy, this time SkImage
> can take the ownership of the scaled copy. However, I don't think add a scale
> function to ImageData is trivial.

Yeah... let's save this for a potential follow-up CL.  Please file a bug for it.
Also, the N32 == BGRA stuff... it means we'll take a hit on windows :-(
Let's file a bug for that too

[Justin Novosad, 2016-07-28 14:35:57]

> Yeah... let's save this for a potential follow-up CL.  Please file a bug for
it.
> Also, the N32 == BGRA stuff... it means we'll take a hit on windows :-(
> Let's file a bug for that too

and lgtm

[xidachen, 2016-07-28 14:41:47]

The CQ bit was checked by xidachen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-28 14:42:11]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[xidachen, 2016-07-28 14:58:41]

On 2016/07/28 14:35:57, Justin Novosad wrote:
> > Yeah... let's save this for a potential follow-up CL.  Please file a bug for
> it.
> > Also, the N32 == BGRA stuff... it means we'll take a hit on windows :-(
> > Let's file a bug for that too
> 
> and lgtm

Bug filed:
https://bugs.chromium.org/p/chromium/issues/detail?id=632349
https://bugs.chromium.org/p/chromium/issues/detail?id=632350

[commit-bot: I haz the power, 2016-07-28 15:39:54]

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

[commit-bot: I haz the power, 2016-07-28 15:39:55]

Dry run: This issue passed the CQ dry run.

[xidachen, 2016-07-28 15:40:28]

The CQ bit was checked by xidachen@chromium.org

[commit-bot: I haz the power, 2016-07-28 15:40:40]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-28 15:44:02]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-07-28 15:46:11]

Description was changed from

==========
Implement resize option for createImageBitmap from ImageData

In the case of ImageData, we use two different code path for the
following two cases:
1). ImageBitmapOptions.premultiplyAlpha = true
2). ImageBitmapOptions.premultiplyAlpha = false

In the first case, we create a SkSurface and draw the original SkImage
onto it for resizing. In the second case, we call scalePixels on the
original SkImage.

BUG=627855
==========

to

==========
Implement resize option for createImageBitmap from ImageData

In the case of ImageData, we use two different code path for the
following two cases:
1). ImageBitmapOptions.premultiplyAlpha = true
2). ImageBitmapOptions.premultiplyAlpha = false

In the first case, we create a SkSurface and draw the original SkImage
onto it for resizing. In the second case, we call scalePixels on the
original SkImage.

BUG=627855

Committed: https://crrev.com/5c5fa614b44d8133ab31280b67fe33638621a192
Cr-Commit-Position: refs/heads/master@{#408402}
==========

[commit-bot: I haz the power, 2016-07-28 15:46:12]

Patchset 2 (id:??) landed as
https://crrev.com/5c5fa614b44d8133ab31280b67fe33638621a192
Cr-Commit-Position: refs/heads/master@{#408402}