Avoid copy pixel data in texImage2D(ImageBitmap)

Avoid copy pixel data in texImage2D(ImageBitmap)

This is the first patch in order to avoid reading back GPU texture.
Currently in texImage2D(ImageBitmap), it makes a copy of the pixel data
stored inside the ImageBitmap (copyBitmapData). We should avoid that.
To solve that, we use SkImage's peekPixels() API. peekPixels() returns
true only when the ImageBitmap is stored in the CPU-side RAM. In the
case when an ImageBitmap is texture backed, this approach will not work.

I will have a follow up patch right after this, to handles the case where
an ImageBitmap is texture backed.

BUG=613411, 613409
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel

Committed: https://crrev.com/c44bccbcfded3d5906dc0fa79c9b0e810369b36c
Cr-Commit-Position: refs/heads/master@{#396529}

[xidachen, 2016-05-26 20:23:18]

Description was changed from

==========
Avoid copy pixel data in texImage2D(ImageBitmap)

This is the first patch in order to avoid reading back GPU texture.
Currently in texImage2D(ImageBitmap), it makes a copy of the pixel data
stored inside the ImageBitmap (copyBitmapData). We should avoid that.
To solve that, we use SkImage's peekPixels() API. peekPixels() returns
true only when the ImageBitmap is stored in the CPU-side RAM. In the
case when an ImageBitmap is texture backed, this approach will not work.

I will have a follow up patch right after this, to handles the case where
an ImageBitmap is texture backed.

BUG=613411, 613409
==========

to

==========
Avoid copy pixel data in texImage2D(ImageBitmap)

This is the first patch in order to avoid reading back GPU texture.
Currently in texImage2D(ImageBitmap), it makes a copy of the pixel data
stored inside the ImageBitmap (copyBitmapData). We should avoid that.
To solve that, we use SkImage's peekPixels() API. peekPixels() returns
true only when the ImageBitmap is stored in the CPU-side RAM. In the
case when an ImageBitmap is texture backed, this approach will not work.

I will have a follow up patch right after this, to handles the case where
an ImageBitmap is texture backed.

BUG=613411, 613409
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel
==========

[xidachen, 2016-05-27 01:49:06]

xidachen@chromium.org changed reviewers:
+ bajones@chromium.org, junov@chromium.org, kbr@chromium.org, zmo@chromium.org

[xidachen, 2016-05-27 01:49:07]

PTAL

[Zhenyao Mo, 2016-05-27 13:49:21]

https://codereview.chromium.org/2016043002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp
(right):

https://codereview.chromium.org/2016043002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp:1162: if
(((peekSucceed && pixmap.colorType() == SkColorType::kRGBA_8888_SkColorType) ||
!peekSucceed) && format == GL_RGBA && type == GL_UNSIGNED_BYTE) {
if peekSucceed is false, why needConversion is set to false? Isn't the above
code if skImage->isTextureBacked() used pixelData?

[xidachen, 2016-05-27 13:54:44]

https://codereview.chromium.org/2016043002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp
(right):

https://codereview.chromium.org/2016043002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp:1162: if
(((peekSucceed && pixmap.colorType() == SkColorType::kRGBA_8888_SkColorType) ||
!peekSucceed) && format == GL_RGBA && type == GL_UNSIGNED_BYTE) {
On 2016/05/27 13:49:21, Zhenyao Mo wrote:
> if peekSucceed is false, why needConversion is set to false? Isn't the above
> code if skImage->isTextureBacked() used pixelData?

Sorry for this long if statement. Allow me to explain a little bit.
The first half of the if statement says: check either one of these two items,
1. peekSucceed && pixmap.colorType() == SkColorType::kRGBA_8888_SkColorType
2. !peekSucceed

No.1 means peekPixels succeed and the pixelData is stored in RGBA format
No.2 means peekPixels failed, in which case we use the pixelData returned from
copyBitmapData() function and that pixelData is always in RGBA format

If anyone of these two is true, and format == GL_RGBA && type ==
GL_UNSIGNED_BYTE, then we don't need to convert.

Does that make sense?

[Zhenyao Mo, 2016-05-27 14:08:30]

lgtm

https://codereview.chromium.org/2016043002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp
(right):

https://codereview.chromium.org/2016043002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp:1162: if
(((peekSucceed && pixmap.colorType() == SkColorType::kRGBA_8888_SkColorType) ||
!peekSucceed) && format == GL_RGBA && type == GL_UNSIGNED_BYTE) {
On 2016/05/27 13:54:44, xidachen wrote:
> On 2016/05/27 13:49:21, Zhenyao Mo wrote:
> > if peekSucceed is false, why needConversion is set to false? Isn't the above
> > code if skImage->isTextureBacked() used pixelData?
> 
> Sorry for this long if statement. Allow me to explain a little bit.
> The first half of the if statement says: check either one of these two items,
> 1. peekSucceed && pixmap.colorType() == SkColorType::kRGBA_8888_SkColorType
> 2. !peekSucceed
> 
> No.1 means peekPixels succeed and the pixelData is stored in RGBA format
> No.2 means peekPixels failed, in which case we use the pixelData returned from
> copyBitmapData() function and that pixelData is always in RGBA format
> 
> If anyone of these two is true, and format == GL_RGBA && type ==
> GL_UNSIGNED_BYTE, then we don't need to convert.
> 
> Does that make sense?

You are right.  I got confused. Should not read code at this early hour.

[Justin Novosad, 2016-05-27 14:16:19]

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

https://codereview.chromium.org/2016043002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/ImageBitmap.cpp:151: if (premultiplyAlpha
&& imageFormat == PremultiplyAlpha)
Unless I am misreading this, I think you mean "premultiplyAlpha && imageFormat
== DontPremultiplyAlpha"

https://codereview.chromium.org/2016043002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/ImageBitmap.cpp:199: if
(!skImage->peekPixels(&pixmap)) {
This condition will force an immediate readback of texture backed images, which
I don't think is what you intend here.

https://codereview.chromium.org/2016043002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp
(right):

https://codereview.chromium.org/2016043002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp:1154:
uint8_t* pixelData = nullptr;
Should use an OwnPtr for this.

https://codereview.chromium.org/2016043002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp:1162: if
(((peekSucceed && pixmap.colorType() == SkColorType::kRGBA_8888_SkColorType) ||
!peekSucceed) && format == GL_RGBA && type == GL_UNSIGNED_BYTE) {
Please break-up long expressions make the code more readable. Split it into
intermediate steps.
Ex: bool havePeekableRGBA8 = ...  bool needReadback = ... etc.

[xidachen, 2016-05-27 15:10:13]

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

https://codereview.chromium.org/2016043002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/ImageBitmap.cpp:151: if (premultiplyAlpha
&& imageFormat == PremultiplyAlpha)
On 2016/05/27 14:16:18, Justin Novosad wrote:
> Unless I am misreading this, I think you mean "premultiplyAlpha && imageFormat
> == DontPremultiplyAlpha"

Made changes so that imageFormat == DontPremultiplyAlpha means the image is in
unpremul format.

https://codereview.chromium.org/2016043002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/ImageBitmap.cpp:199: if
(!skImage->peekPixels(&pixmap)) {
On 2016/05/27 14:16:18, Justin Novosad wrote:
> This condition will force an immediate readback of texture backed images,
which
> I don't think is what you intend here.

Double checked with fmalita@, peekPixels() will not trigger a readback. It
returns false and ignore the pixmap completely, no readback is involved.

https://codereview.chromium.org/2016043002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp
(right):

https://codereview.chromium.org/2016043002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp:1154:
uint8_t* pixelData = nullptr;
On 2016/05/27 14:16:19, Justin Novosad wrote:
> Should use an OwnPtr for this.

I don't think we can do that. SkImage is in charge of the lifetime of the pixel
data. If we make this OwnPtr, then we are trying to take the ownership which
causes problem. I got a "corrupted memory" when I tried to use OwnPtr here.

https://codereview.chromium.org/2016043002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp:1162: if
(((peekSucceed && pixmap.colorType() == SkColorType::kRGBA_8888_SkColorType) ||
!peekSucceed) && format == GL_RGBA && type == GL_UNSIGNED_BYTE) {
On 2016/05/27 14:16:19, Justin Novosad wrote:
> Please break-up long expressions make the code more readable. Split it into
> intermediate steps.
> Ex: bool havePeekableRGBA8 = ...  bool needReadback = ... etc.

Done.

[Justin Novosad, 2016-05-27 16:13:48]

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

https://codereview.chromium.org/2016043002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/ImageBitmap.cpp:199: if
(!skImage->peekPixels(&pixmap)) {
That is not what I meant.
I was not referring to the peekPixels call. I was referring to the condition of
if statement being wrong. If skImage is texture backed this code will enter the
if statement.  Inside the "if" the call to drawImage will trigger a readback.
What I am suggesting is that the condition in the if statement should be
"!skImage->isTextureBacked() && !skImage->peekPixels(&pixmap)" in order to
prevent the readback in drawImage().

https://codereview.chromium.org/2016043002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp
(right):

https://codereview.chromium.org/2016043002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp:1154:
uint8_t* pixelData = nullptr;
On 2016/05/27 15:10:13, xidachen wrote:
> On 2016/05/27 14:16:19, Justin Novosad wrote:
> > Should use an OwnPtr for this.
> 
> I don't think we can do that. SkImage is in charge of the lifetime of the
pixel
> data. If we make this OwnPtr, then we are trying to take the ownership which
> causes problem. I got a "corrupted memory" when I tried to use OwnPtr here.

That just means you were not doing it right.
Not using OwnPtr here goes against the code style. You should only use raw ptrs
for unowned references (and GC stuff).  The rule helps prevent us from writing
code with memory leaks. This code actually proves that point: it has a memory
leak.  In the case where skImage is texture backed, pixelData is never freed. 
The tricky part is that you have a mix of owned vs unowned cases, using the same
variable.  Here is how you fix that:

OwnPtr<uint8_t> pixelData;
uint8_t* pixelDataPtr = nullptr;
if (peekSucceed)
    pixelDataPtr = static_cast<uint8_t*>(pixmap.writable_addr());
else if (skImage->isTextureBacked()) {
    pixelData = bitmap->copyBitmapData(bitmap->isPremultiplied() ?
PremultiplyAlpha : DontPremultiplyAlpha);
    pixelDataPtr = pixelData.get();
}

Then, in the code below, you replace uses of pixelData with pixelDataPtr.

[xidachen, 2016-05-27 17:14:56]

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

https://codereview.chromium.org/2016043002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/ImageBitmap.cpp:199: if
(!skImage->peekPixels(&pixmap)) {
On 2016/05/27 16:13:47, Justin Novosad wrote:
> That is not what I meant.
> I was not referring to the peekPixels call. I was referring to the condition
of
> if statement being wrong. If skImage is texture backed this code will enter
the
> if statement.  Inside the "if" the call to drawImage will trigger a readback.
> What I am suggesting is that the condition in the if statement should be
> "!skImage->isTextureBacked() && !skImage->peekPixels(&pixmap)" in order to
> prevent the readback in drawImage().

Yes, that makes sense, changed.

https://codereview.chromium.org/2016043002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp
(right):

https://codereview.chromium.org/2016043002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp:1154:
uint8_t* pixelData = nullptr;
On 2016/05/27 16:13:48, Justin Novosad wrote:
> On 2016/05/27 15:10:13, xidachen wrote:
> > On 2016/05/27 14:16:19, Justin Novosad wrote:
> > > Should use an OwnPtr for this.
> > 
> > I don't think we can do that. SkImage is in charge of the lifetime of the
> pixel
> > data. If we make this OwnPtr, then we are trying to take the ownership which
> > causes problem. I got a "corrupted memory" when I tried to use OwnPtr here.
> 
> That just means you were not doing it right.
> Not using OwnPtr here goes against the code style. You should only use raw
ptrs
> for unowned references (and GC stuff).  The rule helps prevent us from writing
> code with memory leaks. This code actually proves that point: it has a memory
> leak.  In the case where skImage is texture backed, pixelData is never freed. 
> The tricky part is that you have a mix of owned vs unowned cases, using the
same
> variable.  Here is how you fix that:
> 
> OwnPtr<uint8_t> pixelData;
> uint8_t* pixelDataPtr = nullptr;
> if (peekSucceed)
>     pixelDataPtr = static_cast<uint8_t*>(pixmap.writable_addr());
> else if (skImage->isTextureBacked()) {
>     pixelData = bitmap->copyBitmapData(bitmap->isPremultiplied() ?
> PremultiplyAlpha : DontPremultiplyAlpha);
>     pixelDataPtr = pixelData.get();
> }
> 
> Then, in the code below, you replace uses of pixelData with pixelDataPtr.

Yes, that makes perfect sense. Thanks for pointing it out.

[Justin Novosad, 2016-05-27 17:25:42]

lgtm

[xidachen, 2016-05-27 17:27:12]

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

[commit-bot: I haz the power, 2016-05-27 17:27:40]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2016043002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/2016043002/100001

[commit-bot: I haz the power, 2016-05-27 19:06:49]

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

[commit-bot: I haz the power, 2016-05-27 19:06:50]

Dry run: This issue passed the CQ dry run.

[xidachen, 2016-05-27 19:06:51]

The CQ bit was unchecked by xidachen@chromium.org

[xidachen, 2016-05-27 19:06:58]

The CQ bit was checked by xidachen@chromium.org

[xidachen, 2016-05-27 19:06:58]

The patchset sent to the CQ was uploaded after l-g-t-m from zmo@chromium.org
Link to the patchset: https://codereview.chromium.org/2016043002/#ps100001
(title: "change to OwnPtr")

[commit-bot: I haz the power, 2016-05-27 19:07:30]

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

[commit-bot: I haz the power, 2016-05-27 19:12:54]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-05-27 19:14:46]

Description was changed from

==========
Avoid copy pixel data in texImage2D(ImageBitmap)

This is the first patch in order to avoid reading back GPU texture.
Currently in texImage2D(ImageBitmap), it makes a copy of the pixel data
stored inside the ImageBitmap (copyBitmapData). We should avoid that.
To solve that, we use SkImage's peekPixels() API. peekPixels() returns
true only when the ImageBitmap is stored in the CPU-side RAM. In the
case when an ImageBitmap is texture backed, this approach will not work.

I will have a follow up patch right after this, to handles the case where
an ImageBitmap is texture backed.

BUG=613411, 613409
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel
==========

to

==========
Avoid copy pixel data in texImage2D(ImageBitmap)

This is the first patch in order to avoid reading back GPU texture.
Currently in texImage2D(ImageBitmap), it makes a copy of the pixel data
stored inside the ImageBitmap (copyBitmapData). We should avoid that.
To solve that, we use SkImage's peekPixels() API. peekPixels() returns
true only when the ImageBitmap is stored in the CPU-side RAM. In the
case when an ImageBitmap is texture backed, this approach will not work.

I will have a follow up patch right after this, to handles the case where
an ImageBitmap is texture backed.

BUG=613411, 613409
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel

Committed: https://crrev.com/c44bccbcfded3d5906dc0fa79c9b0e810369b36c
Cr-Commit-Position: refs/heads/master@{#396529}
==========

[commit-bot: I haz the power, 2016-05-27 19:14:47]

Patchset 6 (id:??) landed as
https://crrev.com/c44bccbcfded3d5906dc0fa79c9b0e810369b36c
Cr-Commit-Position: refs/heads/master@{#396529}

[Ken Russell (switch to Gerrit), 2016-05-27 21:17:08]

Are all of these code paths well tested? My primary concern is the possibility
that there is a lossy step where the alpha channel is premultiplied in to the
color channels, and then has to be un-multiplied.

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

https://codereview.chromium.org/2016043002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/ImageBitmap.cpp:193: m_image =
cropImage(image->cachedImage()->getImage(), cropRect, flipY, premultiplyAlpha,
PremultiplyAlpha, ImageDecoder::GammaAndColorProfileApplied);
This change is concerning. It's important that PNGs with an alpha channel can be
used to construct an ImageBitmap without multiplying the alpha channel into the
color channels. Perhaps the new code supports this semantic, but it's difficult
to tell just by reading the code. Is this well tested?

https://codereview.chromium.org/2016043002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/ImageBitmap.cpp:202:
surface->getCanvas()->drawImage(skImage.get(), 0, 0);
This looks to me like it will always force the alpha channel to be multiplied in
to the color channels, which is supposed to be a configurable option. How is
this code path tested?

[xidachen, 2016-05-28 00:51:11]

Hi Ken,

Thanks for your comments. I do believe that we have enough conformance(2) tests
and layout tests in our code base to cover all cases. Basically we have
createImageBitmap() from all possible sources, and with all combination of flipY
and premultiplyAlpha.

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

https://codereview.chromium.org/2016043002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/ImageBitmap.cpp:193: m_image =
cropImage(image->cachedImage()->getImage(), cropRect, flipY, premultiplyAlpha,
PremultiplyAlpha, ImageDecoder::GammaAndColorProfileApplied);
On 2016/05/27 21:17:08, Ken Russell wrote:
> This change is concerning. It's important that PNGs with an alpha channel can
be
> used to construct an ImageBitmap without multiplying the alpha channel into
the
> color channels. Perhaps the new code supports this semantic, but it's
difficult
> to tell just by reading the code. Is this well tested?

Actually there is no change in here in terms of code execution. Please refer to
the change in cropImage() function, I changed all the code that uses this
parameter from "DontPremultiplyAlpha" to "PremultiplyAlpha". The only reason for
this change is to make the code more readable. After this change, the 
"PremultiplyAlpha" indicates that the first parameter of cropImage() is already
in premultiplied format, which is more readable.

https://codereview.chromium.org/2016043002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/ImageBitmap.cpp:202:
surface->getCanvas()->drawImage(skImage.get(), 0, 0);
On 2016/05/27 21:17:08, Ken Russell wrote:
> This looks to me like it will always force the alpha channel to be multiplied
in
> to the color channels, which is supposed to be a configurable option. How is
> this code path tested?

I think the concern here is that this piece of code might be executed when
caller request the premultiplyAlpha=none. However, the code in cropImage() makes
sure that this won't happen. The reason is that the SkImage extracted from an
HTMLImageElement is always in premultiplied format, and if caller request
premultiplyAlpha=none, it will go into the part of code in cropImage() where a
decoder is generated to decode the SkImage. After that, the return value of
cropImage() is always in decoded state and peekPixels() always succeed.

As a matter of fact, I looked into the layout test:
tex-image-and-sub-image-2d-image-bitmap-from-image-bitmap-from-image.html, 
and this drawImage only gets called when premultiplyAlpha is set to
"premultiply" or "default". In the case when premultiplyAlpha="none", this piece
of code is never reached.