1105913 - [b2g] video can't playback after waking up from the dormant state

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Alastor Wu [:alwu]]

STR.
1. goto http://people.mozilla.org/~rlin/MediaRecorder.html
2. play VP9/720WebM/352WebM
3. sleep the phone & wake it up

Expected. 
Video is sought to the correct time, and it could be played by pressing the play button

Actual situation.
1. Video stops in the seeking state without displaying image, and it can't be play again.
2. Video goes to the decoded state with displaying image, but it still can't be play. It is just looping in the decoded state without getting any video frame.

Comment 1

[Alastor Wu [:alwu]]

[Problem]
Fail to arouse the function |SeekComplete()| so that the media can't wake up from the dormant state.

[Situation] 
|SeekComplete()| would be finished after getting both audio/video data. The operation of the dormant state resulted that the flag which controls to send audio/video tasks was always false so that we couldn't success to arouse the function |SeekComplete()|.

[Solution]
To reset the flags |mAudio/VideoRequestPending| in the operation of the dormant state.

[More details]
When we enter the dormant state, the MediaDecoderStateMachine would fire the function |FlushDecoding()|. This function would clear the tasks which are not started yet in the task queue. In the decoding process, we would send the |Decode*()| to task queue, to wait them sending decoded data back for us. Sometimes, the decoding task would be cleared before execution. 

The problem is that, the decoding tasks reset the flags |mAudio/VideoRequestPending| which is the key about whether we can send the task next time. If the flags are false, we couldn't send the decoding task successfully and the failure of |SeekComplete()| would happen.

Comment 2

[Alastor Wu [:alwu]]

Attachment: Bug1105913_v1.patch

Hi, Benjamin,
Could you give me some suggestions?
Thanks a lots :)

Comment 4

[Alastor Wu [:alwu]]

After Bug1105209 is fixed, now video can leave the seek state. But it still can't playback. 

See Bug 1112503, it has a short delay to get the last frame we played. The video would be stuck in this frame rather than continually is played. 

Now I observe something may be the reason of stopping playing video. It is the clock time doesn't change so that we can't match the next frame which can be played.

Comment 5

[Alastor Wu [:alwu]]

[Problem] 
The video can't playback after waking up from the dormant state

[Situation] 
The original situation is that the video stayed in the seek state after waking up from the dormant state, but the Bug 1105209 fixed this problem. After this patch landed, the video would display a image frame now, but it still can't playback. 

[Reason]
We called the function |DestroyDecodedStream()| a twice so that  the output stream of the MediaDecoder got a extra blocking. The first one is called when entering the dormant state, and the second one is called when we need to recreate the decoded stream in |RecreateDecodedStream()|.

[Solution]
Check the existence of the |mDecodedStream| to avoid the redundant blocking.

Comment 6

[Alastor Wu [:alwu]]

Attachment: Bug1105913_v1.patch

Hi, JW,
Could you give my patch some suggestions?
Thanks a lots :)

Comment 7

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8539978 [details] [diff] [review]
Bug1105913_v1.patch

Review of attachment 8539978 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me. However, it looks like we can now revert the change in patch part 1 of bug 1100803 to enforce invariant, ie, the early return should prevent ports from being deleted twice, right?

Comment 8

[Alastor Wu [:alwu]]

Attachment: Bug1105913_v2.patch

Hi, JW,
Here is my revised patch, 
Thanks a lots :)

Comment 9

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8540006 [details] [diff] [review]
Bug1105913_v2.patch

Review of attachment 8540006 [details] [diff] [review]:
-----------------------------------------------------------------

Please put changes reverting bug 1100803 part 1 into another patch. This will be easier to review.

::: dom/media/MediaDecoder.cpp
@@ +841,5 @@
>  
>    if (mShuttingDown ||
>        mPlayState == PLAY_STATE_SEEKING ||
> +      mPlayState == PLAY_STATE_LOADING ||
> +      !GetDecodedStream()) {

This will break the case of not-decoding-to-stream.

Comment 10

[Alastor Wu [:alwu]]

Attachment: Part 1 : Avoid redundant blocking

Here is the patch of the problem on Comment5.

Comment 11

[Alastor Wu [:alwu]]

Attachment: Part 2 : Revert the changeset of bug 1100803

Here is the patch of reverting the changeset of bug 1100803.

Comment 12

[Alastor Wu [:alwu]]

Attachment: Part 1 : Avoid redundant blocking

Here is the patch of the problem on Comment5.

Comment 13

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8540036 [details] [diff] [review]
Part 1 : Avoid redundant blocking

Review of attachment 8540036 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/media/MediaDecoder.cpp
@@ +845,4 @@
>      return;
>    }
>  
> +  if (GetDecodedStream()) {

Why checking GetDecodedStream() here? It is also wrong to call GetDecodedStream() without holding the lock.

Comment 14

[Alastor Wu [:alwu]]

Attachment: Part 1 : Avoid redundant blocking

Comment 15

[Alastor Wu [:alwu]]

Attachment: Part 2 : Revert the changeset of bug 1100803

Comment 16

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8540045 [details] [diff] [review]
Part 2 : Revert the changeset of bug 1100803

Review of attachment 8540045 [details] [diff] [review]:
-----------------------------------------------------------------

Please also revert the changes in MediaDecoder::PlaybackEnded() and put assertions to catch double-delete of the ports.

::: dom/media/MediaDecoder.cpp
@@ +291,5 @@
>      // its Destroy message before this decoder is destroyed. So we have to
>      // be careful not to send any messages after the Destroy().
>      if (os.mStream->IsDestroyed()) {
>        // Probably the DOM MediaStream was GCed. Clean up.
> +      os.mPort->Destroy();

Assert os.mPort is not null. So if double-delete happens again in the future, we will know it.

@@ +299,5 @@
>      os.mStream->ChangeExplicitBlockerCount(1);
>      // Explicitly remove all existing ports. This is not strictly necessary but it's
>      // good form.
> +    os.mPort->Destroy();
> +    os.mPort = nullptr;

Assertion.

Comment 17

[Alastor Wu [:alwu]]

Attachment: Part 1 : Avoid redundant blocking

Hi, Robert.
Sorry to bother you, could you help me review my patches?
Thanks a lots :)

Comment 18

[Alastor Wu [:alwu]]

Attachment: Part 2 : Revert the changeset of bug 1100803

The details of the patch1 is described in the comment5.
Since this changeset also could avoid the ports double-deletion in the bug 1100803, I reverts the change and adds the assertion to prevent the error happens again.

Comment 19

[Alastor Wu [:alwu]]

Attachment: Part 1 : Avoid redundant blocking. r=roc.

Comment 20

[Alastor Wu [:alwu]]

Attachment: Part 2 : Revert the changeset of bug 1100803. r=roc.

Comment 21

[Alastor Wu [:alwu]]

Try result
https://tbpl.mozilla.org/?tree=Try&rev=94ffbf46f9b4

Comment 22

[Carsten Book [:Tomcat]]

landed as https://hg.mozilla.org/integration/mozilla-inbound/rev/2441e90516a0
https://hg.mozilla.org/integration/mozilla-inbound/rev/d2f456f0d141

Comment 23

[Ryan VanderMeulen [:RyanVM]]

It's not clear to me why checkin-needed was re-added to the bug when both patches were already pushed.

Comment 24

[Alastor Wu [:alwu]]

The changeset has been merged.
https://hg.mozilla.org/mozilla-central/diff/19313a129f6d/dom/media/MediaDecoder.cpp

Comment 25

[JW Wang [:jwwang] [:jw_wang]]

You should leave it to Sheriff instead of switching status on your own.