Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1393353003:
Adding debug dump tests. (Closed)
|
Created:
5 years, 2 months ago by minyue-webrtc Modified:
5 years, 1 month ago CC:
webrtc-reviews_webrtc.org, Andrew MacDonald, tterriberry_mozilla.com, hlundin-webrtc, kwiberg-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. |
DescriptionAdding debug dump test.
This test is to verify that the debug dump can perfectly reproduce APM states if the recording is made from the first input sample.
BUG=
Committed: https://crrev.com/275d255e210a1110db3f2faf14a0ded0a656a0b9
Cr-Commit-Position: refs/heads/master@{#10506}
Patch Set 1 : #
Total comments: 59
Patch Set 2 : merge h into cc #Patch Set 3 : after Andrew's review #
Total comments: 12
Patch Set 4 : refine #
Total comments: 18
Patch Set 5 : retouch #
Total comments: 3
Patch Set 6 : refinement #Patch Set 7 : rebase #Patch Set 8 : disable an irrelavent test on Android #
Messages
Total messages: 34 (11 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
minyue@webrtc.org changed reviewers: + andrew@webrtc.org
Patchset #1 (id:40001) has been deleted
Hi Andrew, This is a test for debug dump. Could you help review it?
minyue@webrtc.org changed reviewers: + peah@webrtc.org
On 2015/10/14 19:39:38, minyue-webrtc wrote: > Hi Andrew, > > This is a test for debug dump. Could you help review it? +peah
On 2015/10/16 09:48:31, minyue-webrtc wrote: > On 2015/10/14 19:39:38, minyue-webrtc wrote: > > Hi Andrew, > > > > This is a test for debug dump. Could you help review it? > > +peah Hi, I think it looks great! Most likely you should be able to reduce the number of frames processed though. 50 should definitely be ok. But I think it is also good as it is. lgtm
https://codereview.webrtc.org/1393353003/diff/60001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/test/debug_dump_test.cc (right): https://codereview.webrtc.org/1393353003/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/debug_dump_test.cc:21: namespace { Blank space after this line. https://codereview.webrtc.org/1393353003/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/debug_dump_test.cc:22: const std::string input_file_name = No indent. Use git cl format. These strings are non-POD, so shouldn't be statics. Use class members instead. https://codereview.webrtc.org/1393353003/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/debug_dump_test.cc:30: } Blank space before this line. } // namespace https://codereview.webrtc.org/1393353003/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/debug_dump_test.cc:54: // Buffers will be created upon usage. You could trigger the same InitializeFormat or whatever method I suggest below from the constructor. https://codereview.webrtc.org/1393353003/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/debug_dump_test.cc:63: RTC_DCHECK(input_audio_.get()); This is created in the constructor, so no need for this. https://codereview.webrtc.org/1393353003/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/debug_dump_test.cc:72: output_channels_ = 1; What if ForceInputMono(false) is called? https://codereview.webrtc.org/1393353003/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/debug_dump_test.cc:77: RTC_DCHECK(reverse_audio_.get()); Not needed. https://codereview.webrtc.org/1393353003/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/debug_dump_test.cc:87: RTC_DCHECK(apm_.get()); Not needed. https://codereview.webrtc.org/1393353003/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/debug_dump_test.cc:92: RTC_DCHECK(apm_.get()); Not needed. https://codereview.webrtc.org/1393353003/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/debug_dump_test.cc:102: if (!input_.get() || input_->num_frames() < apm_input_frames || Since this is a test, we don't care about performance. Perhaps have a InitializeFormat method instead of the setters and recreate the buffers there? https://codereview.webrtc.org/1393353003/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/debug_dump_test.cc:177: #ifdef WEBRTC_AUDIOPROC_DEBUG_DUMP This file should only be built when this is defined, right? You shouldn't need it. https://codereview.webrtc.org/1393353003/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/debug_dump_test.cc:194: void DebugDumpTest::VerifyDebugDump(const std::string in_filename) { const std::string& https://codereview.webrtc.org/1393353003/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/debug_dump_test.cc:196: ASSERT_TRUE(in_file != NULL); nullptr https://codereview.webrtc.org/1393353003/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/debug_dump_test.cc:265: if (!input_.get() || input_->num_frames() < frames_per_input_channel || An init event has to occur for these values to change, right? Just recreate the ChannelBuffers immediately there? https://codereview.webrtc.org/1393353003/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/debug_dump_test.cc:282: apm_->ProcessStream(input_->channels(), Can you use the ProcessStream overload with StreamConfigs? This one is deprecated. https://codereview.webrtc.org/1393353003/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/debug_dump_test.cc:290: // Check that output of APM is bit exact identical to the output in the dump. s/bit exact identical/bit-exact https://codereview.webrtc.org/1393353003/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/debug_dump_test.cc:309: if (!reverse_.get() || reverse_->num_frames() < frames_per_channel || Same thing: recreate this directly upon init event? https://codereview.webrtc.org/1393353003/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/debug_dump_test.cc:321: apm_->AnalyzeReverseStream( Can you use ProcessReverseStream instead? This is deprecated. https://codereview.webrtc.org/1393353003/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/debug_dump_test.cc:360: void DebugDumpTest::ConfigurateApm(const audioproc::Config& msg) { ConfigureApm https://codereview.webrtc.org/1393353003/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/debug_dump_test.cc:426: Config config; It looks like you never do anything with config in every test. Is that right? Just remove it in that case. https://codereview.webrtc.org/1393353003/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/debug_dump_test.cc:428: test::TempFilename(test::OutputPath(), "debug_aec"); You use the same name in every test. I'd make dump_file_name_ a member of DebugDumpTest, initialized in the constructor. You can then also remove() the file in the destructor. https://codereview.webrtc.org/1393353003/diff/60001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/test/debug_dump_test.h (right): https://codereview.webrtc.org/1393353003/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/debug_dump_test.h:31: class DebugDumpGenerator { Do you foresee anything else ever using this? If not, you could put the declaration direction in the cc file. https://codereview.webrtc.org/1393353003/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/debug_dump_test.h:35: size_t input_channels, Since these values are ints in the protobuf generated classes, I think it's better to use ints here. Bonus: no casting. https://codereview.webrtc.org/1393353003/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/debug_dump_test.h:76: int input_rate_hz_; Can any of these be const? https://codereview.webrtc.org/1393353003/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/debug_dump_test.h:84: rtc::scoped_ptr<ResampleInputAudioFile> input_audio_; You initialize the ResampleInputAudioFiles in the ctor, so you shouldn't need to use scoped_ptrs. https://codereview.webrtc.org/1393353003/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/debug_dump_test.h:85: size_t input_channels_; const? https://codereview.webrtc.org/1393353003/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/debug_dump_test.h:98: std::string dump_file_name_; const https://codereview.webrtc.org/1393353003/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/debug_dump_test.h:104: class DebugDumpTest : public ::testing::Test { This should definitely go in the cc file.
Thanks for the comments! A new patch is formed, see Patchset 3. Patchset 2 is added to make diff view on Patchset 3 easier. It only moves the h file into the cc. https://codereview.webrtc.org/1393353003/diff/60001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/test/debug_dump_test.cc (right): https://codereview.webrtc.org/1393353003/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/debug_dump_test.cc:21: namespace { On 2015/10/20 01:22:08, Andrew MacDonald wrote: > Blank space after this line. Done. https://codereview.webrtc.org/1393353003/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/debug_dump_test.cc:22: const std::string input_file_name = On 2015/10/20 01:22:08, Andrew MacDonald wrote: > No indent. Use git cl format. > > These strings are non-POD, so shouldn't be statics. Use class members instead. Done. https://codereview.webrtc.org/1393353003/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/debug_dump_test.cc:30: } On 2015/10/20 01:22:08, Andrew MacDonald wrote: > Blank space before this line. > > } // namespace Done. https://codereview.webrtc.org/1393353003/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/debug_dump_test.cc:54: // Buffers will be created upon usage. On 2015/10/20 01:22:08, Andrew MacDonald wrote: > You could trigger the same InitializeFormat or whatever method I suggest below > from the constructor. Acknowledged. https://codereview.webrtc.org/1393353003/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/debug_dump_test.cc:63: RTC_DCHECK(input_audio_.get()); On 2015/10/20 01:22:08, Andrew MacDonald wrote: > This is created in the constructor, so no need for this. Acknowledged. https://codereview.webrtc.org/1393353003/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/debug_dump_test.cc:72: output_channels_ = 1; On 2015/10/20 01:22:08, Andrew MacDonald wrote: > What if ForceInputMono(false) is called? yes agreed. I think it is better not to change the output channels silently like this. Caller must make sure that output channel number is smaller than or equal to input. https://codereview.webrtc.org/1393353003/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/debug_dump_test.cc:77: RTC_DCHECK(reverse_audio_.get()); On 2015/10/20 01:22:08, Andrew MacDonald wrote: > Not needed. Acknowledged. https://codereview.webrtc.org/1393353003/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/debug_dump_test.cc:87: RTC_DCHECK(apm_.get()); On 2015/10/20 01:22:08, Andrew MacDonald wrote: > Not needed. Acknowledged. https://codereview.webrtc.org/1393353003/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/debug_dump_test.cc:92: RTC_DCHECK(apm_.get()); On 2015/10/20 01:22:08, Andrew MacDonald wrote: > Not needed. Acknowledged. https://codereview.webrtc.org/1393353003/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/debug_dump_test.cc:102: if (!input_.get() || input_->num_frames() < apm_input_frames || On 2015/10/20 01:22:08, Andrew MacDonald wrote: > Since this is a test, we don't care about performance. Perhaps have a > InitializeFormat method instead of the setters and recreate the buffers there? Done. https://codereview.webrtc.org/1393353003/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/debug_dump_test.cc:177: #ifdef WEBRTC_AUDIOPROC_DEBUG_DUMP On 2015/10/20 01:22:08, Andrew MacDonald wrote: > This file should only be built when this is defined, right? You shouldn't need > it. Ironically, the DebugDumpGenerator can still work, since APM has WEBRTC_AUDIOPROC_DEBUG_DUMP inside. Then no dump can be generated and DebugDumpTest won't work. Therefore I placed the WEBRTC_AUDIOPROC_DEBUG_DUMP around the implementation of DebugDumpTest. https://codereview.webrtc.org/1393353003/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/debug_dump_test.cc:194: void DebugDumpTest::VerifyDebugDump(const std::string in_filename) { On 2015/10/20 01:22:08, Andrew MacDonald wrote: > const std::string& Done. https://codereview.webrtc.org/1393353003/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/debug_dump_test.cc:196: ASSERT_TRUE(in_file != NULL); On 2015/10/20 01:22:08, Andrew MacDonald wrote: > nullptr maybe better to remove !=NULL at all https://codereview.webrtc.org/1393353003/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/debug_dump_test.cc:265: if (!input_.get() || input_->num_frames() < frames_per_input_channel || On 2015/10/20 01:22:08, Andrew MacDonald wrote: > An init event has to occur for these values to change, right? Just recreate the > ChannelBuffers immediately there? Done. https://codereview.webrtc.org/1393353003/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/debug_dump_test.cc:282: apm_->ProcessStream(input_->channels(), On 2015/10/20 01:22:08, Andrew MacDonald wrote: > Can you use the ProcessStream overload with StreamConfigs? This one is > deprecated. ok https://codereview.webrtc.org/1393353003/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/debug_dump_test.cc:290: // Check that output of APM is bit exact identical to the output in the dump. On 2015/10/20 01:22:08, Andrew MacDonald wrote: > s/bit exact identical/bit-exact Done. https://codereview.webrtc.org/1393353003/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/debug_dump_test.cc:309: if (!reverse_.get() || reverse_->num_frames() < frames_per_channel || On 2015/10/20 01:22:08, Andrew MacDonald wrote: > Same thing: recreate this directly upon init event? Done. https://codereview.webrtc.org/1393353003/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/debug_dump_test.cc:321: apm_->AnalyzeReverseStream( On 2015/10/20 01:22:08, Andrew MacDonald wrote: > Can you use ProcessReverseStream instead? This is deprecated. ok https://codereview.webrtc.org/1393353003/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/debug_dump_test.cc:360: void DebugDumpTest::ConfigurateApm(const audioproc::Config& msg) { On 2015/10/20 01:22:08, Andrew MacDonald wrote: > ConfigureApm Done. https://codereview.webrtc.org/1393353003/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/debug_dump_test.cc:426: Config config; On 2015/10/20 01:22:08, Andrew MacDonald wrote: > It looks like you never do anything with config in every test. Is that right? > Just remove it in that case. There are tests with config modified. see Line 536 e,g. https://codereview.webrtc.org/1393353003/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/debug_dump_test.cc:428: test::TempFilename(test::OutputPath(), "debug_aec"); On 2015/10/20 01:22:08, Andrew MacDonald wrote: > You use the same name in every test. I'd make dump_file_name_ a member of > DebugDumpTest, initialized in the constructor. > > You can then also remove() the file in the destructor. I made dump_file_name_ a member in the generator class. That reads more nicely I think https://codereview.webrtc.org/1393353003/diff/60001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/test/debug_dump_test.h (right): https://codereview.webrtc.org/1393353003/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/debug_dump_test.h:31: class DebugDumpGenerator { On 2015/10/20 01:22:09, Andrew MacDonald wrote: > Do you foresee anything else ever using this? If not, you could put the > declaration direction in the cc file. I thought that cc is too large and therefore put this h. You are right. I'd get them into one cc file. https://codereview.webrtc.org/1393353003/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/debug_dump_test.h:35: size_t input_channels, On 2015/10/20 01:22:09, Andrew MacDonald wrote: > Since these values are ints in the protobuf generated classes, I think it's > better to use ints here. Bonus: no casting. Done. https://codereview.webrtc.org/1393353003/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/debug_dump_test.h:76: int input_rate_hz_; On 2015/10/20 01:22:09, Andrew MacDonald wrote: > Can any of these be const? No. We want to change sample rate in the middle, to verify that the dumping cope well with sample rate changes. https://codereview.webrtc.org/1393353003/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/debug_dump_test.h:84: rtc::scoped_ptr<ResampleInputAudioFile> input_audio_; On 2015/10/20 01:22:09, Andrew MacDonald wrote: > You initialize the ResampleInputAudioFiles in the ctor, so you shouldn't need to > use scoped_ptrs. Done. https://codereview.webrtc.org/1393353003/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/debug_dump_test.h:85: size_t input_channels_; On 2015/10/20 01:22:09, Andrew MacDonald wrote: > const? Yes, this can be const https://codereview.webrtc.org/1393353003/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/debug_dump_test.h:98: std::string dump_file_name_; On 2015/10/20 01:22:09, Andrew MacDonald wrote: > const Done. https://codereview.webrtc.org/1393353003/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/debug_dump_test.h:104: class DebugDumpTest : public ::testing::Test { On 2015/10/20 01:22:09, Andrew MacDonald wrote: > This should definitely go in the cc file. Done.
lgtm assuming you agree with my comments :) https://codereview.webrtc.org/1393353003/diff/60001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/test/debug_dump_test.cc (right): https://codereview.webrtc.org/1393353003/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/debug_dump_test.cc:177: #ifdef WEBRTC_AUDIOPROC_DEBUG_DUMP On 2015/10/23 08:44:45, minyue-webrtc wrote: > On 2015/10/20 01:22:08, Andrew MacDonald wrote: > > This file should only be built when this is defined, right? You shouldn't need > > it. > > Ironically, the DebugDumpGenerator can still work, since APM has > WEBRTC_AUDIOPROC_DEBUG_DUMP inside. Then no dump can be generated and > DebugDumpTest won't work. > > Therefore I placed the WEBRTC_AUDIOPROC_DEBUG_DUMP around the implementation of > DebugDumpTest. But this file is only compiled when enable_protobuf==1. You _know_ this is going to be defined. https://codereview.webrtc.org/1393353003/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/debug_dump_test.cc:426: Config config; On 2015/10/23 08:44:45, minyue-webrtc wrote: > On 2015/10/20 01:22:08, Andrew MacDonald wrote: > > It looks like you never do anything with config in every test. Is that right? > > Just remove it in that case. > > There are tests with config modified. see Line 536 e,g. Ah, OK. https://codereview.webrtc.org/1393353003/diff/100001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/test/debug_dump_test.cc (right): https://codereview.webrtc.org/1393353003/diff/100001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/debug_dump_test.cc:11: #include <stddef.h> // size_t nit: I prefer cstddef, but up to you. https://codereview.webrtc.org/1393353003/diff/100001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/debug_dump_test.cc:34: static void MaybeResetBuffer(rtc::scoped_ptr<ChannelBuffer<float>>& buffer, Remove static. The unnamed namespace gives it internal linkage. Since it's mutable, buffer should be a pointer not a reference. https://codereview.webrtc.org/1393353003/diff/100001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/debug_dump_test.cc:47: DebugDumpGenerator(std::string input_file_name, const std::string& and below. https://codereview.webrtc.org/1393353003/diff/100001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/debug_dump_test.cc:256: const size_t frames_per_channel = config.num_frames(); No. frames == samples_per_channel. Use num_frames. https://codereview.webrtc.org/1393353003/diff/100001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/debug_dump_test.cc:266: // We only allow reducing number channel by discarding some channels. number of channels
Hi, Here is an update. One thing that needs Andrew's input is about scoped_ptr&, see inline. https://codereview.webrtc.org/1393353003/diff/60001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/test/debug_dump_test.cc (right): https://codereview.webrtc.org/1393353003/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/debug_dump_test.cc:177: #ifdef WEBRTC_AUDIOPROC_DEBUG_DUMP On 2015/10/24 00:42:05, Andrew MacDonald wrote: > On 2015/10/23 08:44:45, minyue-webrtc wrote: > > On 2015/10/20 01:22:08, Andrew MacDonald wrote: > > > This file should only be built when this is defined, right? You shouldn't > need > > > it. > > > > Ironically, the DebugDumpGenerator can still work, since APM has > > WEBRTC_AUDIOPROC_DEBUG_DUMP inside. Then no dump can be generated and > > DebugDumpTest won't work. > > > > Therefore I placed the WEBRTC_AUDIOPROC_DEBUG_DUMP around the implementation > of > > DebugDumpTest. > > But this file is only compiled when enable_protobuf==1. You _know_ this is going > to be defined. Now removed https://codereview.webrtc.org/1393353003/diff/100001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/test/debug_dump_test.cc (right): https://codereview.webrtc.org/1393353003/diff/100001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/debug_dump_test.cc:11: #include <stddef.h> // size_t On 2015/10/24 00:42:06, Andrew MacDonald wrote: > nit: I prefer cstddef, but up to you. I don't actually know the pros and cons. here I found some comparison http://stackoverflow.com/questions/5079325/should-i-include-stddef-h-or-cstdd... https://codereview.webrtc.org/1393353003/diff/100001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/debug_dump_test.cc:34: static void MaybeResetBuffer(rtc::scoped_ptr<ChannelBuffer<float>>& buffer, On 2015/10/24 00:42:05, Andrew MacDonald wrote: > Remove static. The unnamed namespace gives it internal linkage. > > Since it's mutable, buffer should be a pointer not a reference. Thanks! I actually wanted to consult you about this. Pointer does not really work here since I do not know of a way of getting a pointer to a scoped_ptr. I see that the swap method of scoped_ptr is implemented with a scoped_ptr& and just followed that way. Do you have a better solution? https://codereview.webrtc.org/1393353003/diff/100001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/debug_dump_test.cc:47: DebugDumpGenerator(std::string input_file_name, On 2015/10/24 00:42:06, Andrew MacDonald wrote: > const std::string& and below. Done. https://codereview.webrtc.org/1393353003/diff/100001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/debug_dump_test.cc:256: const size_t frames_per_channel = config.num_frames(); On 2015/10/24 00:42:06, Andrew MacDonald wrote: > No. frames == samples_per_channel. Use num_frames. Done. https://codereview.webrtc.org/1393353003/diff/100001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/debug_dump_test.cc:266: // We only allow reducing number channel by discarding some channels. On 2015/10/24 00:42:05, Andrew MacDonald wrote: > number of channels Done.
I have some minor comments on the code, but I think it looks good. https://codereview.webrtc.org/1393353003/diff/120001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/test/debug_dump_test.cc (right): https://codereview.webrtc.org/1393353003/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/debug_dump_test.cc:31: void MaybeResetBuffer(rtc::scoped_ptr<ChannelBuffer<float>>& buffer, Since this is only used inside the DebugDumpGenerator, would it make sense to have it as a static method in that class? https://codereview.webrtc.org/1393353003/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/debug_dump_test.cc:42: class DebugDumpGenerator { Since this class is only used inside this function, there should be no need to put it in the webrtc namespace, right? It should be possible to have it in the anonymous namespace I think. https://codereview.webrtc.org/1393353003/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/debug_dump_test.cc:85: void ReadAndDeinterleave(ResampleInputAudioFile* audio, int channels, This does not at all depend on the state apart from the vector signal_. Would it make sense to instead create the vector signal_ a local variable inside this method and make the method static? Otherwise I think the method should be declared const and the signal_ vector be declared mutable, as it is only accessed by this method. https://codereview.webrtc.org/1393353003/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/debug_dump_test.cc:88: void MonoToStereo(float* const* buffer, size_t num_frames); I cannot find the implementation of this method. It should be in this file, right? https://codereview.webrtc.org/1393353003/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/debug_dump_test.cc:90: // APM input/output settings Should end with a "." https://codereview.webrtc.org/1393353003/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/debug_dump_test.cc:298: ASSERT_TRUE(false); Should use FAIL() instead. https://codereview.webrtc.org/1393353003/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/debug_dump_test.cc:491: // #channel of out put should not be larger than that of input. APM will fail Number of output channels should....
https://codereview.webrtc.org/1393353003/diff/100001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/test/debug_dump_test.cc (right): https://codereview.webrtc.org/1393353003/diff/100001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/debug_dump_test.cc:34: static void MaybeResetBuffer(rtc::scoped_ptr<ChannelBuffer<float>>& buffer, On 2015/10/26 12:40:31, minyue-webrtc wrote: > On 2015/10/24 00:42:05, Andrew MacDonald wrote: > > Remove static. The unnamed namespace gives it internal linkage. > > > > Since it's mutable, buffer should be a pointer not a reference. > > Thanks! I actually wanted to consult you about this. Pointer does not really > work here since I do not know of a way of getting a pointer to a scoped_ptr. I > see that the swap method of scoped_ptr is implemented with a scoped_ptr& and > just followed that way. > > Do you have a better solution? Yes, you can dereference the scoped_ptr<T>*: void MaybeResetBuffer(rtc::scoped_ptr<ChannelBuffer<float>>* buffer) { (*buffer).reset(new ChannelBuffer<float>(...)); }
Patchset #5 (id:140001) has been deleted
Thanks. Now see some updates https://codereview.webrtc.org/1393353003/diff/100001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/test/debug_dump_test.cc (right): https://codereview.webrtc.org/1393353003/diff/100001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/debug_dump_test.cc:34: static void MaybeResetBuffer(rtc::scoped_ptr<ChannelBuffer<float>>& buffer, On 2015/10/30 01:56:08, Andrew MacDonald wrote: > On 2015/10/26 12:40:31, minyue-webrtc wrote: > > On 2015/10/24 00:42:05, Andrew MacDonald wrote: > > > Remove static. The unnamed namespace gives it internal linkage. > > > > > > Since it's mutable, buffer should be a pointer not a reference. > > > > Thanks! I actually wanted to consult you about this. Pointer does not really > > work here since I do not know of a way of getting a pointer to a scoped_ptr. I > > see that the swap method of scoped_ptr is implemented with a scoped_ptr& and > > just followed that way. > > > > Do you have a better solution? > > Yes, you can dereference the scoped_ptr<T>*: > > void MaybeResetBuffer(rtc::scoped_ptr<ChannelBuffer<float>>* buffer) { > (*buffer).reset(new ChannelBuffer<float>(...)); > } Cool! It works! https://codereview.webrtc.org/1393353003/diff/120001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/test/debug_dump_test.cc (right): https://codereview.webrtc.org/1393353003/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/debug_dump_test.cc:31: void MaybeResetBuffer(rtc::scoped_ptr<ChannelBuffer<float>>& buffer, On 2015/10/29 22:25:02, peah-webrtc wrote: > Since this is only used inside the DebugDumpGenerator, would it make sense to > have it as a static method in that class? It is used in other test, see line 318 https://codereview.webrtc.org/1393353003/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/debug_dump_test.cc:42: class DebugDumpGenerator { On 2015/10/29 22:25:02, peah-webrtc wrote: > Since this class is only used inside this function, there should be no need to > put it in the webrtc namespace, right? It should be possible to have it in the > anonymous namespace I think. Ok. I made a change. I needed to move some blocks. See the new patch. https://codereview.webrtc.org/1393353003/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/debug_dump_test.cc:85: void ReadAndDeinterleave(ResampleInputAudioFile* audio, int channels, On 2015/10/29 22:25:02, peah-webrtc wrote: > This does not at all depend on the state apart from the vector signal_. Would it > make sense to instead create the vector signal_ a local variable inside this > method and make the method static? > Otherwise I think the method should be declared const and the signal_ vector be > declared mutable, as it is only accessed by this method. agree! https://codereview.webrtc.org/1393353003/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/debug_dump_test.cc:88: void MonoToStereo(float* const* buffer, size_t num_frames); On 2015/10/29 22:25:02, peah-webrtc wrote: > I cannot find the implementation of this method. It should be in this file, > right? Oh, good catch? why is it still there. I thought I needed it to make fake stereo, but it turned out that, with the default file, which is stereo, I need StereoToMono instead. I just forgot to remove this from the class definition. https://codereview.webrtc.org/1393353003/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/debug_dump_test.cc:90: // APM input/output settings On 2015/10/29 22:25:02, peah-webrtc wrote: > Should end with a "." Done. https://codereview.webrtc.org/1393353003/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/debug_dump_test.cc:298: ASSERT_TRUE(false); On 2015/10/29 22:25:02, peah-webrtc wrote: > Should use FAIL() instead. Done. https://codereview.webrtc.org/1393353003/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/debug_dump_test.cc:491: // #channel of out put should not be larger than that of input. APM will fail On 2015/10/29 22:25:02, peah-webrtc wrote: > Number of output channels should.... Done.
https://codereview.webrtc.org/1393353003/diff/120001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/test/debug_dump_test.cc (right): https://codereview.webrtc.org/1393353003/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/debug_dump_test.cc:26: namespace webrtc { Why did you remove this namespace? It's fine to put test tools in the webrtc namespace. https://codereview.webrtc.org/1393353003/diff/160001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/test/debug_dump_test.cc (right): https://codereview.webrtc.org/1393353003/diff/160001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/debug_dump_test.cc:36: if (!(*buffer).get() || (*buffer)->num_frames() != config.num_frames() || Consider adding a local reference to avoid all the dereferencing: auto& buffer_ref = *buffer; https://codereview.webrtc.org/1393353003/diff/160001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/debug_dump_test.cc:38: (*buffer).reset(new webrtc::ChannelBuffer<float>(config.num_frames(), In any case, you have a using webrtc::ChannelBuffer declaration above, so this qualification is unnecessary.
https://codereview.webrtc.org/1393353003/diff/120001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/test/debug_dump_test.cc (right): https://codereview.webrtc.org/1393353003/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/debug_dump_test.cc:26: namespace webrtc { On 2015/10/30 16:26:06, Andrew MacDonald wrote: > Why did you remove this namespace? It's fine to put test tools in the webrtc > namespace. Per mentioned that DebugDumpGenerator is used only in this test (he said "function", which I do not fully understand) and suggested to place it in an anonymous namespace. I don't quite know pros and cons. https://codereview.webrtc.org/1393353003/diff/160001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/test/debug_dump_test.cc (right): https://codereview.webrtc.org/1393353003/diff/160001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/debug_dump_test.cc:38: (*buffer).reset(new webrtc::ChannelBuffer<float>(config.num_frames(), On 2015/10/30 16:26:06, Andrew MacDonald wrote: > In any case, you have a using webrtc::ChannelBuffer declaration above, so this > qualification is unnecessary. right. I found tried to place it all places and feel it was quite a lot before I made "use". This one was forgotten to be changed back
https://codereview.webrtc.org/1393353003/diff/120001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/test/debug_dump_test.cc (right): https://codereview.webrtc.org/1393353003/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/debug_dump_test.cc:26: namespace webrtc { On 2015/10/30 16:32:25, minyue-webrtc wrote: > On 2015/10/30 16:26:06, Andrew MacDonald wrote: > > Why did you remove this namespace? It's fine to put test tools in the webrtc > > namespace. > > Per mentioned that DebugDumpGenerator is used only in this test (he said > "function", which I do not fully understand) and suggested to place it in an > anonymous namespace. I don't quite know pros and cons. You have an anonymous namespace starting on line 26, and it's still fine to wrap that in the webrtc namespace.
https://codereview.webrtc.org/1393353003/diff/120001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/test/debug_dump_test.cc (right): https://codereview.webrtc.org/1393353003/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/test/debug_dump_test.cc:26: namespace webrtc { On 2015/10/30 16:39:42, Andrew MacDonald wrote: > On 2015/10/30 16:32:25, minyue-webrtc wrote: > > On 2015/10/30 16:26:06, Andrew MacDonald wrote: > > > Why did you remove this namespace? It's fine to put test tools in the webrtc > > > namespace. > > > > Per mentioned that DebugDumpGenerator is used only in this test (he said > > "function", which I do not fully understand) and suggested to place it in an > > anonymous namespace. I don't quite know pros and cons. > > You have an anonymous namespace starting on line 26, and it's still fine to wrap > that in the webrtc namespace. ah, I see that you mean a wrapped anonymous namespace.
btw, a new patch is ready.
any more concerns? (I ask because you have given lgtms :))
lgtm
lgtm
The CQ bit was checked by minyue@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1393353003/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1393353003/180001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_rel/builds/8744)
Patchset #8 (id:220001) has been deleted
The CQ bit was checked by minyue@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from peah@webrtc.org, andrew@webrtc.org Link to the patchset: https://codereview.webrtc.org/1393353003/#ps240001 (title: "disable an irrelavent test on Android")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1393353003/240001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1393353003/240001
Message was sent while issue was closed.
Committed patchset #8 (id:240001)
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/275d255e210a1110db3f2faf14a0ded0a656a0b9 Cr-Commit-Position: refs/heads/master@{#10506} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
