Closed
Bug 1381095
Opened 7 years ago
Closed 7 years ago
Make disabling GPU process/WebRender fallback work when creating CompositorSession
Categories
(Core :: Graphics: WebRender, enhancement, P3)
Core
Graphics: WebRender
Tracking
()
RESOLVED
FIXED
mozilla56
Tracking | Status | |
---|---|---|
firefox56 | --- | fixed |
People
(Reporter: aosmond, Assigned: aosmond)
References
Details
(Whiteboard: [gfx-noted])
Attachments
(1 file, 2 obsolete files)
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.
Assignee | ||
Comment 1•7 years ago
|
||
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]
Assignee | ||
Comment 2•7 years ago
|
||
Attachment #8886671 -
Flags: review?(dvander)
Updated•7 years ago
|
Updated•7 years ago
|
Assignee | ||
Comment 3•7 years ago
|
||
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.
Assignee | ||
Comment 5•7 years ago
|
||
(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+
Assignee | ||
Comment 7•7 years ago
|
||
Incorporate review feedback.
Attachment #8887490 -
Attachment is obsolete: true
Attachment #8887927 -
Flags: 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
Comment 9•7 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox56:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in
before you can comment on or make changes to this bug.
Description
•