Use pref to indicate which DLLs&versions make D3D11 crash, to fall back to D3D9

RESOLVED FIXED in Firefox 47

Status

()

Core
Audio/Video: Playback
P1
critical
RESOLVED FIXED
2 years ago
9 months ago

People

(Reporter: gerald, Assigned: gerald)

Tracking

(Depends on: 1 bug)

47 Branch
mozilla49
All
Windows
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox46 unaffected, firefox47 fixed, firefox48 fixed, firefox49 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(3 attachments)

(Assignee)

Description

2 years ago
Spawned from bug 1268905 comment #17 by Marco Castelluccio [:marco]
> With https://hg.mozilla.org/integration/mozilla-inbound/rev/db5cf62c53dd,
> we're blocking specific versions of those libraries (and I assume we're
> going to do the same in bug 1269204 and bug 1273406), but we don't know if
> other versions are affected and we might know only when D3D11 DXVA hits
> release.
> 
> What's the plan if other libraries are affected in release? Can we disable
> DXVA with a pref?

Good idea, Marco!

I will move the hard-coded list of DLLs&versions from dom/media/platforms/wmf/WMFVideoMFTManager.cpp to a pref.
(Assignee)

Updated

2 years ago
Severity: normal → critical
status-firefox46: --- → unaffected
status-firefox47: --- → affected
status-firefox48: --- → affected
status-firefox49: --- → affected
Priority: -- → P1
(Assignee)

Comment 1

2 years ago
Created attachment 8753691 [details]
MozReview Request: Bug 1273691 - Implement 'media.wmf.disable-d3d11-for-dlls' pref - r?cpearce

Format:
"dll1.dll: 1.2.3.4[, more versions...][; more dlls...]"
I.e., DLLs are separated by semicolons, a DLL and all its versions are
separated by a single colon, and versions are separated by commas.
Whitespace between names&numbers is ignored.

Seeding pref with blacklistings from bug 1268905, bug 1269204, and bug 1273406.

Review commit: https://reviewboard.mozilla.org/r/53454/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/53454/
Attachment #8753691 - Flags: review?(cpearce)
Comment on attachment 8753691 [details]
MozReview Request: Bug 1273691 - Implement 'media.wmf.disable-d3d11-for-dlls' pref - r?cpearce

https://reviewboard.mozilla.org/r/53454/#review50214

::: dom/media/platforms/wmf/WMFVideoMFTManager.cpp:165
(Diff revision 1)
>    NS_ASSERTION(NS_IsMainThread(), "Must be on main thread.");
> -  // Cache the result, so we only check DLLs once per browser run.
> -  static const BlacklistedD3D11DLL* sAlreadySearched = nullptr;
> -  if (sAlreadySearched) {
> -    // If we point at the last empty entry, there's no actual blacklisting.
> -    return sAlreadySearched->name ? sAlreadySearched : nullptr;
> +
> +  // Result is cached as long as pref doesn't change.
> +  static nsCString sBlacklistedDLL;
> +
> +  nsAdoptingCString blacklist =

It's a shame jya's MediaPref doesn't support strings yet. :(

::: dom/media/platforms/wmf/WMFVideoMFTManager.cpp:186
(Diff revision 1)
> +
> +  // media.wmf.disable-d3d11-for-dlls format: (whitespace is trimmed)
> +  // "dll1.dll: 1.2.3.4[, more versions...][; more dlls...]"
> +  nsTArray<nsCString> dlls;
> +  SplitAt(";", blacklist, dlls);
> +  for (uint32_t dll = 0; dll < dlls.Length(); ++dll) {

You could use range-based for loops for some of the loops here.

::: dom/media/platforms/wmf/WMFVideoMFTManager.cpp:232
(Diff revision 1)
> +      }
> +      DWORD numbers[4];
> +      nsresult errorCode = NS_OK;
> +      for (int i = 0; i < 4; ++i) {
> +        numberStrings[i].CompressWhitespace();
> +         numbers[i] = DWORD(numberStrings[i].ToInteger(&errorCode));

Nit: indentation of this line is off by 1.
Attachment #8753691 - Flags: review?(cpearce) → review+
(Assignee)

Comment 3

2 years ago
https://reviewboard.mozilla.org/r/53454/#review50214

> It's a shame jya's MediaPref doesn't support strings yet. :(

Yes, I did look and look...
The 'Preferences' class doesn't have an AddStringVarCache, that's probably why.

> You could use range-based for loops for some of the loops here.

Oh, I didn't know nsTArray supported standard iteration interfaces, thanks.

> Nit: indentation of this line is off by 1.

Not sure how that one got there. Congrats, you've passed the visual acuity test.
(Assignee)

Comment 4

2 years ago
Comment on attachment 8753691 [details]
MozReview Request: Bug 1273691 - Implement 'media.wmf.disable-d3d11-for-dlls' pref - r?cpearce

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53454/diff/1-2/

Comment 5

2 years ago
maybe a way to blacklist all versions of a module below a particular version number would be handy as well...

Comment 6

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/510b8f1c5c36
Backed out for 8 bytes leak (nsStringBuffer) in mochitests on Windows 8 x64 debug:

https://hg.mozilla.org/integration/mozilla-inbound/rev/4c07990eb813

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=510b8f1c5c367f461a00cf9e7f016bfa826ea489
Flags: needinfo?(gsquelart)
(Assignee)

Comment 8

2 years ago
Comment on attachment 8753691 [details]
MozReview Request: Bug 1273691 - Implement 'media.wmf.disable-d3d11-for-dlls' pref - r?cpearce

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53454/diff/2-3/
(Assignee)

Comment 9

2 years ago
Comment on attachment 8753691 [details]
MozReview Request: Bug 1273691 - Implement 'media.wmf.disable-d3d11-for-dlls' pref - r?cpearce

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53454/diff/3-4/
(Assignee)

Comment 10

2 years ago
Comment on attachment 8753691 [details]
MozReview Request: Bug 1273691 - Implement 'media.wmf.disable-d3d11-for-dlls' pref - r?cpearce

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53454/diff/4-5/
(Assignee)

Comment 11

2 years ago
Comment on attachment 8753691 [details]
MozReview Request: Bug 1273691 - Implement 'media.wmf.disable-d3d11-for-dlls' pref - r?cpearce

Chris, could you please review this update?
https://reviewboard.mozilla.org/r/53454/diff/2-5/

The main difference is that the static strings used to cache the pref & blacklisting result are now stored in a container that is destroyed at the end of the browser run, to avoid leaks.
Flags: needinfo?(gsquelart)
Attachment #8753691 - Flags: review+ → review?(cpearce)
Attachment #8753691 - Flags: review?(cpearce) → review+
Comment on attachment 8753691 [details]
MozReview Request: Bug 1273691 - Implement 'media.wmf.disable-d3d11-for-dlls' pref - r?cpearce

https://reviewboard.mozilla.org/r/53454/#review50492

::: dom/media/platforms/wmf/WMFVideoMFTManager.cpp:168
(Diff revision 5)
>  };
> +StaticAutoPtr<D3D11BlacklistingCache> sD3D11BlacklistingCache;
>  
> -// If a blacklisted DLL is found, return its information, otherwise nullptr.
> -static const BlacklistedD3D11DLL*
> +// If a blacklisted DLL is found, return its information, otherwise "".
> +static const nsACString&
>  IsD3D11DLLBlacklisted()

Weird to have a function names IsSomething() that doesn't return a bool...
(Assignee)

Comment 13

2 years ago
Comment on attachment 8753691 [details]
MozReview Request: Bug 1273691 - Implement 'media.wmf.disable-d3d11-for-dlls' pref - r?cpearce

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53454/diff/5-6/
(Assignee)

Comment 14

2 years ago
https://reviewboard.mozilla.org/r/53454/#review50492

> Weird to have a function names IsSomething() that doesn't return a bool...

Good catch, thanks. Renamed it 'FindD3D11BlacklistedDLL()'.

Comment 15

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/204bdc8b929c
(Assignee)

Updated

2 years ago
Blocks: 1274115
(Assignee)

Updated

2 years ago
Blocks: 1274127
(Assignee)

Updated

2 years ago
Blocks: 1274132

Comment 16

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/204bdc8b929c
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox49: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
(Assignee)

Comment 17

2 years ago
Created attachment 8754583 [details] [diff] [review]
1273691-aurora.patch

Approval Request Comment
[Feature/regressing bug #]: D3D11 crashes in 47+
[User impact if declined]: ~1000 crashes per week in beta 47.
[Describe test coverage new/current, TreeHerder]: Media tests in Aurora try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=286387211a0fabc7af0d9a65924a184c49fdb9e1
[Risks and why]: A bit of risk as it's quite a bit of new code to parse the pref, but I believe it's fairly simple&safe (splitting strings, handling failure cases). Some risk in disabling too many DLLs, but we're falling back to proven D3D9, so it's not so bad.
[String/UUID change made/needed]: None.

Note that this patch includes the fixes for bug 1268905, bug 1269204, bug 1273406 (D3D11 crashes), so they'll get fixed automatically without having to uplift them.
Attachment #8754583 - Flags: review+
Attachment #8754583 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 18

2 years ago
Created attachment 8754584 [details] [diff] [review]
1273691-beta.patch

Approval Request Comment
[Feature/regressing bug #]: D3D11 crashes in 47+
[User impact if declined]: ~1000 crashes per week in beta 47.
[Describe test coverage new/current, TreeHerder]: Media tests in Aurora try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=286387211a0fabc7af0d9a65924a184c49fdb9e1
[Risks and why]: A bit of risk as it's quite a bit of new code to parse the pref, but I believe it's fairly simple&safe (splitting strings, handling failure cases). Some risk in disabling too many DLLs, but we're falling back to proven D3D9, so it's not so bad.
[String/UUID change made/needed]: None.

This patch *requires* bug 1257028 to be uplifted first, see bug 1257028 comment 4.

Note that this patch includes the fixes for bug 1268905, bug 1269204, bug 1273406 (D3D11 crashes), so they'll get fixed automatically without having to uplift them.
Attachment #8754584 - Flags: review+
Attachment #8754584 - Flags: approval-mozilla-beta?
(Assignee)

Comment 19

2 years ago
Comment on attachment 8754584 [details] [diff] [review]
1273691-beta.patch

Beta try for bug 1257028 and this one: https://treeherder.mozilla.org/#/jobs?repo=try&revision=16097bdb480c1793da143a00610a3d2584c6ffbb
Comment on attachment 8754583 [details] [diff] [review]
1273691-aurora.patch

Let's uplift to Aurora48 and stabilize before uplifting to Beta47.
Attachment #8754583 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Comment 21

2 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/fc50dd4bc0f9
status-firefox48: affected → fixed
Comment on attachment 8754584 [details] [diff] [review]
1273691-beta.patch

Disable D3D11 for some device drivers, this code stabilized on Aurora over the weekend, Beta47+
Attachment #8754584 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Comment 23

2 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/1edc2b60b9f7
status-firefox47: affected → fixed

Updated

2 years ago
Blocks: 1275739

Updated

9 months ago
Depends on: 1346765
You need to log in before you can comment on or make changes to this bug.