Aura: Don't create GL context for CreateSharedSurfaceHandle()

Remove WGC3DCBImpl::GetGPUProcessID()

It's used for blocking renders that are trying to create
 a context after the GPU process crashed through comparing
 gpu process IDs.

Remove this code as we can handle SwapCompositorFrame
 more gracefully nowadays after resources were lost.
 But still fail renderer view context creation when
 gpu browser compositing gets turned off.

BUG=270179, 329737
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=241425

R=danakj@chromium.org, palmer@chromium.org, piman@chromium.org

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

[no sievers, 2013-12-10 21:17:20]

dana, piman: Is this something that would generally work?

I assume with ubercomp we handle things a bit differently and blocking the
renderer until after RWHV::OnLostResources() is not so important?

CreateSharedSurfaceHandle() is what currently still causes us to end up blocking
on the GPU thread to come up even if OutputSurface creation was delayed.

https://codereview.chromium.org/111063003/diff/40001/content/browser/renderer...
File content/browser/renderer_host/gpu_message_filter.cc (left):

https://codereview.chromium.org/111063003/diff/40001/content/browser/renderer...
content/browser/renderer_host/gpu_message_filter.cc:104: if ((*it)->surface_id
!= surface_id) {
Would it be ok to remove this since we notify all observers at the same time
anyway?

[danakj, 2013-12-10 21:19:59]

On Tue, Dec 10, 2013 at 1:17 PM, <sievers@chromium.org> wrote:

> Reviewers: danakj, piman,
>
> Message:
> dana, piman: Is this something that would generally work?
>
> I assume with ubercomp we handle things a bit differently and blocking the
> renderer until after RWHV::OnLostResources() is not so important?
>

FWIW the renderer still uses a context for uploading textures, it just
doesn't draw them into another texture for transport.

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

[no sievers, 2013-12-10 22:09:04]

On 2013/12/10 21:19:59, danakj wrote:
> On Tue, Dec 10, 2013 at 1:17 PM, <mailto:sievers@chromium.org> wrote:
> 
> > Reviewers: danakj, piman,
> >
> > Message:
> > dana, piman: Is this something that would generally work?
> >
> > I assume with ubercomp we handle things a bit differently and blocking the
> > renderer until after RWHV::OnLostResources() is not so important?
> >
> 
> FWIW the renderer still uses a context for uploading textures, it just
> doesn't draw them into another texture for transport.
> 

It's probably fine though if we don't block the renderer in that case. 

Maybe we can avoid blocking the renderer in general to make things simpler. I
think piman's patch (r125727) was also before TextureImageTransportSurface used
mailboxes, and things might have been a bit more sensitive.

I don't think it's possible for the renderer and browser to end up with a
different GPU process unless it's intermittently while they are crashing (or
crashing repeatedly), right?


I thought I had a reason last night to also block the renderer at startup and
compare the gpu host id, but maybe it's not necessary.

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

[piman, 2013-12-11 01:14:03]

https://codereview.chromium.org/111063003/diff/40001/content/browser/aura/gpu...
File content/browser/aura/gpu_process_transport_factory.cc (right):

https://codereview.chromium.org/111063003/diff/40001/content/browser/aura/gpu...
content/browser/aura/gpu_process_transport_factory.cc:296: if
(!GpuDataManagerImpl::GetInstance()->CanUseGpuBrowserCompositor())
In the rest of the patch, we're essentially assuming the GLSurfaceHandle doesn't
change anymore. However, CanUseGpuBrowserCompositor can start succeeding, and
then failing after a few GPU crashes, at which point we'll blacklist the GPU (on
desktop).
When that happens, this will return a different GLSurfaceHandle, that would
cause CreateViewCommandBuffer from the renderer to fail.

That being said, maybe the conditions that lead to this are such that:
1- we only transition from accelerated to non-accelerated
2- when we do, the renderer will also always stop trying to use GPU acceleration

If that's the case, then the patch can simplify code further, and we can pretty
much remove all the CreateViewCommandBuffer delaying logic - see other comments
in the CL

If that is not the case, then we need a chance for the UI to cause a
CreateViewCommandBuffer failure to the renderer, because we can't support the
renderer using GL and the UI to use software. That needs some kind of
throttling, and the current code wouldn't do that correctly I think. However,
we're down to a single bit of information that'd be valid for all renderers, so
maybe this can be handled in a simpler way.

https://codereview.chromium.org/111063003/diff/40001/content/browser/aura/gpu...
content/browser/aura/gpu_process_transport_factory.cc:515: void
GpuProcessTransportFactory::OnGpuChannelReady() {
nit: this could be static, so you wouldn't need weakptrs above.

https://codereview.chromium.org/111063003/diff/40001/content/browser/renderer...
File content/browser/renderer_host/gpu_message_filter.cc (left):

https://codereview.chromium.org/111063003/diff/40001/content/browser/renderer...
content/browser/renderer_host/gpu_message_filter.cc:104: if ((*it)->surface_id
!= surface_id) {
On 2013/12/10 21:17:20, sievers wrote:
> Would it be ok to remove this since we notify all observers at the same time
> anyway?

So, as the code was before, the GLSurfaceHandle was updated for each surface id
separately, each operation under a lock, on the UI thread. Similarly,
OnCreateViewCommandBuffer grabs it separately from the surface id, under the
lock, on the IO thread.
Because we don't atomically update all the surfaces, this was needed to make
sure we only look at surfaces that are known to be updated already.

In this version of the code, we don't update the GLSurfaceHandle for the
surface_id anymore. If we can keep this property, then we indeed don't need this
logic any more.

https://codereview.chromium.org/111063003/diff/40001/content/browser/renderer...
File content/browser/renderer_host/gpu_message_filter.cc (right):

https://codereview.chromium.org/111063003/diff/40001/content/browser/renderer...
content/browser/renderer_host/gpu_message_filter.cc:193: if
(BrowserGpuChannelHostFactory::instance()->GpuProcessHostId() !=
This doesn't guarantee the renderer uses the same channel as the UI is currently
using: after a gpu crash, the renderer may:
1- EstablishGpuChannel, which will bump
BrowserGpuChannelHostFactory::instance()->GpuProcessHostId(), and
GpuMessageFilter will use that as gpu_process_id_
2- CreateViewCommandBuffer, at which point this test will be false

all that before the UI recreated its channel.

So it really means the code to wait on the UI to recreate the channel doesn't do
anything, really.

https://codereview.chromium.org/111063003/diff/40001/content/browser/renderer...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://codereview.chromium.org/111063003/diff/40001/content/browser/renderer...
content/browser/renderer_host/render_widget_host_view_aura.cc:3203:
shared_surface_handle_ = factory->CreateSharedSurfaceHandle();
*if* we don't need to change the GLSurfaceHandle, then we can remove these last
few lines.
We likely can simplify further in that we don't need an explicit
"create/destroy", we really only need something like
ImageTransportFactory::GetGpuProcessIdIfEnabled() or something.

[no sievers, 2013-12-11 01:49:23]

https://codereview.chromium.org/111063003/diff/40001/content/browser/aura/gpu...
File content/browser/aura/gpu_process_transport_factory.cc (right):

https://codereview.chromium.org/111063003/diff/40001/content/browser/aura/gpu...
content/browser/aura/gpu_process_transport_factory.cc:296: if
(!GpuDataManagerImpl::GetInstance()->CanUseGpuBrowserCompositor())
On 2013/12/11 01:14:03, piman wrote:
> In the rest of the patch, we're essentially assuming the GLSurfaceHandle
doesn't
> change anymore. However, CanUseGpuBrowserCompositor can start succeeding, and
> then failing after a few GPU crashes, at which point we'll blacklist the GPU
(on
> desktop).
> When that happens, this will return a different GLSurfaceHandle, that would
> cause CreateViewCommandBuffer from the renderer to fail.
> 
> That being said, maybe the conditions that lead to this are such that:
> 1- we only transition from accelerated to non-accelerated
> 2- when we do, the renderer will also always stop trying to use GPU
acceleration
> 
> If that's the case, then the patch can simplify code further, and we can
pretty
> much remove all the CreateViewCommandBuffer delaying logic - see other
comments
> in the CL
> 
> If that is not the case, then we need a chance for the UI to cause a
> CreateViewCommandBuffer failure to the renderer, because we can't support the
> renderer using GL and the UI to use software. That needs some kind of
> throttling, and the current code wouldn't do that correctly I think. However,
> we're down to a single bit of information that'd be valid for all renderers,
so
> maybe this can be handled in a simpler way.

I think on windows with SwiftShader it's possible that
CanUseGpuBrowserCompositor() initially fails and later succeeds :/

https://codereview.chromium.org/111063003/diff/40001/content/browser/renderer...
File content/browser/renderer_host/gpu_message_filter.cc (right):

https://codereview.chromium.org/111063003/diff/40001/content/browser/renderer...
content/browser/renderer_host/gpu_message_filter.cc:193: if
(BrowserGpuChannelHostFactory::instance()->GpuProcessHostId() !=
On 2013/12/11 01:14:03, piman wrote:
> So it really means the code to wait on the UI to recreate the channel doesn't
do
> anything, really.

Doh, right. So it'd have to get it from the image factory or so.

So the main reason for blocking the renderer was for RWHV to delete the
transport texture before new frames are swapped? And this might not be terribly
important for TextureImageTransportSurface with mailboxes and ubercomp anymore?
Or if don't currently, we should maybe handle it better?

And another reason you mentioned is throttling for when the renderer becomes
accelerated but the browser is still in software. The null GLSurfaceHandle()
then causes it in GpuMessageFilter to not delay the task, but return in ' if
(!host || compositing_surface.is_null()) {'. Is that correct?

Are there any other causes I'd have to worry about wrt comparing the gpu process
id for renderer and browser?

[piman, 2013-12-11 02:20:21]

On Tue, Dec 10, 2013 at 5:49 PM, <sievers@chromium.org> wrote:

>
> https://codereview.chromium.org/111063003/diff/40001/
> content/browser/aura/gpu_process_transport_factory.cc
> File content/browser/aura/gpu_process_transport_factory.cc (right):
>
> https://codereview.chromium.org/111063003/diff/40001/
> content/browser/aura/gpu_process_transport_factory.cc#newcode296
> content/browser/aura/gpu_process_transport_factory.cc:296: if
> (!GpuDataManagerImpl::GetInstance()->CanUseGpuBrowserCompositor())
> On 2013/12/11 01:14:03, piman wrote:
>
>> In the rest of the patch, we're essentially assuming the
>>
> GLSurfaceHandle doesn't
>
>> change anymore. However, CanUseGpuBrowserCompositor can start
>>
> succeeding, and
>
>> then failing after a few GPU crashes, at which point we'll blacklist
>>
> the GPU (on
>
>> desktop).
>> When that happens, this will return a different GLSurfaceHandle, that
>>
> would
>
>> cause CreateViewCommandBuffer from the renderer to fail.
>>
>
>  That being said, maybe the conditions that lead to this are such that:
>> 1- we only transition from accelerated to non-accelerated
>> 2- when we do, the renderer will also always stop trying to use GPU
>>
> acceleration
>
>  If that's the case, then the patch can simplify code further, and we
>>
> can pretty
>
>> much remove all the CreateViewCommandBuffer delaying logic - see other
>>
> comments
>
>> in the CL
>>
>
>  If that is not the case, then we need a chance for the UI to cause a
>> CreateViewCommandBuffer failure to the renderer, because we can't
>>
> support the
>
>> renderer using GL and the UI to use software. That needs some kind of
>> throttling, and the current code wouldn't do that correctly I think.
>>
> However,
>
>> we're down to a single bit of information that'd be valid for all
>>
> renderers, so
>
>> maybe this can be handled in a simpler way.
>>
>
> I think on windows with SwiftShader it's possible that
> CanUseGpuBrowserCompositor() initially fails and later succeeds :/
>
>
> https://codereview.chromium.org/111063003/diff/40001/
> content/browser/renderer_host/gpu_message_filter.cc
> File content/browser/renderer_host/gpu_message_filter.cc (right):
>
> https://codereview.chromium.org/111063003/diff/40001/
> content/browser/renderer_host/gpu_message_filter.cc#newcode193
> content/browser/renderer_host/gpu_message_filter.cc:193: if
> (BrowserGpuChannelHostFactory::instance()->GpuProcessHostId() !=
> On 2013/12/11 01:14:03, piman wrote:
>
>> So it really means the code to wait on the UI to recreate the channel
>>
> doesn't do
>
>> anything, really.
>>
>
> Doh, right. So it'd have to get it from the image factory or so.
>
> So the main reason for blocking the renderer was for RWHV to delete the
> transport texture before new frames are swapped?


Originally, it was to make sure the renderer would not recreate its context
too early, because otherwise it would access the wrong textures and fail
MakeCurrent or something and get lost initially (and the renderer would
fall back to software or cause bad things to the UI compositor that would
get lost in return, etc.).


> And this might not be
> terribly important for TextureImageTransportSurface with mailboxes and
> ubercomp anymore? Or if don't currently, we should maybe handle it
> better?
>

I think with mailboxes, we don't "risk" much anymore, as long as:
- both renderer and UI agree on accelerated vs not
- we don't get into a steady state of the recreation of one causes a lost
context on the other (because they're not talking to the same GPU process).
I don't think that could happen anymore.


>
> And another reason you mentioned is throttling for when the renderer
> becomes accelerated but the browser is still in software.


It's actually the opposite I'm worried about: renderer recreates, still
thinking it's accelerated, but browser only discovers it has to go to
software later.


> The null
> GLSurfaceHandle() then causes it in GpuMessageFilter to not delay the
> task, but return in ' if (!host || compositing_surface.is_null()) {'. Is
> that correct?
>

I don't think we can handle switching from SW to HW in Aura currently to be
honest, so I'm not too worried about this case (though it'd be great to
fix, eventually, but I think we can punt for this CL).
I do want to make sure the HW->SW fallback works fine though, because it's
our safety net.

Are there any other causes I'd have to worry about wrt comparing the gpu
> process id for renderer and browser?
>

As long as both eventually converge to the "latest" GPU process (or absence
thereof, for software), I think we'll be ok.


> https://codereview.chromium.org/111063003/
>

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

[no sievers, 2013-12-14 00:03:41]

See patch set 2. Might as well make it really simple then if we don't have to
delay but only force back to sw.

Seems to work ok in ubercomp on Linux aura (with the blacklist logic in
~GpuProcessHost enabled, and forced to single-threaded browser compositing).
Still need to test without ubercomp.

[piman, 2013-12-14 02:04:47]

On 2013/12/14 00:03:41, sievers wrote:
> See patch set 2. Might as well make it really simple then if we don't have to
> delay but only force back to sw.
> 
> Seems to work ok in ubercomp on Linux aura (with the blacklist logic in
> ~GpuProcessHost enabled, and forced to single-threaded browser compositing).
> Still need to test without ubercomp.

LGTM. I gave it a try myself (both with and without delegated renderer), and
tried killing the GPU process repeatedly while renderers are running (e.g.
poster circle). It seemed fine.
So let's go with this, we'll remove the non-ubercomp path soonish anyway.

[danakj, 2013-12-16 22:57:57]

Super cool, so much deletedness. The aura bits LGTM too.

https://codereview.chromium.org/111063003/diff/70001/content/browser/aura/ima...
File content/browser/aura/image_transport_factory.h (right):

https://codereview.chromium.org/111063003/diff/70001/content/browser/aura/ima...
content/browser/aura/image_transport_factory.h:70: // Creates a shared surface
handle.
Update the comment please. (Why do we have useless comments?)

[no sievers, 2013-12-17 00:23:02]

Should I put a comment in RWHVAura::OnLostResources() to say it's deprecated or
that it might not get called?
With this patch we create the main thread context very lazily and it might not
be created yet when the context is lost (if there was no readback mainly).

Not sure what the impact on |idle_frame_subscriber_textures_| is.

I could also change it so that it only ever gets called without ubercomp and
DCHECK() in there that we are not using that.

[no sievers, 2013-12-17 00:41:09]

+palmer for msg removal in content/common/gpu/gpu_messages.h

[danakj, 2013-12-17 19:13:26]

On 2013/12/17 00:23:02, sievers wrote:
> Should I put a comment in RWHVAura::OnLostResources() to say it's deprecated
or
> that it might not get called?
> With this patch we create the main thread context very lazily and it might not
> be created yet when the context is lost (if there was no readback mainly).

Oh, didn't realize this. Hm. Well, RWHVA won't care about compositor context
loss really, it uses output surface id's to deal with that in the new code
paths, so I think that is okay..

> Not sure what the impact on |idle_frame_subscriber_textures_| is.

If there are any textures, then they were created with the context, so we should
get a notification at that point. So I think that should be ok?

> 
> I could also change it so that it only ever gets called without ubercomp and
> DCHECK() in there that we are not using that.

So what only gets called, OnLostResources? That would mess with
idle_frame_subscriber_textures_.

[palmer, 2013-12-17 19:19:27]

IPC security LGTM.

[no sievers, 2013-12-17 19:19:34]

On 2013/12/17 19:13:26, danakj wrote:
> On 2013/12/17 00:23:02, sievers wrote:
> > Should I put a comment in RWHVAura::OnLostResources() to say it's deprecated
> or
> > that it might not get called?
> > With this patch we create the main thread context very lazily and it might
not
> > be created yet when the context is lost (if there was no readback mainly).
> 
> Oh, didn't realize this. Hm. Well, RWHVA won't care about compositor context
> loss really, it uses output surface id's to deal with that in the new code
> paths, so I think that is okay..
> 
> > Not sure what the impact on |idle_frame_subscriber_textures_| is.
> 
> If there are any textures, then they were created with the context, so we
should
> get a notification at that point. So I think that should be ok?
> 
> > 
> > I could also change it so that it only ever gets called without ubercomp and
> > DCHECK() in there that we are not using that.
> 
> So what only gets called, OnLostResources? That would mess with
> idle_frame_subscriber_textures_.

Ok, I'll leave as-is then. Makes sense that OnLostResources() should only affect
resources that were created with the main thread context.

[commit-bot: I haz the power, 2013-12-17 21:14:48]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sievers@chromium.org/111063003/130001

[commit-bot: I haz the power, 2013-12-18 00:16:29]

Change committed as 241425

[no sievers, 2014-01-15 19:07:13]

Committed patchset #7 manually as r244950 (presubmit successful).