Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1349393003:
Don't link with audio codecs that we don't use (Closed)
|
Created:
5 years, 3 months ago by kwiberg-webrtc Modified:
5 years, 3 months ago CC:
webrtc-reviews_webrtc.org, interface-changes_webrtc.org, rwolff_gocast.it, yujie_mao (webrtc), Andrew MacDonald, hlundin-webrtc, tlegrand-webrtc, tterriberry_mozilla.com, qiang.lu, niklas.enbom, kwiberg-webrtc, the sun Base URL:
https://chromium.googlesource.com/external/webrtc.git@dmove-clean Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionDon't link with audio codecs that we don't use
We used to link with all audio codecs unconditionally (except Opus);
this patch makes gyp and gn only link to the ones that are used.
(This unfortunately fails to have a measurable impact on Chromium
binary size, at least on x86_64 Linux; it turns out that iLBC and iSAC
fix were already being excluded from Chromium by some other means
(likely just the linker omitting compilation units with no incoming
references).)
BUG=webrtc:4557
Committed: https://crrev.com/f66a9251424351ea6d631c54dd1feb64cc13d809
Cr-Commit-Position: refs/heads/master@{#10046}
Patch Set 1 #Patch Set 2 : ARM fix #
Total comments: 34
Patch Set 3 : review fixes #Patch Set 4 : More ARM fixes #
Messages
Total messages: 30 (10 generated)
kwiberg@webrtc.org changed reviewers: + henrik.lundin@webrtc.org, kjellander@webrtc.org, tina.legrand@webrtc.org
Please have a look. Tina: engine_configurations.h Henrik: gyp and gn files (compare with engine_configurations.h) Henrik: Everything
https://codereview.webrtc.org/1349393003/diff/20001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/BUILD.gn (right): https://codereview.webrtc.org/1349393003/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/BUILD.gn:833: } This duplication will go away soon; I'm just about to start working on collecting all the built-in codecs in a single place. https://codereview.webrtc.org/1349393003/diff/20001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/main/acm2/codec_owner.cc (right): https://codereview.webrtc.org/1349393003/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/main/acm2/codec_owner.cc:145: return encoder; This fixes a pre-existing bug, which started causing a link error in Chromium now that we only conditionally link with RED. https://codereview.webrtc.org/1349393003/diff/20001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/neteq/neteq.gypi (right): https://codereview.webrtc.org/1349393003/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/neteq/neteq.gypi:38: }], Same as for gn: This duplication will go away soon; I'm just about to start working on collecting all the built-in codecs in a single place.
https://codereview.webrtc.org/1349393003/diff/20001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/BUILD.gn (right): https://codereview.webrtc.org/1349393003/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/BUILD.gn:833: } On 2015/09/22 08:37:15, kwiberg-webrtc wrote: > This duplication will go away soon; I'm just about to start working on > collecting all the built-in codecs in a single place. Acknowledged. https://codereview.webrtc.org/1349393003/diff/20001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/main/acm2/codec_owner.cc (right): https://codereview.webrtc.org/1349393003/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/main/acm2/codec_owner.cc:145: return encoder; On 2015/09/22 08:37:15, kwiberg-webrtc wrote: > This fixes a pre-existing bug, which started causing a link error in Chromium > now that we only conditionally link with RED. Acknowledged. https://codereview.webrtc.org/1349393003/diff/20001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/main/audio_coding_module.gypi (right): https://codereview.webrtc.org/1349393003/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/main/audio_coding_module.gypi:28: 'audio_coding_dependencies': ['isac_fix',], I'm not sure about the syntax here. Do these lines _add_ new elements to the lists? It looks to me like they are replacing the lists with new ones. https://codereview.webrtc.org/1349393003/diff/20001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/neteq/audio_decoder_unittest.cc (right): https://codereview.webrtc.org/1349393003/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/neteq/audio_decoder_unittest.cc:706: static const bool has_isac_swb = false; Nit: kHasIsacSwb? https://codereview.webrtc.org/1349393003/diff/20001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/neteq/neteq.gypi (right): https://codereview.webrtc.org/1349393003/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/neteq/neteq.gypi:136: 'g722', Aren't these already included in the codecs variable?
https://codereview.webrtc.org/1349393003/diff/20001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/main/audio_coding_module.gypi (right): https://codereview.webrtc.org/1349393003/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/main/audio_coding_module.gypi:28: 'audio_coding_dependencies': ['isac_fix',], On 2015/09/22 09:09:18, hlundin-webrtc wrote: > I'm not sure about the syntax here. Do these lines _add_ new elements to the > lists? It looks to me like they are replacing the lists with new ones. Yes, they add new elements. See https://chromium.googlesource.com/external/gyp/+/master/docs/InputFormatRefer... ("A conditions section introduces a subdictionary that is only merged into the enclosing scope based on the evaluation of a conditional expression.") https://codereview.webrtc.org/1349393003/diff/20001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/neteq/audio_decoder_unittest.cc (right): https://codereview.webrtc.org/1349393003/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/neteq/audio_decoder_unittest.cc:706: static const bool has_isac_swb = false; On 2015/09/22 09:09:18, hlundin-webrtc wrote: > Nit: kHasIsacSwb? "Use a k followed by mixed case, e.g., kDaysInAWeek, for constants defined globally or within a class. [...] This convention may optionally be used for compile-time constants of local scope; otherwise the usual variable naming rules apply." I chose not to use a kCamelCase name because they're uglier and slightly harder to read, and loudly signalling that this is a constant doesn't make this code easier to read. https://codereview.webrtc.org/1349393003/diff/20001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/neteq/neteq.gypi (right): https://codereview.webrtc.org/1349393003/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/neteq/neteq.gypi:136: 'g722', On 2015/09/22 09:09:18, hlundin-webrtc wrote: > Aren't these already included in the codecs variable? They used to be unconditionally included in codecs, but after the change earlier in this file, they're only conditionally included. And audio_decoder_unittests wants all codecs regardless of whether they're included in the rest of the WebRTC build. Lucky for me, it appears that gyp silently accepts listing a dependency twice.
lgtm https://codereview.webrtc.org/1349393003/diff/20001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/main/audio_coding_module.gypi (right): https://codereview.webrtc.org/1349393003/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/main/audio_coding_module.gypi:28: 'audio_coding_dependencies': ['isac_fix',], On 2015/09/22 09:37:04, kwiberg-webrtc wrote: > On 2015/09/22 09:09:18, hlundin-webrtc wrote: > > I'm not sure about the syntax here. Do these lines _add_ new elements to the > > lists? It looks to me like they are replacing the lists with new ones. > > Yes, they add new elements. See > https://chromium.googlesource.com/external/gyp/+/master/docs/InputFormatRefer... > ("A conditions section introduces a subdictionary that is only merged into the > enclosing scope based on the evaluation of a conditional expression.") Acknowledged. https://codereview.webrtc.org/1349393003/diff/20001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/neteq/audio_decoder_unittest.cc (right): https://codereview.webrtc.org/1349393003/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/neteq/audio_decoder_unittest.cc:706: static const bool has_isac_swb = false; On 2015/09/22 09:37:04, kwiberg-webrtc wrote: > On 2015/09/22 09:09:18, hlundin-webrtc wrote: > > Nit: kHasIsacSwb? > > "Use a k followed by mixed case, e.g., kDaysInAWeek, for constants defined > globally or within a class. [...] This convention may optionally be used for > compile-time constants of local scope; otherwise the usual variable naming rules > apply." > > I chose not to use a kCamelCase name because they're uglier and slightly harder > to read, and loudly signalling that this is a constant doesn't make this code > easier to read. Acknowledged. https://codereview.webrtc.org/1349393003/diff/20001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/neteq/neteq.gypi (right): https://codereview.webrtc.org/1349393003/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/neteq/neteq.gypi:136: 'g722', On 2015/09/22 09:37:04, kwiberg-webrtc wrote: > On 2015/09/22 09:09:18, hlundin-webrtc wrote: > > Aren't these already included in the codecs variable? > > They used to be unconditionally included in codecs, but after the change earlier > in this file, they're only conditionally included. And audio_decoder_unittests > wants all codecs regardless of whether they're included in the rest of the > WebRTC build. > > Lucky for me, it appears that gyp silently accepts listing a dependency twice. Ah, of course.
On 2015/09/22 08:33:20, kwiberg-webrtc wrote: > Please have a look. > > Tina: engine_configurations.h > Henrik: gyp and gn files (compare with engine_configurations.h) > Henrik: Everything Which Henrik is me? ;)
https://codereview.webrtc.org/1349393003/diff/20001/webrtc/build/common.gypi File webrtc/build/common.gypi (right): https://codereview.webrtc.org/1349393003/diff/20001/webrtc/build/common.gypi#... webrtc/build/common.gypi:104: 'build_with_mozilla%': '<(build_with_mozilla)', move this to line 51. https://codereview.webrtc.org/1349393003/diff/20001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/BUILD.gn (right): https://codereview.webrtc.org/1349393003/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/BUILD.gn:82: if (!build_with_mozilla) { Since line 82-92 is duplicated below, maybe you can move it to its own config target and depend on it? I'm not entirely sure it'll end up with the same behavior, but I think it might. https://codereview.webrtc.org/1349393003/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/BUILD.gn:83: if (current_cpu == "arm") { or current_cpu=="arm64" ? https://codereview.webrtc.org/1349393003/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/BUILD.gn:820: if (current_cpu == "arm") { arm64 as well. https://codereview.webrtc.org/1349393003/diff/20001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/main/audio_coding_module.gypi (right): https://codereview.webrtc.org/1349393003/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/main/audio_coding_module.gypi:21: ['include_opus == 1', { Please don't use spaces around == See https://chromium.googlesource.com/external/gyp/+/master/docs/InputFormatRefer... for examples on the recommended style. https://codereview.webrtc.org/1349393003/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/main/audio_coding_module.gypi:27: ['target_arch == "arm"', { ... or target_arch=="arm64" https://codereview.webrtc.org/1349393003/diff/20001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/neteq/neteq.gypi (left): https://codereview.webrtc.org/1349393003/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/neteq/neteq.gypi:134: 'WEBRTC_CODEC_ISAC', the test don't need these? https://codereview.webrtc.org/1349393003/diff/20001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/neteq/neteq.gypi (right): https://codereview.webrtc.org/1349393003/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/neteq/neteq.gypi:24: ['target_arch == "arm"', { arm64 too. https://codereview.webrtc.org/1349393003/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/neteq/neteq.gypi:38: }], On 2015/09/22 08:37:15, kwiberg-webrtc wrote: > Same as for gn: This duplication will go away soon; I'm just about to start > working on collecting all the built-in codecs in a single place. Removing the duplication in future CL sgtm.
Comments addressed in patch set #3. https://codereview.webrtc.org/1349393003/diff/20001/webrtc/build/common.gypi File webrtc/build/common.gypi (right): https://codereview.webrtc.org/1349393003/diff/20001/webrtc/build/common.gypi#... webrtc/build/common.gypi:104: 'build_with_mozilla%': '<(build_with_mozilla)', On 2015/09/22 11:01:28, kjellander (webrtc) wrote: > move this to line 51. Done. https://codereview.webrtc.org/1349393003/diff/20001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/BUILD.gn (right): https://codereview.webrtc.org/1349393003/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/BUILD.gn:82: if (!build_with_mozilla) { On 2015/09/22 11:01:28, kjellander (webrtc) wrote: > Since line 82-92 is duplicated below, maybe you can move it to its own config > target and depend on it? I'm not entirely sure it'll end up with the same > behavior, but I think it might. Will do this in a later CL (see discussion below). https://codereview.webrtc.org/1349393003/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/BUILD.gn:83: if (current_cpu == "arm") { On 2015/09/22 11:01:28, kjellander (webrtc) wrote: > or current_cpu=="arm64" ? Well, that might make sense. But it won't preserve the current behavior; see the removed code in engine_configurations.h. There, things are conditional on WEBRTC_ARCH_ARM, which is set iff current_cpu=="arm" (gn) or target_cpu=="arm".) So it doesn't seem like a good move to do that in this CL. And since it's performance needs that made us choose iSAC for arm, it's not obvious (at least to me) that arm64 should automatically be treated the same without investigating the pros and cons first. Other Henrik, what's your opinion? https://codereview.webrtc.org/1349393003/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/BUILD.gn:820: if (current_cpu == "arm") { On 2015/09/22 11:01:28, kjellander (webrtc) wrote: > arm64 as well. Not in this CL; see above. https://codereview.webrtc.org/1349393003/diff/20001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/main/audio_coding_module.gypi (right): https://codereview.webrtc.org/1349393003/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/main/audio_coding_module.gypi:21: ['include_opus == 1', { On 2015/09/22 11:01:28, kjellander (webrtc) wrote: > Please don't use spaces around == > See > https://chromium.googlesource.com/external/gyp/+/master/docs/InputFormatRefer... > for examples on the recommended style. Very well. But the spaces definitely make the conditions easier to read. https://codereview.webrtc.org/1349393003/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/main/audio_coding_module.gypi:27: ['target_arch == "arm"', { On 2015/09/22 11:01:28, kjellander (webrtc) wrote: > ... or target_arch=="arm64" No (see earlier comment). https://codereview.webrtc.org/1349393003/diff/20001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/neteq/neteq.gypi (left): https://codereview.webrtc.org/1349393003/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/neteq/neteq.gypi:134: 'WEBRTC_CODEC_ISAC', On 2015/09/22 11:01:28, kjellander (webrtc) wrote: > the test don't need these? Not anymore (see the change in audio_decoder_impl.h). https://codereview.webrtc.org/1349393003/diff/20001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/neteq/neteq.gypi (right): https://codereview.webrtc.org/1349393003/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/neteq/neteq.gypi:38: }], On 2015/09/22 11:01:28, kjellander (webrtc) wrote: > On 2015/09/22 08:37:15, kwiberg-webrtc wrote: > > Same as for gn: This duplication will go away soon; I'm just about to start > > working on collecting all the built-in codecs in a single place. > > Removing the duplication in future CL sgtm. Acknowledged.
lgtm! Thanks for cleaning this up and reducing unnecessary binary size. https://codereview.webrtc.org/1349393003/diff/20001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/BUILD.gn (right): https://codereview.webrtc.org/1349393003/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/BUILD.gn:83: if (current_cpu == "arm") { On 2015/09/22 12:05:14, kwiberg-webrtc wrote: > On 2015/09/22 11:01:28, kjellander (webrtc) wrote: > > or current_cpu=="arm64" ? > > Well, that might make sense. But it won't preserve the current behavior; see the > removed code in engine_configurations.h. There, things are conditional on > WEBRTC_ARCH_ARM, which is set iff current_cpu=="arm" (gn) or target_cpu=="arm".) > > So it doesn't seem like a good move to do that in this CL. And since it's > performance needs that made us choose iSAC for arm, it's not obvious (at least > to me) that arm64 should automatically be treated the same without investigating > the pros and cons first. > > Other Henrik, what's your opinion? I see. I thought WEBRTC_ARCH_ARM was defined for both. My mistake. https://codereview.webrtc.org/1349393003/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/BUILD.gn:820: if (current_cpu == "arm") { On 2015/09/22 12:05:14, kwiberg-webrtc wrote: > On 2015/09/22 11:01:28, kjellander (webrtc) wrote: > > arm64 as well. > > Not in this CL; see above. Fair enough. https://codereview.webrtc.org/1349393003/diff/20001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/main/audio_coding_module.gypi (right): https://codereview.webrtc.org/1349393003/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/main/audio_coding_module.gypi:27: ['target_arch == "arm"', { On 2015/09/22 12:05:14, kwiberg-webrtc wrote: > On 2015/09/22 11:01:28, kjellander (webrtc) wrote: > > ... or target_arch=="arm64" > > No (see earlier comment). Acknowledged.
Great clean-up! LGTM
The CQ bit was checked by kwiberg@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from henrik.lundin@webrtc.org Link to the patchset: https://codereview.webrtc.org/1349393003/#ps40001 (title: "review fixes")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1349393003/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1349393003/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_compile_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_compile_dbg/builds/42)
The CQ bit was checked by kwiberg@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1349393003/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1349393003/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_rel/builds/7815)
The CQ bit was checked by kwiberg@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from henrik.lundin@webrtc.org, kjellander@webrtc.org, tina.legrand@webrtc.org Link to the patchset: https://codereview.webrtc.org/1349393003/#ps60001 (title: "More ARM fixes")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1349393003/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1349393003/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_compile_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_compile_dbg/builds/50)
The CQ bit was checked by kwiberg@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1349393003/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1349393003/60001
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/f66a9251424351ea6d631c54dd1feb64cc13d809 Cr-Commit-Position: refs/heads/master@{#10046}
Message was sent while issue was closed.
A revert of this CL (patchset #4 id:60001) has been created in https://codereview.webrtc.org/1368933002/ by solenberg@webrtc.org. The reason for reverting is: Breaking Chromium FYI bots.. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
