Refactor Gaussian blur and morphology into GrContext

These functions don't really belong in SkGpuDevice.

[bsalomon, 2012-03-05 14:44:16]

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

http://codereview.appspot.com/5720060/diff/1/include/gpu/GrContext.h#newcode593
include/gpu/GrContext.h:593: GrAutoScratchTexture* temp1,
Why does this take GrASTs while the morphology takes GrTextures? More broadly,
is there an advantage to having the caller provide the textures? I'm wondering
if gaussianBlur should take a enum or bool indicating whether the srcTexture
must be preserved and no textures. It would narrow the API a little bit and
prevent some accidental bad usage (e.g. improperly sized textures or textures
with an incorrect pixel format).

http://codereview.appspot.com/5720060/diff/1/include/gpu/GrContext.h#newcode626
include/gpu/GrContext.h:626: void convolve(GrTexture* texture,
Does this (and applyMorphology below) need to be public anymore?  Also, could
they be locally defined in the cpp (in anony namespace).

[Stephen White, 2012-03-05 15:33:02]

Thanks for your comments.

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

http://codereview.appspot.com/5720060/diff/1/include/gpu/GrContext.h#newcode593
include/gpu/GrContext.h:593: GrAutoScratchTexture* temp1,
On 2012/03/05 14:44:17, bsalomon wrote:
> Why does this take GrASTs while the morphology takes GrTextures? More broadly,
> is there an advantage to having the caller provide the textures?

The blur needs to grow its temporary textures by a variable margin in order to
accommodate scaling and/or convolution margins which the caller doesn't know;
the morphology doesn't.

I'd like just to return a GrTexture*, do all allocation of temporaries
internally, and not require the caller to allocate anything, but the refcounting
is a little murky.  E.g., I could just ref whatever I return (so it doesn't get
freed inadvertently), but then the srcTexture would have two refs, while a
temporary would only have one.  That's probably still the right thing to do,
though.  What do you think?

The other thing is that the scratch textures would get unlocked by
gaussianBlur() before returning, and one of them might be the return value.  I
assume that's ok as long as I don't try to allocate anything else from cache
before it's used?

> if gaussianBlur should take a enum or bool indicating whether the srcTexture
> must be preserved and no textures. It would narrow the API a little bit and
> prevent some accidental bad usage (e.g. improperly sized textures or textures
> with an incorrect pixel format).

A good idea.  I was planning to do all of the above in a followup change, and
keep this one to simply moving code around.

http://codereview.appspot.com/5720060/diff/1/include/gpu/GrContext.h#newcode626
include/gpu/GrContext.h:626: void convolve(GrTexture* texture,
On 2012/03/05 14:44:17, bsalomon wrote:
> Does this (and applyMorphology below) need to be public anymore?

Nope, can be private.

> Also, could
> they be locally defined in the cpp (in anony namespace).

They could, but they'd need to take the GrGpu as a param.  I'll give that a try
to see how it looks.

[bsalomon, 2012-03-05 15:54:05]

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

http://codereview.appspot.com/5720060/diff/1/include/gpu/GrContext.h#newcode593
include/gpu/GrContext.h:593: GrAutoScratchTexture* temp1,
On 2012/03/05 15:33:02, Stephen White wrote:
> On 2012/03/05 14:44:17, bsalomon wrote:
> > Why does this take GrASTs while the morphology takes GrTextures? More
broadly,
> > is there an advantage to having the caller provide the textures?
> 
> The blur needs to grow its temporary textures by a variable margin in order to
> accommodate scaling and/or convolution margins which the caller doesn't know;
> the morphology doesn't.
> 
> I'd like just to return a GrTexture*, do all allocation of temporaries
> internally, and not require the caller to allocate anything, but the
refcounting
> is a little murky.  E.g., I could just ref whatever I return (so it doesn't
get
> freed inadvertently), but then the srcTexture would have two refs, while a
> temporary would only have one.  That's probably still the right thing to do,
> though.  What do you think?


Maybe we new class (or replacement for GrAutoScratchTexture) that can properly
release a texture from two possible creations types:

1) just passed a GrTexture, unrefs on destructor
2) cached texture initialized by GrContext:TextureCacheEntry, calls
context->unlockTexture()

Then the interface could look something like

void guassianBlur(GrTexture* src, bool preserveSrc, const SkRect& rect, float
sigmaX, float sigmaY, GrNewTextureThingy* result)

where result gets initialized in the call, either by an additional reference on
src, or to a scratch texture locked by the function.

 
> 
> The other thing is that the scratch textures would get unlocked by
> gaussianBlur() before returning, and one of them might be the return value.  I
> assume that's ok as long as I don't try to allocate anything else from cache
> before it's used?

I think if the scratch is returned it has to remain locked until the caller is
done with it. In theory it could be deleted as soon as it is unlocked.

> 
> > if gaussianBlur should take a enum or bool indicating whether the srcTexture
> > must be preserved and no textures. It would narrow the API a little bit and
> > prevent some accidental bad usage (e.g. improperly sized textures or
textures
> > with an incorrect pixel format).
> 
> A good idea.  I was planning to do all of the above in a followup change, and
> keep this one to simply moving code around.

That's fine to checkin as is and modify later.

[Stephen White, 2012-03-05 16:41:28]

On 2012/03/05 15:54:05, bsalomon wrote:
> http://codereview.appspot.com/5720060/diff/1/include/gpu/GrContext.h
> File include/gpu/GrContext.h (right):
> 
>
http://codereview.appspot.com/5720060/diff/1/include/gpu/GrContext.h#newcode593
> include/gpu/GrContext.h:593: GrAutoScratchTexture* temp1,
> On 2012/03/05 15:33:02, Stephen White wrote:
> > On 2012/03/05 14:44:17, bsalomon wrote:
> > > Why does this take GrASTs while the morphology takes GrTextures? More
> broadly,
> > > is there an advantage to having the caller provide the textures?
> > 
> > The blur needs to grow its temporary textures by a variable margin in order
to
> > accommodate scaling and/or convolution margins which the caller doesn't
know;
> > the morphology doesn't.
> > 
> > I'd like just to return a GrTexture*, do all allocation of temporaries
> > internally, and not require the caller to allocate anything, but the
> refcounting
> > is a little murky.  E.g., I could just ref whatever I return (so it doesn't
> get
> > freed inadvertently), but then the srcTexture would have two refs, while a
> > temporary would only have one.  That's probably still the right thing to do,
> > though.  What do you think?
> 
> 
> Maybe we new class (or replacement for GrAutoScratchTexture) that can properly
> release a texture from two possible creations types:
> 
> 1) just passed a GrTexture, unrefs on destructor
> 2) cached texture initialized by GrContext:TextureCacheEntry, calls
> context->unlockTexture()
> 
> Then the interface could look something like
> 
> void guassianBlur(GrTexture* src, bool preserveSrc, const SkRect& rect, float
> sigmaX, float sigmaY, GrNewTextureThingy* result)
> 
> where result gets initialized in the call, either by an additional reference
on
> src, or to a scratch texture locked by the function.
> 
>  
> > 
> > The other thing is that the scratch textures would get unlocked by
> > gaussianBlur() before returning, and one of them might be the return value. 
I
> > assume that's ok as long as I don't try to allocate anything else from cache
> > before it's used?
> 
> I think if the scratch is returned it has to remain locked until the caller is
> done with it. In theory it could be deleted as soon as it is unlocked.
> 
> > 
> > > if gaussianBlur should take a enum or bool indicating whether the
srcTexture
> > > must be preserved and no textures. It would narrow the API a little bit
and
> > > prevent some accidental bad usage (e.g. improperly sized textures or
> textures
> > > with an incorrect pixel format).
> > 
> > A good idea.  I was planning to do all of the above in a followup change,
and
> > keep this one to simply moving code around.
> 
> That's fine to checkin as is and modify later.

OK, uploaded as patchset #2; PTAL (not sure if it sent an email).

[bsalomon, 2012-03-05 16:52:43]

On 2012/03/05 16:41:28, Stephen White wrote:
> On 2012/03/05 15:54:05, bsalomon wrote:
> > http://codereview.appspot.com/5720060/diff/1/include/gpu/GrContext.h
> > File include/gpu/GrContext.h (right):
> > 
> >
>
http://codereview.appspot.com/5720060/diff/1/include/gpu/GrContext.h#newcode593
> > include/gpu/GrContext.h:593: GrAutoScratchTexture* temp1,
> > On 2012/03/05 15:33:02, Stephen White wrote:
> > > On 2012/03/05 14:44:17, bsalomon wrote:
> > > > Why does this take GrASTs while the morphology takes GrTextures? More
> > broadly,
> > > > is there an advantage to having the caller provide the textures?
> > > 
> > > The blur needs to grow its temporary textures by a variable margin in
order
> to
> > > accommodate scaling and/or convolution margins which the caller doesn't
> know;
> > > the morphology doesn't.
> > > 
> > > I'd like just to return a GrTexture*, do all allocation of temporaries
> > > internally, and not require the caller to allocate anything, but the
> > refcounting
> > > is a little murky.  E.g., I could just ref whatever I return (so it
doesn't
> > get
> > > freed inadvertently), but then the srcTexture would have two refs, while a
> > > temporary would only have one.  That's probably still the right thing to
do,
> > > though.  What do you think?
> > 
> > 
> > Maybe we new class (or replacement for GrAutoScratchTexture) that can
properly
> > release a texture from two possible creations types:
> > 
> > 1) just passed a GrTexture, unrefs on destructor
> > 2) cached texture initialized by GrContext:TextureCacheEntry, calls
> > context->unlockTexture()
> > 
> > Then the interface could look something like
> > 
> > void guassianBlur(GrTexture* src, bool preserveSrc, const SkRect& rect,
float
> > sigmaX, float sigmaY, GrNewTextureThingy* result)
> > 
> > where result gets initialized in the call, either by an additional reference
> on
> > src, or to a scratch texture locked by the function.
> > 
> >  
> > > 
> > > The other thing is that the scratch textures would get unlocked by
> > > gaussianBlur() before returning, and one of them might be the return
value. 
> I
> > > assume that's ok as long as I don't try to allocate anything else from
cache
> > > before it's used?
> > 
> > I think if the scratch is returned it has to remain locked until the caller
is
> > done with it. In theory it could be deleted as soon as it is unlocked.
> > 
> > > 
> > > > if gaussianBlur should take a enum or bool indicating whether the
> srcTexture
> > > > must be preserved and no textures. It would narrow the API a little bit
> and
> > > > prevent some accidental bad usage (e.g. improperly sized textures or
> > textures
> > > > with an incorrect pixel format).
> > > 
> > > A good idea.  I was planning to do all of the above in a followup change,
> and
> > > keep this one to simply moving code around.
> > 
> > That's fine to checkin as is and modify later.
> 
> OK, uploaded as patchset #2; PTAL (not sure if it sent an email).


LGTM. It's OK to make a second anonymous namespace closer to the call site if
you like.

[Stephen White, 2012-03-05 22:51:09]

On 2012/03/05 16:52:43, bsalomon wrote:
> On 2012/03/05 16:41:28, Stephen White wrote:
> > On 2012/03/05 15:54:05, bsalomon wrote:
> > > http://codereview.appspot.com/5720060/diff/1/include/gpu/GrContext.h
> > > File include/gpu/GrContext.h (right):
> > > 
> > >
> >
>
http://codereview.appspot.com/5720060/diff/1/include/gpu/GrContext.h#newcode593
> > > include/gpu/GrContext.h:593: GrAutoScratchTexture* temp1,
> > > On 2012/03/05 15:33:02, Stephen White wrote:
> > > > On 2012/03/05 14:44:17, bsalomon wrote:
> > > > > Why does this take GrASTs while the morphology takes GrTextures? More
> > > broadly,
> > > > > is there an advantage to having the caller provide the textures?
> > > > 
> > > > The blur needs to grow its temporary textures by a variable margin in
> order
> > to
> > > > accommodate scaling and/or convolution margins which the caller doesn't
> > know;
> > > > the morphology doesn't.
> > > > 
> > > > I'd like just to return a GrTexture*, do all allocation of temporaries
> > > > internally, and not require the caller to allocate anything, but the
> > > refcounting
> > > > is a little murky.  E.g., I could just ref whatever I return (so it
> doesn't
> > > get
> > > > freed inadvertently), but then the srcTexture would have two refs, while
a
> > > > temporary would only have one.  That's probably still the right thing to
> do,
> > > > though.  What do you think?
> > > 
> > > 
> > > Maybe we new class (or replacement for GrAutoScratchTexture) that can
> properly
> > > release a texture from two possible creations types:
> > > 
> > > 1) just passed a GrTexture, unrefs on destructor
> > > 2) cached texture initialized by GrContext:TextureCacheEntry, calls
> > > context->unlockTexture()
> > > 
> > > Then the interface could look something like
> > > 
> > > void guassianBlur(GrTexture* src, bool preserveSrc, const SkRect& rect,
> float
> > > sigmaX, float sigmaY, GrNewTextureThingy* result)
> > > 
> > > where result gets initialized in the call, either by an additional
reference
> > on
> > > src, or to a scratch texture locked by the function.
> > > 
> > >  
> > > > 
> > > > The other thing is that the scratch textures would get unlocked by
> > > > gaussianBlur() before returning, and one of them might be the return
> value. 
> > I
> > > > assume that's ok as long as I don't try to allocate anything else from
> cache
> > > > before it's used?
> > > 
> > > I think if the scratch is returned it has to remain locked until the
caller
> is
> > > done with it. In theory it could be deleted as soon as it is unlocked.
> > > 
> > > > 
> > > > > if gaussianBlur should take a enum or bool indicating whether the
> > srcTexture
> > > > > must be preserved and no textures. It would narrow the API a little
bit
> > and
> > > > > prevent some accidental bad usage (e.g. improperly sized textures or
> > > textures
> > > > > with an incorrect pixel format).
> > > > 
> > > > A good idea.  I was planning to do all of the above in a followup
change,
> > and
> > > > keep this one to simply moving code around.
> > > 
> > > That's fine to checkin as is and modify later.
> > 
> > OK, uploaded as patchset #2; PTAL (not sure if it sent an email).
> 
> 
> LGTM. It's OK to make a second anonymous namespace closer to the call site if
> you like.

Landed with r3327 (and mop-up in r3329); closed.