media: AVDA now doesn't queue EOS for a flush if the codec is empty

media: AVDA now doesn't queue EOS for a flush if the codec is empty

Apparently some MediaCodecs consider it an error to receive an EOS
buffer as their first input. So now we skip queueing the EOS if there
are no pending buffers in the decoder and no pending inputs.

BUG=672268
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel

Committed: https://crrev.com/89afd26f42c7a417841e1657499f5216262906d2
Cr-Commit-Position: refs/heads/master@{#437436}

[watk, 2016-12-07 23:11:52]

Description was changed from

==========
media: AVDA now doesn't queue EOS for a flush if the codec is empty

Apparently some MediaCodecs consider it an error to receive an EOS
buffer as their first input. So now we skip queueing the EOS if there
are no pending buffers in the decoder and no pending inputs.

BUG=672268
==========

to

==========
media: AVDA now doesn't queue EOS for a flush if the codec is empty

Apparently some MediaCodecs consider it an error to receive an EOS
buffer as their first input. So now we skip queueing the EOS if there
are no pending buffers in the decoder and no pending inputs.

BUG=672268
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[watk, 2016-12-07 23:11:58]

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

[commit-bot: I haz the power, 2016-12-07 23:12:25]

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

[watk, 2016-12-07 23:30:31]

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

[watk, 2016-12-07 23:30:32]

https://codereview.chromium.org/2563533002/diff/1/media/gpu/android_video_dec...
File media/gpu/android_video_decode_accelerator.cc (left):

https://codereview.chromium.org/2563533002/diff/1/media/gpu/android_video_dec...
media/gpu/android_video_decode_accelerator.cc:868: if (state_ ==
SURFACE_DESTROYED || defer_surface_creation_)
These both imply media_codec == nullptr so I used that directly below.

[DaleCurtis, 2016-12-08 00:11:14]

lgtm

https://codereview.chromium.org/2563533002/diff/1/media/gpu/android_video_dec...
File media/gpu/android_video_decode_accelerator.cc (right):

https://codereview.chromium.org/2563533002/diff/1/media/gpu/android_video_dec...
media/gpu/android_video_decode_accelerator.cc:969: // reset because MediaCodec
might hang in release() or flush() if we don't.
Comment is a bit misleading, code skips drain if there's nothing in it even for
vp8.

https://codereview.chromium.org/2563533002/diff/1/media/gpu/android_video_dec...
media/gpu/android_video_decode_accelerator.cc:971: if (!media_codec_ ||
(pending_bitstream_records_.empty() &&
Maybe clarify that in the case of Reset() pending bitstream records is already
empty by this point?

[commit-bot: I haz the power, 2016-12-08 00:23:22]

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

[commit-bot: I haz the power, 2016-12-08 00:23:23]

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

[DaleCurtis, 2016-12-08 00:29:35]

https://codereview.chromium.org/2563533002/diff/1/media/gpu/android_video_dec...
File media/gpu/android_video_decode_accelerator.cc (right):

https://codereview.chromium.org/2563533002/diff/1/media/gpu/android_video_dec...
media/gpu/android_video_decode_accelerator.cc:1125: while
(!pending_bitstream_records_.empty())
Because I was curious and have been benchmarking too many things lately, on my
z620:

std::queue<int>().swap(queue); ~50 microseconds for 100000 objects versus ~100
microseconds for the while() pop version. Just as an FYI in case it ever matters
(it certainly doesn't here).

[watk, 2016-12-08 02:24:52]

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

[commit-bot: I haz the power, 2016-12-08 02:25:19]

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

[watk, 2016-12-08 02:29:44]

PTAL

https://codereview.chromium.org/2563533002/diff/20001/media/gpu/android_video...
File media/gpu/android_video_decode_accelerator.cc (right):

https://codereview.chromium.org/2563533002/diff/20001/media/gpu/android_video...
media/gpu/android_video_decode_accelerator.cc:1032: !media_codec_)
This was caught by the unittest!! yay for CQ tests.

Previously when we skipped the drain for flush and destroy we never called
ResetCodecState. Now the destruction path always calls ResetCodecState. I don't
know if this is ideal, but we have the waiting for codec state as well.

https://codereview.chromium.org/2563533002/diff/20001/media/gpu/android_video...
media/gpu/android_video_decode_accelerator.cc:1130:
std::queue<BitstreamRecord>().swap(pending_bitstream_records_);
I actually really disliked the pop() method anyway, so changing it now :)

[watk, 2016-12-08 02:30:27]

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

[commit-bot: I haz the power, 2016-12-08 02:30:48]

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, 2016-12-08 03:52:59]

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

[commit-bot: I haz the power, 2016-12-08 03:53:00]

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

[watk, 2016-12-08 18:56:08]

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

[commit-bot: I haz the power, 2016-12-08 18:56:41]

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

[DaleCurtis, 2016-12-08 19:08:40]

lgtm

https://codereview.chromium.org/2563533002/diff/20001/media/gpu/android_video...
File media/gpu/android_video_decode_accelerator.cc (right):

https://codereview.chromium.org/2563533002/diff/20001/media/gpu/android_video...
media/gpu/android_video_decode_accelerator.cc:1032: !media_codec_)
On 2016/12/08 at 02:29:44, watk wrote:
> This was caught by the unittest!! yay for CQ tests.
> 
> Previously when we skipped the drain for flush and destroy we never called
ResetCodecState. Now the destruction path always calls ResetCodecState. I don't
know if this is ideal, but we have the waiting for codec state as well.

Multiline if needs {}

[watk, 2016-12-08 19:45:08]

The CQ bit was checked by watk@chromium.org

[watk, 2016-12-08 19:45:08]

The patchset sent to the CQ was uploaded after l-g-t-m from
dalecurtis@chromium.org
Link to the patchset: https://codereview.chromium.org/2563533002/#ps40001
(title: "braces")

[commit-bot: I haz the power, 2016-12-08 19:45:42]

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, 2016-12-08 21:22:36]

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

[commit-bot: I haz the power, 2016-12-08 21:22:37]

Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[AndyWu, 2016-12-08 21:27:02]

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

[AndyWu, 2016-12-08 21:27:03]

https://codereview.chromium.org/2563533002/diff/20001/media/gpu/android_video...
File media/gpu/android_video_decode_accelerator.cc (right):

https://codereview.chromium.org/2563533002/diff/20001/media/gpu/android_video...
media/gpu/android_video_decode_accelerator.cc:1005: switch (*drain_type_) {
enumeration value 'DRAIN_TYPE_NONE' not handled in switch

[watk, 2016-12-08 22:19:46]

https://codereview.chromium.org/2563533002/diff/20001/media/gpu/android_video...
File media/gpu/android_video_decode_accelerator.cc (right):

https://codereview.chromium.org/2563533002/diff/20001/media/gpu/android_video...
media/gpu/android_video_decode_accelerator.cc:1005: switch (*drain_type_) {
On 2016/12/08 21:27:02, AndyWu wrote:
> enumeration value 'DRAIN_TYPE_NONE' not handled in switch

NONE was removed from the enum and is now represented by an optional. Was this a
compiler warning somehow?

[AndyWu, 2016-12-08 22:30:30]

> NONE was removed from the enum and is now represented by an optional. Was this
a
> compiler warning somehow?

Oh, maybe I didn't merge the code right, sorry about that.

[watk, 2016-12-09 01:14:01]

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

[commit-bot: I haz the power, 2016-12-09 01:14:23]

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, 2016-12-09 01:36:10]

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

[commit-bot: I haz the power, 2016-12-09 01:36:11]

Dry run: This issue passed the CQ dry run.

[watk, 2016-12-09 01:39:08]

The CQ bit was checked by watk@chromium.org

[commit-bot: I haz the power, 2016-12-09 01:39:46]

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, 2016-12-09 02:22:38]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1481247548447210,
"parent_rev": "e2dae70ef6e0e621937df62e0b045247b7dd26a9", "commit_rev":
"9760d8f4be887e6fab39482fe3d66adf993e7110"}

[commit-bot: I haz the power, 2016-12-09 02:23:27]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-12-09 02:25:47]

Description was changed from

==========
media: AVDA now doesn't queue EOS for a flush if the codec is empty

Apparently some MediaCodecs consider it an error to receive an EOS
buffer as their first input. So now we skip queueing the EOS if there
are no pending buffers in the decoder and no pending inputs.

BUG=672268
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
media: AVDA now doesn't queue EOS for a flush if the codec is empty

Apparently some MediaCodecs consider it an error to receive an EOS
buffer as their first input. So now we skip queueing the EOS if there
are no pending buffers in the decoder and no pending inputs.

BUG=672268
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel

Committed: https://crrev.com/89afd26f42c7a417841e1657499f5216262906d2
Cr-Commit-Position: refs/heads/master@{#437436}
==========

[commit-bot: I haz the power, 2016-12-09 02:25:48]

Patchset 3 (id:??) landed as
https://crrev.com/89afd26f42c7a417841e1657499f5216262906d2
Cr-Commit-Position: refs/heads/master@{#437436}