1381095 - Make disabling GPU process/WebRender fallback work when creating CompositorSession

Status: RESOLVED FIXED

(Core :: Graphics: WebRender, enhancement, P3)

Description

[Andrew Osmond [:aosmond] (he/him)]

Right now when we disable the GPU process due to failing to create the remote compositor session, we try to fallback to a same-process compositor session. However we also disable WebRender when disabling the GPU process, so our state is mixed. We need to recreate the layer manager, compositor options, etc when falling back like this.

Comment 1

[Andrew Osmond [:aosmond] (he/him)]

STR: Open a few content tabs and rapidly try to terminate the GPU process from about:support. Eventually it will trip the condition (will see "Failed to create remote compositor" in the GFX log).

Comment 2

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Fallback correctly to non-WebRender if the GPU process/WebRender are disabled when creating a remote compositor session., v1

Comment 3

[Andrew Osmond [:aosmond] (he/him)]

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=924fcf2e72f90fab416ffc38eec2807a10fb6f4f

Comment 4

[David Anderson [:dvander] - inactive, e-mail if emergency]

Comment on attachment 8886671 [details] [diff] [review]
Fallback correctly to non-WebRender if the GPU process/WebRender are disabled when creating a remote compositor session., v1

Review of attachment 8886671 [details] [diff] [review]:
-----------------------------------------------------------------

::: widget/nsBaseWidget.cpp
@@ +1275,3 @@
>  
> +  do {
> +    CreateCompositorVsyncDispatcher();

I think it might be time to factor this loop out into a separate function, since CreateCompositor is getting pretty complicated.

Comment 5

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Fallback correctly to non-WebRender if the GPU process/WebRender are disabled when creating a remote compositor session., v2

(In reply to David Anderson [:dvander] from comment #4)
> Comment on attachment 8886671 [details] [diff] [review]
> Fallback correctly to non-WebRender if the GPU process/WebRender are
> disabled when creating a remote compositor session., v1
> 
> Review of attachment 8886671 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: widget/nsBaseWidget.cpp
> @@ +1275,3 @@
> >  
> > +  do {
> > +    CreateCompositorVsyncDispatcher();
> 
> I think it might be time to factor this loop out into a separate function,
> since CreateCompositor is getting pretty complicated.

Splintered off as CreateCompositorSession.

Comment 6

[David Anderson [:dvander] - inactive, e-mail if emergency]

Comment on attachment 8887490 [details] [diff] [review]
Fallback correctly to non-WebRender if the GPU process/WebRender are disabled when creating a remote compositor session., v2

Review of attachment 8887490 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/ipc/GPUProcessManager.h
@@ +95,5 @@
>      CSSToLayoutDeviceScale aScale,
>      const CompositorOptions& aOptions,
>      bool aUseExternalSurfaceSize,
> +    const gfx::IntSize& aSurfaceSize,
> +    CompositorSession** aCompositorSession);

Same here, I'd prefer if CompositorSession was the return value and the bool was an outparam. A function returning false as success is unusual, having to check null and then check the retry flag is more idiomatic.

::: widget/nsBaseWidget.h
@@ +747,5 @@
> +
> +private:
> +  mozilla::layers::CompositorOptions
> +  CreateCompositorSession(int aWidth, int aHeight,
> +                          LayerManager** aLayerManagerOut);

I think it will be slightly cleaner to return a RefPtr<LayerManager> and make the options the outparam, especially since the caller does a null check on |lm|.

Comment 7

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Fallback correctly to non-WebRender if the GPU process/WebRender are disabled when creating a remote compositor session., v3 [carries r=dvander]

Incorporate review feedback.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/05e705204d3b
Fallback correctly to non-WebRender if the GPU process/WebRender are disabled when creating a remote compositor session. r=dvander

Comment 9

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/05e705204d3b