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

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Ryan VanderMeulen [:RyanVM]]

For awhile now, there has been a spate of random crashes in nvwgf2umx.dll during various media playback tests (mostly in the web-platform media-source tests). I was finally able to track them down to the push for bug 1289640 and bug 1284672.

https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&filter-searchStr=win%20x64%20w(3)&group_state=expanded&tochange=b407b980351600f6708db179c9bdb3338bf24796&fromchange=577158be08e8c1211ffa1198a21e3c059f98f477

Matt, can you please take a look? They appear to be Win64 only. Affects both opt and debug builds. The below OrangeFactor link has logs for affected runs (may take a few minutes to load).

https://brasstacks.mozilla.com/orangefactor/?display=OrangeFactor&includefiltertype=quicksearch&includefilterdetailsexcludeResolved=false&includefilterdetailsexcludeDisabled=false&includefilterdetailsquicksearch=nvwgf2umx.dll&includefilterdetailsnumbugs=0&includefilterdetailsresolvedIds=&excludefiltertype=quicksearch&excludefilterdetailsquicksearch=&excludefilterdetailsnumbugs=0&excludefilterdetailsresolvedIds=&plat=All&test=All&type=All&startday=2016-10-01&tree=all

Comment 1

[Matt Woodrow (:mattwoodrow)]

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?

Comment 2

[Milan Sreckovic [:milan] (needinfo for best results)]

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.

Comment 3

[Matt Woodrow (:mattwoodrow)]

Attachment: Only reuse devices on AMD

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.

Comment 4

[David Anderson [:dvander] - inactive, e-mail if emergency]

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.

Comment 5

[Bas Schouten (:bas.schouten)]

(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.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
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

Comment 7

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/4e6ed6748067

Comment 8

[Ryan VanderMeulen [:RyanVM]]

None of the dependent bugs have been hit since this landed. Please request Beta approval on this when you get a chance.

Comment 9

[Gerry Chang [:gchang]]

Hi Astely, 
Can you help find someone to create uplift request to Beta51?

Comment 10

[Astley Chen (inactive)]

Leave ni? I'd prefer to let Matt work on the uplift request.

Comment 11

[Matt Woodrow (:mattwoodrow)]

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.

Comment 12

[Gerry Chang [:gchang]]

Comment on attachment 8806569 [details] [diff] [review]
Only reuse devices on AMD

Reduce the risk of crash. Beta51+. Should be in 51 beta 6.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/88ae43bdada9