Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
636083002:
Remove obsoleted target_arch==armv7 from opus.gyp. (Closed)
DescriptionRemove obsoleted target_arch==armv7 from opus.gyp.
- Modify rtcd usage to match the conditions in armcpu.c.
- Make gyp and gn consistent.
TBR=sergeyu
BUG=webrtc:3906
Committed: https://crrev.com/cf18ab2e346a590462e6e1c3676845d4476c4261
Cr-Commit-Position: refs/heads/master@{#302449}
Patch Set 1 #
Total comments: 7
Patch Set 2 : Add correct condition to gyp and gn. #Patch Set 3 : Add missing cflags change to gn. #
Total comments: 4
Patch Set 4 : Rebase. #
Total comments: 7
Patch Set 5 : Use optimize_max with GN. #Patch Set 6 : Use OS=="linux". #Messages
Total messages: 39 (9 generated)
ajm@chromium.org changed reviewers: + kjellander@chromium.org
Henrik, if you lgtm this as-is, feel free to commit, as it's blocking fixing the iOS bot settings. (You should be able to TBR sergeyu for owner-stamp). https://codereview.chromium.org/636083002/diff/1/third_party/opus/opus.gyp File third_party/opus/opus.gyp (right): https://codereview.chromium.org/636083002/diff/1/third_party/opus/opus.gyp#ne... third_party/opus/opus.gyp:8: ['target_arch=="arm" or target_arch=="arm64"', { This is broader than the corresponding BUILD.gn condition. Should we modify gn? https://codereview.chromium.org/636083002/diff/1/third_party/opus/opus.gyp#ne... third_party/opus/opus.gyp:18: ['target_arch=="arm" and (OS=="android" or chromeos==1)', { This is what BUILD.gn uses, though technically it appears that we could support Windows and desktop Linux as well. Do you think I should change both?
kjellander@chromium.org changed reviewers: + tlegrand@chromium.org
lgtm but I would like Tina to take a look as well. Ideally we would update BUILD.gn to reflect the changes in https://codereview.chromium.org/354903003, but that can be done in another CL if this is urgent. https://codereview.chromium.org/636083002/diff/1/third_party/opus/opus.gyp File third_party/opus/opus.gyp (right): https://codereview.chromium.org/636083002/diff/1/third_party/opus/opus.gyp#ne... third_party/opus/opus.gyp:8: ['target_arch=="arm" or target_arch=="arm64"', { On 2014/10/08 06:19:53, ajm wrote: > This is broader than the corresponding BUILD.gn condition. Should we modify gn? Yes, please update BUILD.gn if you want (can be done in a separate CL though). Tina made changes to opus.gyp in https://codereview.chromium.org/354903003 before I was finished with the GN work so it was never updated to reflect those changes. https://codereview.chromium.org/636083002/diff/1/third_party/opus/opus.gyp#ne... third_party/opus/opus.gyp:18: ['target_arch=="arm" and (OS=="android" or chromeos==1)', { On 2014/10/08 06:19:53, ajm wrote: > This is what BUILD.gn uses, though technically it appears that we could support > Windows and desktop Linux as well. Do you think I should change both? GN and GYP should be the same, but I don't know enough about the desktop support to answer that question. I'd prefer if Tina can comment on this.
Let me have a look, and consult Minyue. I think the opus.gyp file is the correct onw, and BUILD.gn needs to be updated. https://codereview.chromium.org/636083002/diff/1/third_party/opus/opus.gyp File third_party/opus/opus.gyp (right): https://codereview.chromium.org/636083002/diff/1/third_party/opus/opus.gyp#ne... third_party/opus/opus.gyp:18: ['target_arch=="arm" and (OS=="android" or chromeos==1)', { This change is not correct, it is the BUILD.gn that needs to be updated.
OK, gyp and gn are now consistent. The condition for enabling rtcd is correct AFAICT. https://codereview.chromium.org/636083002/diff/1/third_party/opus/opus.gyp File third_party/opus/opus.gyp (right): https://codereview.chromium.org/636083002/diff/1/third_party/opus/opus.gyp#ne... third_party/opus/opus.gyp:8: ['target_arch=="arm" or target_arch=="arm64"', { On 2014/10/08 07:24:04, kjellander wrote: > On 2014/10/08 06:19:53, ajm wrote: > > This is broader than the corresponding BUILD.gn condition. Should we modify > gn? > > Yes, please update BUILD.gn if you want (can be done in a separate CL though). > Tina made changes to opus.gyp in https://codereview.chromium.org/354903003 > before I was finished with the GN work so it was never updated to reflect those > changes. Done. https://codereview.chromium.org/636083002/diff/1/third_party/opus/opus.gyp#ne... third_party/opus/opus.gyp:18: ['target_arch=="arm" and (OS=="android" or chromeos==1)', { On 2014/10/08 09:03:27, tlegrand1 wrote: > This change is not correct, it is the BUILD.gn that needs to be updated. Done.
On 2014/10/08 15:55:01, ajm wrote: > OK, gyp and gn are now consistent. The condition for enabling rtcd is correct > AFAICT. > > https://codereview.chromium.org/636083002/diff/1/third_party/opus/opus.gyp > File third_party/opus/opus.gyp (right): > > https://codereview.chromium.org/636083002/diff/1/third_party/opus/opus.gyp#ne... > third_party/opus/opus.gyp:8: ['target_arch=="arm" or target_arch=="arm64"', { > On 2014/10/08 07:24:04, kjellander wrote: > > On 2014/10/08 06:19:53, ajm wrote: > > > This is broader than the corresponding BUILD.gn condition. Should we modify > > gn? > > > > Yes, please update BUILD.gn if you want (can be done in a separate CL though). > > Tina made changes to opus.gyp in https://codereview.chromium.org/354903003 > > before I was finished with the GN work so it was never updated to reflect > those > > changes. > > Done. > > https://codereview.chromium.org/636083002/diff/1/third_party/opus/opus.gyp#ne... > third_party/opus/opus.gyp:18: ['target_arch=="arm" and (OS=="android" or > chromeos==1)', { > On 2014/10/08 09:03:27, tlegrand1 wrote: > > This change is not correct, it is the BUILD.gn that needs to be updated. > > Done. Minyue is having a look, so please wait for his comments.
On 2014/10/08 16:03:09, tlegrand1 wrote: > Minyue is having a look, so please wait for his comments. Will do. I have a workaround to deal with the iOS bots, so this shouldn't be blocking the roll.
minyue@chromium.org changed reviewers: + minyue@chromium.org
https://codereview.chromium.org/636083002/diff/40001/third_party/opus/opus.gyp File third_party/opus/opus.gyp (right): https://codereview.chromium.org/636083002/diff/40001/third_party/opus/opus.gy... third_party/opus/opus.gyp:8: ['target_arch=="arm" or target_arch=="arm64"', { Has armv7 become arm in the building system? If so, has use_opus_rtcd been activated for IOs? How could that work? It should not be able to support rtcd from my understanding.
https://codereview.chromium.org/636083002/diff/40001/third_party/opus/opus.gyp File third_party/opus/opus.gyp (right): https://codereview.chromium.org/636083002/diff/40001/third_party/opus/opus.gy... third_party/opus/opus.gyp:8: ['target_arch=="arm" or target_arch=="arm64"', { On 2014/10/09 07:49:53, minyue wrote: > Has armv7 become arm in the building system? Yes, you now specify armv7 with target_arch=="arm" and arm_version==7. > > If so, has use_opus_rtcd been activated for IOs? How could that work? It should > not be able to support rtcd from my understanding. Not sure what you mean; this condition isn't setting rtcd, just whether to use fixed-point or not. You can see below the conditions I'm using for rtcd, which _does not_ include iOS.
https://codereview.chromium.org/636083002/diff/40001/third_party/opus/opus.gyp File third_party/opus/opus.gyp (left): https://codereview.chromium.org/636083002/diff/40001/third_party/opus/opus.gy... third_party/opus/opus.gyp:18: ['target_arch=="arm"', { With deprecation of armv7, did iOS already end up here. Did any iOS bot get wrong with rtcd? Or is armv7 something going to happen but not yet?
https://codereview.chromium.org/636083002/diff/40001/third_party/opus/opus.gyp File third_party/opus/opus.gyp (left): https://codereview.chromium.org/636083002/diff/40001/third_party/opus/opus.gy... third_party/opus/opus.gyp:18: ['target_arch=="arm"', { On 2014/10/09 15:38:10, minyue wrote: > With deprecation of armv7, did iOS already end up here. Did any iOS bot get > wrong with rtcd? In our standalone webrtc waterfall, we're still (incorrectly) using target_arch=armv7, so that was fine. I had a quick look at the Chromium iOS waterfall and it looks like they're not specifying target_arch at all (at least in GYP_DEFINES) so I'm not totally sure what's going on there. But it's certainly not breaking the build. > > Or is armv7 something going to happen but not yet? It was changed quite awhile ago (Dec 2013). If you have a look at the linked bug: https://code.google.com/p/webrtc/issues/detail?id=3906 I link to the Chromium bug history for this.
On 2014/10/09 15:42:41, ajm wrote: > https://codereview.chromium.org/636083002/diff/40001/third_party/opus/opus.gyp > File third_party/opus/opus.gyp (left): > > https://codereview.chromium.org/636083002/diff/40001/third_party/opus/opus.gy... > third_party/opus/opus.gyp:18: ['target_arch=="arm"', { > On 2014/10/09 15:38:10, minyue wrote: > > With deprecation of armv7, did iOS already end up here. Did any iOS bot get > > wrong with rtcd? > > In our standalone webrtc waterfall, we're still (incorrectly) using > target_arch=armv7, so that was fine. I had a quick look at the Chromium iOS > waterfall and it looks like they're not specifying target_arch at all (at least > in GYP_DEFINES) so I'm not totally sure what's going on there. But it's > certainly not breaking the build. > > > > > > Or is armv7 something going to happen but not yet? > > It was changed quite awhile ago (Dec 2013). If you have a look at the linked > bug: > https://code.google.com/p/webrtc/issues/detail?id=3906 > > I link to the Chromium bug history for this. OK. thanks. then it gets my LGTM.
ajm@chromium.org changed reviewers: + sergeyu@chromium.org
Sergey, can you give this an owner stamp?
On 2014/10/10 06:42:50, tlegrand1 wrote: Hmmm, my text disappeared. I don't think Opus is built for iOS in the chromium waterfall, so this might break in the future. Minyue, any chance you have time to try build today? I understand if you don't, since you have other things to finish before going OOO.
I will try. If I cannot get results today. I will make it over the weekend. On Fri, Oct 10, 2014 at 8:44 AM, <tlegrand@chromium.org> wrote: > On 2014/10/10 06:42:50, tlegrand1 wrote: > > Hmmm, my text disappeared. > > I don't think Opus is built for iOS in the chromium waterfall, so this > might > break in the future. Minyue, any chance you have time to try build today? I > understand if you don't, since you have other things to finish before > going OOO. > > https://codereview.chromium.org/636083002/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2014/10/10 06:44:54, tlegrand1 wrote: > On 2014/10/10 06:42:50, tlegrand1 wrote: > > Hmmm, my text disappeared. > > I don't think Opus is built for iOS in the chromium waterfall, so this might > break in the future. Minyue, any chance you have time to try build today? I > understand if you don't, since you have other things to finish before going OOO. Note that this CL doesn't alter whether Opus is built on iOS or not (at least for gyp). I think we should land it, as it will allow our iOS bots to be target_arch conformant, and you can start another CL around your concerns if needed. Agreed?
On 2014/10/10 17:41:15, ajm wrote: > On 2014/10/10 06:44:54, tlegrand1 wrote: > > On 2014/10/10 06:42:50, tlegrand1 wrote: > > > > Hmmm, my text disappeared. > > > > I don't think Opus is built for iOS in the chromium waterfall, so this might > > break in the future. Minyue, any chance you have time to try build today? I > > understand if you don't, since you have other things to finish before going > OOO. > > Note that this CL doesn't alter whether Opus is built on iOS or not (at least > for gyp). I think we should land it, as it will allow our iOS bots to be > target_arch conformant, and you can start another CL around your concerns if > needed. > > Agreed? Agreed. LGTM
Sergey, can you have a quick look for OWNER approval?
TBRing to Sergey.
The CQ bit was checked by ajm@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/636083002/140001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_chromium_gn_compile_dbg on tryserver.chromium.linux () android_chromium_gn_compile_rel on tryserver.chromium.linux ()
ajm@chromium.org changed reviewers: + brettw@chromium.org
Brett, could you have a look at my GN question if you get a chance? https://codereview.chromium.org/636083002/diff/140001/third_party/opus/BUILD.gn File third_party/opus/BUILD.gn (right): https://codereview.chromium.org/636083002/diff/140001/third_party/opus/BUILD.... third_party/opus/BUILD.gn:88: cflags -= [ "-Os" ] I want to remove -Os from the "global" cflags, but I get this error: ERROR at //third_party/opus/BUILD.gn:88:5: Undefined variable for -=. cflags -= [ "-Os" ] ^----- I don't have something with this name in scope now. This makes sense locally. How do I modify the "global" cflags? Do I just need to make sure cflags is defined here locally (i.e. something like cflags = [] above this)? I've marked the equivalent gyp operation. https://codereview.chromium.org/636083002/diff/140001/third_party/opus/opus.gyp File third_party/opus/opus.gyp (right): https://codereview.chromium.org/636083002/diff/140001/third_party/opus/opus.g... third_party/opus/opus.gyp:87: 'cflags!': ['-Os'], Here's the gyp equivalent.
https://codereview.chromium.org/636083002/diff/140001/third_party/opus/BUILD.gn File third_party/opus/BUILD.gn (right): https://codereview.chromium.org/636083002/diff/140001/third_party/opus/BUILD.... third_party/opus/BUILD.gn:88: cflags -= [ "-Os" ] # Note addition of "is_release" in the condition: if (is_release && is_posix && (cpu_arch == "arm" || cpu_arch == "arm64")) { configs -= [ "//build/config/compiler:optimize" ] configs += [ "//build/config/compiler:optimize_max" ] } This is more-or-less covered in the cookbook https://code.google.com/p/chromium/wiki/GNCookbook under the "optimize: max" section. BTW, optimize:max is probably what you should be doing in GYP instead of manually messing with the flags.
Tina, Minyue: this brings up an interesting point. Do we really only want to have the highest level of speed optimization on ARM? Or should we be doing this for all platforms? https://codereview.chromium.org/636083002/diff/140001/third_party/opus/BUILD.gn File third_party/opus/BUILD.gn (right): https://codereview.chromium.org/636083002/diff/140001/third_party/opus/BUILD.... third_party/opus/BUILD.gn:88: cflags -= [ "-Os" ] On 2014/10/24 23:36:18, brettw wrote: > # Note addition of "is_release" in the condition: > if (is_release && is_posix && (cpu_arch == "arm" || cpu_arch == "arm64")) { > configs -= [ "//build/config/compiler:optimize" ] > configs += [ "//build/config/compiler:optimize_max" ] > } > > This is more-or-less covered in the cookbook > https://code.google.com/p/chromium/wiki/GNCookbook under the "optimize: max" > section. Thanks, updated as advised. > BTW, optimize:max is probably what you should be doing in GYP instead > of manually messing with the flags. I looked into this, but it seems optimize only has an impact on Windows? https://code.google.com/p/chromium/codesearch#chromium/src/build/common.gypi&... (At least I couldn't see anywhere else it would have an impact.)
lgtm https://codereview.chromium.org/636083002/diff/140001/third_party/opus/BUILD.gn File third_party/opus/BUILD.gn (right): https://codereview.chromium.org/636083002/diff/140001/third_party/opus/BUILD.... third_party/opus/BUILD.gn:88: cflags -= [ "-Os" ] Oh yes, you're right. I now remember being offended by that asymmetry. https://codereview.chromium.org/636083002/diff/140001/third_party/opus/opus.gyp File third_party/opus/opus.gyp (right): https://codereview.chromium.org/636083002/diff/140001/third_party/opus/opus.g... third_party/opus/opus.gyp:18: ['target_arch=="arm" and (OS=="win" or OS=="android" or desktop_linux==1 or chromeos==1)', { replace the desktop_linux/chromeos thing with OS == "linux" which is true for both cases.
Patchset #6 (id:180001) has been deleted
https://codereview.chromium.org/636083002/diff/140001/third_party/opus/opus.gyp File third_party/opus/opus.gyp (right): https://codereview.chromium.org/636083002/diff/140001/third_party/opus/opus.g... third_party/opus/opus.gyp:18: ['target_arch=="arm" and (OS=="win" or OS=="android" or desktop_linux==1 or chromeos==1)', { On 2014/10/25 15:58:06, brettw wrote: > replace the desktop_linux/chromeos thing with > OS == "linux" > which is true for both cases. Thanks, I wasn't sure whether OS=="linux" in the case of e.g. BSD. I made the same change in BUILD.gn. Can it be used in the same way?
Sorry for late reply Andrew, on your question "Do we really only want to have the highest level of speed optimization on ARM? Or should we be doing this for all platforms?" The reason we are not optimizing for speed on all platforms, is because you get speed at the cost of binary size. There were concerns raise on this subject when we tried, so we settled for only using it on ARM, where we really need it.
The CQ bit was checked by ajm@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/636083002/200001
Message was sent while issue was closed.
Committed patchset #6 (id:200001)
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/cf18ab2e346a590462e6e1c3676845d4476c4261 Cr-Commit-Position: refs/heads/master@{#302449} |
