Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
398813003:
Cast Port Allocator Factory for PeerConnection (Closed)
|
Created:
6 years, 5 months ago by aiguha Modified:
6 years, 5 months ago Reviewers:
Sergey Ulanov CC:
chromium-reviews, chromoting-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionCast Port Allocator Factory for PeerConnection
This CL contains the implementation of PortAllocatorFactoryInterface in peerconnectioninterface.h.
It bridges the gap in port allocation objects between WebRTC and Chromium. The factory produces a
ChromiumPortAllocator that can successfully be used by the PeerConnection API.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=284589
Patch Set 1 #Patch Set 2 : Fixed gyp file #
Total comments: 17
Patch Set 3 : fixes based on review #Patch Set 4 : Minor fixes #
Total comments: 9
Patch Set 5 : Updates after Review #
Messages
Total messages: 17 (0 generated)
This CL contains the PortAllocator related code required by peer connection. Again, this is part of the larger CL I linked earlier. I apologize for barraging you with CLs, if there is another reviewer who would be appropriate, please tell me. Thanks! https://codereview.chromium.org/398813003/diff/20001/remoting/host/cast_port_... File remoting/host/cast_port_allocator_factory.cc (right): https://codereview.chromium.org/398813003/diff/20001/remoting/host/cast_port_... remoting/host/cast_port_allocator_factory.cc:48: Until we properly set relay hosts, we won't be able to establish PeerConnections in cases where relay servers are needed. https://codereview.chromium.org/398813003/diff/20001/remoting/jingle_glue/chr... File remoting/jingle_glue/chromium_socket_factory.cc (right): https://codereview.chromium.org/398813003/diff/20001/remoting/jingle_glue/chr... remoting/jingle_glue/chromium_socket_factory.cc:374: // NOTREACHED(); There seems to be no way to restrict WebRTC from trying to use TCP. Thus, when performing its ICE establishment, it still tries to call this method and hits the NOTREACHED. TODO(aiguha): Find a better way to deal with this, as this is likely not acceptable.
https://codereview.chromium.org/398813003/diff/20001/remoting/host/cast_port_... File remoting/host/cast_port_allocator_factory.cc (right): https://codereview.chromium.org/398813003/diff/20001/remoting/host/cast_port_... remoting/host/cast_port_allocator_factory.cc:20: } } can be moved on the previous line. https://codereview.chromium.org/398813003/diff/20001/remoting/host/cast_port_... File remoting/host/cast_port_allocator_factory.h (right): https://codereview.chromium.org/398813003/diff/20001/remoting/host/cast_port_... remoting/host/cast_port_allocator_factory.h:9: #include "net/url_request/url_request_context_getter.h" Move this to .cc and forward-declare net::URLRequestContextGetter https://codereview.chromium.org/398813003/diff/20001/remoting/host/cast_port_... remoting/host/cast_port_allocator_factory.h:10: #include "remoting/jingle_glue/chromium_port_allocator.h" Move this to .cc https://codereview.chromium.org/398813003/diff/20001/remoting/host/cast_port_... remoting/host/cast_port_allocator_factory.h:14: class PortAllocator; I don't think you need this - peerconnectioninterface.h should already declare it. https://codereview.chromium.org/398813003/diff/20001/remoting/host/cast_port_... remoting/host/cast_port_allocator_factory.h:21: class CastPortAllocatorFactory : public webrtc::PortAllocatorFactoryInterface { Maybe call it ChromiumPortAllocatorFactory? it's not specific to cast, it just allocates ChromiumPortAllocator instances. (on the side note: I want to cry when I read AllocatorFactoryInterface, it's factory factory interface essentially. bang head on the desk :-( ) https://codereview.chromium.org/398813003/diff/20001/remoting/jingle_glue/chr... File remoting/jingle_glue/chromium_socket_factory.cc (right): https://codereview.chromium.org/398813003/diff/20001/remoting/jingle_glue/chr... remoting/jingle_glue/chromium_socket_factory.cc:5: #include "remoting/jingle_glue/chromium_socket_factory.h" FYI I've moved this file to remoting/protocol - you will need to merge that change. https://codereview.chromium.org/398813003/diff/20001/remoting/jingle_glue/chr... remoting/jingle_glue/chromium_socket_factory.cc:374: // NOTREACHED(); On 2014/07/17 03:11:36, aiguha1 wrote: > There seems to be no way to restrict WebRTC from trying to use TCP. Thus, when > performing its ICE establishment, it still tries to call this method and hits > the NOTREACHED. > TODO(aiguha): Find a better way to deal with this, as this is likely not > acceptable. I think what might be happening is that the peer (i.e. browser) always allocates TCP candidates and PeerConnection tries to use them even when TCP is disabled, i.e. flags disabled only allocation of TCP candidates (i.e. CreateClientTcpSocket() is never called), but incoming TCP candidates are still used. The reason we disable TCP for regular Chromoting connections is because PseudoTCP performs poorly over TCP. But that reason doesn't apply when casting the screen using WebRTC, so we actually may want to enable TCP at some point. I suggest you replace NOTREACHED() with NOTIMPLEMENTED().
Fixes based on comments https://codereview.chromium.org/398813003/diff/20001/remoting/host/cast_port_... File remoting/host/cast_port_allocator_factory.cc (right): https://codereview.chromium.org/398813003/diff/20001/remoting/host/cast_port_... remoting/host/cast_port_allocator_factory.cc:20: } On 2014/07/17 18:54:40, Sergey Ulanov wrote: > } can be moved on the previous line. Done. https://codereview.chromium.org/398813003/diff/20001/remoting/host/cast_port_... File remoting/host/cast_port_allocator_factory.h (right): https://codereview.chromium.org/398813003/diff/20001/remoting/host/cast_port_... remoting/host/cast_port_allocator_factory.h:9: #include "net/url_request/url_request_context_getter.h" On 2014/07/17 18:54:40, Sergey Ulanov wrote: > Move this to .cc and forward-declare net::URLRequestContextGetter Done. https://codereview.chromium.org/398813003/diff/20001/remoting/host/cast_port_... remoting/host/cast_port_allocator_factory.h:10: #include "remoting/jingle_glue/chromium_port_allocator.h" On 2014/07/17 18:54:40, Sergey Ulanov wrote: > Move this to .cc Done. https://codereview.chromium.org/398813003/diff/20001/remoting/host/cast_port_... remoting/host/cast_port_allocator_factory.h:14: class PortAllocator; On 2014/07/17 18:54:40, Sergey Ulanov wrote: > I don't think you need this - peerconnectioninterface.h should already declare > it. Done. https://codereview.chromium.org/398813003/diff/20001/remoting/host/cast_port_... remoting/host/cast_port_allocator_factory.h:21: class CastPortAllocatorFactory : public webrtc::PortAllocatorFactoryInterface { On 2014/07/17 18:54:40, Sergey Ulanov wrote: > Maybe call it ChromiumPortAllocatorFactory? it's not specific to cast, it just > allocates ChromiumPortAllocator instances. > > (on the side note: I want to cry when I read AllocatorFactoryInterface, it's > factory factory interface essentially. bang head on the desk :-( ) Done. And haha, you're absolutely right. I thought the same while typing that out. Someone should make a meme about that! https://codereview.chromium.org/398813003/diff/20001/remoting/jingle_glue/chr... File remoting/jingle_glue/chromium_socket_factory.cc (right): https://codereview.chromium.org/398813003/diff/20001/remoting/jingle_glue/chr... remoting/jingle_glue/chromium_socket_factory.cc:5: #include "remoting/jingle_glue/chromium_socket_factory.h" On 2014/07/17 18:54:40, Sergey Ulanov wrote: > FYI I've moved this file to remoting/protocol - you will need to merge that > change. Thanks. When would be the right time to do that? https://codereview.chromium.org/398813003/diff/20001/remoting/jingle_glue/chr... remoting/jingle_glue/chromium_socket_factory.cc:374: // NOTREACHED(); On 2014/07/17 18:54:40, Sergey Ulanov wrote: > On 2014/07/17 03:11:36, aiguha1 wrote: > > There seems to be no way to restrict WebRTC from trying to use TCP. Thus, when > > performing its ICE establishment, it still tries to call this method and hits > > the NOTREACHED. > > TODO(aiguha): Find a better way to deal with this, as this is likely not > > acceptable. > > I think what might be happening is that the peer (i.e. browser) always allocates > TCP candidates and PeerConnection tries to use them even when TCP is disabled, > i.e. flags disabled only allocation of TCP candidates (i.e. > CreateClientTcpSocket() is never called), but incoming TCP candidates are still > used. > > The reason we disable TCP for regular Chromoting connections is because > PseudoTCP performs poorly over TCP. But that reason doesn't apply when casting > the screen using WebRTC, so we actually may want to enable TCP at some point. > > I suggest you replace NOTREACHED() with NOTIMPLEMENTED(). You're right, I think that's exactly what's happening. Understood. I'll go ahead and replace it with NOTIMPLEMENTED() for now then. https://codereview.chromium.org/398813003/diff/20001/remoting/jingle_glue/chr... remoting/jingle_glue/chromium_socket_factory.cc:374: // NOTREACHED(); On 2014/07/17 18:54:40, Sergey Ulanov wrote: > On 2014/07/17 03:11:36, aiguha1 wrote: > > There seems to be no way to restrict WebRTC from trying to use TCP. Thus, when > > performing its ICE establishment, it still tries to call this method and hits > > the NOTREACHED. > > TODO(aiguha): Find a better way to deal with this, as this is likely not > > acceptable. > > I think what might be happening is that the peer (i.e. browser) always allocates > TCP candidates and PeerConnection tries to use them even when TCP is disabled, > i.e. flags disabled only allocation of TCP candidates (i.e. > CreateClientTcpSocket() is never called), but incoming TCP candidates are still > used. > > The reason we disable TCP for regular Chromoting connections is because > PseudoTCP performs poorly over TCP. But that reason doesn't apply when casting > the screen using WebRTC, so we actually may want to enable TCP at some point. > > I suggest you replace NOTREACHED() with NOTIMPLEMENTED(). Done.
https://codereview.chromium.org/398813003/diff/60001/remoting/jingle_glue/chr... File remoting/jingle_glue/chromium_socket_factory.cc (right): https://codereview.chromium.org/398813003/diff/60001/remoting/jingle_glue/chr... remoting/jingle_glue/chromium_socket_factory.cc:5: #include "remoting/jingle_glue/chromium_socket_factory.h" Since comments don't carry over between patchsets: When would be the best time to merge your change that moved this to remoting/protocol?
https://codereview.chromium.org/398813003/diff/60001/remoting/jingle_glue/chr... File remoting/jingle_glue/chromium_socket_factory.cc (right): https://codereview.chromium.org/398813003/diff/60001/remoting/jingle_glue/chr... remoting/jingle_glue/chromium_socket_factory.cc:5: #include "remoting/jingle_glue/chromium_socket_factory.h" On 2014/07/18 21:36:59, aiguha1 wrote: > Since comments don't carry over between patchsets: When would be the best time > to merge your change that moved this to remoting/protocol? It's already landed so you can merge it now.
LGTM when my comments are addressed https://codereview.chromium.org/398813003/diff/60001/remoting/host/chromium_p... File remoting/host/chromium_port_allocator_factory.h (right): https://codereview.chromium.org/398813003/diff/60001/remoting/host/chromium_p... remoting/host/chromium_port_allocator_factory.h:27: // Thought unspecified, the caller takes control of the returned s/Thought/Though/? (or just remove "Thought unspecified,"). Also s/control/ownership/ Or it's probably better to remove this comment altogether - ownership semantics for a create method in a factory should be obvious without it. https://codereview.chromium.org/398813003/diff/60001/remoting/host/chromium_p... remoting/host/chromium_port_allocator_factory.h:34: ChromiumPortAllocatorFactory( This constructor should be private - it's called only by ChromiumPortAllocatorFactory::Create(). https://codereview.chromium.org/398813003/diff/60001/remoting/host/chromium_p... remoting/host/chromium_port_allocator_factory.h:42: }; add DISALLOW_COPY_AND_ASSIGN(ChromiumPortAllocatorFactory).
https://codereview.chromium.org/398813003/diff/60001/remoting/host/chromium_p... File remoting/host/chromium_port_allocator_factory.h (right): https://codereview.chromium.org/398813003/diff/60001/remoting/host/chromium_p... remoting/host/chromium_port_allocator_factory.h:27: // Thought unspecified, the caller takes control of the returned On 2014/07/21 17:43:13, Sergey Ulanov wrote: > s/Thought/Though/? (or just remove "Thought unspecified,"). Also > s/control/ownership/ > > Or it's probably better to remove this comment altogether - ownership semantics > for a create method in a factory should be obvious without it. Agreed. I've removed the comment. https://codereview.chromium.org/398813003/diff/60001/remoting/host/chromium_p... remoting/host/chromium_port_allocator_factory.h:34: ChromiumPortAllocatorFactory( On 2014/07/21 17:43:13, Sergey Ulanov wrote: > This constructor should be private - it's called only by > ChromiumPortAllocatorFactory::Create(). Actually, in Create(), a talk_base::RefCountedObject<CPAF> is being made, which requires access to the base class' constructor and destructor, so these methods can be protected at most. Making them private throws a compiler error. https://codereview.chromium.org/398813003/diff/60001/remoting/host/chromium_p... remoting/host/chromium_port_allocator_factory.h:42: }; On 2014/07/21 17:43:13, Sergey Ulanov wrote: > add DISALLOW_COPY_AND_ASSIGN(ChromiumPortAllocatorFactory). Done.
lgtm https://codereview.chromium.org/398813003/diff/60001/remoting/host/chromium_p... File remoting/host/chromium_port_allocator_factory.h (right): https://codereview.chromium.org/398813003/diff/60001/remoting/host/chromium_p... remoting/host/chromium_port_allocator_factory.h:34: ChromiumPortAllocatorFactory( On 2014/07/21 18:50:39, aiguha1 wrote: > On 2014/07/21 17:43:13, Sergey Ulanov wrote: > > This constructor should be private - it's called only by > > ChromiumPortAllocatorFactory::Create(). > > Actually, in Create(), a talk_base::RefCountedObject<CPAF> is being made, which > requires access to the base class' constructor and destructor, so these methods > can be protected at most. Making them private throws a compiler error. Acknowledged.
The CQ bit was checked by aiguha@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/aiguha@chromium.org/398813003/80001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: android_dbg_triggered_tests on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_dbg_triggered_tests on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)
The CQ bit was checked by aiguha@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/aiguha@chromium.org/398813003/80001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: android_dbg_triggered_tests on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)
Message was sent while issue was closed.
Change committed as 284589 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
