Altered GrDrawState to always ref texture and render target

Long deferred cleanup

[robertphillips, 2012-05-24 19:04:44]

http://codereview.appspot.com/6251049/diff/1/include/gpu/GrContext.h
File include/gpu/GrContext.h (right):

http://codereview.appspot.com/6251049/diff/1/include/gpu/GrContext.h#newcode862
include/gpu/GrContext.h:862: 
Having the reset function made this a bit clearer for me but this change isn't
necessary

http://codereview.appspot.com/6251049/diff/1/src/gpu/GrDrawState.h
File src/gpu/GrDrawState.h (right):

http://codereview.appspot.com/6251049/diff/1/src/gpu/GrDrawState.h#newcode512
src/gpu/GrDrawState.h:512: 
I felt pulling the restore functionality into its own method made this clearer
but the ref and unref could be buried in the original set method

http://codereview.appspot.com/6251049/diff/1/src/gpu/GrDrawState.h#newcode865
src/gpu/GrDrawState.h:865: 
The GrIODB reffed/unreffed both the textures & the render target so I added
refs/unrefs to both object types in here

http://codereview.appspot.com/6251049/diff/1/src/gpu/GrInOrderDrawBuffer.cpp
File src/gpu/GrInOrderDrawBuffer.cpp (left):

http://codereview.appspot.com/6251049/diff/1/src/gpu/GrInOrderDrawBuffer.cpp#...
src/gpu/GrInOrderDrawBuffer.cpp:462: this->resetIndexSource();
GrDrawState now takes care of all these refs & unrefs (always - not just when
pushed onto a GrIODB stack)

[bsalomon, 2012-05-24 19:30:30]

Mostly LGTM. It feels good to get this one done, been meaning to do it for a
long long time. :)

http://codereview.appspot.com/6251049/diff/1/include/gpu/GrContext.h
File include/gpu/GrContext.h (right):

http://codereview.appspot.com/6251049/diff/1/include/gpu/GrContext.h#newcode860
include/gpu/GrContext.h:860: reset();
this->

http://codereview.appspot.com/6251049/diff/1/include/gpu/GrContext.h#newcode862
include/gpu/GrContext.h:862: 
On 2012/05/24 19:04:44, robertphillips wrote:
> Having the reset function made this a bit clearer for me but this change isn't
> necessary

I agree, it's clearer

http://codereview.appspot.com/6251049/diff/1/include/gpu/GrContext.h#newcode874
include/gpu/GrContext.h:874: reset();
this->

http://codereview.appspot.com/6251049/diff/1/src/gpu/GrContext.cpp
File src/gpu/GrContext.cpp (right):

http://codereview.appspot.com/6251049/diff/1/src/gpu/GrContext.cpp#newcode73
src/gpu/GrContext.cpp:73: fGpu->setDrawState(NULL);
Curious, does the fGpu actually outlive the unref below?

http://codereview.appspot.com/6251049/diff/1/src/gpu/GrDrawState.h
File src/gpu/GrDrawState.h (right):

http://codereview.appspot.com/6251049/diff/1/src/gpu/GrDrawState.h#newcode76
src/gpu/GrDrawState.h:76: this->reset();
Do we think the compiler will be nice enough to throw out everything but the
unrefs?

http://codereview.appspot.com/6251049/diff/1/src/gpu/GrDrawState.h#newcode522
src/gpu/GrDrawState.h:522: restore();
this->

http://codereview.appspot.com/6251049/diff/1/src/gpu/GrDrawState.h#newcode865
src/gpu/GrDrawState.h:865: 
On 2012/05/24 19:04:44, robertphillips wrote:
> The GrIODB reffed/unreffed both the textures & the render target so I added
> refs/unrefs to both object types in here

Def. should be both

Hopefully we've already reduced a bunch of the operator = and operator == called
(and are on the path to shrinking this object via Tom's work) but still I wonder
if fTextures and fRenderTarget should be in the Pod but not Memset section.  It
seems like we can still call memcmp on them.

[robertphillips, 2012-05-24 20:38:51]

http://codereview.appspot.com/6251049/diff/1/include/gpu/GrContext.h
File include/gpu/GrContext.h (right):

http://codereview.appspot.com/6251049/diff/1/include/gpu/GrContext.h#newcode860
include/gpu/GrContext.h:860: reset();
On 2012/05/24 19:30:31, bsalomon wrote:
> this->

Done.

http://codereview.appspot.com/6251049/diff/1/include/gpu/GrContext.h#newcode874
include/gpu/GrContext.h:874: reset();
On 2012/05/24 19:30:31, bsalomon wrote:
> this->

Done.

http://codereview.appspot.com/6251049/diff/1/src/gpu/GrContext.cpp
File src/gpu/GrContext.cpp (right):

http://codereview.appspot.com/6251049/diff/1/src/gpu/GrContext.cpp#newcode73
src/gpu/GrContext.cpp:73: fGpu->setDrawState(NULL);
It doesn't. The problem here is that the texture cache is deleted before the gpu
is freed. If the gpu is holding onto any textures things get messy

http://codereview.appspot.com/6251049/diff/1/src/gpu/GrDrawState.h
File src/gpu/GrDrawState.h (right):

http://codereview.appspot.com/6251049/diff/1/src/gpu/GrDrawState.h#newcode76
src/gpu/GrDrawState.h:76: this->reset();
I doubt it.

http://codereview.appspot.com/6251049/diff/1/src/gpu/GrDrawState.h#newcode522
src/gpu/GrDrawState.h:522: restore();
On 2012/05/24 19:30:31, bsalomon wrote:
> this->

Done.

http://codereview.appspot.com/6251049/diff/1/src/gpu/GrDrawState.h#newcode865
src/gpu/GrDrawState.h:865: 
On 2012/05/24 19:30:31, bsalomon wrote:
> On 2012/05/24 19:04:44, robertphillips wrote:
> > The GrIODB reffed/unreffed both the textures & the render target so I added
> > refs/unrefs to both object types in here
> 
> Def. should be both
> 
> Hopefully we've already reduced a bunch of the operator = and operator ==
called
> (and are on the path to shrinking this object via Tom's work) but still I
wonder
> if fTextures and fRenderTarget should be in the Pod but not Memset section. 
It
> seems like we can still call memcmp on them.

Done.

[bsalomon, 2012-05-24 21:09:00]

LGTM

http://codereview.appspot.com/6251049/diff/1/src/gpu/GrContext.cpp
File src/gpu/GrContext.cpp (right):

http://codereview.appspot.com/6251049/diff/1/src/gpu/GrContext.cpp#newcode73
src/gpu/GrContext.cpp:73: fGpu->setDrawState(NULL);
On 2012/05/24 20:38:51, robertphillips wrote:
> It doesn't. The problem here is that the texture cache is deleted before the
gpu
> is freed. If the gpu is holding onto any textures things get messy

Hmm... I wonder if we can fix that. I think the cache should be able to just
unref all the textures is knows about without caring whether they actually are
deleted or not. Can you file a Skia issue to investigate this (you can assign
me)?

http://codereview.appspot.com/6251049/diff/1/src/gpu/GrDrawState.h
File src/gpu/GrDrawState.h (right):

http://codereview.appspot.com/6251049/diff/1/src/gpu/GrDrawState.h#newcode76
src/gpu/GrDrawState.h:76: this->reset();
On 2012/05/24 20:38:51, robertphillips wrote:
> I doubt it.
> 

Not a suggestion for this CL, but it might be kind of nice to have safe ref,
unref, setnulls, that take a ptr to an array and count.

[robertphillips, 2012-05-25 11:46:09]

committed as r4049

[robertphillips, 2012-05-28 12:38:48]

rolled back in r4051 due to Android Debug crash

[robertphillips, 2012-05-28 14:10:50]

Attempted speculative fix for Android issue in r4053

[robertphillips, 2012-05-28 14:11:07]

Rolled back again

[robertphillips, 2012-06-12 15:29:39]

PTAL

I have "fixed" the Android crash. In the original delivery I was greatly
extending the life span of some of the textures. A texture allocated in 1 GM
might live well into the following GM. When these textures were being freed in
the following GM a crash would ensue. The "fix" is to force the drawstate to
relinquish its ref of the textures once drawing is complete. This restores the
texture's lifespans to what they were before this CL (and "fixes" the Android
crash)

http://codereview.appspot.com/6251049/diff/18001/src/gpu/GrDrawState.h
File src/gpu/GrDrawState.h (right):

http://codereview.appspot.com/6251049/diff/18001/src/gpu/GrDrawState.h#newcod...
src/gpu/GrDrawState.h:847: 
Moved fFirstCoverageStage here to fix Linux alignment issue (the assert on line
125 was failing)

[bsalomon, 2012-06-13 13:21:35]

Can we see the release textures part separately? I think it interacts with stuff
Tom is or will be working on to move texture ownership out of GrDrawState and
into the custom effects.

Here is my proposal for how it could work, though I'd like Tom to comment on
whether this will work for him:

GrDrawState gets a releaseTextures method. Today it safe-set-nulls the fTextures
array. In the future it passes the signal to the list of custom effects to
release their textures.

Rather than adding a GrContext::releaseTextures function (and calls to
setTexture(i, NULL)) it seems like we could have
GrDrawState::AutoReleaseTextures that each GrContext::draw* call instantiates
before SetPaint is called.

[bsalomon, 2012-06-13 13:22:05]

Forgot to add TomH in previous publish.

[robertphillips, 2012-06-13 19:16:37]

Updated for delivery of releaseTextures portion in
http://codereview.appspot.com/6299081/

[bsalomon, 2012-06-13 19:46:46]

The infinite loop prevention is a bit hairy. However, the call
setTexture(NULL)/setRenderTarget(NULL) calls in the notify functions were
workarounds for the fact that draw state doesn't ref count. If the objects can't
disappear while GrGpu's drawState has a pointer to them, then we don't need the
checks/set-to-null code anymore. (We do still need to zero out the fHW* values
since they aren't ref counted and a new object could be created at the same
address).

http://codereview.appspot.com/6251049/diff/13003/src/gpu/GrContext.cpp
File src/gpu/GrContext.cpp (right):

http://codereview.appspot.com/6251049/diff/13003/src/gpu/GrContext.cpp#newcode77
src/gpu/GrContext.cpp:77: fDrawState->reset();
Are we sure this is a problem? Can't the cache just do its unref() before
GrDrawState does its unref()?

http://codereview.appspot.com/6251049/diff/13003/src/gpu/GrContext.cpp#newcode79
src/gpu/GrContext.cpp:79: fGpu->setDrawState(NULL);
I'm not sure I understand. It's draw state is the one above (that we just
reset).

[bsalomon, 2012-06-13 19:49:27]

http://codereview.appspot.com/6251049/diff/13003/src/gpu/GrContext.cpp
File src/gpu/GrContext.cpp (right):

http://codereview.appspot.com/6251049/diff/13003/src/gpu/GrContext.cpp#newcode79
src/gpu/GrContext.cpp:79: fGpu->setDrawState(NULL);
On 2012/06/13 19:46:46, bsalomon wrote:
> I'm not sure I understand. It's draw state is the one above (that we just
> reset).
s/It's/Its

[robertphillips, 2012-06-13 20:46:26]

Apparently the changes we made w/ releaseTextures obviated the need for the
extra cleanup in GrContext.

http://codereview.appspot.com/6251049/diff/13003/src/gpu/GrContext.cpp
File src/gpu/GrContext.cpp (right):

http://codereview.appspot.com/6251049/diff/13003/src/gpu/GrContext.cpp#newcode77
src/gpu/GrContext.cpp:77: fDrawState->reset();
On 2012/06/13 19:46:46, bsalomon wrote:
> Are we sure this is a problem? Can't the cache just do its unref() before
> GrDrawState does its unref()?

Done.

http://codereview.appspot.com/6251049/diff/13003/src/gpu/GrContext.cpp#newcode79
src/gpu/GrContext.cpp:79: fGpu->setDrawState(NULL);
On 2012/06/13 19:46:46, bsalomon wrote:
> I'm not sure I understand. It's draw state is the one above (that we just
> reset).

Done.

[TomH, 2012-06-13 21:01:22]

How could I not LGTM something that is a net reduction of LOC?

http://codereview.appspot.com/6251049/diff/14008/src/gpu/GrDrawState.h
File src/gpu/GrDrawState.h (right):

http://codereview.appspot.com/6251049/diff/14008/src/gpu/GrDrawState.h#newcode70
src/gpu/GrDrawState.h:70: fTextures[i] = NULL;
If we do need to clear these, wouldn't it be nice to just call
this->releaseTextures()?

[bsalomon, 2012-06-13 21:04:44]

LGTM (feel free to ignore or take the GrSafeAssign suggestion.)

http://codereview.appspot.com/6251049/diff/14008/src/gpu/GrDrawState.h
File src/gpu/GrDrawState.h (right):

http://codereview.appspot.com/6251049/diff/14008/src/gpu/GrDrawState.h#newcode70
src/gpu/GrDrawState.h:70: fTextures[i] = NULL;
On 2012/06/13 21:01:22, TomH wrote:
> If we do need to clear these, wouldn't it be nice to just call
> this->releaseTextures()?

They are initially garbage and we don't want to unref() a bad ptr.

http://codereview.appspot.com/6251049/diff/14008/src/gpu/GrDrawState.h#newcod...
src/gpu/GrDrawState.h:497: SkSafeRef(target);
This can be a one-liner: GrSafeAssign(fRenderTarget, target);
Same with setTexture().

[robertphillips, 2012-06-14 12:32:01]

Parking until after M21

http://codereview.appspot.com/6251049/diff/14008/src/gpu/GrDrawState.h
File src/gpu/GrDrawState.h (right):

http://codereview.appspot.com/6251049/diff/14008/src/gpu/GrDrawState.h#newcod...
src/gpu/GrDrawState.h:497: SkSafeRef(target);
On 2012/06/13 21:04:44, bsalomon wrote:
> This can be a one-liner: GrSafeAssign(fRenderTarget, target);
> Same with setTexture().

Done.

[robertphillips, 2012-06-22 12:01:58]

committed as r4298