Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1510493004:
Bitexactness test for the highpass filter (Closed)
|
Created:
5 years ago by peah-webrtc Modified:
4 years, 9 months ago CC:
webrtc-reviews_webrtc.org, 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. |
DescriptionA bitexactness test for the highpass filter in the
audio processing module.
The test also adds a new helper class called
VectorBasedAudioFrame that is intended to be
reused for the bitexactness tests for the other
submodules.
BUG=webrtc:1091
Committed: https://crrev.com/0197363d18354af532b3a01abfd5d79fa63454c6
Cr-Commit-Position: refs/heads/master@{#11864}
Patch Set 1 : Initial test version #Patch Set 2 : Added float specifiers #Patch Set 3 : Added missing float specifiers #Patch Set 4 : Changed printf format #Patch Set 5 : Change printf format #Patch Set 6 : Refactored the test not to use fixtures in response to reviewer comments #
Total comments: 64
Patch Set 7 : Major changes in response to reviewer comments #Patch Set 8 : Sorted the files in the build file #
Total comments: 20
Patch Set 9 : Changes in response to reviewer comments #
Total comments: 18
Patch Set 10 : Changes in response to reviewer comments #
Total comments: 8
Patch Set 11 : Removed redundant include, updated test input to be different for different channels, changed comme… #Patch Set 12 : Merge from latest master and changed type from int to size_t for looping variable #Patch Set 13 : Reverted changes to audio_processing_impl.cc #
Messages
Total messages: 44 (19 generated)
Patchset #2 (id:20001) has been deleted
Patchset #1 (id:1) has been deleted
Description was changed from ========== Format changes Commit 2 FOrmat changes Initial commit BUG= ========== to ========== A bitexactness test for the highpass filter in the audio processing module. The test also adds a new helper class called VectorBasedAudioFrame that is intended to be reused for the bitexactness tests for the other submodules. BUG=webrtc:1091 ==========
peah@webrtc.org changed reviewers: + henrik.lundin@webrtc.org, ivoc@webrtc.org
Description was changed from ========== A bitexactness test for the highpass filter in the audio processing module. The test also adds a new helper class called VectorBasedAudioFrame that is intended to be reused for the bitexactness tests for the other submodules. BUG=webrtc:1091 ========== to ========== A bitexactness test for the highpass filter in the audio processing module. The test also adds a new helper class called VectorBasedAudioFrame that is intended to be reused for the bitexactness tests for the other submodules. BUG=webrtc:1091 ==========
solenberg@google.com changed reviewers: + solenberg@google.com
I don't think a parameterized test is warranted in this case. Normal test cases would be easier to understand and maintain.
On 2015/12/10 08:53:49, solenberg wrote: > I don't think a parameterized test is warranted in this case. Normal test cases > would be easier to understand and maintain. Agree. This has now been changed. Please have another look!
solenberg@webrtc.org changed reviewers: + solenberg@webrtc.org - solenberg@google.com
https://codereview.webrtc.org/1510493004/diff/140001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/high_pass_filter_bitexactness_unittest.cc (right): https://codereview.webrtc.org/1510493004/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/high_pass_filter_bitexactness_unittest.cc:29: const float kReference8000InitMono[1][kReferenceLength] = { Move these down locally, into the test cases. https://codereview.webrtc.org/1510493004/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/high_pass_filter_bitexactness_unittest.cc:62: enum class ChannelsType { kMono, kStereo }; I think this is quite unnecessary. The HPF takes as argument a channel count; this enum just forces you to convert from enum to a count in several places. https://codereview.webrtc.org/1510493004/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/high_pass_filter_bitexactness_unittest.cc:148: void PrintReferenceAndOutput(const float* reference) { You should be able to use EXPECT_PRED_FORMAT2() from gtest instead of rolling your own. https://codereview.webrtc.org/1510493004/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/high_pass_filter_bitexactness_unittest.cc:160: inputoutput_.Randomize(&rand_gen_); It's a bad idea to let the test output rely on the implementation and seed of the randomizer. Create a static stimuli vector instead, or a few of them. https://codereview.webrtc.org/1510493004/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/high_pass_filter_bitexactness_unittest.cc:170: rtc::scoped_ptr<AudioBuffer> audio_buffer_; Don't need a scoped_ptr here, and not for high_pass_filter_ https://codereview.webrtc.org/1510493004/diff/140001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/test/vector_based_audio_frame.cc (right): https://codereview.webrtc.org/1510493004/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/vector_based_audio_frame.cc:39: static_cast<float>(rand_gen_->Rand(0, 32767 + 32768) - 32768) / For testing the HPF, I'd also use an impulse. https://codereview.webrtc.org/1510493004/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/vector_based_audio_frame.cc:64: if (fabs(frame_[ch][k] - (*other_frame_array)[ch][k]) > tolerance) { In gtest there's EXPECT_NEAR also. https://codereview.webrtc.org/1510493004/diff/140001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/test/vector_based_audio_frame.h (right): https://codereview.webrtc.org/1510493004/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/vector_based_audio_frame.h:28: class VectorBasedAudioFrame { AudioBuffer already maintains buffers for the audio data. I think you don't need this separate class to copy to/from. What if you instead make a set of utility functions to initialize and verify the contents of an AudioBuffer?
https://codereview.webrtc.org/1510493004/diff/140001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/high_pass_filter_bitexactness_unittest.cc (right): https://codereview.webrtc.org/1510493004/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/high_pass_filter_bitexactness_unittest.cc:74: num_channels = (channels_type == ChannelsType::kMono ? 1 : 2); Move num_channels to initializer list too. https://codereview.webrtc.org/1510493004/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/high_pass_filter_bitexactness_unittest.cc:94: HighPassFilterBitExactnessTest(TestConfig test_config) explicit https://codereview.webrtc.org/1510493004/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/high_pass_filter_bitexactness_unittest.cc:122: // Processes a specified amoutn of frames, verifies the results and reports amount https://codereview.webrtc.org/1510493004/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/high_pass_filter_bitexactness_unittest.cc:126: std::string report_header) { const std::string& https://codereview.webrtc.org/1510493004/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/high_pass_filter_bitexactness_unittest.cc:128: EXPECT_EQ(ProcessOneFrame(), AudioProcessing::kNoError); Swap the parameters: AudioProcessing::kNoError, ProcessOneFrame() https://codereview.webrtc.org/1510493004/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/high_pass_filter_bitexactness_unittest.cc:133: bool result = const bool https://codereview.webrtc.org/1510493004/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/high_pass_filter_bitexactness_unittest.cc:171: VectorBasedAudioFrame inputoutput_; input_output_ https://codereview.webrtc.org/1510493004/diff/140001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/test/vector_based_audio_frame.cc (right): https://codereview.webrtc.org/1510493004/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/vector_based_audio_frame.cc:19: for (size_t ch = 0; ch < num_channels; ++ch) { Do one memcpy for all of the data: memcpy(&frame_[0][0], initial_values, frame_length * num_channels); https://codereview.webrtc.org/1510493004/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/vector_based_audio_frame.cc:28: channels_.resize(num_channels * frame_length_); num_channels_ - or - frame_length https://codereview.webrtc.org/1510493004/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/vector_based_audio_frame.cc:35: void VectorBasedAudioFrame::Randomize(test::Random* rand_gen_) const { How can this be const when you are in fact modifying frame_? https://codereview.webrtc.org/1510493004/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/vector_based_audio_frame.cc:36: for (size_t ch = 0; ch < num_channels_; ++ch) { Do one grand for-loop. Make it range-based over frame_. for (auto& value : frame_) { value = static_cast<float>(rand_gen_->Rand(0, 32767 + 32768) - 32768) / 32768.0f; } https://codereview.webrtc.org/1510493004/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/vector_based_audio_frame.cc:51: AudioBuffer* buffer) { I would like buffer to be const, but I guess that doesn't work since AudioBuffer::CopyTo isn't const declared. https://codereview.webrtc.org/1510493004/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/vector_based_audio_frame.cc:57: if (num_channels_ != other_frame->num_channels()) { Won't you have to check the frame_lengths too? Or, see my suggestion for total_length() below. https://codereview.webrtc.org/1510493004/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/vector_based_audio_frame.cc:62: for (size_t ch = 0; ch < num_channels_; ++ch) { Can you rewrite this to one grand for loop: for (size_t i = 0; i < total_length(); ++i) { if (fabs(frame_[i] - (*other_frame_array)[i]) > tolerance) return false; } I also suggest you add a convenience getter size_t VectorBasedAudioFrame::total_length() const { return num_channels_ * frame_length_; } https://codereview.webrtc.org/1510493004/diff/140001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/test/vector_based_audio_frame.h (right): https://codereview.webrtc.org/1510493004/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/vector_based_audio_frame.h:2: * Copyright (c) 2014 The WebRTC project authors. All Rights Reserved. 2015 https://codereview.webrtc.org/1510493004/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/vector_based_audio_frame.h:30: VectorBasedAudioFrame(size_t frame_length, Comment that the size of initial_values must be (at least) frame_length * num_channels elements. https://codereview.webrtc.org/1510493004/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/vector_based_audio_frame.h:34: VectorBasedAudioFrame(size_t frame_length, size_t num_channels); What is the initial value in this case? https://codereview.webrtc.org/1510493004/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/vector_based_audio_frame.h:49: // Compare the the frame content to the content of another frame using a Compares https://codereview.webrtc.org/1510493004/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/vector_based_audio_frame.h:51: bool CompareTo(VectorBasedAudioFrame* other_frame, float tolerance) const; Drop To, simply call it Compare (it's shorter, and you avoid fueling the Compare To/With debate). https://codereview.webrtc.org/1510493004/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/vector_based_audio_frame.h:51: bool CompareTo(VectorBasedAudioFrame* other_frame, float tolerance) const; other_frame should be const, right? https://codereview.webrtc.org/1510493004/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/vector_based_audio_frame.h:53: // Prints all the values of the frame. ... to stdout? https://codereview.webrtc.org/1510493004/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/vector_based_audio_frame.h:56: // Prints a specified number of values from the frame. ... to stdout? https://codereview.webrtc.org/1510493004/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/vector_based_audio_frame.h:60: const size_t frame_length_; Do you need frame_length_ and num_channels_ as explicit member variables? Can't you just ask the vectors for that? https://codereview.webrtc.org/1510493004/diff/140001/webrtc/modules/modules.gyp File webrtc/modules/modules.gyp (right): https://codereview.webrtc.org/1510493004/diff/140001/webrtc/modules/modules.g... webrtc/modules/modules.gyp:406: 'audio_processing/high_pass_filter_bitexactness_unittest.cc', Sort the files.
Major update in response to your great comments! https://codereview.webrtc.org/1510493004/diff/140001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/high_pass_filter_bitexactness_unittest.cc (right): https://codereview.webrtc.org/1510493004/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/high_pass_filter_bitexactness_unittest.cc:29: const float kReference8000InitMono[1][kReferenceLength] = { On 2015/12/10 12:03:53, the sun wrote: > Move these down locally, into the test cases. Done. https://codereview.webrtc.org/1510493004/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/high_pass_filter_bitexactness_unittest.cc:62: enum class ChannelsType { kMono, kStereo }; On 2015/12/10 12:03:53, the sun wrote: > I think this is quite unnecessary. The HPF takes as argument a channel count; > this enum just forces you to convert from enum to a count in several places. Done. https://codereview.webrtc.org/1510493004/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/high_pass_filter_bitexactness_unittest.cc:74: num_channels = (channels_type == ChannelsType::kMono ? 1 : 2); On 2015/12/10 12:12:16, hlundin-webrtc wrote: > Move num_channels to initializer list too. Done. https://codereview.webrtc.org/1510493004/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/high_pass_filter_bitexactness_unittest.cc:94: HighPassFilterBitExactnessTest(TestConfig test_config) On 2015/12/10 12:12:16, hlundin-webrtc wrote: > explicit Done. https://codereview.webrtc.org/1510493004/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/high_pass_filter_bitexactness_unittest.cc:122: // Processes a specified amoutn of frames, verifies the results and reports On 2015/12/10 12:12:17, hlundin-webrtc wrote: > amount Done. https://codereview.webrtc.org/1510493004/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/high_pass_filter_bitexactness_unittest.cc:126: std::string report_header) { On 2015/12/10 12:12:17, hlundin-webrtc wrote: > const std::string& Done. https://codereview.webrtc.org/1510493004/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/high_pass_filter_bitexactness_unittest.cc:128: EXPECT_EQ(ProcessOneFrame(), AudioProcessing::kNoError); On 2015/12/10 12:12:16, hlundin-webrtc wrote: > Swap the parameters: AudioProcessing::kNoError, ProcessOneFrame() Done. https://codereview.webrtc.org/1510493004/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/high_pass_filter_bitexactness_unittest.cc:133: bool result = On 2015/12/10 12:12:17, hlundin-webrtc wrote: > const bool Done. https://codereview.webrtc.org/1510493004/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/high_pass_filter_bitexactness_unittest.cc:148: void PrintReferenceAndOutput(const float* reference) { On 2015/12/10 12:03:53, the sun OOO until jan 7th wrote: > You should be able to use EXPECT_PRED_FORMAT2() from gtest instead of rolling > your own. Nice suggestion! Done. https://codereview.webrtc.org/1510493004/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/high_pass_filter_bitexactness_unittest.cc:160: inputoutput_.Randomize(&rand_gen_); On 2015/12/10 12:03:53, the sun OOO until jan 7th wrote: > It's a bad idea to let the test output rely on the implementation and seed of > the randomizer. Create a static stimuli vector instead, or a few of them. Good point! Done. https://codereview.webrtc.org/1510493004/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/high_pass_filter_bitexactness_unittest.cc:170: rtc::scoped_ptr<AudioBuffer> audio_buffer_; On 2015/12/10 12:03:53, the sun OOO until jan 7th wrote: > Don't need a scoped_ptr here, and not for high_pass_filter_ Done. https://codereview.webrtc.org/1510493004/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/high_pass_filter_bitexactness_unittest.cc:171: VectorBasedAudioFrame inputoutput_; On 2015/12/10 12:12:16, hlundin-webrtc wrote: > input_output_ Done. https://codereview.webrtc.org/1510493004/diff/140001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/test/vector_based_audio_frame.cc (right): https://codereview.webrtc.org/1510493004/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/vector_based_audio_frame.cc:19: for (size_t ch = 0; ch < num_channels; ++ch) { On 2015/12/10 12:12:17, hlundin-webrtc wrote: > Do one memcpy for all of the data: > memcpy(&frame_[0][0], initial_values, frame_length * num_channels); True. This is now done in the new code. Acknowledged. https://codereview.webrtc.org/1510493004/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/vector_based_audio_frame.cc:28: channels_.resize(num_channels * frame_length_); On 2015/12/10 12:12:17, hlundin-webrtc wrote: > num_channels_ > - or - > frame_length Good catch! Acknowledged. https://codereview.webrtc.org/1510493004/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/vector_based_audio_frame.cc:35: void VectorBasedAudioFrame::Randomize(test::Random* rand_gen_) const { On 2015/12/10 12:12:17, hlundin-webrtc wrote: > How can this be const when you are in fact modifying frame_? True! It is really strange that that even compiled! The code is, however, now replaced with a better version. Acknowledged. https://codereview.webrtc.org/1510493004/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/vector_based_audio_frame.cc:36: for (size_t ch = 0; ch < num_channels_; ++ch) { On 2015/12/10 12:12:17, hlundin-webrtc wrote: > Do one grand for-loop. Make it range-based over frame_. > for (auto& value : frame_) { > value = static_cast<float>(rand_gen_->Rand(0, 32767 + 32768) - 32768) / > 32768.0f; > } Acknowledged. https://codereview.webrtc.org/1510493004/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/vector_based_audio_frame.cc:39: static_cast<float>(rand_gen_->Rand(0, 32767 + 32768) - 32768) / On 2015/12/10 12:03:53, the sun OOO until jan 7th wrote: > For testing the HPF, I'd also use an impulse. Why would that be better than using WGN signal for testing the bitexactness? They should both excite all aspects of the filter. https://codereview.webrtc.org/1510493004/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/vector_based_audio_frame.cc:51: AudioBuffer* buffer) { On 2015/12/10 12:12:17, hlundin-webrtc wrote: > I would like buffer to be const, but I guess that doesn't work since > AudioBuffer::CopyTo isn't const declared. True. That is the reason for this. And the reason that is not const declared is that it may cache the internal conversion as well during this operation. https://codereview.webrtc.org/1510493004/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/vector_based_audio_frame.cc:57: if (num_channels_ != other_frame->num_channels()) { On 2015/12/10 12:12:17, hlundin-webrtc wrote: > Won't you have to check the frame_lengths too? Or, see my suggestion for > total_length() below. Agree. This is now replaced with another function. Acknowledged. https://codereview.webrtc.org/1510493004/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/vector_based_audio_frame.cc:62: for (size_t ch = 0; ch < num_channels_; ++ch) { On 2015/12/10 12:12:17, hlundin-webrtc wrote: > Can you rewrite this to one grand for loop: > for (size_t i = 0; i < total_length(); ++i) { > if (fabs(frame_[i] - (*other_frame_array)[i]) > tolerance) > return false; > } > > I also suggest you add a convenience getter > size_t VectorBasedAudioFrame::total_length() const { > return num_channels_ * frame_length_; > } Agree. that would be better. This is now replaced by another function. Acknowledged. https://codereview.webrtc.org/1510493004/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/vector_based_audio_frame.cc:64: if (fabs(frame_[ch][k] - (*other_frame_array)[ch][k]) > tolerance) { On 2015/12/10 12:03:54, the sun OOO until jan 7th wrote: > In gtest there's EXPECT_NEAR also. True, but since I now use EXPECT_PRED_FORMAT2() where I want to also catch the error in order to make a proper printout and flag that an error has occurred, I cannot see how I can use EXPECT_NEAR in any good way. Do you have any suggestion for that? https://codereview.webrtc.org/1510493004/diff/140001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/test/vector_based_audio_frame.h (right): https://codereview.webrtc.org/1510493004/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/vector_based_audio_frame.h:2: * Copyright (c) 2014 The WebRTC project authors. All Rights Reserved. On 2015/12/10 12:12:17, hlundin-webrtc wrote: > 2015 Done. https://codereview.webrtc.org/1510493004/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/vector_based_audio_frame.h:28: class VectorBasedAudioFrame { On 2015/12/10 12:03:54, the sun OOO until jan 7th wrote: > AudioBuffer already maintains buffers for the audio data. I think you don't need > this separate class to copy to/from. What if you instead make a set of utility > functions to initialize and verify the contents of an AudioBuffer? Of course, you are right. Have now done that, which also resulted in a file name change. Done. https://codereview.webrtc.org/1510493004/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/vector_based_audio_frame.h:30: VectorBasedAudioFrame(size_t frame_length, On 2015/12/10 12:12:17, hlundin-webrtc wrote: > Comment that the size of initial_values must be (at least) frame_length * > num_channels elements. This is no longer applicable in the new code. Acknowledged. https://codereview.webrtc.org/1510493004/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/vector_based_audio_frame.h:34: VectorBasedAudioFrame(size_t frame_length, size_t num_channels); On 2015/12/10 12:12:17, hlundin-webrtc wrote: > What is the initial value in this case? They are not initialized at all, only allocated. This has been totally changed in the new patchset though. Acknowledged. https://codereview.webrtc.org/1510493004/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/vector_based_audio_frame.h:49: // Compare the the frame content to the content of another frame using a On 2015/12/10 12:12:17, hlundin-webrtc wrote: > Compares Good catch. Fully changed in the new code though. Acknowledged. https://codereview.webrtc.org/1510493004/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/vector_based_audio_frame.h:51: bool CompareTo(VectorBasedAudioFrame* other_frame, float tolerance) const; On 2015/12/10 12:12:17, hlundin-webrtc wrote: > Drop To, simply call it Compare (it's shorter, and you avoid fueling the Compare > To/With debate). Great point! This method has been removed in the new patchset though. Acknowledged. https://codereview.webrtc.org/1510493004/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/vector_based_audio_frame.h:51: bool CompareTo(VectorBasedAudioFrame* other_frame, float tolerance) const; On 2015/12/10 12:12:17, hlundin-webrtc wrote: > other_frame should be const, right? True. Acknowledged. https://codereview.webrtc.org/1510493004/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/vector_based_audio_frame.h:53: // Prints all the values of the frame. On 2015/12/10 12:12:17, hlundin-webrtc wrote: > ... to stdout? Yes, that was how it was. Good point that that should be specified. This function is removed now though. Acknowledged. https://codereview.webrtc.org/1510493004/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/vector_based_audio_frame.h:56: // Prints a specified number of values from the frame. On 2015/12/10 12:12:17, hlundin-webrtc wrote: > ... to stdout? The function is now removed. Acknowledged. https://codereview.webrtc.org/1510493004/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/vector_based_audio_frame.h:60: const size_t frame_length_; On 2015/12/10 12:12:17, hlundin-webrtc wrote: > Do you need frame_length_ and num_channels_ as explicit member variables? Can't > you just ask the vectors for that? Fully true! That would be better. The new implementation of this does that instead. Acknowledged. https://codereview.webrtc.org/1510493004/diff/140001/webrtc/modules/modules.gyp File webrtc/modules/modules.gyp (right): https://codereview.webrtc.org/1510493004/diff/140001/webrtc/modules/modules.g... webrtc/modules/modules.gyp:406: 'audio_processing/high_pass_filter_bitexactness_unittest.cc', On 2015/12/10 12:12:17, hlundin-webrtc wrote: > Sort the files. Done.
https://codereview.webrtc.org/1510493004/diff/180001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/audio_processing_impl.cc (right): https://codereview.webrtc.org/1510493004/diff/180001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl.cc:920: if (public_submodules_->echo_control_mobile->is_enabled() && Why is this change in this CL? https://codereview.webrtc.org/1510493004/diff/180001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/high_pass_filter_bitexactness_unittest.cc (right): https://codereview.webrtc.org/1510493004/diff/180001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/high_pass_filter_bitexactness_unittest.cc:23: class HighPassFilterBitExactnessTest { Great stuff! I think this would be even more readable if you skipped the test class and just made it a function (doing so would also omit the need to copy the input+reference), but this is good enough. https://codereview.webrtc.org/1510493004/diff/180001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/high_pass_filter_bitexactness_unittest.cc:25: explicit HighPassFilterBitExactnessTest(int sample_rate, explicit is not necessary https://codereview.webrtc.org/1510493004/diff/180001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/high_pass_filter_bitexactness_unittest.cc:26: std::vector<float> input, const & https://codereview.webrtc.org/1510493004/diff/180001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/high_pass_filter_bitexactness_unittest.cc:45: EXPECT_PRED_FORMAT2(AssertVectorsNotEqual, output, reference_); Why do you just check the last frame? In this case I would have made one test case with a single impulse as the input and capturing the majority of the IR, for reverse engineering the filter design. :) https://codereview.webrtc.org/1510493004/diff/180001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/high_pass_filter_bitexactness_unittest.cc:49: ::testing::AssertionResult AssertVectorsNotEqual( Move out from class; it doesn't really have anything to do with it. https://codereview.webrtc.org/1510493004/diff/180001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/high_pass_filter_bitexactness_unittest.cc:117: const std::vector<float> kReferenceInput = { This type of initialization is banned: http://chromium-cpp.appspot.com/ You'll have to use a C array instead (the other code may benefit from ArrayView for smooth interfacing). https://codereview.webrtc.org/1510493004/diff/180001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/high_pass_filter_bitexactness_unittest.cc:228: TEST(HighPassFilterBitExactnessTest, Mono16kHzConverged) { You should cover a stereo (or multi-channel) case as well, with individual inputs to make sure channels are processed separately. https://codereview.webrtc.org/1510493004/diff/180001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/test/audio_buffer_tools.cc (right): https://codereview.webrtc.org/1510493004/diff/180001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/audio_buffer_tools.cc:17: std::vector<float>* frame_samples) { nit: RTC_DCHECK(frame); RTC_DCHECK(frame_samples); https://codereview.webrtc.org/1510493004/diff/180001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/audio_buffer_tools.cc:21: for (int ch = 0; ch < stream_config.num_channels(); ++ch) { May need to: int -> size_t (see https://codereview.webrtc.org/1316523002/)
https://codereview.webrtc.org/1510493004/diff/180001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/test/audio_buffer_tools.h (right): https://codereview.webrtc.org/1510493004/diff/180001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/audio_buffer_tools.h:21: void CopyVectorToAudioBuffer(StreamConfig stream_config, Nice. Could this perhaps live in test_utils.* instead?
I don't have any other comments than what solenberg already provided. https://codereview.webrtc.org/1510493004/diff/180001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/high_pass_filter_bitexactness_unittest.cc (right): https://codereview.webrtc.org/1510493004/diff/180001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/high_pass_filter_bitexactness_unittest.cc:25: explicit HighPassFilterBitExactnessTest(int sample_rate, On 2016/01/07 14:35:35, the sun wrote: > explicit is not necessary Acknowledged.
https://codereview.webrtc.org/1510493004/diff/180001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/audio_processing_impl.cc (right): https://codereview.webrtc.org/1510493004/diff/180001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl.cc:920: if (public_submodules_->echo_control_mobile->is_enabled() && On 2016/01/07 14:35:35, the sun wrote: > Why is this change in this CL? Oups! That was supposed to go with another CL. Removed it. Thanks! https://codereview.webrtc.org/1510493004/diff/180001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/high_pass_filter_bitexactness_unittest.cc (right): https://codereview.webrtc.org/1510493004/diff/180001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/high_pass_filter_bitexactness_unittest.cc:23: class HighPassFilterBitExactnessTest { On 2016/01/07 14:35:35, the sun wrote: > Great stuff! > > I think this would be even more readable if you skipped the test class and just > made it a function (doing so would also omit the need to copy the > input+reference), but this is good enough. Good! I made this into functions, which definitely made the code better. https://codereview.webrtc.org/1510493004/diff/180001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/high_pass_filter_bitexactness_unittest.cc:25: explicit HighPassFilterBitExactnessTest(int sample_rate, On 2016/01/07 14:35:35, the sun wrote: > explicit is not necessary Thanks! I removed this since there is no longer any class. Acknowledged. https://codereview.webrtc.org/1510493004/diff/180001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/high_pass_filter_bitexactness_unittest.cc:26: std::vector<float> input, On 2016/01/07 14:35:35, the sun wrote: > const & Done. https://codereview.webrtc.org/1510493004/diff/180001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/high_pass_filter_bitexactness_unittest.cc:45: EXPECT_PRED_FORMAT2(AssertVectorsNotEqual, output, reference_); On 2016/01/07 14:35:35, the sun wrote: > Why do you just check the last frame? The idea is to be able to avoid having to store all the reference samples up until that frame. And since the method being tested has a memory, checking the last frame also implicitly entails checking the preceeding frames. > > In this case I would have made one test case with a single impulse as the input > and capturing the majority of the IR, for reverse engineering the filter design. > :) True, but since the momentary values depend on previous values in this case, I think just checking one frame in the end actually means that the whole IR is tested, at least if sufficiently many frames have been tested (just testing the first frame is most likely not sufficient). A single impulse should be fine, but then care must be taken that the number of frames analyzed is not too big, and not too short. The advantage of the current test is that tests the whole IR, regardless of the coefficients. The disadvantage is that it requires the full input signal to be stored. https://codereview.webrtc.org/1510493004/diff/180001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/high_pass_filter_bitexactness_unittest.cc:49: ::testing::AssertionResult AssertVectorsNotEqual( On 2016/01/07 14:35:35, the sun wrote: > Move out from class; it doesn't really have anything to do with it. Done. https://codereview.webrtc.org/1510493004/diff/180001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/high_pass_filter_bitexactness_unittest.cc:117: const std::vector<float> kReferenceInput = { On 2016/01/07 14:35:35, the sun wrote: > This type of initialization is banned: http://chromium-cpp.appspot.com/ > > You'll have to use a C array instead (the other code may benefit from ArrayView > for smooth interfacing). Done. https://codereview.webrtc.org/1510493004/diff/180001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/high_pass_filter_bitexactness_unittest.cc:228: TEST(HighPassFilterBitExactnessTest, Mono16kHzConverged) { On 2016/01/07 14:35:35, the sun wrote: > You should cover a stereo (or multi-channel) case as well, with individual > inputs to make sure channels are processed separately. Good point! Added stereo.
Very nice! https://codereview.webrtc.org/1510493004/diff/200001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/high_pass_filter_bitexactness_unittest.cc (right): https://codereview.webrtc.org/1510493004/diff/200001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/high_pass_filter_bitexactness_unittest.cc:16: #include "webrtc/modules/audio_processing/audio_processing_impl.h" Why do you need this? https://codereview.webrtc.org/1510493004/diff/200001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/high_pass_filter_bitexactness_unittest.cc:34: equal &= (fabs(output[k] - reference[k]) <= threshold); It's kind of ugly to do bitwise and on two bools. How about changing this to a if (... > threshold) { equal = false; break; } instead? https://codereview.webrtc.org/1510493004/diff/200001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/high_pass_filter_bitexactness_unittest.cc:94: input.size() / stream_config.num_frames() / stream_config.num_channels(); You should use parenthesis around one of these pairs. AFAIK the ordering is not specified for operations of same precedence. https://codereview.webrtc.org/1510493004/diff/200001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/high_pass_filter_bitexactness_unittest.cc:107: // Form vector to compare the reference to. Add comment explaining why you only compare last frame. https://codereview.webrtc.org/1510493004/diff/200001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/high_pass_filter_bitexactness_unittest.cc:110: std::vector<float> output_to_verify(0); remove "(0)" https://codereview.webrtc.org/1510493004/diff/200001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/high_pass_filter_bitexactness_unittest.cc:123: std::vector<float> CreateVector(const rtc::ArrayView<const float>& array_view) { Add a comment that this should be removed once we can use braced initialization. https://codereview.webrtc.org/1510493004/diff/200001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/test/audio_buffer_tools.cc (right): https://codereview.webrtc.org/1510493004/diff/200001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/audio_buffer_tools.cc:20: frame->resize(stream_config.num_frames()); should be sized to stream_config.num_channels() https://codereview.webrtc.org/1510493004/diff/200001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/test/audio_buffer_tools.h (right): https://codereview.webrtc.org/1510493004/diff/200001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/audio_buffer_tools.h:19: Since these are for tests, add the nested namespace test { https://codereview.webrtc.org/1510493004/diff/200001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/audio_buffer_tools.h:21: void CopyVectorToAudioBuffer(StreamConfig stream_config, nit: const StreamConfig& (and below)
https://codereview.webrtc.org/1510493004/diff/200001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/high_pass_filter_bitexactness_unittest.cc (right): https://codereview.webrtc.org/1510493004/diff/200001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/high_pass_filter_bitexactness_unittest.cc:16: #include "webrtc/modules/audio_processing/audio_processing_impl.h" On 2016/02/22 19:50:58, the sun wrote: > Why do you need this? Not needed any more :-). Removed it. Done. https://codereview.webrtc.org/1510493004/diff/200001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/high_pass_filter_bitexactness_unittest.cc:34: equal &= (fabs(output[k] - reference[k]) <= threshold); On 2016/02/22 19:50:58, the sun wrote: > It's kind of ugly to do bitwise and on two bools. How about changing this to a > if (... > threshold) { > equal = false; > break; > } > instead? Done. https://codereview.webrtc.org/1510493004/diff/200001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/high_pass_filter_bitexactness_unittest.cc:94: input.size() / stream_config.num_frames() / stream_config.num_channels(); On 2016/02/22 19:50:58, the sun wrote: > You should use parenthesis around one of these pairs. AFAIK the ordering is not > specified for operations of same precedence. Done. https://codereview.webrtc.org/1510493004/diff/200001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/high_pass_filter_bitexactness_unittest.cc:107: // Form vector to compare the reference to. On 2016/02/22 19:50:58, the sun wrote: > Add comment explaining why you only compare last frame. Done. https://codereview.webrtc.org/1510493004/diff/200001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/high_pass_filter_bitexactness_unittest.cc:110: std::vector<float> output_to_verify(0); On 2016/02/22 19:50:58, the sun wrote: > remove "(0)" Done. https://codereview.webrtc.org/1510493004/diff/200001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/high_pass_filter_bitexactness_unittest.cc:123: std::vector<float> CreateVector(const rtc::ArrayView<const float>& array_view) { On 2016/02/22 19:50:58, the sun wrote: > Add a comment that this should be removed once we can use braced initialization. Done. https://codereview.webrtc.org/1510493004/diff/200001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/test/audio_buffer_tools.cc (right): https://codereview.webrtc.org/1510493004/diff/200001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/audio_buffer_tools.cc:20: frame->resize(stream_config.num_frames()); On 2016/02/22 19:50:58, the sun wrote: > should be sized to stream_config.num_channels() Great find! Done. https://codereview.webrtc.org/1510493004/diff/200001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/test/audio_buffer_tools.h (right): https://codereview.webrtc.org/1510493004/diff/200001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/audio_buffer_tools.h:19: On 2016/02/22 19:50:58, the sun wrote: > Since these are for tests, add the nested > namespace test { Done. https://codereview.webrtc.org/1510493004/diff/200001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/audio_buffer_tools.h:21: void CopyVectorToAudioBuffer(StreamConfig stream_config, On 2016/02/22 19:50:58, the sun wrote: > nit: const StreamConfig& > (and below) 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/1510493004/220001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1510493004/220001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios_arm64_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_dbg/builds/7653) ios_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_rel/builds/11479) mac_compile_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_compile_dbg/builds/...) mac_gn_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_gn_rel/builds/7220) mac_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_rel/builds/12886)
lgtm % last set of comments https://codereview.webrtc.org/1510493004/diff/220001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/high_pass_filter_bitexactness_unittest.cc (right): https://codereview.webrtc.org/1510493004/diff/220001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/high_pass_filter_bitexactness_unittest.cc:14: #include "webrtc/base/scoped_ptr.h" Not needed https://codereview.webrtc.org/1510493004/diff/220001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/high_pass_filter_bitexactness_unittest.cc:112: // estvectors. As the algorithm being tested has a memory, testing only "estvector"? Not familiar with the term. You call it reference elsewhere... https://codereview.webrtc.org/1510493004/diff/220001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/high_pass_filter_bitexactness_unittest.cc:544: 0.147160f, 0.495163f, -0.648346f, 0.234931f, 0.075289f, -0.373779f, Use different input for left/right channels to catch potential mixup of channel order and processing. https://codereview.webrtc.org/1510493004/diff/220001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/test/audio_buffer_tools.h (right): https://codereview.webrtc.org/1510493004/diff/220001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/audio_buffer_tools.h:16: #include "webrtc/modules/audio_processing/include/audio_processing.h" What is audio_processing needed for?
lgtm % solenberg's comments.
https://codereview.webrtc.org/1510493004/diff/220001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/high_pass_filter_bitexactness_unittest.cc (right): https://codereview.webrtc.org/1510493004/diff/220001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/high_pass_filter_bitexactness_unittest.cc:14: #include "webrtc/base/scoped_ptr.h" On 2016/02/23 08:53:39, the sun wrote: > Not needed Done. https://codereview.webrtc.org/1510493004/diff/220001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/high_pass_filter_bitexactness_unittest.cc:112: // estvectors. As the algorithm being tested has a memory, testing only On 2016/02/23 08:53:39, the sun wrote: > "estvector"? Not familiar with the term. You call it reference elsewhere... Sorry, that is a typo :-) Should be testvector. But it is still not consistent with the rest of the comments so I'll change it to "inputs". https://codereview.webrtc.org/1510493004/diff/220001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/high_pass_filter_bitexactness_unittest.cc:112: // estvectors. As the algorithm being tested has a memory, testing only On 2016/02/23 08:53:39, the sun wrote: > "estvector"? Not familiar with the term. You call it reference elsewhere... Done. https://codereview.webrtc.org/1510493004/diff/220001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/high_pass_filter_bitexactness_unittest.cc:544: 0.147160f, 0.495163f, -0.648346f, 0.234931f, 0.075289f, -0.373779f, On 2016/02/23 08:53:39, the sun wrote: > Use different input for left/right channels to catch potential mixup of channel > order and processing. Done.
peah@webrtc.org changed reviewers: - ivoc@webrtc.org
The CQ bit was checked by peah@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from solenberg@webrtc.org, henrik.lundin@webrtc.org Link to the patchset: https://codereview.webrtc.org/1510493004/#ps240001 (title: "Removed redundant include, updated test input to be different for different channels, changed comme…")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1510493004/240001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1510493004/240001
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/5625) ios_arm64_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_rel/builds/7871) ios_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_dbg/builds/13145) ios_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_rel/builds/11782) 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/1274) mac_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_rel/builds/13177)
The CQ bit was checked by peah@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from solenberg@webrtc.org, henrik.lundin@webrtc.org Link to the patchset: https://codereview.webrtc.org/1510493004/#ps280001 (title: "Reverted changes to audio_processing_impl.cc")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1510493004/280001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1510493004/280001
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) win_baremetal 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/1510493004/280001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1510493004/280001
Message was sent while issue was closed.
Description was changed from ========== A bitexactness test for the highpass filter in the audio processing module. The test also adds a new helper class called VectorBasedAudioFrame that is intended to be reused for the bitexactness tests for the other submodules. BUG=webrtc:1091 ========== to ========== A bitexactness test for the highpass filter in the audio processing module. The test also adds a new helper class called VectorBasedAudioFrame that is intended to be reused for the bitexactness tests for the other submodules. BUG=webrtc:1091 ==========
Message was sent while issue was closed.
Committed patchset #13 (id:280001)
Message was sent while issue was closed.
Description was changed from ========== A bitexactness test for the highpass filter in the audio processing module. The test also adds a new helper class called VectorBasedAudioFrame that is intended to be reused for the bitexactness tests for the other submodules. BUG=webrtc:1091 ========== to ========== A bitexactness test for the highpass filter in the audio processing module. The test also adds a new helper class called VectorBasedAudioFrame that is intended to be reused for the bitexactness tests for the other submodules. BUG=webrtc:1091 Committed: https://crrev.com/0197363d18354af532b3a01abfd5d79fa63454c6 Cr-Commit-Position: refs/heads/master@{#11864} ==========
Message was sent while issue was closed.
Patchset 13 (id:??) landed as https://crrev.com/0197363d18354af532b3a01abfd5d79fa63454c6 Cr-Commit-Position: refs/heads/master@{#11864} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
