Adding OrtcFactory, and changing UdpTransport to match current plan.

Adding OrtcFactory, and changing UdpTransport to match current plan.

The factory follows the same principles as PeerConnectionFactory;
various modules can be passed into its constructor but default
implementations are provided. Currently the only object the factory can
create is a UdpTransport (need to start somewhere).

UdpTransportChannel (renamed to UdpTransport)
will now accept a socket passed into its constructor,
relying on the factory to create the socket. This allows some
simplifications to be made, such as getting rid of "State" since the
only states are now "has destination set or doesn't".

BUG=webrtc:7013
Review-Url: https://codereview.webrtc.org/2632613002
Cr-Commit-Position: refs/heads/master@{#16154}
Committed: https://chromium.googlesource.com/external/webrtc/+/d1c0998730443e4901ec329ee816fd1f115b6223

[Taylor Brandstetter, 2017-01-13 07:52:29]

https://codereview.webrtc.org/2632613002/diff/1/webrtc/api/proxy.h
File webrtc/api/proxy.h (right):

https://codereview.webrtc.org/2632613002/diff/1/webrtc/api/proxy.h#newcode366
webrtc/api/proxy.h:366: #define BEGIN_NON_REFCOUNTED_PROXY_MAP(c)               
                \
Before you say "wow these macros are getting out of hand", I agree. I can do
some refactoring before this CL is landed.

[Taylor Brandstetter, 2017-01-13 08:44:56]

Description was changed from

==========
Adding OrtcFactory, and changing UdpTransport to match current plan.

The factory follows the same principles as PeerConnectionFactory;
various modules can be passed into its constructor but default
implementations are provided. Currently the only object the factory can
create is a UdpTransport (need to start somewhere).

UdpTransport will now accept a socket passed into its constructor,
relying on the factory to create the socket. This allows some
simplifications to be made, such as getting rid of "State" since the
only states are now "has destination set or doesn't".

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

to

==========
Adding OrtcFactory, and changing UdpTransport to match current plan.

The factory follows the same principles as PeerConnectionFactory;
various modules can be passed into its constructor but default
implementations are provided. Currently the only object the factory can
create is a UdpTransport (need to start somewhere).

UdpTransportChannel (renamed to UdpTransport)
will now accept a socket passed into its constructor,
relying on the factory to create the socket. This allows some
simplifications to be made, such as getting rid of "State" since the
only states are now "has destination set or doesn't".

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

[pthatcher1, 2017-01-17 20:04:35]

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

[pthatcher1, 2017-01-17 20:04:36]

https://codereview.webrtc.org/2632613002/diff/80001/webrtc/api/ortcfactory.cc
File webrtc/api/ortcfactory.cc (right):

https://codereview.webrtc.org/2632613002/diff/80001/webrtc/api/ortcfactory.cc...
webrtc/api/ortcfactory.cc:36: owns_network_thread_ = true;
With the alternative approach, this is slightly nicer too:

owned_network_thread_ = rtc::Thread::CreateWithSocketServer();
owned_network_thread_->Start();
network_thread_ = owned_network_thread_.get();

https://codereview.webrtc.org/2632613002/diff/80001/webrtc/api/ortcfactory.cc...
webrtc/api/ortcfactory.cc:97: LOG(INFO) << "Local socket allocation failure.";
That seems more than INFO level.  At least WARNING, perhaps ERROR.

https://codereview.webrtc.org/2632613002/diff/80001/webrtc/api/ortcfactory.h
File webrtc/api/ortcfactory.h (right):

https://codereview.webrtc.org/2632613002/diff/80001/webrtc/api/ortcfactory.h#...
webrtc/api/ortcfactory.h:19: #include "webrtc/p2p/base/packetsocketfactory.h"
Should we have a TODO to move this into the api/ directory?

https://codereview.webrtc.org/2632613002/diff/80001/webrtc/api/ortcfactory.h#...
webrtc/api/ortcfactory.h:40: // |worker_thread| is used for CPU-intensive
operations.
This kind of misleading.  If the worker thread is blocked by CPU-intensive
operations, lots of stuff won't work.  Maybe we should call this the
media_thread and say that media operations happen on the media_thread or child
threads.

https://codereview.webrtc.org/2632613002/diff/80001/webrtc/api/ortcfactory.h#...
webrtc/api/ortcfactory.h:46: // rtc::Thread, or by calling
rtc::Thread::Current()->ProcessMessages).
Is it also the case that all methods call on the API must be called on the
signaling_thread?  If not, then would callback_thread be a better name?  If so,
would api_thread be a better name?

https://codereview.webrtc.org/2632613002/diff/80001/webrtc/api/ortcfactory.h#...
webrtc/api/ortcfactory.h:57: // passed in.
You might want to repeat that none of these can be destroyed before the factory
is destroyed.

https://codereview.webrtc.org/2632613002/diff/80001/webrtc/api/ortcfactory.h#...
webrtc/api/ortcfactory.h:97: bool owns_socket_factory_ = false;
The alternative approach which many places in our code uses is this:

rtc::Thread* network_thread_;
std::unique_ptr<rtc::Thread> owned_network_thread_;

That way you get destruction for free.  Just make owned_network_thread_ ==
network_thread_ when it's owned and null when it's not.

[Taylor Brandstetter, 2017-01-18 02:31:38]

https://codereview.webrtc.org/2632613002/diff/80001/webrtc/api/ortcfactory.cc
File webrtc/api/ortcfactory.cc (right):

https://codereview.webrtc.org/2632613002/diff/80001/webrtc/api/ortcfactory.cc...
webrtc/api/ortcfactory.cc:97: LOG(INFO) << "Local socket allocation failure.";
On 2017/01/17 20:04:36, pthatcher1 wrote:
> That seems more than INFO level.  At least WARNING, perhaps ERROR.

Done.

https://codereview.webrtc.org/2632613002/diff/80001/webrtc/api/ortcfactory.h
File webrtc/api/ortcfactory.h (right):

https://codereview.webrtc.org/2632613002/diff/80001/webrtc/api/ortcfactory.h#...
webrtc/api/ortcfactory.h:19: #include "webrtc/p2p/base/packetsocketfactory.h"
On 2017/01/17 20:04:36, pthatcher1 wrote:
> Should we have a TODO to move this into the api/ directory?

It should move, but I don't think a TODO helps; there are hundreds of similar
includes that don't have TODOs.

https://codereview.webrtc.org/2632613002/diff/80001/webrtc/api/ortcfactory.h#...
webrtc/api/ortcfactory.h:40: // |worker_thread| is used for CPU-intensive
operations.
On 2017/01/17 20:04:36, pthatcher1 wrote:
> This kind of misleading.  If the worker thread is blocked by CPU-intensive
> operations, lots of stuff won't work.  Maybe we should call this the
> media_thread and say that media operations happen on the media_thread or child
> threads.  

Technically that's just a limitation of our implementation, not the API, but I
should call it out in a comment.

https://codereview.webrtc.org/2632613002/diff/80001/webrtc/api/ortcfactory.h#...
webrtc/api/ortcfactory.h:46: // rtc::Thread, or by calling
rtc::Thread::Current()->ProcessMessages).
On 2017/01/17 20:04:36, pthatcher1 wrote:
> Is it also the case that all methods call on the API must be called on the
> signaling_thread?  If not, then would callback_thread be a better name?  If
so,
> would api_thread be a better name?

Yes, but a proxy object should handle that for the application. That's something
I was still missing; see most recent patchset.

https://codereview.webrtc.org/2632613002/diff/80001/webrtc/api/ortcfactory.h#...
webrtc/api/ortcfactory.h:57: // passed in.
On 2017/01/17 20:04:36, pthatcher1 wrote:
> You might want to repeat that none of these can be destroyed before the
factory
> is destroyed.

Done

[pthatcher1, 2017-01-18 02:42:46]

lgtm, other than the incorrect comment.  If anything just leave off the comment
for now and we can figure what to add later.

https://codereview.webrtc.org/2632613002/diff/120001/webrtc/api/ortcfactoryin...
File webrtc/api/ortcfactoryinterface.h (right):

https://codereview.webrtc.org/2632613002/diff/120001/webrtc/api/ortcfactoryin...
webrtc/api/ortcfactoryinterface.h:41: // If null, a new rtc::Thread is created.
So the problem here is that the worker_thread is *not* for CPU-intensive
operations.  It's a complete misnomer.  It's more like "the thread that media
operations use to configure stuff".  We can call it "worker_thread" for
consistency, but saying it's for CPU-intensive operations is not correct.

[Taylor Brandstetter, 2017-01-18 22:19:04]

https://codereview.webrtc.org/2632613002/diff/120001/webrtc/api/ortcfactoryin...
File webrtc/api/ortcfactoryinterface.h (right):

https://codereview.webrtc.org/2632613002/diff/120001/webrtc/api/ortcfactoryin...
webrtc/api/ortcfactoryinterface.h:41: // If null, a new rtc::Thread is created.
On 2017/01/18 02:42:46, pthatcher1 wrote:
> So the problem here is that the worker_thread is *not* for CPU-intensive
> operations.  It's a complete misnomer.  It's more like "the thread that media
> operations use to configure stuff".  We can call it "worker_thread" for
> consistency, but saying it's for CPU-intensive operations is not correct.  

If the worker thread is just an artifact, and we want to remove it (or replace
it with a TaskQueue one day), I'll leave it out of the API for now.

[Taylor Brandstetter, 2017-01-18 22:21:34]

The CQ bit was checked by deadbeef@webrtc.org

[Taylor Brandstetter, 2017-01-18 22:21:34]

The patchset sent to the CQ was uploaded after l-g-t-m from pthatcher@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2632613002/#ps140001 (title:
"Rebase and remove worker_thread from API")

[commit-bot: I haz the power, 2017-01-18 22:21:40]

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

[Taylor Brandstetter, 2017-01-18 22:21:55]

The CQ bit was unchecked by deadbeef@webrtc.org

[Taylor Brandstetter, 2017-01-18 22:35:06]

Description was changed from

==========
Adding OrtcFactory, and changing UdpTransport to match current plan.

The factory follows the same principles as PeerConnectionFactory;
various modules can be passed into its constructor but default
implementations are provided. Currently the only object the factory can
create is a UdpTransport (need to start somewhere).

UdpTransportChannel (renamed to UdpTransport)
will now accept a socket passed into its constructor,
relying on the factory to create the socket. This allows some
simplifications to be made, such as getting rid of "State" since the
only states are now "has destination set or doesn't".

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

to

==========
Adding OrtcFactory, and changing UdpTransport to match current plan.

The factory follows the same principles as PeerConnectionFactory;
various modules can be passed into its constructor but default
implementations are provided. Currently the only object the factory can
create is a UdpTransport (need to start somewhere).

UdpTransportChannel (renamed to UdpTransport)
will now accept a socket passed into its constructor,
relying on the factory to create the socket. This allows some
simplifications to be made, such as getting rid of "State" since the
only states are now "has destination set or doesn't".

BUG=webrtc:7013
==========

[Taylor Brandstetter, 2017-01-18 22:35:14]

The CQ bit was checked by deadbeef@webrtc.org

[commit-bot: I haz the power, 2017-01-18 22:35:25]

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-01-18 23:16:38]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1484778914803220,
"parent_rev": "27edfbcec4134f5c656735120ff04b6591bbade8", "commit_rev":
"d1c0998730443e4901ec329ee816fd1f115b6223"}

[commit-bot: I haz the power, 2017-01-18 23:16:40]

Description was changed from

==========
Adding OrtcFactory, and changing UdpTransport to match current plan.

The factory follows the same principles as PeerConnectionFactory;
various modules can be passed into its constructor but default
implementations are provided. Currently the only object the factory can
create is a UdpTransport (need to start somewhere).

UdpTransportChannel (renamed to UdpTransport)
will now accept a socket passed into its constructor,
relying on the factory to create the socket. This allows some
simplifications to be made, such as getting rid of "State" since the
only states are now "has destination set or doesn't".

BUG=webrtc:7013
==========

to

==========
Adding OrtcFactory, and changing UdpTransport to match current plan.

The factory follows the same principles as PeerConnectionFactory;
various modules can be passed into its constructor but default
implementations are provided. Currently the only object the factory can
create is a UdpTransport (need to start somewhere).

UdpTransportChannel (renamed to UdpTransport)
will now accept a socket passed into its constructor,
relying on the factory to create the socket. This allows some
simplifications to be made, such as getting rid of "State" since the
only states are now "has destination set or doesn't".

BUG=webrtc:7013

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

[commit-bot: I haz the power, 2017-01-18 23:16:41]

Committed patchset #8 (id:140001) as
https://chromium.googlesource.com/external/webrtc/+/d1c0998730443e4901ec329ee...