1259571 - Reduce tearing with basic layers on Windows

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Jeff Muizelaar [:jrmuizel]]

This would make things nicer.

Comment 1

[Jeff Muizelaar [:jrmuizel]]

http://www.codeproject.com/Articles/4436/Tearing-free-drawing-with-GDI

Comment 2

[Sotaro Ikeda [:sotaro]]

Tearing happened often when I did scrolling during playback the following video.
  https://www.youtube.com/watch?v=iNJdPyoqt8U

Comment 3

[Sotaro Ikeda [:sotaro]]

Attachment: temporary path - Just to check if the workaround works

Temporary patch just to check if a workaround in Comment 1 work. And confirmed that the tearing was addressed on my pc.

Comment 4

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Try tearing-free drawing with GDI

Comment 5

[Sotaro Ikeda [:sotaro]]

Comment on attachment 8770864 [details] [diff] [review]
patch - Try tearing-free drawing with GDI

:mattwoodrow, can you feedback to the patch?

Comment 6

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8770864 [details] [diff] [review]
patch - Try tearing-free drawing with GDI

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

This seems reasonable, but I have a few questions.

How does this relate to our existing vsync code?

Is the problem that we start the composite with the vsync timer, but compositing is slow, so we miss our interval and end up presenting too late?

Would it be better to make sure the vsync notifications are at the right time, and then *always* do EndRemoteDrawing one frame after the composite?

It seems like currently we might sometimes be able to do EndRemoteDrawing immediately, and other times not (depending on how long the composite is) and then we'll present multiple frames within one vblank and none during others. That will make scrolling look less smooth.

::: widget/windows/WinCompositorWidget.cpp
@@ +131,5 @@
>  
> +bool
> +WinCompositorWidget::NeedsToDeferEndRemoteDrawing()
> +{
> +  IDirect3DDevice9* device = gfxWindowsPlatform::GetPlatform()->GetD3D9Device();

How often do we have a d3d9 device available, but aren't using CompositorD3D9?

Comment 7

[Jeff Muizelaar [:jrmuizel]]

(In reply to Matt Woodrow (:mattwoodrow) from comment #6)
> >  
> > +bool
> > +WinCompositorWidget::NeedsToDeferEndRemoteDrawing()
> > +{
> > +  IDirect3DDevice9* device = gfxWindowsPlatform::GetPlatform()->GetD3D9Device();
> 
> How often do we have a d3d9 device available, but aren't using
> CompositorD3D9?

We could create a D3D9 device that we only use for VSync. This should be pretty safe and would work across a very large set of devices.

This thread suggests that D3DKMTWaitForVerticalBlankEvent might be a better solution:
https://bugs.chromium.org/p/chromium/issues/detail?id=467617

We talked about this in bug 1146942 but dismissed it because of its privateness. I think it's worth reconsidering.

Another option that might work more widely on XP is IDirectDraw7::GetScanLine

Comment 8

[Matt Woodrow (:mattwoodrow)]

(In reply to Jeff Muizelaar [:jrmuizel] from comment #7)
> (In reply to Matt Woodrow (:mattwoodrow) from comment #6)
> > >  
> > > +bool
> > > +WinCompositorWidget::NeedsToDeferEndRemoteDrawing()
> > > +{
> > > +  IDirect3DDevice9* device = gfxWindowsPlatform::GetPlatform()->GetD3D9Device();
> > 
> > How often do we have a d3d9 device available, but aren't using
> > CompositorD3D9?
> 
> We could create a D3D9 device that we only use for VSync. This should be
> pretty safe and would work across a very large set of devices.

We've had crashes in the past that happen just from creating a device. The startup crash detection is managing those though, so we can probably keep using that.

> 
> This thread suggests that D3DKMTWaitForVerticalBlankEvent might be a better
> solution:
> https://bugs.chromium.org/p/chromium/issues/detail?id=467617
> 
> We talked about this in bug 1146942 but dismissed it because of its
> privateness. I think it's worth reconsidering.
> 
> Another option that might work more widely on XP is IDirectDraw7::GetScanLine

These sounds fine to me. Do we still have a problem with compositing being so slow that we miss the interval?

Comment 9

[Sotaro Ikeda [:sotaro]]

(In reply to Matt Woodrow (:mattwoodrow) from comment #6)
> Comment on attachment 8770864 [details] [diff] [review]
> patch - Try tearing-free drawing with GDI
> 
> Review of attachment 8770864 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This seems reasonable, but I have a few questions.
> 
> How does this relate to our existing vsync code?

Current code is not related to vsync code. When BasicCompositor is used, we do not know how long the composition takes. It could be very different depends on the frame. 

> Is the problem that we start the composite with the vsync timer, but
> compositing is slow, so we miss our interval and end up presenting too late?

When the composition is slow, the frame is presented on later timing. The situation seems unavoidable since BasicCompositor's composition is slow.

> Would it be better to make sure the vsync notifications are at the right
> time, and then *always* do EndRemoteDrawing one frame after the composite?

Yes, I think so. In current gecko, CompositorVsyncScheduler::PostCompositeTask() was called during vsync timing on my win10 pc.

> 
> It seems like currently we might sometimes be able to do EndRemoteDrawing
> immediately, and other times not (depending on how long the composite is)
> and then we'll present multiple frames within one vblank and none during
> others. That will make scrolling look less smooth.

Yes, current patch does not handle well the above situation well. I am going to try if there could be a better way than current patch.

> ::: widget/windows/WinCompositorWidget.cpp
> @@ +131,5 @@
> >  
> > +bool
> > +WinCompositorWidget::NeedsToDeferEndRemoteDrawing()
> > +{
> > +  IDirect3DDevice9* device = gfxWindowsPlatform::GetPlatform()->GetD3D9Device();
> 
> How often do we have a d3d9 device available, but aren't using
> CompositorD3D9?

I am going to try the ways in Comment 7.

Comment 10

[Sotaro Ikeda [:sotaro]]

(In reply to Jeff Muizelaar [:jrmuizel] from comment #7)
> 
> This thread suggests that D3DKMTWaitForVerticalBlankEvent might be a better
> solution:
> https://bugs.chromium.org/p/chromium/issues/detail?id=467617
> 
> We talked about this in bug 1146942 but dismissed it because of its
> privateness. I think it's worth reconsidering.
> 
> Another option that might work more widely on XP is IDirectDraw7::GetScanLine

IDirectDraw7::GetScanLine seems a better way as basic implementation, since it is also supported in XP. D3DKMTWaitForVerticalBlankEvent is since Vista.

Comment 11

[Sotaro Ikeda [:sotaro]]

> > 
> > This thread suggests that D3DKMTWaitForVerticalBlankEvent might be a better
> > solution:
> > https://bugs.chromium.org/p/chromium/issues/detail?id=467617
> > 
> > We talked about this in bug 1146942 but dismissed it because of its
> > privateness. I think it's worth reconsidering.
> > 
> > Another option that might work more widely on XP is IDirectDraw7::GetScanLine
> 
> These sounds fine to me. Do we still have a problem with compositing being
> so slow that we miss the interval?

Yes, vsync is normally 60 fps, with BasicCompositor, we miss the interval very often.

Comment 12

[Sotaro Ikeda [:sotaro]]

(In reply to Sotaro Ikeda [:sotaro] from comment #11)
> > These sounds fine to me. Do we still have a problem with compositing being
> > so slow that we miss the interval?
> 
> Yes, vsync is normally 60 fps, with BasicCompositor, we miss the interval
> very often.

Sorry, the above my comment was totally wrong. When I tested, most compositions were within the interval. And main source of tearing was caused by too early composition complete. In this case, we need to wait until ScanLine becomes large enough.

Comment 13

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Try tearing-free drawing with GDI

Comment 14

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 1 - Try tearing-free drawing with GDI

Comment 15

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 2 - Add IDirectDraw7 supportfor getting ScanLine

Comment 16

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 1 - Try tearing-free drawing with GDI

Comment 17

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 2 - Add IDirectDraw7 supportfor getting ScanLine

Comment 18

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 1 - Try tearing-free drawing with GDI

Rebased.

Comment 19

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 2 - Add IDirectDraw7 supportfor getting ScanLine

Comment 20

[Sotaro Ikeda [:sotaro]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=701f42faa0ab

Comment 21

[Matt Woodrow (:mattwoodrow)]

(In reply to Sotaro Ikeda [:sotaro] from comment #9)
 
> > 
> > It seems like currently we might sometimes be able to do EndRemoteDrawing
> > immediately, and other times not (depending on how long the composite is)
> > and then we'll present multiple frames within one vblank and none during
> > others. That will make scrolling look less smooth.
> 
> Yes, current patch does not handle well the above situation well. I am going
> to try if there could be a better way than current patch.

Did you make any progress on this? It would be nice to try minimize jitter.

Comment 22

[Sotaro Ikeda [:sotaro]]

Comment on attachment 8773180 [details] [diff] [review]
patch part 1 - Try tearing-free drawing with GDI

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

::: gfx/layers/ipc/CompositorBridgeParent.cpp
@@ +489,5 @@
> +  MOZ_ASSERT(mCompositorBridgeParent);
> +  if (!mAsapScheduling && mCompositorBridgeParent->IsPendingComposite()) {
> +    mCompositorBridgeParent->FinishPendingComposite();
> +    return;
> +  }

This code prevents to produce multiple frames in one v-blank in majority of cases.

Comment 23

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 1 - Try tearing-free drawing with GDI

Comment 24

[Sotaro Ikeda [:sotaro]]

Attachment: temporay patch - Use D3DKMTWaitForVerticalBlankEvent as flip timing

The patch is for testing if D3DKMTWaitForVerticalBlankEvent as flip timing.

Comment 25

[Sotaro Ikeda [:sotaro]]

When I tested attachment 8774573 [details] [diff] [review] locally, D3DKMTWaitForVerticalBlankEvent was too late as flip timing. It made the flip as scan line was small and caused more tearing.

Comment 26

[Sotaro Ikeda [:sotaro]]

From comment 25, D3DKMTWaitForVerticalBlankEvent seemed not good for the flip timing.

Comment 27

[Sotaro Ikeda [:sotaro]]

(In reply to Sotaro Ikeda [:sotaro] from comment #26)
> From comment 25, D3DKMTWaitForVerticalBlankEvent seemed not good for the
> flip timing.

There were also cases that GetScanLine() returns DDERR_VERTICALBLANKINPROGRESS, but there were also cases that vblank was already ended.

Comment 28

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 2 - Add IDirectDraw7 supportfor getting ScanLine

Comment 29

[Sotaro Ikeda [:sotaro]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=bef7ff318cd1&selectedJob=24701086

Comment 30

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8774201 [details] [diff] [review]
patch part 1 - Try tearing-free drawing with GDI

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

::: widget/windows/WinCompositorWidget.h
@@ +105,5 @@
>  
>    // Locked back buffer of BasicCompositor
>    uint8_t* mLockedBackBufferData;
> +
> +  bool mNotDeferEndRemoteDrawing;

How about mDeferRemoteDrawingDisabled?

Comment 31

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8774634 [details] [diff] [review]
patch part 2 - Add IDirectDraw7 supportfor getting ScanLine

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

Looks good, but I'll get dvander to review the gfxFeature changes, not sure if they handle being modified off the main thread.

Comment 32

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

Comment on attachment 8774634 [details] [diff] [review]
patch part 2 - Add IDirectDraw7 supportfor getting ScanLine

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

This will break the GPU process, since we can't use gfxWindowsPlatform instances from WinCompositorWidget. Can we stick this somewhere else instead?

We could stick it in DeviceManagerD3D11, and rename DeviceManagerD3D11 to DeviceManagerDx. Or we could keep everything in gfxWindowsPlatform, but make variables/methods static instead. Having a separate singleton (DeviceManagerDDraw) would work too.

Comment 33

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

It'd also be better to just create the ddraw device unconditionally on the main thread. If we run into startup time/first paint regressions we can defer it until we know the compositor type, rather than when the compositor needs to paint.

Comment 34

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 2 - Add IDirectDraw7 supportfor getting ScanLine

Comment 35

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 3 - Rename DeviceManagerD3D11 to DeviceManagerDx

Comment 36

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

Comment on attachment 8776851 [details] [diff] [review]
patch part 2 - Add IDirectDraw7 supportfor getting ScanLine

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

Thanks!

Comment 37

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 1 - Try tearing-free drawing with GDI

Rebased.

Comment 38

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 2 - Add IDirectDraw7 supportfor getting ScanLine

Rebased.

Comment 39

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 3 - Rename DeviceManagerD3D11 to DeviceManagerDx

Rebased.

Comment 40

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 3 - Rename DeviceManagerD3D11 to DeviceManagerDx

Comment 41

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/02d76f158f3d
Try tearing-free drawing with GDI r=dvander,mattwoodrow

Comment 42

[Carsten Book [:Tomcat]]

sorry had to back this out for xperf regressions like https://treeherder.mozilla.org/logviewer.html#?job_id=33479787&repo=mozilla-inbound#L1577

Comment 43

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f33e41f8366b
Backed out changeset 02d76f158f3d for talos xperf regressions

Comment 44

[Sotaro Ikeda [:sotaro]]

I am going to look into the problem.

Comment 45

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 2 - Add IDirectDraw7 supportfor getting ScanLine

Rebased.

Comment 46

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 3 - Rename DeviceManagerD3D11 to DeviceManagerDx

Rebased.

Comment 47

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 3 - Rename DeviceManagerD3D11 to DeviceManagerDx

Comment 48

[Sotaro Ikeda [:sotaro]]

Adding usage of DDraw increased disk access and xperf was failed.
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=b756d3e1cd1c&selectedJob=25799517

Comment 49

[Sotaro Ikeda [:sotaro]]

From comment 33, we can defer DDraw creation to avoid xperf failure.

> we can defer it until we know the compositor type, rather than when the compositor needs to paint.

Comment 50

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 2.1 - Defer DirectDraw creation

Comment 51

[Sotaro Ikeda [:sotaro]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1a6ee265bdac

Comment 52

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

Comment on attachment 8781820 [details] [diff] [review]
patch part 2.1 - Defer DirectDraw creation

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

::: gfx/thebes/DeviceManagerD3D11.cpp
@@ +583,5 @@
>  
>  void
>  DeviceManagerD3D11::InitializeDirectDraw()
>  {
>    MOZ_ASSERT(!mDirectDraw);

Can you assert that this is only called on the compositor thread? If it can be called from multiple threads, the final assignment needs to hold mDeviceLock.

Comment 53

[Sotaro Ikeda [:sotaro]]

(In reply to David Anderson [:dvander] from comment #52)
-------------------------------------------------------
> 
> ::: gfx/thebes/DeviceManagerD3D11.cpp
> @@ +583,5 @@
> >  
> >  void
> >  DeviceManagerD3D11::InitializeDirectDraw()
> >  {
> >    MOZ_ASSERT(!mDirectDraw);
> 
> Can you assert that this is only called on the compositor thread? If it can
> be called from multiple threads, the final assignment needs to hold
> mDeviceLock.

Yes, I will update the patch. Thanks for the review.

Comment 54

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 2.1 - Defer DirectDraw creation

Apply the comment.

Comment 55

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 3 - Rename DeviceManagerD3D11 to DeviceManagerDx

Rebased.

Comment 56

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 3 - Rename DeviceManagerD3D11 to DeviceManagerDx

Comment 57

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 1 - Try tearing-free drawing with GDI

Fix test failure.

Comment 58

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 1 - Try tearing-free drawing with GDI

Comment 59

[Sotaro Ikeda [:sotaro]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f63abdd10942&selectedJob=26014447

Comment 60

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 2 - Add IDirectDraw7 supportfor getting ScanLine

Rebased.

Comment 61

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 2.1 - Defer DirectDraw creation

Rebased.

Comment 62

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 3 - Rename DeviceManagerD3D11 to DeviceManagerDx

Rebased.

Comment 63

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 3 - Rename DeviceManagerD3D11 to DeviceManagerDx

Comment 64

[Sotaro Ikeda [:sotaro]]

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

Comment 65

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 2 - Add IDirectDraw7 supportfor getting ScanLine

Rebased.

Comment 66

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 2.1 - Defer DirectDraw creation

Comment 67

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 3 - Rename DeviceManagerD3D11 to DeviceManagerDx

Comment 68

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7cae917bd519
Reduce tearing with basic layers on Windows r=dvander

Comment 69

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/7cae917bd519