Closed Bug 1273691 Opened 4 years ago Closed 4 years ago

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

Categories

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

47 Branch
All
Windows
defect

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox46 --- unaffected
firefox47 --- fixed
firefox48 --- fixed
firefox49 --- fixed

People

(Reporter: gerald, Assigned: gerald)

References

Details

Attachments

(3 files)

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.
Severity: normal → critical
Priority: -- → P1
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+
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.
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/
maybe a way to blacklist all versions of a module below a particular version number would be handy as well...
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/
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/
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/
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...
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/
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()'.
Blocks: 1274115
Blocks: 1274127
Blocks: 1274132
https://hg.mozilla.org/mozilla-central/rev/204bdc8b929c
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
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?
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?
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 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+
Blocks: 1275739
Depends on: 1346765
You need to log in before you can comment on or make changes to this bug.