Stop using hardcoded payload types for video codecs

webrtc/media/engine/webrtcvideoengine2.cc

Index: webrtc/media/engine/webrtcvideoengine2.cc
diff --git a/webrtc/media/engine/webrtcvideoengine2.cc b/webrtc/media/engine/webrtcvideoengine2.cc
index 73894d3ba8ec4a215c687c90e0c227bf93650798..ef071f16332e03dbb38186b37c8f5819a817a68a 100644
--- a/webrtc/media/engine/webrtcvideoengine2.cc
+++ b/webrtc/media/engine/webrtcvideoengine2.cc
@@ -23,14 +23,13 @@
 #include "webrtc/base/trace_event.h"
 #include "webrtc/call.h"
 #include "webrtc/media/engine/constants.h"
+#include "webrtc/media/engine/internalencoderfactory.h"
 #include "webrtc/media/engine/simulcast.h"
 #include "webrtc/media/engine/videoencodersoftwarefallbackwrapper.h"
 #include "webrtc/media/engine/webrtcmediaengine.h"
 #include "webrtc/media/engine/webrtcvideoencoderfactory.h"
 #include "webrtc/media/engine/webrtcvoiceengine.h"
-#include "webrtc/modules/video_coding/codecs/h264/include/h264.h"
 #include "webrtc/modules/video_coding/codecs/vp8/simulcast_encoder_adapter.h"
-#include "webrtc/modules/video_coding/codecs/vp9/include/vp9.h"
 #include "webrtc/system_wrappers/include/field_trial.h"
 #include "webrtc/video_decoder.h"
 #include "webrtc/video_encoder.h"
@@ -169,13 +168,6 @@ void AddDefaultFeedbackParams(VideoCodec* codec) {
       FeedbackParam(kRtcpFbParamTransportCc, kParamValueEmpty));
 }
 
-static VideoCodec MakeVideoCodecWithDefaultFeedbackParams(int payload_type,
-                                                          const char* name) {
-  VideoCodec codec(payload_type, name);
-  AddDefaultFeedbackParams(&codec);
-  return codec;
-}
-
 static std::string CodecVectorToString(const std::vector<VideoCodec>& codecs) {
   std::stringstream out;
   out << '{';
@@ -391,59 +383,6 @@ static const int kDefaultRtcpReceiverReportSsrc = 1;
 // Minimum time interval for logging stats.
 static const int64_t kStatsLogIntervalMs = 10000;
 
-// Adds |codec| to |list|, and also adds an RTX codec if |codec|'s name is
-// recognized.
-// TODO(deadbeef): Should we add RTX codecs for external codecs whose names we
-// don't recognize?
-void AddCodecAndMaybeRtxCodec(const VideoCodec& codec,
-                              std::vector<VideoCodec>* codecs) {
-  codecs->push_back(codec);
-  int rtx_payload_type = 0;
-  if (CodecNamesEq(codec.name, kVp8CodecName)) {
-    rtx_payload_type = kDefaultRtxVp8PlType;
-  } else if (CodecNamesEq(codec.name, kVp9CodecName)) {
-    rtx_payload_type = kDefaultRtxVp9PlType;
-  } else if (CodecNamesEq(codec.name, kH264CodecName)) {
-    rtx_payload_type = kDefaultRtxH264PlType;
-  } else if (CodecNamesEq(codec.name, kRedCodecName)) {
-    rtx_payload_type = kDefaultRtxRedPlType;
-  } else {
-    return;
-  }
-  codecs->push_back(VideoCodec::CreateRtxCodec(rtx_payload_type, codec.id));
-}
-
-std::vector<VideoCodec> DefaultVideoCodecList() {
-  std::vector<VideoCodec> codecs;
-  AddCodecAndMaybeRtxCodec(
-      MakeVideoCodecWithDefaultFeedbackParams(kDefaultVp8PlType, kVp8CodecName),
-      &codecs);
-  if (webrtc::VP9Encoder::IsSupported() && webrtc::VP9Decoder::IsSupported()) {
-    AddCodecAndMaybeRtxCodec(MakeVideoCodecWithDefaultFeedbackParams(
-                                 kDefaultVp9PlType, kVp9CodecName),
-                             &codecs);
-  }
-  if (webrtc::H264Encoder::IsSupported() &&
-      webrtc::H264Decoder::IsSupported()) {
-    VideoCodec codec = MakeVideoCodecWithDefaultFeedbackParams(
-        kDefaultH264PlType, kH264CodecName);
-    // TODO(hta): Move all parameter generation for SDP into the codec
-    // implementation, for all codecs and parameters.
-    // TODO(hta): Move selection of profile-level-id to H.264 codec
-    // implementation.
-    // TODO(hta): Set FMTP parameters for all codecs of type H264.
-    codec.SetParam(kH264FmtpProfileLevelId,
-                   kH264ProfileLevelConstrainedBaseline);
-    codec.SetParam(kH264FmtpLevelAsymmetryAllowed, "1");
-    codec.SetParam(kH264FmtpPacketizationMode, "1");
-    AddCodecAndMaybeRtxCodec(codec, &codecs);
-  }
-  AddCodecAndMaybeRtxCodec(VideoCodec(kDefaultRedPlType, kRedCodecName),
-                           &codecs);
-  codecs.push_back(VideoCodec(kDefaultUlpfecType, kUlpfecCodecName));
-  return codecs;
-}
-
 static std::vector<VideoCodec> GetSupportedCodecs(
     const WebRtcVideoEncoderFactory* external_encoder_factory);
 
@@ -619,50 +558,80 @@ void WebRtcVideoEngine2::SetExternalEncoderFactory(
   video_codecs_ = GetSupportedCodecs(encoder_factory);
 }
 
+// This is a helper function for AppendVideoCodecs below. The payload types of
+// |codecs| should be incremental values of the dynamic payload types [96, 127].

[hta-webrtc, 2016/11/15 11:34:59]

Grammar: I think "incrementing" is better than "incremental".

[magjed_webrtc, 2016/11/15 17:18:26]

Done.

+// This function will return nothing if no more free payload types are
+// available.
+static rtc::Optional<int> NextFreePayloadType(
+    const std::vector<VideoCodec>& codecs) {
+  static const int kFirstDynamicPayloadType = 96;
+  static const int kLastDynamicPayloadType = 127;
+  if (codecs.empty())
+    return rtc::Optional<int>(kFirstDynamicPayloadType);
+  const int next_payload_type = codecs.back().id + 1;

[hta-webrtc, 2016/11/15 11:34:59]

This strategy has two weaknesses:

1) If something gets added with a high number by another function, it will leave
a hole of unused numbers behind it.
2) If something gets added out of order, codecs.back().id + 1 may return a
number that is already in use.

If "codecs" had been a datatype that guaranteed that it was ordered (is there a
std::ordered_set?), I would be more happy with this line of code.

[magjed_webrtc, 2016/11/15 17:18:26]

Yeah, it was not supposed to be generic. Like the function comment says; it's a
helper function for AppendVideoCodecs and the payload types of |codecs| must be
incremental values. I kept it simple since it's a very simple algorithm that
just assigns the payload types incrementally from 96 to 127.

I changed the code and the comments so that it's actually generic now. The code
is slightly more complex now and we will waste a few more cycles. Let me know if
you prefer this version.

+  if (next_payload_type > kLastDynamicPayloadType)
+    return rtc::Optional<int>();
+  return rtc::Optional<int>(next_payload_type);
+}
+
+// This is a helper function for GetSupportedCodecs below. It will append new
+// unique codecs from |input_codecs| to |unified_codecs|. It will add default
+// feedback params to the codecs and will also add an associated RTX codec for
+// recognized codecs (VP8, VP9, H264, and Red).
+static void AppendVideoCodecs(const std::vector<VideoCodec>& input_codecs,
+                              std::vector<VideoCodec>* unified_codecs) {
+  for (VideoCodec codec : input_codecs) {
+    const rtc::Optional<int> payload_type =
+        NextFreePayloadType(*unified_codecs);
+    if (!payload_type)
+      return;
+    codec.id = *payload_type;
+    // TODO(magjed): Move the responsibility of setting these parameters to the
+    // encoder factories instead.
+    if (codec.name != kRedCodecName && codec.name != kUlpfecCodecName)
+      AddDefaultFeedbackParams(&codec);
+    // Don't add same codec twice.
+    if (FindMatchingCodec(*unified_codecs, codec))
+      continue;
+
+    unified_codecs->push_back(codec);
+
+    // Add associated RTX codec for recognized codecs.
+    // TODO(deadbeef): Should we add RTX codecs for external codecs whose names
+    // we don't recognize?
+    if (CodecNamesEq(codec.name, kVp8CodecName) ||
+        CodecNamesEq(codec.name, kVp9CodecName) ||
+        CodecNamesEq(codec.name, kH264CodecName) ||
+        CodecNamesEq(codec.name, kRedCodecName)) {
+      const rtc::Optional<int> rtx_payload_type =
+          NextFreePayloadType(*unified_codecs);
+      if (!rtx_payload_type)
+        return;
+      unified_codecs->push_back(
+          VideoCodec::CreateRtxCodec(*rtx_payload_type, codec.id));
+    }
+  }
+}
+
 static std::vector<VideoCodec> GetSupportedCodecs(
     const WebRtcVideoEncoderFactory* external_encoder_factory) {
-  std::vector<VideoCodec> supported_codecs = DefaultVideoCodecList();
+  const std::vector<VideoCodec>& internal_codecs =
+      InternalEncoderFactory::GetInstance().supported_codecs();
+  LOG(LS_INFO) << "Internally supported codecs: "
+               << CodecVectorToString(internal_codecs);
 
-  if (external_encoder_factory == nullptr) {
-    LOG(LS_INFO) << "Supported codecs: "
-                 << CodecVectorToString(supported_codecs);
-    return supported_codecs;
-  }
+  std::vector<VideoCodec> unified_codecs;
+  AppendVideoCodecs(internal_codecs, &unified_codecs);
 
-  std::stringstream out;
-  const std::vector<VideoCodec>& codecs =
-      external_encoder_factory->supported_codecs();
-  for (size_t i = 0; i < codecs.size(); ++i) {
-    VideoCodec codec = codecs[i];
-    out << codec.name;
-    if (i != codecs.size() - 1) {
-      out << ", ";
-    }
-    // Don't add internally-supported codecs twice.
-    if (FindMatchingCodec(supported_codecs, codec))
-      continue;
+  if (external_encoder_factory != nullptr) {

[hta-webrtc, 2016/11/15 11:34:59]

What is the result of this strategy if the internal and external factories
support exactly the same codec? (vp8, for instance)?
Will both be present with different payload types? Is that a good or a bad
thing?

[magjed_webrtc, 2016/11/15 17:18:26]

Only one codec of a specific type will be added to the list. For H264, we also
check the profile, so we will only ever have one H264 Baseline for example, but
we can have for example both H264 Baseline and H264 High.

This is enforced by the code:
// Don't add same codec twice.
if (FindMatchingCodec(*unified_codecs, codec))
  continue;
This behavior is unchanged. That check used to be at line 641 and now it's at
593.

[hta-webrtc, 2016/11/15 20:36:24]

OK, so if something's internally supported, we can't add an external codec for
the same codec and profile. Nice to know.

[magjed_webrtc, 2016/11/16 13:44:10]

Yes, we can't add the codec twice, but we will actually prefer the external
encoder over the internal encoder in WebRtcVideoSendStream::CreateVideoEncoder.
So the internal codecs provide default implementations that you can override
with the external factories. That's how iOS and Android sets up their VP8 and
H264 HW encoders.

+    const std::vector<VideoCodec>& external_codecs =
+        external_encoder_factory->supported_codecs();
+    AppendVideoCodecs(external_codecs, &unified_codecs);
+    LOG(LS_INFO) << "Codecs supported by the external encoder factory: "
+                 << CodecVectorToString(external_codecs);
+  }
 
-    // External video encoders are given payloads 120-127. This also means that
-    // we only support up to 8 external payload types.
-    // TODO(deadbeef): mediasession.cc already has code to dynamically
-    // determine a payload type. We should be able to just leave the payload
-    // type empty and let mediasession determine it. However, currently RTX
-    // codecs are associated to codecs by payload type, meaning we DO need
-    // to allocate unique payload types here. So to make this change we would
-    // need to make RTX codecs associated by name instead.
-    const int kExternalVideoPayloadTypeBase = 120;
-    size_t payload_type = kExternalVideoPayloadTypeBase + i;
-    RTC_DCHECK(payload_type < 128);
-    codec.id = payload_type;
-
-    AddDefaultFeedbackParams(&codec);
-    AddCodecAndMaybeRtxCodec(codec, &supported_codecs);
-  }
-  LOG(LS_INFO) << "Supported codecs (incl. external codecs): "
-               << CodecVectorToString(supported_codecs);
-  LOG(LS_INFO) << "Codecs supported by the external encoder factory: "
-               << out.str();
-  return supported_codecs;
+  return unified_codecs;
 }
 
 WebRtcVideoChannel2::WebRtcVideoChannel2(
@@ -1525,16 +1494,16 @@ WebRtcVideoChannel2::WebRtcVideoSendStream::VideoSendStreamParameters::
 
 WebRtcVideoChannel2::WebRtcVideoSendStream::AllocatedEncoder::AllocatedEncoder(
     webrtc::VideoEncoder* encoder,
-    webrtc::VideoCodecType type,
+    const cricket::VideoCodec& codec,
     bool external)
     : encoder(encoder),
       external_encoder(nullptr),
-      type(type),
+      codec(codec),
       external(external) {
   if (external) {
     external_encoder = encoder;
     this->encoder =
-        new webrtc::VideoEncoderSoftwareFallbackWrapper(type, encoder);
+        new webrtc::VideoEncoderSoftwareFallbackWrapper(codec, encoder);
   }
 }
 
@@ -1562,7 +1531,7 @@ WebRtcVideoChannel2::WebRtcVideoSendStream::WebRtcVideoSendStream(
       encoder_sink_(nullptr),
       parameters_(std::move(config), options, max_bitrate_bps, codec_settings),
       rtp_parameters_(CreateRtpParametersWithOneEncoding()),
-      allocated_encoder_(nullptr, webrtc::kVideoCodecUnknown, false),
+      allocated_encoder_(nullptr, cricket::VideoCodec(), false),
       sending_(false),
       last_frame_timestamp_us_(0) {
   parameters_.config.rtp.max_packet_size = kVideoMtu;
@@ -1697,36 +1666,33 @@ WebRtcVideoChannel2::WebRtcVideoSendStream::AllocatedEncoder
 WebRtcVideoChannel2::WebRtcVideoSendStream::CreateVideoEncoder(
     const VideoCodec& codec) {
   RTC_DCHECK_RUN_ON(&thread_checker_);
-  webrtc::VideoCodecType type = CodecTypeFromName(codec.name);
-
   // Do not re-create encoders of the same type.
-  if (type == allocated_encoder_.type && allocated_encoder_.encoder != NULL) {
+  if (codec == allocated_encoder_.codec &&
+      allocated_encoder_.encoder != nullptr) {
     return allocated_encoder_;
   }
 
-  if (external_encoder_factory_ != NULL) {
+  // Try creating external encoder.
+  if (external_encoder_factory_ != nullptr &&
+      FindMatchingCodec(external_encoder_factory_->supported_codecs(), codec)) {
     webrtc::VideoEncoder* encoder =
         external_encoder_factory_->CreateVideoEncoder(codec);
-    if (encoder != NULL) {
-      return AllocatedEncoder(encoder, type, true);
-    }
+    if (encoder != nullptr)
+      return AllocatedEncoder(encoder, codec, true /* is_external */);
   }
 
-  if (type == webrtc::kVideoCodecVP8) {
-    return AllocatedEncoder(
-        webrtc::VideoEncoder::Create(webrtc::VideoEncoder::kVp8), type, false);
-  } else if (type == webrtc::kVideoCodecVP9) {
-    return AllocatedEncoder(
-        webrtc::VideoEncoder::Create(webrtc::VideoEncoder::kVp9), type, false);
-  } else if (type == webrtc::kVideoCodecH264) {
-    return AllocatedEncoder(
-        webrtc::VideoEncoder::Create(webrtc::VideoEncoder::kH264), type, false);
+  // Try creating internal encoder.
+  WebRtcVideoEncoderFactory& internal_encoder_factory =
+      InternalEncoderFactory::GetInstance();
+  if (FindMatchingCodec(internal_encoder_factory.supported_codecs(), codec)) {
+    return AllocatedEncoder(internal_encoder_factory.CreateVideoEncoder(codec),
+                            codec, false /* is_external */);
   }
 
   // This shouldn't happen, we should not be trying to create something we don't
   // support.
   RTC_DCHECK(false);
-  return AllocatedEncoder(NULL, webrtc::kVideoCodecUnknown, false);
+  return AllocatedEncoder(NULL, cricket::VideoCodec(), false);
 }
 
 void WebRtcVideoChannel2::WebRtcVideoSendStream::DestroyVideoEncoder(