Reuse allocated encoders in SimulcastEncoderAdapter.

webrtc/video/end_to_end_tests.cc

 /*
  *  Copyright (c) 2013 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 <algorithm>
 #include <list>
 #include <map>
 #include <memory>
 #include <sstream>
 #include <string>
 #include <vector>
 
 #include "webrtc/api/video_codecs/video_encoder.h"
 #include "webrtc/base/checks.h"
 #include "webrtc/base/event.h"
 #include "webrtc/base/file.h"
 #include "webrtc/base/optional.h"
 #include "webrtc/base/random.h"
 #include "webrtc/base/rate_limiter.h"
 #include "webrtc/call/call.h"
 #include "webrtc/common_video/include/frame_callback.h"
 #include "webrtc/logging/rtc_event_log/rtc_event_log.h"
 #include "webrtc/media/base/fakevideorenderer.h"
+#include "webrtc/media/base/mediaconstants.h"
+#include "webrtc/media/engine/internalencoderfactory.h"
+#include "webrtc/media/engine/webrtcvideoencoderfactory.h"
 #include "webrtc/modules/include/module_common_types.h"
 #include "webrtc/modules/rtp_rtcp/include/rtp_rtcp.h"
 #include "webrtc/modules/rtp_rtcp/source/byte_io.h"
 #include "webrtc/modules/rtp_rtcp/source/rtcp_packet/nack.h"
 #include "webrtc/modules/rtp_rtcp/source/rtcp_packet/rapid_resync_request.h"
+#include "webrtc/modules/rtp_rtcp/source/rtp_format.h"
 #include "webrtc/modules/rtp_rtcp/source/rtp_utility.h"
 #include "webrtc/modules/video_coding/codecs/h264/include/h264.h"
 #include "webrtc/modules/video_coding/codecs/vp8/include/vp8.h"
+#include "webrtc/modules/video_coding/codecs/vp8/simulcast_encoder_adapter.h"
 #include "webrtc/modules/video_coding/codecs/vp9/include/vp9.h"
 #include "webrtc/modules/video_coding/include/video_coding_defines.h"
 #include "webrtc/system_wrappers/include/metrics.h"
 #include "webrtc/system_wrappers/include/metrics_default.h"
 #include "webrtc/system_wrappers/include/sleep.h"
 #include "webrtc/test/call_test.h"
 #include "webrtc/test/direct_transport.h"
 #include "webrtc/test/encoder_settings.h"
 #include "webrtc/test/fake_decoder.h"
 #include "webrtc/test/fake_encoder.h"
 #include "webrtc/test/field_trial.h"
 #include "webrtc/test/frame_generator.h"
 #include "webrtc/test/frame_generator_capturer.h"
+#include "webrtc/test/gmock.h"
 #include "webrtc/test/gtest.h"
-#include "webrtc/test/gmock.h"
 #include "webrtc/test/null_transport.h"
 #include "webrtc/test/rtcp_packet_parser.h"
 #include "webrtc/test/rtp_rtcp_observer.h"
 #include "webrtc/test/testsupport/fileutils.h"
 #include "webrtc/test/testsupport/perf_test.h"
 #include "webrtc/video/transport_adapter.h"
 
 #if defined(MEMORY_SANITIZER)
 // Flaky under MemorySanitizer, see
 // https://bugs.chromium.org/p/webrtc/issues/detail?id=7419
@@ skipping 63 matching lines @@
     bool need_rtp_;
     bool need_rtcp_;
     rtc::CriticalSection crit_;
   };
 
   void DecodesRetransmittedFrame(bool enable_rtx, bool enable_red);
   void ReceivesPliAndRecovers(int rtp_history_ms);
   void RespectsRtcpMode(RtcpMode rtcp_mode);
   void TestSendsSetSsrcs(size_t num_ssrcs, bool send_single_ssrc_first);
   void TestRtpStatePreservation(bool use_rtx, bool provoke_rtcpsr_before_rtp);
+  void TestPictureIdStatePreservation(VideoEncoder* encoder);
   void VerifyHistogramStats(bool use_rtx, bool use_red, bool screenshare);
   void VerifyNewVideoSendStreamsRespectNetworkState(
       MediaType network_to_bring_up,
       VideoEncoder* encoder,
       Transport* transport);
   void VerifyNewVideoReceiveStreamsRespectNetworkState(
       MediaType network_to_bring_up,
       Transport* transport);
 };
 
@@ skipping 3483 matching lines @@
     size_t ssrcs_to_observe_;
     std::map<uint32_t, bool> observed_redundant_retransmission_;
     std::map<uint32_t, bool> registered_rtx_ssrc_;
   } test;
 
   RunBaseTest(&test);
 }
 
 void EndToEndTest::TestRtpStatePreservation(bool use_rtx,
                                             bool provoke_rtcpsr_before_rtp) {
-  // This test use other VideoStream settings than the the default settings
-  // implemented in DefaultVideoStreamFactory. Therefore  this test implement
+  // This test uses other VideoStream settings than the the default settings
+  // implemented in DefaultVideoStreamFactory. Therefore this test implements
   // its own VideoEncoderConfig::VideoStreamFactoryInterface which is created
   // in ModifyVideoConfigs.
   class VideoStreamFactory
       : public VideoEncoderConfig::VideoStreamFactoryInterface {
    public:
     VideoStreamFactory() {}
 
    private:
     std::vector<VideoStream> CreateEncoderStreams(
         int width,
@@ skipping 229 matching lines @@
 }
 
 TEST_F(EndToEndTest, RestartingSendStreamPreservesRtpStatesWithRtx) {
   TestRtpStatePreservation(true, false);
 }
 
 TEST_F(EndToEndTest, RestartingSendStreamKeepsRtpAndRtcpTimestampsSynced) {
   TestRtpStatePreservation(true, true);
 }
 
+void EndToEndTest::TestPictureIdStatePreservation(VideoEncoder* encoder) {
+  const size_t kFrameMaxWidth = 1280;
+  const size_t kFrameMaxHeight = 720;
+  const size_t kFrameRate = 30;
+
+  // Use a special stream factory in this test to ensure that all simulcast
+  // streams are being sent.
+  class VideoStreamFactory
+      : public VideoEncoderConfig::VideoStreamFactoryInterface {
+   public:
+    VideoStreamFactory() = default;
+
+   private:
+    std::vector<VideoStream> CreateEncoderStreams(
+        int width,
+        int height,
+        const VideoEncoderConfig& encoder_config) override {
+      std::vector<VideoStream> streams =
+          test::CreateVideoStreams(width, height, encoder_config);
+
+      const size_t kBitrate = 100000;
+
+      if (encoder_config.number_of_streams > 1) {
+        RTC_DCHECK_EQ(3, encoder_config.number_of_streams);
+
+        for (size_t i = 0; i < encoder_config.number_of_streams; ++i) {
+          streams[i].min_bitrate_bps = kBitrate;
+          streams[i].target_bitrate_bps = kBitrate;
+          streams[i].max_bitrate_bps = kBitrate;
+        }
+
+        // test::CreateVideoStreams does not return frame sizes for the lower
+        // streams that are accepted by VP8Impl::InitEncode.
+        // TODO(brandtr): Fix the problem in test::CreateVideoStreams, rather
+        // than overriding the values here.
+        streams[1].width = streams[2].width / 2;
+        streams[1].height = streams[2].height / 2;
+        streams[0].width = streams[1].width / 2;
+        streams[0].height = streams[1].height / 2;
+      } else {
+        // Use the same total bitrates when sending a single stream to avoid
+        // lowering the bitrate estimate and requiring a subsequent rampup.
+        streams[0].min_bitrate_bps = 3 * kBitrate;
+        streams[0].target_bitrate_bps = 3 * kBitrate;
+        streams[0].max_bitrate_bps = 3 * kBitrate;
+      }
+
+      return streams;
+    }
+  };
+
+  class PictureIdObserver : public test::RtpRtcpObserver {
+   public:
+    PictureIdObserver()
+        : test::RtpRtcpObserver(kDefaultTimeoutMs), num_ssrcs_to_observe_(1) {}
+
+    void ResetExpectations(size_t num_expected_ssrcs) {
+      rtc::CritScope lock(&crit_);
+      // Do not clear the timestamp and picture_id, to ensure that we check
+      // consistency between reinits and recreations.
+      num_packets_sent_.clear();
+      num_ssrcs_to_observe_ = num_expected_ssrcs;
+      ssrc_observed_.clear();
+    }
+
+   private:
+    Action OnSendRtp(const uint8_t* packet, size_t length) override {
+      rtc::CritScope lock(&crit_);
+
+      // RTP header.
+      RTPHeader header;
+      EXPECT_TRUE(parser_->Parse(packet, length, &header));
+      const uint32_t timestamp = header.timestamp;
+      const uint32_t ssrc = header.ssrc;
+
+      const bool known_ssrc =
+          (ssrc == kVideoSendSsrcs[0] || ssrc == kVideoSendSsrcs[1] ||
+           ssrc == kVideoSendSsrcs[2]);
+      EXPECT_TRUE(known_ssrc) << "Unknown SSRC sent.";
+
+      const bool is_padding =
+          (length == header.headerLength + header.paddingLength);
+      if (is_padding) {
+        return SEND_PACKET;
+      }
+
+      // VP8 header.
+      std::unique_ptr<RtpDepacketizer> depacketizer(
+          RtpDepacketizer::Create(kRtpVideoVp8));
+      RtpDepacketizer::ParsedPayload parsed_payload;
+      EXPECT_TRUE(depacketizer->Parse(
+          &parsed_payload, &packet[header.headerLength],
+          length - header.headerLength - header.paddingLength));
+      const uint16_t picture_id =
+          parsed_payload.type.Video.codecHeader.VP8.pictureId;
+
+      // If this is the first packet, we have nothing to compare to.
+      if (last_observed_timestamp_.find(ssrc) ==
+          last_observed_timestamp_.end()) {
+        last_observed_timestamp_[ssrc] = timestamp;
+        last_observed_picture_id_[ssrc] = picture_id;
+        ++num_packets_sent_[ssrc];
+
+        return SEND_PACKET;
+      }
+
+      // Verify continuity and monotonicity of picture_id sequence.
+      if (last_observed_timestamp_[ssrc] == timestamp) {
+        // Packet belongs to same frame as before.
+        EXPECT_EQ(last_observed_picture_id_[ssrc], picture_id);
+      } else {
+        // Packet is a new frame.
+        EXPECT_EQ((last_observed_picture_id_[ssrc] + 1) % (1 << 15),
+                  picture_id);
+      }
+      last_observed_timestamp_[ssrc] = timestamp;
+      last_observed_picture_id_[ssrc] = picture_id;
+
+      // Pass the test when enough media packets have been received
+      // on all streams.
+      if (++num_packets_sent_[ssrc] >= 10 && !ssrc_observed_[ssrc]) {
+        ssrc_observed_[ssrc] = true;
+        if (--num_ssrcs_to_observe_ == 0) {
+          observation_complete_.Set();
+        }
+      }
+
+      return SEND_PACKET;
+    }
+
+    rtc::CriticalSection crit_;
+    std::map<uint32_t, uint32_t> last_observed_timestamp_ GUARDED_BY(crit_);
+    std::map<uint32_t, uint16_t> last_observed_picture_id_ GUARDED_BY(crit_);
+    std::map<uint32_t, size_t> num_packets_sent_ GUARDED_BY(crit_);
+    size_t num_ssrcs_to_observe_ GUARDED_BY(crit_);
+    std::map<uint32_t, bool> ssrc_observed_ GUARDED_BY(crit_);
+  } observer;
+
+  Call::Config config(event_log_.get());
+  CreateCalls(config, config);
+
+  test::PacketTransport send_transport(
+      sender_call_.get(), &observer, test::PacketTransport::kSender,
+      payload_type_map_, FakeNetworkPipe::Config());
+  test::PacketTransport receive_transport(
+      nullptr, &observer, test::PacketTransport::kReceiver, payload_type_map_,
+      FakeNetworkPipe::Config());
+  send_transport.SetReceiver(receiver_call_->Receiver());
+  receive_transport.SetReceiver(sender_call_->Receiver());
+
+  CreateSendConfig(kNumSsrcs, 0, 0, &send_transport);
+  video_send_config_.encoder_settings.encoder = encoder;
+  video_send_config_.encoder_settings.payload_name = "VP8";
+  video_encoder_config_.video_stream_factory =
+      new rtc::RefCountedObject<VideoStreamFactory>();
+  video_encoder_config_.number_of_streams = 1;
+  CreateMatchingReceiveConfigs(&receive_transport);
+
+  CreateVideoStreams();
+  CreateFrameGeneratorCapturer(kFrameRate, kFrameMaxWidth, kFrameMaxHeight);
+
+  // Test helpers.
+  auto reinit_encoder_and_test = [this, &observer](int num_expected_ssrcs) {
+    video_send_stream_->ReconfigureVideoEncoder(video_encoder_config_.Copy());
+    observer.ResetExpectations(num_expected_ssrcs);
+    EXPECT_TRUE(observer.Wait()) << "Timed out waiting for packets.";
+  };
+
+  auto recreate_video_send_stream_and_test = [this, &observer](
+                                                 int num_expected_ssrcs) {
+    frame_generator_capturer_->Stop();
+    // Leave some time for the last frame to be encoded, packetized, and sent.
+    SleepMs(100);

[sprang_webrtc, 2017/05/15 12:03:39]

This seems like it could cause flakiness. Any way to avoid it without major
effort?

[brandtr, 2017/05/15 17:14:04]

Yes, turns out this is super flaky :( I'll delete the VideoSendStream
recreations for now and leave a TODO. One robust solution would be to
synchronize the frame input with the packet output.

Long-term, I think we should move the picture_id state to the RTP module, where
the rest of the RTP state lives. Then we wouldn't have to worry about our
different encoder implementations doing the right thing w.r.t. picture_id, and
the synchronization wouldn't be needed here.

+    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.ResetExpectations(num_expected_ssrcs);
+    EXPECT_TRUE(observer.Wait()) << "Timed out waiting for packets.";
+  };
+
+  // Initial test with a single stream.
+  Start();
+  EXPECT_TRUE(observer.Wait()) << "Timed out waiting for packets.";
+
+  // Reinit the encoder, and then recreate the whole VideoSendStream.
+  reinit_encoder_and_test(1);
+  recreate_video_send_stream_and_test(1);
+  reinit_encoder_and_test(1);
+
+  // Go up to three streams.
+  video_encoder_config_.number_of_streams = 3;
+  reinit_encoder_and_test(3);
+  recreate_video_send_stream_and_test(3);
+  reinit_encoder_and_test(3);
+
+  // Go back to one stream.
+  video_encoder_config_.number_of_streams = 1;
+  reinit_encoder_and_test(1);
+  recreate_video_send_stream_and_test(1);
+  reinit_encoder_and_test(1);
+
+  send_transport.StopSending();
+  receive_transport.StopSending();
+
+  Stop();
+  DestroyStreams();
+}
+
+TEST_F(EndToEndTest, RestartingVp8SendStreamPreservesPictureIdState) {
+  std::unique_ptr<VideoEncoder> encoder(VP8Encoder::Create());
+  TestPictureIdStatePreservation(encoder.get());
+}
+
+TEST_F(EndToEndTest,
+       RestartingSimulcastEncoderAdapterSendStreamPreservesPictureIdState) {
+  class VideoEncoderFactoryAdapter : public webrtc::VideoEncoderFactory {
+   public:
+    explicit VideoEncoderFactoryAdapter(
+        cricket::WebRtcVideoEncoderFactory* factory)
+        : factory_(factory) {}
+    virtual ~VideoEncoderFactoryAdapter() {}
+
+    // Implements webrtc::VideoEncoderFactory.
+    webrtc::VideoEncoder* Create() override {
+      return factory_->CreateVideoEncoder(
+          cricket::VideoCodec(cricket::kVp8CodecName));
+    }
+
+    void Destroy(webrtc::VideoEncoder* encoder) override {
+      return factory_->DestroyVideoEncoder(encoder);
+    }
+
+   private:
+    cricket::WebRtcVideoEncoderFactory* const factory_;
+  };
+
+  cricket::InternalEncoderFactory internal_encoder_factory;
+  VideoEncoderFactoryAdapter* video_encoder_factory_adapter =
+      new VideoEncoderFactoryAdapter(&internal_encoder_factory);
+  // Ownership of |video_encoder_factory_adapter| is transferred.

[sprang_webrtc, 2017/05/15 12:03:39]

nit: Doesn't look like we need this temporary then?

[brandtr, 2017/05/15 17:14:04]

Nope. Gone.

+  SimulcastEncoderAdapter simulcast_encoder_adapter(
+      video_encoder_factory_adapter);
+
+  TestPictureIdStatePreservation(&simulcast_encoder_adapter);
+}
+
 TEST_F(EndToEndTest, RespectsNetworkState) {
   // TODO(pbos): Remove accepted downtime packets etc. when signaling network
   // down blocks until no more packets will be sent.
 
   // Pacer will send from its packet list and then send required padding before
   // checking paused_ again. This should be enough for one round of pacing,
   // otherwise increase.
   static const int kNumAcceptedDowntimeRtp = 5;
   // A single RTCP may be in the pipeline.
   static const int kNumAcceptedDowntimeRtcp = 1;
@@ skipping 571 matching lines @@
     std::unique_ptr<VideoEncoder> encoder_;
     std::unique_ptr<VideoDecoder> decoder_;
     rtc::CriticalSection crit_;
     int recorded_frames_ GUARDED_BY(crit_);
   } test(this);
 
   RunBaseTest(&test);
 }
 
 }  // namespace webrtc