Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1877713002:
Replaced the data logging functionality in the AEC with a generic logging functionality (Closed)
|
Created:
4 years, 8 months ago by peah-webrtc Modified:
4 years, 7 months ago CC:
webrtc-reviews_webrtc.org, peah-webrtc, Andrew MacDonald, tterriberry_mozilla.com, audio-team_agora.io, hlundin-webrtc, kwiberg-webrtc, minyue-webrtc, the sun, aluebs-webrtc, bjornv1 Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionThis CL introduces a new data logging functionality
to use for the APM. It allows simple and rapid
additions of exploratory data logpoints to use
during bug investigations and module performance
analysis.
The new data logging functionality is also in this CL
used to replace the existing data logging functionality
present in the AEC.
Additional information:
As there was an issue with that the build flag for
activating this feature was not present in all
compilation units that included the feature additional
changes were needed. A summary of the changes are
-The build files were modified to ensure that the
logging build flag always is set to either 0 or 1
for compilation units that include the feature.
-Build-time checks in the appropriate places were added
to ensure that the above is fulfilled.
-The build object was added dynamically to the AEC state
as a pointer to ensure that the size of that state is not
dependent on whether the logging build flag is set or not.
-The constructor of the AEC class needed to be modified in
order to construct the logging object. For this a destructor
was also needed.
-An unused method without any declaration was removed in
order to avoid any issues with the logging flag being set to
0 or 1.
This CL will be immediately followed with an upcoming CL
that replaces the logging in echo_cancellation.cc with the
new functionality which will ensure that the logging flag
is only used in one place within WebRTC, which in turn will
fully ensure that all compilation units that uses the feature
also have the flag properly set.
BUG=webrtc:5201, webrtc:5298
Committed: https://crrev.com/b46083ed6308b95f1e83570342dbe3eb0a54729e
Cr-Commit-Position: refs/heads/master@{#12607}
Patch Set 1 #
Total comments: 51
Patch Set 2 : Changes in response to reviewer comments. Also fixed an issue with the build files. #Patch Set 3 : Rebase with latest master #Patch Set 4 : Updated build files to handle NEON code properly #Patch Set 5 : Rebase with latest master #Patch Set 6 : Major changes in order to ensure proper handling of debug dumping in all parts of the code #Patch Set 7 : Removed unused function declaration #Patch Set 8 : Added a destructor for the AecCore struct #Patch Set 9 : Made the destructor out-of-line #
Total comments: 8
Patch Set 10 : Updated the buildfile according to reviewer comments #
Total comments: 4
Patch Set 11 : Changed construction of the filename and corrected the typo in the comment #
Total comments: 10
Patch Set 12 : Changes in response to latest reviewer comments #
Total comments: 4
Patch Set 13 : Introduced the proposed alternative map lookup #Patch Set 14 : Rebase with latest master #Messages
Total messages: 72 (35 generated)
Patchset #1 (id:1) has been deleted
Description was changed from ========== Replaced the logging BUG= ========== to ========== This CL introduces a new data logging functionality to use for the APM. It allows simple and rapid additions of exploratory data logpoints to use during bug investigations and module performance analysis. The new data logging functionality is also in this CL used to replace the existing data logging functionality present in the AEC. BUG=webrtc:5201, webrtc:5298 ==========
peah@webrtc.org changed reviewers: + henrik.lundin@webrtc.org, kwiberg@webrtc.org
https://codereview.webrtc.org/1877713002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/logging/apm_data_dumper.cc (right): https://codereview.webrtc.org/1877713002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/logging/apm_data_dumper.cc:28: } You can probably unify these two. https://codereview.webrtc.org/1877713002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/logging/apm_data_dumper.cc:79: fwrite(v.data(), sizeof(v[0]), v.size(), file); Do floats suffer from the same endian problems as integers, or do they always use the same byte order? https://codereview.webrtc.org/1877713002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/logging/apm_data_dumper.cc:87: #endif Implement one DumpRaw in terms of the other? https://codereview.webrtc.org/1877713002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/logging/apm_data_dumper.cc:106: #endif Implement one DumpWav in terms of the other? https://codereview.webrtc.org/1877713002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/logging/apm_data_dumper.h (right): https://codereview.webrtc.org/1877713002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/logging/apm_data_dumper.h:35: void Initialize(); The name of this function suggests that it has to be called before the object is ready. If that's the case, I'd like to object. If it's not the case, I'd like to suggest a different name. https://codereview.webrtc.org/1877713002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/logging/apm_data_dumper.h:47: int sample_rate_hz); By making these methods not inline, you'll prevent the compiler from omitting the calls altogether when not logging. https://codereview.webrtc.org/1877713002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/logging/apm_data_dumper.h:49: private: Consider #ifdeffing the members away too. https://codereview.webrtc.org/1877713002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/logging/apm_data_dumper.h:55: FILE* GetRawFile(const std::string& name); You don't include anything that provides FILE except by accident. https://codereview.webrtc.org/1877713002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/logging/apm_data_dumper.h:56: rtc_WavWriter* GetWavFile(const std::string& name, int sample_rate_hz); It looks like you're using the C API for the wav writer. Use the C++ API instead.
https://codereview.webrtc.org/1877713002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/logging/apm_data_dumper.cc (right): https://codereview.webrtc.org/1877713002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/logging/apm_data_dumper.cc:16: std::string FormRawFileName(std::string name, const std::string& https://codereview.webrtc.org/1877713002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/logging/apm_data_dumper.cc:19: return name + "_" + std::to_string(instance_index) + "-" + std::to_string is currently not allowed in Chrome. See https://chromium-cpp.appspot.com/#library-review. https://codereview.webrtc.org/1877713002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/logging/apm_data_dumper.cc:28: } On 2016/04/11 10:43:37, kwiberg-webrtc wrote: > You can probably unify these two. Agree. I see (at least) two options: 1. std::string MakeFileName(std::string name, int instance_index, int reinit_index, const std::string& suffix) {...} called as MakeFileName("basename", 17, 4711, "dat"); Or 2. std::string MakeFileName(std::string name, int instance_index, int reinit_index) {...} called as MakeFileName("basename", 17, 4711) + ".dat"; https://codereview.webrtc.org/1877713002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/logging/apm_data_dumper.cc:39: FILE** file = &raw_files_element.second; Can you not just call fclose(raw_files_element.second); ? https://codereview.webrtc.org/1877713002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/logging/apm_data_dumper.cc:44: rtc_WavWriter** file = &wav_files_element.second; Similar as above? https://codereview.webrtc.org/1877713002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/logging/apm_data_dumper.cc:76: rtc::ArrayView<const float> v) { Is "pass-by-value" just as good as passing const rtc::ArrayView<const float>& v ? https://codereview.webrtc.org/1877713002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/logging/apm_data_dumper.cc:83: void ApmDataDumper::DumpRaw(const std::string& name, int v_length, float* v) { Order of methods should be the same in h and cpp files. https://codereview.webrtc.org/1877713002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/logging/apm_data_dumper.cc:83: void ApmDataDumper::DumpRaw(const std::string& name, int v_length, float* v) { const float* v https://codereview.webrtc.org/1877713002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/logging/apm_data_dumper.cc:87: #endif On 2016/04/11 10:43:37, kwiberg-webrtc wrote: > Implement one DumpRaw in terms of the other? Acknowledged. https://codereview.webrtc.org/1877713002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/logging/apm_data_dumper.cc:99: void ApmDataDumper::DumpWav(const std::string& name, Order of methods should be the same in h and cpp files. https://codereview.webrtc.org/1877713002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/logging/apm_data_dumper.cc:101: float* v, const float* v https://codereview.webrtc.org/1877713002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/logging/apm_data_dumper.h (right): https://codereview.webrtc.org/1877713002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/logging/apm_data_dumper.h:18: #include "webrtc/base/array_view.h" Order of includes. https://codereview.webrtc.org/1877713002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/logging/apm_data_dumper.h:35: void Initialize(); On 2016/04/11 10:43:37, kwiberg-webrtc wrote: > The name of this function suggests that it has to be called before the object is > ready. If that's the case, I'd like to object. If it's not the case, I'd like to > suggest a different name. Acknowledged. https://codereview.webrtc.org/1877713002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/logging/apm_data_dumper.h:55: FILE* GetRawFile(const std::string& name); On 2016/04/11 10:43:37, kwiberg-webrtc wrote: > You don't include anything that provides FILE except by accident. Forward declare?
https://codereview.webrtc.org/1877713002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/logging/apm_data_dumper.cc (right): https://codereview.webrtc.org/1877713002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/logging/apm_data_dumper.cc:16: std::string FormRawFileName(std::string name, On 2016/04/11 11:58:29, hlundin-webrtc wrote: > const std::string& +1. Good point. https://codereview.webrtc.org/1877713002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/logging/apm_data_dumper.cc:76: rtc::ArrayView<const float> v) { On 2016/04/11 11:58:29, hlundin-webrtc wrote: > Is "pass-by-value" just as good as passing const rtc::ArrayView<const float>& v > ? Yes (ArrayView is just a pointer + a length), and IIRC, the docs for that class even say so explicitly.
Patchset #2 (id:40001) has been deleted
Hi, Thanks for the great comments. I have now addressed them in a new patch-set where I also correct an issue with the build files where the DEBUG_DUMP... #define was not set for the sse2 optimized aec code. It would be awesome if you could have a close look at what I did there. https://codereview.webrtc.org/1877713002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/logging/apm_data_dumper.cc (right): https://codereview.webrtc.org/1877713002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/logging/apm_data_dumper.cc:16: std::string FormRawFileName(std::string name, On 2016/04/11 11:58:29, hlundin-webrtc wrote: > const std::string& Good find! Done. https://codereview.webrtc.org/1877713002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/logging/apm_data_dumper.cc:16: std::string FormRawFileName(std::string name, On 2016/04/11 12:59:11, kwiberg-webrtc wrote: > On 2016/04/11 11:58:29, hlundin-webrtc wrote: > > const std::string& > > +1. Good point. Acknowledged. https://codereview.webrtc.org/1877713002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/logging/apm_data_dumper.cc:19: return name + "_" + std::to_string(instance_index) + "-" + On 2016/04/11 11:58:29, hlundin-webrtc wrote: > std::to_string is currently not allowed in Chrome. See > https://chromium-cpp.appspot.com/#library-review. Ah, great find! Too bad. Will do this in another way instead. Done. https://codereview.webrtc.org/1877713002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/logging/apm_data_dumper.cc:28: } On 2016/04/11 10:43:37, kwiberg-webrtc wrote: > You can probably unify these two. Done. https://codereview.webrtc.org/1877713002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/logging/apm_data_dumper.cc:28: } On 2016/04/11 11:58:29, hlundin-webrtc wrote: > On 2016/04/11 10:43:37, kwiberg-webrtc wrote: > > You can probably unify these two. > > Agree. I see (at least) two options: > 1. std::string MakeFileName(std::string name, > int instance_index, > int reinit_index, > const std::string& suffix) {...} > called as MakeFileName("basename", 17, 4711, "dat"); > > Or > 2. std::string MakeFileName(std::string name, > int instance_index, > int reinit_index) {...} > called as MakeFileName("basename", 17, 4711) + ".dat"; Done. https://codereview.webrtc.org/1877713002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/logging/apm_data_dumper.cc:39: FILE** file = &raw_files_element.second; On 2016/04/11 11:58:29, hlundin-webrtc wrote: > Can you not just call > fclose(raw_files_element.second); > ? True. Done. https://codereview.webrtc.org/1877713002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/logging/apm_data_dumper.cc:44: rtc_WavWriter** file = &wav_files_element.second; On 2016/04/11 11:58:29, hlundin-webrtc wrote: > Similar as above? Done. https://codereview.webrtc.org/1877713002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/logging/apm_data_dumper.cc:76: rtc::ArrayView<const float> v) { On 2016/04/11 11:58:29, hlundin-webrtc wrote: > Is "pass-by-value" just as good as passing const rtc::ArrayView<const float>& v > ? Acknowledged. https://codereview.webrtc.org/1877713002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/logging/apm_data_dumper.cc:76: rtc::ArrayView<const float> v) { On 2016/04/11 12:59:11, kwiberg-webrtc wrote: > On 2016/04/11 11:58:29, hlundin-webrtc wrote: > > Is "pass-by-value" just as good as passing const rtc::ArrayView<const float>& > v > > ? > > Yes (ArrayView is just a pointer + a length), and IIRC, the docs for that class > even say so explicitly. Acknowledged. https://codereview.webrtc.org/1877713002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/logging/apm_data_dumper.cc:79: fwrite(v.data(), sizeof(v[0]), v.size(), file); On 2016/04/11 10:43:36, kwiberg-webrtc wrote: > Do floats suffer from the same endian problems as integers, or do they always > use the same byte order? Yes, they do suffer from the same problem. But I don't think that is a problem here as these files are only to be generated in an offline manner and will never need to be converted to another endianness. https://codereview.webrtc.org/1877713002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/logging/apm_data_dumper.cc:83: void ApmDataDumper::DumpRaw(const std::string& name, int v_length, float* v) { On 2016/04/11 11:58:28, hlundin-webrtc wrote: > const float* v Done. https://codereview.webrtc.org/1877713002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/logging/apm_data_dumper.cc:83: void ApmDataDumper::DumpRaw(const std::string& name, int v_length, float* v) { On 2016/04/11 11:58:29, hlundin-webrtc wrote: > Order of methods should be the same in h and cpp files. Good point! I now anyway moved these to the header file in order to allow the possibility of inlining. Done. https://codereview.webrtc.org/1877713002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/logging/apm_data_dumper.cc:87: #endif On 2016/04/11 10:43:37, kwiberg-webrtc wrote: > Implement one DumpRaw in terms of the other? Done. https://codereview.webrtc.org/1877713002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/logging/apm_data_dumper.cc:87: #endif On 2016/04/11 11:58:29, hlundin-webrtc wrote: > On 2016/04/11 10:43:37, kwiberg-webrtc wrote: > > Implement one DumpRaw in terms of the other? > > Acknowledged. Done. https://codereview.webrtc.org/1877713002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/logging/apm_data_dumper.cc:99: void ApmDataDumper::DumpWav(const std::string& name, On 2016/04/11 11:58:29, hlundin-webrtc wrote: > Order of methods should be the same in h and cpp files. Good point! I now anyway moved these to the header file in order to allow the possibility of inlining. Done. https://codereview.webrtc.org/1877713002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/logging/apm_data_dumper.cc:101: float* v, On 2016/04/11 11:58:29, hlundin-webrtc wrote: > const float* v Done. https://codereview.webrtc.org/1877713002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/logging/apm_data_dumper.cc:106: #endif On 2016/04/11 10:43:37, kwiberg-webrtc wrote: > Implement one DumpWav in terms of the other? Done. https://codereview.webrtc.org/1877713002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/logging/apm_data_dumper.h (right): https://codereview.webrtc.org/1877713002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/logging/apm_data_dumper.h:18: #include "webrtc/base/array_view.h" On 2016/04/11 11:58:29, hlundin-webrtc wrote: > Order of includes. Done. https://codereview.webrtc.org/1877713002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/logging/apm_data_dumper.h:35: void Initialize(); On 2016/04/11 10:43:37, kwiberg-webrtc wrote: > The name of this function suggests that it has to be called before the object is > ready. If that's the case, I'd like to object. If it's not the case, I'd like to > suggest a different name. Good point! Did not think of that. I modified this a bit. PTAL. https://codereview.webrtc.org/1877713002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/logging/apm_data_dumper.h:35: void Initialize(); On 2016/04/11 11:58:29, hlundin-webrtc wrote: > On 2016/04/11 10:43:37, kwiberg-webrtc wrote: > > The name of this function suggests that it has to be called before the object > is > > ready. If that's the case, I'd like to object. If it's not the case, I'd like > to > > suggest a different name. > > Acknowledged. Done. https://codereview.webrtc.org/1877713002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/logging/apm_data_dumper.h:47: int sample_rate_hz); On 2016/04/11 10:43:37, kwiberg-webrtc wrote: > By making these methods not inline, you'll prevent the compiler from omitting > the calls altogether when not logging. Good point! Done. https://codereview.webrtc.org/1877713002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/logging/apm_data_dumper.h:49: private: On 2016/04/11 10:43:37, kwiberg-webrtc wrote: > Consider #ifdeffing the members away too. Done. https://codereview.webrtc.org/1877713002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/logging/apm_data_dumper.h:55: FILE* GetRawFile(const std::string& name); On 2016/04/11 10:43:37, kwiberg-webrtc wrote: > You don't include anything that provides FILE except by accident. Good find! https://codereview.webrtc.org/1877713002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/logging/apm_data_dumper.h:55: FILE* GetRawFile(const std::string& name); On 2016/04/11 11:58:29, hlundin-webrtc wrote: > On 2016/04/11 10:43:37, kwiberg-webrtc wrote: > > You don't include anything that provides FILE except by accident. > > Forward declare? I'm not sure what you mean with this. https://codereview.webrtc.org/1877713002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/logging/apm_data_dumper.h:56: rtc_WavWriter* GetWavFile(const std::string& name, int sample_rate_hz); On 2016/04/11 10:43:37, kwiberg-webrtc wrote: > It looks like you're using the C API for the wav writer. Use the C++ API > instead. Good point! Done.
The CQ bit was checked by peah@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1877713002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1877713002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios32_sim_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios32_sim_dbg/builds/6618) ios64_sim_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios64_sim_dbg/builds/6609) ios_arm64_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_dbg/builds/8941) ios_arm64_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_rel/builds/8861) ios_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_dbg/builds/14149) ios_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_rel/builds/12784) linux_libfuzzer_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_libfuzzer_rel/bui...) linux_tsan2 on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_tsan2/builds/11288) linux_ubsan on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_ubsan/builds/1175) mac_asan on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_asan/builds/13783) mac_baremetal on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_baremetal/builds/10498) mac_compile_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_compile_dbg/builds/...) mac_gn_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_gn_dbg/builds/2258) mac_gn_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_gn_rel/builds/8484) mac_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_rel/builds/14208) win_drmemory_light on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_drmemory_light/buil...) win_x64_clang_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_x64_clang_rel/build...)
The CQ bit was checked by peah@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1877713002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1877713002/80001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_rel/builds/12792) mac_gn_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_gn_dbg/builds/2267)
Please hold on for some more hours before reviewing this. There were some build file related issues on the try bots that I need to resolve.
The CQ bit was checked by peah@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1877713002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1877713002/100001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios_arm64_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_rel/builds/8920) mac_asan on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_asan/builds/13842) mac_compile_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_compile_dbg/builds/...) mac_gn_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_gn_dbg/builds/2320) mac_gn_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_gn_rel/builds/8547)
The CQ bit was checked by peah@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1877713002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1877713002/120001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_arm64_rel/build...) android_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_dbg/builds/12476) ios_arm64_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_rel/builds/8921) ios_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_dbg/builds/14211)
The CQ bit was checked by peah@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1877713002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1877713002/140001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_rel/builds/12852)
Patchset #8 (id:180001) has been deleted
Patchset #8 (id:200001) has been deleted
Patchset #9 (id:230001) has been deleted
The CQ bit was checked by peah@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1877713002/330001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1877713002/330001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_gn_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_gn_dbg/builds/2571) mac_gn_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_gn_rel/builds/8798)
Patchset #11 (id:290001) has been deleted
The CQ bit was checked by peah@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1877713002/370001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1877713002/370001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_rel on tryserver.webrtc (JOB_TIMED_OUT, no build URL)
Patchset #3 (id:80001) has been deleted
Patchset #7 (id:190015) has been deleted
Patchset #3 (id:100001) has been deleted
Patchset #5 (id:160001) has been deleted
Patchset #6 (id:270001) has been deleted
Description was changed from ========== This CL introduces a new data logging functionality to use for the APM. It allows simple and rapid additions of exploratory data logpoints to use during bug investigations and module performance analysis. The new data logging functionality is also in this CL used to replace the existing data logging functionality present in the AEC. BUG=webrtc:5201, webrtc:5298 ========== to ========== This CL introduces a new data logging functionality to use for the APM. It allows simple and rapid additions of exploratory data logpoints to use during bug investigations and module performance analysis. The new data logging functionality is also in this CL used to replace the existing data logging functionality present in the AEC. Additional information: As there was an issue with that the build flag for activating this feature was not present in all compilation units that included the feature additional changes were needed. A summary of the changes are -The build files were modified to ensure that the logging build flag always is set to either 0 or 1 for compilation units that include the feature. -Build-time checks in the appropriate places were added to ensure that the above is fulfilled. -The build object was added dynamically to the AEC state as a pointer to ensure that the size of that state is not dependent on whether the logging build flag is set or not. -The constructor of the AEC class needed to be modified in order to construct the logging object. For this a destructor was also needed. -An unused method without any declaration was removed in order to avoid any issues with the logging flag being set to 0 or 1. This CL will be immediately followed with an upcoming CL that replaces the logging in echo_cancellation.cc with the new functionality which will ensure that the logging flag is only used in one place within WebRTC, which in turn will fully ensure that all compilation units that uses the feature also have the flag properly set. BUG=webrtc:5201, webrtc:5298 ==========
This CL is again ready for review.
peah@webrtc.org changed reviewers: + kjellander@webrtc.org
+ @kjellander Could you please have a look at the build file changes that are introduced by this CL?
generally looks good but webrtc/modules/audio_processing/audio_processing.gypi needs a little work. https://codereview.webrtc.org/1877713002/diff/370001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/audio_processing.gypi (right): https://codereview.webrtc.org/1877713002/diff/370001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing.gypi:251: 'aec_debug_dump%': 0, Can you try to move the declaration of 'aec_debug_dump' in the 'audio_processing' target out up to the top-level variables scope instead? Then I hope you can remove this and the one in the _neon target. https://codereview.webrtc.org/1877713002/diff/370001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing.gypi:259: 'defines': ['WEBRTC_AEC_DEBUG_DUMP=1',], +2 spaces indent (applies to rest of file as well). https://codereview.webrtc.org/1877713002/diff/370001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing.gypi:261: ['aec_debug_dump==0', { You can write this as: ['aec_debug_dump==1', { 'defines': ['WEBRTC_AEC_DEBUG_DUMP=1',], }, { 'defines': ['WEBRTC_AEC_DEBUG_DUMP=0',], }], i.e. there is something like else-functionality possible in GYP. https://codereview.webrtc.org/1877713002/diff/370001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing.gypi:295: }], Use 'else' clause instead.
https://codereview.webrtc.org/1877713002/diff/370001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/audio_processing.gypi (right): https://codereview.webrtc.org/1877713002/diff/370001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing.gypi:251: 'aec_debug_dump%': 0, On 2016/04/25 05:46:40, kjellander (webrtc) wrote: > Can you try to move the declaration of 'aec_debug_dump' in the > 'audio_processing' target out up to the top-level variables scope instead? Then > I hope you can remove this and the one in the _neon target. Good suggestion! It worked! Done. https://codereview.webrtc.org/1877713002/diff/370001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing.gypi:259: 'defines': ['WEBRTC_AEC_DEBUG_DUMP=1',], On 2016/04/25 05:46:40, kjellander (webrtc) wrote: > +2 spaces indent (applies to rest of file as well). Done. https://codereview.webrtc.org/1877713002/diff/370001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing.gypi:261: ['aec_debug_dump==0', { On 2016/04/25 05:46:39, kjellander (webrtc) wrote: > You can write this as: > ['aec_debug_dump==1', { > 'defines': ['WEBRTC_AEC_DEBUG_DUMP=1',], > }, { > 'defines': ['WEBRTC_AEC_DEBUG_DUMP=0',], > }], > > i.e. there is something like else-functionality possible in GYP. Great! It became much nicer with that way of doing it! Done. https://codereview.webrtc.org/1877713002/diff/370001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing.gypi:295: }], On 2016/04/25 05:46:40, kjellander (webrtc) wrote: > Use 'else' clause instead. Done.
LG, just a few minor comments. https://codereview.webrtc.org/1877713002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/logging/apm_data_dumper.h (right): https://codereview.webrtc.org/1877713002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/logging/apm_data_dumper.h:55: FILE* GetRawFile(const std::string& name); On 2016/04/12 21:46:39, peah-webrtc wrote: > On 2016/04/11 11:58:29, hlundin-webrtc wrote: > > On 2016/04/11 10:43:37, kwiberg-webrtc wrote: > > > You don't include anything that provides FILE except by accident. > > > > Forward declare? > > I'm not sure what you mean with this. What I think I meant was that since your previous version only referenced this as a pointer in the .h file, it would suffice to forward declare it here, and then do the full include in the .cc file. But now that is water under the bridges. https://codereview.webrtc.org/1877713002/diff/390001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/logging/apm_data_dumper.cc (right): https://codereview.webrtc.org/1877713002/diff/390001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/logging/apm_data_dumper.cc:30: char instance_index_string[10]; I think the C++ way of doing this (in the absence of std::to_string) is to use a stringstream; see for instance https://code.google.com/p/chromium/codesearch#chromium/src/third_party/webrtc.... https://codereview.webrtc.org/1877713002/diff/390001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/logging/apm_data_dumper.h (right): https://codereview.webrtc.org/1877713002/diff/390001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/logging/apm_data_dumper.h:46: // Reitializes the data dumping such that new versions Reinitializes
https://codereview.webrtc.org/1877713002/diff/390001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/logging/apm_data_dumper.cc (right): https://codereview.webrtc.org/1877713002/diff/390001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/logging/apm_data_dumper.cc:30: char instance_index_string[10]; On 2016/04/25 11:29:59, hlundin-webrtc wrote: > I think the C++ way of doing this (in the absence of std::to_string) is to use a > stringstream; see for instance > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/webrtc.... Great! Thanks!! With that the code became much nicer! Done. https://codereview.webrtc.org/1877713002/diff/390001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/logging/apm_data_dumper.h (right): https://codereview.webrtc.org/1877713002/diff/390001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/logging/apm_data_dumper.h:46: // Reitializes the data dumping such that new versions On 2016/04/25 11:29:59, hlundin-webrtc wrote: > Reinitializes Thanks! Done.
lgtm
*.gypi,*.gn: lgtm
https://codereview.webrtc.org/1877713002/diff/410001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/aec/aec_core.cc (right): https://codereview.webrtc.org/1877713002/diff/410001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/aec/aec_core.cc:1306: aec->data_dumper->DumpWav("aec_far", PART_LEN, &farend_ptr[PART_LEN], At each of these call sites, the compiler is converting the const char* to a temporary std::string for you, even if the debug dumping is switched off. I suspect the compiler may not even be allowed to optimize that away, since the heap allocations can have the side effect of exhausting memory. Make the dump methods take const char* instead, and convert to std::string in the #if-protected method bodies if necessary. https://codereview.webrtc.org/1877713002/diff/410001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/aec/aec_core.cc:1309: aec->sampFreq > 16000 ? 16000 : aec->sampFreq, 1); std::min(aec->sampFreq, 16000) (Also in several places below.) https://codereview.webrtc.org/1877713002/diff/410001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/logging/apm_data_dumper.cc (right): https://codereview.webrtc.org/1877713002/diff/410001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/logging/apm_data_dumper.cc:50: #endif If you use unique_ptrs, the manual destructor won't be necessary. https://codereview.webrtc.org/1877713002/diff/410001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/logging/apm_data_dumper.h (right): https://codereview.webrtc.org/1877713002/diff/410001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/logging/apm_data_dumper.h:80: void DumpMonoWav(const std::string& name, What's mono about this method? https://codereview.webrtc.org/1877713002/diff/410001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/logging/apm_data_dumper.h:94: std::map<std::string, WavWriter*> wav_files_; As far as I can tell, these two maps own their values using raw pointers. Seriously consider using unique_ptr instead. (For the first one, it looks like you'll have to use a custom deleter.) Also, consider using std::unordered_map instead, since you don't need the keys to be ordered.
https://codereview.webrtc.org/1877713002/diff/410001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/aec/aec_core.cc (right): https://codereview.webrtc.org/1877713002/diff/410001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/aec/aec_core.cc:1306: aec->data_dumper->DumpWav("aec_far", PART_LEN, &farend_ptr[PART_LEN], On 2016/05/03 00:53:00, kwiberg-webrtc wrote: > At each of these call sites, the compiler is converting the const char* to a > temporary std::string for you, even if the debug dumping is switched off. I > suspect the compiler may not even be allowed to optimize that away, since the > heap allocations can have the side effect of exhausting memory. > > Make the dump methods take const char* instead, and convert to std::string in > the #if-protected method bodies if necessary. Great point!!! Done. https://codereview.webrtc.org/1877713002/diff/410001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/aec/aec_core.cc:1309: aec->sampFreq > 16000 ? 16000 : aec->sampFreq, 1); On 2016/05/03 00:53:01, kwiberg-webrtc wrote: > std::min(aec->sampFreq, 16000) > > (Also in several places below.) Done. https://codereview.webrtc.org/1877713002/diff/410001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/logging/apm_data_dumper.cc (right): https://codereview.webrtc.org/1877713002/diff/410001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/logging/apm_data_dumper.cc:50: #endif On 2016/05/03 00:53:01, kwiberg-webrtc wrote: > If you use unique_ptrs, the manual destructor won't be necessary. Thanks! The change allowed removing the constructor completely. https://codereview.webrtc.org/1877713002/diff/410001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/logging/apm_data_dumper.h (right): https://codereview.webrtc.org/1877713002/diff/410001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/logging/apm_data_dumper.h:80: void DumpMonoWav(const std::string& name, On 2016/05/03 00:53:01, kwiberg-webrtc wrote: > What's mono about this method? Nothing, that should be removed. Done. https://codereview.webrtc.org/1877713002/diff/410001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/logging/apm_data_dumper.h:94: std::map<std::string, WavWriter*> wav_files_; On 2016/05/03 00:53:01, kwiberg-webrtc wrote: > As far as I can tell, these two maps own their values using raw pointers. > Seriously consider using unique_ptr instead. (For the first one, it looks like > you'll have to use a custom deleter.) > > Also, consider using std::unordered_map instead, since you don't need the keys > to be ordered. Great! Nice!!! I ended up with a functor as a custom deleter. That was the only option I got working for this. I also made some slight modifications to the GetRawFile and GetWavFile in order to achieve a single return points as these functions anyway needed some changes due to this. PTAL!
lgtm, but see addtional comments. https://codereview.webrtc.org/1877713002/diff/430001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/logging/apm_data_dumper.cc (right): https://codereview.webrtc.org/1877713002/diff/430001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/logging/apm_data_dumper.cc:48: return raw_files_[filename].get(); You do 2-3 map lookups here. Try something like auto& f = raw_files_[filename]; if (!f) { f.reset(fopen(filename.c_str(), "wb")); } return f.get(); This assumes that all map values are non-null, but that's the case, right? https://codereview.webrtc.org/1877713002/diff/430001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/logging/apm_data_dumper.cc:61: return wav_files_[filename].get(); Same here.
https://codereview.webrtc.org/1877713002/diff/430001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/logging/apm_data_dumper.cc (right): https://codereview.webrtc.org/1877713002/diff/430001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/logging/apm_data_dumper.cc:48: return raw_files_[filename].get(); On 2016/05/03 09:02:50, kwiberg-webrtc wrote: > You do 2-3 map lookups here. Try something like > > auto& f = raw_files_[filename]; > if (!f) { > f.reset(fopen(filename.c_str(), "wb")); > } > return f.get(); > > This assumes that all map values are non-null, but that's the case, right? Awesome suggestion! Yes, they are all non-null. Done. https://codereview.webrtc.org/1877713002/diff/430001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/logging/apm_data_dumper.cc:61: return wav_files_[filename].get(); On 2016/05/03 09:02:50, kwiberg-webrtc wrote: > Same here. Done.
The CQ bit was checked by peah@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from henrik.lundin@webrtc.org, kjellander@webrtc.org, kwiberg@webrtc.org Link to the patchset: https://codereview.webrtc.org/1877713002/#ps470001 (title: "Rebase with latest master")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1877713002/470001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1877713002/470001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_arm64_rel on tryserver.webrtc (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by peah@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1877713002/470001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1877713002/470001
Message was sent while issue was closed.
Description was changed from ========== This CL introduces a new data logging functionality to use for the APM. It allows simple and rapid additions of exploratory data logpoints to use during bug investigations and module performance analysis. The new data logging functionality is also in this CL used to replace the existing data logging functionality present in the AEC. Additional information: As there was an issue with that the build flag for activating this feature was not present in all compilation units that included the feature additional changes were needed. A summary of the changes are -The build files were modified to ensure that the logging build flag always is set to either 0 or 1 for compilation units that include the feature. -Build-time checks in the appropriate places were added to ensure that the above is fulfilled. -The build object was added dynamically to the AEC state as a pointer to ensure that the size of that state is not dependent on whether the logging build flag is set or not. -The constructor of the AEC class needed to be modified in order to construct the logging object. For this a destructor was also needed. -An unused method without any declaration was removed in order to avoid any issues with the logging flag being set to 0 or 1. This CL will be immediately followed with an upcoming CL that replaces the logging in echo_cancellation.cc with the new functionality which will ensure that the logging flag is only used in one place within WebRTC, which in turn will fully ensure that all compilation units that uses the feature also have the flag properly set. BUG=webrtc:5201, webrtc:5298 ========== to ========== This CL introduces a new data logging functionality to use for the APM. It allows simple and rapid additions of exploratory data logpoints to use during bug investigations and module performance analysis. The new data logging functionality is also in this CL used to replace the existing data logging functionality present in the AEC. Additional information: As there was an issue with that the build flag for activating this feature was not present in all compilation units that included the feature additional changes were needed. A summary of the changes are -The build files were modified to ensure that the logging build flag always is set to either 0 or 1 for compilation units that include the feature. -Build-time checks in the appropriate places were added to ensure that the above is fulfilled. -The build object was added dynamically to the AEC state as a pointer to ensure that the size of that state is not dependent on whether the logging build flag is set or not. -The constructor of the AEC class needed to be modified in order to construct the logging object. For this a destructor was also needed. -An unused method without any declaration was removed in order to avoid any issues with the logging flag being set to 0 or 1. This CL will be immediately followed with an upcoming CL that replaces the logging in echo_cancellation.cc with the new functionality which will ensure that the logging flag is only used in one place within WebRTC, which in turn will fully ensure that all compilation units that uses the feature also have the flag properly set. BUG=webrtc:5201, webrtc:5298 ==========
Message was sent while issue was closed.
Committed patchset #14 (id:470001)
Message was sent while issue was closed.
Description was changed from ========== This CL introduces a new data logging functionality to use for the APM. It allows simple and rapid additions of exploratory data logpoints to use during bug investigations and module performance analysis. The new data logging functionality is also in this CL used to replace the existing data logging functionality present in the AEC. Additional information: As there was an issue with that the build flag for activating this feature was not present in all compilation units that included the feature additional changes were needed. A summary of the changes are -The build files were modified to ensure that the logging build flag always is set to either 0 or 1 for compilation units that include the feature. -Build-time checks in the appropriate places were added to ensure that the above is fulfilled. -The build object was added dynamically to the AEC state as a pointer to ensure that the size of that state is not dependent on whether the logging build flag is set or not. -The constructor of the AEC class needed to be modified in order to construct the logging object. For this a destructor was also needed. -An unused method without any declaration was removed in order to avoid any issues with the logging flag being set to 0 or 1. This CL will be immediately followed with an upcoming CL that replaces the logging in echo_cancellation.cc with the new functionality which will ensure that the logging flag is only used in one place within WebRTC, which in turn will fully ensure that all compilation units that uses the feature also have the flag properly set. BUG=webrtc:5201, webrtc:5298 ========== to ========== This CL introduces a new data logging functionality to use for the APM. It allows simple and rapid additions of exploratory data logpoints to use during bug investigations and module performance analysis. The new data logging functionality is also in this CL used to replace the existing data logging functionality present in the AEC. Additional information: As there was an issue with that the build flag for activating this feature was not present in all compilation units that included the feature additional changes were needed. A summary of the changes are -The build files were modified to ensure that the logging build flag always is set to either 0 or 1 for compilation units that include the feature. -Build-time checks in the appropriate places were added to ensure that the above is fulfilled. -The build object was added dynamically to the AEC state as a pointer to ensure that the size of that state is not dependent on whether the logging build flag is set or not. -The constructor of the AEC class needed to be modified in order to construct the logging object. For this a destructor was also needed. -An unused method without any declaration was removed in order to avoid any issues with the logging flag being set to 0 or 1. This CL will be immediately followed with an upcoming CL that replaces the logging in echo_cancellation.cc with the new functionality which will ensure that the logging flag is only used in one place within WebRTC, which in turn will fully ensure that all compilation units that uses the feature also have the flag properly set. BUG=webrtc:5201, webrtc:5298 Committed: https://crrev.com/b46083ed6308b95f1e83570342dbe3eb0a54729e Cr-Commit-Position: refs/heads/master@{#12607} ==========
Message was sent while issue was closed.
Patchset 14 (id:??) landed as https://crrev.com/b46083ed6308b95f1e83570342dbe3eb0a54729e Cr-Commit-Position: refs/heads/master@{#12607} |
