From 06f3aae345854ba9dcc5ae3b603de1f86505acf9 Mon Sep 17 00:00:00 2001
From: magjed <magjed@webrtc.org>
Date: Fri, 14 Jul 2017 10:36:23 -0700
Subject: [PATCH] Prefer external video codecs over internal in SDP

Currently, when we generate the list of supported video codecs that will
be signaled in SDP, we start with the internal video codecs and then
append the external video codecs. When we create a video encoder for a
given codec, we prefer an external encoder over an internal encoder.

This CL lists the external video codecs first in SDP instead, so that we
consistently prefer external video codecs over internal.

The reason for doing this is that we will otherwise prefer an internal
SW H264 encoder over an external HW H264 encoder if the H264 profiles
differs.

BUG=chromium:688541

Review-Url: https://codereview.webrtc.org/2974383002
Cr-Commit-Position: refs/heads/master@{#19026}
---
 webrtc/media/engine/webrtcvideoengine.cc      |  4 ++--
 .../engine/webrtcvideoengine_unittest.cc      | 21 ++++++++++++-------
 2 files changed, 16 insertions(+), 9 deletions(-)

diff --git a/webrtc/media/engine/webrtcvideoengine.cc b/webrtc/media/engine/webrtcvideoengine.cc
index b6447329e2..e1939c35d0 100644
--- a/webrtc/media/engine/webrtcvideoengine.cc
+++ b/webrtc/media/engine/webrtcvideoengine.cc
@@ -537,8 +537,6 @@ static std::vector<VideoCodec> GetSupportedCodecs(
                << CodecVectorToString(internal_codecs);
 
   std::vector<VideoCodec> unified_codecs;
-  AppendVideoCodecs(internal_codecs, &unified_codecs);
-
   if (external_encoder_factory != nullptr) {
     const std::vector<VideoCodec>& external_codecs =
         external_encoder_factory->supported_codecs();
@@ -547,6 +545,8 @@ static std::vector<VideoCodec> GetSupportedCodecs(
                  << CodecVectorToString(external_codecs);
   }
 
+  AppendVideoCodecs(internal_codecs, &unified_codecs);
+
   return unified_codecs;
 }
 
diff --git a/webrtc/media/engine/webrtcvideoengine_unittest.cc b/webrtc/media/engine/webrtcvideoengine_unittest.cc
index 9d4c809f4d..51f6e9c656 100644
--- a/webrtc/media/engine/webrtcvideoengine_unittest.cc
+++ b/webrtc/media/engine/webrtcvideoengine_unittest.cc
@@ -819,12 +819,12 @@ TEST_F(WebRtcVideoEngineTest, ReportSupportedExternalCodecs) {
 
   std::vector<cricket::VideoCodec> codecs(engine_.codecs());
   ASSERT_GE(codecs.size(), 2u);
-  cricket::VideoCodec internal_codec = codecs.front();
-  cricket::VideoCodec external_codec = codecs.back();
+  cricket::VideoCodec external_codec = codecs[0];
+  cricket::VideoCodec internal_codec = codecs[1];
 
-  // The external codec will appear last in the vector.
-  EXPECT_EQ("VP8", internal_codec.name);
+  // The external codec will appear first in the vector.
   EXPECT_EQ("FakeExternalCodec", external_codec.name);
+  EXPECT_EQ("VP8", internal_codec.name);
 }
 
 // Test that an external codec that was added after the engine was initialized
@@ -836,15 +836,15 @@ TEST_F(WebRtcVideoEngineTest, ReportSupportedExternalCodecsWithAddedCodec) {
   engine_.SetExternalEncoderFactory(&encoder_factory);
   engine_.Init();
 
-  // The first external codec will appear last in the vector.
+  // The first external codec will appear first in the vector.
   std::vector<cricket::VideoCodec> codecs_before(engine_.codecs());
-  EXPECT_EQ("FakeExternalCodec1", codecs_before.back().name);
+  EXPECT_EQ("FakeExternalCodec1", codecs_before.front().name);
 
   // Add second codec.
   encoder_factory.AddSupportedVideoCodecType("FakeExternalCodec2");
   std::vector<cricket::VideoCodec> codecs_after(engine_.codecs());
   EXPECT_EQ(codecs_before.size() + 1, codecs_after.size());
-  EXPECT_EQ("FakeExternalCodec2", codecs_after.back().name);
+  EXPECT_EQ("FakeExternalCodec2", codecs_after[1].name);
 }
 
 TEST_F(WebRtcVideoEngineTest, RegisterExternalDecodersIfSupported) {
@@ -1066,6 +1066,11 @@ class WebRtcVideoChannelTest : public WebRtcVideoEngineTest {
     ASSERT_TRUE(channel_->SetSendParameters(send_parameters_));
   }
 
+  void TearDown() override {
+    channel_ = nullptr;
+    fake_call_ = nullptr;
+  }
+
  protected:
   FakeVideoSendStream* AddSendStream() {
     return AddSendStream(StreamParams::CreateLegacy(++last_ssrc_));
@@ -1975,6 +1980,8 @@ class Vp9SettingsTest : public WebRtcVideoChannelTest {
     // Remove references to encoder_factory_ since this will be destroyed
     // before channel_ and engine_.
     ASSERT_TRUE(channel_->SetSendParameters(send_parameters_));
+
+    WebRtcVideoChannelTest::TearDown();
   }
 
   cricket::FakeWebRtcVideoEncoderFactory encoder_factory_;