|
|
Created:
6 years, 10 months ago by tlegrand1 Modified:
6 years, 9 months ago CC:
chromium-reviews Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionEnabling ARM optimizations for Opus on Android and ChromeOS
This CL turns on the ARM optimizations for the Opus codec on Android and ChromeOS. It gives roughly a 40% CPU reduction on Android, and probably the same on Chrome OS (don't have any numbers there to compare with).
BUG=296844
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=258909
Patch Set 1 #
Total comments: 11
Patch Set 2 : Addressing comments #
Total comments: 3
Patch Set 3 : Enable for ChromeOS #Patch Set 4 : Rename file #Patch Set 5 : Changed conditions #
Total comments: 2
Patch Set 6 : Use float unless on ARM #
Total comments: 2
Patch Set 7 : Adding back two comments #Patch Set 8 : Renaming #
Total comments: 2
Messages
Total messages: 34 (0 generated)
Hi Sergey, Thi CL enables the ARM optimizations in the new Opus 1.1. Please help reviewing.
There is nothing android-specific in this CL. So maybe remove 'Android' from the description and title? E.g. this should also benefit ARM-based Chromebooks. https://codereview.chromium.org/150103006/diff/1/third_party/opus/opus.gyp File third_party/opus/opus.gyp (right): https://codereview.chromium.org/150103006/diff/1/third_party/opus/opus.gyp#ne... third_party/opus/opus.gyp:78: 'OPUS_ARM_MAY_HAVE_NEON', Sort these alphabetically please https://codereview.chromium.org/150103006/diff/1/third_party/opus/opus.gyp#ne... third_party/opus/opus.gyp:79: ], # defines nit: two spaces before comments. Also I don't think this comment and similar comments below improve readability, so may be remove them completely. They are not used in the rest of this file (but comments at the end of conditions and targets are still useful). https://codereview.chromium.org/150103006/diff/1/third_party/opus/opus.gyp#ne... third_party/opus/opus.gyp:89: '-mfpu=neon', Why do you need this? I think build/common.gypi should add this flag automatically when arm_neon=1. https://codereview.chromium.org/150103006/diff/1/third_party/opus/opus.gyp#ne... third_party/opus/opus.gyp:99: ], # sources sources!, but I suggest to just remove this comment. https://codereview.chromium.org/150103006/diff/1/third_party/opus/opus_arm_sr... File third_party/opus/opus_arm_srcs.gypi (right): https://codereview.chromium.org/150103006/diff/1/third_party/opus/opus_arm_sr... third_party/opus/opus_arm_srcs.gypi:1: # Copyright 2014 The Chromium Authors. All rights reserved. call this file opus_srcs_arm.gypi, for consistency with third_party/libvpx
Thanks for your great comments! I will upload a new patch set, but please wait with second round of review until I have removed the -mfpu=neon. I need to verify my changes. https://codereview.chromium.org/150103006/diff/1/third_party/opus/opus.gyp File third_party/opus/opus.gyp (right): https://codereview.chromium.org/150103006/diff/1/third_party/opus/opus.gyp#ne... third_party/opus/opus.gyp:8: ['OS=="android"', { We only enable the optimizations for android at the moment. https://codereview.chromium.org/150103006/diff/1/third_party/opus/opus.gyp#ne... third_party/opus/opus.gyp:78: 'OPUS_ARM_MAY_HAVE_NEON', On 2014/02/06 18:23:13, Sergey Ulanov wrote: > Sort these alphabetically please Done. https://codereview.chromium.org/150103006/diff/1/third_party/opus/opus.gyp#ne... third_party/opus/opus.gyp:79: ], # defines On 2014/02/06 18:23:13, Sergey Ulanov wrote: > nit: two spaces before comments. Also I don't think this comment and similar > comments below improve readability, so may be remove them completely. They are > not used in the rest of this file (but comments at the end of conditions and > targets are still useful). Done. https://codereview.chromium.org/150103006/diff/1/third_party/opus/opus.gyp#ne... third_party/opus/opus.gyp:89: '-mfpu=neon', I think you are right, but I need to verify that it is safe to remove this for WebRTC too. On 2014/02/06 18:23:13, Sergey Ulanov wrote: > Why do you need this? I think build/common.gypi should add this flag > automatically when arm_neon=1. https://codereview.chromium.org/150103006/diff/1/third_party/opus/opus.gyp#ne... third_party/opus/opus.gyp:99: ], # sources On 2014/02/06 18:23:13, Sergey Ulanov wrote: > sources!, but I suggest to just remove this comment. Done. https://codereview.chromium.org/150103006/diff/1/third_party/opus/opus_arm_sr... File third_party/opus/opus_arm_srcs.gypi (right): https://codereview.chromium.org/150103006/diff/1/third_party/opus/opus_arm_sr... third_party/opus/opus_arm_srcs.gypi:1: # Copyright 2014 The Chromium Authors. All rights reserved. On 2014/02/06 18:23:13, Sergey Ulanov wrote: > call this file opus_srcs_arm.gypi, for consistency with third_party/libvpx Done.
Hi Sergey, I realized I needed to land another CL before this one (in your inbox). At the moment we are only enabling the ARM opts for Android, because of a Clang compiler issue on iOS. Maybe we should consider enabling for all platforms except iOS instead? https://codereview.chromium.org/150103006/diff/80001/third_party/opus/opus.gyp File third_party/opus/opus.gyp (right): https://codereview.chromium.org/150103006/diff/80001/third_party/opus/opus.gy... third_party/opus/opus.gyp:10: 'opus_arm_optimization%': 1, As you see we only enable for Android at the moment.
On 2014/02/11 15:45:07, tlegrand1 wrote: > Hi Sergey, > > I realized I needed to land another CL before this one (in your inbox). > > At the moment we are only enabling the ARM opts for Android, because of a Clang > compiler issue on iOS. But what about ChromeOS? > Maybe we should consider enabling for all platforms except iOS instead? What is Opus used for on iOS? I don't have objections for disabling optimization on iOS. https://codereview.chromium.org/150103006/diff/80001/third_party/opus/opus.gyp File third_party/opus/opus.gyp (right): https://codereview.chromium.org/150103006/diff/80001/third_party/opus/opus.gy... third_party/opus/opus.gyp:10: 'opus_arm_optimization%': 1, On 2014/02/11 15:45:07, tlegrand1 wrote: > As you see we only enable for Android at the moment. Android is now always ARM (e.g. there are Intel-based Android devices), and ARM not always Android. So this doesn't look right to me. Should we look for target_arch=="arm" instead?
Thanks for you input Sergey! I realize my knowledge is too limited, and I need to consult my colleague who actually started this work. So, I will pause the work on this CL until next week. 14/02/13 20:34:28, Sergey Ulanov wrote: > On 2014/02/11 15:45:07, tlegrand1 wrote: > > Hi Sergey, > > > > I realized I needed to land another CL before this one (in your inbox). > > > > At the moment we are only enabling the ARM opts for Android, because of a > Clang > > compiler issue on iOS. > > But what about ChromeOS? > > > > Maybe we should consider enabling for all platforms except iOS instead? > > What is Opus used for on iOS? I don't have objections for disabling optimization > on iOS. > > https://codereview.chromium.org/150103006/diff/80001/third_party/opus/opus.gyp > File third_party/opus/opus.gyp (right): > > https://codereview.chromium.org/150103006/diff/80001/third_party/opus/opus.gy... > third_party/opus/opus.gyp:10: 'opus_arm_optimization%': 1, > On 2014/02/11 15:45:07, tlegrand1 wrote: > > As you see we only enable for Android at the moment. > > Android is now always ARM (e.g. there are Intel-based Android devices), and ARM > not always Android. So this doesn't look right to me. Should we look for > target_arch=="arm" instead?
https://codereview.chromium.org/150103006/diff/80001/third_party/opus/opus.gyp File third_party/opus/opus.gyp (right): https://codereview.chromium.org/150103006/diff/80001/third_party/opus/opus.gy... third_party/opus/opus.gyp:87: ['(target_arch=="arm" and arm_version==7) or target_arch=="armv7"', { We may try to change this line to ['target_arch=="arm" and arm_fpu=="neon"'], and remove 88 'cflags': [ 89 '-mfpu=neon', 90 ], 91 'cflags/': [ 92 ['exclude', '^-mfpu=.*'], 93 ['include', '-mfpu=neon'], 94 ],
Hi Sergey, The CL is ready for a new round of review. The optimizations will be enabled on Android and ChromeOS, as you suggested. I've updated the description accordingly. Thanks!
Hi Sergey, Do you have time to review this CL? I'd really like to get it in this week, because we have a couple of changes in WebRTC depending on this one. On 2014/03/14 09:35:57, tlegrand1 wrote: > Hi Sergey, > > The CL is ready for a new round of review. The optimizations will be enabled on > Android and ChromeOS, as you suggested. I've updated the description > accordingly. > Thanks!
https://codereview.chromium.org/150103006/diff/190001/third_party/opus/opus.gyp File third_party/opus/opus.gyp (right): https://codereview.chromium.org/150103006/diff/190001/third_party/opus/opus.g... third_party/opus/opus.gyp:9: 'use_opus_fixed_point%': 1, This will enable fixed-point math on Intel Chromebooks. It doesn't look correct to me. If fixed-point is faster on Intel CPUs then we should always use it, not only on ChromeOS. I _think_ we need just one condition that sets both use_opus_fixed_point and use_opus_arm_optimization on ARM builds. Or is it that you need fixed-point on Intel Android builds.
https://codereview.chromium.org/150103006/diff/190001/third_party/opus/opus.gyp File third_party/opus/opus.gyp (right): https://codereview.chromium.org/150103006/diff/190001/third_party/opus/opus.g... third_party/opus/opus.gyp:9: 'use_opus_fixed_point%': 1, I wanted to preserve the previous setting for Android, where we always used fixed point. But I think you are right, we don't need fixed point in Intel CPUs. Now is the best time to make the change. On 2014/03/19 19:21:51, Sergey Ulanov wrote: > This will enable fixed-point math on Intel Chromebooks. It doesn't look correct > to me. If fixed-point is faster on Intel CPUs then we should always use it, not > only on ChromeOS. I _think_ we need just one condition that sets both > use_opus_fixed_point and use_opus_arm_optimization on ARM builds. Or is it that > you need fixed-point on Intel Android builds.
New patch set is available, where I only enable fixed point if we are on ARM. Now we have two parameters that are always equal to each other, 'use_opus_fixed_point' and 'use_opus_arm_optimization'. I'd like to keep both of them for now, in case we later want to enable fixed point on all Android devices, or if we in the future have optimizations for other platforms. Let me know if you disagree. And thank you for helping out reviewing! On 2014/03/20 10:10:56, tlegrand1 wrote: > https://codereview.chromium.org/150103006/diff/190001/third_party/opus/opus.gyp > File third_party/opus/opus.gyp (right): > > https://codereview.chromium.org/150103006/diff/190001/third_party/opus/opus.g... > third_party/opus/opus.gyp:9: 'use_opus_fixed_point%': 1, > I wanted to preserve the previous setting for Android, where we always used > fixed point. But I think you are right, we don't need fixed point in Intel CPUs. > Now is the best time to make the change. > > On 2014/03/19 19:21:51, Sergey Ulanov wrote: > > This will enable fixed-point math on Intel Chromebooks. It doesn't look > correct > > to me. If fixed-point is faster on Intel CPUs then we should always use it, > not > > only on ChromeOS. I _think_ we need just one condition that sets both > > use_opus_fixed_point and use_opus_arm_optimization on ARM builds. Or is it > that > > you need fixed-point on Intel Android builds.
lgtm https://codereview.chromium.org/150103006/diff/210001/third_party/opus/opus.gyp File third_party/opus/opus.gyp (left): https://codereview.chromium.org/150103006/diff/210001/third_party/opus/opus.g... third_party/opus/opus.gyp:70: }, # target opus Keep this comment? https://codereview.chromium.org/150103006/diff/210001/third_party/opus/opus.g... third_party/opus/opus.gyp:98: }, # target opus_demo and this one
The CQ bit was checked by tlegrand@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/tlegrand@chromium.org/150103006/230001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.chromium on linux_chromium_clang_dbg
Sergey, I added back the comments you asked about, and fixed the name of opus_srcs_arm.gypi in opus.gyp. I messed up the name on the file in one of the Patch Sets, and managed to rename in one place but not the other...
The CQ bit was checked by tlegrand@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/tlegrand@chromium.org/150103006/250001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.chromium on mac_chromium_rel
The CQ bit was checked by tlegrand@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/tlegrand@chromium.org/150103006/250001
Message was sent while issue was closed.
Change committed as 258909
Message was sent while issue was closed.
Post-commit drive-by https://codereview.chromium.org/150103006/diff/250001/third_party/opus/opus.gyp File third_party/opus/opus.gyp (right): https://codereview.chromium.org/150103006/diff/250001/third_party/opus/opus.g... third_party/opus/opus.gyp:8: ['(OS=="android" or chromeos==1) and target_arch=="arm"', { Why not OS=="ios" (which would also likely mean target_arch=="armv7")? https://codereview.chromium.org/150103006/diff/250001/third_party/opus/opus.g... third_party/opus/opus.gyp:78: 'OPUS_ARM_MAY_HAVE_NEON', Does the run-time check this turns into https://code.google.com/p/chromium/codesearch#chromium/src/third_party/opus/s... really work inside the chrome renderer sandbox? If so that could be a boon to a few other projects that have struggled with this recently (libvpx, libyuv).
Message was sent while issue was closed.
Hi Ami, Thank you for the comments! Currently, the optimization on iOS has some issue with Clang. This will be revisited in the next step. We have no idea if the real-time check works with renderer sandbox. May you do some investigation and let us know also? Minyue On 2014/03/24 16:16:33, Ami Fischman wrote: > Post-commit drive-by > > https://codereview.chromium.org/150103006/diff/250001/third_party/opus/opus.gyp > File third_party/opus/opus.gyp (right): > > https://codereview.chromium.org/150103006/diff/250001/third_party/opus/opus.g... > third_party/opus/opus.gyp:8: ['(OS=="android" or chromeos==1) and > target_arch=="arm"', { > Why not OS=="ios" (which would also likely mean target_arch=="armv7")? > > https://codereview.chromium.org/150103006/diff/250001/third_party/opus/opus.g... > third_party/opus/opus.gyp:78: 'OPUS_ARM_MAY_HAVE_NEON', > Does the run-time check this turns into > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/opus/s... > really work inside the chrome renderer sandbox? If so that could be a boon to a > few other projects that have struggled with this recently (libvpx, libyuv).
> > Currently, the optimization on iOS has some issue with Clang. This will be > revisited in the next step. > Great. Do you have a bug I can star to follow progress? We have no idea if the real-time check works with renderer sandbox. May you > do > some investigation and let us know also? > The CL description claims "a 40% CPU reduction on Android"; how was that measured if you don't know if NEON is being detected??? I realized the bit of code I was looking at before: __emit(0xF2200150); is actually for MSC, not for linux/android, where instead it appears to try to read /proc/cpuinfo which is known to fail from the renderer process; e.g. https://code.google.com/p/chromium/issues/detail?id=161834 I suspect that in order to accrue the benefits of NEON for OPUS you're going to have to unconditionally rely on it; see e.g. what libyuv settled on: https://code.google.com/p/libyuv/source/browse/trunk/source/cpu_id.cc To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Message was sent while issue was closed.
Hi, 1. I was suggested to report the issue with Clang upstream, it is now here http://llvm.org/bugs/show_bug.cgi?id=19003 2. We compared the performance with/without each switch, which brought to us the number of reduction. Yes, on Android/Linux, the Real-time cpu detection (RTCD) depends on /proc/cpuinfo. On 2014/03/24 16:53:54, Ami Fischman wrote: > > > > Currently, the optimization on iOS has some issue with Clang. This will be > > revisited in the next step. > > > > Great. Do you have a bug I can star to follow progress? > > We have no idea if the real-time check works with renderer sandbox. May you > > do > > some investigation and let us know also? > > > > The CL description claims "a 40% CPU reduction on Android"; how was that > measured if you don't know if NEON is being detected??? > > I realized the bit of code I was looking at before: > __emit(0xF2200150); > is actually for MSC, not for linux/android, where instead it appears to try > to read /proc/cpuinfo which is known to fail from the renderer process; > e.g. https://code.google.com/p/chromium/issues/detail?id=161834 > I suspect that in order to accrue the benefits of NEON for OPUS you're > going to have to unconditionally rely on it; see e.g. what libyuv settled > on: https://code.google.com/p/libyuv/source/browse/trunk/source/cpu_id.cc > > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org.
On Tue, Mar 25, 2014 at 1:41 AM, <minyue@chromium.org> wrote: > 2. We compared the performance with/without each switch, which brought to > us the > number of reduction. > > Yes, on Android/Linux, the Real-time cpu detection (RTCD) depends on > /proc/cpuinfo. > Does this mean that the measured gains were measured outside of chrome, and that a further change must be made to accumulate the gains inside chrome? To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Yes, It was measured outside chrome. Thank you for bringing up this. We will look more into it. On Tue, Mar 25, 2014 at 9:55 AM, Ami Fischman <fischman@chromium.org> wrote: > > On Tue, Mar 25, 2014 at 1:41 AM, <minyue@chromium.org> wrote: > >> 2. We compared the performance with/without each switch, which brought to >> us the >> number of reduction. >> >> Yes, on Android/Linux, the Real-time cpu detection (RTCD) depends on >> /proc/cpuinfo. >> > > Does this mean that the measured gains were measured outside of chrome, > and that a further change must be made to accumulate the gains inside > chrome? > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Thanks for doing this! On Tue, Mar 25, 2014 at 2:05 AM, Minyue Li <minyue@google.com> wrote: > Yes, It was measured outside chrome. > > Thank you for bringing up this. We will look more into it. > > > On Tue, Mar 25, 2014 at 9:55 AM, Ami Fischman <fischman@chromium.org>wrote: > >> >> On Tue, Mar 25, 2014 at 1:41 AM, <minyue@chromium.org> wrote: >> >>> 2. We compared the performance with/without each switch, which brought >>> to us the >>> number of reduction. >>> >>> Yes, on Android/Linux, the Real-time cpu detection (RTCD) depends on >>> /proc/cpuinfo. >>> >> >> Does this mean that the measured gains were measured outside of chrome, >> and that a further change must be made to accumulate the gains inside >> chrome? >> > > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Message was sent while issue was closed.
As Ami states. The code seems to rely on cpuinfo = fopen("/proc/cpuinfo", "r") and I don't think that will work inside the sandbox. I think that some more work is needed here to verify that the performance gains really exists in Chrome.
good news is that Opus's optimization does not only rely on real time cpu detection (RTCD) 1. RTCD brings a relatively small contribution compared to inline assemblers 2. Opus provides micros to hard code Neon. On Tue, Mar 25, 2014 at 11:54 AM, <henrika@chromium.org> wrote: > As Ami states. The code seems to rely on cpuinfo = fopen("/proc/cpuinfo", > "r") > and I don't think that will work inside the sandbox. I think that some > more work > is needed here to verify that the performance gains really exists in > Chrome. > > https://codereview.chromium.org/150103006/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org. |