cc: Allow DeferredInitialize to use DelegatingRenderer

cc: Allow DeferredInitialize to use DelegatingRenderer

Currently DeferredInitialize assumes OutputSurface never has
delegated_rendering capability. With this change, remove this
assumption by always first creating SoftwareRenderer for
OutputSurface with deferred_gl_initialization. Then create
either DelegatingRenderer or GLRenderer depending on
delegated_rendering capability in DeferredInitialize.

With support for DelegatingRenderer and DeferredInititalize,
have to ensure that all GL resources are returned to child
before ReleasGL is called, because ContextProvider is going
away after ReleaseGL. Add a DCHECK for this in
ResourceProvider::CleanUpGLIfNeeded.

New path covered by LayerTreeHostTestDeferredInitialize.

BUG=344087
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=272070

[boliu, 2014-05-16 21:51:15]

PTAL, thanks!

[danakj, 2014-05-16 22:08:47]

https://codereview.chromium.org/286953008/diff/1/cc/trees/layer_tree_host_uni...
File cc/trees/layer_tree_host_unittest.cc (right):

https://codereview.chromium.org/286953008/diff/1/cc/trees/layer_tree_host_uni...
cc/trees/layer_tree_host_unittest.cc:2769:
output_surface->set_forced_draw_to_software_device(true);
Why are these set calls needed/correct?

[boliu, 2014-05-16 22:15:10]

https://codereview.chromium.org/286953008/diff/1/cc/trees/layer_tree_host_uni...
File cc/trees/layer_tree_host_unittest.cc (right):

https://codereview.chromium.org/286953008/diff/1/cc/trees/layer_tree_host_uni...
cc/trees/layer_tree_host_unittest.cc:2769:
output_surface->set_forced_draw_to_software_device(true);
On 2014/05/16 22:08:47, danakj wrote:
> Why are these set calls needed/correct?

To avoid failing this DCHECK in GetDrawMode:
https://code.google.com/p/chromium/codesearch#chromium/src/cc/trees/layer_tre...

It is now totally possible for OutputSurface to have a both software device as
well as delegated_rendering capability. Webview will never produce real software
compositor frames, only the Resourceless kind by setting
force_draw_to_software_device, so the DCHECK should still hold technically.
These calls are mimicking this behavior in webview.

I supppose the DCHECK can be removed. But I'd also like the test reflect real
code.

[danakj, 2014-05-16 22:19:29]

https://codereview.chromium.org/286953008/diff/1/cc/trees/layer_tree_host_uni...
File cc/trees/layer_tree_host_unittest.cc (right):

https://codereview.chromium.org/286953008/diff/1/cc/trees/layer_tree_host_uni...
cc/trees/layer_tree_host_unittest.cc:2769:
output_surface->set_forced_draw_to_software_device(true);
On 2014/05/16 22:15:10, boliu wrote:
> On 2014/05/16 22:08:47, danakj wrote:
> > Why are these set calls needed/correct?
> 
> To avoid failing this DCHECK in GetDrawMode:
>
https://code.google.com/p/chromium/codesearch#chromium/src/cc/trees/layer_tre...
> 
> It is now totally possible for OutputSurface to have a both software device as
> well as delegated_rendering capability. Webview will never produce real
software
> compositor frames, only the Resourceless kind by setting
> force_draw_to_software_device, so the DCHECK should still hold technically.
> These calls are mimicking this behavior in webview.
> 
> I supppose the DCHECK can be removed. But I'd also like the test reflect real
> code.

We should update this DCHECK then, to allow this when deferred_gl is present?
(Look forward to being able to swap OutputSurfaces instead of having this mega
modal OutputSurface..)

[boliu, 2014-05-16 22:34:37]

https://codereview.chromium.org/286953008/diff/1/cc/trees/layer_tree_host_uni...
File cc/trees/layer_tree_host_unittest.cc (right):

https://codereview.chromium.org/286953008/diff/1/cc/trees/layer_tree_host_uni...
cc/trees/layer_tree_host_unittest.cc:2769:
output_surface->set_forced_draw_to_software_device(true);
On 2014/05/16 22:19:29, danakj wrote:
> We should update this DCHECK then, to allow this when deferred_gl is present?

Relaxed DCHECK to allow deferred_gl too. It should also check it's not using the
DelegatingRenderer, but I'm not sure what to check that.

Didn't change the test.

> (Look forward to being able to swap OutputSurfaces instead of having this mega
> modal OutputSurface..)

ForcedDrawToSoftwareDevice won't go away, but yes, OS won't need to set
delegated_rendering before DeferredInitialize, and can get rid of all this
"force_software_renderer" cruft.

[danakj, 2014-05-16 22:44:29]

Do you still need those in the test? Or why did you want to keep them? Should we
be testing both with and without them?

https://codereview.chromium.org/286953008/diff/20001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/286953008/diff/20001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl.cc:576:
!output_surface->capabilities().deferred_gl_initialization);
Can you << the two flags after the DCHECK so if it fails we know why since it'll
be lost in the &&?

[boliu, 2014-05-16 23:08:27]

Actually removed all the set_forced_draw_to_software_device things.
deferred_initialize should not forbid normal software draws. And the
resourceless software path is already tested independently.

https://codereview.chromium.org/286953008/diff/20001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/286953008/diff/20001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl.cc:576:
!output_surface->capabilities().deferred_gl_initialization);
On 2014/05/16 22:44:29, danakj wrote:
> Can you << the two flags after the DCHECK so if it fails we know why since
it'll
> be lost in the &&?

Done.

[danakj, 2014-05-20 18:09:51]

https://codereview.chromium.org/286953008/diff/40001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/286953008/diff/40001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl.cc:1822: !force_software_renderer) {
Ok I am confused by this change here. Delegated rendering has nothing to do with
hardware vs software, we ship software frames using delegated rendering too. The
only time we couldn't right now is when we want to use SkPicture backed draw
quads (ie resourceless software mode?)

[boliu, 2014-05-20 18:31:49]

https://codereview.chromium.org/286953008/diff/40001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/286953008/diff/40001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl.cc:1822: !force_software_renderer) {
On 2014/05/20 18:09:51, danakj wrote:
> Ok I am confused by this change here. Delegated rendering has nothing to do
with
> hardware vs software, we ship software frames using delegated rendering too.
The
> only time we couldn't right now is when we want to use SkPicture backed draw
> quads (ie resourceless software mode?)

Hmm, I think you are right.

The resourceless software mode draw creates its own SoftwareRenderer instance on
the stack for that particular draw, and doesn't need whatever renderer is in
LTHI.

So the SoftwareRenderer created here for webview really is just a place holder
and never used for any draws. And DelegatingRenderer can be created without a
context provider. Then in DeferredInitialize, we tear it down and recreate one
with a context, and everything should just work...

I'll test this now. Assuming it works, this CL will be just DCHECK fixes and
tests, which is just fine with me :)

[boliu, 2014-05-20 18:47:52]

Works, reverted changes around CreateAndSetRenderer.

[danakj, 2014-05-20 18:52:31]

LGTM

[boliu, 2014-05-20 20:37:49]

PTAL. Sorry for the back and forth, but had to bring back
force_software_renderer as it's actually needed...

https://codereview.chromium.org/286953008/diff/120001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/286953008/diff/120001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl.cc:2050:
DCHECK(GetRendererCapabilities().using_map_image);
Sorry, realized we do need force_software_renderer, so brought it back.

Found out because this DCHECK failed in the new test. It never failed before
since SoftwareRenderer always has using_map_image, but DelegatingRenderer
depends on the context.

Which exposed the actual problem, we want to create a Renderer there that does
not use a context_provider, but output_surface does have a context_provider, so
DelegatingRenderer is doing the wrong thing.

Yeah, this will all be better after we switch output surface as well...

[danakj, 2014-05-20 20:48:11]

On Tue, May 20, 2014 at 4:37 PM, <boliu@chromium.org> wrote:

> PTAL. Sorry for the back and forth, but had to bring back
> force_software_renderer as it's actually needed...
>
>
> https://codereview.chromium.org/286953008/diff/120001/cc/
> trees/layer_tree_host_impl.cc
> File cc/trees/layer_tree_host_impl.cc (right):
>
> https://codereview.chromium.org/286953008/diff/120001/cc/
> trees/layer_tree_host_impl.cc#newcode2050
> cc/trees/layer_tree_host_impl.cc:2050:
> DCHECK(GetRendererCapabilities().using_map_image);
> Sorry, realized we do need force_software_renderer, so brought it back.
>
> Found out because this DCHECK failed in the new test. It never failed
> before since SoftwareRenderer always has using_map_image, but
> DelegatingRenderer depends on the context.
>
> Which exposed the actual problem, we want to create a Renderer there
> that does not use a context_provider, but output_surface does have a
> context_provider, so DelegatingRenderer is doing the wrong thing.
>

Ugh... it means you're turning off Ubercomp in software mode though. You're
going to incur a fullscreen copy every frame while in software mode.

All the "has context provider?" checks for being in GL mode are lies with a
WebView output surface.

Can we either:
a) Make the WebView OutputSurface return a null context provider until it
switches to GL mode?

or, less preferably,

b) Stop checking for the presence of a context provider on OutputSurface to
decide if we're in GL mode, use something else like
IsUsingContextProvider()? But this is more fragile.


> Yeah, this will all be better after we switch output surface as well...
>
> https://codereview.chromium.org/286953008/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[boliu, 2014-05-20 20:59:57]

On May 20, 2014 1:48 PM, "Dana Jansens" <danakj@chromium.org> wrote:
>
> On Tue, May 20, 2014 at 4:37 PM, <boliu@chromium.org> wrote:
>>
>> PTAL. Sorry for the back and forth, but had to bring back
>> force_software_renderer as it's actually needed...
>>
>>
>>
https://codereview.chromium.org/286953008/diff/120001/cc/trees/layer_tree_hos...
>> File cc/trees/layer_tree_host_impl.cc (right):
>>
>>
https://codereview.chromium.org/286953008/diff/120001/cc/trees/layer_tree_hos...
>> cc/trees/layer_tree_host_impl.cc:2050:
>> DCHECK(GetRendererCapabilities().using_map_image);
>> Sorry, realized we do need force_software_renderer, so brought it back.
>>
>> Found out because this DCHECK failed in the new test. It never failed
>> before since SoftwareRenderer always has using_map_image, but
>> DelegatingRenderer depends on the context.
>>
>> Which exposed the actual problem, we want to create a Renderer there
>> that does not use a context_provider, but output_surface does have a
>> context_provider, so DelegatingRenderer is doing the wrong thing.
>
>
> Ugh... it means you're turning off Ubercomp in software mode though.
You're going to incur a fullscreen copy every frame while in software mode.
>
> All the "has context provider?" checks for being in GL mode are lies with
a WebView output surface.

Only for the time in ReleaseGL, which is doing both clean up of current
context, and also creation of the renderer that should not depend on that
context.

>
> Can we either:
> a) Make the WebView OutputSurface return a null context provider until it
switches to GL mode?

This is already the case before DeferredInitialize is called. The tricky
bit is ReleaseGL. I think we can release the context before creating the
new renderer in ReleaseGL , then it will do the right thing.

>
> or, less preferably,
>
> b) Stop checking for the presence of a context provider on OutputSurface
to decide if we're in GL mode, use something else like
IsUsingContextProvider()? But this is more fragile.
>
>>
>> Yeah, this will all be better after we switch output surface as well...
>>
>> https://codereview.chromium.org/286953008/
>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[danakj, 2014-05-20 21:01:56]

On Tue, May 20, 2014 at 4:59 PM, Bo Liu <boliu@chromium.org> wrote:

>
> On May 20, 2014 1:48 PM, "Dana Jansens" <danakj@chromium.org> wrote:
> >
> > On Tue, May 20, 2014 at 4:37 PM, <boliu@chromium.org> wrote:
> >>
> >> PTAL. Sorry for the back and forth, but had to bring back
> >> force_software_renderer as it's actually needed...
> >>
> >>
> >>
>
https://codereview.chromium.org/286953008/diff/120001/cc/trees/layer_tree_hos...
> >> File cc/trees/layer_tree_host_impl.cc (right):
> >>
> >>
>
https://codereview.chromium.org/286953008/diff/120001/cc/trees/layer_tree_hos...
> >> cc/trees/layer_tree_host_impl.cc:2050:
> >> DCHECK(GetRendererCapabilities().using_map_image);
> >> Sorry, realized we do need force_software_renderer, so brought it back.
> >>
> >> Found out because this DCHECK failed in the new test. It never failed
> >> before since SoftwareRenderer always has using_map_image, but
> >> DelegatingRenderer depends on the context.
> >>
> >> Which exposed the actual problem, we want to create a Renderer there
> >> that does not use a context_provider, but output_surface does have a
> >> context_provider, so DelegatingRenderer is doing the wrong thing.
> >
> >
> > Ugh... it means you're turning off Ubercomp in software mode though.
> You're going to incur a fullscreen copy every frame while in software mode.
> >
> > All the "has context provider?" checks for being in GL mode are lies
> with a WebView output surface.
>
> Only for the time in ReleaseGL, which is doing both clean up of current
> context, and also creation of the renderer that should not depend on that
> context.
>

Ah ok good!


>  >
> > Can we either:
> > a) Make the WebView OutputSurface return a null context provider until
> it switches to GL mode?
>
> This is already the case before DeferredInitialize is called. The tricky
> bit is ReleaseGL. I think we can release the context before creating the
> new renderer in ReleaseGL , then it will do the right thing.
>
Ya ok surely there's some dance we can do in here to grab the context
provider off the OutputSurface if we need to use it, but make the
OutputSurface stop providing it.

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[boliu, 2014-05-20 22:58:34]

On 2014/05/20 21:01:56, danakj wrote:
> > This is already the case before DeferredInitialize is called. The tricky
> > bit is ReleaseGL. I think we can release the context before creating the
> > new renderer in ReleaseGL , then it will do the right thing.
> >
> Ya ok surely there's some dance we can do in here to grab the context
> provider off the OutputSurface if we need to use it, but make the
> OutputSurface stop providing it.

So the dance I added is OutputSurface::ReleaseContextProvider, called by LTHI
(and test client) to reset it the context in the middle of ReleaseGL. This
allows skip_gl_renderer to be entirely removed as CreateAndSetRenderer, as it
will always do the right thing.

Had to fix a few ReleaseGL

[danakj, 2014-05-20 23:09:11]

Is there a test that verifies LTHI is calling ReleaseContextProvider() inside of
ReleaseGL()?

https://codereview.chromium.org/286953008/diff/140001/cc/output/output_surface.h
File cc/output/output_surface.h (right):

https://codereview.chromium.org/286953008/diff/140001/cc/output/output_surfac...
cc/output/output_surface.h:109: // in order to release the ContextProvider.
Mention only valid with deferred_gl?

https://codereview.chromium.org/286953008/diff/140001/cc/test/fake_output_sur...
File cc/test/fake_output_surface_client.cc (right):

https://codereview.chromium.org/286953008/diff/140001/cc/test/fake_output_sur...
cc/test/fake_output_surface_client.cc:16:
output_surface_->ReleaseContextProvider();
Can you mention in a comment on the ReleaseGL method that it must call
ReleaseContextProvider?

[boliu, 2014-05-20 23:31:38]

https://codereview.chromium.org/286953008/diff/140001/cc/output/output_surfac...
File cc/output/output_surface.cc (right):

https://codereview.chromium.org/286953008/diff/140001/cc/output/output_surfac...
cc/output/output_surface.cc:166: DCHECK(!context_provider_);
This DCHECK and two lines above is verifying that ReleaseContextProvider is
called.

https://codereview.chromium.org/286953008/diff/140001/cc/output/output_surface.h
File cc/output/output_surface.h (right):

https://codereview.chromium.org/286953008/diff/140001/cc/output/output_surfac...
cc/output/output_surface.h:109: // in order to release the ContextProvider.
On 2014/05/20 23:09:11, danakj wrote:
> Mention only valid with deferred_gl?

Done.

https://codereview.chromium.org/286953008/diff/140001/cc/test/fake_output_sur...
File cc/test/fake_output_surface_client.cc (right):

https://codereview.chromium.org/286953008/diff/140001/cc/test/fake_output_sur...
cc/test/fake_output_surface_client.cc:16:
output_surface_->ReleaseContextProvider();
On 2014/05/20 23:09:11, danakj wrote:
> Can you mention in a comment on the ReleaseGL method that it must call
> ReleaseContextProvider?

Done. Put the comment in the client definition.

[danakj, 2014-05-21 15:34:16]

LGTM

https://codereview.chromium.org/286953008/diff/140001/cc/output/output_surfac...
File cc/output/output_surface.cc (right):

https://codereview.chromium.org/286953008/diff/140001/cc/output/output_surfac...
cc/output/output_surface.cc:166: DCHECK(!context_provider_);
On 2014/05/20 23:31:38, boliu wrote:
> This DCHECK and two lines above is verifying that ReleaseContextProvider is
> called.

Ah, yeh okay thanks.

[boliu, 2014-05-21 15:37:54]

The CQ bit was checked by boliu@chromium.org

[boliu, 2014-05-21 17:06:19]

The CQ bit was unchecked by boliu@chromium.org

[boliu, 2014-05-21 17:12:31]

The CQ bit was checked by boliu@chromium.org

[boliu, 2014-05-21 17:38:49]

https://codereview.chromium.org/286953008/diff/180001/cc/test/fake_output_sur...
File cc/test/fake_output_surface_client.h (right):

https://codereview.chromium.org/286953008/diff/180001/cc/test/fake_output_sur...
cc/test/fake_output_surface_client.h:17: FakeOutputSurfaceClient()
According to SO
(http://stackoverflow.com/questions/308276/c-call-constructor-from-constructor)
calling another constructor from initializer isn't allowed until C++11. So have
to copy the initializer list again here instead.

Not sure what I have locally that did make this compile..

[boliu, 2014-05-21 18:33:30]

The CQ bit was checked by boliu@chromium.org

[commit-bot: I haz the power, 2014-05-21 19:58:39]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/boliu@chromium.org/286953008/180001

[commit-bot: I haz the power, 2014-05-22 03:22:21]

Change committed as 272070