Add assembly for x86 to OpenH264 encoder

third_party/openh264/BUILD.gn

 # Copyright 2015 The Chromium Authors. All rights reserved.
 # Use of this source code is governed by a BSD-style license that can be
 # found in the LICENSE file.
 
 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") {
   cflags = []
   defines = []
 
   # GCC and clang flags. MSVS (is_win && !is_clang) does not use cflags.
   if (!is_win || is_clang) {
     cflags += [
       "-Wno-format",
       "-Wno-header-hygiene",
       "-Wno-unused-function",
       "-Wno-unused-value",
     ]
   }
 
   # Platform-specific defines.
   if (is_android) {
     # Android NDK is necessary for its cpufeatures and this define is what
     # OpenH264 code uses to check if it should be used.
     defines += [ "ANDROID_NDK" ]
   }
 }
 
+# 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
 
   configs -= [ "//build/config/compiler:chromium_code" ]
   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
       # files replaces these using macros for "wels_..." versions of the same
       # functions. We do not have access to these and use the <cpu-features.h>
       # ones instead.
       "//third_party/android_tools:cpu_features",
     ]
   }
 }
 
 source_set("processing") {
   sources = openh264_processing_sources
   include_dirs = openh264_processing_include_dirs
 
   configs -= [ "//build/config/compiler:chromium_code" ]
   configs += [ "//build/config/compiler:no_chromium_code" ]
   configs += [ ":config" ]
   deps = [
     ":common",
   ]
+  if (is_win && (target_cpu == "x86" || target_cpu == "x64")) {
+    defines = [ "X86_ASM" ]
+    deps += [ ":openh264_processing_yasm" ]
+  }
 }
 
 source_set("encoder") {
   sources = openh264_encoder_sources
   include_dirs = openh264_encoder_include_dirs
 
   configs -= [ "//build/config/compiler:chromium_code" ]
   configs += [ "//build/config/compiler:no_chromium_code" ]
   configs += [ ":config" ]
 
   # TODO: Remove after fixing always-true condition
   # third_party/openh264/src/codec/encoder/core/src/encoder_ext.cpp:142.
   if (is_clang) {
     configs -= [ "//build/config/clang:extra_warnings" ]
   }
   deps = [
     ":common",
     ":processing",
   ]
+  if (is_win && (target_cpu == "x86" || target_cpu == "x64")) {
+    defines = [ "X86_ASM" ]
+    deps += [ ":openh264_encoder_yasm" ]
+  }
 }