Let audio wait for video to finish prerolling after seek

Let audio wait for video to finish prerolling after seek

After a seek, both audio and video decoder job will start prerolling.
However, audio decoder job can finish prerolling much earlier than video decoder job.
As a result, audio will start playing before video starts.
And we will see some fast motion when video try to catch up with audio after it starts rendering.

internal b/17527385

Committed: https://crrev.com/367bd6822d68ef787fa16efc3b7053db6b56c906
Cr-Commit-Position: refs/heads/master@{#296607}

[qinmin, 2014-09-23 21:37:23]

qinmin@chromium.org changed reviewers:
+ wolenetz@chromium.org

[qinmin, 2014-09-23 21:37:23]

PTAL

[wolenetz, 2014-09-24 20:39:04]

Thanks for working on fixing this!

https://codereview.chromium.org/596093002/diff/1/media/base/android/media_sou...
File media/base/android/media_source_player_unittest.cc (left):

https://codereview.chromium.org/596093002/diff/1/media/base/android/media_sou...
media/base/android/media_source_player_unittest.cc:1624: DemuxerConfigs configs
= CreateAudioDemuxerConfigs(kCodecVorbis, true);
Do we have another test case that covers the case where a config change occurs
between the result of the seek and the preroll target? (Why was this piece of
this test removed?) Granted, this scenario is fairly unlikely, since without any
intervening changes to the underlying audio and video source_buffer_streams,
they will begin with buffers from the last keyframe with dts <= the seek target
(and config changes should be followed by keyframe, so there shouldn't be an
intervening config change). I'm uncertain if an overlapped appendBuffer for a
different config during preroll could cause config change during preroll, but I
suspect it could.

https://codereview.chromium.org/596093002/diff/1/media/base/android/media_sou...
File media/base/android/media_source_player_unittest.cc (right):

https://codereview.chromium.org/596093002/diff/1/media/base/android/media_sou...
media/base/android/media_source_player_unittest.cc:490: EXPECT_TRUE(!is_audio ||
target_timestamp == player_.GetCurrentTime());
nit: Can you more precisely capture the expectation of player currenttime for a
video-only player that is prerolling?

https://codereview.chromium.org/596093002/diff/1/media/base/android/media_sou...
media/base/android/media_source_player_unittest.cc:508: EXPECT_TRUE(!is_audio ||
target_timestamp == player_.GetCurrentTime());
nit: ditto

[qinmin, 2014-09-24 22:49:50]

https://codereview.chromium.org/596093002/diff/1/media/base/android/media_sou...
File media/base/android/media_source_player_unittest.cc (left):

https://codereview.chromium.org/596093002/diff/1/media/base/android/media_sou...
media/base/android/media_source_player_unittest.cc:1624: DemuxerConfigs configs
= CreateAudioDemuxerConfigs(kCodecVorbis, true);
Wierd, I didn't remember I made any change to this function.
Reverted all changes here.

On 2014/09/24 20:39:04, wolenetz wrote:
> Do we have another test case that covers the case where a config change occurs
> between the result of the seek and the preroll target? (Why was this piece of
> this test removed?) Granted, this scenario is fairly unlikely, since without
any
> intervening changes to the underlying audio and video source_buffer_streams,
> they will begin with buffers from the last keyframe with dts <= the seek
target
> (and config changes should be followed by keyframe, so there shouldn't be an
> intervening config change). I'm uncertain if an overlapped appendBuffer for a
> different config during preroll could cause config change during preroll, but
I
> suspect it could.

https://codereview.chromium.org/596093002/diff/1/media/base/android/media_sou...
File media/base/android/media_source_player_unittest.cc (right):

https://codereview.chromium.org/596093002/diff/1/media/base/android/media_sou...
media/base/android/media_source_player_unittest.cc:490: EXPECT_TRUE(!is_audio ||
target_timestamp == player_.GetCurrentTime());
On 2014/09/24 20:39:04, wolenetz wrote:
> nit: Can you more precisely capture the expectation of player currenttime for
a
> video-only player that is prerolling?

added a boolean to indicate whether the decoder is the clock manager. So for
video only player, the boolean should be true.

https://codereview.chromium.org/596093002/diff/1/media/base/android/media_sou...
media/base/android/media_source_player_unittest.cc:508: EXPECT_TRUE(!is_audio ||
target_timestamp == player_.GetCurrentTime());
is_clock_manager should handle the case.

On 2014/09/24 20:39:04, wolenetz wrote:
> nit: ditto

[wolenetz, 2014-09-24 23:34:47]

lgtm!

[qinmin, 2014-09-24 23:47:53]

The CQ bit was checked by qinmin@chromium.org

[commit-bot: I haz the power, 2014-09-24 23:49:02]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/596093002/20001

[commit-bot: I haz the power, 2014-09-25 01:15:47]

Committed patchset #2 (id:20001) as 3324cd04c73b3caed074340203d4c06112fa05bb

[commit-bot: I haz the power, 2014-09-25 01:16:22]

Patchset 2 (id:??) landed as
https://crrev.com/367bd6822d68ef787fa16efc3b7053db6b56c906
Cr-Commit-Position: refs/heads/master@{#296607}