Use SingleThreadedTaskQueue in DirectTransport

Use SingleThreadedTaskQueue in DirectTransport

DirectTransport has so far used its own thread, which led to a different threading-model for in the unit-tests than is used in actual WebRTC. Because of that, some critical-sections that weren't truly necessary in WebRTC could not be replaced with thread-checks, because those checks failed in unit-tests.

This CL introduces SingleThreadedTaskQueue - a TaskQueue which guarantees to run all of its tasks on the same thread (rtc::TaskQueue doesn't guarantee that on Mac) - and uses that for DirectTransport. CLs based on top of this will uncomment thread-checks which had to be commented out before, and remove unnecessary critical-sections.

Future work would probably replace the thread-checkers by more sophisticated serialized-access checks, allowing us to move from the SingleThreadedTaskQueue to a normal TaskQueue.

Related implementation notes:
* This CL has made DirectTransport::StopSending() superfluous, and so it was deleted.

BUG=webrtc:8113, webrtc:7405, webrtc:8056, webrtc:8116
Review-Url: https://codereview.webrtc.org/2998923002
Cr-Commit-Position: refs/heads/master@{#19445}
Committed: https://chromium.googlesource.com/external/webrtc/+/413ee9a010eca70712a6935c68b815201bee1270

[eladalon, 2017-08-14 11:16:02]

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

[eladalon, 2017-08-14 11:16:03]

This isn't 100% finished yet, but perhaps you could already take a look?

[eladalon, 2017-08-14 11:33:39]

Description was changed from

==========
Use SingleThreadedTaskQueue in DirectTransport

DirectTransport has so far used its own thread, which led to a different
threading-model for in the unit-tests than is used in actual WebRTC. Because of
that, some critical-sections that weren't truly necessary in WebRTC could not be
replaced with thread-checks, because those checks failed in unit-tests.

This CL introduces SingleThreadedTaskQueue - a TaskQueue which guarantees to run
all of its tasks on the same thread (rtc::TaskQueue doesn't guarantee that on
Mac) - and uses that for DirectTransport. CLs based on top of this will
uncomment thread-checks which had to be commented out before, and remove
unnecessary critical-sections.

Future work would probably replace the thread-checkers by more sophisticated
serialized-access checks, allowing us to move from the SingleThreadedTaskQueue
to a normal TaskQueue.

Please note that this CL is still WIP; some tests fail on Android/Linux, and
some TODOs remain.

TODO: List the bugs associated with this CL.

BUG=None
==========

to

==========
Use SingleThreadedTaskQueue in DirectTransport

DirectTransport has so far used its own thread, which led to a different
threading-model for in the unit-tests than is used in actual WebRTC. Because of
that, some critical-sections that weren't truly necessary in WebRTC could not be
replaced with thread-checks, because those checks failed in unit-tests.

This CL introduces SingleThreadedTaskQueue - a TaskQueue which guarantees to run
all of its tasks on the same thread (rtc::TaskQueue doesn't guarantee that on
Mac) - and uses that for DirectTransport. CLs based on top of this will
uncomment thread-checks which had to be commented out before, and remove
unnecessary critical-sections.

Future work would probably replace the thread-checkers by more sophisticated
serialized-access checks, allowing us to move from the SingleThreadedTaskQueue
to a normal TaskQueue.

Please note that this CL is still WIP; some tests fail on Android/Linux, and
some TODOs remain.

TODO: List the bugs associated with this CL.

BUG=None
==========

[eladalon, 2017-08-14 11:33:39]

eladalon@webrtc.org changed reviewers:
+ nisse@webrtc.org

[stefan-webrtc, 2017-08-14 11:34:54]

https://codereview.webrtc.org/2998923002/diff/1/webrtc/test/direct_transport.cc
File webrtc/test/direct_transport.cc (right):

https://codereview.webrtc.org/2998923002/diff/1/webrtc/test/direct_transport....
webrtc/test/direct_transport.cc:116: SendPackets();
This will busy loop, won't it?

[nisse-webrtc, 2017-08-14 12:04:52]

One alternative could be to extend MessageQueue (base class of rtc::Thread) with
a method

  MessageQueue::PostTask(std::unique_ptr<QueuedTask> task);

and then use a rtc::Thread rather than the new SingleThreadedTaskQueue.

The idea is to aid gradual conversion from rtc::Thread to TaskQueue. See
discussion on cl https://codereview.webrtc.org/2915253002 (unfortunately
reverted a few weeks back).

https://codereview.webrtc.org/2998923002/diff/1/webrtc/test/BUILD.gn
File webrtc/test/BUILD.gn (right):

https://codereview.webrtc.org/2998923002/diff/1/webrtc/test/BUILD.gn#newcode417
webrtc/test/BUILD.gn:417: # Suppress warnings from the Chromium Clang plugin
(bugs.webrtc.org/163).
Don't do this for new code.

[eladalon, 2017-08-14 15:38:00]

Unit-tests added. They might be a bit more extensive than a unit intended for
testing merits, but I think it's better to be safe than sorry.

https://codereview.webrtc.org/2998923002/diff/1/webrtc/test/BUILD.gn
File webrtc/test/BUILD.gn (right):

https://codereview.webrtc.org/2998923002/diff/1/webrtc/test/BUILD.gn#newcode417
webrtc/test/BUILD.gn:417: # Suppress warnings from the Chromium Clang plugin
(bugs.webrtc.org/163).
On 2017/08/14 12:04:52, nisse-webrtc wrote:
> Don't do this for new code.

Is there something I need to do instead, or should I just remove this block and
that's it?

https://codereview.webrtc.org/2998923002/diff/1/webrtc/test/direct_transport.cc
File webrtc/test/direct_transport.cc (right):

https://codereview.webrtc.org/2998923002/diff/1/webrtc/test/direct_transport....
webrtc/test/direct_transport.cc:116: SendPackets();
On 2017/08/14 11:34:54, stefan-webrtc wrote:
> This will busy loop, won't it?

It doesn't execute SendPackets(), but rather schedules it for a next call. Other
things might get scheduled in between, or the thread might decide to rest in
between to make sure it doesn't hog the CPU.

(From an empirical stand-point, I can tell you that the this-thread-is-too-busy
error is not scheduled, and that I can see several things happening on the same
queue at the same time without interruption. The video-loopback works fine, for
example.)

[stefan-webrtc, 2017-08-14 16:30:22]

https://codereview.webrtc.org/2998923002/diff/1/webrtc/test/direct_transport.cc
File webrtc/test/direct_transport.cc (right):

https://codereview.webrtc.org/2998923002/diff/1/webrtc/test/direct_transport....
webrtc/test/direct_transport.cc:116: SendPackets();
On 2017/08/14 15:38:00, eladalon wrote:
> On 2017/08/14 11:34:54, stefan-webrtc wrote:
> > This will busy loop, won't it?
> 
> It doesn't execute SendPackets(), but rather schedules it for a next call.
Other
> things might get scheduled in between, or the thread might decide to rest in
> between to make sure it doesn't hog the CPU.
> 
> (From an empirical stand-point, I can tell you that the
this-thread-is-too-busy
> error is not scheduled, and that I can see several things happening on the
same
> queue at the same time without interruption. The video-loopback works fine,
for
> example.)

But can't we delay the task until it's time to actually send the next packet?

[eladalon, 2017-08-14 17:08:26]

https://codereview.webrtc.org/2998923002/diff/1/webrtc/test/direct_transport.cc
File webrtc/test/direct_transport.cc (right):

https://codereview.webrtc.org/2998923002/diff/1/webrtc/test/direct_transport....
webrtc/test/direct_transport.cc:116: SendPackets();
On 2017/08/14 16:30:21, stefan-webrtc wrote:
> On 2017/08/14 15:38:00, eladalon wrote:
> > On 2017/08/14 11:34:54, stefan-webrtc wrote:
> > > This will busy loop, won't it?
> > 
> > It doesn't execute SendPackets(), but rather schedules it for a next call.
> Other
> > things might get scheduled in between, or the thread might decide to rest in
> > between to make sure it doesn't hog the CPU.
> > 
> > (From an empirical stand-point, I can tell you that the
> this-thread-is-too-busy
> > error is not scheduled, and that I can see several things happening on the
> same
> > queue at the same time without interruption. The video-loopback works fine,
> for
> > example.)
> 
> But can't we delay the task until it's time to actually send the next packet?

More things might be working on this task than this one DirectTransport
instance.

[eladalon, 2017-08-14 17:17:54]

https://codereview.webrtc.org/2998923002/diff/1/webrtc/test/direct_transport.cc
File webrtc/test/direct_transport.cc (right):

https://codereview.webrtc.org/2998923002/diff/1/webrtc/test/direct_transport....
webrtc/test/direct_transport.cc:116: SendPackets();
On 2017/08/14 17:08:26, eladalon wrote:
> On 2017/08/14 16:30:21, stefan-webrtc wrote:
> > On 2017/08/14 15:38:00, eladalon wrote:
> > > On 2017/08/14 11:34:54, stefan-webrtc wrote:
> > > > This will busy loop, won't it?
> > > 
> > > It doesn't execute SendPackets(), but rather schedules it for a next call.
> > Other
> > > things might get scheduled in between, or the thread might decide to rest
in
> > > between to make sure it doesn't hog the CPU.
> > > 
> > > (From an empirical stand-point, I can tell you that the
> > this-thread-is-too-busy
> > > error is not scheduled, and that I can see several things happening on the
> > same
> > > queue at the same time without interruption. The video-loopback works
fine,
> > for
> > > example.)
> > 
> > But can't we delay the task until it's time to actually send the next
packet?
> 
> More things might be working on this task than this one DirectTransport
> instance.

I mean to say, we have two options here:
1. Wait() from here, as before. This is not a real option, because more things
might be working on this SingleThreadedTaskQueue than just this one
DirectTransport.
2. Introduce PostDelayedTask(). This is possible, but would increase the
complexity of SingleThreadedTaskQueue. Do you think we should do it? If so, in
this CL, or a separate additional one?

[nisse-webrtc, 2017-08-15 07:16:27]

On 2017/08/14 17:17:54, eladalon wrote:
> 2. Introduce PostDelayedTask(). This is possible, but would increase the
> complexity of SingleThreadedTaskQueue. Do you think we should do it? If so, in
> this CL, or a separate additional one?

Before doing that, consider extending MessageQueue with methods to post a
functor and/or QueuedTask. See the messagequeue.{h,cc} changes in cl
https://codereview.webrtc.org/2915253002/ for inspiration, they're pretty
minimal. These methods can then be called also in the child class rtc::Thread.

I think it should solve the problem at hand, and at the same time be generally
useful for migration to TaskQueue in other places. And then I'm sure there are
also some drawbacks. There's no obvious (to me) way to cancel posted tasks,
you'd just have to use the post-functor-signalling-an-event trick to wait until
completion.

[eladalon, 2017-08-16 12:54:26]

After we've run into a circular-dependency problem on sequence-checker when
trying to use TaskQueue, we're back to using a SingleThreadedTaskQueueForTesting
class. This is not 100% done yet, but PTAL. 
*Please bear in mind that I aim to make SingleThreadedTaskQueueForTesting
simple, not to make it efficient.*

[stefan-webrtc, 2017-08-18 10:57:32]

lgtm from me assuming comments during pair review yesterday are addressed.

[stefan-webrtc, 2017-08-18 10:57:51]

Would probably be good with a bug to reference though.

[nisse-webrtc, 2017-08-18 11:08:30]

I've looked through this, but it's not so easy to review, since there's so much
code which is moved around. Any way to reduce patch size would help.

https://codereview.webrtc.org/2998923002/diff/120001/webrtc/call/call_perf_te...
File webrtc/call/call_perf_tests.cc (right):

https://codereview.webrtc.org/2998923002/diff/120001/webrtc/call/call_perf_te...
webrtc/call/call_perf_tests.cc:171: task_queue_.SendTask([&]() {
Check what coding style says about capture-everything, I think it's discouraged
even if I guess the alternative wouldn't look very nice here.

Also, I think I'd prefer to only put work that must be done on the right thread
inside the task (and comment on why it has to be there), and keep simple local
setup, like the copy_if blocks, outside of the task.

https://codereview.webrtc.org/2998923002/diff/120001/webrtc/test/BUILD.gn
File webrtc/test/BUILD.gn (right):

https://codereview.webrtc.org/2998923002/diff/120001/webrtc/test/BUILD.gn#new...
webrtc/test/BUILD.gn:401: ":single_threaded_task_queue",
Add :single_thread_task_queue to public_deps instead, then you don't need to
list it everywhere the direct_transport target is used.

https://codereview.webrtc.org/2998923002/diff/120001/webrtc/test/call_test.cc
File webrtc/test/call_test.cc (right):

https://codereview.webrtc.org/2998923002/diff/120001/webrtc/test/call_test.cc...
webrtc/test/call_test.cc:60: task_queue_.SendTask([this, test]() {
There's a lot of code moving around here, not clear to me why.

Some setup could be left out of the task, if that would make anything clearer,
e.g., the assignments to num_*_streams_.

https://codereview.webrtc.org/2998923002/diff/120001/webrtc/test/direct_trans...
File webrtc/test/direct_transport.cc (right):

https://codereview.webrtc.org/2998923002/diff/120001/webrtc/test/direct_trans...
webrtc/test/direct_transport.cc:62:
task_queue_->CancelTask(next_scheduled_task_);
So we only need Cancel for tasks which were posted with a delay?

https://codereview.webrtc.org/2998923002/diff/120001/webrtc/test/single_threa...
File webrtc/test/single_threaded_task_queue.h (right):

https://codereview.webrtc.org/2998923002/diff/120001/webrtc/test/single_threa...
webrtc/test/single_threaded_task_queue.h:81: // By only Set()-ing when going
from 0 pending tasks to 1 (or stopping), and
To me, it seems the implementation doesn't agree with this comment.

To make it clear that the code isn't racy, I think you'd need to document more
precisely what this event means.

https://codereview.webrtc.org/2998923002/diff/120001/webrtc/test/single_threa...
File webrtc/test/single_threaded_task_queue_unittest.cc (right):

https://codereview.webrtc.org/2998923002/diff/120001/webrtc/test/single_threa...
webrtc/test/single_threaded_task_queue_unittest.cc:42: task_queue.PostTask([&]()
{
Prefer explicit capture list, for this and other lambdas.

https://codereview.webrtc.org/2998923002/diff/120001/webrtc/test/single_threa...
webrtc/test/single_threaded_task_queue_unittest.cc:181: // Wait less than is
enough, make sure the task was not yet executed.
Tests like this easily become flaky. Ideally, use a fake clock
(rtc_base/fakeclock.h), or if that doesn't work, you really need huge margins.

https://codereview.webrtc.org/2998923002/diff/120001/webrtc/test/single_threa...
webrtc/test/single_threaded_task_queue_unittest.cc:188:
TaskWithLesserDelayPostedAfterFirstDelayedTaskExectuedBeforeFirst) {
This is also potentially flaky.

https://codereview.webrtc.org/2998923002/diff/120001/webrtc/test/single_threa...
webrtc/test/single_threaded_task_queue_unittest.cc:332:
ASSERT_FALSE(delay.Wait(1000));
When delay is all you want, prefer the sleep function.

Not sure what's the preferred sleep function is, but perhaps Thread::SleepMs.

https://codereview.webrtc.org/2998923002/diff/120001/webrtc/video/end_to_end_...
File webrtc/video/end_to_end_tests.cc (right):

https://codereview.webrtc.org/2998923002/diff/120001/webrtc/video/end_to_end_...
webrtc/video/end_to_end_tests.cc:229: sender_transport =
rtc::MakeUnique<test::DirectTransport>(
Looks odd to me to create the transports from a task on the task queue. I takie
it CreateCalls must be on the task queue, but creation of the transports don't
have to, right?

As for the calls to SetReceiver, I don't know. Would perhaps be more natural to
pass the receivers to the DirectTransport constructor?

(This pattern is used in several places, if changed, change them all).

[eladalon, 2017-08-18 11:09:22]

Description was changed from

==========
Use SingleThreadedTaskQueue in DirectTransport

DirectTransport has so far used its own thread, which led to a different
threading-model for in the unit-tests than is used in actual WebRTC. Because of
that, some critical-sections that weren't truly necessary in WebRTC could not be
replaced with thread-checks, because those checks failed in unit-tests.

This CL introduces SingleThreadedTaskQueue - a TaskQueue which guarantees to run
all of its tasks on the same thread (rtc::TaskQueue doesn't guarantee that on
Mac) - and uses that for DirectTransport. CLs based on top of this will
uncomment thread-checks which had to be commented out before, and remove
unnecessary critical-sections.

Future work would probably replace the thread-checkers by more sophisticated
serialized-access checks, allowing us to move from the SingleThreadedTaskQueue
to a normal TaskQueue.

Please note that this CL is still WIP; some tests fail on Android/Linux, and
some TODOs remain.

TODO: List the bugs associated with this CL.

BUG=None
==========

to

==========
Use SingleThreadedTaskQueue in DirectTransport

DirectTransport has so far used its own thread, which led to a different
threading-model for in the unit-tests than is used in actual WebRTC. Because of
that, some critical-sections that weren't truly necessary in WebRTC could not be
replaced with thread-checks, because those checks failed in unit-tests.

This CL introduces SingleThreadedTaskQueue - a TaskQueue which guarantees to run
all of its tasks on the same thread (rtc::TaskQueue doesn't guarantee that on
Mac) - and uses that for DirectTransport. CLs based on top of this will
uncomment thread-checks which had to be commented out before, and remove
unnecessary critical-sections.

Future work would probably replace the thread-checkers by more sophisticated
serialized-access checks, allowing us to move from the SingleThreadedTaskQueue
to a normal TaskQueue.

BUG=webrtc:8113, werbrtc:7405, werbrtc:8056,
==========

[eladalon, 2017-08-18 12:12:38]

https://codereview.webrtc.org/2998923002/diff/120001/webrtc/call/call_perf_te...
File webrtc/call/call_perf_tests.cc (right):

https://codereview.webrtc.org/2998923002/diff/120001/webrtc/call/call_perf_te...
webrtc/call/call_perf_tests.cc:171: task_queue_.SendTask([&]() {
On 2017/08/18 11:08:29, nisse-webrtc wrote:
> Check what coding style says about capture-everything, I think it's
discouraged
> even if I guess the alternative wouldn't look very nice here.
> 
> Also, I think I'd prefer to only put work that must be done on the right
thread
> inside the task (and comment on why it has to be there), and keep simple local
> setup, like the copy_if blocks, outside of the task.

* When there weren't too many objects to capture, I listed them explicitly. This
is one case where this would have been excessive.
* About the lambdas - the coding style appears to allow that in cases such as
this, where the lambda cannot outlive the captured object.
* The copy_if is in the lambda because I am aiming to minimize the amount of
changes this CL introduces. Moving it outside of the capture block would have
required either rearranging the function, which would have made the review more
difficult, or breaking things into more lambdas than is strictly necessary,
which I think would have been sub-optimal.
* Please also note that when creating a CL with so many changes, I could not
delve into all of the details of each test I was modifying. I had to work
somewhat mechanically (outside of the direct_transport and
single_thread_task_queue files), leaving things as well/badly-written as they
were before.

https://codereview.webrtc.org/2998923002/diff/120001/webrtc/test/BUILD.gn
File webrtc/test/BUILD.gn (right):

https://codereview.webrtc.org/2998923002/diff/120001/webrtc/test/BUILD.gn#new...
webrtc/test/BUILD.gn:401: ":single_threaded_task_queue",
On 2017/08/18 11:08:29, nisse-webrtc wrote:
> Add :single_thread_task_queue to public_deps instead, then you don't need to
> list it everywhere the direct_transport target is used.

Done.

https://codereview.webrtc.org/2998923002/diff/120001/webrtc/test/call_test.cc
File webrtc/test/call_test.cc (right):

https://codereview.webrtc.org/2998923002/diff/120001/webrtc/test/call_test.cc...
webrtc/test/call_test.cc:60: task_queue_.SendTask([this, test]() {
On 2017/08/18 11:08:29, nisse-webrtc wrote:
> There's a lot of code moving around here, not clear to me why.
> 
> Some setup could be left out of the task, if that would make anything clearer,
> e.g., the assignments to num_*_streams_.

* The diff-tool used by the code-review system is misleading (I recommend
BeyondCompare, btw). Actually very few lines where changed; it's mostly
indentation.
* I think it's clearer to have everything happen on the task queue, except for
what really must not (PerformTest, because it often blocks while waiting on the
task-queue's network events), than hunt around for which things must happen on
the queue and which don't have to. That also creates a more minimalistic, more
tractable CL.

https://codereview.webrtc.org/2998923002/diff/120001/webrtc/test/direct_trans...
File webrtc/test/direct_transport.cc (right):

https://codereview.webrtc.org/2998923002/diff/120001/webrtc/test/direct_trans...
webrtc/test/direct_transport.cc:62:
task_queue_->CancelTask(next_scheduled_task_);
On 2017/08/18 11:08:29, nisse-webrtc wrote:
> So we only need Cancel for tasks which were posted with a delay?

The delay doesn't matter. What matters is that we can only delay tasks which
were posted (asynchronous), not ones that were sent (synchronous).
1. The reason is mostly that we don't really ever need to cancel the latter, and
since this class will only be used in tests, there's no reason to support more
than we need.
2. An additional reason is that an asynchronous task has an obvious entity to
provide the cancellation handle to - the caller of Post(). But a synchronous
task won't be able to do that until the task has already been executed, meaning
it's un-cancellable. Ways around this would be significant overkill, especially
given that it isn't used.

https://codereview.webrtc.org/2998923002/diff/120001/webrtc/test/single_threa...
File webrtc/test/single_threaded_task_queue.h (right):

https://codereview.webrtc.org/2998923002/diff/120001/webrtc/test/single_threa...
webrtc/test/single_threaded_task_queue.h:81: // By only Set()-ing when going
from 0 pending tasks to 1 (or stopping), and
On 2017/08/18 11:08:29, nisse-webrtc wrote:
> To me, it seems the implementation doesn't agree with this comment.
> 
> To make it clear that the code isn't racy, I think you'd need to document more
> precisely what this event means.

You're right, this comment went out-of-date while I was writing this class.
Thanks for catching.

https://codereview.webrtc.org/2998923002/diff/120001/webrtc/test/single_threa...
File webrtc/test/single_threaded_task_queue_unittest.cc (right):

https://codereview.webrtc.org/2998923002/diff/120001/webrtc/test/single_threa...
webrtc/test/single_threaded_task_queue_unittest.cc:42: task_queue.PostTask([&]()
{
On 2017/08/18 11:08:29, nisse-webrtc wrote:
> Prefer explicit capture list, for this and other lambdas.

Done.

https://codereview.webrtc.org/2998923002/diff/120001/webrtc/test/single_threa...
webrtc/test/single_threaded_task_queue_unittest.cc:181: // Wait less than is
enough, make sure the task was not yet executed.
On 2017/08/18 11:08:29, nisse-webrtc wrote:
> Tests like this easily become flaky. Ideally, use a fake clock
> (rtc_base/fakeclock.h), or if that doesn't work, you really need huge margins.

I had a TODO for using a fake-clock here, but I removed it when I realized that
after the fake-clock was advanced, I'd still need to wait some time in
real-clock-space, because the CPU would still need to have time to execute the
scheduled tasks.

Perhaps you would like to suggest different values for |delay_ms|, etc., to
reduce the chances of flakiness? If so, please bear in mind that many small
delays add up to long run times for the entire test suite.

https://codereview.webrtc.org/2998923002/diff/120001/webrtc/test/single_threa...
webrtc/test/single_threaded_task_queue_unittest.cc:188:
TaskWithLesserDelayPostedAfterFirstDelayedTaskExectuedBeforeFirst) {
On 2017/08/18 11:08:29, nisse-webrtc wrote:
> This is also potentially flaky.

Same answer.

https://codereview.webrtc.org/2998923002/diff/120001/webrtc/test/single_threa...
webrtc/test/single_threaded_task_queue_unittest.cc:332:
ASSERT_FALSE(delay.Wait(1000));
On 2017/08/18 11:08:29, nisse-webrtc wrote:
> When delay is all you want, prefer the sleep function.
> 
> Not sure what's the preferred sleep function is, but perhaps Thread::SleepMs.

Thread::SleepMs() is only used in six places in our codebase, so I'm wary of
using it. Perhaps something else?

https://codereview.webrtc.org/2998923002/diff/120001/webrtc/video/end_to_end_...
File webrtc/video/end_to_end_tests.cc (right):

https://codereview.webrtc.org/2998923002/diff/120001/webrtc/video/end_to_end_...
webrtc/video/end_to_end_tests.cc:229: sender_transport =
rtc::MakeUnique<test::DirectTransport>(
On 2017/08/18 11:08:30, nisse-webrtc wrote:
> Looks odd to me to create the transports from a task on the task queue. I
takie
> it CreateCalls must be on the task queue, but creation of the transports don't
> have to, right?
> 
> As for the calls to SetReceiver, I don't know. Would perhaps be more natural
to
> pass the receivers to the DirectTransport constructor?
> 
> (This pattern is used in several places, if changed, change them all).

* The way I understand it, the transports also need to happen on the task queue.
* I am aiming to make this CL, which is already big, as small as possible. I
would rather not make more changes than we have to, or make the separation of
on-task-queue and off-task-queue less than trivial. I try to follow a pattern of
"everything on the task queue, except for those things that are the test itself,
waiting for events to happen, events that the task queue is causing."

[eladalon, 2017-08-18 14:16:49]

Description was changed from

==========
Use SingleThreadedTaskQueue in DirectTransport

DirectTransport has so far used its own thread, which led to a different
threading-model for in the unit-tests than is used in actual WebRTC. Because of
that, some critical-sections that weren't truly necessary in WebRTC could not be
replaced with thread-checks, because those checks failed in unit-tests.

This CL introduces SingleThreadedTaskQueue - a TaskQueue which guarantees to run
all of its tasks on the same thread (rtc::TaskQueue doesn't guarantee that on
Mac) - and uses that for DirectTransport. CLs based on top of this will
uncomment thread-checks which had to be commented out before, and remove
unnecessary critical-sections.

Future work would probably replace the thread-checkers by more sophisticated
serialized-access checks, allowing us to move from the SingleThreadedTaskQueue
to a normal TaskQueue.

BUG=webrtc:8113, werbrtc:7405, werbrtc:8056,
==========

to

==========
Use SingleThreadedTaskQueue in DirectTransport

DirectTransport has so far used its own thread, which led to a different
threading-model for in the unit-tests than is used in actual WebRTC. Because of
that, some critical-sections that weren't truly necessary in WebRTC could not be
replaced with thread-checks, because those checks failed in unit-tests.

This CL introduces SingleThreadedTaskQueue - a TaskQueue which guarantees to run
all of its tasks on the same thread (rtc::TaskQueue doesn't guarantee that on
Mac) - and uses that for DirectTransport. CLs based on top of this will
uncomment thread-checks which had to be commented out before, and remove
unnecessary critical-sections.

Future work would probably replace the thread-checkers by more sophisticated
serialized-access checks, allowing us to move from the SingleThreadedTaskQueue
to a normal TaskQueue.

BUG=webrtc:8113, werbrtc:7405, werbrtc:8056, webrtc:8116
==========

[eladalon, 2017-08-18 14:17:07]

Description was changed from

==========
Use SingleThreadedTaskQueue in DirectTransport

DirectTransport has so far used its own thread, which led to a different
threading-model for in the unit-tests than is used in actual WebRTC. Because of
that, some critical-sections that weren't truly necessary in WebRTC could not be
replaced with thread-checks, because those checks failed in unit-tests.

This CL introduces SingleThreadedTaskQueue - a TaskQueue which guarantees to run
all of its tasks on the same thread (rtc::TaskQueue doesn't guarantee that on
Mac) - and uses that for DirectTransport. CLs based on top of this will
uncomment thread-checks which had to be commented out before, and remove
unnecessary critical-sections.

Future work would probably replace the thread-checkers by more sophisticated
serialized-access checks, allowing us to move from the SingleThreadedTaskQueue
to a normal TaskQueue.

BUG=webrtc:8113, werbrtc:7405, werbrtc:8056, webrtc:8116
==========

to

==========
Use SingleThreadedTaskQueue in DirectTransport

DirectTransport has so far used its own thread, which led to a different
threading-model for in the unit-tests than is used in actual WebRTC. Because of
that, some critical-sections that weren't truly necessary in WebRTC could not be
replaced with thread-checks, because those checks failed in unit-tests.

This CL introduces SingleThreadedTaskQueue - a TaskQueue which guarantees to run
all of its tasks on the same thread (rtc::TaskQueue doesn't guarantee that on
Mac) - and uses that for DirectTransport. CLs based on top of this will
uncomment thread-checks which had to be commented out before, and remove
unnecessary critical-sections.

Future work would probably replace the thread-checkers by more sophisticated
serialized-access checks, allowing us to move from the SingleThreadedTaskQueue
to a normal TaskQueue.

BUG=webrtc:8113, webrtc:7405, webrtc:8056, webrtc:8116
==========

[nisse-webrtc, 2017-08-21 09:07:07]

lgtm, with some nits for comment and cl description.

For review, I applied the patch locally, and then git diff -b produced a
reasonable diff omitting the indentation changes.

As for use of the real clock in the tests, be prepared to handle any problems
with flakiness. It's hard to guess in advance if it's going to be a problem.

https://codereview.webrtc.org/2998923002/diff/140001/webrtc/call/bitrate_esti...
File webrtc/call/bitrate_estimator_tests.cc (left):

https://codereview.webrtc.org/2998923002/diff/140001/webrtc/call/bitrate_esti...
webrtc/call/bitrate_estimator_tests.cc:140: send_transport_->StopSending();
Why is StopSending no longer needed?

Also dropped in call_test.cc.

https://codereview.webrtc.org/2998923002/diff/140001/webrtc/test/call_test.h
File webrtc/test/call_test.h (right):

https://codereview.webrtc.org/2998923002/diff/140001/webrtc/test/call_test.h#...
webrtc/test/call_test.h:168: // Should be last, to destruct first.
Explain briefly *why* it has to be destructed first. I guess it will refer to
some task which in turn will refer to other instance variables?

https://codereview.webrtc.org/2998923002/diff/140001/webrtc/test/direct_trans...
File webrtc/test/direct_transport.h (left):

https://codereview.webrtc.org/2998923002/diff/140001/webrtc/test/direct_trans...
webrtc/test/direct_transport.h:53: virtual void StopSending();
Ok, so you're deleting this method? Worth mentioning in the cl description.

https://codereview.webrtc.org/2998923002/diff/140001/webrtc/test/single_threa...
File webrtc/test/single_threaded_task_queue.h (right):

https://codereview.webrtc.org/2998923002/diff/140001/webrtc/test/single_threa...
webrtc/test/single_threaded_task_queue.h:25: // This class gives capabilities
similar to rtc::TaskQueue, but insures
s/insures/ensures/

https://codereview.webrtc.org/2998923002/diff/140001/webrtc/test/single_threa...
webrtc/test/single_threaded_task_queue.h:89: // * When the next task in the
queue is a delayed-task, and the time for
Thanks for the clarification. Maybe mention that it's only in the first two
cases that this event is set.

[eladalon, 2017-08-21 10:43:24]

Description was changed from

==========
Use SingleThreadedTaskQueue in DirectTransport

DirectTransport has so far used its own thread, which led to a different
threading-model for in the unit-tests than is used in actual WebRTC. Because of
that, some critical-sections that weren't truly necessary in WebRTC could not be
replaced with thread-checks, because those checks failed in unit-tests.

This CL introduces SingleThreadedTaskQueue - a TaskQueue which guarantees to run
all of its tasks on the same thread (rtc::TaskQueue doesn't guarantee that on
Mac) - and uses that for DirectTransport. CLs based on top of this will
uncomment thread-checks which had to be commented out before, and remove
unnecessary critical-sections.

Future work would probably replace the thread-checkers by more sophisticated
serialized-access checks, allowing us to move from the SingleThreadedTaskQueue
to a normal TaskQueue.

BUG=webrtc:8113, webrtc:7405, webrtc:8056, webrtc:8116
==========

to

==========
Use SingleThreadedTaskQueue in DirectTransport

DirectTransport has so far used its own thread, which led to a different
threading-model for in the unit-tests than is used in actual WebRTC. Because of
that, some critical-sections that weren't truly necessary in WebRTC could not be
replaced with thread-checks, because those checks failed in unit-tests.

This CL introduces SingleThreadedTaskQueue - a TaskQueue which guarantees to run
all of its tasks on the same thread (rtc::TaskQueue doesn't guarantee that on
Mac) - and uses that for DirectTransport. CLs based on top of this will
uncomment thread-checks which had to be commented out before, and remove
unnecessary critical-sections.

Future work would probably replace the thread-checkers by more sophisticated
serialized-access checks, allowing us to move from the SingleThreadedTaskQueue
to a normal TaskQueue.

Related implementation notes:
* This CL has made DirectTransport::StopSending() superfluous, and so it was
deleted.

BUG=webrtc:8113, webrtc:7405, webrtc:8056, webrtc:8116
==========

[eladalon, 2017-08-21 10:56:54]

https://codereview.webrtc.org/2998923002/diff/140001/webrtc/call/bitrate_esti...
File webrtc/call/bitrate_estimator_tests.cc (left):

https://codereview.webrtc.org/2998923002/diff/140001/webrtc/call/bitrate_esti...
webrtc/call/bitrate_estimator_tests.cc:140: send_transport_->StopSending();
On 2017/08/21 09:07:07, nisse-webrtc wrote:
> Why is StopSending no longer needed?
> 
> Also dropped in call_test.cc.

reset() both stops as well as deallocates, so StopSending() became superfluous,
and so I deleted it.

https://codereview.webrtc.org/2998923002/diff/140001/webrtc/test/call_test.h
File webrtc/test/call_test.h (right):

https://codereview.webrtc.org/2998923002/diff/140001/webrtc/test/call_test.h#...
webrtc/test/call_test.h:168: // Should be last, to destruct first.
On 2017/08/21 09:07:07, nisse-webrtc wrote:
> Explain briefly *why* it has to be destructed first. I guess it will refer to
> some task which in turn will refer to other instance variables?

Actually, come to think of it, putting this last does not provide enough of a
guarantee, because this class is not final; sub-classes might add tasks which
refer to their own member-variables.

https://codereview.webrtc.org/2998923002/diff/140001/webrtc/test/direct_trans...
File webrtc/test/direct_transport.h (left):

https://codereview.webrtc.org/2998923002/diff/140001/webrtc/test/direct_trans...
webrtc/test/direct_transport.h:53: virtual void StopSending();
On 2017/08/21 09:07:07, nisse-webrtc wrote:
> Ok, so you're deleting this method? Worth mentioning in the cl description.

Done.

https://codereview.webrtc.org/2998923002/diff/140001/webrtc/test/single_threa...
File webrtc/test/single_threaded_task_queue.h (right):

https://codereview.webrtc.org/2998923002/diff/140001/webrtc/test/single_threa...
webrtc/test/single_threaded_task_queue.h:25: // This class gives capabilities
similar to rtc::TaskQueue, but insures
On 2017/08/21 09:07:07, nisse-webrtc wrote:
> s/insures/ensures/

Done.

https://codereview.webrtc.org/2998923002/diff/140001/webrtc/test/single_threa...
webrtc/test/single_threaded_task_queue.h:89: // * When the next task in the
queue is a delayed-task, and the time for
On 2017/08/21 09:07:07, nisse-webrtc wrote:
> Thanks for the clarification. Maybe mention that it's only in the first two
> cases that this event is set.

Done.

[eladalon, 2017-08-21 15:00:22]

The CQ bit was checked by eladalon@webrtc.org

[eladalon, 2017-08-21 15:00:22]

The patchset sent to the CQ was uploaded after l-g-t-m from stefan@webrtc.org,
nisse@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2998923002/#ps180001 (title:
"Handle deprecated functions for internal projects.")

[commit-bot: I haz the power, 2017-08-21 15:00:28]

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-08-21 15:04:37]

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

[commit-bot: I haz the power, 2017-08-21 15:04:38]

Try jobs failed on following builders:
  mac_baremetal on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/mac_baremetal/builds/24110)

[eladalon, 2017-08-21 15:06:56]

The CQ bit was checked by eladalon@webrtc.org

[eladalon, 2017-08-21 15:06:57]

The patchset sent to the CQ was uploaded after l-g-t-m from nisse@webrtc.org,
stefan@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2998923002/#ps200001 (title:
"Sequenced-checker dependency.")

[commit-bot: I haz the power, 2017-08-21 15:07:07]

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-08-21 15:31:56]

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

[commit-bot: I haz the power, 2017-08-21 15:31:58]

Try jobs failed on following builders:
  linux_baremetal on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/linux_baremetal/builds/...)

[eladalon, 2017-08-21 16:06:39]

The CQ bit was checked by eladalon@webrtc.org

[eladalon, 2017-08-21 16:06:40]

The patchset sent to the CQ was uploaded after l-g-t-m from nisse@webrtc.org,
stefan@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2998923002/#ps220001 (title:
"Fix perf-tests.")

[commit-bot: I haz the power, 2017-08-21 16:06:47]

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-08-21 16:27:56]

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

[commit-bot: I haz the power, 2017-08-21 16:27:57]

Try jobs failed on following builders:
  linux_baremetal on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/linux_baremetal/builds/...)

[eladalon, 2017-08-21 17:29:27]

The CQ bit was checked by eladalon@webrtc.org

[eladalon, 2017-08-21 17:29:28]

The patchset sent to the CQ was uploaded after l-g-t-m from nisse@webrtc.org,
stefan@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2998923002/#ps240001 (title:
"Release capturer on correct thread.")

[commit-bot: I haz the power, 2017-08-21 17:29:38]

CQ is trying da patch.

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

[eladalon, 2017-08-21 17:32:19]

The CQ bit was checked by eladalon@webrtc.org

[eladalon, 2017-08-21 17:32:20]

The patchset sent to the CQ was uploaded after l-g-t-m from nisse@webrtc.org,
stefan@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2998923002/#ps260001 (title:
"Fix video_loopback teardown on Linux.")

[commit-bot: I haz the power, 2017-08-21 17:32:28]

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-08-21 17:46:36]

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

[commit-bot: I haz the power, 2017-08-21 17:46:38]

Try jobs failed on following builders:
  linux_memcheck on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/linux_memcheck/builds/1...)

[eladalon, 2017-08-22 09:56:05]

The CQ bit was checked by eladalon@webrtc.org

[eladalon, 2017-08-22 09:56:06]

The patchset sent to the CQ was uploaded after l-g-t-m from nisse@webrtc.org,
stefan@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2998923002/#ps280001 (title:
"Some fixes.")

[commit-bot: I haz the power, 2017-08-22 09:56:09]

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-08-22 10:20:37]

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

[commit-bot: I haz the power, 2017-08-22 10:20:39]

Try jobs failed on following builders:
  win_msvc_rel on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/win_msvc_rel/builds/42)

[eladalon, 2017-08-22 10:32:49]

The CQ bit was checked by eladalon@webrtc.org

[eladalon, 2017-08-22 10:32:50]

The patchset sent to the CQ was uploaded after l-g-t-m from nisse@webrtc.org,
stefan@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2998923002/#ps300001 (title:
"Appease win_msvc_rel.")

[commit-bot: I haz the power, 2017-08-22 10:32:58]

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-08-22 11:02:52]

CQ is committing da patch.

Bot data: {"patchset_id": 300001, "attempt_start_ts": 1503397969759060,
"parent_rev": "2a596549ca6fba655727dbf24505c75711920340", "commit_rev":
"413ee9a010eca70712a6935c68b815201bee1270"}

[commit-bot: I haz the power, 2017-08-22 11:02:58]

Description was changed from

==========
Use SingleThreadedTaskQueue in DirectTransport

DirectTransport has so far used its own thread, which led to a different
threading-model for in the unit-tests than is used in actual WebRTC. Because of
that, some critical-sections that weren't truly necessary in WebRTC could not be
replaced with thread-checks, because those checks failed in unit-tests.

This CL introduces SingleThreadedTaskQueue - a TaskQueue which guarantees to run
all of its tasks on the same thread (rtc::TaskQueue doesn't guarantee that on
Mac) - and uses that for DirectTransport. CLs based on top of this will
uncomment thread-checks which had to be commented out before, and remove
unnecessary critical-sections.

Future work would probably replace the thread-checkers by more sophisticated
serialized-access checks, allowing us to move from the SingleThreadedTaskQueue
to a normal TaskQueue.

Related implementation notes:
* This CL has made DirectTransport::StopSending() superfluous, and so it was
deleted.

BUG=webrtc:8113, webrtc:7405, webrtc:8056, webrtc:8116
==========

to

==========
Use SingleThreadedTaskQueue in DirectTransport

DirectTransport has so far used its own thread, which led to a different
threading-model for in the unit-tests than is used in actual WebRTC. Because of
that, some critical-sections that weren't truly necessary in WebRTC could not be
replaced with thread-checks, because those checks failed in unit-tests.

This CL introduces SingleThreadedTaskQueue - a TaskQueue which guarantees to run
all of its tasks on the same thread (rtc::TaskQueue doesn't guarantee that on
Mac) - and uses that for DirectTransport. CLs based on top of this will
uncomment thread-checks which had to be commented out before, and remove
unnecessary critical-sections.

Future work would probably replace the thread-checkers by more sophisticated
serialized-access checks, allowing us to move from the SingleThreadedTaskQueue
to a normal TaskQueue.

Related implementation notes:
* This CL has made DirectTransport::StopSending() superfluous, and so it was
deleted.

BUG=webrtc:8113, webrtc:7405, webrtc:8056, webrtc:8116

Review-Url: https://codereview.webrtc.org/2998923002
Cr-Commit-Position: refs/heads/master@{#19445}
Committed:
https://chromium.googlesource.com/external/webrtc/+/413ee9a010eca70712a6935c6...
==========

[commit-bot: I haz the power, 2017-08-22 11:03:00]

Committed patchset #16 (id:300001) as
https://chromium.googlesource.com/external/webrtc/+/413ee9a010eca70712a6935c6...

[danilchap, 2017-08-23 15:49:22]

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

[danilchap, 2017-08-23 15:49:23]

https://codereview.webrtc.org/2998923002/diff/300001/webrtc/test/direct_trans...
File webrtc/test/direct_transport.h (right):

https://codereview.webrtc.org/2998923002/diff/300001/webrtc/test/direct_trans...
webrtc/test/direct_transport.h:105: SingleThreadedTaskQueueForTesting::TaskId
next_scheduled_task_;
why this member is unguarded? access to it seems racy:
1) it is accessed in constructor, but before it is assigned, there is a call
PostTask on a different thread that also tries to assign same variable.

2) it is used to cancel task (read) which seems expected to be done outside of
the task queue thread, but task queue can write into it same time on different
thread.

[eladalon, 2017-08-24 07:56:52]

https://codereview.webrtc.org/2998923002/diff/300001/webrtc/test/direct_trans...
File webrtc/test/direct_transport.h (right):

https://codereview.webrtc.org/2998923002/diff/300001/webrtc/test/direct_trans...
webrtc/test/direct_transport.h:105: SingleThreadedTaskQueueForTesting::TaskId
next_scheduled_task_;
On 2017/08/23 15:49:23, danilchap wrote:
> why this member is unguarded? access to it seems racy:
> 1) it is accessed in constructor, but before it is assigned, there is a call
> PostTask on a different thread that also tries to assign same variable.
> 
> 2) it is used to cancel task (read) which seems expected to be done outside of
> the task queue thread, but task queue can write into it same time on different
> thread.

The *deprecated* version is racy, because it runs the ctor outside of the
task-queue.
The "approved" version isn't.
The solution is to migrate all projects away from the deprecated version, then
remove it.