Make 2D canvas smarter about chosing whether or not to use GPU acceleration

Make 2D canvas smarter about chosing whether or not to use GPU acceleration

The CL takes advantage of the fact that canvas rendering is deferred
to defer the decision of whether or not a canvas should be
GPU-accelerated. The decision is made based on the conditions
under which a canvas is flushed for the first time. For example,
if the purpose of the first flush is to perform a readback, then
GPU-acceleration will be disabled for the canvas in order to avoid
reading GPU memory into RAM, which is known to be a slow path.

BUG=265849

[Justin Novosad, 2015-09-09 18:31:23]

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

[Justin Novosad, 2015-09-09 18:31:23]

PTAL.

This one can wait for senorblanco to return from vacation.

[Justin Novosad, 2015-09-15 16:27:42]

senorblanco ping.

[Stephen White, 2015-09-15 20:43:58]

Great to see this functionality get implemented.

https://codereview.chromium.org/1307143005/diff/20001/Source/platform/graphic...
File Source/platform/graphics/Canvas2DLayerBridge.cpp (right):

https://codereview.chromium.org/1307143005/diff/20001/Source/platform/graphic...
Source/platform/graphics/Canvas2DLayerBridge.cpp:187: m_surface =
createSkSurface(accelerate ? m_contextProvider->grContext() : nullptr, m_size,
m_msaaSampleCount, m_opacityMode);
As discussed, let's make sure we have a software fallback if accelerated surface
creation fails.

https://codereview.chromium.org/1307143005/diff/20001/Source/platform/graphic...
File Source/platform/graphics/Canvas2DLayerBridge.h (right):

https://codereview.chromium.org/1307143005/diff/20001/Source/platform/graphic...
Source/platform/graphics/Canvas2DLayerBridge.h:57: ForceAcceleration,
Please rename ForceAcceleration -> ForceAccelerationForTesting.

https://codereview.chromium.org/1307143005/diff/20001/Source/platform/graphic...
Source/platform/graphics/Canvas2DLayerBridge.h:101: SkSurface*
surface(AccelerationHint = PreferAcceleration);
<bikeshed> Prefer a name that indicates that this is not just an accessor.
getOrCreateSurface(), or ensureSurface(), or something. </bikeshed>

[Justin Novosad, 2015-09-16 15:39:43]

done.

[Justin Novosad, 2015-09-16 15:43:25]

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

[commit-bot: I haz the power, 2015-09-16 15:43:43]

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

[commit-bot: I haz the power, 2015-09-16 15:45:01]

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

[commit-bot: I haz the power, 2015-09-16 15:45:02]

Dry run: Try jobs failed on following builders:
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Justin Novosad, 2015-09-16 17:47:41]

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

[Justin Novosad, 2015-09-16 17:47:57]

Did a rebase

[commit-bot: I haz the power, 2015-09-16 17:48:10]

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

[Stephen White, 2015-09-16 18:19:33]

https://codereview.chromium.org/1307143005/diff/60001/Source/platform/graphic...
File Source/platform/graphics/Canvas2DLayerBridge.cpp (right):

https://codereview.chromium.org/1307143005/diff/60001/Source/platform/graphic...
Source/platform/graphics/Canvas2DLayerBridge.cpp:72: if (!surface)
IWBN if we had a test for this.

[commit-bot: I haz the power, 2015-09-16 18:54:35]

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

[commit-bot: I haz the power, 2015-09-16 18:54:35]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Justin Novosad, 2015-09-21 19:30:42]

The CQ bit was checked by junov@chromium.org

[Justin Novosad, 2015-09-21 19:30:48]

The CQ bit was unchecked by junov@chromium.org

[Justin Novosad, 2015-09-22 19:12:33]

On 2015/09/16 18:19:33, Stephen White wrote:
> Source/platform/graphics/Canvas2DLayerBridge.cpp:72: if (!surface)
> IWBN if we had a test for this.
Done.

I added unit test Canvas2DLayerBridge.fallbackToSoftwareIfContextLost(), which
exercises a failure to allocate on the GPU.

[Justin Novosad, 2015-09-22 19:12:52]

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

[commit-bot: I haz the power, 2015-09-22 19:13:19]

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

[commit-bot: I haz the power, 2015-09-22 20:09:05]

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

[commit-bot: I haz the power, 2015-09-22 20:09:06]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Stephen White, 2015-09-23 14:13:26]

LGTM

https://codereview.chromium.org/1307143005/diff/100001/Source/platform/graphi...
File Source/platform/graphics/Canvas2DLayerBridgeTest.cpp (right):

https://codereview.chromium.org/1307143005/diff/100001/Source/platform/graphi...
Source/platform/graphics/Canvas2DLayerBridgeTest.cpp:141:
mainMock.fakeContextLost();
I was actually hoping that we could test the fallback to software when the
texture allocation failed (and the context is still valid), not what context is
lost. But I think this is a good start.

[Justin Novosad, 2015-09-23 14:19:39]

On 2015/09/23 14:13:26, Stephen White wrote:
> LGTM
> 
>
https://codereview.chromium.org/1307143005/diff/100001/Source/platform/graphi...
> File Source/platform/graphics/Canvas2DLayerBridgeTest.cpp (right):
> 
>
https://codereview.chromium.org/1307143005/diff/100001/Source/platform/graphi...
> Source/platform/graphics/Canvas2DLayerBridgeTest.cpp:141:
> mainMock.fakeContextLost();
> I was actually hoping that we could test the fallback to software when the
> texture allocation failed (and the context is still valid), not what context
is
> lost. But I think this is a good start.

As far as Canvas2DLayerBridge is concerned, there is no difference in the code
paths taken between what I wrote, and what you are describing. In both cases,
the attempting to allocate a gpu-backed SkSurface fails, and we take the
fallback path.

[Justin Novosad, 2015-09-23 14:22:52]

On 2015/09/23 14:19:39, Justin Novosad wrote:
> On 2015/09/23 14:13:26, Stephen White wrote:
> > LGTM
> > 
> >
>
https://codereview.chromium.org/1307143005/diff/100001/Source/platform/graphi...
> > File Source/platform/graphics/Canvas2DLayerBridgeTest.cpp (right):
> > 
> >
>
https://codereview.chromium.org/1307143005/diff/100001/Source/platform/graphi...
> > Source/platform/graphics/Canvas2DLayerBridgeTest.cpp:141:
> > mainMock.fakeContextLost();
> > I was actually hoping that we could test the fallback to software when the
> > texture allocation failed (and the context is still valid), not what context
> is
> > lost. But I think this is a good start.
> 
> As far as Canvas2DLayerBridge is concerned, there is no difference in the code
> paths taken between what I wrote, and what you are describing. In both cases,
> the attempting to allocate a gpu-backed SkSurface fails, and we take the
> fallback path.

Oh wait, I'm wrong... I can do better here.

[Justin Novosad, 2015-09-23 18:47:52]

New patch. Added unit test to cover texture allocation failure. Turns out the
test found a bug.

[Justin Novosad, 2015-09-23 18:48:17]

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

[commit-bot: I haz the power, 2015-09-23 18:48:43]

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

[commit-bot: I haz the power, 2015-09-23 18:49:15]

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

[commit-bot: I haz the power, 2015-09-23 18:49:16]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_clobber_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Justin Novosad, 2015-09-23 20:02:03]

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

[Justin Novosad, 2015-09-23 20:05:34]

+kbr for modules/webgl approval

[Justin Novosad, 2015-09-23 20:06:02]

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

[commit-bot: I haz the power, 2015-09-23 20:07:43]

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

[Ken Russell (switch to Gerrit), 2015-09-23 20:31:14]

modules/webgl/ LGTM

I think you should close this CL and open a new one. The base URL is still
pointing at svn:/​/​svn.chromium.org/​blink/​trunk .

[Justin Novosad, 2015-09-23 20:41:05]

The CQ bit was unchecked by junov@chromium.org

[Noel Gordon, 2015-09-28 06:47:17]

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

[Noel Gordon, 2015-09-28 06:47:18]

https://codereview.chromium.org/1307143005/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/HTMLImageElement.cpp (left):

https://codereview.chromium.org/1307143005/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLImageElement.cpp:591: {
Seems like a layering violation?

[Justin Novosad, 2015-09-28 17:12:30]

https://codereview.chromium.org/1307143005/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/HTMLImageElement.cpp (left):

https://codereview.chromium.org/1307143005/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLImageElement.cpp:591: {
On 2015/09/28 06:47:18, noel gordon wrote:
> Seems like a layering violation?

How do you mean? core depends on platform.