Recycle SharedTextureClientD3D9 during video playback

RESOLVED FIXED in Firefox 43

Status

()

RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: sotaro, Assigned: mattwoodrow)

Tracking

(Blocks: 3 bugs)

unspecified
mozilla43
x86_64
Windows 7
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox43 fixed)

Details

Attachments

(6 attachments, 6 obsolete attachments)

3.15 KB, patch
sotaro
: review+
Details | Diff | Splinter Review
11.63 KB, patch
sotaro
: review+
Details | Diff | Splinter Review
9.85 KB, patch
sotaro
: review+
Details | Diff | Splinter Review
17.36 KB, patch
jrmuizel
: review+
Details | Diff | Splinter Review
7.92 KB, patch
jrmuizel
: review+
Details | Diff | Splinter Review
9.02 KB, patch
jrmuizel
: review+
Details | Diff | Splinter Review
(Reporter)

Description

4 years ago
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.
(Reporter)

Updated

4 years ago
Assignee: nobody → sotaro.ikeda.g
(Reporter)

Updated

4 years ago
Summary: Recycle SharedTextureClientD3D9 in WMFReader → Recycle SharedTextureClientD3D9 during video playback
(Reporter)

Comment 5

4 years ago
(Reporter)

Comment 6

4 years ago
Fix nits.
Attachment #8582045 - Attachment is obsolete: true
What's needed to make this work with the IOSurfaces we use on OS X?
(Reporter)

Comment 8

4 years ago
We need to change that MacIOSurfaceImage could recycle MacIOSurfaceTextureClientOGL like attachment 8582052 [details] [diff] [review].
(Assignee)

Updated

4 years ago
Blocks: 1191521
(Assignee)

Comment 9

4 years ago
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.
Assignee: sotaro.ikeda.g → matt.woodrow
(Assignee)

Comment 10

4 years ago
I don't see what this gains us.
Attachment #8580243 - Attachment is obsolete: true
Attachment #8582043 - Attachment is obsolete: true
Attachment #8582047 - Attachment is obsolete: true
Attachment #8582052 - Attachment is obsolete: true
Attachment #8645910 - Flags: review?(sotaro.ikeda.g)
(Assignee)

Comment 11

4 years ago
Same with this, it just seems to add complexity.
Attachment #8645911 - Flags: review?(sotaro.ikeda.g)
(Assignee)

Comment 14

4 years ago
Attachment #8645914 - Flags: review?(nical.bugzilla)
(Assignee)

Comment 15

4 years ago
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.
Attachment #8645916 - Flags: review?(nical.bugzilla)
(Assignee)

Comment 16

4 years ago
(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 18

4 years ago
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.
(Reporter)

Comment 19

4 years ago
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()?
(Assignee)

Comment 20

4 years ago
(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'?
(Reporter)

Comment 21

4 years ago
(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.
(Reporter)

Comment 22

4 years ago
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?
(Reporter)

Updated

4 years ago
Attachment #8645910 - Flags: review?(sotaro.ikeda.g)
(Assignee)

Comment 23

4 years ago
(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.
(Assignee)

Comment 24

4 years ago
Attachment #8645916 - Attachment is obsolete: true
Attachment #8645916 - Flags: review?(nical.bugzilla)
Attachment #8646424 - Flags: review?(nical.bugzilla)
(Assignee)

Comment 25

4 years ago
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.
(Assignee)

Updated

4 years ago
Attachment #8645913 - Flags: review?(nical.bugzilla) → review?(jmuizelaar)
(Assignee)

Updated

4 years ago
Attachment #8645914 - Flags: review?(nical.bugzilla) → review?(jmuizelaar)
(Assignee)

Updated

4 years ago
Attachment #8646424 - Flags: review?(nical.bugzilla) → review?(jmuizelaar)
(Reporter)

Comment 26

4 years ago
(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].
(Reporter)

Comment 27

4 years ago
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.
Attachment #8645910 - Flags: review+
(Reporter)

Updated

4 years ago
Attachment #8645911 - Flags: review?(sotaro.ikeda.g) → review+
(Reporter)

Updated

4 years ago
Attachment #8645912 - Flags: review?(sotaro.ikeda.g) → review+
(Assignee)

Updated

4 years ago
Blocks: 1193468
(Assignee)

Comment 28

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

Confirmed that these are actually recycling correctly now.
Attachment #8645913 - Flags: review?(jmuizelaar) → review+
Attachment #8645914 - Flags: review?(jmuizelaar) → review+
Attachment #8646424 - Flags: review?(jmuizelaar) → review+

Comment 29

4 years ago
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.
(Assignee)

Comment 30

4 years ago
(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 32

4 years ago
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.
(Assignee)

Comment 33

4 years ago
(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

4 years ago
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.
(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?
(Assignee)

Comment 36

4 years ago
(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.
(Assignee)

Updated

4 years ago
Blocks: 1195527
(Reporter)

Updated

4 years ago
Duplicate of this bug: 988941
You need to log in before you can comment on or make changes to this bug.