Optimize visualbench offscreen blits

Optimize visualbench offscreen blits

Modifies NvprWrappedBenchmark to present its offscreen canvas via
GrContext::copySurface rather than going through the SkCanvas API.
Only copies a subrectangle the size of the benchmark, not the entire
canvas.

BUG=skia:

Committed: https://skia.googlesource.com/skia/+/0f6cca8a2070b974368652ec04b6003e9205a8a3

[Chris Dalton, 2015-11-13 21:33:20]

cdalton@nvidia.com changed reviewers:
+ joshualitt@google.com

[Chris Dalton, 2015-11-13 21:33:20]

I'm not sure if this is the base way to go about it but it's a step in the right
direction.

[bsalomon, 2015-11-16 13:59:19]

On 2015/11/13 21:33:20, Chris Dalton wrote:
> I'm not sure if this is the base way to go about it but it's a step in the
right
> direction.

It doesn't seem too bad to me. I don't love relying on accessing the RTs and
getting at the GrContext but short of adding a SkSurface blit API I can't think
of anything better. Maybe you could add a comment in the nvpr blit override
about why you're doing this. Leaving to Josh for a real review on the VB
details.

[joshualitt, 2015-11-16 14:53:30]

On 2015/11/16 13:59:19, bsalomon wrote:
> On 2015/11/13 21:33:20, Chris Dalton wrote:
> > I'm not sure if this is the base way to go about it but it's a step in the
> right
> > direction.
> 
> It doesn't seem too bad to me. I don't love relying on accessing the RTs and
> getting at the GrContext but short of adding a SkSurface blit API I can't
think
> of anything better. Maybe you could add a comment in the nvpr blit override
> about why you're doing this. Leaving to Josh for a real review on the VB
> details.

VB details lgtm.

[Chris Dalton, 2015-11-16 18:30:35]

Thanks for the replies. I added that comment to the nvpr blit routine and added
a couple quick questions.

https://codereview.chromium.org/1443263002/diff/40001/tools/VisualBench/Wrapp...
File tools/VisualBench/WrappedBenchmark.h (right):

https://codereview.chromium.org/1443263002/diff/40001/tools/VisualBench/Wrapp...
tools/VisualBench/WrappedBenchmark.h:105: SkCanvas::LayerIter canvasIter(canvas,
false);
I'd rather call getTopDevice() than use the LayerIter, except it's private. Is
there a secret internal way to get the top device or is this the best we can do?

https://codereview.chromium.org/1443263002/diff/40001/tools/VisualBench/Wrapp...
tools/VisualBench/WrappedBenchmark.h:112: !dst ||
canvas->imageInfo().dimensions() != SkISize::Make(dst->width(),dst->height())) {
I added the check of RT size because I understand if the SkCanvas is larger than
the max RT size supported by the GPU, tiling will happen somewhere. Is this
correct? Is this check sufficient to handle that case?

[bsalomon, 2015-11-16 21:14:59]

bsalomon@google.com changed reviewers:
+ bsalomon@google.com

[bsalomon, 2015-11-16 21:14:59]

https://codereview.chromium.org/1443263002/diff/40001/tools/VisualBench/Wrapp...
File tools/VisualBench/WrappedBenchmark.h (right):

https://codereview.chromium.org/1443263002/diff/40001/tools/VisualBench/Wrapp...
tools/VisualBench/WrappedBenchmark.h:105: SkCanvas::LayerIter canvasIter(canvas,
false);
On 2015/11/16 18:30:35, Chris Dalton wrote:
> I'd rather call getTopDevice() than use the LayerIter, except it's private. Is
> there a secret internal way to get the top device or is this the best we can
do?

I think this is the best we can do right now.

https://codereview.chromium.org/1443263002/diff/40001/tools/VisualBench/Wrapp...
tools/VisualBench/WrappedBenchmark.h:112: !dst ||
canvas->imageInfo().dimensions() != SkISize::Make(dst->width(),dst->height())) {
On 2015/11/16 18:30:35, Chris Dalton wrote:
> I added the check of RT size because I understand if the SkCanvas is larger
than
> the max RT size supported by the GPU, tiling will happen somewhere. Is this
> correct? Is this check sufficient to handle that case?

I think the RT is always the size of the screen. Josh?

[Chris Dalton, 2015-11-16 21:23:17]

https://codereview.chromium.org/1443263002/diff/40001/tools/VisualBench/Wrapp...
File tools/VisualBench/WrappedBenchmark.h (right):

https://codereview.chromium.org/1443263002/diff/40001/tools/VisualBench/Wrapp...
tools/VisualBench/WrappedBenchmark.h:112: !dst ||
canvas->imageInfo().dimensions() != SkISize::Make(dst->width(),dst->height())) {
On 2015/11/16 21:14:59, bsalomon wrote:
> On 2015/11/16 18:30:35, Chris Dalton wrote:
> > I added the check of RT size because I understand if the SkCanvas is larger
> than
> > the max RT size supported by the GPU, tiling will happen somewhere. Is this
> > correct? Is this check sufficient to handle that case?
> 
> I think the RT is always the size of the screen. Josh?

Oh that's right, unless running in windowed mode (--fullscreen false)

In theory though, if this were ever given a gigantic canvas (say 10k x 10k),
larger than GL_MAX_TEXTURE_SIZE, would the SkGpuDevice run tiled and make it so
a simple copySurface call wouldn't work?

[joshualitt, 2015-11-18 17:02:03]

https://codereview.chromium.org/1443263002/diff/40001/tools/VisualBench/Wrapp...
File tools/VisualBench/WrappedBenchmark.h (right):

https://codereview.chromium.org/1443263002/diff/40001/tools/VisualBench/Wrapp...
tools/VisualBench/WrappedBenchmark.h:112: !dst ||
canvas->imageInfo().dimensions() != SkISize::Make(dst->width(),dst->height())) {
On 2015/11/16 21:23:17, Chris Dalton wrote:
> On 2015/11/16 21:14:59, bsalomon wrote:
> > On 2015/11/16 18:30:35, Chris Dalton wrote:
> > > I added the check of RT size because I understand if the SkCanvas is
larger
> > than
> > > the max RT size supported by the GPU, tiling will happen somewhere. Is
this
> > > correct? Is this check sufficient to handle that case?
> > 
> > I think the RT is always the size of the screen. Josh?
> 
> Oh that's right, unless running in windowed mode (--fullscreen false)
> 
> In theory though, if this were ever given a gigantic canvas (say 10k x 10k),
> larger than GL_MAX_TEXTURE_SIZE, would the SkGpuDevice run tiled and make it
so
> a simple copySurface call wouldn't work?

just print not supported in that case.

[joshualitt, 2015-11-24 14:50:53]

On 2015/11/18 17:02:03, joshualitt wrote:
>
https://codereview.chromium.org/1443263002/diff/40001/tools/VisualBench/Wrapp...
> File tools/VisualBench/WrappedBenchmark.h (right):
> 
>
https://codereview.chromium.org/1443263002/diff/40001/tools/VisualBench/Wrapp...
> tools/VisualBench/WrappedBenchmark.h:112: !dst ||
> canvas->imageInfo().dimensions() != SkISize::Make(dst->width(),dst->height()))
{
> On 2015/11/16 21:23:17, Chris Dalton wrote:
> > On 2015/11/16 21:14:59, bsalomon wrote:
> > > On 2015/11/16 18:30:35, Chris Dalton wrote:
> > > > I added the check of RT size because I understand if the SkCanvas is
> larger
> > > than
> > > > the max RT size supported by the GPU, tiling will happen somewhere. Is
> this
> > > > correct? Is this check sufficient to handle that case?
> > > 
> > > I think the RT is always the size of the screen. Josh?
> > 
> > Oh that's right, unless running in windowed mode (--fullscreen false)
> > 
> > In theory though, if this were ever given a gigantic canvas (say 10k x 10k),
> > larger than GL_MAX_TEXTURE_SIZE, would the SkGpuDevice run tiled and make it
> so
> > a simple copySurface call wouldn't work?
> 
> just print not supported in that case.

lgtm

[Chris Dalton, 2015-11-24 16:14:45]

The CQ bit was checked by cdalton@nvidia.com

[commit-bot: I haz the power, 2015-11-24 16:14:58]

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

[commit-bot: I haz the power, 2015-11-24 16:14:58]

This CL has an open dependency (Issue 1473033002 Patch 1). Please resolve the
dependency and try again.

[Chris Dalton, 2015-11-24 16:28:56]

The CQ bit was checked by cdalton@nvidia.com

[Chris Dalton, 2015-11-24 16:28:56]

The patchset sent to the CQ was uploaded after l-g-t-m from
joshualitt@google.com
Link to the patchset: https://codereview.chromium.org/1443263002/#ps120001
(title: "Remove dependency on unrelated issue")

[commit-bot: I haz the power, 2015-11-24 16:29:05]

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

[commit-bot: I haz the power, 2015-11-24 16:31:11]

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

[commit-bot: I haz the power, 2015-11-24 16:31:12]

Try jobs failed on following builders:
  Build-Win-MSVC-x86_64-Debug-Trybot on client.skia.compile (JOB_FAILED,
http://build.chromium.org/p/client.skia.compile/builders/Build-Win-MSVC-x86_6...)

[Chris Dalton, 2015-11-24 16:41:00]

The CQ bit was checked by cdalton@nvidia.com

[Chris Dalton, 2015-11-24 16:41:00]

The patchset sent to the CQ was uploaded after l-g-t-m from
joshualitt@google.com
Link to the patchset: https://codereview.chromium.org/1443263002/#ps140001
(title: "Fix msvc warnings")

[commit-bot: I haz the power, 2015-11-24 16:41:06]

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

[commit-bot: I haz the power, 2015-11-24 16:54:33]

Committed patchset #8 (id:140001) as
https://skia.googlesource.com/skia/+/0f6cca8a2070b974368652ec04b6003e9205a8a3