|
|
Created:
4 years ago by hta - Chromium Modified:
3 years, 10 months ago CC:
chromium-reviews, brian4 Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd 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
Patch Set 1 #
Total comments: 2
Patch Set 2 : Rebased to asm-only portions #Patch Set 3 : Limit assembly to Windows #Patch Set 4 : Enabled assembly on Linux #
Total comments: 6
Patch Set 5 : Enable Linux for real this time #Messages
Total messages: 52 (38 generated)
The CQ bit was checked by hta@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
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 ==========
The CQ bit was checked by hta@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
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.org changed reviewers: + kthelgason@chromium.org
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?
The CQ bit was checked by hta@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
The CQ bit was checked by hta@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
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 ==========
The CQ bit was checked by hta@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
hta@chromium.org changed reviewers: + ehmaldonado@chromium.org
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")
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
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.
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@webrtc.org changed reviewers: + kthelgason@webrtc.org
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?
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 ==========
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.
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...)
The CQ bit was checked by hta@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
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 ==========
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 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
This lgtm. Is there any plan going forward for enabling this on the mac as well?
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.
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.
The CQ bit was checked by hta@chromium.org
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")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch. Bot data: {"patchset_id": 80001, "attempt_start_ts": 1484904625245410, "parent_rev": "135e4b7a3bd92d8fd4e18315528284655c9e0191", "commit_rev": "6d1501f500099ff3e25f92593e9efcb27d352252"}
Message was sent while issue was closed.
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... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/6d1501f500099ff3e25f92593e9e...
Message was sent while issue was closed.
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... .
Message was sent while issue was closed.
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... ==========
Message was sent while issue was closed.
wez@chromium.org changed reviewers: + mflodman@chromium.org |