Add overhead per packet observer to the rtp_sender.

webrtc/modules/rtp_rtcp/source/rtp_sender.cc

Index: webrtc/modules/rtp_rtcp/source/rtp_sender.cc
diff --git a/webrtc/modules/rtp_rtcp/source/rtp_sender.cc b/webrtc/modules/rtp_rtcp/source/rtp_sender.cc
index 0a244d13e9bd12f0faa6366786b542f2f89d9870..68fc5b1568610746a519861214290a09ad06db21 100644
--- a/webrtc/modules/rtp_rtcp/source/rtp_sender.cc
+++ b/webrtc/modules/rtp_rtcp/source/rtp_sender.cc
@@ -75,7 +75,8 @@ RTPSender::RTPSender(
     SendSideDelayObserver* send_side_delay_observer,
     RtcEventLog* event_log,
     SendPacketObserver* send_packet_observer,
-    RateLimiter* retransmission_rate_limiter)
+    RateLimiter* retransmission_rate_limiter,
+    OverheadSizeObserver* overhead_size_observer)
     : clock_(clock),
       // TODO(holmer): Remove this conversion?
       clock_delta_ms_(clock_->TimeInMilliseconds() - rtc::TimeMillis()),
@@ -116,7 +117,10 @@ RTPSender::RTPSender(
       last_packet_marker_bit_(false),
       csrcs_(),
       rtx_(kRtxOff),
-      retransmission_rate_limiter_(retransmission_rate_limiter) {
+      transport_overhead_bytes_per_packet_(0),
+      rtp_overhead_bytes_per_packet_(0),
+      retransmission_rate_limiter_(retransmission_rate_limiter),
+      overhead_size_observer_(overhead_size_observer) {
   ssrc_ = ssrc_db_->CreateSSRC();
   RTC_DCHECK(ssrc_ != 0);
   ssrc_rtx_ = ssrc_db_->CreateSSRC();
@@ -561,18 +565,15 @@ size_t RTPSender::DeprecatedSendPadData(size_t bytes,
           kTimestampTicksPerMs * (now_ms - capture_time_ms));
     }
     padding_packet.SetExtension<AbsoluteSendTime>(now_ms);
-
     PacketOptions options;
-    bool has_transport_seq_no =
+    bool has_transport_seq_num =
         UpdateTransportSequenceNumber(&padding_packet, &options.packet_id);
-
     padding_packet.SetPadding(padding_bytes_in_packet, &random_);
 
-    if (has_transport_seq_no && transport_feedback_observer_)
-      transport_feedback_observer_->AddPacket(
-          options.packet_id,
-          padding_packet.payload_size() + padding_packet.padding_size(),
-          probe_cluster_id);
+    if (has_transport_seq_num) {
+      AddPacketToTransportFeedback(options.packet_id, padding_packet,
+                                   probe_cluster_id);
+    }
 
     if (!SendPacketToNetwork(padding_packet, options))
       break;
@@ -630,6 +631,7 @@ bool RTPSender::SendPacketToNetwork(const RtpPacketToSend& packet,
                                     const PacketOptions& options) {
   int bytes_sent = -1;
   if (transport_) {
+    UpdateRtpOverheadSize(packet);
     bytes_sent = transport_->SendRtp(packet.data(), packet.size(), options)
                      ? static_cast<int>(packet.size())
                      : -1;
@@ -735,12 +737,9 @@ bool RTPSender::PrepareAndSendPacket(std::unique_ptr<RtpPacketToSend> packet,
   packet_to_send->SetExtension<AbsoluteSendTime>(now_ms);
 
   PacketOptions options;
-  if (UpdateTransportSequenceNumber(packet_to_send, &options.packet_id) &&
-      transport_feedback_observer_) {
-    transport_feedback_observer_->AddPacket(
-        options.packet_id,
-        packet_to_send->payload_size() + packet_to_send->padding_size(),
-        probe_cluster_id);
+  if (UpdateTransportSequenceNumber(packet_to_send, &options.packet_id)) {
+    AddPacketToTransportFeedback(options.packet_id, *packet_to_send,
+                                 probe_cluster_id);
   }
 
   if (!is_retransmit && !send_over_rtx) {
@@ -858,11 +857,9 @@ bool RTPSender::SendToNetwork(std::unique_ptr<RtpPacketToSend> packet,
   }
 
   PacketOptions options;
-  if (UpdateTransportSequenceNumber(packet.get(), &options.packet_id) &&
-      transport_feedback_observer_) {
-    transport_feedback_observer_->AddPacket(
-        options.packet_id, packet->payload_size() + packet->padding_size(),
-        PacketInfo::kNotAProbe);
+  if (UpdateTransportSequenceNumber(packet.get(), &options.packet_id)) {
+    AddPacketToTransportFeedback(options.packet_id, *packet.get(),
+                                 PacketInfo::kNotAProbe);
   }
 
   UpdateDelayStatistics(packet->capture_time_ms(), now_ms);
@@ -1240,4 +1237,47 @@ RtpState RTPSender::GetRtxRtpState() const {
   return state;
 }
 
+void RTPSender::SetTransportOverhead(int transport_overhead) {

[minyue-webrtc, 2016/11/16 08:48:59]

how about

void RTPSender::SetTransportOverheadSize(size_t
transport_overhead_bytes_per_packet)

[michaelt, 2016/11/16 09:27:09]

SetTransportOverheadSize is used in all the rest of the functions. 
Since the value is passed down from audio/video stream. We can change it but,
would prefer to have the same name for all the functions.
So maybe change it in a different CL ?

[minyue-webrtc, 2016/11/16 12:39:52]

I see. After rethinking, I think overhead has a sense of "size" in it. So no
need to add "size" after overhead. I'd hold back my suggestion about
OverheadSizeObserver. my apologies. OverheadObserver should be good.

BTW, how about my other part of the previous comment: size_t on
transport_overhead and adding _byte_per_packet?

[michaelt, 2016/11/16 13:29:37]

Done

[minyue-webrtc, 2016/11/16 15:13:38]

Have you uploaded new patch set?

[michaelt, 2016/11/17 08:25:53]

Now yes :)

+  if (!overhead_size_observer_)
+    return;
+  int overhead_bytes_per_packet = 0;
+  {
+    rtc::CritScope lock(&send_critsect_);
+    if (rtp_overhead_bytes_per_packet_ ==

[minyue-webrtc, 2016/11/16 08:48:59]

transport_overhead_bytes_per_packet_?

[michaelt, 2016/11/16 09:27:09]

Oops. I may should write a unit test for this :)

[minyue-webrtc, 2016/11/16 12:39:52]

no need for unittest (too many of these may possible), but need careful writing
and reviewing.

[michaelt, 2016/11/16 13:29:37]

I already added a test for this. Should I remove it ? 

[minyue-webrtc, 2016/11/16 15:13:38]

Ok. I missed it, can take a look now. I think the important failure we want to
catch in the test is a case that we should update but we do not update. We do
not care much if OnOverheadSizeChanged is called twice. So I would remove that
test.

+        static_cast<size_t>(transport_overhead)) {
+      return;
+    }
+    transport_overhead_bytes_per_packet_ = transport_overhead;
+    overhead_bytes_per_packet =
+        rtp_overhead_bytes_per_packet_ + transport_overhead_bytes_per_packet_;
+  }
+  overhead_size_observer_->OnOverheadSizeChanged(overhead_bytes_per_packet);
+}
+
+void RTPSender::AddPacketToTransportFeedback(uint16_t packet_id,

[minyue-webrtc, 2016/11/16 08:48:59]

To me, this help function does not bring much more value,  do we really need it?
How about the way before the change?

[michaelt, 2016/11/16 09:27:09]

To me the helper function brings a bit more value and i would like to keep it.
For example. We like to add the Overhead for the packets in future. Without this
function we have to change the code in three place, While with this function we
have to do it just at one place.
 

[minyue-webrtc, 2016/11/16 12:39:52]

Acknowledged.

+                                             const RtpPacketToSend& packet,
+                                             int probe_cluster_id) {
+  if (transport_feedback_observer_) {
+    transport_feedback_observer_->AddPacket(
+        packet_id, packet.payload_size() + packet.padding_size(),
+        probe_cluster_id);
+  }
+}
+
+void RTPSender::UpdateRtpOverheadSize(const RtpPacketToSend& packet) {
+  if (!overhead_size_observer_)
+    return;
+  int overhead_bytes_per_packet = 0;
+  {
+    rtc::CritScope lock(&send_critsect_);
+    if (rtp_overhead_bytes_per_packet_ == packet.headers_size()) {
+      return;
+    }
+    rtp_overhead_bytes_per_packet_ = packet.headers_size();
+    overhead_bytes_per_packet =
+        rtp_overhead_bytes_per_packet_ + transport_overhead_bytes_per_packet_;
+  }
+  overhead_size_observer_->OnOverheadSizeChanged(overhead_bytes_per_packet);
+}
+
 }  // namespace webrtc