MediaCodecPlayer implementation (stage 3 - browser seek and surface change)

MediaCodecPlayer implementation (stage 3 - browser seek and surface change)

We initiate a browser seek if at the time of video codec
reconfiguration the frame that would be the first one is not
a key frame. This usually happens when we replace video surface
in the middle of the playback.

In this implementation when the video surface is changed during
the playback both audio and video streams are stopped and then both
are restarted.

BUG=407577
Committed: https://crrev.com/0b80663bae09866fb8a5aa5431312c957e3f0f4e
Cr-Commit-Position: refs/heads/master@{#341223}

[Tima Vaisburd, 2015-07-20 23:13:23]

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

[Tima Vaisburd, 2015-07-20 23:13:23]

Min, this is my first attempt at the browser seek and surface change. Could you,
please, take a look?

[qinmin, 2015-07-22 17:30:33]

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

https://codereview.chromium.org/1242913004/diff/60001/media/base/android/medi...
media/base/android/media_codec_player.cc:152: StopDecoders();  // synchronous
stop
we only need to do this if video is available.

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

https://codereview.chromium.org/1242913004/diff/60001/media/base/android/medi...
media/base/android/media_codec_video_decoder.cc:247: void
MediaCodecVideoDecoder::ClearDelayedBuffers() {
I think you can merge this function with ReleaseDelayedBuffers(), something like
ClearDelayedBuffers(bool release_output_buffer)

[Tima Vaisburd, 2015-07-23 00:37:49]

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

https://codereview.chromium.org/1242913004/diff/60001/media/base/android/medi...
media/base/android/media_codec_player.cc:152: StopDecoders();  // synchronous
stop
On 2015/07/22 17:30:33, qinmin wrote:
> we only need to do this if video is available.

I added checks in both remove and replace sections.

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

https://codereview.chromium.org/1242913004/diff/60001/media/base/android/medi...
media/base/android/media_codec_video_decoder.cc:247: void
MediaCodecVideoDecoder::ClearDelayedBuffers() {
On 2015/07/22 17:30:33, qinmin wrote:
> I think you can merge this function with ReleaseDelayedBuffers(), something
like
> ClearDelayedBuffers(bool release_output_buffer)

Done.

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

https://codereview.chromium.org/1242913004/diff/80001/media/base/android/medi...
media/base/android/media_codec_decoder.cc:398: // Ignore codec errors from the
moment surface is changed till the
Also added this check.

[qinmin, 2015-07-24 00:02:40]

lgtm

[Tima Vaisburd, 2015-07-24 23:39:43]

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

[Tima Vaisburd, 2015-07-24 23:39:44]

Adding Matt. This CL currently lacks the fix for Release() coming in the
STATE_WAITING_FOR_SEEK.

[Tima Vaisburd, 2015-07-28 22:56:44]

I modified the Release to not interfere with SeekTo processing and added
corresponding unit tests. Please take a look.

https://codereview.chromium.org/1242913004/diff/120001/media/base/android/med...
File media/base/android/media_codec_player.cc (right):

https://codereview.chromium.org/1242913004/diff/120001/media/base/android/med...
media/base/android/media_codec_player.cc:296: // Stop decoding threads and
delete MediaCodecs, but keep IPC between browser
Changed this Release().

https://codereview.chromium.org/1242913004/diff/120001/media/base/android/med...
media/base/android/media_codec_player.cc:418: DCHECK(state_ ==
STATE_WAITING_FOR_SEEK);
Added this DCHECK instead of ignoring the signal if not in the expected state.

[wolenetz, 2015-07-29 22:37:03]

lgtm % nits

https://codereview.chromium.org/1242913004/diff/120001/media/base/android/med...
File media/base/android/media_codec_decoder.cc (right):

https://codereview.chromium.org/1242913004/diff/120001/media/base/android/med...
media/base/android/media_codec_decoder.cc:200: // Shall we move
|delayed_buffers_| from VideoDecoder to Decoder class?
Seems there should be either a TODO or a CR comment instead of a question here.

https://codereview.chromium.org/1242913004/diff/120001/media/base/android/med...
media/base/android/media_codec_decoder.cc:294: // Shall we move
|delayed_buffers_| from VideoDecoder to Decoder class?
ditto

https://codereview.chromium.org/1242913004/diff/120001/media/base/android/med...
File media/base/android/media_codec_decoder.h (right):

https://codereview.chromium.org/1242913004/diff/120001/media/base/android/med...
media/base/android/media_codec_decoder.h:35: // Decoder's internal state machine
goes through the following states:
nit: InEmergencyStop is missing from these diagrams

https://codereview.chromium.org/1242913004/diff/120001/media/base/android/med...
media/base/android/media_codec_decoder.h:109: CONFIG_FAILURE = 0,
Hmm. Here and elsewhere in the new mediacodecplayer/decoder/etc, please use
kConstant, not MACRO, naming style for enumerated values (Chromium style guide:
"New code should prefer constant-style naming if possible").

https://codereview.chromium.org/1242913004/diff/120001/media/base/android/med...
File media/base/android/media_codec_player.cc (right):

https://codereview.chromium.org/1242913004/diff/120001/media/base/android/med...
media/base/android/media_codec_player.cc:530: // This is an error, initial
configuration is expected
nit: we might want to DCHECK that internal_error_cb_ is indeed set to something
(e.g. that CreateDecoders() has done that setup...)

[wolenetz, 2015-07-29 22:37:04]

lgtm % nits

[Tima Vaisburd, 2015-07-30 20:28:36]

Added a fix for potential Flash() error, please take another look.

https://codereview.chromium.org/1242913004/diff/120001/media/base/android/med...
File media/base/android/media_codec_decoder.cc (right):

https://codereview.chromium.org/1242913004/diff/120001/media/base/android/med...
media/base/android/media_codec_decoder.cc:200: // Shall we move
|delayed_buffers_| from VideoDecoder to Decoder class?
On 2015/07/29 22:37:02, wolenetz wrote:
> Seems there should be either a TODO or a CR comment instead of a question
here.

Yes, but now I tend to think that having a method is actually cleaner in this
situation, so I will just delete the comment in the code, and the CR comment
already exists.

https://codereview.chromium.org/1242913004/diff/120001/media/base/android/med...
media/base/android/media_codec_decoder.cc:294: // Shall we move
|delayed_buffers_| from VideoDecoder to Decoder class?
On 2015/07/29 22:37:02, wolenetz wrote:
> ditto

Same as above, removed the comment in the code as I came to like
ClearDelayerBuffers().

https://codereview.chromium.org/1242913004/diff/120001/media/base/android/med...
File media/base/android/media_codec_decoder.h (right):

https://codereview.chromium.org/1242913004/diff/120001/media/base/android/med...
media/base/android/media_codec_decoder.h:35: // Decoder's internal state machine
goes through the following states:
On 2015/07/29 22:37:03, wolenetz wrote:
> nit: InEmergencyStop is missing from these diagrams

Done.

https://codereview.chromium.org/1242913004/diff/120001/media/base/android/med...
media/base/android/media_codec_decoder.h:109: CONFIG_FAILURE = 0,
On 2015/07/29 22:37:02, wolenetz wrote:
> Hmm. Here and elsewhere in the new mediacodecplayer/decoder/etc, please use
> kConstant, not MACRO, naming style for enumerated values (Chromium style
guide:
> "New code should prefer constant-style naming if possible").

Done.

https://codereview.chromium.org/1242913004/diff/120001/media/base/android/med...
File media/base/android/media_codec_player.cc (right):

https://codereview.chromium.org/1242913004/diff/120001/media/base/android/med...
media/base/android/media_codec_player.cc:530: // This is an error, initial
configuration is expected
On 2015/07/29 22:37:03, wolenetz wrote:
> nit: we might want to DCHECK that internal_error_cb_ is indeed set to
something
> (e.g. that CreateDecoders() has done that setup...)

Done.

https://codereview.chromium.org/1242913004/diff/140001/media/base/android/med...
File media/base/android/media_codec_player.cc (right):

https://codereview.chromium.org/1242913004/diff/140001/media/base/android/med...
media/base/android/media_codec_player.cc:437: if (state_ == kStateError) {
This is the new error state handling.

[wolenetz, 2015-07-30 21:10:39]

https://codereview.chromium.org/1242913004/diff/120001/media/base/android/med...
File media/base/android/media_codec_decoder.cc (right):

https://codereview.chromium.org/1242913004/diff/120001/media/base/android/med...
media/base/android/media_codec_decoder.cc:200: // Shall we move
|delayed_buffers_| from VideoDecoder to Decoder class?
On 2015/07/30 20:28:35, Tima Vaisburd wrote:
> On 2015/07/29 22:37:02, wolenetz wrote:
> > Seems there should be either a TODO or a CR comment instead of a question
> here.
> 
> Yes, but now I tend to think that having a method is actually cleaner in this
> situation, so I will just delete the comment in the code, and the CR comment
> already exists.

Acknowledged.

https://codereview.chromium.org/1242913004/diff/120001/media/base/android/med...
media/base/android/media_codec_decoder.cc:294: // Shall we move
|delayed_buffers_| from VideoDecoder to Decoder class?
On 2015/07/30 20:28:35, Tima Vaisburd wrote:
> On 2015/07/29 22:37:02, wolenetz wrote:
> > ditto
> 
> Same as above, removed the comment in the code as I came to like
> ClearDelayerBuffers().

Acknowledged.

https://codereview.chromium.org/1242913004/diff/140001/media/base/android/med...
File media/base/android/media_codec_player.cc (right):

https://codereview.chromium.org/1242913004/diff/140001/media/base/android/med...
media/base/android/media_codec_player.cc:437: if (state_ == kStateError) {
On 2015/07/30 20:28:36, Tima Vaisburd wrote:
> This is the new error state handling.

I'm having difficulty understanding this scenario. Is this a pre-existing defect
of MediaSourcePlayer, too? And for desktop Pipeline.cc?

What is the net effect of signaling seek done back to WMPA (if somehow we are in
error state when we receive notice that the demuxer has finished a requested
seek) versus not signaling completion of the seek? I think what you have here is
more correct, I just would like to understand more clearly the impact.

https://codereview.chromium.org/1242913004/diff/140001/media/base/android/med...
media/base/android/media_codec_player.cc:447: DCHECK(state_ ==
kStateWaitingForSeek);
nit: DCHECK_EQ(kStateWaitingForSeek, state_);

https://codereview.chromium.org/1242913004/diff/140001/media/base/android/med...
media/base/android/media_codec_player.cc:893: DCHECK(state_ ==
kStateWaitingForSeek);
nit ditto

[wolenetz, 2015-07-30 21:41:10]

lgtm % 2 previous nits

https://codereview.chromium.org/1242913004/diff/140001/media/base/android/med...
File media/base/android/media_codec_player.cc (right):

https://codereview.chromium.org/1242913004/diff/140001/media/base/android/med...
media/base/android/media_codec_player.cc:437: if (state_ == kStateError) {
On 2015/07/30 21:10:39, wolenetz wrote:
> On 2015/07/30 20:28:36, Tima Vaisburd wrote:
> > This is the new error state handling.
> 
> I'm having difficulty understanding this scenario. Is this a pre-existing
defect
> of MediaSourcePlayer, too? And for desktop Pipeline.cc?
> 
> What is the net effect of signaling seek done back to WMPA (if somehow we are
in
> error state when we receive notice that the demuxer has finished a requested
> seek) versus not signaling completion of the seek? I think what you have here
is
> more correct, I just would like to understand more clearly the impact.

I looked further into desktop WMPI and Pipeline behavior when error is
encountered during a seek operation. tl;dr: signaling seek_done_cb_ here sgtm
and is more in parity with desktop implementation of Pipeline.

Detail:
desktop uses a SerialRunner to process the various steps in a seek. Any step
(including flush) that results in error (anything but PIPELINE_OK) cancels the
remaining steps. There's some funkiness around pending seek in WMPI too, but the
seek completion callback into WMPI is called even in the case of error. So I
think your change is not regressing.

lgtm % previous 2 nits

[Tima Vaisburd, 2015-07-30 22:25:32]

https://codereview.chromium.org/1242913004/diff/140001/media/base/android/med...
File media/base/android/media_codec_player.cc (right):

https://codereview.chromium.org/1242913004/diff/140001/media/base/android/med...
media/base/android/media_codec_player.cc:447: DCHECK(state_ ==
kStateWaitingForSeek);
On 2015/07/30 21:10:39, wolenetz wrote:
> nit: DCHECK_EQ(kStateWaitingForSeek, state_);

Done.

https://codereview.chromium.org/1242913004/diff/140001/media/base/android/med...
media/base/android/media_codec_player.cc:893: DCHECK(state_ ==
kStateWaitingForSeek);
On 2015/07/30 21:10:39, wolenetz wrote:
> nit ditto

Done.

[Tima Vaisburd, 2015-07-30 22:25:41]

The CQ bit was checked by timav@chromium.org

[Tima Vaisburd, 2015-07-30 22:25:41]

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

[commit-bot: I haz the power, 2015-07-30 22:26:03]

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

[commit-bot: I haz the power, 2015-07-30 23:29:53]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2015-07-30 23:30:43]

Patchset 9 (id:??) landed as
https://crrev.com/0b80663bae09866fb8a5aa5431312c957e3f0f4e
Cr-Commit-Position: refs/heads/master@{#341223}