Add overhead per packet observer to the rtp_sender.

webrtc/modules/rtp_rtcp/source/rtp_sender.cc

 /*
  *  Copyright (c) 2012 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 57 matching lines @@
     Transport* transport,
     RtpPacketSender* paced_sender,
     FlexfecSender* flexfec_sender,
     TransportSequenceNumberAllocator* sequence_number_allocator,
     TransportFeedbackObserver* transport_feedback_observer,
     BitrateStatisticsObserver* bitrate_callback,
     FrameCountObserver* frame_count_observer,
     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()),
       random_(clock_->TimeInMicroseconds()),
       audio_configured_(audio),
       audio_(audio ? new RTPSenderAudio(clock, this) : nullptr),
       video_(audio ? nullptr : new RTPSenderVideo(clock, this, flexfec_sender)),
       paced_sender_(paced_sender),
       transport_sequence_number_allocator_(sequence_number_allocator),
       transport_feedback_observer_(transport_feedback_observer),
@@ skipping 20 matching lines @@
       remote_ssrc_(0),
       sequence_number_forced_(false),
       ssrc_forced_(false),
       last_rtp_timestamp_(0),
       capture_time_ms_(0),
       last_timestamp_time_ms_(0),
       media_has_been_sent_(false),
       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();
   RTC_DCHECK(ssrc_rtx_ != 0);
 
   // This random initialization is not intended to be cryptographic strong.
   timestamp_offset_ = random_.Rand<uint32_t>();
   // Random start, 16 bits. Can't be 0.
   sequence_number_rtx_ = random_.Rand(1, kMaxInitRtpSeqNumber);
   sequence_number_ = random_.Rand(1, kMaxInitRtpSeqNumber);
@@ skipping 424 matching lines @@
     padding_packet.SetTimestamp(timestamp);
     padding_packet.SetSsrc(ssrc);
 
     int64_t now_ms = clock_->TimeInMilliseconds();
 
     if (capture_time_ms > 0) {
       padding_packet.SetExtension<TransmissionOffset>(
           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;
 
     bytes_sent += padding_bytes_in_packet;
     UpdateRtpStats(padding_packet, over_rtx, false);
   }
 
   return bytes_sent;
 }
@@ skipping 37 matching lines @@
   if (!PrepareAndSendPacket(std::move(packet), rtx, true,
                             PacketInfo::kNotAProbe))
     return -1;
   return packet_size;
 }
 
 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;
     if (event_log_ && bytes_sent > 0) {
       event_log_->LogRtpHeader(kOutgoingPacket, MediaType::ANY, packet.data(),
                                packet.size());
     }
   }
   TRACE_EVENT_INSTANT2(TRACE_DISABLED_BY_DEFAULT("webrtc_rtp"),
                        "RTPSender::SendPacketToNetwork", "size", packet.size(),
@@ skipping 85 matching lines @@
     packet_to_send = packet_rtx.get();
   }
 
   int64_t now_ms = clock_->TimeInMilliseconds();
   int64_t diff_ms = now_ms - capture_time_ms;
   packet_to_send->SetExtension<TransmissionOffset>(kTimestampTicksPerMs *
                                                    diff_ms);
   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) {
     UpdateDelayStatistics(packet->capture_time_ms(), now_ms);
     UpdateOnSendPacket(options.packet_id, packet->capture_time_ms(),
                        packet->Ssrc());
   }
 
   if (!SendPacketToNetwork(*packet_to_send, options))
     return false;
@@ skipping 97 matching lines @@
         corrected_time_ms > last_capture_time_ms_sent_) {
       last_capture_time_ms_sent_ = corrected_time_ms;
       TRACE_EVENT_ASYNC_BEGIN1(TRACE_DISABLED_BY_DEFAULT("webrtc_rtp"),
                                "PacedSend", corrected_time_ms,
                                "capture_time_ms", corrected_time_ms);
     }
     return true;
   }
 
   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);
   UpdateOnSendPacket(options.packet_id, packet->capture_time_ms(),
                      packet->Ssrc());
 
   bool sent = SendPacketToNetwork(*packet, options);
 
   if (sent) {
     {
@@ skipping 357 matching lines @@
 RtpState RTPSender::GetRtxRtpState() const {
   rtc::CritScope lock(&send_critsect_);
 
   RtpState state;
   state.sequence_number = sequence_number_rtx_;
   state.start_timestamp = timestamp_offset_;
 
   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