Send RTCP packets via RtcpPacket callback

webrtc/modules/rtp_rtcp/source/rtcp_sender.cc

Index: webrtc/modules/rtp_rtcp/source/rtcp_sender.cc
diff --git a/webrtc/modules/rtp_rtcp/source/rtcp_sender.cc b/webrtc/modules/rtp_rtcp/source/rtcp_sender.cc
index f250e29af7dec7ff29f6d8f1cf5b404cd1f1a2d6..974f867a1c60e924e140949e41e949843941378f 100644
--- a/webrtc/modules/rtp_rtcp/source/rtcp_sender.cc
+++ b/webrtc/modules/rtp_rtcp/source/rtcp_sender.cc
@@ -74,61 +74,59 @@ RTCPSender::FeedbackState::FeedbackState()
       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) {}
-
-  uint8_t* AllocateData(uint32_t bytes) {
-    RTC_DCHECK_LE(position + bytes, buffer_size);
-    uint8_t* ptr = &buffer[position];
-    position += bytes;
-    return ptr;
-  }
+              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) {}
+
+  virtual ~RtcpContext() {}
 
-  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;
+    if (transport_->SendRtcp(data, length))
+      bytes_sent_ += length;
+    position_ = 0;
   }
+
   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);
+  }
+
+  void SendRemaining() {
+    if (position_ > 0)
+      OnPacketReady(buffer_, position_);
   }
 
- private:
-  RtcpContext* const context_;
+  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(
@@ -468,15 +466,15 @@ int32_t RTCPSender::AddReportBlock(const RTCPReportBlock& report_block) {
   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
@@ -485,28 +483,27 @@ RTCPSender::BuildResult RTCPSender::BuildSR(RtcpContext* ctx) {
   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));
 
@@ -516,36 +513,29 @@ RTCPSender::BuildResult RTCPSender::BuildSDES(RtcpContext* ctx) {
   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");
@@ -553,11 +543,11 @@ RTCPSender::BuildResult RTCPSender::BuildPLI(RtcpContext* ctx) {
   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;
@@ -565,9 +555,8 @@ RTCPSender::BuildResult RTCPSender::BuildFIR(RtcpContext* ctx) {
   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");
@@ -575,7 +564,7 @@ RTCPSender::BuildResult RTCPSender::BuildFIR(RtcpContext* ctx) {
   TRACE_COUNTER_ID1(TRACE_DISABLED_BY_DEFAULT("webrtc_rtp"), "RTCP_FIRCount",
                     ssrc_, packet_type_counter_.fir_packets);
 
-  return BuildResult::kSuccess;
+  return true;
 }
 
 /*
@@ -585,20 +574,16 @@ RTCPSender::BuildResult RTCPSender::BuildFIR(RtcpContext* ctx) {
    |            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);
 }
 
 /*
@@ -613,38 +598,33 @@ RTCPSender::BuildResult RTCPSender::BuildSLI(RtcpContext* ctx) {
 /*
 *    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);
-
-  PacketBuiltCallback callback(ctx);
-  if (!callback.BuildPacket(rpsi))
-    return BuildResult::kTruncated;
+  rpsi.WithPayloadType(ctx->feedback_state_.send_payload_type);
+  rpsi.WithPictureId(ctx->picture_id_);
 
-  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) {
@@ -652,9 +632,9 @@ void RTCPSender::SetTargetBitrate(unsigned int target_bitrate) {
   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
@@ -669,14 +649,14 @@ RTCPSender::BuildResult RTCPSender::BuildTMMBR(RtcpContext* ctx) {
   // 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) {
@@ -687,13 +667,13 @@ RTCPSender::BuildResult RTCPSender::BuildTMMBR(RtcpContext* ctx) {
       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;
       }
     }
   }
@@ -705,17 +685,15 @@ RTCPSender::BuildResult RTCPSender::BuildTMMBR(RtcpContext* ctx) {
     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_);
@@ -726,85 +704,33 @@ RTCPSender::BuildResult RTCPSender::BuildTMMBN(RtcpContext* ctx) {
     }
   }
 
-  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;
-  }
-
-  // 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);
+bool RTCPSender::BuildNACK(RtcpContext* ctx) {
+  rtcp::Nack nack;
+  nack.From(ssrc_);
+  nack.To(remote_ssrc_);
+  nack.WithList(ctx->nack_list_, ctx->nack_size_);
 
-  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();
@@ -816,68 +742,54 @@ RTCPSender::BuildResult RTCPSender::BuildNACK(RtcpContext* ctx) {
   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_);
 
@@ -906,11 +818,7 @@ RTCPSender::BuildResult RTCPSender::BuildVoIPMetric(RtcpContext* ctx) {
 
   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,
@@ -926,43 +834,55 @@ int32_t RTCPSender::SendRTCP(const FeedbackState& feedback_state,
 
 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_);
+  }
 
-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());
+  RTC_DCHECK(AllVolatileFlagsConsumed());
 
-  RtcpContext context(feedback_state, nack_size, nack_list, repeat, pictureID,
-                      rtcp_buffer, buffer_size);
+  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?

+}
 
+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);
@@ -986,9 +906,6 @@ int RTCPSender::PrepareRTCP(const FeedbackState& feedback_state,
   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);
@@ -1022,55 +939,19 @@ int RTCPSender::PrepareRTCP(const FeedbackState& feedback_state,
     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))
@@ -1108,12 +989,6 @@ bool RTCPSender::PrepareReport(const FeedbackState& feedback_state,
   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());