Remove RTPPayloadStrategy and simplify RTPPayloadRegistry

webrtc/modules/rtp_rtcp/source/rtp_payload_registry.cc

 /*
  *  Copyright (c) 2013 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.
  */
 
 #include "webrtc/modules/rtp_rtcp/include/rtp_payload_registry.h"
 
 #include "webrtc/base/logging.h"
 #include "webrtc/common_types.h"
 #include "webrtc/modules/rtp_rtcp/source/byte_io.h"
 
 namespace webrtc {
 
-RTPPayloadRegistry::RTPPayloadRegistry(RTPPayloadStrategy* rtp_payload_strategy)
-    : rtp_payload_strategy_(rtp_payload_strategy),
-      red_payload_type_(-1),
-      ulpfec_payload_type_(-1),
-      incoming_payload_type_(-1),
+namespace {
+
+bool PayloadIsCompatible(const RtpUtility::Payload& payload,
+                         const CodecInst& audio_codec) {
+  if (!payload.audio)
+    return false;
+  if (!RtpUtility::StringCompare(payload.name, audio_codec.plname))
+    return false;
+  const AudioPayload& audio_payload = payload.typeSpecific.Audio;
+  const uint32_t rate = std::max(0, audio_codec.rate);
+  return audio_payload.frequency == static_cast<uint32_t>(audio_codec.plfreq) &&
+         audio_payload.channels == audio_codec.channels &&
+         (audio_payload.rate == rate || audio_payload.rate == 0 || rate == 0);
+}
+
+bool PayloadIsCompatible(const RtpUtility::Payload& payload,
+                         const VideoCodec& video_codec) {
+  return !payload.audio &&
+         RtpUtility::StringCompare(payload.name, video_codec.plName);
+}
+
+RtpUtility::Payload CreatePayloadType(const CodecInst& audio_codec) {
+  RtpUtility::Payload payload;
+  payload.name[RTP_PAYLOAD_NAME_SIZE - 1] = 0;
+  strncpy(payload.name, audio_codec.plname, RTP_PAYLOAD_NAME_SIZE - 1);
+  assert(audio_codec.plfreq >= 1000);

[danilchap, 2016/11/23 19:06:18]

RTC_DCHECK_GE(audio_codec.plfreq, 1000);
(and #include "webrtc/base/checks.h" for that)

[magjed_webrtc, 2016/11/24 12:20:39]

Done. This logic is copy-pasted from line 427 if you wonder where it comes from.

+  payload.typeSpecific.Audio.frequency = audio_codec.plfreq;
+  payload.typeSpecific.Audio.channels = audio_codec.channels;
+  payload.typeSpecific.Audio.rate = std::max(0, audio_codec.rate);
+  payload.audio = true;
+  return payload;
+}
+
+RtpVideoCodecTypes ConvertToRtpVideoCodecType(VideoCodecType type) {
+  switch (type) {
+    case kVideoCodecVP8:
+      return kRtpVideoVp8;
+    case kVideoCodecVP9:
+      return kRtpVideoVp9;
+    case kVideoCodecH264:
+      return kRtpVideoH264;
+    case kVideoCodecRED:
+    case kVideoCodecULPFEC:
+      return kRtpVideoNone;
+    default:
+      return kRtpVideoGeneric;
+  }
+}
+
+RtpUtility::Payload CreatePayloadType(const VideoCodec& video_codec) {
+  RtpUtility::Payload payload;
+  payload.name[RTP_PAYLOAD_NAME_SIZE - 1] = 0;
+  strncpy(payload.name, video_codec.plName, RTP_PAYLOAD_NAME_SIZE - 1);
+  payload.typeSpecific.Video.videoCodecType =
+      ConvertToRtpVideoCodecType(video_codec.codecType);
+  payload.audio = false;
+  return payload;
+}
+
+}  // anonymous namespace
+
+RTPPayloadRegistry::RTPPayloadRegistry()
+    : incoming_payload_type_(-1),
       last_received_payload_type_(-1),
       last_received_media_payload_type_(-1),
       rtx_(false),
       ssrc_rtx_(0) {}
 
-RTPPayloadRegistry::~RTPPayloadRegistry() {
-  while (!payload_type_map_.empty()) {
-    RtpUtility::PayloadTypeMap::iterator it = payload_type_map_.begin();
-    delete it->second;
-    payload_type_map_.erase(it);
-  }
-}
-
-int32_t RTPPayloadRegistry::RegisterReceivePayload(const CodecInst& audio_codec,
-                                                   bool* created_new_payload) {
-  return RegisterReceivePayload(
-      audio_codec.plname, audio_codec.pltype, audio_codec.plfreq,
-      audio_codec.channels, std::max(0, audio_codec.rate), created_new_payload);
-}
-
-int32_t RTPPayloadRegistry::RegisterReceivePayload(
-    const VideoCodec& video_codec) {
-  bool dummy_created_new_payload;
-  return RegisterReceivePayload(video_codec.plName, video_codec.plType,
-                                kVideoPayloadTypeFrequency, 0 /* channels */,
-                                0 /* rate */, &dummy_created_new_payload);
-}
-
-int32_t RTPPayloadRegistry::RegisterReceivePayload(
-    const char* const payload_name,
-    const int8_t payload_type,
-    const uint32_t frequency,
-    const size_t channels,
-    const uint32_t rate,
-    bool* created_new_payload) {
+bool IsPayloadTypeValid(int8_t payload_type) {
   assert(payload_type >= 0);
-  assert(payload_name);
-  *created_new_payload = false;
 
   // Sanity check.
   switch (payload_type) {
     // Reserved payload types to avoid RTCP conflicts when marker bit is set.
     case 64:        //  192 Full INTRA-frame request.
     case 72:        //  200 Sender report.
     case 73:        //  201 Receiver report.
     case 74:        //  202 Source description.
     case 75:        //  203 Goodbye.
     case 76:        //  204 Application-defined.
     case 77:        //  205 Transport layer FB message.
     case 78:        //  206 Payload-specific FB message.
     case 79:        //  207 Extended report.
       LOG(LS_ERROR) << "Can't register invalid receiver payload type: "
                     << payload_type;
-      return -1;
+      return false;
     default:
-      break;
+      return true;
   }
+}
 
-  size_t payload_name_length = strlen(payload_name);
+int32_t RTPPayloadRegistry::RegisterReceivePayload(const CodecInst& audio_codec,
+                                                   bool* created_new_payload) {
+  *created_new_payload = false;
+  if (!IsPayloadTypeValid(audio_codec.pltype))
+    return -1;
+
+  const uint32_t rate = std::max(0, audio_codec.rate);

[danilchap, 2016/11/23 19:06:18]

while you recalculate rate, you do not set it back to audio_codec,
as a result, when calling PayloadIsCompatible, audio_codec.rate is used even if
it is negative.

[magjed_webrtc, 2016/11/24 12:20:39]

Yes, that's why I recalculate the rate from audio_codec.rate in
PayloadIsCompatible as well.

[danilchap, 2016/11/24 13:24:36]

oops, missed that, not a problem then.
may be then clean rate only where it used since it is used once in this
function.

[magjed_webrtc, 2016/11/24 14:29:29]

Done.

 
   rtc::CritScope cs(&crit_sect_);
 
-  RtpUtility::PayloadTypeMap::iterator it =
-      payload_type_map_.find(payload_type);
-
+  auto it = payload_type_map_.find(audio_codec.pltype);
   if (it != payload_type_map_.end()) {
-    // We already use this payload type.
-    RtpUtility::Payload* payload = it->second;
-
-    assert(payload);
-
-    size_t name_length = strlen(payload->name);
-
-    // Check if it's the same as we already have.
-    // If same, ignore sending an error.
-    if (payload_name_length == name_length &&
-        RtpUtility::StringCompare(
-            payload->name, payload_name, payload_name_length)) {
-      if (rtp_payload_strategy_->PayloadIsCompatible(*payload, frequency,
-                                                     channels, rate)) {
-        rtp_payload_strategy_->UpdatePayloadRate(payload, rate);
-        return 0;
-      }
+    // We already use this payload type. Check if it's the same as we already
+    // have. If same, ignore sending an error.
+    if (PayloadIsCompatible(it->second, audio_codec)) {
+      it->second.typeSpecific.Audio.rate = rate;
+      return 0;
     }
-    LOG(LS_ERROR) << "Payload type already registered: "
-                  << static_cast<int>(payload_type);
+    LOG(LS_ERROR) << "Payload type already registered: " << audio_codec.pltype;
     return -1;
   }
 
-  if (rtp_payload_strategy_->CodecsMustBeUnique()) {
-    DeregisterAudioCodecOrRedTypeRegardlessOfPayloadType(
-        payload_name, payload_name_length, frequency, channels, rate);
-  }
+  // Audio codecs must be unique.
+  DeregisterAudioCodecOrRedTypeRegardlessOfPayloadType(audio_codec);
 
-  RtpUtility::Payload* payload = rtp_payload_strategy_->CreatePayloadType(
-      payload_name, payload_type, frequency, channels, rate);
-
-  payload_type_map_[payload_type] = payload;
+  payload_type_map_[audio_codec.pltype] = CreatePayloadType(audio_codec);
   *created_new_payload = true;
 
-  if (RtpUtility::StringCompare(payload_name, "red", 3)) {
-    red_payload_type_ = payload_type;
-  } else if (RtpUtility::StringCompare(payload_name, "ulpfec", 6)) {
-    ulpfec_payload_type_ = payload_type;
-  }
-
   // Successful set of payload type, clear the value of last received payload
   // type since it might mean something else.
   last_received_payload_type_ = -1;
+  last_received_media_payload_type_ = -1;
+  return 0;
+}
+
+int32_t RTPPayloadRegistry::RegisterReceivePayload(
+    const VideoCodec& video_codec) {
+  if (!IsPayloadTypeValid(video_codec.plType))
+    return -1;
+
+  rtc::CritScope cs(&crit_sect_);
+
+  auto it = payload_type_map_.find(video_codec.plType);
+  if (it != payload_type_map_.end()) {
+    // We already use this payload type. Check if it's the same as we already
+    // have. If same, ignore sending an error.
+    if (PayloadIsCompatible(it->second, video_codec))
+      return 0;
+    LOG(LS_ERROR) << "Payload type already registered: "
+                  << static_cast<int>(video_codec.plType);
+    return -1;
+  }
+
+  payload_type_map_[video_codec.plType] = CreatePayloadType(video_codec);
+
+  // Successful set of payload type, clear the value of last received payload
+  // type since it might mean something else.
+  last_received_payload_type_ = -1;
   last_received_media_payload_type_ = -1;
   return 0;
 }
 
 int32_t RTPPayloadRegistry::DeRegisterReceivePayload(
     const int8_t payload_type) {
   rtc::CritScope cs(&crit_sect_);
-  RtpUtility::PayloadTypeMap::iterator it =
-      payload_type_map_.find(payload_type);
-  assert(it != payload_type_map_.end());
-  delete it->second;
-  payload_type_map_.erase(it);
+  payload_type_map_.erase(payload_type);
   return 0;
 }
 
 // There can't be several codecs with the same rate, frequency and channels
 // for audio codecs, but there can for video.
 // Always called from within a critical section.
 void RTPPayloadRegistry::DeregisterAudioCodecOrRedTypeRegardlessOfPayloadType(
-    const char* const payload_name,
-    const size_t payload_name_length,
-    const uint32_t frequency,
-    const size_t channels,
-    const uint32_t rate) {
-  RtpUtility::PayloadTypeMap::iterator iterator = payload_type_map_.begin();
-  for (; iterator != payload_type_map_.end(); ++iterator) {
-    RtpUtility::Payload* payload = iterator->second;
-    size_t name_length = strlen(payload->name);
-
-    if (payload_name_length == name_length &&
-        RtpUtility::StringCompare(
-            payload->name, payload_name, payload_name_length)) {
-      // We found the payload name in the list.
-      // If audio, check frequency and rate.
-      if (payload->audio) {
-        if (rtp_payload_strategy_->PayloadIsCompatible(*payload, frequency,
-                                                       channels, rate)) {
-          // Remove old setting.
-          delete payload;
-          payload_type_map_.erase(iterator);
-          break;
-        }
-      } else if (RtpUtility::StringCompare(payload_name, "red", 3)) {
-        delete payload;
-        payload_type_map_.erase(iterator);
-        break;
-      }
+    const CodecInst& audio_codec) {
+  for (auto iterator = payload_type_map_.begin();
+       iterator != payload_type_map_.end(); ++iterator) {
+    if (PayloadIsCompatible(iterator->second, audio_codec)) {
+      // Remove old setting.
+      payload_type_map_.erase(iterator);
+      break;
     }
   }
 }
 
 int32_t RTPPayloadRegistry::ReceivePayloadType(const CodecInst& audio_codec,
                                                int8_t* payload_type) const {
-  return ReceivePayloadType(audio_codec.plname, audio_codec.plfreq,
-                            audio_codec.channels, std::max(0, audio_codec.rate),
-                            payload_type);
+  assert(payload_type);
+  rtc::CritScope cs(&crit_sect_);
+
+  for (const auto& it : payload_type_map_) {
+    if (PayloadIsCompatible(it.second, audio_codec)) {
+      *payload_type = it.first;
+      return 0;
+    }
+  }
+  return -1;
 }
 
 int32_t RTPPayloadRegistry::ReceivePayloadType(const VideoCodec& video_codec,
                                                int8_t* payload_type) const {
-  return ReceivePayloadType(video_codec.plName, kVideoPayloadTypeFrequency,
-                            0 /* channels */, 0 /* rate */, payload_type);
-}
-
-int32_t RTPPayloadRegistry::ReceivePayloadType(const char* const payload_name,
-                                               const uint32_t frequency,
-                                               const size_t channels,
-                                               const uint32_t rate,
-                                               int8_t* payload_type) const {
   assert(payload_type);
-  size_t payload_name_length = strlen(payload_name);
-
   rtc::CritScope cs(&crit_sect_);
 
-  RtpUtility::PayloadTypeMap::const_iterator it = payload_type_map_.begin();
-
-  for (; it != payload_type_map_.end(); ++it) {
-    RtpUtility::Payload* payload = it->second;
-    assert(payload);
-
-    size_t name_length = strlen(payload->name);
-    if (payload_name_length == name_length &&
-        RtpUtility::StringCompare(
-            payload->name, payload_name, payload_name_length)) {
-      // Name matches.
-      if (payload->audio) {
-        if (rate == 0) {

[danilchap, 2016/11/23 19:06:18]

if (stored->Audio.rate == 0 && received->audio_codec.rate != 0)
this expresion would be false but IsCompatible() will be true.
Better check with someone familiar with audio if current change fix or introduce
a bug.

[magjed_webrtc, 2016/11/24 12:20:39]

Yes, this is true, thanks for reviewing so carefully. I also noticed this
discrepancy, but I assumed this was just old code, and that the real intent is
to use the same IsCompatible() logic that is used in the other functions
RegisterReceivePayload and DeregisterAudioCodecOrRedTypeRegardlessOfPayloadType.
I added solenberg as a reviewer and maybe he knows.

-          // [default] audio, check freq and channels.
-          if (payload->typeSpecific.Audio.frequency == frequency &&
-              payload->typeSpecific.Audio.channels == channels) {
-            *payload_type = it->first;
-            return 0;
-          }
-        } else {
-          // Non-default audio, check freq, channels and rate.
-          if (payload->typeSpecific.Audio.frequency == frequency &&
-              payload->typeSpecific.Audio.channels == channels &&
-              payload->typeSpecific.Audio.rate == rate) {
-            // extra rate condition added
-            *payload_type = it->first;
-            return 0;
-          }
-        }
-      } else {
-        // Video.
-        *payload_type = it->first;
-        return 0;
-      }
+  for (const auto& it : payload_type_map_) {
+    if (PayloadIsCompatible(it.second, video_codec)) {
+      *payload_type = it.first;
+      return 0;
     }
   }
   return -1;
 }
 
 bool RTPPayloadRegistry::RtxEnabled() const {
   rtc::CritScope cs(&crit_sect_);
   return rtx_;
 }
 
@@ skipping 75 matching lines @@
     LOG(LS_ERROR) << "Invalid RTX payload type: " << payload_type;
     return;
   }
 
   rtx_payload_type_map_[payload_type] = associated_payload_type;
   rtx_ = true;
 }
 
 bool RTPPayloadRegistry::IsRed(const RTPHeader& header) const {
   rtc::CritScope cs(&crit_sect_);
-  return red_payload_type_ == header.payloadType;
+  auto it = payload_type_map_.find(header.payloadType);
+  return it != payload_type_map_.end() &&
+         RtpUtility::StringCompare(it->second.name, "red");
 }
 
 bool RTPPayloadRegistry::IsEncapsulated(const RTPHeader& header) const {
   return IsRed(header) || IsRtx(header);
 }
 
 bool RTPPayloadRegistry::GetPayloadSpecifics(uint8_t payload_type,
                                              PayloadUnion* payload) const {
   rtc::CritScope cs(&crit_sect_);
-  RtpUtility::PayloadTypeMap::const_iterator it =
-      payload_type_map_.find(payload_type);
+  auto it = payload_type_map_.find(payload_type);
 
   // Check that this is a registered payload type.
   if (it == payload_type_map_.end()) {
     return false;
   }
-  *payload = it->second->typeSpecific;
+  *payload = it->second.typeSpecific;
   return true;
 }
 
 int RTPPayloadRegistry::GetPayloadTypeFrequency(
     uint8_t payload_type) const {
   const RtpUtility::Payload* payload = PayloadTypeToPayload(payload_type);
   if (!payload) {
     return -1;
   }
   rtc::CritScope cs(&crit_sect_);
-  return rtp_payload_strategy_->GetPayloadTypeFrequency(*payload);
+  return payload->audio ? payload->typeSpecific.Audio.frequency
+                        : kVideoPayloadTypeFrequency;
 }
 
 const RtpUtility::Payload* RTPPayloadRegistry::PayloadTypeToPayload(
     uint8_t payload_type) const {
   rtc::CritScope cs(&crit_sect_);
 
-  RtpUtility::PayloadTypeMap::const_iterator it =
-      payload_type_map_.find(payload_type);
+  auto it = payload_type_map_.find(payload_type);
 
   // Check that this is a registered payload type.
   if (it == payload_type_map_.end()) {
     return nullptr;
   }
 
-  return it->second;
+  return &it->second;
 }
 
 void RTPPayloadRegistry::SetIncomingPayloadType(const RTPHeader& header) {
   rtc::CritScope cs(&crit_sect_);
   if (!IsRtxInternal(header))
     incoming_payload_type_ = header.payloadType;
 }
 
 bool RTPPayloadRegistry::ReportMediaPayloadType(uint8_t media_payload_type) {
   rtc::CritScope cs(&crit_sect_);
   if (last_received_media_payload_type_ == media_payload_type) {
     // Media type unchanged.
     return true;
   }
   last_received_media_payload_type_ = media_payload_type;
   return false;
 }
 
-class RTPPayloadAudioStrategy : public RTPPayloadStrategy {
- public:
-  bool CodecsMustBeUnique() const override { return true; }
-
-  bool PayloadIsCompatible(const RtpUtility::Payload& payload,
-                           const uint32_t frequency,
-                           const size_t channels,
-                           const uint32_t rate) const override {
-    return
-        payload.audio &&
-        payload.typeSpecific.Audio.frequency == frequency &&
-        payload.typeSpecific.Audio.channels == channels &&
-        (payload.typeSpecific.Audio.rate == rate ||
-            payload.typeSpecific.Audio.rate == 0 || rate == 0);
+// Returns -1 if a payload with name |payload_name| is not registered.
+int8_t RTPPayloadRegistry::GetPayloadTypeWithName(
+    const char* payload_name) const {
+  rtc::CritScope cs(&crit_sect_);
+  for (const auto& it : payload_type_map_) {
+    if (RtpUtility::StringCompare(it.second.name, payload_name))
+      return it.first;
   }
-
-  void UpdatePayloadRate(RtpUtility::Payload* payload,
-                         const uint32_t rate) const override {
-    payload->typeSpecific.Audio.rate = rate;
-  }
-
-  RtpUtility::Payload* CreatePayloadType(const char* const payloadName,
-                                         const int8_t payloadType,
-                                         const uint32_t frequency,
-                                         const size_t channels,
-                                         const uint32_t rate) const override {
-    RtpUtility::Payload* payload = new RtpUtility::Payload;
-    payload->name[RTP_PAYLOAD_NAME_SIZE - 1] = 0;
-    strncpy(payload->name, payloadName, RTP_PAYLOAD_NAME_SIZE - 1);
-    assert(frequency >= 1000);
-    payload->typeSpecific.Audio.frequency = frequency;
-    payload->typeSpecific.Audio.channels = channels;
-    payload->typeSpecific.Audio.rate = rate;
-    payload->audio = true;
-    return payload;
-  }
-
-  int GetPayloadTypeFrequency(
-      const RtpUtility::Payload& payload) const override {
-    return payload.typeSpecific.Audio.frequency;
-  }
-};
-
-class RTPPayloadVideoStrategy : public RTPPayloadStrategy {
- public:
-  bool CodecsMustBeUnique() const override { return false; }
-
-  bool PayloadIsCompatible(const RtpUtility::Payload& payload,
-                           const uint32_t frequency,
-                           const size_t channels,
-                           const uint32_t rate) const override {
-    return !payload.audio;
-  }
-
-  void UpdatePayloadRate(RtpUtility::Payload* payload,
-                         const uint32_t rate) const override {}
-
-  RtpUtility::Payload* CreatePayloadType(const char* const payloadName,
-                                         const int8_t payloadType,
-                                         const uint32_t frequency,
-                                         const size_t channels,
-                                         const uint32_t rate) const override {
-    RtpVideoCodecTypes videoType = kRtpVideoGeneric;
-
-    if (RtpUtility::StringCompare(payloadName, "VP8", 3)) {
-      videoType = kRtpVideoVp8;
-    } else if (RtpUtility::StringCompare(payloadName, "VP9", 3)) {
-      videoType = kRtpVideoVp9;
-    } else if (RtpUtility::StringCompare(payloadName, "H264", 4)) {
-      videoType = kRtpVideoH264;
-    } else if (RtpUtility::StringCompare(payloadName, "I420", 4)) {
-      videoType = kRtpVideoGeneric;
-    } else if (RtpUtility::StringCompare(payloadName, "ULPFEC", 6) ||
-        RtpUtility::StringCompare(payloadName, "RED", 3)) {
-      videoType = kRtpVideoNone;
-    } else {
-      videoType = kRtpVideoGeneric;
-    }
-    RtpUtility::Payload* payload = new RtpUtility::Payload;
-
-    payload->name[RTP_PAYLOAD_NAME_SIZE - 1] = 0;
-    strncpy(payload->name, payloadName, RTP_PAYLOAD_NAME_SIZE - 1);
-    payload->typeSpecific.Video.videoCodecType = videoType;
-    payload->audio = false;
-    return payload;
-  }
-
-  int GetPayloadTypeFrequency(
-      const RtpUtility::Payload& payload) const override {
-    return kVideoPayloadTypeFrequency;
-  }
-};
-
-RTPPayloadStrategy* RTPPayloadStrategy::CreateStrategy(
-    const bool handling_audio) {
-  if (handling_audio) {
-    return new RTPPayloadAudioStrategy();
-  } else {
-    return new RTPPayloadVideoStrategy();
-  }
+  return -1;
 }
 
 }  // namespace webrtc