Simplify {,Set}UlpfecConfig interface.

webrtc/modules/rtp_rtcp/source/rtp_sender_video.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 112 matching lines @@
     if (protect) {
       producer_fec_.AddRtpPacketAndGenerateFec(media_packet->data(),
                                                media_packet->payload_size(),
                                                media_packet->headers_size());
     }
     uint16_t num_fec_packets = producer_fec_.NumAvailableFecPackets();
     if (num_fec_packets > 0) {
       uint16_t first_fec_sequence_number =
           rtp_sender_->AllocateSequenceNumber(num_fec_packets);
       fec_packets = producer_fec_.GetUlpfecPacketsAsRed(
-          red_payload_type_, fec_payload_type_, first_fec_sequence_number,
+          red_payload_type_, ulpfec_payload_type_, first_fec_sequence_number,
           media_packet->headers_size());
       RTC_DCHECK_EQ(num_fec_packets, fec_packets.size());
       if (retransmission_settings_ & kRetransmitFECPackets)
         fec_storage = kAllowRetransmission;
     }
   }
   // Send |red_packet| instead of |packet| for allocated sequence number.
   size_t red_packet_size = red_packet->size();
   if (rtp_sender_->SendToNetwork(std::move(red_packet), media_packet_storage,
                                  RtpPacketSender::kLowPriority)) {
@@ skipping 19 matching lines @@
       fec_bitrate_.Update(fec_packet->length(), clock_->TimeInMilliseconds());
       TRACE_EVENT_INSTANT2(TRACE_DISABLED_BY_DEFAULT("webrtc_rtp"),
                            "Video::PacketFec", "timestamp", rtp_timestamp,
                            "seqnum", fec_sequence_number);
     } else {
       LOG(LS_WARNING) << "Failed to send FEC packet " << fec_sequence_number;
     }
   }
 }
 
-void RTPSenderVideo::SetUlpfecConfig(bool enabled,
-                                     int red_payload_type,
+bool RTPSenderVideo::RedEnabled() const {
+  rtc::CritScope cs(&crit_);

[danilchap, 2016/10/28 09:29:43]

do not (re)take lock, instead annotate these functions with
EXCLUSIVE_LOCKS_REQUIRED(crit_)

[brandtr, 2016/10/28 11:05:06]

Got it! Done. Also changed name to cheap_accessor_style().

+  return red_payload_type_ >= 0;
+}
+
+bool RTPSenderVideo::UlpfecEnabled() const {
+  rtc::CritScope cs(&crit_);
+  return ulpfec_payload_type_ >= 0;
+}
+
+void RTPSenderVideo::SetUlpfecConfig(int red_payload_type,
                                      int ulpfec_payload_type) {
-  RTC_DCHECK(!enabled || red_payload_type > 0);
+  // Sanity check. Per the definition of UlpfecConfig (see config.h),
+  // a payload type of -1 means that the corresponding feature is
+  // turned off.
+  RTC_DCHECK_GE(red_payload_type, -1);
   RTC_DCHECK_LE(red_payload_type, 127);
+  RTC_DCHECK_GE(ulpfec_payload_type, -1);
   RTC_DCHECK_LE(ulpfec_payload_type, 127);
 
   rtc::CritScope cs(&crit_);
-  fec_enabled_ = enabled;
   red_payload_type_ = red_payload_type;
-  fec_payload_type_ = ulpfec_payload_type;
+  ulpfec_payload_type_ = ulpfec_payload_type;
+
+  // Must not enable ULPFEC without RED.
+  // TODO(brandtr): We currently support enabling RED without ULPFEC. Change
+  // this when we have removed the RED/RTX send-side workaround, so that we
+  // ensure that RED and ULPFEC are only enabled together.
+  RTC_DCHECK(RedEnabled() || !UlpfecEnabled());
 
   // Reset FEC rates.
   delta_fec_params_ = FecProtectionParams{0, 1, kFecMaskRandom};
   key_fec_params_ = FecProtectionParams{0, 1, kFecMaskRandom};
 }
 
-void RTPSenderVideo::UlpfecConfig(bool* enabled,
-                                  int* red_payload_type,
+void RTPSenderVideo::UlpfecConfig(int* red_payload_type,
                                   int* ulpfec_payload_type) const {
   rtc::CritScope cs(&crit_);
-  *enabled = fec_enabled_;
   *red_payload_type = red_payload_type_;
-  *ulpfec_payload_type = fec_payload_type_;
+  *ulpfec_payload_type = ulpfec_payload_type_;
 }
 
 size_t RTPSenderVideo::FecPacketOverhead() const {
   rtc::CritScope cs(&crit_);
   size_t overhead = 0;
-  if (red_payload_type_ != -1) {
+  if (RedEnabled()) {
     // Overhead is FEC headers plus RED for FEC header plus anything in RTP
     // header beyond the 12 bytes base header (CSRC list, extensions...)
     // This reason for the header extensions to be included here is that
     // from an FEC viewpoint, they are part of the payload to be protected.
     // (The base RTP header is already protected by the FEC header.)
     return producer_fec_.MaxPacketOverhead() + kRedForFecHeaderLength +
            (rtp_sender_->RtpHeaderLength() - kRtpHeaderSize);
   }
-  if (fec_enabled_)
+  if (UlpfecEnabled())
     overhead += producer_fec_.MaxPacketOverhead();
   return overhead;
 }
 
 void RTPSenderVideo::SetFecParameters(const FecProtectionParams* delta_params,
                                       const FecProtectionParams* key_params) {
   rtc::CritScope cs(&crit_);
   RTC_DCHECK(delta_params);
   RTC_DCHECK(key_params);
-  if (fec_enabled_) {
+  if (UlpfecEnabled()) {
     delta_fec_params_ = *delta_params;
     key_fec_params_ = *key_params;
   }
 }
 
 bool RTPSenderVideo::SendVideo(RtpVideoCodecTypes video_type,
                                FrameType frame_type,
                                int8_t payload_type,
                                uint32_t rtp_timestamp,
                                int64_t capture_time_ms,
@@ skipping 33 matching lines @@
                            (rtp_sender_->RtxStatus() ? kRtxHeaderSize : 0);
   RTC_DCHECK_LE(packet_capacity, rtp_header->capacity());
   RTC_DCHECK_GT(packet_capacity, rtp_header->headers_size());
   size_t max_data_payload_length = packet_capacity - rtp_header->headers_size();
 
   std::unique_ptr<RtpPacketizer> packetizer(RtpPacketizer::Create(
       video_type, max_data_payload_length,
       video_header ? &(video_header->codecHeader) : nullptr, frame_type));
 
   StorageType storage;
-  int red_payload_type;
+  bool red_enabled;
   bool first_frame = first_frame_sent_();
   {
     rtc::CritScope cs(&crit_);
     FecProtectionParams* fec_params =
         frame_type == kVideoFrameKey ? &key_fec_params_ : &delta_fec_params_;
     producer_fec_.SetFecParameters(fec_params);
     storage = packetizer->GetStorageType(retransmission_settings_);
-    red_payload_type = red_payload_type_;
+    red_enabled = RedEnabled();
   }
 
   // TODO(changbin): we currently don't support to configure the codec to
   // output multiple partitions for VP8. Should remove below check after the
   // issue is fixed.
   const RTPFragmentationHeader* frag =
       (video_type == kRtpVideoVp8) ? NULL : fragmentation;
 
   packetizer->SetPayloadData(payload_data, payload_size, frag);
 
   bool first = true;
   bool last = false;
   while (!last) {
     std::unique_ptr<RtpPacketToSend> packet(new RtpPacketToSend(*rtp_header));
     uint8_t* payload = packet->AllocatePayload(max_data_payload_length);
     RTC_DCHECK(payload);
 
     size_t payload_bytes_in_packet = 0;
     if (!packetizer->NextPacket(payload, &payload_bytes_in_packet, &last))
       return false;
 
     packet->SetPayloadSize(payload_bytes_in_packet);
     packet->SetMarker(last);
     if (!rtp_sender_->AssignSequenceNumber(packet.get()))
       return false;
 
-    if (red_payload_type != -1) {
+    if (red_enabled) {
       SendVideoPacketAsRed(std::move(packet), storage,
                            packetizer->GetProtectionType() == kProtectedPacket);
     } else {
       SendVideoPacket(std::move(packet), storage);
     }
 
     if (first_frame) {
       if (first) {
         LOG(LS_INFO)
             << "Sent first RTP packet of the first video frame (pre-pacer)";
@@ skipping 25 matching lines @@
   rtc::CritScope cs(&crit_);
   return retransmission_settings_;
 }
 
 void RTPSenderVideo::SetSelectiveRetransmissions(uint8_t settings) {
   rtc::CritScope cs(&crit_);
   retransmission_settings_ = settings;
 }
 
 }  // namespace webrtc