Issue 2675173003: Adding "adapter" ORTC objects on top of ChannelManager/BaseChannel/etc.

Taylor Brandstetter

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

3 years, 10 months ago (2017-02-06 07:51:56 UTC) #1

Taylor Brandstetter

I got this to the point of a simple end-to-end audio test working, so I ...

3 years, 10 months ago (2017-02-06 07:51:57 UTC) #2

the sun

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

3 years, 10 months ago (2017-02-07 08:26:26 UTC) #3

the sun

https://codereview.webrtc.org/2675173003/diff/1/webrtc/api/ortcfactoryinterface.h File webrtc/api/ortcfactoryinterface.h (right): https://codereview.webrtc.org/2675173003/diff/1/webrtc/api/ortcfactoryinterface.h#newcode11 webrtc/api/ortcfactoryinterface.h:11: #ifndef WEBRTC_API_ORTCFACTORYINTERFACE_H_ Would it make sense to put this ...

3 years, 10 months ago (2017-02-07 08:26:27 UTC) #4

Taylor Brandstetter

https://codereview.webrtc.org/2675173003/diff/1/webrtc/api/ortcfactoryinterface.h File webrtc/api/ortcfactoryinterface.h (right): https://codereview.webrtc.org/2675173003/diff/1/webrtc/api/ortcfactoryinterface.h#newcode11 webrtc/api/ortcfactoryinterface.h:11: #ifndef WEBRTC_API_ORTCFACTORYINTERFACE_H_ On 2017/02/07 08:26:27, the sun wrote: > ...

3 years, 10 months ago (2017-02-07 16:30:00 UTC) #5

pthatcher1

Partly through it. Will do more tomorrow. https://codereview.webrtc.org/2675173003/diff/1/webrtc/api/mediatypes.cc File webrtc/api/mediatypes.cc (right): https://codereview.webrtc.org/2675173003/diff/1/webrtc/api/mediatypes.cc#newcode23 webrtc/api/mediatypes.cc:23: return "data"; ...

3 years, 10 months ago (2017-02-08 01:33:50 UTC) #6

pthatcher1

Oh... that was surprisingly small, actually. If you pull out the "rename interface to internal" ...

3 years, 10 months ago (2017-02-09 00:11:44 UTC) #7

Taylor Brandstetter

Addressed all comments so far. Just desperately in need of tests. I'm thinking of writing ...

3 years, 10 months ago (2017-02-10 00:19:46 UTC) #8

Addressed all comments so far. Just desperately in need of tests.

I'm thinking of writing tests only at the "public interface" level (meaning, the
tests for RtpSenderShim will only use methods from RtpSenderInterface, and so
on). That way, the tests won't have to be rewritten once the shim implementation
is replaced with a better one.

I feel like testing at a lower layer than that would be more effort than it's
worth, since the code will just be deleted soon enough.

https://codereview.webrtc.org/2675173003/diff/1/webrtc/api/mediatypes.cc
File webrtc/api/mediatypes.cc (right):

https://codereview.webrtc.org/2675173003/diff/1/webrtc/api/mediatypes.cc#newc...
webrtc/api/mediatypes.cc:23: return "data";
On 2017/02/08 01:33:48, pthatcher1 wrote:
> It doesn't matter too much, but it seems like making these string into
constants
> would make sense.  

Done.

https://codereview.webrtc.org/2675173003/diff/1/webrtc/api/rtcerror.h
File webrtc/api/rtcerror.h (right):

https://codereview.webrtc.org/2675173003/diff/1/webrtc/api/rtcerror.h#newcode156
webrtc/api/rtcerror.h:156: friend class RTCErrorOr;
On 2017/02/08 01:33:49, pthatcher1 wrote:
> This could use a comment about how it's used below in the copy constructor and
> other places.

Done.

https://codereview.webrtc.org/2675173003/diff/1/webrtc/api/rtcerror.h#newcode162
webrtc/api/rtcerror.h:162: // 'explicit' to try to catch cases like 'return
{};', where people think
On 2017/02/08 01:33:49, pthatcher1 wrote:
> Did I miss the "explicit" on here?  I don't see it.  Or is "default" the new
> "explicit"?

Oh, I think I removed it because the presubmit complained, but in this case it
was actually intentional. I'll add it back.

https://codereview.webrtc.org/2675173003/diff/1/webrtc/api/rtcerror.h#newcode238
webrtc/api/rtcerror.h:238: rtc::Optional<T> value_;
On 2017/02/08 01:33:49, pthatcher1 wrote:
> rtc::Optional<T> is basically a T with a bool to track if it's set.  But you
> already have that bool, via the error_ being None or not.  So why do you need
> the rtc::Optional<T> instead of just a T?

Good point, I forgot that's how rtc::Optional was implemented. For some reason I
thought it allocated the inner object on the heap (but in retrospect, that would
make it identical to std::unique_ptr, so that was a silly assumption).

https://codereview.webrtc.org/2675173003/diff/1/webrtc/api/rtpparameters.h
File webrtc/api/rtpparameters.h (right):

https://codereview.webrtc.org/2675173003/diff/1/webrtc/api/rtpparameters.h#ne...
webrtc/api/rtpparameters.h:255: // internally without any event.
On 2017/02/08 01:33:49, pthatcher1 wrote:
> I don't see why we couldn't return it in GetParameters still.  Because someone
> might call SetParameters with it?  

Yeah. You wouldn't want to do "Set" with an optional SSRC (that conveniently
handles SSRC conflicts automatically), then do a "Get/Set" and have it turn into
a hard-coded SSRC. If I understand correctly, this is how the ORTC API is
designed.

https://codereview.webrtc.org/2675173003/diff/1/webrtc/api/rtptransportcontro...
File webrtc/api/rtptransportcontrollerinterface.h (right):

https://codereview.webrtc.org/2675173003/diff/1/webrtc/api/rtptransportcontro...
webrtc/api/rtptransportcontrollerinterface.h:23: // and other things that exist
on a "per-call" basis.
On 2017/02/08 01:33:49, pthatcher1 wrote:
> I think a better way to phrase this (instead of "per-call") would be that many
> RtpTransports that are to the same remote endpoint should share an
> RtpTransportController.  It may make sense to highlight that when bundling,
> there will be only one RtpTransport per RtpTransportController, and there are
> only many when not bundling.

Done.

https://codereview.webrtc.org/2675173003/diff/1/webrtc/api/rtptransportcontro...
webrtc/api/rtptransportcontrollerinterface.h:29: // This is the RTP equivalent
of "IceTransportController".
On 2017/02/08 01:33:49, pthatcher1 wrote:
> I'm not sure what value this comment brings to someone that will be reading
it.

If you're familiar with the ORTC object heirarchy, you may be confused about
what an RtpTransportController represents. But it makes sense when you realize
"RtpTransportController is to RtpTransport as IceTransportController is to
IceTransport". I'll make this more clear.

https://codereview.webrtc.org/2675173003/diff/1/webrtc/api/rtptransportcontro...
webrtc/api/rtptransportcontrollerinterface.h:41: virtual
RtpTransportControllerShim* GetInternal() = 0;
On 2017/02/08 01:33:49, pthatcher1 wrote:
> I'm not a fan of "XShim".  Why not "XInternal"?
> 

There needs to be some name that differentiates these shim objects from the real
ones, assuming there's some point in time during our refactoring at which both
will exist. I'm open to suggestions, but "XInternal" seems like it would be
confusing.

https://codereview.webrtc.org/2675173003/diff/1/webrtc/api/rtptransportinterf...
File webrtc/api/rtptransportinterface.h (right):

https://codereview.webrtc.org/2675173003/diff/1/webrtc/api/rtptransportinterf...
webrtc/api/rtptransportinterface.h:60: virtual RtpTransportShim* GetInternal() =
0;
On 2017/02/08 01:33:49, pthatcher1 wrote:
> Again, I'd like to understand this model better.

Ok, so suppose the user of the API passes an "RtpTransportInterface" into
CreateRtpSender. There needs to be some way to downcast that
RtpTransportInterface pointer (which is a pointer to a proxy object) to the
internal interface. This approach - a protected "GetInternal" method - seemed
cleaner than having a mess of static casting and crossing fingers.

But I'm open to any other ideas. We haven't really had to deal with this problem
before, so it would be good to establish a pattern we like (even though it could
be easily changed later since it's not public).

P.S. When you say "Again," in a code review, I'm not sure what you're referring
to since comments end up in alphabetical order by files.

https://codereview.webrtc.org/2675173003/diff/1/webrtc/base/messagehandler.h
File webrtc/base/messagehandler.h (right):

https://codereview.webrtc.org/2675173003/diff/1/webrtc/base/messagehandler.h#...
webrtc/base/messagehandler.h:47: ReturnT moved_result() { return
std::move(result_); }
On 2017/02/08 01:33:49, pthatcher1 wrote:
> Would MoveResult be a better name, since it has an action that takes place?  
> 
> Also, what happens if you call it twice?  I assume that's not safe, so we
should
> probably document that.

ConsumeResult could work, I've seen that naming elsewhere for something
equivalent.

https://codereview.webrtc.org/2675173003/diff/1/webrtc/base/thread.h
File webrtc/base/thread.h (right):

https://codereview.webrtc.org/2675173003/diff/1/webrtc/base/thread.h#newcode171
webrtc/base/thread.h:171: return handler.moved_result();
On 2017/02/08 01:33:49, pthatcher1 wrote:
> This seems like a big enough change to warrant its own CL.

Done.

https://codereview.webrtc.org/2675173003/diff/1/webrtc/media/engine/webrtcmed...
File webrtc/media/engine/webrtcmediaengine.h (right):

https://codereview.webrtc.org/2675173003/diff/1/webrtc/media/engine/webrtcmed...
webrtc/media/engine/webrtcmediaengine.h:51: static
std::unique_ptr<MediaEngineInterface> Create(
On 2017/02/08 01:33:49, pthatcher1 wrote:
> This allow seems like a big enough change to warrant its own CL.

Actually, I'm putting this specific change off, since these methods are used by
chromium and other downstream applications.

https://codereview.webrtc.org/2675173003/diff/1/webrtc/media/sctp/sctptranspo...
File webrtc/media/sctp/sctptransport.h (right):

https://codereview.webrtc.org/2675173003/diff/1/webrtc/media/sctp/sctptranspo...
webrtc/media/sctp/sctptransport.h:70: rtc::PacketTransportInternal* channel);
On 2017/02/08 01:33:49, pthatcher1 wrote:
> Why does an SctpTransport need the internal version?

Because it uses it to send and receive packets, and receive events. Or, do you
mean, why does it not take a PacketTransportInterface and call GetInternal()?
Because that's not necessary anywhere yet.

https://codereview.webrtc.org/2675173003/diff/1/webrtc/p2p/base/dtlstransport...
File webrtc/p2p/base/dtlstransportchannel.h (right):

https://codereview.webrtc.org/2675173003/diff/1/webrtc/p2p/base/dtlstransport...
webrtc/p2p/base/dtlstransportchannel.h:26: namespace rtc {
On 2017/02/08 01:33:49, pthatcher1 wrote:
> Why does the DtlsTransport need the internal interface?

Because that's the interface that the signals use.

https://codereview.webrtc.org/2675173003/diff/1/webrtc/pc/channelmanager_unit...
File webrtc/pc/channelmanager_unittest.cc (right):

https://codereview.webrtc.org/2675173003/diff/1/webrtc/pc/channelmanager_unit...
webrtc/pc/channelmanager_unittest.cc:147: rtc::Thread::Current(),
cricket::CN_DATA, kDefaultSrtpRequired);
On 2017/02/08 01:33:49, pthatcher1 wrote:
> It feels like it would make sense to do this in a separate CL.

Done.

https://codereview.webrtc.org/2675173003/diff/1/webrtc/pc/mediasession.h
File webrtc/pc/mediasession.h (right):

https://codereview.webrtc.org/2675173003/diff/1/webrtc/pc/mediasession.h#newc...
webrtc/pc/mediasession.h:96: RtpTransceiverDirection MakeReversed() const {
On 2017/02/08 01:33:49, pthatcher1 wrote:
> Reverse()?

To me, that implies that the object on which the method is called is being
reversed, rather than a new object being returned.

https://codereview.webrtc.org/2675173003/diff/1/webrtc/pc/ortcfactory.h
File webrtc/pc/ortcfactory.h (right):

https://codereview.webrtc.org/2675173003/diff/1/webrtc/pc/ortcfactory.h#newco...
webrtc/pc/ortcfactory.h:38: RtpTransportControllerInterface*
transport_controller) override;
On 2017/02/08 01:33:49, pthatcher1 wrote:
> I think the transport_controller should be the first arg and the
rtcp_parameters
> the last arg.

const parameters should go first according to the style guide. And especially if
transport_controller becomes an optional parameter, it makes sense to go on the
end.

https://codereview.webrtc.org/2675173003/diff/1/webrtc/pc/ortcfactory.h#newco...
webrtc/pc/ortcfactory.h:38: RtpTransportControllerInterface*
transport_controller) override;
On 2017/02/08 01:33:49, pthatcher1 wrote:
> I think we should consider making the common case the easy one:
> CreateRtpTransport without an RtpTransportController passed in creates on and
> then it's owned by the RtpTransport.  Basically, when bundled.  Then you only
> have to create an RtpTransportController if you don't want it bundled.

Good idea. Done.

https://codereview.webrtc.org/2675173003/diff/1/webrtc/pc/ortcfactory.h#newco...
webrtc/pc/ortcfactory.h:44: rtc::scoped_refptr<MediaStreamTrackInterface> track,
On 2017/02/08 01:33:49, pthatcher1 wrote:
> Why do you need to pass in a track?  

Because this is a combination of "create and send". Though, after thinking about
this more, I'm going to change this. I didn't like how in ORTC, you needed to
construct a transport object and then call "start". But constructing a
sender/receiver and then calling "send/receive" makes more sense.

https://codereview.webrtc.org/2675173003/diff/1/webrtc/pc/ortcfactory.h#newco...
webrtc/pc/ortcfactory.h:69: const MediaConstraintsInterface* constraints)
override;
On 2017/02/08 01:33:49, pthatcher1 wrote:
> Why are CreateAudioSource and CreateVideoSource part of the ORTC?

Because these objects may need to use the same threads and/or other resources as
the other ORTC objects to function properly. The same pattern is used with
PeerConnectionFactory. Maybe we could move this to a separate factory object,
but I didn't want to try to design that right now.

https://codereview.webrtc.org/2675173003/diff/20001/webrtc/pc/rtpreceiver.h
File webrtc/pc/rtpreceiver.h (right):

https://codereview.webrtc.org/2675173003/diff/20001/webrtc/pc/rtpreceiver.h#n...
webrtc/pc/rtpreceiver.h:37: virtual uint32_t ssrc() const = 0;
On 2017/02/09 00:11:44, pthatcher1 wrote:
> Can you leave a comment about what this is and why we need it?  I assume it's
> used as an RtpReceiver ID of sorts, but that's not clear.

Done.

https://codereview.webrtc.org/2675173003/diff/20001/webrtc/pc/rtpreceiver.h#n...
webrtc/pc/rtpreceiver.h:46: // sees.
On 2017/02/09 00:11:44, pthatcher1 wrote:
> Instead of making a comment like this, could we do this?
> 
> 1.  This constructor DCHECKs a non-zero SSRC.
> 2.  A different constructor doesn't take an SSRC and is clearly marked as the
> "match the first SSRC it sees" type.
> 
> Also, what happens if you create more than one such receiver?  Is it random
> which one "sees" the SSRC first?

We could do that, I was just trying to change as few things as possible in this
CL; this comment is just documenting existing behavior. I'll leave a TODO.

pthatcher1

A few more things. I haven't check the unit test yet, but I agree with ...

3 years, 10 months ago (2017-02-10 22:36:54 UTC) #9

pthatcher1

Looked at the tests. I like them. https://codereview.webrtc.org/2675173003/diff/170001/webrtc/ortc/ortcfactory_integrationtest.cc File webrtc/ortc/ortcfactory_integrationtest.cc (right): https://codereview.webrtc.org/2675173003/diff/170001/webrtc/ortc/ortcfactory_integrationtest.cc#newcode26 webrtc/ortc/ortcfactory_integrationtest.cc:26: const int ...

3 years, 10 months ago (2017-02-10 22:41:13 UTC) #10

Taylor Brandstetter

Description was changed from ========== Adding "shim" ORTC objects on top of ChannelManager/BaseChannel/etc. Will fill ...

3 years, 10 months ago (2017-02-14 05:26:53 UTC) #11

Taylor Brandstetter

Still working on tests, but wanted to respond to comments. https://codereview.webrtc.org/2675173003/diff/1/webrtc/pc/mediasession.h File webrtc/pc/mediasession.h (right): https://codereview.webrtc.org/2675173003/diff/1/webrtc/pc/mediasession.h#newcode96 ...

3 years, 10 months ago (2017-02-14 06:55:06 UTC) #12

Taylor Brandstetter

deadbeef@webrtc.org changed reviewers: - solenberg@webrtc.org

3 years, 10 months ago (2017-02-14 07:05:47 UTC) #13

Taylor Brandstetter

By the way, may want to hold off on reviewing more until I've finished the ...

3 years, 10 months ago (2017-02-14 07:05:48 UTC) #14

pthatcher1

I need to go through the new unit tests. The code looks good to me ...

3 years, 10 months ago (2017-02-17 23:10:22 UTC) #15

I need to go through the new unit tests.  The code looks good to me except for
the AddTransport/RemoteTransport and the Attach/Detach parts.  I think we if
clean that up a little, we could land this even without complete unit tests so
"people" can start using it, even if in prototype form.

https://codereview.webrtc.org/2675173003/diff/170001/webrtc/ortc/ortcfactory.cc
File webrtc/ortc/ortcfactory.cc (right):

https://codereview.webrtc.org/2675173003/diff/170001/webrtc/ortc/ortcfactory....
webrtc/ortc/ortcfactory.cc:39: webrtc::RtpCodecCapability* codec) {
On 2017/02/14 06:55:05, Taylor Brandstetter wrote:
> On 2017/02/10 22:36:52, pthatcher1 wrote:
> > Would a better name be AddCodecCapabilityFeedbackParam?
> 
> As long as we're renaming it, AddRtcpFeedbackFromFeedbackParam is better

I like it

https://codereview.webrtc.org/2675173003/diff/170001/webrtc/ortc/ortcfactory....
webrtc/ortc/ortcfactory.cc:48: RTC_NOTREACHED();
On 2017/02/14 06:55:05, Taylor Brandstetter wrote:
> On 2017/02/10 22:36:52, pthatcher1 wrote:
> > Wouldn't an RTC_DCHECK be more reasonable? 
> > 
> > Here and below.
> 
> How would that work?

I went through and read other uses of RTC_NOTREACHED.  It is consistent with
this.  I think it's a poor name, but at least it's consistent, so I forget I
mentioned anything.

https://codereview.webrtc.org/2675173003/diff/480001/webrtc/ortc/rtpparameter...
File webrtc/ortc/rtpparametersconversion.cc (right):

https://codereview.webrtc.org/2675173003/diff/480001/webrtc/ortc/rtpparameter...
webrtc/ortc/rtpparametersconversion.cc:189: if (extension.id < 1 || extension.id
> 14) {
These could use some name constants (kMinRtpHeaderExtensionId and
kMaxrtpHeaderExtensionId)

https://codereview.webrtc.org/2675173003/diff/480001/webrtc/ortc/rtptransport...
File webrtc/ortc/rtptransportcontrolleradapter.cc (right):

https://codereview.webrtc.org/2675173003/diff/480001/webrtc/ortc/rtptransport...
webrtc/ortc/rtptransportcontrolleradapter.cc:73: return transport_proxies_;
What is this even used for?  It seems like it's not really needed.  In fact, I
don't understand why you even need transport_proxies_.

https://codereview.webrtc.org/2675173003/diff/480001/webrtc/ortc/rtptransport...
File webrtc/ortc/rtptransportcontrolleradapter.h (right):

https://codereview.webrtc.org/2675173003/diff/480001/webrtc/ortc/rtptransport...
webrtc/ortc/rtptransportcontrolleradapter.h:83: void
AddTransport(RtpTransportInterface* transport_proxy);
Does anything call AddTransport other than the Transport itself?  In that case,
why not require an RtpTransportAdapter here?  Then at least it's symmetric with
RemoveTransport.

https://codereview.webrtc.org/2675173003/diff/480001/webrtc/ortc/rtptransport...
webrtc/ortc/rtptransportcontrolleradapter.h:105: void DetachVideoReceiver();
I still find it awkward to have a method AttachX(Y y) where X and Y are
completely different.

It seems what you are really doing is saying "Create a VideoChannel with this
transport if there isn't already one" and "Destroy a VideoChannel if I'm the
last one using it".  

Would it be better to do this?  

VoiceChannel* GetOrCreateVoiceChannel(RtpTransportInterface* transport);
VideoChannel* GetOrCreateVideoChannel(RtpTransportInterface* transport);
void DestroyVoiceChannel(VoiceChannel*);
void DestroyVideoChannel(VideoChannel*);

Then it would be almost exactly like TransportController::CreateDtlsTransport
and DestroyDtlsTransport.  Just refcount the number of times Create/Destroy is
called and create/destroy the channels on demand. 

Then you don't need voice_channel() and video_channel() either.

Taylor Brandstetter

I should be finished with tests today or this weekend, so I don't think landing ...

3 years, 10 months ago (2017-02-17 23:48:03 UTC) #16

I should be finished with tests today or this weekend, so I don't think landing
it before tests are done will be necessary. How many of the existing tests have
you reviewed so far? I've been adding them throughout the week

https://codereview.webrtc.org/2675173003/diff/480001/webrtc/ortc/rtpparameter...
File webrtc/ortc/rtpparametersconversion.cc (right):

https://codereview.webrtc.org/2675173003/diff/480001/webrtc/ortc/rtpparameter...
webrtc/ortc/rtpparametersconversion.cc:189: if (extension.id < 1 || extension.id
> 14) {
On 2017/02/17 23:10:22, pthatcher1 wrote:
> These could use some name constants (kMinRtpHeaderExtensionId and
> kMaxrtpHeaderExtensionId)

Done.

https://codereview.webrtc.org/2675173003/diff/480001/webrtc/ortc/rtptransport...
File webrtc/ortc/rtptransportcontrolleradapter.cc (right):

https://codereview.webrtc.org/2675173003/diff/480001/webrtc/ortc/rtptransport...
webrtc/ortc/rtptransportcontrolleradapter.cc:73: return transport_proxies_;
On 2017/02/17 23:10:22, pthatcher1 wrote:
> What is this even used for?  It seems like it's not really needed.  In fact, I
> don't understand why you even need transport_proxies_.  

To return in GetTransports.

https://codereview.webrtc.org/2675173003/diff/480001/webrtc/ortc/rtptransport...
File webrtc/ortc/rtptransportcontrolleradapter.h (right):

https://codereview.webrtc.org/2675173003/diff/480001/webrtc/ortc/rtptransport...
webrtc/ortc/rtptransportcontrolleradapter.h:83: void
AddTransport(RtpTransportInterface* transport_proxy);
On 2017/02/17 23:10:22, pthatcher1 wrote:
> Does anything call AddTransport other than the Transport itself?  In that
case,
> why not require an RtpTransportAdapter here?  Then at least it's symmetric
with
> RemoveTransport.

Actually, it's RtpTransportAdapter::Create that ultimately will call this. Like
I mentioned, this CL isn't finished yet.

https://codereview.webrtc.org/2675173003/diff/480001/webrtc/ortc/rtptransport...
webrtc/ortc/rtptransportcontrolleradapter.h:105: void DetachVideoReceiver();
On 2017/02/17 23:10:22, pthatcher1 wrote:
> I still find it awkward to have a method AttachX(Y y) where X and Y are
> completely different.
> 
> It seems what you are really doing is saying "Create a VideoChannel with this
> transport if there isn't already one" and "Destroy a VideoChannel if I'm the
> last one using it".  
> 
> Would it be better to do this?  
> 
> VoiceChannel* GetOrCreateVoiceChannel(RtpTransportInterface* transport);
> VideoChannel* GetOrCreateVideoChannel(RtpTransportInterface* transport);
> void DestroyVoiceChannel(VoiceChannel*);
> void DestroyVideoChannel(VideoChannel*);
> 
> Then it would be almost exactly like TransportController::CreateDtlsTransport
> and DestroyDtlsTransport.  Just refcount the number of times Create/Destroy is
> called and create/destroy the channels on demand. 
> 
> Then you don't need voice_channel() and video_channel() either. 
>  

That doesn't work, because "AttachAudioSender" needs to fail if called twice
before "DetachAudioSender" is called.

There are certainly more elegant ways this could be implemented, with sigslots,
dependency injection, etc. But I went with what was simplest since this is
throwaway code and probably won't need to be changed before it's deleted. If you
think it's not sufficient, let's sit down and talk about it some time.

pthatcher1

https://codereview.webrtc.org/2675173003/diff/480001/webrtc/ortc/rtptransportcontrolleradapter.cc File webrtc/ortc/rtptransportcontrolleradapter.cc (right): https://codereview.webrtc.org/2675173003/diff/480001/webrtc/ortc/rtptransportcontrolleradapter.cc#newcode73 webrtc/ortc/rtptransportcontrolleradapter.cc:73: return transport_proxies_; On 2017/02/17 23:48:03, Taylor Brandstetter wrote: > ...

3 years, 10 months ago (2017-02-18 00:25:00 UTC) #17

https://codereview.webrtc.org/2675173003/diff/480001/webrtc/ortc/rtptransport...
File webrtc/ortc/rtptransportcontrolleradapter.cc (right):

https://codereview.webrtc.org/2675173003/diff/480001/webrtc/ortc/rtptransport...
webrtc/ortc/rtptransportcontrolleradapter.cc:73: return transport_proxies_;
On 2017/02/17 23:48:03, Taylor Brandstetter wrote:
> On 2017/02/17 23:10:22, pthatcher1 wrote:
> > What is this even used for?  It seems like it's not really needed.  In fact,
I
> > don't understand why you even need transport_proxies_.  
> 
> To return in GetTransports.

But why do we need GetTransports?

https://codereview.webrtc.org/2675173003/diff/480001/webrtc/ortc/rtptransport...
File webrtc/ortc/rtptransportcontrolleradapter.h (right):

https://codereview.webrtc.org/2675173003/diff/480001/webrtc/ortc/rtptransport...
webrtc/ortc/rtptransportcontrolleradapter.h:105: void DetachVideoReceiver();
On 2017/02/17 23:48:03, Taylor Brandstetter wrote:
> On 2017/02/17 23:10:22, pthatcher1 wrote:
> > I still find it awkward to have a method AttachX(Y y) where X and Y are
> > completely different.
> > 
> > It seems what you are really doing is saying "Create a VideoChannel with
this
> > transport if there isn't already one" and "Destroy a VideoChannel if I'm the
> > last one using it".  
> > 
> > Would it be better to do this?  
> > 
> > VoiceChannel* GetOrCreateVoiceChannel(RtpTransportInterface* transport);
> > VideoChannel* GetOrCreateVideoChannel(RtpTransportInterface* transport);
> > void DestroyVoiceChannel(VoiceChannel*);
> > void DestroyVideoChannel(VideoChannel*);
> > 
> > Then it would be almost exactly like
TransportController::CreateDtlsTransport
> > and DestroyDtlsTransport.  Just refcount the number of times Create/Destroy
is
> > called and create/destroy the channels on demand. 
> > 
> > Then you don't need voice_channel() and video_channel() either. 
> >  
> 
> That doesn't work, because "AttachAudioSender" needs to fail if called twice
> before "DetachAudioSender" is called.
> 
> There are certainly more elegant ways this could be implemented, with
sigslots,
> dependency injection, etc. But I went with what was simplest since this is
> throwaway code and probably won't need to be changed before it's deleted. If
you
> think it's not sufficient, let's sit down and talk about it some time.

I understand the "this is a temporary hack" part of it.  Perhaps if it were at
least called what it actually does, regardless of how hacky it is.  And what it
actually does is kind of like:

SetVoiceChannelNeededForSending()
SetVoiceChannelNeededForReceiving()
SetVideoChannelNeededForSending()
SetVideoChannelNeededForReceiving() 
ClearVoiceChannelNeededForSending()
ClearVoiceChannelNeededForReceiving()
ClearVideoChannelNeededForSending()
ClearVideoChannelNeededForReceiving()  

Which perhaps would be better just as:

// 0 => 1 causes voice_channel() to be non-null
// 1 => 0 causes voice_channel() to be null
IncrementNeedsForVoiceChannel();
DecrementNeedsForVoiceChannel();
// 0 => 1 causes video_channel() to be non-null
// 1 => 0 causes video_channel() to be null
IncrementNeedsForVideoChannel();
DecrementNeedsForVideoChannel();


The only thing left is the transport thing.  So here's my thinking on that:  can
we just do something hacky like always use the first transport as audio and the
second as video (or use the first as video if there is no second one)?  There
are only ever going to be 1 or 2 transports for a long time, especially since
this code is temporary.

Taylor Brandstetter

https://codereview.webrtc.org/2675173003/diff/480001/webrtc/ortc/rtptransportcontrolleradapter.cc File webrtc/ortc/rtptransportcontrolleradapter.cc (right): https://codereview.webrtc.org/2675173003/diff/480001/webrtc/ortc/rtptransportcontrolleradapter.cc#newcode73 webrtc/ortc/rtptransportcontrolleradapter.cc:73: return transport_proxies_; On 2017/02/18 00:25:00, pthatcher1 wrote: > On ...

3 years, 10 months ago (2017-02-18 00:55:15 UTC) #18

https://codereview.webrtc.org/2675173003/diff/480001/webrtc/ortc/rtptransport...
File webrtc/ortc/rtptransportcontrolleradapter.cc (right):

https://codereview.webrtc.org/2675173003/diff/480001/webrtc/ortc/rtptransport...
webrtc/ortc/rtptransportcontrolleradapter.cc:73: return transport_proxies_;
On 2017/02/18 00:25:00, pthatcher1 wrote:
> On 2017/02/17 23:48:03, Taylor Brandstetter wrote:
> > On 2017/02/17 23:10:22, pthatcher1 wrote:
> > > What is this even used for?  It seems like it's not really needed.  In
fact,
> I
> > > don't understand why you even need transport_proxies_.  
> > 
> > To return in GetTransports.
> 
> But why do we need GetTransports?

For consistency with other APIs, and for convenience. None of the methods like
"GetRtpPacketTransport()" or "GetRtpTransport()" are *really* necessary; the
application can keep track of the object relationships itself. But it's a
nice-to-have, and very simple to implement.

https://codereview.webrtc.org/2675173003/diff/480001/webrtc/ortc/rtptransport...
File webrtc/ortc/rtptransportcontrolleradapter.h (right):

https://codereview.webrtc.org/2675173003/diff/480001/webrtc/ortc/rtptransport...
webrtc/ortc/rtptransportcontrolleradapter.h:105: void DetachVideoReceiver();
On 2017/02/18 00:25:00, pthatcher1 wrote:
> 
> I understand the "this is a temporary hack" part of it.  Perhaps if it were at
> least called what it actually does, regardless of how hacky it is.  And what
it
> actually does is kind of like:
> 
> SetVoiceChannelNeededForSending()
> SetVoiceChannelNeededForReceiving()
> SetVideoChannelNeededForSending()
> SetVideoChannelNeededForReceiving() 
> ClearVoiceChannelNeededForSending()
> ClearVoiceChannelNeededForReceiving()
> ClearVideoChannelNeededForSending()
> ClearVideoChannelNeededForReceiving()  
> 
> Which perhaps would be better just as:
> 
> // 0 => 1 causes voice_channel() to be non-null
> // 1 => 0 causes voice_channel() to be null
> IncrementNeedsForVoiceChannel();
> DecrementNeedsForVoiceChannel();
> // 0 => 1 causes video_channel() to be non-null
> // 1 => 0 causes video_channel() to be null
> IncrementNeedsForVideoChannel();
> DecrementNeedsForVideoChannel();

It wouldn't make sense if I called "SetX" twice in a row and it failed though,
so that naming is more misleading in my opinion.

> The only thing left is the transport thing.  So here's my thinking on that: 
can
> we just do something hacky like always use the first transport as audio and
the
> second as video (or use the first as video if there is no second one)?  There
> are only ever going to be 1 or 2 transports for a long time, especially since
> this code is temporary.  

What's "the transport thing"? What's "the first transport"; the first transport
passed into what method?

Can we please talk about this in person like I asked? We aren't really getting
anywhere.

Taylor Brandstetter

Still working on tests. https://codereview.webrtc.org/2675173003/diff/480001/webrtc/ortc/rtptransportcontrolleradapter.h File webrtc/ortc/rtptransportcontrolleradapter.h (right): https://codereview.webrtc.org/2675173003/diff/480001/webrtc/ortc/rtptransportcontrolleradapter.h#newcode105 webrtc/ortc/rtptransportcontrolleradapter.h:105: void DetachVideoReceiver(); On 2017/02/18 00:55:15, ...

3 years, 10 months ago (2017-02-18 04:05:10 UTC) #19

Still working on tests.

https://codereview.webrtc.org/2675173003/diff/480001/webrtc/ortc/rtptransport...
File webrtc/ortc/rtptransportcontrolleradapter.h (right):

https://codereview.webrtc.org/2675173003/diff/480001/webrtc/ortc/rtptransport...
webrtc/ortc/rtptransportcontrolleradapter.h:105: void DetachVideoReceiver();
On 2017/02/18 00:55:15, Taylor Brandstetter wrote:
> On 2017/02/18 00:25:00, pthatcher1 wrote:
> > 
> > I understand the "this is a temporary hack" part of it.  Perhaps if it were
at
> > least called what it actually does, regardless of how hacky it is.  And what
> it
> > actually does is kind of like:
> > 
> > SetVoiceChannelNeededForSending()
> > SetVoiceChannelNeededForReceiving()
> > SetVideoChannelNeededForSending()
> > SetVideoChannelNeededForReceiving() 
> > ClearVoiceChannelNeededForSending()
> > ClearVoiceChannelNeededForReceiving()
> > ClearVideoChannelNeededForSending()
> > ClearVideoChannelNeededForReceiving()  
> > 
> > Which perhaps would be better just as:
> > 
> > // 0 => 1 causes voice_channel() to be non-null
> > // 1 => 0 causes voice_channel() to be null
> > IncrementNeedsForVoiceChannel();
> > DecrementNeedsForVoiceChannel();
> > // 0 => 1 causes video_channel() to be non-null
> > // 1 => 0 causes video_channel() to be null
> > IncrementNeedsForVideoChannel();
> > DecrementNeedsForVideoChannel();
> 
> It wouldn't make sense if I called "SetX" twice in a row and it failed though,
> so that naming is more misleading in my opinion.
> 
> > The only thing left is the transport thing.  So here's my thinking on that: 
> can
> > we just do something hacky like always use the first transport as audio and
> the
> > second as video (or use the first as video if there is no second one)? 
There
> > are only ever going to be 1 or 2 transports for a long time, especially
since
> > this code is temporary.  
> 
> What's "the transport thing"? What's "the first transport"; the first
transport
> passed into what method?
> 
> Can we please talk about this in person like I asked? We aren't really getting
> anywhere.
> 

I went ahead and implemented the sigslot thing (patchset #31) in the interest of
resolving this quickly. I could go a step further and use dependency injection,
but that would likely cause more complexity than it's worth.

Taylor Brandstetter

Ok, I think this is ready for a final round of review. I know it's ...

3 years, 10 months ago (2017-02-21 10:41:50 UTC) #20

pthatcher1

I still need to go over the unit tests. What I have found in the ...

3 years, 10 months ago (2017-02-21 20:11:56 UTC) #21

pthatcher1

lgtm Those tests look very good. The only question I have is why you use ...

3 years, 10 months ago (2017-02-21 20:34:54 UTC) #22

Taylor Brandstetter

deadbeef@webrtc.org changed reviewers: + kjellander@webrtc.org

3 years, 10 months ago (2017-02-22 01:41:56 UTC) #23

Taylor Brandstetter

kjellander: Can you review the build files? One thing to note is that, as-is, this ...

3 years, 10 months ago (2017-02-22 01:41:59 UTC) #24

kjellander: Can you review the build files?

One thing to note is that, as-is, this CL creates an additional ortc_unittests
target. Is that ok or would you prefer putting these tests in another
executable? I think "peerconnection_unittests" would be a little deceiving,
since ORTC is meant to be an alternative to PeerConnection.

https://codereview.webrtc.org/2675173003/diff/720001/webrtc/media/base/codec.h
File webrtc/media/base/codec.h (right):

https://codereview.webrtc.org/2675173003/diff/720001/webrtc/media/base/codec....
webrtc/media/base/codec.h:29: FeedbackParam() {}
On 2017/02/21 20:11:55, pthatcher1 wrote:
> = default
> 
> ?

Done.

https://codereview.webrtc.org/2675173003/diff/720001/webrtc/ortc/ortcfactory_...
File webrtc/ortc/ortcfactory_unittest.cc (right):

https://codereview.webrtc.org/2675173003/diff/720001/webrtc/ortc/ortcfactory_...
webrtc/ortc/ortcfactory_unittest.cc:41: // Uses a single premade
FakePacketTransport, so shouldn't be called twice in
On 2017/02/21 20:11:55, pthatcher1 wrote:
> pre-made

Done.

https://codereview.webrtc.org/2675173003/diff/720001/webrtc/ortc/rtpparameter...
File webrtc/ortc/rtpparametersconversion.h (right):

https://codereview.webrtc.org/2675173003/diff/720001/webrtc/ortc/rtpparameter...
webrtc/ortc/rtpparametersconversion.h:62: const
std::vector<RtpHeaderExtensionParameters>& extensions);
On 2017/02/21 20:11:55, pthatcher1 wrote:
> ToCricketX everywhere?

Done.

https://codereview.webrtc.org/2675173003/diff/720001/webrtc/ortc/rtptransport...
File webrtc/ortc/rtptransportcontroller_unittest.cc (right):

https://codereview.webrtc.org/2675173003/diff/720001/webrtc/ortc/rtptransport...
webrtc/ortc/rtptransportcontroller_unittest.cc:35: :
virtual_socket_server_(&physical_socket_server_),
On 2017/02/21 20:34:54, pthatcher1 wrote:
> Why do you need a virtual socket server when you're using FakeTransports?

Copy pasted from the wrong test probably. Fixed.

https://codereview.webrtc.org/2675173003/diff/720001/webrtc/ortc/rtptransport...
webrtc/ortc/rtptransportcontroller_unittest.cc:67:
ASSERT_TRUE(rtp_transport_result2.ok());
On 2017/02/21 20:34:54, pthatcher1 wrote:
> This makes it feel like rtp_tranpsort_controller_->CreateRtpTransport would
make
> more sense.  But, as I said, I would rather get this in as-is than make it
> perfect.

Oh, you mean for CreateRtpTransport to be on RtpTransportController rather than
OrtcFactory?

Well... If we go down that path, then we'd have one CreateRtpTransport method on
OrtcFactory (that doesn't take a transport controller) and one on
RtpTransportController, which is a little odd. I'd prefer just keeping a pattern
of "every object gets created by the OrtcFactory", for consistency, even if it
means "CreateRtpTransport" takes one more argument than it really needs.

https://codereview.webrtc.org/2675173003/diff/720001/webrtc/ortc/rtptransport...
File webrtc/ortc/rtptransportcontrolleradapter.h (right):

https://codereview.webrtc.org/2675173003/diff/720001/webrtc/ortc/rtptransport...
webrtc/ortc/rtptransportcontrolleradapter.h:77: // that will safely call methods
on the correct thread.
On 2017/02/21 20:11:55, pthatcher1 wrote:
> The transport argument needs to be a proxy to a transport because we expect
this
> to be called by the OrtcFactory, which receives a proxy to a transport because
> that's what it gives out.

Not completely accurate; it needs to be a proxy to a transport because the
application may call GetTransport() on the returned sender, and expects this to
be a thread-safe transport proxy. That's what I put in a new comment.

https://codereview.webrtc.org/2675173003/diff/720001/webrtc/ortc/rtptransport...
webrtc/ortc/rtptransportcontrolleradapter.h:96: void
RemoveTransport(RtpTransportAdapter* inner_transport);
On 2017/02/21 20:11:55, pthatcher1 wrote:
> Could we use the same model of CreateProxiedRtpTransport with a
SignalDestroyed
> that ends up removing it?

Done.

kjellander_webrtc

On 2017/02/22 01:41:59, Taylor Brandstetter wrote: > kjellander: Can you review the build files? Yes, ...

3 years, 10 months ago (2017-02-22 12:42:36 UTC) #25

On 2017/02/22 01:41:59, Taylor Brandstetter wrote:
> kjellander: Can you review the build files?

Yes, lgtm on BUILD.gn and DEPS files.

> One thing to note is that, as-is, this CL creates an additional ortc_unittests
> target. Is that ok or would you prefer putting these tests in another
> executable? I think "peerconnection_unittests" would be a little deceiving,
> since ORTC is meant to be an alternative to PeerConnection.

You're right, that sounds reasonable since ortc is also a top-level directory.
I'll take care of adding the tests once this is landed. Actually, could you in
another CL (before landing this) create a dummy test target ortc_unittests with
only one test that returns success? That way you'll be able to test the contents
of this CL on all the trybots before landing (my experience says that when
adding a new test, it's always failing on some platform).

> 
> https://codereview.webrtc.org/2675173003/diff/720001/webrtc/media/base/codec.h
> File webrtc/media/base/codec.h (right):
> 
>
https://codereview.webrtc.org/2675173003/diff/720001/webrtc/media/base/codec....
> webrtc/media/base/codec.h:29: FeedbackParam() {}
> On 2017/02/21 20:11:55, pthatcher1 wrote:
> > = default
> > 
> > ?
> 
> Done.
> 
>
https://codereview.webrtc.org/2675173003/diff/720001/webrtc/ortc/ortcfactory_...
> File webrtc/ortc/ortcfactory_unittest.cc (right):
> 
>
https://codereview.webrtc.org/2675173003/diff/720001/webrtc/ortc/ortcfactory_...
> webrtc/ortc/ortcfactory_unittest.cc:41: // Uses a single premade
> FakePacketTransport, so shouldn't be called twice in
> On 2017/02/21 20:11:55, pthatcher1 wrote:
> > pre-made
> 
> Done.
> 
>
https://codereview.webrtc.org/2675173003/diff/720001/webrtc/ortc/rtpparameter...
> File webrtc/ortc/rtpparametersconversion.h (right):
> 
>
https://codereview.webrtc.org/2675173003/diff/720001/webrtc/ortc/rtpparameter...
> webrtc/ortc/rtpparametersconversion.h:62: const
> std::vector<RtpHeaderExtensionParameters>& extensions);
> On 2017/02/21 20:11:55, pthatcher1 wrote:
> > ToCricketX everywhere?
> 
> Done.
> 
>
https://codereview.webrtc.org/2675173003/diff/720001/webrtc/ortc/rtptransport...
> File webrtc/ortc/rtptransportcontroller_unittest.cc (right):
> 
>
https://codereview.webrtc.org/2675173003/diff/720001/webrtc/ortc/rtptransport...
> webrtc/ortc/rtptransportcontroller_unittest.cc:35: :
> virtual_socket_server_(&physical_socket_server_),
> On 2017/02/21 20:34:54, pthatcher1 wrote:
> > Why do you need a virtual socket server when you're using FakeTransports?
> 
> Copy pasted from the wrong test probably. Fixed.
> 
>
https://codereview.webrtc.org/2675173003/diff/720001/webrtc/ortc/rtptransport...
> webrtc/ortc/rtptransportcontroller_unittest.cc:67:
> ASSERT_TRUE(rtp_transport_result2.ok());
> On 2017/02/21 20:34:54, pthatcher1 wrote:
> > This makes it feel like rtp_tranpsort_controller_->CreateRtpTransport would
> make
> > more sense.  But, as I said, I would rather get this in as-is than make it
> > perfect.
> 
> Oh, you mean for CreateRtpTransport to be on RtpTransportController rather
than
> OrtcFactory?
> 
> Well... If we go down that path, then we'd have one CreateRtpTransport method
on
> OrtcFactory (that doesn't take a transport controller) and one on
> RtpTransportController, which is a little odd. I'd prefer just keeping a
pattern
> of "every object gets created by the OrtcFactory", for consistency, even if it
> means "CreateRtpTransport" takes one more argument than it really needs.
> 
>
https://codereview.webrtc.org/2675173003/diff/720001/webrtc/ortc/rtptransport...
> File webrtc/ortc/rtptransportcontrolleradapter.h (right):
> 
>
https://codereview.webrtc.org/2675173003/diff/720001/webrtc/ortc/rtptransport...
> webrtc/ortc/rtptransportcontrolleradapter.h:77: // that will safely call
methods
> on the correct thread.
> On 2017/02/21 20:11:55, pthatcher1 wrote:
> > The transport argument needs to be a proxy to a transport because we expect
> this
> > to be called by the OrtcFactory, which receives a proxy to a transport
because
> > that's what it gives out.
> 
> Not completely accurate; it needs to be a proxy to a transport because the
> application may call GetTransport() on the returned sender, and expects this
to
> be a thread-safe transport proxy. That's what I put in a new comment.
> 
>
https://codereview.webrtc.org/2675173003/diff/720001/webrtc/ortc/rtptransport...
> webrtc/ortc/rtptransportcontrolleradapter.h:96: void
> RemoveTransport(RtpTransportAdapter* inner_transport);
> On 2017/02/21 20:11:55, pthatcher1 wrote:
> > Could we use the same model of CreateProxiedRtpTransport with a
> SignalDestroyed
> > that ends up removing it?
> 
> Done.

Taylor Brandstetter

Description was changed from ========== Adding "adapter" ORTC objects on top of ChannelManager/BaseChannel/etc. Will fill ...

3 years, 10 months ago (2017-02-23 05:58:48 UTC) #26

Taylor Brandstetter

Description was changed from ========== Adding "adapter" ORTC objects on top of ChannelManager/BaseChannel/etc. This CL ...

3 years, 10 months ago (2017-02-23 05:59:24 UTC) #27

Description was changed from

==========
Adding "adapter" ORTC objects on top of ChannelManager/BaseChannel/etc.

This CL adds the following interfaces:
* RtpTransportController
* RtpTransport
* RtpSender
* RtpReceiver

They're implemented on top of the "BaseChannel" object, which is normally used
in a PeerConnection, and roughly corresponds to an SDP "m=" section. As a result
of this, there are several limitations:

* You can only have one of each type of sender and receiver (audio/video) on top
   of the same transport controller.
* The sender/receiver with the same media type must use the same RTP transport.
* You can't change the transport after creating the sender or receiver.
* Some of the parameters aren't supported.

Later, these "adapter" objects will be gradually replaced by real objects that
don't
have these limitations, as "BaseChannel", "MediaChannel" and related code is
restructured. In this CL, we essentially have:

ORTC adapter objects -> BaseChannel -> Media engine
PeerConnection -> BaseChannel -> Media engine

And later we hope to have simply:

PeerConnection -> "Real" ORTC objects -> Media engine

See the linked bug for more context.

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

to

==========
Adding "adapter" ORTC objects on top of ChannelManager/BaseChannel/etc.

This CL adds the following interfaces:
* RtpTransportController
* RtpTransport
* RtpSender
* RtpReceiver

They're implemented on top of the "BaseChannel" object, which is normally used
in a PeerConnection, and roughly corresponds to an SDP "m=" section. As a result
of this, there are several limitations:

* You can only have one of each type of sender and receiver (audio/video) on top
  of the same transport controller.
* The sender/receiver with the same media type must use the same RTP transport.
* You can't change the transport after creating the sender or receiver.
* Some of the parameters aren't supported.

Later, these "adapter" objects will be gradually replaced by real objects that
don't
have these limitations, as "BaseChannel", "MediaChannel" and related code is
restructured. In this CL, we essentially have:

ORTC adapter objects -> BaseChannel -> Media engine
PeerConnection -> BaseChannel -> Media engine

And later we hope to have simply:

PeerConnection -> "Real" ORTC objects -> Media engine

See the linked bug for more context.

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

Taylor Brandstetter

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

3 years, 10 months ago (2017-02-24 07:56:00 UTC) #28

commit-bot: I haz the power

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

3 years, 10 months ago (2017-02-24 07:56:09 UTC) #29

commit-bot: I haz the power

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

3 years, 10 months ago (2017-02-24 07:57:58 UTC) #30

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: ios32_sim_ios9_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios32_sim_ios9_dbg/builds/1836) ios64_sim_ios9_dbg on ...

3 years, 10 months ago (2017-02-24 07:58:00 UTC) #31

Taylor Brandstetter

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

3 years, 10 months ago (2017-02-24 10:29:26 UTC) #32

commit-bot: I haz the power

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

3 years, 10 months ago (2017-02-24 10:29:33 UTC) #33

commit-bot: I haz the power

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

3 years, 10 months ago (2017-02-24 10:31:23 UTC) #34

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: ios32_sim_ios9_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios32_sim_ios9_dbg/builds/1840) ios64_sim_ios10_dbg on ...

3 years, 10 months ago (2017-02-24 10:31:25 UTC) #35

Taylor Brandstetter

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

3 years, 10 months ago (2017-02-24 10:40:27 UTC) #36

commit-bot: I haz the power

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

3 years, 10 months ago (2017-02-24 10:40:37 UTC) #37

commit-bot: I haz the power

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

3 years, 10 months ago (2017-02-24 10:45:06 UTC) #38

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: ios64_sim_ios10_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios64_sim_ios10_dbg/builds/1786) ios_arm64_rel on ...

3 years, 10 months ago (2017-02-24 10:45:09 UTC) #39

Taylor Brandstetter

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

3 years, 10 months ago (2017-02-25 02:51:50 UTC) #40

commit-bot: I haz the power

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

3 years, 10 months ago (2017-02-25 02:52:02 UTC) #41

commit-bot: I haz the power

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

3 years, 10 months ago (2017-02-25 02:55:29 UTC) #42

commit-bot: I haz the power

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

3 years, 10 months ago (2017-02-25 02:55:34 UTC) #43

Taylor Brandstetter

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

3 years, 10 months ago (2017-02-25 04:11:41 UTC) #44

commit-bot: I haz the power

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

3 years, 10 months ago (2017-02-25 04:11:48 UTC) #45

commit-bot: I haz the power

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

3 years, 10 months ago (2017-02-25 04:16:36 UTC) #46

commit-bot: I haz the power

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

3 years, 10 months ago (2017-02-25 04:16:38 UTC) #47

Taylor Brandstetter

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

3 years, 10 months ago (2017-02-25 09:30:55 UTC) #48

commit-bot: I haz the power

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

3 years, 10 months ago (2017-02-25 09:31:06 UTC) #49

commit-bot: I haz the power

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

3 years, 10 months ago (2017-02-25 09:42:57 UTC) #50

commit-bot: I haz the power

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

3 years, 10 months ago (2017-02-25 09:42:59 UTC) #51

Taylor Brandstetter

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

3 years, 10 months ago (2017-02-25 10:15:11 UTC) #52

commit-bot: I haz the power

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

3 years, 10 months ago (2017-02-25 10:15:23 UTC) #53

commit-bot: I haz the power

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

3 years, 10 months ago (2017-02-25 10:19:32 UTC) #54

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: win_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_dbg/builds/16668) win_rel on ...

3 years, 10 months ago (2017-02-25 10:19:33 UTC) #55

Taylor Brandstetter

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

3 years, 10 months ago (2017-02-25 22:27:10 UTC) #56

commit-bot: I haz the power

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

3 years, 10 months ago (2017-02-25 22:27:22 UTC) #57

commit-bot: I haz the power

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

3 years, 10 months ago (2017-02-25 23:40:35 UTC) #58

commit-bot: I haz the power

Dry run: This issue passed the CQ dry run.

3 years, 10 months ago (2017-02-25 23:40:37 UTC) #59

Taylor Brandstetter

The CQ bit was checked by deadbeef@webrtc.org

3 years, 10 months ago (2017-02-25 23:41:41 UTC) #60

Taylor Brandstetter

The patchset sent to the CQ was uploaded after l-g-t-m from pthatcher@webrtc.org, kjellander@webrtc.org Link to ...

3 years, 10 months ago (2017-02-25 23:41:43 UTC) #61

commit-bot: I haz the power

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

3 years, 10 months ago (2017-02-25 23:41:46 UTC) #62

commit-bot: I haz the power

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

3 years, 10 months ago (2017-02-25 23:46:34 UTC) #63

commit-bot: I haz the power

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

3 years, 10 months ago (2017-02-25 23:46:37 UTC) #64

Taylor Brandstetter

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

3 years, 10 months ago (2017-02-25 23:55:22 UTC) #65

Taylor Brandstetter

Description was changed from ========== Adding "adapter" ORTC objects on top of ChannelManager/BaseChannel/etc. This CL ...

3 years, 10 months ago (2017-02-25 23:58:55 UTC) #67

Description was changed from

==========
Adding "adapter" ORTC objects on top of ChannelManager/BaseChannel/etc.

This CL adds the following interfaces:
* RtpTransportController
* RtpTransport
* RtpSender
* RtpReceiver

They're implemented on top of the "BaseChannel" object, which is normally used
in a PeerConnection, and roughly corresponds to an SDP "m=" section. As a result
of this, there are several limitations:

* You can only have one of each type of sender and receiver (audio/video) on top
  of the same transport controller.
* The sender/receiver with the same media type must use the same RTP transport.
* You can't change the transport after creating the sender or receiver.
* Some of the parameters aren't supported.

Later, these "adapter" objects will be gradually replaced by real objects that
don't
have these limitations, as "BaseChannel", "MediaChannel" and related code is
restructured. In this CL, we essentially have:

ORTC adapter objects -> BaseChannel -> Media engine
PeerConnection -> BaseChannel -> Media engine

And later we hope to have simply:

PeerConnection -> "Real" ORTC objects -> Media engine

See the linked bug for more context.

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

to

==========
Adding "adapter" ORTC objects on top of ChannelManager/BaseChannel/etc.

This CL adds the following interfaces:
* RtpTransportController
* RtpTransport
* RtpSender
* RtpReceiver

They're implemented on top of the "BaseChannel" object, which is normally used
in a PeerConnection, and roughly corresponds to an SDP "m=" section. As a result
of this, there are several limitations:

* You can only have one of each type of sender and receiver (audio/video) on top
  of the same transport controller.
* The sender/receiver with the same media type must use the same RTP transport.
* You can't change the transport after creating the sender or receiver.
* Some of the parameters aren't supported.

Later, these "adapter" objects will be gradually replaced by real objects that
don't
have these limitations, as "BaseChannel", "MediaChannel" and related code is
restructured. In this CL, we essentially have:

ORTC adapter objects -> BaseChannel -> Media engine
PeerConnection -> BaseChannel -> Media engine

And later we hope to have simply:

PeerConnection -> "Real" ORTC objects -> Media engine

See the linked bug for more context.

BUG=webrtc:7013
TBR=stefan@webrtc.org
==========

Taylor Brandstetter

The CQ bit was checked by deadbeef@webrtc.org

3 years, 10 months ago (2017-02-25 23:59:09 UTC) #68

Taylor Brandstetter

The patchset sent to the CQ was uploaded after l-g-t-m from kjellander@webrtc.org, pthatcher@webrtc.org Link to ...

3 years, 10 months ago (2017-02-25 23:59:10 UTC) #69

commit-bot: I haz the power

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

3 years, 10 months ago (2017-02-25 23:59:17 UTC) #70

commit-bot: I haz the power

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

3 years, 10 months ago (2017-02-26 00:34:00 UTC) #71

commit-bot: I haz the power

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/5675)

3 years, 10 months ago (2017-02-26 00:34:02 UTC) #72

Taylor Brandstetter

The CQ bit was checked by deadbeef@webrtc.org

3 years, 10 months ago (2017-02-26 01:08:04 UTC) #73

Taylor Brandstetter

The patchset sent to the CQ was uploaded after l-g-t-m from kjellander@webrtc.org, pthatcher@webrtc.org Link to ...

3 years, 10 months ago (2017-02-26 01:08:05 UTC) #74

commit-bot: I haz the power

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

3 years, 10 months ago (2017-02-26 01:08:15 UTC) #75

commit-bot: I haz the power

CQ is committing da patch. Bot data: {"patchset_id": 1080001, "attempt_start_ts": 1488071284657340, "parent_rev": "f15fb452efe5a7082dd49faceba1ff123985f258", "commit_rev": "e814a0dee015fe06b53673f6b021825130413b56"}

3 years, 10 months ago (2017-02-26 02:15:11 UTC) #76

commit-bot: I haz the power

Description was changed from ========== Adding "adapter" ORTC objects on top of ChannelManager/BaseChannel/etc. This CL ...

3 years, 10 months ago (2017-02-26 02:15:16 UTC) #77

Message was sent while issue was closed.

Description was changed from

==========
Adding "adapter" ORTC objects on top of ChannelManager/BaseChannel/etc.

This CL adds the following interfaces:
* RtpTransportController
* RtpTransport
* RtpSender
* RtpReceiver

They're implemented on top of the "BaseChannel" object, which is normally used
in a PeerConnection, and roughly corresponds to an SDP "m=" section. As a result
of this, there are several limitations:

* You can only have one of each type of sender and receiver (audio/video) on top
  of the same transport controller.
* The sender/receiver with the same media type must use the same RTP transport.
* You can't change the transport after creating the sender or receiver.
* Some of the parameters aren't supported.

Later, these "adapter" objects will be gradually replaced by real objects that
don't
have these limitations, as "BaseChannel", "MediaChannel" and related code is
restructured. In this CL, we essentially have:

ORTC adapter objects -> BaseChannel -> Media engine
PeerConnection -> BaseChannel -> Media engine

And later we hope to have simply:

PeerConnection -> "Real" ORTC objects -> Media engine

See the linked bug for more context.

BUG=webrtc:7013
TBR=stefan@webrtc.org
==========

to

==========
Adding "adapter" ORTC objects on top of ChannelManager/BaseChannel/etc.

This CL adds the following interfaces:
* RtpTransportController
* RtpTransport
* RtpSender
* RtpReceiver

They're implemented on top of the "BaseChannel" object, which is normally used
in a PeerConnection, and roughly corresponds to an SDP "m=" section. As a result
of this, there are several limitations:

* You can only have one of each type of sender and receiver (audio/video) on top
  of the same transport controller.
* The sender/receiver with the same media type must use the same RTP transport.
* You can't change the transport after creating the sender or receiver.
* Some of the parameters aren't supported.

Later, these "adapter" objects will be gradually replaced by real objects that
don't
have these limitations, as "BaseChannel", "MediaChannel" and related code is
restructured. In this CL, we essentially have:

ORTC adapter objects -> BaseChannel -> Media engine
PeerConnection -> BaseChannel -> Media engine

And later we hope to have simply:

PeerConnection -> "Real" ORTC objects -> Media engine

See the linked bug for more context.

BUG=webrtc:7013
TBR=stefan@webrtc.org

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

commit-bot: I haz the power

Committed patchset #57 (id:1080001) as https://chromium.googlesource.com/external/webrtc/+/e814a0dee015fe06b53673f6b021825130413b56

3 years, 10 months ago (2017-02-26 02:15:19 UTC) #78

tommi

On 2017/02/26 02:15:19, commit-bot: I haz the power wrote: > Committed patchset #57 (id:1080001) as ...

3 years, 9 months ago (2017-02-26 22:01:26 UTC) #79

tommi

On 2017/02/26 22:01:26, tommi (webrtc) wrote: > On 2017/02/26 02:15:19, commit-bot: I haz the power ...

3 years, 9 months ago (2017-02-26 22:03:30 UTC) #80

Taylor Brandstetter

On 2017/02/26 22:03:30, tommi (webrtc) wrote: > On 2017/02/26 22:01:26, tommi (webrtc) wrote: > > ...

3 years, 9 months ago (2017-02-27 21:34:59 UTC) #81

Taylor Brandstetter

3 years, 9 months ago (2017-02-27 21:43:44 UTC) #82

Message was sent while issue was closed.

On 2017/02/27 21:34:59, Taylor Brandstetter wrote:
> kjellander: Do you know why
> tools-webrtc/valgrind/gtest_exclude/ortc_unittests.gtest-memcheck.txt isn't
> having the expected effect?

Oh, I see you already fixed it. Sorry about that, I assumed since the trybot
passed, the suppression was taking effect.

Issue 2675173003: Adding "adapter" ORTC objects on top of ChannelManager/BaseChannel/etc. (Closed)