Merge texture & render target pixel refs

Minor refactoring

[robertphillips, 2012-06-28 18:48:38]

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

http://codereview.appspot.com/6359045/diff/1/include/gpu/SkGrTexturePixelRef....
include/gpu/SkGrTexturePixelRef.h:10: 
I will rename the file (and update the .gyp (and begin the DEPS roll)) in
another pass

[bsalomon, 2012-06-28 19:16:17]

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

http://codereview.appspot.com/6359045/diff/1/src/gpu/SkGrTexturePixelRef.cpp#...
src/gpu/SkGrTexturePixelRef.cpp:64: desc.fFlags = kRenderTarget_GrTextureFlagBit
| kNoStencil_GrTextureFlagBit;
Digression:

This is a bit odd. We keep the RT aspect of this around (don't call abandonRT on
dst). However, if the caller actually drew to it using an SkGpuDevice it would
draw incorrectly because it has no stencil buffer.

I'm tempted to say we should fix this up (in another change). However, I think
the real answer is getting rid of kNoStencil_GrTextureFlagBit and getting
dynamic SB attach / detach working.

http://codereview.appspot.com/6359045/diff/1/src/gpu/SkGrTexturePixelRef.cpp#...
src/gpu/SkGrTexturePixelRef.cpp:81: fSurface = surface;
Should we always grab the asTexture() value if it's non-NULL (since it retains a
ref on the rt but not vice versa)?

Also, question tangentially to this change: Should GpuDevice just take a
Surface* as input (instead of separate RT and tex cons)? We'd say that the
surface param must have a non-NULL asRenderTarget() while asTexture() may or may
not be NULL. Right now its logic around holding onto one or both representations
of the same surface is confusing. The above suggestion might make fixing this a
bit neater.

http://codereview.appspot.com/6359045/diff/1/src/gpu/SkGrTexturePixelRef.cpp#...
src/gpu/SkGrTexturePixelRef.cpp:101: // Note that when copying a
render-target-backed pixel ref, we
I think with both this and your previous change, that introduced GrSurface, this
comment is less relevant. I'd be OK removing it since the user of SkGrPixelRef
has to go through either asTexture() or asRenderTarget(). But I don't feel
strongly about it.

[bsalomon, 2012-06-28 19:39:51]

I meant to also say LGTM.


On Thu, Jun 28, 2012 at 3:16 PM, <bsalomon@google.com> wrote:

>
> http://codereview.appspot.com/**6359045/diff/1/src/gpu/**
>
SkGrTexturePixelRef.cpp<http://codereview.appspot.com/6359045/diff/1/src/gpu/SkGrTexturePixelRef.cpp>
> File src/gpu/SkGrTexturePixelRef.**cpp (right):
>
> http://codereview.appspot.com/**6359045/diff/1/src/gpu/**
>
SkGrTexturePixelRef.cpp#**newcode64<http://codereview.appspot.com/6359045/diff/1/src/gpu/SkGrTexturePixelRef.cpp#newcode64>
> src/gpu/SkGrTexturePixelRef.**cpp:64: desc.fFlags =
> kRenderTarget_GrTextureFlagBit | kNoStencil_GrTextureFlagBit;
> Digression:
>
> This is a bit odd. We keep the RT aspect of this around (don't call
> abandonRT on dst). However, if the caller actually drew to it using an
> SkGpuDevice it would draw incorrectly because it has no stencil buffer.
>
> I'm tempted to say we should fix this up (in another change). However, I
> think the real answer is getting rid of kNoStencil_GrTextureFlagBit and
> getting dynamic SB attach / detach working.
>
> http://codereview.appspot.com/**6359045/diff/1/src/gpu/**
>
SkGrTexturePixelRef.cpp#**newcode81<http://codereview.appspot.com/6359045/diff/1/src/gpu/SkGrTexturePixelRef.cpp#newcode81>
> src/gpu/SkGrTexturePixelRef.**cpp:81: fSurface = surface;
> Should we always grab the asTexture() value if it's non-NULL (since it
> retains a ref on the rt but not vice versa)?
>
> Also, question tangentially to this change: Should GpuDevice just take a
> Surface* as input (instead of separate RT and tex cons)? We'd say that
> the surface param must have a non-NULL asRenderTarget() while
> asTexture() may or may not be NULL. Right now its logic around holding
> onto one or both representations of the same surface is confusing. The
> above suggestion might make fixing this a bit neater.
>
> http://codereview.appspot.com/**6359045/diff/1/src/gpu/**
>
SkGrTexturePixelRef.cpp#**newcode101<http://codereview.appspot.com/6359045/diff/1/src/gpu/SkGrTexturePixelRef.cpp#newcode101>
> src/gpu/SkGrTexturePixelRef.**cpp:101: // Note that when copying a
> render-target-backed pixel ref, we
> I think with both this and your previous change, that introduced
> GrSurface, this comment is less relevant. I'd be OK removing it since
> the user of SkGrPixelRef has to go through either asTexture() or
> asRenderTarget(). But I don't feel strongly about it.
>
>
http://codereview.appspot.com/**6359045/<http://codereview.appspot.com/6359045/>
>

[robertphillips, 2012-06-28 19:49:47]

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

http://codereview.appspot.com/6359045/diff/1/src/gpu/SkGrTexturePixelRef.cpp#...
src/gpu/SkGrTexturePixelRef.cpp:64: desc.fFlags = kRenderTarget_GrTextureFlagBit
| kNoStencil_GrTextureFlagBit;
It is pretty weird and I agree that getting the dynamic SB attach/detach working
is the correct fix. What do you think about this short term fix?

http://codereview.appspot.com/6359045/diff/1/src/gpu/SkGrTexturePixelRef.cpp#...
src/gpu/SkGrTexturePixelRef.cpp:81: fSurface = surface;
First part - Done.

Second part - this was just something easy to do during the IO9 keynote. I think
the GpuDevice refactoring would be a good next step. Even with that change I
think we would still want to do this here.

http://codereview.appspot.com/6359045/diff/1/src/gpu/SkGrTexturePixelRef.cpp#...
src/gpu/SkGrTexturePixelRef.cpp:101: // Note that when copying a
render-target-backed pixel ref, we
I would like to leave it in for the time being. It was informative for me.

[bsalomon, 2012-06-28 19:54:15]

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

http://codereview.appspot.com/6359045/diff/1/src/gpu/SkGrTexturePixelRef.cpp#...
src/gpu/SkGrTexturePixelRef.cpp:64: desc.fFlags = kRenderTarget_GrTextureFlagBit
| kNoStencil_GrTextureFlagBit;
On 2012/06/28 19:49:47, robertphillips wrote:
> It is pretty weird and I agree that getting the dynamic SB attach/detach
working
> is the correct fix. What do you think about this short term fix?

LGTM

http://codereview.appspot.com/6359045/diff/1/src/gpu/SkGrTexturePixelRef.cpp#...
src/gpu/SkGrTexturePixelRef.cpp:81: fSurface = surface;
On 2012/06/28 19:49:47, robertphillips wrote:
> First part - Done.
> 
> Second part - this was just something easy to do during the IO9 keynote. I
think
> the GpuDevice refactoring would be a good next step. Even with that change I
> think we would still want to do this here.

Let's not call the virtual twice:

fSurface = surface->asTexture();
if (!fSurface) {
   fSurface = surface;
}

http://codereview.appspot.com/6359045/diff/1/src/gpu/SkGrTexturePixelRef.cpp#...
src/gpu/SkGrTexturePixelRef.cpp:101: // Note that when copying a
render-target-backed pixel ref, we
On 2012/06/28 19:49:47, robertphillips wrote:
> I would like to leave it in for the time being. It was informative for me.

That's a good reason to leave it in!

[robertphillips, 2012-06-28 20:03:03]

committed as r4396

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

http://codereview.appspot.com/6359045/diff/1/src/gpu/SkGrTexturePixelRef.cpp#...
src/gpu/SkGrTexturePixelRef.cpp:81: fSurface = surface;
On 2012/06/28 19:54:16, bsalomon wrote:
> On 2012/06/28 19:49:47, robertphillips wrote:
> > First part - Done.
> > 
> > Second part - this was just something easy to do during the IO9 keynote. I
> think
> > the GpuDevice refactoring would be a good next step. Even with that change I
> > think we would still want to do this here.
> 
> Let's not call the virtual twice:
> 
> fSurface = surface->asTexture();
> if (!fSurface) {
>    fSurface = surface;
> }

Done.