Add code path for client resolve of GrRenderTarget. SkGpuDevice resolves its rendertarget in ::flush

[junov1, 2012-01-30 20:28:41]

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

http://codereview.appspot.com/5580049/diff/2001/src/gpu/GrContext.cpp#newcode...
src/gpu/GrContext.cpp:1726: fGpu->resolveRenderTarget(this->getRenderTarget());
I think the docs for kForceCurrentRenderTarget_FlushBit need to be a bit clearer
about what happens here.  In particular, mention that flushing can be heavy when
AA is enabled

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

http://codereview.appspot.com/5580049/diff/2001/src/gpu/SkGpuDevice.cpp#newco...
src/gpu/SkGpuDevice.cpp:1773: fContext->resolveRenderTarget(fRenderTarget);
You<re making flush quite a bit heavier than it used to be...
Suppose chromium's compositor started using SkCanvas::flush to prep canvas
layers.  Would this CL make it undesirable to to have redundant flush calls?  I
think the compositor is smart enough to only update layer contents when the
layer is dirty, but still...  is it something we need to be careful about going
forward?

Would it be worth considering using a flag like in GrContext::flush?

Should the documentation mention something about this new behavior?

[bsalomon, 2012-01-30 20:40:12]

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

http://codereview.appspot.com/5580049/diff/2001/src/gpu/GrContext.cpp#newcode...
src/gpu/GrContext.cpp:1726: fGpu->resolveRenderTarget(this->getRenderTarget());
On 2012/01/30 20:28:41, junov1 wrote:
> I think the docs for kForceCurrentRenderTarget_FlushBit need to be a bit
clearer
> about what happens here.  In particular, mention that flushing can be heavy
when
> AA is enabled

Actually I don't think we should resolve here at all. The bound FBO will be the
MSAA buffer not the resolved buffer. I'll revert this part of the change. (Also,
I'm not sure that we really want to preserve this flag at all).

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

http://codereview.appspot.com/5580049/diff/2001/src/gpu/SkGpuDevice.cpp#newco...
src/gpu/SkGpuDevice.cpp:1773: fContext->resolveRenderTarget(fRenderTarget);
On 2012/01/30 20:28:41, junov1 wrote:
> You<re making flush quite a bit heavier than it used to be...
> Suppose chromium's compositor started using SkCanvas::flush to prep canvas
> layers.  Would this CL make it undesirable to to have redundant flush calls? 
I
> think the compositor is smart enough to only update layer contents when the
> layer is dirty, but still...  is it something we need to be careful about
going
> forward?
> 
> Would it be worth considering using a flag like in GrContext::flush?
> 
> Should the documentation mention something about this new behavior?

The render target tracks whether it needs resolving or not. Redundant device
flushes wouldn't wind up re-resolving the render target.

[TomH, 2012-01-30 21:17:47]

Only nits in comments, but as may be obvious I don't think I understand enough
of this to be a reviewer. Let's make sure junov agrees with your responses to
his review...

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

http://codereview.appspot.com/5580049/diff/2001/include/gpu/GrContext.h#newco...
include/gpu/GrContext.h:567: * Resolves a render target that has full scene AA.
Any pending draws to the
I don't know what "full scene" means in this context.

http://codereview.appspot.com/5580049/diff/2001/include/gpu/GrContext.h#newco...
include/gpu/GrContext.h:570: * This is only necessary when a client wants to use
the underlying 3D API
Do we really need to say "3D" here?
How about when a client wants direct access to the underlying graphics API?

http://codereview.appspot.com/5580049/diff/6001/include/gpu/GrRenderTarget.h
File include/gpu/GrRenderTarget.h (right):

http://codereview.appspot.com/5580049/diff/6001/include/gpu/GrRenderTarget.h#...
include/gpu/GrRenderTarget.h:116: * Resolves MSAA buffer to the resolved
renderbuffer or texture.
Resolves foo to the resolved bar? I can make neither heads nor tails of that -
even if "resolve" was a well-defined verb (which it isn't to the uninitiated).

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

http://codereview.appspot.com/5580049/diff/6001/src/gpu/GrContext.cpp#newcode...
src/gpu/GrContext.cpp:1908: // this to our clients, though.
Do we promise that any pending draws will be flushed?
Let's be explicit about that semantic?

[bsalomon, 2012-01-30 21:34:19]

The latest patch attempts to improve the comments cited by Tom.

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

http://codereview.appspot.com/5580049/diff/2001/include/gpu/GrContext.h#newco...
include/gpu/GrContext.h:567: * Resolves a render target that has full scene AA.
Any pending draws to the
On 2012/01/30 21:17:47, TomH wrote:
> I don't know what "full scene" means in this context.
Changed to MSAA. I was using "full scene AA" to be more general since there are
other MSAA like techniques that can be applied to the whole framebuffer (e.g.
coverage sampling antialiasing). It is confusing, though.

http://codereview.appspot.com/5580049/diff/2001/include/gpu/GrContext.h#newco...
include/gpu/GrContext.h:570: * This is only necessary when a client wants to use
the underlying 3D API
On 2012/01/30 21:17:47, TomH wrote:
> Do we really need to say "3D" here?
> How about when a client wants direct access to the underlying graphics API?

Done.

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

http://codereview.appspot.com/5580049/diff/6001/src/gpu/GrContext.cpp#newcode...
src/gpu/GrContext.cpp:1908: // this to our clients, though.
On 2012/01/30 21:17:48, TomH wrote:
> Do we promise that any pending draws will be flushed?
> Let's be explicit about that semantic?

No, we only promise that pending draws TO THIS TARGET are flushed. In practice
we are flushing all pending draws but that may change.

[junov1, 2012-01-30 21:55:04]

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

http://codereview.appspot.com/5580049/diff/6001/include/gpu/GrContext.h#newco...
include/gpu/GrContext.h:567: * Resolves a render target that has full scene AA.
Any pending draws to the
full scene == MS ?

http://codereview.appspot.com/5580049/diff/6001/include/gpu/GrContext.h#newco...
include/gpu/GrContext.h:568: * target will be executed before the resolve.
For sake of clarity, perhaps explain that resolve means downsampling the
intermediate MSAA buffer into the final render target.

http://codereview.appspot.com/5580049/diff/6001/include/gpu/GrContext.h#newco...
include/gpu/GrContext.h:570: * This is only necessary when a client wants to use
the underlying 3D API
This is a little vague.  How about: "This is only necessary when a client wants
to bypass skia and use the underlying render target directly with the native
graphics API."

http://codereview.appspot.com/5580049/diff/6001/include/gpu/GrRenderTarget.h
File include/gpu/GrRenderTarget.h (right):

http://codereview.appspot.com/5580049/diff/6001/include/gpu/GrRenderTarget.h#...
include/gpu/GrRenderTarget.h:116: * Resolves MSAA buffer to the resolved
renderbuffer or texture.
On 2012/01/30 21:17:48, TomH wrote:
> Resolves foo to the resolved bar? I can make neither heads nor tails of that -
> even if "resolve" was a well-defined verb (which it isn't to the uninitiated).
How about this:

Resolves the render target by downsampling the intermediate MSAA buffer into the
final renderbuffer or texture.

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

http://codereview.appspot.com/5580049/diff/6001/src/gpu/GrContext.cpp#newcode...
src/gpu/GrContext.cpp:1910: fGpu->resolveRenderTarget(target);
Do you think we might also need this->flush() after resolveRenderTarget?

http://codereview.appspot.com/5580049/diff/6001/src/gpu/GrGpuGL.cpp
File src/gpu/GrGpuGL.cpp (right):

http://codereview.appspot.com/5580049/diff/6001/src/gpu/GrGpuGL.cpp#newcode2055
src/gpu/GrGpuGL.cpp:2055: this->onResolveRenderTarget(texRT);
Is there are reason not go through resolveRenderTarget?  Are you deliberately
skipping handleDirtyContext()?

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

http://codereview.appspot.com/5580049/diff/6001/src/gpu/SkGpuDevice.cpp#newco...
src/gpu/SkGpuDevice.cpp:1772: fContext->flush();
This call to flush is probably not necessary anymore since
GrContext::resolveRenderTarget calls flush

[junov1, 2012-01-30 21:56:41]

FYI: Comment #6 was based on patch set 3

[bsalomon, 2012-01-30 22:22:06]

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

http://codereview.appspot.com/5580049/diff/6001/src/gpu/GrContext.cpp#newcode...
src/gpu/GrContext.cpp:1910: fGpu->resolveRenderTarget(target);
On 2012/01/30 21:55:04, junov1 wrote:
> Do you think we might also need this->flush() after resolveRenderTarget?

No, we don't need it. GrGpu has no facility for deferring any operations, it
always performs draws, clears, resolves, etc immediately.

http://codereview.appspot.com/5580049/diff/6001/src/gpu/GrGpuGL.cpp
File src/gpu/GrGpuGL.cpp (right):

http://codereview.appspot.com/5580049/diff/6001/src/gpu/GrGpuGL.cpp#newcode2055
src/gpu/GrGpuGL.cpp:2055: this->onResolveRenderTarget(texRT);
On 2012/01/30 21:55:04, junov1 wrote:
> Is there are reason not go through resolveRenderTarget?  Are you deliberately
> skipping handleDirtyContext()?

Yes, it's unnecessary. We should already have looked at whether the 3d context
is dirty by the time we get to any of the callsites from within grgpu (or
subclasses). Otherwise, we likely already made wrong assumptions about the
context state by the time the GrGpu object decides it needs to resolve.

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

http://codereview.appspot.com/5580049/diff/6001/src/gpu/SkGpuDevice.cpp#newco...
src/gpu/SkGpuDevice.cpp:1772: fContext->flush();
On 2012/01/30 21:55:04, junov1 wrote:
> This call to flush is probably not necessary anymore since
> GrContext::resolveRenderTarget calls flush

Good point, removed in the latest patch.

[junov1, 2012-01-30 23:00:28]

LGTM