1196417 - Make DXVA fallback only affect the current video

Status: RESOLVED FIXED

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

Description

[Matt Woodrow (:mattwoodrow)]

When we added the fallback, we did so under the impression that some GPUs were fundamentally broken with DXVA and we wanted to disable it entirely.

It appears now that some GPUs just can't handle the throughput requirements of 60fps (see bug 1196411), or we're trying to decode too many videos at once.

I'm planning on not attempting DXVA for the former case (bug 1196411 again), so we now only need to handle the overloaded GPU case.

For this, I think just disabling DXVA for the current video makes the most sense. In an ideal world we'd be able to step back up again if things calm down, but I'll leave that for another day.

Comment 1

[Matt Woodrow (:mattwoodrow)]

Attachment: fallback-only-one-decoder

How do you feel about this?

This only disables DXVA for the current decoder, so if the resolution changes then we'll  maybe enable it again. This seems beneficial if they drop to a lower bitrate, and worse if they step up, so even overall. Chrome doesn't ever disable DXVA this way, so I think we can get away with this (and provide a better experience).

Switching the layers backend to BACKEND_NONE to disable h/w accel seems a bit disingenuous, but it's a lot easier than adding a new bool parameter to both CreateDecoder and CreateVideoDecoder and all the subclasses that implement those. I can do that if you prefer though.

Comment 2

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

Comment on attachment 8650111 [details] [diff] [review]
fallback-only-one-decoder

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

I think this is OK; passing the layers type basically means what type of hardware acceleration to use, so I'm OK passing Basic to mean disable HW acceleration.

::: dom/media/MediaFormatReader.cpp
@@ +586,5 @@
>  void
>  MediaFormatReader::DisableHardwareAcceleration()
>  {
>    MOZ_ASSERT(OnTaskQueue());
> +  if (HasVideo() && !mHardwareAccelerationDisabled) { 

Trailing whitespace.

Comment 3

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/87a4cfa9fe0a

Comment 4

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/87a4cfa9fe0a

Comment 5

[Jean-Yves Avenard [:jya]]

should we get this uplifted to 42 and the various dxva changes?

Comment 6

[Matt Woodrow (:mattwoodrow)]

I've requested uplift for some of the others, I'm not too worried about uplifting this one though. It's low risk though, so feel free to do so if you think we want it.

Comment 7

[Chris Peterson [:cpeterson]]

Based on Matt's comment, I don't think we should bother uplifting this fix. Firefox 42 is the next big campaign release, so the Release Management team would like to minimize churn and stability risks. IIUC, this bug is an improvement for an uncommon use case (playing multiple 60fps videos simultaneously?), not a bug fix.