Closed
Bug 1147258
Opened 9 years ago
Closed 9 years ago
MSE playback crashed in GonkMediaDataDecoder
Categories
(Core :: Audio/Video, defect)
Tracking
()
RESOLVED
FIXED
mozilla40
Tracking | Status | |
---|---|---|
firefox40 | --- | fixed |
People
(Reporter: jwwang, Assigned: bwu)
References
Details
Attachments
(2 files, 2 obsolete files)
6.15 KB,
patch
|
Details | Diff | Splinter Review | |
1.48 KB,
patch
|
bwu
:
review+
|
Details | Diff | Splinter Review |
1. apply the patch to Central 2. have a debug build of emulator-kk with Central 3. run tests with |./mach mochitest-remote dom/media/test/test_mse_playback.html| 4. open |adb logcat| 5. hit the assertion: F/MOZ_Assert( 709): Assertion failure: !mManager->HasQueuedSample(), at ../../../../../../mozilla-central2/dom/media/fmp4/gonk/GonkMediaDataDecoder.cpp:205
Reporter | ||
Comment 1•9 years ago
|
||
Hi Blake, Can you check this bug?
Assignee: nobody → bwu
Blocks: 1142899
Assignee | ||
Comment 2•9 years ago
|
||
(In reply to JW Wang [:jwwang] from comment #1) > Hi Blake, > Can you check this bug? Sure!
Reporter | ||
Comment 3•9 years ago
|
||
(In reply to Blake Wu [:bwu][:blakewu] from comment #2) > Sure! Thanks!
Assignee | ||
Comment 4•9 years ago
|
||
The rootcause is the assert [1] MOZ_ASSERT_IF(mSignaledEOS, !mManager->HasQueuedSample()); There exists a chance that mSignaledEOS is trure and mManager->HasQueuedSample() is true as well which is normal case since mSignaledEOS means "no more input data from demuxer" but doesn't mean there is no data queued waiting to be feed to decoder. Therefore there is no reason to have an assert to check it here. [1]https://dxr.mozilla.org/mozilla-central/source/dom/media/fmp4/gonk/GonkMediaDataDecoder.cpp?from=GonkMediaDataDecoder.cpp&case=true#205
Comment 6•9 years ago
|
||
Comment on attachment 8584263 [details] [diff] [review] Bug-1147258-Remove-unnecessary-assertion-to-check-EOS-SampleQueued.patch Review of attachment 8584263 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/media/fmp4/gonk/GonkMediaDataDecoder.cpp @@ -202,5 @@ > } > } > > - MOZ_ASSERT_IF(mSignaledEOS, !mManager->HasQueuedSample()); > - The input queue should be empty when EOS signal is on except for nullptr EOS sample. I think we should make sure the queue is empty at this point.
Attachment #8584263 -
Flags: feedback?(ayang)
Assignee | ||
Comment 7•9 years ago
|
||
(In reply to Alfredo Yang from comment #6) > Comment on attachment 8584263 [details] [diff] [review] > Bug-1147258-Remove-unnecessary-assertion-to-check-EOS-SampleQueued.patch > > Review of attachment 8584263 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: dom/media/fmp4/gonk/GonkMediaDataDecoder.cpp > @@ -202,5 @@ > > } > > } > > > > - MOZ_ASSERT_IF(mSignaledEOS, !mManager->HasQueuedSample()); > > - > > The input queue should be empty when EOS signal is on except for nullptr EOS > sample. I think we should make sure the queue is empty at this point. Thanks for your feedback! Since this assertion is in ProcessOutput(), if we really want to make sure no any sample queued when mSignaledEOS is ture in this function, it will require ProcessOutput to wait all queued sample to be sent to decoder successfully which may block some available decoded frames to be rendered. I think we could move this assertion to [1] before clearing mSignaledEOS to double check there is no queued encoded sample when getting the last decoded frame. [1]https://dxr.mozilla.org/mozilla-central/source/dom/media/fmp4/gonk/GonkMediaDataDecoder.cpp?from=GonkMediaDataDecoder.cpp&case=true#219
Assignee | ||
Comment 8•9 years ago
|
||
Per comment 7.
Attachment #8584263 -
Attachment is obsolete: true
Attachment #8584416 -
Flags: review?(ajones)
Updated•9 years ago
|
Attachment #8584416 -
Flags: review?(ajones) → review+
Assignee | ||
Comment 9•9 years ago
|
||
Carry r+ from ajones.
Attachment #8584416 -
Attachment is obsolete: true
Attachment #8585907 -
Flags: review+
Assignee | ||
Comment 10•9 years ago
|
||
TryServer: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f221e6658469 Testing results are good.
Status: NEW → ASSIGNED
Keywords: checkin-needed
Assignee | ||
Comment 11•9 years ago
|
||
After applying this patch and running the test case as comment 0, assertion will not happen, but hit the decoder error problem as jwang told me earlier. I think it should be the same problem bug 1132832 is trying to fix. I will try the patch in bug 1132832 and if it still happens, a following bug will be fired.
Comment 12•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/f2a5d22b1bbf
Keywords: checkin-needed
Comment 13•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/f2a5d22b1bbf
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox40:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
You need to log in
before you can comment on or make changes to this bug.
Description
•