Closed Bug 1176071 Opened 5 years ago Closed 4 years ago

crash in mozilla::MFTDecoder::Output(mozilla::RefPtr<T>*)

Categories

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

Unspecified
Windows NT
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla46
Tracking Status
firefox46 --- fixed

People

(Reporter: mats, Assigned: cpearce)

References

Details

(Keywords: crash, platform-parity)

Crash Data

Attachments

(1 file)

This bug was filed from the Socorro interface and is 
report bp-86f0c324-7159-44fc-915c-251c42150617.
=============================================================

~700 crashes in the past 28 days, all on Windows (naturally):

Operating System         Percentage     Number Of Crashes
Windows 8.1              62.29 %        436
Windows 8                19.29 %        135
Windows 7                14.71 %        103
Windows Unknown           3.71 %         26

v40/v41 seems overrepresented:

Product         Version        Percentage     Number Of Crashes
Firefox         38.0.5         49.57 %        347
Firefox         40.0a2         23.29 %        163
Firefox         41.0a1         11.57 %         81
Firefox         39.0b5          5.57 %         39
Firefox         38.0.6          1.71 %         12
...

I looked at a few stacks, they all crashed on a null-pointer here:
http://hg.mozilla.org/releases/mozilla-release/annotate/f6680de4071d/dom/media/fmp4/wmf/MFTDecoder.cpp#l251

Perhaps related to what bug 1167045 is trying to fix?
Perhaps we could add a null check there as a wallpaper?
Flags: needinfo?(karlt)
I came to the conclusion that the only real problems involving bug 1167045 
in the current use of MFTDecoder::Output() were from D3D11DXVA2Manager, but
that is not on the stack at bp-86f0c324-7159-44fc-915c-251c42150617, so I
think this is a separate issue.

This does look like another case of a misbehaving decoder failing to set
pSample and yet returning a successful HRESULT [1].

Changing MOZ_ASSERT(output.pSample) to an early return seems sensible.

I don't know whether there is a way to identify this decoder and choose
something different.

[1] https://msdn.microsoft.com/en-us/library/windows/desktop/ms697247%28v=vs.85%29.aspx
Flags: needinfo?(karlt)
Component: Audio/Video → Audio/Video: Playback
Crash Signature: [@ mozilla::MFTDecoder::Output(mozilla::RefPtr<T>*)] → [@ mozilla::MFTDecoder::Output(mozilla::RefPtr<T>*)] [@ mozilla::MFTDecoder::Output]
Duplicate of this bug: 1168237
The mozilla::MFTDecoder::Output signature still has a fairly high number of crashes.
Do we have someone who can look into this soon?
Flags: needinfo?(cpearce)
I'll look now.
Assignee: nobody → cpearce
Flags: needinfo?(cpearce)
Sometimes the Windows Media Foundation MFT video decoder reports that it will
provide memory for output video frames, and the decoder returns success, but it
doesn't output a valid video frame. So change our code to not assume that
output is always valid (to fix a null pointer dereference). We can't reproduce
this locally, so we don't know how to get a best behaviour here, so add
telemetry to figure out whether the decoder will right itself, or whether we
should just give up completely.

Review commit: https://reviewboard.mozilla.org/r/31349/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/31349/
Attachment #8709263 - Flags: review?(vladan.bugzilla)
Attachment #8709263 - Flags: review?(matt.woodrow)
Comment on attachment 8709263 [details]
MozReview Request: Bug 1176071 - Handle WMF MFTDecoder returning success but no output, with telemetry. r?mattwoodrow,r?vladan

https://reviewboard.mozilla.org/r/31349/#review28205

::: dom/media/platforms/wmf/WMFVideoMFTManager.cpp:606
(Diff revision 1)
> +        if (mNullOutputCount > 500) {

500 might be unecessarily large, that's more than 20 seconds of playback that would be skipped
Attachment #8709263 - Flags: review?(matt.woodrow) → review+
(In reply to Matt Woodrow (:mattwoodrow) from comment #8)
> 500 might be unecessarily large, that's more than 20 seconds of playback
> that would be skipped

Hmm.... 250 then? We need the threshold to be big enough that it's convincing that we won't recover.
(In reply to Chris Pearce (:cpearce) from comment #9)
> (In reply to Matt Woodrow (:mattwoodrow) from comment #8)
> > 500 might be unecessarily large, that's more than 20 seconds of playback
> > that would be skipped
> 
> Hmm.... 250 then? We need the threshold to be big enough that it's
> convincing that we won't recover.

I guess it depends on the length of the video... 250 videos over an hour documentary isn't much, but that many in a 30 second clip would be unwatchable.

I guess 250 is reasonable, adding 'drop rate' monitoring is probably over engineering.
Comment on attachment 8709263 [details]
MozReview Request: Bug 1176071 - Handle WMF MFTDecoder returning success but no output, with telemetry. r?mattwoodrow,r?vladan

https://reviewboard.mozilla.org/r/31349/#review28371

::: toolkit/components/telemetry/Histograms.json:6091
(Diff revision 1)
> +    "kind": "flag",

are you sure you want a flag histogram here (vs a count histogram)?
Attachment #8709263 - Flags: review?(vladan.bugzilla)
https://reviewboard.mozilla.org/r/31349/#review28371

> are you sure you want a flag histogram here (vs a count histogram)?

r+ otherwise
(In reply to Vladan Djeric (:vladan) -- please needinfo from comment #11)
> Comment on attachment 8709263 [details]
> MozReview Request: Bug 1176071 - Handle WMF MFTDecoder returning success but
> no output, with telemetry. r?mattwoodrow,r?vladan
> 
> https://reviewboard.mozilla.org/r/31349/#review28371
> 
> ::: toolkit/components/telemetry/Histograms.json:6091
> (Diff revision 1)
> > +    "kind": "flag",
> 
> are you sure you want a flag histogram here (vs a count histogram)?

Uh, yeah, using a count histogram sounds like what I want! Thanks.  :)
https://hg.mozilla.org/mozilla-central/rev/1f44b1b6fcaa
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
You need to log in before you can comment on or make changes to this bug.