Reland of rewrite frame generator capturer to use TaskQueue instead of EventTimeWrapper

Reland of rewrite frame generator capturer to use TaskQueue instead of EventTimeWrapper.

Fix CallPerfTest.ReceivesCpuOveruseAndUnderuse to not fail on Android with new FrameGeneratorCapturer.

BUG=webrtc:7301
Review-Url: https://codereview.webrtc.org/2745583006
Cr-Commit-Position: refs/heads/master@{#17168}
Committed: https://chromium.googlesource.com/external/webrtc/+/b00742508ae9320ce86ad2963c18443a83976999

[ilnik, 2017-03-10 12:06:36]

Created Reland of write frame generator capturer to use TaskQueue instead of
EventTimeWrapper

[ilnik, 2017-03-10 12:08:14]

Description was changed from

==========
Reland of write frame generator capturer to use TaskQueue instead of
EventTimeWrapper (patchset #1 id:1 of https://codereview.webrtc.org/2739393002/
)

Reason for revert:
Reland reverted patch to add fixes for failing tests

Original issue's description:
> Revert of Rewrite frame generator capturer to use TaskQueue instead of
EventTimeWrapper (patchset #4 id:60001 of
https://codereview.webrtc.org/2740723002/ )
> 
> Reason for revert:
> Less precise FrameGeneratorCapturer broke
CallPerfTest.ReceivesCpuOveruseAndUnderuse on Android
> 
> Original issue's description:
> > Rewrite frame generator capturer to use TaskQueue instead of
EventTimeWrapper
> > 
> > BUG=webrtc:7301
> > 
> > Review-Url: https://codereview.webrtc.org/2740723002
> > Cr-Commit-Position: refs/heads/master@{#17161}
> > Committed:
https://chromium.googlesource.com/external/webrtc/+/7c5503a8b30ae43a8ccaa1851...
> 
> TBR=kjellander@webrtc.org,sprang@webrtc.org,tommi@webrtc.org
> # Skipping CQ checks because original CL landed less than 1 days ago.
> NOPRESUBMIT=true
> NOTREECHECKS=true
> NOTRY=true
> BUG=webrtc:7301
> 
> Review-Url: https://codereview.webrtc.org/2739393002
> Cr-Commit-Position: refs/heads/master@{#17163}
> Committed:
https://chromium.googlesource.com/external/webrtc/+/c37d2b96b812b4ec2c81d7bce...

TBR=kjellander@webrtc.org,sprang@webrtc.org,tommi@webrtc.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG=webrtc:7301
==========

to

==========
Reland of rewrite frame generator capturer to use TaskQueue instead of
EventTimeWrapper 

BUG=webrtc:7301
==========

[ilnik, 2017-03-10 12:08:49]

On 2017/03/10 12:06:36, ilnik wrote:
> Created Reland of write frame generator capturer to use TaskQueue instead of
> EventTimeWrapper

Will add patches for failing tests.

[ilnik, 2017-03-10 12:09:36]

ilnik@webrtc.org changed reviewers:
- kjellander@webrtc.org, sprang@webrtc.org, tommi@webrtc.org

[ilnik, 2017-03-10 12:10:51]

Description was changed from

==========
Reland of rewrite frame generator capturer to use TaskQueue instead of
EventTimeWrapper 

BUG=webrtc:7301
==========

to

==========
Reland of rewrite frame generator capturer to use TaskQueue instead of
EventTimeWrapper.

Fix CallPerfTest.ReceivesCpuOveruseAndUnderuse to not fail on Android with new
FrameGeneratorCapturer.

BUG=webrtc:7301
==========

[ilnik, 2017-03-10 14:17:31]

ilnik@webrtc.org changed reviewers:
+ stefan@webrtc.org

[ilnik, 2017-03-10 14:17:31]

Stefan, please take a look at CallPerfTest. 
Rewritten FrameGeneratorCapturer seems to be less accurate on android. This
makes ReceivesCpuOveruseAndUnderuse fail.

[stefan-webrtc, 2017-03-10 14:23:32]

On 2017/03/10 14:17:31, ilnik wrote:
> Stefan, please take a look at CallPerfTest. 
> Rewritten FrameGeneratorCapturer seems to be less accurate on android. This
> makes ReceivesCpuOveruseAndUnderuse fail.

What is "delay" and what does it solve by being 40?

[ilnik, 2017-03-10 15:00:02]

On 2017/03/10 14:23:32, stefan-webrtc wrote:
> On 2017/03/10 14:17:31, ilnik wrote:
> > Stefan, please take a look at CallPerfTest. 
> > Rewritten FrameGeneratorCapturer seems to be less accurate on android. This
> > makes ReceivesCpuOveruseAndUnderuse fail.
> 
> What is "delay" and what does it solve by being 40?

From my understanding, it is a fake encoder which takes "delay" ms to produce a
frame. Target fps is 30. The idea is that 35ms is too much per frame and it will
cause overuse detection. However, it's not enough if frame generator generates
less than 30 frames per second.

[ilnik, 2017-03-10 15:02:23]

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

[commit-bot: I haz the power, 2017-03-10 15:02:27]

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

[stefan-webrtc, 2017-03-10 15:10:10]

lgtm

[ilnik, 2017-03-10 15:11:15]

The CQ bit was unchecked by ilnik@webrtc.org

[ilnik, 2017-03-10 15:18:33]

The CQ bit was checked by ilnik@webrtc.org

[commit-bot: I haz the power, 2017-03-10 15:18:37]

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

[ilnik, 2017-03-10 15:41:19]

The CQ bit was unchecked by ilnik@webrtc.org

[ilnik, 2017-03-10 15:41:23]

The CQ bit was checked by ilnik@webrtc.org

[commit-bot: I haz the power, 2017-03-10 15:41:27]

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-10 15:43:33]

CQ is committing da patch.

Bot data: {"patchset_id": 70001, "attempt_start_ts": 1489160483139530,
"parent_rev": "144475b3286f4de5059e0333044a72faf5d0171c", "commit_rev":
"b00742508ae9320ce86ad2963c18443a83976999"}

[commit-bot: I haz the power, 2017-03-10 15:43:36]

Description was changed from

==========
Reland of rewrite frame generator capturer to use TaskQueue instead of
EventTimeWrapper.

Fix CallPerfTest.ReceivesCpuOveruseAndUnderuse to not fail on Android with new
FrameGeneratorCapturer.

BUG=webrtc:7301
==========

to

==========
Reland of rewrite frame generator capturer to use TaskQueue instead of
EventTimeWrapper.

Fix CallPerfTest.ReceivesCpuOveruseAndUnderuse to not fail on Android with new
FrameGeneratorCapturer.

BUG=webrtc:7301

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

[commit-bot: I haz the power, 2017-03-10 15:43:37]

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

[tommi, 2017-03-10 17:30:00]

tommi@webrtc.org changed reviewers:
+ tommi@webrtc.org

[tommi, 2017-03-10 17:30:01]

I filed issue: https://bugs.chromium.org/p/webrtc/issues/detail?id=7325

https://codereview.webrtc.org/2745583006/diff/70001/webrtc/test/frame_generat...
File webrtc/test/frame_generator_capturer.cc (right):

https://codereview.webrtc.org/2745583006/diff/70001/webrtc/test/frame_generat...
webrtc/test/frame_generator_capturer.cc:48:
webrtc::test::FrameGeneratorCapturer* const frame_generator_capturer_;
There's a race related to using this pointer since the task queue can live
longer than the FrameGeneratorCapturer instance and the InsertFrameTask can
therefore outlive the object pointed to here.

[ilnik, 2017-03-10 17:44:06]

https://codereview.webrtc.org/2745583006/diff/70001/webrtc/test/frame_generat...
File webrtc/test/frame_generator_capturer.cc (right):

https://codereview.webrtc.org/2745583006/diff/70001/webrtc/test/frame_generat...
webrtc/test/frame_generator_capturer.cc:48:
webrtc::test::FrameGeneratorCapturer* const frame_generator_capturer_;
On 2017/03/10 17:30:01, tommi (webrtc) wrote:
> There's a race related to using this pointer since the task queue can live
> longer than the FrameGeneratorCapturer instance and the InsertFrameTask can
> therefore outlive the object pointed to here.

But the task queue in question is the member of the frame generator. How can it
outlive it?

[ilnik, 2017-03-10 17:49:31]

A revert of this CL (patchset #2 id:70001) has been created in
https://codereview.webrtc.org/2743993002/ by ilnik@webrtc.org.

The reason for reverting is: Causes problems with TSAN:
https://bugs.chromium.org/p/webrtc/issues/detail?id=7325.

[tommi, 2017-03-10 18:00:40]

On 2017/03/10 17:44:06, ilnik wrote:
>
https://codereview.webrtc.org/2745583006/diff/70001/webrtc/test/frame_generat...
> File webrtc/test/frame_generator_capturer.cc (right):
> 
>
https://codereview.webrtc.org/2745583006/diff/70001/webrtc/test/frame_generat...
> webrtc/test/frame_generator_capturer.cc:48:
> webrtc::test::FrameGeneratorCapturer* const frame_generator_capturer_;
> On 2017/03/10 17:30:01, tommi (webrtc) wrote:
> > There's a race related to using this pointer since the task queue can live
> > longer than the FrameGeneratorCapturer instance and the InsertFrameTask can
> > therefore outlive the object pointed to here.
> 
> But the task queue in question is the member of the frame generator. How can
it
> outlive it?

ah - could it be that the criticalsection is deleted before the taskqueue?

[ilnik, 2017-03-10 18:06:09]

On 2017/03/10 18:00:40, tommi (webrtc) wrote:
> On 2017/03/10 17:44:06, ilnik wrote:
> >
>
https://codereview.webrtc.org/2745583006/diff/70001/webrtc/test/frame_generat...
> > File webrtc/test/frame_generator_capturer.cc (right):
> > 
> >
>
https://codereview.webrtc.org/2745583006/diff/70001/webrtc/test/frame_generat...
> > webrtc/test/frame_generator_capturer.cc:48:
> > webrtc::test::FrameGeneratorCapturer* const frame_generator_capturer_;
> > On 2017/03/10 17:30:01, tommi (webrtc) wrote:
> > > There's a race related to using this pointer since the task queue can live
> > > longer than the FrameGeneratorCapturer instance and the InsertFrameTask
can
> > > therefore outlive the object pointed to here.
> > 
> > But the task queue in question is the member of the frame generator. How can
> it
> > outlive it?
> 
> ah - could it be that the criticalsection is deleted before the taskqueue?

That sounds like the only plausible explanation. Also sending_ variable is used
inside the task queue thread.
Could you give advice on how to deal with it? I need to somehow check that
taskQueue stopped working completely inside the destructor of the frame
generator? Will that be enough?

[tommi, 2017-03-10 18:21:57]

On 2017/03/10 18:06:09, ilnik wrote:
> On 2017/03/10 18:00:40, tommi (webrtc) wrote:
> > On 2017/03/10 17:44:06, ilnik wrote:
> > >
> >
>
https://codereview.webrtc.org/2745583006/diff/70001/webrtc/test/frame_generat...
> > > File webrtc/test/frame_generator_capturer.cc (right):
> > > 
> > >
> >
>
https://codereview.webrtc.org/2745583006/diff/70001/webrtc/test/frame_generat...
> > > webrtc/test/frame_generator_capturer.cc:48:
> > > webrtc::test::FrameGeneratorCapturer* const frame_generator_capturer_;
> > > On 2017/03/10 17:30:01, tommi (webrtc) wrote:
> > > > There's a race related to using this pointer since the task queue can
live
> > > > longer than the FrameGeneratorCapturer instance and the InsertFrameTask
> can
> > > > therefore outlive the object pointed to here.
> > > 
> > > But the task queue in question is the member of the frame generator. How
can
> > it
> > > outlive it?
> > 
> > ah - could it be that the criticalsection is deleted before the taskqueue?
> 
> That sounds like the only plausible explanation. Also sending_ variable is
used
> inside the task queue thread.
> Could you give advice on how to deal with it? I need to somehow check that
> taskQueue stopped working completely inside the destructor of the frame
> generator? Will that be enough?

I'd make the task queue the last member variable in the class and add a comment
that explains that it needs to be destructed before all other member variables
to ensure that pending tasks that might have a pointer back to |this|, are
deleted before destruction continues.