Closed Bug 1105913 Opened 5 years ago Closed 5 years ago

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

Categories

(Core :: Audio/Video, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: alwu, Assigned: alwu)

References

Details

Attachments

(2 files, 10 obsolete files)

1.67 KB, patch
alwu
: review+
Details | Diff | Splinter Review
2.83 KB, patch
alwu
: review+
Details | Diff | Splinter Review
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.
Assignee: nobody → 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.
Attached patch Bug1105913_v1.patch (obsolete) — Splinter Review
Hi, Benjamin,
Could you give me some suggestions?
Thanks a lots :)
Attachment #8537124 - Flags: feedback?(bechen)
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 1105209
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
Depends on: 1105209
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.
Status: REOPENED → NEW
[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.
Attached patch Bug1105913_v1.patch (obsolete) — Splinter Review
Hi, JW,
Could you give my patch some suggestions?
Thanks a lots :)
Attachment #8537124 - Attachment is obsolete: true
Attachment #8537124 - Flags: feedback?(bechen)
Attachment #8539978 - Flags: feedback?(jwwang)
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?
Attachment #8539978 - Flags: feedback?(jwwang) → feedback+
Attached patch Bug1105913_v2.patch (obsolete) — Splinter Review
Hi, JW,
Here is my revised patch, 
Thanks a lots :)
Attachment #8539978 - Attachment is obsolete: true
Attachment #8540006 - Flags: feedback?(jwwang)
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.
Attachment #8540006 - Flags: feedback?(jwwang) → feedback-
Here is the patch of the problem on Comment5.
Attachment #8540006 - Attachment is obsolete: true
Attachment #8540029 - Flags: feedback?(jwwang)
Here is the patch of reverting the changeset of bug 1100803.
Attachment #8540029 - Attachment is obsolete: true
Attachment #8540029 - Flags: feedback?(jwwang)
Here is the patch of the problem on Comment5.
Attachment #8540036 - Flags: feedback?(jwwang)
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.
Attachment #8540036 - Flags: feedback?(jwwang) → feedback-
Attachment #8540034 - Attachment is obsolete: true
Attachment #8540036 - Attachment is obsolete: true
Attachment #8540045 - Flags: feedback?(jwwang)
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.
Attachment #8540045 - Flags: feedback?(jwwang) → feedback+
Hi, Robert.
Sorry to bother you, could you help me review my patches?
Thanks a lots :)
Attachment #8540043 - Attachment is obsolete: true
Attachment #8540480 - Flags: review?(roc)
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.
Attachment #8540045 - Attachment is obsolete: true
Attachment #8540481 - Flags: review?(roc)
Attachment #8540480 - Attachment is obsolete: true
Attachment #8540553 - Flags: review+
Attachment #8540481 - Attachment is obsolete: true
Attachment #8540554 - Flags: review+
Keywords: checkin-needed
Keywords: checkin-needed
It's not clear to me why checkin-needed was re-added to the bug when both patches were already pushed.
Keywords: checkin-needed
The changeset has been merged.
https://hg.mozilla.org/mozilla-central/diff/19313a129f6d/dom/media/MediaDecoder.cpp
Status: NEW → RESOLVED
Closed: 5 years ago5 years ago
Resolution: --- → FIXED
You should leave it to Sheriff instead of switching status on your own.
Depends on: 1100803
You need to log in before you can comment on or make changes to this bug.