Closed
Bug 1278228
Opened 7 years ago
Closed 7 years ago
Crash in mozilla::MediaDecoderStateMachine::InitiateSeek
Categories
(Core :: Audio/Video: Playback, defect, P1)
Tracking
()
RESOLVED
FIXED
mozilla50
Tracking | Status | |
---|---|---|
firefox48 | --- | unaffected |
firefox49 | --- | fixed |
firefox50 | --- | fixed |
People
(Reporter: marcia, Assigned: kaku)
Details
(Keywords: crash, regression)
Crash Data
Attachments
(2 files, 1 obsolete file)
58 bytes,
text/x-review-board-request
|
jwwang
:
review+
lizzard
:
approval-mozilla-aurora+
|
Details |
2.63 KB,
patch
|
Details | Diff | Splinter Review |
This bug was filed from the Socorro interface and is report bp-f24ae9fa-5889-49d7-b6bf-eba212160606. ============================================================= Seen while looking at crash-stats. http://bit.ly/1YaTiWF has the full set of crashes so far. Crashes begin on trunk with build ID 20160531030258 ni on kaku@mozilla.com since it appears that he worked in that part of the code.
Flags: needinfo?(kaku)
Assignee | ||
Comment 1•7 years ago
|
||
JW, Is it possible that a "AccurateVideoOnly" seek be queued into the MDSM::mQueuedSeek anyhow and then be resumed again later in the MDSM::StartDecoding() which calls MDSM::InitialSeek()? MDSM::InitialSeek() does not handle the "AccurateVideoOnly" seek type so that the MDSM::mSeekTask is a nullptr at the crashing point.
Assignee: nobody → kaku
Flags: needinfo?(kaku) → needinfo?(jwwang)
Comment 2•7 years ago
|
||
|AccurateVideoOnly| is used in InitiateVideoDecodeRecoverySeek() only which never touches mQueuedSeek. I can't see how this is possible.
Flags: needinfo?(jwwang)
Assignee | ||
Comment 3•7 years ago
|
||
Per discussion offline, we still suspect that it is an "AccurateVideoOnly" seek which does not handled in the MDSM::InitialSeek() so that the MDSM::mSeekTask is a nullptr at the crashing point. The possibility comes from that an "AccurateVideoOnly" is suspended while the MDSM is going to change to dormant state. We would like to land a patch with MOZ_DIAGNOSTIC_ASSERT() to confirm our guess first. Patch is following.
Assignee | ||
Comment 4•7 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/58156/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/58156/
Attachment #8760617 -
Flags: review?(jwwang)
Comment 5•7 years ago
|
||
Comment on attachment 8760617 [details] Bug 1278228 - use MOZ_DIAGNOSTIC_ASSERT() to confirm that AccurateVideoOnly seek might reach MDSM::InitiateSeek(); https://reviewboard.mozilla.org/r/58156/#review55012
Attachment #8760617 -
Flags: review?(jwwang) → review+
Assignee | ||
Updated•7 years ago
|
Keywords: checkin-needed
Pushed by cbook@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/48b1777f5fe3 use MOZ_DIAGNOSTIC_ASSERT() to confirm that AccurateVideoOnly seek might reach MDSM::InitiateSeek(); r=jwwang
Keywords: checkin-needed
Comment 7•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/48b1777f5fe3
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox50:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Assignee | ||
Comment 8•7 years ago
|
||
New crash reports crashed at the MOZ_DIAGNOSTIC_ASSERT(). So, our guess is confirmed that "AccurateVideoOnly" seek should be handled by the MDSM::InitialSeek() method. Reopen this issue and handle it.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Comment 9•7 years ago
|
||
The MDSM::InitiateSeek() is only called in two cases: (1) In MDSM::Seek(): pass a new seek task which could only be AccurateSeek, FastSeek or NextFrameSeek. (2) In MDSM::MaybeFinishDecodeFirstFrame() and MDSM::StartDecoding(): the argument is the MDSM::mQueuedSeek which could by any types include the "AccurateVideoOnly". And, the MDSM::mQueuedSeek is only initialized in two cases: (1) In MDSM::Seek(); this would never be the "AccurateVideoOnly" type. (2) In MDSM::SetDormant(); this could be anything again includes the "AccurateVideoOnly". So, the problem we faced here is that a "AccurateVideoOnly" type seek task was queued in the MDSM::SetDormant() and then be resumed later in the MDSM::MaybeFinishDecodeFirstFrame() or MDSM::StartDecoding() which pass the queued seek task as an argument into the MDSM::InitiateSeek(). @JW, there are two possible solutions: (1) As you suggested, treat the "AccurateVideoOnly" as normal "AccurateSeek" and handle it in the MDSM::InitiateSeek(). This is the easier one. (2) While a "AccurateVideoOnly" seek task is going to be queued, we don't queue it as an "AccurateVideoOnly" but queue it as an "AccurateSeek" instead. I think this solution is more explicit and keep the MDSM::InitiateSeek() away from "AccurateVideoOnly" type. WDYT?
Flags: needinfo?(jwwang)
Comment 10•7 years ago
|
||
IIUC, the problem is that SetDormant() is called before an AccurateVideoOnly seek is completed and therefore AccurateVideoOnly is stored in |mQueuedSeek|, right? I think we should always do an AccurateSeek after waking up from dormant state because we reset both audio and video decoders when entering the dormant state.
Flags: needinfo?(jwwang)
Assignee | ||
Comment 11•7 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/59900/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/59900/
Attachment #8763750 -
Flags: review?(jwwang)
Assignee | ||
Updated•7 years ago
|
Attachment #8760617 -
Attachment is obsolete: true
Comment 12•7 years ago
|
||
Comment on attachment 8763750 [details] Bug 1278228 - handle VideoOnly seek as an accurate seek; https://reviewboard.mozilla.org/r/59900/#review56836 I think we should store SeekTarget with mType==Accurate into mQueuedSeek instead in SetDormant() to prevet MediaFormatReader from doing something interesting when it sees SeekTarget::IsVideoOnly() is true.
Attachment #8763750 -
Flags: review?(jwwang) → review-
Assignee | ||
Comment 13•7 years ago
|
||
Comment on attachment 8763750 [details] Bug 1278228 - handle VideoOnly seek as an accurate seek; Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59900/diff/1-2/
Attachment #8763750 -
Flags: review- → review?(jwwang)
Comment 14•7 years ago
|
||
Comment on attachment 8763750 [details] Bug 1278228 - handle VideoOnly seek as an accurate seek; https://reviewboard.mozilla.org/r/59900/#review56842
Attachment #8763750 -
Flags: review?(jwwang) → review+
Assignee | ||
Comment 15•7 years ago
|
||
Try looks good: https://treeherder.mozilla.org/#/jobs?repo=try&revision=60712b1cffba Thanks for the review.
Keywords: checkin-needed,
regression
Comment 16•7 years ago
|
||
Pushed by cbook@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/b486db198015 handle VideoOnly seek as an accurate seek; r=jwwang
Keywords: checkin-needed
Updated•7 years ago
|
Priority: -- → P1
Comment 17•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/b486db198015
Status: REOPENED → RESOLVED
Closed: 7 years ago → 7 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 18•7 years ago
|
||
Remind myself to check the crash reports and uplift to 49.
Flags: needinfo?(kaku)
Assignee | ||
Comment 19•7 years ago
|
||
No more crash is reported after landed this patch. Will uplift this patch to 49.
Flags: needinfo?(kaku)
Assignee | ||
Comment 20•7 years ago
|
||
Comment on attachment 8763750 [details] Bug 1278228 - handle VideoOnly seek as an accurate seek; Approval Request Comment [Feature/regressing bug #]: bug 1235301 [User impact if declined]: crash. see the crash report in the comment 1. [Describe test coverage new/current, TreeHerder]: No more crash is reported after this patch landed. http://bit.ly/1YaTiWF [Risks and why]: Uplifting this patch have few risk. [String/UUID change made/needed]: None.
Attachment #8763750 -
Flags: approval-mozilla-aurora?
Comment on attachment 8763750 [details] Bug 1278228 - handle VideoOnly seek as an accurate seek; Crash fix, worked on nightly, let's uplift it! Thanks!
Attachment #8763750 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
status-firefox48:
--- → unaffected
Comment 22•7 years ago
|
||
has problem applying to aurora: merging dom/media/MediaDecoderStateMachine.cpp warning: conflicts while merging dom/media/MediaDecoderStateMachine.cpp! can you take a look, thanks!
Flags: needinfo?(kaku)
Assignee | ||
Comment 24•7 years ago
|
||
Carsten, here you are, thanks for helping uplifting.
Comment 25•7 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-aurora/rev/2921bfa5a494
Comment 26•7 years ago
|
||
Crash volume for signature 'mozilla::MediaDecoderStateMachine::InitiateSeek': - nightly (version 50): 32 crashes from 2016-06-06. - aurora (version 49): 0 crashes from 2016-06-07. - beta (version 48): 0 crashes from 2016-06-06. - release (version 47): 1 crash from 2016-05-31. - esr (version 45): 0 crashes from 2016-04-07. Crash volume on the last weeks: W. N-1 W. N-2 W. N-3 W. N-4 W. N-5 W. N-6 W. N-7 - nightly 0 1 2 1 1 7 9 - aurora 0 0 0 0 0 0 0 - beta 0 0 0 0 0 0 0 - release 1 0 0 0 0 0 0 - esr 0 0 0 0 0 0 0 Affected platforms: Windows, Mac OS X, Linux
You need to log in
before you can comment on or make changes to this bug.
Description
•