Make disabling GPU process/WebRender fallback work when creating CompositorSession

RESOLVED FIXED in Firefox 56

Status

()

enhancement
P3
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: aosmond, Assigned: aosmond)

Tracking

unspecified
mozilla56
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox56 fixed)

Details

(Whiteboard: [gfx-noted])

Attachments

(1 attachment, 2 obsolete attachments)

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.
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).
Assignee: nobody → aosmond
Status: NEW → ASSIGNED
Has STR: --- → yes
Priority: -- → P3
Whiteboard: [gfx-noted]
Depends on: 1350408
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.
(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.
Attachment #8886671 - Attachment is obsolete: true
Attachment #8886671 - Flags: review?(dvander)
Attachment #8887490 - Flags: review?(dvander)
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|.
Attachment #8887490 - Flags: review?(dvander) → review+
Pushed by aosmond@gmail.com:
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
https://hg.mozilla.org/mozilla-central/rev/05e705204d3b
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.