Closed Bug 1151611 Opened 7 years ago Closed 7 years ago

Expose DXVA status in about:support and crashes

Categories

(Core :: Audio/Video, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla40
Tracking Status
firefox40 --- fixed

People

(Reporter: jrmuizel, Assigned: mattwoodrow)

References

Details

Attachments

(1 file)

This would be helpful for diagnosing problems.
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?
Blocks: 1151721
Flags: needinfo?(ajones)
(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%
Flags: needinfo?(ajones)
(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.
Assignee: nobody → matt.woodrow
Attachment #8594615 - Flags: review?(cpearce)
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...
Attachment #8594615 - Flags: review?(cpearce) → review+
Attachment #8594615 - Flags: review?(felipc)
(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 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
Attachment #8594615 - Flags: review?(felipc) → review+
(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.
https://hg.mozilla.org/mozilla-central/rev/7362533abe5a
https://hg.mozilla.org/mozilla-central/rev/fafcc7f4f663
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
Blocks: 1180561
See Also: → 1219381
You need to log in before you can comment on or make changes to this bug.