1319706 - Allow seeking before finishing decoding first frames

Status: RESOLVED FIXED

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

Description

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

Thanks to bug 1309516, we have start time after decoding the metadata. So MDSM is able to seek even before decoding first frames.

Comment 1

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

Attachment: Bug 1319706. Part 1 - remove DecodingFirstFrameState::mPendingSeek since it can handle seek requests immediately.

Review commit: https://reviewboard.mozilla.org/r/95028/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/95028/

Comment 2

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

Attachment: Bug 1319706. Part 2 - remove the SeekJob parameter from DecodingFirstFrameState::Enter().

Review commit: https://reviewboard.mozilla.org/r/95030/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/95030/

Comment 3

[Tzuhao Kuo [:kaku]]

Comment on attachment 8813591 [details]
Bug 1319706. Part 1 - remove DecodingFirstFrameState::mPendingSeek since it can handle seek requests immediately.

https://reviewboard.mozilla.org/r/95028/#review95198

Comment 4

[Tzuhao Kuo [:kaku]]

Comment on attachment 8813592 [details]
Bug 1319706. Part 2 - remove the SeekJob parameter from DecodingFirstFrameState::Enter().

https://reviewboard.mozilla.org/r/95030/#review95200

Comment 5

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

Attachment: Bug 1319706. Part 3 - remove the assertion that is no longer valid.

Review commit: https://reviewboard.mozilla.org/r/95274/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/95274/

Comment 6

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

Attachment: Bug 1319706. Part 4 - add some logs to debug 'ended' not fired.

Review commit: https://reviewboard.mozilla.org/r/95276/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/95276/

Comment 7

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

Comment on attachment 8813591 [details]
Bug 1319706. Part 1 - remove DecodingFirstFrameState::mPendingSeek since it can handle seek requests immediately.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/95028/diff/1-2/

Comment 8

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

Comment on attachment 8813592 [details]
Bug 1319706. Part 2 - remove the SeekJob parameter from DecodingFirstFrameState::Enter().

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/95030/diff/1-2/

Comment 9

[Tzuhao Kuo [:kaku]]

Comment on attachment 8813974 [details]
Bug 1319706. Part 3 - remove the assertion that is no longer valid.

https://reviewboard.mozilla.org/r/95274/#review95454

Comment 10

[Tzuhao Kuo [:kaku]]

Comment on attachment 8813975 [details]
Bug 1319706. Part 4 - add some logs to debug 'ended' not fired.

https://reviewboard.mozilla.org/r/95276/#review95456

Comment 11

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

dom/media/test/test_bug1242338.html times out because:

http://searchfox.org/mozilla-central/rev/feef954874af9a18168e61a75629a9406b847c53/dom/media/MediaDecoderStateMachine.cpp#2720
mFirstFrameLoadedEvent won't be notified until mBufferedUpdateRequest is resolved which could happen after

http://searchfox.org/mozilla-central/rev/feef954874af9a18168e61a75629a9406b847c53/dom/media/MediaDecoderStateMachine.cpp#1066
notifying mOnPlaybackEvent when seeking to the end of the media (as test_bug1242338.html does).

http://searchfox.org/mozilla-central/rev/feef954874af9a18168e61a75629a9406b847c53/dom/media/MediaDecoder.cpp#983
MediaDecoder::PlaybackEnded() returns early for mPlayState == PLAY_STATE_LOADING (which is changed in FirstFrameLoaded).

Comment 12

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

Comment on attachment 8813591 [details]
Bug 1319706. Part 1 - remove DecodingFirstFrameState::mPendingSeek since it can handle seek requests immediately.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/95028/diff/2-3/

Comment 13

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

Comment on attachment 8813592 [details]
Bug 1319706. Part 2 - remove the SeekJob parameter from DecodingFirstFrameState::Enter().

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/95030/diff/2-3/

Comment 14

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

Comment on attachment 8813974 [details]
Bug 1319706. Part 3 - remove the assertion that is no longer valid.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/95274/diff/1-2/

Comment 15

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

Comment on attachment 8813975 [details]
Bug 1319706. Part 4 - add some logs to debug 'ended' not fired.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/95276/diff/1-2/

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8aaa89d73c3f
Part 1 - remove DecodingFirstFrameState::mPendingSeek since it can handle seek requests immediately. r=kaku
https://hg.mozilla.org/integration/autoland/rev/03144421b4a6
Part 2 - remove the SeekJob parameter from DecodingFirstFrameState::Enter(). r=kaku
https://hg.mozilla.org/integration/autoland/rev/3dc995228de4
Part 3 - remove the assertion that is no longer valid. r=kaku
https://hg.mozilla.org/integration/autoland/rev/e355836470f9
Part 4 - add some logs to debug 'ended' not fired. r=kaku

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8aaa89d73c3f
https://hg.mozilla.org/mozilla-central/rev/03144421b4a6
https://hg.mozilla.org/mozilla-central/rev/3dc995228de4
https://hg.mozilla.org/mozilla-central/rev/e355836470f9

Comment 18

[Jean-Yves Avenard [:jya]]

was just thinking that this could cause potential regression if we seek prior decoding the first frame for decoders such as Opus or Vorbis (and AAC once bug 1321249 is done)

The Opus decoder has a PreSkip value; that is the number of frames that needs to be dropped from the first decoded sample.

This should only be done on the first sample in the stream, not the first sample ever decoded. Which if you seek before decoding is what will happen.

if not, we need to modify all demuxers to add a flag to indicate that a packet is the first in the stream.

Comment 19

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

So what's the path forward? Should I back out this bug  or you will modify the demuxer to add the flag? Will this issue happen to MSE as well (which allows seeking even before decoding the 1st sample) or it doesn't support Opus and Vorbis at all?