Closed
Bug 1273691
Opened 9 years ago
Closed 8 years ago
Use pref to indicate which DLLs&versions make D3D11 crash, to fall back to D3D9
Categories
(Core :: Audio/Video: Playback, defect, P1)
Tracking
()
RESOLVED
FIXED
mozilla49
Tracking | Status | |
---|---|---|
firefox46 | --- | unaffected |
firefox47 | --- | fixed |
firefox48 | --- | fixed |
firefox49 | --- | fixed |
People
(Reporter: mozbugz, Assigned: mozbugz)
References
Details
Attachments
(3 files)
(deleted),
text/x-review-board-request
|
cpearce
:
review+
|
Details |
(deleted),
patch
|
mozbugz
:
review+
ritu
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
mozbugz
:
review+
ritu
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
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•9 years ago
|
Severity: normal → critical
status-firefox46:
--- → unaffected
status-firefox47:
--- → affected
status-firefox48:
--- → affected
status-firefox49:
--- → affected
Priority: -- → P1
Assignee | ||
Comment 1•8 years ago
|
||
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 2•8 years ago
|
||
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•8 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•8 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•8 years ago
|
||
maybe a way to blacklist all versions of a module below a particular version number would be handy as well...
Comment 7•8 years ago
|
||
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•8 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•8 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•8 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•8 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)
Updated•8 years ago
|
Attachment #8753691 -
Flags: review?(cpearce) → review+
Comment 12•8 years ago
|
||
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•8 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•8 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•8 years ago
|
||
Comment 16•8 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Assignee | ||
Comment 17•8 years ago
|
||
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•8 years ago
|
||
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•8 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•8 years ago
|
||
bugherder uplift |
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•8 years ago
|
||
bugherder uplift |
You need to log in
before you can comment on or make changes to this bug.
Description
•