Make all pixel ops go thru ctx so we can correctly flush. Unify two texture upload code paths.

[TomH, 2011-11-16 19:56:15]

Rushed and low-quality LGTM. :)

Please create a bug to consider a common base class for RenderTarget & Texture
so we can reduce code duplication.

http://codereview.appspot.com/5373108/diff/2001/gm/texdata.cpp
File gm/texdata.cpp (right):

http://codereview.appspot.com/5373108/diff/2001/gm/texdata.cpp#newcode127
gm/texdata.cpp:127: // BUG: uploadTextureData doesn't force a flush
This comment is now out of date.

http://codereview.appspot.com/5373108/diff/2001/src/gpu/GrAtlas.cpp
File src/gpu/GrAtlas.cpp (right):

http://codereview.appspot.com/5373108/diff/2001/src/gpu/GrAtlas.cpp#newcode113
src/gpu/GrAtlas.cpp:113: GrContext* context = fTexture->getContext();
Maybe a comment here (or in the GrContext header) about why you're calling the
internal version directly?

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

http://codereview.appspot.com/5373108/diff/2001/src/gpu/GrContext.cpp#newcode...
src/gpu/GrContext.cpp:1670: // TODO: code read pixels for textures that aren't
rendertargets
Comment obsolete? Parameter to this fn is a texture.

http://codereview.appspot.com/5373108/diff/2001/src/gpu/GrContext.cpp#newcode...
src/gpu/GrContext.cpp:1807: // (e.g. glDrawPixels on desktop GL).
Is this a significant performance issue?
Also, you've inserted a flush() between this and the writePixels() aka
uploadTextureData() that it presumably formerly applied to - should it be moved?

http://codereview.appspot.com/5373108/diff/2001/src/gpu/SkGpuDevice.cpp
File src/gpu/SkGpuDevice.cpp (right):

http://codereview.appspot.com/5373108/diff/2001/src/gpu/SkGpuDevice.cpp#newco...
src/gpu/SkGpuDevice.cpp:318: config, bitmap.getPixels(), bitmap.rowBytes());
The need to set fNeedPrepareRenderTarget went away?

[bsalomon, 2011-11-16 20:14:02]

Thanks, good comments!

http://codereview.appspot.com/5373108/diff/2001/gm/texdata.cpp
File gm/texdata.cpp (right):

http://codereview.appspot.com/5373108/diff/2001/gm/texdata.cpp#newcode127
gm/texdata.cpp:127: // BUG: uploadTextureData doesn't force a flush
On 2011/11/16 19:56:15, TomH wrote:
> This comment is now out of date.
Ah, yes, I didn't notice that. Will fix.

http://codereview.appspot.com/5373108/diff/2001/src/gpu/GrAtlas.cpp
File src/gpu/GrAtlas.cpp (right):

http://codereview.appspot.com/5373108/diff/2001/src/gpu/GrAtlas.cpp#newcode113
src/gpu/GrAtlas.cpp:113: GrContext* context = fTexture->getContext();
On 2011/11/16 19:56:15, TomH wrote:
> Maybe a comment here (or in the GrContext header) about why you're calling the
> internal version directly?
Will do.

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

http://codereview.appspot.com/5373108/diff/2001/src/gpu/GrContext.cpp#newcode...
src/gpu/GrContext.cpp:1670: // TODO: code read pixels for textures that aren't
rendertargets
On 2011/11/16 19:56:15, TomH wrote:
> Comment obsolete? Parameter to this fn is a texture.

The impl below calls readRenderTargetPixels() and fails if the texture isn't
also an RT. So, the comment is still valid, we fail read pixels on textures that
aren't also RTs.

http://codereview.appspot.com/5373108/diff/2001/src/gpu/GrContext.cpp#newcode...
src/gpu/GrContext.cpp:1807: // (e.g. glDrawPixels on desktop GL).
On 2011/11/16 19:56:15, TomH wrote:
> Is this a significant performance issue?
Not sure. I think Justin looked at it and didn't see a win on one particular
platform.

> Also, you've inserted a flush() between this and the writePixels() aka
> uploadTextureData() that it presumably formerly applied to - should it be
moved?

I will remove these lines:

if (!(kDontFlush_PixelOpsFlag & flags)) {
    this->flush();
}

and change the writePixels line (1817) to
this->internalWritePixels(texture, 0, 0, width, height, config, buffer,
rowBytes, flags)

Then we would never flush twice, and never flush when the incoming flag says not
to.

http://codereview.appspot.com/5373108/diff/2001/src/gpu/SkGpuDevice.cpp
File src/gpu/SkGpuDevice.cpp (right):

http://codereview.appspot.com/5373108/diff/2001/src/gpu/SkGpuDevice.cpp#newco...
src/gpu/SkGpuDevice.cpp:318: config, bitmap.getPixels(), bitmap.rowBytes());
On 2011/11/16 19:56:15, TomH wrote:
> The need to set fNeedPrepareRenderTarget went away?

Yeah, the SkGpuDevice has a bad habit of calling getRenderTarget() to determine
whether to re-send some state to GrContext. fNeedPrepareRenderTarget was being
set because we we're calling setRenderTarget without emitting all that state.
The new code doesn't call setRenderTarget.

[TomH, 2011-11-16 20:21:43]

On 2011/11/16 20:14:02, bsalomon wrote:
> On 2011/11/16 19:56:15, TomH wrote:
> > The need to set fNeedPrepareRenderTarget went away?
> 
> Yeah, the SkGpuDevice has a bad habit of calling getRenderTarget() to
determine
> whether to re-send some state to GrContext. fNeedPrepareRenderTarget was being
> set because we we're calling setRenderTarget without emitting all that state.
> The new code doesn't call setRenderTarget.

OK. Please also file a bug to clean up our bad habits?
(May need multiple bugs for that, my sugar habit is not easy to break.)

[bsalomon, 2011-11-16 20:54:45]

Closed with r2701.