Closed Bug 1313883 Opened 3 years ago Closed 3 years ago

Intermittent crashes in nvwgf2umx.dll during media playback tests on Win64

Categories

(Core :: Graphics: Layers, defect)

x86_64
Windows
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
firefox50 --- unaffected
firefox51 --- fixed
firefox52 --- fixed

People

(Reporter: RyanVM, Assigned: mattwoodrow)

References

Details

(Keywords: crash, intermittent-failure)

Attachments

(1 file)

This is sad, we made this change (after a recommendation from AMD) to avoid crashes. However, it looks like it actually made things worse on both Intel (bug 1292923) and NVIDIA (this one).

I guess we should try reverting this change, and supporting both configurations (single device for all threads, as well as one device per thread) and choose which one to take based on the driver vendor.

Does that sound reasonable you Jeff? Bas?
Flags: needinfo?(matt.woodrow)
Flags: needinfo?(jmuizelaar)
Flags: needinfo?(bas)
We discussed having a preference for this in bug 1295075 (Ryan started a patch in that bug with gfx.direct3d11.reuse-decoder-device, but we didn't get far with it.)  This would be a way to override the default behaviour, but we could easily have a vendor specific bias to go on top of the preference (vendor default, force on, force off.)  We can then run the A/B test and see what can be associated with each of these values.
Assignee: nobody → matt.woodrow
I did a try push to confirm that the decoder devices were the problem:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=aad588202486

This makes the gfx.direct3d11.reuse-decoder-device a tri-state pref.

< 0 is the default, shares devices on AMD, creates new ones elsewhere.
0 disables reuse, always creates new devices.
> 0 forces reuse (when possible), always tries to use existing devices.
Attachment #8806569 - Flags: review?(dvander)
Comment on attachment 8806569 [details] [diff] [review]
Only reuse devices on AMD

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

::: gfx/thebes/DeviceManagerDx.cpp
@@ +521,5 @@
> +  }
> +
> +  if (reuseDevice) {
> +    if (mCompositorDevice && mCompositorDeviceSupportsVideo && !mDecoderDevice) {
> +      mDecoderDevice = mCompositorDevice;

I was wondering why this assignment occurs outside of the mDeviceLock - it looks like it's safe because mDecoderDevice is not reset on driver resets. We should probably fix both of those things.
Attachment #8806569 - Flags: review?(dvander) → review+
(In reply to Matt Woodrow (:mattwoodrow) from comment #1)
> This is sad, we made this change (after a recommendation from AMD) to avoid
> crashes. However, it looks like it actually made things worse on both Intel
> (bug 1292923) and NVIDIA (this one).
> 
> I guess we should try reverting this change, and supporting both
> configurations (single device for all threads, as well as one device per
> thread) and choose which one to take based on the driver vendor.
> 
> Does that sound reasonable you Jeff? Bas?

It does sound reasonable.
Flags: needinfo?(bas)
Pushed by mwoodrow@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4e6ed6748067
Allow using multiple decoder devices on non-AMD hardware since they seem to crash less that way. r=dvander
https://hg.mozilla.org/mozilla-central/rev/4e6ed6748067
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
None of the dependent bugs have been hit since this landed. Please request Beta approval on this when you get a chance.
Flags: needinfo?(matt.woodrow)
No longer blocks: 1289640
Hi Astely, 
Can you help find someone to create uplift request to Beta51?
Flags: needinfo?(aschen)
Leave ni? I'd prefer to let Matt work on the uplift request.
Flags: needinfo?(aschen)
Flags: needinfo?(aschen)
Comment on attachment 8806569 [details] [diff] [review]
Only reuse devices on AMD

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1284672.
[User impact if declined]: More frequent crashes in automation, likely for real users too.
[Is this code covered by automated tests?]: Yes.
[Has the fix been verified in Nightly?]: Yes.
[Needs manual test from QE? If yes, steps to reproduce]: No, changes in automation crashes verify the fix.
[List of other uplifts needed for the feature/fix]: None.
[Is the change risky?]: No. 
[Why is the change risky/not risky?]: Just reverts to our existing behaviour for affected drivers.
[String changes made/needed]: None.
Flags: needinfo?(matt.woodrow)
Attachment #8806569 - Flags: approval-mozilla-beta?
Flags: needinfo?(jmuizelaar)
Comment on attachment 8806569 [details] [diff] [review]
Only reuse devices on AMD

Reduce the risk of crash. Beta51+. Should be in 51 beta 6.
Attachment #8806569 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Flags: needinfo?(aschen)
Duplicate of this bug: 1310506
Duplicate of this bug: 1306845
Duplicate of this bug: 1303971
Duplicate of this bug: 1303498
Duplicate of this bug: 1300664
Duplicate of this bug: 1300613
Duplicate of this bug: 1300344
Duplicate of this bug: 1298306
Duplicate of this bug: 1298301
Duplicate of this bug: 1295464
Duplicate of this bug: 1294280
You need to log in before you can comment on or make changes to this bug.