Closed Bug 1365190 Opened 7 years ago Closed 7 years ago

NextFrameSeekingState should be aware of existing video data request.

Categories

(Core :: Audio/Video: Playback, defect)

Product:
Core
Component:
Audio/Video: Playback
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: kaku, Assigned: kaku)

References

Details

Attachments

(2 files)

Bug 1365190 P1 - NextFrameSeekingState should be aware of the existing video data request;
59 bytes, text/x-review-board-request
jwwang
: review+
Details
Bug 1365190 P2 - don't assert NeedMoreVideo() in audio events;
59 bytes, text/x-review-board-request
jwwang
: review+
Details 
http://searchfox.org/mozilla-central/rev/ae24a3c83d22e0e35aecfd9049c2b463ca7e045b/dom/media/MediaDecoderStateMachine.cpp#1624

If there is a existing video data request while entering NextFrameSeekState (for example, the previous state is DECODING state) and NextFrameSeekState::HandleVideoDecoded() is called thereafter, the assertion above might fail.
Assignee: nobody → kaku
Status: NEW → ASSIGNED
Blocks: 1360452
Summary: NextFrameSeekingState should be award of existing video data request. → NextFrameSeekingState should be aware of existing video data request.
Comment on attachment 8868071 [details]
Bug 1365190 P1 - NextFrameSeekingState should be aware of the existing video data request;

https://reviewboard.mozilla.org/r/139678/#review143312

::: dom/media/MediaDecoderStateMachine.cpp:1614
(Diff revision 1)
>      // promise resolving is postponed and then the JS developer receives the
>      // "ended" event before the seek promise is resolved.
>      // An asynchronous seek operation helps to solve this issue since while the
>      // seek is actually performed, the ThenValue of SeekPromise has already
>      // been set so that it won't be postponed.
>      RefPtr<Runnable> r = mAsyncSeekTask = new AysncNextFrameSeekTask(this);

We should change the code here:

1. Call DiscardFrames().
2. If a video request is pending, finish seeking now if possible or wait for HandleVideoDecoded() to finish seeking. Note we won't hit the problem of bug504613.ogv in this case.
3. If not, dispatch a task to check if we can finish seek.

::: dom/media/MediaDecoderStateMachine.cpp:1744
(Diff revision 1)
>    }
>  
>    /*
>     * Internal state.
>     */
> +  bool mWasRequestingVideoData;

It feels wrong to me to add a member just for the assertion.
Comment on attachment 8868071 [details]
Bug 1365190 P1 - NextFrameSeekingState should be aware of the existing video data request;

https://reviewboard.mozilla.org/r/139678/#review143348
Attachment #8868071 - Flags: review?(jwwang) → review+
Comment on attachment 8868071 [details]
Bug 1365190 P1 - NextFrameSeekingState should be aware of the existing video data request;

https://reviewboard.mozilla.org/r/139678/#review143402

::: dom/media/MediaDecoderStateMachine.cpp:1608
(Diff revision 2)
> +
> +    // If there is a pending video request, finish the seeking if we don't need
> +    // more data, or wait for HandleVideoDecoded() to finish seeking.
> +    if (mMaster->IsRequestingVideoData()) {
> +      if (!NeedMoreVideo()) {
> +        FinishSeek();

We cannot synchronously call FinishSeek() here because it will then call `mSeekJob.Resolve()` but we haven't call `mSeekJob.mPromise.Ensure(__func__)` yet.
Comment on attachment 8868071 [details]
Bug 1365190 P1 - NextFrameSeekingState should be aware of the existing video data request;

https://reviewboard.mozilla.org/r/139678/#review143402

> We cannot synchronously call FinishSeek() here because it will then call `mSeekJob.Resolve()` but we haven't call `mSeekJob.mPromise.Ensure(__func__)` yet.

It is easy to fix in SeekingState::Enter():
p = mSeekJob.mPromise.Ensure(__func__);
DoSeek();
return p;
A failed case on the try server (comment 10):
https://treeherder.mozilla.org/logviewer.html#?job_id=100005735&repo=try&lineNumber=3802

We hit the assertion here: https://hg.mozilla.org/try/file/232342576639/dom/media/MediaDecoderStateMachine.cpp#l1668

However, I think the assertion here is invalid, we should just ignore any "audio"-decoding errors in NextFrameSeekState.
Comment on attachment 8868071 [details]
Bug 1365190 P1 - NextFrameSeekingState should be aware of the existing video data request;

https://reviewboard.mozilla.org/r/139678/#review146164

::: dom/media/MediaDecoderStateMachine.cpp:1556
(Diff revision 4)
>    }
>  
>    void Exit() override
>    {
>      // Disconnect my async seek operation.
> -    mAsyncSeekTask->Cancel();
> +    if (mAsyncSeekTask) mAsyncSeekTask->Cancel();

missing { }
Comment on attachment 8870768 [details]
Bug 1365190 P2 - don't assert NeedMoreVideo() in audio events;

https://reviewboard.mozilla.org/r/142266/#review146292
Attachment #8870768 - Flags: review?(jwwang) → review+
Thanks for the review!
Pushed by tkuo@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/9a3781bd18cb
P1 - NextFrameSeekingState should be aware of the existing video data request; r=jwwang
https://hg.mozilla.org/integration/autoland/rev/b861944ceca6
P2 - don't assert NeedMoreVideo() in audio events; r=jwwang
https://hg.mozilla.org/mozilla-central/rev/9a3781bd18cb
https://hg.mozilla.org/mozilla-central/rev/b861944ceca6
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: