Don't use DXVA if texture sharing is broken

RESOLVED FIXED in Firefox 40

Status

()

Core
Graphics: Layers
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: mattwoodrow, Assigned: mattwoodrow)

Tracking

29 Branch
mozilla41
Points:
---

Firefox Tracking Flags

(firefox39 affected, firefox40 fixed, firefox41 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

3 years ago
Created attachment 8623417 [details] [diff] [review]
dxva-texture-sharing

On some older intel drivers, texture sharing with D3D11 is broken.

We have existing code to check for this and disable D2D, but we didn't do the same for DXVA. We should.

Interestingly, D3D9 texture sharing on the same machine seems to work fine, so we could have DXVA video that way. We'd have to make that decision for the entire session though, before we know if the user is going to use video or not.

This was causing crashes in debug builds (when the blacklist was removed) because we MOZ_CRASH when texture sharing fails (with E_OUTOFMEMORY) in the compositor.
Attachment #8623417 - Flags: review?(jmuizelaar)
Comment on attachment 8623417 [details] [diff] [review]
dxva-texture-sharing

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

::: widget/windows/GfxInfo.cpp
@@ -1102,5 @@
> -    APPEND_TO_DRIVER_BLOCKLIST2(DRIVER_OS_ALL,
> -      (nsAString&)GfxDriverInfo::GetDeviceVendor(VendorATI), (GfxDeviceFamily*)GfxDriverInfo::GetDeviceFamily(AMDRadeonHD5800),
> -      nsIGfxInfo::FEATURE_HARDWARE_VIDEO_DECODING, nsIGfxInfo::FEATURE_BLOCKED_DEVICE,
> -      DRIVER_LESS_THAN, GfxDriverInfo::allDriverVersions);
> -

Is this the right entry? You talked about Intel cards.
(Assignee)

Comment 2

3 years ago
(In reply to Jeff Muizelaar [:jrmuizel] from comment #1)

> Is this the right entry? You talked about Intel cards.

It certainly isn't. The comment matched though.

(In reply to Matt Woodrow (:mattwoodrow) from comment #0)
> Interestingly, D3D9 texture sharing on the same machine seems to work fine,
> so we could have DXVA video that way. We'd have to make that decision for
> the entire session though, before we know if the user is going to use video
> or not.

The more I think about it, the more attractive this option is.

Do we have anything to gain by sticking with D3D11 when we don't get D2D?
(Assignee)

Comment 3

3 years ago
Created attachment 8623427 [details] [diff] [review]
dxva-texture-sharing v2
Attachment #8623417 - Attachment is obsolete: true
Attachment #8623417 - Flags: review?(jmuizelaar)
Attachment #8623427 - Flags: review?(jmuizelaar)
(In reply to Matt Woodrow (:mattwoodrow) from comment #2)
> (In reply to Jeff Muizelaar [:jrmuizel] from comment #1)
> 
> Do we have anything to gain by sticking with D3D11 when we don't get D2D?

Bas thinks so, I don't remember what his reasons were though.
Attachment #8623427 - Flags: review?(jmuizelaar) → review+
https://hg.mozilla.org/mozilla-central/rev/41a555505600
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox41: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
I'd really be interested in having this uplifted.  I have a feeling it could help with a bunch of unexplained crashes.  Matt, thoughts?
Flags: needinfo?(matt.woodrow)
(In reply to Milan Sreckovic [:milan] from comment #7)
> I'd really be interested in having this uplifted.  I have a feeling it could
> help with a bunch of unexplained crashes.  Matt, thoughts?

I'm guessing Matt will say yes.
Comment on attachment 8623427 [details] [diff] [review]
dxva-texture-sharing v2

Approval Request Comment
[Feature/regressing bug #]: DXVA
[User impact if declined]: Crashes or black video
[Describe test coverage new/current, TreeHerder]: Tested locally
[Risks and why]: Could disable DXVA unnecessarily. Changes feature detection/blacklist which is sometimes scary.
Attachment #8623427 - Flags: approval-mozilla-beta?
Attachment #8623427 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 10

3 years ago
Correct, thanks Jeff.
Flags: needinfo?(matt.woodrow)
status-firefox39: --- → affected
status-firefox40: --- → affected
Comment on attachment 8623427 [details] [diff] [review]
dxva-texture-sharing v2

Potentially improve the stability of the graphic, taking it.
I guess this is a bit late for beta.
Attachment #8623427 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
I think this is still worth taking in beta, but I'll try to come up with a better justification.
This sounds promising to help with a long standing bad problem. Are you sure enough to risk that we may need to do an RC2 in the middle of next week  (and ready to fix this or back it out)?
Flags: needinfo?(jmuizelaar)
Looking at this more closely might suggests that it might be better to do a version of this that adds the check but doesn't remove the blacklist entry for beta. What do you think matt?
Flags: needinfo?(jmuizelaar) → needinfo?(matt.woodrow)
Depends on: 1176506
(Assignee)

Comment 16

3 years ago
Landing without the blacklist change is probably a good idea, and safer.

We'll need to uplift bug 1176506 too though.
Flags: needinfo?(matt.woodrow)
See Also: → bug 1178823
See Also: bug 1178823
Comment on attachment 8623427 [details] [diff] [review]
dxva-texture-sharing v2

Clearing the Beta nom as Beta is now 40, which has this fix. Please renom for release if you think this warrants a ride along in a 39 point release.
Attachment #8623427 - Flags: approval-mozilla-beta?
You need to log in before you can comment on or make changes to this bug.