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

webrtc/modules/video_coding/video_receiver.cc

Index: webrtc/modules/video_coding/video_receiver.cc
diff --git a/webrtc/modules/video_coding/video_receiver.cc b/webrtc/modules/video_coding/video_receiver.cc
index 9dc8eaab996388346def18b23c8d09cf9d0234b0..b8dedd02bbab9449f67db05b45110da6c43571a6 100644
--- a/webrtc/modules/video_coding/video_receiver.cc
+++ b/webrtc/modules/video_coding/video_receiver.cc
@@ -41,7 +41,6 @@ VideoReceiver::VideoReceiver(Clock* clock,
       _receiveStatsCallback(nullptr),
       _decoderTimingCallback(nullptr),
       _packetRequestCallback(nullptr),
-      _decoder(nullptr),
       _frameFromFile(),
       _scheduleKeyRequest(false),
       drop_frames_until_keyframe_(false),
@@ -168,6 +167,9 @@ int32_t VideoReceiver::SetVideoProtection(VCMVideoProtection videoProtection,
 // 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;
@@ -175,6 +177,7 @@ int32_t VideoReceiver::RegisterReceiveCallback(
 
 int32_t VideoReceiver::RegisterReceiveStatisticsCallback(
     VCMReceiveStatisticsCallback* receiveStats) {
+  RTC_DCHECK(construction_thread_.CalledOnValidThread());
   rtc::CritScope cs(&process_crit_);
   _receiver.RegisterStatsCallback(receiveStats);
   _receiveStatsCallback = receiveStats;
@@ -183,6 +186,7 @@ int32_t VideoReceiver::RegisterReceiveStatisticsCallback(
 
 int32_t VideoReceiver::RegisterDecoderTimingCallback(
     VCMDecoderTimingCallback* decoderTiming) {
+  RTC_DCHECK(construction_thread_.CalledOnValidThread());
   rtc::CritScope cs(&process_crit_);
   _decoderTimingCallback = decoderTiming;
   return VCM_OK;
@@ -191,10 +195,11 @@ int32_t VideoReceiver::RegisterDecoderTimingCallback(
 // 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;
   }
@@ -217,6 +222,7 @@ int32_t VideoReceiver::RegisterPacketRequestCallback(
 }
 
 void VideoReceiver::TriggerDecoderShutdown() {
+  RTC_DCHECK(construction_thread_.CalledOnValidThread());
   _receiver.TriggerDecoderShutdown();
 }
 
@@ -225,6 +231,7 @@ void VideoReceiver::TriggerDecoderShutdown() {
 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();
   }
@@ -292,6 +299,11 @@ int32_t VideoReceiver::Decode(const webrtc::VCMEncodedFrame* frame) {
   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);
@@ -327,12 +339,13 @@ int32_t VideoReceiver::RequestKeyFrame() {
 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;
@@ -362,6 +375,10 @@ int32_t VideoReceiver::Decode(const VCMEncodedFrame& frame) {
 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;
@@ -374,6 +391,8 @@ int32_t VideoReceiver::RegisterReceiveCodec(const VideoCodec* receiveCodec,
 }
 
 // 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) {
@@ -383,6 +402,8 @@ int32_t VideoReceiver::ReceiveCodec(VideoCodec* currentReceiveCodec) const {
 }
 
 // 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();