Remove all glue interfaces and use existing webrtc interfaces

Initially when the design was to do this experiment in browser, which doesn't have webrtc code, it requires some glue code to bridge the difference between what's available in webrtc::base and browser process. Now since we're moving to renderer process, we could reuse a lot of existing interfaces instead of rolling our own.

BUG=webrtc:4576
R=pthatcher@webrtc.org

Committed: https://chromium.googlesource.com/external/webrtc/+/dc13abc331b8e5ce2bdf99722aba3fdd9c0444ff

[guoweis_webrtc, 2015-06-11 17:17:49]

Patchset #2 (id:20001) has been deleted

[guoweis_webrtc, 2015-06-11 17:17:53]

Patchset #1 (id:1) has been deleted

[guoweis_webrtc, 2015-06-11 17:20:12]

guoweis@webrtc.org changed reviewers:
+ pthatcher@webrtc.org

[guoweis_webrtc, 2015-06-11 17:20:13]

PTAL.

[guoweis_webrtc, 2015-06-11 17:38:11]

Patchset #2 (id:60001) has been deleted

[Sergey Ulanov, 2015-06-12 19:28:26]

sergeyu@chromium.org changed reviewers:
+ sergeyu@chromium.org

[Sergey Ulanov, 2015-06-12 19:28:27]

https://codereview.webrtc.org/1173353002/diff/130001/webrtc/p2p/stunprober/st...
File webrtc/p2p/stunprober/stunprober.cc (right):

https://codereview.webrtc.org/1173353002/diff/130001/webrtc/p2p/stunprober/st...
webrtc/p2p/stunprober/stunprober.cc:394: 1 /* ms */);
Why does the task need to be scheduled every 1 ms? As far as I can tell in most
cases it doesn't do anything except scheduling another task. Can you just
schedule a task with delay (next_request_time_ms_ - now)?

[guoweis_webrtc, 2015-06-12 21:04:22]

On 2015/06/12 19:28:27, Sergey Ulanov wrote:
>
https://codereview.webrtc.org/1173353002/diff/130001/webrtc/p2p/stunprober/st...
> File webrtc/p2p/stunprober/stunprober.cc (right):
> 
>
https://codereview.webrtc.org/1173353002/diff/130001/webrtc/p2p/stunprober/st...
> webrtc/p2p/stunprober/stunprober.cc:394: 1 /* ms */);
> Why does the task need to be scheduled every 1 ms? As far as I can tell in
most
> cases it doesn't do anything except scheduling another task. Can you just
> schedule a task with delay (next_request_time_ms_ - now)?

There are 2 reasons, both are for accuracy of timing.
1. On windows, the default timer is at ~15 to 16ms. This could introduce a lot
of inaccuracy for our test. By scheduling 1 ms task, this ensures the high
resolution timer will kick in.
2. This is also a safe guard against any high CPU usage case where the thread
didn't wait up in time.

[Sergey Ulanov, 2015-06-12 21:21:24]

On 2015/06/12 21:04:22, guoweis wrote:
> On 2015/06/12 19:28:27, Sergey Ulanov wrote:
> >
>
https://codereview.webrtc.org/1173353002/diff/130001/webrtc/p2p/stunprober/st...
> > File webrtc/p2p/stunprober/stunprober.cc (right):
> > 
> >
>
https://codereview.webrtc.org/1173353002/diff/130001/webrtc/p2p/stunprober/st...
> > webrtc/p2p/stunprober/stunprober.cc:394: 1 /* ms */);
> > Why does the task need to be scheduled every 1 ms? As far as I can tell in
> most
> > cases it doesn't do anything except scheduling another task. Can you just
> > schedule a task with delay (next_request_time_ms_ - now)?
> 
> There are 2 reasons, both are for accuracy of timing.
> 1. On windows, the default timer is at ~15 to 16ms. This could introduce a lot
> of inaccuracy for our test. By scheduling 1 ms task, this ensures the high
> resolution timer will kick in.

AFAIK hi-res timers are activated for tasks that have delay lower than 32 ms:
https://code.google.com/p/chromium/codesearch#chromium/src/base/message_loop/...
So you can schedule tasks with 30ms interval and still get hi-res timers.

> 2. This is also a safe guard against any high CPU usage case where the thread
> didn't wait up in time.

Actually scheduling tasks more frequently will work against you when the system
is busy. The more frequently a thread wakes up the more CPU it consumes, which
means scheduler will give it lower priority and so it will be less likely to
preempt other processes when you need it.

[guoweis_webrtc, 2015-06-12 21:50:53]

On 2015/06/12 21:21:24, Sergey Ulanov wrote:
> On 2015/06/12 21:04:22, guoweis wrote:
> > On 2015/06/12 19:28:27, Sergey Ulanov wrote:
> > >
> >
>
https://codereview.webrtc.org/1173353002/diff/130001/webrtc/p2p/stunprober/st...
> > > File webrtc/p2p/stunprober/stunprober.cc (right):
> > > 
> > >
> >
>
https://codereview.webrtc.org/1173353002/diff/130001/webrtc/p2p/stunprober/st...
> > > webrtc/p2p/stunprober/stunprober.cc:394: 1 /* ms */);
> > > Why does the task need to be scheduled every 1 ms? As far as I can tell in
> > most
> > > cases it doesn't do anything except scheduling another task. Can you just
> > > schedule a task with delay (next_request_time_ms_ - now)?
> > 
> > There are 2 reasons, both are for accuracy of timing.
> > 1. On windows, the default timer is at ~15 to 16ms. This could introduce a
lot
> > of inaccuracy for our test. By scheduling 1 ms task, this ensures the high
> > resolution timer will kick in.
> 
> AFAIK hi-res timers are activated for tasks that have delay lower than 32 ms:
>
https://code.google.com/p/chromium/codesearch#chromium/src/base/message_loop/...
> So you can schedule tasks with 30ms interval and still get hi-res timers.
> 
> > 2. This is also a safe guard against any high CPU usage case where the
thread
> > didn't wait up in time.
> 
> Actually scheduling tasks more frequently will work against you when the
system
> is busy. The more frequently a thread wakes up the more CPU it consumes, which
> means scheduler will give it lower priority and so it will be less likely to
> preempt other processes when you need it.

For anything > 30ms, the 30ms scheduling interval will introduce inaccuracy. I
think a better way is to schedule at 5ms. I'll test it with the linux machine
and see how well it perform. Does this sound reasonable to you?

[Sergey Ulanov, 2015-06-17 23:18:51]

On 2015/06/12 21:50:53, guoweis wrote:
> On 2015/06/12 21:21:24, Sergey Ulanov wrote:
> > On 2015/06/12 21:04:22, guoweis wrote:
> > > On 2015/06/12 19:28:27, Sergey Ulanov wrote:
> > > >
> > >
> >
>
https://codereview.webrtc.org/1173353002/diff/130001/webrtc/p2p/stunprober/st...
> > > > File webrtc/p2p/stunprober/stunprober.cc (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.webrtc.org/1173353002/diff/130001/webrtc/p2p/stunprober/st...
> > > > webrtc/p2p/stunprober/stunprober.cc:394: 1 /* ms */);
> > > > Why does the task need to be scheduled every 1 ms? As far as I can tell
in
> > > most
> > > > cases it doesn't do anything except scheduling another task. Can you
just
> > > > schedule a task with delay (next_request_time_ms_ - now)?
> > > 
> > > There are 2 reasons, both are for accuracy of timing.
> > > 1. On windows, the default timer is at ~15 to 16ms. This could introduce a
> lot
> > > of inaccuracy for our test. By scheduling 1 ms task, this ensures the high
> > > resolution timer will kick in.
> > 
> > AFAIK hi-res timers are activated for tasks that have delay lower than 32
ms:
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/base/message_loop/...
> > So you can schedule tasks with 30ms interval and still get hi-res timers.
> > 
> > > 2. This is also a safe guard against any high CPU usage case where the
> thread
> > > didn't wait up in time.
> > 
> > Actually scheduling tasks more frequently will work against you when the
> system
> > is busy. The more frequently a thread wakes up the more CPU it consumes,
which
> > means scheduler will give it lower priority and so it will be less likely to
> > preempt other processes when you need it.
> 
> For anything > 30ms, the 30ms scheduling interval will introduce inaccuracy. I
> think a better way is to schedule at 5ms. I'll test it with the linux machine
> and see how well it perform. Does this sound reasonable to you?

every 5ms is better than every 1ms, but I still don't think it's necessary.
Maybe schedule it every 30 ms if the total interval is longer than 30. That way
you will still get high accuracy without unnecessary wake ups.

[guoweis_left_chromium, 2015-06-18 16:42:41]

On 2015/06/17 23:18:51, Sergey Ulanov wrote:
> On 2015/06/12 21:50:53, guoweis wrote:
> > On 2015/06/12 21:21:24, Sergey Ulanov wrote:
> > > On 2015/06/12 21:04:22, guoweis wrote:
> > > > On 2015/06/12 19:28:27, Sergey Ulanov wrote:
> > > > >
> > > >
> > >
> >
>
https://codereview.webrtc.org/1173353002/diff/130001/webrtc/p2p/stunprober/st...
> > > > > File webrtc/p2p/stunprober/stunprober.cc (right):
> > > > > 
> > > > >
> > > >
> > >
> >
>
https://codereview.webrtc.org/1173353002/diff/130001/webrtc/p2p/stunprober/st...
> > > > > webrtc/p2p/stunprober/stunprober.cc:394: 1 /* ms */);
> > > > > Why does the task need to be scheduled every 1 ms? As far as I can
tell
> in
> > > > most
> > > > > cases it doesn't do anything except scheduling another task. Can you
> just
> > > > > schedule a task with delay (next_request_time_ms_ - now)?
> > > > 
> > > > There are 2 reasons, both are for accuracy of timing.
> > > > 1. On windows, the default timer is at ~15 to 16ms. This could introduce
a
> > lot
> > > > of inaccuracy for our test. By scheduling 1 ms task, this ensures the
high
> > > > resolution timer will kick in.
> > > 
> > > AFAIK hi-res timers are activated for tasks that have delay lower than 32
> ms:
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/base/message_loop/...
> > > So you can schedule tasks with 30ms interval and still get hi-res timers.
> > > 
> > > > 2. This is also a safe guard against any high CPU usage case where the
> > thread
> > > > didn't wait up in time.
> > > 
> > > Actually scheduling tasks more frequently will work against you when the
> > system
> > > is busy. The more frequently a thread wakes up the more CPU it consumes,
> which
> > > means scheduler will give it lower priority and so it will be less likely
to
> > > preempt other processes when you need it.
> > 
> > For anything > 30ms, the 30ms scheduling interval will introduce inaccuracy.
I
> > think a better way is to schedule at 5ms. I'll test it with the linux
machine
> > and see how well it perform. Does this sound reasonable to you?
> 
> every 5ms is better than every 1ms, but I still don't think it's necessary.
> Maybe schedule it every 30 ms if the total interval is longer than 30. That
way
> you will still get high accuracy without unnecessary wake ups.

I still like 5 ms better. For the case of 50ms, I'll have to schedule a 30ms and
then another 20ms. This feels more complicated than it needs to be. Plus, I'm
not sure about the accuracy of other platforms that this code might run on. What
about Mac? What's the behavior there? I was scheduling at 1ms such that even if
system scheduling is not so accurate, (assuming CPU is not pegged), at least I'm
missing the minimum time I possibly have.

[Sergey Ulanov, 2015-06-18 17:09:46]

On 2015/06/18 16:42:41, guoweis_chromium wrote:
> I still like 5 ms better. For the case of 50ms, I'll have to schedule a 30ms
and
> then another 20ms. This feels more complicated than it needs to be.

I don't think that makes the code any less or more complicated.

> Plus, I'm
> not sure about the accuracy of other platforms that this code might run on.
What
> about Mac? What's the behavior there? I was scheduling at 1ms such that even
if
> system scheduling is not so accurate, (assuming CPU is not pegged), at least
I'm
> missing the minimum time I possibly have.

I'm pretty sure that the timer should be precise enough, the error should be
less than 5ms. But I don't mind much keeping it at 5ms as long as this is just a
temporary experiment that will be removed in the future

[pthatcher1, 2015-06-18 20:26:59]

https://codereview.chromium.org/1173353002/diff/130001/webrtc/p2p/stunprober/...
File webrtc/p2p/stunprober/stunprober.cc (right):

https://codereview.chromium.org/1173353002/diff/130001/webrtc/p2p/stunprober/...
webrtc/p2p/stunprober/stunprober.cc:288: }
What if not all of the sockets get ready?  Do we simply never schedule the stun
requests?

https://codereview.chromium.org/1173353002/diff/130001/webrtc/p2p/stunprober/...
webrtc/p2p/stunprober/stunprober.cc:359: sockets_.pop_back();
Does the Requester take ownership of the socket and delete it?

https://codereview.chromium.org/1173353002/diff/130001/webrtc/p2p/stunprober/...
File webrtc/p2p/stunprober/stunprober.h (right):

https://codereview.chromium.org/1173353002/diff/130001/webrtc/p2p/stunprober/...
webrtc/p2p/stunprober/stunprober.h:186: size_t total_ready_sockets_ = 0;
How is total_ready_sockets_ different than sockets_.size()?

[guoweis_left_chromium, 2015-06-18 20:39:38]

guoweis@chromium.org changed reviewers:
+ guoweis@chromium.org

[guoweis_left_chromium, 2015-06-18 20:39:39]

PTAL.

Changed to 5 ms interval for threading.

https://codereview.chromium.org/1173353002/diff/130001/webrtc/p2p/stunprober/...
File webrtc/p2p/stunprober/stunprober.cc (right):

https://codereview.chromium.org/1173353002/diff/130001/webrtc/p2p/stunprober/...
webrtc/p2p/stunprober/stunprober.cc:288: }
On 2015/06/18 20:26:59, pthatcher1 wrote:
> What if not all of the sockets get ready?  Do we simply never schedule the
stun
> requests?

yes.

https://codereview.chromium.org/1173353002/diff/130001/webrtc/p2p/stunprober/...
webrtc/p2p/stunprober/stunprober.cc:359: sockets_.pop_back();
On 2015/06/18 20:26:59, pthatcher1 wrote:
> Does the Requester take ownership of the socket and delete it?

yes.

https://codereview.chromium.org/1173353002/diff/130001/webrtc/p2p/stunprober/...
webrtc/p2p/stunprober/stunprober.cc:394: 1 /* ms */);
On 2015/06/12 19:28:27, Sergey Ulanov wrote:
> Why does the task need to be scheduled every 1 ms? As far as I can tell in
most
> cases it doesn't do anything except scheduling another task. Can you just
> schedule a task with delay (next_request_time_ms_ - now)?

changed to 5ms.

https://codereview.chromium.org/1173353002/diff/130001/webrtc/p2p/stunprober/...
File webrtc/p2p/stunprober/stunprober.h (right):

https://codereview.chromium.org/1173353002/diff/130001/webrtc/p2p/stunprober/...
webrtc/p2p/stunprober/stunprober.h:186: size_t total_ready_sockets_ = 0;
On 2015/06/18 20:26:59, pthatcher1 wrote:
> How is total_ready_sockets_ different than sockets_.size()?

total_ready_sockets always <= then sockets_.size(). 
We can't just move to the next step when sockets are all created, have to wait
when all of them are ready.

[pthatcher1, 2015-06-18 20:50:43]

lgtm

[guoweis_left_chromium, 2015-06-18 21:01:51]

Patchset #6 (id:150001) has been deleted

[guoweis_webrtc, 2015-06-18 21:44:52]

Committed patchset #6 (id:170001) manually as
dc13abc331b8e5ce2bdf99722aba3fdd9c0444ff (presubmit successful).