Move keyframe requests outside encoder mutex.

Enables faster keyframe requests since they are no longer blocked by
calls to the encoder.

BUG=webrtc:5410
R=stefan@webrtc.org

Review URL: https://codereview.webrtc.org/1600553003 .

Cr-Commit-Position: refs/heads/master@{#11294}

webrtc/modules/video_coding/video_coding_impl.h

@@ -99,19 +99,18 @@ class VideoSender {
 
  private:
   void SetEncoderParameters(EncoderParameters params)
-      EXCLUSIVE_LOCKS_REQUIRED(send_crit_);
+      EXCLUSIVE_LOCKS_REQUIRED(encoder_crit_);
 
   Clock* const clock_;
 
   rtc::scoped_ptr<CriticalSectionWrapper> process_crit_sect_;
-  mutable rtc::CriticalSection send_crit_;
+  mutable rtc::CriticalSection encoder_crit_;
   VCMGenericEncoder* _encoder;
-  VCMEncodedFrameCallback _encodedFrameCallback;
+  VCMEncodedFrameCallback _encodedFrameCallback GUARDED_BY(encoder_crit_);
-  std::vector<FrameType> _nextFrameTypes;
   media_optimization::MediaOptimization _mediaOpt;
   VCMSendStatisticsCallback* _sendStatsCallback GUARDED_BY(process_crit_sect_);
-  VCMCodecDataBase _codecDataBase GUARDED_BY(send_crit_);
+  VCMCodecDataBase _codecDataBase GUARDED_BY(encoder_crit_);
-  bool frame_dropper_enabled_ GUARDED_BY(send_crit_);
+  bool frame_dropper_enabled_ GUARDED_BY(encoder_crit_);
   VCMProcessTimer _sendStatsTimer;
 
   // Must be accessed on the construction thread of VideoSender.
@@ -121,8 +120,10 @@ class VideoSender {
   VCMQMSettingsCallback* const qm_settings_callback_;
   VCMProtectionCallback* protection_callback_;
 
-  rtc::CriticalSection params_lock_;
+  rtc::CriticalSection params_crit_;
-  EncoderParameters encoder_params_ GUARDED_BY(params_lock_);
+  EncoderParameters encoder_params_ GUARDED_BY(params_crit_);
+  bool encoder_has_internal_source_ GUARDED_BY(params_crit_);
+  std::vector<FrameType> next_frame_types_ GUARDED_BY(params_crit_);
 };
 
 class VideoReceiver {

webrtc/modules/video_coding/video_sender.cc

@@ -32,7 +32,6 @@ VideoSender::VideoSender(Clock* clock,
       process_crit_sect_(CriticalSectionWrapper::CreateCriticalSection()),
       _encoder(nullptr),
       _encodedFrameCallback(post_encode_callback),
-      _nextFrameTypes(1, kVideoFrameDelta),
       _mediaOpt(clock_),
       _sendStatsCallback(nullptr),
       _codecDataBase(encoder_rate_observer, &_encodedFrameCallback),
@@ -41,7 +40,9 @@ VideoSender::VideoSender(Clock* clock,
       current_codec_(),
       qm_settings_callback_(qm_settings_callback),
       protection_callback_(nullptr),
-      encoder_params_({0, 0, 0, 0}) {
+      encoder_params_({0, 0, 0, 0}),
+      encoder_has_internal_source_(false),
+      next_frame_types_(1, kVideoFrameDelta) {
   // Allow VideoSender to be created on one thread but used on another, post
   // construction. This is currently how this class is being used by at least
   // one external project (diffractor).
@@ -66,7 +67,7 @@ int32_t VideoSender::Process() {
   }
 
   {
-    rtc::CritScope cs(&params_lock_);
+    rtc::CritScope cs(&params_crit_);
     // Force an encoder parameters update, so that incoming frame rate is
     // updated even if bandwidth hasn't changed.
     encoder_params_.input_frame_rate = _mediaOpt.InputFrameRate();
@@ -84,7 +85,7 @@ int32_t VideoSender::RegisterSendCodec(const VideoCodec* sendCodec,
                                        uint32_t numberOfCores,
                                        uint32_t maxPayloadSize) {
   RTC_DCHECK(main_thread_.CalledOnValidThread());
-  rtc::CritScope lock(&send_crit_);
+  rtc::CritScope lock(&encoder_crit_);
   if (sendCodec == nullptr) {
     return VCM_PARAMETER_ERROR;
   }
@@ -105,6 +106,9 @@ int32_t VideoSender::RegisterSendCodec(const VideoCodec* sendCodec,
     return VCM_CODEC_ERROR;
   }
 
+  // SetSendCodec succeeded, _encoder should be set.
+  RTC_DCHECK(_encoder);
+
   int numLayers;
   if (sendCodec->codecType == kVideoCodecVP8) {
     numLayers = sendCodec->codecSpecific.VP8.numberOfTemporalLayers;
@@ -122,9 +126,15 @@ int32_t VideoSender::RegisterSendCodec(const VideoCodec* sendCodec,
   } else if (frame_dropper_enabled_) {
     _mediaOpt.EnableFrameDropper(true);
   }
-  _nextFrameTypes.clear();
+  {
-  _nextFrameTypes.resize(VCM_MAX(sendCodec->numberOfSimulcastStreams, 1),
+    rtc::CritScope cs(&params_crit_);
-                         kVideoFrameDelta);
+    next_frame_types_.clear();
+    next_frame_types_.resize(VCM_MAX(sendCodec->numberOfSimulcastStreams, 1),
+                             kVideoFrameKey);
+    // Cache InternalSource() to have this available from IntraFrameRequest()
+    // without having to acquire encoder_crit_ (avoid blocking on encoder use).
+    encoder_has_internal_source_ = _encoder->InternalSource();
+  }
 
   _mediaOpt.SetEncodingData(sendCodec->codecType, sendCodec->maxBitrate * 1000,
                             sendCodec->startBitrate * 1000, sendCodec->width,
@@ -140,7 +150,7 @@ void VideoSender::RegisterExternalEncoder(VideoEncoder* externalEncoder,
                                           bool internalSource /*= false*/) {
   RTC_DCHECK(main_thread_.CalledOnValidThread());
 
-  rtc::CritScope lock(&send_crit_);
+  rtc::CritScope lock(&encoder_crit_);
 
   if (externalEncoder == nullptr) {
     bool wasSendCodec = false;
@@ -148,7 +158,9 @@ void VideoSender::RegisterExternalEncoder(VideoEncoder* externalEncoder,
         _codecDataBase.DeregisterExternalEncoder(payloadType, &wasSendCodec));
     if (wasSendCodec) {
       // Make sure the VCM doesn't use the de-registered codec
+      rtc::CritScope params_lock(&params_crit_);
       _encoder = nullptr;
+      encoder_has_internal_source_ = false;
     }
     return;
   }
@@ -190,7 +202,7 @@ int32_t VideoSender::SetChannelParameters(uint32_t target_bitrate,
 
   uint32_t input_frame_rate = _mediaOpt.InputFrameRate();
 
-  rtc::CritScope cs(&params_lock_);
+  rtc::CritScope cs(&params_crit_);
   encoder_params_ = {target_rate, lossRate, rtt, input_frame_rate};
 
   return VCM_OK;
@@ -210,7 +222,7 @@ void VideoSender::SetEncoderParameters(EncoderParameters params) {
 
 int32_t VideoSender::RegisterTransportCallback(
     VCMPacketizationCallback* transport) {
-  rtc::CritScope lock(&send_crit_);
+  rtc::CritScope lock(&encoder_crit_);
   _encodedFrameCallback.SetMediaOpt(&_mediaOpt);
   _encodedFrameCallback.SetTransportCallback(transport);
   return VCM_OK;
@@ -239,7 +251,7 @@ int32_t VideoSender::RegisterProtectionCallback(
 
 // Enable or disable a video protection method.
 void VideoSender::SetVideoProtection(VCMVideoProtection videoProtection) {
-  rtc::CritScope lock(&send_crit_);
+  rtc::CritScope lock(&encoder_crit_);
   switch (videoProtection) {
     case kProtectionNone:
       _mediaOpt.SetProtectionMethod(media_optimization::kNone);
@@ -260,19 +272,16 @@ int32_t VideoSender::AddVideoFrame(const VideoFrame& videoFrame,
                                    const VideoContentMetrics* contentMetrics,
                                    const CodecSpecificInfo* codecSpecificInfo) {
   EncoderParameters encoder_params;
+  std::vector<FrameType> next_frame_types;
   {
-    rtc::CritScope lock(&params_lock_);
+    rtc::CritScope lock(&params_crit_);
     encoder_params = encoder_params_;
+    next_frame_types = next_frame_types_;
   }
-  rtc::CritScope lock(&send_crit_);
+  rtc::CritScope lock(&encoder_crit_);
   if (_encoder == nullptr)
     return VCM_UNINITIALIZED;
   SetEncoderParameters(encoder_params);
-  // TODO(holmer): Add support for dropping frames per stream. Currently we
-  // only have one frame dropper for all streams.
-  if (_nextFrameTypes[0] == kEmptyFrame) {
-    return VCM_OK;
-  }
   if (_mediaOpt.DropFrame()) {
     _encoder->OnDroppedFrame();
     return VCM_OK;
@@ -294,13 +303,20 @@ int32_t VideoSender::AddVideoFrame(const VideoFrame& videoFrame,
         << "Frame conversion failed, won't be able to encode frame.";
   }
   int32_t ret =
-      _encoder->Encode(converted_frame, codecSpecificInfo, _nextFrameTypes);
+      _encoder->Encode(converted_frame, codecSpecificInfo, next_frame_types);
   if (ret < 0) {
     LOG(LS_ERROR) << "Failed to encode frame. Error code: " << ret;
     return ret;
   }
-  for (size_t i = 0; i < _nextFrameTypes.size(); ++i) {
+  {
-    _nextFrameTypes[i] = kVideoFrameDelta;  // Default frame type.
+    // Change all keyframe requests to encode delta frames the next time.
+    rtc::CritScope lock(&params_crit_);
+    for (size_t i = 0; i < next_frame_types_.size(); ++i) {
+      // Check for equality (same requested as before encoding) to not
+      // accidentally drop a keyframe request while encoding.
+      if (next_frame_types[i] == next_frame_types_[i])
+        next_frame_types_[i] = kVideoFrameDelta;
+    }
   }
   if (qm_settings_callback_)
     qm_settings_callback_->SetTargetFramerate(_encoder->GetTargetFramerate());
@@ -308,24 +324,38 @@ int32_t VideoSender::AddVideoFrame(const VideoFrame& videoFrame,
 }
 
 int32_t VideoSender::IntraFrameRequest(int stream_index) {
-  rtc::CritScope lock(&send_crit_);
+  {
+    rtc::CritScope lock(&params_crit_);
     if (stream_index < 0 ||
-      static_cast<unsigned int>(stream_index) >= _nextFrameTypes.size()) {
+        static_cast<size_t>(stream_index) >= next_frame_types_.size()) {
       return -1;
     }
-  _nextFrameTypes[stream_index] = kVideoFrameKey;
+    next_frame_types_[stream_index] = kVideoFrameKey;
+    if (!encoder_has_internal_source_)
+      return VCM_OK;
+  }
+  // TODO(pbos): Remove when InternalSource() is gone. Both locks have to be
+  // held here for internal consistency, since _encoder could be removed while
+  // not holding encoder_crit_. Checks have to be performed again since
+  // params_crit_ was dropped to not cause lock-order inversions with
+  // encoder_crit_.
+  rtc::CritScope lock(&encoder_crit_);
+  rtc::CritScope params_lock(&params_crit_);
+  if (static_cast<size_t>(stream_index) >= next_frame_types_.size())
+    return -1;
   if (_encoder != nullptr && _encoder->InternalSource()) {
     // Try to request the frame if we have an external encoder with
     // internal source since AddVideoFrame never will be called.
-    if (_encoder->RequestFrame(_nextFrameTypes) == WEBRTC_VIDEO_CODEC_OK) {
+    if (_encoder->RequestFrame(next_frame_types_) == WEBRTC_VIDEO_CODEC_OK) {
-      _nextFrameTypes[stream_index] = kVideoFrameDelta;
+      // Try to remove just-performed keyframe request, if stream still exists.
+      next_frame_types_[stream_index] = kVideoFrameDelta;
     }
   }
   return VCM_OK;
 }
 
 int32_t VideoSender::EnableFrameDropper(bool enable) {
-  rtc::CritScope lock(&send_crit_);
+  rtc::CritScope lock(&encoder_crit_);
   frame_dropper_enabled_ = enable;
   _mediaOpt.EnableFrameDropper(enable);
   return VCM_OK;

webrtc/modules/video_coding/video_sender_unittest.cc

@@ -40,7 +40,12 @@ using webrtc::test::FrameGenerator;
 namespace webrtc {
 namespace vcm {
 namespace {
-enum { kMaxNumberOfTemporalLayers = 3 };
+static const int kDefaultHeight = 720;
+static const int kDefaultWidth = 1280;
+static const int kMaxNumberOfTemporalLayers = 3;
+static const int kNumberOfLayers = 3;
+static const int kNumberOfStreams = 3;
+static const int kUnusedPayloadType = 10;
 
 struct Vp8StreamInfo {
   float framerate_fps[kMaxNumberOfTemporalLayers];
@@ -196,12 +201,6 @@ class TestVideoSender : public ::testing::Test {
 
 class TestVideoSenderWithMockEncoder : public TestVideoSender {
  protected:
-  static const int kDefaultWidth = 1280;
-  static const int kDefaultHeight = 720;
-  static const int kNumberOfStreams = 3;
-  static const int kNumberOfLayers = 3;
-  static const int kUnusedPayloadType = 10;
-
   void SetUp() override {
     TestVideoSender::SetUp();
     sender_->RegisterExternalEncoder(&encoder_, kUnusedPayloadType, false);
@@ -222,20 +221,29 @@ class TestVideoSenderWithMockEncoder : public TestVideoSender {
   void TearDown() override { sender_.reset(); }
 
   void ExpectIntraRequest(int stream) {
-    if (stream == -1) {
+    ExpectEncodeWithFrameTypes(stream, false);
-      // No intra request expected.
+  }
-      EXPECT_CALL(
+
-          encoder_,
+  void ExpectInitialKeyFrames() {
-          Encode(_, _, Pointee(ElementsAre(kVideoFrameDelta, kVideoFrameDelta,
+    ExpectEncodeWithFrameTypes(-1, true);
-                                           kVideoFrameDelta))))
+  }
+
+  void ExpectEncodeWithFrameTypes(int intra_request_stream, bool first_frame) {
+    if (intra_request_stream == -1) {
+      // No intra request expected, keyframes on first frame.
+      FrameType frame_type = first_frame ? kVideoFrameKey : kVideoFrameDelta;
+      EXPECT_CALL(encoder_,
+                  Encode(_, _, Pointee(ElementsAre(frame_type, frame_type,
+                                                   frame_type))))
           .Times(1)
           .WillRepeatedly(Return(0));
       return;
     }
-    assert(stream >= 0);
+    ASSERT_FALSE(first_frame);
-    assert(stream < kNumberOfStreams);
+    ASSERT_GE(intra_request_stream, 0);
+    ASSERT_LT(intra_request_stream, kNumberOfStreams);
     std::vector<FrameType> frame_types(kNumberOfStreams, kVideoFrameDelta);
-    frame_types[stream] = kVideoFrameKey;
+    frame_types[intra_request_stream] = kVideoFrameKey;
     EXPECT_CALL(encoder_,
                 Encode(_, _, Pointee(ElementsAreArray(&frame_types[0],
                                                       frame_types.size()))))
@@ -260,6 +268,9 @@ class TestVideoSenderWithMockEncoder : public TestVideoSender {
 };
 
 TEST_F(TestVideoSenderWithMockEncoder, TestIntraRequests) {
+  // Initial request should be all keyframes.
+  ExpectInitialKeyFrames();
+  AddFrame();
   EXPECT_EQ(0, sender_->IntraFrameRequest(0));
   ExpectIntraRequest(0);
   AddFrame();
@@ -293,6 +304,9 @@ TEST_F(TestVideoSenderWithMockEncoder, TestIntraRequestsInternalCapture) {
   // Register encoder with internal capture.
   sender_->RegisterExternalEncoder(&encoder_, kUnusedPayloadType, true);
   EXPECT_EQ(0, sender_->RegisterSendCodec(&settings_, 1, 1200));
+  // Initial request should be all keyframes.
+  ExpectInitialKeyFrames();
+  AddFrame();
   ExpectIntraRequest(0);
   EXPECT_EQ(0, sender_->IntraFrameRequest(0));
   ExpectIntraRequest(1);

webrtc/video/vie_encoder.cc

@@ -401,8 +401,8 @@ void ViEEncoder::DeliverFrame(VideoFrame video_frame) {
   vcm_->AddVideoFrame(*frame_to_send);
 }
 
-int ViEEncoder::SendKeyFrame() {
+void ViEEncoder::SendKeyFrame() {
-  return vcm_->IntraFrameRequest(0);
+  vcm_->IntraFrameRequest(0);
 }
 
 uint32_t ViEEncoder::LastObservedBitrateBps() const {

webrtc/video/vie_encoder.h

@@ -91,7 +91,7 @@ class ViEEncoder : public RtcpIntraFrameObserver,
   // Implementing VideoCaptureCallback.
   void DeliverFrame(VideoFrame video_frame) override;
 
-  int32_t SendKeyFrame();
+  void SendKeyFrame();
 
   uint32_t LastObservedBitrateBps() const;
   int CodecTargetBitrate(uint32_t* bitrate) const;