Closed Bug 1259571 Opened 4 years ago Closed 3 years ago

Reduce tearing with basic layers on Windows

Categories

(Core :: Graphics: Layers, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla51
Tracking Status
firefox51 --- fixed

People

(Reporter: jrmuizel, Assigned: sotaro)

References

(Blocks 1 open bug)

Details

(Whiteboard: gfx-noted)

Attachments

(5 files, 29 obsolete files)

9.22 KB, patch
Details | Diff | Splinter Review
14.61 KB, patch
sotaro
: review+
Details | Diff | Splinter Review
11.62 KB, patch
sotaro
: review+
Details | Diff | Splinter Review
5.54 KB, patch
sotaro
: review+
Details | Diff | Splinter Review
56.20 KB, patch
sotaro
: review+
Details | Diff | Splinter Review
This would make things nicer.
Whiteboard: gfx-noted
Assignee: nobody → sotaro.ikeda.g
Assignee: sotaro.ikeda.g → nobody
Assignee: nobody → sotaro.ikeda.g
Tearing happened often when I did scrolling during playback the following video.
  https://www.youtube.com/watch?v=iNJdPyoqt8U
Temporary patch just to check if a workaround in Comment 1 work. And confirmed that the tearing was addressed on my pc.
Attachment #8770526 - Attachment is obsolete: true
Comment on attachment 8770864 [details] [diff] [review]
patch - Try tearing-free drawing with GDI

:mattwoodrow, can you feedback to the patch?
Attachment #8770864 - Flags: feedback?(matt.woodrow)
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?
Attachment #8770864 - Flags: feedback?(matt.woodrow)
(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
(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?
(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.
(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.
> > 
> > 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.
(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.
Attachment #8770864 - Attachment is obsolete: true
Attachment #8772734 - Attachment is obsolete: true
Attachment #8773125 - Attachment is obsolete: true
Attachment #8773128 - Attachment is obsolete: true
Rebased.
Attachment #8773173 - Attachment is obsolete: true
Attachment #8773175 - Attachment is obsolete: true
Attachment #8773180 - Flags: feedback?(matt.woodrow)
Attachment #8773181 - Flags: feedback?(matt.woodrow)
(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.
Attachment #8773181 - Flags: feedback?(matt.woodrow) → feedback+
Attachment #8773180 - Flags: feedback?(matt.woodrow) → feedback+
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.
Attachment #8773180 - Attachment is obsolete: true
The patch is for testing if D3DKMTWaitForVerticalBlankEvent as flip timing.
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.
From comment 25, D3DKMTWaitForVerticalBlankEvent seemed not good for the flip timing.
(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.
Attachment #8773181 - Attachment is obsolete: true
Attachment #8774201 - Flags: review?(matt.woodrow)
Attachment #8774634 - Flags: review?(matt.woodrow)
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?
Attachment #8774201 - Flags: review?(matt.woodrow) → review+
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.
Attachment #8774634 - Flags: review?(matt.woodrow)
Attachment #8774634 - Flags: review?(dvander)
Attachment #8774634 - Flags: review+
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.
Attachment #8774634 - Flags: review?(dvander) → review-
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.
Attachment #8774634 - Attachment is obsolete: true
Attachment #8776861 - Attachment description: patch - Rename DeviceManagerD3D11 to DeviceManagerDx → patch part 3 - Rename DeviceManagerD3D11 to DeviceManagerDx
Attachment #8776851 - Flags: review?(dvander)
Attachment #8776861 - Flags: review?(dvander)
Attachment #8776861 - Flags: review?(dvander) → review+
Comment on attachment 8776851 [details] [diff] [review]
patch part 2 - Add IDirectDraw7 supportfor getting ScanLine

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

Thanks!
Attachment #8776851 - Flags: review?(dvander) → review+
Rebased.
Attachment #8774201 - Attachment is obsolete: true
Attachment #8778738 - Flags: review+
Rebased.
Attachment #8776851 - Attachment is obsolete: true
Attachment #8778741 - Flags: review+
Rebased.
Attachment #8776861 - Attachment is obsolete: true
Attachment #8778752 - Flags: review+
Attachment #8778752 - Attachment is obsolete: true
Attachment #8778759 - Flags: review+
Pushed by sikeda@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/02d76f158f3d
Try tearing-free drawing with GDI r=dvander,mattwoodrow
sorry had to back this out for xperf regressions like https://treeherder.mozilla.org/logviewer.html#?job_id=33479787&repo=mozilla-inbound#L1577
Flags: needinfo?(sotaro.ikeda.g)
Backout by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f33e41f8366b
Backed out changeset 02d76f158f3d for talos xperf regressions
I am going to look into the problem.
Flags: needinfo?(sotaro.ikeda.g)
Rebased.
Attachment #8778741 - Attachment is obsolete: true
Attachment #8781416 - Flags: review+
Rebased.
Attachment #8778759 - Attachment is obsolete: true
Attachment #8781417 - Flags: review+
Attachment #8781417 - Attachment is obsolete: true
Attachment #8781441 - Flags: review+
Adding usage of DDraw increased disk access and xperf was failed.
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=b756d3e1cd1c&selectedJob=25799517
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.
Attachment #8781820 - Flags: review?(dvander)
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.
Attachment #8781820 - Flags: review?(dvander) → review+
(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.
Apply the comment.
Attachment #8781820 - Attachment is obsolete: true
Attachment #8781856 - Flags: review+
Rebased.
Attachment #8781441 - Attachment is obsolete: true
Attachment #8781871 - Flags: review+
Attachment #8781871 - Attachment is obsolete: true
Attachment #8781884 - Flags: review+
Fix test failure.
Attachment #8778738 - Attachment is obsolete: true
Attachment #8782767 - Flags: review+
Attachment #8782767 - Attachment is obsolete: true
Attachment #8782815 - Flags: review+
Rebased.
Attachment #8781416 - Attachment is obsolete: true
Attachment #8783811 - Flags: review+
Rebased.
Attachment #8781856 - Attachment is obsolete: true
Attachment #8783812 - Flags: review+
Rebased.
Attachment #8781884 - Attachment is obsolete: true
Attachment #8783814 - Flags: review+
Attachment #8783814 - Attachment is obsolete: true
Attachment #8783818 - Flags: review+
Rebased.
Attachment #8783811 - Attachment is obsolete: true
Attachment #8783975 - Flags: review+
Attachment #8783812 - Attachment is obsolete: true
Attachment #8783976 - Flags: review+
Pushed by sikeda@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7cae917bd519
Reduce tearing with basic layers on Windows r=dvander
https://hg.mozilla.org/mozilla-central/rev/7cae917bd519
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
You need to log in before you can comment on or make changes to this bug.