1151611 - Expose DXVA status in about:support and crashes

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Jeff Muizelaar [:jrmuizel]]

This would be helpful for diagnosing problems.

Comment 1

[Matt Woodrow (:mattwoodrow)]

A few thoughts about this:

We can't really tell if DXVA will be used until we have an actual video playing, and we've attempted to initialize the decoder.

Possible ideas:

* Add about:media to all versions of firefox, and display DXVA status for any currently playing videos.

* Attempt to initialize a dummy DXVA decoder when we open about:support and check if that succeeds.

* Make about:support report the status of the prefs/blacklisting controlling DXVA, and don't worry about checking if the hardware supports it.

* Have a 'ever initialized' value reported in about:support that will start returning true after the first DXVA video is played.

Any thoughts Anthony?

Comment 2

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

(In reply to Matt Woodrow (:mattwoodrow) from comment #1)
> * Add about:media to all versions of firefox, and display DXVA status for
> any currently playing videos.

Gavin won't have a bar of it.

> * Attempt to initialize a dummy DXVA decoder when we open about:support and
> check if that succeeds.

This seems a good option on the basis that it will give us the most accurate answer.

We may also want to record information about fallbacks. Perhaps we could record the success/failure of dxva in a string of 1s and 0s so we can work out a percentage of time DXVA gets used. It may be worth splitting this between different video resolutions.

Perhaps: DXVA 2160=0% 1080=90% 720=100% <=480=100%

Comment 3

[Matt Woodrow (:mattwoodrow)]

(In reply to Anthony Jones (:kentuckyfriedtakahe, :k17e) from comment #2)

> 
> > * Attempt to initialize a dummy DXVA decoder when we open about:support and
> > check if that succeeds.
> 
> This seems a good option on the basis that it will give us the most accurate
> answer.

Ok.

> 
> We may also want to record information about fallbacks. Perhaps we could
> record the success/failure of dxva in a string of 1s and 0s so we can work
> out a percentage of time DXVA gets used. It may be worth splitting this
> between different video resolutions.
> 
> Perhaps: DXVA 2160=0% 1080=90% 720=100% <=480=100%

Why would initialization succeed for a given resolution some of the time but not others? With the shared decoders we also just initialize once, and then reconfigure for different resolutions.

Comment 4

[Matt Woodrow (:mattwoodrow)]

Attachment: Add DXVA status to about:support

Comment 5

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8594615 [details] [diff] [review]
Add DXVA status to about:support

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

::: dom/media/fmp4/MP4Reader.cpp
@@ +91,5 @@
> +  nsRefPtr<PlatformDecoderModule> platform = PlatformDecoderModule::Create();
> +
> +  nsRefPtr<MediaDataDecoder> decoder =
> +    platform->CreateDecoder(config, nullptr, nullptr, aBackend, nullptr);
> +  nsresult rv = decoder->Init();

I'm a little wary here of calling WMFVideoMFTManager::Init() on the main thread, as it calls WMFVideoMFTManager::InitializeDXVA() which does a sync dispatch to the main thread, and it looks like nsThread just spins the event loop in that case. So we might get unexpected results there.

So I think you should add a check to WMFVideoMFTManager::InitializeDXVA() that tests if we're on the main thread and just calls the runnable if so immediately, rather than doing an unnecessary sync dispatch to the main thread.

::: dom/media/fmp4/wmf/WMFVideoMFTManager.cpp
@@ -78,5 @@
>    , mLayersBackend(aLayersBackend)
>    // mVideoStride, mVideoWidth, mVideoHeight, mUseHwAccel are initialized in
>    // Init().
>  {
> -  NS_ASSERTION(!NS_IsMainThread(), "Should not be on main thread.");

The same assertion was cargo-culted into GonkVideoDecoderManager's constructor... So you should probably remove it from there too.

::: toolkit/modules/tests/browser/browser_Troubleshoot.js
@@ +198,5 @@
>          },
>          windowLayerManagerRemote: {
>            type: "boolean",
>          },
> +		supportsHardwareH264: {

This doesn't look like it's indented correctly.

Also, I'm not sure how qualified I am to review changes to this code...

Comment 6

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

(In reply to Matt Woodrow (:mattwoodrow) from comment #3)
> Why would initialization succeed for a given resolution some of the time but
> not others? With the shared decoders we also just initialize once, and then
> reconfigure for different resolutions.

The black frame detection on AMD will cause us to fall back. It happens more on higher resolutions. Maybe we can track this better through telemetry.

Comment 7

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8594615 [details] [diff] [review]
Add DXVA status to about:support

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

The front-end changes look fine. I'm a bit uneasy about knowing that opening about:support will trigger this less-common path of initializing the platform decoder in the main thread.

Also, shouldn't it be possible to cache an answer if h264 decoding has already happened during this session? (instead of trying to instantiate the test video). Is that worth it?

::: toolkit/modules/Troubleshoot.jsm
@@ +312,5 @@
>                       getInterface(Ci.nsIDOMWindowUtils);
>        try {
>          data.windowLayerManagerType = winUtils.layerManagerType;
>          data.windowLayerManagerRemote = winUtils.layerManagerRemote;
> +		data.supportsHardwareH264 = winUtils.supportsHardwareH264Decoding;

nit: tabs

Comment 8

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7362533abe5a

Comment 9

[Matt Woodrow (:mattwoodrow)]

(In reply to :Felipe Gomes from comment #7)
> 
> Also, shouldn't it be possible to cache an answer if h264 decoding has
> already happened during this session? (instead of trying to instantiate the
> test video). Is that worth it?
> 

We could do that, but we'd still need this code for the cases where a video hasn't been watched before. Given that, I don't think it's worth adding the extra complexity.

Comment 10

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/fafcc7f4f663

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/7362533abe5a
https://hg.mozilla.org/mozilla-central/rev/fafcc7f4f663