Use DirectComposition for compositing when possible

RESOLVED FIXED in Firefox 61

Status

()

defect
P3
normal
RESOLVED FIXED
4 years ago
3 months ago

People

(Reporter: jrmuizel, Assigned: sotaro)

Tracking

(Blocks 3 bugs)

unspecified
mozilla61
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox61 fixed)

Details

(Whiteboard: [wr-reserve] [gfx-noted], URL)

Attachments

(2 attachments, 16 obsolete attachments)

17.11 KB, patch
bas.schouten
: review+
Details | Diff | Splinter Review
23.94 KB, patch
bas.schouten
: review+
Details | Diff | Splinter Review
This is similar to bug 1191965. It's less clear whether it's possible to do this with DirectComposition.

There's no obvious way to take an existing surface and turn it into something that DirectComposition can use. However, it does seem to possible to do that with a SwapChain using IDCompositionDevice::CreateSurfaceFromHandle.

There's also MF_MEDIA_ENGINE_PLAYBACK_VISUAL which seems used for getting DXVA stuff into a DirectComposition visual
Whiteboard: [gfx-noted]
I take this bug as to enable DXGI_SWAP_EFFECT_FLIP_SEQUENTIAL usage of Bug 1419293. The flag usage make composition performance better.
Assignee: nobody → sotaro.ikeda.g
See Also: → 1419293
Component: Graphics: Layers → Graphics: WebRender
Status: NEW → ASSIGNED
Priority: P3 → P1
Whiteboard: [gfx-noted] → [wr-reserve] [gfx-noted]
Direct Composition could have 3 pars.
[1] Enable DirectComposition on Windows if possible.
[2] Separate Chrome and content compositions to different CompositionSurface
[3] Render video as overlays if possible.

This bug only handle [1]. It could improve performance and power consumption.
Posted patch wip (obsolete) — Splinter Review
Attachment #8939771 - Attachment is patch: true
Attachment #8939771 - Attachment mime type: text/x-patch → text/plain
Posted patch wip (obsolete) — Splinter Review
Attachment #8939771 - Attachment is obsolete: true
Posted patch wip: simpler approach (obsolete) — Splinter Review
Attachment #8943138 - Attachment is obsolete: true
Attachment #8945716 - Attachment is obsolete: true
Attachment #8943114 - Attachment is obsolete: true
Attachment #8952301 - Attachment description: patch - Create child window in gpu process → patch part 1 - Create child window in gpu process
Attachment #8952312 - Attachment description: patch - Add capatiblity to enable DComp → patch part 2 - Add capatiblity to enable DComp
attachment 8952301 [details] [diff] [review] add capability to create child window in GPU process. It is necessary for DirectComposition. IDCompositionDevice::CreateTargetForHwnd() requests that window must belong to the calling process.

https://msdn.microsoft.com/en-us/library/windows/desktop/hh437396(v=vs.85).aspx
attachment 8952312 [details] [diff] [review] adds capability to enable DirectComposition if gfx.webrender.dcomp-win.enabled pref is true and platform supports it.

attachment 8952301 [details] [diff] [review] just enable DirectComposition. But with it, we could get performance improvement by using DXGI_SWAP_EFFECT_FLIP_SEQUENTIAL. I tried to enable DXGI_SWAP_EFFECT_FLIP_SEQUENTIAL without using DirectComposition, but it caused a problem like Bug 1435995 and disabled now.
Comment on attachment 8952301 [details] [diff] [review]
patch part 1 - Create child window in gpu process

:jrmuizel, can you feedback to the patches?
Attachment #8952301 - Flags: feedback?(jmuizelaar)
Attachment #8952312 - Flags: feedback?(jmuizelaar)
Sotaro, do you know why weren't creating a child window in the gpu process before? i.e. even when not using DirectComposition wouldn't it be valuable to create a child window in that process?
(In reply to Jeff Muizelaar [:jrmuizel] from comment #15)
> Sotaro, do you know why weren't creating a child window in the gpu process
> before? i.e. even when not using DirectComposition wouldn't it be valuable
> to create a child window in that process?

I do not know the reason. But gpu process use HWND just to create swap chain and to get window size when d3d is used for composite. In these cases, we do not need to create child window in gpu process. I do not know if there could be another reason to create the child window other than for DirectComposition. 

Chromium also seems to create child window only for DirectComposition in gpu process.

https://cs.chromium.org/chromium/src/gpu/ipc/service/direct_composition_surface_win.h?l=100
https://cs.chromium.org/chromium/src/gpu/ipc/service/child_window_win.cc?l=156
Comment on attachment 8952312 [details] [diff] [review]
patch part 2 - Add capatiblity to enable DComp

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

I haven't looked to closely but this seems fine.
Attachment #8952312 - Flags: feedback?(jmuizelaar) → feedback+
Comment on attachment 8952301 [details] [diff] [review]
patch part 1 - Create child window in gpu process

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

I've looked even less closely at this, but the basic idea seems sound.
Attachment #8952301 - Flags: feedback?(jmuizelaar) → feedback+
(In reply to Jeff Muizelaar [:jrmuizel] from comment #15)
> even when not using DirectComposition wouldn't it be valuable to create a child window in that process?

I noticed that it seems valuable to use child window on gpu process to prevent jiggle during resizing. The some jiggling still exists during resizing with WebRender. When child window was used, the jiggling becomes really smaller.
The patch has a problem of handling multiple windows.
Attachment #8952301 - Attachment is obsolete: true
Fixed multiple windows bug.
Attachment #8962269 - Attachment is obsolete: true
Clean up.
Attachment #8962582 - Attachment is obsolete: true
Some clean ups.
Attachment #8962614 - Attachment is obsolete: true
Fixed CompositorBridgeParent::DeallocPWebRenderBridgeParent().
Attachment #8962617 - Attachment is obsolete: true
Rebased.
Attachment #8952312 - Attachment is obsolete: true
Update pref.
Attachment #8962633 - Attachment is obsolete: true
Fixed build failure on linux.
Attachment #8962621 - Attachment is obsolete: true
Rebased.
Attachment #8962642 - Attachment is obsolete: true
Fixed debug build.
Attachment #8962649 - Attachment is obsolete: true
Attachment #8962645 - Flags: review?(jmuizelaar)
Attachment #8962705 - Flags: review?(jmuizelaar)
Comment on attachment 8962705 [details] [diff] [review]
patch part 2 - Add capatiblity to enable DComp

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

Bas is probably a better reviewer for these than me.
Attachment #8962705 - Flags: review?(jmuizelaar) → review?(bas)
Comment on attachment 8962645 [details] [diff] [review]
patch part 1 - Create child window in gpu process

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

Bumping to Bas
Attachment #8962645 - Flags: review?(jmuizelaar) → review?(bas)
Comment on attachment 8962645 [details] [diff] [review]
patch part 1 - Create child window in gpu process

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

::: widget/windows/WinCompositorWindowThread.cpp
@@ +62,5 @@
> +    RefPtr<WinCompositorWindowThread>(sWinCompositorWindowThread.get()),
> +    &WinCompositorWindowThread::ShutDownTask,
> +    &task);
> +  sWinCompositorWindowThread->Loop()->PostTask(runnable.forget());
> +  task.Wait();

It's not clear to me how this actually shuts the compositor thread down? As far as I can tell all this function does is ensure all messages currently queued have been processed.
Attachment #8962645 - Flags: review?(bas) → review+
Comment on attachment 8962705 [details] [diff] [review]
patch part 2 - Add capatiblity to enable DComp

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

::: gfx/layers/ipc/CompositorBridgeParent.cpp
@@ +1752,5 @@
>    MOZ_ASSERT(!mCompositorScheduler);
>    MOZ_ASSERT(mWidget);
>  
>  #ifdef XP_WIN
> +  if (DeviceManagerDx::Get()->CanUseDComp() && mWidget) {

How does this now ensure we only do this in the GPU process?

::: gfx/webrender_bindings/RenderCompositor.h
@@ +7,5 @@
>  #ifndef MOZILLA_GFX_RENDERCOMPOSITOR_H
>  #define MOZILLA_GFX_RENDERCOMPOSITOR_H
>  
>  #include "mozilla/RefPtr.h"
> +#include "mozilla/UniquePtr.h"

I assume this was just a fix for unified builds somehow?
Attachment #8962705 - Flags: review?(bas) → review+
(In reply to Bas Schouten (:bas.schouten) from comment #34)
> 
> It's not clear to me how this actually shuts the compositor thread down? As
> far as I can tell all this function does is ensure all messages currently
> queued have been processed.

Compositor thread was shut down by CompositorThreadHolder::Shutdown(). It is called before RenderThread::ShutDown().
(In reply to Bas Schouten (:bas.schouten) from comment #35)
> Comment on attachment 8962705 [details] [diff] [review]
> patch part 2 - Add capatiblity to enable DComp
> 
> Review of attachment 8962705 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/layers/ipc/CompositorBridgeParent.cpp
> @@ +1752,5 @@
> >    MOZ_ASSERT(!mCompositorScheduler);
> >    MOZ_ASSERT(mWidget);
> >  
> >  #ifdef XP_WIN
> > +  if (DeviceManagerDx::Get()->CanUseDComp() && mWidget) {
> 
> How does this now ensure we only do this in the GPU process?

DCompositionDevice is created only in "GPU process + WebRender enabled" for now.

> 
> ::: gfx/webrender_bindings/RenderCompositor.h
> @@ +7,5 @@
> >  #ifndef MOZILLA_GFX_RENDERCOMPOSITOR_H
> >  #define MOZILLA_GFX_RENDERCOMPOSITOR_H
> >  
> >  #include "mozilla/RefPtr.h"
> > +#include "mozilla/UniquePtr.h"
> 
> I assume this was just a fix for unified builds somehow?

Yes.

Comment 38

a year ago
Pushed by sikeda@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/19142efa84a5
part 1 - Create child window in gpu process r=bas
https://hg.mozilla.org/integration/mozilla-inbound/rev/d328d8eda8e6
part 2 - Add capatiblity to enable DComp r=bas
(In reply to Sotaro Ikeda [:sotaro] from comment #36)
> (In reply to Bas Schouten (:bas.schouten) from comment #34)
> > 
> > It's not clear to me how this actually shuts the compositor thread down? As
> > far as I can tell all this function does is ensure all messages currently
> > queued have been processed.
> 
> Compositor thread was shut down by CompositorThreadHolder::Shutdown(). It is
> called before RenderThread::ShutDown().

Oh, I misunderstood your question. WinCompositorWindowThread was shut down by clearing sWinCompositorWindowThread. It mimics how RenderThread is shut down.

> static StaticRefPtr<WinCompositorWindowThread> sWinCompositorWindowThread;
Depends on: 1449934

Comment 40

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/19142efa84a5
https://hg.mozilla.org/mozilla-central/rev/d328d8eda8e6
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Depends on: 1450086
Depends on: 1451229
Depends on: 1466454

Updated

3 months ago
Depends on: 1524591
You need to log in before you can comment on or make changes to this bug.