cc: Use the onscreen context to perform composited ganesh filters.

cc: Use the onscreen context to perform composited ganesh filters.

This avoids the need for an offscreen context at all. Since the GPU
rasterization path is already doing similar stuff, we need to support
both cc and ganesh using the same context. This is a bit trickier
since it occurs in the middle of drawing a frame instead of between
frames, but as long as we restore all state that GLRenderer ever sets,
it works.

R=enne@chromium.org, senorblanco@chromium.org
BUG=366132
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=265810

[danakj, 2014-04-23 19:44:11]

WDYT? Does this look like a clean way to do this? Any suggestions?

[enne (OOO), 2014-04-23 19:53:40]

https://codereview.chromium.org/250083002/diff/40001/cc/output/gl_renderer.cc
File cc/output/gl_renderer.cc (right):

https://codereview.chromium.org/250083002/diff/40001/cc/output/gl_renderer.cc...
cc/output/gl_renderer.cc:180: class GLRenderer::UseGrContext {
ScopedUseGrContext?

https://codereview.chromium.org/250083002/diff/40001/cc/output/gl_renderer.cc...
cc/output/gl_renderer.cc:210: DISALLOW_COPY_AND_ASSIGN(UseGrContext);
Stick a blank line before DISALLOW for readability?

https://codereview.chromium.org/250083002/diff/40001/cc/output/gl_renderer.cc...
cc/output/gl_renderer.cc:3140: void GLRenderer::RestoreGLState(DrawingFrame*
frame) {
How does this function compare to ReinitializeGLState?

https://codereview.chromium.org/250083002/diff/40001/cc/output/gl_renderer.h
File cc/output/gl_renderer.h (right):

https://codereview.chromium.org/250083002/diff/40001/cc/output/gl_renderer.h#...
cc/output/gl_renderer.h:48: class UseGrContext;
Does this need to be a nested class? Can you just hide it in the source file?

[danakj, 2014-04-23 19:56:05]

https://codereview.chromium.org/250083002/diff/40001/cc/output/gl_renderer.cc
File cc/output/gl_renderer.cc (right):

https://codereview.chromium.org/250083002/diff/40001/cc/output/gl_renderer.cc...
cc/output/gl_renderer.cc:3140: void GLRenderer::RestoreGLState(DrawingFrame*
frame) {
On 2014/04/23 19:53:41, enne wrote:
> How does this function compare to ReinitializeGLState?

It's similar but

1. It doesn't change any state on the GLRenderer class itself, except the
program.

(Whereas the other resets variables to default states and forces things off/on
which is not valid in the middle of drawing a set of quads.)

2. It does UseRenderPass() to re-setup the viewport.

https://codereview.chromium.org/250083002/diff/40001/cc/output/gl_renderer.h
File cc/output/gl_renderer.h (right):

https://codereview.chromium.org/250083002/diff/40001/cc/output/gl_renderer.h#...
cc/output/gl_renderer.h:48: class UseGrContext;
On 2014/04/23 19:53:41, enne wrote:
> Does this need to be a nested class? Can you just hide it in the source file?

It accesses the protected output_surface_ member and RestoreGLState() methods. I
could add an accessor and make Restore..() public instead?

[Stephen White, 2014-04-23 19:57:19]

Muchas gracias for taking this on!

https://codereview.chromium.org/250083002/diff/40001/cc/output/gl_renderer.cc
File cc/output/gl_renderer.cc (right):

https://codereview.chromium.org/250083002/diff/40001/cc/output/gl_renderer.cc...
cc/output/gl_renderer.cc:180: class GLRenderer::UseGrContext {
I find this guard class to be too much magic. I like guard classes which do
cleanup on destruction, but I'm not fond of meaty code that's done in a guard's
destructor. Are there actual early-outs that are made cleaner/less verbose by
the use of this guard, or could we simply make the explicit calls when
necessary?

https://codereview.chromium.org/250083002/diff/40001/cc/output/gl_renderer.cc...
cc/output/gl_renderer.cc:3149: if (stencil_shadow_)
I'm a little nervous that this state-setting code is only used by filters, and
not by GPU rasterization, for example, which might leave it fragile.

Is there a way we could refactor this with ReinitializeGLState()? I.e.,
ReinitializeGLState() could set the member vars (stencil_shadow_, blend_shadow_,
is_scissor_enabled_, etc), and then just call this function to actually set GL
state? Then it would get exercised more broadly.

[enne (OOO), 2014-04-23 20:01:09]

https://codereview.chromium.org/250083002/diff/40001/cc/output/gl_renderer.cc
File cc/output/gl_renderer.cc (right):

https://codereview.chromium.org/250083002/diff/40001/cc/output/gl_renderer.cc...
cc/output/gl_renderer.cc:3140: void GLRenderer::RestoreGLState(DrawingFrame*
frame) {
On 2014/04/23 19:56:05, danakj wrote:
> On 2014/04/23 19:53:41, enne wrote:
> > How does this function compare to ReinitializeGLState?
> 
> It's similar but
> 
> 1. It doesn't change any state on the GLRenderer class itself, except the
> program.

I think you should combine these if possible.  I think you could make
ReinitializeGLState do what you do here, to set stencil and blend to the
shadowed state rather than clobbering.  I also think this doesn't do the right
thing wrt scissor.  You should have to set scissor_rect_needs_reset_ to true and
do something with is_scissor_enabled_ as well, right?

> (Whereas the other resets variables to default states and forces things off/on
> which is not valid in the middle of drawing a set of quads.)
> 
> 2. It does UseRenderPass() to re-setup the viewport.

That is different, but maybe that can be moved into the UseGrContext class?

[danakj, 2014-04-23 20:11:10]

https://codereview.chromium.org/250083002/diff/40001/cc/output/gl_renderer.cc
File cc/output/gl_renderer.cc (right):

https://codereview.chromium.org/250083002/diff/40001/cc/output/gl_renderer.cc...
cc/output/gl_renderer.cc:180: class GLRenderer::UseGrContext {
 2014/04/23 19:57:20, Stephen White wrote:
> I find this guard class to be too much magic. I like guard classes which do
> cleanup on destruction, but I'm not fond of meaty code that's done in a
guard's
> destructor. Are there actual early-outs that are made cleaner/less verbose by
> the use of this guard, or could we simply make the explicit calls when
> necessary?

This ensures that we clean up before returning to the GLRenderer. There's no
reason we can't add explicit calls and have that work. But then when someone
adds an early out some day in the future, we'll have to fix bugs.

This reminds me a lot of the skia manual-refcounting vs smart pointer
discussions. As with there, I sit on the side of do-it-with-scoped-objects when
you want to do something that is scoped in nature.

https://codereview.chromium.org/250083002/diff/40001/cc/output/gl_renderer.cc...
cc/output/gl_renderer.cc:3140: void GLRenderer::RestoreGLState(DrawingFrame*
frame) {
On 2014/04/23 20:01:10, enne wrote:
> On 2014/04/23 19:56:05, danakj wrote:
> > On 2014/04/23 19:53:41, enne wrote:
> > > How does this function compare to ReinitializeGLState?
> > 
> > It's similar but
> > 
> > 1. It doesn't change any state on the GLRenderer class itself, except the
> > program.
> 
> I think you should combine these if possible.  I think you could make
> ReinitializeGLState do what you do here, to set stencil and blend to the
> shadowed state rather than clobbering.  I also think this doesn't do the right

I can give merging them a try again.

> thing wrt scissor.  You should have to set scissor_rect_needs_reset_ to true
and
> do something with is_scissor_enabled_ as well, right?

I don't think so. The scissor_rect_needs_reset_ = true causes gl_->Scissor() to
be called. But we're calling it here. I specifically want to not change any
member variables on GLRenderer, so it can come back in the same state it was in.

Because I've set the Scissor, I could change scissor_rect_needs_reset_ to false
here, but it will always be false during the drawing of the frame if the scissor
was ever enabled.

> 
> > (Whereas the other resets variables to default states and forces things
off/on
> > which is not valid in the middle of drawing a set of quads.)
> > 
> > 2. It does UseRenderPass() to re-setup the viewport.
> 
> That is different, but maybe that can be moved into the UseGrContext class?

I'll try making it a separate method on the GLRenderer.

[danakj, 2014-04-23 20:21:07]

I've renamed to ScopedUseGrContext, split the UseRenderPass call out to
GLRenderer::RestoreFramebuffer(DrawingFrame*) and made the ReinitializeGLState()
call RestoreGLState().

[Stephen White, 2014-04-23 20:30:39]

On 2014/04/23 20:11:10, danakj wrote:
> https://codereview.chromium.org/250083002/diff/40001/cc/output/gl_renderer.cc
> File cc/output/gl_renderer.cc (right):
> 
>
https://codereview.chromium.org/250083002/diff/40001/cc/output/gl_renderer.cc...
> cc/output/gl_renderer.cc:180: class GLRenderer::UseGrContext {
>  2014/04/23 19:57:20, Stephen White wrote:
> > I find this guard class to be too much magic. I like guard classes which do
> > cleanup on destruction, but I'm not fond of meaty code that's done in a
> guard's
> > destructor. Are there actual early-outs that are made cleaner/less verbose
by
> > the use of this guard, or could we simply make the explicit calls when
> > necessary?
> 
> This ensures that we clean up before returning to the GLRenderer. There's no
> reason we can't add explicit calls and have that work. But then when someone
> adds an early out some day in the future, we'll have to fix bugs.
> 
> This reminds me a lot of the skia manual-refcounting vs smart pointer
> discussions. As with there, I sit on the side of do-it-with-scoped-objects
when
> you want to do something that is scoped in nature.

I'm definitely with you on the smart pointer issue. WRT guard classes generally,
I like them when the destructors do obvious cleanup-like things that are self-
contained, and/or they reduce the amount of code required, neither of which is
the case here.

If someone else looks at this code later, is it obvious several dozen lines 
away at the end of the function that the GrContext will be flushed, which in 
turn might cause quite a few GL draw calls to be made, due to Ganesh's call 
batching? That seems non-obvious to me, and quite important on call order. 
Of course, that is all just my opinion.

>
https://codereview.chromium.org/250083002/diff/40001/cc/output/gl_renderer.cc...
> cc/output/gl_renderer.cc:3140: void GLRenderer::RestoreGLState(DrawingFrame*
> frame) {
> On 2014/04/23 20:01:10, enne wrote:
> > On 2014/04/23 19:56:05, danakj wrote:
> > > On 2014/04/23 19:53:41, enne wrote:
> > > > How does this function compare to ReinitializeGLState?
> > > 
> > > It's similar but
> > > 
> > > 1. It doesn't change any state on the GLRenderer class itself, except the
> > > program.
> > 
> > I think you should combine these if possible.  I think you could make
> > ReinitializeGLState do what you do here, to set stencil and blend to the
> > shadowed state rather than clobbering.  I also think this doesn't do the
right
> 
> I can give merging them a try again.
> 
> > thing wrt scissor.  You should have to set scissor_rect_needs_reset_ to true
> and
> > do something with is_scissor_enabled_ as well, right?
> 
> I don't think so. The scissor_rect_needs_reset_ = true causes gl_->Scissor()
to
> be called. But we're calling it here. I specifically want to not change any
> member variables on GLRenderer, so it can come back in the same state it was
in.
> 
> Because I've set the Scissor, I could change scissor_rect_needs_reset_ to
false
> here, but it will always be false during the drawing of the frame if the
scissor
> was ever enabled.
> 
> > 
> > > (Whereas the other resets variables to default states and forces things
> off/on
> > > which is not valid in the middle of drawing a set of quads.)
> > > 
> > > 2. It does UseRenderPass() to re-setup the viewport.
> > 
> > That is different, but maybe that can be moved into the UseGrContext class?
> 
> I'll try making it a separate method on the GLRenderer.

[enne (OOO), 2014-04-23 20:51:32]

Re: scissor.  Here's the call ordering I'm worried about:

SetScissorTestRect(A)
Ganesh changes scissor to B
SetScissorTestRect(A) <- early out in SetScissorTestRect, noop
Incorrect scissor during draw

How do you deal with Ganesh clobbering the glScissor rect?

[enne (OOO), 2014-04-23 20:52:53]

On 2014/04/23 20:51:32, enne wrote:
> Re: scissor.  Here's the call ordering I'm worried about:
> 
> SetScissorTestRect(A)
> Ganesh changes scissor to B
> SetScissorTestRect(A) <- early out in SetScissorTestRect, noop
> Incorrect scissor during draw
> 
> How do you deal with Ganesh clobbering the glScissor rect?

Nevermind, I see what you're doing.

lgtm

[danakj, 2014-04-23 21:14:08]

OK, I've moved the flush() out of the ScopedUseGrContext class. This puts it
just before the accessBitmap(false) calls, instead of just after, as it used to
be.

I'd like to keep everything else scoped, but this seems like the nail biter for
you.

[Stephen White, 2014-04-23 21:57:21]

LGTM. Thanks for the extra effort.

https://codereview.chromium.org/250083002/diff/60001/cc/output/gl_renderer.cc
File cc/output/gl_renderer.cc (right):

https://codereview.chromium.org/250083002/diff/60001/cc/output/gl_renderer.cc...
cc/output/gl_renderer.cc:338: capabilities_.using_offscreen_context3d = false;
Does anyone use this now? WebGL, maybe? (Sorry I'm kind of ignorant, but
curious.)

https://codereview.chromium.org/250083002/diff/80001/cc/output/gl_renderer.cc
File cc/output/gl_renderer.cc (right):

https://codereview.chromium.org/250083002/diff/80001/cc/output/gl_renderer.cc...
cc/output/gl_renderer.cc:786: // Flush the GrContext to ensure everything is run
on the GL context.
Nit: I think the old comment is a bit clearer; either that or maybe something
like "ensure all buffered GL calls are drawn to the backing store before we
access and return it"

[danakj, 2014-04-23 22:00:14]

Thanks!

https://codereview.chromium.org/250083002/diff/60001/cc/output/gl_renderer.cc
File cc/output/gl_renderer.cc (right):

https://codereview.chromium.org/250083002/diff/60001/cc/output/gl_renderer.cc...
cc/output/gl_renderer.cc:338: capabilities_.using_offscreen_context3d = false;
On 2014/04/23 21:57:21, Stephen White wrote:
> Does anyone use this now? WebGL, maybe? (Sorry I'm kind of ignorant, but
> curious.)

Nope, I'm working on a CL to remove this and the whole Offscreen compositor
context thing from existance.

https://codereview.chromium.org/250083002/diff/80001/cc/output/gl_renderer.cc
File cc/output/gl_renderer.cc (right):

https://codereview.chromium.org/250083002/diff/80001/cc/output/gl_renderer.cc...
cc/output/gl_renderer.cc:786: // Flush the GrContext to ensure everything is run
on the GL context.
On 2014/04/23 21:57:21, Stephen White wrote:
> Nit: I think the old comment is a bit clearer; either that or maybe something
> like "ensure all buffered GL calls are drawn to the backing store before we
> access and return it"

k, added to the comment.

[danakj, 2014-04-23 22:01:33]

The CQ bit was checked by danakj@chromium.org

[commit-bot: I haz the power, 2014-04-23 22:03:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/danakj@chromium.org/250083002/100001

[commit-bot: I haz the power, 2014-04-23 22:43:47]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-23 22:43:47]

Try jobs failed on following builders:
  tryserver.chromium on win_chromium_rel

[danakj, 2014-04-23 23:22:07]

The CQ bit was checked by danakj@chromium.org

[commit-bot: I haz the power, 2014-04-23 23:22:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/danakj@chromium.org/250083002/100001

[commit-bot: I haz the power, 2014-04-24 01:17:56]

Change committed as 265810