Updates to VCMDecodedFrameCallback, VideoReceiver and a few related classes/tests.

webrtc/modules/video_coding/video_receiver.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.
  */
 
@@ skipping 23 matching lines @@
       _receiver(_timing,
                 clock_,
                 event_factory,
                 nack_sender,
                 keyframe_request_sender),
       _decodedFrameCallback(_timing, clock_),
       _frameTypeCallback(nullptr),
       _receiveStatsCallback(nullptr),
       _decoderTimingCallback(nullptr),
       _packetRequestCallback(nullptr),
-      _decoder(nullptr),
       _frameFromFile(),
       _scheduleKeyRequest(false),
       drop_frames_until_keyframe_(false),
       max_nack_list_size_(0),
       _codecDataBase(nullptr),
       pre_decode_image_callback_(pre_decode_image_callback),
       _receiveStatsTimer(1000, clock_),
       _retransmissionTimer(10, clock_),
       _keyRequestTimer(500, clock_) {}
 
@@ skipping 106 matching lines @@
       _receiver.SetDecodeErrorMode(kWithErrors);
       break;
   }
   return VCM_OK;
 }
 
 // Register a receive callback. Will be called whenever there is a new frame
 // ready for rendering.
 int32_t VideoReceiver::RegisterReceiveCallback(
     VCMReceiveCallback* receiveCallback) {
+  RTC_DCHECK(construction_thread_.CalledOnValidThread());
+  // TODO(tommi): Callback may be null, but only after the decoder thread has
+  // been stopped.

[stefan-webrtc, 2017/03/11 12:57:37]

Clarify what is to be done to remove this TODO

[tommi, 2017/03/11 15:24:03]

Done.

   rtc::CritScope cs(&receive_crit_);
   _decodedFrameCallback.SetUserReceiveCallback(receiveCallback);
   return VCM_OK;
 }
 
 int32_t VideoReceiver::RegisterReceiveStatisticsCallback(
     VCMReceiveStatisticsCallback* receiveStats) {
+  RTC_DCHECK(construction_thread_.CalledOnValidThread());
   rtc::CritScope cs(&process_crit_);
   _receiver.RegisterStatsCallback(receiveStats);
   _receiveStatsCallback = receiveStats;
   return VCM_OK;
 }
 
 int32_t VideoReceiver::RegisterDecoderTimingCallback(
     VCMDecoderTimingCallback* decoderTiming) {
+  RTC_DCHECK(construction_thread_.CalledOnValidThread());
   rtc::CritScope cs(&process_crit_);
   _decoderTimingCallback = decoderTiming;
   return VCM_OK;
 }
 
 // Register an externally defined decoder object.
 void VideoReceiver::RegisterExternalDecoder(VideoDecoder* externalDecoder,
                                             uint8_t payloadType) {
+  RTC_DCHECK(construction_thread_.CalledOnValidThread());
+  // TODO(tommi): This method must be called when the decoder thread is not
+  // running.  Do we need a lock in that case?

[stefan-webrtc, 2017/03/11 12:57:37]

I think the answer is no as _codecDataBase only seems to be accessed on the
decoder thread and from these Register/Deregister methods. Maybe that's what you
already also have concluded... :)

[tommi, 2017/03/11 15:24:03]

Yes, this is more of a rhetorical question/TODO from myself to me.  A followup
change would be to remove locking, which I think would be nice since as is, all
methods that grab the lock, contend with a decode operation.

   rtc::CritScope cs(&receive_crit_);
   if (externalDecoder == nullptr) {
-    // Make sure the VCM updates the decoder next time it decodes.
-    _decoder = nullptr;
     RTC_CHECK(_codecDataBase.DeregisterExternalDecoder(payloadType));
     return;
   }
   _codecDataBase.RegisterExternalDecoder(externalDecoder, payloadType);
 }
 
 // Register a frame type request callback.
 int32_t VideoReceiver::RegisterFrameTypeCallback(
     VCMFrameTypeCallback* frameTypeCallback) {
   rtc::CritScope cs(&process_crit_);
   _frameTypeCallback = frameTypeCallback;
   return VCM_OK;
 }
 
 int32_t VideoReceiver::RegisterPacketRequestCallback(
     VCMPacketRequestCallback* callback) {
   rtc::CritScope cs(&process_crit_);
   _packetRequestCallback = callback;
   return VCM_OK;
 }
 
 void VideoReceiver::TriggerDecoderShutdown() {
+  RTC_DCHECK(construction_thread_.CalledOnValidThread());
   _receiver.TriggerDecoderShutdown();
 }
 
 // Decode next frame, blocking.
 // Should be called as often as possible to get the most out of the decoder.
 int32_t VideoReceiver::Decode(uint16_t maxWaitTimeMs) {
   bool prefer_late_decoding = false;
   {
+    // TODO(tommi): Chances are that this lock isn't required.
     rtc::CritScope cs(&receive_crit_);
     prefer_late_decoding = _codecDataBase.PrefersLateDecoding();
   }
 
   VCMEncodedFrame* frame =
       _receiver.FrameForDecoding(maxWaitTimeMs, prefer_late_decoding);
 
   if (!frame)
     return VCM_FRAME_NOT_READY;
 
@@ skipping 47 matching lines @@
     int qp = -1;
     if (qp_parser_.GetQp(*frame, &qp)) {
       encoded_image.qp_ = qp;
     }
     pre_decode_image_callback_->OnEncodedImage(encoded_image,
                                                frame->CodecSpecific(), nullptr);
   }
   return Decode(*frame);
 }
 
+void VideoReceiver::DecodingStopped() {
+  // No further calls to Decode() will be made after this point.
+  // TODO(tommi): Make use of this to clarify and check threading model.
+}
+
 int32_t VideoReceiver::RequestSliceLossIndication(
     const uint64_t pictureID) const {
   TRACE_EVENT1("webrtc", "RequestSLI", "picture_id", pictureID);
   rtc::CritScope cs(&process_crit_);
   if (_frameTypeCallback != nullptr) {
     const int32_t ret =
         _frameTypeCallback->SliceLossIndicationRequest(pictureID);
     if (ret < 0) {
       return ret;
     }
@@ skipping 15 matching lines @@
   } else {
     return VCM_MISSING_CALLBACK;
   }
   return VCM_OK;
 }
 
 // Must be called from inside the receive side critical section.
 int32_t VideoReceiver::Decode(const VCMEncodedFrame& frame) {
   TRACE_EVENT0("webrtc", "VideoReceiver::Decode");
   // Change decoder if payload type has changed
-  _decoder = _codecDataBase.GetDecoder(frame, &_decodedFrameCallback);
-  if (_decoder == nullptr) {
+  VCMGenericDecoder* decoder =
+      _codecDataBase.GetDecoder(frame, &_decodedFrameCallback);
+  if (decoder == nullptr) {
     return VCM_NO_CODEC_REGISTERED;
   }
   // Decode a frame
-  int32_t ret = _decoder->Decode(frame, clock_->TimeInMilliseconds());
+  int32_t ret = decoder->Decode(frame, clock_->TimeInMilliseconds());
 
   // Check for failed decoding, run frame type request callback if needed.
   bool request_key_frame = false;
   if (ret < 0) {
     if (ret == VCM_ERROR_REQUEST_SLI) {
       return RequestSliceLossIndication(
           _decodedFrameCallback.LastReceivedPictureID() + 1);
     } else {
       request_key_frame = true;
     }
   } else if (ret == VCM_REQUEST_SLI) {
     ret = RequestSliceLossIndication(
         _decodedFrameCallback.LastReceivedPictureID() + 1);
   }
   if (!frame.Complete() || frame.MissingFrame()) {
     request_key_frame = true;
     ret = VCM_OK;
   }
   if (request_key_frame) {
     rtc::CritScope cs(&process_crit_);
     _scheduleKeyRequest = true;
   }
   return ret;
 }
 
 // Register possible receive codecs, can be called multiple times
 int32_t VideoReceiver::RegisterReceiveCodec(const VideoCodec* receiveCodec,
                                             int32_t numberOfCores,
                                             bool requireKeyFrame) {
+  RTC_DCHECK(construction_thread_.CalledOnValidThread());
+  // TODO(tommi): This method must only be called when the decoder thread
+  // is not running. Do we need a lock? If not, it looks like we might not need
+  // a lock at all for |_codecDataBase|.
   rtc::CritScope cs(&receive_crit_);
   if (receiveCodec == nullptr) {
     return VCM_PARAMETER_ERROR;
   }
   if (!_codecDataBase.RegisterReceiveCodec(receiveCodec, numberOfCores,
                                            requireKeyFrame)) {
     return -1;
   }
   return 0;
 }
 
 // Get current received codec
+// TODO(tommi): See if there are any actual callers to this method.
+// If not, it will make threading simpler.

[stefan-webrtc, 2017/03/11 12:57:37]

No calls that I can find.

[tommi, 2017/03/11 15:24:03]

Thanks for looking. I didn't find any either.  I'll note that neither one of us
found a caller.

 int32_t VideoReceiver::ReceiveCodec(VideoCodec* currentReceiveCodec) const {
   rtc::CritScope cs(&receive_crit_);
   if (currentReceiveCodec == nullptr) {
     return VCM_PARAMETER_ERROR;
   }
   return _codecDataBase.ReceiveCodec(currentReceiveCodec) ? 0 : -1;
 }
 
 // Get current received codec
+// TODO(tommi): See if there are any actual callers to this method.
+// If not, it will make threading simpler.
 VideoCodecType VideoReceiver::ReceiveCodec() const {
   rtc::CritScope cs(&receive_crit_);
   return _codecDataBase.ReceiveCodec();
 }
 
 // Incoming packet from network parsed and ready for decode, non blocking.
 int32_t VideoReceiver::IncomingPacket(const uint8_t* incomingPayload,
                                       size_t payloadLength,
                                       const WebRtcRTPHeader& rtpInfo) {
   if (rtpInfo.frameType == kVideoFrameKey) {
@@ skipping 100 matching lines @@
   _receiver.SetNackSettings(max_nack_list_size, max_packet_age_to_nack,
                             max_incomplete_time_ms);
 }
 
 int VideoReceiver::SetMinReceiverDelay(int desired_delay_ms) {
   return _receiver.SetMinReceiverDelay(desired_delay_ms);
 }
 
 }  // namespace vcm
 }  // namespace webrtc