Add assembly for x86 to OpenH264 encoder

third_party/openh264/BUILD.gn

Index: third_party/openh264/BUILD.gn
diff --git a/third_party/openh264/BUILD.gn b/third_party/openh264/BUILD.gn
index 4301abd65e1ad83b9156bd46b2d9b750270be0b8..b7d38486e4a65c96d149d7ff7867d9d6464ba782 100644
--- a/third_party/openh264/BUILD.gn
+++ b/third_party/openh264/BUILD.gn
@@ -4,6 +4,7 @@
 
 import("//third_party/openh264/openh264_args.gni")
 import("//third_party/openh264/openh264_sources.gni")
+import("//third_party/yasm/yasm_assemble.gni")
 
 # Config shared by all openh264 targets.
 config("config") {
@@ -28,6 +29,87 @@ config("config") {
   }
 }
 
+# YASM assembly is only checked to be working on Windows.
+# This IF statement will make the targets visible only on Windows builds,
+# which will lead to failures on other platforms if accidentally invoked.
+if (is_win || is_linux) {

[ehmaldonado_chromium, 2017/01/17 14:17:33]

Do you plan to enable it for Mac too, and get rid of the conditional?
If not, please update the comment, indent the targets below, and get rid of the
conditionals testing for platforms you won't include.

[hta - Chromium, 2017/01/17 14:34:45]

Mac has a special problem with symbol handling (well, it might be a generic
problem that is only tested for on mac), which is why this is not included. I've
also had buildbot failures on other platforms (android, cast_shell) that I don't
want to include.

Is there a canonical list of is_ platforms and what they include/exclude?

[ehmaldonado_chromium, 2017/01/17 14:52:20]

The best place to look at would be:
https://cs.chromium.org/chromium/src/build/config/BUILDCONFIG.gn?q=%22is_win+...

+
+yasm_assemble("openh264_common_yasm") {
+  include_dirs = openh264_common_include_dirs
+  if (target_cpu == "x86" || target_cpu == "x64") {
+    sources = openh264_common_sources_asm_x86
+    if (target_cpu == "x86") {
+      defines = [ "X86_32" ]
+    } else {  # x64
+      if (is_mac || is_linux) {
+        defines = [
+          "PREFIX",
+          "UNIX64",
+        ]
+      } else if (is_win) {
+        defines = [ "WIN64" ]
+      } else {
+        assert(false, "Assembly not defined for this platform")
+      }
+    }
+  } else {
+    # TODO(hta): Add support for other platforms
+    assert(false, "Assembly not defined for this CPU")
+  }
+}
+
+yasm_assemble("openh264_processing_yasm") {
+  include_dirs = openh264_processing_include_dirs
+  include_dirs += [ "./src/codec/common/x86" ]
+  if (target_cpu == "x86" || target_cpu == "x64") {
+    sources = openh264_processing_sources_asm_x86
+    if (target_cpu == "x86") {
+      defines = [ "X86_32" ]
+    } else {  # x64
+      if (is_mac || is_linux) {
+        defines = [
+          "PREFIX",
+          "UNIX64",
+        ]
+      } else if (is_win) {
+        defines = [ "WIN64" ]
+      } else {
+        assert(false, "Assembly not defined for this platform")
+      }
+    }
+  } else {
+    # TODO(hta): Add support for other platforms
+    assert(false, "Assembly not defined for this CPU")

[ehmaldonado_chromium, 2017/01/17 14:17:33]

What if you move this above and make it something like:
assert(target_cpu == "x86" || target_cpu == "x64", "Assembly not defined for
this CPU")

[hta - Chromium, 2017/01/17 14:34:45]

Sounds better. Will do.

+  }
+}
+
+yasm_assemble("openh264_encoder_yasm") {
+  include_dirs = openh264_encoder_include_dirs
+  include_dirs += [ "./src/codec/common/x86" ]
+  if (target_cpu == "x86" || target_cpu == "x64") {
+    sources = openh264_encoder_sources_asm_x86
+    if (target_cpu == "x86") {
+      defines = [ "X86_32" ]
+    } else {  # x64
+      if (is_mac || is_linux) {
+        defines = [
+          "PREFIX",
+          "UNIX64",
+        ]
+      } else if (is_win) {
+        defines = [ "WIN64" ]
+      } else {
+        assert(false, "Assembly not defined for this platform")
+      }
+    }
+  } else {
+    # TODO(hta): Add support for other platforms
+    assert(false, "Assembly not defined for this CPU")
+  }
+}
+
+}  # if is_win
+
 source_set("common") {
   sources = openh264_common_sources
   include_dirs = openh264_common_include_dirs
@@ -36,6 +118,10 @@ source_set("common") {
   configs += [ "//build/config/compiler:no_chromium_code" ]
   configs += [ ":config" ]
   deps = []
+  if (is_win && (target_cpu == "x86" || target_cpu == "x64")) {

[kthelgason, 2017/01/18 08:10:33]

Targets are defined for windows and linux, but only included on windows. Is this
intentional?

+    defines = [ "X86_ASM" ]
+    deps += [ ":openh264_common_yasm" ]
+  }
   if (is_android) {
     deps += [
       # Defines "android_get/setCpu..." functions. The original OpenH264 build
@@ -57,6 +143,10 @@ source_set("processing") {
   deps = [
     ":common",
   ]
+  if (is_win && (target_cpu == "x86" || target_cpu == "x64")) {
+    defines = [ "X86_ASM" ]
+    deps += [ ":openh264_processing_yasm" ]
+  }
 }
 
 source_set("encoder") {
@@ -76,4 +166,8 @@ source_set("encoder") {
     ":common",
     ":processing",
   ]
+  if (is_win && (target_cpu == "x86" || target_cpu == "x64")) {
+    defines = [ "X86_ASM" ]
+    deps += [ ":openh264_encoder_yasm" ]
+  }
 }