Wire up RTP keep-alive in ortc api.

webrtc/ortc/rtptransport_unittest.cc

 /*
  *  Copyright 2017 The WebRTC project authors. All Rights Reserved.
  *
  *  Use of this source code is governed by a BSD-style license
  *  that can be found in the LICENSE file in the root of the source
  *  tree. An additional intellectual property rights grant can be found
  *  in the file PATENTS.  All contributing project authors may
  *  be found in the AUTHORS file in the root of the source tree.
  */
 
 #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"
 #include "webrtc/rtc_base/gunit.h"
 
 namespace webrtc {
 
 // 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.
@@ skipping 14 matching lines @@
   cricket::FakeMediaEngine* fake_media_engine_;
   std::unique_ptr<OrtcFactoryInterface> ortc_factory_;
 };
 
 // Test GetRtpPacketTransport and GetRtcpPacketTransport, with and without RTCP
 // muxing.
 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());
 }
 
 // If an RtpTransport starts out un-muxed and then starts muxing, the RTCP
 // packet transport should be forgotten and GetRtcpPacketTransport should
 // return null.
 TEST_F(RtpTransportTest, EnablingRtcpMuxingUnsetsRtcpTransport) {
   rtc::FakePacketTransport rtp("rtp");
   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());
 }
 
 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
 // from the RtpTransport underneath the sender should be applied to the created
 // media stream. The only relevant parameters (currently) are |cname| and
 // |reduced_size|.
 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.
   auto sender_result = ortc_factory_->CreateRtpSender(cricket::MEDIA_TYPE_VIDEO,
                                                       video_transport.get());
   auto video_sender = sender_result.MoveValue();
   EXPECT_TRUE(video_sender->Send(MakeMinimalVp8Parameters()).ok());
   cricket::FakeVideoMediaChannel* fake_video_channel =
       fake_media_engine_->GetVideoChannel(0);
   ASSERT_NE(nullptr, fake_video_channel);
   EXPECT_TRUE(fake_video_channel->send_rtcp_parameters().reduced_size);
   ASSERT_EQ(1u, fake_video_channel->send_streams().size());
   const cricket::StreamParams& video_send_stream =
       fake_video_channel->send_streams()[0];
   EXPECT_EQ("foo", video_send_stream.cname);
 
   // Create video receiver and call Receive, expecting parameters to be applied
   // (minus |cname|, since that's the sent cname, not received).
   auto receiver_result = ortc_factory_->CreateRtpReceiver(
       cricket::MEDIA_TYPE_VIDEO, video_transport.get());
   auto video_receiver = receiver_result.MoveValue();
   EXPECT_TRUE(
       video_receiver->Receive(MakeMinimalVp8ParametersWithSsrc(0xdeadbeef))
           .ok());
   EXPECT_TRUE(fake_video_channel->recv_rtcp_parameters().reduced_size);
 
   // 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.
   sender_result = ortc_factory_->CreateRtpSender(cricket::MEDIA_TYPE_AUDIO,
                                                  audio_transport.get());
   auto audio_sender = sender_result.MoveValue();
   EXPECT_TRUE(audio_sender->Send(MakeMinimalIsacParameters()).ok());
 
   cricket::FakeVoiceMediaChannel* fake_voice_channel =
       fake_media_engine_->GetVoiceChannel(0);
   ASSERT_NE(nullptr, fake_voice_channel);
   EXPECT_FALSE(fake_voice_channel->send_rtcp_parameters().reduced_size);
   ASSERT_EQ(1u, fake_voice_channel->send_streams().size());
   const cricket::StreamParams& audio_send_stream =
       fake_voice_channel->send_streams()[0];
   EXPECT_EQ("bar", audio_send_stream.cname);
 
   // Create audio receiver and call Receive, expecting parameters to be applied
   // (minus |cname|, since that's the sent cname, not received).
   receiver_result = ortc_factory_->CreateRtpReceiver(cricket::MEDIA_TYPE_AUDIO,
                                                      audio_transport.get());
   auto audio_receiver = receiver_result.MoveValue();
   EXPECT_TRUE(
       audio_receiver->Receive(MakeMinimalOpusParametersWithSsrc(0xbaadf00d))
           .ok());
   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.
   auto sender_result = ortc_factory_->CreateRtpSender(cricket::MEDIA_TYPE_VIDEO,
                                                       rtp_transport.get());
   auto sender = sender_result.MoveValue();
   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);
   ASSERT_NE(nullptr, fake_video_channel);
   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