Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1783203002:
Bitexactness test for the noise suppressor (Closed)
|
Created:
4 years, 9 months ago by peah-webrtc Modified:
4 years, 9 months ago Reviewers:
hlundin-webrtc 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. |
DescriptionAdded a bitexactness test for the noise suppressor.
This CL also extracts part of the functionality used
in the bitexactness test for the high-pass filter into
a separate file in order to be able to reuse that
functionality in bitexactness tests for the other
submodules in APM (including the bitexactness test for
the noise suppressor).
BUG=wertc:5336
Committed: https://crrev.com/5585001e5d3f867753752a8569a4f778636c583d
Cr-Commit-Position: refs/heads/master@{#12061}
Patch Set 1 #Patch Set 2 : Added missing include #Patch Set 3 : Corrected include #Patch Set 4 : Changed format to be float in order to avoid implicit conversion #Patch Set 5 : Added missing variable initialization #Patch Set 6 : Added all tests #Patch Set 7 : Minor modifications #Patch Set 8 : Update the unit test reference values #Patch Set 9 : Added correct test vectors for Android #Patch Set 10 : Corrected comments #
Total comments: 51
Patch Set 11 : Changes in response to reviewer comments, and harmonize the code with the other bitexactness tests #Patch Set 12 : Corrected usage of ArrayView and changed to using a constant for the number of frames to process #
Total comments: 11
Patch Set 13 : Changes in response to reviewer comments" #
Total comments: 2
Patch Set 14 : Changed to use CHECK #Patch Set 15 : Rebase #Patch Set 16 : Rebase #Patch Set 17 : Added explicit truncation to float #Patch Set 18 : Added new reference vectors for Android #Patch Set 19 : Rebase #Patch Set 20 : Corrected syntax error #Patch Set 21 : Fixed another Android specific code syntax error #Patch Set 22 : Updated testvectors for Android #Patch Set 23 : Fixed typo #Patch Set 24 : Removed accidental code change #Patch Set 25 : Removed effects from bad merge #Patch Set 26 : Corrected reference value for Android #Patch Set 27 : Furher Android bitexactness test changes #Patch Set 28 : Corrected preprocessor directive #Patch Set 29 : Fixed syntax error #Messages
Total messages: 80 (41 generated)
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/1783203002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1783203002/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_dbg/builds/11626) mac_compile_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_compile_dbg/builds/...)
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/1783203002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1783203002/20001
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/11627)
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/1783203002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1783203002/100001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_rel/builds/11646)
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/1783203002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1783203002/120001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_dbg/builds/11686) linux_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_rel/builds/13477)
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/1783203002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1783203002/140001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_rel/builds/11742)
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/1783203002/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1783203002/160001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_baremetal on tryserver.webrtc (JOB_TIMED_OUT, no build URL)
Description was changed from ========== Bitexactness test for the noise suppressor BUG=wertc:5336 ========== to ========== Added a bitexactness test for the noise suppressor. This CL also extracts part of the functionality used in the bitexactness test for the high-pass filter into a separate file in order to be able to reuse that functionality in bitexactness tests for the other submodules in APM (including the bitexactness test for the noise suppressor). BUG=wertc:5336 ==========
peah@webrtc.org changed reviewers: + henrik.lundin@webrtc.org
https://codereview.webrtc.org/1783203002/diff/180001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/noise_suppression_bitexactness_unittest.cc (right): https://codereview.webrtc.org/1783203002/diff/180001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/noise_suppression_bitexactness_unittest.cc:2: * Copyright (c) 2015 The WebRTC project authors. All Rights Reserved. 2016 https://codereview.webrtc.org/1783203002/diff/180001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/noise_suppression_bitexactness_unittest.cc:44: // Forms a predefined random test vector- End with '.', not '-'. https://codereview.webrtc.org/1783203002/diff/180001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/noise_suppression_bitexactness_unittest.cc:44: // Forms a predefined random test vector- Please, tell me what the range of the samples is. https://codereview.webrtc.org/1783203002/diff/180001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/noise_suppression_bitexactness_unittest.cc:45: void ConstructTestVector(int samples_per_channel, Make all int parameters size_t. https://codereview.webrtc.org/1783203002/diff/180001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/noise_suppression_bitexactness_unittest.cc:52: bool low_level = ((frame_counter / 10) > 5); Why not bool low_level = (frame_counter >= 60); ? https://codereview.webrtc.org/1783203002/diff/180001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/noise_suppression_bitexactness_unittest.cc:53: float scale = (low_level ? 0.01f : 1.0f); ... or even const float scale = frame_counter >= 60 ? : 0.01f : 1.0f; https://codereview.webrtc.org/1783203002/diff/180001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/noise_suppression_bitexactness_unittest.cc:75: const std::vector<float>& output, Nit: the internal order of reference and test parameters should preferably be equal. https://codereview.webrtc.org/1783203002/diff/180001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/noise_suppression_bitexactness_unittest.cc:82: output_reference.size() / stream_config.num_channels(); This should be an exact division, right? Consider using rtc::CheckedDivExact(a, b). https://codereview.webrtc.org/1783203002/diff/180001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/noise_suppression_bitexactness_unittest.cc:97: EXPECT_PRED_FORMAT2(test::AssertFloatsNotEqual, speech_probability, Did you consider using EXPECT_FLOAT_EQ or EXPECT_NEAR? https://codereview.webrtc.org/1783203002/diff/180001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/noise_suppression_bitexactness_unittest.cc:105: int num_channels, int -> size_t https://codereview.webrtc.org/1783203002/diff/180001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/noise_suppression_bitexactness_unittest.cc:106: int num_frames_to_process, int -> size_t https://codereview.webrtc.org/1783203002/diff/180001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/noise_suppression_bitexactness_unittest.cc:112: int samples_per_channel = 80 * sample_rate_hz / 8000; I think sample_rate_hz / 100 works just as well, with a comment that this is 10 ms. https://codereview.webrtc.org/1783203002/diff/180001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/noise_suppression_bitexactness_unittest.cc:128: for (int frame_no = 0; frame_no < num_frames_to_process; ++frame_no) { size_t frame_no https://codereview.webrtc.org/1783203002/diff/180001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/noise_suppression_bitexactness_unittest.cc:141: // Compare the output to the reference. Only the first values of the output Wonky line-breaks in this paragraph. https://codereview.webrtc.org/1783203002/diff/180001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/noise_suppression_bitexactness_unittest.cc:141: // Compare the output to the reference. Only the first values of the output Compare ... with https://codereview.webrtc.org/1783203002/diff/180001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/noise_suppression_bitexactness_unittest.cc:143: // is compared in order not having to specify all preceeding frames as are compared https://codereview.webrtc.org/1783203002/diff/180001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/test/audio_buffer_tools.cc (right): https://codereview.webrtc.org/1783203002/diff/180001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/audio_buffer_tools.cc:42: AudioBuffer* source, Since you are changing the signature, can the input source be made const? I realize that the CopyTo method is not const declared, but can it be? https://codereview.webrtc.org/1783203002/diff/180001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/test/bitexactness_tools.cc (right): https://codereview.webrtc.org/1783203002/diff/180001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/bitexactness_tools.cc:15: #include "webrtc/modules/audio_processing/test/bitexactness_tools.h" bitexactness_tools.h should be included first of all. https://google.github.io/styleguide/cppguide.html#Names_and_Order_of_Includes https://codereview.webrtc.org/1783203002/diff/180001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/bitexactness_tools.cc:25: float threshold = 1.0f / 32768.0f; const float kThreshold https://codereview.webrtc.org/1783203002/diff/180001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/bitexactness_tools.cc:31: // Lambda function that produces a formatted string with the values. Is this a lambda? https://codereview.webrtc.org/1783203002/diff/180001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/bitexactness_tools.cc:40: const char* m_expr, It seems to me that you are not using m_expr and n_expr, so I believe you can use the slightly simpler approach of a "function that returns an AssertionResult" (https://github.com/google/googletest/blob/master/googletest/docs/AdvancedGuid...) instead of a predicate-formatter (https://github.com/google/googletest/blob/master/googletest/docs/AdvancedGuid...). This means that the predicate can be invoked using simple {ASSERT|EXPECT}_{TRUE|FALSE} instead of the clunkier {ASSERT|EXPECT}_PRED_FORMATn. https://codereview.webrtc.org/1783203002/diff/180001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/bitexactness_tools.cc:45: float threshold = 1.0f / 32768.0f; const float kThreshold https://codereview.webrtc.org/1783203002/diff/180001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/bitexactness_tools.cc:47: for (size_t k = 0; k < reference.size(); ++k) { What if output is shorter than reference? Also, it seems that if the output is longer than reference, they can still be considered equal. If this is the intention, please, document this in the header file. https://codereview.webrtc.org/1783203002/diff/180001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/bitexactness_tools.cc:53: You may want to consider early return here in the case of success. if (equal) return ::testing::AssertionSuccess(); This saves you one level of indentation on the rest of the code. https://codereview.webrtc.org/1783203002/diff/180001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/bitexactness_tools.cc:62: for (size_t k = 0; k < num_values_to_print; ++k) { What if num_values_to_print is larger than the length of v?
https://codereview.webrtc.org/1783203002/diff/180001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/noise_suppression_bitexactness_unittest.cc (right): https://codereview.webrtc.org/1783203002/diff/180001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/noise_suppression_bitexactness_unittest.cc:2: * Copyright (c) 2015 The WebRTC project authors. All Rights Reserved. On 2016/03/16 12:44:28, hlundin-webrtc wrote: > 2016 Great find! Done. https://codereview.webrtc.org/1783203002/diff/180001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/noise_suppression_bitexactness_unittest.cc:44: // Forms a predefined random test vector- On 2016/03/16 12:44:27, hlundin-webrtc wrote: > End with '.', not '-'. Done. https://codereview.webrtc.org/1783203002/diff/180001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/noise_suppression_bitexactness_unittest.cc:44: // Forms a predefined random test vector- On 2016/03/16 12:44:28, hlundin-webrtc wrote: > Please, tell me what the range of the samples is. Good point! This part of the code is now removed. https://codereview.webrtc.org/1783203002/diff/180001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/noise_suppression_bitexactness_unittest.cc:45: void ConstructTestVector(int samples_per_channel, On 2016/03/16 12:44:28, hlundin-webrtc wrote: > Make all int parameters size_t. Done. https://codereview.webrtc.org/1783203002/diff/180001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/noise_suppression_bitexactness_unittest.cc:52: bool low_level = ((frame_counter / 10) > 5); On 2016/03/16 12:44:28, hlundin-webrtc wrote: > Why not > bool low_level = (frame_counter >= 60); > ? Good point! The code is now removed. https://codereview.webrtc.org/1783203002/diff/180001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/noise_suppression_bitexactness_unittest.cc:53: float scale = (low_level ? 0.01f : 1.0f); On 2016/03/16 12:44:28, hlundin-webrtc wrote: > ... or even > const float scale = frame_counter >= 60 ? : 0.01f : 1.0f; Another good point! The code is now removed. https://codereview.webrtc.org/1783203002/diff/180001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/noise_suppression_bitexactness_unittest.cc:75: const std::vector<float>& output, On 2016/03/16 12:44:27, hlundin-webrtc wrote: > Nit: the internal order of reference and test parameters should preferably be > equal. Done. https://codereview.webrtc.org/1783203002/diff/180001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/noise_suppression_bitexactness_unittest.cc:82: output_reference.size() / stream_config.num_channels(); On 2016/03/16 12:44:28, hlundin-webrtc wrote: > This should be an exact division, right? Consider using rtc::CheckedDivExact(a, > b). Good point! Thanks for the suggestion! Done. https://codereview.webrtc.org/1783203002/diff/180001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/noise_suppression_bitexactness_unittest.cc:97: EXPECT_PRED_FORMAT2(test::AssertFloatsNotEqual, speech_probability, On 2016/03/16 12:44:28, hlundin-webrtc wrote: > Did you consider using EXPECT_FLOAT_EQ or EXPECT_NEAR? Great suggestion! Done. https://codereview.webrtc.org/1783203002/diff/180001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/noise_suppression_bitexactness_unittest.cc:105: int num_channels, On 2016/03/16 12:44:28, hlundin-webrtc wrote: > int -> size_t Done. https://codereview.webrtc.org/1783203002/diff/180001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/noise_suppression_bitexactness_unittest.cc:106: int num_frames_to_process, On 2016/03/16 12:44:28, hlundin-webrtc wrote: > int -> size_t Done. https://codereview.webrtc.org/1783203002/diff/180001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/noise_suppression_bitexactness_unittest.cc:112: int samples_per_channel = 80 * sample_rate_hz / 8000; On 2016/03/16 12:44:28, hlundin-webrtc wrote: > I think sample_rate_hz / 100 works just as well, with a comment that this is 10 > ms. Done. https://codereview.webrtc.org/1783203002/diff/180001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/noise_suppression_bitexactness_unittest.cc:128: for (int frame_no = 0; frame_no < num_frames_to_process; ++frame_no) { On 2016/03/16 12:44:28, hlundin-webrtc wrote: > size_t frame_no Done. https://codereview.webrtc.org/1783203002/diff/180001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/noise_suppression_bitexactness_unittest.cc:141: // Compare the output to the reference. Only the first values of the output On 2016/03/16 12:44:28, hlundin-webrtc wrote: > Compare ... with Done. https://codereview.webrtc.org/1783203002/diff/180001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/noise_suppression_bitexactness_unittest.cc:141: // Compare the output to the reference. Only the first values of the output On 2016/03/16 12:44:28, hlundin-webrtc wrote: > Wonky line-breaks in this paragraph. Done. https://codereview.webrtc.org/1783203002/diff/180001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/noise_suppression_bitexactness_unittest.cc:143: // is compared in order not having to specify all preceeding frames as On 2016/03/16 12:44:27, hlundin-webrtc wrote: > are compared Done. https://codereview.webrtc.org/1783203002/diff/180001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/test/audio_buffer_tools.cc (right): https://codereview.webrtc.org/1783203002/diff/180001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/audio_buffer_tools.cc:42: AudioBuffer* source, On 2016/03/16 12:44:28, hlundin-webrtc wrote: > Since you are changing the signature, can the input source be made const? I > realize that the CopyTo method is not const declared, but can it be? Unfortunately not :-(. This is because the CopyTo command in AudioBuffer is not const declared, and the reason for that is that it has an internal cache that it updates depending on what kind of data has been accessed. This is another motivation why audiobuffer should be refactored. https://codereview.webrtc.org/1783203002/diff/180001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/test/bitexactness_tools.cc (right): https://codereview.webrtc.org/1783203002/diff/180001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/bitexactness_tools.cc:15: #include "webrtc/modules/audio_processing/test/bitexactness_tools.h" On 2016/03/16 12:44:28, hlundin-webrtc wrote: > bitexactness_tools.h should be included first of all. > https://google.github.io/styleguide/cppguide.html#Names_and_Order_of_Includes Done. https://codereview.webrtc.org/1783203002/diff/180001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/bitexactness_tools.cc:25: float threshold = 1.0f / 32768.0f; On 2016/03/16 12:44:28, hlundin-webrtc wrote: > const float kThreshold Done. https://codereview.webrtc.org/1783203002/diff/180001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/bitexactness_tools.cc:31: // Lambda function that produces a formatted string with the values. On 2016/03/16 12:44:29, hlundin-webrtc wrote: > Is this a lambda? No, the comment in the code is not correct. This function has now been removed. https://codereview.webrtc.org/1783203002/diff/180001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/bitexactness_tools.cc:40: const char* m_expr, On 2016/03/16 12:44:28, hlundin-webrtc wrote: > It seems to me that you are not using m_expr and n_expr, so I believe you can > use the slightly simpler approach of a "function that returns an > AssertionResult" > (https://github.com/google/googletest/blob/master/googletest/docs/AdvancedGuid...) > instead of a predicate-formatter > (https://github.com/google/googletest/blob/master/googletest/docs/AdvancedGuid...). > This means that the predicate can be invoked using simple > {ASSERT|EXPECT}_{TRUE|FALSE} instead of the clunkier > {ASSERT|EXPECT}_PRED_FORMATn. Awesome!!! Splendid suggestion! Done. https://codereview.webrtc.org/1783203002/diff/180001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/bitexactness_tools.cc:45: float threshold = 1.0f / 32768.0f; On 2016/03/16 12:44:28, hlundin-webrtc wrote: > const float kThreshold Good point. This code has now been refactored, so it is a bit different now. Done. https://codereview.webrtc.org/1783203002/diff/180001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/bitexactness_tools.cc:47: for (size_t k = 0; k < reference.size(); ++k) { On 2016/03/16 12:44:29, hlundin-webrtc wrote: > What if output is shorter than reference? Also, it seems that if the output is > longer than reference, they can still be considered equal. If this is the > intention, please, document this in the header file. Good find! Done. https://codereview.webrtc.org/1783203002/diff/180001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/bitexactness_tools.cc:53: On 2016/03/16 12:44:28, hlundin-webrtc wrote: > You may want to consider early return here in the case of success. > if (equal) > return ::testing::AssertionSuccess(); > > This saves you one level of indentation on the rest of the code. Done. https://codereview.webrtc.org/1783203002/diff/180001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/bitexactness_tools.cc:62: for (size_t k = 0; k < num_values_to_print; ++k) { On 2016/03/16 12:44:28, hlundin-webrtc wrote: > What if num_values_to_print is larger than the length of v? Done.
Some minor further updates.
Much improved. Just a few minor comments left. https://codereview.webrtc.org/1783203002/diff/180001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/test/audio_buffer_tools.cc (right): https://codereview.webrtc.org/1783203002/diff/180001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/audio_buffer_tools.cc:42: AudioBuffer* source, On 2016/03/17 13:15:01, peah-webrtc wrote: > On 2016/03/16 12:44:28, hlundin-webrtc wrote: > > Since you are changing the signature, can the input source be made const? I > > realize that the CopyTo method is not const declared, but can it be? > > Unfortunately not :-(. This is because the CopyTo command in AudioBuffer is not > const declared, and the reason for that is that it has an internal cache that it > updates depending on what kind of data has been accessed. > > This is another motivation why audiobuffer should be refactored. Acknowledged. https://codereview.webrtc.org/1783203002/diff/220001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/noise_suppression_bitexactness_unittest.cc (right): https://codereview.webrtc.org/1783203002/diff/220001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/noise_suppression_bitexactness_unittest.cc:36: capture_buffer->MergeFrequencyBands(); I think I asked this in another CL, but what's the purpose of the merging in this test? Are you looking at the frame after it is processed above? https://codereview.webrtc.org/1783203002/diff/220001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/test/audio_buffer_tools.cc (right): https://codereview.webrtc.org/1783203002/diff/220001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/audio_buffer_tools.cc:38: memcpy(input_samples.data(), &source[0], &source[0] -> source.data() https://codereview.webrtc.org/1783203002/diff/220001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/audio_buffer_tools.cc:39: input_samples.size() * sizeof(input_samples[0])); Using input_samples.size() and sizeof(input_samples[0]) seems backwards to me. I'd rather use source.size() and sizeof(source[0]). And, somehow make sure that the dimension of input_samples is enough. https://codereview.webrtc.org/1783203002/diff/220001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/test/bitexactness_tools.cc (right): https://codereview.webrtc.org/1783203002/diff/220001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/bitexactness_tools.cc:28: break; You don't need break after return. https://codereview.webrtc.org/1783203002/diff/220001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/bitexactness_tools.cc:48: break; Skip the breaks! :)
https://codereview.webrtc.org/1783203002/diff/220001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/noise_suppression_bitexactness_unittest.cc (right): https://codereview.webrtc.org/1783203002/diff/220001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/noise_suppression_bitexactness_unittest.cc:36: capture_buffer->MergeFrequencyBands(); On 2016/03/17 14:14:40, hlundin-webrtc wrote: > I think I asked this in another CL, but what's the purpose of the merging in > this test? Are you looking at the frame after it is processed above? The purpose of the merging is that I want to verify the output after the merge. The subcomponents in APM operate in 16 kHz but extends their processing to higher frequencies than 8 kHz for 32-48 kHz. Therefore, in order to be able to verify bitexactness for 32-48 kHz the bands need to be merged again. This is the same as happens in APM. Your other comment related to LevelEstimator which is an exception in that it actually operates on the full band. Therefore, in that CL I removed the band split/merge. In this CL it is needed though. Note that after I have processed a the desired number of frames the output is extracted in RunBitexactness(). https://codereview.webrtc.org/1783203002/diff/220001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/test/audio_buffer_tools.cc (right): https://codereview.webrtc.org/1783203002/diff/220001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/audio_buffer_tools.cc:38: memcpy(input_samples.data(), &source[0], On 2016/03/17 14:14:40, hlundin-webrtc wrote: > &source[0] -> source.data() Done. https://codereview.webrtc.org/1783203002/diff/220001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/audio_buffer_tools.cc:39: input_samples.size() * sizeof(input_samples[0])); On 2016/03/17 14:14:40, hlundin-webrtc wrote: > Using input_samples.size() and sizeof(input_samples[0]) seems backwards to me. > I'd rather use source.size() and sizeof(source[0]). And, somehow make sure that > the dimension of input_samples is enough. I agree that it is a bit backward. But it ensures that there is no copy outside of the destination buffer. There is an assert before the copy though so it is safe to change. I'll take your suggestion. https://codereview.webrtc.org/1783203002/diff/220001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/test/bitexactness_tools.cc (right): https://codereview.webrtc.org/1783203002/diff/220001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/bitexactness_tools.cc:28: break; On 2016/03/17 14:14:40, hlundin-webrtc wrote: > You don't need break after return. Done. https://codereview.webrtc.org/1783203002/diff/220001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/bitexactness_tools.cc:48: break; On 2016/03/17 14:14:40, hlundin-webrtc wrote: > Skip the breaks! :) Done.
LGTM with one nit. https://codereview.webrtc.org/1783203002/diff/220001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/noise_suppression_bitexactness_unittest.cc (right): https://codereview.webrtc.org/1783203002/diff/220001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/noise_suppression_bitexactness_unittest.cc:36: capture_buffer->MergeFrequencyBands(); On 2016/03/17 22:26:02, peah-webrtc wrote: > On 2016/03/17 14:14:40, hlundin-webrtc wrote: > > I think I asked this in another CL, but what's the purpose of the merging in > > this test? Are you looking at the frame after it is processed above? > > The purpose of the merging is that I want to verify the output after the merge. > The subcomponents in APM operate in 16 kHz but extends their processing to > higher frequencies than 8 kHz for 32-48 kHz. Therefore, in order to be able to > verify bitexactness for 32-48 kHz the bands need to be merged again. This is the > same as happens in APM. > > Your other comment related to LevelEstimator which is an exception in that it > actually operates on the full band. Therefore, in that CL I removed the band > split/merge. In this CL it is needed though. > > Note that after I have processed a the desired number of frames the output is > extracted in RunBitexactness(). Acknowledged. https://codereview.webrtc.org/1783203002/diff/240001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/test/audio_buffer_tools.cc (right): https://codereview.webrtc.org/1783203002/diff/240001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/audio_buffer_tools.cc:37: RTC_DCHECK_EQ(input_samples.size(), source.size()); Change to CHECK, since this ensures the sanity of the memory.
https://codereview.webrtc.org/1783203002/diff/240001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/test/audio_buffer_tools.cc (right): https://codereview.webrtc.org/1783203002/diff/240001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/audio_buffer_tools.cc:37: RTC_DCHECK_EQ(input_samples.size(), source.size()); On 2016/03/18 07:53:35, hlundin-webrtc wrote: > Change to CHECK, since this ensures the sanity of the memory. 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/1783203002/300001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1783203002/300001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_rel/builds/13611)
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 Link to the patchset: https://codereview.webrtc.org/1783203002/#ps360001 (title: "Rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1783203002/360001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1783203002/360001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_compile_mips_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_compile_mips_db...)
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 Link to the patchset: https://codereview.webrtc.org/1783203002/#ps380001 (title: "Corrected syntax error")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1783203002/380001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1783203002/380001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_compile_mips_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_compile_mips_db...)
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 Link to the patchset: https://codereview.webrtc.org/1783203002/#ps400001 (title: "Fixed another Android specific code syntax error")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1783203002/400001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1783203002/400001
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) linux_baremetal on tryserver.webrtc (JOB_TIMED_OUT, no build URL) win_baremetal on tryserver.webrtc (JOB_TIMED_OUT, no build URL)
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 Link to the patchset: https://codereview.webrtc.org/1783203002/#ps420001 (title: "Updated testvectors for Android")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1783203002/420001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1783203002/420001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_clang_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_clang_dbg/build...) android_compile_x86_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_compile_x86_dbg...)
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 Link to the patchset: https://codereview.webrtc.org/1783203002/#ps440001 (title: "Fixed typo")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1783203002/440001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1783203002/440001
The CQ bit was unchecked by commit-bot@chromium.org
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/6096) ios_arm64_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_dbg/builds/8422) ios_arm64_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_rel/builds/8339) ios_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_dbg/builds/13622) ios_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_rel/builds/12253) mac_asan on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_asan/builds/13277) mac_baremetal on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_baremetal/builds/9987) mac_compile_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_compile_dbg/builds/...)
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 Link to the patchset: https://codereview.webrtc.org/1783203002/#ps470001 (title: "Removed effects from bad merge")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1783203002/470001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1783203002/470001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_dbg/builds/11864)
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 Link to the patchset: https://codereview.webrtc.org/1783203002/#ps490001 (title: "Corrected reference value for Android")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1783203002/490001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1783203002/490001
The CQ bit was unchecked by commit-bot@chromium.org
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...)
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 Link to the patchset: https://codereview.webrtc.org/1783203002/#ps550001 (title: "Fixed syntax error")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1783203002/550001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1783203002/550001
Message was sent while issue was closed.
Description was changed from ========== Added a bitexactness test for the noise suppressor. This CL also extracts part of the functionality used in the bitexactness test for the high-pass filter into a separate file in order to be able to reuse that functionality in bitexactness tests for the other submodules in APM (including the bitexactness test for the noise suppressor). BUG=wertc:5336 ========== to ========== Added a bitexactness test for the noise suppressor. This CL also extracts part of the functionality used in the bitexactness test for the high-pass filter into a separate file in order to be able to reuse that functionality in bitexactness tests for the other submodules in APM (including the bitexactness test for the noise suppressor). BUG=wertc:5336 ==========
Message was sent while issue was closed.
Committed patchset #29 (id:550001)
Message was sent while issue was closed.
Description was changed from ========== Added a bitexactness test for the noise suppressor. This CL also extracts part of the functionality used in the bitexactness test for the high-pass filter into a separate file in order to be able to reuse that functionality in bitexactness tests for the other submodules in APM (including the bitexactness test for the noise suppressor). BUG=wertc:5336 ========== to ========== Added a bitexactness test for the noise suppressor. This CL also extracts part of the functionality used in the bitexactness test for the high-pass filter into a separate file in order to be able to reuse that functionality in bitexactness tests for the other submodules in APM (including the bitexactness test for the noise suppressor). BUG=wertc:5336 Committed: https://crrev.com/5585001e5d3f867753752a8569a4f778636c583d Cr-Commit-Position: refs/heads/master@{#12061} ==========
Message was sent while issue was closed.
Patchset 29 (id:??) landed as https://crrev.com/5585001e5d3f867753752a8569a4f778636c583d Cr-Commit-Position: refs/heads/master@{#12061} |
