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)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla40
Tracking Status
firefox40 --- fixed

People

(Reporter: ayang, Assigned: bwu)

References

Details

Attachments

(1 file, 1 obsolete file)

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.
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
Blocks: 1123603, MSE-FxOS
This problem can be reproduced with Video APP as well.
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.
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
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.
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#
(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
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
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 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+
(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.
Carry r+ from jya.
Attachment #8598481 - Attachment is obsolete: true
Attachment #8599033 - Flags: review+
(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)
(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.
https://hg.mozilla.org/mozilla-central/rev/aba92cf22b0b
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
You need to log in before you can comment on or make changes to this bug.