Chromium Code Reviews
|
|
Chromium Code Reviews|
Created:
3 years, 5 months ago by sprang_webrtc Modified:
3 years, 4 months ago Reviewers:
Taylor Brandstetter, pthatcher1, ilnik, stefan-webrtc CC:
webrtc-reviews_webrtc.org, interface-changes_webrtc.org, yujie_mao (webrtc), peah-webrtc, tterriberry_mozilla.com, qiang.lu, niklas.enbom, kwiberg-webrtc, the sun, mflodman Target Ref:
refs/heads/master Project:
webrtc Visibility:
Public. |
DescriptionWire up RTP keep-alive in ortc api.
[This CL is work in progress.]
Wire up the rtp keep-alive in webrtc::Call::Config using new
SetRtpTransportParameters() method on RtpTransportInterface.
BUG=webrtc:7907
Review-Url: https://codereview.webrtc.org/2981513002
Cr-Commit-Position: refs/heads/master@{#19287}
Committed: https://chromium.googlesource.com/external/webrtc/+/db2a9fc6ec119052139d42f8897412bba7fa848b
Patch Set 1 #Patch Set 2 : . #
Total comments: 6
Patch Set 3 : wip #
Total comments: 10
Patch Set 4 : Moved RtcpParameters into RtpTransportParameters #
Total comments: 8
Patch Set 5 : Merge work by ilnik in cl 2984793002, rebase #Patch Set 6 : Test #
Total comments: 14
Patch Set 7 : Moved tests around #
Total comments: 17
Patch Set 8 : stuff #
Total comments: 6
Patch Set 9 : comments #Patch Set 10 : Undo accidental chagne #Patch Set 11 : Removed unneeded includes #Patch Set 12 : Rebase #Patch Set 13 : Update check for existing-RtpTransport-keepalive-settings #Patch Set 14 : Add workaround for gcc #
Total comments: 6
Patch Set 15 : simplify #Patch Set 16 : Configure call on worker thread #Patch Set 17 : Move keep-alive config from Call::Config to RtpTransportControllerSend #
Total comments: 2
Patch Set 18 : deps #Patch Set 19 : deps #Patch Set 20 : deps, again #
Created: 3 years, 4 months ago
Messages
Total messages: 77 (39 generated)
pthatcher@webrtc.org changed reviewers: + pthatcher@webrtc.org
https://codereview.webrtc.org/2981513002/diff/20001/webrtc/api/ortc/rtptransp... File webrtc/api/ortc/rtptransportinterface.h (right): https://codereview.webrtc.org/2981513002/diff/20001/webrtc/api/ortc/rtptransp... webrtc/api/ortc/rtptransportinterface.h:58: RtpKeepAliveConfig keepalive_config; Can we just call this "keepalive" (or keep_alive?)? https://codereview.webrtc.org/2981513002/diff/20001/webrtc/api/ortc/rtptransp... webrtc/api/ortc/rtptransportinterface.h:104: virtual RtpTransportParameters GetRtpTransportParameters() const = 0; I think we should merge these into one set of get/set: SetParameters(RtpTransportParameters) RtpTransportParameters GetParameters() Where RtpTransportParameters has RtcpParameters: struct RtpTrnsportParameters { RtcpParameters rtcp; RtpKeepAliveParameters keepalive; }
https://codereview.webrtc.org/2981513002/diff/20001/webrtc/api/ortc/rtptransp... File webrtc/api/ortc/rtptransportinterface.h (right): https://codereview.webrtc.org/2981513002/diff/20001/webrtc/api/ortc/rtptransp... webrtc/api/ortc/rtptransportinterface.h:58: RtpKeepAliveConfig keepalive_config; On 2017/07/11 18:25:09, pthatcher1 wrote: > Can we just call this "keepalive" (or keep_alive?)? Sure. https://codereview.webrtc.org/2981513002/diff/20001/webrtc/api/ortc/rtptransp... webrtc/api/ortc/rtptransportinterface.h:104: virtual RtpTransportParameters GetRtpTransportParameters() const = 0; On 2017/07/11 18:25:09, pthatcher1 wrote: > I think we should merge these into one set of get/set: > > SetParameters(RtpTransportParameters) > RtpTransportParameters GetParameters() > > > Where RtpTransportParameters has RtcpParameters: > > struct RtpTrnsportParameters { > RtcpParameters rtcp; > RtpKeepAliveParameters keepalive; > } I pondered that, but then we're mixing a transport level feature with a stream-level one. It got strange to be forced to pass along unused RtcpParameters in places were just the RtpTransportParameters made sense. Having a third "RtpRtcpParameters" or whatever struct joining these two also feels a bit overkill.
sprang@webrtc.org changed reviewers: + deadbeef@webrtc.org
Input appreciated. Will start adding tests.
https://codereview.webrtc.org/2981513002/diff/40001/webrtc/call/call.h File webrtc/call/call.h (right): https://codereview.webrtc.org/2981513002/diff/40001/webrtc/call/call.h#newcod... webrtc/call/call.h:209: // only before creating and send streams. Returns true on success. I think "and" was meant to be "any" here https://codereview.webrtc.org/2981513002/diff/40001/webrtc/ortc/rtptransporta... File webrtc/ortc/rtptransportadapter.cc (right): https://codereview.webrtc.org/2981513002/diff/40001/webrtc/ortc/rtptransporta... webrtc/ortc/rtptransportadapter.cc:192: rtp_transport_controller_->SetRtpTransportParameters(parameters); Ok, so setting RTP transport parameters on one RtpTransport will affect the entire transport controller (in other words, the whole Call)? This limitation should be called out in the applicable header file(s) in webrtc/api/ortc/. https://codereview.webrtc.org/2981513002/diff/40001/webrtc/ortc/rtptransporta... File webrtc/ortc/rtptransportadapter.h (right): https://codereview.webrtc.org/2981513002/diff/40001/webrtc/ortc/rtptransporta... webrtc/ortc/rtptransportadapter.h:88: RtpTransportAdapter(const RtcpParameters& rtcp_parameters, Since the constructor (and factory methods) take RtcpParameters, I think they should take the RtpTransportParameters as well for consistency. Maybe for the keepalive options it doesn't matter, but it seems likely we could add things to RtpTransportParameters in the future which we would want to have available at construction time. https://codereview.webrtc.org/2981513002/diff/40001/webrtc/pc/rtptransport.cc File webrtc/pc/rtptransport.cc (right): https://codereview.webrtc.org/2981513002/diff/40001/webrtc/pc/rtptransport.cc... webrtc/pc/rtptransport.cc:140: if (rtp_transport_parameters_.keepalive.timeout_interval_ms > 0 && The parameters in this class don't appear to be used. So it may be better to leave this method as a TODO and just return an error, so people don't wrongly assume things are wired up already. Note that this class is currently only used in the PeerConnection object model (hence it being in "pc/"), but it's what we plan to eventually use for both the ORTC and PeerConnection code paths, replacing "RtpTransportAdapter" with it.
https://codereview.webrtc.org/2981513002/diff/20001/webrtc/api/ortc/rtptransp... File webrtc/api/ortc/rtptransportinterface.h (right): https://codereview.webrtc.org/2981513002/diff/20001/webrtc/api/ortc/rtptransp... webrtc/api/ortc/rtptransportinterface.h:104: virtual RtpTransportParameters GetRtpTransportParameters() const = 0; On 2017/07/11 19:54:05, sprang_webrtc wrote: > On 2017/07/11 18:25:09, pthatcher1 wrote: > > I think we should merge these into one set of get/set: > > > > SetParameters(RtpTransportParameters) > > RtpTransportParameters GetParameters() > > > > > > Where RtpTransportParameters has RtcpParameters: > > > > struct RtpTrnsportParameters { > > RtcpParameters rtcp; > > RtpKeepAliveParameters keepalive; > > } > > I pondered that, but then we're mixing a transport level feature with a > stream-level one. It got strange to be forced to pass along unused > RtcpParameters in places were just the RtpTransportParameters made sense. > Having a third "RtpRtcpParameters" or whatever struct joining these two also > feels a bit overkill. Ah, that's a good point. I was thrown off by the fact that we use it here as transport-level parameters. I presume we will change that in the future to be per-stream. In that case, I'm fine with an additional getter/setter. But can you call it GetParameters and SetParameters? RtpTransport::SetParameters(RtpTransportParameters) makes a lot more sense than RtpTransport::SetRtpTransportParameters(RtpTransportParameters).
https://codereview.webrtc.org/2981513002/diff/20001/webrtc/api/ortc/rtptransp... File webrtc/api/ortc/rtptransportinterface.h (right): https://codereview.webrtc.org/2981513002/diff/20001/webrtc/api/ortc/rtptransp... webrtc/api/ortc/rtptransportinterface.h:104: virtual RtpTransportParameters GetRtpTransportParameters() const = 0; On 2017/07/11 19:54:05, sprang_webrtc wrote: > On 2017/07/11 18:25:09, pthatcher1 wrote: > > I think we should merge these into one set of get/set: > > > > SetParameters(RtpTransportParameters) > > RtpTransportParameters GetParameters() > > > > > > Where RtpTransportParameters has RtcpParameters: > > > > struct RtpTrnsportParameters { > > RtcpParameters rtcp; > > RtpKeepAliveParameters keepalive; > > } > > I pondered that, but then we're mixing a transport level feature with a > stream-level one. It got strange to be forced to pass along unused > RtcpParameters in places were just the RtpTransportParameters made sense. > Having a third "RtpRtcpParameters" or whatever struct joining these two also > feels a bit overkill. Wait, what feature here is stream-level? The keepalives? But I thought the reason we're putting it here (as opposed to on RtpSender) is that it's conceptually transport-level. I'd generally agree with Peter about merging the two structs. It's more consistent with the ORTC programming model, where if you need to change something you're expected to do "getParameters, modify stuff, setParameters". And all the objects defined thus far have used a single "parameters" structure. This seems sensible to me, and consistent with other things: struct RtpTransportParameters { RtcpParameters rtcp; RtpKeepAliveConfig keepalive_config; };
On 2017/07/12 23:15:50, Taylor Brandstetter wrote: > https://codereview.webrtc.org/2981513002/diff/20001/webrtc/api/ortc/rtptransp... > File webrtc/api/ortc/rtptransportinterface.h (right): > > https://codereview.webrtc.org/2981513002/diff/20001/webrtc/api/ortc/rtptransp... > webrtc/api/ortc/rtptransportinterface.h:104: virtual RtpTransportParameters > GetRtpTransportParameters() const = 0; > On 2017/07/11 19:54:05, sprang_webrtc wrote: > > On 2017/07/11 18:25:09, pthatcher1 wrote: > > > I think we should merge these into one set of get/set: > > > > > > SetParameters(RtpTransportParameters) > > > RtpTransportParameters GetParameters() > > > > > > > > > Where RtpTransportParameters has RtcpParameters: > > > > > > struct RtpTrnsportParameters { > > > RtcpParameters rtcp; > > > RtpKeepAliveParameters keepalive; > > > } > > > > I pondered that, but then we're mixing a transport level feature with a > > stream-level one. It got strange to be forced to pass along unused > > RtcpParameters in places were just the RtpTransportParameters made sense. > > Having a third "RtpRtcpParameters" or whatever struct joining these two also > > feels a bit overkill. > > Wait, what feature here is stream-level? The keepalives? But I thought the > reason we're putting it here (as opposed to on RtpSender) is that it's > conceptually transport-level. The RtcpParameters. > > I'd generally agree with Peter about merging the two structs. It's more > consistent with the ORTC programming model, where if you need to change > something you're expected to do "getParameters, modify stuff, setParameters". > And all the objects defined thus far have used a single "parameters" structure. > > This seems sensible to me, and consistent with other things: > > struct RtpTransportParameters { > RtcpParameters rtcp; > RtpKeepAliveConfig keepalive_config; > };
I just talked to Taylor about it, and he corrected me that RtcpParameters are transport-level. And since this new keep alive thing is transport-level. So nothing is stream-level, and I go back to my original argument that we should combine them into one RtpTransportParameter that combines RTCP and RTP keep alives and has one getter and setter.
sprang@webrtc.org changed reviewers: + ilnik@webrtc.org
Moved RtcpParameters into RtpTransportParameters. I also propagate that struct further. Maybe I took that a bit too far now? +ilnik, could you work with pthatcher@ and deadbeef@ in getting this into a landable state while I'm OOO? https://codereview.webrtc.org/2981513002/diff/40001/webrtc/call/call.h File webrtc/call/call.h (right): https://codereview.webrtc.org/2981513002/diff/40001/webrtc/call/call.h#newcod... webrtc/call/call.h:209: // only before creating and send streams. Returns true on success. On 2017/07/12 16:19:50, Taylor Brandstetter wrote: > I think "and" was meant to be "any" here Done. https://codereview.webrtc.org/2981513002/diff/40001/webrtc/ortc/rtptransporta... File webrtc/ortc/rtptransportadapter.cc (right): https://codereview.webrtc.org/2981513002/diff/40001/webrtc/ortc/rtptransporta... webrtc/ortc/rtptransportadapter.cc:192: rtp_transport_controller_->SetRtpTransportParameters(parameters); On 2017/07/12 16:19:50, Taylor Brandstetter wrote: > Ok, so setting RTP transport parameters on one RtpTransport will affect the > entire transport controller (in other words, the whole Call)? This limitation > should be called out in the applicable header file(s) in webrtc/api/ortc/. That was the idea, since the keep-alive is about keeping the underlying transport (ie udp socket) alive, rather than and individual rtp transport. Hope I'm not confusing terminology here. Right now, the implementation is handling the keep-alive on a stream-basis, but ultimately it would probably be better to do it on a call basis, that's why I moved the config to webrtc::Call::Config Do you think it would make more sense to move it back? https://codereview.webrtc.org/2981513002/diff/40001/webrtc/ortc/rtptransporta... File webrtc/ortc/rtptransportadapter.h (right): https://codereview.webrtc.org/2981513002/diff/40001/webrtc/ortc/rtptransporta... webrtc/ortc/rtptransportadapter.h:88: RtpTransportAdapter(const RtcpParameters& rtcp_parameters, On 2017/07/12 16:19:50, Taylor Brandstetter wrote: > Since the constructor (and factory methods) take RtcpParameters, I think they > should take the RtpTransportParameters as well for consistency. Maybe for the > keepalive options it doesn't matter, but it seems likely we could add things to > RtpTransportParameters in the future which we would want to have available at > construction time. Done.
lgtm https://codereview.webrtc.org/2981513002/diff/60001/webrtc/api/ortc/rtptransp... File webrtc/api/ortc/rtptransportinterface.h (right): https://codereview.webrtc.org/2981513002/diff/60001/webrtc/api/ortc/rtptransp... webrtc/api/ortc/rtptransportinterface.h:99: // altering the payload type of timeout interval after this point is not of timeout => or timeout
Looks great so far. Just needs some tests: - Test that the keepalive parameters get applied correctly at the "media engine" level, using a fake media engine to accomplish this. See for example "SetRtcpParametersAppliesParametersToMediaEngine". - Tests for the API contract related to the keepalive parameters, including testing that errors are thrown for things that are documented as unsupported. See for example "GetAndSetRtcpParameters". https://codereview.webrtc.org/2981513002/diff/40001/webrtc/ortc/rtptransporta... File webrtc/ortc/rtptransportadapter.cc (right): https://codereview.webrtc.org/2981513002/diff/40001/webrtc/ortc/rtptransporta... webrtc/ortc/rtptransportadapter.cc:192: rtp_transport_controller_->SetRtpTransportParameters(parameters); On 2017/07/16 09:34:03, sprang_webrtc wrote: > On 2017/07/12 16:19:50, Taylor Brandstetter wrote: > > Ok, so setting RTP transport parameters on one RtpTransport will affect the > > entire transport controller (in other words, the whole Call)? This limitation > > should be called out in the applicable header file(s) in webrtc/api/ortc/. > > That was the idea, since the keep-alive is about keeping the underlying > transport (ie udp socket) alive, rather than and individual rtp transport. Hope > I'm not confusing terminology here. > Right now, the implementation is handling the keep-alive on a stream-basis, but > ultimately it would probably be better to do it on a call basis, that's why I > moved the config to webrtc::Call::Config > > Do you think it would make more sense to move it back? We may be confusing terminology. Here's what the objects in this API represent, conceptually: RtpTransport = RTP transport layer on top of one underlying 5-tuple (or ICE component, if using ICE). Equates to a "media transport" as defined in RFC7656. RtpTransportController = Controlling multiple RTP transports between the same pair of "endpoints". Some things, like bandwidth estimation and A/V sync, operate at this level. Most closely equates to a "multimedia session" as defined in RFC7656, or a PeerConnection in the WebRTC 1.0 object model. So "Call" is conceptually more like RtpTransportController. It encompasses *multiple* underlying transports (when BUNDLE is not used). There's unfortunately not an object that maps easily to "RtpTransport" at the "Call" level. The closest thing is RtpStreamReceiverController, but that's only for the receive side. So, I'd be fine with what you've done, as long as we call out the limitation. Or you could move the config out of webrtc::Call::Config, and leave it per-stream for now, until we have a nice "transport" abstraction to use. https://codereview.webrtc.org/2981513002/diff/60001/webrtc/api/ortc/ortcfacto... File webrtc/api/ortc/ortcfactoryinterface.h (right): https://codereview.webrtc.org/2981513002/diff/60001/webrtc/api/ortc/ortcfacto... webrtc/api/ortc/ortcfactoryinterface.h:118: // later through SetRtpTransportParameters. The method ended up just being called SetParameters (at Peter's suggestion) https://codereview.webrtc.org/2981513002/diff/60001/webrtc/api/ortc/ortcfacto... webrtc/api/ortc/ortcfactoryinterface.h:125: const RtpTransportParameters& rtcp_parameters, Need to replace "rtcp_parameters" with "rtp_parameters" here and elsewhere. https://codereview.webrtc.org/2981513002/diff/60001/webrtc/ortc/rtptransportc... File webrtc/ortc/rtptransportcontrolleradapter.h (right): https://codereview.webrtc.org/2981513002/diff/60001/webrtc/ortc/rtptransportc... webrtc/ortc/rtptransportcontrolleradapter.h:105: RTCError SetRtpTransportParameters(const RtpTransportParameters& parameters); Do we need two methods here or just one?
I have implemented the comments. Now working on tests - there are some problems. How can I on a level of rtptransport_unittest force some streams to be created? I've experimented around, but somehow, no streams are created. Also, I can't upload anything to this issue, so my patch is there: https://codereview.webrtc.org/2984793002 I've added you both as reviewers there too. https://codereview.webrtc.org/2981513002/diff/40001/webrtc/ortc/rtptransporta... File webrtc/ortc/rtptransportadapter.cc (right): https://codereview.webrtc.org/2981513002/diff/40001/webrtc/ortc/rtptransporta... webrtc/ortc/rtptransportadapter.cc:192: rtp_transport_controller_->SetRtpTransportParameters(parameters); On 2017/07/17 23:13:11, Taylor Brandstetter wrote: > On 2017/07/16 09:34:03, sprang_webrtc wrote: > > On 2017/07/12 16:19:50, Taylor Brandstetter wrote: > > > Ok, so setting RTP transport parameters on one RtpTransport will affect the > > > entire transport controller (in other words, the whole Call)? This > limitation > > > should be called out in the applicable header file(s) in webrtc/api/ortc/. > > > > That was the idea, since the keep-alive is about keeping the underlying > > transport (ie udp socket) alive, rather than and individual rtp transport. > Hope > > I'm not confusing terminology here. > > Right now, the implementation is handling the keep-alive on a stream-basis, > but > > ultimately it would probably be better to do it on a call basis, that's why I > > moved the config to webrtc::Call::Config > > > > Do you think it would make more sense to move it back? > > We may be confusing terminology. > > Here's what the objects in this API represent, conceptually: > > RtpTransport = RTP transport layer on top of one underlying 5-tuple (or ICE > component, if using ICE). Equates to a "media transport" as defined in RFC7656. > > RtpTransportController = Controlling multiple RTP transports between the same > pair of "endpoints". Some things, like bandwidth estimation and A/V sync, > operate at this level. Most closely equates to a "multimedia session" as defined > in RFC7656, or a PeerConnection in the WebRTC 1.0 object model. > > So "Call" is conceptually more like RtpTransportController. It encompasses > *multiple* underlying transports (when BUNDLE is not used). > > There's unfortunately not an object that maps easily to "RtpTransport" at the > "Call" level. The closest thing is RtpStreamReceiverController, but that's only > for the receive side. So, I'd be fine with what you've done, as long as we call > out the limitation. Or you could move the config out of webrtc::Call::Config, > and leave it per-stream for now, until we have a nice "transport" abstraction to > use. Done. https://codereview.webrtc.org/2981513002/diff/40001/webrtc/pc/rtptransport.cc File webrtc/pc/rtptransport.cc (right): https://codereview.webrtc.org/2981513002/diff/40001/webrtc/pc/rtptransport.cc... webrtc/pc/rtptransport.cc:140: if (rtp_transport_parameters_.keepalive.timeout_interval_ms > 0 && On 2017/07/12 16:19:50, Taylor Brandstetter wrote: > The parameters in this class don't appear to be used. So it may be better to > leave this method as a TODO and just return an error, so people don't wrongly > assume things are wired up already. > > Note that this class is currently only used in the PeerConnection object model > (hence it being in "pc/"), but it's what we plan to eventually use for both the > ORTC and PeerConnection code paths, replacing "RtpTransportAdapter" with it. Acknowledged. https://codereview.webrtc.org/2981513002/diff/60001/webrtc/api/ortc/ortcfacto... File webrtc/api/ortc/ortcfactoryinterface.h (right): https://codereview.webrtc.org/2981513002/diff/60001/webrtc/api/ortc/ortcfacto... webrtc/api/ortc/ortcfactoryinterface.h:118: // later through SetRtpTransportParameters. On 2017/07/17 23:13:11, Taylor Brandstetter wrote: > The method ended up just being called SetParameters (at Peter's suggestion) Done. https://codereview.webrtc.org/2981513002/diff/60001/webrtc/api/ortc/ortcfacto... webrtc/api/ortc/ortcfactoryinterface.h:125: const RtpTransportParameters& rtcp_parameters, On 2017/07/17 23:13:11, Taylor Brandstetter wrote: > Need to replace "rtcp_parameters" with "rtp_parameters" here and elsewhere. Done. https://codereview.webrtc.org/2981513002/diff/60001/webrtc/api/ortc/rtptransp... File webrtc/api/ortc/rtptransportinterface.h (right): https://codereview.webrtc.org/2981513002/diff/60001/webrtc/api/ortc/rtptransp... webrtc/api/ortc/rtptransportinterface.h:99: // altering the payload type of timeout interval after this point is not On 2017/07/17 22:43:27, pthatcher1 wrote: > of timeout => or timeout Done. https://codereview.webrtc.org/2981513002/diff/60001/webrtc/ortc/rtptransportc... File webrtc/ortc/rtptransportcontrolleradapter.h (right): https://codereview.webrtc.org/2981513002/diff/60001/webrtc/ortc/rtptransportc... webrtc/ortc/rtptransportcontrolleradapter.h:105: RTCError SetRtpTransportParameters(const RtpTransportParameters& parameters); On 2017/07/17 23:13:11, Taylor Brandstetter wrote: > Do we need two methods here or just one? I think one method is enough here. Removed SetRtcpParameters and moved it's code to the SetRtpTransportParameters.
On 2017/07/21 12:21:19, ilnik wrote: > I have implemented the comments. > > Now working on tests - there are some problems. > How can I on a level of rtptransport_unittest force some streams to be created? > I've experimented around, but somehow, no streams are created. > You need to create an RtpSender prepare it for sending to cause a send stream to be created. For example: auto sender = ortc_factory_->CreateRtpSender(cricket::MEDIA_TYPE_AUDIO, rtp_transport_.get()); ASSERT_NE(nullptr, sender.get()); ASSERT_TRUE(sender->Send(MakeMinimalVp8Parameters().ok());
On 2017/07/21 17:02:17, Taylor Brandstetter wrote: > On 2017/07/21 12:21:19, ilnik wrote: > > I have implemented the comments. > > > > Now working on tests - there are some problems. > > How can I on a level of rtptransport_unittest force some streams to be > created? > > I've experimented around, but somehow, no streams are created. > > > > You need to create an RtpSender prepare it for sending to cause a send stream to > be created. For example: > > auto sender = ortc_factory_->CreateRtpSender(cricket::MEDIA_TYPE_AUDIO, > rtp_transport_.get()); > ASSERT_NE(nullptr, sender.get()); > ASSERT_TRUE(sender->Send(MakeMinimalVp8Parameters().ok()); Strange... in the test, there's already rtpsender created like that for video and send called (lines 239-245 in rtptransport_unittest.cc), yet underlying call have absolutely no streams (then second SetParameters gets down to rtptransportcontrolleradapter.cc:224, the call_ doesn't have any streams and keepalive settings are changed).
On 2017/07/21 17:33:23, ilnik wrote: > On 2017/07/21 17:02:17, Taylor Brandstetter wrote: > > On 2017/07/21 12:21:19, ilnik wrote: > > > I have implemented the comments. > > > > > > Now working on tests - there are some problems. > > > How can I on a level of rtptransport_unittest force some streams to be > > created? > > > I've experimented around, but somehow, no streams are created. > > > > > > > You need to create an RtpSender prepare it for sending to cause a send stream > to > > be created. For example: > > > > auto sender = ortc_factory_->CreateRtpSender(cricket::MEDIA_TYPE_AUDIO, > > rtp_transport_.get()); > > ASSERT_NE(nullptr, sender.get()); > > ASSERT_TRUE(sender->Send(MakeMinimalVp8Parameters().ok()); > > Strange... in the test, there's already rtpsender created like that for video > and send called (lines 239-245 in rtptransport_unittest.cc), yet underlying call > have absolutely no streams (then second SetParameters gets down to > rtptransportcontrolleradapter.cc:224, the call_ doesn't have any streams and > keepalive settings are changed). Oh, the issue is that we're verifying the application of parameters by using "FakeMediaEngine". But there's still a real "Call", which ends up not getting used at all aside from the SetRtpKeepaliveParameters method. So, we'll need to either: 1. Move the API off of "Call" (which was suggested earlier for other reasons). 2. Add a way to test with a FakeCall. Wouldn't be too much work, just need a new test-only "OrtcFactory::Create" method. 3. Just leave this untested until we've done more refactoring (pulling RTP stuff out of Call).
Had to do some unpleasant hacking to get the test going, but at least it's working now... ptal
https://codereview.webrtc.org/2981513002/diff/100001/webrtc/ortc/rtptransport... File webrtc/ortc/rtptransport_unittest.cc (right): https://codereview.webrtc.org/2981513002/diff/100001/webrtc/ortc/rtptransport... webrtc/ortc/rtptransport_unittest.cc:228: // SetParameters should set keepalive for all rtp transports. nit: Capitalize "RTP" https://codereview.webrtc.org/2981513002/diff/100001/webrtc/ortc/rtptransport... webrtc/ortc/rtptransport_unittest.cc:230: TEST_F(RtpTransportTest, CantChangeKeepAliveAfterCreatedSendStreams) { From a "unit test" perspective, I'd think of this as testing two things: 1. That Call::SetRtpKeepAliveConfig fails if video send streams are created. 2. That RtpTransport returns INTERNAL_ERROR if Call::SetRtpKeepAliveConfig fails. #1 can be tested with a Call unit test, #2 can be tested here using a FakeCall, but the combination doesn't really belong here, since that's more of an integration test. Note the comment at the top: // This test uses fake packet transports and a fake media engine, in order to // test the RtpTransport at only an API level. Any end-to-end test should go in // ortcfactory_integrationtest.cc instead. I think we can avoid the need for this test in the first place though, if we make RtpTransport responsible for ensuring the keepalive settings don't change, and return INVALID_MODIFICATION. Then this test could work with just the fake media engine. https://codereview.webrtc.org/2981513002/diff/100001/webrtc/ortc/rtptransport... webrtc/ortc/rtptransport_unittest.cc:240: // Make sure real video channels are create, so that webrtc::Call code is nit: "are created" https://codereview.webrtc.org/2981513002/diff/100001/webrtc/ortc/rtptransport... webrtc/ortc/rtptransport_unittest.cc:250: // Need to explicitly call SetParameters() to actually set keepalive. Even though the keepalive parameters were passed into CreateRtpTransport? That's pretty unintuitive; can that be fixed? https://codereview.webrtc.org/2981513002/diff/100001/webrtc/ortc/rtptransport... webrtc/ortc/rtptransport_unittest.cc:268: Is there way to have another test that verifies the keepalive settings reach Call? See my comment from the previous round of review. "Just leave this untested until we've done more refactoring" is still a valid option. https://codereview.webrtc.org/2981513002/diff/100001/webrtc/ortc/rtptransport... File webrtc/ortc/rtptransportcontrolleradapter.h (right): https://codereview.webrtc.org/2981513002/diff/100001/webrtc/ortc/rtptransport... webrtc/ortc/rtptransportcontrolleradapter.h:103: // |parameters.keepalive| will be set for ALL rtp transports in the call. nit: Capitalize RTP.
https://codereview.webrtc.org/2981513002/diff/100001/webrtc/ortc/rtptransport... File webrtc/ortc/rtptransport_unittest.cc (right): https://codereview.webrtc.org/2981513002/diff/100001/webrtc/ortc/rtptransport... webrtc/ortc/rtptransport_unittest.cc:228: // SetParameters should set keepalive for all rtp transports. On 2017/08/02 00:12:56, Taylor Brandstetter wrote: > nit: Capitalize "RTP" Done. https://codereview.webrtc.org/2981513002/diff/100001/webrtc/ortc/rtptransport... webrtc/ortc/rtptransport_unittest.cc:230: TEST_F(RtpTransportTest, CantChangeKeepAliveAfterCreatedSendStreams) { On 2017/08/02 00:12:56, Taylor Brandstetter wrote: > From a "unit test" perspective, I'd think of this as testing two things: > > 1. That Call::SetRtpKeepAliveConfig fails if video send streams are created. > 2. That RtpTransport returns INTERNAL_ERROR if Call::SetRtpKeepAliveConfig > fails. > > #1 can be tested with a Call unit test, #2 can be tested here using a FakeCall, > but the combination doesn't really belong here, since that's more of an > integration test. Note the comment at the top: > > // This test uses fake packet transports and a fake media engine, in order to > // test the RtpTransport at only an API level. Any end-to-end test should go in > // ortcfactory_integrationtest.cc instead. > > I think we can avoid the need for this test in the first place though, if we > make RtpTransport responsible for ensuring the keepalive settings don't change, > and return INVALID_MODIFICATION. Then this test could work with just the fake > media engine. I've removed the "real" parts and added validation in RtpTransportControllerAdapter instead. This is a little clunky, but otherwise we can't tell if a stream has been created. https://codereview.webrtc.org/2981513002/diff/100001/webrtc/ortc/rtptransport... webrtc/ortc/rtptransport_unittest.cc:240: // Make sure real video channels are create, so that webrtc::Call code is On 2017/08/02 00:12:56, Taylor Brandstetter wrote: > nit: "are created" removed https://codereview.webrtc.org/2981513002/diff/100001/webrtc/ortc/rtptransport... webrtc/ortc/rtptransport_unittest.cc:250: // Need to explicitly call SetParameters() to actually set keepalive. On 2017/08/02 00:12:56, Taylor Brandstetter wrote: > Even though the keepalive parameters were passed into CreateRtpTransport? That's > pretty unintuitive; can that be fixed? Yes, wired that up now. https://codereview.webrtc.org/2981513002/diff/100001/webrtc/ortc/rtptransport... webrtc/ortc/rtptransport_unittest.cc:268: On 2017/08/02 00:12:56, Taylor Brandstetter wrote: > Is there way to have another test that verifies the keepalive settings reach > Call? See my comment from the previous round of review. "Just leave this > untested until we've done more refactoring" is still a valid option. The previous test with the real stream did (since RtpTransportControllerAdapter has an actual Call instance), but now it's a little more work. Do you want an ortc integration test? https://codereview.webrtc.org/2981513002/diff/100001/webrtc/ortc/rtptransport... File webrtc/ortc/rtptransportcontrolleradapter.h (right): https://codereview.webrtc.org/2981513002/diff/100001/webrtc/ortc/rtptransport... webrtc/ortc/rtptransportcontrolleradapter.h:103: // |parameters.keepalive| will be set for ALL rtp transports in the call. On 2017/08/02 00:12:56, Taylor Brandstetter wrote: > nit: Capitalize RTP. Done.
By the way, sorry if this code is confusing. I tried to make it as clear as I could, but as mentioned in various comments, it's all hacks on top of BaseChannel/MediaChannel, so it's going to be difficult to work with until that's fixed (which should hopefully be some time this year). https://codereview.webrtc.org/2981513002/diff/100001/webrtc/ortc/rtptransport... File webrtc/ortc/rtptransport_unittest.cc (right): https://codereview.webrtc.org/2981513002/diff/100001/webrtc/ortc/rtptransport... webrtc/ortc/rtptransport_unittest.cc:250: // Need to explicitly call SetParameters() to actually set keepalive. On 2017/08/02 16:45:08, sprang_webrtc wrote: > On 2017/08/02 00:12:56, Taylor Brandstetter wrote: > > Even though the keepalive parameters were passed into CreateRtpTransport? > That's > > pretty unintuitive; can that be fixed? > > Yes, wired that up now. Where is that wired up? I don't see it. I was thinking the most sensible thing would be to call RtpTransportControllerAdapter::SetRtpTransportParameters from RtpTransportAdapter::CreateProxied (bubbling up the error if it fails). Meaning, if you did: 1. controller = CreateRtpTransportController() 2. transport1 = CreateRtpTransport(keepalive_params_1, controller, ...) 3. sender1 = CreateRtpSender(track1, transport1, ...) 4. sender1.Send(...) 5. transport2 = CreateRtpTransport(keepalive_params_2, controller, ...) ... The second CreateRtpTransport would return an error, because it's implicitly trying to change the keepalive params for the Call after sending has started. https://codereview.webrtc.org/2981513002/diff/100001/webrtc/ortc/rtptransport... webrtc/ortc/rtptransport_unittest.cc:268: On 2017/08/02 16:45:08, sprang_webrtc wrote: > On 2017/08/02 00:12:56, Taylor Brandstetter wrote: > > Is there way to have another test that verifies the keepalive settings reach > > Call? See my comment from the previous round of review. "Just leave this > > untested until we've done more refactoring" is still a valid option. > > The previous test with the real stream did (since RtpTransportControllerAdapter > has an actual Call instance), but now it's a little more work. > > Do you want an ortc integration test? The previous test verifies that the setting can't be changed, but it doesn't verify that the correct value actually gets applied to the stream. If you don't think that's worth the effort to test, I'm ok with that. https://codereview.webrtc.org/2981513002/diff/120001/webrtc/api/ortc/rtptrans... File webrtc/api/ortc/rtptransportinterface.h (right): https://codereview.webrtc.org/2981513002/diff/120001/webrtc/api/ortc/rtptrans... webrtc/api/ortc/rtptransportinterface.h:98: // RTP keep-alive settings need to be set before creating any send-streams, nit: Someone using this API probably doesn't know what a "send stream" is; maybe "before an RtpSender has started sending" instead (if that's accurate)? https://codereview.webrtc.org/2981513002/diff/120001/webrtc/ortc/ortcfactory.h File webrtc/ortc/ortcfactory.h (right): https://codereview.webrtc.org/2981513002/diff/120001/webrtc/ortc/ortcfactory.... webrtc/ortc/ortcfactory.h:47: CreateRtpTransportController(const RtpTransportParameters& parameters); We shouldn't add this method; the fact that RtpTransportControllerAdapter knows about RtpTransportParameters is an implementation detail. From an API perspective, each RtpTransport has its own RtpTransportParameters, just like how each RtpSender/RtpReceiver has its own RtpParameters. https://codereview.webrtc.org/2981513002/diff/120001/webrtc/ortc/rtptransport... File webrtc/ortc/rtptransportcontrolleradapter.cc (right): https://codereview.webrtc.org/2981513002/diff/120001/webrtc/ortc/rtptransport... webrtc/ortc/rtptransportcontrolleradapter.cc:232: RTC_DCHECK(call_->SetRtpKeepAliveConfig(parameters.keepalive)); If this is in a DCHECK, it will only be executed in debug builds, right? https://codereview.webrtc.org/2981513002/diff/120001/webrtc/ortc/rtptransport... File webrtc/ortc/rtptransportcontrolleradapter.h (right): https://codereview.webrtc.org/2981513002/diff/120001/webrtc/ortc/rtptransport... webrtc/ortc/rtptransportcontrolleradapter.h:105: RTCError SetRtpTransportAndParameters( Can remove "And" from this name. https://codereview.webrtc.org/2981513002/diff/120001/webrtc/pc/rtptransport.cc File webrtc/pc/rtptransport.cc (right): https://codereview.webrtc.org/2981513002/diff/120001/webrtc/pc/rtptransport.c... webrtc/pc/rtptransport.cc:126: "RTP keep-alive parameters not supported by this channel."); Why is this message changing? https://codereview.webrtc.org/2981513002/diff/120001/webrtc/pc/rtptransport_u... File webrtc/pc/rtptransport_unittest.cc (right): https://codereview.webrtc.org/2981513002/diff/120001/webrtc/pc/rtptransport_u... webrtc/pc/rtptransport_unittest.cc:44: TEST(RtpTransportTest, SetRtpTransportKeepAliveNotSupported) { Can you add a comment explaining that this is a limitation of the current implementation, and may be fixed later?
> By the way, sorry if this code is confusing. I tried to make it as clear as I > could, but as mentioned in various comments, it's all hacks on top of > BaseChannel/MediaChannel, so it's going to be difficult to work with until > that's fixed (which should hopefully be some time this year). > > https://codereview.webrtc.org/2981513002/diff/100001/webrtc/ortc/rtptransport... > File webrtc/ortc/rtptransport_unittest.cc (right): > > https://codereview.webrtc.org/2981513002/diff/100001/webrtc/ortc/rtptransport... > webrtc/ortc/rtptransport_unittest.cc:250: // Need to explicitly call > SetParameters() to actually set keepalive. > On 2017/08/02 16:45:08, sprang_webrtc wrote: > > On 2017/08/02 00:12:56, Taylor Brandstetter wrote: > > > Even though the keepalive parameters were passed into CreateRtpTransport? > > That's > > > pretty unintuitive; can that be fixed? > > > > Yes, wired that up now. > > Where is that wired up? I don't see it. > > I was thinking the most sensible thing would be to call > RtpTransportControllerAdapter::SetRtpTransportParameters from > RtpTransportAdapter::CreateProxied (bubbling up the error if it fails). That makes sense. I did the wireup via RtpTransportControllerAdapter ctor and Init() call. This had the benefit of allowing construction time parameters for the Call instance, but has other problems. Removed it a changed so we set parameters through the proper setters when we create the proxied instead. > Meaning, if you did: > > 1. controller = CreateRtpTransportController() > 2. transport1 = CreateRtpTransport(keepalive_params_1, controller, ...) > 3. sender1 = CreateRtpSender(track1, transport1, ...) > 4. sender1.Send(...) > 5. transport2 = CreateRtpTransport(keepalive_params_2, controller, ...) > > ... The second CreateRtpTransport would return an error, because it's implicitly > trying to change the keepalive params for the Call after sending has started. > Creating another transport (bound to the same controller) is now a different thing. I've added unit test for that as well. I don't think it even makes sense to allow creating another transport with new keepalive config even if there have been no sending started.
https://codereview.webrtc.org/2981513002/diff/120001/webrtc/api/ortc/rtptrans... File webrtc/api/ortc/rtptransportinterface.h (right): https://codereview.webrtc.org/2981513002/diff/120001/webrtc/api/ortc/rtptrans... webrtc/api/ortc/rtptransportinterface.h:98: // RTP keep-alive settings need to be set before creating any send-streams, On 2017/08/03 01:18:03, Taylor Brandstetter wrote: > nit: Someone using this API probably doesn't know what a "send stream" is; maybe > "before an RtpSender has started sending" instead (if that's accurate)? How about changing "send stream" to "media channel"? https://codereview.webrtc.org/2981513002/diff/120001/webrtc/ortc/ortcfactory.h File webrtc/ortc/ortcfactory.h (right): https://codereview.webrtc.org/2981513002/diff/120001/webrtc/ortc/ortcfactory.... webrtc/ortc/ortcfactory.h:47: CreateRtpTransportController(const RtpTransportParameters& parameters); On 2017/08/03 01:18:03, Taylor Brandstetter wrote: > We shouldn't add this method; the fact that RtpTransportControllerAdapter knows > about RtpTransportParameters is an implementation detail. From an API > perspective, each RtpTransport has its own RtpTransportParameters, just like how > each RtpSender/RtpReceiver has its own RtpParameters. True, that's why I didn't add it to the interface. What if we just make it private? I did this since it allows setting the call config on construction. https://codereview.webrtc.org/2981513002/diff/120001/webrtc/ortc/rtptransport... File webrtc/ortc/rtptransportcontrolleradapter.cc (right): https://codereview.webrtc.org/2981513002/diff/120001/webrtc/ortc/rtptransport... webrtc/ortc/rtptransportcontrolleradapter.cc:232: RTC_DCHECK(call_->SetRtpKeepAliveConfig(parameters.keepalive)); On 2017/08/03 01:18:03, Taylor Brandstetter wrote: > If this is in a DCHECK, it will only be executed in debug builds, right? Oops, right. Let's do a check instead then. https://codereview.webrtc.org/2981513002/diff/120001/webrtc/ortc/rtptransport... File webrtc/ortc/rtptransportcontrolleradapter.h (right): https://codereview.webrtc.org/2981513002/diff/120001/webrtc/ortc/rtptransport... webrtc/ortc/rtptransportcontrolleradapter.h:105: RTCError SetRtpTransportAndParameters( On 2017/08/03 01:18:03, Taylor Brandstetter wrote: > Can remove "And" from this name. Done. https://codereview.webrtc.org/2981513002/diff/120001/webrtc/pc/rtptransport.cc File webrtc/pc/rtptransport.cc (right): https://codereview.webrtc.org/2981513002/diff/120001/webrtc/pc/rtptransport.c... webrtc/pc/rtptransport.cc:126: "RTP keep-alive parameters not supported by this channel."); On 2017/08/03 01:18:04, Taylor Brandstetter wrote: > Why is this message changing? This is a new message. I just verify that you don't try to set keep-alive config, since it's not wired up with this implementation. https://codereview.webrtc.org/2981513002/diff/120001/webrtc/pc/rtptransport_u... File webrtc/pc/rtptransport_unittest.cc (right): https://codereview.webrtc.org/2981513002/diff/120001/webrtc/pc/rtptransport_u... webrtc/pc/rtptransport_unittest.cc:44: TEST(RtpTransportTest, SetRtpTransportKeepAliveNotSupported) { On 2017/08/03 01:18:04, Taylor Brandstetter wrote: > Can you add a comment explaining that this is a limitation of the current > implementation, and may be fixed later? Done.
lgtm, assuming nits about comments and error messages are addressed https://codereview.webrtc.org/2981513002/diff/120001/webrtc/api/ortc/rtptrans... File webrtc/api/ortc/rtptransportinterface.h (right): https://codereview.webrtc.org/2981513002/diff/120001/webrtc/api/ortc/rtptrans... webrtc/api/ortc/rtptransportinterface.h:98: // RTP keep-alive settings need to be set before creating any send-streams, On 2017/08/03 13:08:14, sprang_webrtc wrote: > On 2017/08/03 01:18:03, Taylor Brandstetter wrote: > > nit: Someone using this API probably doesn't know what a "send stream" is; > maybe > > "before an RtpSender has started sending" instead (if that's accurate)? > > How about changing "send stream" to "media channel"? Again, we don't define what a "media channel" is in this API. https://codereview.webrtc.org/2981513002/diff/120001/webrtc/ortc/ortcfactory.h File webrtc/ortc/ortcfactory.h (right): https://codereview.webrtc.org/2981513002/diff/120001/webrtc/ortc/ortcfactory.... webrtc/ortc/ortcfactory.h:47: CreateRtpTransportController(const RtpTransportParameters& parameters); On 2017/08/03 13:08:14, sprang_webrtc wrote: > On 2017/08/03 01:18:03, Taylor Brandstetter wrote: > > We shouldn't add this method; the fact that RtpTransportControllerAdapter > knows > > about RtpTransportParameters is an implementation detail. From an API > > perspective, each RtpTransport has its own RtpTransportParameters, just like > how > > each RtpSender/RtpReceiver has its own RtpParameters. > > True, that's why I didn't add it to the interface. What if we just make it > private? I did this since it allows setting the call config on construction. I don't see that allowing setting the call config on construction buys much, since you need to be able to set it after construction anyway, in the normal case where the RtpTransport is created after the RtpTransportController. I also can't find where this constructor is called (or even defined). https://codereview.webrtc.org/2981513002/diff/120001/webrtc/ortc/rtptransport... File webrtc/ortc/rtptransportcontrolleradapter.cc (right): https://codereview.webrtc.org/2981513002/diff/120001/webrtc/ortc/rtptransport... webrtc/ortc/rtptransportcontrolleradapter.cc:230: "media streams."); nit: Someone using this API may interpret "media stream" as referring to a MediaStream object, which isn't what this is talking about. "Stream" is a pretty ambiguous term for us, since it has different meanings in the contexts of WebRTC, SDP and RTP. Can you rephrase this message in the terminology used by this API? Which I think would be "RtpSender has started sending"? https://codereview.webrtc.org/2981513002/diff/120001/webrtc/pc/rtptransport.cc File webrtc/pc/rtptransport.cc (right): https://codereview.webrtc.org/2981513002/diff/120001/webrtc/pc/rtptransport.c... webrtc/pc/rtptransport.cc:126: "RTP keep-alive parameters not supported by this channel."); On 2017/08/03 13:08:14, sprang_webrtc wrote: > On 2017/08/03 01:18:04, Taylor Brandstetter wrote: > > Why is this message changing? > > This is a new message. I just verify that you don't try to set keep-alive > config, since it's not wired up with this implementation. Sorry, I thought this was RtpTransportAdapter when reviewing, carry on. https://codereview.webrtc.org/2981513002/diff/140001/webrtc/ortc/rtptransport... File webrtc/ortc/rtptransport_unittest.cc (right): https://codereview.webrtc.org/2981513002/diff/140001/webrtc/ortc/rtptransport... webrtc/ortc/rtptransport_unittest.cc:229: // It is impossible to modify keepalive parameters if any streams are created. Can you comment that this is an limitation of the existing implementation (since the keepalive settings go on Call, instead of a more granular object), and may be fixed later? https://codereview.webrtc.org/2981513002/diff/140001/webrtc/ortc/rtptransport... webrtc/ortc/rtptransport_unittest.cc:256: TEST_F(RtpTransportTest, KeepAliveMustBeSameAcrossTransportController) { Again, can you comment that this is a limitation of the existing implementation, and the test should be updated when it's fixed? Can you also document this limitation in one of the API headers? rtptransportinterface.h says: // RTP keep-alive settings need to be set before creating any media channels, // altering the payload type or timeout interval after this point is not // supported. But this doesn't talk about "must be same across controller". https://codereview.webrtc.org/2981513002/diff/140001/webrtc/ortc/rtptransport... webrtc/ortc/rtptransport_unittest.cc:272: // Update the parameters, and crate another transport for the same controller. nit: "crate"
The CQ bit was checked by sprang@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_ubsan on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_ubsan/builds/14365) mac_baremetal on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_baremetal/builds/23630) mac_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_rel/builds/27915)
The CQ bit was checked by sprang@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
https://codereview.webrtc.org/2981513002/diff/120001/webrtc/api/ortc/rtptrans... File webrtc/api/ortc/rtptransportinterface.h (right): https://codereview.webrtc.org/2981513002/diff/120001/webrtc/api/ortc/rtptrans... webrtc/api/ortc/rtptransportinterface.h:98: // RTP keep-alive settings need to be set before creating any send-streams, On 2017/08/03 16:50:36, Taylor Brandstetter wrote: > On 2017/08/03 13:08:14, sprang_webrtc wrote: > > On 2017/08/03 01:18:03, Taylor Brandstetter wrote: > > > nit: Someone using this API probably doesn't know what a "send stream" is; > > maybe > > > "before an RtpSender has started sending" instead (if that's accurate)? > > > > How about changing "send stream" to "media channel"? > > Again, we don't define what a "media channel" is in this API. Alright, "before an RtpSender has started sending" it is then. https://codereview.webrtc.org/2981513002/diff/140001/webrtc/ortc/rtptransport... File webrtc/ortc/rtptransport_unittest.cc (right): https://codereview.webrtc.org/2981513002/diff/140001/webrtc/ortc/rtptransport... webrtc/ortc/rtptransport_unittest.cc:229: // It is impossible to modify keepalive parameters if any streams are created. On 2017/08/03 16:50:36, Taylor Brandstetter wrote: > Can you comment that this is an limitation of the existing implementation (since > the keepalive settings go on Call, instead of a more granular object), and may > be fixed later? Done. https://codereview.webrtc.org/2981513002/diff/140001/webrtc/ortc/rtptransport... webrtc/ortc/rtptransport_unittest.cc:256: TEST_F(RtpTransportTest, KeepAliveMustBeSameAcrossTransportController) { On 2017/08/03 16:50:37, Taylor Brandstetter wrote: > Again, can you comment that this is a limitation of the existing implementation, > and the test should be updated when it's fixed? Can you also document this > limitation in one of the API headers? rtptransportinterface.h says: > > // RTP keep-alive settings need to be set before creating any media channels, > // altering the payload type or timeout interval after this point is not > // supported. > > But this doesn't talk about "must be same across controller". Done. https://codereview.webrtc.org/2981513002/diff/140001/webrtc/ortc/rtptransport... webrtc/ortc/rtptransport_unittest.cc:272: // Update the parameters, and crate another transport for the same controller. On 2017/08/03 16:50:37, Taylor Brandstetter wrote: > nit: "crate" Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_gcc_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_gcc_rel/builds/1057)
The CQ bit was checked by sprang@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_gcc_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_gcc_rel/builds/1059)
The CQ bit was checked by sprang@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_gcc_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_gcc_rel/builds/1061)
The CQ bit was checked by sprang@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by sprang@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from pthatcher@webrtc.org, deadbeef@webrtc.org Link to the patchset: https://codereview.webrtc.org/2981513002/#ps260001 (title: "Add workaround for gcc")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: presubmit on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/19896)
sprang@webrtc.org changed reviewers: + stefan@webrtc.org
+stefan, can you please review common_types.h and call/* ?
https://codereview.webrtc.org/2981513002/diff/260001/webrtc/ortc/rtptransport... File webrtc/ortc/rtptransportadapter.cc (right): https://codereview.webrtc.org/2981513002/diff/260001/webrtc/ortc/rtptransport... webrtc/ortc/rtptransportadapter.cc:84: std::move(params_result)); nit: I think you should be able to just return "std::move(params_result)". RTCErrorOr can be constructed implicitly from an RTCError&&, for exactly these kinds of situations.
https://codereview.webrtc.org/2981513002/diff/260001/webrtc/call/call.cc File webrtc/call/call.cc (right): https://codereview.webrtc.org/2981513002/diff/260001/webrtc/call/call.cc#newc... webrtc/call/call.cc:1148: // Can be called by RtpTransportController not on the configuration thread. This comment will become confusing. Call currently has an RtpTransportControllerSend object, which is what I'd expect that this refers to. Should this method be added to that class instead? That way the send streams can ask the RtpTransportControllerSend whether or not to enable keepalive. Later on, the RtpTransportController will be responsible for RTP and also keepalive. https://codereview.webrtc.org/2981513002/diff/260001/webrtc/call/call.h File webrtc/call/call.h (right): https://codereview.webrtc.org/2981513002/diff/260001/webrtc/call/call.h#newco... webrtc/call/call.h:210: virtual bool SetRtpKeepAliveConfig(const RtpKeepAliveConfig& config) = 0; Is there a reason why we have to set this after we create Call? Seems like a step in the wrong direction.
https://codereview.webrtc.org/2981513002/diff/260001/webrtc/ortc/rtptransport... File webrtc/ortc/rtptransportadapter.cc (right): https://codereview.webrtc.org/2981513002/diff/260001/webrtc/ortc/rtptransport... webrtc/ortc/rtptransportadapter.cc:84: std::move(params_result)); On 2017/08/07 19:59:36, Taylor Brandstetter wrote: > nit: I think you should be able to just return "std::move(params_result)". > RTCErrorOr can be constructed implicitly from an RTCError&&, for exactly these > kinds of situations. Right, thanks!
https://codereview.webrtc.org/2981513002/diff/260001/webrtc/call/call.cc File webrtc/call/call.cc (right): https://codereview.webrtc.org/2981513002/diff/260001/webrtc/call/call.cc#newc... webrtc/call/call.cc:1148: // Can be called by RtpTransportController not on the configuration thread. On 2017/08/08 08:13:06, stefan-webrtc wrote: > This comment will become confusing. Call currently has an > RtpTransportControllerSend object, which is what I'd expect that this refers to. > Should this method be added to that class instead? That way the send streams can > ask the RtpTransportControllerSend whether or not to enable keepalive. Later on, > the RtpTransportController will be responsible for RTP and also keepalive. OK, so this badly phrased comment referred to the RtpTransportControllerAdapter in the ortc api, which is not quite the same. I've updated that class to actually configure this on the configuration thread as well. So removed comment and added back thread check. https://codereview.webrtc.org/2981513002/diff/260001/webrtc/call/call.h File webrtc/call/call.h (right): https://codereview.webrtc.org/2981513002/diff/260001/webrtc/call/call.h#newco... webrtc/call/call.h:210: virtual bool SetRtpKeepAliveConfig(const RtpKeepAliveConfig& config) = 0; On 2017/08/08 08:13:06, stefan-webrtc wrote: > Is there a reason why we have to set this after we create Call? Seems like a > step in the wrong direction. I know, it was my intention to have this at construction time, but due to where the Call instance lives (in the RtpTransportControllerAdapter), it get's complicated as we want the parameters to be set on the RtpTransport interface of the ORTC API...
After offline discussion with stefan@, I've moved the keep-alive config away from (supposedly transport-agnostic) Call::Config and put in the fledgling RtpTransportControllerSendInterface instead. This should make it easier to wire it up for audio streams as well (?) and in the future make a truly transport-level feature. It however means I had to remove the Call unit tests, and now rely on the ORTC classes to verify that we don't unexpectedly update change settings. ptal
The CQ bit was checked by sprang@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
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/buil...) ios64_sim_ios10_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios64_sim_ios10_dbg/bui...) ios_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_dbg/builds/27885) linux_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_rel/builds/28321)
Patchset 17 lgtm.
This is much better IMO, thanks! :) One comment that I'd like to hear your opinion on before landing. https://codereview.webrtc.org/2981513002/diff/320001/webrtc/ortc/rtptransport... File webrtc/ortc/rtptransportcontrolleradapter.cc (right): https://codereview.webrtc.org/2981513002/diff/320001/webrtc/ortc/rtptransport... webrtc/ortc/rtptransportcontrolleradapter.cc:235: "transport controller."); Just a thought; if it's not possible to set after creating streams or transports, isn't it safe to simply recreate Call and the RtpTransportControllerSend when this API is called, instead of having a setter?
This is much better IMO, thanks! :) One comment that I'd like to hear your opinion on before landing. https://codereview.webrtc.org/2981513002/diff/320001/webrtc/ortc/rtptransport... File webrtc/ortc/rtptransportcontrolleradapter.cc (right): https://codereview.webrtc.org/2981513002/diff/320001/webrtc/ortc/rtptransport... webrtc/ortc/rtptransportcontrolleradapter.cc:235: "transport controller."); Just a thought; if it's not possible to set after creating streams or transports, isn't it safe to simply recreate Call and the RtpTransportControllerSend when this API is called, instead of having a setter?
The CQ bit was checked by sprang@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
https://codereview.webrtc.org/2981513002/diff/320001/webrtc/ortc/rtptransport... File webrtc/ortc/rtptransportcontrolleradapter.cc (right): https://codereview.webrtc.org/2981513002/diff/320001/webrtc/ortc/rtptransport... webrtc/ortc/rtptransportcontrolleradapter.cc:235: "transport controller."); On 2017/08/09 07:23:10, stefan-webrtc wrote: > Just a thought; if it's not possible to set after creating streams or > transports, isn't it safe to simply recreate Call and the > RtpTransportControllerSend when this API is called, instead of having a setter? Agree the would be nice (at least from the Call perspective), but seems to be somewhat in conflict with the ortc api and how it's used. See comment here https://codereview.webrtc.org/2981513002/diff/120001/webrtc/ortc/ortcfactory.... Maybe we can clean this up when the ortc implementation matures?
The CQ bit was unchecked by commit-bot@chromium.org
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/buil...) ios64_sim_ios10_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios64_sim_ios10_dbg/bui...) ios64_sim_ios9_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios64_sim_ios9_dbg/buil...) ios_arm64_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_dbg/builds/22584) ios_arm64_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_rel/builds/22363) mac_asan on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_asan/builds/26846) mac_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_rel/builds/27979)
On 2017/08/09 07:59:24, sprang_webrtc wrote: > https://codereview.webrtc.org/2981513002/diff/320001/webrtc/ortc/rtptransport... > File webrtc/ortc/rtptransportcontrolleradapter.cc (right): > > https://codereview.webrtc.org/2981513002/diff/320001/webrtc/ortc/rtptransport... > webrtc/ortc/rtptransportcontrolleradapter.cc:235: "transport controller."); > On 2017/08/09 07:23:10, stefan-webrtc wrote: > > Just a thought; if it's not possible to set after creating streams or > > transports, isn't it safe to simply recreate Call and the > > RtpTransportControllerSend when this API is called, instead of having a > setter? > > Agree the would be nice (at least from the Call perspective), but seems to be > somewhat in conflict with the ortc api and how it's used. See comment here > https://codereview.webrtc.org/2981513002/diff/120001/webrtc/ortc/ortcfactory.... > > Maybe we can clean this up when the ortc implementation matures? sgtm lgtm
The CQ bit was checked by sprang@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from pthatcher@webrtc.org, deadbeef@webrtc.org Link to the patchset: https://codereview.webrtc.org/2981513002/#ps380001 (title: "deps, again")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
CQ is committing da patch.
Bot data: {"patchset_id": 380001, "attempt_start_ts": 1502282757319950,
"parent_rev": "3e69e5c2c0450002a03be5184f28647efccbb47a", "commit_rev":
"db2a9fc6ec119052139d42f8897412bba7fa848b"}
Message was sent while issue was closed.
Description was changed from ========== Wire up RTP keep-alive in ortc api. [This CL is work in progress.] Wire up the rtp keep-alive in webrtc::Call::Config using new SetRtpTransportParameters() method on RtpTransportInterface. BUG=webrtc:7907 ========== to ========== Wire up RTP keep-alive in ortc api. [This CL is work in progress.] Wire up the rtp keep-alive in webrtc::Call::Config using new SetRtpTransportParameters() method on RtpTransportInterface. BUG=webrtc:7907 Review-Url: https://codereview.webrtc.org/2981513002 Cr-Commit-Position: refs/heads/master@{#19287} Committed: https://chromium.googlesource.com/external/webrtc/+/db2a9fc6ec119052139d42f88... ==========
Message was sent while issue was closed.
Committed patchset #20 (id:380001) as https://chromium.googlesource.com/external/webrtc/+/db2a9fc6ec119052139d42f88... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
