Closed
Bug 1278228
Opened 9 years ago
Closed 9 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•9 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•9 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•9 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•9 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•9 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•9 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•9 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox50:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Assignee | ||
Comment 8•9 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•9 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•9 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•9 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•9 years ago
|
Attachment #8760617 -
Attachment is obsolete: true
Comment 12•9 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•9 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•9 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•9 years ago
|
||
Try looks good: https://treeherder.mozilla.org/#/jobs?repo=try&revision=60712b1cffba
Thanks for the review.
Keywords: checkin-needed,
regression
Comment 16•9 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•9 years ago
|
Priority: -- → P1
Comment 17•9 years ago
|
||
bugherder |
Status: REOPENED → RESOLVED
Closed: 9 years ago → 9 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 18•9 years ago
|
||
Remind myself to check the crash reports and uplift to 49.
Flags: needinfo?(kaku)
Assignee | ||
Comment 19•9 years ago
|
||
No more crash is reported after landed this patch. Will uplift this patch to 49.
Flags: needinfo?(kaku)
Assignee | ||
Comment 20•9 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 21•9 years ago
|
||
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+
Updated•9 years ago
|
status-firefox48:
--- → unaffected
Comment 22•9 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•9 years ago
|
||
Carsten, here you are, thanks for helping uplifting.
Comment 25•9 years ago
|
||
bugherder uplift |
Comment 26•9 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
•