Closed Bug 1196417 Opened 4 years ago Closed 4 years ago

Make DXVA fallback only affect the current video


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

Not set



Tracking Status
firefox42 --- wontfix
firefox43 --- fixed


(Reporter: mattwoodrow, Assigned: mattwoodrow)




(1 file)

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.
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.
Attachment #8650111 - Flags: review?(cpearce)
Comment on attachment 8650111 [details] [diff] [review]

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.
Attachment #8650111 - Flags: review?(cpearce) → review+
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
should we get this uplifted to 42 and the various dxva changes?
Flags: needinfo?(matt.woodrow)
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.
Flags: needinfo?(matt.woodrow)
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.
You need to log in before you can comment on or make changes to this bug.