media: Lazily reconfigure MediaCodecs in AVDA for JellyBean

media: Lazily reconfigure MediaCodecs in AVDA for JellyBean

Previously, on Jellybean only, AVDA::Reset() released its MediaCodec
and created a new one. MediaCodec creation can take 100-200 ms so
to avoid doing unless it's necessary, now it will be created lazily
on the next call to Decode(). 

BUG=599288
TEST=manual

[watk, 2016-03-24 01:02:59]

https://codereview.chromium.org/1828033002/diff/1/content/common/gpu/media/an...
File content/common/gpu/media/android_video_decode_accelerator.cc (right):

https://codereview.chromium.org/1828033002/diff/1/content/common/gpu/media/an...
content/common/gpu/media/android_video_decode_accelerator.cc:414: } else if
(state_ == PENDING_CODEC_RECONFIGURE) {
Should probably put this in Decode().

https://codereview.chromium.org/1828033002/diff/1/content/common/gpu/media/an...
content/common/gpu/media/android_video_decode_accelerator.cc:564: return false;
This has to be moved because otherwise we might pop an input buffer, and never
NotifyEnded, which will get GVD out of sync.

[ddorwin, 2016-03-24 02:14:05]

ddorwin@chromium.org changed reviewers:
+ ddorwin@chromium.org

[ddorwin, 2016-03-24 02:14:06]

drive-by comments:
* Please explain why, especially why only on <18. in the description and/or a
bug.
* It's not clear how this only applies to <18
* nit: Please spell out level (even if it is longer than the recommended Git
commit characters)

[watk, 2016-03-24 02:36:10]

On 2016/03/24 02:14:06, ddorwin wrote:
> drive-by comments:
> * Please explain why, especially why only on <18. in the description and/or a
> bug.
> * It's not clear how this only applies to <18
> * nit: Please spell out level (even if it is longer than the recommended Git
> commit characters)

Sorry, I didn't really mean to publish this. I'm not sure if we're going to go
ahead with this or not yet.

[liberato (no reviews please), 2016-03-29 14:54:39]

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

[liberato (no reviews please), 2016-03-29 14:54:40]

https://codereview.chromium.org/1828033002/diff/1/content/common/gpu/media/an...
File content/common/gpu/media/android_video_decode_accelerator.cc (right):

https://codereview.chromium.org/1828033002/diff/1/content/common/gpu/media/an...
content/common/gpu/media/android_video_decode_accelerator.cc:414: } else if
(state_ == PENDING_CODEC_RECONFIGURE) {
On 2016/03/24 01:02:58, watk wrote:
> Should probably put this in Decode().

probably, yes, since the codec can't take any action until a decode comes in.

alternatively, if you keep it here, then it might be a good idea to see if
|pending_bitstream_buffers_| is not empty before allocating the codec.  we call
DoIOTask a lot, though maybe not after a Reset.  still, makes more sense to
check if there's input to the codec before we bother.  then i think it's the
same as in Decode

https://codereview.chromium.org/1828033002/diff/1/content/common/gpu/media/an...
content/common/gpu/media/android_video_decode_accelerator.cc:925: 
should this early out here if |state_| is already PENDING_CODEC_RECONFIGURE?  i
think it works as written, but it seems like a good idea anyway.  none of this
code was written with the idea that !|media_coded_|.  no need to re-notify the
strategy, nor re-stop the timer.

i'm not sure that it's even possible to get into this state right now. maybe one
can via EOS due to format changed, followed by a Reset before that completes. 
either way, that's not obvious and somewhat fragile, so an explicit early out
after clearing the buffers might be worthwhile.

https://codereview.chromium.org/1828033002/diff/1/content/common/gpu/media/an...
content/common/gpu/media/android_video_decode_accelerator.cc:954: state_ =
PENDING_CODEC_RECONFIGURE;
you might consider not deferring the reset in the case of a call to
ResetCodecState for a format change -- it's a slow process, and we know that
more decodes will happen.  might as well start sooner to hide the latency. 
right now, it doesn't make much difference, because it's all synchronous.  with
async init, it could actually proceed in parallel.

alternatively, please add a comment about it here, else i'll forget to change it
later.  :)

[watk, 2016-03-30 23:21:28]

Patchset #2 (id:20001) has been deleted

[watk, 2016-03-30 23:40:20]

Description was changed from

==========
media: Lazily reconfigure MediaCodecs in AVDA for API lvl < 18



BUG=
==========

to

==========
media: Lazily reconfigure MediaCodecs in AVDA for JellyBean

Previously, on Jellybean only, AVDA::Reset() released its MediaCodec
and created a new one. MediaCodec creation can take 100-200 ms so
to avoid doing unless it's necessary, now it will be created lazily
on the next call to Decode(). 

BUG=599288
TEST=manual
==========

[liberato (no reviews please), 2016-03-31 16:24:56]

lgtm.

[watk, 2016-05-24 19:44:15]

Closing in favor of https://codereview.chromium.org/2000833003/

[watk, 2016-05-24 19:44:18]

Closing in favor of https://codereview.chromium.org/2000833003/