Reuse allocated encoders in SimulcastEncoderAdapter.

webrtc/modules/video_coding/codecs/vp8/simulcast_encoder_adapter.cc

 /*
  *  Copyright (c) 2014 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.
  */
 
@@ skipping 44 matching lines @@
   for (int i = 0; i < num_streams; ++i) {
     if (codec.width * codec.simulcastStream[i].height !=
         codec.height * codec.simulcastStream[i].width) {
       return false;
     }
   }
   return true;
 }
 
 int VerifyCodec(const webrtc::VideoCodec* inst) {
-  if (inst == NULL) {
+  if (inst == nullptr) {
     return WEBRTC_VIDEO_CODEC_ERR_PARAMETER;
   }
   if (inst->maxFramerate < 1) {
     return WEBRTC_VIDEO_CODEC_ERR_PARAMETER;
   }
   // allow zero to represent an unspecified maxBitRate
   if (inst->maxBitrate > 0 && inst->startBitrate > inst->maxBitrate) {
     return WEBRTC_VIDEO_CODEC_ERR_PARAMETER;
   }
   if (inst->width <= 1 || inst->height <= 1) {
@@ skipping 44 matching lines @@
 
   const int adapted_simulcast_id_;
   const TemporalLayersFactory& tl_factory_;
 };
 
 }  // namespace
 
 namespace webrtc {
 
 SimulcastEncoderAdapter::SimulcastEncoderAdapter(VideoEncoderFactory* factory)
-    : factory_(factory),
+    : inited_(false),
+      factory_(factory),
       encoded_complete_callback_(nullptr),
       implementation_name_("SimulcastEncoderAdapter") {
+  // The adapter is typically created on the worker thread, but operated on
+  // the encoder task queue.
+  encoder_queue_.Detach();
+
   memset(&codec_, 0, sizeof(webrtc::VideoCodec));
 }
 
 SimulcastEncoderAdapter::~SimulcastEncoderAdapter() {
-  Release();
+  RTC_DCHECK(!Initialized());
+  DestroyStoredEncoders();
 }
 
 int SimulcastEncoderAdapter::Release() {
-  // TODO(pbos): Keep the last encoder instance but call ::Release() on it, then
-  // re-use this instance in ::InitEncode(). This means that changing
-  // resolutions doesn't require reallocation of the first encoder, but only
-  // reinitialization, which makes sense. Then Destroy this instance instead in
-  // ~SimulcastEncoderAdapter().
+  RTC_DCHECK_CALLED_SEQUENTIALLY(&encoder_queue_);
   while (!streaminfos_.empty()) {
     VideoEncoder* encoder = streaminfos_.back().encoder;
-    EncodedImageCallback* callback = streaminfos_.back().callback;
     encoder->Release();
-    factory_->Destroy(encoder);
-    delete callback;
+    stored_encoders_.push_back(encoder);
     streaminfos_.pop_back();
   }
+#if RTC_DCHECK_IS_ON
+  rtc::CritScope cs(&inited_crit_);
+#endif
+  inited_ = false;
   return WEBRTC_VIDEO_CODEC_OK;
 }
 
 int SimulcastEncoderAdapter::InitEncode(const VideoCodec* inst,
                                         int number_of_cores,
                                         size_t max_payload_size) {
+  RTC_DCHECK_CALLED_SEQUENTIALLY(&encoder_queue_);
+
   if (number_of_cores < 1) {
     return WEBRTC_VIDEO_CODEC_ERR_PARAMETER;
   }
 
   int ret = VerifyCodec(inst);
   if (ret < 0) {
     return ret;
   }
 
   ret = Release();
   if (ret < 0) {
     return ret;
   }
 
   int number_of_streams = NumberOfStreams(*inst);
+  RTC_DCHECK_LE(number_of_streams, kMaxSimulcastStreams);
   const bool doing_simulcast = (number_of_streams > 1);
 
   if (doing_simulcast && !ValidSimulcastResolutions(*inst, number_of_streams)) {
     return WEBRTC_VIDEO_CODEC_ERR_PARAMETER;
   }
 
   codec_ = *inst;
   SimulcastRateAllocator rate_allocator(codec_, nullptr);
   BitrateAllocation allocation = rate_allocator.GetAllocation(
       codec_.startBitrate * 1000, codec_.maxFramerate);
@@ skipping 10 matching lines @@
     uint32_t start_bitrate_kbps = start_bitrates[i];
     if (!doing_simulcast) {
       stream_codec = codec_;
       stream_codec.numberOfSimulcastStreams = 1;
     } else {
       // Cap start bitrate to the min bitrate in order to avoid strange codec
       // behavior. Since sending sending will be false, this should not matter.
       start_bitrate_kbps =
           std::max(codec_.simulcastStream[i].minBitrate, start_bitrate_kbps);
       bool highest_resolution_stream = (i == (number_of_streams - 1));
-      PopulateStreamCodec(&codec_, i, start_bitrate_kbps,
+      PopulateStreamCodec(codec_, i, start_bitrate_kbps,
                           highest_resolution_stream, &stream_codec);
     }
     TemporalLayersFactoryAdapter tl_factory_adapter(i,
                                                     *codec_.VP8()->tl_factory);
     stream_codec.VP8()->tl_factory = &tl_factory_adapter;
 
     // TODO(ronghuawu): Remove once this is handled in VP8EncoderImpl.
     if (stream_codec.qpMax < kDefaultMinQp) {
       stream_codec.qpMax = kDefaultMaxQp;
     }
 
-    VideoEncoder* encoder = factory_->Create();
+    // If an existing encoder instance exists, reuse it.
+    // TODO(brandtr): Set initial RTP state (e.g., picture_id/tl0_pic_idx) here,
+    // when we start storing that state outside the encoder wrappers.
+    VideoEncoder* encoder;
+    if (!stored_encoders_.empty()) {
+      encoder = stored_encoders_.back();

[brandtr, 2017/04/20 11:58:49]

I may want to rethink the order here, to ensure that the higher streams get to
reuse encoders before the lower streams get to do that.

This is based on an intuition that HW codec factories generally will output HW
codecs first, and when unable to do so, will output internal SW fallback codecs.
(Is this true in general?) We probably want to use the HW codecs for the highest
resolutions, i.e., the highest streams.

[noahric, 2017/04/24 17:27:01]

Two things:
1) You should try to use an equivalently sized codec, given that many encoders
can't dynamically be resized (though they can dynamically handle bitrate
changes)

2) I'm guessing they won't be re-used anyways in the current model if they've
been released. It will, at least, in the code I maintain. Though it's generally
a hard problem; that code also uses internal source encoders, so *not* calling
release will have it continue to produce frames.

I'm guessing the answer is that you want encoder resources destroyed not during
release but during destroy. My slight worry there is that inactive channels are
also released (I think) and not destroyed, so that may not work.

The answer may be to try to rework this so instead of doing a release + init, it
happens in a single pass, and equivalently configured encoders are just skipped.

[brandtr, 2017/04/26 14:59:44]

Yes, that would be a nice optimization. Not sure it's worth the complexity
however, because my feeling is that this class is mainly used in two ways:

a) "Simulcast mode": Wrapping multiple encoders, with the QualityScaler
disabled. SimulcastEncoderAdapter::InitEncode() will be called at the beginning
of the call, and ::Release() will be called at the end.

b) "Single-stream mode": Wrapping a single encoder, with the QualityScaler
enabled. SimulcastEncoderAdapter::InitEncode() will be called at the beginning
of the call, and ::Release() will be called at the end. Mid-call, there might be
reinits due to the QualityScaler changing the resolution.

The difference that this CL makes is that in scenario b), the wrapped single
encoder C++ object will not be destroyed, and thus, the contained RTP state will
not be re-randomized mid call, thus reducing the number of mid-call picture_id
jumps. In addition to breaking the spec, these mid-call picture_id jumps have
been shown to confuse the jitter buffer, possibly leading to video distortions
on the receiving end.

Long-term, we should move the RTP state out of the encoder wrappers. This CL is
just intended to improve the situation in the interim.
Although it would be nice to reuse the actual codec, in this CL I'm mainly
concerned with the wrapper object which contains the RTP state. So although your
proposed optimization sounds nice, I'd prefer not to change the semantics of
::InitEncode() and ::Release() right now.

[noahric, 2017/04/26 21:38:51]

Ah, ok. Then I guess the question is just do I expect this to break anything in
the codebase I maintain, and the answer is "I don't think so".

I'll say that I think it is valuable to improve the simulcast case. In the code
I maintain, we basically do what I explained for Android: our MediaCodec
instances are loosely tied to the webrtc encoder instances and periodically
synchronize. When a configuration change happens, the manager of the (Java)
MediaCodec instances diffs the old/new state and keeps around MediaCodec
instances that haven't changed. But it's complex, and it'd be nicer to just have
this work at the SimulcastEncoderAdapter layer instead of trying to figure it
out after the fact.

[brandtr, 2017/04/27 10:43:31]

Thanks, that's good input to have!
 
I agree that it would make sense to improve this case -- trying to reduce the
number of effective reinits that are happening.

We are currently doing some work to refactor and clean up the video_coding
module. The insights gained from that work could hopefully inform any future
steps we want to take, for example, to try to actually reuse the underlying HW
codecs and not just the wrappers.

+      stored_encoders_.pop_back();
+    } else {
+      encoder = factory_->Create();
+    }
+
     ret = encoder->InitEncode(&stream_codec, number_of_cores, max_payload_size);
     if (ret < 0) {
       // Explicitly destroy the current encoder; because we haven't registered a
       // StreamInfo for it yet, Release won't do anything about it.
       factory_->Destroy(encoder);
       Release();
       return ret;
     }
-    EncodedImageCallback* callback = new AdapterEncodedImageCallback(this, i);
-    encoder->RegisterEncodeCompleteCallback(callback);
-    streaminfos_.push_back(StreamInfo(encoder, callback, stream_codec.width,
-                                      stream_codec.height,
-                                      start_bitrate_kbps > 0));
-    if (i != 0)
+    std::unique_ptr<EncodedImageCallback> callback(
+        new AdapterEncodedImageCallback(this, i));
+    encoder->RegisterEncodeCompleteCallback(callback.get());
+    streaminfos_.emplace_back(encoder, std::move(callback), stream_codec.width,
+                              stream_codec.height, start_bitrate_kbps > 0);
+
+    if (i != 0) {
       implementation_name += ", ";
+    }
     implementation_name += streaminfos_[i].encoder->ImplementationName();
   }
+
   if (doing_simulcast) {
     implementation_name_ =
         "SimulcastEncoderAdapter (" + implementation_name + ")";
   } else {
     implementation_name_ = implementation_name;
   }
+
+  // To save memory, don't store encoders that we don't use.
+  DestroyStoredEncoders();
+
+#if RTC_DCHECK_IS_ON
+  rtc::CritScope cs(&inited_crit_);
+#endif
+  inited_ = true;
+
   return WEBRTC_VIDEO_CODEC_OK;
 }
 
 int SimulcastEncoderAdapter::Encode(
     const VideoFrame& input_image,
     const CodecSpecificInfo* codec_specific_info,
     const std::vector<FrameType>* frame_types) {
+  RTC_DCHECK_CALLED_SEQUENTIALLY(&encoder_queue_);
+
   if (!Initialized()) {
     return WEBRTC_VIDEO_CODEC_UNINITIALIZED;
   }
-  if (encoded_complete_callback_ == NULL) {
+  if (encoded_complete_callback_ == nullptr) {
     return WEBRTC_VIDEO_CODEC_UNINITIALIZED;
   }
 
   // All active streams should generate a key frame if
   // a key frame is requested by any stream.
   bool send_key_frame = false;
   if (frame_types) {
     for (size_t i = 0; i < frame_types->size(); ++i) {
       if (frame_types->at(i) == kVideoFrameKey) {
         send_key_frame = true;
         break;
       }
     }
   }
   for (size_t stream_idx = 0; stream_idx < streaminfos_.size(); ++stream_idx) {
     if (streaminfos_[stream_idx].key_frame_request &&
         streaminfos_[stream_idx].send_stream) {
       send_key_frame = true;
       break;
     }
   }
 
   int src_width = input_image.width();
   int src_height = input_image.height();
   for (size_t stream_idx = 0; stream_idx < streaminfos_.size(); ++stream_idx) {
     // Don't encode frames in resolutions that we don't intend to send.
-    if (!streaminfos_[stream_idx].send_stream)
+    if (!streaminfos_[stream_idx].send_stream) {
       continue;
+    }
 
     std::vector<FrameType> stream_frame_types;
     if (send_key_frame) {
       stream_frame_types.push_back(kVideoFrameKey);
       streaminfos_[stream_idx].key_frame_request = false;
     } else {
       stream_frame_types.push_back(kVideoFrameDelta);
     }
 
     int dst_width = streaminfos_[stream_idx].width;
@@ skipping 41 matching lines @@
         return ret;
       }
     }
   }
 
   return WEBRTC_VIDEO_CODEC_OK;
 }
 
 int SimulcastEncoderAdapter::RegisterEncodeCompleteCallback(
     EncodedImageCallback* callback) {
+  RTC_DCHECK_CALLED_SEQUENTIALLY(&encoder_queue_);
   encoded_complete_callback_ = callback;
   return WEBRTC_VIDEO_CODEC_OK;
 }
 
 int SimulcastEncoderAdapter::SetChannelParameters(uint32_t packet_loss,
                                                   int64_t rtt) {
+  RTC_DCHECK_CALLED_SEQUENTIALLY(&encoder_queue_);
   for (size_t stream_idx = 0; stream_idx < streaminfos_.size(); ++stream_idx) {
     streaminfos_[stream_idx].encoder->SetChannelParameters(packet_loss, rtt);
   }
   return WEBRTC_VIDEO_CODEC_OK;
 }
 
 int SimulcastEncoderAdapter::SetRateAllocation(const BitrateAllocation& bitrate,
                                                uint32_t new_framerate) {
-  if (!Initialized())
+  RTC_DCHECK_CALLED_SEQUENTIALLY(&encoder_queue_);
+
+  if (!Initialized()) {
     return WEBRTC_VIDEO_CODEC_UNINITIALIZED;
+  }
 
-  if (new_framerate < 1)
+  if (new_framerate < 1) {
     return WEBRTC_VIDEO_CODEC_ERR_PARAMETER;
+  }
 
-  if (codec_.maxBitrate > 0 && bitrate.get_sum_kbps() > codec_.maxBitrate)
+  if (codec_.maxBitrate > 0 && bitrate.get_sum_kbps() > codec_.maxBitrate) {
     return WEBRTC_VIDEO_CODEC_ERR_PARAMETER;
+  }
 
   if (bitrate.get_sum_bps() > 0) {
     // Make sure the bitrate fits the configured min bitrates. 0 is a special
     // value that means paused, though, so leave it alone.
-    if (bitrate.get_sum_kbps() < codec_.minBitrate)
+    if (bitrate.get_sum_kbps() < codec_.minBitrate) {
       return WEBRTC_VIDEO_CODEC_ERR_PARAMETER;
+    }
 
     if (codec_.numberOfSimulcastStreams > 0 &&
         bitrate.get_sum_kbps() < codec_.simulcastStream[0].minBitrate) {
       return WEBRTC_VIDEO_CODEC_ERR_PARAMETER;
     }
   }
 
   codec_.maxFramerate = new_framerate;
 
   for (size_t stream_idx = 0; stream_idx < streaminfos_.size(); ++stream_idx) {
     uint32_t stream_bitrate_kbps =
         bitrate.GetSpatialLayerSum(stream_idx) / 1000;
 
     // Need a key frame if we have not sent this stream before.
     if (stream_bitrate_kbps > 0 && !streaminfos_[stream_idx].send_stream) {
       streaminfos_[stream_idx].key_frame_request = true;
     }
     streaminfos_[stream_idx].send_stream = stream_bitrate_kbps > 0;
 
     // Slice the temporal layers out of the full allocation and pass it on to
     // the encoder handling the current simulcast stream.
     BitrateAllocation stream_allocation;
-    for (int i = 0; i < kMaxTemporalStreams; ++i)
+    for (int i = 0; i < kMaxTemporalStreams; ++i) {
       stream_allocation.SetBitrate(0, i, bitrate.GetBitrate(stream_idx, i));
+    }
     streaminfos_[stream_idx].encoder->SetRateAllocation(stream_allocation,
                                                         new_framerate);
   }
 
   return WEBRTC_VIDEO_CODEC_OK;
 }
 
+// TODO(brandtr): Add task checker to this member function, when all encoder
+// callbacks are coming in on the encoder queue.
 EncodedImageCallback::Result SimulcastEncoderAdapter::OnEncodedImage(
     size_t stream_idx,
     const EncodedImage& encodedImage,
     const CodecSpecificInfo* codecSpecificInfo,
     const RTPFragmentationHeader* fragmentation) {
   CodecSpecificInfo stream_codec_specific = *codecSpecificInfo;
   stream_codec_specific.codec_name = implementation_name_.c_str();
   CodecSpecificInfoVP8* vp8Info = &(stream_codec_specific.codecSpecific.VP8);
   vp8Info->simulcastIdx = stream_idx;
 
   return encoded_complete_callback_->OnEncodedImage(
       encodedImage, &stream_codec_specific, fragmentation);
 }
 
 void SimulcastEncoderAdapter::PopulateStreamCodec(
-    const webrtc::VideoCodec* inst,
+    const webrtc::VideoCodec& inst,
     int stream_index,
     uint32_t start_bitrate_kbps,
     bool highest_resolution_stream,
     webrtc::VideoCodec* stream_codec) {
-  *stream_codec = *inst;
+  *stream_codec = inst;
 
   // Stream specific settings.
   stream_codec->VP8()->numberOfTemporalLayers =
-      inst->simulcastStream[stream_index].numberOfTemporalLayers;
+      inst.simulcastStream[stream_index].numberOfTemporalLayers;
   stream_codec->numberOfSimulcastStreams = 0;
-  stream_codec->width = inst->simulcastStream[stream_index].width;
-  stream_codec->height = inst->simulcastStream[stream_index].height;
-  stream_codec->maxBitrate = inst->simulcastStream[stream_index].maxBitrate;
-  stream_codec->minBitrate = inst->simulcastStream[stream_index].minBitrate;
-  stream_codec->qpMax = inst->simulcastStream[stream_index].qpMax;
+  stream_codec->width = inst.simulcastStream[stream_index].width;
+  stream_codec->height = inst.simulcastStream[stream_index].height;
+  stream_codec->maxBitrate = inst.simulcastStream[stream_index].maxBitrate;
+  stream_codec->minBitrate = inst.simulcastStream[stream_index].minBitrate;
+  stream_codec->qpMax = inst.simulcastStream[stream_index].qpMax;
   // Settings that are based on stream/resolution.
-  if (stream_index == 0) {
+  const bool lowest_resolution_stream = (stream_index == 0);
+  if (lowest_resolution_stream) {
     // Settings for lowest spatial resolutions.
     stream_codec->qpMax = kLowestResMaxQp;
   }
   if (!highest_resolution_stream) {
     // For resolutions below CIF, set the codec |complexity| parameter to
     // kComplexityHigher, which maps to cpu_used = -4.
     int pixels_per_frame = stream_codec->width * stream_codec->height;
     if (pixels_per_frame < 352 * 288) {
       stream_codec->VP8()->complexity = webrtc::kComplexityHigher;
     }
     // Turn off denoising for all streams but the highest resolution.
     stream_codec->VP8()->denoisingOn = false;
   }
   // TODO(ronghuawu): what to do with targetBitrate.
 
   stream_codec->startBitrate = start_bitrate_kbps;
 }
 
 bool SimulcastEncoderAdapter::Initialized() const {
-  return !streaminfos_.empty();
+#if RTC_DCHECK_IS_ON
+  rtc::CritScope cs(&inited_crit_);
+#endif
+  return inited_;
+}
+
+void SimulcastEncoderAdapter::DestroyStoredEncoders() {
+  for (const auto& encoder : stored_encoders_) {
+    factory_->Destroy(encoder);
+  }
+  stored_encoders_.clear();
 }
 
 bool SimulcastEncoderAdapter::SupportsNativeHandle() const {
+  RTC_DCHECK_CALLED_SEQUENTIALLY(&encoder_queue_);
   // We should not be calling this method before streaminfos_ are configured.
   RTC_DCHECK(!streaminfos_.empty());
   for (const auto& streaminfo : streaminfos_) {
-    if (!streaminfo.encoder->SupportsNativeHandle())
+    if (!streaminfo.encoder->SupportsNativeHandle()) {
       return false;
+    }
   }
   return true;
 }
 
 VideoEncoder::ScalingSettings SimulcastEncoderAdapter::GetScalingSettings()
     const {
+  RTC_DCHECK_CALLED_SEQUENTIALLY(&encoder_queue_);
   // Turn off quality scaling for simulcast.
-  if (!Initialized() || NumberOfStreams(codec_) != 1)
+  if (!Initialized() || NumberOfStreams(codec_) != 1) {
     return VideoEncoder::ScalingSettings(false);
+  }
   return streaminfos_[0].encoder->GetScalingSettings();
 }
 
 const char* SimulcastEncoderAdapter::ImplementationName() const {
+  RTC_DCHECK_CALLED_SEQUENTIALLY(&encoder_queue_);
   return implementation_name_.c_str();
 }
 
 }  // namespace webrtc