Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2270403002:
Migrated ILBC and ISAC test targets for GN. (Closed)
|
Created:
4 years, 4 months ago by aleloi Modified:
4 years, 3 months ago Reviewers:
kjellander_webrtc CC:
webrtc-reviews_webrtc.org, yujie_mao (webrtc), kwiberg-webrtc, hlundin-webrtc, tlegrand-webrtc, tterriberry_mozilla.com, audio-team_agora.io, qiang.lu, niklas.enbom, peah-webrtc, minyue-webrtc, ivoc Base URL:
https://chromium.googlesource.com/external/webrtc.git@real_master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionMigrated ILBC and ISAC test targets for GN.
Migrated GN targets ilbc_test, isac_api_test,
isac_switch_samprate_test from webrtc/modules/audio_coding/codecs
NOTRY=True
NOPRESUBMIT=True
BUG=webrtc:6191
Committed: https://crrev.com/cfee215b23105775ec89f80162695bcb7cf4b073
Cr-Commit-Position: refs/heads/master@{#13953}
Patch Set 1 : . #
Total comments: 13
Patch Set 2 : Minor changes in response to comments. #
Total comments: 6
Patch Set 3 : Removed rtc_base dependence. #Patch Set 4 : Reorder. #
Messages
Total messages: 22 (12 generated)
Patchset #1 (id:1) has been deleted
Description was changed from ========== Migrated ILBC and ISAC test targets for GN. Migrated GN targets ilbc_test, isac_test, isac_api_test, isac_switch_samprate_test from webrtc/modules/audio_coding/codecs NOTRY=True BUG=webrtc:6191 ========== to ========== Migrated ILBC and ISAC test targets for GN. Migrated GN targets ilbc_test, isac_api_test, isac_switch_samprate_test from webrtc/modules/audio_coding/codecs NOTRY=True BUG=webrtc:6191 ==========
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
Patchset #1 (id:60001) has been deleted
Description was changed from ========== Migrated ILBC and ISAC test targets for GN. Migrated GN targets ilbc_test, isac_api_test, isac_switch_samprate_test from webrtc/modules/audio_coding/codecs NOTRY=True BUG=webrtc:6191 ========== to ========== Migrated ILBC and ISAC test targets for GN. Migrated GN targets ilbc_test, isac_api_test, isac_switch_samprate_test from webrtc/modules/audio_coding/codecs NOTRY=True NOPRESUBMIT=True BUG=webrtc:6191 ==========
aleloi@webrtc.org changed reviewers: + kjellander@webrtc.org
Added 3 last remaining GN targets from audio_coding. https://codereview.webrtc.org/2270403002/diff/80001/.gn File .gn (right): https://codereview.webrtc.org/2270403002/diff/80001/.gn#newcode33 .gn:33: "//webrtc/modules/audio_coding:isac", Also added :isac, as it was very simple to make pass the checks. https://codereview.webrtc.org/2270403002/diff/80001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/BUILD.gn (right): https://codereview.webrtc.org/2270403002/diff/80001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/BUILD.gn:1586: "//build/config/sanitizers:deps", Added dependence to :isac in order for it to pass gn check https://codereview.webrtc.org/2270403002/diff/80001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/BUILD.gn:1603: "//build/config/sanitizers:deps", Added sanitizers deps to executable. https://codereview.webrtc.org/2270403002/diff/80001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/BUILD.gn:1617: "../../base:rtc_base", 'Forbidden' dependency; also added matching dependency in gyp files.
https://codereview.webrtc.org/2270403002/diff/80001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/BUILD.gn (right): https://codereview.webrtc.org/2270403002/diff/80001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/BUILD.gn:1615: deps = [ For all three targets, please add: configs += [ "../..:common_config" ] public_configs = [ "../..:common_inherited_config" ] It'll go away as soon we've figure out how to use GN templates for it, but for now it should be used for all targets (although it may not be needed for these, it's better to be safe so all the right defines are set for the code). https://codereview.webrtc.org/2270403002/diff/80001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/BUILD.gn:1617: "../../base:rtc_base", On 2016/08/24 11:02:51, aleloi wrote: > 'Forbidden' dependency; also added matching dependency in gyp files. Do you need rtc_base or is rtc_base_approved enough? (preferred) https://codereview.webrtc.org/2270403002/diff/80001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/BUILD.gn:1623: "codecs/isac/main/include", nit: sort includes https://codereview.webrtc.org/2270403002/diff/80001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/BUILD.gn:1643: "codecs/isac/main/include", nit: sort includes.
New patch set. Ptal! https://codereview.webrtc.org/2270403002/diff/80001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/BUILD.gn (right): https://codereview.webrtc.org/2270403002/diff/80001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/BUILD.gn:1617: "../../base:rtc_base", On 2016/08/26 14:07:37, kjellander_webrtc wrote: > On 2016/08/24 11:02:51, aleloi wrote: > > 'Forbidden' dependency; also added matching dependency in gyp files. > > Do you need rtc_base or is rtc_base_approved enough? (preferred) Yes, rtc_base is needed. Specifically webrtc/base/format_macros.h https://codereview.webrtc.org/2270403002/diff/80001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/BUILD.gn:1623: "codecs/isac/main/include", On 2016/08/26 14:07:37, kjellander_webrtc wrote: > nit: sort includes Done. https://codereview.webrtc.org/2270403002/diff/80001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/BUILD.gn:1643: "codecs/isac/main/include", On 2016/08/26 14:07:38, kjellander_webrtc wrote: > nit: sort includes. Done.
Patchset #2 (id:100001) has been deleted
https://codereview.webrtc.org/2270403002/diff/80001/.gn File .gn (right): https://codereview.webrtc.org/2270403002/diff/80001/.gn#newcode28 .gn:28: "//webrtc/modules/audio_mixer:audio_conference_mixer", Not changed in this CL, diff from rebase. https://codereview.webrtc.org/2270403002/diff/120001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/BUILD.gn (right): https://codereview.webrtc.org/2270403002/diff/120001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/BUILD.gn:1040: if (is_android || is_ios) { This change was done in upsteram (not part of this CL) https://codereview.webrtc.org/2270403002/diff/120001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/BUILD.gn:1574: Also change from upstream.
https://codereview.webrtc.org/2270403002/diff/80001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/BUILD.gn (right): https://codereview.webrtc.org/2270403002/diff/80001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/BUILD.gn:1617: "../../base:rtc_base", On 2016/08/29 09:39:10, aleloi wrote: > On 2016/08/26 14:07:37, kjellander_webrtc wrote: > > On 2016/08/24 11:02:51, aleloi wrote: > > > 'Forbidden' dependency; also added matching dependency in gyp files. > > > > Do you need rtc_base or is rtc_base_approved enough? (preferred) > > Yes, rtc_base is needed. Specifically webrtc/base/format_macros.h Actually, ivoc@ broke out format_macros.h from rtc_base, and now I can depend on base_approved. I uploaded a new patch set.
lgtm with nit and question. https://codereview.webrtc.org/2270403002/diff/120001/.gn File .gn (right): https://codereview.webrtc.org/2270403002/diff/120001/.gn#newcode30 .gn:30: "//webrtc/voice_engine:level_indicator", nit: sort alphabetically. https://codereview.webrtc.org/2270403002/diff/120001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/BUILD.gn (right): https://codereview.webrtc.org/2270403002/diff/120001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/BUILD.gn:1574: On 2016/08/29 10:21:34, aleloi wrote: > Also change from upstream. What do you mean with this comment?
https://codereview.webrtc.org/2270403002/diff/120001/.gn File .gn (right): https://codereview.webrtc.org/2270403002/diff/120001/.gn#newcode30 .gn:30: "//webrtc/voice_engine:level_indicator", On 2016/08/29 11:01:32, kjellander_webrtc wrote: > nit: sort alphabetically. Done. https://codereview.webrtc.org/2270403002/diff/120001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/BUILD.gn (right): https://codereview.webrtc.org/2270403002/diff/120001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/BUILD.gn:1574: On 2016/08/29 11:01:32, kjellander_webrtc wrote: > On 2016/08/29 10:21:34, aleloi wrote: > > Also change from upstream. > > What do you mean with this comment? I think I formulated it badly. The review system showed a diff between patch sets 1 and 2 that had nothing to do with my changes; it is because I uploaded 2 after a rebase on top of master.
The CQ bit was checked by aleloi@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from kjellander@webrtc.org Link to the patchset: https://codereview.webrtc.org/2270403002/#ps160001 (title: "Reorder.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
Message was sent while issue was closed.
Description was changed from ========== Migrated ILBC and ISAC test targets for GN. Migrated GN targets ilbc_test, isac_api_test, isac_switch_samprate_test from webrtc/modules/audio_coding/codecs NOTRY=True NOPRESUBMIT=True BUG=webrtc:6191 ========== to ========== Migrated ILBC and ISAC test targets for GN. Migrated GN targets ilbc_test, isac_api_test, isac_switch_samprate_test from webrtc/modules/audio_coding/codecs NOTRY=True NOPRESUBMIT=True BUG=webrtc:6191 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:160001)
Message was sent while issue was closed.
Description was changed from ========== Migrated ILBC and ISAC test targets for GN. Migrated GN targets ilbc_test, isac_api_test, isac_switch_samprate_test from webrtc/modules/audio_coding/codecs NOTRY=True NOPRESUBMIT=True BUG=webrtc:6191 ========== to ========== Migrated ILBC and ISAC test targets for GN. Migrated GN targets ilbc_test, isac_api_test, isac_switch_samprate_test from webrtc/modules/audio_coding/codecs NOTRY=True NOPRESUBMIT=True BUG=webrtc:6191 Committed: https://crrev.com/cfee215b23105775ec89f80162695bcb7cf4b073 Cr-Commit-Position: refs/heads/master@{#13953} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/cfee215b23105775ec89f80162695bcb7cf4b073 Cr-Commit-Position: refs/heads/master@{#13953} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
