MediaCodecPlayer - preroll and reconfiguration fixes

MediaCodecPlayer - preroll and reconfiguration fixes

Now the decision to do preroll is controlled by two variables:
the preroll timestamp, which is set by seek, and the MediaCodec
state "is_prepared" - it is considered prepared after we got
the output buffer.

With these changes preroll won't be done again if the decoder is
already prerolled (thus there might be a case where only one
stream is prerolled, e.g. the one that is drained, for 2 streams
A/V playback). Also the preroll timestamp is not reset when the
preroll starts, only when it is done or flush happens.

Added more unit tests for decoder reconfiguration.

BUG=407577
Committed: https://crrev.com/4b000195a8d27e9c3c4bfdd77113674840286a48
Cr-Commit-Position: refs/heads/master@{#347780}

[Tima Vaisburd, 2015-09-02 02:34:58]

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

[Tima Vaisburd, 2015-09-02 02:34:58]

Min,

I did write the variant with a separate Prepare() according to what we
discussed. But I did not like it that much. I saved it in a local patch. While
working on it I realized that there is possibly a much simpler fix that would
improve the system. It is based on your ideas, but Preroll() is still combined -
maybe we need to rename it.

I tried to summarize the changes in the description. Please take a look at the
approach.

This is obviously not finished, I need to implement the tests that are still
empty.

[qinmin, 2015-09-03 05:30:04]

https://codereview.chromium.org/1312103006/diff/60001/media/base/android/medi...
File media/base/android/media_codec_decoder.cc (right):

https://codereview.chromium.org/1312103006/diff/60001/media/base/android/medi...
media/base/android/media_codec_decoder.cc:126: is_prepared_ = false;
this variable is updated on both decoder thread and  media thread, is that safe?

https://codereview.chromium.org/1312103006/diff/60001/media/base/android/medi...
media/base/android/media_codec_decoder.cc:312: // preroll_timestamp_ =
base::TimeDelta();
remove this if unused

https://codereview.chromium.org/1312103006/diff/60001/media/base/android/medi...
File media/base/android/media_codec_player.cc (right):

https://codereview.chromium.org/1312103006/diff/60001/media/base/android/medi...
media/base/android/media_codec_player.cc:1002: if (do_audio) {
simply if  (do_audio && !audio_decoder_->Preroll(preroll_cb))

https://codereview.chromium.org/1312103006/diff/60001/media/base/android/medi...
media/base/android/media_codec_player.cc:1007: if (do_video) {
ditto

[Tima Vaisburd, 2015-09-03 19:55:27]

I also made two small changes related to stopping that look more correct to me.
Please take another look.

https://codereview.chromium.org/1312103006/diff/60001/media/base/android/medi...
File media/base/android/media_codec_decoder.cc (right):

https://codereview.chromium.org/1312103006/diff/60001/media/base/android/medi...
media/base/android/media_codec_decoder.cc:126: is_prepared_ = false;
On 2015/09/03 05:30:04, qinmin wrote:
> this variable is updated on both decoder thread and  media thread, is that
safe?

During Flush() and ReleaseMediaCodec() the decoder thread should not be running.
At least this was my intention. I added DCHECKs.

https://codereview.chromium.org/1312103006/diff/60001/media/base/android/medi...
media/base/android/media_codec_decoder.cc:312: // preroll_timestamp_ =
base::TimeDelta();
On 2015/09/03 05:30:04, qinmin wrote:
> remove this if unused

Replaced with
  DCHECK(preroll_timestamp_ == base::TimeDelta());

https://codereview.chromium.org/1312103006/diff/60001/media/base/android/medi...
File media/base/android/media_codec_player.cc (right):

https://codereview.chromium.org/1312103006/diff/60001/media/base/android/medi...
media/base/android/media_codec_player.cc:1002: if (do_audio) {
On 2015/09/03 05:30:04, qinmin wrote:
> simply if  (do_audio && !audio_decoder_->Preroll(preroll_cb)) 
> 

Done.

https://codereview.chromium.org/1312103006/diff/60001/media/base/android/medi...
media/base/android/media_codec_player.cc:1007: if (do_video) {
On 2015/09/03 05:30:04, qinmin wrote:
> ditto

Done.

https://codereview.chromium.org/1312103006/diff/80001/media/base/android/medi...
File media/base/android/media_codec_decoder.cc (right):

https://codereview.chromium.org/1312103006/diff/80001/media/base/android/medi...
media/base/android/media_codec_decoder.cc:378: case kStopped:
Moved kStopped here so RequestToStop will be ignore in kStopped state.

https://codereview.chromium.org/1312103006/diff/80001/media/base/android/medi...
File media/base/android/media_codec_player.cc (right):

https://codereview.chromium.org/1312103006/diff/80001/media/base/android/medi...
media/base/android/media_codec_player.cc:697: return;
Changed |break| to |return| so we do not post notifications to the upper layer
if we are in the unexpected state.

[qinmin, 2015-09-03 23:29:03]

lgtm

[Tima Vaisburd, 2015-09-04 06:18:13]

timav@chromium.org changed reviewers:
+ liberato@chromium.org

[liberato (no reviews please), 2015-09-04 21:59:14]

lgtm % nits, just a few comments.

https://codereview.chromium.org/1312103006/diff/80001/media/base/android/medi...
File media/base/android/media_codec_decoder.cc (right):

https://codereview.chromium.org/1312103006/diff/80001/media/base/android/medi...
media/base/android/media_codec_decoder.cc:407: preroll_timestamp_ =
base::TimeDelta();
there's a kNoTimestamp that you might want to consider for this case.

https://codereview.chromium.org/1312103006/diff/80001/media/base/android/medi...
File media/base/android/media_codec_decoder.h (right):

https://codereview.chromium.org/1312103006/diff/80001/media/base/android/medi...
media/base/android/media_codec_decoder.h:198: void
SetDecodingUntilOutputIsPresent();
did you mean to delete this?

https://codereview.chromium.org/1312103006/diff/80001/media/base/android/medi...
File media/base/android/media_codec_player.cc (right):

https://codereview.chromium.org/1312103006/diff/80001/media/base/android/medi...
media/base/android/media_codec_player.cc:962: MediaCodecPlayer::StartStatus
MediaCodecPlayer::MaybePrerollDecoders(
now that the preroll timestamp is being maintained differently in the decoders,
i'm not sure that we need all of this.  could we always go through the preroll
state, and get rid of NotCompletedAndNeedsPreroll()?

i don't know if it would work exactly as it is now, but i think it can get
there.

[liberato (no reviews please), 2015-09-04 21:59:14]

lgtm % nits, just a few comments.

https://codereview.chromium.org/1312103006/diff/80001/media/base/android/medi...
File media/base/android/media_codec_decoder.cc (right):

https://codereview.chromium.org/1312103006/diff/80001/media/base/android/medi...
media/base/android/media_codec_decoder.cc:407: preroll_timestamp_ =
base::TimeDelta();
there's a kNoTimestamp that you might want to consider for this case.

https://codereview.chromium.org/1312103006/diff/80001/media/base/android/medi...
File media/base/android/media_codec_decoder.h (right):

https://codereview.chromium.org/1312103006/diff/80001/media/base/android/medi...
media/base/android/media_codec_decoder.h:198: void
SetDecodingUntilOutputIsPresent();
did you mean to delete this?

https://codereview.chromium.org/1312103006/diff/80001/media/base/android/medi...
File media/base/android/media_codec_player.cc (right):

https://codereview.chromium.org/1312103006/diff/80001/media/base/android/medi...
media/base/android/media_codec_player.cc:962: MediaCodecPlayer::StartStatus
MediaCodecPlayer::MaybePrerollDecoders(
now that the preroll timestamp is being maintained differently in the decoders,
i'm not sure that we need all of this.  could we always go through the preroll
state, and get rid of NotCompletedAndNeedsPreroll()?

i don't know if it would work exactly as it is now, but i think it can get
there.

[Tima Vaisburd, 2015-09-04 23:17:36]

https://codereview.chromium.org/1312103006/diff/80001/media/base/android/medi...
File media/base/android/media_codec_decoder.cc (right):

https://codereview.chromium.org/1312103006/diff/80001/media/base/android/medi...
media/base/android/media_codec_decoder.cc:407: preroll_timestamp_ =
base::TimeDelta();
On 2015/09/04 21:59:13, liberato wrote:
> there's a kNoTimestamp that you might want to consider for this case.

Zero timestamp compares nicely at line 807, it was sort of intentional...

https://codereview.chromium.org/1312103006/diff/80001/media/base/android/medi...
File media/base/android/media_codec_decoder.h (right):

https://codereview.chromium.org/1312103006/diff/80001/media/base/android/medi...
media/base/android/media_codec_decoder.h:198: void
SetDecodingUntilOutputIsPresent();
On 2015/09/04 21:59:13, liberato wrote:
> did you mean to delete this?

Yes, thank you, I will delete it.

https://codereview.chromium.org/1312103006/diff/80001/media/base/android/medi...
File media/base/android/media_codec_player.cc (right):

https://codereview.chromium.org/1312103006/diff/80001/media/base/android/medi...
media/base/android/media_codec_player.cc:962: MediaCodecPlayer::StartStatus
MediaCodecPlayer::MaybePrerollDecoders(
On 2015/09/04 21:59:14, liberato wrote:
> now that the preroll timestamp is being maintained differently in the
decoders,
> i'm not sure that we need all of this.  could we always go through the preroll
> state, and get rid of NotCompletedAndNeedsPreroll()?

We do not always perform preroll. For instance, in case of plain pause and
restart we did not do it before this change and do not do it now.

After this change the number of cases where we skip preroll just increased.

Consider e.g. reconfiguration (with drain) on one stream. Now the stream that
drained does the preroll, whereas the other one does nothing, it waits for the
preroll on the other stream to finish. (Then both resume).

Because of that I do not think we could get rid of
NotCompletedAndNeedsPreroll(), this CL heavily relies on this.

I think I could have made the preroll always happen but sometimes do nothing,
but I decided against for the sake of the simplicity of the change.

[Tima Vaisburd, 2015-09-08 19:06:03]

The CQ bit was checked by timav@chromium.org

[Tima Vaisburd, 2015-09-08 19:06:03]

The patchset sent to the CQ was uploaded after l-g-t-m from qinmin@chromium.org,
liberato@chromium.org
Link to the patchset: https://codereview.chromium.org/1312103006/#ps100001
(title: "Rebased")

[commit-bot: I haz the power, 2015-09-08 19:06:49]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1312103006/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1312103006/100001

[commit-bot: I haz the power, 2015-09-08 20:29:27]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-09-08 20:30:08]

Patchset 6 (id:??) landed as
https://crrev.com/4b000195a8d27e9c3c4bfdd77113674840286a48
Cr-Commit-Position: refs/heads/master@{#347780}