Cancel GPU acceleration for 2D canvas when drawing very large images

Cancel GPU acceleration for 2D canvas when drawing very large images

GPU texture upload overhead is often prohibitively expensive when
drawing very large images into a canvas. This change adds an
heuristic to trigger a fallback to software rendering when this
happens

TBR=kbr@chromium.org
BUG=367355
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel

Committed: https://crrev.com/03b1013d9a855aba76fa626a640b08b95146154f
Cr-Commit-Position: refs/heads/master@{#421651}

[Justin Novosad, 2016-09-23 18:38:03]

Description was changed from

==========
Cancel GPU acceleration for 2D canvas when drawing very large images

GPU texture upload overhead is often prohibitively expensive when
drawing very large images into a canvas. This change adds an
heuristic to trigger a fallback to software rendering when this
happens

BUG=367355
==========

to

==========
Cancel GPU acceleration for 2D canvas when drawing very large images

GPU texture upload overhead is often prohibitively expensive when
drawing very large images into a canvas. This change adds an
heuristic to trigger a fallback to software rendering when this
happens

BUG=367355
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel
==========

[Justin Novosad, 2016-09-23 18:38:21]

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

[Justin Novosad, 2016-09-23 18:38:51]

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

[Justin Novosad, 2016-09-23 18:38:51]

PTAL

[commit-bot: I haz the power, 2016-09-23 18:39:26]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-23 19:12:52]

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

[commit-bot: I haz the power, 2016-09-23 19:12:53]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[xlai (Olivia), 2016-09-23 20:48:04]

https://codereview.chromium.org/2362363002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/offscreencanvas/OffscreenCanvas.cpp (right):

https://codereview.chromium.org/2362363002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/offscreencanvas/OffscreenCanvas.cpp:161: return
m_context && m_context->isAccelerated();
Just want to let you know that in my previous patch I remove the
OffscreenCanvasRenderingContext2D::isAccelerated() overriding function based on
suggestion. So this function here may not return correct value
of Offscreen 2d context.

[xidachen, 2016-09-23 20:56:36]

https://codereview.chromium.org/2362363002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/canvas2d/BaseRenderingContext2D.cpp
(right):

https://codereview.chromium.org/2362363002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/canvas2d/BaseRenderingContext2D.cpp:1090: if
(srcArea > dstArea *
ExpensiveCanvasHeuristicParameters::DrawImageTextureUploadSoftSizeLimitScaleThreshold)
{
I am a little confused by this logic.

Say that srcArea = 7M*7M, so it goes into this else if block. Assuming that
dstArea is the same, then this if statement is false, and we don't disable it?
So we disable acceleration either:
1). srcArea is very large
2). srcArea is large, and dstArea is small by factor of 4

I don't quite understand the second logic.

[Justin Novosad, 2016-09-23 21:04:07]

On 2016/09/23 20:56:36, xidachen wrote:
>
https://codereview.chromium.org/2362363002/diff/1/third_party/WebKit/Source/m...
> File third_party/WebKit/Source/modules/canvas2d/BaseRenderingContext2D.cpp
> (right):
> 
>
https://codereview.chromium.org/2362363002/diff/1/third_party/WebKit/Source/m...
> third_party/WebKit/Source/modules/canvas2d/BaseRenderingContext2D.cpp:1090: if
> (srcArea > dstArea *
>
ExpensiveCanvasHeuristicParameters::DrawImageTextureUploadSoftSizeLimitScaleThreshold)
> {
> I am a little confused by this logic.
> 
> Say that srcArea = 7M*7M, so it goes into this else if block. Assuming that
> dstArea is the same, then this if statement is false, and we don't disable it?
> So we disable acceleration either:
> 1). srcArea is very large
> 2). srcArea is large, and dstArea is small by factor of 4
> 
> I don't quite understand the second logic.

Your interpretation is correct the idea is that when we are downscaling images
by a large factor, we end-up uploading the entire image to a texture only to
query a small subset of the image's pixels.  This use case is notoriously
inefficient.

[xidachen, 2016-09-24 01:36:34]

lgtm with nits and all error fixed.

https://codereview.chromium.org/2362363002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/canvas2d/CanvasRenderingContext2DTest.cpp
(right):

https://codereview.chromium.org/2362363002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/canvas2d/CanvasRenderingContext2DTest.cpp:804:
{
I can see that these two tests are sharing a lot of common code. Is there a way
to share some code in unit tests?

https://codereview.chromium.org/2362363002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/canvas2d/CanvasRenderingContext2DTest.cpp:814:
IntSize size(dstDim, dstDim);
Could we name this dstSize?

https://codereview.chromium.org/2362363002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/canvas2d/CanvasRenderingContext2DTest.cpp:821:
EXPECT_EQ(8*dstDim*dstDim, getGlobalGPUMemoryUsage());
I don't quite understand why is this *8? My impression is number of pixels *
number of channels (*4).

https://codereview.chromium.org/2362363002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/canvas2d/CanvasRenderingContext2DTest.cpp:829:
EXPECT_TRUE(!canvasElement().buffer()->isAccelerated());
Maybe change to EXPECT_FALSE(canvasElement().buffer()->isAccelerated())?

https://codereview.chromium.org/2362363002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/canvas2d/CanvasRenderingContext2DTest.cpp:848:
IntSize size(dstDim, dstDim);
dstSize?

[Justin Novosad, 2016-09-26 16:52:40]

The CQ bit was checked by junov@chromium.org

[Justin Novosad, 2016-09-26 16:52:40]

The patchset sent to the CQ was uploaded after l-g-t-m from
xidachen@chromium.org
Link to the patchset: https://codereview.chromium.org/2362363002/#ps20001
(title: "nits")

[commit-bot: I haz the power, 2016-09-26 16:53:18]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-26 17:02:42]

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

[commit-bot: I haz the power, 2016-09-26 17:02:43]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Justin Novosad, 2016-09-26 18:14:26]

Description was changed from

==========
Cancel GPU acceleration for 2D canvas when drawing very large images

GPU texture upload overhead is often prohibitively expensive when
drawing very large images into a canvas. This change adds an
heuristic to trigger a fallback to software rendering when this
happens

BUG=367355
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel
==========

to

==========
Cancel GPU acceleration for 2D canvas when drawing very large images

GPU texture upload overhead is often prohibitively expensive when
drawing very large images into a canvas. This change adds an
heuristic to trigger a fallback to software rendering when this
happens

TBR=kbr@chromium.org
BUG=367355
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel
==========

[Justin Novosad, 2016-09-26 18:15:43]

The CQ bit was checked by junov@chromium.org

[Justin Novosad, 2016-09-26 18:15:43]

The patchset sent to the CQ was uploaded after l-g-t-m from
xidachen@chromium.org
Link to the patchset: https://codereview.chromium.org/2362363002/#ps40001
(title: "fix sqrt")

[commit-bot: I haz the power, 2016-09-26 18:16:38]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-26 20:12:03]

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

[commit-bot: I haz the power, 2016-09-26 20:12:05]

Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Justin Novosad, 2016-09-26 20:14:54]

The CQ bit was checked by junov@chromium.org

[commit-bot: I haz the power, 2016-09-26 20:15:32]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-26 22:38:33]

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

[commit-bot: I haz the power, 2016-09-26 22:38:35]

Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Justin Novosad, 2016-09-28 20:24:04]

The CQ bit was checked by junov@chromium.org

[commit-bot: I haz the power, 2016-09-28 20:24:28]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-28 21:53:18]

Description was changed from

==========
Cancel GPU acceleration for 2D canvas when drawing very large images

GPU texture upload overhead is often prohibitively expensive when
drawing very large images into a canvas. This change adds an
heuristic to trigger a fallback to software rendering when this
happens

TBR=kbr@chromium.org
BUG=367355
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel
==========

to

==========
Cancel GPU acceleration for 2D canvas when drawing very large images

GPU texture upload overhead is often prohibitively expensive when
drawing very large images into a canvas. This change adds an
heuristic to trigger a fallback to software rendering when this
happens

TBR=kbr@chromium.org
BUG=367355
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel
==========

[commit-bot: I haz the power, 2016-09-28 21:53:21]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-09-28 21:54:48]

Description was changed from

==========
Cancel GPU acceleration for 2D canvas when drawing very large images

GPU texture upload overhead is often prohibitively expensive when
drawing very large images into a canvas. This change adds an
heuristic to trigger a fallback to software rendering when this
happens

TBR=kbr@chromium.org
BUG=367355
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel
==========

to

==========
Cancel GPU acceleration for 2D canvas when drawing very large images

GPU texture upload overhead is often prohibitively expensive when
drawing very large images into a canvas. This change adds an
heuristic to trigger a fallback to software rendering when this
happens

TBR=kbr@chromium.org
BUG=367355
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel

Committed: https://crrev.com/03b1013d9a855aba76fa626a640b08b95146154f
Cr-Commit-Position: refs/heads/master@{#421651}
==========

[commit-bot: I haz the power, 2016-09-28 21:54:49]

Patchset 3 (id:??) landed as
https://crrev.com/03b1013d9a855aba76fa626a640b08b95146154f
Cr-Commit-Position: refs/heads/master@{#421651}