media: Don't create a new MediaCodec during AVDA teardown

media: Don't create a new MediaCodec during AVDA teardown

OnDrainCompleted() is always called during AVDA teardown, and it
previously always called ResetCodecState() which resulted in
unnecessarily creating new MediaCodecs during shutdown whenever state_
was ERROR, or on older devices where flush() doesn't work.

Now we don't bother calling ResetCodecState() during destruction, and
we also don't try to recover from errors. Errors are generally
unrecoverable, so retrying is usually a bad idea.

BUG=688030
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

Review-Url: https://codereview.chromium.org/2662173002
Cr-Commit-Position: refs/heads/master@{#450565}
Committed: https://chromium.googlesource.com/chromium/src/+/1e1ea2bcd2e632ba3b3dcbd45ca3405771e57a9c

[watk, 2017-01-31 02:08:20]

Description was changed from

==========
media: Don't create a new MediaCodec during AVDA teardown

OnDrainCompleted() is always called during AVDA teardown, and it
previously always called ResetCodecState() which resulted in
unnecessarily creating new MediaCodecs during shutdown whenever state_
was ERROR, or on older devices where flush() doesn't work.

Now we don't bother calling ResetCodecState() during destruction, and
we also don't try to recover from errors. Errors are generally
unrecoverable, so retrying is usually a bad idea.

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

to

==========
media: Don't create a new MediaCodec during AVDA teardown

OnDrainCompleted() is always called during AVDA teardown, and it
previously always called ResetCodecState() which resulted in
unnecessarily creating new MediaCodecs during shutdown whenever state_
was ERROR, or on older devices where flush() doesn't work.

Now we don't bother calling ResetCodecState() during destruction, and
we also don't try to recover from errors. Errors are generally
unrecoverable, so retrying is usually a bad idea.

BUG=
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, 2017-01-31 02:10:55]

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

[watk, 2017-01-31 02:10:56]

PTAL. The deferred reset logic was only for FLUSH drains, not RESETs or
DESTROYs. We used to also create a new codec during teardown even if we were
exiting due to an error :\

[watk, 2017-01-31 02:14:15]

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

[commit-bot: I haz the power, 2017-01-31 02:14:53]

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-31 05:04:01]

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

[commit-bot: I haz the power, 2017-01-31 05:04:02]

Dry run: This issue passed the CQ dry run.

[DaleCurtis, 2017-02-01 00:45:51]

Ah, crappy; thought I fixed all the cases of this. lgtm - should definitely help
our crash rates so probably worth merging too.

[DaleCurtis, 2017-02-01 00:46:06]

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

[DaleCurtis, 2017-02-01 00:46:06]

+liberato fyi

[DaleCurtis, 2017-02-01 00:46:24]

Should have a unit test which verifies only 1 codec is created probably :)

[watk, 2017-02-02 17:15:24]

Description was changed from

==========
media: Don't create a new MediaCodec during AVDA teardown

OnDrainCompleted() is always called during AVDA teardown, and it
previously always called ResetCodecState() which resulted in
unnecessarily creating new MediaCodecs during shutdown whenever state_
was ERROR, or on older devices where flush() doesn't work.

Now we don't bother calling ResetCodecState() during destruction, and
we also don't try to recover from errors. Errors are generally
unrecoverable, so retrying is usually a bad idea.

BUG=
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: Don't create a new MediaCodec during AVDA teardown

OnDrainCompleted() is always called during AVDA teardown, and it
previously always called ResetCodecState() which resulted in
unnecessarily creating new MediaCodecs during shutdown whenever state_
was ERROR, or on older devices where flush() doesn't work.

Now we don't bother calling ResetCodecState() during destruction, and
we also don't try to recover from errors. Errors are generally
unrecoverable, so retrying is usually a bad idea.

BUG=688030
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, 2017-02-02 17:15:55]

The CQ bit was checked by watk@chromium.org

[commit-bot: I haz the power, 2017-02-02 17:16:19]

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

[watk, 2017-02-02 17:22:46]

The CQ bit was unchecked by watk@chromium.org

[watk, 2017-02-03 19:07:39]

On 2017/02/01 00:46:24, DaleCurtis wrote:
> Should have a unit test which verifies only 1 codec is created probably :)

Ok, let me see how difficult that is.

[DaleCurtis, 2017-02-03 19:30:38]

(fine in a followup cl)

[watk, 2017-02-15 00:00:38]

The CQ bit was checked by watk@chromium.org

[commit-bot: I haz the power, 2017-02-15 00:01:31]

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-02-15 00:07:24]

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

[commit-bot: I haz the power, 2017-02-15 00:07:25]

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...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[watk, 2017-02-15 00:13:14]

The CQ bit was checked by watk@chromium.org

[watk, 2017-02-15 00:13:15]

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/2662173002/#ps40001
(title: "rebase")

[commit-bot: I haz the power, 2017-02-15 00:14:10]

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-02-15 05:41:34]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1487117594885740,
"parent_rev": "2d511bceabaa2c2a7e53dea20277cb45e08255f5", "commit_rev":
"1e1ea2bcd2e632ba3b3dcbd45ca3405771e57a9c"}

[commit-bot: I haz the power, 2017-02-15 05:42:51]

Description was changed from

==========
media: Don't create a new MediaCodec during AVDA teardown

OnDrainCompleted() is always called during AVDA teardown, and it
previously always called ResetCodecState() which resulted in
unnecessarily creating new MediaCodecs during shutdown whenever state_
was ERROR, or on older devices where flush() doesn't work.

Now we don't bother calling ResetCodecState() during destruction, and
we also don't try to recover from errors. Errors are generally
unrecoverable, so retrying is usually a bad idea.

BUG=688030
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: Don't create a new MediaCodec during AVDA teardown

OnDrainCompleted() is always called during AVDA teardown, and it
previously always called ResetCodecState() which resulted in
unnecessarily creating new MediaCodecs during shutdown whenever state_
was ERROR, or on older devices where flush() doesn't work.

Now we don't bother calling ResetCodecState() during destruction, and
we also don't try to recover from errors. Errors are generally
unrecoverable, so retrying is usually a bad idea.

BUG=688030
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

Review-Url: https://codereview.chromium.org/2662173002
Cr-Commit-Position: refs/heads/master@{#450565}
Committed:
https://chromium.googlesource.com/chromium/src/+/1e1ea2bcd2e632ba3b3dcbd45ca3...
==========

[commit-bot: I haz the power, 2017-02-15 05:42:52]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/1e1ea2bcd2e632ba3b3dcbd45ca3...

[watk, 2017-02-15 21:43:18]

That's awkward. I tried to just rebase the patchset that I'd already uploaded
but I accidentally committed my local changes to add a test, which you hadn't
reviewed.

The tests that I committed don't actually catch this issue (after I wrote them I
reverted the fix and they didn't fail). The exact path we need to exercise is
getting a non-null MC, it returning some error from e.g., DequeueInputBuffer,
and then we destroy AVDA. The part I couldn't do in this CL was make MC return
an error and that's why I went on a mission to make MediaCodec mockable.

In any case, I don't think the tests as written are bad.. they still assert
behavior we want. They're just not complete.

So PTAL and let me know if you are happy with what was committed. If not I'll
revert or post a follow-up. Thanks

[DaleCurtis, 2017-02-15 22:48:51]

tests lgtm, always happy to have more tests :)