Closed
Bug 1147304
Opened 9 years ago
Closed 9 years ago
[FFOS] MP4 decoding doesn't resume from dormant state.
Categories
(Core :: Audio/Video, defect)
Tracking
()
RESOLVED
FIXED
mozilla40
Tracking | Status | |
---|---|---|
firefox40 | --- | fixed |
People
(Reporter: ayang, Assigned: bwu)
References
Details
Attachments
(1 file, 1 obsolete file)
2.47 KB,
patch
|
bwu
:
review+
|
Details | Diff | Splinter Review |
Play a mp4 video on browser. Back to home and resume playing the video, repeat it few times. Decoded frame from https://dxr.mozilla.org/mozilla-central/source/dom/media/fmp4/MP4Reader.cpp#903 doesn't go to ReturnOutput https://dxr.mozilla.org/mozilla-central/source/dom/media/fmp4/MP4Reader.cpp#729. And then it stops input mp4 sample to decoder.
Assignee | ||
Comment 1•9 years ago
|
||
Take this bug as first priority since this impacts MSE and mp4 local playback.
Assignee: nobody → bwu
OS: Mac OS X → Gonk (Firefox OS)
Hardware: x86 → ARM
Assignee | ||
Updated•9 years ago
|
Assignee | ||
Comment 2•9 years ago
|
||
This problem can be reproduced with Video APP as well.
Assignee | ||
Comment 3•9 years ago
|
||
What I currently hit most is video will not be played automatically via browser. The root cause is different from comment 0. I am checking this first. STR is below. 1. Go to http://people.mozilla.org/~bwu/ffos-720p-s.mp4. 2. Press the home key to leave. 3. Double click the home key to go back the previous browser. Actual result: Sometimes it will not play, even user presses the play button again. Expected result: It should play.
Assignee | ||
Comment 4•9 years ago
|
||
The root cause of the problem described in comment 3 is In step 2, mPlayingThroughTheAudioChannel and mAudioChannelAgent will be set *false* and null respectively due to mReadyState is set to HAVE_METADATA in UpdateReadyStateForData[1]. So when backing to resume, since mPlayingThroughTheAudioChannel is false, SetVisibilityChange will not be called in NotifyOwnerDocumentActivityChanged[2] and the state of mMuted is still MUTED_BY_AUDIO_CHANNEL. Therefore, playback will not be resumed to play. [1]https://dxr.mozilla.org/mozilla-central/source/dom/html/HTMLMediaElement.cpp?from=HTMLMediaElement.cpp&case=true#3534 [2]https://dxr.mozilla.org/mozilla-central/source/dom/html/HTMLMediaElement.cpp?from=HTMLMediaElement.cpp&case=true#3960
Assignee | ||
Comment 5•9 years ago
|
||
This problem described in comment 3 and 4 doesn't only happen in MP4Reader and it is similar to the bug 1139276. I am going to continue checking this problem in bug 1139276.
Assignee | ||
Comment 6•9 years ago
|
||
With the latest code base, sometimes video app cannot play when resuming, which is caused by GonkVideoDecoderManager's blocking in flush action[1]. It looks like omx decoder cannot finish flush. This could be a POVB. I am trying to dig more. [1]https://dxr.mozilla.org/mozilla-central/source/dom/media/fmp4/gonk/GonkVideoDecoderManager.cpp?from=GonkVideoDecoderManager.cpp&case=true#
Assignee | ||
Comment 7•9 years ago
|
||
(In reply to Blake Wu [:bwu][:blakewu] from comment #6) > With the latest code base, sometimes video app cannot play when resuming, > which is caused by GonkVideoDecoderManager's blocking in flush action[1]. It > looks like omx decoder cannot finish flush. > This could be a POVB. I am trying to dig more. > > [1]https://dxr.mozilla.org/mozilla-central/source/dom/media/fmp4/gonk/ > GonkVideoDecoderManager.cpp?from=GonkVideoDecoderManager.cpp&case=true# Correct the link which is pointed to mDecoder->flush(); https://dxr.mozilla.org/mozilla-central/source/dom/media/fmp4/gonk/GonkVideoDecoderManager.cpp?from=GonkVideoDecoderManager.cpp&case=true#484
Assignee | ||
Comment 8•9 years ago
|
||
The rootcause of the problem described in comment 6 and 7 is there exists a chance to hit the dead lock in send_codec_config() in omx_vdec_msm8974.cpp[1] (Flame) if client inputs a empty buffer for codec specific data to codec. With landed bug 1153876, GonkVideoDecoderManager will send out a empty buffer for codec specific data when the encoded data is h.264. Below is the detailed sequence to hit this problem. 1. ReadMetaData Under this state, codec will be reserved and get prepared. The patch in bug 1153876 will "input" (empty_this_buffer) codec specific data to decoder right after codec preparation [2]. 2. Seek Right after step 1 and during inputting codec specific data to decoder, a seek request from MediaDecoderStateMachine will trigger a flush command. Since the command's priority[3] is higher than empty_this_buffer, that input message will not be executed first, even it comes first. And this flush command will eventually call send_codec_config()[1] which will handle the unprocessed empty_this_buffer. A member mutex is locked first inside, so it makes the post_event() in empty_this_buffer_proxy keep waiting that lock. [1]https://www.codeaurora.org/cgit/quic/la/platform/hardware/qcom/media/tree/mm-video-v4l2/vidc/vdec/src/omx_vdec_msm8974.cpp#n10484 [2]https://dxr.mozilla.org/mozilla-central/source/dom/media/fmp4/gonk/GonkVideoDecoderManager.cpp?from=GonkVideoDecoderManager.cpp&case=true#508 [3]https://www.codeaurora.org/cgit/quic/la/platform/hardware/qcom/media/tree/mm-video-v4l2/vidc/vdec/src/omx_vdec_msm8974.cpp#n881
Assignee | ||
Comment 9•9 years ago
|
||
This patch is to send codec specific data for mpeg4 codec type only. Since for h.264 the length of mCodecSpecificData is 0, this empty data sometimes can cause omx_vdec dead lock. It looks omx_vdec is not robust. For more details, please refer to comment 8. Since omx_vdec is owned and designed by chipset partner, it may be different from partner to partner, it would be better to be fixed in gecko side.
Attachment #8598481 -
Flags: review?(jyavenard)
Comment 10•9 years ago
|
||
Comment on attachment 8598481 [details] [diff] [review] Bug-1147304-Send-codec-specific-data-for-MPEG4-codec.patch Review of attachment 8598481 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/media/fmp4/gonk/GonkVideoDecoderManager.cpp @@ +396,4 @@ > } > case -EAGAIN: > { > +// GVDM_LOG("Need to try again!"); remove or uncomment.
Attachment #8598481 -
Flags: review?(jyavenard) → review+
Assignee | ||
Comment 11•9 years ago
|
||
(In reply to Jean-Yves Avenard [:jya] from comment #10) > Comment on attachment 8598481 [details] [diff] [review] > Bug-1147304-Send-codec-specific-data-for-MPEG4-codec.patch > > Review of attachment 8598481 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: dom/media/fmp4/gonk/GonkVideoDecoderManager.cpp > @@ +396,4 @@ > > } > > case -EAGAIN: > > { > > +// GVDM_LOG("Need to try again!"); > > remove or uncomment. Thanks for your review! I keep the log since it is very handy to enable the log when doing performance check, but don't enable it in normal case to avoid too many logs.
Assignee | ||
Comment 12•9 years ago
|
||
Carry r+ from jya.
Attachment #8598481 -
Attachment is obsolete: true
Attachment #8599033 -
Flags: review+
Assignee | ||
Comment 13•9 years ago
|
||
Test results look good. https://treeherder.mozilla.org/#/jobs?repo=try&revision=0673ea0d486b
Keywords: checkin-needed
Comment 14•9 years ago
|
||
(In reply to Blake Wu [:bwu][:blakewu] from comment #11) > (In reply to Jean-Yves Avenard [:jya] from comment #10) > > Comment on attachment 8598481 [details] [diff] [review] > Thanks for your review! > I keep the log since it is very handy to enable the log when doing > performance check, but don't enable it in normal case to avoid too many logs. Well, make it conditional then. Or have a private patch for your own use adding the logs you would need. Having commented code in release looks unclean to me (still IMHO)
Assignee | ||
Comment 15•9 years ago
|
||
(In reply to Jean-Yves Avenard [:jya] from comment #14) > (In reply to Blake Wu [:bwu][:blakewu] from comment #11) > > (In reply to Jean-Yves Avenard [:jya] from comment #10) > > > Comment on attachment 8598481 [details] [diff] [review] > > Thanks for your review! > > I keep the log since it is very handy to enable the log when doing > > performance check, but don't enable it in normal case to avoid too many logs. > > Well, make it conditional then. > > Or have a private patch for your own use adding the logs you would need. > > Having commented code in release looks unclean to me (still IMHO) Yeah. It would be better to make it conditional. I am thinking how to group those performance check log and can be enabled/disabled conditionally and easily.
Comment 16•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/aba92cf22b0b
Keywords: checkin-needed
Comment 18•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/aba92cf22b0b
Status: NEW → 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
•