1157764 - Video (presumably accelerated) causes a large increase in TDRs

Status: RESOLVED INCOMPLETE

(Core :: Audio/Video, defect)

Description

[Bas Schouten (:bas.schouten)]

There's a lot of information that suggests video causes an increase in TDRs on some hardware with some fairly recent drivers (drivers on which D3D11 and D2D seem to be just fine!). We should start figuring out which drivers those are and start blacklisting DXVA on them to see if that improves the situation. Not only are we not great at recovering from TDRs, some other software is quite sensitive to it as well, and I've seen it lead to BSODs too. We should keep causing them down to a minimum.

One example of a bug with a user explicitly complaining about TDRs in Video where neither D3D11 or D2D are blacklisted:

https://crash-stats.mozilla.com/report/index/5e2e363f-20d8-4195-ae5e-b7fa22150423

Comment 1

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

Anthony, is this actionable, should we track it?

Comment 2

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

(In reply to Milan Sreckovic [:milan] from comment #1)
> Anthony, is this actionable, should we track it?

I don't have any objection to aggressively or even speculatively blacklisting drivers for DXVA. Can you just go ahead and do that or do you want some involvement from me?

Comment 3

[Matt Woodrow (:mattwoodrow)]

That sounds fine with me too, DXVA isn't *that* important, so we should blacklist anything that looks risky.

Comment 4

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

Should we go to the other extreme and whitelist instead?

Comment 5

[Bas Schouten (:bas.schouten)]

So for getting data on how much this improves crashiness, the pref we're talking about here is media.windows-media-foundation.use-dxva, is that correct media guys? :)

Comment 6

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

I'm going to put up a patch to disable DXVA by disabling this pref.  According to offline conversations, media+graphics and some user accounts are telling us DXVA is causing a lot of TDRs and we just can't recover from them.

Once up, I'm going to ask for the 38 and 39 uplift, and get it done today.

Comment 7

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

Attachment: Nightly patch only. Disable dxva by default. r=bschouten

Approval Request Comment
[Feature/regressing bug #]:
[User impact if declined]:
[Describe test coverage new/current, TreeHerder]:
[Risks and why]: 
[String/UUID change made/needed]:

A lot of TDR crashes.  DXVA seems to be correlated with getting them.

Comment 8

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8598720 [details] [diff] [review]
Nightly patch only.  Disable dxva by default. r=bschouten

[Triage Comment]
Should be in 38 beta 9.

Comment 9

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

Attachment: Aurora patch. Disable dxva by default. r=bschouten

Comment 10

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

Attachment: Beta patch. Disable dxva by default. r=bschouten

Comment 11

[Matt Woodrow (:mattwoodrow)]

I don't love this approach, since it seems like a dead end. How are we going to get DXVA enabled again, or is this intended are a temporary measure?

A whitelist sounds nice, but I don't see any easy way to populate it. We don't really have the resources to manually stress test all the configurations in use by our users.

We also have evidence that disabling DXVA can *increase* the number of crashes, see bug 1146313.

I'd much prefer if we just aggressively blacklist driver versions that crash a lot of youtube URLs, and then blacklist layers as well if they continue to crash.

Comment 12

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Comment on attachment 8598720 [details] [diff] [review]
Nightly patch only.  Disable dxva by default. r=bschouten

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

I can't let this happen. I'm OK with disabling DXVA drivers/hardware that is suspected of causing a problem, but we simply can't just disable it wholesale.

Comment 13

[Benjamin Smedberg]

Why not? Are you saying that the non-DXVA codepath is perceivably worse for users than DXVA?

Comment 14

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

Asking for tracking as this blocks a tracked graphics bug 1116812.

Comment 15

[Chris Pearce [:cpearce (Not reading bugmail)]]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #13)
> Why not? Are you saying that the non-DXVA codepath is perceivably worse for
> users than DXVA?

Yes. Some machines, notably older ones and those with some Atom CPUs, can't smoothly play videos with resolutions greater than 480p without hardware decoding. Battery drain is pretty bad without GPU decoding too.

Comment 16

[Sylvestre Ledru [:Sylvestre]]

Too bad we are not taking this change...
bug 1116812 is impacting the 38 release (it is the most important source of crash).

Comment 17

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

(In reply to Sylvestre Ledru [:sylvestre] from comment #16)
> Too bad we are not taking this change...
> bug 1116812 is impacting the 38 release (it is the most important source of
> crash).

It is also possible this is unrelated, and that DXVA off does not reduce the number of driver resets. We don't actually know, so this change was going to be speculative at best. The change is in A/V area, even thought the code is in gfx/, so Anthony makes a call (we don't seem to track a "video" module owner.)  See comment 12 and comment 15.

Comment 18

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

Comment on attachment 8598741 [details] [diff] [review]
Aurora patch.  Disable dxva by default. r=bschouten

Removing Bas' r+, as we got an r- on the original patch.

Comment 19

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

Comment on attachment 8598742 [details] [diff] [review]
Beta patch.  Disable dxva by default. r=bschouten

Removing Bas' r+, as we got an r- on the original patch.

Comment 20

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

Comment on attachment 8598720 [details] [diff] [review]
Nightly patch only.  Disable dxva by default. r=bschouten

Removing sledru's aurora(39) and beta (38) approval, because of an r-

Comment 21

[Bas Schouten (:bas.schouten)]

I still think we should land this on nightly for a couple of days to get data on how much of the TDRs this is responsible for.

Comment 22

[Robert Kaiser]

(In reply to Bas Schouten (:bas.schouten) from comment #21)
> I still think we should land this on nightly for a couple of days to get
> data on how much of the TDRs this is responsible for.

If you will collect the data from that and make it comparable, sure. I don't trust that I'd be able to make enough heads or tails from the low volume and noise from random checkins that we see on nightly.

Comment 23

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

I'm OK with testing it in nightly. It will cause a fairly serious regression for people with old machines (less common in nightly) but it is worth the pain.

Comment 24

[(Away)]

Don't we keep getting bit by the fact that the beta (and even release) population have hardware/driver combinations that we didn't see on earlier channels?

Comment 25

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

We decided we're going to run the "experiment" instead.  Bas & Smedberg will sort it out.

Comment 26

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Tracking for Firefox41. n-i to Bas. Do we have a fix for Firefox41? There hasn't been any activity on the bug in 2-3 weeks.

Comment 27

[Bas Schouten (:bas.schouten)]

(In reply to Ritu Kothari (:ritu) from comment #26)
> Tracking for Firefox41. n-i to Bas. Do we have a fix for Firefox41? There
> hasn't been any activity on the bug in 2-3 weeks.

I need to talk to Benjamin and get the experiment sorted out, I will do so somewhere this week.

Comment 28

[Sylvestre Ledru [:Sylvestre]]

Too late for 38.0.5 but happy to take a fix for 39.

Comment 29

[Liz Henry (:lizzard) (privacy/fingerprinting team)]

Milan is this a priority at this point?  It sounds like a pretty bad problem, on the other hand I know we have others we're addressing. 

Benjamin is this part of the early startup work you were mentioning lately?

Comment 30

[Benjamin Smedberg]

I'll let Milan comment.

Comment 31

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

I'm starting to think this is not and won't ever be actionable.  Whatever experiments we may do, it won't be under this bug.  Please reopen if you disagree...

Comment 32

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Setting FF40 and FF41 as won't fix on this one based on comment 31.