pepper: Use the RenderThread's shared context as the parent context.

pepper: Use the RenderThread's shared context as the parent context.

Remove the PepperParentContextProvider interface entirely. Have the
pepper PlatformContext3DImpl go directly to RenderThreadImpl and get
the offscreen shared context, and create its texture with that context
and make it the parent of the PlatformContext3DImpl context3d.

Remove the SetParentContext() method from the PlatformContext3DImpl and
the plugin delegate, and the fullscreen widget. Instead, when the plugin
delegate or fullscreen widget become active, in ReparentContext() they
have the PlatformContext3DImpl require access to the offscreen shared
context and create the parent texture id with it, if needed.

The fullscreen widget context management needs to be adjusted. Previously
PlatformContext3DImpl would call back into the fullscreen widget which
would create the fullscreen widget's context. But we have removed that
circular trip. Now in RenderWidgetFullscreenPepper::CheckCompositing() if
compositing mode is being used, we create the fullscreen context3d.
We also recreate it if the old context was lost. When leaving compositing
mode, the fullscreen widget's context is destroyed.

The fullscreen widget's context is now made to share resources so that it
can access the texture created by the RenderThread's context.

This fixes the known issue where fullscreen remains black after a context
loss.

R=piman
BUG=181052
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=187109

[piman, 2013-03-08 04:43:16]

https://codereview.chromium.org/12673002/diff/6015/content/renderer/pepper/pe...
File content/renderer/pepper/pepper_platform_context_3d_impl.cc (right):

https://codereview.chromium.org/12673002/diff/6015/content/renderer/pepper/pe...
content/renderer/pepper/pepper_platform_context_3d_impl.cc:127:
!parent_context_provider_->BindToCurrentThread()) {
Is it ok to do those every time? Should this be done by
RenderThreadImpl::OffscreenContextProviderForMainThread then?

https://codereview.chromium.org/12673002/diff/6015/content/renderer/render_wi...
File content/renderer/render_widget_fullscreen_pepper.cc (right):

https://codereview.chromium.org/12673002/diff/6015/content/renderer/render_wi...
content/renderer/render_widget_fullscreen_pepper.cc:688: CreateContext();
We should only create the context if the plugin has a context/texture. You don't
want to pay for a context for software.

I'd think you could do this below after you checked
webwidget_->isAcceleratedCompositingActive, provided that one doesn't check back
for this context (I don't think it needs to any more).

https://codereview.chromium.org/12673002/diff/6015/content/renderer/render_wi...
content/renderer/render_widget_fullscreen_pepper.cc:689: } else if (context_) {
I don't really think you need this clause. The webwidget_ should always be there
except maybe at construction and destruction, but we will destroy the context in
the destructor.

However we do need to destroy the context if we switch from compositing to not.

https://codereview.chromium.org/12673002/diff/6015/webkit/plugins/ppapi/fulls...
File webkit/plugins/ppapi/fullscreen_container.h (right):

https://codereview.chromium.org/12673002/diff/6015/webkit/plugins/ppapi/fulls...
webkit/plugins/ppapi/fullscreen_container.h:38: virtual
PluginDelegate::PlatformContext3D* CreateContext3D() = 0;
I think you can remove this one now, and always go through the delegate to
create the context.

[danakj, 2013-03-08 18:13:34]

PTAL

https://codereview.chromium.org/12673002/diff/6015/content/renderer/pepper/pe...
File content/renderer/pepper/pepper_platform_context_3d_impl.cc (right):

https://codereview.chromium.org/12673002/diff/6015/content/renderer/pepper/pe...
content/renderer/pepper/pepper_platform_context_3d_impl.cc:127:
!parent_context_provider_->BindToCurrentThread()) {
On 2013/03/08 04:43:16, piman wrote:
> Is it ok to do those every time? Should this be done by
> RenderThreadImpl::OffscreenContextProviderForMainThread then?

Ya this is a noop if you already did it once. I like requesting the context
provider to not have side effects.

https://codereview.chromium.org/12673002/diff/6015/content/renderer/render_wi...
File content/renderer/render_widget_fullscreen_pepper.cc (right):

https://codereview.chromium.org/12673002/diff/6015/content/renderer/render_wi...
content/renderer/render_widget_fullscreen_pepper.cc:688: CreateContext();
On 2013/03/08 04:43:16, piman wrote:
> We should only create the context if the plugin has a context/texture. You
don't
> want to pay for a context for software.
> 
> I'd think you could do this below after you checked
> webwidget_->isAcceleratedCompositingActive, provided that one doesn't check
back
> for this context (I don't think it needs to any more).

Done.

https://codereview.chromium.org/12673002/diff/6015/content/renderer/render_wi...
content/renderer/render_widget_fullscreen_pepper.cc:689: } else if (context_) {
On 2013/03/08 04:43:16, piman wrote:
> I don't really think you need this clause. The webwidget_ should always be
there
> except maybe at construction and destruction, but we will destroy the context
in
> the destructor.
> 
> However we do need to destroy the context if we switch from compositing to
not.

Done.

[piman, 2013-03-08 18:27:08]

+jbauman

https://codereview.chromium.org/12673002/diff/6015/content/renderer/pepper/pe...
File content/renderer/pepper/pepper_platform_context_3d_impl.cc (right):

https://codereview.chromium.org/12673002/diff/6015/content/renderer/pepper/pe...
content/renderer/pepper/pepper_platform_context_3d_impl.cc:127:
!parent_context_provider_->BindToCurrentThread()) {
On 2013/03/08 18:13:34, danakj wrote:
> On 2013/03/08 04:43:16, piman wrote:
> > Is it ok to do those every time? Should this be done by
> > RenderThreadImpl::OffscreenContextProviderForMainThread then?
> 
> Ya this is a noop if you already did it once. I like requesting the context
> provider to not have side effects.

It does have side effect in the sense that it creates one. It's a bit of an
arbitrary distinction in that if InitializeOnMainThread was done in the
constructor that would happen too.

At the same time, it's a bit obscure what exact set of calls in what order has
to be done before the ContextProvider is usable, and this code is repeated in
several places... It'd be nice to factor into a single place.

https://codereview.chromium.org/12673002/diff/17001/content/renderer/render_w...
File content/renderer/render_widget_fullscreen_pepper.cc (right):

https://codereview.chromium.org/12673002/diff/17001/content/renderer/render_w...
content/renderer/render_widget_fullscreen_pepper.cc:557: return;
I just thought of something else: what about this part? Left as is, if this flag
is on, we will let the plugin create a context and a texture, but we will never
composite it.
I think it used to be that the ReparentContext would have failed and so Pepper
would fall back to software. Maybe John can comment on how this should work...

https://codereview.chromium.org/12673002/diff/17001/content/renderer/render_w...
content/renderer/render_widget_fullscreen_pepper.cc:690: CreateContext();
If CreateContext fails, we should reset compositing to false, otherwise we'll
try to composite with no context and bad things will happen.

[danakj, 2013-03-08 18:40:28]

https://codereview.chromium.org/12673002/diff/6015/content/renderer/pepper/pe...
File content/renderer/pepper/pepper_platform_context_3d_impl.cc (right):

https://codereview.chromium.org/12673002/diff/6015/content/renderer/pepper/pe...
content/renderer/pepper/pepper_platform_context_3d_impl.cc:127:
!parent_context_provider_->BindToCurrentThread()) {
On 2013/03/08 18:27:08, piman wrote:
> On 2013/03/08 18:13:34, danakj wrote:
> > On 2013/03/08 04:43:16, piman wrote:
> > > Is it ok to do those every time? Should this be done by
> > > RenderThreadImpl::OffscreenContextProviderForMainThread then?
> > 
> > Ya this is a noop if you already did it once. I like requesting the context
> > provider to not have side effects.
> 
> It does have side effect in the sense that it creates one. It's a bit of an
> arbitrary distinction in that if InitializeOnMainThread was done in the
> constructor that would happen too.

It creates a context provider, which is just a wrapper basically, but not a
WGC3D. I wanted the getter for OffscreenContextProviderOnSomeThread to always
return a valid pointer rather than do initialization and maybe return a null. I
guess it could do this anyways, and the callsite could check initialized() or
valid() or something, idk.

> At the same time, it's a bit obscure what exact set of calls in what order has
> to be done before the ContextProvider is usable, and this code is repeated in
> several places... It'd be nice to factor into a single place.

I guess we can build in the assumption that ContextProvider is always created on
the main thread, and then it can call Initialize right away. The call site will
still want to call bind though. For the main thread one, it could avoid it I
guess, but for other threads it would need to be explicit, and I like it being
consistent I think?

Anyhow.. let me open a bug.

https://codereview.chromium.org/12673002/diff/17001/content/renderer/render_w...
File content/renderer/render_widget_fullscreen_pepper.cc (right):

https://codereview.chromium.org/12673002/diff/17001/content/renderer/render_w...
content/renderer/render_widget_fullscreen_pepper.cc:690: CreateContext();
On 2013/03/08 18:27:08, piman wrote:
> If CreateContext fails, we should reset compositing to false, otherwise we'll
> try to composite with no context and bad things will happen.

Done.

[danakj, 2013-03-08 18:58:13]

https://codereview.chromium.org/12673002/diff/17001/content/renderer/render_w...
File content/renderer/render_widget_fullscreen_pepper.cc (right):

https://codereview.chromium.org/12673002/diff/17001/content/renderer/render_w...
content/renderer/render_widget_fullscreen_pepper.cc:557: return;
On 2013/03/08 18:27:08, piman wrote:
> I just thought of something else: what about this part? Left as is, if this
flag
> is on, we will let the plugin create a context and a texture, but we will
never
> composite it.
> I think it used to be that the ReparentContext would have failed and so Pepper
> would fall back to software. Maybe John can comment on how this should work...

Right! You're right, SetParentContext() would early out and not make a texture.
Then isAcceleratedCompositingActive() would be false because it checks for the
texture id.

I think we can just move this check to CheckCompositing(). If it is on, we set
compositing=false, and we don't CreateContext() and we go down the
disable-compositing path.

How's that sound? Patch uploaded with that.

[danakj, 2013-03-08 21:00:33]

This seems to behave the same as before this CL wrt fullscreen and enabling
compositing or not. I tried:

With --ignore-gpu-blacklist I get composited fullscreen.
Without --ignore-gpu-blacklist I don't.
In ash, I always get composited fullscreen.

I tried forcing 3d flash off by hacking in code in gpu_data_manager_impl.cc and
it seems to have the same effect on disabling compositing or not with or without
this CL.

[danakj, 2013-03-08 22:09:31]

Ok this should preserve the same semantics as before with respect to reparenting
into fullscreen now.

When switching to fullscreen, we call ReparentContext() on the fullscreen
widget.

If 3d is disabled via the command line, it destroys the parent texture and
contextprovider.

Then isAcceleratedCompositingActive() will see no parent texture and not enable
compositing.

When we switch out of fullscreen, we will call ReparentContext() on the plugin
delegate. This will create the parent texture if needed.

[piman, 2013-03-08 22:37:52]

https://codereview.chromium.org/12673002/diff/35002/content/renderer/render_w...
File content/renderer/render_widget_fullscreen_pepper.cc (right):

https://codereview.chromium.org/12673002/diff/35002/content/renderer/render_w...
content/renderer/render_widget_fullscreen_pepper.cc:453: return new
PlatformContext3DImpl;
we probably want to return NULL if switches::kDisableFlashFullscreen3d is set.
(BTW you can remove the #ifdef ENABLE_GPU)

[danakj, 2013-03-08 22:40:45]

https://codereview.chromium.org/12673002/diff/35002/content/renderer/render_w...
File content/renderer/render_widget_fullscreen_pepper.cc (right):

https://codereview.chromium.org/12673002/diff/35002/content/renderer/render_w...
content/renderer/render_widget_fullscreen_pepper.cc:453: return new
PlatformContext3DImpl;
On 2013/03/08 22:37:52, piman wrote:
> we probably want to return NULL if switches::kDisableFlashFullscreen3d is set.
> (BTW you can remove the #ifdef ENABLE_GPU)

I am scared of that, the old code would not have returned NULL here. Are you
sure it's safe at the call sites to do that? Should I do an audit?

[piman, 2013-03-08 22:44:08]

https://codereview.chromium.org/12673002/diff/35002/content/renderer/render_w...
File content/renderer/render_widget_fullscreen_pepper.cc (right):

https://codereview.chromium.org/12673002/diff/35002/content/renderer/render_w...
content/renderer/render_widget_fullscreen_pepper.cc:453: return new
PlatformContext3DImpl;
On 2013/03/08 22:40:46, danakj wrote:
> On 2013/03/08 22:37:52, piman wrote:
> > we probably want to return NULL if switches::kDisableFlashFullscreen3d is
set.
> > (BTW you can remove the #ifdef ENABLE_GPU)
> 
> I am scared of that, the old code would not have returned NULL here. Are you
> sure it's safe at the call sites to do that? Should I do an audit?

It should be safe. It's only called from  PPB_Graphics3D_Impl::InitRaw through
PluginInstance::CreateContext3D, which checks, and the delegate version (for
non-fullscreen) can return NULL.

[piman, 2013-03-08 22:45:49]

On 2013/03/08 22:44:08, piman wrote:
>
https://codereview.chromium.org/12673002/diff/35002/content/renderer/render_w...
> File content/renderer/render_widget_fullscreen_pepper.cc (right):
> 
>
https://codereview.chromium.org/12673002/diff/35002/content/renderer/render_w...
> content/renderer/render_widget_fullscreen_pepper.cc:453: return new
> PlatformContext3DImpl;
> On 2013/03/08 22:40:46, danakj wrote:
> > On 2013/03/08 22:37:52, piman wrote:
> > > we probably want to return NULL if switches::kDisableFlashFullscreen3d is
> set.
> > > (BTW you can remove the #ifdef ENABLE_GPU)
> > 
> > I am scared of that, the old code would not have returned NULL here. Are you
> > sure it's safe at the call sites to do that? Should I do an audit?
> 
> It should be safe. It's only called from  PPB_Graphics3D_Impl::InitRaw through
> PluginInstance::CreateContext3D, which checks, and the delegate version (for
> non-fullscreen) can return NULL.

note: the reason I want that is that in the old version,
kDisableFlashFullscreen3d would make the context initialization fail, whereas it
won't now.

[jbauman, 2013-03-08 22:47:05]

On 2013/03/08 22:45:49, piman wrote:
> On 2013/03/08 22:44:08, piman wrote:
> >
>
https://codereview.chromium.org/12673002/diff/35002/content/renderer/render_w...
> > File content/renderer/render_widget_fullscreen_pepper.cc (right):
> > 
> >
>
https://codereview.chromium.org/12673002/diff/35002/content/renderer/render_w...
> > content/renderer/render_widget_fullscreen_pepper.cc:453: return new
> > PlatformContext3DImpl;
> > On 2013/03/08 22:40:46, danakj wrote:
> > > On 2013/03/08 22:37:52, piman wrote:
> > > > we probably want to return NULL if switches::kDisableFlashFullscreen3d
is
> > set.
> > > > (BTW you can remove the #ifdef ENABLE_GPU)
> > > 
> > > I am scared of that, the old code would not have returned NULL here. Are
you
> > > sure it's safe at the call sites to do that? Should I do an audit?
> > 
> > It should be safe. It's only called from  PPB_Graphics3D_Impl::InitRaw
through
> > PluginInstance::CreateContext3D, which checks, and the delegate version (for
> > non-fullscreen) can return NULL.
> 
> note: the reason I want that is that in the old version,
> kDisableFlashFullscreen3d would make the context initialization fail, whereas
it
> won't now.

Yeah, the context initialization ideally will fail completely with
kDisableFlashFullscreen3d (which is intended for use in the swiftshader case
where we never want flash to use 3d).

[danakj, 2013-03-08 22:48:29]

I see, ok done!

[piman, 2013-03-08 22:56:28]

LGTM - crossing fingers. I hope this is going to fix the bug and not make
anything else worse :)

[danakj, 2013-03-08 22:57:14]

On 2013/03/08 22:56:28, piman wrote:
> LGTM - crossing fingers. I hope this is going to fix the bug and not make
> anything else worse :)

Same ^_^;

[commit-bot: I haz the power, 2013-03-08 22:57:50]

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

[commit-bot: I haz the power, 2013-03-09 01:54:13]

Change committed as 187109