Synchronize task queue operations in QualityScaler tests.

Synchronize task queue operations in QualityScaler tests.

These tests were disabled due to flakiness when running on the bots.
Hopefully synchronizing all operations that run on Task Queue will
fix this.

BUG=webrtc:6799
Review-Url: https://codereview.webrtc.org/2774643002
Cr-Commit-Position: refs/heads/master@{#17463}
Committed: https://chromium.googlesource.com/external/webrtc/+/b12a3e342728f66238eaa158264af9a483bf6fd0

[kthelgason, 2017-03-23 13:23:48]

The CQ bit was checked by kthelgason@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-23 13:23:55]

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

[commit-bot: I haz the power, 2017-03-23 14:05:38]

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

[commit-bot: I haz the power, 2017-03-23 14:05:39]

Dry run: This issue passed the CQ dry run.

[kthelgason, 2017-03-23 14:20:59]

kthelgason@webrtc.org changed reviewers:
+ sprang@webrtc.org

[kthelgason, 2017-03-23 14:21:00]

ptal.

[sprang_webrtc, 2017-03-26 12:24:48]

https://codereview.webrtc.org/2774643002/diff/1/webrtc/modules/video_coding/u...
File webrtc/modules/video_coding/utility/quality_scaler_unittest.cc (right):

https://codereview.webrtc.org/2774643002/diff/1/webrtc/modules/video_coding/u...
webrtc/modules/video_coding/utility/quality_scaler_unittest.cc:37: 
I don't think this needs to be macro. Add a helper function or struct to do this
instead if you feel there's too much code duplication.
Also this feels a little like an antipattern, now we really have no use for the
queue at all?

Also, why do we suspect this will help? I don't like sprinkling synchronizations
around for blind luck.

[kthelgason, 2017-03-27 06:55:04]

https://codereview.webrtc.org/2774643002/diff/1/webrtc/modules/video_coding/u...
File webrtc/modules/video_coding/utility/quality_scaler_unittest.cc (right):

https://codereview.webrtc.org/2774643002/diff/1/webrtc/modules/video_coding/u...
webrtc/modules/video_coding/utility/quality_scaler_unittest.cc:37: 
On 2017/03/26 12:24:47, språng wrote:
> I don't think this needs to be macro. Add a helper function or struct to do
this
> instead if you feel there's too much code duplication.

This can't be a function, at least with the current design. To make it a regular
function would mean wrapping the code currently passed as  `block` in a lambda.
It's less code and IMO less distracting from the point of the test to do this
with a macro.

> Also this feels a little like an antipattern, now we really have no use for
the
> queue at all?

I agree, doing this in production code would be ridiculous, but the issue with
these tests is that they have to run on Task Queue but that causes a data race.

> Also, why do we suspect this will help? I don't like sprinkling
synchronizations
> around for blind luck.

My hypothesis for why these tests were flaky is that ScaleDown does not actually
execute on the task queue until the timeout expires. Waiting for it to execute
would eliminate that condition. These tests are only flaky on the trybots, where
resources are limited, and I suspect this is why. 

I may absolutely be wrong though, and I'd love to hear competing theories!

[sprang_webrtc, 2017-03-29 08:53:06]

lgtm with nit

https://codereview.webrtc.org/2774643002/diff/1/webrtc/modules/video_coding/u...
File webrtc/modules/video_coding/utility/quality_scaler_unittest.cc (right):

https://codereview.webrtc.org/2774643002/diff/1/webrtc/modules/video_coding/u...
webrtc/modules/video_coding/utility/quality_scaler_unittest.cc:35:
event.Wait(1000);               \
RTC_CHECK this wait

https://codereview.webrtc.org/2774643002/diff/1/webrtc/modules/video_coding/u...
webrtc/modules/video_coding/utility/quality_scaler_unittest.cc:37: 
On 2017/03/27 06:55:04, kthelgason wrote:
> On 2017/03/26 12:24:47, språng wrote:
> > I don't think this needs to be macro. Add a helper function or struct to do
> this
> > instead if you feel there's too much code duplication.
> 
> This can't be a function, at least with the current design. To make it a
regular
> function would mean wrapping the code currently passed as  `block` in a
lambda.
> It's less code and IMO less distracting from the point of the test to do this
> with a macro.
> 
> > Also this feels a little like an antipattern, now we really have no use for
> the
> > queue at all?
> 
> I agree, doing this in production code would be ridiculous, but the issue with
> these tests is that they have to run on Task Queue but that causes a data
race.
> 
> > Also, why do we suspect this will help? I don't like sprinkling
> synchronizations
> > around for blind luck.
> 
> My hypothesis for why these tests were flaky is that ScaleDown does not
actually
> execute on the task queue until the timeout expires. Waiting for it to execute
> would eliminate that condition. These tests are only flaky on the trybots,
where
> resources are limited, and I suspect this is why. 
> 
> I may absolutely be wrong though, and I'd love to hear competing theories!
> 

I don't like it, but fair enough :)
Let's see if this helps. I don't have time to dig further into it right now.

[kthelgason, 2017-03-29 11:46:16]

On 2017/03/29 08:53:06, språng wrote:
> I don't like it, but fair enough :)
> Let's see if this helps. I don't have time to dig further into it right now.

I agree that I'm not super happy with this, but rearchitecting the entire thing
to work without Task Queue is a lot of work, and these tests aren't doing us any
favours when they're disabled. I'll gladly revert this if it doesn't work.

[kthelgason, 2017-03-29 11:46:22]

The CQ bit was checked by kthelgason@webrtc.org

[kthelgason, 2017-03-29 11:46:23]

The patchset sent to the CQ was uploaded after l-g-t-m from sprang@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2774643002/#ps20001 (title:
"Add CHECK for event.wait")

[commit-bot: I haz the power, 2017-03-29 11:46:30]

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

[commit-bot: I haz the power, 2017-03-29 11:47:39]

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

[commit-bot: I haz the power, 2017-03-29 11:47:40]

Try jobs failed on following builders:
  linux_arm on master.tryserver.webrtc (JOB_FAILED, build hasn't started yet,
builder probably lacks capacity)

[kthelgason, 2017-03-30 07:45:50]

The CQ bit was checked by kthelgason@webrtc.org

[commit-bot: I haz the power, 2017-03-30 07:45:57]

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

[commit-bot: I haz the power, 2017-03-30 08:04:55]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1490859950881610,
"parent_rev": "2da7a24fcd48a50d4edb7f3cd2abe99e9c5ef6f1", "commit_rev":
"b12a3e342728f66238eaa158264af9a483bf6fd0"}

[commit-bot: I haz the power, 2017-03-30 08:04:59]

Description was changed from

==========
Synchronize task queue operations in QualityScaler tests.

These tests were disabled due to flakiness when running on the bots.
Hopefully synchronizing all operations that run on Task Queue will
fix this.

BUG=webrtc:6799
==========

to

==========
Synchronize task queue operations in QualityScaler tests.

These tests were disabled due to flakiness when running on the bots.
Hopefully synchronizing all operations that run on Task Queue will
fix this.

BUG=webrtc:6799

Review-Url: https://codereview.webrtc.org/2774643002
Cr-Commit-Position: refs/heads/master@{#17463}
Committed:
https://chromium.googlesource.com/external/webrtc/+/b12a3e342728f66238eaa1582...
==========

[commit-bot: I haz the power, 2017-03-30 08:04:59]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/external/webrtc/+/b12a3e342728f66238eaa1582...