Add assembly for x86 to OpenH264 encoder

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

[hta - Chromium, 2016-12-16 10:43:58]

The CQ bit was checked by hta@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-16 10:44:13]

Dry run: 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, 2016-12-16 11:13:38]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-16 11:13:39]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[hta - Chromium, 2016-12-16 11:37:15]

Description was changed from

==========
Add assembly for ARM and x86 to OpenH264 encoder

Credit: These build changes were written by brian@highfive.com.

BUG=600399
==========

to

==========
Add assembly for ARM and x86 to OpenH264 encoder

Credit: These build changes were written by brian@highfive.com aka
bbaldino@gmail.com.

BUG=600399
==========

[hta - Chromium, 2017-01-10 08:13:58]

The CQ bit was checked by hta@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-10 08:14:08]

Dry run: 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-10 08:37:58]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-10 08:37:58]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[kjellander_chromium, 2017-01-10 09:43:55]

On 2017/01/10 08:37:58, commit-bot: I haz the power wrote:
> Dry run: Try jobs failed on following builders:
>   mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

While waiting for the Mac compile error and the Linux test failure to be
resolved; could we get the refactoring into
	third_party/openh264/openh264_sources.gni landed in a separate CL, so it's
easier to see what sources are actually added to the build in this CL?

[kthelgason_chromium, 2017-01-10 13:43:56]

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

[kthelgason_chromium, 2017-01-10 13:43:56]

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

https://codereview.chromium.org/2585733002/diff/1/third_party/openh264/BUILD....
third_party/openh264/BUILD.gn:61: if (target_os == "mac") {
I believe it's conventional to use the helpers `is_mac` and `is_win` etc.

https://codereview.chromium.org/2585733002/diff/1/third_party/openh264/BUILD....
third_party/openh264/BUILD.gn:67: defines = [ "WIN64" ]
Couldn't this also be linux or iOS simulator?

[hta - Chromium, 2017-01-16 12:32:22]

The CQ bit was checked by hta@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-16 12:32:33]

Dry run: 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-16 12:59:44]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-16 12:59:45]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[hta - Chromium, 2017-01-16 14:49:10]

The CQ bit was checked by hta@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-16 14:49:26]

Dry run: 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-16 15:42:01]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-16 15:42:02]

Dry run: This issue passed the CQ dry run.

[hta - Chromium, 2017-01-17 13:36:45]

Description was changed from

==========
Add assembly for ARM and x86 to OpenH264 encoder

Credit: These build changes were written by brian@highfive.com aka
bbaldino@gmail.com.

BUG=600399
==========

to

==========
Add assembly for ARM and x86 to OpenH264 encoder

On Linux on a build workstation, these changes seem to give around 30% faster
encoding.

Credit: These build changes were written by brian@highfive.com aka
bbaldino@gmail.com.

BUG=600399
==========

[hta - Chromium, 2017-01-17 13:36:53]

The CQ bit was checked by hta@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-17 13:37:04]

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

[hta - Chromium, 2017-01-17 13:59:55]

hta@chromium.org changed reviewers:
+ ehmaldonado@chromium.org

[ehmaldonado_chromium, 2017-01-17 14:17:34]

LGTM
I see no obvious mistakes

https://codereview.chromium.org/2585733002/diff/60001/third_party/openh264/BU...
File third_party/openh264/BUILD.gn (right):

https://codereview.chromium.org/2585733002/diff/60001/third_party/openh264/BU...
third_party/openh264/BUILD.gn:35: if (is_win || is_linux) {
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.

https://codereview.chromium.org/2585733002/diff/60001/third_party/openh264/BU...
third_party/openh264/BUILD.gn:82: assert(false, "Assembly not defined for this
CPU")
What if you move this above and make it something like:
assert(target_cpu == "x86" || target_cpu == "x64", "Assembly not defined for
this CPU")

[commit-bot: I haz the power, 2017-01-17 14:29:54]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-17 14:29:55]

Dry run: This issue passed the CQ dry run.

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

Thanks, questions!

https://codereview.chromium.org/2585733002/diff/60001/third_party/openh264/BU...
File third_party/openh264/BUILD.gn (right):

https://codereview.chromium.org/2585733002/diff/60001/third_party/openh264/BU...
third_party/openh264/BUILD.gn:35: if (is_win || is_linux) {
On 2017/01/17 14:17:33, ehmaldonado_chromium wrote:
> 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.

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?

https://codereview.chromium.org/2585733002/diff/60001/third_party/openh264/BU...
third_party/openh264/BUILD.gn:82: assert(false, "Assembly not defined for this
CPU")
On 2017/01/17 14:17:33, ehmaldonado_chromium wrote:
> What if you move this above and make it something like:
> assert(target_cpu == "x86" || target_cpu == "x64", "Assembly not defined for
> this CPU")

Sounds better. Will do.

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

https://codereview.chromium.org/2585733002/diff/60001/third_party/openh264/BU...
File third_party/openh264/BUILD.gn (right):

https://codereview.chromium.org/2585733002/diff/60001/third_party/openh264/BU...
third_party/openh264/BUILD.gn:35: if (is_win || is_linux) {
On 2017/01/17 14:34:45, hta - Chromium wrote:
> On 2017/01/17 14:17:33, ehmaldonado_chromium wrote:
> > 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.
> 
> 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?

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

[kthelgason, 2017-01-18 08:10:32]

kthelgason@webrtc.org changed reviewers:
+ kthelgason@webrtc.org

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

Perhaps you should edit the description, as there are no ARM targets currently
defined.

https://codereview.chromium.org/2585733002/diff/60001/third_party/openh264/BU...
File third_party/openh264/BUILD.gn (right):

https://codereview.chromium.org/2585733002/diff/60001/third_party/openh264/BU...
third_party/openh264/BUILD.gn:121: if (is_win && (target_cpu == "x86" ||
target_cpu == "x64")) {
Targets are defined for windows and linux, but only included on windows. Is this
intentional?

[hta - Chromium, 2017-01-18 08:26:29]

Description was changed from

==========
Add assembly for ARM and x86 to OpenH264 encoder

On Linux on a build workstation, these changes seem to give around 30% faster
encoding.

Credit: These build changes were written by brian@highfive.com aka
bbaldino@gmail.com.

BUG=600399
==========

to

==========
Add assembly for x86 to OpenH264 encoder

On Linux on a build workstation, these changes seem to give around 30% faster
encoding.

Credit: These build changes were written by brian@highfive.com aka
bbaldino@gmail.com.

BUG=600399
==========

[hta - Chromium, 2017-01-18 08:27:32]

On 2017/01/18 08:10:33, kthelgason wrote:
> Perhaps you should edit the description, as there are no ARM targets currently
> defined.

Oops. Done.
(Original CL included ARM files, but not ARM targets.)

> 
>
https://codereview.chromium.org/2585733002/diff/60001/third_party/openh264/BU...
> File third_party/openh264/BUILD.gn (right):
> 
>
https://codereview.chromium.org/2585733002/diff/60001/third_party/openh264/BU...
> third_party/openh264/BUILD.gn:121: if (is_win && (target_cpu == "x86" ||
> target_cpu == "x64")) {
> Targets are defined for windows and linux, but only included on windows. Is
this
> intentional?

Fixed in patch set 4.

[hta - Chromium, 2017-01-18 08:28:37]

On 2017/01/18 08:27:32, hta - Chromium wrote:
> On 2017/01/18 08:10:33, kthelgason wrote:
> > Perhaps you should edit the description, as there are no ARM targets
currently
> > defined.
> 
> Oops. Done.
> (Original CL included ARM files, but not ARM targets.)
> 
> > 
> >
>
https://codereview.chromium.org/2585733002/diff/60001/third_party/openh264/BU...
> > File third_party/openh264/BUILD.gn (right):
> > 
> >
>
https://codereview.chromium.org/2585733002/diff/60001/third_party/openh264/BU...
> > third_party/openh264/BUILD.gn:121: if (is_win && (target_cpu == "x86" ||
> > target_cpu == "x64")) {
> > Targets are defined for windows and linux, but only included on windows. Is
> this
> > intentional?
> 
> Fixed in patch set 4.

No, it's not. Will fix. (Speaks to variance of results... I got 30% difference
in 2 runs while not changing the code...)

[hta - Chromium, 2017-01-18 09:43:18]

The CQ bit was checked by hta@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-18 09:43:43]

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

[hta - Chromium, 2017-01-18 09:44:31]

Description was changed from

==========
Add assembly for x86 to OpenH264 encoder

On Linux on a build workstation, these changes seem to give around 30% faster
encoding.

Credit: These build changes were written by brian@highfive.com aka
bbaldino@gmail.com.

BUG=600399
==========

to

==========
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)

Credit: These build changes were written by brian@highfive.com aka
bbaldino@gmail.com.

BUG=600399
==========

[hta - Chromium, 2017-01-18 10:23:35]

Description was changed from

==========
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)

Credit: These build changes were written by brian@highfive.com aka
bbaldino@gmail.com.

BUG=600399
==========

to

==========
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
==========

[commit-bot: I haz the power, 2017-01-18 10:49:02]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-18 10:49:03]

Dry run: This issue passed the CQ dry run.

[kthelgason_chromium, 2017-01-19 09:41:10]

This lgtm. Is there any plan going forward for enabling this on the mac as well?

[hta - Chromium, 2017-01-19 09:53:16]

On 2017/01/19 09:41:10, kthelgason_chromium wrote:
> This lgtm. Is there any plan going forward for enabling this on the mac as
well?

I plan to file a bug and see if anyone picks it up.... it seems that 97% of our
Mac H.264 usage is with hardware encoder, so the target market seems small.

We also have assembly on ARM to enable - if we can figure out if we need it.

[kthelgason_chromium, 2017-01-19 10:26:15]

On 2017/01/19 09:53:16, hta - Chromium wrote:
> On 2017/01/19 09:41:10, kthelgason_chromium wrote:
> > This lgtm. Is there any plan going forward for enabling this on the mac as
> well?
> 
> I plan to file a bug and see if anyone picks it up.... it seems that 97% of
our
> Mac H.264 usage is with hardware encoder, so the target market seems small.
> 
> We also have assembly on ARM to enable - if we can figure out if we need it.

Sounds good. IMO we should focus on hardware on ARM where available. Software
encode/decode and low-power platforms don't mix well.

[hta - Chromium, 2017-01-20 09:30:25]

The CQ bit was checked by hta@chromium.org

[hta - Chromium, 2017-01-20 09:30:25]

The patchset sent to the CQ was uploaded after l-g-t-m from
ehmaldonado@chromium.org
Link to the patchset: https://codereview.chromium.org/2585733002/#ps80001
(title: "Enable Linux for real this time")

[commit-bot: I haz the power, 2017-01-20 09:30:41]

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-20 10:31:57]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1484904625245410,
"parent_rev": "135e4b7a3bd92d8fd4e18315528284655c9e0191", "commit_rev":
"6d1501f500099ff3e25f92593e9efcb27d352252"}

[commit-bot: I haz the power, 2017-01-20 10:32:43]

Description was changed from

==========
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
==========

to

==========
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...
==========

[commit-bot: I haz the power, 2017-01-20 10:32:47]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/6d1501f500099ff3e25f92593e9e...

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

A revert of this CL (patchset #5 id:80001) has been created in
https://codereview.chromium.org/2651543002/ by haraken@chromium.org.

The reason for reverting is: This broke Chrome OS builders:

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

[Wez, 2017-02-08 02:17:20]

Description was changed from

==========
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...
==========

to

==========
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...
==========

[Wez, 2017-02-08 02:17:20]

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