Replace WebRtcVideoEncoderFactory::VideoCodec with cricket::VideoCodec

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 490bbf4504d865d160dc85bb5204d268134c6cc7..154d5e680218deb129f67d4a36cc5bb2bd249eb1 100644
--- a/webrtc/media/engine/webrtcvideoengine2.cc
+++ b/webrtc/media/engine/webrtcvideoengine2.cc
@@ -49,7 +49,7 @@ class EncoderFactoryAdapter : public webrtc::VideoEncoderFactory {
 
   // Implement webrtc::VideoEncoderFactory.
   webrtc::VideoEncoder* Create() override {
-    return factory_->CreateVideoEncoder(webrtc::kVideoCodecVP8);
+    return factory_->CreateVideoEncoder(VideoCodec(kVp8CodecName));
   }
 
   void Destroy(webrtc::VideoEncoder* encoder) override {
@@ -99,11 +99,11 @@ class WebRtcSimulcastEncoderFactory
       : factory_(factory) {}
 
   static bool UseSimulcastEncoderFactory(
-      const std::vector<VideoCodec>& codecs) {
+      const std::vector<cricket::VideoCodec>& codecs) {
     // If any codec is VP8, use the simulcast factory. If asked to create a
     // non-VP8 codec, we'll just return a contained factory encoder directly.
     for (const auto& codec : codecs) {
-      if (codec.type == webrtc::kVideoCodecVP8) {
+      if (CodecNamesEq(codec.name.c_str(), kVp8CodecName)) {
         return true;
       }
     }
@@ -111,22 +111,22 @@ class WebRtcSimulcastEncoderFactory
   }
 
   webrtc::VideoEncoder* CreateVideoEncoder(
-      webrtc::VideoCodecType type) override {
+      const cricket::VideoCodec& codec) override {
     RTC_DCHECK(factory_ != NULL);
     // If it's a codec type we can simulcast, create a wrapped encoder.
-    if (type == webrtc::kVideoCodecVP8) {
+    if (CodecNamesEq(codec.name.c_str(), kVp8CodecName)) {
       return new webrtc::SimulcastEncoderAdapter(
           new EncoderFactoryAdapter(factory_));
     }
-    webrtc::VideoEncoder* encoder = factory_->CreateVideoEncoder(type);
+    webrtc::VideoEncoder* encoder = factory_->CreateVideoEncoder(codec);
     if (encoder) {
       non_simulcast_encoders_.push_back(encoder);
     }
     return encoder;
   }
 
-  const std::vector<VideoCodec>& codecs() const override {
-    return factory_->codecs();
+  const std::vector<cricket::VideoCodec>& supported_codecs() const override {
+    return factory_->supported_codecs();
   }
 
   bool EncoderTypeHasInternalSource(
@@ -156,21 +156,6 @@ class WebRtcSimulcastEncoderFactory
   std::vector<webrtc::VideoEncoder*> non_simulcast_encoders_;
 };
 
-bool CodecIsInternallySupported(const std::string& codec_name) {
-  if (CodecNamesEq(codec_name, kVp8CodecName)) {
-    return true;
-  }
-  if (CodecNamesEq(codec_name, kVp9CodecName)) {
-    return webrtc::VP9Encoder::IsSupported() &&
-           webrtc::VP9Decoder::IsSupported();
-  }
-  if (CodecNamesEq(codec_name, kH264CodecName)) {
-    return webrtc::H264Encoder::IsSupported() &&
-        webrtc::H264Decoder::IsSupported();
-  }
-  return false;
-}
-
 void AddDefaultFeedbackParams(VideoCodec* codec) {
   codec->AddFeedbackParam(FeedbackParam(kRtcpFbParamCcm, kRtcpFbCcmParamFir));
   codec->AddFeedbackParam(FeedbackParam(kRtcpFbParamNack, kParamValueEmpty));
@@ -443,12 +428,13 @@ std::vector<VideoCodec> DefaultVideoCodecList() {
   AddCodecAndMaybeRtxCodec(
       MakeVideoCodecWithDefaultFeedbackParams(kDefaultVp8PlType, kVp8CodecName),
       &codecs);
-  if (CodecIsInternallySupported(kVp9CodecName)) {
+  if (webrtc::VP9Encoder::IsSupported() && webrtc::VP9Decoder::IsSupported()) {
     AddCodecAndMaybeRtxCodec(MakeVideoCodecWithDefaultFeedbackParams(
                                  kDefaultVp9PlType, kVp9CodecName),
                              &codecs);
   }
-  if (CodecIsInternallySupported(kH264CodecName)) {
+  if (webrtc::H264Encoder::IsSupported() &&
+      webrtc::H264Decoder::IsSupported()) {
     VideoCodec codec = MakeVideoCodecWithDefaultFeedbackParams(
         kDefaultH264PlType, kH264CodecName);
     // TODO(hta): Move all parameter generation for SDP into the codec
@@ -468,6 +454,9 @@ std::vector<VideoCodec> DefaultVideoCodecList() {
   return codecs;
 }
 
+static std::vector<VideoCodec> GetSupportedCodecs(
+    const WebRtcVideoEncoderFactory* external_encoder_factory);
+
 rtc::scoped_refptr<webrtc::VideoEncoderConfig::EncoderSpecificSettings>
 WebRtcVideoChannel2::WebRtcVideoSendStream::ConfigureVideoEncoderSettings(
     const VideoCodec& codec) {
@@ -565,7 +554,7 @@ WebRtcVideoEngine2::WebRtcVideoEngine2()
       external_decoder_factory_(NULL),
       external_encoder_factory_(NULL) {
   LOG(LS_INFO) << "WebRtcVideoEngine2::WebRtcVideoEngine2()";
-  video_codecs_ = GetSupportedCodecs();
+  video_codecs_ = GetSupportedCodecs(external_encoder_factory_);
 }
 
 WebRtcVideoEngine2::~WebRtcVideoEngine2() {
@@ -630,35 +619,44 @@ void WebRtcVideoEngine2::SetExternalEncoderFactory(
 
   if (encoder_factory &&
       WebRtcSimulcastEncoderFactory::UseSimulcastEncoderFactory(
-          encoder_factory->codecs())) {
+          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();
+  video_codecs_ = GetSupportedCodecs(encoder_factory);
 }
 
-std::vector<VideoCodec> WebRtcVideoEngine2::GetSupportedCodecs() const {
+static std::vector<VideoCodec> GetSupportedCodecs(
+    const WebRtcVideoEncoderFactory* external_encoder_factory) {
   std::vector<VideoCodec> supported_codecs = DefaultVideoCodecList();
 
-  if (external_encoder_factory_ == NULL) {
+  if (external_encoder_factory == nullptr) {
     LOG(LS_INFO) << "Supported codecs: "
                  << CodecVectorToString(supported_codecs);
     return supported_codecs;
   }
 
   std::stringstream out;
-  const std::vector<WebRtcVideoEncoderFactory::VideoCodec>& codecs =
-      external_encoder_factory_->codecs();
+  const std::vector<VideoCodec>& codecs =
+      external_encoder_factory->supported_codecs();
   for (size_t i = 0; i < codecs.size(); ++i) {
-    out << codecs[i].name;
+    VideoCodec codec = codecs[i];
+    out << codec.name;
     if (i != codecs.size() - 1) {
       out << ", ";
     }
     // Don't add internally-supported codecs twice.
-    if (CodecIsInternallySupported(codecs[i].name)) {
+    bool is_codec_already_added = false;
+    for (const VideoCodec& supported_codec : supported_codecs) {
+      if (codec.Matches(supported_codec)) {
+        is_codec_already_added = true;
+        break;
+      }
+    }

[kthelgason, 2016/10/28 08:26:54]

This code exists in a few places. We could use std::any_of here to avoid
manually writing the loop, and to make the intent more clear.

[magjed_webrtc, 2016/10/28 10:27:31]

Thanks! Much better to extract it to a function. I went with std::any_of, but it
might not be more clear than a regular for-loop. It's this:
return std::any_of(supported_codecs.begin(), supported_codecs.end(),
                   [&codec](const VideoCodec& supported_codec) -> bool {
                     return supported_codec.Matches(codec);
                   });
vs this:
for (const VideoCodec& supported_codec : supported_codecs) {
  if (supported_codec.Matches(codec))
    return true;
}
return false;

+    if (is_codec_already_added) {
       continue;
     }
 
@@ -673,7 +671,7 @@ std::vector<VideoCodec> WebRtcVideoEngine2::GetSupportedCodecs() const {
     const int kExternalVideoPayloadTypeBase = 120;
     size_t payload_type = kExternalVideoPayloadTypeBase + i;
     RTC_DCHECK(payload_type < 128);
-    VideoCodec codec(static_cast<int>(payload_type), codecs[i].name);
+    codec.id = payload_type;
 
     AddDefaultFeedbackParams(&codec);
     AddCodecAndMaybeRtxCodec(codec, &supported_codecs);
@@ -716,35 +714,21 @@ WebRtcVideoChannel2::~WebRtcVideoChannel2() {
     delete kv.second;
 }
 
-bool WebRtcVideoChannel2::CodecIsExternallySupported(
-    const std::string& name) const {
-  if (external_encoder_factory_ == NULL) {
-    return false;
-  }
-
-  const std::vector<WebRtcVideoEncoderFactory::VideoCodec> external_codecs =
-      external_encoder_factory_->codecs();
-  for (size_t c = 0; c < external_codecs.size(); ++c) {
-    if (CodecNamesEq(name, external_codecs[c].name)) {
-      return true;
-    }
-  }
-  return false;
-}
-
 std::vector<WebRtcVideoChannel2::VideoCodecSettings>
 WebRtcVideoChannel2::FilterSupportedCodecs(
-    const std::vector<WebRtcVideoChannel2::VideoCodecSettings>& mapped_codecs)
-    const {
-  std::vector<VideoCodecSettings> supported_codecs;
-  for (size_t i = 0; i < mapped_codecs.size(); ++i) {
-    const VideoCodecSettings& codec = mapped_codecs[i];
-    if (CodecIsInternallySupported(codec.codec.name) ||
-        CodecIsExternallySupported(codec.codec.name)) {
-      supported_codecs.push_back(codec);
+    const std::vector<VideoCodecSettings>& mapped_codecs) const {
+  const std::vector<VideoCodec> supported_codecs =
+      GetSupportedCodecs(external_encoder_factory_);
+  std::vector<VideoCodecSettings> filtered_codecs;
+  for (const VideoCodecSettings& mapped_codec : mapped_codecs) {
+    for (const VideoCodec& supported_codec : supported_codecs) {
+      if (mapped_codec.codec.Matches(supported_codec)) {
+        filtered_codecs.push_back(mapped_codec);
+        break;
+      }
     }
   }

[kthelgason, 2016/10/28 08:26:54]

The inner loop here is identical to a previous one in this class. Perhaps we can
extract that out into a function, perhaps "bool IsCodecSupported(VideoCodec
codec)" and reuse that here. Then this whole thing could be a single
std::copy_if call.

[magjed_webrtc, 2016/10/28 10:27:31]

Done, the inner loop is now extracted out into a IsCodecSupported helper
function. I didn't go with std::copy_if, because I think it adds more noise than
clarity. It's this:
  std::copy_if(
      mapped_codecs.begin(), mapped_codecs.end(),
      std::back_inserter(filtered_codecs),
      [&supported_codecs](const VideoCodecSettings& mapped_codec) -> bool {
        return IsCodecSupported(supported_codecs, mapped_codec.codec);
      });
vs
  for (const VideoCodecSettings& mapped_codec : mapped_codecs) {
    if (IsCodecSupported(supported_codecs, mapped_codec.codec))
      filtered_codecs.push_back(mapped_codec);
  }

[kthelgason, 2016/10/28 13:19:45]

I agree with your assessment that this adds more noise than clarity. I had
imagined something like:
std::copy_if(mapped_codecs.begin(), mapped_codecs.end(),
    std::back_inserter(filtered_codecs), &IsCodecSupported)

but this is not javascript, and that obviously does not work :)

[magjed_webrtc, 2016/10/28 13:30:17]

Yeah, it's close to working with:
std::copy_if(mapped_codecs.begin(), mapped_codecs.end(),
    std::back_inserter(filtered_codecs),
    std::bind(&IsCodecSupported, supported_codecs));
Not quite though, because:
 1. std::bind is not yet an allowed C++11 feature in WebRTC.
 2. mapped_codec is of type VideoCodecSettings and not VideoCodec, so I need to
do mapped_codec.codec which essentially requires a lambda in C++.
Haskell has much nicer and cleaner glue functions :)

-  return supported_codecs;
+  return filtered_codecs;
 }
 
 bool WebRtcVideoChannel2::ReceiveCodecsHaveChanged(
@@ -1770,17 +1754,6 @@ WebRtcVideoChannel2::WebRtcVideoSendStream::GetSsrcs() const {
   return ssrcs_;
 }
 
-webrtc::VideoCodecType CodecTypeFromName(const std::string& name) {
-  if (CodecNamesEq(name, kVp8CodecName)) {
-    return webrtc::kVideoCodecVP8;
-  } else if (CodecNamesEq(name, kVp9CodecName)) {
-    return webrtc::kVideoCodecVP9;
-  } else if (CodecNamesEq(name, kH264CodecName)) {
-    return webrtc::kVideoCodecH264;
-  }
-  return webrtc::kVideoCodecUnknown;
-}
-
 WebRtcVideoChannel2::WebRtcVideoSendStream::AllocatedEncoder
 WebRtcVideoChannel2::WebRtcVideoSendStream::CreateVideoEncoder(
     const VideoCodec& codec) {
@@ -1794,7 +1767,7 @@ WebRtcVideoChannel2::WebRtcVideoSendStream::CreateVideoEncoder(
 
   if (external_encoder_factory_ != NULL) {
     webrtc::VideoEncoder* encoder =
-        external_encoder_factory_->CreateVideoEncoder(type);
+        external_encoder_factory_->CreateVideoEncoder(codec);
     if (encoder != NULL) {
       return AllocatedEncoder(encoder, type, true);
     }