Require budget decision when creating a RenderTarget SkSurface

Require budget decision when creating a RenderTarget SkSurface.

Restructure SkGpuDevice creation:
*SkSurfaceProps are optional.
*Use SkSurfaceProps to communicate DF text rather than a flag.
*Tell SkGpuDevice::Create whether RT comes from cache or not.

Committed: https://skia.googlesource.com/skia/+/afe3005be3392e43bc51eb7eb2017eefaed85ad1

[bsalomon, 2015-01-15 20:29:38]

bsalomon@google.com changed reviewers:
+ jvanverth@google.com, reed@google.com, robertphillips@google.com

[robertphillips, 2015-01-15 20:48:18]

half way through

https://codereview.chromium.org/848903004/diff/40001/gm/xfermodes3.cpp
File gm/xfermodes3.cpp (right):

https://codereview.chromium.org/848903004/diff/40001/gm/xfermodes3.cpp#newcod...
gm/xfermodes3.cpp:128: SkImageInfo baseInfo = baseCanvas->imageInfo();
info -> baseInfo in this line?

https://codereview.chromium.org/848903004/diff/40001/include/core/SkSurface.h
File include/core/SkSurface.h (right):

https://codereview.chromium.org/848903004/diff/40001/include/core/SkSurface.h...
include/core/SkSurface.h:120: const SkSurfaceProps* props) {
I think this should be kYes_Budgeted.

https://codereview.chromium.org/848903004/diff/40001/src/gpu/GrGpuResource.cpp
File src/gpu/GrGpuResource.cpp (right):

https://codereview.chromium.org/848903004/diff/40001/src/gpu/GrGpuResource.cp...
src/gpu/GrGpuResource.cpp:136: void GrGpuResource::makeBudgeted() {
Should we add an assert that we're not trying to make a wrapped resource
budgeted?

https://codereview.chromium.org/848903004/diff/40001/src/gpu/GrResourceCache2...
File src/gpu/GrResourceCache2.cpp (right):

https://codereview.chromium.org/848903004/diff/40001/src/gpu/GrResourceCache2...
src/gpu/GrResourceCache2.cpp:308: fBudgetedBytes += size;
It seems like we should be purging here ?

[jvanverth1, 2015-01-15 21:12:17]

Taking a preliminary look -- dftext setup looks okay.

[bsalomon, 2015-01-15 21:14:06]

https://codereview.chromium.org/848903004/diff/40001/gm/xfermodes3.cpp
File gm/xfermodes3.cpp (right):

https://codereview.chromium.org/848903004/diff/40001/gm/xfermodes3.cpp#newcod...
gm/xfermodes3.cpp:128: SkImageInfo baseInfo = baseCanvas->imageInfo();
On 2015/01/15 20:48:17, robertphillips wrote:
> info -> baseInfo in this line?

d'oh

https://codereview.chromium.org/848903004/diff/40001/include/core/SkSurface.h
File include/core/SkSurface.h (right):

https://codereview.chromium.org/848903004/diff/40001/include/core/SkSurface.h...
include/core/SkSurface.h:120: const SkSurfaceProps* props) {
On 2015/01/15 20:48:17, robertphillips wrote:
> I think this should be kYes_Budgeted.

The old NewRT did not count against the budget (it called
createUncachedTexture). I'm mimicking that behavior here.

https://codereview.chromium.org/848903004/diff/40001/src/gpu/GrGpuResource.cpp
File src/gpu/GrGpuResource.cpp (right):

https://codereview.chromium.org/848903004/diff/40001/src/gpu/GrGpuResource.cp...
src/gpu/GrGpuResource.cpp:136: void GrGpuResource::makeBudgeted() {
On 2015/01/15 20:48:17, robertphillips wrote:
> Should we add an assert that we're not trying to make a wrapped resource
> budgeted?

Seems ok to just ignore the request to me...

https://codereview.chromium.org/848903004/diff/40001/src/gpu/GrResourceCache2...
File src/gpu/GrResourceCache2.cpp (right):

https://codereview.chromium.org/848903004/diff/40001/src/gpu/GrResourceCache2...
src/gpu/GrResourceCache2.cpp:308: fBudgetedBytes += size;
On 2015/01/15 20:48:17, robertphillips wrote:
> It seems like we should be purging here ?

good point, done.

[robertphillips, 2015-01-15 21:30:37]

other half

https://codereview.chromium.org/848903004/diff/40001/src/gpu/SkGpuDevice.cpp
File src/gpu/SkGpuDevice.cpp (right):

https://codereview.chromium.org/848903004/diff/40001/src/gpu/SkGpuDevice.cpp#...
src/gpu/SkGpuDevice.cpp:163: 
Can't we do something like fSurfaceProps.isUseDistanceFieldFonts() ?
Don't know if it is any better though.

https://codereview.chromium.org/848903004/diff/40001/src/image/SkSurface_Gpu.cpp
File src/image/SkSurface_Gpu.cpp (right):

https://codereview.chromium.org/848903004/diff/40001/src/image/SkSurface_Gpu....
src/image/SkSurface_Gpu.cpp:73: SkGpuDevice::Create(oldRT->getContext(),
budgeted, fDevice->imageInfo(),
tab this over ?

https://codereview.chromium.org/848903004/diff/40001/src/image/SkSurface_Gpu....
src/image/SkSurface_Gpu.cpp:75: 
What happened to kRetain_ContentChangeMode?

https://codereview.chromium.org/848903004/diff/40001/tests/DeferredCanvasTest...
File tests/DeferredCanvasTest.cpp (right):

https://codereview.chromium.org/848903004/diff/40001/tests/DeferredCanvasTest...
tests/DeferredCanvasTest.cpp:795: SkASSERT(alternateSurface);
Can we just make surface & alternateSurface be SkAutoTUnrefs from the start?

Low priority - ignore at will.

https://codereview.chromium.org/848903004/diff/40001/tests/ImageIsOpaqueTest.cpp
File tests/ImageIsOpaqueTest.cpp (right):

https://codereview.chromium.org/848903004/diff/40001/tests/ImageIsOpaqueTest....
tests/ImageIsOpaqueTest.cpp:83: SkAutoTUnref<SkSurface>
surfaceTransparent(SkSurface::NewRenderTarget(context,
Tab this over?

https://codereview.chromium.org/848903004/diff/40001/tests/ImageIsOpaqueTest....
tests/ImageIsOpaqueTest.cpp:88: SkAutoTUnref<SkSurface>
surfaceOpaque(SkSurface::NewRenderTarget(context,
This too ?

https://codereview.chromium.org/848903004/diff/40001/tests/ImageNewShaderTest...
File tests/ImageNewShaderTest.cpp (right):

https://codereview.chromium.org/848903004/diff/40001/tests/ImageNewShaderTest...
tests/ImageNewShaderTest.cpp:118: SkAutoTUnref<SkSurface>
sourceSurface(SkSurface::NewRenderTarget(context,
tab over ?

https://codereview.chromium.org/848903004/diff/40001/tests/ImageNewShaderTest...
tests/ImageNewShaderTest.cpp:130: SkSurface::kNo_Budgeted, info));
Shouldn't this remain NewRaster ?

https://codereview.chromium.org/848903004/diff/40001/tests/ResourceCacheTest.cpp
File tests/ResourceCacheTest.cpp (right):

https://codereview.chromium.org/848903004/diff/40001/tests/ResourceCacheTest....
tests/ResourceCacheTest.cpp:800: SkAutoTUnref<SkSurface>
surface(SkSurface::NewRenderTarget(context,
tab over ?

https://codereview.chromium.org/848903004/diff/60001/tests/SurfaceTest.cpp
File tests/SurfaceTest.cpp (right):

https://codereview.chromium.org/848903004/diff/60001/tests/SurfaceTest.cpp#ne...
tests/SurfaceTest.cpp:53: #if SK_SUPPORT_GPU
Do we need the "if (context) {" check any more ?

[robertphillips, 2015-01-15 21:34:09]

https://codereview.chromium.org/848903004/diff/40001/include/core/SkSurface.h
File include/core/SkSurface.h (right):

https://codereview.chromium.org/848903004/diff/40001/include/core/SkSurface.h...
include/core/SkSurface.h:120: const SkSurfaceProps* props) {
On 2015/01/15 21:14:05, bsalomon wrote:
> On 2015/01/15 20:48:17, robertphillips wrote:
> > I think this should be kYes_Budgeted.
> 
> The old NewRT did not count against the budget (it called
> createUncachedTexture). I'm mimicking that behavior here.

Ah right. I was mixing it up with the old alternate form of
NewScratchRenderTarget.

[bsalomon, 2015-01-15 22:15:50]

https://codereview.chromium.org/848903004/diff/40001/src/gpu/SkGpuDevice.cpp
File src/gpu/SkGpuDevice.cpp (right):

https://codereview.chromium.org/848903004/diff/40001/src/gpu/SkGpuDevice.cpp#...
src/gpu/SkGpuDevice.cpp:163: 
On 2015/01/15 21:30:36, robertphillips wrote:
> Can't we do something like fSurfaceProps.isUseDistanceFieldFonts() ?
> Don't know if it is any better though.

Done.

https://codereview.chromium.org/848903004/diff/40001/src/image/SkSurface_Gpu.cpp
File src/image/SkSurface_Gpu.cpp (right):

https://codereview.chromium.org/848903004/diff/40001/src/image/SkSurface_Gpu....
src/image/SkSurface_Gpu.cpp:73: SkGpuDevice::Create(oldRT->getContext(),
budgeted, fDevice->imageInfo(),
On 2015/01/15 21:30:36, robertphillips wrote:
> tab this over ?

Done.

https://codereview.chromium.org/848903004/diff/40001/src/image/SkSurface_Gpu....
src/image/SkSurface_Gpu.cpp:75: 
On 2015/01/15 21:30:36, robertphillips wrote:
> What happened to kRetain_ContentChangeMode?

eek, it's back.

https://codereview.chromium.org/848903004/diff/40001/tests/DeferredCanvasTest...
File tests/DeferredCanvasTest.cpp (right):

https://codereview.chromium.org/848903004/diff/40001/tests/DeferredCanvasTest...
tests/DeferredCanvasTest.cpp:795: SkASSERT(alternateSurface);
On 2015/01/15 21:30:36, robertphillips wrote:
> Can we just make surface & alternateSurface be SkAutoTUnrefs from the start?
> 
> Low priority - ignore at will.

Done (the ignoring, that is  :) ) Just don't want to get into adding more
unrelated changes into a fairly big cl.

https://codereview.chromium.org/848903004/diff/40001/tests/ImageIsOpaqueTest.cpp
File tests/ImageIsOpaqueTest.cpp (right):

https://codereview.chromium.org/848903004/diff/40001/tests/ImageIsOpaqueTest....
tests/ImageIsOpaqueTest.cpp:83: SkAutoTUnref<SkSurface>
surfaceTransparent(SkSurface::NewRenderTarget(context,
On 2015/01/15 21:30:36, robertphillips wrote:
> Tab this over?

Done.

https://codereview.chromium.org/848903004/diff/40001/tests/ImageIsOpaqueTest....
tests/ImageIsOpaqueTest.cpp:88: SkAutoTUnref<SkSurface>
surfaceOpaque(SkSurface::NewRenderTarget(context,
On 2015/01/15 21:30:36, robertphillips wrote:
> This too ?

Done.

https://codereview.chromium.org/848903004/diff/40001/tests/ImageNewShaderTest...
File tests/ImageNewShaderTest.cpp (right):

https://codereview.chromium.org/848903004/diff/40001/tests/ImageNewShaderTest...
tests/ImageNewShaderTest.cpp:118: SkAutoTUnref<SkSurface>
sourceSurface(SkSurface::NewRenderTarget(context,
On 2015/01/15 21:30:36, robertphillips wrote:
> tab over ?

Done.

https://codereview.chromium.org/848903004/diff/40001/tests/ImageNewShaderTest...
tests/ImageNewShaderTest.cpp:130: SkSurface::kNo_Budgeted, info));
On 2015/01/15 21:30:36, robertphillips wrote:
> Shouldn't this remain NewRaster ?

Yup, I got overzealous.

https://codereview.chromium.org/848903004/diff/40001/tests/ResourceCacheTest.cpp
File tests/ResourceCacheTest.cpp (right):

https://codereview.chromium.org/848903004/diff/40001/tests/ResourceCacheTest....
tests/ResourceCacheTest.cpp:800: SkAutoTUnref<SkSurface>
surface(SkSurface::NewRenderTarget(context,
On 2015/01/15 21:30:36, robertphillips wrote:
> tab over ?

Done.

https://codereview.chromium.org/848903004/diff/60001/tests/SurfaceTest.cpp
File tests/SurfaceTest.cpp (right):

https://codereview.chromium.org/848903004/diff/60001/tests/SurfaceTest.cpp#ne...
tests/SurfaceTest.cpp:53: #if SK_SUPPORT_GPU
On 2015/01/15 21:30:36, robertphillips wrote:
> Do we need the "if (context) {" check any more ?

good point, gonzo

Actually don't need the #if SK_SUPPORT_GPU either since NewRT just returns null
in that case.

[robertphillips, 2015-01-16 00:53:51]

lgtm

[bsalomon, 2015-01-16 14:45:23]

The CQ bit was checked by bsalomon@google.com

[commit-bot: I haz the power, 2015-01-16 14:46:12]

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

[commit-bot: I haz the power, 2015-01-16 14:49:35]

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

[commit-bot: I haz the power, 2015-01-16 14:49:36]

Try jobs failed on following builders:
  Build-Ubuntu13.10-GCC4.8-x86_64-Release-Trybot on client.skia.compile
(http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu13.10-GC...)

[bsalomon, 2015-01-16 15:20:52]

The CQ bit was checked by bsalomon@google.com

[commit-bot: I haz the power, 2015-01-16 15:21:35]

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

[commit-bot: I haz the power, 2015-01-16 15:32:37]

Committed patchset #7 (id:120001) as
https://skia.googlesource.com/skia/+/afe3005be3392e43bc51eb7eb2017eefaed85ad1