Fixing performance issue of creating imagebitmap from ImageData

Fixing performance issue of creating imagebitmap from ImageData.

BUG=538253
Committed: https://crrev.com/1db4c96ec3d12c0a121325d1b6e63ecd4bbf0ed8
Cr-Commit-Position: refs/heads/master@{#354060}

[xidachen, 2015-10-01 15:41:10]

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

[xidachen, 2015-10-01 15:42:12]

Hi Justin,

Please review this CL.

Thanks.

[chrishtr, 2015-10-01 17:04:49]

chrishtr@chromium.org changed reviewers:
+ chrishtr@chromium.org

[chrishtr, 2015-10-01 17:04:49]

Please do create a bug, and mark it as blocking your image bitmap tracking bug.
Also, CL descriptions should not have commentary like your second sentence, that
should be in message comments.

[Justin Novosad, 2015-10-03 03:22:55]

Is this performance improvement captured by an existing test? I think it is not.
I would suggest adding a simple benchmark in
third_party/Webkit/PerformanceTests/Canvas. You should land the benchmark first
so that we get to see the impact of this change.

https://codereview.chromium.org/1382883002/diff/1/third_party/WebKit/Source/p...
File
third_party/WebKit/Source/platform/graphics/UnacceleratedSimpleImageBufferSurface.cpp
(left):

https://codereview.chromium.org/1382883002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/UnacceleratedSimpleImageBufferSurface.cpp:50:
clear();
This is too small of a difference to justify creating a new class.  You should
just add an alternate constructor to UnacceleratedImageBufferSurface or add a
parameter to the existing constructor

https://codereview.chromium.org/1382883002/diff/1/third_party/WebKit/Source/p...
File
third_party/WebKit/Source/platform/graphics/UnacceleratedSimpleImageBufferSurface.h
(right):

https://codereview.chromium.org/1382883002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/UnacceleratedSimpleImageBufferSurface.h:2:
* Copyright (c) 2013, Google Inc. All rights reserved.
This is not the header we put in new files. See
https://www.chromium.org/blink/coding-style#TOC-License

[xidachen, 2015-10-05 14:21:34]

Fix the issues according to Justin's comments. 
1. Add a benchmark test. To run the test,
"--enable-experimental-canvas-features" is required. Improvement is about 25% on
a 1024*1024 image source, measured on my desktop.
2. Delete the new class that was created, add a new argument on the existing
class.

[xidachen, 2015-10-05 14:21:34]

Fix the issues according to Justin's comments. 
1. Add a benchmark test. To run the test,
"--enable-experimental-canvas-features" is required. Improvement is about 25% on
a 1024*1024 image source, measured on my desktop.
2. Delete the new class that was created, add a new argument on the existing
class.

[Justin Novosad, 2015-10-05 19:22:05]

Could you submit the benchmark in a separate change, which we will land first.
That way we'll capture the impact of the optimization.

[xidachen, 2015-10-14 12:35:50]

Hi Justin,

The benchmark test has been landed in this CL:
https://codereview.chromium.org/1387933002/

This CL is for fixing the performance issue. Please review.

Thank you.

[Justin Novosad, 2015-10-14 14:25:28]

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

https://codereview.chromium.org/1382883002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/ImageBitmap.cpp:102: ImageBitmapSourceType
sourceType = FromImageData;
No need to use temporaries like this when you already have nicely named enum
types.

https://codereview.chromium.org/1382883002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/GraphicsTypes.h (right):

https://codereview.chromium.org/1382883002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/GraphicsTypes.h:83: enum
ImageBitmapSourceType {
The code would be more readable if this enum was more direct. I would call this
ImageInitializationMode { InitialializeImagePixels,
DoNotInitialializeImagePixels };

[xidachen, 2015-10-14 15:23:46]

Hi Justin,

Really great comments, thank you.

I have changed the enum type name and removed the temporary variables.

[xidachen, 2015-10-14 15:24:16]

Hi Justin,

Really great comments, thank you.

I have changed the enum type name and removed the temporary variables.

[Justin Novosad, 2015-10-14 15:40:37]

lgtm

[xidachen, 2015-10-14 17:01:38]

The CQ bit was checked by xidachen@chromium.org

[commit-bot: I haz the power, 2015-10-14 17:02:36]

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

[commit-bot: I haz the power, 2015-10-14 18:05:15]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-10-14 18:06:02]

Patchset 4 (id:??) landed as
https://crrev.com/1db4c96ec3d12c0a121325d1b6e63ecd4bbf0ed8
Cr-Commit-Position: refs/heads/master@{#354060}

[xidachen, 2015-10-20 18:31:53]

Patchset #5 (id:80001) has been deleted

[rickyz (no longer on Chrome), 2016-01-19 23:34:48]

rickyz@chromium.org changed reviewers:
+ rickyz@chromium.org

[rickyz (no longer on Chrome), 2016-01-19 23:34:49]

https://codereview.chromium.org/1382883002/diff/60001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/platform/graphics/UnacceleratedImageBufferSurface.cpp
(right):

https://codereview.chromium.org/1382883002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/UnacceleratedImageBufferSurface.cpp:49:
if (initializationMode == DoNotInitializeImagePixels) {
Is this logic correct? Shouldn't this clear initializationMode ==
InitializeImagePixels?

On the other hand, ClusterFuzz said that this CL fixed the issue, so maybe I'm
just seeing things?

[rickyz (no longer on Chrome), 2016-01-19 23:43:10]

https://codereview.chromium.org/1382883002/diff/60001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/platform/graphics/UnacceleratedImageBufferSurface.cpp
(right):

https://codereview.chromium.org/1382883002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/UnacceleratedImageBufferSurface.cpp:49:
if (initializationMode == DoNotInitializeImagePixels) {
On 2016/01/19 at 23:34:49, rickyz wrote:
> Is this logic correct? Shouldn't this clear initializationMode ==
InitializeImagePixels?
> 
> On the other hand, ClusterFuzz said that this CL fixed the issue, so maybe I'm
just seeing things?

Never mind, I see that this was fixed in a later CL
(https://codereview.chromium.org/1407393002).