New issue
Advanced search Search tips

Issue 590859 link

Starred by 1 user

Issue metadata

Status: WontFix
Owner:
Closed: Mar 2016
Cc:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 2
Type: Bug



Sign in to add a comment

Investigate changing ImageHijackCanvas to override SkCanvas

Project Member Reported by vmp...@chromium.org, Feb 29 2016

Issue description

Right now we override SkNWayCanvas which has an overhead for all draw commands. Instead we should override SkCanvas for the image hijack canvas. However, this requires more plumbing since we need to create an SkCanvas earlier in the pipeline. 



 
Project Member

Comment 1 by bugdroid1@chromium.org, Mar 4 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/2c9805ca261ad5a791a9abb7ec385884c9c81016

commit 2c9805ca261ad5a791a9abb7ec385884c9c81016
Author: vmpstr <vmpstr@chromium.org>
Date: Fri Mar 04 00:07:41 2016

cc: ImageDecodes: Move ImageHijackCanvas to a separate file.

This patch moves ImageHijackCanvas to a separate file in preparation
for overriding SkCanvas instead. This would mean that the canvas has
to be plumbed from earlier in the pipeline and having it in a separate
file helps.

R=enne, ericrk
BUG= 590859 
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Review URL: https://codereview.chromium.org/1758113004

Cr-Commit-Position: refs/heads/master@{#379146}

[modify] https://crrev.com/2c9805ca261ad5a791a9abb7ec385884c9c81016/cc/BUILD.gn
[modify] https://crrev.com/2c9805ca261ad5a791a9abb7ec385884c9c81016/cc/cc.gyp
[modify] https://crrev.com/2c9805ca261ad5a791a9abb7ec385884c9c81016/cc/playback/display_list_raster_source.cc
[add] https://crrev.com/2c9805ca261ad5a791a9abb7ec385884c9c81016/cc/playback/image_hijack_canvas.cc
[add] https://crrev.com/2c9805ca261ad5a791a9abb7ec385884c9c81016/cc/playback/image_hijack_canvas.h

Status: WontFix (was: Assigned)
So, I started looking at how to plumb a direct canvas override and tldr is that it's not easy. GPU plays the raster source into a recording canvas and SW plays the raster source into canvas that is created by a surface.

So, I ended up writing a perftest to compare the performance of SkCanvas vs SkNWayCanvas with one canvas attached (which mimics a direct canvas override vs what we do now). Here are the results (from N9):

*RESULT test: skcanvas= 14604.1748046875 runs/s
*RESULT test: sknwaycanvas= 15934.1875 runs/s
*RESULT test: skcanvas= 15987.08203125 runs/s
*RESULT test: sknwaycanvas= 15934.6650390625 runs/s

This does a save/drawRect of a transluscent rect/restore continuously. With those numbers, I don't think it's worth the trouble of doing anything. That is, the Nway canvas isn't that much overhead it seems.

In other news, the perf regression that prompted this bug seems to have recovered. At this point, I'm wondering if it was the extra copy of images that we were doing that showed up as a regression (which was recently removed). 

I'm going to WontFix this, but please reopen if you feel this is worth pursuing 
Project Member

Comment 3 by bugdroid1@chromium.org, Mar 4 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/6f96b29fbe8b226eb85c455a5645702ab8871242

commit 6f96b29fbe8b226eb85c455a5645702ab8871242
Author: vmpstr <vmpstr@chromium.org>
Date: Fri Mar 04 03:02:28 2016

cc: ImageDecodes: Remove a TODO.

Investigated the bug and the TODO isn't worth doing, so removing it.

R=enne
BUG= 590859 
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Review URL: https://codereview.chromium.org/1764873002

Cr-Commit-Position: refs/heads/master@{#379193}

[modify] https://crrev.com/6f96b29fbe8b226eb85c455a5645702ab8871242/cc/playback/image_hijack_canvas.h

Sign in to add a comment