Simpliy RtcpTransceiver::SendImmediateFeedback signature

and add implementation comment

Bug: webrtc:8239
Change-Id: Id24937018d386e386b8241aca8f5d686e7cc527a
Reviewed-on: https://webrtc-review.googlesource.com/26600
Reviewed-by: Niels Moller <nisse@webrtc.org>
Commit-Queue: Danil Chapovalov <danilchap@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#20925}

modules/rtp_rtcp/source/rtcp_transceiver.cc

@@ -106,18 +106,12 @@ void RtcpTransceiver::SendNack(uint32_t ssrc,
   task_queue_->PostTask(Closure{ptr_, ssrc, std::move(sequence_numbers)});
 }
 
-void RtcpTransceiver::SendPictureLossIndication(std::vector<uint32_t> ssrcs) {
+void RtcpTransceiver::SendPictureLossIndication(uint32_t ssrc) {
-  // TODO(danilchap): Replace with lambda with move capture when available.
+  rtc::WeakPtr<RtcpTransceiverImpl> ptr = ptr_;
-  struct Closure {
+  task_queue_->PostTask([ptr, ssrc] {
-    void operator()() {
+    if (ptr)
-      if (ptr)
+      ptr->SendPictureLossIndication(ssrc);
-        ptr->SendPictureLossIndication(ssrcs);
+  });
-    }
-
-    rtc::WeakPtr<RtcpTransceiverImpl> ptr;
-    std::vector<uint32_t> ssrcs;
-  };
-  task_queue_->PostTask(Closure{ptr_, std::move(ssrcs)});
 }
 
 void RtcpTransceiver::SendFullIntraRequest(std::vector<uint32_t> ssrcs) {

modules/rtp_rtcp/source/rtcp_transceiver.h

@@ -49,7 +49,7 @@ class RtcpTransceiver {
 
   // Requests new key frame.
   // using PLI, https://tools.ietf.org/html/rfc4585#section-6.3.1.1
-  void SendPictureLossIndication(std::vector<uint32_t> ssrcs);
+  void SendPictureLossIndication(uint32_t ssrc);
   // using FIR, https://tools.ietf.org/html/rfc5104#section-4.3.1.2
   void SendFullIntraRequest(std::vector<uint32_t> ssrcs);
 

modules/rtp_rtcp/source/rtcp_transceiver_impl.cc

@@ -134,39 +134,29 @@ void RtcpTransceiverImpl::UnsetRemb() {
 void RtcpTransceiverImpl::SendNack(uint32_t ssrc,
                                    std::vector<uint16_t> sequence_numbers) {
   RTC_DCHECK(!sequence_numbers.empty());
-  SendImmediateFeedback([&](PacketSender* sender) {
+  rtcp::Nack nack;
-    rtcp::Nack nack;
+  nack.SetSenderSsrc(config_.feedback_ssrc);
-    nack.SetSenderSsrc(config_.feedback_ssrc);
+  nack.SetMediaSsrc(ssrc);
-    nack.SetMediaSsrc(ssrc);
+  nack.SetPacketIds(std::move(sequence_numbers));
-    nack.SetPacketIds(std::move(sequence_numbers));
+  SendImmediateFeedback(nack);
-    sender->AppendPacket(nack);
-  });
 }
 
-void RtcpTransceiverImpl::SendPictureLossIndication(
+void RtcpTransceiverImpl::SendPictureLossIndication(uint32_t ssrc) {
-    rtc::ArrayView<const uint32_t> ssrcs) {
+  rtcp::Pli pli;
-  RTC_DCHECK(!ssrcs.empty());
+  pli.SetSenderSsrc(config_.feedback_ssrc);
-  SendImmediateFeedback([this, ssrcs](PacketSender* sender) {
+  pli.SetMediaSsrc(ssrc);
-    for (uint32_t media_ssrc : ssrcs) {
+  SendImmediateFeedback(pli);
-      rtcp::Pli pli;
-      pli.SetSenderSsrc(config_.feedback_ssrc);
-      pli.SetMediaSsrc(media_ssrc);
-      sender->AppendPacket(pli);
-    }
-  });
 }
 
 void RtcpTransceiverImpl::SendFullIntraRequest(
     rtc::ArrayView<const uint32_t> ssrcs) {
   RTC_DCHECK(!ssrcs.empty());
-  SendImmediateFeedback([this, ssrcs](PacketSender* sender) {
+  rtcp::Fir fir;
-    rtcp::Fir fir;
+  fir.SetSenderSsrc(config_.feedback_ssrc);
-    fir.SetSenderSsrc(config_.feedback_ssrc);
+  for (uint32_t media_ssrc : ssrcs)
-    for (uint32_t media_ssrc : ssrcs)
+    fir.AddRequestTo(media_ssrc,
-      fir.AddRequestTo(media_ssrc,
+                     remote_senders_[media_ssrc].fir_sequence_number++);
-                       remote_senders_[media_ssrc].fir_sequence_number++);
+  SendImmediateFeedback(fir);
-    sender->AppendPacket(fir);
-  });
 }
 
 void RtcpTransceiverImpl::HandleReceivedPacket(
@@ -255,15 +245,17 @@ void RtcpTransceiverImpl::SendPeriodicCompoundPacket() {
 }
 
 void RtcpTransceiverImpl::SendImmediateFeedback(
-    rtc::FunctionView<void(PacketSender*)> append_feedback) {
+    const rtcp::RtcpPacket& rtcp_packet) {
   PacketSender sender(config_.outgoing_transport, config_.max_packet_size);
+  // Compound mode requires every sent rtcp packet to be compound, i.e. start
+  // with a sender or receiver report.
   if (config_.rtcp_mode == RtcpMode::kCompound)
     CreateCompoundPacket(&sender);
 
-  append_feedback(&sender);
+  sender.AppendPacket(rtcp_packet);
-
   sender.Send();
 
+  // If compound packet was sent, delay (reschedule) the periodic one.
   if (config_.rtcp_mode == RtcpMode::kCompound)
     ReschedulePeriodicCompoundPackets();
 }

modules/rtp_rtcp/source/rtcp_transceiver_impl.h

@@ -23,7 +23,6 @@
 #include "modules/rtp_rtcp/source/rtcp_packet/report_block.h"
 #include "modules/rtp_rtcp/source/rtcp_transceiver_config.h"
 #include "rtc_base/constructormagic.h"
-#include "rtc_base/function_view.h"
 #include "rtc_base/weak_ptr.h"
 #include "system_wrappers/include/ntp_time.h"
 
@@ -46,7 +45,7 @@ class RtcpTransceiverImpl {
 
   void SendNack(uint32_t ssrc, std::vector<uint16_t> sequence_numbers);
 
-  void SendPictureLossIndication(rtc::ArrayView<const uint32_t> ssrcs);
+  void SendPictureLossIndication(uint32_t ssrc);
   void SendFullIntraRequest(rtc::ArrayView<const uint32_t> ssrcs);
 
  private:
@@ -63,8 +62,7 @@ class RtcpTransceiverImpl {
   void CreateCompoundPacket(PacketSender* sender);
   // Sends RTCP packets.
   void SendPeriodicCompoundPacket();
-  void SendImmediateFeedback(
+  void SendImmediateFeedback(const rtcp::RtcpPacket& rtcp_packet);
-      rtc::FunctionView<void(PacketSender*)> append_feedback);
   // Generate Report Blocks to be send in Sender or Receiver Report.
   std::vector<rtcp::ReportBlock> CreateReportBlocks();
 

modules/rtp_rtcp/source/rtcp_transceiver_impl_unittest.cc

@@ -476,7 +476,7 @@ TEST(RtcpTransceiverImplTest, SendsNack) {
 
 TEST(RtcpTransceiverImplTest, RequestKeyFrameWithPictureLossIndication) {
   const uint32_t kSenderSsrc = 1234;
-  const uint32_t kRemoteSsrcs[] = {4321, 5321};
+  const uint32_t kRemoteSsrc = 4321;
   RtcpTransceiverConfig config;
   config.feedback_ssrc = kSenderSsrc;
   config.schedule_periodic_compound_packets = false;
@@ -485,14 +485,12 @@ TEST(RtcpTransceiverImplTest, RequestKeyFrameWithPictureLossIndication) {
   config.outgoing_transport = &transport;
   RtcpTransceiverImpl rtcp_transceiver(config);
 
-  rtcp_transceiver.SendPictureLossIndication(kRemoteSsrcs);
+  rtcp_transceiver.SendPictureLossIndication(kRemoteSsrc);
 
-  // Expect a pli packet per ssrc in the sent single compound packet.
   EXPECT_EQ(transport.num_packets(), 1);
-  EXPECT_EQ(rtcp_parser.pli()->num_packets(), 2);
+  EXPECT_EQ(rtcp_parser.pli()->num_packets(), 1);
   EXPECT_EQ(rtcp_parser.pli()->sender_ssrc(), kSenderSsrc);
-  // test::RtcpPacketParser overwrites first pli packet with second one.
+  EXPECT_EQ(rtcp_parser.pli()->media_ssrc(), kRemoteSsrc);
-  EXPECT_EQ(rtcp_parser.pli()->media_ssrc(), kRemoteSsrcs[1]);
 }
 
 TEST(RtcpTransceiverImplTest, RequestKeyFrameWithFullIntraRequest) {