Remove VideoCodingModule::VCMPacketizationCallback

And move encoder name cb to VCMSendStatisticsCallback.

BUG=webrtc:5687

Review-Url: https://codereview.webrtc.org/1900193004
Cr-Commit-Position: refs/heads/master@{#12596}

webrtc/modules/video_coding/generic_encoder.cc

@@ -72,11 +72,6 @@ int32_t VCMGenericEncoder::Encode(const VideoFrame& frame,
 
   int32_t result = encoder_->Encode(frame, codec_specific, &frame_types);
 
-  if (vcm_encoded_frame_callback_) {
-    vcm_encoded_frame_callback_->SignalLastEncoderImplementationUsed(
-        encoder_->ImplementationName());
-  }
-
   if (is_screenshare_ &&
       result == WEBRTC_VIDEO_CODEC_TARGET_BITRATE_OVERSHOOT) {
     // Target bitrate exceeded, encoder state has been reset - try again.
@@ -86,6 +81,10 @@ int32_t VCMGenericEncoder::Encode(const VideoFrame& frame,
   return result;
 }
 
+const char* VCMGenericEncoder::ImplementationName() const {
+  return encoder_->ImplementationName();
+}
+
 void VCMGenericEncoder::SetEncoderParameters(const EncoderParameters& params) {
   bool channel_parameters_have_changed;
   bool rates_have_changed;
@@ -139,20 +138,14 @@ bool VCMGenericEncoder::SupportsNativeHandle() const {
 }
 
 VCMEncodedFrameCallback::VCMEncodedFrameCallback(
-    EncodedImageCallback* post_encode_callback)
+    EncodedImageCallback* post_encode_callback,
-    : send_callback_(),
+    media_optimization::MediaOptimization* media_opt)
-      media_opt_(nullptr),
+    : internal_source_(false),
-      internal_source_(false),
+      post_encode_callback_(post_encode_callback),
-      post_encode_callback_(post_encode_callback) {}
+      media_opt_(media_opt) {}
 
 VCMEncodedFrameCallback::~VCMEncodedFrameCallback() {}
 
-int32_t VCMEncodedFrameCallback::SetTransportCallback(
-    VCMPacketizationCallback* transport) {
-  send_callback_ = transport;
-  return VCM_OK;
-}
-
 int32_t VCMEncodedFrameCallback::Encoded(
     const EncodedImage& encoded_image,
     const CodecSpecificInfo* codec_specific,
@@ -172,14 +165,4 @@ int32_t VCMEncodedFrameCallback::Encoded(
   return VCM_OK;
 }
 
-void VCMEncodedFrameCallback::SetMediaOpt(
-    media_optimization::MediaOptimization* mediaOpt) {
-  media_opt_ = mediaOpt;
-}
-
-void VCMEncodedFrameCallback::SignalLastEncoderImplementationUsed(
-    const char* implementation_name) {
-  if (send_callback_)
-    send_callback_->OnEncoderImplementationName(implementation_name);
-}
 }  // namespace webrtc

webrtc/modules/video_coding/generic_encoder.h

@@ -35,27 +35,22 @@ struct EncoderParameters {
 
 class VCMEncodedFrameCallback : public EncodedImageCallback {
  public:
-  explicit VCMEncodedFrameCallback(EncodedImageCallback* post_encode_callback);
+  VCMEncodedFrameCallback(EncodedImageCallback* post_encode_callback,
+                          media_optimization::MediaOptimization* media_opt);
   virtual ~VCMEncodedFrameCallback();
 
   // Implements EncodedImageCallback.
   int32_t Encoded(const EncodedImage& encoded_image,
                   const CodecSpecificInfo* codec_specific,
                   const RTPFragmentationHeader* fragmentation_header) override;
-  int32_t SetTransportCallback(VCMPacketizationCallback* transport);
-  void SetMediaOpt(media_optimization::MediaOptimization* media_opt);
   void SetInternalSource(bool internal_source) {
     internal_source_ = internal_source;
   }
-  void SignalLastEncoderImplementationUsed(
-      const char* encoder_implementation_name);
 
  private:
-  VCMPacketizationCallback* send_callback_;
-  media_optimization::MediaOptimization* media_opt_;
   bool internal_source_;
-
+  EncodedImageCallback* const post_encode_callback_;
-  EncodedImageCallback* post_encode_callback_;
+  media_optimization::MediaOptimization* const media_opt_;
 };
 
 class VCMGenericEncoder {
@@ -75,6 +70,8 @@ class VCMGenericEncoder {
                  const CodecSpecificInfo* codec_specific,
                  const std::vector<FrameType>& frame_types);
 
+  const char* ImplementationName() const;
+
   void SetEncoderParameters(const EncoderParameters& params);
   EncoderParameters GetEncoderParameters() const;
 

webrtc/modules/video_coding/include/video_coding.h

@@ -184,32 +184,6 @@ class VideoCodingModule : public Module {
   //                     < 0,    on error.
   virtual int32_t SetReceiveChannelParameters(int64_t rtt) = 0;
 
-  // Register a transport callback which will be called to deliver the encoded
-  // data and
-  // side information.
-  //
-  // Input:
-  //      - transport  : The callback object to register.
-  //
-  // Return value      : VCM_OK, on success.
-  //                     < 0,    on error.
-  virtual int32_t RegisterTransportCallback(
-      VCMPacketizationCallback* transport) = 0;
-
-  // Register video output information callback which will be called to deliver
-  // information
-  // about the video stream produced by the encoder, for instance the average
-  // frame rate and
-  // bit rate.
-  //
-  // Input:
-  //      - outputInformation  : The callback object to register.
-  //
-  // Return value      : VCM_OK, on success.
-  //                     < 0,    on error.
-  virtual int32_t RegisterSendStatisticsCallback(
-      VCMSendStatisticsCallback* sendStats) = 0;
-
   // Register a video protection callback which will be called to deliver
   // the requested FEC rate and NACK status (on/off).
   //

webrtc/modules/video_coding/include/video_coding_defines.h

@@ -11,6 +11,7 @@
 #ifndef WEBRTC_MODULES_VIDEO_CODING_INCLUDE_VIDEO_CODING_DEFINES_H_
 #define WEBRTC_MODULES_VIDEO_CODING_INCLUDE_VIDEO_CODING_DEFINES_H_
 
+#include <string>
 #include <vector>
 
 #include "webrtc/modules/include/module_common_types.h"
@@ -56,18 +57,6 @@ struct VCMFrameCount {
   uint32_t numDeltaFrames;
 };
 
-// Callback class used for sending data ready to be packetized
-// Deprecated.
-// TODO(perkj): Remove once OnEncoderImplementationName is not used.
-class VCMPacketizationCallback {
- public:
-  // TODO(perkj): Refactor this. It does not belong in VCMPacketizationCallback.
-  virtual void OnEncoderImplementationName(const char* implementation_name) {}
-
- protected:
-  virtual ~VCMPacketizationCallback() {}
-};
-
 // Callback class used for passing decoded frames which are ready to be
 // rendered.
 class VCMReceiveCallback {
@@ -84,13 +73,13 @@ class VCMReceiveCallback {
   virtual ~VCMReceiveCallback() {}
 };
 
-// Callback class used for informing the user of the bit rate and frame rate
+// Callback class used for informing the user of the bit rate and frame rate,
-// produced by the
+// and the name of the encoder.
-// encoder.
 class VCMSendStatisticsCallback {
  public:
-  virtual int32_t SendStatistics(const uint32_t bitRate,
+  virtual void SendStatistics(uint32_t bitRate,
-                                 const uint32_t frameRate) = 0;
+                              uint32_t frameRate,
+                              const std::string& encoder_name) = 0;
 
  protected:
   virtual ~VCMSendStatisticsCallback() {}

webrtc/modules/video_coding/video_coding_impl.cc

@@ -82,7 +82,8 @@ class VideoCodingModuleImpl : public VideoCodingModule {
         sender_(clock,
                 &post_encode_callback_,
                 encoder_rate_observer,
-                qm_settings_callback),
+                qm_settings_callback,
+                nullptr),
         receiver_(clock,
                   event_factory,
                   pre_decode_image_callback,
@@ -132,16 +133,6 @@ class VideoCodingModuleImpl : public VideoCodingModule {
     return sender_.SetChannelParameters(target_bitrate, lossRate, rtt);
   }
 
-  int32_t RegisterTransportCallback(
-      VCMPacketizationCallback* transport) override {
-    return sender_.RegisterTransportCallback(transport);
-  }
-
-  int32_t RegisterSendStatisticsCallback(
-      VCMSendStatisticsCallback* sendStats) override {
-    return sender_.RegisterSendStatisticsCallback(sendStats);
-  }
-
   int32_t RegisterProtectionCallback(
       VCMProtectionCallback* protection) override {
     return sender_.RegisterProtectionCallback(protection);

webrtc/modules/video_coding/video_coding_impl.h

@@ -14,6 +14,7 @@
 #include "webrtc/modules/video_coding/include/video_coding.h"
 
 #include <memory>
+#include <string>
 #include <vector>
 
 #include "webrtc/base/onetimeevent.h"
@@ -58,7 +59,8 @@ class VideoSender : public Module {
   VideoSender(Clock* clock,
               EncodedImageCallback* post_encode_callback,
               VideoEncoderRateObserver* encoder_rate_observer,
-              VCMQMSettingsCallback* qm_settings_callback);
+              VCMQMSettingsCallback* qm_settings_callback,
+              VCMSendStatisticsCallback* send_stats_callback);
 
   ~VideoSender();
 
@@ -79,10 +81,6 @@ class VideoSender : public Module {
                                uint8_t lossRate,
                                int64_t rtt);
 
-  // Deprecated. Use |post_encode_callback| instead.
-  // TODO(perkj): Remove once |OnEncoderImplementationName| is not used.
-  int32_t RegisterTransportCallback(VCMPacketizationCallback* transport);
-  int32_t RegisterSendStatisticsCallback(VCMSendStatisticsCallback* sendStats);
   int32_t RegisterProtectionCallback(VCMProtectionCallback* protection);
   void SetVideoProtection(VCMVideoProtection videoProtection);
 
@@ -105,12 +103,11 @@ class VideoSender : public Module {
 
   Clock* const clock_;
 
-  rtc::CriticalSection process_crit_;
   rtc::CriticalSection encoder_crit_;
   VCMGenericEncoder* _encoder;
-  VCMEncodedFrameCallback _encodedFrameCallback GUARDED_BY(encoder_crit_);
   media_optimization::MediaOptimization _mediaOpt;
-  VCMSendStatisticsCallback* _sendStatsCallback GUARDED_BY(process_crit_);
+  VCMEncodedFrameCallback _encodedFrameCallback GUARDED_BY(encoder_crit_);
+  VCMSendStatisticsCallback* const send_stats_callback_;
   VCMCodecDataBase _codecDataBase GUARDED_BY(encoder_crit_);
   bool frame_dropper_enabled_ GUARDED_BY(encoder_crit_);
   VCMProcessTimer _sendStatsTimer;
@@ -125,6 +122,7 @@ class VideoSender : public Module {
   rtc::CriticalSection params_crit_;
   EncoderParameters encoder_params_ GUARDED_BY(params_crit_);
   bool encoder_has_internal_source_ GUARDED_BY(params_crit_);
+  std::string encoder_name_ GUARDED_BY(params_crit_);
   std::vector<FrameType> next_frame_types_ GUARDED_BY(params_crit_);
 };
 

webrtc/modules/video_coding/video_sender.cc

@@ -27,12 +27,13 @@ namespace vcm {
 VideoSender::VideoSender(Clock* clock,
                          EncodedImageCallback* post_encode_callback,
                          VideoEncoderRateObserver* encoder_rate_observer,
-                         VCMQMSettingsCallback* qm_settings_callback)
+                         VCMQMSettingsCallback* qm_settings_callback,
+                         VCMSendStatisticsCallback* send_stats_callback)
     : clock_(clock),
       _encoder(nullptr),
-      _encodedFrameCallback(post_encode_callback),
       _mediaOpt(clock_),
-      _sendStatsCallback(nullptr),
+      _encodedFrameCallback(post_encode_callback, &_mediaOpt),
+      send_stats_callback_(send_stats_callback),
       _codecDataBase(encoder_rate_observer, &_encodedFrameCallback),
       frame_dropper_enabled_(true),
       _sendStatsTimer(1000, clock_),
@@ -54,12 +55,19 @@ VideoSender::~VideoSender() {}
 
 void VideoSender::Process() {
   if (_sendStatsTimer.TimeUntilProcess() == 0) {
+    // |_sendStatsTimer.Processed()| must be called. Otherwise
+    // VideoSender::Process() will be called in an infinite loop.
     _sendStatsTimer.Processed();
-    rtc::CritScope cs(&process_crit_);
+    if (send_stats_callback_) {
-    if (_sendStatsCallback != nullptr) {
       uint32_t bitRate = _mediaOpt.SentBitRate();
       uint32_t frameRate = _mediaOpt.SentFrameRate();
-      _sendStatsCallback->SendStatistics(bitRate, frameRate);
+      std::string encoder_name;
+      {
+        rtc::CritScope cs(&params_crit_);
+        // Copy the string here so that we don't hold |params_crit_| in the CB.
+        encoder_name = encoder_name_;
+      }
+      send_stats_callback_->SendStatistics(bitRate, frameRate, encoder_name);
     }
   }
 
@@ -235,24 +243,6 @@ void VideoSender::SetEncoderParameters(EncoderParameters params) {
     _encoder->SetEncoderParameters(params);
 }
 
-int32_t VideoSender::RegisterTransportCallback(
-    VCMPacketizationCallback* transport) {
-  rtc::CritScope lock(&encoder_crit_);
-  _encodedFrameCallback.SetMediaOpt(&_mediaOpt);
-  _encodedFrameCallback.SetTransportCallback(transport);
-  return VCM_OK;
-}
-
-// Register video output information callback which will be called to deliver
-// information about the video stream produced by the encoder, for instance the
-// average frame rate and bit rate.
-int32_t VideoSender::RegisterSendStatisticsCallback(
-    VCMSendStatisticsCallback* sendStats) {
-  rtc::CritScope cs(&process_crit_);
-  _sendStatsCallback = sendStats;
-  return VCM_OK;
-}
-
 // Register a video protection callback which will be called to deliver the
 // requested FEC rate and NACK status (on/off).
 // Note: this callback is assumed to only be registered once and before it is
@@ -329,9 +319,12 @@ int32_t VideoSender::AddVideoFrame(const VideoFrame& videoFrame,
     LOG(LS_ERROR) << "Failed to encode frame. Error code: " << ret;
     return ret;
   }
+
   {
-    // Change all keyframe requests to encode delta frames the next time.
     rtc::CritScope lock(&params_crit_);
+    encoder_name_ = _encoder->ImplementationName();
+
+    // Change all keyframe requests to encode delta frames the next time.
     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.

webrtc/modules/video_coding/video_sender_unittest.cc

@@ -180,8 +180,8 @@ class TestVideoSender : public ::testing::Test {
   TestVideoSender() : clock_(1000), encoded_frame_callback_(&clock_) {}
 
   void SetUp() override {
-    sender_.reset(
+    sender_.reset(new VideoSender(&clock_, &encoded_frame_callback_, nullptr,
-        new VideoSender(&clock_, &encoded_frame_callback_, nullptr, nullptr));
+                                  nullptr, nullptr));
   }
 
   void AddFrame() {

webrtc/video/send_statistics_proxy.cc

@@ -342,16 +342,14 @@ void SendStatisticsProxy::SetContentType(
   }
 }
 
-void SendStatisticsProxy::OnEncoderImplementationName(
+void SendStatisticsProxy::OnEncoderStatsUpdate(
-    const char* implementation_name) {
+    uint32_t framerate,
-  rtc::CritScope lock(&crit_);
+    uint32_t bitrate,
-  stats_.encoder_implementation_name = implementation_name;
+    const std::string& encoder_name) {
-}
-
-void SendStatisticsProxy::OnOutgoingRate(uint32_t framerate, uint32_t bitrate) {
   rtc::CritScope lock(&crit_);
   stats_.encode_frame_rate = framerate;
   stats_.media_bitrate_bps = bitrate;
+  stats_.encoder_implementation_name = encoder_name;
 }
 
 void SendStatisticsProxy::OnEncodedFrameTimeMeasured(

webrtc/video/send_statistics_proxy.h

@@ -53,8 +53,9 @@ class SendStatisticsProxy : public CpuOveruseMetricsObserver,
   // Used to update incoming frame rate.
   void OnIncomingFrame(int width, int height);
 
-  void OnEncoderImplementationName(const char* implementation_name);
+  void OnEncoderStatsUpdate(uint32_t framerate,
-  void OnOutgoingRate(uint32_t framerate, uint32_t bitrate);
+                            uint32_t bitrate,
+                            const std::string& encoder_name);
   void OnSuspendChange(bool is_suspended);
   void OnInactiveSsrc(uint32_t ssrc);
 

webrtc/video/send_statistics_proxy_unittest.cc

@@ -146,11 +146,13 @@ TEST_F(SendStatisticsProxyTest, EncodedBitrateAndFramerate) {
   int media_bitrate_bps = 500;
   int encode_fps = 29;
 
-  statistics_proxy_->OnOutgoingRate(encode_fps, media_bitrate_bps);
+  statistics_proxy_->OnEncoderStatsUpdate(encode_fps, media_bitrate_bps,
+                                          "encoder name");
 
   VideoSendStream::Stats stats = statistics_proxy_->GetStats();
   EXPECT_EQ(media_bitrate_bps, stats.media_bitrate_bps);
   EXPECT_EQ(encode_fps, stats.encode_frame_rate);
+  EXPECT_EQ("encoder name", stats.encoder_implementation_name);
 }
 
 TEST_F(SendStatisticsProxyTest, Suspended) {

webrtc/video/video_send_stream.cc

@@ -405,7 +405,6 @@ VideoSendStream::VideoSendStream(
   RTC_DCHECK(congestion_controller_);
   RTC_DCHECK(remb_);
 
-  RTC_CHECK(vie_encoder_.Init());
   encoder_feedback_.Init(config_.rtp.ssrcs, &vie_encoder_);
 
   // RTP/RTCP initialization.

webrtc/video/vie_encoder.cc

@@ -64,7 +64,11 @@ ViEEncoder::ViEEncoder(uint32_t number_of_cores,
       ssrcs_(ssrcs),
       vp_(VideoProcessing::Create()),
       qm_callback_(new QMVideoSettingsCallback(vp_.get())),
-      video_sender_(Clock::GetRealTimeClock(), this, this, qm_callback_.get()),
+      video_sender_(Clock::GetRealTimeClock(),
+                    this,
+                    this,
+                    qm_callback_.get(),
+                    this),
       stats_proxy_(stats_proxy),
       pre_encode_callback_(pre_encode_callback),
       overuse_detector_(overuse_detector),
@@ -84,23 +88,10 @@ ViEEncoder::ViEEncoder(uint32_t number_of_cores,
       picture_id_rpsi_(0),
       video_suspended_(false) {
   module_process_thread_->RegisterModule(&video_sender_);
-}
-
-bool ViEEncoder::Init() {
   vp_->EnableTemporalDecimation(true);
 
   // Enable/disable content analysis: off by default for now.
   vp_->EnableContentAnalysis(false);
-
-  // TODO(perkj): Remove  |RegisterTransportCallback| as soon as we don't use
-  // VCMPacketizationCallback::OnEncoderImplementationName.
-  if (video_sender_.RegisterTransportCallback(this) != 0) {
-    return false;
-  }
-  if (video_sender_.RegisterSendStatisticsCallback(this) != 0) {
-    return false;
-  }
-  return true;
 }
 
 vcm::VideoSender* ViEEncoder::video_sender() {
@@ -353,11 +344,6 @@ void ViEEncoder::OnSetRates(uint32_t bitrate_bps, int framerate) {
     stats_proxy_->OnSetRates(bitrate_bps, framerate);
 }
 
-void ViEEncoder::OnEncoderImplementationName(const char* implementation_name) {
-  if (stats_proxy_)
-    stats_proxy_->OnEncoderImplementationName(implementation_name);
-}
-
 int32_t ViEEncoder::Encoded(const EncodedImage& encoded_image,
                             const CodecSpecificInfo* codec_specific_info,
                             const RTPFragmentationHeader* fragmentation) {
@@ -403,11 +389,11 @@ int32_t ViEEncoder::Encoded(const EncodedImage& encoded_image,
   return success;
 }
 
-int32_t ViEEncoder::SendStatistics(const uint32_t bit_rate,
+void ViEEncoder::SendStatistics(uint32_t bit_rate,
-                                   const uint32_t frame_rate) {
+                                uint32_t frame_rate,
+                                const std::string& encoder_name) {
   if (stats_proxy_)
-    stats_proxy_->OnOutgoingRate(frame_rate, bit_rate);
+    stats_proxy_->OnEncoderStatsUpdate(frame_rate, bit_rate, encoder_name);
-  return 0;
 }
 
 void ViEEncoder::OnReceivedSLI(uint32_t /*ssrc*/,

webrtc/video/vie_encoder.h

@@ -12,6 +12,7 @@
 #define WEBRTC_VIDEO_VIE_ENCODER_H_
 
 #include <memory>
+#include <string>
 #include <vector>
 
 #include "webrtc/base/criticalsection.h"
@@ -52,7 +53,6 @@ class VideoEncoder;
 // 6. For each available raw video frame call EncodeVideoFrame.
 class ViEEncoder : public VideoEncoderRateObserver,
                    public EncodedImageCallback,
-                   public VCMPacketizationCallback,
                    public VCMSendStatisticsCallback {
  public:
   friend class ViEBitrateObserver;
@@ -67,8 +67,6 @@ class ViEEncoder : public VideoEncoderRateObserver,
              PacedSender* pacer);
   ~ViEEncoder();
 
-  bool Init();
-
   vcm::VideoSender* video_sender();
 
   void SetNetworkTransmissionState(bool is_transmitting);
@@ -102,17 +100,15 @@ class ViEEncoder : public VideoEncoderRateObserver,
   // Implements VideoEncoderRateObserver.
   void OnSetRates(uint32_t bitrate_bps, int framerate) override;
 
-  // Implements VCMPacketizationCallback.
-  void OnEncoderImplementationName(const char* implementation_name) override;
-
   // Implements EncodedImageCallback.
   int32_t Encoded(const EncodedImage& encoded_image,
                   const CodecSpecificInfo* codec_specific_info,
                   const RTPFragmentationHeader* fragmentation) override;
 
   // Implements VideoSendStatisticsCallback.
-  int32_t SendStatistics(const uint32_t bit_rate,
+  void SendStatistics(uint32_t bit_rate,
-                         const uint32_t frame_rate) override;
+                      uint32_t frame_rate,
+                      const std::string& encoder_name) override;
 
   // virtual to test EncoderStateFeedback with mocks.
   virtual void OnReceivedIntraFrameRequest(uint32_t ssrc);