Closed Bug 1147258 Opened 9 years ago Closed 9 years ago

MSE playback crashed in GonkMediaDataDecoder

Categories

(Core :: Audio/Video, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla40
Tracking Status
firefox40 --- fixed

People

(Reporter: jwwang, Assigned: bwu)

References

Details

Attachments

(2 files, 2 obsolete files)

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
Hi Blake,
Can you check this bug?
Assignee: nobody → bwu
Blocks: 1142899
(In reply to JW Wang [:jwwang] from comment #1)
> Hi Blake,
> Can you check this bug?
Sure!
(In reply to Blake Wu [:bwu][:blakewu] from comment #2)
> Sure!
Thanks!
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 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)
(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
Per comment 7.
Attachment #8584263 - Attachment is obsolete: true
Attachment #8584416 - Flags: review?(ajones)
Attachment #8584416 - Flags: review?(ajones) → review+
Carry r+ from ajones.
Attachment #8584416 - Attachment is obsolete: true
Attachment #8585907 - Flags: review+
TryServer: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f221e6658469 
Testing results are good.
Status: NEW → ASSIGNED
Keywords: checkin-needed
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.
https://hg.mozilla.org/mozilla-central/rev/f2a5d22b1bbf
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
Blocks: 1159119
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: