media: Fix a flaky AVDACodecAllocator test

media: Fix a flaky AVDACodecAllocator test

This fixes a race condition where we previously asserted that both
threads were stopped, but only after receiving a notification that the
first had stopped. Now we don't assert that the second has stopped.
It also includes some minor refactors: deleting TestInformation
and injects the params directly, and splitting the failing test into
two smaller tests.

BUG=668291
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/d8c18fcf4f0e2e2e186319e6a439bfec73c4e796
Cr-Commit-Position: refs/heads/master@{#435698}

[watk, 2016-11-30 01:00:54]

Description was changed from

==========
media: Refactor AVDACodecAllocator unittests

It's not clear what's causing the linked bug so this refactor cleans
some things up to make it easier to find. This adds a second TestParams
instance to eliminate the sharing between allocators, and splits the
failing test into two smaller tests.

BUG=668291
==========

to

==========
media: Refactor AVDACodecAllocator unittests

It's not clear what's causing the linked bug so this refactor cleans
some things up to make it easier to find. This adds a second TestParams
instance to eliminate the sharing between allocators, and splits the
failing test into two smaller tests.

BUG=668291
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-11-30 01:01:08]

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

[watk, 2016-11-30 01:02:19]

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

[commit-bot: I haz the power, 2016-11-30 01:03: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, 2016-11-30 01:30:07]

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

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

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

[watk, 2016-11-30 01:36:34]

Patchset #2 (id:20001) has been deleted

[watk, 2016-11-30 01:37:58]

Description was changed from

==========
media: Refactor AVDACodecAllocator unittests

It's not clear what's causing the linked bug so this refactor cleans
some things up to make it easier to find. This adds a second TestParams
instance to eliminate the sharing between allocators, and splits the
failing test into two smaller tests.

BUG=668291
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: Refactor AVDACodecAllocator unittests

It's not clear what's causing the linked bug so this refactor cleans
some things up to make it easier to find. This deletes TestInformation
and injects the params directly, and splits the failing test into two
smaller tests.

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

[liberato (no reviews please), 2016-11-30 06:47:30]

https://codereview.chromium.org/2540593005/diff/40001/media/gpu/avda_codec_al...
File media/gpu/avda_codec_allocator_unittest.cc (left):

https://codereview.chromium.org/2540593005/diff/40001/media/gpu/avda_codec_al...
media/gpu/avda_codec_allocator_unittest.cc:240:
ASSERT_FALSE(IsThreadRunning(TaskType::SW_CODEC));
i'm not sure why this thread must be stopped by now.  only auto_codec signals
the event; the sw codec stop task might still be pending on the stop thread, i
think.  might be the cause of the flakiness.

the caveat is it's late.  i just happened to notice this CL when i logged in to
write down some thoughts i had this evening on the avda refactor (i didn't want
to forget them by morning; turns out that i broke threading for sync surface
destruction, oops!)  so, if this comment seems totally wrong, then please just
ignore it and i'll do an actual review tomorrow in which i, you know, actually
read your code before commenting. :)

[watk, 2016-11-30 20:48:42]

https://codereview.chromium.org/2540593005/diff/40001/media/gpu/avda_codec_al...
File media/gpu/avda_codec_allocator_unittest.cc (left):

https://codereview.chromium.org/2540593005/diff/40001/media/gpu/avda_codec_al...
media/gpu/avda_codec_allocator_unittest.cc:240:
ASSERT_FALSE(IsThreadRunning(TaskType::SW_CODEC));
On 2016/11/30 06:47:30, liberato wrote:
> i'm not sure why this thread must be stopped by now.  only auto_codec signals
> the event; the sw codec stop task might still be pending on the stop thread, i
> think.  might be the cause of the flakiness.
> 
> the caveat is it's late.  i just happened to notice this CL when i logged in
to
> write down some thoughts i had this evening on the avda refactor (i didn't
want
> to forget them by morning; turns out that i broke threading for sync surface
> destruction, oops!)  so, if this comment seems totally wrong, then please just
> ignore it and i'll do an actual review tomorrow in which i, you know, actually
> read your code before commenting. :)

Oh genius! I think you've got it.

[watk, 2016-11-30 21:02:16]

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

[commit-bot: I haz the power, 2016-11-30 21:03:04]

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-11-30 21:08:20]

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

[commit-bot: I haz the power, 2016-11-30 21:08:20]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  android_cronet on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)

[watk, 2016-11-30 21:12:53]

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

[commit-bot: I haz the power, 2016-11-30 21:13: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 01:03:55]

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

[commit-bot: I haz the power, 2016-12-01 01:03:56]

Dry run: This issue passed the CQ dry run.

[liberato (no reviews please), 2016-12-01 16:43:32]

lgtm.  thanks for fixing this -- it was broken in my original CL and I didn't
notice.

thanks
-fl

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

Description was changed from

==========
media: Refactor AVDACodecAllocator unittests

It's not clear what's causing the linked bug so this refactor cleans
some things up to make it easier to find. This deletes TestInformation
and injects the params directly, and splits the failing test into two
smaller tests.

BUG=668291
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: Fix a flaky AVDACodecAllocator test

This fixes a race condition where we previously asserted that both
threads were stopped, but only after receiving a notification that the
first had stopped. Now we don't assert that the second has stopped.
It also includes some minor refactors: deleting TestInformation
and injects the params directly, and splitting the failing test into
two smaller tests.

BUG=668291
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 20:03:44]

The CQ bit was checked by watk@chromium.org

[commit-bot: I haz the power, 2016-12-01 20:04:38]

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

[watk, 2016-12-01 20:05:00]

Well it was pretty subtle, I read all those lines and had no idea where the bug
was :) I thought it might have been something to do with reusing the
TestInformation struct for both allocators (or at least it wasn't trivial to
rule out) which is why I did the original patch.

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

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1480622624124920,
"parent_rev": "92a8cc98e0ba479c6bed9bf219e5b0d37ea1d0c4", "commit_rev":
"3a230c8d78ec544ccd34146ab5cdfee11437eba3"}

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

Description was changed from

==========
media: Fix a flaky AVDACodecAllocator test

This fixes a race condition where we previously asserted that both
threads were stopped, but only after receiving a notification that the
first had stopped. Now we don't assert that the second has stopped.
It also includes some minor refactors: deleting TestInformation
and injects the params directly, and splitting the failing test into
two smaller tests.

BUG=668291
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: Fix a flaky AVDACodecAllocator test

This fixes a race condition where we previously asserted that both
threads were stopped, but only after receiving a notification that the
first had stopped. Now we don't assert that the second has stopped.
It also includes some minor refactors: deleting TestInformation
and injects the params directly, and splitting the failing test into
two smaller tests.

BUG=668291
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 20:22:01]

Committed patchset #4 (id:80001)

[commit-bot: I haz the power, 2016-12-01 20:26:24]

Description was changed from

==========
media: Fix a flaky AVDACodecAllocator test

This fixes a race condition where we previously asserted that both
threads were stopped, but only after receiving a notification that the
first had stopped. Now we don't assert that the second has stopped.
It also includes some minor refactors: deleting TestInformation
and injects the params directly, and splitting the failing test into
two smaller tests.

BUG=668291
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: Fix a flaky AVDACodecAllocator test

This fixes a race condition where we previously asserted that both
threads were stopped, but only after receiving a notification that the
first had stopped. Now we don't assert that the second has stopped.
It also includes some minor refactors: deleting TestInformation
and injects the params directly, and splitting the failing test into
two smaller tests.

BUG=668291
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/d8c18fcf4f0e2e2e186319e6a439bfec73c4e796
Cr-Commit-Position: refs/heads/master@{#435698}
==========

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

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