media: Fix MediaCodecAudioDecoder reinitialization

media: Fix MediaCodecAudioDecoder reinitialization

According to media::AudioDecoder documentation, a decoder can be 
reinitialized with a different config after it's previously initialized.
In this case, the decoder should drop all internal buffers and reset to
a clean state.

Currently this is not supported by MediaCodecAudioDecoder and running
the newly added test would cause DCHECKs to fire.

This CL fixes MediaCodecAudioDecoder to support reinitialization, where
it will always call SetInitialConfiguration() and CreateMediaCodecLoop()
to initialize the decoder with the new config.

New unit tests are added to cover this case. As part of the process I
also made some cleanup changes in the unit test file.

BUG=679095
TEST=New test added.

Review-Url: https://codereview.chromium.org/2642823004
Cr-Commit-Position: refs/heads/master@{#445280}
Committed: https://chromium.googlesource.com/chromium/src/+/2c1f8ed028edcb44c954cb2a0625a8f278933481

[xhwang, 2017-01-19 01:27:37]

The CQ bit was checked by xhwang@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-19 01:28:22]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-19 01:35:02]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-19 01:35:02]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[xhwang, 2017-01-19 06:31:19]

The CQ bit was checked by xhwang@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-19 06:31:59]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[xhwang, 2017-01-19 06:39:48]

Patchset #1 (id:1) has been deleted

[xhwang, 2017-01-19 06:44:31]

The CQ bit was checked by xhwang@chromium.org to run a CQ dry run

[xhwang, 2017-01-19 06:44:46]

Patchset #1 (id:20001) has been deleted

[commit-bot: I haz the power, 2017-01-19 06:44:49]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[xhwang, 2017-01-19 07:00:21]

The CQ bit was checked by xhwang@chromium.org to run a CQ dry run

[xhwang, 2017-01-19 07:00:44]

Patchset #1 (id:40001) has been deleted

[commit-bot: I haz the power, 2017-01-19 07:00:46]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[xhwang, 2017-01-19 07:00:52]

Patchset #1 (id:60001) has been deleted

[commit-bot: I haz the power, 2017-01-19 07:34:57]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-19 07:34:58]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)

[xhwang, 2017-01-19 08:51:01]

The CQ bit was checked by xhwang@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-19 08:51:24]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-19 09:49:11]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-19 09:49:12]

Dry run: This issue passed the CQ dry run.

[xhwang, 2017-01-19 17:06:38]

Description was changed from

==========
media: Fix MediaCodecAudioDecoder reinitialization

Also added a new test to cover this. As part of the process I also made
some cleanup changes in the test file.

BUG=679095
TEST=New test added.
==========

to

==========
media: Fix MediaCodecAudioDecoder reinitialization

Also added a new test to cover this. As part of the process I also made
some cleanup changes in the test.

BUG=679095
TEST=New test added.
==========

[xhwang, 2017-01-19 17:06:38]

xhwang@chromium.org changed reviewers:
+ dalecurtis@chromium.org

[xhwang, 2017-01-19 17:06:59]

PTAL

[xhwang, 2017-01-19 17:30:55]

Forgot to mention. PS1 will give you a nice diff. PS2 is the same as PS1 except
that I moved code around to keep all test params together. PS3 has some
additional cleanup/fix around the SKIP_TEST* macro. It would be easiest to
follow this order to review this CL.

[xhwang, 2017-01-19 17:31:19]

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

[xhwang, 2017-01-19 17:31:20]

timav: FYI

[DaleCurtis, 2017-01-19 18:09:29]

lgtm

https://codereview.chromium.org/2642823004/diff/120001/media/filters/audio_de...
File media/filters/audio_decoder_unittest.cc (right):

https://codereview.chromium.org/2642823004/diff/120001/media/filters/audio_de...
media/filters/audio_decoder_unittest.cc:151: #if defined(OS_ANDROID)
Could probably use mime_util_internal for this if you wanted.

[Tima Vaisburd, 2017-01-19 21:43:57]

Could you, please, add more details in the CL description, what does this fix
do?

https://codereview.chromium.org/2642823004/diff/120001/media/filters/android/...
File media/filters/android/media_codec_audio_decoder.cc (right):

https://codereview.chromium.org/2642823004/diff/120001/media/filters/android/...
media/filters/android/media_codec_audio_decoder.cc:60: DCHECK_NE(state_,
STATE_WAITING_FOR_MEDIA_CRYPTO);
Shall we do something about STATE_READY?

[xhwang, 2017-01-19 22:34:09]

Description was changed from

==========
media: Fix MediaCodecAudioDecoder reinitialization

Also added a new test to cover this. As part of the process I also made
some cleanup changes in the test.

BUG=679095
TEST=New test added.
==========

to

==========
media: Fix MediaCodecAudioDecoder reinitialization

According to media::AudioDecoder documentation, a decoder can be 
reinitialized with a different config after it's previously initialized
and reset. Currently this is not supported by MediaCodecAudioDecoder
and running the newly added test would cause DCHECKs to fire.

This CL fixes MediaCodecAudioDecoder to support reinitialization. Also
added a new test to cover this. As part of the process I also made
some cleanup changes in the test framework.

BUG=679095
TEST=New test added.
==========

[xhwang, 2017-01-19 22:36:06]

Tima: Updated CL description. PTAL again!

https://codereview.chromium.org/2642823004/diff/120001/media/filters/android/...
File media/filters/android/media_codec_audio_decoder.cc (right):

https://codereview.chromium.org/2642823004/diff/120001/media/filters/android/...
media/filters/android/media_codec_audio_decoder.cc:60: DCHECK_NE(state_,
STATE_WAITING_FOR_MEDIA_CRYPTO);
On 2017/01/19 21:43:57, Tima Vaisburd wrote:
> Shall we do something about STATE_READY?

When it's STATE_READY, we'll just go ahead and recreate the MediaCodecLoop. ISTM
we don't need to do anything special in that case...

[Tima Vaisburd, 2017-01-19 23:22:38]

https://codereview.chromium.org/2642823004/diff/120001/media/filters/android/...
File media/filters/android/media_codec_audio_decoder.cc (right):

https://codereview.chromium.org/2642823004/diff/120001/media/filters/android/...
media/filters/android/media_codec_audio_decoder.cc:60: DCHECK_NE(state_,
STATE_WAITING_FOR_MEDIA_CRYPTO);
On 2017/01/19 21:43:57, Tima Vaisburd wrote:
> Shall we do something about STATE_READY?

Sorry, is the man intention of this CL to allow initialization in STATE_READY?
Then never mind. The details in the description would be helpful though.

https://codereview.chromium.org/2642823004/diff/120001/media/filters/android/...
media/filters/android/media_codec_audio_decoder.cc:60: DCHECK_NE(state_,
STATE_WAITING_FOR_MEDIA_CRYPTO);
On 2017/01/19 22:36:06, xhwang_slow wrote:
> On 2017/01/19 21:43:57, Tima Vaisburd wrote:
> > Shall we do something about STATE_READY?
> 
> When it's STATE_READY, we'll just go ahead and recreate the MediaCodecLoop.
ISTM
> we don't need to do anything special in that case...

The explanation in CL was really helpful :-)
If I get it right, right now nothing prevents the second Initialize() to happen
without Reset(), although description says Reset() should come first. In that
case it seems we'd miss ClearInputQueue(). Is this something to worry about?

[Tima Vaisburd, 2017-01-19 23:25:41]

https://codereview.chromium.org/2642823004/diff/120001/media/filters/android/...
File media/filters/android/media_codec_audio_decoder.cc (right):

https://codereview.chromium.org/2642823004/diff/120001/media/filters/android/...
media/filters/android/media_codec_audio_decoder.cc:60: DCHECK_NE(state_,
STATE_WAITING_FOR_MEDIA_CRYPTO);
On 2017/01/19 23:22:38, Tima Vaisburd wrote:
>  Sorry, is the man intention of this CL to allow initialization in
STATE_READY?
>  Then never mind. The details in the description would be helpful though.

Silly, silly rietveld and silly, silly me! Please ignore this particular
comment.

[xhwang, 2017-01-20 18:25:56]

Description was changed from

==========
media: Fix MediaCodecAudioDecoder reinitialization

According to media::AudioDecoder documentation, a decoder can be 
reinitialized with a different config after it's previously initialized
and reset. Currently this is not supported by MediaCodecAudioDecoder
and running the newly added test would cause DCHECKs to fire.

This CL fixes MediaCodecAudioDecoder to support reinitialization. Also
added a new test to cover this. As part of the process I also made
some cleanup changes in the test framework.

BUG=679095
TEST=New test added.
==========

to

==========
media: Fix MediaCodecAudioDecoder reinitialization

According to media::AudioDecoder documentation, a decoder can be 
reinitialized with a different config after it's previously initialized.
In this case, the decoder should drop all internal buffers and reset to
a clean state.

Currently this is not supported by MediaCodecAudioDecoder and running
the newly added test would cause DCHECKs to fire.

This CL fixes MediaCodecAudioDecoder to support reinitialization, where
it will always call SetInitialConfiguration() and CreateMediaCodecLoop()
to initialize the decoder with the new config.

New unit tests are added to cover this case. As part of the process I
also made some cleanup changes in the unit test file.

BUG=679095
TEST=New test added.
==========

[xhwang, 2017-01-20 18:49:03]

https://codereview.chromium.org/2642823004/diff/120001/media/filters/android/...
File media/filters/android/media_codec_audio_decoder.cc (right):

https://codereview.chromium.org/2642823004/diff/120001/media/filters/android/...
media/filters/android/media_codec_audio_decoder.cc:60: DCHECK_NE(state_,
STATE_WAITING_FOR_MEDIA_CRYPTO);
On 2017/01/19 23:22:38, Tima Vaisburd wrote:
> On 2017/01/19 22:36:06, xhwang_slow wrote:
> > On 2017/01/19 21:43:57, Tima Vaisburd wrote:
> > > Shall we do something about STATE_READY?
> > 
> > When it's STATE_READY, we'll just go ahead and recreate the MediaCodecLoop.
> ISTM
> > we don't need to do anything special in that case...
> 
> The explanation in CL was really helpful :-)
> If I get it right, right now nothing prevents the second Initialize() to
happen
> without Reset(), although description says Reset() should come first. 

Sorry I got confused about the API as well. I read it again and it turns out we
don't necessarily call Reset() before reinitialization. The only cases we cannot
call it are during pending decode or pending reset.

When reinit happens, we should drop all internal buffers and reconfig the
decoder using the new config.

> In that case it seems we'd miss ClearInputQueue(). Is this something to worry
about?

Good point! Since we should not reinitialize during pending decode, the input
queue should be empty. I added a DCHECK() to cover this.

Also, I didn't call codec_loop_->TryFlush() since CreateMediaCodecLoop() should
cover it. Is that correct?

[xhwang, 2017-01-20 19:31:40]

The CQ bit was checked by xhwang@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-20 19:32:36]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Tima Vaisburd, 2017-01-20 20:02:28]

https://codereview.chromium.org/2642823004/diff/120001/media/filters/android/...
File media/filters/android/media_codec_audio_decoder.cc (right):

https://codereview.chromium.org/2642823004/diff/120001/media/filters/android/...
media/filters/android/media_codec_audio_decoder.cc:60: DCHECK_NE(state_,
STATE_WAITING_FOR_MEDIA_CRYPTO);
On 2017/01/20 18:49:03, xhwang_slow wrote:
> Also, I didn't call codec_loop_->TryFlush() since CreateMediaCodecLoop()
should
> cover it. Is that correct?

Yes, as far as I understand CreateMediaCodecLoop() fully destroys and recreates
MediaCodec.

> Since we should not reinitialize during pending decode...

As well you can just call ClearInputQueue() instead of DCHECK(). Would it hurt
anything?

lgtm

[xhwang, 2017-01-20 20:07:49]

https://codereview.chromium.org/2642823004/diff/120001/media/filters/android/...
File media/filters/android/media_codec_audio_decoder.cc (right):

https://codereview.chromium.org/2642823004/diff/120001/media/filters/android/...
media/filters/android/media_codec_audio_decoder.cc:60: DCHECK_NE(state_,
STATE_WAITING_FOR_MEDIA_CRYPTO);
On 2017/01/20 20:02:27, Tima Vaisburd wrote:
> On 2017/01/20 18:49:03, xhwang_slow wrote:
> > Also, I didn't call codec_loop_->TryFlush() since CreateMediaCodecLoop()
> should
> > cover it. Is that correct?
> 
> Yes, as far as I understand CreateMediaCodecLoop() fully destroys and
recreates
> MediaCodec.
> 
> > Since we should not reinitialize during pending decode...
> 
> As well you can just call ClearInputQueue() instead of DCHECK(). Would it hurt
> anything?
> 
> lgtm
> 

DCHECK() helps us enforce the API contract and helps reader understand the
assumptions. So I would like to keep it.

Added ClearInputQueue() just in case we have bugs in release build. Though we
don't usually do it :)

[xhwang, 2017-01-20 20:07:53]

The CQ bit was checked by xhwang@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-20 20:08:34]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-20 21:11:38]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-20 21:11:39]

Dry run: This issue passed the CQ dry run.

[Tima Vaisburd, 2017-01-20 21:11:44]

On 2017/01/20 20:07:49, xhwang_slow wrote:
>
https://codereview.chromium.org/2642823004/diff/120001/media/filters/android/...
> File media/filters/android/media_codec_audio_decoder.cc (right):
> 
>
https://codereview.chromium.org/2642823004/diff/120001/media/filters/android/...
> media/filters/android/media_codec_audio_decoder.cc:60: DCHECK_NE(state_,
> STATE_WAITING_FOR_MEDIA_CRYPTO);
> On 2017/01/20 20:02:27, Tima Vaisburd wrote:
> > On 2017/01/20 18:49:03, xhwang_slow wrote:
> > > Also, I didn't call codec_loop_->TryFlush() since CreateMediaCodecLoop()
> > should
> > > cover it. Is that correct?
> > 
> > Yes, as far as I understand CreateMediaCodecLoop() fully destroys and
> recreates
> > MediaCodec.
> > 
> > > Since we should not reinitialize during pending decode...
> > 
> > As well you can just call ClearInputQueue() instead of DCHECK(). Would it
hurt
> > anything?
> > 
> > lgtm
> > 
> 
> DCHECK() helps us enforce the API contract and helps reader understand the
> assumptions. So I would like to keep it.
> 
> Added ClearInputQueue() just in case we have bugs in release build. Though we
> don't usually do it :)

Yes, I remember reading about "either assert, or handle the condition, but not
both" and the arguments were quite convincing!
I meant either or, it's up to you. I trust your judgement :-)

[xhwang, 2017-01-21 00:58:32]

The CQ bit was checked by xhwang@chromium.org

[xhwang, 2017-01-21 00:58:33]

The patchset sent to the CQ was uploaded after l-g-t-m from
dalecurtis@chromium.org, timav@chromium.org
Link to the patchset: https://codereview.chromium.org/2642823004/#ps160001
(title: "media: Fix MediaCodecAudioDecoder reinitialization")

[commit-bot: I haz the power, 2017-01-21 00:58:51]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-21 01:06:02]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-21 01:06:03]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[xhwang, 2017-01-21 04:47:12]

The CQ bit was checked by xhwang@chromium.org

[commit-bot: I haz the power, 2017-01-21 04:47:25]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-21 04:52:42]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-21 04:52:43]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[xhwang, 2017-01-21 05:49:47]

The CQ bit was checked by xhwang@chromium.org

[xhwang, 2017-01-21 05:49:47]

The patchset sent to the CQ was uploaded after l-g-t-m from timav@chromium.org,
dalecurtis@chromium.org
Link to the patchset: https://codereview.chromium.org/2642823004/#ps180001
(title: "rebase")

[commit-bot: I haz the power, 2017-01-21 05:49:58]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-21 05:51:29]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-21 05:51:30]

Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[xhwang, 2017-01-21 06:35:13]

The CQ bit was checked by xhwang@chromium.org

[xhwang, 2017-01-21 06:35:13]

The patchset sent to the CQ was uploaded after l-g-t-m from timav@chromium.org,
dalecurtis@chromium.org
Link to the patchset: https://codereview.chromium.org/2642823004/#ps200001
(title: "rebase")

[commit-bot: I haz the power, 2017-01-21 06:35:26]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-21 07:58:00]

CQ is committing da patch.

Bot data: {"patchset_id": 200001, "attempt_start_ts": 1484980513118030,
"parent_rev": "1450a5e9034e2202045880234d886e58ad4b3a52", "commit_rev":
"2c1f8ed028edcb44c954cb2a0625a8f278933481"}

[commit-bot: I haz the power, 2017-01-21 07:58:31]

Description was changed from

==========
media: Fix MediaCodecAudioDecoder reinitialization

According to media::AudioDecoder documentation, a decoder can be 
reinitialized with a different config after it's previously initialized.
In this case, the decoder should drop all internal buffers and reset to
a clean state.

Currently this is not supported by MediaCodecAudioDecoder and running
the newly added test would cause DCHECKs to fire.

This CL fixes MediaCodecAudioDecoder to support reinitialization, where
it will always call SetInitialConfiguration() and CreateMediaCodecLoop()
to initialize the decoder with the new config.

New unit tests are added to cover this case. As part of the process I
also made some cleanup changes in the unit test file.

BUG=679095
TEST=New test added.
==========

to

==========
media: Fix MediaCodecAudioDecoder reinitialization

According to media::AudioDecoder documentation, a decoder can be 
reinitialized with a different config after it's previously initialized.
In this case, the decoder should drop all internal buffers and reset to
a clean state.

Currently this is not supported by MediaCodecAudioDecoder and running
the newly added test would cause DCHECKs to fire.

This CL fixes MediaCodecAudioDecoder to support reinitialization, where
it will always call SetInitialConfiguration() and CreateMediaCodecLoop()
to initialize the decoder with the new config.

New unit tests are added to cover this case. As part of the process I
also made some cleanup changes in the unit test file.

BUG=679095
TEST=New test added.

Review-Url: https://codereview.chromium.org/2642823004
Cr-Commit-Position: refs/heads/master@{#445280}
Committed:
https://chromium.googlesource.com/chromium/src/+/2c1f8ed028edcb44c954cb2a0625...
==========

[commit-bot: I haz the power, 2017-01-21 07:58:32]

Committed patchset #7 (id:200001) as
https://chromium.googlesource.com/chromium/src/+/2c1f8ed028edcb44c954cb2a0625...