Closed
Bug 1175366
Opened 9 years ago
Closed 9 years ago
Don't use DXVA if texture sharing is broken
Categories
(Core :: Graphics: Layers, defect)
Tracking
()
RESOLVED
FIXED
mozilla41
People
(Reporter: mattwoodrow, Assigned: mattwoodrow)
References
Details
Attachments
(1 file, 1 obsolete file)
8.82 KB,
patch
|
jrmuizel
:
review+
Sylvestre
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
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 1•9 years ago
|
||
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•9 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•9 years ago
|
||
Attachment #8623417 -
Attachment is obsolete: true
Attachment #8623417 -
Flags: review?(jmuizelaar)
Attachment #8623427 -
Flags: review?(jmuizelaar)
Comment 4•9 years ago
|
||
(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.
Updated•9 years ago
|
Attachment #8623427 -
Flags: review?(jmuizelaar) → review+
Comment 6•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/41a555505600
Status: NEW → RESOLVED
Closed: 9 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)
Comment 8•9 years ago
|
||
(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 9•9 years ago
|
||
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?
Updated•9 years ago
|
status-firefox39:
--- → affected
status-firefox40:
--- → affected
Comment 11•9 years ago
|
||
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+
Comment 12•9 years ago
|
||
I think this is still worth taking in beta, but I'll try to come up with a better justification.
Comment 14•9 years ago
|
||
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)
Comment 15•9 years ago
|
||
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)
Assignee | ||
Comment 16•9 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)
Comment 17•9 years ago
|
||
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.
Description
•