[FFOS] MP4 decoding doesn't resume from dormant state.

RESOLVED FIXED in Firefox 40

Status

()

Core
Audio/Video
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: alfredo, Assigned: bwu)

Tracking

(Blocks: 1 bug)

unspecified
mozilla40
ARM
Gonk (Firefox OS)
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox40 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

3 years ago
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

3 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

3 years ago
Blocks: 1123603, 1027519
(Assignee)

Comment 2

3 years ago
This problem can be reproduced with Video APP as well.
(Assignee)

Comment 3

3 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

3 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

3 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

3 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

3 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

3 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

3 years ago
Created attachment 8598481 [details] [diff] [review]
Bug-1147304-Send-codec-specific-data-for-MPEG4-codec.patch

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+
(Assignee)

Comment 11

3 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

3 years ago
Created attachment 8599033 [details] [diff] [review]
Bug-1147304-Send-codec-specific-data-for-MPEG4-codec-only-hg.patch

Carry r+ from jya.
Attachment #8598481 - Attachment is obsolete: true
Attachment #8599033 - Flags: review+
(Assignee)

Comment 13

3 years ago
Test results look good.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0673ea0d486b
Keywords: checkin-needed
(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

3 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.
https://hg.mozilla.org/mozilla-central/rev/aba92cf22b0b
Status: NEW → RESOLVED
Last Resolved: 3 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.