Use decoder's timestamp instead of the timestamp from AU during prerolling

Use decoder's timestamp instead of the timestamp from AU during prerolling

When prerolling happens, MediaDecoderJob currently rolls to the timestamp from the access unit.
However, for video, the access unit's timestamp may not match that from the decoder due to reordering.
Even worse, the unit's timestamp may be greatly ahead of the presentation timestamp from the decoder since the decoder is still decoding previous access units. Some of the unit tests actually show this problem.

Fixed all the problematic unit tests along with this CL.

BUG=310823
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=243210

[qinmin, 2014-01-02 18:16:25]

PTAL

[acolwell GONE FROM CHROMIUM, 2014-01-02 19:07:49]

It doesn't look like any of the tests are actually testing this fix. They all
seem to be WebM only and out-of-order presentation timestamp case can only
happen w/ MP4 B-Frame content. We should test w/ B-Frame content so we can
properly verify this.

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

https://codereview.chromium.org/113963005/diff/1/media/base/android/media_sou...
media/base/android/media_source_player_unittest.cc:454: while
(player_.GetCurrentTime() == target_timestamp) {
Could you use IsPrerolling(is_audio) here instead?

[wolenetz, 2014-01-02 19:24:52]

https://codereview.chromium.org/113963005/diff/1/media/base/android/media_dec...
File media/base/android/media_decoder_job.cc (right):

https://codereview.chromium.org/113963005/diff/1/media/base/android/media_dec...
media/base/android/media_decoder_job.cc:353: bool render_output  =
presentation_timestamp >= preroll_timestamp_ &&
The previous implementation had a comment mentioning audio is a special case for
which |size| should be used to calculate the timestamp. Was that comment
incorrect, or if correct, where is |presentation_timestamp| calculated correctly
for audio prior to this line of code?

Also, the adjusted unit tests feed decoder with the same data repeatedly with
only AU timestamp changing (e.g., MSPUT::PrerollDecoderToTime()). Is it
guaranteed that all MediaCodec implementations will similarly increment the
output |presentation_timestamp| regardless of the encoded content being a series
of duplicate AUs?

It looks like usage and implementation of MCB::QueueInputBuffer() tells
MediaCodec that the encoded unit's presentation time is the AU's timestamp. Is
this always the case (even in out-of-order decodes?) I suspect this is incorrect
and more tests are necessary to confirm.

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

https://codereview.chromium.org/113963005/diff/1/media/base/android/media_sou...
media/base/android/media_source_player_unittest.cc:435: // to decode have a
timestamp equal to |start_timestamp|.
nit: s/have/will have/ ?

https://codereview.chromium.org/113963005/diff/1/media/base/android/media_sou...
media/base/android/media_source_player_unittest.cc:449: // Send some data with
access unit timestamps before the preroll_time, and
nit: s/preroll_time/|target_timestamp|/ ?

https://codereview.chromium.org/113963005/diff/1/media/base/android/media_sou...
media/base/android/media_source_player_unittest.cc:451: // This simulates the
common condition that AUs received after browser
nit: de-duplicate this versus earlier comment.

[qinmin, 2014-01-02 19:53:00]

On 2014/01/02 19:07:49, acolwell wrote:
> It doesn't look like any of the tests are actually testing this fix. They all
> seem to be WebM only and out-of-order presentation timestamp case can only
> happen w/ MP4 B-Frame content. We should test w/ B-Frame content so we can
> properly verify this.

There are 2 reasons we should not rely on AU's timestamp:
The first one is the one you mentioned. I need to find a test file for that.
The 2nd one is that when we pass an access unit that passed preroll timestamp to
the MediaCodec, the MediaCodec is still outputing frames from previous access
units,which have timestamps smaller than the preroll timestamp. The fixed tests
captured this case

> 
>
https://codereview.chromium.org/113963005/diff/1/media/base/android/media_sou...
> File media/base/android/media_source_player_unittest.cc (right):
> 
>
https://codereview.chromium.org/113963005/diff/1/media/base/android/media_sou...
> media/base/android/media_source_player_unittest.cc:454: while
> (player_.GetCurrentTime() == target_timestamp) {
> Could you use IsPrerolling(is_audio) here instead?

[qinmin, 2014-01-02 22:57:01]

Addressed all the code comments. 
For out-of-order case, I am not sure how we can test it. The android MediaCodec
may not decode the input frame immediately when dequeueOutputBuffer() is called.
So the B-frame is not decoded immediately after we queue it into the
InputBuffer. And MediaCodec won't tell us when the frame gets decoded. Added a
TODO to investigate this later.

https://codereview.chromium.org/113963005/diff/1/media/base/android/media_dec...
File media/base/android/media_decoder_job.cc (right):

https://codereview.chromium.org/113963005/diff/1/media/base/android/media_dec...
media/base/android/media_decoder_job.cc:353: bool render_output  =
presentation_timestamp >= preroll_timestamp_ &&

On 2014/01/02 19:24:53, wolenetz wrote:
> The previous implementation had a comment mentioning audio is a special case
for
> which |size| should be used to calculate the timestamp. Was that comment
> incorrect, or if correct, where is |presentation_timestamp| calculated
correctly
> for audio prior to this line of code?

audio timestamp is equal to input AU's timestamp, so it is not as accurate as
the clock_(which uses size） we maintained in MSP. However, since both Audio and
video presentation timestamp are obtained from AU's timestamp, so the same
inaccuracy will be contained in both streams. If we currently relying on AU's
timestamp to do preroll, then using presentation timestamp should not introduce
extra inaccuracies.

> 
> Also, the adjusted unit tests feed decoder with the same data repeatedly with
> only AU timestamp changing (e.g., MSPUT::PrerollDecoderToTime()). Is it
> guaranteed that all MediaCodec implementations will similarly increment the
> output |presentation_timestamp| regardless of the encoded content being a
series
> of duplicate AUs?
> 

Yes, MediaCodec uses the relies on the input AU's timestamp to calculate the
output |presentation_timestamp|. If input AU's presentation timestamp is
changed, so do output if they are all I-frames.

> It looks like usage and implementation of MCB::QueueInputBuffer() tells
> MediaCodec that the encoded unit's presentation time is the AU's timestamp. Is
> this always the case (even in out-of-order decodes?) I suspect this is
incorrect
> and more tests are necessary to confirm.

For audio, the decoded presentation timestamp is equal to the input timestamp.
For video, i have no idea how timestamp for out-of-order cases are calculated.

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

https://codereview.chromium.org/113963005/diff/1/media/base/android/media_sou...
media/base/android/media_source_player_unittest.cc:435: // to decode have a
timestamp equal to |start_timestamp|.
On 2014/01/02 19:24:53, wolenetz wrote:
> nit: s/have/will have/ ?

Done.

https://codereview.chromium.org/113963005/diff/1/media/base/android/media_sou...
media/base/android/media_source_player_unittest.cc:449: // Send some data with
access unit timestamps before the preroll_time, and
On 2014/01/02 19:24:53, wolenetz wrote:
> nit: s/preroll_time/|target_timestamp|/ ?

Done.

https://codereview.chromium.org/113963005/diff/1/media/base/android/media_sou...
media/base/android/media_source_player_unittest.cc:451: // This simulates the
common condition that AUs received after browser
On 2014/01/02 19:24:53, wolenetz wrote:
> nit: de-duplicate this versus earlier comment.

Done.

[wolenetz, 2014-01-02 23:24:21]

https://codereview.chromium.org/113963005/diff/90001/media/base/android/media...
File media/base/android/media_source_player_unittest.cc (right):

https://codereview.chromium.org/113963005/diff/90001/media/base/android/media...
media/base/android/media_source_player_unittest.cc:436: // TODO(qinmin): Add
additional test cases for out-of-order decodes.
nit: I would prefer we add a Chromium bug to track this TODO. Mismatch of Decode
order versus Presentation order may be the root cause of http://b/11356652.

https://codereview.chromium.org/113963005/diff/90001/media/base/android/media...
media/base/android/media_source_player_unittest.cc:461:
EXPECT_LT(target_timestamp, player_.GetCurrentTime());
It's possible that prerolling has completed and the player's time is exactly
|target_timestamp|.
s/_LT/_LE/ ?

[qinmin, 2014-01-02 23:35:22]

https://codereview.chromium.org/113963005/diff/90001/media/base/android/media...
File media/base/android/media_source_player_unittest.cc (right):

https://codereview.chromium.org/113963005/diff/90001/media/base/android/media...
media/base/android/media_source_player_unittest.cc:436: // TODO(qinmin): Add
additional test cases for out-of-order decodes.
On 2014/01/02 23:24:22, wolenetz wrote:
> nit: I would prefer we add a Chromium bug to track this TODO. Mismatch of
Decode
> order versus Presentation order may be the root cause of http://b/11356652.

Done.

https://codereview.chromium.org/113963005/diff/90001/media/base/android/media...
media/base/android/media_source_player_unittest.cc:461:
EXPECT_LT(target_timestamp, player_.GetCurrentTime());
On 2014/01/02 23:24:22, wolenetz wrote:
> It's possible that prerolling has completed and the player's time is exactly
> |target_timestamp|.
> s/_LT/_LE/ ?

Done.

[wolenetz, 2014-01-02 23:49:32]

lgtm

[qinmin, 2014-01-06 18:07:32]

acolwell@, is the newly added TODO looking good to you? Thanks

[acolwell GONE FROM CHROMIUM, 2014-01-06 19:56:17]

lgtm

[commit-bot: I haz the power, 2014-01-06 20:00:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/qinmin@chromium.org/113963005/150001

[commit-bot: I haz the power, 2014-01-07 00:02:22]

Change committed as 243210