Persist RTP state for FlexFEC.

webrtc/video/end_to_end_tests.cc

Index: webrtc/video/end_to_end_tests.cc
diff --git a/webrtc/video/end_to_end_tests.cc b/webrtc/video/end_to_end_tests.cc
index 3324bbf5f636f1ab66d8e06e4b09ae16d59b1111..424517f4b29e500a39be2fd3cb3abdaa7788affc 100644
--- a/webrtc/video/end_to_end_tests.cc
+++ b/webrtc/video/end_to_end_tests.cc
@@ -4145,6 +4145,169 @@ TEST_F(EndToEndTest, MAYBE_PictureIdStateRetainedAfterReinitingVp8) {
   TestPictureIdStatePreservation(encoder.get());
 }
 
+TEST_F(EndToEndTest, TestFlexfecRtpStatePreservation) {
+  class RtpSequenceObserver : public test::RtpRtcpObserver {
+   public:
+    RtpSequenceObserver()
+        : test::RtpRtcpObserver(kDefaultTimeoutMs),
+          num_flexfec_packets_sent_(0) {}
+
+    void ResetPacketCount() {
+      rtc::CritScope lock(&crit_);
+      num_flexfec_packets_sent_ = 0;
+    }
+
+   private:
+    Action OnSendRtp(const uint8_t* packet, size_t length) override {
+      rtc::CritScope lock(&crit_);
+
+      RTPHeader header;
+      EXPECT_TRUE(parser_->Parse(packet, length, &header));
+      const uint16_t sequence_number = header.sequenceNumber;
+      const uint32_t timestamp = header.timestamp;
+      const uint32_t ssrc = header.ssrc;
+
+      if (ssrc == kVideoSendSsrcs[0] || ssrc == kSendRtxSsrcs[0]) {
+        return SEND_PACKET;
+      }
+      EXPECT_EQ(kFlexfecSendSsrc, ssrc) << "Unknown SSRC sent.";
+
+      ++num_flexfec_packets_sent_;
+
+      // If this is the first packet, we have nothing to compare to.
+      if (!last_observed_sequence_number_) {
+        last_observed_sequence_number_.emplace(sequence_number);
+        last_observed_timestamp_.emplace(timestamp);
+
+        return SEND_PACKET;
+      }
+
+      // Verify continuity and monotonicity of RTP sequence numbers.

[danilchap, 2017/05/29 08:09:35]

what are testing with this expectation?
FakeNetworkPipe allowed (if configured) to reorder packets.

[brandtr, 2017/05/29 14:40:15]

I'm checking that the sequence number increments by exactly 1 between all sent
packets and that the RTP timestamp is not wildly varying. The observer is not
recreated when the VideoSendStream is, thus checking that the RTP state is
properly stored at the Call layer.

This callback is called before the fake network pipe, so reordering should not
be a problem.

+      EXPECT_EQ(static_cast<uint16_t>(*last_observed_sequence_number_ + 1),
+                sequence_number);
+      last_observed_sequence_number_.emplace(sequence_number);
+
+      // Verify that timestamps are reasonably close.
+      const int timestamp_abs_diff =
+          std::abs(static_cast<int>(timestamp) -
+                   static_cast<int>(*last_observed_timestamp_));

[danilchap, 2017/05/29 08:09:35]

what about timestamp wrap?
modules/module_common_types.h have functions to check which timestamp is newer,
base/mod_ops.h have more genertic functions to calculate difference with wrap in
mind

[brandtr, 2017/05/29 14:40:15]

Cool, I'll use module_common_types.h :)

+      EXPECT_LT(timestamp_abs_diff, 10 * 90000);

[danilchap, 2017/05/29 08:09:35]

prefer to use named constant instead of 90000.
May be name multiplication too:
k10Seconds = 10 * kVideo...Frequency;

[brandtr, 2017/05/29 14:40:15]

Done.

+      last_observed_timestamp_.emplace(timestamp);
+
+      // Pass test when enough packets have been let through.
+      if (num_flexfec_packets_sent_ >= 10) {
+        observation_complete_.Set();
+      }
+
+      return SEND_PACKET;
+    }
+
+    rtc::Optional<uint16_t> last_observed_sequence_number_ GUARDED_BY(crit_);
+    rtc::Optional<uint32_t> last_observed_timestamp_ GUARDED_BY(crit_);
+    size_t num_flexfec_packets_sent_ GUARDED_BY(crit_);
+    rtc::CriticalSection crit_;
+  } observer;
+
+  Call::Config config(event_log_.get());
+  CreateCalls(config, config);
+
+  FakeNetworkPipe::Config lossy_delayed_link;
+  lossy_delayed_link.loss_percent = 2;
+  lossy_delayed_link.queue_delay_ms = 50;
+  test::PacketTransport send_transport(sender_call_.get(), &observer,
+                                       test::PacketTransport::kSender,
+                                       payload_type_map_, lossy_delayed_link);
+  send_transport.SetReceiver(receiver_call_->Receiver());
+
+  FakeNetworkPipe::Config flawless_link;
+  test::PacketTransport receive_transport(nullptr, &observer,
+                                          test::PacketTransport::kReceiver,
+                                          payload_type_map_, flawless_link);
+  receive_transport.SetReceiver(sender_call_->Receiver());
+
+  // For reduced flakyness, we use a real VP8 encoder together with NACK
+  // and RTX.
+  const int kNumVideoStreams = 1;
+  const int kNumFlexfecStreams = 1;
+  CreateSendConfig(kNumVideoStreams, 0, kNumFlexfecStreams, &send_transport);
+  std::unique_ptr<VideoEncoder> encoder(VP8Encoder::Create());
+  video_send_config_.encoder_settings.encoder = encoder.get();
+  video_send_config_.encoder_settings.payload_name = "VP8";
+  video_send_config_.encoder_settings.payload_type = kVideoSendPayloadType;
+  video_send_config_.rtp.nack.rtp_history_ms = kNackRtpHistoryMs;
+  video_send_config_.rtp.rtx.ssrcs.push_back(kSendRtxSsrcs[0]);
+  video_send_config_.rtp.rtx.payload_type = kSendRtxPayloadType;
+
+  CreateMatchingReceiveConfigs(&receive_transport);
+  video_receive_configs_[0].rtp.nack.rtp_history_ms = kNackRtpHistoryMs;
+  video_receive_configs_[0].rtp.rtx_ssrc = kSendRtxSsrcs[0];
+  video_receive_configs_[0].rtp.rtx_payload_types[kVideoSendPayloadType] =
+      kSendRtxPayloadType;
+
+  // The matching FlexFEC receive config is not created by
+  // CreateMatchingReceiveConfigs since this is not a test::BaseTest.
+  // Set up the receive config manually instead.
+  FlexfecReceiveStream::Config flexfec_receive_config(&receive_transport);
+  flexfec_receive_config.payload_type =
+      video_send_config_.rtp.flexfec.payload_type;
+  flexfec_receive_config.remote_ssrc = video_send_config_.rtp.flexfec.ssrc;
+  flexfec_receive_config.protected_media_ssrcs =
+      video_send_config_.rtp.flexfec.protected_media_ssrcs;
+  flexfec_receive_config.local_ssrc = kReceiverLocalVideoSsrc;
+  flexfec_receive_config.transport_cc = true;
+  flexfec_receive_config.rtp_header_extensions.push_back(

[danilchap, 2017/05/29 08:09:35]

there is shortcut for pattern std::vector<T>::push_back(T(a, b)):
std::vector<T>::emplace(a, b);

[brandtr, 2017/05/29 14:40:14]

Done.

+      RtpExtension(RtpExtension::kTransportSequenceNumberUri,
+                   test::kTransportSequenceNumberExtensionId));
+  flexfec_receive_configs_.push_back(flexfec_receive_config);
+
+  CreateFlexfecStreams();
+  CreateVideoStreams();
+
+  // RTCP might be disabled if the network is "down".
+  sender_call_->SignalChannelNetworkState(MediaType::VIDEO, kNetworkUp);
+  receiver_call_->SignalChannelNetworkState(MediaType::VIDEO, kNetworkUp);
+
+  const int kFrameMaxWidth = 320;
+  const int kFrameMaxHeight = 180;
+  const int kFrameRate = 15;
+  CreateFrameGeneratorCapturer(kFrameRate, kFrameMaxWidth, kFrameMaxHeight);
+
+  // Initial test.
+  Start();
+  EXPECT_TRUE(observer.Wait()) << "Timed out waiting for packets.";
+
+  // Ensure monotonicity when the VideoSendStream is restarted.
+  auto restart_video_send_stream_and_test = [this, &observer]() {
+    Stop();
+    observer.ResetPacketCount();
+    Start();
+    EXPECT_TRUE(observer.Wait()) << "Timed out waiting for packets.";
+  };
+  restart_video_send_stream_and_test();

[danilchap, 2017/05/29 08:09:35]

is it a simple loop?
for (size_t i = 0; i < 2; ++i) {
  lambda body.
}

[brandtr, 2017/05/29 14:40:14]

Yes. I find this a bit easier to read though.

+  restart_video_send_stream_and_test();
+
+  // Ensure monotonicity when the VideoSendStream is recreated.
+  auto recreate_video_send_stream_and_test = [this, &observer]() {
+    frame_generator_capturer_->Stop();
+    sender_call_->DestroyVideoSendStream(video_send_stream_);
+    video_send_stream_ = sender_call_->CreateVideoSendStream(
+        video_send_config_.Copy(), video_encoder_config_.Copy());
+    video_send_stream_->Start();
+    CreateFrameGeneratorCapturer(kFrameRate, kFrameMaxWidth, kFrameMaxHeight);
+    frame_generator_capturer_->Start();
+    observer.ResetPacketCount();
+    EXPECT_TRUE(observer.Wait()) << "Timed out waiting for packets.";
+  };
+  recreate_video_send_stream_and_test();
+  recreate_video_send_stream_and_test();
+
+  // Cleanup.
+  send_transport.StopSending();
+  receive_transport.StopSending();
+  Stop();
+  DestroyStreams();
+}
+
 TEST_F(EndToEndTest,
        MAYBE_PictureIdStateRetainedAfterReinitingSimulcastEncoderAdapter) {
   class VideoEncoderFactoryAdapter : public webrtc::VideoEncoderFactory {