Make 2D canvas disable gpu acceleration when getImageData is called

Make 2D canvas disable gpu acceleration when getImageData is called

In order to optimize performance, GPU-accelerated canvases should
permanently switch out of accalerated mode as soon as
getImageData is called.  The cost of getImageData on the GPU is
so prohibitively high that it almost always outways the benefit
of acceleration.

This is the first time we implement a SW/GPU switch that can happen
after the first frame was presented, which may result in a small one
time glitch due to rendering engine discrepancies. Let's see if we get
any complaints about this.  If there are complaints, we may inhibit
the switch based on what kind of content was rendered to the canvas.

BUG=606688
Committed: https://crrev.com/33732aec33c88cb18c212544da7d0a4ba53cd9ce
Cr-Commit-Position: refs/heads/master@{#401751}

[Justin Novosad, 2016-06-10 21:49:37]

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

[Justin Novosad, 2016-06-10 21:49:38]

PTAL

[xidachen, 2016-06-15 14:12:36]

https://codereview.chromium.org/2063473002/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/graphics/Canvas2DLayerBridge.cpp
(right):

https://codereview.chromium.org/2063473002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/Canvas2DLayerBridge.cpp:483: }
By looking at the test results failure, it looks like we need some kind of
enableAcceleration() at some point to make the m_surface be a acclerated
SkSurface.

https://codereview.chromium.org/2063473002/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/graphics/ImageBuffer.cpp (right):

https://codereview.chromium.org/2063473002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/ImageBuffer.cpp:304:
AccelerationHint hint =
ExpensiveCanvasHeuristicParameters::GetImageDataForcesNoAcceleration ?
ForceNoAcceleration : PreferNoAcceleration;
We should probably do the same thing in putImageData() as well.

[xlai (Olivia), 2016-06-15 17:54:47]

I have two high-level comments:
1. Should you update GPU memory usage when you do the GPU/SW switch?
2. There are quite a few "restore" functions scattered in
2DRenderingCanvasContext, ImageBuffer and so on. How does this patch interfere
with all these "restore" functions?

[Justin Novosad, 2016-06-16 18:52:08]

On 2016/06/15 14:12:36, xidachen wrote:
>
https://codereview.chromium.org/2063473002/diff/1/third_party/WebKit/Source/p...
> File third_party/WebKit/Source/platform/graphics/Canvas2DLayerBridge.cpp
> (right):
> 
>
https://codereview.chromium.org/2063473002/diff/1/third_party/WebKit/Source/p...
> third_party/WebKit/Source/platform/graphics/Canvas2DLayerBridge.cpp:483: }
> By looking at the test results failure, it looks like we need some kind of
> enableAcceleration() at some point to make the m_surface be a acclerated
> SkSurface.

Actually, what was missing was a paint invalidation after the switch. Also, some
of the tests were failing due to accelerated vs non-accelerated discrepancies.
To solve, that and to make sure we do not lose test coverage, I am going to add
a run-time flag that blocks the switching when executing layout tests.
> 
>
https://codereview.chromium.org/2063473002/diff/1/third_party/WebKit/Source/p...
> File third_party/WebKit/Source/platform/graphics/ImageBuffer.cpp (right):
> 
>
https://codereview.chromium.org/2063473002/diff/1/third_party/WebKit/Source/p...
> third_party/WebKit/Source/platform/graphics/ImageBuffer.cpp:304:
> AccelerationHint hint =
> ExpensiveCanvasHeuristicParameters::GetImageDataForcesNoAcceleration ?
> ForceNoAcceleration : PreferNoAcceleration;
> We should probably do the same thing in putImageData() as well.

Yes, but put image data is not as critical.  IT is not clear to me whether we
want to force vs prefer vs something else.  Sebastien will be looking into
creating better heuristics around this.

[Justin Novosad, 2016-06-17 14:25:50]

On 2016/06/15 17:54:47, xlai (Olivia) wrote:
> I have two high-level comments:
> 1. Should you update GPU memory usage when you do the GPU/SW switch?

Fixed and tested.

> 2. There are quite a few "restore" functions scattered in
> 2DRenderingCanvasContext, ImageBuffer and so on. How does this patch interfere
> with all these "restore" functions?

Not an issue. Switch canoot happen while a restore is pending because
getImageData exits early if !checkSurfaceIsValid(). Also restore() in not
possible once switch to SW is complete because at that point the canvas is no
longer tied to a gpu context. (restore is for restoring from a lost gpu context)


PTAL

[Justin Novosad, 2016-06-23 18:33:02]

New Patch

[xlai (Olivia), 2016-06-23 19:55:02]

lgtm with nits

https://codereview.chromium.org/2063473002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/Canvas2DImageBufferSurface.h
(right):

https://codereview.chromium.org/2063473002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/Canvas2DImageBufferSurface.h:50: :
ImageBufferSurface(size, opacityMode)
I think you could take away the opacityMode para from the constructor and just
deduce it from bridge->opacityMode. This is to avoid potential use of
constructor with two diffferent opacity mode for ImageBufferSurface and bridge.

https://codereview.chromium.org/2063473002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/canvas2d/CanvasRenderingContext2DTest.cpp
(right):

https://codereview.chromium.org/2063473002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/canvas2d/CanvasRenderingContext2DTest.cpp:778:
RuntimeEnabledFeatures::setCanvas2dFixedRenderingModeEnabled(false);
Nits: Why don't you set the Canvas2dFixedRenderingModeEnabled=false at the
beginning of the test (before 773)? In theory, as long as you don't call
getImageData, the ForceDisableAcceleration should not happen.

[Justin Novosad, 2016-06-23 21:02:09]

https://codereview.chromium.org/2063473002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/Canvas2DImageBufferSurface.h
(right):

https://codereview.chromium.org/2063473002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/Canvas2DImageBufferSurface.h:50: :
ImageBufferSurface(size, opacityMode)
On 2016/06/23 19:55:02, xlai (Olivia) wrote:
> I think you could take away the opacityMode para from the constructor and just
> deduce it from bridge->opacityMode. This is to avoid potential use of
> constructor with two diffferent opacity mode for ImageBufferSurface and
bridge.

Done.

https://codereview.chromium.org/2063473002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/canvas2d/CanvasRenderingContext2DTest.cpp
(right):

https://codereview.chromium.org/2063473002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/canvas2d/CanvasRenderingContext2DTest.cpp:778:
RuntimeEnabledFeatures::setCanvas2dFixedRenderingModeEnabled(false);
On 2016/06/23 19:55:02, xlai (Olivia) wrote:
> Nits: Why don't you set the Canvas2dFixedRenderingModeEnabled=false at the
> beginning of the test (before 773)? In theory, as long as you don't call
> getImageData, the ForceDisableAcceleration should not happen.
Done.

[Justin Novosad, 2016-06-23 21:02:19]

The CQ bit was checked by junov@chromium.org

[Justin Novosad, 2016-06-23 21:02:20]

The patchset sent to the CQ was uploaded after l-g-t-m from xlai@chromium.org
Link to the patchset: https://codereview.chromium.org/2063473002/#ps100001
(title: "address comments and fix test failures")

[commit-bot: I haz the power, 2016-06-23 21:02:49]

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

[commit-bot: I haz the power, 2016-06-23 23:20:26]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-06-23 23:22:17]

Description was changed from

==========
Make 2D canvas disable gpu acceleration when getImageData is called

In order to optimize performance, GPU-accelerated canvases should
permanently switch out of accalerated mode as soon as
getImageData is called.  The cost of getImageData on the GPU is
so prohibitively high that it almost always outways the benefit
of acceleration.

This is the first time we implement a SW/GPU switch that can happen
after the first frame was presented, which may result in a small one
time glitch due to rendering engine discrepancies. Let's see if we get
any complaints about this.  If there are complaints, we may inhibit
the switch based on what kind of content was rendered to the canvas.

BUG=606688
==========

to

==========
Make 2D canvas disable gpu acceleration when getImageData is called

In order to optimize performance, GPU-accelerated canvases should
permanently switch out of accalerated mode as soon as
getImageData is called.  The cost of getImageData on the GPU is
so prohibitively high that it almost always outways the benefit
of acceleration.

This is the first time we implement a SW/GPU switch that can happen
after the first frame was presented, which may result in a small one
time glitch due to rendering engine discrepancies. Let's see if we get
any complaints about this.  If there are complaints, we may inhibit
the switch based on what kind of content was rendered to the canvas.

BUG=606688

Committed: https://crrev.com/33732aec33c88cb18c212544da7d0a4ba53cd9ce
Cr-Commit-Position: refs/heads/master@{#401751}
==========

[commit-bot: I haz the power, 2016-06-23 23:22:19]

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

[sunnyps, 2016-06-28 00:02:18]

A revert of this CL (patchset #6 id:100001) has been created in
https://codereview.chromium.org/2101823002/ by sunnyps@chromium.org.

The reason for reverting is: Causing crash in mailbox release callback - bug
623101.