Revert of Add assembly for x86 to OpenH264 encoder

Revert of Add assembly for x86 to OpenH264 encoder (patchset #5 id:80001 of https://codereview.chromium.org/2585733002/ )

Reason for revert:
This broke Chrome OS builders:

https://uberchromegw.corp.google.com/i/chromeos/builders/veyron_minnie-chrome-pfq/builds/669/steps/BuildPackages/logs/stdio


Original issue's description:
> Add assembly for x86 to OpenH264 encoder
> 
> On Linux on a build workstation, these changes seem to give around 2.5x faster encoding (frame encode time in test goes from ~25 to ~9 ms)
> 
> We do expect alerts from our performance bots when this lands.
> 
> Credit: These build changes were written by brian@highfive.com aka bbaldino@gmail.com.
> 
> BUG=600399
> 
> Review-Url: https://codereview.chromium.org/2585733002
> Cr-Commit-Position: refs/heads/master@{#445033}
> Committed: https://chromium.googlesource.com/chromium/src/+/6d1501f500099ff3e25f92593e9efcb27d352252

TBR=kthelgason@chromium.org,ehmaldonado@chromium.org,kthelgason@webrtc.org,hta@chromium.org,mflodman@chromium.org
# Not skipping CQ checks because original CL landed more than 1 days ago.
BUG=600399
Review-Url: https://codereview.chromium.org/2651543002
Cr-Commit-Position: refs/heads/master@{#445311}
Committed: https://chromium.googlesource.com/chromium/src/+/3f4ee1b009741853987773717ebc2f22c5f0c60b

[haraken, 2017-01-23 00:31:42]

The CQ bit was checked by haraken@chromium.org

[haraken, 2017-01-23 00:31:42]

Created Revert of Add assembly for x86 to OpenH264 encoder

[commit-bot: I haz the power, 2017-01-23 00:31:54]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-23 01:29:13]

CQ is committing da patch.

Bot data: {"patchset_id": 1, "attempt_start_ts": 1485131502233590, "parent_rev":
"af286b82f997fada45bb41a90e6ffb6bf9d2a242", "commit_rev":
"3f4ee1b009741853987773717ebc2f22c5f0c60b"}

[commit-bot: I haz the power, 2017-01-23 01:30:49]

Description was changed from

==========
Revert of Add assembly for x86 to OpenH264 encoder (patchset #5 id:80001 of
https://codereview.chromium.org/2585733002/ )

Reason for revert:
This broke Chrome OS builders:

https://uberchromegw.corp.google.com/i/chromeos/builders/veyron_minnie-chrome...


Original issue's description:
> Add assembly for x86 to OpenH264 encoder
> 
> On Linux on a build workstation, these changes seem to give around 2.5x faster
encoding (frame encode time in test goes from ~25 to ~9 ms)
> 
> We do expect alerts from our performance bots when this lands.
> 
> Credit: These build changes were written by brian@highfive.com aka
bbaldino@gmail.com.
> 
> BUG=600399
> 
> Review-Url: https://codereview.chromium.org/2585733002
> Cr-Commit-Position: refs/heads/master@{#445033}
> Committed:
https://chromium.googlesource.com/chromium/src/+/6d1501f500099ff3e25f92593e9e...

TBR=kthelgason@chromium.org,ehmaldonado@chromium.org,kthelgason@webrtc.org,ht...
# Not skipping CQ checks because original CL landed more than 1 days ago.
BUG=600399
==========

to

==========
Revert of Add assembly for x86 to OpenH264 encoder (patchset #5 id:80001 of
https://codereview.chromium.org/2585733002/ )

Reason for revert:
This broke Chrome OS builders:

https://uberchromegw.corp.google.com/i/chromeos/builders/veyron_minnie-chrome...


Original issue's description:
> Add assembly for x86 to OpenH264 encoder
> 
> On Linux on a build workstation, these changes seem to give around 2.5x faster
encoding (frame encode time in test goes from ~25 to ~9 ms)
> 
> We do expect alerts from our performance bots when this lands.
> 
> Credit: These build changes were written by brian@highfive.com aka
bbaldino@gmail.com.
> 
> BUG=600399
> 
> Review-Url: https://codereview.chromium.org/2585733002
> Cr-Commit-Position: refs/heads/master@{#445033}
> Committed:
https://chromium.googlesource.com/chromium/src/+/6d1501f500099ff3e25f92593e9e...

TBR=kthelgason@chromium.org,ehmaldonado@chromium.org,kthelgason@webrtc.org,ht...
# Not skipping CQ checks because original CL landed more than 1 days ago.
BUG=600399

Review-Url: https://codereview.chromium.org/2651543002
Cr-Commit-Position: refs/heads/master@{#445311}
Committed:
https://chromium.googlesource.com/chromium/src/+/3f4ee1b009741853987773717ebc...
==========

[commit-bot: I haz the power, 2017-01-23 01:30:52]

Committed patchset #1 (id:1) as
https://chromium.googlesource.com/chromium/src/+/3f4ee1b009741853987773717ebc...

[chromium-reviews, 2017-01-23 06:39:53]

Haraken, can you tell me what the target_cpu value was for this builder?

Is there a trybot for this config that is not in the standard trybot set,
so that I can try it?


On Mon, Jan 23, 2017 at 1:31 AM, <haraken@chromium.org> wrote:

> Reviewers: kthelgason_chromium, ehmaldonado_chromium, kthelgason, hta -
> Chromium
> CL: https://codereview.chromium.org/2651543002/
>
> Message:
> Created Revert of Add assembly for x86 to OpenH264 encoder
>
> Description:
> Revert of Add assembly for x86 to OpenH264 encoder (patchset #5 id:80001 of
> https://codereview.chromium.org/2585733002/ )
>
> Reason for revert:
> This broke Chrome OS builders:
>
> https://uberchromegw.corp.google.com/i/chromeos/
> builders/veyron_minnie-chrome-pfq/builds/669/steps/
> BuildPackages/logs/stdio
>
>
> Original issue's description:
> > Add assembly for x86 to OpenH264 encoder
> >
> > On Linux on a build workstation, these changes seem to give around 2.5x
> faster
> encoding (frame encode time in test goes from ~25 to ~9 ms)
> >
> > We do expect alerts from our performance bots when this lands.
> >
> > Credit: These build changes were written by brian@highfive.com aka
> bbaldino@gmail.com.
> >
> > BUG=600399
> >
> > Review-Url: https://codereview.chromium.org/2585733002
> > Cr-Commit-Position: refs/heads/master@{#445033}
> > Committed:
> https://chromium.googlesource.com/chromium/src/+/
> 6d1501f500099ff3e25f92593e9efcb27d352252
>
> TBR=kthelgason@chromium.org,ehmaldonado@chromium.org,
> kthelgason@webrtc.org,hta@chromium.org
> # Not skipping CQ checks because original CL landed more than 1 days ago.
> BUG=600399
>
> Affected files (+0, -86 lines):
> M 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 dfefac587585017937c962e544e5c393a9d3ca71..
> 4301abd65e1ad83b9156bd46b2d9b750270be0b8 100644
> --- a/third_party/openh264/BUILD.gn
> +++ b/third_party/openh264/BUILD.gn
> @@ -4,7 +4,6 @@
>
> 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") {
> @@ -29,79 +28,6 @@
> }
> }
>
> -# YASM assembly is only checked to be working on Windows and Linux.
> -# Mac is known to fail certain tests when building, but actual assembly
> -# is believed to work.
> -#
> -# This IF statement will make the targets visible only on specific builds,
> -# which will lead to failures on other platforms if accidentally invoked.
> -if (is_win || is_linux) {
> - yasm_assemble("openh264_common_yasm") {
> - include_dirs = openh264_common_include_dirs
> - assert(target_cpu == "x86" || target_cpu == "x64",
> - "Assembly not defined for this CPU")
> - sources = openh264_common_sources_asm_x86
> - if (target_cpu == "x86") {
> - defines = [ "X86_32" ]
> - } else { # x64
> - if (is_mac) {
> - defines = [
> - "PREFIX",
> - "UNIX64",
> - ]
> - } else if (is_win) {
> - defines = [ "WIN64" ]
> - } else if (is_linux) {
> - defines = [ "UNIX64" ]
> - }
> - }
> - }
> -
> - yasm_assemble("openh264_processing_yasm") {
> - include_dirs = openh264_processing_include_dirs
> - include_dirs += [ "./src/codec/common/x86" ]
> - assert(target_cpu == "x86" || target_cpu == "x64",
> - "Assembly not defined for this CPU")
> - sources = openh264_processing_sources_asm_x86
> - if (target_cpu == "x86") {
> - defines = [ "X86_32" ]
> - } else { # x64
> - if (is_mac) {
> - defines = [
> - "PREFIX",
> - "UNIX64",
> - ]
> - } else if (is_win) {
> - defines = [ "WIN64" ]
> - } else if (is_linux) {
> - defines = [ "UNIX64" ]
> - }
> - }
> - }
> -
> - yasm_assemble("openh264_encoder_yasm") {
> - include_dirs = openh264_encoder_include_dirs
> - include_dirs += [ "./src/codec/common/x86" ]
> - assert(target_cpu == "x86" || target_cpu == "x64",
> - "Assembly not defined for this CPU")
> - sources = openh264_encoder_sources_asm_x86
> - if (target_cpu == "x86") {
> - defines = [ "X86_32" ]
> - } else { # x64
> - if (is_mac) {
> - defines = [
> - "PREFIX",
> - "UNIX64",
> - ]
> - } else if (is_win) {
> - defines = [ "WIN64" ]
> - } else if (is_linux) {
> - defines = [ "UNIX64" ]
> - }
> - }
> - }
> -} # if (is_win || is_linux)
> -
> source_set("common") {
> sources = openh264_common_sources
> include_dirs = openh264_common_include_dirs
> @@ -110,10 +36,6 @@
> configs += [ "//build/config/compiler:no_chromium_code" ]
> configs += [ ":config" ]
> deps = []
> - if ((is_win || is_linux) && (target_cpu == "x86" || target_cpu ==
> "x64")) {
> - defines = [ "X86_ASM" ]
> - deps += [ ":openh264_common_yasm" ]
> - }
> if (is_android) {
> deps += [
> # Defines "android_get/setCpu..." functions. The original OpenH264 build
> @@ -135,10 +57,6 @@
> deps = [
> ":common",
> ]
> - if ((is_win || is_linux) && (target_cpu == "x86" || target_cpu ==
> "x64")) {
> - defines = [ "X86_ASM" ]
> - deps += [ ":openh264_processing_yasm" ]
> - }
> }
>
> source_set("encoder") {
> @@ -158,8 +76,4 @@
> ":common",
> ":processing",
> ]
> - if ((is_win || is_linux) && (target_cpu == "x86" || target_cpu ==
> "x64")) {
> - defines = [ "X86_ASM" ]
> - deps += [ ":openh264_encoder_yasm" ]
> - }
> }
>
>
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[chromium-reviews, 2017-01-23 06:41:10]

Found the answer - it's ARM linux, a combination not on trybots. Know how
to fix.


On Mon, Jan 23, 2017 at 7:39 AM, Harald Alvestrand <hta@google.com> wrote:

> Haraken, can you tell me what the target_cpu value was for this builder?
>
> Is there a trybot for this config that is not in the standard trybot set,
> so that I can try it?
>
>
> On Mon, Jan 23, 2017 at 1:31 AM, <haraken@chromium.org> wrote:
>
>> Reviewers: kthelgason_chromium, ehmaldonado_chromium, kthelgason, hta -
>> Chromium
>> CL: https://codereview.chromium.org/2651543002/
>>
>> Message:
>> Created Revert of Add assembly for x86 to OpenH264 encoder
>>
>> Description:
>> Revert of Add assembly for x86 to OpenH264 encoder (patchset #5 id:80001
>> of
>> https://codereview.chromium.org/2585733002/ )
>>
>> Reason for revert:
>> This broke Chrome OS builders:
>>
>> https://uberchromegw.corp.google.com/i/chromeos/builders/
>> veyron_minnie-chrome-pfq/builds/669/steps/BuildPackages/logs/stdio
>>
>>
>> Original issue's description:
>> > Add assembly for x86 to OpenH264 encoder
>> >
>> > On Linux on a build workstation, these changes seem to give around 2.5x
>> faster
>> encoding (frame encode time in test goes from ~25 to ~9 ms)
>> >
>> > We do expect alerts from our performance bots when this lands.
>> >
>> > Credit: These build changes were written by brian@highfive.com aka
>> bbaldino@gmail.com.
>> >
>> > BUG=600399
>> >
>> > Review-Url: https://codereview.chromium.org/2585733002
>> > Cr-Commit-Position: refs/heads/master@{#445033}
>> > Committed:
>> https://chromium.googlesource.com/chromium/src/+/6d1501f5000
>> 99ff3e25f92593e9efcb27d352252
>>
>> TBR=kthelgason@chromium.org,ehmaldonado@chromium.org,kthelga
>> son@webrtc.org,hta@chromium.org
>> # Not skipping CQ checks because original CL landed more than 1 days ago.
>> BUG=600399
>>
>> Affected files (+0, -86 lines):
>> M third_party/openh264/BUILD.gn
>>
>>
>> Index: third_party/openh264/BUILD.gn
>> diff --git a/third_party/openh264/BUILD.gn b/third_party/openh264/BUILD.g
>> n
>> index
dfefac587585017937c962e544e5c393a9d3ca71..4301abd65e1ad83b9156bd46b2d9b750270be0b8
>> 100644
>> --- a/third_party/openh264/BUILD.gn
>> +++ b/third_party/openh264/BUILD.gn
>> @@ -4,7 +4,6 @@
>>
>> 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") {
>> @@ -29,79 +28,6 @@
>> }
>> }
>>
>> -# YASM assembly is only checked to be working on Windows and Linux.
>> -# Mac is known to fail certain tests when building, but actual assembly
>> -# is believed to work.
>> -#
>> -# This IF statement will make the targets visible only on specific
>> builds,
>> -# which will lead to failures on other platforms if accidentally invoked.
>> -if (is_win || is_linux) {
>> - yasm_assemble("openh264_common_yasm") {
>> - include_dirs = openh264_common_include_dirs
>> - assert(target_cpu == "x86" || target_cpu == "x64",
>> - "Assembly not defined for this CPU")
>> - sources = openh264_common_sources_asm_x86
>> - if (target_cpu == "x86") {
>> - defines = [ "X86_32" ]
>> - } else { # x64
>> - if (is_mac) {
>> - defines = [
>> - "PREFIX",
>> - "UNIX64",
>> - ]
>> - } else if (is_win) {
>> - defines = [ "WIN64" ]
>> - } else if (is_linux) {
>> - defines = [ "UNIX64" ]
>> - }
>> - }
>> - }
>> -
>> - yasm_assemble("openh264_processing_yasm") {
>> - include_dirs = openh264_processing_include_dirs
>> - include_dirs += [ "./src/codec/common/x86" ]
>> - assert(target_cpu == "x86" || target_cpu == "x64",
>> - "Assembly not defined for this CPU")
>> - sources = openh264_processing_sources_asm_x86
>> - if (target_cpu == "x86") {
>> - defines = [ "X86_32" ]
>> - } else { # x64
>> - if (is_mac) {
>> - defines = [
>> - "PREFIX",
>> - "UNIX64",
>> - ]
>> - } else if (is_win) {
>> - defines = [ "WIN64" ]
>> - } else if (is_linux) {
>> - defines = [ "UNIX64" ]
>> - }
>> - }
>> - }
>> -
>> - yasm_assemble("openh264_encoder_yasm") {
>> - include_dirs = openh264_encoder_include_dirs
>> - include_dirs += [ "./src/codec/common/x86" ]
>> - assert(target_cpu == "x86" || target_cpu == "x64",
>> - "Assembly not defined for this CPU")
>> - sources = openh264_encoder_sources_asm_x86
>> - if (target_cpu == "x86") {
>> - defines = [ "X86_32" ]
>> - } else { # x64
>> - if (is_mac) {
>> - defines = [
>> - "PREFIX",
>> - "UNIX64",
>> - ]
>> - } else if (is_win) {
>> - defines = [ "WIN64" ]
>> - } else if (is_linux) {
>> - defines = [ "UNIX64" ]
>> - }
>> - }
>> - }
>> -} # if (is_win || is_linux)
>> -
>> source_set("common") {
>> sources = openh264_common_sources
>> include_dirs = openh264_common_include_dirs
>> @@ -110,10 +36,6 @@
>> configs += [ "//build/config/compiler:no_chromium_code" ]
>> configs += [ ":config" ]
>> deps = []
>> - if ((is_win || is_linux) && (target_cpu == "x86" || target_cpu ==
>> "x64")) {
>> - defines = [ "X86_ASM" ]
>> - deps += [ ":openh264_common_yasm" ]
>> - }
>> if (is_android) {
>> deps += [
>> # Defines "android_get/setCpu..." functions. The original OpenH264 build
>> @@ -135,10 +57,6 @@
>> deps = [
>> ":common",
>> ]
>> - if ((is_win || is_linux) && (target_cpu == "x86" || target_cpu ==
>> "x64")) {
>> - defines = [ "X86_ASM" ]
>> - deps += [ ":openh264_processing_yasm" ]
>> - }
>> }
>>
>> source_set("encoder") {
>> @@ -158,8 +76,4 @@
>> ":common",
>> ":processing",
>> ]
>> - if ((is_win || is_linux) && (target_cpu == "x86" || target_cpu ==
>> "x64")) {
>> - defines = [ "X86_ASM" ]
>> - deps += [ ":openh264_encoder_yasm" ]
>> - }
>> }
>>
>>
>>
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[kjellander_chromium, 2017-01-23 07:28:25]

kjellander@chromium.org changed reviewers:
+ kjellander@chromium.org

[kjellander_chromium, 2017-01-23 07:28:25]

Thanks for the revert. I added another comment since the change also broke
rolling WebRTC DEPS (so this revert was good for us too, thanks).

https://codereview.chromium.org/2651543002/diff/1/third_party/openh264/BUILD.gn
File third_party/openh264/BUILD.gn (left):

https://codereview.chromium.org/2651543002/diff/1/third_party/openh264/BUILD....
third_party/openh264/BUILD.gn:41: assert(target_cpu == "x86" || target_cpu ==
"x64",
This assertion fails for the linux_arm bot when WebRTC tries to roll in this
change from Chromium:

ERROR at //third_party/openh264/BUILD.gn:41:5: Assertion failed.
    assert(target_cpu == "x86" || target_cpu == "x64",
    ^-----
Assembly not defined for this CPU
See //webrtc/modules/video_coding/BUILD.gn:188:7: which caused the file to be
included.
      "//third_party/openh264:encoder",
      ^-------------------------------


So we must find a way to not assert hard on x86 or x64 if we're going to
continue supporting ARM for Linux.

[hta - Chromium, 2017-01-23 08:12:07]

A revert of this CL (patchset #1 id:1) has been created in
https://codereview.chromium.org/2645293002/ by hta@chromium.org.

The reason for reverting is: Reland.

[Wez, 2017-02-08 02:16:25]

Description was changed from

==========
Revert of Add assembly for x86 to OpenH264 encoder (patchset #5 id:80001 of
https://codereview.chromium.org/2585733002/ )

Reason for revert:
This broke Chrome OS builders:

https://uberchromegw.corp.google.com/i/chromeos/builders/veyron_minnie-chrome...


Original issue's description:
> Add assembly for x86 to OpenH264 encoder
> 
> On Linux on a build workstation, these changes seem to give around 2.5x faster
encoding (frame encode time in test goes from ~25 to ~9 ms)
> 
> We do expect alerts from our performance bots when this lands.
> 
> Credit: These build changes were written by brian@highfive.com aka
bbaldino@gmail.com.
> 
> BUG=600399
> 
> Review-Url: https://codereview.chromium.org/2585733002
> Cr-Commit-Position: refs/heads/master@{#445033}
> Committed:
https://chromium.googlesource.com/chromium/src/+/6d1501f500099ff3e25f92593e9e...

TBR=kthelgason@chromium.org,ehmaldonado@chromium.org,kthelgason@webrtc.org,ht...
# Not skipping CQ checks because original CL landed more than 1 days ago.
BUG=600399

Review-Url: https://codereview.chromium.org/2651543002
Cr-Commit-Position: refs/heads/master@{#445311}
Committed:
https://chromium.googlesource.com/chromium/src/+/3f4ee1b009741853987773717ebc...
==========

to

==========
Revert of Add assembly for x86 to OpenH264 encoder (patchset #5 id:80001 of
https://codereview.chromium.org/2585733002/ )

Reason for revert:
This broke Chrome OS builders:

https://uberchromegw.corp.google.com/i/chromeos/builders/veyron_minnie-chrome...


Original issue's description:
> Add assembly for x86 to OpenH264 encoder
> 
> On Linux on a build workstation, these changes seem to give around 2.5x faster
encoding (frame encode time in test goes from ~25 to ~9 ms)
> 
> We do expect alerts from our performance bots when this lands.
> 
> Credit: These build changes were written by brian@highfive.com aka
bbaldino@gmail.com.
> 
> BUG=600399
> 
> Review-Url: https://codereview.chromium.org/2585733002
> Cr-Commit-Position: refs/heads/master@{#445033}
> Committed:
https://chromium.googlesource.com/chromium/src/+/6d1501f500099ff3e25f92593e9e...

TBR=kthelgason@chromium.org,ehmaldonado@chromium.org,kthelgason@webrtc.org,ht...
# Not skipping CQ checks because original CL landed more than 1 days ago.
BUG=600399

Review-Url: https://codereview.chromium.org/2651543002
Cr-Commit-Position: refs/heads/master@{#445311}
Committed:
https://chromium.googlesource.com/chromium/src/+/3f4ee1b009741853987773717ebc...
==========

[Wez, 2017-02-08 02:16:25]

wez@chromium.org changed reviewers:
+ mflodman@chromium.org