Stop using hardcoded payload types for video codecs

webrtc/media/engine/webrtcvideoengine2.cc

 /*
  *  Copyright (c) 2014 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/media/engine/webrtcvideoengine2.h"
 
 #include <stdio.h>
 #include <algorithm>
 #include <set>
 #include <string>
 #include <utility>
 
 #include "webrtc/base/copyonwritebuffer.h"
 #include "webrtc/base/logging.h"
 #include "webrtc/base/stringutils.h"
 #include "webrtc/base/timeutils.h"
 #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"
 
 namespace cricket {
 namespace {
 
 // Wrap cricket::WebRtcVideoEncoderFactory as a webrtc::VideoEncoderFactory.
 class EncoderFactoryAdapter : public webrtc::VideoEncoderFactory {
  public:
@@ skipping 118 matching lines @@
 
 void AddDefaultFeedbackParams(VideoCodec* codec) {
   codec->AddFeedbackParam(FeedbackParam(kRtcpFbParamCcm, kRtcpFbCcmParamFir));
   codec->AddFeedbackParam(FeedbackParam(kRtcpFbParamNack, kParamValueEmpty));
   codec->AddFeedbackParam(FeedbackParam(kRtcpFbParamNack, kRtcpFbNackParamPli));
   codec->AddFeedbackParam(FeedbackParam(kRtcpFbParamRemb, kParamValueEmpty));
   codec->AddFeedbackParam(
       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 << '{';
   for (size_t i = 0; i < codecs.size(); ++i) {
     out << codecs[i].ToString();
     if (i != codecs.size() - 1) {
       out << ", ";
     }
   }
   out << '}';
@@ skipping 195 matching lines @@
 // duration hasn't been implemented.
 static const int kNackHistoryMs = 1000;
 
 static const int kDefaultQpMax = 56;
 
 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);
 
 rtc::scoped_refptr<webrtc::VideoEncoderConfig::EncoderSpecificSettings>
 WebRtcVideoChannel2::WebRtcVideoSendStream::ConfigureVideoEncoderSettings(
     const VideoCodec& codec) {
   RTC_DCHECK_RUN_ON(&thread_checker_);
   bool is_screencast = parameters_.options.is_screencast.value_or(false);
   // No automatic resizing when using simulcast or screencast.
   bool automatic_resize =
@@ skipping 155 matching lines @@
           encoder_factory->supported_codecs())) {
     simulcast_encoder_factory_.reset(
         new WebRtcSimulcastEncoderFactory(encoder_factory));
     encoder_factory = simulcast_encoder_factory_.get();
   }
   external_encoder_factory_ = encoder_factory;
 
   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);
+
+  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);
   }
 
-  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;
-
-    // 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(
     webrtc::Call* call,
     const MediaConfig& config,
     const VideoOptions& options,
     WebRtcVideoEncoderFactory* external_encoder_factory,
     WebRtcVideoDecoderFactory* external_decoder_factory)
     : VideoMediaChannel(config),
       call_(call),
@@ skipping 842 matching lines @@
         int max_bitrate_bps,
         const rtc::Optional<VideoCodecSettings>& codec_settings)
     : config(std::move(config)),
       options(options),
       max_bitrate_bps(max_bitrate_bps),
       conference_mode(false),
       codec_settings(codec_settings) {}
 
 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);
   }
 }
 
 WebRtcVideoChannel2::WebRtcVideoSendStream::WebRtcVideoSendStream(
     webrtc::Call* call,
     const StreamParams& sp,
     webrtc::VideoSendStream::Config config,
     const VideoOptions& options,
     WebRtcVideoEncoderFactory* external_encoder_factory,
     bool enable_cpu_overuse_detection,
     int max_bitrate_bps,
     const rtc::Optional<VideoCodecSettings>& codec_settings,
     const rtc::Optional<std::vector<webrtc::RtpExtension>>& rtp_extensions,
     // TODO(deadbeef): Don't duplicate information between send_params,
     // rtp_extensions, options, etc.
     const VideoSendParameters& send_params)
     : worker_thread_(rtc::Thread::Current()),
       ssrcs_(sp.ssrcs),
       ssrc_groups_(sp.ssrc_groups),
       call_(call),
       enable_cpu_overuse_detection_(enable_cpu_overuse_detection),
       source_(nullptr),
       external_encoder_factory_(external_encoder_factory),
       stream_(nullptr),
       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;
   parameters_.conference_mode = send_params.conference_mode;
 
   sp.GetPrimarySsrcs(&parameters_.config.rtp.ssrcs);
   sp.GetFidSsrcs(parameters_.config.rtp.ssrcs,
                  &parameters_.config.rtp.rtx.ssrcs);
   parameters_.config.rtp.c_name = sp.cname;
   if (rtp_extensions) {
@@ skipping 114 matching lines @@
 
 const std::vector<uint32_t>&
 WebRtcVideoChannel2::WebRtcVideoSendStream::GetSsrcs() const {
   return ssrcs_;
 }
 
 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(
     AllocatedEncoder* encoder) {
   RTC_DCHECK_RUN_ON(&thread_checker_);
   if (encoder->external) {
     external_encoder_factory_->DestroyVideoEncoder(encoder->external_encoder);
   }
   delete encoder->encoder;
 }
@@ skipping 813 matching lines @@
         rtx_mapping[video_codecs[i].codec.id] !=
             ulpfec_config.red_payload_type) {
       video_codecs[i].rtx_payload_type = rtx_mapping[video_codecs[i].codec.id];
     }
   }
 
   return video_codecs;
 }
 
 }  // namespace cricket