1135935 - Recycle SharedTextureClientD3D9 during video playback

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Sotaro Ikeda [:sotaro]]

new SharedTextureClientD3D9 is created for each video frame in WMFReader. It add new Direct3DTexture allocation and TextureClient setup/shutdown via IPC. It would be nice if we could remove this.

Comment 1

[Sotaro Ikeda [:sotaro]]

The following diagrams relate to D3D9SurfaceImage and mp4 playback.

https://github.com/sotaroikeda/firefox-diagrams/blob/master/media/dom_media_fmp4_WMFDecoderModule_Firefox_39.pdf?raw=true
https://github.com/sotaroikeda/firefox-diagrams/blob/master/gfx/gfx_D3D9SurfaceImage.pdf?raw=true

Comment 2

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 1 - Add ipc/chromium to moz.build

Comment 3

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 2 - Change to TextureClientRecycleAllocator

Comment 4

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 3 - Change related to SharedTextureClientD3D9

Comment 5

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 4 - Change to DXVA2Manager

Comment 6

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 3 - Change related to SharedTextureClientD3D9

Fix nits.

Comment 7

[Jeff Muizelaar [:jrmuizel]]

What's needed to make this work with the IOSurfaces we use on OS X?

Comment 8

[Sotaro Ikeda [:sotaro]]

We need to change that MacIOSurfaceImage could recycle MacIOSurfaceTextureClientOGL like attachment 8582052 [details] [diff] [review].

Comment 9

[Matt Woodrow (:mattwoodrow)]

I rebased this, fixed some bugs and am seeing big wins when playing 4k video.

I'm going to work on getting these ready for review/landing.

Comment 10

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 1: Don't implement ISurfaceAllocator in the recycler

I don't see what this gains us.

Comment 11

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 2: Merge recycler impl class into the base

Same with this, it just seems to add complexity.

Comment 12

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 3: Allow overriding of the allocations for the recycler

Comment 13

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 4: Move d3d9 texture allocation into TextureClient

Comment 14

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 5: Add d3d9 recycler

Comment 15

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 6: Wait for the host to stop using textures before recycling

This makes me a bit sad. We have way too many ways of waiting on the compositor thread for various things, I'm sure we can do better.

I'll file a follow up bug for investigating this and trying to clean it up.

Comment 16

[Matt Woodrow (:mattwoodrow)]

(In reply to Jeff Muizelaar [:jrmuizel] from comment #7)
> What's needed to make this work with the IOSurfaces we use on OS X?

OSX provides the IOSurfaces it outputs, we don't allocate them explicitly.

I believe it recycles them internally though.

Comment 17

[Matt Woodrow (:mattwoodrow)]

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

Comment 18

[...]

I tested this build, DXVA2 keeps crashing on 4K YouTube videos, whereas D3D11 doesn't.
For DXVA2, 1080p YouTube videos have no noticeable performance improvements compared to the latest Nightly.
4K YouTube videos successfully load without crashing using D3D11, however, there are also no performance improvements compared to the latest Nightly.

Comment 19

[Sotaro Ikeda [:sotaro]]

Comment on attachment 8645916 [details] [diff] [review]
Part 6: Wait for the host to stop using textures before recycling

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

TextureClientRecycleAllocator is not the only TextureClient recycler. On gonk, camera preview and video playback do texture recycling without using TextureClientRecycleAllocator. Can's we use HasRecycleCallback() for MayBeRecycled()?

Comment 20

[Matt Woodrow (:mattwoodrow)]

(In reply to ... from comment #18)
> I tested this build, DXVA2 keeps crashing on 4K YouTube videos, whereas
> D3D11 doesn't.
> For DXVA2, 1080p YouTube videos have no noticeable performance improvements
> compared to the latest Nightly.
> 4K YouTube videos successfully load without crashing using D3D11, however,
> there are also no performance improvements compared to the latest Nightly.

What exactly do you mean by 'D3D11' as compared to 'DXVA2'?

Comment 21

[Sotaro Ikeda [:sotaro]]

(In reply to Sotaro Ikeda [:sotaro] from comment #19)
> Comment on attachment 8645916 [details] [diff] [review]
> Part 6: Wait for the host to stop using textures before recycling
> 
> Review of attachment 8645916 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> TextureClientRecycleAllocator is not the only TextureClient recycler. On
> gonk, camera preview and video playback do texture recycling without using
> TextureClientRecycleAllocator. Can's we use HasRecycleCallback() for
> MayBeRecycled()?

It just means recycling could be done without using TextureClientRecycleAllocator. In the patch, MayBeRecycled() is not used on gonk though.

Comment 22

[Sotaro Ikeda [:sotaro]]

Comment on attachment 8645910 [details] [diff] [review]
Part 1: Don't implement ISurfaceAllocator in the recycler

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

::: gfx/layers/client/TextureClientRecycleAllocator.cpp
@@ -15,5 @@
>  
>  namespace mozilla {
>  namespace layers {
>  
> -class TextureClientRecycleAllocatorImp : public ISurfaceAllocator

In current gecko, TextureClientRecycleAllocatorImp is used just to keep it alive untill all TextureClients' recycle callbacks are called. The actual callback function is TextureClientRecycleAllocatorImp::RecycleCallbackImp(). To make the function working safely, TextureClientRecycleAllocatorImp is derived from ISurfaceAllocator.

The patch removes TextureClientRecycleAllocatorImp. How do patches keep TextureClientRecycleAllocator live when TextureClientRecycleAllocator::RecycleCallbackImp() is called?

Comment 23

[Matt Woodrow (:mattwoodrow)]

(In reply to Sotaro Ikeda [:sotaro] from comment #22)
> Comment on attachment 8645910 [details] [diff] [review]
> Part 1: Don't implement ISurfaceAllocator in the recycler
> 
> Review of attachment 8645910 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/layers/client/TextureClientRecycleAllocator.cpp
> @@ -15,5 @@
> >  
> >  namespace mozilla {
> >  namespace layers {
> >  
> > -class TextureClientRecycleAllocatorImp : public ISurfaceAllocator
> 
> In current gecko, TextureClientRecycleAllocatorImp is used just to keep it
> alive untill all TextureClients' recycle callbacks are called. The actual
> callback function is TextureClientRecycleAllocatorImp::RecycleCallbackImp().
> To make the function working safely, TextureClientRecycleAllocatorImp is
> derived from ISurfaceAllocator.
> 
> The patch removes TextureClientRecycleAllocatorImp. How do patches keep
> TextureClientRecycleAllocator live when
> TextureClientRecycleAllocator::RecycleCallbackImp() is called?

You're right. This was really non-obvious that we were implementing ISurfaceAllocator just so that the TextureClient would hold a reference to us. Having the recycle callback hold a raw pointer to a refcounted object seems like the main problem here.

Comment 24

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 6: Wait for the host to stop using textures before recycling v2

Comment 25

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8646424 [details] [diff] [review]
Part 6: Wait for the host to stop using textures before recycling v2

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

::: gfx/layers/client/TextureClient.h
@@ +539,5 @@
>    gl::GfxTextureWasteTracker mWasteTracker;
>    bool mShared;
>    bool mValid;
>    bool mAddedToCompositableClient;
> +  bool mMayBeRecycled;

This is no longer needed, removed locally.

Comment 26

[Sotaro Ikeda [:sotaro]]

(In reply to Matt Woodrow (:mattwoodrow) from comment #24)
> Created attachment 8646424 [details] [diff] [review]
> Part 6: Wait for the host to stop using textures before recycling v2

I confirmed comment 22 is addressed by attachment 8646424 [details] [diff] [review].

Comment 27

[Sotaro Ikeda [:sotaro]]

Comment on attachment 8645910 [details] [diff] [review]
Part 1: Don't implement ISurfaceAllocator in the recycler

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

review+ based on comment 26.

Comment 28

[Matt Woodrow (:mattwoodrow)]

Updated builds: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d36f7e1d0236

Confirmed that these are actually recycling correctly now.

Comment 29

[...]

I mean D3D11 video decoding as in through toggling "media.windows-media-foundation.allow-d3d11-dxva" to true.
The new build crashes more consistently on all video qualities on a new profile after a few seconds of playing through DXVA2.
D3D11-DXVA doesn't crash at all.

Comment 30

[Matt Woodrow (:mattwoodrow)]

(In reply to ... from comment #29)
> I mean D3D11 video decoding as in through toggling
> "media.windows-media-foundation.allow-d3d11-dxva" to true.
> The new build crashes more consistently on all video qualities on a new
> profile after a few seconds of playing through DXVA2.
> D3D11-DXVA doesn't crash at all.

Ah, ok. These patches don't affect D3D11-DXVA at all sorry, though I'll consider adding recycling to that too if we get success here.

I think I've got the crash issues sorted now.

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

Comment 31

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/844bfdb58072
https://hg.mozilla.org/integration/mozilla-inbound/rev/47d0d08c131c
https://hg.mozilla.org/integration/mozilla-inbound/rev/62a8922fe05c
https://hg.mozilla.org/integration/mozilla-inbound/rev/1e3f1190f6ed
https://hg.mozilla.org/integration/mozilla-inbound/rev/9e3380c34533
https://hg.mozilla.org/integration/mozilla-inbound/rev/fc15644207d8

Comment 32

[...]

I tested the new build which has indeed resolved the crashing issues.

4K and 60fps performance has significantly improved. Performance is nearly but not quite up to par with IE11 but still superior to Chrome.

There are occasional random kinks such as huge sustained spikes in CPU usage when changing video quality, going into full screen, scrubbing etc.

Looking forward to seeing these improvements applied to D3D11-DXVA, which should be enabled by default on supporting hardware.

Comment 33

[Matt Woodrow (:mattwoodrow)]

(In reply to ... from comment #32)
> I tested the new build which has indeed resolved the crashing issues.
> 
> 4K and 60fps performance has significantly improved. Performance is nearly
> but not quite up to par with IE11 but still superior to Chrome.

Awesome! Thanks for testing this.

> 
> There are occasional random kinks such as huge sustained spikes in CPU usage
> when changing video quality, going into full screen, scrubbing etc.

What do you mean by 'sustained'? For a few seconds, or do we stick with high CPU for the remainder of the video? Or the remainder of the browser session?

> 
> Looking forward to seeing these improvements applied to D3D11-DXVA, which
> should be enabled by default on supporting hardware.

I haven't yet seen evidence of any wins from D3D11-DXVA, and saw big regressions on some hardware (which I'm sure is our bug).

Given that, it's hard to justify investing too much time in improving it, but I'll try get to it soon.

Comment 34

[...]

I'm having difficulties trying to reproduce the CPU spike consistently.

1. Play this video https://www.youtube.com/watch?v=iNJdPyoqt8U at the default quality (1080P for me) for about 10 or so seconds
2. Change the video quality to 4K and let the video play for a few more seconds
3. Continue to play the video but keep seeking between any points of the video in the 1080P buffered region (first 10 seconds) and the huge CPU spike appears and the video stutters substantially.

Once the CPU shoots up to ~50%, it persists for the entire video, unless paused or the tab closed. Reloading the video by refreshing the tab or closing it and opening a new tab also seems ineffective. Once the bug has occurred it also affects all other videos with huge persistent rises in CPU usage albeit at a slightly lower rate for lower quality videos, until the browser is restarted.

For me personally, I need D3D11 to have a colour correction utility software work correctly. It only corrects colours in browsers using D3D11 for video (Flash, IE11, Firefox with pref toggled) but doesn't seem to work with DXVA2 in Firefox.

Comment 35

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

(In reply to ... from comment #34)
> Once the CPU shoots up to ~50%, it persists for the entire video, unless
> paused or the tab closed. Reloading the video by refreshing the tab or
> closing it and opening a new tab also seems ineffective. Once the bug has
> occurred it also affects all other videos with huge persistent rises in CPU
> usage albeit at a slightly lower rate for lower quality videos, until the
> browser is restarted.

Matt - is this the Intel/AMD driver bug detection kicking in?

Comment 36

[Matt Woodrow (:mattwoodrow)]

(In reply to Anthony Jones (:kentuckyfriedtakahe, :k17e) from comment #35) 
> Matt - is this the Intel/AMD driver bug detection kicking in?

Almost certainly.

I'd like to make this fallback only affect the current video, not the entire browser session.

We probably should also try make it more lenient during 'high load' events (like seeking), since it is clearly kicking in for otherwise working drivers.

Comment 37

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/844bfdb58072
https://hg.mozilla.org/mozilla-central/rev/47d0d08c131c
https://hg.mozilla.org/mozilla-central/rev/62a8922fe05c
https://hg.mozilla.org/mozilla-central/rev/1e3f1190f6ed
https://hg.mozilla.org/mozilla-central/rev/9e3380c34533
https://hg.mozilla.org/mozilla-central/rev/fc15644207d8