media: AVDACodecAllocator now fails if threads don't start

media: AVDACodecAllocator now fails if threads don't start

Previously AVDACodecAllocator tried to gracefully handle threads failing
to start. To simplify its contract, now it considers threads failing to
to be an error.

BUG=670140
TEST=existing tests
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/997e9e7f24cf2c6b8433bbfeedeeaf5a6b7c6d13
Cr-Commit-Position: refs/heads/master@{#435756}

[watk, 2016-12-01 01:56:27]

Description was changed from

==========
media: AVDACodecAllocator now fails if threads don't start

Previously AVDACodecAllocator tried to gracefully handle threads failing
to start. To simplify its contract, now it considers threads failing to
to be an error.

TEST=existing tests
==========

to

==========
media: AVDACodecAllocator now fails if threads don't start

Previously AVDACodecAllocator tried to gracefully handle threads failing
to start. To simplify its contract, now it considers threads failing to
to be an error.

TEST=existing tests
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-01 02:00:03]

Description was changed from

==========
media: AVDACodecAllocator now fails if threads don't start

Previously AVDACodecAllocator tried to gracefully handle threads failing
to start. To simplify its contract, now it considers threads failing to
to be an error.

TEST=existing tests
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: AVDACodecAllocator now fails if threads don't start

Previously AVDACodecAllocator tried to gracefully handle threads failing
to start. To simplify its contract, now it considers threads failing to
to be an error.

BUG=670140
TEST=existing tests
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-01 02:00:08]

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

[watk, 2016-12-01 02:00:14]

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

[commit-bot: I haz the power, 2016-12-01 02:00:37]

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-01 04:02:49]

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

[commit-bot: I haz the power, 2016-12-01 04:02:50]

Dry run: This issue passed the CQ dry run.

[liberato (no reviews please), 2016-12-01 16:52:58]

lgtm.  nice code cleanup too.

-fl

https://codereview.chromium.org/2540393002/diff/1/media/gpu/avda_codec_alloca...
File media/gpu/avda_codec_allocator.cc (left):

https://codereview.chromium.org/2540393002/diff/1/media/gpu/avda_codec_alloca...
media/gpu/avda_codec_allocator.cc:376: DCHECK_EQ(threads_.size(),
TaskType::AUTO_CODEC);
i think that we still want the DCHECKs.  if one switches the order of the
push_backs, then the thread names will be wrong.

https://codereview.chromium.org/2540393002/diff/1/media/gpu/avda_codec_alloca...
media/gpu/avda_codec_allocator.cc:386: if (!threads_[i]->thread.IsRunning())
good point.

https://codereview.chromium.org/2540393002/diff/1/media/gpu/avda_codec_alloca...
File media/gpu/avda_codec_allocator.cc (right):

https://codereview.chromium.org/2540393002/diff/1/media/gpu/avda_codec_alloca...
media/gpu/avda_codec_allocator.cc:364: if (!threads_[0]->thread.IsRunning() &&
!threads_[1]->thread.IsRunning())
please either loop over all threads, or replace 0,1 with AUTO_CODEC, SW_CODEC .

also, i like this way of handling the event much better than it was before.

[watk, 2016-12-01 20:21:53]

The CQ bit was checked by watk@chromium.org

[watk, 2016-12-01 20:21:54]

The patchset sent to the CQ was uploaded after l-g-t-m from
liberato@chromium.org
Link to the patchset: https://codereview.chromium.org/2540393002/#ps40001
(title: "put back assert")

[watk, 2016-12-01 20:22:03]

Thanks!

https://codereview.chromium.org/2540393002/diff/1/media/gpu/avda_codec_alloca...
File media/gpu/avda_codec_allocator.cc (left):

https://codereview.chromium.org/2540393002/diff/1/media/gpu/avda_codec_alloca...
media/gpu/avda_codec_allocator.cc:376: DCHECK_EQ(threads_.size(),
TaskType::AUTO_CODEC);
On 2016/12/01 16:52:58, liberato wrote:
> i think that we still want the DCHECKs.  if one switches the order of the
> push_backs, then the thread names will be wrong.

I changed it to a static_assert that is slightly clearer IMO. Hopefully you
agree

https://codereview.chromium.org/2540393002/diff/1/media/gpu/avda_codec_alloca...
File media/gpu/avda_codec_allocator.cc (right):

https://codereview.chromium.org/2540393002/diff/1/media/gpu/avda_codec_alloca...
media/gpu/avda_codec_allocator.cc:364: if (!threads_[0]->thread.IsRunning() &&
!threads_[1]->thread.IsRunning())
On 2016/12/01 16:52:58, liberato wrote:
> please either loop over all threads, or replace 0,1 with AUTO_CODEC, SW_CODEC
.
> 
> also, i like this way of handling the event much better than it was before.

Done

[commit-bot: I haz the power, 2016-12-01 20:22:32]

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-01 20:25:46]

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

[commit-bot: I haz the power, 2016-12-01 20:25:48]

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...)

[watk, 2016-12-01 21:19:15]

The CQ bit was checked by watk@chromium.org

[watk, 2016-12-01 21:19:16]

The patchset sent to the CQ was uploaded after l-g-t-m from
liberato@chromium.org
Link to the patchset: https://codereview.chromium.org/2540393002/#ps60001
(title: "blah")

[commit-bot: I haz the power, 2016-12-01 21:19: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, 2016-12-01 23:29:37]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1480627155945510,
"parent_rev": "5f36e841ed8959e67b52990f88be6bf128000e1c", "commit_rev":
"b27653231467d2347e06ed2c0f0e4e12cb85df46"}

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

Description was changed from

==========
media: AVDACodecAllocator now fails if threads don't start

Previously AVDACodecAllocator tried to gracefully handle threads failing
to start. To simplify its contract, now it considers threads failing to
to be an error.

BUG=670140
TEST=existing tests
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: AVDACodecAllocator now fails if threads don't start

Previously AVDACodecAllocator tried to gracefully handle threads failing
to start. To simplify its contract, now it considers threads failing to
to be an error.

BUG=670140
TEST=existing tests
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
==========

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

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-12-01 23:33:06]

Description was changed from

==========
media: AVDACodecAllocator now fails if threads don't start

Previously AVDACodecAllocator tried to gracefully handle threads failing
to start. To simplify its contract, now it considers threads failing to
to be an error.

BUG=670140
TEST=existing tests
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: AVDACodecAllocator now fails if threads don't start

Previously AVDACodecAllocator tried to gracefully handle threads failing
to start. To simplify its contract, now it considers threads failing to
to be an error.

BUG=670140
TEST=existing tests
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/997e9e7f24cf2c6b8433bbfeedeeaf5a6b7c6d13
Cr-Commit-Position: refs/heads/master@{#435756}
==========

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

Patchset 4 (id:??) landed as
https://crrev.com/997e9e7f24cf2c6b8433bbfeedeeaf5a6b7c6d13
Cr-Commit-Position: refs/heads/master@{#435756}