Closed Bug 1257013 Opened 4 years ago Closed 4 years ago

Use readback to synchronize DXVA textures

Categories

(Core :: Audio/Video: Playback, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox47 --- fixed
firefox48 --- fixed

People

(Reporter: mattwoodrow, Assigned: mattwoodrow)

References

Details

Attachments

(4 files)

We're currently using a query object for d3d9, and KeyedMutex for d3d11.

Query objects have caused us a bunch of issues, since they can sometimes never complete and we have to give up. We have a lot of complexity to deal with these 'corrupt' frames that causes us to disable DXVA at runtime for some users.

KeyedMutex seems more reliable, but I don't think it's actually sufficient (See bug 1236112 comment 42). The KeyedMutex stops the compositor from reading from the RGB texture before it completes, but nothing prevents the decoder from writing to a YUV texture before the colour conversion finishes reading from it.

The Microsoft recommended way to synchronize between APIs is to do a blocking readback of the pixel data. This is documented at [1], with the code sample being mirrored at [2].

I think we should do this, as it should reduce a lot of our pain points with DXVA.

It's possible that this blocking will reduce concurrency (between decoding and colour conversion), and we'll need to keep a queue of decoded but not colour converted frames to fix this. I haven't seen any evidence that this will be required yet though.


[1] https://msdn.microsoft.com/en-nz/library/windows/desktop/ee913554(v=vs.85).aspx#interoperability_between_direct3d_9ex_and_dxgi_based_apis
[2] https://people.mozilla.org/~jmuizelaar/SharedSurfaceQueue/
Attachment #8730978 - Flags: review?(cpearce)
Attachment #8730979 - Flags: review?(cpearce)
Attachment #8730978 - Flags: review?(bas)
Attachment #8730979 - Flags: review?(bas)
(In reply to Matt Woodrow (:mattwoodrow) from comment #0)
> KeyedMutex seems more reliable, but I don't think it's actually sufficient
> (See bug 1236112 comment 42). The KeyedMutex stops the compositor from
> reading from the RGB texture before it completes, but nothing prevents the
> decoder from writing to a YUV texture before the colour conversion finishes
> reading from it.

I'm a bit confused by this. Is writer of the YUV texture using the same device as the color conversion code?
Can you elaborate on what you think is going wrong in the keyed mutex case?
Flags: needinfo?(matt.woodrow)
(In reply to Jeff Muizelaar [:jrmuizel] from comment #4)
> 
> I'm a bit confused by this. Is writer of the YUV texture using the same
> device as the color conversion code?
> Can you elaborate on what you think is going wrong in the keyed mutex case?

They are on the same device, but it was never really clear to me if DXVA and D3D synchronized correctly when on the same device.

I don't have evidence for this actually being a problem.

It seemed plausible that we wouldn't have guaranteed ordering between IDirectXVideoDecoderService calls and IDirectXVideoProcessorService ones.

We're definitely releasing the IMFSample immediately which is what notifies the MFTransform that the D3D surface is available for re-use, so we need the device to be internally synchronizing access for things to work correctly.

It seems nice to block within the decoder so that the MediaDecoderStateMachine can accurately judge decoding speed. Returning frames that asynchronously complete runs the risk of blocking the compositor instead, which we have no reporting of.

It also seems valuable to match the d3d9 behavior, though I could be convinced otherwise.
Flags: needinfo?(matt.woodrow) → needinfo?(jmuizelaar)
(In reply to Matt Woodrow (:mattwoodrow) from comment #5)
> (In reply to Jeff Muizelaar [:jrmuizel] from comment #4)
> > 
> > I'm a bit confused by this. Is writer of the YUV texture using the same
> > device as the color conversion code?
> > Can you elaborate on what you think is going wrong in the keyed mutex case?
> 
> They are on the same device, but it was never really clear to me if DXVA and
> D3D synchronized correctly when on the same device.
> 
> I don't have evidence for this actually being a problem.
> 
> It seemed plausible that we wouldn't have guaranteed ordering between
> IDirectXVideoDecoderService calls and IDirectXVideoProcessorService ones.

I'd expect that all calls through the same device should be properly synchronized. It also seems like something we could confirm with Microsoft. Is there something concrete we can ask them?
Flags: needinfo?(matt.woodrow)
(In reply to Jeff Muizelaar [:jrmuizel] from comment #6)
> 
> I'd expect that all calls through the same device should be properly
> synchronized. It also seems like something we could confirm with Microsoft.
> Is there something concrete we can ask them?

Just if they guarantee ordering between IDirectXVideoDecoderService and IDirectXVideoProcessorService calls, on the same d3d device.
Flags: needinfo?(matt.woodrow)
(In reply to Matt Woodrow (:mattwoodrow) from comment #7)
> (In reply to Jeff Muizelaar [:jrmuizel] from comment #6)
> > 
> > I'd expect that all calls through the same device should be properly
> > synchronized. It also seems like something we could confirm with Microsoft.
> > Is there something concrete we can ask them?
> 
> Just if they guarantee ordering between IDirectXVideoDecoderService and
> IDirectXVideoProcessorService calls, on the same d3d device.

Ok, I'll try to ask.
Flags: needinfo?(jmuizelaar)
Comment on attachment 8730978 [details] [diff] [review]
Part 1: Use readback for d3d9

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

::: dom/media/platforms/wmf/DXVA2Manager.cpp
@@ +440,5 @@
>    hr = image->AllocateAndCopy(mTextureClientAllocator, surface, aRegion);
>    NS_ENSURE_TRUE(SUCCEEDED(hr), hr);
>  
> +  RefPtr<IDirect3DSurface9> sourceSurf = image->GetD3D9Surface();
> +

Should have a comment explaining why we do this.
Attachment #8730978 - Flags: review?(cpearce) → review+
Comment on attachment 8730979 [details] [diff] [review]
Part 2: Use readback for d3d11

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

::: dom/media/platforms/wmf/DXVA2Manager.cpp
@@ +767,5 @@
>    RefPtr<ID3D11Texture2D> texture = image->GetTexture();
>    hr = CreateOutputSample(sample, texture);
>    NS_ENSURE_TRUE(SUCCEEDED(hr), hr);
>  
> +  hr = mTransform->Output(&sample); 

Trailing whitespace.

Also add a comment here explaining why you're doing what looks like an unnecessary copy.
Attachment #8730979 - Flags: review?(cpearce) → review+
Attachment #8730980 - Flags: review?(ajones) → review+
Attachment #8730978 - Flags: review?(bas) → review+
Attachment #8730979 - Flags: review?(bas) → review+
Jeff, do you want me to hold off on landing the d3d11 piece of this until you find out more?
Flags: needinfo?(jmuizelaar)
(In reply to Matt Woodrow (:mattwoodrow) from comment #11)
> Jeff, do you want me to hold off on landing the d3d11 piece of this until
> you find out more?

No, we can always back it out and landing it will give us more information than waiting.
Flags: needinfo?(jmuizelaar)
The trace log in DXVA Checker no longer reports any Firefox DXVA events. Performance has also regressed compared with yesterday's Nightly.
Duplicate of this bug: 1248152
Comment on attachment 8730978 [details] [diff] [review]
Part 1: Use readback for d3d9

Approval Request Comment
[Feature/regressing bug #]:
[User impact if declined]: We believe this fixes the out of order video frames issue (bug 1236112) for d3d9 and want to test it on a wider audience. We may want to back it out again before shipping.
[Describe test coverage new/current, TreeHerder]: Tested manually.
[Risks and why]: Medium to low risk, only affects video playback, changes the way we detect when decoding is done. Possible fallout would likely be worse playback performance, crashes etc are very unlikely.
[String/UUID change made/needed]: None
Attachment #8730978 - Flags: approval-mozilla-aurora?
I briefly tested this on Youtube, and it seems to work very well, and playback performance (or at least my subjective experience of it) is actually improved with it, on my system. (Core2 E8400, ATI HD4890)
Before this patch, I was pretty much routinely getting DXVA disabled. Tested with and without multiprocess/APZ, both seems to work.
Duplicate of this bug: 1208415
Hi Jet, while comment 18 is reassuring, the risk assessment from comment 17 is making me anxious. I would like to get another opinion on whether the reward on uplifting this patch is worth the risk it brings. Could you please let me know whether you think it's a good idea to uplift this to Aurora 47?

Hi Matt, you mentioned backing this out before we go to release. If we do decide to uplift this (waiting for a second opinion), what metrics will we used to decide whether to backout?
Flags: needinfo?(matt.woodrow)
Flags: needinfo?(bugs)
Anthony, want to weigh in here?
Flags: needinfo?(matt.woodrow) → needinfo?(ajones)
Ritu - this fix makes video more reliable at the risk of making it go more slowly. It will make decode faster for many users because it eliminates the issue of falling back to software decode. We're not doing more computation, we're stopping our decode and waiting for the GPU to catch up. There is a risk that this will make things slow so Matt is suggesting we try it.

We can keep an eye on VIDEO_FRAME_DROPS_PROPORTION but it is more than likely we'll have to rely on people reporting bugs.
Flags: needinfo?(ajones)
One thing I might add: if the GPU cannot keep up (for whatever reason), then we have to make sure the AV desync is corrected/recovered from as gracefully as possible in these cases.
Comment on attachment 8730978 [details] [diff] [review]
Part 1: Use readback for d3d9

Based on comment 18 and 22 (Anthony thinks this is worth uplifting), taking it in Aurora47
Attachment #8730978 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Flags: needinfo?(bugs)
has problems uplifting to aurora

grafting 335436:b4e1e8c8dc0b "Bug 1257013 - Part 1: Use readback to synchronize d3d9 video. r=cpearce,Bas"
merging dom/media/platforms/wmf/DXVA2Manager.cpp
merging gfx/layers/D3D9SurfaceImage.cpp
merging gfx/layers/D3D9SurfaceImage.h
warning: conflicts while merging dom/media/platforms/wmf/DXVA2Manager.cpp! (edit, then use 'hg resolve --mark')
warning: conflicts while merging gfx/layers/D3D9SurfaceImage.cpp! (edit, then use 'hg resolve --mark')
warning: conflicts while merging gfx/layers/D3D9SurfaceImage.h! (edit, then use 'hg resolve --mark')
abort: unresolved conflicts, can't continue
(use hg resolve and hg graft --continue)
Flags: needinfo?(matt.woodrow)
The patches here depend on bug 1236112, and it's a pain to rebase around that. I'll request uplift for that too.
Depends on: 1236112
Flags: needinfo?(matt.woodrow)
Somewhat confused. Should I go ahead and land these without 1236112?
Flags: needinfo?(matt.woodrow)
(In reply to Wes Kocher (:KWierso) from comment #28)
> Somewhat confused. Should I go ahead and land these without 1236112?

Wes, if it helps, I have A+'d the uplift from bug 1236112.
(In reply to Wes Kocher (:KWierso) from comment #28)
> Somewhat confused. Should I go ahead and land these without 1236112?

No, you need that bug to land first. The aurora patch I uploaded was for an unrelated (and tiny) rebase change.
Flags: needinfo?(matt.woodrow)
I think i got it right now and pushed this to aurora :)
You need to log in before you can comment on or make changes to this bug.