Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1287663002:
Adding audio RepetitionDetector in AudioProcessingModule. (Closed)
|
Created:
5 years, 4 months ago by minyue-webrtc Modified:
5 years ago CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com, hlundin-webrtc, kwiberg-webrtc, aluebs-webrtc, bjornv1 Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionAdding audio RepetitionDetector in AudioProcessingModule.
BUG=
Patch Set 1 : #
Total comments: 46
Patch Set 2 : some redesign #
Total comments: 52
Patch Set 3 : restrict to float #
Total comments: 13
Patch Set 4 : new updates #
Total comments: 27
Patch Set 5 : more comments addressed #
Messages
Total messages: 45 (6 generated)
Patchset #1 (id:1) has been deleted
minyue@webrtc.org changed reviewers: + henrik.lundin@webrtc.org, peah@webrtc.org
Patchset #1 (id:20001) has been deleted
Hi Henrik and Per, Here I implemented the basic algorithm for the repetition detection. Missing parts include 1. Actual repetition needed to include 2. Wiring up UMA reporting I'd like to listen to your early inputs on the methodology. You may also wait until the missing parts are finished. I will send a notice then.
andrew@webrtc.org changed reviewers: + andrew@webrtc.org
Drive-by. Do you think this is of interest to any users outside of Chromium? I'm wondering if it make sense to integrate directly there, rather than through APM. https://codereview.webrtc.org/1287663002/diff/40001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/audio_processing_impl.cc (right): https://codereview.webrtc.org/1287663002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl.cc:215: repetition_detector_(new RepetitionDetector()) { We usually lazily initialize components, so you would create this only if requested.
On 2015/08/11 18:53:05, andrew_ooo_to_aug14 wrote: > Drive-by. > > Do you think this is of interest to any users outside of Chromium? I'm wondering > if it make sense to integrate directly there, rather than through APM. > > https://codereview.webrtc.org/1287663002/diff/40001/webrtc/modules/audio_proc... > File webrtc/modules/audio_processing/audio_processing_impl.cc (right): > > https://codereview.webrtc.org/1287663002/diff/40001/webrtc/modules/audio_proc... > webrtc/modules/audio_processing/audio_processing_impl.cc:215: > repetition_detector_(new RepetitionDetector()) { > We usually lazily initialize components, so you would create this only if > requested. Do you mean "in" Chromium? Efforts to catch the true cause of repeated audio in Chromium is being conducted as well. It is not clear when a solution will be found. Since APM is the most frontier receiver of repeated audio (no other obvious place to surely observe repeated audio at the moment), it would be good to collect data here, which can be used to help identifying the problem and verifying any future solutions.
On 2015/08/11 19:04:30, minyue-webrtc wrote: > Do you mean "in" Chromium? Yep. > > Efforts to catch the true cause of repeated audio in Chromium is being conducted > as well. It is not clear when a solution will be found. > > Since APM is the most frontier receiver of repeated audio (no other obvious > place to surely observe repeated audio at the moment), it would be good to > collect data here, which can be used to help identifying the problem and > verifying any future solutions. You could call in to your repeated audio detector at the same place where APM is called in Chromium and avoid another layer. It's not clear to me which choice is better, but I wanted to point it out as an alternative in case you hadn't considered it.
https://codereview.webrtc.org/1287663002/diff/40001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/audio_processing_impl.cc (right): https://codereview.webrtc.org/1287663002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl.cc:574: repetition_detector_->Detect(frame->data_, frame->samples_per_channel_, All processing/analysis should be done in ProcessStreamLocked. https://codereview.webrtc.org/1287663002/diff/40001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/repetition_detector.h (right): https://codereview.webrtc.org/1287663002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/repetition_detector.h:39: void Detect(const int16_t* data, int samples_per_channel, float is preferred now. https://codereview.webrtc.org/1287663002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/repetition_detector.h:71: int16_t** audio_buffer_; // Ring buffers to store input audio channels. Use vectors for this. (Or consider AudioRingBuffer if you want something full-featured.)
On 2015/08/11 20:44:57, andrew_ooo_to_aug14 wrote: > On 2015/08/11 19:04:30, minyue-webrtc wrote: > > Do you mean "in" Chromium? > > Yep. > > > > > Efforts to catch the true cause of repeated audio in Chromium is being > conducted > > as well. It is not clear when a solution will be found. > > > > Since APM is the most frontier receiver of repeated audio (no other obvious > > place to surely observe repeated audio at the moment), it would be good to > > collect data here, which can be used to help identifying the problem and > > verifying any future solutions. > > You could call in to your repeated audio detector at the same place where APM is > called in Chromium and avoid another layer. It's not clear to me which choice is > better, but I wanted to point it out as an alternative in case you hadn't > considered it. I see, and will try to look deeper. Thanks!
https://codereview.webrtc.org/1287663002/diff/40001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/repetition_detector.cc (right): https://codereview.webrtc.org/1287663002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/repetition_detector.cc:20: static const RepetitionDetector::Pattern kRepetitionPatterns[] = { Both look_back_range_ and length_range_ are zero both in the code and the unittests. Therefore the code corresponding to these is not being exercised and is untested (at least in this code). Please add active use of that code (by using nonzero values of the aforementioned variables), both in the unittests and the detector. https://codereview.webrtc.org/1287663002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/repetition_detector.cc:42: void RepetitionDetector::RegisterRepititionPatterns(const Pattern* patterns, Typo, should be RegisterRepetitionPatterns https://codereview.webrtc.org/1287663002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/repetition_detector.cc:48: for (int offset = -pattern.look_back_range_; In the specified patterns, look_back_range_ is set to zero. What is the intended role of it being nonzero? The number of states, and the complexity depends a lot on the values for this. https://codereview.webrtc.org/1287663002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/repetition_detector.cc:60: void RepetitionDetector::ClearRepititionPatterns() { A typo, should be ClearRepetitionPatterns https://codereview.webrtc.org/1287663002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/repetition_detector.cc:114: buffer_length_samples_) % buffer_length_samples_; The addition of buffer_length_samples_ seems to be redundant, as it always results in 0 in the modulus operation https://codereview.webrtc.org/1287663002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/repetition_detector.cc:116: // For multichannel audio, all channels have to repeat the same way. Is this a neccessary requirement, would it not just be sufficient to look at the leftmost channel for repetitions? If we are to look at the repetitions for both channels, I think we should look at them separately, and count a repetition if any of them contains a repetition https://codereview.webrtc.org/1287663002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/repetition_detector.cc:129: if (state.count_ >= state.length_ - state.length_range_ && I'm not fully sure about the role of the length_range_ since it is always set to zero in the code, could you please explain its role in a comment, or specify a pattern where it is nonzero. https://codereview.webrtc.org/1287663002/diff/40001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/repetition_detector.h (right): https://codereview.webrtc.org/1287663002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/repetition_detector.h:29: // rate, since repetition patterns are supposedly independent of sample I'm not sure that the repetition pattern length is independent of the sample rate. Is that the case?
Thank you for the comments! Updates may follow. But we can continue discuss over the open questions. https://codereview.webrtc.org/1287663002/diff/40001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/audio_processing_impl.cc (right): https://codereview.webrtc.org/1287663002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl.cc:215: repetition_detector_(new RepetitionDetector()) { On 2015/08/11 18:53:05, andrew_ooo_to_aug14 wrote: > We usually lazily initialize components, so you would create this only if > requested. Acknowledged. https://codereview.webrtc.org/1287663002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl.cc:574: repetition_detector_->Detect(frame->data_, frame->samples_per_channel_, On 2015/08/11 20:50:42, andrew_ooo_to_aug14 wrote: > All processing/analysis should be done in ProcessStreamLocked. We'd better access raw data (frame), ProcessStreamLocked() does not have access to it. What would you suggest? https://codereview.webrtc.org/1287663002/diff/40001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/repetition_detector.cc (right): https://codereview.webrtc.org/1287663002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/repetition_detector.cc:20: static const RepetitionDetector::Pattern kRepetitionPatterns[] = { On 2015/08/12 11:02:54, peah-webrtc wrote: > Both look_back_range_ and length_range_ are zero both in the code and the > unittests. Therefore the code corresponding to these is not being exercised and > is untested (at least in this code). Please add active use of that code (by > using nonzero values of the aforementioned variables), both in the unittests and > the detector. Maybe not enough but there is a unittest around this, called RangedLookBack. https://codereview.webrtc.org/1287663002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/repetition_detector.cc:42: void RepetitionDetector::RegisterRepititionPatterns(const Pattern* patterns, On 2015/08/12 11:02:54, peah-webrtc wrote: > Typo, should be RegisterRepetitionPatterns Acknowledged. https://codereview.webrtc.org/1287663002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/repetition_detector.cc:48: for (int offset = -pattern.look_back_range_; On 2015/08/12 11:02:54, peah-webrtc wrote: > In the specified patterns, look_back_range_ is set to zero. What is the intended > role of it being nonzero? The number of states, and the complexity depends a lot > on the values for this. I think the look_back and length may not be necessarily exact. It may be too strict if we allow only exact match. But this is disputable. https://codereview.webrtc.org/1287663002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/repetition_detector.cc:60: void RepetitionDetector::ClearRepititionPatterns() { On 2015/08/12 11:02:54, peah-webrtc wrote: > A typo, should be ClearRepetitionPatterns Acknowledged. https://codereview.webrtc.org/1287663002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/repetition_detector.cc:114: buffer_length_samples_) % buffer_length_samples_; On 2015/08/12 11:02:54, peah-webrtc wrote: > The addition of buffer_length_samples_ seems to be redundant, as it always > results in 0 in the modulus operation This is to avoid negative, e,g, -2%3=-2 https://codereview.webrtc.org/1287663002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/repetition_detector.cc:116: // For multichannel audio, all channels have to repeat the same way. On 2015/08/12 11:02:54, peah-webrtc wrote: > Is this a neccessary requirement, would it not just be sufficient to look at the > leftmost channel for repetitions? If we are to look at the repetitions for both > channels, I think we should look at them separately, and count a repetition if > any of them contains a repetition I consulted Chromium people. It is supposed that should always happen in both channels. But this is disputable too, since detection may be made looser. https://codereview.webrtc.org/1287663002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/repetition_detector.cc:129: if (state.count_ >= state.length_ - state.length_range_ && On 2015/08/12 11:02:54, peah-webrtc wrote: > I'm not fully sure about the role of the length_range_ since it is always set to > zero in the code, could you please explain its role in a comment, or specify a > pattern where it is nonzero. see discussion on look_back_range https://codereview.webrtc.org/1287663002/diff/40001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/repetition_detector.h (right): https://codereview.webrtc.org/1287663002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/repetition_detector.h:29: // rate, since repetition patterns are supposedly independent of sample On 2015/08/12 11:02:54, peah-webrtc wrote: > I'm not sure that the repetition pattern length is independent of the sample > rate. Is that the case? Right. I had this in my mind when I wrote this, and planned to consult Chromium people on this. I wrote it out anyway knowing that the change will be small if we change the unit to a time unit. Now I've known that it is better to use time unit and a change is at hand. https://codereview.webrtc.org/1287663002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/repetition_detector.h:39: void Detect(const int16_t* data, int samples_per_channel, On 2015/08/11 20:50:42, andrew_ooo_to_aug14 wrote: > float is preferred now. Oh. We depend strongly on the assumption that the repetition consists of bit exact replica. Is that then too strict an assumption? https://codereview.webrtc.org/1287663002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/repetition_detector.h:71: int16_t** audio_buffer_; // Ring buffers to store input audio channels. On 2015/08/11 20:50:42, andrew_ooo_to_aug14 wrote: > Use vectors for this. (Or consider AudioRingBuffer if you want something > full-featured.) Acknowledged.
Hi, Per Andrew's consideration of moving this out of APM and place in higher level, I am considering following. I do think it makes better sense there. 1. VoEBaseImpl::OnData() 2. WebRtcVoiceMediaChannel::OnData()
I think this design is too complicated. Either compare only one channel, or explore my suggestion to use memcmp to compare one frame as a one-liner. I didn't look at the unit tests yet. I will come back to those when the next patch set is up. :) https://codereview.webrtc.org/1287663002/diff/40001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/audio_processing_impl.cc (right): https://codereview.webrtc.org/1287663002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl.cc:631: Why new line? https://codereview.webrtc.org/1287663002/diff/40001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/repetition_detector.cc (right): https://codereview.webrtc.org/1287663002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/repetition_detector.cc:95: // We do not reset |occurances_| to be able to count issues that have What is occurances_? https://codereview.webrtc.org/1287663002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/repetition_detector.cc:106: int look_back_pointer; This is not a pointer. Suggest look_back_index or look_back_ix. And declare it in the for loop instead. https://codereview.webrtc.org/1287663002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/repetition_detector.cc:110: for (int idx = 0; idx < samples_per_channel; idx++) { This nested for loop and its associated code is very complicated. Shouldn't the following be enough? for each offset to check { if 0 == memcmp(audio_buffer_[at offset], data, length(data)); // Compare all channels. // We have a match; report it. } // Write all of data to buffer in one operation. https://codereview.webrtc.org/1287663002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/repetition_detector.cc:116: // For multichannel audio, all channels have to repeat the same way. On 2015/08/12 11:32:14, minyue-webrtc wrote: > On 2015/08/12 11:02:54, peah-webrtc wrote: > > Is this a neccessary requirement, would it not just be sufficient to look at > the > > leftmost channel for repetitions? If we are to look at the repetitions for > both > > channels, I think we should look at them separately, and count a repetition if > > any of them contains a repetition > > I consulted Chromium people. It is supposed that should always happen in both > channels. But this is disputable too, since detection may be made looser. I agree with Per. Look at only one channel for now. https://codereview.webrtc.org/1287663002/diff/40001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/repetition_detector.h (right): https://codereview.webrtc.org/1287663002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/repetition_detector.h:39: void Detect(const int16_t* data, int samples_per_channel, Use size_t for samples_per_channel and num_channels. https://codereview.webrtc.org/1287663002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/repetition_detector.h:43: void RegisterRepititionPatterns(const Pattern* patterns, Why not use some std container class instead of ol' array-and-length? https://codereview.webrtc.org/1287663002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/repetition_detector.h:44: int num_patterns); Align. https://codereview.webrtc.org/1287663002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/repetition_detector.h:58: int look_back_; I'd prefer size_t for the length and count variables.
see some quick reply https://codereview.webrtc.org/1287663002/diff/40001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/repetition_detector.cc (right): https://codereview.webrtc.org/1287663002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/repetition_detector.cc:95: // We do not reset |occurances_| to be able to count issues that have On 2015/08/12 14:12:04, hlundin-webrtc wrote: > What is occurances_? Sorry. it was a counter, but now removed, since I plan to do one report at a time. https://codereview.webrtc.org/1287663002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/repetition_detector.cc:110: for (int idx = 0; idx < samples_per_channel; idx++) { On 2015/08/12 14:12:04, hlundin-webrtc wrote: > This nested for loop and its associated code is very complicated. Shouldn't the > following be enough? > > for each offset to check { > if 0 == memcmp(audio_buffer_[at offset], data, length(data)); // Compare all > channels. > // We have a match; report it. > } > > // Write all of data to buffer in one operation. It can happen that repetition starts in the middle of a frame?
https://codereview.webrtc.org/1287663002/diff/40001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/repetition_detector.cc (right): https://codereview.webrtc.org/1287663002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/repetition_detector.cc:110: for (int idx = 0; idx < samples_per_channel; idx++) { On 2015/08/12 14:20:20, minyue-webrtc wrote: > On 2015/08/12 14:12:04, hlundin-webrtc wrote: > > This nested for loop and its associated code is very complicated. Shouldn't > the > > following be enough? > > > > for each offset to check { > > if 0 == memcmp(audio_buffer_[at offset], data, length(data)); // Compare > all > > channels. > > // We have a match; report it. > > } > > > > // Write all of data to buffer in one operation. > > It can happen that repetition starts in the middle of a frame? Hm, ok. But I still have a feeling you could simplify the implementation. I may be wrong...
On 2015/08/12 13:09:43, minyue-webrtc wrote: > Hi, > > Per Andrew's consideration of moving this out of APM and place in higher level, > I am considering following. I do think it makes better sense there. > > 1. VoEBaseImpl::OnData() > > 2. WebRtcVoiceMediaChannel::OnData() Chromium does not use the audio processing in voice engine. It calls into APM directly. See here: https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/m...
On 2015/08/12 20:19:58, andrew_ooo_to_aug14 wrote: > On 2015/08/12 13:09:43, minyue-webrtc wrote: > > Hi, > > > > Per Andrew's consideration of moving this out of APM and place in higher > level, > > I am considering following. I do think it makes better sense there. > > > > 1. VoEBaseImpl::OnData() > > > > 2. WebRtcVoiceMediaChannel::OnData() > > Chromium does not use the audio processing in voice engine. It calls into APM > directly. See here: > https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/m... So it uses the float-point API? Thanks for pointing this out. I am confused by the different APM APIs and their usage. Anyways, I will test Chromium before landing this work.
https://codereview.webrtc.org/1287663002/diff/40001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/audio_processing_impl.cc (right): https://codereview.webrtc.org/1287663002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl.cc:574: repetition_detector_->Detect(frame->data_, frame->samples_per_channel_, On 2015/08/12 11:32:13, minyue-webrtc wrote: > On 2015/08/11 20:50:42, andrew_ooo_to_aug14 wrote: > > All processing/analysis should be done in ProcessStreamLocked. > > We'd better access raw data (frame), ProcessStreamLocked() does not have access > to it. What would you suggest? See how the other components access the data (through AudioBuffer). You're right that resampling could occur before you get access to the audio, but that could have happened outside APM already anyway. I'd attempt to make your algorithm robust to minor variations. https://codereview.webrtc.org/1287663002/diff/40001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/repetition_detector.h (right): https://codereview.webrtc.org/1287663002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/repetition_detector.h:39: void Detect(const int16_t* data, int samples_per_channel, On 2015/08/12 11:32:14, minyue-webrtc wrote: > On 2015/08/11 20:50:42, andrew_ooo_to_aug14 wrote: > > float is preferred now. > > Oh. We depend strongly on the assumption that the repetition consists of bit > exact replica. Is that then too strict an assumption? Not necessarily, but the Chromium audio pipeline is float, so you gain nothing by making this int16.
On 2015/08/12 20:24:44, minyue-webrtc wrote: > On 2015/08/12 20:19:58, andrew_ooo_to_aug14 wrote: > > Chromium does not use the audio processing in voice engine. It calls into APM > > directly. See here: > > > https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/m... > > So it uses the float-point API? Yes.
https://codereview.webrtc.org/1287663002/diff/40001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/audio_processing_impl.cc (right): https://codereview.webrtc.org/1287663002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl.cc:574: repetition_detector_->Detect(frame->data_, frame->samples_per_channel_, On 2015/08/12 20:25:32, andrew_ooo_to_aug14 wrote: > On 2015/08/12 11:32:13, minyue-webrtc wrote: > > On 2015/08/11 20:50:42, andrew_ooo_to_aug14 wrote: > > > All processing/analysis should be done in ProcessStreamLocked. > > > > We'd better access raw data (frame), ProcessStreamLocked() does not have > access > > to it. What would you suggest? > > See how the other components access the data (through AudioBuffer). You're right > that resampling could occur before you get access to the audio, but that could > have happened outside APM already anyway. I'd attempt to make your algorithm > robust to minor variations. That makes sense. I today looked into a recording where the matching was not 100% identical sample wise but where the difference was tiny and it was clearly repeated audio. https://codereview.webrtc.org/1287663002/diff/40001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/repetition_detector.cc (right): https://codereview.webrtc.org/1287663002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/repetition_detector.cc:20: static const RepetitionDetector::Pattern kRepetitionPatterns[] = { On 2015/08/12 11:32:14, minyue-webrtc wrote: > On 2015/08/12 11:02:54, peah-webrtc wrote: > > Both look_back_range_ and length_range_ are zero both in the code and the > > unittests. Therefore the code corresponding to these is not being exercised > and > > is untested (at least in this code). Please add active use of that code (by > > using nonzero values of the aforementioned variables), both in the unittests > and > > the detector. > > Maybe not enough but there is a unittest around this, called RangedLookBack. Sorry, I missed the unit test. Great! https://codereview.webrtc.org/1287663002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/repetition_detector.cc:110: for (int idx = 0; idx < samples_per_channel; idx++) { On 2015/08/12 14:38:00, hlundin-webrtc wrote: > On 2015/08/12 14:20:20, minyue-webrtc wrote: > > On 2015/08/12 14:12:04, hlundin-webrtc wrote: > > > This nested for loop and its associated code is very complicated. Shouldn't > > the > > > following be enough? > > > > > > for each offset to check { > > > if 0 == memcmp(audio_buffer_[at offset], data, length(data)); // Compare > > all > > > channels. > > > // We have a match; report it. > > > } > > > > > > // Write all of data to buffer in one operation. > > > > It can happen that repetition starts in the middle of a frame? > > Hm, ok. But I still have a feeling you could simplify the implementation. I may > be wrong... I agree that this feels more complex than needed. And for sure if the look_back_range and length_range are used it could get significantly complex. Since this occurs rather seldomly, I think it would make sense to optimize for the most common case. What about a probalistic solution where for each frame (for simplicity assuming a linear buffer) { repetition_detected = false; buffer_index = last_buffer_insertion_index-frame_length; // Assume at least one frame of repetition length while (buffer_index > (last_buffer_insertion_index - 10 * frame_length) // Search over 10 frames { // Find a match candidate if two values in a row are identical (also need to take values 0, 32767, -32767 and -32768 specially into account (see below) if ((sample[0] == audio_buffer[buffer_index]) && (sample[1] == audio_buffer[buffer_index+1])) { // Assume 10 matching samples in a row sufficient for a repeated audio being present with a very high probability int k; for (k = 0; k < 8; k++) if ( sample[2+k] != audio_buffer[buffer_index+2+k]) break; if (k == 8) SendUMAMessage("Repetition found") } buffer_index--; } } That code will find any type of repetition pattern, but will fail to report the length of the repetition. https://codereview.webrtc.org/1287663002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/repetition_detector.cc:114: buffer_length_samples_) % buffer_length_samples_; On 2015/08/12 11:32:13, minyue-webrtc wrote: > On 2015/08/12 11:02:54, peah-webrtc wrote: > > The addition of buffer_length_samples_ seems to be redundant, as it always > > results in 0 in the modulus operation > > This is to avoid negative, e,g, -2%3=-2 Good point! Thanks! https://codereview.webrtc.org/1287663002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/repetition_detector.cc:120: sample_repeated = false; Inequality is not a sufficient condition for repeated audio. That would for instance cause a constant zero signal to count as repeated audio, which is not neccessarily. Most likely we should treat the value zero specially, as well as the saturating values (in case we have a very strong sound constantly saturating the microphone signal (-32768, -32767 and 32767 in 16 bit integers). We most probably also should take a constant DC offset from zero as a special case, depending on whether we HP filter the signal before the check or not.
On 2015/08/12 21:05:16, peah-webrtc wrote: > https://codereview.webrtc.org/1287663002/diff/40001/webrtc/modules/audio_proc... > File webrtc/modules/audio_processing/audio_processing_impl.cc (right): > > https://codereview.webrtc.org/1287663002/diff/40001/webrtc/modules/audio_proc... > webrtc/modules/audio_processing/audio_processing_impl.cc:574: > repetition_detector_->Detect(frame->data_, frame->samples_per_channel_, > On 2015/08/12 20:25:32, andrew_ooo_to_aug14 wrote: > > On 2015/08/12 11:32:13, minyue-webrtc wrote: > > > On 2015/08/11 20:50:42, andrew_ooo_to_aug14 wrote: > > > > All processing/analysis should be done in ProcessStreamLocked. > > > > > > We'd better access raw data (frame), ProcessStreamLocked() does not have > > access > > > to it. What would you suggest? > > > > See how the other components access the data (through AudioBuffer). You're > right > > that resampling could occur before you get access to the audio, but that could > > have happened outside APM already anyway. I'd attempt to make your algorithm > > robust to minor variations. > > That makes sense. I today looked into a recording where the matching was not > 100% identical sample wise but where the difference was tiny and it was clearly > repeated audio. > > https://codereview.webrtc.org/1287663002/diff/40001/webrtc/modules/audio_proc... > File webrtc/modules/audio_processing/repetition_detector.cc (right): > > https://codereview.webrtc.org/1287663002/diff/40001/webrtc/modules/audio_proc... > webrtc/modules/audio_processing/repetition_detector.cc:20: static const > RepetitionDetector::Pattern kRepetitionPatterns[] = { > On 2015/08/12 11:32:14, minyue-webrtc wrote: > > On 2015/08/12 11:02:54, peah-webrtc wrote: > > > Both look_back_range_ and length_range_ are zero both in the code and the > > > unittests. Therefore the code corresponding to these is not being exercised > > and > > > is untested (at least in this code). Please add active use of that code (by > > > using nonzero values of the aforementioned variables), both in the unittests > > and > > > the detector. > > > > Maybe not enough but there is a unittest around this, called RangedLookBack. > > Sorry, I missed the unit test. Great! > > https://codereview.webrtc.org/1287663002/diff/40001/webrtc/modules/audio_proc... > webrtc/modules/audio_processing/repetition_detector.cc:110: for (int idx = 0; > idx < samples_per_channel; idx++) { > On 2015/08/12 14:38:00, hlundin-webrtc wrote: > > On 2015/08/12 14:20:20, minyue-webrtc wrote: > > > On 2015/08/12 14:12:04, hlundin-webrtc wrote: > > > > This nested for loop and its associated code is very complicated. > Shouldn't > > > the > > > > following be enough? > > > > > > > > for each offset to check { > > > > if 0 == memcmp(audio_buffer_[at offset], data, length(data)); // > Compare > > > all > > > > channels. > > > > // We have a match; report it. > > > > } > > > > > > > > // Write all of data to buffer in one operation. > > > > > > It can happen that repetition starts in the middle of a frame? > > > > Hm, ok. But I still have a feeling you could simplify the implementation. I > may > > be wrong... > > I agree that this feels more complex than needed. And for sure if the > look_back_range and length_range are used it could get significantly complex. > > Since this occurs rather seldomly, I think it would make sense to optimize for > the most common case. > > What about a probalistic solution where for each frame (for simplicity assuming > a linear buffer) > { > repetition_detected = false; > buffer_index = last_buffer_insertion_index-frame_length; // Assume at least one > frame of repetition length > while (buffer_index > (last_buffer_insertion_index - 10 * frame_length) // > Search over 10 frames > { > // Find a match candidate if two values in a row are identical (also need to > take values 0, 32767, -32767 and -32768 specially into account (see below) > if ((sample[0] == audio_buffer[buffer_index]) && (sample[1] == > audio_buffer[buffer_index+1])) { > // Assume 10 matching samples in a row sufficient for a repeated audio > being present with a very high probability > int k; > for (k = 0; k < 8; k++) > if ( sample[2+k] != audio_buffer[buffer_index+2+k]) > break; > if (k == 8) > SendUMAMessage("Repetition found") > } > buffer_index--; > } > } > > That code will find any type of repetition pattern, but will fail to report the > length of the repetition. The reason of using matched loopback and length is to be able to report more information than yes/no in the report, yet avoiding a pair<int, int> report, which UMA cannot handle and is too big computation. This is more of a tradeoff among demands/feasibility/computation. We need to understand 1. if it is needed to analyze what patterns on what OS etc. 2. if length is an import dimension. I feel so since we saw partial repetition like 123xx123yy 3. if we can list patterns we are interested in, and ignore other opportunistic repetitions. > > https://codereview.webrtc.org/1287663002/diff/40001/webrtc/modules/audio_proc... > webrtc/modules/audio_processing/repetition_detector.cc:114: > buffer_length_samples_) % buffer_length_samples_; > On 2015/08/12 11:32:13, minyue-webrtc wrote: > > On 2015/08/12 11:02:54, peah-webrtc wrote: > > > The addition of buffer_length_samples_ seems to be redundant, as it always > > > results in 0 in the modulus operation > > > > This is to avoid negative, e,g, -2%3=-2 > > Good point! Thanks! > > https://codereview.webrtc.org/1287663002/diff/40001/webrtc/modules/audio_proc... > webrtc/modules/audio_processing/repetition_detector.cc:120: sample_repeated = > false; > Inequality is not a sufficient condition for repeated audio. That would for > instance cause a constant zero signal to count as repeated audio, which is not > neccessarily. This will not, since the length will not match. > > Most likely we should treat the value zero specially, as well as the saturating > values (in case we have a very strong sound constantly saturating the microphone > signal (-32768, -32767 and 32767 in 16 bit integers). > > We most probably also should take a constant DC offset from zero as a special > case, depending on whether we HP filter the signal before the check or not.
On 2015/08/12 22:02:44, minyue-webrtc wrote: > On 2015/08/12 21:05:16, peah-webrtc wrote: > > > https://codereview.webrtc.org/1287663002/diff/40001/webrtc/modules/audio_proc... > > File webrtc/modules/audio_processing/audio_processing_impl.cc (right): > > > > > https://codereview.webrtc.org/1287663002/diff/40001/webrtc/modules/audio_proc... > > webrtc/modules/audio_processing/audio_processing_impl.cc:574: > > repetition_detector_->Detect(frame->data_, frame->samples_per_channel_, > > On 2015/08/12 20:25:32, andrew_ooo_to_aug14 wrote: > > > On 2015/08/12 11:32:13, minyue-webrtc wrote: > > > > On 2015/08/11 20:50:42, andrew_ooo_to_aug14 wrote: > > > > > All processing/analysis should be done in ProcessStreamLocked. > > > > > > > > We'd better access raw data (frame), ProcessStreamLocked() does not have > > > access > > > > to it. What would you suggest? > > > > > > See how the other components access the data (through AudioBuffer). You're > > right > > > that resampling could occur before you get access to the audio, but that > could > > > have happened outside APM already anyway. I'd attempt to make your algorithm > > > robust to minor variations. > > > > That makes sense. I today looked into a recording where the matching was not > > 100% identical sample wise but where the difference was tiny and it was > clearly > > repeated audio. > > > > > https://codereview.webrtc.org/1287663002/diff/40001/webrtc/modules/audio_proc... > > File webrtc/modules/audio_processing/repetition_detector.cc (right): > > > > > https://codereview.webrtc.org/1287663002/diff/40001/webrtc/modules/audio_proc... > > webrtc/modules/audio_processing/repetition_detector.cc:20: static const > > RepetitionDetector::Pattern kRepetitionPatterns[] = { > > On 2015/08/12 11:32:14, minyue-webrtc wrote: > > > On 2015/08/12 11:02:54, peah-webrtc wrote: > > > > Both look_back_range_ and length_range_ are zero both in the code and the > > > > unittests. Therefore the code corresponding to these is not being > exercised > > > and > > > > is untested (at least in this code). Please add active use of that code > (by > > > > using nonzero values of the aforementioned variables), both in the > unittests > > > and > > > > the detector. > > > > > > Maybe not enough but there is a unittest around this, called RangedLookBack. > > > > Sorry, I missed the unit test. Great! > > > > > https://codereview.webrtc.org/1287663002/diff/40001/webrtc/modules/audio_proc... > > webrtc/modules/audio_processing/repetition_detector.cc:110: for (int idx = 0; > > idx < samples_per_channel; idx++) { > > On 2015/08/12 14:38:00, hlundin-webrtc wrote: > > > On 2015/08/12 14:20:20, minyue-webrtc wrote: > > > > On 2015/08/12 14:12:04, hlundin-webrtc wrote: > > > > > This nested for loop and its associated code is very complicated. > > Shouldn't > > > > the > > > > > following be enough? > > > > > > > > > > for each offset to check { > > > > > if 0 == memcmp(audio_buffer_[at offset], data, length(data)); // > > Compare > > > > all > > > > > channels. > > > > > // We have a match; report it. > > > > > } > > > > > > > > > > // Write all of data to buffer in one operation. > > > > > > > > It can happen that repetition starts in the middle of a frame? > > > > > > Hm, ok. But I still have a feeling you could simplify the implementation. I > > may > > > be wrong... > > > > I agree that this feels more complex than needed. And for sure if the > > look_back_range and length_range are used it could get significantly complex. > > > > Since this occurs rather seldomly, I think it would make sense to optimize for > > the most common case. > > > > What about a probalistic solution where for each frame (for simplicity > assuming > > a linear buffer) > > { > > repetition_detected = false; > > buffer_index = last_buffer_insertion_index-frame_length; // Assume at least > one > > frame of repetition length > > while (buffer_index > (last_buffer_insertion_index - 10 * frame_length) // > > Search over 10 frames > > { > > // Find a match candidate if two values in a row are identical (also need to > > take values 0, 32767, -32767 and -32768 specially into account (see below) > > if ((sample[0] == audio_buffer[buffer_index]) && (sample[1] == > > audio_buffer[buffer_index+1])) { > > // Assume 10 matching samples in a row sufficient for a repeated audio > > being present with a very high probability > > int k; > > for (k = 0; k < 8; k++) > > if ( sample[2+k] != audio_buffer[buffer_index+2+k]) > > break; > > if (k == 8) > > SendUMAMessage("Repetition found") > > } > > buffer_index--; > > } > > } > > > > That code will find any type of repetition pattern, but will fail to report > the > > length of the repetition. > > The reason of using matched loopback and length is to be able to report more > information than yes/no in the report, yet avoiding a pair<int, int> report, > which UMA cannot handle and is too big computation. > > This is more of a tradeoff among demands/feasibility/computation. We need to > understand > 1. if it is needed to analyze what patterns on what OS etc. > 2. if length is an import dimension. I feel so since we saw partial repetition > like 123xx123yy > 3. if we can list patterns we are interested in, and ignore other opportunistic > repetitions. > > > > > > https://codereview.webrtc.org/1287663002/diff/40001/webrtc/modules/audio_proc... > > webrtc/modules/audio_processing/repetition_detector.cc:114: > > buffer_length_samples_) % buffer_length_samples_; > > On 2015/08/12 11:32:13, minyue-webrtc wrote: > > > On 2015/08/12 11:02:54, peah-webrtc wrote: > > > > The addition of buffer_length_samples_ seems to be redundant, as it always > > > > results in 0 in the modulus operation > > > > > > This is to avoid negative, e,g, -2%3=-2 > > > > Good point! Thanks! > > > > > https://codereview.webrtc.org/1287663002/diff/40001/webrtc/modules/audio_proc... > > webrtc/modules/audio_processing/repetition_detector.cc:120: sample_repeated = > > false; > > Inequality is not a sufficient condition for repeated audio. That would for > > instance cause a constant zero signal to count as repeated audio, which is not > > neccessarily. > This will not, since the length will not match. Oh, you are right, the length won't fully limit it. I thought about it, but the length cannot protect well now due to the need of break multiple repetitions. Let me find a solution. Thank you for pointing this out. > > > > > Most likely we should treat the value zero specially, as well as the > saturating > > values (in case we have a very strong sound constantly saturating the > microphone > > signal (-32768, -32767 and 32767 in 16 bit integers). > > > > We most probably also should take a constant DC offset from zero as a special > > case, depending on whether we HP filter the signal before the check or not.
Hi, I updated the method now. To keep track, I upload the new version. But please minded that I will most likely move this from APM to Chromium. Then I will start a new CL and drop this one. So please comment in a macroscopic viewpoint. The updated method is based on the observation that 1. in our observed repetition, the look back value is always exact, which should correspond to some buffer length. 2. the length of repetition can vary, which may be related to threading. It seems that we can rely on a minimum length. https://codereview.webrtc.org/1287663002/diff/40001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/repetition_detector.cc (right): https://codereview.webrtc.org/1287663002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/repetition_detector.cc:116: // For multichannel audio, all channels have to repeat the same way. On 2015/08/12 14:12:04, hlundin-webrtc wrote: > On 2015/08/12 11:32:14, minyue-webrtc wrote: > > On 2015/08/12 11:02:54, peah-webrtc wrote: > > > Is this a neccessary requirement, would it not just be sufficient to look at > > the > > > leftmost channel for repetitions? If we are to look at the repetitions for > > both > > > channels, I think we should look at them separately, and count a repetition > if > > > any of them contains a repetition > > > > I consulted Chromium people. It is supposed that should always happen in both > > channels. But this is disputable too, since detection may be made looser. > > I agree with Per. Look at only one channel for now. In our recordings, we found that repetition should happen to both channels. In the new implementation, I take the advantage of this and made the detector (and API) simpler. Let me know if you think it is still better good to check one channel only https://codereview.webrtc.org/1287663002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/repetition_detector.cc:120: sample_repeated = false; On 2015/08/12 21:05:16, peah-webrtc wrote: > Inequality is not a sufficient condition for repeated audio. That would for > instance cause a constant zero signal to count as repeated audio, which is not > neccessarily. > > Most likely we should treat the value zero specially, as well as the saturating > values (in case we have a very strong sound constantly saturating the microphone > signal (-32768, -32767 and 32767 in 16 bit integers). > > We most probably also should take a constant DC offset from zero as a special > case, depending on whether we HP filter the signal before the check or not. Zeros are treated specially now. https://codereview.webrtc.org/1287663002/diff/40001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/repetition_detector.h (right): https://codereview.webrtc.org/1287663002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/repetition_detector.h:58: int look_back_; On 2015/08/12 14:12:04, hlundin-webrtc wrote: > I'd prefer size_t for the length and count variables. Now measure in millisecond. But other counters are made to size_t https://codereview.webrtc.org/1287663002/diff/60001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/audio_processing_impl.cc (right): https://codereview.webrtc.org/1287663002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl.cc:18: #include "webrtc/common_audio/audio_converter.h" changes in this file are mainly due to rebase. Please skip https://codereview.webrtc.org/1287663002/diff/60001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/audio_processing_impl.h (right): https://codereview.webrtc.org/1287663002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl.h:26: class AudioConverter; changes in this file are mainly due to rebase. Please skip https://codereview.webrtc.org/1287663002/diff/60001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/repetition_detector.cc (right): https://codereview.webrtc.org/1287663002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/repetition_detector.cc:21: // {id_, look_back_, look_back_range_, length_, length_range_} Comment here should be updated, I forgot. https://codereview.webrtc.org/1287663002/diff/60001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/repetition_detector_unittest.cc (right): https://codereview.webrtc.org/1287663002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/repetition_detector_unittest.cc:27: void ResetRepetitionPattern(const Pattern* patterns, size_t num_patterns) { the diff is not smart enough to align the two versions. Sorry for the messy display.
I didn't look at the actual algorithm, and from your comments I'm assuming you'll revert the AudioProcessing changes. Let me know if you change your mind and you want me to review that. https://codereview.webrtc.org/1287663002/diff/60001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/repetition_detector.cc (right): https://codereview.webrtc.org/1287663002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/repetition_detector.cc:91: for (auto state : states_) { How often is this called? You should strive to avoid dynamic memory management during real-time operation. Can you instead give state a Reset() method or something? ...wait it _has_ a Reset() method :) Why not call that here? If this is just for destruction time, you shouldn't need to do this at all. Wrap these in an RAII type that will clean itself up. If you want a vector-of-state pointers, use ScopedVector. https://codereview.webrtc.org/1287663002/diff/60001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/repetition_detector.h (right): https://codereview.webrtc.org/1287663002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/repetition_detector.h:38: void Detect(const void* data, size_t bytes_per_sample, Don't use void. Be confident about your type safety! Use templates or overloaded methods. https://codereview.webrtc.org/1287663002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/repetition_detector.h:39: size_t samples_per_channel, int sample_rate_hz); We've been using num_frames for the samples_per_channel quantity in newer code. This is consistent with Chromium. https://codereview.webrtc.org/1287663002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/repetition_detector.h:42: void RegisterRepetitionPatterns(const Pattern* patterns, Why do you need these protected methods? Is it just for the test? Instead consider a public RegisterRepetitionPatternsForTest or make this private and friend the needed test. https://codereview.webrtc.org/1287663002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/repetition_detector.h:75: std::unique_ptr<char[]> audio_buffer_; // Ring buffers to store input audio. unique_ptr is disallowed because it's C++11 standard library. What's wrong with vectors? Also did you consider AudioRingBuffer? It may already do exactly what you want. (I'm not certain).
On 2015/08/28 17:43:43, Andrew MacDonald wrote: > I didn't look at the actual algorithm, and from your comments I'm assuming > you'll revert the AudioProcessing changes. Let me know if you change your mind > and you want me to review that. > > https://codereview.webrtc.org/1287663002/diff/60001/webrtc/modules/audio_proc... > File webrtc/modules/audio_processing/repetition_detector.cc (right): > > https://codereview.webrtc.org/1287663002/diff/60001/webrtc/modules/audio_proc... > webrtc/modules/audio_processing/repetition_detector.cc:91: for (auto state : > states_) { > How often is this called? You should strive to avoid dynamic memory management > during real-time operation. Can you instead give state a Reset() method or > something? ...wait it _has_ a Reset() method :) Why not call that here? > > If this is just for destruction time, you shouldn't need to do this at all. Wrap > these in an RAII type that will clean itself up. If you want a vector-of-state > pointers, use ScopedVector. > > https://codereview.webrtc.org/1287663002/diff/60001/webrtc/modules/audio_proc... > File webrtc/modules/audio_processing/repetition_detector.h (right): > > https://codereview.webrtc.org/1287663002/diff/60001/webrtc/modules/audio_proc... > webrtc/modules/audio_processing/repetition_detector.h:38: void Detect(const > void* data, size_t bytes_per_sample, > Don't use void. Be confident about your type safety! Use templates or overloaded > methods. > > https://codereview.webrtc.org/1287663002/diff/60001/webrtc/modules/audio_proc... > webrtc/modules/audio_processing/repetition_detector.h:39: size_t > samples_per_channel, int sample_rate_hz); > We've been using num_frames for the samples_per_channel quantity in newer code. > This is consistent with Chromium. > > https://codereview.webrtc.org/1287663002/diff/60001/webrtc/modules/audio_proc... > webrtc/modules/audio_processing/repetition_detector.h:42: void > RegisterRepetitionPatterns(const Pattern* patterns, > Why do you need these protected methods? Is it just for the test? Instead > consider a public RegisterRepetitionPatternsForTest or make this private and > friend the needed test. > > https://codereview.webrtc.org/1287663002/diff/60001/webrtc/modules/audio_proc... > webrtc/modules/audio_processing/repetition_detector.h:75: > std::unique_ptr<char[]> audio_buffer_; // Ring buffers to store input audio. > unique_ptr is disallowed because it's C++11 standard library. What's wrong with > vectors? > > Also did you consider AudioRingBuffer? It may already do exactly what you want. > (I'm not certain). Yes, I think it should be very nice tool, the thing is that I may move RepetitionDetector to Chromium, and not sure if Webrtc tool can still be proper (I think it should be, why not).
On 2015/08/28 18:43:41, minyue-webrtc wrote: > Yes, I think it should be very nice tool, the thing is that I may move > RepetitionDetector to Chromium, and not sure if Webrtc tool can still be proper > (I think it should be, why not). Yes, it's OK to use webrtc code in Chromium (but not vice versa).
See comments. I didn't review the test code yet, except that I gave one comment on the structure. I'll let you digest that and comment before I review the rest of it. :) https://codereview.webrtc.org/1287663002/diff/60001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/repetition_detector.cc (right): https://codereview.webrtc.org/1287663002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/repetition_detector.cc:35: if (0 == count_samples_ && zero) { You do not need this if statement. https://codereview.webrtc.org/1287663002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/repetition_detector.cc:45: return (!all_zero_ && static_cast<int>(count_samples_) >= rtc::checked_cast. But I do think it would be better to cast rtc::checked_cast<size_t>(min_length_ms_ * sample_rate_hz / 1000). https://codereview.webrtc.org/1287663002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/repetition_detector.cc:49: bool RepetitionDetector::State::AlreadyReported() const { Change name to reported(). https://codereview.webrtc.org/1287663002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/repetition_detector.cc:53: void RepetitionDetector::State::SetReported() { Perhaps change name to set_reported(bool value). https://codereview.webrtc.org/1287663002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/repetition_detector.cc:91: for (auto state : states_) { On 2015/08/28 17:43:42, Andrew MacDonald wrote: > How often is this called? You should strive to avoid dynamic memory management > during real-time operation. Can you instead give state a Reset() method or > something? ...wait it _has_ a Reset() method :) Why not call that here? > > If this is just for destruction time, you shouldn't need to do this at all. Wrap > these in an RAII type that will clean itself up. If you want a vector-of-state > pointers, use ScopedVector. It's only called in the destructor of RepetitionDetector. Move this code to the destructor, or use ScopedVector as suggested. https://codereview.webrtc.org/1287663002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/repetition_detector.cc:102: buffer_size_samples_ = sample_1k / 1000 + (sample_1k % 1000 != 0); This is rounding up, right? Consider (sample_1k + 999) / 1000, instead. https://codereview.webrtc.org/1287663002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/repetition_detector.cc:110: buffer_end_index_++; buffer_end_index_ is initialized to 0, but it is incremented _before_ the first samples are added. This seems backwards to me. https://codereview.webrtc.org/1287663002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/repetition_detector.cc:139: if (memcmp(look_back_pointer, sample, bytes_per_sample) == 0) { This memcmp is to find out if all of |sample| is zero, right? I'm thinking that there is potentially a lot of overhead involved in allocating a dynamic array just to compare with. https://codereview.webrtc.org/1287663002/diff/60001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/repetition_detector.h (right): https://codereview.webrtc.org/1287663002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/repetition_detector.h:15: #include <memory> Order of includes. https://codereview.webrtc.org/1287663002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/repetition_detector.h:77: int sample_rate_hz_; // Sample rate in kHz. Name says Hz, comment says kHz. https://codereview.webrtc.org/1287663002/diff/60001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/repetition_detector_unittest.cc (right): https://codereview.webrtc.org/1287663002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/repetition_detector_unittest.cc:18: class RepetitionDetectorTest : public RepetitionDetector, This class is both a test fixture for the class under test, and an implementation of the class itself. This is slightly confusing to me. I would prefer that the test fixture only inherits from ::testing::Test, and then instead _owns_ a RepetitionDetector. This of course denies it access to protected members in the class under test, but I think it makes the test cleaner anyway.
https://codereview.webrtc.org/1287663002/diff/60001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/repetition_detector.cc (right): https://codereview.webrtc.org/1287663002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/repetition_detector.cc:127: std::unique_ptr<char[]> zero(new char[bytes_per_sample]()); You shouldn't need this. https://codereview.webrtc.org/1287663002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/repetition_detector.cc:128: const char* sample = reinterpret_cast<const char*>(data); No, no, use the type you want. https://codereview.webrtc.org/1287663002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/repetition_detector.cc:139: if (memcmp(look_back_pointer, sample, bytes_per_sample) == 0) { On 2015/08/31 13:46:46, hlundin-webrtc wrote: > This memcmp is to find out if all of |sample| is zero, right? I'm thinking that > there is potentially a lot of overhead involved in allocating a dynamic array > just to compare with. Absolutely. Use the correct types and just compare each value with zero. Don't use dynamic allocation in functions called for each chunk of audio.
I tried to update the code, but there are comments that I want to discuss. See inline, https://codereview.chromium.org/1287663002/diff/60001/webrtc/modules/audio_pr... File webrtc/modules/audio_processing/repetition_detector.cc (right): https://codereview.chromium.org/1287663002/diff/60001/webrtc/modules/audio_pr... webrtc/modules/audio_processing/repetition_detector.cc:102: buffer_size_samples_ = sample_1k / 1000 + (sample_1k % 1000 != 0); On 2015/08/31 13:46:46, hlundin-webrtc wrote: > This is rounding up, right? Consider (sample_1k + 999) / 1000, instead. nice! https://codereview.chromium.org/1287663002/diff/60001/webrtc/modules/audio_pr... webrtc/modules/audio_processing/repetition_detector.cc:128: const char* sample = reinterpret_cast<const char*>(data); On 2015/08/31 16:05:31, Andrew MacDonald wrote: > No, no, use the type you want. Yes, we may use template, but I also add two comments: 1. current way might be easier for multichannel sample 2. Even if we use types, it may not be best to compare values in the fashion of a==b, since the behavior of == for floating point may be unpredictable. https://codereview.chromium.org/1287663002/diff/60001/webrtc/modules/audio_pr... webrtc/modules/audio_processing/repetition_detector.cc:141: state->Increment(memcmp(zero.get(), sample, bytes_per_sample) == 0); Zero is actually this line. I can move the zero allocation in Reset(), to avoid dynamic allocation for each frame of audio. I think memcmp is still better than <T>a == <T>b since if T is floating point, the behavior may be unclear. https://codereview.chromium.org/1287663002/diff/60001/webrtc/modules/audio_pr... File webrtc/modules/audio_processing/repetition_detector.h (right): https://codereview.chromium.org/1287663002/diff/60001/webrtc/modules/audio_pr... webrtc/modules/audio_processing/repetition_detector.h:38: void Detect(const void* data, size_t bytes_per_sample, On 2015/08/28 17:43:42, Andrew MacDonald wrote: > Don't use void. Be confident about your type safety! Use templates or overloaded > methods. A benefit of this is that we can compare even larger sizes (a stereo float sample is 2 * sizeof(float), e.g.), but of course, we can make channels explicit too. https://codereview.chromium.org/1287663002/diff/60001/webrtc/modules/audio_pr... webrtc/modules/audio_processing/repetition_detector.h:39: size_t samples_per_channel, int sample_rate_hz); On 2015/08/28 17:43:42, Andrew MacDonald wrote: > We've been using num_frames for the samples_per_channel quantity in newer code. > This is consistent with Chromium. Ok. will change. https://codereview.chromium.org/1287663002/diff/60001/webrtc/modules/audio_pr... webrtc/modules/audio_processing/repetition_detector.h:75: std::unique_ptr<char[]> audio_buffer_; // Ring buffers to store input audio. On 2015/08/28 17:43:42, Andrew MacDonald wrote: > unique_ptr is disallowed because it's C++11 standard library. What's wrong with > vectors? > > Also did you consider AudioRingBuffer? It may already do exactly what you want. > (I'm not certain). Now I tried to switch to RingBuffer and I see a point that AudioRingBuffer does not fit well. I need random access to the data, for different look back values. But AudioRingBuffer (and RingBuffer) uses an internal read pointer. https://codereview.chromium.org/1287663002/diff/60001/webrtc/modules/audio_pr... webrtc/modules/audio_processing/repetition_detector.h:77: int sample_rate_hz_; // Sample rate in kHz. On 2015/08/31 13:46:46, hlundin-webrtc wrote: > Name says Hz, comment says kHz. Thanks, it was kHz, but to handle 44.1, I made it Hz https://codereview.chromium.org/1287663002/diff/60001/webrtc/modules/audio_pr... File webrtc/modules/audio_processing/repetition_detector_unittest.cc (right): https://codereview.chromium.org/1287663002/diff/60001/webrtc/modules/audio_pr... webrtc/modules/audio_processing/repetition_detector_unittest.cc:18: class RepetitionDetectorTest : public RepetitionDetector, On 2015/08/31 13:46:46, hlundin-webrtc wrote: > This class is both a test fixture for the class under test, and an > implementation of the class itself. This is slightly confusing to me. I would > prefer that the test fixture only inherits from ::testing::Test, and then > instead _owns_ a RepetitionDetector. This of course denies it access to > protected members in the class under test, but I think it makes the test cleaner > anyway. Ok. I can do that.
ajm@chromium.org changed reviewers: + ajm@chromium.org
https://codereview.chromium.org/1287663002/diff/60001/webrtc/modules/audio_pr... File webrtc/modules/audio_processing/repetition_detector.cc (right): https://codereview.chromium.org/1287663002/diff/60001/webrtc/modules/audio_pr... webrtc/modules/audio_processing/repetition_detector.cc:128: const char* sample = reinterpret_cast<const char*>(data); On 2015/09/01 10:21:48, minyue-webrtc wrote: > On 2015/08/31 16:05:31, Andrew MacDonald wrote: > > No, no, use the type you want. > > Yes, we may use template, but I also add two comments: > 1. current way might be easier for multichannel sample As described above, you should be explicit about your multiple channels. > 2. Even if we use types, it may not be best to compare values in the fashion of > a==b, since the behavior of == for floating point may be unpredictable. It's certainly no worse than memcmp! As in another comment, you want to use an epsilon for floating point comparison. https://codereview.chromium.org/1287663002/diff/60001/webrtc/modules/audio_pr... webrtc/modules/audio_processing/repetition_detector.cc:141: state->Increment(memcmp(zero.get(), sample, bytes_per_sample) == 0); On 2015/09/01 10:21:48, minyue-webrtc wrote: > Zero is actually this line. I can move the zero allocation in Reset(), to avoid > dynamic allocation for each frame of audio. > > I think memcmp is still better than <T>a == <T>b since if T is floating point, > the behavior may be unclear. How is a == b any less clear than memcmp for floating point? If you're worried about floating point equality, you should use a small epsilon around the diff to compare. Consider for example EXPECT_FLOAT_EQ or EXPECT_NEAR in gtest. https://codereview.chromium.org/1287663002/diff/60001/webrtc/modules/audio_pr... File webrtc/modules/audio_processing/repetition_detector.h (right): https://codereview.chromium.org/1287663002/diff/60001/webrtc/modules/audio_pr... webrtc/modules/audio_processing/repetition_detector.h:38: void Detect(const void* data, size_t bytes_per_sample, On 2015/09/01 10:21:48, minyue-webrtc wrote: > On 2015/08/28 17:43:42, Andrew MacDonald wrote: > > Don't use void. Be confident about your type safety! Use templates or > overloaded > > methods. > > A benefit of this is that we can compare even larger sizes (a stereo float > sample is 2 * sizeof(float), e.g.), but of course, we can make channels explicit > too. Yes, make them explicit. I see that lack of type safety as a clear negative, not a benefit. https://codereview.chromium.org/1287663002/diff/60001/webrtc/modules/audio_pr... webrtc/modules/audio_processing/repetition_detector.h:75: std::unique_ptr<char[]> audio_buffer_; // Ring buffers to store input audio. On 2015/09/01 10:21:48, minyue-webrtc wrote: > On 2015/08/28 17:43:42, Andrew MacDonald wrote: > > unique_ptr is disallowed because it's C++11 standard library. What's wrong > with > > vectors? > > > > Also did you consider AudioRingBuffer? It may already do exactly what you > want. > > (I'm not certain). > > Now I tried to switch to RingBuffer and I see a point that AudioRingBuffer does > not fit well. I need random access to the data, for different look back values. > But AudioRingBuffer (and RingBuffer) uses an internal read pointer. Ah OK. Use a vector here then.
Thanks for the discussion and a new patch created. https://codereview.webrtc.org/1287663002/diff/60001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/repetition_detector.cc (right): https://codereview.webrtc.org/1287663002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/repetition_detector.cc:21: // {id_, look_back_, look_back_range_, length_, length_range_} On 2015/08/28 14:27:00, minyue-webrtc wrote: > Comment here should be updated, I forgot. Done. https://codereview.webrtc.org/1287663002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/repetition_detector.cc:35: if (0 == count_samples_ && zero) { On 2015/08/31 13:46:46, hlundin-webrtc wrote: > You do not need this if statement. I know that this is redundant if Reset was called properly, well, it helps to make this function more complete. If to reduce redundancy, I'd rather remove all_zero_ = true; in Reset() https://codereview.webrtc.org/1287663002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/repetition_detector.cc:45: return (!all_zero_ && static_cast<int>(count_samples_) >= On 2015/08/31 13:46:46, hlundin-webrtc wrote: > rtc::checked_cast. But I do think it would be better to cast > rtc::checked_cast<size_t>(min_length_ms_ * sample_rate_hz / 1000). Ok, I like it, and hope that it can stay when I move the whole thing to Chromium https://codereview.webrtc.org/1287663002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/repetition_detector.cc:49: bool RepetitionDetector::State::AlreadyReported() const { On 2015/08/31 13:46:46, hlundin-webrtc wrote: > Change name to reported(). Done. https://codereview.webrtc.org/1287663002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/repetition_detector.cc:53: void RepetitionDetector::State::SetReported() { On 2015/08/31 13:46:46, hlundin-webrtc wrote: > Perhaps change name to set_reported(bool value). Done. https://codereview.webrtc.org/1287663002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/repetition_detector.cc:91: for (auto state : states_) { On 2015/08/31 13:46:46, hlundin-webrtc wrote: > On 2015/08/28 17:43:42, Andrew MacDonald wrote: > > How often is this called? You should strive to avoid dynamic memory management > > during real-time operation. Can you instead give state a Reset() method or > > something? ...wait it _has_ a Reset() method :) Why not call that here? > > > > If this is just for destruction time, you shouldn't need to do this at all. > Wrap > > these in an RAII type that will clean itself up. If you want a vector-of-state > > pointers, use ScopedVector. > > It's only called in the destructor of RepetitionDetector. Move this code to the > destructor, or use ScopedVector as suggested. Done. https://codereview.webrtc.org/1287663002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/repetition_detector.cc:110: buffer_end_index_++; On 2015/08/31 13:46:46, hlundin-webrtc wrote: > buffer_end_index_ is initialized to 0, but it is incremented _before_ the first > samples are added. This seems backwards to me. Done. https://codereview.webrtc.org/1287663002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/repetition_detector.cc:127: std::unique_ptr<char[]> zero(new char[bytes_per_sample]()); On 2015/08/31 16:05:31, Andrew MacDonald wrote: > You shouldn't need this. Done. https://codereview.webrtc.org/1287663002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/repetition_detector.cc:128: const char* sample = reinterpret_cast<const char*>(data); On 2015/09/02 05:28:28, ajm wrote: > On 2015/09/01 10:21:48, minyue-webrtc wrote: > > On 2015/08/31 16:05:31, Andrew MacDonald wrote: > > > No, no, use the type you want. > > > > Yes, we may use template, but I also add two comments: > > 1. current way might be easier for multichannel sample > > As described above, you should be explicit about your multiple channels. > > > 2. Even if we use types, it may not be best to compare values in the fashion > of > > a==b, since the behavior of == for floating point may be unpredictable. > > It's certainly no worse than memcmp! As in another comment, you want to use an > epsilon for floating point comparison. Done. https://codereview.webrtc.org/1287663002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/repetition_detector.cc:139: if (memcmp(look_back_pointer, sample, bytes_per_sample) == 0) { On 2015/08/31 16:05:31, Andrew MacDonald wrote: > On 2015/08/31 13:46:46, hlundin-webrtc wrote: > > This memcmp is to find out if all of |sample| is zero, right? I'm thinking > that > > there is potentially a lot of overhead involved in allocating a dynamic array > > just to compare with. > > Absolutely. Use the correct types and just compare each value with zero. > > Don't use dynamic allocation in functions called for each chunk of audio. Done. https://codereview.webrtc.org/1287663002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/repetition_detector.cc:141: state->Increment(memcmp(zero.get(), sample, bytes_per_sample) == 0); On 2015/09/02 05:28:28, ajm wrote: > On 2015/09/01 10:21:48, minyue-webrtc wrote: > > Zero is actually this line. I can move the zero allocation in Reset(), to > avoid > > dynamic allocation for each frame of audio. > > > > I think memcmp is still better than <T>a == <T>b since if T is floating point, > > the behavior may be unclear. > > How is a == b any less clear than memcmp for floating point? > > If you're worried about floating point equality, you should use a small epsilon > around the diff to compare. Consider for example EXPECT_FLOAT_EQ or EXPECT_NEAR > in gtest. Done. https://codereview.webrtc.org/1287663002/diff/60001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/repetition_detector.h (right): https://codereview.webrtc.org/1287663002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/repetition_detector.h:15: #include <memory> On 2015/08/31 13:46:46, hlundin-webrtc wrote: > Order of includes. Yes, but memory is not needed any longer now https://codereview.webrtc.org/1287663002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/repetition_detector.h:38: void Detect(const void* data, size_t bytes_per_sample, On 2015/09/02 05:28:28, ajm wrote: > On 2015/09/01 10:21:48, minyue-webrtc wrote: > > On 2015/08/28 17:43:42, Andrew MacDonald wrote: > > > Don't use void. Be confident about your type safety! Use templates or > > overloaded > > > methods. > > > > A benefit of this is that we can compare even larger sizes (a stereo float > > sample is 2 * sizeof(float), e.g.), but of course, we can make channels > explicit > > too. > > Yes, make them explicit. I see that lack of type safety as a clear negative, not > a benefit. Ok. I take it. To make it simple enough, I even restrict it to float. https://codereview.webrtc.org/1287663002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/repetition_detector.h:75: std::unique_ptr<char[]> audio_buffer_; // Ring buffers to store input audio. On 2015/09/02 05:28:28, ajm wrote: > On 2015/09/01 10:21:48, minyue-webrtc wrote: > > On 2015/08/28 17:43:42, Andrew MacDonald wrote: > > > unique_ptr is disallowed because it's C++11 standard library. What's wrong > > with > > > vectors? > > > > > > Also did you consider AudioRingBuffer? It may already do exactly what you > > want. > > > (I'm not certain). > > > > Now I tried to switch to RingBuffer and I see a point that AudioRingBuffer > does > > not fit well. I need random access to the data, for different look back > values. > > But AudioRingBuffer (and RingBuffer) uses an internal read pointer. > > Ah OK. Use a vector here then. will scoped_ptr be better, I feel that vector has a lot of overhead, and it is weird or maybe wrong if we do something like float* sample = &audio_buffer_[0] if (*(sample + 1) == 0) ...
https://codereview.webrtc.org/1287663002/diff/60001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/repetition_detector.h (right): https://codereview.webrtc.org/1287663002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/repetition_detector.h:75: std::unique_ptr<char[]> audio_buffer_; // Ring buffers to store input audio. On 2015/09/03 13:23:58, minyue-webrtc wrote: > On 2015/09/02 05:28:28, ajm wrote: > > On 2015/09/01 10:21:48, minyue-webrtc wrote: > > > On 2015/08/28 17:43:42, Andrew MacDonald wrote: > > > > unique_ptr is disallowed because it's C++11 standard library. What's wrong > > > with > > > > vectors? > > > > > > > > Also did you consider AudioRingBuffer? It may already do exactly what you > > > want. > > > > (I'm not certain). > > > > > > Now I tried to switch to RingBuffer and I see a point that AudioRingBuffer > > does > > > not fit well. I need random access to the data, for different look back > > values. > > > But AudioRingBuffer (and RingBuffer) uses an internal read pointer. > > > > Ah OK. Use a vector here then. > > will scoped_ptr be better, I feel that vector has a lot of overhead, and it is > weird or maybe wrong if we do something like > float* sample = &audio_buffer_[0] > if (*(sample + 1) == 0) > ... You can expect an empty vector to consume 12 bytes (pointer + size + capacity). You store an explicit pointer and size here so the overhead is 4 bytes. Don't worry about it. audio_buffer_ can be a vector in your example and it will behave exactly as you would expect. There is no problem. https://codereview.webrtc.org/1287663002/diff/80001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/repetition_detector.cc (right): https://codereview.webrtc.org/1287663002/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/repetition_detector.cc:76: void RepetitionDetector::Reset(size_t num_channels, int sample_rate_hz) { Do you need this? Why not have users just create a new instance? Ah I see, you're lazily initializing things. Will users really require that? https://codereview.webrtc.org/1287663002/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/repetition_detector.cc:89: for (size_t cdx = 0; cdx < num_channels_; ++cdx, ++ref, ++sample) { memcpy? (or even better, std::copy) Might not be worth it for the low num_channels_ you'll have in practice. Sorry I didn't look at this more closely, but why are you adding data sample-by-sample to the buffer? Wouldn't you add chunks of data at a time? https://codereview.webrtc.org/1287663002/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/repetition_detector.cc:105: if (*sample != *ref) { I thought you were worried about exact floating point comparisons? Same thing in IsZero below. https://codereview.webrtc.org/1287663002/diff/80001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/repetition_detector.h (right): https://codereview.webrtc.org/1287663002/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/repetition_detector.h:43: friend class RepetitionDetectorForTest; // For testing. Normally you'd use the FRIEND_TEST macro. https://codereview.webrtc.org/1287663002/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/repetition_detector.h:46: virtual void ReportRepetition(int id) { } Why protected?
andrew@webrtc.org changed reviewers: - ajm@chromium.org
https://codereview.webrtc.org/1287663002/diff/80001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/repetition_detector.cc (right): https://codereview.webrtc.org/1287663002/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/repetition_detector.cc:76: void RepetitionDetector::Reset(size_t num_channels, int sample_rate_hz) { On 2015/09/07 06:52:18, Andrew MacDonald wrote: > Do you need this? Why not have users just create a new instance? > > Ah I see, you're lazily initializing things. Will users really require that? This is needed when the num_channels, or sampling rate changes on-the-fly. I am not sure how practical is this, but we need to handle it. https://codereview.webrtc.org/1287663002/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/repetition_detector.cc:89: for (size_t cdx = 0; cdx < num_channels_; ++cdx, ++ref, ++sample) { On 2015/09/07 06:52:18, Andrew MacDonald wrote: > memcpy? (or even better, std::copy) > > Might not be worth it for the low num_channels_ you'll have in practice. Sorry I > didn't look at this more closely, but why are you adding data sample-by-sample > to the buffer? Wouldn't you add chunks of data at a time? I also thought about adding data chunks. It is tricky if I need to compare in-chunk repetition. I can try, though. https://codereview.webrtc.org/1287663002/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/repetition_detector.cc:105: if (*sample != *ref) { On 2015/09/07 06:52:18, Andrew MacDonald wrote: > I thought you were worried about exact floating point comparisons? Same thing in > IsZero below. I want to check bit exact equality, and with current unittests, it seems that "==" works fine https://codereview.webrtc.org/1287663002/diff/80001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/repetition_detector.h (right): https://codereview.webrtc.org/1287663002/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/repetition_detector.h:43: friend class RepetitionDetectorForTest; // For testing. On 2015/09/07 06:52:18, Andrew MacDonald wrote: > Normally you'd use the FRIEND_TEST macro. Acknowledged. https://codereview.webrtc.org/1287663002/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/repetition_detector.h:46: virtual void ReportRepetition(int id) { } On 2015/09/07 06:52:18, Andrew MacDonald wrote: > Why protected? true, not needed.
https://codereview.webrtc.org/1287663002/diff/80001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/repetition_detector.cc (right): https://codereview.webrtc.org/1287663002/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/repetition_detector.cc:76: void RepetitionDetector::Reset(size_t num_channels, int sample_rate_hz) { On 2015/09/07 07:33:24, minyue-webrtc wrote: > On 2015/09/07 06:52:18, Andrew MacDonald wrote: > > Do you need this? Why not have users just create a new instance? > > > > Ah I see, you're lazily initializing things. Will users really require that? > > This is needed when the num_channels, or sampling rate changes on-the-fly. I am > not sure how practical is this, but we need to handle it. The other solution is to have clients recreate it when those parameters change. But if that's tedious for clients to do, what you have makes sense.
some updates https://codereview.webrtc.org/1287663002/diff/80001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/repetition_detector.cc (right): https://codereview.webrtc.org/1287663002/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/repetition_detector.cc:76: void RepetitionDetector::Reset(size_t num_channels, int sample_rate_hz) { On 2015/09/07 22:52:19, Andrew MacDonald wrote: > On 2015/09/07 07:33:24, minyue-webrtc wrote: > > On 2015/09/07 06:52:18, Andrew MacDonald wrote: > > > Do you need this? Why not have users just create a new instance? > > > > > > Ah I see, you're lazily initializing things. Will users really require that? > > > > This is needed when the num_channels, or sampling rate changes on-the-fly. I > am > > not sure how practical is this, but we need to handle it. > > The other solution is to have clients recreate it when those parameters change. > But if that's tedious for clients to do, what you have makes sense. I prefer that RepetitionDetector takes care of that. https://codereview.webrtc.org/1287663002/diff/80001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/repetition_detector.h (right): https://codereview.webrtc.org/1287663002/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/repetition_detector.h:43: friend class RepetitionDetectorForTest; // For testing. On 2015/09/07 07:33:24, minyue-webrtc wrote: > On 2015/09/07 06:52:18, Andrew MacDonald wrote: > > Normally you'd use the FRIEND_TEST macro. > > Acknowledged. It turns out a bit complicated here. RepetitionDetectorForTest is not Test, but an inherit of RepetitionDetector, which defines ReportRepetition(). RepetitionDetectorTest is the actually test. FRIEND_TEST is used upon gTests, such RepetitionDetectorTest, but it is RepetitionDetectorForTest that needs to access private members. I liked to make RepetitionDetectorTest to inherit both RepetitionDetector and ::testing::Test, but henrik did not like it.
Nice! A few minor comments, then I think it is good. https://codereview.webrtc.org/1287663002/diff/60001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/repetition_detector.cc (right): https://codereview.webrtc.org/1287663002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/repetition_detector.cc:35: if (0 == count_samples_ && zero) { On 2015/09/03 13:23:58, minyue-webrtc wrote: > On 2015/08/31 13:46:46, hlundin-webrtc wrote: > > You do not need this if statement. > > I know that this is redundant if Reset was called properly, well, it helps to > make this function more complete. > > If to reduce redundancy, I'd rather remove all_zero_ = true; in Reset() I think you are being overly aggressive against your own code. If you worry about bugs in your own code, use a DCHECK. https://codereview.webrtc.org/1287663002/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/repetition_detector.cc:45: return (!all_zero_ && static_cast<int>(count_samples_) >= On 2015/09/03 13:23:58, minyue-webrtc wrote: > On 2015/08/31 13:46:46, hlundin-webrtc wrote: > > rtc::checked_cast. But I do think it would be better to cast > > rtc::checked_cast<size_t>(min_length_ms_ * sample_rate_hz / 1000). > > Ok, I like it, and hope that it can stay when I move the whole thing to Chromium Acknowledged. https://codereview.webrtc.org/1287663002/diff/100001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/repetition_detector.cc (right): https://codereview.webrtc.org/1287663002/diff/100001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/repetition_detector.cc:84: buffer_size_frames_ = (sample_1k + 999) / 1000 + max_frames_; Now just comment that what you implemented is in fact rounding up. Courtesy to future you. :) https://codereview.webrtc.org/1287663002/diff/100001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/repetition_detector.cc:94: size_t margin = buffer_size_frames_ - buffer_end_index_; const https://codereview.webrtc.org/1287663002/diff/100001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/repetition_detector.h (right): https://codereview.webrtc.org/1287663002/diff/100001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/repetition_detector.h:25: virtual ~RepetitionDetector() {} virtual ~RepetitionDetector() = default; https://codereview.webrtc.org/1287663002/diff/100001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/repetition_detector.h:41: friend class RepetitionDetectorForTest; // For testing. Consider using the FRIEND_TEST macro instead. https://code.google.com/p/googletest/wiki/AdvancedGuide#Private_Class_Members https://codereview.webrtc.org/1287663002/diff/100001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/repetition_detector.h:68: // Register repetition patterns. This comment adds no more information than the name already does. Either remove the comment or flesh it out with more information. https://codereview.webrtc.org/1287663002/diff/100001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/repetition_detector.h:72: // Clear registered repetition patterns. Same for this comment. Contribute or die. https://codereview.webrtc.org/1287663002/diff/100001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/repetition_detector.h:96: int sample_rate_hz_; // Sample rate in kHz. Still mismatch between name and comment. https://codereview.webrtc.org/1287663002/diff/100001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/repetition_detector_unittest.cc (right): https://codereview.webrtc.org/1287663002/diff/100001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/repetition_detector_unittest.cc:110: RepetitionDetectorForTest detector; detector_ https://codereview.webrtc.org/1287663002/diff/100001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/repetition_detector_unittest.cc:129: ResetRepetitionPattern(kRepetitionPatterns, sizeof(kRepetitionPatterns) / Use arraysize() from webrtc/base/arraysize.h. Here and several places below. https://codereview.webrtc.org/1287663002/diff/100001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/repetition_detector_unittest.cc:377: // Previous tests users short signal to test the detection algorithm, this one users -> uses? https://codereview.webrtc.org/1287663002/diff/100001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/repetition_detector_unittest.cc:378: // tests the build-in pattern in RepetitionDetector. built-int https://codereview.webrtc.org/1287663002/diff/100001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/repetition_detector_unittest.cc:379: TEST_F(RepetitionDetectorTest, BuildInPattern) { BuiltInPattern
Thanks for the comments. an update is made now. Then this CL is gone be shipped to Chrome media renderer, and a new CL will be formed, which will take this as a reference. https://codereview.webrtc.org/1287663002/diff/100001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/repetition_detector.cc (right): https://codereview.webrtc.org/1287663002/diff/100001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/repetition_detector.cc:84: buffer_size_frames_ = (sample_1k + 999) / 1000 + max_frames_; On 2015/09/15 09:26:38, hlundin-webrtc wrote: > Now just comment that what you implemented is in fact rounding up. Courtesy to > future you. :) Done. https://codereview.webrtc.org/1287663002/diff/100001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/repetition_detector.cc:94: size_t margin = buffer_size_frames_ - buffer_end_index_; On 2015/09/15 09:26:38, hlundin-webrtc wrote: > const Done. https://codereview.webrtc.org/1287663002/diff/100001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/repetition_detector.h (right): https://codereview.webrtc.org/1287663002/diff/100001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/repetition_detector.h:25: virtual ~RepetitionDetector() {} On 2015/09/15 09:26:38, hlundin-webrtc wrote: > virtual ~RepetitionDetector() = default; Done. https://codereview.webrtc.org/1287663002/diff/100001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/repetition_detector.h:41: friend class RepetitionDetectorForTest; // For testing. On 2015/09/15 09:26:38, hlundin-webrtc wrote: > Consider using the FRIEND_TEST macro instead. > https://code.google.com/p/googletest/wiki/AdvancedGuide#Private_Class_Members Please see my earlier comment. If you have a better solution, let me know. https://codereview.webrtc.org/1287663002/diff/100001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/repetition_detector.h:68: // Register repetition patterns. On 2015/09/15 09:26:38, hlundin-webrtc wrote: > This comment adds no more information than the name already does. Either remove > the comment or flesh it out with more information. Done. https://codereview.webrtc.org/1287663002/diff/100001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/repetition_detector.h:72: // Clear registered repetition patterns. On 2015/09/15 09:26:38, hlundin-webrtc wrote: > Same for this comment. Contribute or die. Done. https://codereview.webrtc.org/1287663002/diff/100001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/repetition_detector.h:96: int sample_rate_hz_; // Sample rate in kHz. On 2015/09/15 09:26:39, hlundin-webrtc wrote: > Still mismatch between name and comment. oh, sorry. I might have missed your earlier comment. https://codereview.webrtc.org/1287663002/diff/100001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/repetition_detector_unittest.cc (right): https://codereview.webrtc.org/1287663002/diff/100001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/repetition_detector_unittest.cc:110: RepetitionDetectorForTest detector; On 2015/09/15 09:26:39, hlundin-webrtc wrote: > detector_ Done. https://codereview.webrtc.org/1287663002/diff/100001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/repetition_detector_unittest.cc:129: ResetRepetitionPattern(kRepetitionPatterns, sizeof(kRepetitionPatterns) / On 2015/09/15 09:26:39, hlundin-webrtc wrote: > Use arraysize() from webrtc/base/arraysize.h. Here and several places below. Very good https://codereview.webrtc.org/1287663002/diff/100001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/repetition_detector_unittest.cc:377: // Previous tests users short signal to test the detection algorithm, this one On 2015/09/15 09:26:39, hlundin-webrtc wrote: > users -> uses? use https://codereview.webrtc.org/1287663002/diff/100001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/repetition_detector_unittest.cc:378: // tests the build-in pattern in RepetitionDetector. On 2015/09/15 09:26:39, hlundin-webrtc wrote: > built-int built-in https://codereview.webrtc.org/1287663002/diff/100001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/repetition_detector_unittest.cc:379: TEST_F(RepetitionDetectorTest, BuildInPattern) { On 2015/09/15 09:26:39, hlundin-webrtc wrote: > BuiltInPattern Done.
lgtm https://codereview.webrtc.org/1287663002/diff/100001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/repetition_detector.h (right): https://codereview.webrtc.org/1287663002/diff/100001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/repetition_detector.h:41: friend class RepetitionDetectorForTest; // For testing. On 2015/09/17 13:45:22, minyue-webrtc wrote: > On 2015/09/15 09:26:38, hlundin-webrtc wrote: > > Consider using the FRIEND_TEST macro instead. > > https://code.google.com/p/googletest/wiki/AdvancedGuide#Private_Class_Members > > Please see my earlier comment. If you have a better solution, let me know. Acknowledged. https://codereview.webrtc.org/1287663002/diff/100001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/repetition_detector_unittest.cc (right): https://codereview.webrtc.org/1287663002/diff/100001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/repetition_detector_unittest.cc:377: // Previous tests users short signal to test the detection algorithm, this one On 2015/09/17 13:45:23, minyue-webrtc wrote: > On 2015/09/15 09:26:39, hlundin-webrtc wrote: > > users -> uses? > > use Acknowledged. https://codereview.webrtc.org/1287663002/diff/100001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/repetition_detector_unittest.cc:378: // tests the build-in pattern in RepetitionDetector. On 2015/09/17 13:45:23, minyue-webrtc wrote: > On 2015/09/15 09:26:39, hlundin-webrtc wrote: > > built-int > > built-in :) Worst kind of spelling complaint....
On 2015/09/25 09:48:44, minyue-webrtc wrote: Mistakenly sent an empty message, sorry, just wanted to add that the CL has been moved to https://codereview.chromium.org/1357013006
On 2015/09/25 09:51:45, minyue-webrtc wrote: > On 2015/09/25 09:48:44, minyue-webrtc wrote: > > Mistakenly sent an empty message, sorry, just wanted to add that > > the CL has been moved to https://codereview.chromium.org/1357013006 Minyue, I closed this one.
Message was sent while issue was closed.
On 2015/11/25 19:28:07, Andrew MacDonald wrote: > On 2015/09/25 09:51:45, minyue-webrtc wrote: > > On 2015/09/25 09:48:44, minyue-webrtc wrote: > > > > Mistakenly sent an empty message, sorry, just wanted to add that > > > > the CL has been moved to https://codereview.chromium.org/1357013006 > > Minyue, I closed this one. thank you! It should have been closed. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
