Fixing performance issue of creating imagebitmap from ImageData (issue 1382883002 by xidachen@chromium.org)

2 views
Skip to first unread message

xida...@chromium.org

unread,
Oct 1, 2015, 11:42:13 AM10/1/15
to ju...@chromium.org, chromium...@chromium.org, pdr+graphi...@chromium.org, drott+bl...@chromium.org, caba...@adobe.com, blink-reviews-p...@chromium.org, dongseo...@intel.com, jbr...@chromium.org, ju...@chromium.org, dan...@chromium.org, dsch...@chromium.org, fma...@chromium.org, blink-...@chromium.org, sche...@chromium.org, rob....@samsung.com
Reviewers: Justin Novosad,

Message:
Hi Justin,

Please review this CL.

Thanks.

Description:
Fixing performance issue of creating imagebitmap from ImageData.
There is no bug related to this issue, maybe we should create one?

Please review this at https://codereview.chromium.org/1382883002/

Base URL: https://chromium.googlesource.com/chromium/src.git@master

Affected files (+29, -17 lines):
M third_party/WebKit/Source/core/frame/ImageBitmap.cpp
M third_party/WebKit/Source/platform/blink_platform.gypi
M third_party/WebKit/Source/platform/graphics/ImageBuffer.h
M third_party/WebKit/Source/platform/graphics/ImageBuffer.cpp
A +
third_party/WebKit/Source/platform/graphics/UnacceleratedSimpleImageBufferSurface.h
A +
third_party/WebKit/Source/platform/graphics/UnacceleratedSimpleImageBufferSurface.cpp


Index: third_party/WebKit/Source/core/frame/ImageBitmap.cpp
diff --git a/third_party/WebKit/Source/core/frame/ImageBitmap.cpp
b/third_party/WebKit/Source/core/frame/ImageBitmap.cpp
index
62388bc0fb578bd02059e4556ea598892a587f45..f07d175c2d6b0b1c3ce2dd5d80f1c0ed9899890c
100644
--- a/third_party/WebKit/Source/core/frame/ImageBitmap.cpp
+++ b/third_party/WebKit/Source/core/frame/ImageBitmap.cpp
@@ -98,7 +98,9 @@ ImageBitmap::ImageBitmap(ImageData* data, const IntRect&
cropRect)
, m_bitmapOffset(IntPoint())
{
IntRect srcRect = intersection(cropRect, IntRect(IntPoint(),
data->size()));
- OwnPtr<ImageBuffer> buffer = ImageBuffer::create(data->size());
+ OpacityMode opacityMode = Opaque;
+ OwnPtr<ImageBuffer> buffer = ImageBuffer::createSimple(data->size(),
opacityMode);
+
if (!buffer)
return;

Index: third_party/WebKit/Source/platform/blink_platform.gypi
diff --git a/third_party/WebKit/Source/platform/blink_platform.gypi
b/third_party/WebKit/Source/platform/blink_platform.gypi
index
2a8a406a455f37e2fd1fe24e1499d7da1f187f43..be0e390c36fff9f24ec91588c859d42cbb8972f7
100644
--- a/third_party/WebKit/Source/platform/blink_platform.gypi
+++ b/third_party/WebKit/Source/platform/blink_platform.gypi
@@ -582,6 +582,8 @@
'graphics/ThreadSafeDataTransport.h',
'graphics/UnacceleratedImageBufferSurface.cpp',
'graphics/UnacceleratedImageBufferSurface.h',
+ 'graphics/UnacceleratedSimpleImageBufferSurface.cpp',
+ 'graphics/UnacceleratedSimpleImageBufferSurface.h',
'graphics/cpu/arm/WebGLImageConversionNEON.h',
'graphics/cpu/x86/WebGLImageConversionSSE.h',
'graphics/filters/DistantLightSource.cpp',
Index: third_party/WebKit/Source/platform/graphics/ImageBuffer.cpp
diff --git a/third_party/WebKit/Source/platform/graphics/ImageBuffer.cpp
b/third_party/WebKit/Source/platform/graphics/ImageBuffer.cpp
index
d63909e4b47783dfea59bfdc68f16d8b8df9cda8..df902c960517c4c27b1e137126dee76fa1e68c57
100644
--- a/third_party/WebKit/Source/platform/graphics/ImageBuffer.cpp
+++ b/third_party/WebKit/Source/platform/graphics/ImageBuffer.cpp
@@ -41,6 +41,7 @@
#include "platform/graphics/ImageBufferClient.h"
#include "platform/graphics/StaticBitmapImage.h"
#include "platform/graphics/UnacceleratedImageBufferSurface.h"
+#include "platform/graphics/UnacceleratedSimpleImageBufferSurface.h"
#include "platform/graphics/gpu/DrawingBuffer.h"
#include "platform/graphics/gpu/Extensions3DUtil.h"
#include "platform/graphics/skia/SkiaUtils.h"
@@ -75,6 +76,14 @@ PassOwnPtr<ImageBuffer> ImageBuffer::create(const
IntSize& size, OpacityMode opa
return adoptPtr(new ImageBuffer(surface.release()));
}

+PassOwnPtr<ImageBuffer> ImageBuffer::createSimple(const IntSize& size,
OpacityMode opacityMode)
+{
+ OwnPtr<ImageBufferSurface> surface(adoptPtr(new
UnacceleratedSimpleImageBufferSurface(size, opacityMode)));
+ if (!surface->isValid())
+ return nullptr;
+ return adoptPtr(new ImageBuffer(surface.release()));
+}
+
ImageBuffer::ImageBuffer(PassOwnPtr<ImageBufferSurface> surface)
: m_snapshotState(InitialSnapshotState)
, m_surface(surface)
Index: third_party/WebKit/Source/platform/graphics/ImageBuffer.h
diff --git a/third_party/WebKit/Source/platform/graphics/ImageBuffer.h
b/third_party/WebKit/Source/platform/graphics/ImageBuffer.h
index
7d73455e2fc3ce7dde70951906fea587e7e6f0d1..47c9754b41b094d774bd7f9f922cfc1035c47b13
100644
--- a/third_party/WebKit/Source/platform/graphics/ImageBuffer.h
+++ b/third_party/WebKit/Source/platform/graphics/ImageBuffer.h
@@ -72,6 +72,7 @@ class PLATFORM_EXPORT ImageBuffer {
public:
static PassOwnPtr<ImageBuffer> create(const IntSize&, OpacityMode =
NonOpaque);
static PassOwnPtr<ImageBuffer> create(PassOwnPtr<ImageBufferSurface>);
+ static PassOwnPtr<ImageBuffer> createSimple(const IntSize&,
OpacityMode = NonOpaque);

~ImageBuffer();

Index:
third_party/WebKit/Source/platform/graphics/UnacceleratedSimpleImageBufferSurface.cpp
diff --git
a/third_party/WebKit/Source/platform/graphics/UnacceleratedImageBufferSurface.cpp
b/third_party/WebKit/Source/platform/graphics/UnacceleratedSimpleImageBufferSurface.cpp
similarity index 81%
copy from
third_party/WebKit/Source/platform/graphics/UnacceleratedImageBufferSurface.cpp
copy to
third_party/WebKit/Source/platform/graphics/UnacceleratedSimpleImageBufferSurface.cpp
index
f01b7a7c7a05ab75ca9b3ab42603c815cefebff0..060b198070edb2f06d3b3ead77bb914886029826
100644
---
a/third_party/WebKit/Source/platform/graphics/UnacceleratedImageBufferSurface.cpp
+++
b/third_party/WebKit/Source/platform/graphics/UnacceleratedSimpleImageBufferSurface.cpp
@@ -29,7 +29,7 @@
*/

#include "config.h"
-#include "platform/graphics/UnacceleratedImageBufferSurface.h"
+#include "platform/graphics/UnacceleratedSimpleImageBufferSurface.h"

#include "third_party/skia/include/core/SkCanvas.h"
#include "third_party/skia/include/core/SkDevice.h"
@@ -38,37 +38,34 @@

namespace blink {

-UnacceleratedImageBufferSurface::UnacceleratedImageBufferSurface(const
IntSize& size, OpacityMode opacityMode)
+UnacceleratedSimpleImageBufferSurface::UnacceleratedSimpleImageBufferSurface(const
IntSize&
size, OpacityMode opacityMode)
: ImageBufferSurface(size, opacityMode)
{
SkAlphaType alphaType = (Opaque == opacityMode) ?
kOpaque_SkAlphaType : kPremul_SkAlphaType;
SkImageInfo info = SkImageInfo::MakeN32(size.width(), size.height(),
alphaType);
SkSurfaceProps disableLCDProps(0, kUnknown_SkPixelGeometry);
m_surface = adoptRef(SkSurface::NewRaster(info, Opaque ==
opacityMode ? 0 : &disableLCDProps));
-
- if (m_surface)
- clear();
}

-UnacceleratedImageBufferSurface::~UnacceleratedImageBufferSurface() { }
+UnacceleratedSimpleImageBufferSurface::~UnacceleratedSimpleImageBufferSurface()
{
}

-SkCanvas* UnacceleratedImageBufferSurface::canvas()
+SkCanvas* UnacceleratedSimpleImageBufferSurface::canvas()
{
return m_surface->getCanvas();
}

-const SkBitmap&
UnacceleratedImageBufferSurface::deprecatedBitmapForOverwrite()
+const SkBitmap&
UnacceleratedSimpleImageBufferSurface::deprecatedBitmapForOverwrite()
{

m_surface->notifyContentWillChange(SkSurface::kDiscard_ContentChangeMode);
return m_surface->getCanvas()->getDevice()->accessBitmap(false);
}

-bool UnacceleratedImageBufferSurface::isValid() const
+bool UnacceleratedSimpleImageBufferSurface::isValid() const
{
return m_surface;
}

-PassRefPtr<SkImage>
UnacceleratedImageBufferSurface::newImageSnapshot(AccelerationHint)
+PassRefPtr<SkImage>
UnacceleratedSimpleImageBufferSurface::newImageSnapshot(AccelerationHint)
{
return adoptRef(m_surface->newImageSnapshot());
}
Index:
third_party/WebKit/Source/platform/graphics/UnacceleratedSimpleImageBufferSurface.h
diff --git
a/third_party/WebKit/Source/platform/graphics/UnacceleratedImageBufferSurface.h
b/third_party/WebKit/Source/platform/graphics/UnacceleratedSimpleImageBufferSurface.h
similarity index 81%
copy from
third_party/WebKit/Source/platform/graphics/UnacceleratedImageBufferSurface.h
copy to
third_party/WebKit/Source/platform/graphics/UnacceleratedSimpleImageBufferSurface.h
index
bc1139c1e23027497922aae1bd19948ff6d04559..d337c1c2bca3ff58634b27a7298375f5a244c1db
100644
---
a/third_party/WebKit/Source/platform/graphics/UnacceleratedImageBufferSurface.h
+++
b/third_party/WebKit/Source/platform/graphics/UnacceleratedSimpleImageBufferSurface.h
@@ -28,8 +28,8 @@
* OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
*/

-#ifndef UnacceleratedImageBufferSurface_h
-#define UnacceleratedImageBufferSurface_h
+#ifndef UnacceleratedSimpleImageBufferSurface_h
+#define UnacceleratedSimpleImageBufferSurface_h

#include "platform/graphics/ImageBufferSurface.h"
#include "wtf/RefPtr.h"
@@ -38,11 +38,11 @@ class SkSurface;

namespace blink {

-class PLATFORM_EXPORT UnacceleratedImageBufferSurface : public
ImageBufferSurface {
- WTF_MAKE_NONCOPYABLE(UnacceleratedImageBufferSurface);
WTF_MAKE_FAST_ALLOCATED(UnacceleratedImageBufferSurface);
+class PLATFORM_EXPORT UnacceleratedSimpleImageBufferSurface : public
ImageBufferSurface {
+ WTF_MAKE_NONCOPYABLE(UnacceleratedSimpleImageBufferSurface);
WTF_MAKE_FAST_ALLOCATED(UnacceleratedSimpleImageBufferSurface);
public:
- UnacceleratedImageBufferSurface(const IntSize&, OpacityMode =
NonOpaque);
- ~UnacceleratedImageBufferSurface() override;
+ UnacceleratedSimpleImageBufferSurface(const IntSize&, OpacityMode =
NonOpaque);
+ ~UnacceleratedSimpleImageBufferSurface() override;

SkCanvas* canvas() override;
bool isValid() const override;
@@ -56,3 +56,4 @@ private:
} // namespace blink

#endif
+


chri...@chromium.org

unread,
Oct 1, 2015, 1:04:49 PM10/1/15
to xida...@chromium.org, ju...@chromium.org, blink-...@chromium.org, blink-reviews-p...@chromium.org, caba...@adobe.com, chromium...@chromium.org, dan...@chromium.org, dongseo...@intel.com, drott+bl...@chromium.org, dsch...@chromium.org, fma...@chromium.org, jbr...@chromium.org, ju...@chromium.org, pdr+graphi...@chromium.org, rob....@samsung.com, sche...@chromium.org
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.

https://codereview.chromium.org/1382883002/

ju...@chromium.org

unread,
Oct 2, 2015, 11:22:55 PM10/2/15
to xida...@chromium.org, chri...@chromium.org, blink-...@chromium.org, blink-reviews-p...@chromium.org, caba...@adobe.com, chromium...@chromium.org, dan...@chromium.org, dongseo...@intel.com, drott+bl...@chromium.org, dsch...@chromium.org, fma...@chromium.org, jbr...@chromium.org, pdr+graphi...@chromium.org, rob....@samsung.com, sche...@chromium.org
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/platform/graphics/UnacceleratedSimpleImageBufferSurface.cpp
File
third_party/WebKit/Source/platform/graphics/UnacceleratedSimpleImageBufferSurface.cpp
(left):

https://codereview.chromium.org/1382883002/diff/1/third_party/WebKit/Source/platform/graphics/UnacceleratedSimpleImageBufferSurface.cpp#oldcode50
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/platform/graphics/UnacceleratedSimpleImageBufferSurface.h
File
third_party/WebKit/Source/platform/graphics/UnacceleratedSimpleImageBufferSurface.h
(right):

https://codereview.chromium.org/1382883002/diff/1/third_party/WebKit/Source/platform/graphics/UnacceleratedSimpleImageBufferSurface.h#newcode2
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

https://codereview.chromium.org/1382883002/

xida...@chromium.org

unread,
Oct 5, 2015, 10:21:34 AM10/5/15
to ju...@chromium.org, chri...@chromium.org, blink-...@chromium.org, blink-reviews-p...@chromium.org, caba...@adobe.com, chromium...@chromium.org, dan...@chromium.org, dongseo...@intel.com, drott+bl...@chromium.org, dsch...@chromium.org, fma...@chromium.org, jbr...@chromium.org, ju...@chromium.org, pdr+graphi...@chromium.org, rob....@samsung.com, sche...@chromium.org
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.

https://codereview.chromium.org/1382883002/

xida...@chromium.org

unread,
Oct 5, 2015, 10:21:34 AM10/5/15
to ju...@chromium.org, chri...@chromium.org, blink-...@chromium.org, blink-reviews-p...@chromium.org, caba...@adobe.com, chromium...@chromium.org, dan...@chromium.org, dongseo...@intel.com, drott+bl...@chromium.org, dsch...@chromium.org, fma...@chromium.org, jbr...@chromium.org, ju...@chromium.org, pdr+graphi...@chromium.org, rob....@samsung.com, sche...@chromium.org

ju...@chromium.org

unread,
Oct 5, 2015, 3:22:05 PM10/5/15
to xida...@chromium.org, chri...@chromium.org, blink-...@chromium.org, blink-reviews-p...@chromium.org, caba...@adobe.com, chromium...@chromium.org, dan...@chromium.org, dongseo...@intel.com, drott+bl...@chromium.org, dsch...@chromium.org, fma...@chromium.org, jbr...@chromium.org, pdr+graphi...@chromium.org, rob....@samsung.com, sche...@chromium.org

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

https://codereview.chromium.org/1382883002/

xida...@chromium.org

unread,
Oct 14, 2015, 8:35:52 AM10/14/15
to ju...@chromium.org, chri...@chromium.org, blink-...@chromium.org, blink-reviews-p...@chromium.org, caba...@adobe.com, chromium...@chromium.org, dan...@chromium.org, dongseo...@intel.com, drott+bl...@chromium.org, dsch...@chromium.org, fma...@chromium.org, jbr...@chromium.org, pdr+graphi...@chromium.org, rob....@samsung.com, sche...@chromium.org
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.

https://codereview.chromium.org/1382883002/

ju...@chromium.org

unread,
Oct 14, 2015, 10:25:29 AM10/14/15
to xida...@chromium.org, chri...@chromium.org, blink-...@chromium.org, blink-reviews-p...@chromium.org, caba...@adobe.com, chromium...@chromium.org, dan...@chromium.org, dongseo...@intel.com, drott+bl...@chromium.org, dsch...@chromium.org, fma...@chromium.org, jbr...@chromium.org, pdr+graphi...@chromium.org, rob....@samsung.com, sche...@chromium.org

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

https://codereview.chromium.org/1382883002/diff/40001/third_party/WebKit/Source/core/frame/ImageBitmap.cpp#newcode102
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/Source/platform/graphics/GraphicsTypes.h
File third_party/WebKit/Source/platform/graphics/GraphicsTypes.h
(right):

https://codereview.chromium.org/1382883002/diff/40001/third_party/WebKit/Source/platform/graphics/GraphicsTypes.h#newcode83
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 };

https://codereview.chromium.org/1382883002/

xida...@chromium.org

unread,
Oct 14, 2015, 11:23:47 AM10/14/15
to ju...@chromium.org, chri...@chromium.org, blink-...@chromium.org, blink-reviews-p...@chromium.org, caba...@adobe.com, chromium...@chromium.org, dan...@chromium.org, dongseo...@intel.com, drott+bl...@chromium.org, dsch...@chromium.org, fma...@chromium.org, jbr...@chromium.org, pdr+graphi...@chromium.org, rob....@samsung.com, sche...@chromium.org
Hi Justin,

Really great comments, thank you.

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

https://codereview.chromium.org/1382883002/

xida...@chromium.org

unread,
Oct 14, 2015, 11:24:16 AM10/14/15
to ju...@chromium.org, chri...@chromium.org, blink-...@chromium.org, blink-reviews-p...@chromium.org, caba...@adobe.com, chromium...@chromium.org, dan...@chromium.org, dongseo...@intel.com, drott+bl...@chromium.org, dsch...@chromium.org, fma...@chromium.org, jbr...@chromium.org, pdr+graphi...@chromium.org, rob....@samsung.com, sche...@chromium.org

ju...@chromium.org

unread,
Oct 14, 2015, 11:40:37 AM10/14/15
to xida...@chromium.org, chri...@chromium.org, blink-...@chromium.org, blink-reviews-p...@chromium.org, caba...@adobe.com, chromium...@chromium.org, dan...@chromium.org, dongseo...@intel.com, drott+bl...@chromium.org, dsch...@chromium.org, fma...@chromium.org, jbr...@chromium.org, pdr+graphi...@chromium.org, rob....@samsung.com, sche...@chromium.org
lgtm

https://codereview.chromium.org/1382883002/

commit-bot@chromium.org via codereview.chromium.org

unread,
Oct 14, 2015, 1:02:36 PM10/14/15
to xida...@chromium.org, ju...@chromium.org, chri...@chromium.org, commi...@chromium.org, blink-...@chromium.org, blink-reviews-p...@chromium.org, caba...@adobe.com, chromium...@chromium.org, dan...@chromium.org, dongseo...@intel.com, drott+bl...@chromium.org, dsch...@chromium.org, fma...@chromium.org, jbr...@chromium.org, pdr+graphi...@chromium.org, rob....@samsung.com, sche...@chromium.org
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


https://codereview.chromium.org/1382883002/

commit-bot@chromium.org via codereview.chromium.org

unread,
Oct 14, 2015, 2:05:16 PM10/14/15
to xida...@chromium.org, ju...@chromium.org, chri...@chromium.org, commi...@chromium.org, blink-...@chromium.org, blink-reviews-p...@chromium.org, caba...@adobe.com, chromium...@chromium.org, dan...@chromium.org, dongseo...@intel.com, drott+bl...@chromium.org, dsch...@chromium.org, fma...@chromium.org, jbr...@chromium.org, pdr+graphi...@chromium.org, rob....@samsung.com, sche...@chromium.org
Committed patchset #4 (id:60001)

https://codereview.chromium.org/1382883002/

commit-bot@chromium.org via codereview.chromium.org

unread,
Oct 14, 2015, 2:06:05 PM10/14/15
to xida...@chromium.org, ju...@chromium.org, chri...@chromium.org, commi...@chromium.org, blink-...@chromium.org, blink-reviews-p...@chromium.org, caba...@adobe.com, chromium...@chromium.org, dan...@chromium.org, dongseo...@intel.com, drott+bl...@chromium.org, dsch...@chromium.org, fma...@chromium.org, jbr...@chromium.org, pdr+graphi...@chromium.org, rob....@samsung.com, sche...@chromium.org
Patchset 4 (id:??) landed as
https://crrev.com/1db4c96ec3d12c0a121325d1b6e63ecd4bbf0ed8
Cr-Commit-Position: refs/heads/master@{#354060}

https://codereview.chromium.org/1382883002/
Reply all
Reply to author
Forward
0 new messages