Wire up RTP keep-alive in ortc api.

webrtc/ortc/rtptransport_unittest.cc

Index: webrtc/ortc/rtptransport_unittest.cc
diff --git a/webrtc/ortc/rtptransport_unittest.cc b/webrtc/ortc/rtptransport_unittest.cc
index 2fc1b23586bfbe8c847b38e2e737689b31582dff..9e92e4f55e627fd897fe1ee88fd20414c94b1c7f 100644
--- a/webrtc/ortc/rtptransport_unittest.cc
+++ b/webrtc/ortc/rtptransport_unittest.cc
@@ -10,7 +10,9 @@
 
 #include <memory>
 
+#include "webrtc/api/test/fakeconstraints.h"
 #include "webrtc/media/base/fakemediaengine.h"
+#include "webrtc/media/base/fakevideocapturer.h"
 #include "webrtc/ortc/ortcfactory.h"
 #include "webrtc/ortc/testrtpparameters.h"
 #include "webrtc/p2p/base/fakepackettransport.h"
@@ -45,18 +47,17 @@ TEST_F(RtpTransportTest, GetPacketTransports) {
   rtc::FakePacketTransport rtp("rtp");
   rtc::FakePacketTransport rtcp("rtcp");
   // With muxed RTCP.
-  RtcpParameters rtcp_parameters;
-  rtcp_parameters.mux = true;
-  auto result = ortc_factory_->CreateRtpTransport(rtcp_parameters, &rtp,
-                                                  nullptr, nullptr);
+  RtpTransportParameters parameters;
+  parameters.rtcp.mux = true;
+  auto result =
+      ortc_factory_->CreateRtpTransport(parameters, &rtp, nullptr, nullptr);
   ASSERT_TRUE(result.ok());
   EXPECT_EQ(&rtp, result.value()->GetRtpPacketTransport());
   EXPECT_EQ(nullptr, result.value()->GetRtcpPacketTransport());
   result.MoveValue().reset();
   // With non-muxed RTCP.
-  rtcp_parameters.mux = false;
-  result =
-      ortc_factory_->CreateRtpTransport(rtcp_parameters, &rtp, &rtcp, nullptr);
+  parameters.rtcp.mux = false;
+  result = ortc_factory_->CreateRtpTransport(parameters, &rtp, &rtcp, nullptr);
   ASSERT_TRUE(result.ok());
   EXPECT_EQ(&rtp, result.value()->GetRtpPacketTransport());
   EXPECT_EQ(&rtcp, result.value()->GetRtcpPacketTransport());
@@ -70,16 +71,16 @@ TEST_F(RtpTransportTest, EnablingRtcpMuxingUnsetsRtcpTransport) {
   rtc::FakePacketTransport rtcp("rtcp");
 
   // Create non-muxed.
-  RtcpParameters rtcp_parameters;
-  rtcp_parameters.mux = false;
+  RtpTransportParameters parameters;
+  parameters.rtcp.mux = false;
   auto result =
-      ortc_factory_->CreateRtpTransport(rtcp_parameters, &rtp, &rtcp, nullptr);
+      ortc_factory_->CreateRtpTransport(parameters, &rtp, &rtcp, nullptr);
   ASSERT_TRUE(result.ok());
   auto rtp_transport = result.MoveValue();
 
   // Enable muxing.
-  rtcp_parameters.mux = true;
-  EXPECT_TRUE(rtp_transport->SetRtcpParameters(rtcp_parameters).ok());
+  parameters.rtcp.mux = true;
+  EXPECT_TRUE(rtp_transport->SetParameters(parameters).ok());
   EXPECT_EQ(nullptr, rtp_transport->GetRtcpPacketTransport());
 }
 
@@ -87,39 +88,39 @@ TEST_F(RtpTransportTest, GetAndSetRtcpParameters) {
   rtc::FakePacketTransport rtp("rtp");
   rtc::FakePacketTransport rtcp("rtcp");
   // Start with non-muxed RTCP.
-  RtcpParameters rtcp_parameters;
-  rtcp_parameters.mux = false;
-  rtcp_parameters.cname = "teST";
-  rtcp_parameters.reduced_size = false;
+  RtpTransportParameters parameters;
+  parameters.rtcp.mux = false;
+  parameters.rtcp.cname = "teST";
+  parameters.rtcp.reduced_size = false;
   auto result =
-      ortc_factory_->CreateRtpTransport(rtcp_parameters, &rtp, &rtcp, nullptr);
+      ortc_factory_->CreateRtpTransport(parameters, &rtp, &rtcp, nullptr);
   ASSERT_TRUE(result.ok());
   auto transport = result.MoveValue();
-  EXPECT_EQ(rtcp_parameters, transport->GetRtcpParameters());
+  EXPECT_EQ(parameters, transport->GetParameters());
 
   // Changing the CNAME is currently unsupported.
-  rtcp_parameters.cname = "different";
+  parameters.rtcp.cname = "different";
   EXPECT_EQ(RTCErrorType::UNSUPPORTED_OPERATION,
-            transport->SetRtcpParameters(rtcp_parameters).type());
-  rtcp_parameters.cname = "teST";
+            transport->SetParameters(parameters).type());
+  parameters.rtcp.cname = "teST";
 
   // Enable RTCP muxing and reduced-size RTCP.
-  rtcp_parameters.mux = true;
-  rtcp_parameters.reduced_size = true;
-  EXPECT_TRUE(transport->SetRtcpParameters(rtcp_parameters).ok());
-  EXPECT_EQ(rtcp_parameters, transport->GetRtcpParameters());
+  parameters.rtcp.mux = true;
+  parameters.rtcp.reduced_size = true;
+  EXPECT_TRUE(transport->SetParameters(parameters).ok());
+  EXPECT_EQ(parameters, transport->GetParameters());
 
   // Empty CNAME should result in the existing CNAME being used.
-  rtcp_parameters.cname.clear();
-  EXPECT_TRUE(transport->SetRtcpParameters(rtcp_parameters).ok());
-  EXPECT_EQ("teST", transport->GetRtcpParameters().cname);
+  parameters.rtcp.cname.clear();
+  EXPECT_TRUE(transport->SetParameters(parameters).ok());
+  EXPECT_EQ("teST", transport->GetParameters().rtcp.cname);
 
   // Disabling RTCP muxing after enabling shouldn't be allowed, since enabling
   // muxing should have made the RTP transport forget about the RTCP packet
   // transport initially passed into it.
-  rtcp_parameters.mux = false;
+  parameters.rtcp.mux = false;
   EXPECT_EQ(RTCErrorType::INVALID_STATE,
-            transport->SetRtcpParameters(rtcp_parameters).type());
+            transport->SetParameters(parameters).type());
 }
 
 // When Send or Receive is called on a sender or receiver, the RTCP parameters
@@ -129,12 +130,12 @@ TEST_F(RtpTransportTest, GetAndSetRtcpParameters) {
 TEST_F(RtpTransportTest, SendAndReceiveApplyRtcpParametersToMediaEngine) {
   // First, create video transport with reduced-size RTCP.
   rtc::FakePacketTransport fake_packet_transport1("1");
-  RtcpParameters rtcp_parameters;
-  rtcp_parameters.mux = true;
-  rtcp_parameters.reduced_size = true;
-  rtcp_parameters.cname = "foo";
+  RtpTransportParameters parameters;
+  parameters.rtcp.mux = true;
+  parameters.rtcp.reduced_size = true;
+  parameters.rtcp.cname = "foo";
   auto rtp_transport_result = ortc_factory_->CreateRtpTransport(
-      rtcp_parameters, &fake_packet_transport1, nullptr, nullptr);
+      parameters, &fake_packet_transport1, nullptr, nullptr);
   auto video_transport = rtp_transport_result.MoveValue();
 
   // Create video sender and call Send, expecting parameters to be applied.
@@ -163,10 +164,10 @@ TEST_F(RtpTransportTest, SendAndReceiveApplyRtcpParametersToMediaEngine) {
 
   // Create audio transport with non-reduced size RTCP.
   rtc::FakePacketTransport fake_packet_transport2("2");
-  rtcp_parameters.reduced_size = false;
-  rtcp_parameters.cname = "bar";
+  parameters.rtcp.reduced_size = false;
+  parameters.rtcp.cname = "bar";
   rtp_transport_result = ortc_factory_->CreateRtpTransport(
-      rtcp_parameters, &fake_packet_transport2, nullptr, nullptr);
+      parameters, &fake_packet_transport2, nullptr, nullptr);
   auto audio_transport = rtp_transport_result.MoveValue();
 
   // Create audio sender and call Send, expecting parameters to be applied.
@@ -195,17 +196,17 @@ TEST_F(RtpTransportTest, SendAndReceiveApplyRtcpParametersToMediaEngine) {
   EXPECT_FALSE(fake_voice_channel->recv_rtcp_parameters().reduced_size);
 }
 
-// When SetRtcpParameters is called, the modified parameters should be applied
+// When SetParameters is called, the modified parameters should be applied
 // to the media engine.
 // TODO(deadbeef): Once the implementation supports changing the CNAME,
 // test that here.
 TEST_F(RtpTransportTest, SetRtcpParametersAppliesParametersToMediaEngine) {
   rtc::FakePacketTransport fake_packet_transport("fake");
-  RtcpParameters rtcp_parameters;
-  rtcp_parameters.mux = true;
-  rtcp_parameters.reduced_size = false;
+  RtpTransportParameters parameters;
+  parameters.rtcp.mux = true;
+  parameters.rtcp.reduced_size = false;
   auto rtp_transport_result = ortc_factory_->CreateRtpTransport(
-      rtcp_parameters, &fake_packet_transport, nullptr, nullptr);
+      parameters, &fake_packet_transport, nullptr, nullptr);
   auto rtp_transport = rtp_transport_result.MoveValue();
 
   // Create video sender and call Send, applying an initial set of parameters.
@@ -215,8 +216,8 @@ TEST_F(RtpTransportTest, SetRtcpParametersAppliesParametersToMediaEngine) {
   EXPECT_TRUE(sender->Send(MakeMinimalVp8Parameters()).ok());
 
   // Modify parameters and expect them to be changed at the media engine level.
-  rtcp_parameters.reduced_size = true;
-  EXPECT_TRUE(rtp_transport->SetRtcpParameters(rtcp_parameters).ok());
+  parameters.rtcp.reduced_size = true;
+  EXPECT_TRUE(rtp_transport->SetParameters(parameters).ok());
 
   cricket::FakeVideoMediaChannel* fake_video_channel =
       fake_media_engine_->GetVideoChannel(0);
@@ -224,4 +225,45 @@ TEST_F(RtpTransportTest, SetRtcpParametersAppliesParametersToMediaEngine) {
   EXPECT_TRUE(fake_video_channel->send_rtcp_parameters().reduced_size);
 }
 
+// SetParameters should set keepalive for all rtp transports.

[Taylor Brandstetter, 2017/08/02 00:12:56]

nit: Capitalize "RTP"

[sprang_webrtc, 2017/08/02 16:45:08]

Done.

+// It is impossible to modify keepalive parameters if any streams are created.
+TEST_F(RtpTransportTest, CantChangeKeepAliveAfterCreatedSendStreams) {

[Taylor Brandstetter, 2017/08/02 00:12:56]

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.

[sprang_webrtc, 2017/08/02 16:45:08]

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.

+  rtc::FakePacketTransport fake_packet_transport("fake");
+  RtpTransportParameters parameters;
+  parameters.keepalive.timeout_interval_ms = 100;
+  auto rtp_transport_result = ortc_factory_->CreateRtpTransport(
+      parameters, &fake_packet_transport, nullptr, nullptr);
+  ASSERT_TRUE(rtp_transport_result.ok());
+  std::unique_ptr<RtpTransportInterface> rtp_transport =
+      rtp_transport_result.MoveValue();
+
+  // Make sure real video channels are create, so that webrtc::Call code is

[Taylor Brandstetter, 2017/08/02 00:12:56]

nit: "are created"

[sprang_webrtc, 2017/08/02 16:45:08]

removed

+  // properly excercised.
+  fake_media_engine_->GetVideoEngine()->SetCreateRealChannel(true);
+
+  // Create video sender and call Send, applying an initial set of parameters.
+  auto sender_result = ortc_factory_->CreateRtpSender(cricket::MEDIA_TYPE_VIDEO,
+                                                      rtp_transport.get());
+  ASSERT_TRUE(sender_result.ok());
+  std::unique_ptr<OrtcRtpSenderInterface> sender = sender_result.MoveValue();
+
+  // Need to explicitly call SetParameters() to actually set keepalive.

[Taylor Brandstetter, 2017/08/02 00:12:56]

Even though the keepalive parameters were passed into CreateRtpTransport? That's
pretty unintuitive; can that be fixed?

[sprang_webrtc, 2017/08/02 16:45:08]

Yes, wired that up now.

[Taylor Brandstetter, 2017/08/03 01:18:03]

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.

+  EXPECT_TRUE(rtp_transport->SetParameters(parameters).ok());
+  EXPECT_TRUE(sender->Send(MakeMinimalVp8Parameters()).ok());
+
+  std::unique_ptr<cricket::VideoCapturer> capturer(
+      new cricket::FakeVideoCapturer(false));
+  FakeConstraints constraints;
+  rtc::scoped_refptr<VideoTrackSourceInterface> source =
+      ortc_factory_->CreateVideoSource(std::move(capturer), &constraints);
+  rtc::scoped_refptr<VideoTrackInterface> video =
+      ortc_factory_->CreateVideoTrack("TestVideo", source);
+  sender->SetTrack(video);
+
+  // Modify parameters second time after video send stream created.
+  parameters.keepalive.timeout_interval_ms = 10;
+  EXPECT_EQ(RTCErrorType::INTERNAL_ERROR,
+            rtp_transport->SetParameters(parameters).type());
+}
+

[Taylor Brandstetter, 2017/08/02 00:12:56]

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.

[sprang_webrtc, 2017/08/02 16:45:08]

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?

[Taylor Brandstetter, 2017/08/03 01:18:03]

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.

 }  // namespace webrtc