Refactor registerable callbacks for VideoBitrateObserver from rtp_rtcp module into vie_channel.

R=stefan@webrtc.org

Review URL: https://webrtc-codereview.appspot.com/20879004

git-svn-id: http://webrtc.googlecode.com/svn/trunk@6626 4adac7df-926f-26a2-2b94-8c16560cd09d

webrtc/modules/rtp_rtcp/interface/rtp_rtcp.h

@@ -67,6 +67,7 @@ class RtpRtcp : public Module {
     RtpAudioFeedback* audio_messages;
     RemoteBitrateEstimator* remote_bitrate_estimator;
     PacedSender* paced_sender;
+    BitrateStatisticsObserver* send_bitrate_observer;
   };
 
   /*
@@ -308,13 +309,6 @@ class RtpRtcp : public Module {
                              uint32_t* fecRate,
                              uint32_t* nackRate) const = 0;
 
-    /*
-    *   Called on any new send bitrate estimate.
-    */
-    virtual void RegisterVideoBitrateObserver(
-        BitrateStatisticsObserver* observer) = 0;
-    virtual BitrateStatisticsObserver* GetVideoBitrateObserver() const = 0;
-
     /*
     *   Used by the codec module to deliver a video or audio frame for
     *   packetization.

webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.cc

@@ -37,7 +37,8 @@ RtpRtcp::Configuration::Configuration()
       rtt_stats(NULL),
       audio_messages(NullObjectRtpAudioFeedback()),
       remote_bitrate_estimator(NULL),
-      paced_sender(NULL) {
+      paced_sender(NULL),
+      send_bitrate_observer(NULL) {
 }
 
 RtpRtcp* RtpRtcp::CreateRtpRtcp(const RtpRtcp::Configuration& configuration) {
@@ -60,7 +61,8 @@ ModuleRtpRtcpImpl::ModuleRtpRtcpImpl(const Configuration& configuration)
                   configuration.clock,
                   configuration.outgoing_transport,
                   configuration.audio_messages,
-                  configuration.paced_sender),
+                  configuration.paced_sender,
+                  configuration.send_bitrate_observer),
       rtcp_sender_(configuration.id,
                    configuration.audio,
                    configuration.clock,
@@ -1234,16 +1236,6 @@ void ModuleRtpRtcpImpl::BitrateSent(uint32_t* total_rate,
     *nack_rate = rtp_sender_.NackOverheadRate();
 }
 
-void ModuleRtpRtcpImpl::RegisterVideoBitrateObserver(
-    BitrateStatisticsObserver* observer) {
-  assert(!IsDefaultModule());
-  rtp_sender_.RegisterBitrateObserver(observer);
-}
-
-BitrateStatisticsObserver* ModuleRtpRtcpImpl::GetVideoBitrateObserver() const {
-  return rtp_sender_.GetBitrateObserver();
-}
-
 void ModuleRtpRtcpImpl::OnRequestIntraFrame() {
   RequestKeyFrame();
 }

webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.h

@@ -344,11 +344,6 @@ class ModuleRtpRtcpImpl : public RtpRtcp {
                            uint32_t* fec_rate,
                            uint32_t* nackRate) const OVERRIDE;
 
-  virtual void RegisterVideoBitrateObserver(BitrateStatisticsObserver* observer)
-      OVERRIDE;
-
-  virtual BitrateStatisticsObserver* GetVideoBitrateObserver() const OVERRIDE;
-
   virtual uint32_t SendTimeOfSendReport(const uint32_t send_report);
 
   virtual bool SendTimeOfXrRrReport(uint32_t mid_ntp, int64_t* time_ms) const;

webrtc/modules/rtp_rtcp/source/rtp_sender.cc

@@ -45,7 +45,8 @@ RTPSender::RTPSender(const int32_t id,
                      Clock* clock,
                      Transport* transport,
                      RtpAudioFeedback* audio_feedback,
-                     PacedSender* paced_sender)
+                     PacedSender* paced_sender,
+                     BitrateStatisticsObserver* bitrate_callback)
     : clock_(clock),
       bitrate_sent_(clock, this),
       id_(id),
@@ -72,7 +73,7 @@ RTPSender::RTPSender(const int32_t id,
       statistics_crit_(CriticalSectionWrapper::CreateCriticalSection()),
       frame_count_observer_(NULL),
       rtp_stats_callback_(NULL),
-      bitrate_callback_(NULL),
+      bitrate_callback_(bitrate_callback),
       // RTP variables
       start_timestamp_forced_(false),
       start_timestamp_(0),
@@ -1684,16 +1685,6 @@ StreamDataCountersCallback* RTPSender::GetRtpStatisticsCallback() const {
   return rtp_stats_callback_;
 }
 
-void RTPSender::RegisterBitrateObserver(BitrateStatisticsObserver* observer) {
-  CriticalSectionScoped cs(statistics_crit_.get());
-  bitrate_callback_ = observer;
-}
-
-BitrateStatisticsObserver* RTPSender::GetBitrateObserver() const {
-  CriticalSectionScoped cs(statistics_crit_.get());
-  return bitrate_callback_;
-}
-
 uint32_t RTPSender::BitrateSent() const { return bitrate_sent_.BitrateLast(); }
 
 void RTPSender::BitrateUpdated(const BitrateStatistics& stats) {
@@ -1702,7 +1693,6 @@ void RTPSender::BitrateUpdated(const BitrateStatistics& stats) {
     CriticalSectionScoped ssrc_lock(send_critsect_);
     ssrc = ssrc_;
   }
-  CriticalSectionScoped cs(statistics_crit_.get());
   if (bitrate_callback_) {
     bitrate_callback_->Notify(stats, ssrc);
   }

webrtc/modules/rtp_rtcp/source/rtp_sender.h

@@ -69,7 +69,8 @@ class RTPSender : public RTPSenderInterface, public Bitrate::Observer {
  public:
   RTPSender(const int32_t id, const bool audio, Clock *clock,
             Transport *transport, RtpAudioFeedback *audio_feedback,
-            PacedSender *paced_sender);
+            PacedSender *paced_sender,
+            BitrateStatisticsObserver* bitrate_callback);
   virtual ~RTPSender();
 
   void ProcessBitrate();
@@ -275,10 +276,6 @@ class RTPSender : public RTPSenderInterface, public Bitrate::Observer {
   void RegisterRtpStatisticsCallback(StreamDataCountersCallback* callback);
   StreamDataCountersCallback* GetRtpStatisticsCallback() const;
 
-  // Called on new send bitrate estimate.
-  void RegisterBitrateObserver(BitrateStatisticsObserver* observer);
-  BitrateStatisticsObserver* GetBitrateObserver() const;
-
   uint32_t BitrateSent() const;
 
   virtual void BitrateUpdated(const BitrateStatistics& stats) OVERRIDE;
@@ -380,7 +377,7 @@ class RTPSender : public RTPSenderInterface, public Bitrate::Observer {
   StreamDataCounters rtp_stats_ GUARDED_BY(statistics_crit_);
   StreamDataCounters rtx_rtp_stats_ GUARDED_BY(statistics_crit_);
   StreamDataCountersCallback* rtp_stats_callback_ GUARDED_BY(statistics_crit_);
-  BitrateStatisticsObserver* bitrate_callback_ GUARDED_BY(statistics_crit_);
+  BitrateStatisticsObserver* const bitrate_callback_;
 
   // RTP variables
   bool start_timestamp_forced_ GUARDED_BY(send_critsect_);

webrtc/modules/rtp_rtcp/source/rtp_sender_unittest.cc

@@ -94,7 +94,7 @@ class RtpSenderTest : public ::testing::Test {
 
   virtual void SetUp() {
     rtp_sender_.reset(new RTPSender(0, false, &fake_clock_, &transport_, NULL,
-                                    &mock_paced_sender_));
+                                    &mock_paced_sender_, NULL));
     rtp_sender_->SetSequenceNumber(kSeqNum);
   }
 
@@ -672,7 +672,7 @@ TEST_F(RtpSenderTest, SendPadding) {
 TEST_F(RtpSenderTest, SendRedundantPayloads) {
   MockTransport transport;
   rtp_sender_.reset(new RTPSender(0, false, &fake_clock_, &transport, NULL,
-                                  &mock_paced_sender_));
+                                  &mock_paced_sender_, NULL));
   rtp_sender_->SetSequenceNumber(kSeqNum);
   // Make all packets go through the pacer.
   EXPECT_CALL(mock_paced_sender_,
@@ -865,6 +865,8 @@ TEST_F(RtpSenderTest, BitrateCallbacks) {
     uint32_t ssrc_;
     BitrateStatistics bitrate_;
   } callback;
+  rtp_sender_.reset(new RTPSender(0, false, &fake_clock_, &transport_, NULL,
+                                  &mock_paced_sender_, &callback));
 
   // Simulate kNumPackets sent with kPacketInterval ms intervals.
   const uint32_t kNumPackets = 15;
@@ -881,8 +883,6 @@ TEST_F(RtpSenderTest, BitrateCallbacks) {
   rtp_sender_->SetStorePacketsStatus(true, 1);
   uint32_t ssrc = rtp_sender_->SSRC();
 
-  rtp_sender_->RegisterBitrateObserver(&callback);
-
   // Initial process call so we get a new time window.
   rtp_sender_->ProcessBitrate();
   uint64_t start_time = fake_clock_.CurrentNtpInMilliseconds();
@@ -912,7 +912,7 @@ TEST_F(RtpSenderTest, BitrateCallbacks) {
   EXPECT_EQ((kPacketOverhead + sizeof(payload)) * 8 * expected_packet_rate,
             callback.bitrate_.bitrate_bps);
 
-  rtp_sender_->RegisterBitrateObserver(NULL);
+  rtp_sender_.reset();
 }
 
 class RtpSenderAudioTest : public RtpSenderTest {
@@ -922,7 +922,7 @@ class RtpSenderAudioTest : public RtpSenderTest {
   virtual void SetUp() {
     payload_ = kAudioPayload;
     rtp_sender_.reset(new RTPSender(0, true, &fake_clock_, &transport_, NULL,
-                                    &mock_paced_sender_));
+                                    &mock_paced_sender_, NULL));
     rtp_sender_->SetSequenceNumber(kSeqNum);
   }
 };

webrtc/video_engine/vie_channel.cc

@@ -116,6 +116,7 @@ ViEChannel::ViEChannel(int32_t channel_id,
   configuration.remote_bitrate_estimator = remote_bitrate_estimator;
   configuration.paced_sender = paced_sender;
   configuration.receive_statistics = vie_receiver_.GetReceiveStatistics();
+  configuration.send_bitrate_observer = &send_bitrate_observer_;
 
   rtp_rtcp_.reset(RtpRtcp::CreateRtpRtcp(configuration));
   vie_receiver_.SetRtpRtcpModule(rtp_rtcp_.get());
@@ -300,7 +301,6 @@ int32_t ViEChannel::SetSendCodec(const VideoCodec& video_codec,
       rtp_rtcp->RegisterSendFrameCountObserver(NULL);
       rtp_rtcp->RegisterSendChannelRtcpStatisticsCallback(NULL);
       rtp_rtcp->RegisterSendChannelRtpStatisticsCallback(NULL);
-      rtp_rtcp->RegisterVideoBitrateObserver(NULL);
       simulcast_rtp_rtcp_.pop_back();
       removed_rtp_rtcp_.push_front(rtp_rtcp);
     }
@@ -352,8 +352,6 @@ int32_t ViEChannel::SetSendCodec(const VideoCodec& video_codec,
           rtp_rtcp_->GetSendChannelRtcpStatisticsCallback());
       rtp_rtcp->RegisterSendChannelRtpStatisticsCallback(
           rtp_rtcp_->GetSendChannelRtpStatisticsCallback());
-      rtp_rtcp->RegisterVideoBitrateObserver(
-          rtp_rtcp_->GetVideoBitrateObserver());
     }
     // |RegisterSimulcastRtpRtcpModules| resets all old weak pointers and old
     // modules can be deleted after this step.
@@ -367,7 +365,6 @@ int32_t ViEChannel::SetSendCodec(const VideoCodec& video_codec,
       rtp_rtcp->RegisterSendFrameCountObserver(NULL);
       rtp_rtcp->RegisterSendChannelRtcpStatisticsCallback(NULL);
       rtp_rtcp->RegisterSendChannelRtpStatisticsCallback(NULL);
-      rtp_rtcp->RegisterVideoBitrateObserver(NULL);
       simulcast_rtp_rtcp_.pop_back();
       removed_rtp_rtcp_.push_front(rtp_rtcp);
     }
@@ -1212,13 +1209,7 @@ bool ViEChannel::GetSendSideDelay(int* avg_send_delay,
 
 void ViEChannel::RegisterSendBitrateObserver(
     BitrateStatisticsObserver* observer) {
-  rtp_rtcp_->RegisterVideoBitrateObserver(observer);
+  send_bitrate_observer_.Set(observer);
-  CriticalSectionScoped cs(rtp_rtcp_cs_.get());
-  for (std::list<RtpRtcp*>::const_iterator it = simulcast_rtp_rtcp_.begin();
-       it != simulcast_rtp_rtcp_.end();
-       it++) {
-    (*it)->RegisterVideoBitrateObserver(observer);
-  }
 }
 
 void ViEChannel::GetReceiveBandwidthEstimatorStats(
@@ -1536,7 +1527,6 @@ void ViEChannel::ReserveRtpRtcpModules(size_t num_modules) {
     rtp_rtcp->RegisterSendFrameCountObserver(NULL);
     rtp_rtcp->RegisterSendChannelRtcpStatisticsCallback(NULL);
     rtp_rtcp->RegisterSendChannelRtpStatisticsCallback(NULL);
-    rtp_rtcp->RegisterVideoBitrateObserver(NULL);
     removed_rtp_rtcp_.push_back(rtp_rtcp);
   }
 }

webrtc/video_engine/vie_channel.h

@@ -16,6 +16,7 @@
 #include "webrtc/modules/remote_bitrate_estimator/include/remote_bitrate_estimator.h"
 #include "webrtc/modules/rtp_rtcp/interface/rtp_rtcp_defines.h"
 #include "webrtc/modules/video_coding/main/interface/video_coding_defines.h"
+#include "webrtc/system_wrappers/interface/critical_section_wrapper.h"
 #include "webrtc/system_wrappers/interface/scoped_ptr.h"
 #include "webrtc/system_wrappers/interface/tick_util.h"
 #include "webrtc/typedefs.h"
@@ -382,6 +383,43 @@ class ViEChannel
   int GetRequiredNackListSize(int target_delay_ms);
   void SetRtxSendStatus(bool enable);
 
+  // ViEChannel exposes methods that allow to modify observers and callbacks
+  // to be modified. Such an API-style is cumbersome to implement and maintain
+  // at all the levels when comparing to only setting them at construction. As
+  // so this class instantiates its children with a wrapper that can be modified
+  // at a later time.
+  template <class T>
+  class RegisterableCallback : public T {
+   public:
+    RegisterableCallback()
+        : critsect_(CriticalSectionWrapper::CreateCriticalSection()),
+          callback_(NULL) {}
+
+    void Set(T* callback) {
+      CriticalSectionScoped cs(critsect_.get());
+      callback_ = callback;
+    }
+
+   protected:
+    // Note: this should be implemented with a RW-lock to allow simultaneous
+    // calls into the callback. However that doesn't seem to be needed for the
+    // current type of callbacks covered by this class.
+    scoped_ptr<CriticalSectionWrapper> critsect_;
+    T* callback_ GUARDED_BY(critsect_);
+
+   private:
+    DISALLOW_COPY_AND_ASSIGN(RegisterableCallback);
+  };
+
+  class : public RegisterableCallback<BitrateStatisticsObserver> {
+    virtual void Notify(const BitrateStatistics& stats, uint32_t ssrc) {
+      CriticalSectionScoped cs(critsect_.get());
+      if (callback_)
+        callback_->Notify(stats, ssrc);
+    }
+  }
+  send_bitrate_observer_;
+
   int32_t channel_id_;
   int32_t engine_id_;
   uint32_t number_of_cores_;