Send RTCP packets via RtcpPacket callback

webrtc/modules/rtp_rtcp/source/rtcp_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 56 matching lines @@
       packets_sent(0),
       media_bytes_sent(0),
       send_bitrate(0),
       last_rr_ntp_secs(0),
       last_rr_ntp_frac(0),
       remote_sr(0),
       has_last_xr_rr(false),
       module(nullptr) {
 }
 
-struct RTCPSender::RtcpContext {
-  RtcpContext(const FeedbackState& feedback_state,
+class RTCPSender::RtcpContext : public rtcp::RtcpPacket::PacketReadyCallback {
+ public:
+  RtcpContext(Transport* transport,
+              const FeedbackState& feedback_state,
               int32_t nack_size,
               const uint16_t* nack_list,
               bool repeat,
               uint64_t picture_id,
-              uint8_t* buffer,
-              uint32_t buffer_size)
-      : feedback_state(feedback_state),
-        nack_size(nack_size),
-        nack_list(nack_list),
-        repeat(repeat),
-        picture_id(picture_id),
-        buffer(buffer),
-        buffer_size(buffer_size),
-        ntp_sec(0),
-        ntp_frac(0),
-        position(0) {}
+              uint32_t ntp_sec,
+              uint32_t ntp_frac)
+      : transport_(transport),
+        buffer_{},

[åsapersson, 2015/11/16 14:54:39]

needed?

[åsapersson, 2015/12/01 13:49:20]

see comment

[sprang_webrtc, 2015/12/02 10:33:26]

Done.

+        position_(0),
+        bytes_sent_(0),
+        feedback_state_(feedback_state),
+        nack_size_(nack_size),
+        nack_list_(nack_list),
+        repeat_(repeat),
+        picture_id_(picture_id),
+        ntp_sec_(ntp_sec),
+        ntp_frac_(ntp_frac) {}
 
-  uint8_t* AllocateData(uint32_t bytes) {
-    RTC_DCHECK_LE(position + bytes, buffer_size);
-    uint8_t* ptr = &buffer[position];
-    position += bytes;
-    return ptr;
+  virtual ~RtcpContext() {}
+
+  void OnPacketReady(uint8_t* data, size_t length) override {
+    if (transport_->SendRtcp(data, length))
+      bytes_sent_ += length;
+    position_ = 0;
   }
 
-  const FeedbackState& feedback_state;
-  int32_t nack_size;
-  const uint16_t* nack_list;
-  bool repeat;
-  uint64_t picture_id;
-  uint8_t* buffer;
-  uint32_t buffer_size;
-  uint32_t ntp_sec;
-  uint32_t ntp_frac;
-  uint32_t position;
-};
-
-// TODO(sprang): Once all builders use RtcpPacket, call SendToNetwork() here.
-class RTCPSender::PacketBuiltCallback
-    : public rtcp::RtcpPacket::PacketReadyCallback {
- public:
-  PacketBuiltCallback(RtcpContext* context) : context_(context) {}
-  virtual ~PacketBuiltCallback() {}
-  void OnPacketReady(uint8_t* data, size_t length) override {
-    context_->position += length;
-  }
   bool BuildPacket(const rtcp::RtcpPacket& packet) {

[åsapersson, 2015/10/13 08:50:12]

empty.Append(packet); ?

[sprang_webrtc, 2015/12/02 10:33:26]

Done.

-    return packet.BuildExternalBuffer(
-        &context_->buffer[context_->position],
-        context_->buffer_size - context_->position, this);
+    return packet.CreateAndAddAppended(buffer_, &position_, kBufferSize, this);
   }
 
- private:
-  RtcpContext* const context_;
+  void SendRemaining() {
+    if (position_ > 0)
+      OnPacketReady(buffer_, position_);
+  }
+
+  static const size_t kBufferSize = IP_PACKET_SIZE - 24;
+
+  Transport* const transport_;
+  uint8_t buffer_[kBufferSize];
+  size_t position_;
+  uint32_t bytes_sent_;
+
+  const FeedbackState& feedback_state_;
+  const int32_t nack_size_;
+  const uint16_t* nack_list_;
+  const bool repeat_;
+  const uint64_t picture_id_;
+  const uint32_t ntp_sec_;
+  const uint32_t ntp_frac_;
 };
 
 RTCPSender::RTCPSender(
     bool audio,
     Clock* clock,
     ReceiveStatistics* receive_statistics,
     RtcpPacketTypeCounterObserver* packet_type_counter_observer,
     Transport* outgoing_transport)
     : audio_(audio),
       clock_(clock),
@@ skipping 319 matching lines @@
   block->WithFractionLost(report_block.fractionLost);
   block->WithCumulativeLost(report_block.cumulativeLost);
   block->WithExtHighestSeqNum(report_block.extendedHighSeqNum);
   block->WithJitter(report_block.jitter);
   block->WithLastSr(report_block.lastSR);
   block->WithDelayLastSr(report_block.delaySinceLastSR);
 
   return 0;
 }
 
-RTCPSender::BuildResult RTCPSender::BuildSR(RtcpContext* ctx) {
+bool RTCPSender::BuildSR(RtcpContext* ctx) {
   for (int i = (RTCP_NUMBER_OF_SR - 2); i >= 0; i--) {
     // shift old
     last_send_report_[i + 1] = last_send_report_[i];
     last_rtcp_time_[i + 1] = last_rtcp_time_[i];
   }
 
-  last_rtcp_time_[0] = Clock::NtpToMs(ctx->ntp_sec, ctx->ntp_frac);
-  last_send_report_[0] = (ctx->ntp_sec << 16) + (ctx->ntp_frac >> 16);
+  last_rtcp_time_[0] = Clock::NtpToMs(ctx->ntp_sec_, ctx->ntp_frac_);
+  last_send_report_[0] = (ctx->ntp_sec_ << 16) + (ctx->ntp_frac_ >> 16);
 
   // The timestamp of this RTCP packet should be estimated as the timestamp of
   // the frame being captured at this moment. We are calculating that
   // timestamp as the last frame's timestamp + the time since the last frame
   // was captured.
   uint32_t rtp_timestamp =
       start_timestamp_ + last_rtp_timestamp_ +
       (clock_->TimeInMilliseconds() - last_frame_capture_time_ms_) *
-          (ctx->feedback_state.frequency_hz / 1000);
+          (ctx->feedback_state_.frequency_hz / 1000);
 
   rtcp::SenderReport report;
   report.From(ssrc_);
-  report.WithNtpSec(ctx->ntp_sec);
-  report.WithNtpFrac(ctx->ntp_frac);
+  report.WithNtpSec(ctx->ntp_sec_);
+  report.WithNtpFrac(ctx->ntp_frac_);
   report.WithRtpTimestamp(rtp_timestamp);
-  report.WithPacketCount(ctx->feedback_state.packets_sent);
-  report.WithOctetCount(ctx->feedback_state.media_bytes_sent);
+  report.WithPacketCount(ctx->feedback_state_.packets_sent);
+  report.WithOctetCount(ctx->feedback_state_.media_bytes_sent);
 
   for (auto it : report_blocks_)
     report.WithReportBlock(it.second);
 
-  PacketBuiltCallback callback(ctx);
-  if (!callback.BuildPacket(report))
-    return BuildResult::kTruncated;
+  if (!ctx->BuildPacket(report))

[åsapersson, 2015/10/13 08:50:12]

Shouldn't the packet be appended here (e.g. appended on an rtcp::Empty packet)?

[sprang_webrtc, 2015/12/02 10:33:26]

Done.

+    return false;
 
   report_blocks_.clear();
-  return BuildResult::kSuccess;
+  return true;
 }
 
-RTCPSender::BuildResult RTCPSender::BuildSDES(RtcpContext* ctx) {
+bool RTCPSender::BuildSDES(RtcpContext* ctx) {
   size_t length_cname = cname_.length();
   RTC_CHECK_LT(length_cname, static_cast<size_t>(RTCP_CNAME_SIZE));
 
   rtcp::Sdes sdes;
   sdes.WithCName(ssrc_, cname_);
 
   for (const auto it : csrc_cnames_)
     sdes.WithCName(it.first, it.second);
 
-  PacketBuiltCallback callback(ctx);
-  if (!callback.BuildPacket(sdes))
-    return BuildResult::kTruncated;
-
-  return BuildResult::kSuccess;
+  return ctx->BuildPacket(sdes);
 }
 
-RTCPSender::BuildResult RTCPSender::BuildRR(RtcpContext* ctx) {
+bool RTCPSender::BuildRR(RtcpContext* ctx) {
   rtcp::ReceiverReport report;
   report.From(ssrc_);
   for (auto it : report_blocks_)
     report.WithReportBlock(it.second);
 
-  PacketBuiltCallback callback(ctx);
-  if (!callback.BuildPacket(report))
-    return BuildResult::kTruncated;
+  if (!ctx->BuildPacket(report))
+    return false;
 
   report_blocks_.clear();
-
-  return BuildResult::kSuccess;
+  return true;
 }
 
-RTCPSender::BuildResult RTCPSender::BuildPLI(RtcpContext* ctx) {
+bool RTCPSender::BuildPLI(RtcpContext* ctx) {
   rtcp::Pli pli;
   pli.From(ssrc_);
   pli.To(remote_ssrc_);
 
-  PacketBuiltCallback callback(ctx);
-  if (!callback.BuildPacket(pli))
-    return BuildResult::kTruncated;
+  if (!ctx->BuildPacket(pli))
+    return false;
 
   TRACE_EVENT_INSTANT0(TRACE_DISABLED_BY_DEFAULT("webrtc_rtp"),
                        "RTCPSender::PLI");
   ++packet_type_counter_.pli_packets;
   TRACE_COUNTER_ID1(TRACE_DISABLED_BY_DEFAULT("webrtc_rtp"), "RTCP_PLICount",
                     ssrc_, packet_type_counter_.pli_packets);
 
-  return BuildResult::kSuccess;
+  return true;
 }
 
-RTCPSender::BuildResult RTCPSender::BuildFIR(RtcpContext* ctx) {
-  if (!ctx->repeat)
+bool RTCPSender::BuildFIR(RtcpContext* ctx) {
+  if (!ctx->repeat_)
     ++sequence_number_fir_;  // Do not increase if repetition.
 
   rtcp::Fir fir;
   fir.From(ssrc_);
   fir.To(remote_ssrc_);
   fir.WithCommandSeqNum(sequence_number_fir_);
 
-  PacketBuiltCallback callback(ctx);
-  if (!callback.BuildPacket(fir))
-    return BuildResult::kTruncated;
+  if (!ctx->BuildPacket(fir))
+    return false;
 
   TRACE_EVENT_INSTANT0(TRACE_DISABLED_BY_DEFAULT("webrtc_rtp"),
                        "RTCPSender::FIR");
   ++packet_type_counter_.fir_packets;
   TRACE_COUNTER_ID1(TRACE_DISABLED_BY_DEFAULT("webrtc_rtp"), "RTCP_FIRCount",
                     ssrc_, packet_type_counter_.fir_packets);
 
-  return BuildResult::kSuccess;
+  return true;
 }
 
 /*
     0                   1                   2                   3
     0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1
    +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
    |            First        |        Number           | PictureID |
    +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
 */
-RTCPSender::BuildResult RTCPSender::BuildSLI(RtcpContext* ctx) {
+bool RTCPSender::BuildSLI(RtcpContext* ctx) {
   rtcp::Sli sli;
   sli.From(ssrc_);
   sli.To(remote_ssrc_);
   // Crop picture id to 6 least significant bits.
-  sli.WithPictureId(ctx->picture_id & 0x3F);
+  sli.WithPictureId(ctx->picture_id_ & 0x3F);
   sli.WithFirstMb(0);
   sli.WithNumberOfMb(0x1FFF);  // 13 bits, only ones for now.
 
-  PacketBuiltCallback callback(ctx);
-  if (!callback.BuildPacket(sli))
-    return BuildResult::kTruncated;
-
-  return BuildResult::kSuccess;
+  return ctx->BuildPacket(sli);
 }
 
 /*
     0                   1                   2                   3
     0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1
    +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
    |      PB       |0| Payload Type|    Native RPSI bit string     |
    +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
    |   defined per codec          ...                | Padding (0) |
    +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
 */
 /*
 *    Note: not generic made for VP8
 */
-RTCPSender::BuildResult RTCPSender::BuildRPSI(RtcpContext* ctx) {
-  if (ctx->feedback_state.send_payload_type == 0xFF)
-    return BuildResult::kError;
+bool RTCPSender::BuildRPSI(RtcpContext* ctx) {
+  if (ctx->feedback_state_.send_payload_type == 0xFF)
+    return false;
 
   rtcp::Rpsi rpsi;
   rpsi.From(ssrc_);
   rpsi.To(remote_ssrc_);
-  rpsi.WithPayloadType(ctx->feedback_state.send_payload_type);
-  rpsi.WithPictureId(ctx->picture_id);
+  rpsi.WithPayloadType(ctx->feedback_state_.send_payload_type);
+  rpsi.WithPictureId(ctx->picture_id_);
 
-  PacketBuiltCallback callback(ctx);
-  if (!callback.BuildPacket(rpsi))
-    return BuildResult::kTruncated;
-
-  return BuildResult::kSuccess;
+  return ctx->BuildPacket(rpsi);
 }
 
-RTCPSender::BuildResult RTCPSender::BuildREMB(RtcpContext* ctx) {
+bool RTCPSender::BuildREMB(RtcpContext* ctx) {
   rtcp::Remb remb;
   remb.From(ssrc_);
   for (uint32_t ssrc : remb_ssrcs_)
     remb.AppliesTo(ssrc);
   remb.WithBitrateBps(remb_bitrate_);
 
-  PacketBuiltCallback callback(ctx);
-  if (!callback.BuildPacket(remb))
-    return BuildResult::kTruncated;
+  if (!ctx->BuildPacket(remb))
+    return false;
 
   TRACE_EVENT_INSTANT0(TRACE_DISABLED_BY_DEFAULT("webrtc_rtp"),
                        "RTCPSender::REMB");
 
-  return BuildResult::kSuccess;
+  return true;
 }
 
 void RTCPSender::SetTargetBitrate(unsigned int target_bitrate) {
   CriticalSectionScoped lock(critical_section_rtcp_sender_.get());
   tmmbr_send_ = target_bitrate / 1000;
 }
 
-RTCPSender::BuildResult RTCPSender::BuildTMMBR(RtcpContext* ctx) {
-  if (ctx->feedback_state.module == NULL)
-    return BuildResult::kError;
+bool RTCPSender::BuildTMMBR(RtcpContext* ctx) {
+  if (ctx->feedback_state_.module == nullptr)
+    return false;
   // Before sending the TMMBR check the received TMMBN, only an owner is
   // allowed to raise the bitrate:
   // * If the sender is an owner of the TMMBN -> send TMMBR
   // * If not an owner but the TMMBR would enter the TMMBN -> send TMMBR
 
   // get current bounding set from RTCP receiver
   bool tmmbrOwner = false;
   // store in candidateSet, allocates one extra slot
   TMMBRSet* candidateSet = tmmbr_help_.CandidateSet();
 
   // holding critical_section_rtcp_sender_ while calling RTCPreceiver which
   // will accuire criticalSectionRTCPReceiver_ is a potental deadlock but
   // since RTCPreceiver is not doing the reverse we should be fine
   int32_t lengthOfBoundingSet =
-      ctx->feedback_state.module->BoundingSet(tmmbrOwner, candidateSet);
+      ctx->feedback_state_.module->BoundingSet(tmmbrOwner, candidateSet);
 
   if (lengthOfBoundingSet > 0) {
     for (int32_t i = 0; i < lengthOfBoundingSet; i++) {
       if (candidateSet->Tmmbr(i) == tmmbr_send_ &&
           candidateSet->PacketOH(i) == packet_oh_send_) {
-        // do not send the same tuple
-        return BuildResult::kAborted;
+        // Do not send the same tuple.
+        return true;
       }
     }
     if (!tmmbrOwner) {
       // use received bounding set as candidate set
       // add current tuple
       candidateSet->SetEntry(lengthOfBoundingSet, tmmbr_send_, packet_oh_send_,
                              ssrc_);
       int numCandidates = lengthOfBoundingSet + 1;
 
       // find bounding set
-      TMMBRSet* boundingSet = NULL;
+      TMMBRSet* boundingSet = nullptr;
       int numBoundingSet = tmmbr_help_.FindTMMBRBoundingSet(boundingSet);
       if (numBoundingSet > 0 || numBoundingSet <= numCandidates)
         tmmbrOwner = tmmbr_help_.IsOwner(ssrc_, numBoundingSet);
       if (!tmmbrOwner) {
-        // did not enter bounding set, no meaning to send this request
-        return BuildResult::kAborted;
+        // Did not enter bounding set, no meaning to send this request.
+        return true;
       }
     }
   }
 
   if (tmmbr_send_) {
     rtcp::Tmmbr tmmbr;
     tmmbr.From(ssrc_);
     tmmbr.To(remote_ssrc_);
     tmmbr.WithBitrateKbps(tmmbr_send_);
     tmmbr.WithOverhead(packet_oh_send_);
 
-    PacketBuiltCallback callback(ctx);
-    if (!callback.BuildPacket(tmmbr))
-      return BuildResult::kTruncated;
+    return ctx->BuildPacket(tmmbr);
   }
-  return BuildResult::kSuccess;
+  return true;
 }
 
-RTCPSender::BuildResult RTCPSender::BuildTMMBN(RtcpContext* ctx) {
+bool RTCPSender::BuildTMMBN(RtcpContext* ctx) {
   TMMBRSet* boundingSet = tmmbr_help_.BoundingSetToSend();
-  if (boundingSet == NULL)
-    return BuildResult::kError;
+  if (boundingSet == nullptr)
+    return false;
 
   rtcp::Tmmbn tmmbn;
   tmmbn.From(ssrc_);
   for (uint32_t i = 0; i < boundingSet->lengthOfSet(); i++) {
     if (boundingSet->Tmmbr(i) > 0) {
       tmmbn.WithTmmbr(boundingSet->Ssrc(i), boundingSet->Tmmbr(i),
                       boundingSet->PacketOH(i));
     }
   }
 
-  PacketBuiltCallback callback(ctx);
-  if (!callback.BuildPacket(tmmbn))
-    return BuildResult::kTruncated;
-
-  return BuildResult::kSuccess;
+  return ctx->BuildPacket(tmmbn);
 }
 
-RTCPSender::BuildResult RTCPSender::BuildAPP(RtcpContext* ctx) {
+bool RTCPSender::BuildAPP(RtcpContext* ctx) {
   rtcp::App app;
   app.From(ssrc_);
   app.WithSubType(app_sub_type_);
   app.WithName(app_name_);
   app.WithData(app_data_.get(), app_length_);
 
-  PacketBuiltCallback callback(ctx);
-  if (!callback.BuildPacket(app))
-    return BuildResult::kTruncated;
-
-  return BuildResult::kSuccess;
+  return ctx->BuildPacket(app);
 }
 
-RTCPSender::BuildResult RTCPSender::BuildNACK(RtcpContext* ctx) {
-  // sanity
-  if (ctx->position + 16 >= IP_PACKET_SIZE) {
-    LOG(LS_WARNING) << "Failed to build NACK.";
-    return BuildResult::kTruncated;
-  }
+bool RTCPSender::BuildNACK(RtcpContext* ctx) {
+  rtcp::Nack nack;
+  nack.From(ssrc_);
+  nack.To(remote_ssrc_);
+  nack.WithList(ctx->nack_list_, ctx->nack_size_);
 
-  // int size, uint16_t* nack_list
-  // add nack list
-  uint8_t FMT = 1;
-  *ctx->AllocateData(1) = 0x80 + FMT;
-  *ctx->AllocateData(1) = 205;
-
-  *ctx->AllocateData(1) = 0;
-  int nack_size_pos_ = ctx->position;
-  *ctx->AllocateData(1) = 3;  // setting it to one kNACK signal as default
-
-  // Add our own SSRC
-  ByteWriter<uint32_t>::WriteBigEndian(ctx->AllocateData(4), ssrc_);
-
-  // Add the remote SSRC
-  ByteWriter<uint32_t>::WriteBigEndian(ctx->AllocateData(4), remote_ssrc_);
-
-  // Build NACK bitmasks and write them to the RTCP message.
-  // The nack list should be sorted and not contain duplicates if one
-  // wants to build the smallest rtcp nack packet.
-  int numOfNackFields = 0;
-  int maxNackFields =
-      std::min<int>(kRtcpMaxNackFields, (IP_PACKET_SIZE - ctx->position) / 4);
-  int i = 0;
-  while (i < ctx->nack_size && numOfNackFields < maxNackFields) {
-    uint16_t nack = ctx->nack_list[i++];
-    uint16_t bitmask = 0;
-    while (i < ctx->nack_size) {
-      int shift = static_cast<uint16_t>(ctx->nack_list[i] - nack) - 1;
-      if (shift >= 0 && shift <= 15) {
-        bitmask |= (1 << shift);
-        ++i;
-      } else {
-        break;
-      }
-    }
-    // Write the sequence number and the bitmask to the packet.
-    assert(ctx->position + 4 < IP_PACKET_SIZE);
-    ByteWriter<uint16_t>::WriteBigEndian(ctx->AllocateData(2), nack);
-    ByteWriter<uint16_t>::WriteBigEndian(ctx->AllocateData(2), bitmask);
-    numOfNackFields++;
-  }
-  ctx->buffer[nack_size_pos_] = static_cast<uint8_t>(2 + numOfNackFields);
-
-  if (i != ctx->nack_size)
-    LOG(LS_WARNING) << "Nack list too large for one packet.";
+  if (!ctx->BuildPacket(nack))
+    return false;
 
   // Report stats.
   NACKStringBuilder stringBuilder;
-  for (int idx = 0; idx < i; ++idx) {
-    stringBuilder.PushNACK(ctx->nack_list[idx]);
-    nack_stats_.ReportRequest(ctx->nack_list[idx]);
+  for (int idx = 0; idx < ctx->nack_size_; ++idx) {
+    stringBuilder.PushNACK(ctx->nack_list_[idx]);
+    nack_stats_.ReportRequest(ctx->nack_list_[idx]);
   }
   packet_type_counter_.nack_requests = nack_stats_.requests();
   packet_type_counter_.unique_nack_requests = nack_stats_.unique_requests();
 
   TRACE_EVENT_INSTANT1(TRACE_DISABLED_BY_DEFAULT("webrtc_rtp"),
                        "RTCPSender::NACK", "nacks",
                        TRACE_STR_COPY(stringBuilder.GetResult().c_str()));
   ++packet_type_counter_.nack_packets;
   TRACE_COUNTER_ID1(TRACE_DISABLED_BY_DEFAULT("webrtc_rtp"), "RTCP_NACKCount",
                     ssrc_, packet_type_counter_.nack_packets);
 
-  return BuildResult::kSuccess;
+  return true;
 }
 
-RTCPSender::BuildResult RTCPSender::BuildBYE(RtcpContext* ctx) {
+bool RTCPSender::BuildBYE(RtcpContext* ctx) {
   rtcp::Bye bye;
   bye.From(ssrc_);
   for (uint32_t csrc : csrcs_)
     bye.WithCsrc(csrc);
 
-  PacketBuiltCallback callback(ctx);
-  if (!callback.BuildPacket(bye))
-    return BuildResult::kTruncated;
-
-  return BuildResult::kSuccess;
+  return ctx->BuildPacket(bye);
 }
 
-RTCPSender::BuildResult RTCPSender::BuildReceiverReferenceTime(
-    RtcpContext* ctx) {
-
+bool RTCPSender::BuildReceiverReferenceTime(RtcpContext* ctx) {
   if (last_xr_rr_.size() >= RTCP_NUMBER_OF_SR)
     last_xr_rr_.erase(last_xr_rr_.begin());
   last_xr_rr_.insert(std::pair<uint32_t, int64_t>(
-      RTCPUtility::MidNtp(ctx->ntp_sec, ctx->ntp_frac),
-      Clock::NtpToMs(ctx->ntp_sec, ctx->ntp_frac)));
+      RTCPUtility::MidNtp(ctx->ntp_sec_, ctx->ntp_frac_),
+      Clock::NtpToMs(ctx->ntp_sec_, ctx->ntp_frac_)));
 
   rtcp::Xr xr;
   xr.From(ssrc_);
 
   rtcp::Rrtr rrtr;
-  rrtr.WithNtpSec(ctx->ntp_sec);
-  rrtr.WithNtpFrac(ctx->ntp_frac);
+  rrtr.WithNtpSec(ctx->ntp_sec_);
+  rrtr.WithNtpFrac(ctx->ntp_frac_);
 
   xr.WithRrtr(&rrtr);
 
   // TODO(sprang): Merge XR report sending to contain all of RRTR, DLRR, VOIP?
 
-  PacketBuiltCallback callback(ctx);
-  if (!callback.BuildPacket(xr))
-    return BuildResult::kTruncated;
-
-  return BuildResult::kSuccess;
+  return ctx->BuildPacket(xr);
 }
 
-RTCPSender::BuildResult RTCPSender::BuildDlrr(RtcpContext* ctx) {
+bool RTCPSender::BuildDlrr(RtcpContext* ctx) {
   rtcp::Xr xr;
   xr.From(ssrc_);
 
   rtcp::Dlrr dlrr;
-  const RtcpReceiveTimeInfo& info = ctx->feedback_state.last_xr_rr;
+  const RtcpReceiveTimeInfo& info = ctx->feedback_state_.last_xr_rr;
   dlrr.WithDlrrItem(info.sourceSSRC, info.lastRR, info.delaySinceLastRR);
 
   xr.WithDlrr(&dlrr);
 
-  PacketBuiltCallback callback(ctx);
-  if (!callback.BuildPacket(xr))
-    return BuildResult::kTruncated;
-
-  return BuildResult::kSuccess;
+  return ctx->BuildPacket(xr);
 }
 
 // TODO(sprang): Add a unit test for this, or remove if the code isn't used.
-RTCPSender::BuildResult RTCPSender::BuildVoIPMetric(RtcpContext* ctx) {
+bool RTCPSender::BuildVoIPMetric(RtcpContext* ctx) {
   rtcp::Xr xr;
   xr.From(ssrc_);
 
   rtcp::VoipMetric voip;
   voip.To(remote_ssrc_);
   voip.LossRate(xr_voip_metric_.lossRate);
   voip.DiscardRate(xr_voip_metric_.discardRate);
   voip.BurstDensity(xr_voip_metric_.burstDensity);
   voip.GapDensity(xr_voip_metric_.gapDensity);
   voip.BurstDuration(xr_voip_metric_.burstDuration);
   voip.GapDuration(xr_voip_metric_.gapDuration);
   voip.RoundTripDelay(xr_voip_metric_.roundTripDelay);
   voip.EndSystemDelay(xr_voip_metric_.endSystemDelay);
   voip.SignalLevel(xr_voip_metric_.signalLevel);
   voip.NoiseLevel(xr_voip_metric_.noiseLevel);
   voip.Rerl(xr_voip_metric_.RERL);
   voip.Gmin(xr_voip_metric_.Gmin);
   voip.Rfactor(xr_voip_metric_.Rfactor);
   voip.ExtRfactor(xr_voip_metric_.extRfactor);
   voip.MosLq(xr_voip_metric_.MOSLQ);
   voip.MosCq(xr_voip_metric_.MOSCQ);
   voip.RxConfig(xr_voip_metric_.RXconfig);
   voip.JbNominal(xr_voip_metric_.JBnominal);
   voip.JbMax(xr_voip_metric_.JBmax);
   voip.JbAbsMax(xr_voip_metric_.JBabsMax);
 
   xr.WithVoipMetric(&voip);
 
-  PacketBuiltCallback callback(ctx);
-  if (!callback.BuildPacket(xr))
-    return BuildResult::kTruncated;
-
-  return BuildResult::kSuccess;
+  return ctx->BuildPacket(xr);
 }
 
 int32_t RTCPSender::SendRTCP(const FeedbackState& feedback_state,
                              RTCPPacketType packetType,
                              int32_t nack_size,
                              const uint16_t* nack_list,
                              bool repeat,
                              uint64_t pictureID) {
   return SendCompoundRTCP(
       feedback_state, std::set<RTCPPacketType>(&packetType, &packetType + 1),
       nack_size, nack_list, repeat, pictureID);
 }
 
 int32_t RTCPSender::SendCompoundRTCP(
     const FeedbackState& feedback_state,
-    const std::set<RTCPPacketType>& packetTypes,
+    const std::set<RTCPPacketType>& packet_types,
     int32_t nack_size,
     const uint16_t* nack_list,
     bool repeat,
     uint64_t pictureID) {
-  {
-    CriticalSectionScoped lock(critical_section_rtcp_sender_.get());
-    if (method_ == kRtcpOff) {
-      LOG(LS_WARNING) << "Can't send rtcp if it is disabled.";
-      return -1;
+  CriticalSectionScoped lock(critical_section_rtcp_sender_.get());
+
+  if (method_ == kRtcpOff) {
+    LOG(LS_WARNING) << "Can't send rtcp if it is disabled.";
+    return -1;
+  }
+
+  // We need to send our NTP even if we haven't received any reports.
+  uint32_t ntp_sec;
+  uint32_t ntp_frac;
+  clock_->CurrentNtp(ntp_sec, ntp_frac);
+  RtcpContext context(transport_, feedback_state, nack_size, nack_list, repeat,
+                      pictureID, ntp_sec, ntp_frac);
+
+  PrepareReport(packet_types, feedback_state);
+
+  auto it = report_flags_.begin();
+  while (it != report_flags_.end()) {
+    auto builder = builders_.find(it->type);
+    RTC_DCHECK(builder != builders_.end());
+    if (it->is_volatile) {
+      report_flags_.erase(it++);
+    } else {
+      ++it;
     }
+
+    if (!(this->*(builder->second))(&context))
+      return false;

[åsapersson, 2015/11/16 14:54:39]

return -1;

[åsapersson, 2015/12/01 13:49:20]

see comment

[sprang_webrtc, 2015/12/02 10:33:26]

Done.

   }
-  uint8_t rtcp_buffer[IP_PACKET_SIZE];
-  int rtcp_length =
-      PrepareRTCP(feedback_state, packetTypes, nack_size, nack_list, repeat,
-                  pictureID, rtcp_buffer, IP_PACKET_SIZE);
 

[åsapersson, 2015/11/16 14:54:39]

lock needed when sending?

[sprang_webrtc, 2015/12/01 10:23:06]

No, the lock was needed before to protect the pointer to the transport (which
could change a run-time). Since it's now set at construction we don't need the
lock.

[åsapersson, 2015/12/01 13:49:20]

Isn't the lock now held when sending (while it wasn't before)?

[sprang_webrtc, 2015/12/02 10:33:26]

Changed so we collect RtcpPacket instances first, as requested. Then release
lock and send.

-  // Sanity don't send empty packets.
-  if (rtcp_length <= 0)
-    return -1;
+  context.SendRemaining();
 
-  return SendToNetwork(rtcp_buffer, static_cast<size_t>(rtcp_length));
+  if (packet_type_counter_observer_ != nullptr) {
+    packet_type_counter_observer_->RtcpPacketTypesCounterUpdated(
+        remote_ssrc_, packet_type_counter_);
+  }
+
+  RTC_DCHECK(AllVolatileFlagsConsumed());
+
+  return context.bytes_sent_ > 0 ? 0 : -1;

[åsapersson, 2015/10/13 08:50:12]

And build the packet here?

something like 
context.BuildAndSentPacket();

BuildAndSentPacket() {
  uint8_t buffer[IP_PACKET_SIZE];
  return empty.BuildExternalBuffer(buffer, IP_PACKET_SIZE, &transport);
}

[sprang_webrtc, 2015/11/03 15:40:32]

That was my initial thought, but it has some drawbacks. Specifically, you would
need to heap allocate all the RtcpPacket subclasses used, and then keep them
both in the EmptyPacket and another structure, so that we can delete them after
serialization. 

By doing the serialization directly, we can stack-allocate the messages instead,
so they have a very clear scope, and should also have better performance.

WDYT?

 }
 
-int RTCPSender::PrepareRTCP(const FeedbackState& feedback_state,
-                            const std::set<RTCPPacketType>& packetTypes,
-                            int32_t nack_size,
-                            const uint16_t* nack_list,
-                            bool repeat,
-                            uint64_t pictureID,
-                            uint8_t* rtcp_buffer,
-                            int buffer_size) {
-  CriticalSectionScoped lock(critical_section_rtcp_sender_.get());
-
-  RtcpContext context(feedback_state, nack_size, nack_list, repeat, pictureID,
-                      rtcp_buffer, buffer_size);
-
+void RTCPSender::PrepareReport(const std::set<RTCPPacketType>& packetTypes,
+                               const FeedbackState& feedback_state) {
   // Add all flags as volatile. Non volatile entries will not be overwritten
   // and all new volatile flags added will be consumed by the end of this call.
   SetFlags(packetTypes, true);
 
   if (packet_type_counter_.first_packet_time_ms == -1)
     packet_type_counter_.first_packet_time_ms = clock_->TimeInMilliseconds();
 
   bool generate_report;
   if (IsFlagPresent(kRtcpSr) || IsFlagPresent(kRtcpRr)) {
     // Report type already explicitly set, don't automatically populate.
     generate_report = true;
     RTC_DCHECK(ConsumeFlag(kRtcpReport) == false);
   } else {
     generate_report =
         (ConsumeFlag(kRtcpReport) && method_ == kRtcpNonCompound) ||
         method_ == kRtcpCompound;
     if (generate_report)
       SetFlag(sending_ ? kRtcpSr : kRtcpRr, true);
   }
 
   if (IsFlagPresent(kRtcpSr) || (IsFlagPresent(kRtcpRr) && !cname_.empty()))
     SetFlag(kRtcpSdes, true);
 
-  // We need to send our NTP even if we haven't received any reports.
-  clock_->CurrentNtp(context.ntp_sec, context.ntp_frac);
-
   if (generate_report) {
     if (!sending_ && xr_send_receiver_reference_time_enabled_)
       SetFlag(kRtcpXrReceiverReferenceTime, true);
     if (feedback_state.has_last_xr_rr)
       SetFlag(kRtcpXrDlrrReportBlock, true);
 
     // generate next time to send an RTCP report
     // seeded from RTP constructor
     int32_t random = rand() % 1000;
     int32_t timeToNext = RTCP_INTERVAL_AUDIO_MS;
@@ skipping 13 matching lines @@
         minIntervalMs = RTCP_INTERVAL_VIDEO_MS;
       timeToNext = (minIntervalMs / 2) + (minIntervalMs * random / 1000);
     }
     next_time_to_send_rtcp_ = clock_->TimeInMilliseconds() + timeToNext;
 
     StatisticianMap statisticians =
         receive_statistics_->GetActiveStatisticians();
     if (!statisticians.empty()) {
       for (auto it = statisticians.begin(); it != statisticians.end(); ++it) {
         RTCPReportBlock report_block;
-        if (PrepareReport(feedback_state, it->first, it->second,
-                          &report_block)) {
+        if (PrepareReportBlock(feedback_state, it->first, it->second,
+                               &report_block)) {
           AddReportBlock(report_block);
         }
       }
     }
   }
-
-  auto it = report_flags_.begin();
-  while (it != report_flags_.end()) {
-    auto builder = builders_.find(it->type);
-    RTC_DCHECK(builder != builders_.end());
-    if (it->is_volatile) {
-      report_flags_.erase(it++);
-    } else {
-      ++it;
-    }
-
-    uint32_t start_position = context.position;
-    BuildResult result = (this->*(builder->second))(&context);
-    switch (result) {
-      case BuildResult::kError:
-        return -1;
-      case BuildResult::kTruncated:
-        return context.position;
-      case BuildResult::kAborted:
-        context.position = start_position;
-        FALLTHROUGH();
-      case BuildResult::kSuccess:
-        continue;
-      default:
-        abort();
-    }
-  }
-
-  if (packet_type_counter_observer_ != NULL) {
-    packet_type_counter_observer_->RtcpPacketTypesCounterUpdated(
-        remote_ssrc_, packet_type_counter_);
-  }
-
-  RTC_DCHECK(AllVolatileFlagsConsumed());
-
-  return context.position;
 }
 
-bool RTCPSender::PrepareReport(const FeedbackState& feedback_state,
-                               uint32_t ssrc,
-                               StreamStatistician* statistician,
-                               RTCPReportBlock* report_block) {
+bool RTCPSender::PrepareReportBlock(const FeedbackState& feedback_state,
+                                    uint32_t ssrc,
+                                    StreamStatistician* statistician,
+                                    RTCPReportBlock* report_block) {
   // Do we have receive statistics to send?
   RtcpStatistics stats;
   if (!statistician->GetStatistics(&stats, true))
     return false;
   report_block->fractionLost = stats.fraction_lost;
   report_block->cumulativeLost = stats.cumulative_lost;
   report_block->extendedHighSeqNum =
       stats.extended_max_sequence_number;
   report_block->jitter = stats.jitter;
   report_block->remoteSSRC = ssrc;
@@ skipping 17 matching lines @@
     receiveTime <<= 16;
     receiveTime += (feedback_state.last_rr_ntp_frac & 0xffff0000) >> 16;
 
     delaySinceLastReceivedSR = now-receiveTime;
   }
   report_block->delaySinceLastSR = delaySinceLastReceivedSR;
   report_block->lastSR = feedback_state.remote_sr;
   return true;
 }
 
-int32_t RTCPSender::SendToNetwork(const uint8_t* dataBuffer, size_t length) {
-  if (transport_->SendRtcp(dataBuffer, length))
-    return 0;
-  return -1;
-}
-
 void RTCPSender::SetCsrcs(const std::vector<uint32_t>& csrcs) {
   assert(csrcs.size() <= kRtpCsrcSize);
   CriticalSectionScoped lock(critical_section_rtcp_sender_.get());
   csrcs_ = csrcs;
 }
 
 int32_t RTCPSender::SetApplicationSpecificData(uint8_t subType,
                                                uint32_t name,
                                                const uint8_t* data,
                                                uint16_t length) {
@@ skipping 87 matching lines @@
     Transport* const transport_;
     bool send_failure_;
   } sender(transport_);
 
   uint8_t buffer[IP_PACKET_SIZE];
   return packet.BuildExternalBuffer(buffer, IP_PACKET_SIZE, &sender) &&
          !sender.send_failure_;
 }
 
 }  // namespace webrtc