Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1357013006:
Add detection for repeated audio in capturing. (Closed)
|
Created:
5 years, 3 months ago by minyue Modified:
5 years, 1 month ago CC:
chromium-reviews, mlamouri+watch-content_chromium.org, posciak+watch_chromium.org, jam, mcasas+watch_chromium.org, feature-media-reviews_chromium.org, darin-cc_chromium.org, asvitkine+watch_chromium.org, mkwst+moarreviews-renderer_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd detection for repeated audio in capturing.
This CL is to add a light-weight detector for repeated audio that has been observed in some recent recording. The issue might have been resolved in a recent fix, see
https://chromium.googlesource.com/chromium/src/+/8d9071da52c70d300bfc0cdc0448c564b39764f4
It is still good to add a detector and UMA stats to verify the fix and fire alarms for occurrences of audio repetition due to other potential causes.
The repetition detector was planned to be placed in WebRTC Audio Processing Module, but per discussion, it is better to be placed in content renderer. The algorithm has been reviewed, see
https://codereview.webrtc.org/1287663002/
BUG=520425
TEST=build with custom Chromium that produces repeated audio.
Committed: https://crrev.com/6a05781d7334b28a75b3321301a8927b5cd7dc36
Cr-Commit-Position: refs/heads/master@{#358555}
Patch Set 1 #Patch Set 2 : fixing a unittest #Patch Set 3 : fixing dll #
Total comments: 89
Patch Set 4 : after review #
Total comments: 4
Patch Set 5 : relaxing dcheck on sample rate #
Total comments: 11
Patch Set 6 : rename: repetition_detector -> audio_repetition_detector #Patch Set 7 : on tommi's comments #
Total comments: 11
Patch Set 8 : some small issues #
Total comments: 16
Patch Set 9 : some small changes #
Total comments: 10
Patch Set 10 : reording of functions #Patch Set 11 : on andrew's comment #
Total comments: 18
Patch Set 12 : on tommi's comments #Patch Set 13 : allow reassign thread after creation #
Total comments: 4
Patch Set 14 : allow reassign thread after creation (cont'd) #Patch Set 15 : dropping duration for easier logging #
Total comments: 27
Patch Set 16 : decoupling uma reporting from detection #
Total comments: 10
Patch Set 17 : pure virtual function -> base::Callback #
Total comments: 12
Patch Set 18 : refinement #
Total comments: 2
Patch Set 19 : refinement #
Total comments: 3
Patch Set 20 : use std::sort and unique #
Total comments: 19
Patch Set 21 : on tommi's comments #Patch Set 22 : rebase #Messages
Total messages: 91 (14 generated)
minyue@webrtc.org changed reviewers: + ajm@chromium.org, grunell@chromium.org, hlundin@chromium.org
Hi, Would you like to help review this CL? For your reference, the algorithm and implementation has been reviewed https://codereview.webrtc.org/1287663002/ Patch set 2 fixes an old unit test, which uses an unsupported 22050 sampling rate. Patch set 3 solves a compiling error.
Can you summarize why it's better to have the detector in Chromium rather than WebRTC? It's hard to find in all the comments in the other CL. https://codereview.chromium.org/1357013006/diff/40001/content/renderer/media/... File content/renderer/media/media_stream_audio_processor.cc (right): https://codereview.chromium.org/1357013006/diff/40001/content/renderer/media/... content/renderer/media/media_stream_audio_processor.cc:311: repetition_detector_->Detect(process_bus->bus()->channel(0), How much complexity does this add? Would be good to document that in the bug if it isn't already. How does the detector handle expected repetitions, like hooking a file or generate a sinus directly to the input? (Will that be bit exact?) https://codereview.chromium.org/1357013006/diff/40001/content/renderer/media/... content/renderer/media/media_stream_audio_processor.cc:314: input_format_.sample_rate()); Does the |process_bus| contain the sample rate?
Hi Henrik, The reason of placing it in Chrome but not APM is mainly because the repetition is clearly not caused by APM. The earlier it sits, the more accurate information it can provide. There are no other strong reasons. If you have concerns against placing it here, please let me know. I may be not aware of risks of placing it here. https://codereview.chromium.org/1357013006/diff/40001/content/renderer/media/... File content/renderer/media/media_stream_audio_processor.cc (right): https://codereview.chromium.org/1357013006/diff/40001/content/renderer/media/... content/renderer/media/media_stream_audio_processor.cc:311: repetition_detector_->Detect(process_bus->bus()->channel(0), On 2015/09/23 06:54:15, Henrik Grunell wrote: > How much complexity does this add? Would be good to document that in the bug if > it isn't already. > > How does the detector handle expected repetitions, like hooking a file or > generate a sinus directly to the input? (Will that be bit exact?) It is O(n), n for number of samples. Each sample compares with the samples sitting at the look back positions of the registered repetition patterns. https://codereview.chromium.org/1357013006/diff/40001/content/renderer/media/... content/renderer/media/media_stream_audio_processor.cc:314: input_format_.sample_rate()); On 2015/09/23 06:54:15, Henrik Grunell wrote: > Does the |process_bus| contain the sample rate? Unfortunately not. and that is why other places uses input/output_format_.sample_rate() too, see, e.g., https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/m...
lgtm, although I have no power here.
I won't review the detection algorithm. Generally, should there perhaps be more (D)CHECKS? https://codereview.chromium.org/1357013006/diff/40001/content/renderer/media/... File content/renderer/media/media_stream_audio_processor.cc (right): https://codereview.chromium.org/1357013006/diff/40001/content/renderer/media/... content/renderer/media/media_stream_audio_processor.cc:308: // Detect if bit-exact repetition of audio present in the captured audio. Nit: "Detect bit-exact repetition of audio." https://codereview.chromium.org/1357013006/diff/40001/content/renderer/media/... content/renderer/media/media_stream_audio_processor.cc:314: input_format_.sample_rate()); On 2015/09/23 08:48:07, minyue wrote: > On 2015/09/23 06:54:15, Henrik Grunell wrote: > > Does the |process_bus| contain the sample rate? > > Unfortunately not. and that is why other places uses > input/output_format_.sample_rate() too, see, e.g., > > https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/m... > I looked into this more. The MediaStreamAudioBus wraps an AudioBus which contains all information needed. Let take a an AudioBus RepetitionDetector::Detect(const AudioBus*) and handle it there. https://codereview.chromium.org/1357013006/diff/40001/content/renderer/media/... File content/renderer/media/repetition_detector.cc (right): https://codereview.chromium.org/1357013006/diff/40001/content/renderer/media/... content/renderer/media/repetition_detector.cc:7: #include "base/macros.h" // arraysize Remove comments. https://codereview.chromium.org/1357013006/diff/40001/content/renderer/media/... content/renderer/media/repetition_detector.cc:10: #include "third_party/webrtc/base/checks.h" // rtc::CheckedDivExact Remove. Use CHECK or DCHECK before division instead. https://codereview.chromium.org/1357013006/diff/40001/content/renderer/media/... content/renderer/media/repetition_detector.cc:15: static const RepetitionDetector::Pattern kRepetitionPatterns[] = { Empty line before. https://codereview.chromium.org/1357013006/diff/40001/content/renderer/media/... content/renderer/media/repetition_detector.cc:15: static const RepetitionDetector::Pattern kRepetitionPatterns[] = { Let the user of this class give the repetitions patterns in the constructor instead, as an std::vector or std::array. https://codereview.chromium.org/1357013006/diff/40001/content/renderer/media/... content/renderer/media/repetition_detector.cc:16: // {id_, look_back_ms_, min_length_ms_} Remove this comment. It duplicates info. https://codereview.chromium.org/1357013006/diff/40001/content/renderer/media/... content/renderer/media/repetition_detector.cc:29: static const size_t kMaxFrames = 480; // 10ms * 48kHz Empty line before. https://codereview.chromium.org/1357013006/diff/40001/content/renderer/media/... content/renderer/media/repetition_detector.cc:29: static const size_t kMaxFrames = 480; // 10ms * 48kHz Nit: 10 ms * 48 kHz https://codereview.chromium.org/1357013006/diff/40001/content/renderer/media/... content/renderer/media/repetition_detector.cc:30: } Empty line before. Add " // namespace" https://codereview.chromium.org/1357013006/diff/40001/content/renderer/media/... content/renderer/media/repetition_detector.cc:40: if (0 == count_frames_ && zero) { Nit: You can remove {} from single line if blocks if you like. https://codereview.chromium.org/1357013006/diff/40001/content/renderer/media/... content/renderer/media/repetition_detector.cc:40: if (0 == count_frames_ && zero) { count_frames_ == 0 https://codereview.chromium.org/1357013006/diff/40001/content/renderer/media/... content/renderer/media/repetition_detector.cc:45: all_zero_ = false; Is this correct? |zero| alone will determine what |all_zero_| is set to. The first if block is useless. This function could be ++count_frames_; all_zero_ = zero; https://codereview.chromium.org/1357013006/diff/40001/content/renderer/media/... content/renderer/media/repetition_detector.cc:77: ids_.push_back(pattern.id_); Can the ids and states be a map? Or do they need to be separate? What is |ids_| used for, when the id is in the state already? https://codereview.chromium.org/1357013006/diff/40001/content/renderer/media/... content/renderer/media/repetition_detector.cc:89: int sample_1k = max_look_back_ms_ * sample_rate_hz_; What does the name mean? One sample? 1k samples? Nit: Make it const. https://codereview.chromium.org/1357013006/diff/40001/content/renderer/media/... content/renderer/media/repetition_detector.cc:121: for (size_t cdx = 0; cdx < num_channels_; ++cdx, ++frame, ++it) { What is cdx? https://codereview.chromium.org/1357013006/diff/40001/content/renderer/media/... content/renderer/media/repetition_detector.cc:167: ReportRepetition(state->id()); This should be reported at the end of a session as a percentage of repetitions detected (of all Detect calls), by the user. Keep count of the number of calls to detect, and the number of detections, then add a function that returns the percentage. Log it in MSAP. It would be nice to have it per pattern as you did, but I'm not sure if that can be done with one histogram. And we shouldn't add more than one. https://codereview.chromium.org/1357013006/diff/40001/content/renderer/media/... File content/renderer/media/repetition_detector.h (right): https://codereview.chromium.org/1357013006/diff/40001/content/renderer/media/... content/renderer/media/repetition_detector.h:15: class RepetitionDetectorForTest; Remove. https://codereview.chromium.org/1357013006/diff/40001/content/renderer/media/... content/renderer/media/repetition_detector.h:17: class CONTENT_EXPORT RepetitionDetector { Call it AudioRepetitionDetector. (And rename files etc.) https://codereview.chromium.org/1357013006/diff/40001/content/renderer/media/... content/renderer/media/repetition_detector.h:17: class CONTENT_EXPORT RepetitionDetector { Add comment describing the class briefly. Also give an overview of the detection algorithm. https://codereview.chromium.org/1357013006/diff/40001/content/renderer/media/... content/renderer/media/repetition_detector.h:22: struct Pattern { Can this be private? https://codereview.chromium.org/1357013006/diff/40001/content/renderer/media/... content/renderer/media/repetition_detector.h:24: // All followings are in milliseconds, since repetition patterns are Move this comment to before the struct statement, and expand it to be a general comment about the struct. What is is used for? https://codereview.chromium.org/1357013006/diff/40001/content/renderer/media/... content/renderer/media/repetition_detector.h:31: // interleaved. Describe what it does if repetition is detected. https://codereview.chromium.org/1357013006/diff/40001/content/renderer/media/... content/renderer/media/repetition_detector.h:32: void Detect(const float* data, size_t num_frames, size_t num_channels, This should take a const AudioBus*. https://codereview.chromium.org/1357013006/diff/40001/content/renderer/media/... content/renderer/media/repetition_detector.h:36: friend class RepetitionDetectorForTest; // For testing. Should you use FRIEND_TEST_ALL_PREFIXES()? See other tests. https://codereview.chromium.org/1357013006/diff/40001/content/renderer/media/... content/renderer/media/repetition_detector.h:38: class State { Comment and describe the class and what it's used for. Also, the functions in it need to be described. https://codereview.chromium.org/1357013006/diff/40001/content/renderer/media/... content/renderer/media/repetition_detector.h:55: const int id_; Comment on all member variables. https://codereview.chromium.org/1357013006/diff/40001/content/renderer/media/... content/renderer/media/repetition_detector.h:84: std::vector<int> ids_; Comment on all member variables. https://codereview.chromium.org/1357013006/diff/40001/content/renderer/media/... content/renderer/media/repetition_detector.h:88: std::vector<float> audio_buffer_; // Ring buffers to store input audio. Put comments on its own line before the variable.
On 2015/09/23 08:48:07, minyue wrote: > Hi Henrik, > > The reason of placing it in Chrome but not APM is mainly because the repetition > is clearly not caused by APM. The earlier it sits, the more accurate information > it can provide. There are no other strong reasons. If you have concerns against > placing it here, please let me know. I may be not aware of risks of placing it > here. OK. I'm fine with having it here. > > https://codereview.chromium.org/1357013006/diff/40001/content/renderer/media/... > File content/renderer/media/media_stream_audio_processor.cc (right): > > https://codereview.chromium.org/1357013006/diff/40001/content/renderer/media/... > content/renderer/media/media_stream_audio_processor.cc:311: > repetition_detector_->Detect(process_bus->bus()->channel(0), > On 2015/09/23 06:54:15, Henrik Grunell wrote: > > How much complexity does this add? Would be good to document that in the bug > if > > it isn't already. > > > > How does the detector handle expected repetitions, like hooking a file or > > generate a sinus directly to the input? (Will that be bit exact?) > > It is O(n), n for number of samples. Each sample compares with the samples > sitting at the look back positions of the registered repetition patterns. > > https://codereview.chromium.org/1357013006/diff/40001/content/renderer/media/... > content/renderer/media/media_stream_audio_processor.cc:314: > input_format_.sample_rate()); > On 2015/09/23 06:54:15, Henrik Grunell wrote: > > Does the |process_bus| contain the sample rate? > > Unfortunately not. and that is why other places uses > input/output_format_.sample_rate() too, see, e.g., > > https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/m...
Also note that I'm not an owner either.
On 2015/09/23 11:10:38, Henrik Grunell wrote: > Also note that I'm not an owner either. Yes, I know. I plan to add tommi when it basically looks good.
On 2015/09/23 11:13:29, minyue wrote: > On 2015/09/23 11:10:38, Henrik Grunell wrote: > > Also note that I'm not an owner either. > > Yes, I know. I plan to add tommi when it basically looks good. You might want to get a clearance from owners to add the detector here before addressing CR comments.
minyue@chromium.org changed reviewers: + tommi@chromium.org
+tommi Hi Tommi, Please take a look at this CL and add comments on general questions like it is good to add this detector here. For the detection algorithm, to help with your review, I’d add that the method has been reviewed in a separate CL (which won't be committed), see https://codereview.webrtc.org/1287663002/
took a quick look through about half the CL :) I'll take a look again when the comments have been addressed. https://codereview.chromium.org/1357013006/diff/40001/content/renderer/media/... File content/renderer/media/media_stream_audio_processor_unittest.cc (right): https://codereview.chromium.org/1357013006/diff/40001/content/renderer/media/... content/renderer/media/media_stream_audio_processor_unittest.cc:459: { 8000, 16000, 32000, 44100, 48000, 88200, 96000 }; why this change? (I didn't see anything in the description that 22050 wasn't supported) https://codereview.chromium.org/1357013006/diff/40001/content/renderer/media/... File content/renderer/media/repetition_detector.cc (right): https://codereview.chromium.org/1357013006/diff/40001/content/renderer/media/... content/renderer/media/repetition_detector.cc:15: static const RepetitionDetector::Pattern kRepetitionPatterns[] = { no need for static since this is in an anonymous namespace. same for other globals. https://codereview.chromium.org/1357013006/diff/40001/content/renderer/media/... content/renderer/media/repetition_detector.cc:17: {1, 10, 10}, is id_ needed? Looks like it's just pos+1. https://codereview.chromium.org/1357013006/diff/40001/content/renderer/media/... content/renderer/media/repetition_detector.cc:29: static const size_t kMaxFrames = 480; // 10ms * 48kHz looks like this is assuming a certain sample rate. What happens if we're capturing at e.g. 88.2kHz? https://codereview.chromium.org/1357013006/diff/40001/content/renderer/media/... content/renderer/media/repetition_detector.cc:41: all_zero_ = true; is there a chance that all_zero_ is true when the function is called? if not, please add DCHECK(!all_zero_) at the top. If there is, then all_zero_ could be true even if zero is false or count_frames_ is non-zero. https://codereview.chromium.org/1357013006/diff/40001/content/renderer/media/... content/renderer/media/repetition_detector.cc:43: ++count_frames_; the class doesn't have any locking. please add a thread checker. https://codereview.chromium.org/1357013006/diff/40001/content/renderer/media/... content/renderer/media/repetition_detector.cc:45: all_zero_ = false; it looks like we're checking zero twice, setting all_zero_ to true or false depending... can we simplify? https://codereview.chromium.org/1357013006/diff/40001/content/renderer/media/... content/renderer/media/repetition_detector.cc:51: base::checked_cast<size_t>(min_length_ms_ * sample_rate_hz / 1000)); what benefit does checked_cast provide? https://codereview.chromium.org/1357013006/diff/40001/content/renderer/media/... content/renderer/media/repetition_detector.cc:78: states_.push_back(new State(pattern.id_, pattern.look_back_ms_, +1 on Henrik's comments. it seems strange that we're storing the id twice and using two arrays for the same information. https://codereview.chromium.org/1357013006/diff/40001/content/renderer/media/... File content/renderer/media/repetition_detector.h (right): https://codereview.chromium.org/1357013006/diff/40001/content/renderer/media/... content/renderer/media/repetition_detector.h:20: virtual ~RepetitionDetector(); Can you add documentation for the class, what it does, why we have it etc. Would also be good to document why there are virtual methods.
Thanks for your comments! Now comes a new patch. Happy reviewing :) https://codereview.chromium.org/1357013006/diff/40001/content/renderer/media/... File content/renderer/media/media_stream_audio_processor.cc (right): https://codereview.chromium.org/1357013006/diff/40001/content/renderer/media/... content/renderer/media/media_stream_audio_processor.cc:308: // Detect if bit-exact repetition of audio present in the captured audio. On 2015/09/23 10:36:41, Henrik Grunell wrote: > Nit: "Detect bit-exact repetition of audio." Done. https://codereview.chromium.org/1357013006/diff/40001/content/renderer/media/... content/renderer/media/media_stream_audio_processor.cc:314: input_format_.sample_rate()); On 2015/09/23 10:36:41, Henrik Grunell wrote: > On 2015/09/23 08:48:07, minyue wrote: > > On 2015/09/23 06:54:15, Henrik Grunell wrote: > > > Does the |process_bus| contain the sample rate? > > > > Unfortunately not. and that is why other places uses > > input/output_format_.sample_rate() too, see, e.g., > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/m... > > > > I looked into this more. The MediaStreamAudioBus wraps an AudioBus which > contains all information needed. Let take a an AudioBus > RepetitionDetector::Detect(const AudioBus*) > and handle it there. It is good for me to know that AudioBus has all info. and I can remove input_format_ here. but I am a little bit against the idea of taking AudioBus as input to Detect(). Since the detector is written a bit more general to accept any number of channels, and it is up to the user to decide whether to check all channels or only one (and which one). https://codereview.chromium.org/1357013006/diff/40001/content/renderer/media/... File content/renderer/media/media_stream_audio_processor_unittest.cc (right): https://codereview.chromium.org/1357013006/diff/40001/content/renderer/media/... content/renderer/media/media_stream_audio_processor_unittest.cc:459: { 8000, 16000, 32000, 44100, 48000, 88200, 96000 }; On 2015/09/25 08:57:14, tommi wrote: > why this change? (I didn't see anything in the description that 22050 wasn't > supported) The problem is 10 ms @ 22050 Hz is 220.5 samples. I don't know how that is handled. I had an offline discussion with Henrik and we thought that it would be good to remove this rate in this test. https://codereview.chromium.org/1357013006/diff/40001/content/renderer/media/... File content/renderer/media/repetition_detector.cc (right): https://codereview.chromium.org/1357013006/diff/40001/content/renderer/media/... content/renderer/media/repetition_detector.cc:7: #include "base/macros.h" // arraysize On 2015/09/23 10:36:41, Henrik Grunell wrote: > Remove comments. Done. https://codereview.chromium.org/1357013006/diff/40001/content/renderer/media/... content/renderer/media/repetition_detector.cc:10: #include "third_party/webrtc/base/checks.h" // rtc::CheckedDivExact On 2015/09/23 10:36:41, Henrik Grunell wrote: > Remove. Use CHECK or DCHECK before division instead. Done. https://codereview.chromium.org/1357013006/diff/40001/content/renderer/media/... content/renderer/media/repetition_detector.cc:15: static const RepetitionDetector::Pattern kRepetitionPatterns[] = { On 2015/09/23 10:36:42, Henrik Grunell wrote: > Empty line before. Done. https://codereview.chromium.org/1357013006/diff/40001/content/renderer/media/... content/renderer/media/repetition_detector.cc:15: static const RepetitionDetector::Pattern kRepetitionPatterns[] = { On 2015/09/25 08:57:14, tommi wrote: > no need for static since this is in an anonymous namespace. same for other > globals. Done. https://codereview.chromium.org/1357013006/diff/40001/content/renderer/media/... content/renderer/media/repetition_detector.cc:15: static const RepetitionDetector::Pattern kRepetitionPatterns[] = { On 2015/09/23 10:36:41, Henrik Grunell wrote: > Let the user of this class give the repetitions patterns in the constructor > instead, as an std::vector or std::array. I thought about letting user to register the patterns, but I think it is better make the detector more self-contained, since it is related to UMA. The user may or may not aware of this, and mess up things. https://codereview.chromium.org/1357013006/diff/40001/content/renderer/media/... content/renderer/media/repetition_detector.cc:16: // {id_, look_back_ms_, min_length_ms_} On 2015/09/23 10:36:42, Henrik Grunell wrote: > Remove this comment. It duplicates info. Done. https://codereview.chromium.org/1357013006/diff/40001/content/renderer/media/... content/renderer/media/repetition_detector.cc:17: {1, 10, 10}, On 2015/09/25 08:57:14, tommi wrote: > is id_ needed? Looks like it's just pos+1. Yes, indeed, but not necessarily, Imagine that we want to add pos=5 in future. https://codereview.chromium.org/1357013006/diff/40001/content/renderer/media/... content/renderer/media/repetition_detector.cc:29: static const size_t kMaxFrames = 480; // 10ms * 48kHz On 2015/09/23 10:36:41, Henrik Grunell wrote: > Nit: 10 ms * 48 kHz Done. https://codereview.chromium.org/1357013006/diff/40001/content/renderer/media/... content/renderer/media/repetition_detector.cc:29: static const size_t kMaxFrames = 480; // 10ms * 48kHz On 2015/09/25 08:57:14, tommi wrote: > looks like this is assuming a certain sample rate. What happens if we're > capturing at e.g. 88.2kHz? No it does not assume any sample rate. kMaxFrames can be anything. It is only used for copying a audio into buffer in a trunk. A frame longer than kMaxFrames will divided into trunks and processed one-by-one. I added a comment here. https://codereview.chromium.org/1357013006/diff/40001/content/renderer/media/... content/renderer/media/repetition_detector.cc:29: static const size_t kMaxFrames = 480; // 10ms * 48kHz On 2015/09/23 10:36:41, Henrik Grunell wrote: > Nit: 10 ms * 48 kHz Done. https://codereview.chromium.org/1357013006/diff/40001/content/renderer/media/... content/renderer/media/repetition_detector.cc:30: } On 2015/09/23 10:36:42, Henrik Grunell wrote: > Empty line before. > Add " // namespace" Done. https://codereview.chromium.org/1357013006/diff/40001/content/renderer/media/... content/renderer/media/repetition_detector.cc:40: if (0 == count_frames_ && zero) { On 2015/09/23 10:36:41, Henrik Grunell wrote: > count_frames_ == 0 Yes, I generally prefer "0 == xxx" so that mistype of "==" into "=" can be easily found. Is it necessary to change to "xxx == 0" https://codereview.chromium.org/1357013006/diff/40001/content/renderer/media/... content/renderer/media/repetition_detector.cc:40: if (0 == count_frames_ && zero) { On 2015/09/23 10:36:41, Henrik Grunell wrote: > Nit: You can remove {} from single line if blocks if you like. I am sometimes confused of which one is preferable. To me, one-line is like temporal, if one adds anything, like lines of comment I just did, it should be braced in {}. https://codereview.chromium.org/1357013006/diff/40001/content/renderer/media/... content/renderer/media/repetition_detector.cc:41: all_zero_ = true; On 2015/09/25 08:57:14, tommi wrote: > is there a chance that all_zero_ is true when the function is called? > > if not, please add DCHECK(!all_zero_) at the top. If there is, then all_zero_ > could be true even if zero is false or count_frames_ is non-zero. The point is that when we find "0 == 0" as the first sample in a repeat, we do not know if it will be an all-zero repetition (which we throw away), or this "0 == 0" is part of a valid repetition. So we should count it, but keep |all_zero_| set until we find a non zero. I added a comment here. https://codereview.chromium.org/1357013006/diff/40001/content/renderer/media/... content/renderer/media/repetition_detector.cc:43: ++count_frames_; On 2015/09/25 08:57:14, tommi wrote: > the class doesn't have any locking. please add a thread checker. Done. https://codereview.chromium.org/1357013006/diff/40001/content/renderer/media/... content/renderer/media/repetition_detector.cc:45: all_zero_ = false; On 2015/09/23 10:36:42, Henrik Grunell wrote: > Is this correct? |zero| alone will determine what |all_zero_| is set to. The > first if block is useless. This function could be > ++count_frames_; > all_zero_ = zero; not really. 1 2 3 0 1 2 3 0 will be considered all zero, with your proposal, right? |all_zero_| is used to make sure that the length of 0 1 2 3 0 1 2 3 is 4 instead of 3, while 0 0 0 0 0 0 0 0 won't be counted. https://codereview.chromium.org/1357013006/diff/40001/content/renderer/media/... content/renderer/media/repetition_detector.cc:51: base::checked_cast<size_t>(min_length_ms_ * sample_rate_hz / 1000)); On 2015/09/25 08:57:14, tommi wrote: > what benefit does checked_cast provide? not much indeed. Only if min_length_ms_ or sample_rate_hz are mistakenly set negative. So I think static_cast is enough. https://codereview.chromium.org/1357013006/diff/40001/content/renderer/media/... content/renderer/media/repetition_detector.cc:77: ids_.push_back(pattern.id_); On 2015/09/23 10:36:42, Henrik Grunell wrote: > Can the ids and states be a map? Or do they need to be separate? What is |ids_| > used for, when the id is in the state already? |ids_| is provided to UMA_HISTOGRAM_CUSTOM_ENUMERATION as the range of values. It would be cumbersome to create |ids_| every time we call UMA_HISTOGRAM_CUSTOM_ENUMERATION. https://codereview.chromium.org/1357013006/diff/40001/content/renderer/media/... content/renderer/media/repetition_detector.cc:78: states_.push_back(new State(pattern.id_, pattern.look_back_ms_, On 2015/09/25 08:57:14, tommi wrote: > +1 on Henrik's comments. it seems strange that we're storing the id twice and > using two arrays for the same information. please refer to my comment above https://codereview.chromium.org/1357013006/diff/40001/content/renderer/media/... content/renderer/media/repetition_detector.cc:89: int sample_1k = max_look_back_ms_ * sample_rate_hz_; On 2015/09/23 10:36:41, Henrik Grunell wrote: > What does the name mean? One sample? 1k samples? > Nit: Make it const. Yes, it is a bad name, it means sample * 1000. I'd just remove it. https://codereview.chromium.org/1357013006/diff/40001/content/renderer/media/... content/renderer/media/repetition_detector.cc:121: for (size_t cdx = 0; cdx < num_channels_; ++cdx, ++frame, ++it) { On 2015/09/23 10:36:42, Henrik Grunell wrote: > What is cdx? i'd rename it. https://codereview.chromium.org/1357013006/diff/40001/content/renderer/media/... content/renderer/media/repetition_detector.cc:167: ReportRepetition(state->id()); On 2015/09/23 10:36:41, Henrik Grunell wrote: > This should be reported at the end of a session as a percentage of repetitions > detected (of all Detect calls), by the user. Keep count of the number of calls > to detect, and the number of detections, then add a function that returns the > percentage. Log it in MSAP. It would be nice to have it per pattern as you did, > but I'm not sure if that can be done with one histogram. And we shouldn't add > more than one. Yes, what to add as UMA in this case is really disputable. UMA is one-dimensional, meaning reporting a % for each repetition id is not straightforward. The only way I can think of is send multiple reports at the end. Something like id=1 : 3 times id=2 : 2 times By this way, we can still tell the relative occurrence of different patterns. This is not very different from reporting-upon-occurring. What missing now is no report is made if no repetition happened. So we seems lack of how often we see repetition from Media.AudioCapturerRepetition alone. But my idea is 1. This frequency will be very small, and we are most interested in whether repetition happened and what kind. 2. together with other measures (call time, etc) we can still know of the absolute probability of repetitions. If you have better ideas, let's discuss. https://codereview.chromium.org/1357013006/diff/40001/content/renderer/media/... File content/renderer/media/repetition_detector.h (right): https://codereview.chromium.org/1357013006/diff/40001/content/renderer/media/... content/renderer/media/repetition_detector.h:15: class RepetitionDetectorForTest; On 2015/09/23 10:36:42, Henrik Grunell wrote: > Remove. Done. https://codereview.chromium.org/1357013006/diff/40001/content/renderer/media/... content/renderer/media/repetition_detector.h:17: class CONTENT_EXPORT RepetitionDetector { On 2015/09/23 10:36:42, Henrik Grunell wrote: > Add comment describing the class briefly. Also give an overview of the detection > algorithm. Done. https://codereview.chromium.org/1357013006/diff/40001/content/renderer/media/... content/renderer/media/repetition_detector.h:17: class CONTENT_EXPORT RepetitionDetector { On 2015/09/23 10:36:42, Henrik Grunell wrote: > Call it AudioRepetitionDetector. (And rename files etc.) Done. https://codereview.chromium.org/1357013006/diff/40001/content/renderer/media/... content/renderer/media/repetition_detector.h:20: virtual ~RepetitionDetector(); On 2015/09/25 08:57:14, tommi wrote: > Can you add documentation for the class, what it does, why we have it etc. Would > also be good to document why there are virtual methods. Yes, also per Henrik's request. The virtual is because we have another virtual function, I don't know if this needs any additional comment. https://codereview.chromium.org/1357013006/diff/40001/content/renderer/media/... content/renderer/media/repetition_detector.h:22: struct Pattern { On 2015/09/23 10:36:42, Henrik Grunell wrote: > Can this be private? Not really if we want the RegisterRepetitionPatterns to take outside (like global) variable. https://codereview.chromium.org/1357013006/diff/40001/content/renderer/media/... content/renderer/media/repetition_detector.h:24: // All followings are in milliseconds, since repetition patterns are On 2015/09/23 10:36:42, Henrik Grunell wrote: > Move this comment to before the struct statement, and expand it to be a general > comment about the struct. What is is used for? Done. https://codereview.chromium.org/1357013006/diff/40001/content/renderer/media/... content/renderer/media/repetition_detector.h:31: // interleaved. On 2015/09/23 10:36:42, Henrik Grunell wrote: > Describe what it does if repetition is detected. Done. https://codereview.chromium.org/1357013006/diff/40001/content/renderer/media/... content/renderer/media/repetition_detector.h:32: void Detect(const float* data, size_t num_frames, size_t num_channels, On 2015/09/23 10:36:42, Henrik Grunell wrote: > This should take a const AudioBus*. See my comment on a concern of making input format generic. https://codereview.chromium.org/1357013006/diff/40001/content/renderer/media/... content/renderer/media/repetition_detector.h:36: friend class RepetitionDetectorForTest; // For testing. On 2015/09/23 10:36:42, Henrik Grunell wrote: > Should you use FRIEND_TEST_ALL_PREFIXES()? See other tests. This has been considered and discussed in the parallel CL. The complication is RepetitionDetectorForTest is not Test, but an inherit of RepetitionDetector. RepetitionDetectorTest is the actually test. FRIEND_TEST is used upon gTests, such as RepetitionDetectorTest, but it is RepetitionDetectorForTest that needs to access private members. https://codereview.chromium.org/1357013006/diff/40001/content/renderer/media/... content/renderer/media/repetition_detector.h:38: class State { On 2015/09/23 10:36:42, Henrik Grunell wrote: > Comment and describe the class and what it's used for. Also, the functions in it > need to be described. Done. https://codereview.chromium.org/1357013006/diff/40001/content/renderer/media/... content/renderer/media/repetition_detector.h:55: const int id_; On 2015/09/23 10:36:42, Henrik Grunell wrote: > Comment on all member variables. Done. https://codereview.chromium.org/1357013006/diff/40001/content/renderer/media/... content/renderer/media/repetition_detector.h:84: std::vector<int> ids_; On 2015/09/23 10:36:42, Henrik Grunell wrote: > Comment on all member variables. Done. https://codereview.chromium.org/1357013006/diff/40001/content/renderer/media/... content/renderer/media/repetition_detector.h:88: std::vector<float> audio_buffer_; // Ring buffers to store input audio. On 2015/09/23 10:36:42, Henrik Grunell wrote: > Put comments on its own line before the variable. Done. https://codereview.chromium.org/1357013006/diff/60001/content/renderer/media/... File content/renderer/media/repetition_detector.h (right): https://codereview.chromium.org/1357013006/diff/60001/content/renderer/media/... content/renderer/media/repetition_detector.h:5: #ifndef CONTENT_RENDERER_MEDIA_AUDIO_REPETITION_DETECTOR_H_ I have not forgotten to change the file name, taking an intermediate step may make the review easier https://codereview.chromium.org/1357013006/diff/60001/content/renderer/media/... content/renderer/media/repetition_detector.h:68: const Pattern pattern_; I use a Pattern instead of three separate variables here.
https://codereview.chromium.org/1357013006/diff/40001/content/renderer/media/... File content/renderer/media/media_stream_audio_processor_unittest.cc (right): https://codereview.chromium.org/1357013006/diff/40001/content/renderer/media/... content/renderer/media/media_stream_audio_processor_unittest.cc:459: { 8000, 16000, 32000, 44100, 48000, 88200, 96000 }; On 2015/09/25 14:40:06, minyue wrote: > On 2015/09/25 08:57:14, tommi wrote: > > why this change? (I didn't see anything in the description that 22050 wasn't > > supported) > > The problem is 10 ms @ 22050 Hz is 220.5 samples. I don't know how that is > handled. I had an offline discussion with Henrik and we thought that it would be > good to remove this rate in this test. Hmm... from that answer it sounds like it's a corner case and we don't know how our code handles it and therefore there's even more reason for it to be in there... or? what sort of problems did you run into?
Hi Tommi, I made a mark at the place where 22050 Hz has a problem. https://codereview.chromium.org/1357013006/diff/60001/content/renderer/media/... File content/renderer/media/repetition_detector.cc (right): https://codereview.chromium.org/1357013006/diff/60001/content/renderer/media/... content/renderer/media/repetition_detector.cc:173: DCHECK_EQ(0, state->look_back_ms() * sample_rate_hz_ % 1000); The 22050 hz was removed from the unittest because of a failure here. But of course, we can relax this DCHECK. Then 10 ms won't be exact and will mean 220 frames.
https://codereview.chromium.org/1357013006/diff/40001/content/renderer/media/... File content/renderer/media/media_stream_audio_processor_unittest.cc (right): https://codereview.chromium.org/1357013006/diff/40001/content/renderer/media/... content/renderer/media/media_stream_audio_processor_unittest.cc:459: { 8000, 16000, 32000, 44100, 48000, 88200, 96000 }; On 2015/09/25 14:56:58, tommi wrote: > On 2015/09/25 14:40:06, minyue wrote: > > On 2015/09/25 08:57:14, tommi wrote: > > > why this change? (I didn't see anything in the description that 22050 wasn't > > > supported) > > > > The problem is 10 ms @ 22050 Hz is 220.5 samples. I don't know how that is > > handled. I had an offline discussion with Henrik and we thought that it would > be > > good to remove this rate in this test. > > Hmm... from that answer it sounds like it's a corner case and we don't know how > our code handles it and therefore there's even more reason for it to be in > there... or? > > what sort of problems did you run into? According to Henrik A, Chrome doesn't support 22050 for input, hence I thought it's no point for the MSAP to support that. If this isn't correct we need to look over the support for that since the test failed. I don'd see any sample rate value sanity checks in MSAP, seems to swallow anything > 0. Maybe supported rates should be explicit there.
On 2015/09/27 07:34:11, Henrik Grunell wrote: > https://codereview.chromium.org/1357013006/diff/40001/content/renderer/media/... > File content/renderer/media/media_stream_audio_processor_unittest.cc (right): > > https://codereview.chromium.org/1357013006/diff/40001/content/renderer/media/... > content/renderer/media/media_stream_audio_processor_unittest.cc:459: { 8000, > 16000, 32000, 44100, 48000, 88200, 96000 }; > On 2015/09/25 14:56:58, tommi wrote: > > On 2015/09/25 14:40:06, minyue wrote: > > > On 2015/09/25 08:57:14, tommi wrote: > > > > why this change? (I didn't see anything in the description that 22050 > wasn't > > > > supported) > > > > > > The problem is 10 ms @ 22050 Hz is 220.5 samples. I don't know how that is > > > handled. I had an offline discussion with Henrik and we thought that it > would > > be > > > good to remove this rate in this test. > > > > Hmm... from that answer it sounds like it's a corner case and we don't know > how > > our code handles it and therefore there's even more reason for it to be in > > there... or? > > > > what sort of problems did you run into? > > According to Henrik A, Chrome doesn't support 22050 for input, hence I thought > it's no point for the MSAP to support that. If this isn't correct we need to > look over the support for that since the test failed. Fortunately, it's easy to check those things. 22050 is very rare (1/2000 fwict), but it is supported. Same goes for 11025. Fwict we're talking about something like 0.005% in those two cases which may not seem like a lot until you convert that into number of users :) > I don'd see any sample rate value sanity checks in MSAP, seems to swallow > anything > 0. Maybe supported rates should be explicit there. Agree. The repetition detector shouldn't change what the audio stack already supports. If it can't support some sample rates, let's simply not use it for those sample rates. I don't think it will affect the stats.
On 2015/09/27 08:12:49, tommi wrote: > On 2015/09/27 07:34:11, Henrik Grunell wrote: > > > https://codereview.chromium.org/1357013006/diff/40001/content/renderer/media/... > > File content/renderer/media/media_stream_audio_processor_unittest.cc (right): > > > > > https://codereview.chromium.org/1357013006/diff/40001/content/renderer/media/... > > content/renderer/media/media_stream_audio_processor_unittest.cc:459: { 8000, > > 16000, 32000, 44100, 48000, 88200, 96000 }; > > On 2015/09/25 14:56:58, tommi wrote: > > > On 2015/09/25 14:40:06, minyue wrote: > > > > On 2015/09/25 08:57:14, tommi wrote: > > > > > why this change? (I didn't see anything in the description that 22050 > > wasn't > > > > > supported) > > > > > > > > The problem is 10 ms @ 22050 Hz is 220.5 samples. I don't know how that is > > > > handled. I had an offline discussion with Henrik and we thought that it > > would > > > be > > > > good to remove this rate in this test. > > > > > > Hmm... from that answer it sounds like it's a corner case and we don't know > > how > > > our code handles it and therefore there's even more reason for it to be in > > > there... or? > > > > > > what sort of problems did you run into? > > > > According to Henrik A, Chrome doesn't support 22050 for input, hence I thought > > it's no point for the MSAP to support that. If this isn't correct we need to > > look over the support for that since the test failed. > > Fortunately, it's easy to check those things. 22050 is very rare (1/2000 > fwict), but it is supported. Same goes for 11025. > Fwict we're talking about something like 0.005% in those two cases which may not > seem like a lot until you convert that into number of users :) > > > I don'd see any sample rate value sanity checks in MSAP, seems to swallow > > anything > 0. Maybe supported rates should be explicit there. > > Agree. The repetition detector shouldn't change what the audio stack already > supports. If it can't support some sample rates, let's simply not use it for > those sample rates. I don't think it will affect the stats. sorry, that was supposed to be 1/20000 :)
On 2015/09/27 08:12:49, tommi wrote: > On 2015/09/27 07:34:11, Henrik Grunell wrote: > > > https://codereview.chromium.org/1357013006/diff/40001/content/renderer/media/... > > File content/renderer/media/media_stream_audio_processor_unittest.cc (right): > > > > > https://codereview.chromium.org/1357013006/diff/40001/content/renderer/media/... > > content/renderer/media/media_stream_audio_processor_unittest.cc:459: { 8000, > > 16000, 32000, 44100, 48000, 88200, 96000 }; > > On 2015/09/25 14:56:58, tommi wrote: > > > On 2015/09/25 14:40:06, minyue wrote: > > > > On 2015/09/25 08:57:14, tommi wrote: > > > > > why this change? (I didn't see anything in the description that 22050 > > wasn't > > > > > supported) > > > > > > > > The problem is 10 ms @ 22050 Hz is 220.5 samples. I don't know how that is > > > > handled. I had an offline discussion with Henrik and we thought that it > > would > > > be > > > > good to remove this rate in this test. > > > > > > Hmm... from that answer it sounds like it's a corner case and we don't know > > how > > > our code handles it and therefore there's even more reason for it to be in > > > there... or? > > > > > > what sort of problems did you run into? > > > > According to Henrik A, Chrome doesn't support 22050 for input, hence I thought > > it's no point for the MSAP to support that. If this isn't correct we need to > > look over the support for that since the test failed. > > Fortunately, it's easy to check those things. 22050 is very rare (1/2000 > fwict), but it is supported. Same goes for 11025. > Fwict we're talking about something like 0.005% in those two cases which may not > seem like a lot until you convert that into number of users :) OK, good to know. > > I don'd see any sample rate value sanity checks in MSAP, seems to swallow > > anything > 0. Maybe supported rates should be explicit there. > > Agree. The repetition detector shouldn't change what the audio stack already > supports. If it can't support some sample rates, let's simply not use it for > those sample rates. I don't think it will affect the stats. Yep, sounds good. Minyue: can you update the CL accordingly? (You don't have to add sample rate checks in MSAP.)
Yes, thanks for the comments. An small update made. I am working on another computer and could not handle rebase well. So may changes are due to rebase. The only two real changes are marked. https://codereview.chromium.org/1357013006/diff/60001/content/renderer/media/... File content/renderer/media/media_stream_audio_processor_unittest.cc (left): https://codereview.chromium.org/1357013006/diff/60001/content/renderer/media/... content/renderer/media/media_stream_audio_processor_unittest.cc:459: { 8000, 16000, 22050, 32000, 44100, 48000, 88200, 96000 }; added back https://codereview.chromium.org/1357013006/diff/80001/content/renderer/media/... File content/renderer/media/repetition_detector.cc (right): https://codereview.chromium.org/1357013006/diff/80001/content/renderer/media/... content/renderer/media/repetition_detector.cc:173: // the closest integer. new change
started looking but it looks like some comments haven't been addressed. Will you ping back? https://codereview.chromium.org/1357013006/diff/80001/content/renderer/media/... File content/renderer/media/repetition_detector.h (right): https://codereview.chromium.org/1357013006/diff/80001/content/renderer/media/... content/renderer/media/repetition_detector.h:54: // Increase the counter by 1, and tell if the counted audio is zero. can you clarify what counter? Also, should the parameter name perhaps be all_zero? https://codereview.chromium.org/1357013006/diff/80001/content/renderer/media/... content/renderer/media/repetition_detector.h:58: bool HasValidReport(int sample_rate_khz) const; is this really khz? (and not just Hz). If it's kHz it needs to be float/double. https://codereview.chromium.org/1357013006/diff/80001/content/renderer/media/... content/renderer/media/repetition_detector.h:88: void Reset(size_t num_channels, int sample_rate_hz); nit: If sample_rate_hz is the same thing as sample_rate in most other places, including AudioParameters, I suggest just sticking to sample_rate. https://codereview.chromium.org/1357013006/diff/80001/content/renderer/media/... content/renderer/media/repetition_detector.h:123: int sample_rate_hz_; nit: sample_rate_;
Patchset #6 (id:100001) has been deleted
Hi Tommi and Henrik, I did a careful check of the comments, they seems to be all addressed (some are replied with discussions) Now Patch set 6 is purely renaming of files as quested. Patch set 7 is small and it addresses Tommi's new comments. https://codereview.chromium.org/1357013006/diff/40001/content/renderer/media/... File content/renderer/media/media_stream_audio_processor_unittest.cc (right): https://codereview.chromium.org/1357013006/diff/40001/content/renderer/media/... content/renderer/media/media_stream_audio_processor_unittest.cc:459: { 8000, 16000, 32000, 44100, 48000, 88200, 96000 }; On 2015/09/27 07:34:11, Henrik Grunell wrote: > On 2015/09/25 14:56:58, tommi wrote: > > On 2015/09/25 14:40:06, minyue wrote: > > > On 2015/09/25 08:57:14, tommi wrote: > > > > why this change? (I didn't see anything in the description that 22050 > wasn't > > > > supported) > > > > > > The problem is 10 ms @ 22050 Hz is 220.5 samples. I don't know how that is > > > handled. I had an offline discussion with Henrik and we thought that it > would > > be > > > good to remove this rate in this test. > > > > Hmm... from that answer it sounds like it's a corner case and we don't know > how > > our code handles it and therefore there's even more reason for it to be in > > there... or? > > > > what sort of problems did you run into? > > According to Henrik A, Chrome doesn't support 22050 for input, hence I thought > it's no point for the MSAP to support that. If this isn't correct we need to > look over the support for that since the test failed. > > I don'd see any sample rate value sanity checks in MSAP, seems to swallow > anything > 0. Maybe supported rates should be explicit there. 22050 has been added back https://codereview.chromium.org/1357013006/diff/80001/content/renderer/media/... File content/renderer/media/repetition_detector.h (right): https://codereview.chromium.org/1357013006/diff/80001/content/renderer/media/... content/renderer/media/repetition_detector.h:58: bool HasValidReport(int sample_rate_khz) const; On 2015/09/27 12:54:34, tommi wrote: > is this really khz? (and not just Hz). If it's kHz it needs to be float/double. Sorry, this is a typo. the .cc file has it right https://codereview.chromium.org/1357013006/diff/80001/content/renderer/media/... content/renderer/media/repetition_detector.h:88: void Reset(size_t num_channels, int sample_rate_hz); On 2015/09/27 12:54:34, tommi wrote: > nit: If sample_rate_hz is the same thing as sample_rate in most other places, > including AudioParameters, I suggest just sticking to sample_rate. It would be always a good practice to add the unit to leave no ambiguity to developers and users. But I am not aware of "other places". If they all omit this, we may do the same here. WDYT? https://codereview.chromium.org/1357013006/diff/80001/content/renderer/media/... content/renderer/media/repetition_detector.h:123: int sample_rate_hz_; On 2015/09/27 12:54:34, tommi wrote: > nit: sample_rate_; same as above
https://codereview.chromium.org/1357013006/diff/40001/content/renderer/media/... File content/renderer/media/repetition_detector.cc (right): https://codereview.chromium.org/1357013006/diff/40001/content/renderer/media/... content/renderer/media/repetition_detector.cc:17: {1, 10, 10}, On 2015/09/25 14:40:07, minyue wrote: > On 2015/09/25 08:57:14, tommi wrote: > > is id_ needed? Looks like it's just pos+1. > > Yes, indeed, but not necessarily, Imagine that we want to add pos=5 in future. Sorry, I'm not following... it still looks to me like the id_ member in Pattern isn't needed. https://codereview.chromium.org/1357013006/diff/40001/content/renderer/media/... File content/renderer/media/repetition_detector.h (right): https://codereview.chromium.org/1357013006/diff/40001/content/renderer/media/... content/renderer/media/repetition_detector.h:36: friend class RepetitionDetectorForTest; // For testing. On 2015/09/25 14:40:08, minyue wrote: > On 2015/09/23 10:36:42, Henrik Grunell wrote: > > Should you use FRIEND_TEST_ALL_PREFIXES()? See other tests. > > This has been considered and discussed in the parallel CL. The complication is > > RepetitionDetectorForTest is not Test, but an inherit of RepetitionDetector. > RepetitionDetectorTest is the actually test. > > FRIEND_TEST is used upon gTests, such as RepetitionDetectorTest, but it is > RepetitionDetectorForTest that needs to access private members. Did you discuss making the parts that the derived class needs access to, protected instead? https://codereview.chromium.org/1357013006/diff/80001/content/renderer/media/... File content/renderer/media/repetition_detector.h (right): https://codereview.chromium.org/1357013006/diff/80001/content/renderer/media/... content/renderer/media/repetition_detector.h:88: void Reset(size_t num_channels, int sample_rate_hz); On 2015/09/28 05:22:00, minyue wrote: > On 2015/09/27 12:54:34, tommi wrote: > > nit: If sample_rate_hz is the same thing as sample_rate in most other places, > > including AudioParameters, I suggest just sticking to sample_rate. > > It would be always a good practice to add the unit to leave no ambiguity to > developers and users. > > But I am not aware of "other places". If they all omit this, we may do the same > here. WDYT? AudioParameters is well known so I think we should follow that example.
https://codereview.chromium.org/1357013006/diff/40001/content/renderer/media/... File content/renderer/media/repetition_detector.cc (right): https://codereview.chromium.org/1357013006/diff/40001/content/renderer/media/... content/renderer/media/repetition_detector.cc:17: {1, 10, 10}, On 2015/09/28 10:28:39, tommi wrote: > On 2015/09/25 14:40:07, minyue wrote: > > On 2015/09/25 08:57:14, tommi wrote: > > > is id_ needed? Looks like it's just pos+1. > > > > Yes, indeed, but not necessarily, Imagine that we want to add pos=5 in future. > > Sorry, I'm not following... it still looks to me like the id_ member in Pattern > isn't needed. Sorry for being not clear enough. I meant that if we'd, by some reason, add to the look back time (10 ms, 20, 30, ...) a 5 ms, or 15, 25 ..., we could not make the id if we only allow id == look back time/10 https://codereview.chromium.org/1357013006/diff/40001/content/renderer/media/... File content/renderer/media/repetition_detector.h (right): https://codereview.chromium.org/1357013006/diff/40001/content/renderer/media/... content/renderer/media/repetition_detector.h:36: friend class RepetitionDetectorForTest; // For testing. On 2015/09/28 10:28:39, tommi wrote: > On 2015/09/25 14:40:08, minyue wrote: > > On 2015/09/23 10:36:42, Henrik Grunell wrote: > > > Should you use FRIEND_TEST_ALL_PREFIXES()? See other tests. > > > > This has been considered and discussed in the parallel CL. The complication is > > > > RepetitionDetectorForTest is not Test, but an inherit of RepetitionDetector. > > RepetitionDetectorTest is the actually test. > > > > FRIEND_TEST is used upon gTests, such as RepetitionDetectorTest, but it is > > RepetitionDetectorForTest that needs to access private members. > > Did you discuss making the parts that the derived class needs access to, > protected instead? I used protected, and reviewers wanted me to use Friend instead. https://codereview.chromium.org/1357013006/diff/80001/content/renderer/media/... File content/renderer/media/repetition_detector.h (right): https://codereview.chromium.org/1357013006/diff/80001/content/renderer/media/... content/renderer/media/repetition_detector.h:54: // Increase the counter by 1, and tell if the counted audio is zero. On 2015/09/27 12:54:34, tommi wrote: > can you clarify what counter? > Also, should the parameter name perhaps be all_zero? solved in patch set 7 (but the file had been renamed by then) https://codereview.chromium.org/1357013006/diff/80001/content/renderer/media/... content/renderer/media/repetition_detector.h:88: void Reset(size_t num_channels, int sample_rate_hz); On 2015/09/28 10:28:39, tommi wrote: > On 2015/09/28 05:22:00, minyue wrote: > > On 2015/09/27 12:54:34, tommi wrote: > > > nit: If sample_rate_hz is the same thing as sample_rate in most other > places, > > > including AudioParameters, I suggest just sticking to sample_rate. > > > > It would be always a good practice to add the unit to leave no ambiguity to > > developers and users. > > > > But I am not aware of "other places". If they all omit this, we may do the > same > > here. WDYT? > > AudioParameters is well known so I think we should follow that example. Acknowledged. and will do
https://codereview.chromium.org/1357013006/diff/40001/content/renderer/media/... File content/renderer/media/repetition_detector.cc (right): https://codereview.chromium.org/1357013006/diff/40001/content/renderer/media/... content/renderer/media/repetition_detector.cc:17: {1, 10, 10}, On 2015/09/28 14:47:50, minyue wrote: > On 2015/09/28 10:28:39, tommi wrote: > > On 2015/09/25 14:40:07, minyue wrote: > > > On 2015/09/25 08:57:14, tommi wrote: > > > > is id_ needed? Looks like it's just pos+1. > > > > > > Yes, indeed, but not necessarily, Imagine that we want to add pos=5 in > future. > > > > Sorry, I'm not following... it still looks to me like the id_ member in > Pattern > > isn't needed. > > Sorry for being not clear enough. I meant that if we'd, by some reason, add to > the look back time (10 ms, 20, 30, ...) a 5 ms, or 15, 25 ..., we could not make > the id if we only allow id == look back time/10 Ah I see. Thanks. https://codereview.chromium.org/1357013006/diff/140001/content/renderer/media... File content/renderer/media/audio_repetition_detector.cc (right): https://codereview.chromium.org/1357013006/diff/140001/content/renderer/media... content/renderer/media/audio_repetition_detector.cc:88: if (pattern.look_back_ms > max_look_back_ms_) { nit: no {} https://codereview.chromium.org/1357013006/diff/140001/content/renderer/media... content/renderer/media/audio_repetition_detector.cc:104: for (auto state : states_) { nit: no {} https://codereview.chromium.org/1357013006/diff/140001/content/renderer/media... content/renderer/media/audio_repetition_detector.cc:134: if (*frame != *it) { no {} (same for rest of file) https://codereview.chromium.org/1357013006/diff/140001/content/renderer/media... File content/renderer/media/audio_repetition_detector_unittest.cc (right): https://codereview.chromium.org/1357013006/diff/140001/content/renderer/media... content/renderer/media/audio_repetition_detector_unittest.cc:18: if (it == counters_.end()) { no {} for one line scopes (inclusive of comments) https://codereview.chromium.org/1357013006/diff/140001/content/renderer/media... File content/renderer/media/media_stream_audio_processor.cc (right): https://codereview.chromium.org/1357013006/diff/140001/content/renderer/media... content/renderer/media/media_stream_audio_processor.cc:242: audio_repetition_detector_(new AudioRepetitionDetector()), do we always want to create a detector? (what about the odd sample rates?) https://codereview.chromium.org/1357013006/diff/140001/content/renderer/media... content/renderer/media/media_stream_audio_processor.cc:304: if (audio_repetition_detector_) { if we always allocate a repetition detector, we shouldn't have this check.
Hi, There is an update. Please happy review:) https://codereview.chromium.org/1357013006/diff/140001/content/renderer/media... File content/renderer/media/audio_repetition_detector.cc (right): https://codereview.chromium.org/1357013006/diff/140001/content/renderer/media... content/renderer/media/audio_repetition_detector.cc:88: if (pattern.look_back_ms > max_look_back_ms_) { On 2015/09/28 16:13:03, tommi wrote: > nit: no {} I got a similar comment from Henrik too. I generally do not favor of dealing with one line {} any specially, since one line is not stable and can become multiple lines easily, e.g., adding some comments. But since you both pointed this out. I am ok with it since you know Chromium style better. I did a search of similar cases and tried to cover all of them. https://codereview.chromium.org/1357013006/diff/140001/content/renderer/media... content/renderer/media/audio_repetition_detector.cc:104: for (auto state : states_) { On 2015/09/28 16:13:03, tommi wrote: > nit: no {} Done. https://codereview.chromium.org/1357013006/diff/140001/content/renderer/media... content/renderer/media/audio_repetition_detector.cc:134: if (*frame != *it) { On 2015/09/28 16:13:03, tommi wrote: > no {} (same for rest of file) Done. https://codereview.chromium.org/1357013006/diff/140001/content/renderer/media... File content/renderer/media/audio_repetition_detector_unittest.cc (right): https://codereview.chromium.org/1357013006/diff/140001/content/renderer/media... content/renderer/media/audio_repetition_detector_unittest.cc:18: if (it == counters_.end()) { On 2015/09/28 16:13:03, tommi wrote: > no {} for one line scopes (inclusive of comments) Done.
Starting to look good! https://codereview.chromium.org/1357013006/diff/160001/content/renderer/media... File content/renderer/media/audio_repetition_detector.cc (right): https://codereview.chromium.org/1357013006/diff/160001/content/renderer/media... content/renderer/media/audio_repetition_detector.cc:15: const AudioRepetitionDetector::Pattern kRepetitionPatterns[] = { I still think that patterns and UMA stats reporting should be decoupled from the detector class. Tommi, wdyt? https://codereview.chromium.org/1357013006/diff/160001/content/renderer/media... content/renderer/media/audio_repetition_detector.cc:37: : pattern_(pattern) { I think the variables should be initialized in the init list. I'm not sure what the code style guide says, but I do think it's better. https://codereview.chromium.org/1357013006/diff/160001/content/renderer/media... content/renderer/media/audio_repetition_detector.cc:146: void AudioRepetitionDetector::Detect(const float* data, size_t num_frames, Put all function definitions in the same order as the declarations. (Group classes/structs together.) https://codereview.chromium.org/1357013006/diff/160001/content/renderer/media... content/renderer/media/audio_repetition_detector.cc:153: while (num_frames > max_frames_) { Comment on what this does. It's not obvious at a first read. It's good to get a high level understanding of the algorithm when just glancing over the code. https://codereview.chromium.org/1357013006/diff/160001/content/renderer/media... content/renderer/media/audio_repetition_detector.cc:159: if (num_frames == 0) Can |num_frames| be < 0 at this point? https://codereview.chromium.org/1357013006/diff/160001/content/renderer/media... content/renderer/media/audio_repetition_detector.cc:177: ReportRepetition(state->id()); I saw your reply to my previous comment on this. Let's talk offline about how to craft the stats we report. I suspect we won't have use of this data as is now. https://codereview.chromium.org/1357013006/diff/160001/content/renderer/media... File content/renderer/media/audio_repetition_detector.h (right): https://codereview.chromium.org/1357013006/diff/160001/content/renderer/media... content/renderer/media/audio_repetition_detector.h:69: // repetition pattern this state keeps track of. Nit: Start sentences with capital letter. Here and below. https://codereview.chromium.org/1357013006/diff/160001/content/renderer/media... content/renderer/media/audio_repetition_detector.h:110: // The Ids of all registered patterns. This defines the range of values for Will this set of ids be the exactly the same as the ids in the set of states? If yes, why is this needed? If no, why does it differ?
Thank you for the comments! I made some updates and replies. https://codereview.chromium.org/1357013006/diff/160001/content/renderer/media... File content/renderer/media/audio_repetition_detector.cc (right): https://codereview.chromium.org/1357013006/diff/160001/content/renderer/media... content/renderer/media/audio_repetition_detector.cc:15: const AudioRepetitionDetector::Pattern kRepetitionPatterns[] = { On 2015/09/29 07:23:03, Henrik Grunell wrote: > I still think that patterns and UMA stats reporting should be decoupled from the > detector class. Tommi, wdyt? I do not mind making changes. But I think letting users of AudioRepetitionDetector do pattern registration will potentially mess up the UMA stats, since it is hardcoded in AudioRepetitionDetector. Tommi, would you comment on this? https://codereview.chromium.org/1357013006/diff/160001/content/renderer/media... content/renderer/media/audio_repetition_detector.cc:37: : pattern_(pattern) { On 2015/09/29 07:23:03, Henrik Grunell wrote: > I think the variables should be initialized in the init list. I'm not sure what > the code style guide says, but I do think it's better. I could not find explicit guide lines on this either. I think adding " count_frames_ = 0; all_zero_ = true; reported_ = false; " In both initializer list and Reset(), which is anyway needed, is a redundant. https://codereview.chromium.org/1357013006/diff/160001/content/renderer/media... content/renderer/media/audio_repetition_detector.cc:146: void AudioRepetitionDetector::Detect(const float* data, size_t num_frames, On 2015/09/29 07:23:04, Henrik Grunell wrote: > Put all function definitions in the same order as the declarations. (Group > classes/structs together.) I'd do this when else are done, in a separate patch set. Otherwise, review will be difficult. https://codereview.chromium.org/1357013006/diff/160001/content/renderer/media... content/renderer/media/audio_repetition_detector.cc:153: while (num_frames > max_frames_) { On 2015/09/29 07:23:03, Henrik Grunell wrote: > Comment on what this does. It's not obvious at a first read. It's good to get a > high level understanding of the algorithm when just glancing over the code. Done. https://codereview.chromium.org/1357013006/diff/160001/content/renderer/media... content/renderer/media/audio_repetition_detector.cc:159: if (num_frames == 0) On 2015/09/29 07:23:03, Henrik Grunell wrote: > Can |num_frames| be < 0 at this point? num_fames is size_t, so cannot be negative. https://codereview.chromium.org/1357013006/diff/160001/content/renderer/media... content/renderer/media/audio_repetition_detector.cc:177: ReportRepetition(state->id()); On 2015/09/29 07:23:03, Henrik Grunell wrote: > I saw your reply to my previous comment on this. Let's talk offline about how to > craft the stats we report. I suspect we won't have use of this data as is now. ok, will do https://codereview.chromium.org/1357013006/diff/160001/content/renderer/media... File content/renderer/media/audio_repetition_detector.h (right): https://codereview.chromium.org/1357013006/diff/160001/content/renderer/media... content/renderer/media/audio_repetition_detector.h:69: // repetition pattern this state keeps track of. On 2015/09/29 07:23:04, Henrik Grunell wrote: > Nit: Start sentences with capital letter. Here and below. Done. https://codereview.chromium.org/1357013006/diff/160001/content/renderer/media... content/renderer/media/audio_repetition_detector.h:110: // The Ids of all registered patterns. This defines the range of values for On 2015/09/29 07:23:04, Henrik Grunell wrote: > Will this set of ids be the exactly the same as the ids in the set of states? If > yes, why is this needed? If no, why does it differ? I maybe have answered this. This is to make ReportRepetition() less computation, since it needs all ids as a vector<int>.
I didn't look at AudioRepetitionDetector closely, as I think you're getting a good review from Tommi/Henrik already. Minyue, let me know if you'd like me to take a closer look. https://codereview.chromium.org/1357013006/diff/180001/content/renderer/media... File content/renderer/media/audio_repetition_detector.cc (right): https://codereview.chromium.org/1357013006/diff/180001/content/renderer/media... content/renderer/media/audio_repetition_detector.cc:154: // Therefore, input data with larger frames needs be divided into trunks. s/trunks/chunks https://codereview.chromium.org/1357013006/diff/180001/content/renderer/media... File content/renderer/media/audio_repetition_detector.h (right): https://codereview.chromium.org/1357013006/diff/180001/content/renderer/media... content/renderer/media/audio_repetition_detector.h:30: struct Pattern { I didn't look closely at this, so feel free to ignore, but this looks like it would be natural to omit id here and store a collection of Patterns in a map rather than a vector below. https://codereview.chromium.org/1357013006/diff/180001/content/renderer/media... File content/renderer/media/media_stream_audio_processor.cc (right): https://codereview.chromium.org/1357013006/diff/180001/content/renderer/media... content/renderer/media/media_stream_audio_processor.cc:304: if (audio_repetition_detector_) { Why do you need to check this? https://codereview.chromium.org/1357013006/diff/180001/content/renderer/media... File content/renderer/media/media_stream_audio_processor.h (right): https://codereview.chromium.org/1357013006/diff/180001/content/renderer/media... content/renderer/media/media_stream_audio_processor.h:158: scoped_ptr<AudioRepetitionDetector> audio_repetition_detector_; I don't see any reason this needs to use a scoped_ptr.
Thanks for the new comments. Patch set 10 is upon Henrik's request on the function's ordering + I found one empty function (marked) Patch set 11 is upon Andrew's comments. There are two remaining questions 1. whether let caller register patterns. My idea of not letting the caller do it is because the UMA is hard-coded and user may accidentally mess up UMA. 2. what UMA stats are best. being discussed offline. ~~~~ To Andrew, The algorithm is the same as in the WebRTC CL. I did not get LG from you (got it from Henrik L) but all your comments have been addressed there. You may be involved in the discussion of the above two questions if you like, and/or take a look at the WebRTC CL and see if there are some issues that are not solved perfectly. https://codereview.chromium.org/1357013006/diff/180001/content/renderer/media... File content/renderer/media/audio_repetition_detector.h (right): https://codereview.chromium.org/1357013006/diff/180001/content/renderer/media... content/renderer/media/audio_repetition_detector.h:30: struct Pattern { On 2015/09/30 00:23:43, ajm wrote: > I didn't look closely at this, so feel free to ignore, but this looks like it > would be natural to omit id here and store a collection of Patterns in a map > rather than a vector below. This is a good point. Yes, map is natural way to avoid id duplication, but Pattern is a simpler, and thus I can write const AudioRepetitionDetector::Pattern kRepetitionPatterns[] = { {1, 10, 10}, {2, 20, 10}, ... C++11's std:map initializer may make it as simple, but it is a C++11 feature. Do you have some suggestions? https://codereview.chromium.org/1357013006/diff/180001/content/renderer/media... content/renderer/media/audio_repetition_detector.h:87: void ClearRepetitionPatterns(); I found this useless and therefore removed it. https://codereview.chromium.org/1357013006/diff/180001/content/renderer/media... File content/renderer/media/media_stream_audio_processor.cc (right): https://codereview.chromium.org/1357013006/diff/180001/content/renderer/media... content/renderer/media/media_stream_audio_processor.cc:304: if (audio_repetition_detector_) { On 2015/09/30 00:23:43, ajm wrote: > Why do you need to check this? Good point. Tommi asked about whether audio_repetition_detector_ needed to be always created, I answered it, but should have realized that this check could be removed. https://codereview.chromium.org/1357013006/diff/180001/content/renderer/media... File content/renderer/media/media_stream_audio_processor.h (right): https://codereview.chromium.org/1357013006/diff/180001/content/renderer/media... content/renderer/media/media_stream_audio_processor.h:158: scoped_ptr<AudioRepetitionDetector> audio_repetition_detector_; On 2015/09/30 00:23:43, ajm wrote: > I don't see any reason this needs to use a scoped_ptr. good point. I simply followed the momentum of other modules, but true, it does not need a pointer.
lgtm on media_stream_audio_processor. https://chromiumcodereview.appspot.com/1357013006/diff/180001/content/rendere... File content/renderer/media/audio_repetition_detector.h (right): https://chromiumcodereview.appspot.com/1357013006/diff/180001/content/rendere... content/renderer/media/audio_repetition_detector.h:30: struct Pattern { On 2015/09/30 19:52:32, minyue wrote: > On 2015/09/30 00:23:43, ajm wrote: > > I didn't look closely at this, so feel free to ignore, but this looks like it > > would be natural to omit id here and store a collection of Patterns in a map > > rather than a vector below. > > This is a good point. Yes, map is natural way to avoid id duplication, but > Pattern is a simpler, and thus I can write > const AudioRepetitionDetector::Pattern kRepetitionPatterns[] = { > {1, 10, 10}, > {2, 20, 10}, ... > > C++11's std:map initializer may make it as simple, but it is a C++11 feature. Do > you have some suggestions? Looked more into how you're using this, and you're not looking up a Pattern by its ID anywhere, so I don't think a map would be useful to you actually. What you have seems fine.
lgtm. You need and owner for histograms.xml. I didn't review the actual detection algorithm, since my impression is that it has been done in the original WebRTC CL. https://codereview.chromium.org/1357013006/diff/220001/content/renderer/media... File content/renderer/media/audio_repetition_detector.cc (right): https://codereview.chromium.org/1357013006/diff/220001/content/renderer/media... content/renderer/media/audio_repetition_detector.cc:54: DCHECK_GT(states_.size(), 0ul); Use state_.empty(). https://codereview.chromium.org/1357013006/diff/220001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1357013006/diff/220001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:17064: +<histogram name="Media.AudioCapturerRepetition" units="RepetitionPatternId"> What is RepetitionPatternId? You'll need to specify it as an enum= and define it below in the enum section. See other histograms.
https://codereview.chromium.org/1357013006/diff/140001/content/renderer/media... File content/renderer/media/audio_repetition_detector.cc (right): https://codereview.chromium.org/1357013006/diff/140001/content/renderer/media... content/renderer/media/audio_repetition_detector.cc:88: if (pattern.look_back_ms > max_look_back_ms_) { On 2015/09/28 20:18:54, minyue wrote: > On 2015/09/28 16:13:03, tommi wrote: > > nit: no {} > > I got a similar comment from Henrik too. I generally do not favor of dealing > with one line {} any specially, since one line is not stable and can become > multiple lines easily, e.g., adding some comments. When adding more lines braces are added. Until then, fewer braces save lines of code. > But since you both pointed this out. I am ok with it since you know Chromium > style better. I did a search of similar cases and tried to cover all of them. It saves time to just do these things the first time a reviewer requests it. (unless there's a good reason not to do it of course) https://codereview.chromium.org/1357013006/diff/220001/content/renderer/media... File content/renderer/media/audio_repetition_detector.cc (right): https://codereview.chromium.org/1357013006/diff/220001/content/renderer/media... content/renderer/media/audio_repetition_detector.cc:51: void AudioRepetitionDetector::Detect(const float* data, size_t num_frames, Has someone reviewed the implementation in this file? (hlundin?) https://codereview.chromium.org/1357013006/diff/220001/content/renderer/media... content/renderer/media/audio_repetition_detector.cc:62: data += max_frames_ * num_channels; nit: could calculate max_frames_ * num_channels outside the loop https://codereview.chromium.org/1357013006/diff/220001/content/renderer/media... content/renderer/media/audio_repetition_detector.cc:72: for (auto state : states_) { can you change this to: for (const State* state : states_) { https://codereview.chromium.org/1357013006/diff/220001/content/renderer/media... content/renderer/media/audio_repetition_detector.cc:100: void AudioRepetitionDetector::State::Increment(bool zero) { what about adding a thread checker to these methods? https://codereview.chromium.org/1357013006/diff/220001/content/renderer/media... File content/renderer/media/audio_repetition_detector.h (right): https://codereview.chromium.org/1357013006/diff/220001/content/renderer/media... content/renderer/media/audio_repetition_detector.h:47: class State { add DISALLOW_COPY_AND_ASSIGN to this class and any new classes you're introducing. It would be a shame to mistakenly have e.g. a loop create copies (which can easily be overlooked when using auto). https://codereview.chromium.org/1357013006/diff/220001/content/renderer/media... File content/renderer/media/audio_repetition_detector_unittest.cc (right): https://codereview.chromium.org/1357013006/diff/220001/content/renderer/media... content/renderer/media/audio_repetition_detector_unittest.cc:20: return counters_[id]; nit: this is the second time the function looks up the id. you might as well return what's already been looked up. https://codereview.chromium.org/1357013006/diff/220001/content/renderer/media... content/renderer/media/audio_repetition_detector_unittest.cc:41: counters_[id]++; same thing here. three lookups in the same function. you can just use |it| instead
On 2015/10/01 16:06:01, Henrik Grunell wrote: > lgtm. > > You need and owner for histograms.xml. Thanks. will add. > > I didn't review the actual detection algorithm, since my impression is that it > has been done in the original WebRTC CL. > > https://codereview.chromium.org/1357013006/diff/220001/content/renderer/media... > File content/renderer/media/audio_repetition_detector.cc (right): > > https://codereview.chromium.org/1357013006/diff/220001/content/renderer/media... > content/renderer/media/audio_repetition_detector.cc:54: > DCHECK_GT(states_.size(), 0ul); > Use state_.empty(). > > https://codereview.chromium.org/1357013006/diff/220001/tools/metrics/histogra... > File tools/metrics/histograms/histograms.xml (right): > > https://codereview.chromium.org/1357013006/diff/220001/tools/metrics/histogra... > tools/metrics/histograms/histograms.xml:17064: +<histogram > name="Media.AudioCapturerRepetition" units="RepetitionPatternId"> > What is RepetitionPatternId? You'll need to specify it as an enum= and define it > below in the enum section. See other histograms.
minyue@chromium.org changed reviewers: + rkaplow@chromium.org
minyue@chromium.org changed reviewers: - rkaplow@chromium.org
Tommi's new comments are addressed now. https://codereview.chromium.org/1357013006/diff/180001/content/renderer/media... File content/renderer/media/audio_repetition_detector.h (right): https://codereview.chromium.org/1357013006/diff/180001/content/renderer/media... content/renderer/media/audio_repetition_detector.h:30: struct Pattern { On 2015/10/01 05:13:49, ajm wrote: > On 2015/09/30 19:52:32, minyue wrote: > > On 2015/09/30 00:23:43, ajm wrote: > > > I didn't look closely at this, so feel free to ignore, but this looks like > it > > > would be natural to omit id here and store a collection of Patterns in a map > > > rather than a vector below. > > > > This is a good point. Yes, map is natural way to avoid id duplication, but > > Pattern is a simpler, and thus I can write > > const AudioRepetitionDetector::Pattern kRepetitionPatterns[] = { > > {1, 10, 10}, > > {2, 20, 10}, ... > > > > C++11's std:map initializer may make it as simple, but it is a C++11 feature. > Do > > you have some suggestions? > > Looked more into how you're using this, and you're not looking up a Pattern by > its ID anywhere, so I don't think a map would be useful to you actually. What > you have seems fine. Acknowledged. https://codereview.chromium.org/1357013006/diff/220001/content/renderer/media... File content/renderer/media/audio_repetition_detector.cc (right): https://codereview.chromium.org/1357013006/diff/220001/content/renderer/media... content/renderer/media/audio_repetition_detector.cc:51: void AudioRepetitionDetector::Detect(const float* data, size_t num_frames, On 2015/10/01 17:37:46, tommi wrote: > Has someone reviewed the implementation in this file? (hlundin?) Henrik Lundin, Per and Andrew looked at it. Henrik gave a LGTM before I moved the CL from WebRTC to here. https://codereview.chromium.org/1357013006/diff/220001/content/renderer/media... content/renderer/media/audio_repetition_detector.cc:54: DCHECK_GT(states_.size(), 0ul); On 2015/10/01 16:06:01, Henrik Grunell wrote: > Use state_.empty(). good point https://codereview.chromium.org/1357013006/diff/220001/content/renderer/media... content/renderer/media/audio_repetition_detector.cc:62: data += max_frames_ * num_channels; On 2015/10/01 17:37:47, tommi wrote: > nit: could calculate max_frames_ * num_channels outside the loop Done. https://codereview.chromium.org/1357013006/diff/220001/content/renderer/media... content/renderer/media/audio_repetition_detector.cc:72: for (auto state : states_) { On 2015/10/01 17:37:46, tommi wrote: > can you change this to: > for (const State* state : states_) { Done. cannot be const though https://codereview.chromium.org/1357013006/diff/220001/content/renderer/media... content/renderer/media/audio_repetition_detector.cc:100: void AudioRepetitionDetector::State::Increment(bool zero) { On 2015/10/01 17:37:46, tommi wrote: > what about adding a thread checker to these methods? My idea is that State is an inner private class, and it may be adequate to check thread at all outer class functions that operate on instances of State. https://codereview.chromium.org/1357013006/diff/220001/content/renderer/media... content/renderer/media/audio_repetition_detector.cc:147: for (auto state : states_) also changed this auto https://codereview.chromium.org/1357013006/diff/220001/content/renderer/media... File content/renderer/media/audio_repetition_detector.h (right): https://codereview.chromium.org/1357013006/diff/220001/content/renderer/media... content/renderer/media/audio_repetition_detector.h:47: class State { On 2015/10/01 17:37:47, tommi wrote: > add DISALLOW_COPY_AND_ASSIGN to this class and any new classes you're > introducing. It would be a shame to mistakenly have e.g. a loop create copies > (which can easily be overlooked when using auto). Indeed. I forgot to add DISALLOW_COPY_AND_ASSIGN to this inner class. https://codereview.chromium.org/1357013006/diff/220001/content/renderer/media... File content/renderer/media/audio_repetition_detector_unittest.cc (right): https://codereview.chromium.org/1357013006/diff/220001/content/renderer/media... content/renderer/media/audio_repetition_detector_unittest.cc:20: return counters_[id]; On 2015/10/01 17:37:47, tommi wrote: > nit: this is the second time the function looks up the id. you might as well > return what's already been looked up. good point https://codereview.chromium.org/1357013006/diff/220001/content/renderer/media... content/renderer/media/audio_repetition_detector_unittest.cc:41: counters_[id]++; On 2015/10/01 17:37:47, tommi wrote: > same thing here. three lookups in the same function. you can just use |it| > instead Done.
minyue@chromium.org changed reviewers: + rkaplow@chromium.org
Hi Robert, I added you to review the changes in histograms.xml here and give owner's approval. Thanks!
https://codereview.chromium.org/1357013006/diff/260001/content/renderer/media... File content/renderer/media/audio_repetition_detector.cc (right): https://codereview.chromium.org/1357013006/diff/260001/content/renderer/media... content/renderer/media/audio_repetition_detector.cc:197: "Media.AudioCapturerRepetition", id, ids_); what is the output space for the ids? Normally an enum should have each entry mapped to a specific output type.
https://codereview.chromium.org/1357013006/diff/260001/content/renderer/media... File content/renderer/media/audio_repetition_detector.cc (right): https://codereview.chromium.org/1357013006/diff/260001/content/renderer/media... content/renderer/media/audio_repetition_detector.cc:197: "Media.AudioCapturerRepetition", id, ids_); On 2015/10/02 14:42:33, rkaplow wrote: > what is the output space for the ids? Normally an enum should have each entry > mapped to a specific output type. Our idea is to use the ID (integer) of some pattern as the histogram bins. Different from enums, the IDs does not need alias. The current IDs of interest include {1,2,...10, 20}, 11 to 19 are omitted to make the ids more meaningful. It is likely that we add more IDs in the future. I find UMA_HISTOGRAM_CUSTOM_ENUMERATION, which takes a vector<int> as the value space, to be useful in this scenario. What do you suggest, Robert?
can you ping back when the bots are green? https://codereview.chromium.org/1357013006/diff/260001/content/renderer/media... File content/renderer/media/audio_repetition_detector.cc (right): https://codereview.chromium.org/1357013006/diff/260001/content/renderer/media... content/renderer/media/audio_repetition_detector.cc:49: DCHECK(thread_checker_.CalledOnValidThread()); looks like we're hitting this in the tests.
On 2015/10/02 19:27:52, minyue wrote: > https://codereview.chromium.org/1357013006/diff/260001/content/renderer/media... > File content/renderer/media/audio_repetition_detector.cc (right): > > https://codereview.chromium.org/1357013006/diff/260001/content/renderer/media... > content/renderer/media/audio_repetition_detector.cc:197: > "Media.AudioCapturerRepetition", id, ids_); > On 2015/10/02 14:42:33, rkaplow wrote: > > what is the output space for the ids? Normally an enum should have each entry > > mapped to a specific output type. > > Our idea is to use the ID (integer) of some pattern as the histogram bins. > Different from enums, the IDs does not need alias. The current IDs of interest > include {1,2,...10, 20}, 11 to 19 are omitted to make the ids more meaningful. > It is likely that we add more IDs in the future. > > I find UMA_HISTOGRAM_CUSTOM_ENUMERATION, which takes a vector<int> as the value > space, to be useful in this scenario. What do you suggest, Robert? Ok, since you're custom building your histogram entirely it seems ok. However I find the docs are lacking - someone visiting this metric from the dashboard won't get a sense for what '3' means here, for example. I think you should add more comments around kRepeatedPatterns. ANd then give more description to how the units are working in the xml description, and have it point back to the kRepeated part. Does that make sense?
On 2015/10/02 19:47:13, rkaplow wrote: > On 2015/10/02 19:27:52, minyue wrote: > > > https://codereview.chromium.org/1357013006/diff/260001/content/renderer/media... > > File content/renderer/media/audio_repetition_detector.cc (right): > > > > > https://codereview.chromium.org/1357013006/diff/260001/content/renderer/media... > > content/renderer/media/audio_repetition_detector.cc:197: > > "Media.AudioCapturerRepetition", id, ids_); > > On 2015/10/02 14:42:33, rkaplow wrote: > > > what is the output space for the ids? Normally an enum should have each > entry > > > mapped to a specific output type. > > > > Our idea is to use the ID (integer) of some pattern as the histogram bins. > > Different from enums, the IDs does not need alias. The current IDs of interest > > include {1,2,...10, 20}, 11 to 19 are omitted to make the ids more meaningful. > > It is likely that we add more IDs in the future. > > > > I find UMA_HISTOGRAM_CUSTOM_ENUMERATION, which takes a vector<int> as the > value > > space, to be useful in this scenario. What do you suggest, Robert? > > Ok, since you're custom building your histogram entirely it seems ok. > However I find the docs are lacking - someone visiting this metric from the > dashboard won't get a sense for what '3' means here, for example. > > I think you should add more comments around kRepeatedPatterns. ANd then give > more description to how the units are working in the xml description, and have > it point back to the kRepeated part. Does that make sense? It would be best if you could put in an Enum in the xml that gives a description for each individual value. That lets the tools parse the data and make the output much more understandable.
On 2015/10/02 19:48:23, rkaplow wrote: > On 2015/10/02 19:47:13, rkaplow wrote: > > On 2015/10/02 19:27:52, minyue wrote: > > > > > > https://codereview.chromium.org/1357013006/diff/260001/content/renderer/media... > > > File content/renderer/media/audio_repetition_detector.cc (right): > > > > > > > > > https://codereview.chromium.org/1357013006/diff/260001/content/renderer/media... > > > content/renderer/media/audio_repetition_detector.cc:197: > > > "Media.AudioCapturerRepetition", id, ids_); > > > On 2015/10/02 14:42:33, rkaplow wrote: > > > > what is the output space for the ids? Normally an enum should have each > > entry > > > > mapped to a specific output type. > > > > > > Our idea is to use the ID (integer) of some pattern as the histogram bins. > > > Different from enums, the IDs does not need alias. The current IDs of > interest > > > include {1,2,...10, 20}, 11 to 19 are omitted to make the ids more > meaningful. > > > It is likely that we add more IDs in the future. > > > > > > I find UMA_HISTOGRAM_CUSTOM_ENUMERATION, which takes a vector<int> as the > > value > > > space, to be useful in this scenario. What do you suggest, Robert? > > > > Ok, since you're custom building your histogram entirely it seems ok. > > However I find the docs are lacking - someone visiting this metric from the > > dashboard won't get a sense for what '3' means here, for example. > > > > I think you should add more comments around kRepeatedPatterns. ANd then give > > more description to how the units are working in the xml description, and have > > it point back to the kRepeated part. Does that make sense? > > It would be best if you could put in an Enum in the xml that gives a description > for each individual value. That lets the tools parse the data and make the > output much more understandable. OK. I see that people would like to understand the histogram by reading the xml only. I can add description of ID in the xml. Do you think it is necessary to add enum or is it simpler to add a description for each ID in the hist descriptor? And does adding an enum means that, every time we add a new ID, we need to make a change in the xml?
On 2015/10/02 20:15:41, minyue wrote: > On 2015/10/02 19:48:23, rkaplow wrote: > > On 2015/10/02 19:47:13, rkaplow wrote: > > > On 2015/10/02 19:27:52, minyue wrote: > > > > > > > > > > https://codereview.chromium.org/1357013006/diff/260001/content/renderer/media... > > > > File content/renderer/media/audio_repetition_detector.cc (right): > > > > > > > > > > > > > > https://codereview.chromium.org/1357013006/diff/260001/content/renderer/media... > > > > content/renderer/media/audio_repetition_detector.cc:197: > > > > "Media.AudioCapturerRepetition", id, ids_); > > > > On 2015/10/02 14:42:33, rkaplow wrote: > > > > > what is the output space for the ids? Normally an enum should have each > > > entry > > > > > mapped to a specific output type. > > > > > > > > Our idea is to use the ID (integer) of some pattern as the histogram bins. > > > > Different from enums, the IDs does not need alias. The current IDs of > > interest > > > > include {1,2,...10, 20}, 11 to 19 are omitted to make the ids more > > meaningful. > > > > It is likely that we add more IDs in the future. > > > > > > > > I find UMA_HISTOGRAM_CUSTOM_ENUMERATION, which takes a vector<int> as the > > > value > > > > space, to be useful in this scenario. What do you suggest, Robert? > > > > > > Ok, since you're custom building your histogram entirely it seems ok. > > > However I find the docs are lacking - someone visiting this metric from the > > > dashboard won't get a sense for what '3' means here, for example. > > > > > > I think you should add more comments around kRepeatedPatterns. ANd then give > > > more description to how the units are working in the xml description, and > have > > > it point back to the kRepeated part. Does that make sense? > > > > It would be best if you could put in an Enum in the xml that gives a > description > > for each individual value. That lets the tools parse the data and make the > > output much more understandable. > > OK. I see that people would like to understand the histogram by reading the xml > only. I can add description of ID in the xml. > > Do you think it is necessary to add enum or is it simpler to add a description > for each ID in the hist descriptor? > > And does adding an enum means that, every time we add a new ID, we need to make > a change in the xml? It is best to add an enum - that's what occurs 99% of the time. How often will new IDs be added?
On 2015/10/02 20:41:25, rkaplow wrote: > On 2015/10/02 20:15:41, minyue wrote: > > On 2015/10/02 19:48:23, rkaplow wrote: > > > On 2015/10/02 19:47:13, rkaplow wrote: > > > > On 2015/10/02 19:27:52, minyue wrote: > > > > > > > > > > > > > > > https://codereview.chromium.org/1357013006/diff/260001/content/renderer/media... > > > > > File content/renderer/media/audio_repetition_detector.cc (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1357013006/diff/260001/content/renderer/media... > > > > > content/renderer/media/audio_repetition_detector.cc:197: > > > > > "Media.AudioCapturerRepetition", id, ids_); > > > > > On 2015/10/02 14:42:33, rkaplow wrote: > > > > > > what is the output space for the ids? Normally an enum should have > each > > > > entry > > > > > > mapped to a specific output type. > > > > > > > > > > Our idea is to use the ID (integer) of some pattern as the histogram > bins. > > > > > Different from enums, the IDs does not need alias. The current IDs of > > > interest > > > > > include {1,2,...10, 20}, 11 to 19 are omitted to make the ids more > > > meaningful. > > > > > It is likely that we add more IDs in the future. > > > > > > > > > > I find UMA_HISTOGRAM_CUSTOM_ENUMERATION, which takes a vector<int> as > the > > > > value > > > > > space, to be useful in this scenario. What do you suggest, Robert? > > > > > > > > Ok, since you're custom building your histogram entirely it seems ok. > > > > However I find the docs are lacking - someone visiting this metric from > the > > > > dashboard won't get a sense for what '3' means here, for example. > > > > > > > > I think you should add more comments around kRepeatedPatterns. ANd then > give > > > > more description to how the units are working in the xml description, and > > have > > > > it point back to the kRepeated part. Does that make sense? > > > > > > It would be best if you could put in an Enum in the xml that gives a > > description > > > for each individual value. That lets the tools parse the data and make the > > > output much more understandable. > > > > OK. I see that people would like to understand the histogram by reading the > xml > > only. I can add description of ID in the xml. > > > > Do you think it is necessary to add enum or is it simpler to add a description > > for each ID in the hist descriptor? > > > > And does adding an enum means that, every time we add a new ID, we need to > make > > a change in the xml? > > It is best to add an enum - that's what occurs 99% of the time. How often will > new IDs be added? Although there is a possibility, I don't think adding IDs will really happen often.
> > > OK. I see that people would like to understand the histogram by reading the > > xml > > > only. I can add description of ID in the xml. > > > > > > Do you think it is necessary to add enum or is it simpler to add a > description > > > for each ID in the hist descriptor? > > > > > > And does adding an enum means that, every time we add a new ID, we need to > > make > > > a change in the xml? > > > > It is best to add an enum - that's what occurs 99% of the time. How often will > > new IDs be added? > > Although there is a possibility, I don't think adding IDs will really happen > often. Then go with the ordinary way - add an enum.
Patchset #15 (id:290001) has been deleted
Hi, With a consideration of making the histogram clearer, we had an offline discussion and decided to drop one descriptor of a repetition pattern: the duration. Now we only log the look back time in UMA when repetition is detected. Now a new patch is formed, tests are updated accordingly. Please take a look. https://codereview.chromium.org/1357013006/diff/260001/content/renderer/media... File content/renderer/media/audio_repetition_detector.cc (right): https://codereview.chromium.org/1357013006/diff/260001/content/renderer/media... content/renderer/media/audio_repetition_detector.cc:49: DCHECK(thread_checker_.CalledOnValidThread()); On 2015/10/02 19:45:31, tommi (Slow to respond) wrote: > looks like we're hitting this in the tests. Yes, a DetachFromThread() at ctor should also mean to remove this check.
https://codereview.chromium.org/1357013006/diff/310001/content/renderer/media... File content/renderer/media/audio_repetition_detector.cc (right): https://codereview.chromium.org/1357013006/diff/310001/content/renderer/media... content/renderer/media/audio_repetition_detector.cc:20: const int kMinLookbackTimeMS = 10; Nit: Ms https://codereview.chromium.org/1357013006/diff/310001/content/renderer/media... content/renderer/media/audio_repetition_detector.cc:33: min_length_ms_(kMinLengthMs), Can |min_length_ms_| be removed? https://codereview.chromium.org/1357013006/diff/310001/content/renderer/media... content/renderer/media/audio_repetition_detector.cc:37: max_frames_(kMaxFrames) { Can |max_frames_| be removed? https://codereview.chromium.org/1357013006/diff/310001/content/renderer/media... content/renderer/media/audio_repetition_detector.cc:39: time -= kLookbackTimeStepMS) Use {} (Since total for block is more than 2 lines.) Nit: Align "time -= ..." with "int" above. https://codereview.chromium.org/1357013006/diff/310001/content/renderer/media... File content/renderer/media/audio_repetition_detector.h (right): https://codereview.chromium.org/1357013006/diff/310001/content/renderer/media... content/renderer/media/audio_repetition_detector.h:21: // duration of any consecutive equality. Re the dtor check hit, maybe comment on the threading and lifetime here? https://codereview.chromium.org/1357013006/diff/310001/content/renderer/media... content/renderer/media/audio_repetition_detector.h:33: friend class AudioRepetitionDetectorForTest; // For testing. Nit: I think you can remove the comment, it's clear from the name it's for testing. https://codereview.chromium.org/1357013006/diff/310001/content/renderer/media... content/renderer/media/audio_repetition_detector.h:58: // counter of frames in a consecutive repetition. Nit: begin with capital letter. Here and below.
Hi Henrik, Thanks for the comments. The nits have been fixed locally. I will wait for more comments to upload another patch. There is one comment that I am not sure to understand fully, see inline. https://codereview.chromium.org/1357013006/diff/310001/content/renderer/media... File content/renderer/media/audio_repetition_detector.cc (right): https://codereview.chromium.org/1357013006/diff/310001/content/renderer/media... content/renderer/media/audio_repetition_detector.cc:20: const int kMinLookbackTimeMS = 10; On 2015/10/16 07:59:50, Henrik Grunell wrote: > Nit: Ms Oh, yes https://codereview.chromium.org/1357013006/diff/310001/content/renderer/media... content/renderer/media/audio_repetition_detector.cc:33: min_length_ms_(kMinLengthMs), On 2015/10/16 07:59:50, Henrik Grunell wrote: > Can |min_length_ms_| be removed? |min_length_ms_| is important in the algorithm, and it would be good to make it a member variable. Actually, there is a unittest on it, and it changes its value to verify that it functions. https://codereview.chromium.org/1357013006/diff/310001/content/renderer/media... content/renderer/media/audio_repetition_detector.cc:37: max_frames_(kMaxFrames) { On 2015/10/16 07:59:50, Henrik Grunell wrote: > Can |max_frames_| be removed? same as min_length_ms_ https://codereview.chromium.org/1357013006/diff/310001/content/renderer/media... content/renderer/media/audio_repetition_detector.cc:39: time -= kLookbackTimeStepMS) On 2015/10/16 07:59:50, Henrik Grunell wrote: > Use {} (Since total for block is more than 2 lines.) > > Nit: Align "time -= ..." with "int" above. Ok. But why alligning -=, I am not aware that line breaking rule. https://codereview.chromium.org/1357013006/diff/310001/content/renderer/media... File content/renderer/media/audio_repetition_detector.h (right): https://codereview.chromium.org/1357013006/diff/310001/content/renderer/media... content/renderer/media/audio_repetition_detector.h:21: // duration of any consecutive equality. On 2015/10/16 07:59:50, Henrik Grunell wrote: > Re the dtor check hit, maybe comment on the threading and lifetime here? Do you mean to write something like: An instance of this class can only be used in one thread, although the construction and deconstruction can be made in a different thread. https://codereview.chromium.org/1357013006/diff/310001/content/renderer/media... content/renderer/media/audio_repetition_detector.h:33: friend class AudioRepetitionDetectorForTest; // For testing. On 2015/10/16 07:59:50, Henrik Grunell wrote: > Nit: I think you can remove the comment, it's clear from the name it's for > testing. Acknowledged. https://codereview.chromium.org/1357013006/diff/310001/content/renderer/media... content/renderer/media/audio_repetition_detector.h:58: // counter of frames in a consecutive repetition. On 2015/10/16 07:59:50, Henrik Grunell wrote: > Nit: begin with capital letter. Here and below. Acknowledged.
https://codereview.chromium.org/1357013006/diff/310001/content/renderer/media... File content/renderer/media/audio_repetition_detector.cc (right): https://codereview.chromium.org/1357013006/diff/310001/content/renderer/media... content/renderer/media/audio_repetition_detector.cc:33: min_length_ms_(kMinLengthMs), On 2015/10/16 08:34:42, minyue wrote: > On 2015/10/16 07:59:50, Henrik Grunell wrote: > > Can |min_length_ms_| be removed? > > |min_length_ms_| is important in the algorithm, and it would be good to make it > a member variable. I meant to use the constant instead since it never changes. > Actually, there is a unittest on it, and it changes its value to verify that it > functions. Why does that have to be tested? It's not supposed to change in the code. https://codereview.chromium.org/1357013006/diff/310001/content/renderer/media... content/renderer/media/audio_repetition_detector.cc:39: time -= kLookbackTimeStepMS) On 2015/10/16 08:34:42, minyue wrote: > On 2015/10/16 07:59:50, Henrik Grunell wrote: > > Use {} (Since total for block is more than 2 lines.) > > > > Nit: Align "time -= ..." with "int" above. > > Ok. But why alligning -=, I am not aware that line breaking rule. To align the second line with the (. for (int time = kMaxLookbackTimeMS; time >= kMinLookbackTimeMS; time -= kLookbackTimeStepMS) https://codereview.chromium.org/1357013006/diff/310001/content/renderer/media... File content/renderer/media/audio_repetition_detector.h (right): https://codereview.chromium.org/1357013006/diff/310001/content/renderer/media... content/renderer/media/audio_repetition_detector.h:21: // duration of any consecutive equality. On 2015/10/16 08:34:42, minyue wrote: > On 2015/10/16 07:59:50, Henrik Grunell wrote: > > Re the dtor check hit, maybe comment on the threading and lifetime here? > > Do you mean to write something like: > > An instance of this class can only be used in one thread, although the > construction and deconstruction can be made in a different thread. Yes, something like that. And why destructing it on another thread than the calls are made on is safe.
Hi Henrik, Thanks for the further comments! Here are my considerations. https://codereview.chromium.org/1357013006/diff/310001/content/renderer/media... File content/renderer/media/audio_repetition_detector.cc (right): https://codereview.chromium.org/1357013006/diff/310001/content/renderer/media... content/renderer/media/audio_repetition_detector.cc:33: min_length_ms_(kMinLengthMs), On 2015/10/16 08:44:11, Henrik Grunell wrote: > On 2015/10/16 08:34:42, minyue wrote: > > On 2015/10/16 07:59:50, Henrik Grunell wrote: > > > Can |min_length_ms_| be removed? > > > > |min_length_ms_| is important in the algorithm, and it would be good to make > it > > a member variable. > > I meant to use the constant instead since it never changes. > > > Actually, there is a unittest on it, and it changes its value to verify that > it > > functions. > > Why does that have to be tested? It's not supposed to change in the code. Sure it is not really subjected to change in current implementation. But it should be robust to changes in |kMinLengthMs|. Since |kMinLengthMs| is quite arbitrarily set, it may change in future. Now back to the test. In the test, the sampling rate is set very low, 1 ms for 1 sample, so that the test can be very clean, with "kMinLengthMs = 1", it means any 1 sample replication will trigger a report. To make the test cover the case that shorter repetition won't trigger report, I modified |min_length_ms_|, which also protect future changes in |kMinLengthMs|. https://codereview.chromium.org/1357013006/diff/310001/content/renderer/media... content/renderer/media/audio_repetition_detector.cc:37: max_frames_(kMaxFrames) { On 2015/10/16 08:34:42, minyue wrote: > On 2015/10/16 07:59:50, Henrik Grunell wrote: > > Can |max_frames_| be removed? > > same as min_length_ms_ |kMaxFrames| is set to be the largest possible frame size, that means Detect() does not need to slice the input sequence to chunks in all cases. But we need to test cases that the code that performs the slicing works correctly. For that purpose, I modified |max_frames_| in one test. https://codereview.chromium.org/1357013006/diff/310001/content/renderer/media... content/renderer/media/audio_repetition_detector.cc:39: time -= kLookbackTimeStepMS) On 2015/10/16 08:44:11, Henrik Grunell wrote: > On 2015/10/16 08:34:42, minyue wrote: > > On 2015/10/16 07:59:50, Henrik Grunell wrote: > > > Use {} (Since total for block is more than 2 lines.) > > > > > > Nit: Align "time -= ..." with "int" above. > > > > Ok. But why alligning -=, I am not aware that line breaking rule. > > To align the second line with the (. > > for (int time = kMaxLookbackTimeMS; time >= kMinLookbackTimeMS; > time -= kLookbackTimeStepMS) ok. these ;-separated lines seems to me different from arguments to a function, and therefore, I usually apply a 4-space line-breaking rule. But I may be wrong. https://codereview.chromium.org/1357013006/diff/310001/content/renderer/media... File content/renderer/media/audio_repetition_detector.h (right): https://codereview.chromium.org/1357013006/diff/310001/content/renderer/media... content/renderer/media/audio_repetition_detector.h:21: // duration of any consecutive equality. On 2015/10/16 08:44:11, Henrik Grunell wrote: > On 2015/10/16 08:34:42, minyue wrote: > > On 2015/10/16 07:59:50, Henrik Grunell wrote: > > > Re the dtor check hit, maybe comment on the threading and lifetime here? > > > > Do you mean to write something like: > > > > An instance of this class can only be used in one thread, although the > > construction and deconstruction can be made in a different thread. > > Yes, something like that. And why destructing it on another thread than the > calls are made on is safe. The failure in an earlier patch that triggered me to change the threading checker seems to have the same reason as in https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/m...
https://codereview.chromium.org/1357013006/diff/310001/content/renderer/media... File content/renderer/media/audio_repetition_detector.cc (right): https://codereview.chromium.org/1357013006/diff/310001/content/renderer/media... content/renderer/media/audio_repetition_detector.cc:33: min_length_ms_(kMinLengthMs), On 2015/10/16 09:27:27, minyue wrote: > On 2015/10/16 08:44:11, Henrik Grunell wrote: > > On 2015/10/16 08:34:42, minyue wrote: > > > On 2015/10/16 07:59:50, Henrik Grunell wrote: > > > > Can |min_length_ms_| be removed? > > > > > > |min_length_ms_| is important in the algorithm, and it would be good to make > > it > > > a member variable. > > > > I meant to use the constant instead since it never changes. > > > > > Actually, there is a unittest on it, and it changes its value to verify that > > it > > > functions. > > > > Why does that have to be tested? It's not supposed to change in the code. > > Sure it is not really subjected to change in current implementation. But it > should be robust to changes in |kMinLengthMs|. Since |kMinLengthMs| is quite > arbitrarily set, it may change in future. > > Now back to the test. In the test, the sampling rate is set very low, 1 ms for 1 > sample, so that the test can be very clean, with "kMinLengthMs = 1", it means > any 1 sample replication will trigger a report. To make the test cover the case > that shorter repetition won't trigger report, I modified |min_length_ms_|, which > also protect future changes in |kMinLengthMs|. I'm not sure I understand why the test has a lower sampling rate. Is it because of the data chunk size will be large? It's better to not clutter the production code for test purposes. Let the test handle that. https://codereview.chromium.org/1357013006/diff/310001/content/renderer/media... content/renderer/media/audio_repetition_detector.cc:37: max_frames_(kMaxFrames) { On 2015/10/16 09:27:28, minyue wrote: > On 2015/10/16 08:34:42, minyue wrote: > > On 2015/10/16 07:59:50, Henrik Grunell wrote: > > > Can |max_frames_| be removed? > > > > same as min_length_ms_ > > |kMaxFrames| is set to be the largest possible frame size, that means Detect() > does not need to slice the input sequence to chunks in all cases. But we need to > test cases that the code that performs the slicing works correctly. For that > purpose, I modified |max_frames_| in one test. Can you explain why we need to test with some other value than the constant that's actually used? It's confusing, another design around this would probably make things clearer. https://codereview.chromium.org/1357013006/diff/310001/content/renderer/media... content/renderer/media/audio_repetition_detector.cc:39: time -= kLookbackTimeStepMS) On 2015/10/16 09:27:28, minyue wrote: > On 2015/10/16 08:44:11, Henrik Grunell wrote: > > On 2015/10/16 08:34:42, minyue wrote: > > > On 2015/10/16 07:59:50, Henrik Grunell wrote: > > > > Use {} (Since total for block is more than 2 lines.) > > > > > > > > Nit: Align "time -= ..." with "int" above. > > > > > > Ok. But why alligning -=, I am not aware that line breaking rule. > > > > To align the second line with the (. > > > > for (int time = kMaxLookbackTimeMS; time >= kMinLookbackTimeMS; > > time -= kLookbackTimeStepMS) > > ok. these ;-separated lines seems to me different from arguments to a function, > and therefore, I usually apply a 4-space line-breaking rule. But I may be wrong. Align as functions is common practice in Chromium. https://codereview.chromium.org/1357013006/diff/310001/content/renderer/media... File content/renderer/media/audio_repetition_detector.h (right): https://codereview.chromium.org/1357013006/diff/310001/content/renderer/media... content/renderer/media/audio_repetition_detector.h:21: // duration of any consecutive equality. On 2015/10/16 09:27:28, minyue wrote: > On 2015/10/16 08:44:11, Henrik Grunell wrote: > > On 2015/10/16 08:34:42, minyue wrote: > > > On 2015/10/16 07:59:50, Henrik Grunell wrote: > > > > Re the dtor check hit, maybe comment on the threading and lifetime here? > > > > > > Do you mean to write something like: > > > > > > An instance of this class can only be used in one thread, although the > > > construction and deconstruction can be made in a different thread. > > > > Yes, something like that. And why destructing it on another thread than the > > calls are made on is safe. > > The failure in an earlier patch that triggered me to change the threading > checker seems to have the same reason as in > https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/m... Yes, that's fine. I think it's good to document that the owner of this object will ensure no calls from the audio thread after destruction. (If that's the case.)
https://codereview.chromium.org/1357013006/diff/310001/content/renderer/media... File content/renderer/media/audio_repetition_detector.cc (right): https://codereview.chromium.org/1357013006/diff/310001/content/renderer/media... content/renderer/media/audio_repetition_detector.cc:39: time -= kLookbackTimeStepMS) On 2015/10/16 11:08:11, Henrik Grunell wrote: > Align as functions is common practice in Chromium. Just run git cl format and be done with it :)
lgtm histogram lgtm
Per offline discussion, Henrik and I decide to decouple the UMA reporting from the actual detection, so that the detection is more generic and can be tested in the generic way as it is. It is only a structural change. The algorithm and histogram are untouched. Please see patch set 16.
Great that you moved out the repetition patterns and UMA stats reporting. I saw that the MSAP class is already large, so I think the new class should be in its own file. https://codereview.chromium.org/1357013006/diff/330001/content/renderer/media... File content/renderer/media/audio_repetition_detector.cc (right): https://codereview.chromium.org/1357013006/diff/330001/content/renderer/media... content/renderer/media/audio_repetition_detector.cc:104: DCHECK(main_thread_checker_.CalledOnValidThread()); |states_| is accessed to different threads. https://codereview.chromium.org/1357013006/diff/330001/content/renderer/media... File content/renderer/media/media_stream_audio_processor.cc (right): https://codereview.chromium.org/1357013006/diff/330001/content/renderer/media... content/renderer/media/media_stream_audio_processor.cc:32: // Minimum duration of a repetition. Put these in the AudioRepetitionReporter class. https://codereview.chromium.org/1357013006/diff/330001/content/renderer/media... File content/renderer/media/media_stream_audio_processor.h (right): https://codereview.chromium.org/1357013006/diff/330001/content/renderer/media... content/renderer/media/media_stream_audio_processor.h:116: class AudioRepetitionReporter : public AudioRepetitionDetector { Comment on the class. https://codereview.chromium.org/1357013006/diff/330001/content/renderer/media... content/renderer/media/media_stream_audio_processor.h:116: class AudioRepetitionReporter : public AudioRepetitionDetector { Let this class own a detector instead of inheriting. Also, I saw that the MSAP class is already very large. Put this class in its own file. I suggest to name it WebRtcAudioRepetitionDetector.
Hi, Per offline discussion with Tommi, it might be better to use base::Callback in stead of leaving a pure virtual function to allow caller implementation of actions upon repetition being detected. Please see the new patch. https://codereview.chromium.org/1357013006/diff/310001/content/renderer/media... File content/renderer/media/audio_repetition_detector.cc (right): https://codereview.chromium.org/1357013006/diff/310001/content/renderer/media... content/renderer/media/audio_repetition_detector.cc:33: min_length_ms_(kMinLengthMs), On 2015/10/16 11:08:11, Henrik Grunell wrote: > On 2015/10/16 09:27:27, minyue wrote: > > On 2015/10/16 08:44:11, Henrik Grunell wrote: > > > On 2015/10/16 08:34:42, minyue wrote: > > > > On 2015/10/16 07:59:50, Henrik Grunell wrote: > > > > > Can |min_length_ms_| be removed? > > > > > > > > |min_length_ms_| is important in the algorithm, and it would be good to > make > > > it > > > > a member variable. > > > > > > I meant to use the constant instead since it never changes. > > > > > > > Actually, there is a unittest on it, and it changes its value to verify > that > > > it > > > > functions. > > > > > > Why does that have to be tested? It's not supposed to change in the code. > > > > Sure it is not really subjected to change in current implementation. But it > > should be robust to changes in |kMinLengthMs|. Since |kMinLengthMs| is quite > > arbitrarily set, it may change in future. > > > > Now back to the test. In the test, the sampling rate is set very low, 1 ms for > 1 > > sample, so that the test can be very clean, with "kMinLengthMs = 1", it means > > any 1 sample replication will trigger a report. To make the test cover the > case > > that shorter repetition won't trigger report, I modified |min_length_ms_|, > which > > also protect future changes in |kMinLengthMs|. > > I'm not sure I understand why the test has a lower sampling rate. Is it because > of the data chunk size will be large? It's better to not clutter the production > code for test purposes. Let the test handle that. Already covered with offline discussion. Adding a memo here With separation of the detector and the UMA reporting mechanism, the detector does not have min_length_ms_ and max_frames_ any more. https://codereview.chromium.org/1357013006/diff/330001/content/renderer/media... File content/renderer/media/audio_repetition_detector.cc (right): https://codereview.chromium.org/1357013006/diff/330001/content/renderer/media... content/renderer/media/audio_repetition_detector.cc:104: DCHECK(main_thread_checker_.CalledOnValidThread()); On 2015/10/20 08:50:16, Henrik Grunell wrote: > |states_| is accessed to different threads. That is true. But I have no better idea than limiting main_thread to call ctor and dtor only. Do you have better suggestion? https://codereview.chromium.org/1357013006/diff/330001/content/renderer/media... File content/renderer/media/media_stream_audio_processor.cc (right): https://codereview.chromium.org/1357013006/diff/330001/content/renderer/media... content/renderer/media/media_stream_audio_processor.cc:32: // Minimum duration of a repetition. On 2015/10/20 08:50:16, Henrik Grunell wrote: > Put these in the AudioRepetitionReporter class. The class is not present any longer. We may put these to MediaStreamAudioProcessor, but that will make the class messier. https://codereview.chromium.org/1357013006/diff/330001/content/renderer/media... File content/renderer/media/media_stream_audio_processor.h (right): https://codereview.chromium.org/1357013006/diff/330001/content/renderer/media... content/renderer/media/media_stream_audio_processor.h:116: class AudioRepetitionReporter : public AudioRepetitionDetector { On 2015/10/20 08:50:16, Henrik Grunell wrote: > Comment on the class. Per offline discussion with Tommi, we use base::Callback to let user implement ReportRepetition. With this, your concern about inheritance also goes away.
https://codereview.chromium.org/1357013006/diff/330001/content/renderer/media... File content/renderer/media/audio_repetition_detector.cc (right): https://codereview.chromium.org/1357013006/diff/330001/content/renderer/media... content/renderer/media/audio_repetition_detector.cc:104: DCHECK(main_thread_checker_.CalledOnValidThread()); On 2015/10/23 12:05:23, minyue wrote: > On 2015/10/20 08:50:16, Henrik Grunell wrote: > > |states_| is accessed to different threads. > > That is true. But I have no better idea than limiting main_thread to call ctor > and dtor only. Do you have better suggestion? You have a few options: call this function on the processing thread, inject the lookback times in the ctor, use a lock. https://codereview.chromium.org/1357013006/diff/350001/content/renderer/media... File content/renderer/media/audio_repetition_detector.h (right): https://codereview.chromium.org/1357013006/diff/350001/content/renderer/media... content/renderer/media/audio_repetition_detector.h:39: const base::Callback<void(int)>& on_detection); Perhaps name it "repetition_callback"? Clearer that it's a callback. Same for member variable. https://codereview.chromium.org/1357013006/diff/350001/content/renderer/media... content/renderer/media/audio_repetition_detector.h:39: const base::Callback<void(int)>& on_detection); Typedef the callback to avoid duplication and put the description of the callback and its variable in the typedef comment. https://codereview.chromium.org/1357013006/diff/350001/content/renderer/media... File content/renderer/media/audio_repetition_detector_unittest.cc (right): https://codereview.chromium.org/1357013006/diff/350001/content/renderer/media... content/renderer/media/audio_repetition_detector_unittest.cc:43: void ReportRepetition(int look_back_ms) { "RepetitionDetected" is a clearer name. https://codereview.chromium.org/1357013006/diff/350001/content/renderer/media... content/renderer/media/audio_repetition_detector_unittest.cc:117: // To make the test signal most obvious, we choose a special sample rate. This is duplicated a lot, make it global. Is there anything else common that can be broken out? https://codereview.chromium.org/1357013006/diff/350001/content/renderer/media... File content/renderer/media/media_stream_audio_processor.cc (right): https://codereview.chromium.org/1357013006/diff/350001/content/renderer/media... content/renderer/media/media_stream_audio_processor.cc:288: new AudioRepetitionDetector(kMinLengthMs, kMaxFrames, &look_back_times[0], Instead of a ptr and size, let it take a const std::vector<int>&.
Hi, Thanks for the new feedback. Some refinement according to the comments are made now. https://codereview.chromium.org/1357013006/diff/330001/content/renderer/media... File content/renderer/media/audio_repetition_detector.cc (right): https://codereview.chromium.org/1357013006/diff/330001/content/renderer/media... content/renderer/media/audio_repetition_detector.cc:104: DCHECK(main_thread_checker_.CalledOnValidThread()); On 2015/10/23 13:49:02, Henrik Grunell wrote: > On 2015/10/23 12:05:23, minyue wrote: > > On 2015/10/20 08:50:16, Henrik Grunell wrote: > > > |states_| is accessed to different threads. > > > > That is true. But I have no better idea than limiting main_thread to call ctor > > and dtor only. Do you have better suggestion? > > You have a few options: call this function on the processing thread, inject the > lookback times in the ctor, use a lock. This is already a private member that is called from ctor, so it is actually "inject the lookback times in the ctor". Do you mean we even need to merge this helper function into ctor? https://codereview.chromium.org/1357013006/diff/350001/content/renderer/media... File content/renderer/media/audio_repetition_detector.h (right): https://codereview.chromium.org/1357013006/diff/350001/content/renderer/media... content/renderer/media/audio_repetition_detector.h:39: const base::Callback<void(int)>& on_detection); On 2015/10/23 13:49:02, Henrik Grunell wrote: > Typedef the callback to avoid duplication and put the description of the > callback and its variable in the typedef comment. Done. https://codereview.chromium.org/1357013006/diff/350001/content/renderer/media... content/renderer/media/audio_repetition_detector.h:39: const base::Callback<void(int)>& on_detection); On 2015/10/23 13:49:02, Henrik Grunell wrote: > Typedef the callback to avoid duplication and put the description of the > callback and its variable in the typedef comment. Done. https://codereview.chromium.org/1357013006/diff/350001/content/renderer/media... File content/renderer/media/audio_repetition_detector_unittest.cc (right): https://codereview.chromium.org/1357013006/diff/350001/content/renderer/media... content/renderer/media/audio_repetition_detector_unittest.cc:43: void ReportRepetition(int look_back_ms) { On 2015/10/23 13:49:02, Henrik Grunell wrote: > "RepetitionDetected" is a clearer name. Yes, but may be OnRepetitionDetected https://codereview.chromium.org/1357013006/diff/350001/content/renderer/media... content/renderer/media/audio_repetition_detector_unittest.cc:117: // To make the test signal most obvious, we choose a special sample rate. On 2015/10/23 13:49:02, Henrik Grunell wrote: > This is duplicated a lot, make it global. Is there anything else common that can > be broken out? I broke this out and did not find more https://codereview.chromium.org/1357013006/diff/350001/content/renderer/media... File content/renderer/media/media_stream_audio_processor.cc (right): https://codereview.chromium.org/1357013006/diff/350001/content/renderer/media... content/renderer/media/media_stream_audio_processor.cc:288: new AudioRepetitionDetector(kMinLengthMs, kMaxFrames, &look_back_times[0], On 2015/10/23 13:49:02, Henrik Grunell wrote: > Instead of a ptr and size, let it take a const std::vector<int>&. good,changed the signature
https://codereview.chromium.org/1357013006/diff/330001/content/renderer/media... File content/renderer/media/audio_repetition_detector.cc (right): https://codereview.chromium.org/1357013006/diff/330001/content/renderer/media... content/renderer/media/audio_repetition_detector.cc:104: DCHECK(main_thread_checker_.CalledOnValidThread()); On 2015/10/26 10:41:46, minyue wrote: > On 2015/10/23 13:49:02, Henrik Grunell wrote: > > On 2015/10/23 12:05:23, minyue wrote: > > > On 2015/10/20 08:50:16, Henrik Grunell wrote: > > > > |states_| is accessed to different threads. > > > > > > That is true. But I have no better idea than limiting main_thread to call > ctor > > > and dtor only. Do you have better suggestion? > > > > You have a few options: call this function on the processing thread, inject > the > > lookback times in the ctor, use a lock. > > This is already a private member that is called from ctor, so it is actually > "inject the lookback times in the ctor". Do you mean we even need to merge this > helper function into ctor? Ah, I see. Then it's fine. Actually, since the ctor only does this and this function may only be called from the ctor, move it to the ctor. https://codereview.chromium.org/1357013006/diff/350001/content/renderer/media... File content/renderer/media/audio_repetition_detector_unittest.cc (right): https://codereview.chromium.org/1357013006/diff/350001/content/renderer/media... content/renderer/media/audio_repetition_detector_unittest.cc:43: void ReportRepetition(int look_back_ms) { On 2015/10/26 10:41:46, minyue wrote: > On 2015/10/23 13:49:02, Henrik Grunell wrote: > > "RepetitionDetected" is a clearer name. > > Yes, but may be OnRepetitionDetected Acknowledged. https://codereview.chromium.org/1357013006/diff/350001/content/renderer/media... content/renderer/media/audio_repetition_detector_unittest.cc:117: // To make the test signal most obvious, we choose a special sample rate. On 2015/10/26 10:41:46, minyue wrote: > On 2015/10/23 13:49:02, Henrik Grunell wrote: > > This is duplicated a lot, make it global. Is there anything else common that > can > > be broken out? > > I broke this out and did not find more Acknowledged. https://codereview.chromium.org/1357013006/diff/370001/content/renderer/media... File content/renderer/media/audio_repetition_detector.h (right): https://codereview.chromium.org/1357013006/diff/370001/content/renderer/media... content/renderer/media/audio_repetition_detector.h:37: // |look_back_times| is a vector of look back times (in millisecond) for the Nit: milliseconds
Thanks for the review. Here are some small refinements https://codereview.chromium.org/1357013006/diff/370001/content/renderer/media... File content/renderer/media/audio_repetition_detector.h (right): https://codereview.chromium.org/1357013006/diff/370001/content/renderer/media... content/renderer/media/audio_repetition_detector.h:37: // |look_back_times| is a vector of look back times (in millisecond) for the On 2015/10/28 14:09:23, Henrik Grunell wrote: > Nit: milliseconds Done.
https://codereview.chromium.org/1357013006/diff/390001/content/renderer/media... File content/renderer/media/audio_repetition_detector.cc (right): https://codereview.chromium.org/1357013006/diff/390001/content/renderer/media... content/renderer/media/audio_repetition_detector.cc:26: // Build |states_| for the all look back times. Automatically avoid Is this part only for sorting and avoiding duplicates? Could you use a set? (Could take a set in the ctor in that case.) Otherwise, perhaps std::sort and std::unique?
https://codereview.chromium.org/1357013006/diff/390001/content/renderer/media... File content/renderer/media/audio_repetition_detector.cc (right): https://codereview.chromium.org/1357013006/diff/390001/content/renderer/media... content/renderer/media/audio_repetition_detector.cc:26: // Build |states_| for the all look back times. Automatically avoid On 2015/10/28 17:52:01, Henrik Grunell wrote: > Is this part only for sorting and avoiding duplicates? Could you use a set? > (Could take a set in the ctor in that case.) Otherwise, perhaps std::sort and > std::unique? I actually tried to use set at the first place. It is not handy since expect for C++11, map<int, Object> needs a copy ctor of Object when you do insert(), map<int, Object*> can solve it but the lifetime of an Object needs to be taken care of carefully. Then I used this not-so-efficient way of avoiding duplication, given that there won't be many look back values. But thank you for mentioning std:unique. I will try to utilize it here.
switched to std::sort and unique now
https://codereview.chromium.org/1357013006/diff/390001/content/renderer/media... File content/renderer/media/audio_repetition_detector.cc (right): https://codereview.chromium.org/1357013006/diff/390001/content/renderer/media... content/renderer/media/audio_repetition_detector.cc:26: // Build |states_| for the all look back times. Automatically avoid On 2015/10/28 19:45:29, minyue wrote: > On 2015/10/28 17:52:01, Henrik Grunell wrote: > > Is this part only for sorting and avoiding duplicates? Could you use a set? > > (Could take a set in the ctor in that case.) Otherwise, perhaps std::sort and > > std::unique? > > I actually tried to use set at the first place. It is not handy since expect for > C++11, map<int, Object> needs a copy ctor of Object when you do insert(), > map<int, Object*> can solve it but the lifetime of an Object needs to be taken > care of carefully. Then I used this not-so-efficient way of avoiding > duplication, given that there won't be many look back values. > > But thank you for mentioning std:unique. I will try to utilize it here. I mean std::set<int>& look_back_times as argument. Then order and uniqueness is guaranteed already.
On 2015/10/30 10:40:16, Henrik Grunell wrote: > https://codereview.chromium.org/1357013006/diff/390001/content/renderer/media... > File content/renderer/media/audio_repetition_detector.cc (right): > > https://codereview.chromium.org/1357013006/diff/390001/content/renderer/media... > content/renderer/media/audio_repetition_detector.cc:26: // Build |states_| for > the all look back times. Automatically avoid > On 2015/10/28 19:45:29, minyue wrote: > > On 2015/10/28 17:52:01, Henrik Grunell wrote: > > > Is this part only for sorting and avoiding duplicates? Could you use a set? > > > (Could take a set in the ctor in that case.) Otherwise, perhaps std::sort > and > > > std::unique? > > > > I actually tried to use set at the first place. It is not handy since expect > for > > C++11, map<int, Object> needs a copy ctor of Object when you do insert(), > > map<int, Object*> can solve it but the lifetime of an Object needs to be taken > > care of carefully. Then I used this not-so-efficient way of avoiding > > duplication, given that there won't be many look back values. > > > > But thank you for mentioning std:unique. I will try to utilize it here. > > I mean std::set<int>& look_back_times as argument. Then order and uniqueness is > guaranteed already. I see that you talked about set, sorry. I spent some time struggling with <map> and it would be really nice if |states_| it defined as "map" frame a look back time. So it just came back to me when I saw your comment. Regarding set vs unique, sort + unique is faster than set according to some report.
On 2015/10/30 10:49:05, minyue wrote: > On 2015/10/30 10:40:16, Henrik Grunell wrote: > > > https://codereview.chromium.org/1357013006/diff/390001/content/renderer/media... > > File content/renderer/media/audio_repetition_detector.cc (right): > > > > > https://codereview.chromium.org/1357013006/diff/390001/content/renderer/media... > > content/renderer/media/audio_repetition_detector.cc:26: // Build |states_| for > > the all look back times. Automatically avoid > > On 2015/10/28 19:45:29, minyue wrote: > > > On 2015/10/28 17:52:01, Henrik Grunell wrote: > > > > Is this part only for sorting and avoiding duplicates? Could you use a > set? > > > > (Could take a set in the ctor in that case.) Otherwise, perhaps std::sort > > and > > > > std::unique? > > > > > > I actually tried to use set at the first place. It is not handy since expect > > for > > > C++11, map<int, Object> needs a copy ctor of Object when you do insert(), > > > map<int, Object*> can solve it but the lifetime of an Object needs to be > taken > > > care of carefully. Then I used this not-so-efficient way of avoiding > > > duplication, given that there won't be many look back values. > > > > > > But thank you for mentioning std:unique. I will try to utilize it here. > > > > I mean std::set<int>& look_back_times as argument. Then order and uniqueness > is > > guaranteed already. > > I see that you talked about set, sorry. I spent some time struggling with <map> > and it would be really nice if |states_| it defined as "map" frame a look back > time. So it just came back to me when I saw your comment. > > Regarding set vs unique, sort + unique is faster than set according to some > report. OK. I'm fine with that if it's faster. Final nit is that you could use std::resize instead of erase after unique, up to you. lgtm. Note that Tommi needs to review as owner.
just a couple of minor questions and it looks like you might be about to upload one more patch. https://codereview.chromium.org/1357013006/diff/410001/content/renderer/media... File content/renderer/media/audio_repetition_detector.cc (right): https://codereview.chromium.org/1357013006/diff/410001/content/renderer/media... content/renderer/media/audio_repetition_detector.cc:26: // Avoid duplications in |look_back_times| if any. Since we always create a copy here, would it make sense to expect look_back_times to be sorted already? (and have a DCHECK for that) https://codereview.chromium.org/1357013006/diff/410001/content/renderer/media... content/renderer/media/audio_repetition_detector.cc:29: temp.erase(std::unique(temp.begin(), temp.end()), temp.end()); I guess this is what Henrik commented on (will you address?) https://codereview.chromium.org/1357013006/diff/410001/content/renderer/media... content/renderer/media/audio_repetition_detector.cc:32: for (auto look_back : temp) { instead of auto, just use int here. The way auto is being used raises "by value" questions. Besides, 'int' is shorter than 'auto' ;) https://codereview.chromium.org/1357013006/diff/410001/content/renderer/media... content/renderer/media/audio_repetition_detector.cc:32: for (auto look_back : temp) { no {} https://codereview.chromium.org/1357013006/diff/410001/content/renderer/media... content/renderer/media/audio_repetition_detector.cc:93: if (0 == count_frames_ && zero) { nit: convention is how you would read it: |count_frames_ == 0| https://codereview.chromium.org/1357013006/diff/410001/content/renderer/media... content/renderer/media/audio_repetition_detector.cc:101: if (!zero) it feels a bit strange to check this flag twice in the same function. what about: ----- if (zero) { if (count_frames_ == 0) { // <comment> all_zero_ = true; } } else { all_zero_ = false; } ++count_frames_; ----- Btw, what is the expected value of all_zero_ at the end of this function if zero is true but count_frames_ != 0? (can it be whatever?) https://codereview.chromium.org/1357013006/diff/410001/content/renderer/media... File content/renderer/media/audio_repetition_detector_unittest.cc (right): https://codereview.chromium.org/1357013006/diff/410001/content/renderer/media... content/renderer/media/audio_repetition_detector_unittest.cc:37: int GetCount(int look_back_ms) { nit: const? https://codereview.chromium.org/1357013006/diff/410001/content/renderer/media... content/renderer/media/audio_repetition_detector_unittest.cc:39: if (it == counters_.end()) nit: could shorten to return it == counters_.end() ? 0 : it->second; https://codereview.chromium.org/1357013006/diff/410001/content/renderer/media... content/renderer/media/audio_repetition_detector_unittest.cc:91: const size_t kNumChannels = 2; nit: static const size_t kNumChannels = 2;
Patchset #21 (id:430001) has been deleted
Hi Tommi, Thank you for the comments. And they are addressed now. https://codereview.chromium.org/1357013006/diff/410001/content/renderer/media... File content/renderer/media/audio_repetition_detector.cc (right): https://codereview.chromium.org/1357013006/diff/410001/content/renderer/media... content/renderer/media/audio_repetition_detector.cc:26: // Avoid duplications in |look_back_times| if any. On 2015/10/30 15:33:01, tommi wrote: > Since we always create a copy here, would it make sense to expect > look_back_times to be sorted already? > (and have a DCHECK for that) being sorted or not is not essential (except for the fact that we need to find out |max_look_back_ms_|), the goal is to avoid duplication. The reason or std::sort is because std::unique needs to go with std::sort to avoid duplication. And unique + sort is known as a most efficient way to remove duplication. We may consider DCHECK of duplication. But I am not aware of a light-weighted check for duplication. Thus DCHECK of duplication may be of the same complexity as just removing the duplication when there is (removing it is a almost free lunch) https://codereview.chromium.org/1357013006/diff/410001/content/renderer/media... content/renderer/media/audio_repetition_detector.cc:29: temp.erase(std::unique(temp.begin(), temp.end()), temp.end()); On 2015/10/30 15:33:01, tommi wrote: > I guess this is what Henrik commented on (will you address?) You mean the nit Henrik pointed out: i.e., erase vs resize. I tried to search about a comparison of the two, I don't find any. I am not aware of any benefit. https://codereview.chromium.org/1357013006/diff/410001/content/renderer/media... content/renderer/media/audio_repetition_detector.cc:32: for (auto look_back : temp) { On 2015/10/30 15:33:01, tommi wrote: > no {} Done. https://codereview.chromium.org/1357013006/diff/410001/content/renderer/media... content/renderer/media/audio_repetition_detector.cc:32: for (auto look_back : temp) { On 2015/10/30 15:33:01, tommi wrote: > instead of auto, just use int here. The way auto is being used raises "by > value" questions. Besides, 'int' is shorter than 'auto' ;) Done. https://codereview.chromium.org/1357013006/diff/410001/content/renderer/media... content/renderer/media/audio_repetition_detector.cc:93: if (0 == count_frames_ && zero) { On 2015/10/30 15:33:01, tommi wrote: > nit: convention is how you would read it: |count_frames_ == 0| Done. https://codereview.chromium.org/1357013006/diff/410001/content/renderer/media... content/renderer/media/audio_repetition_detector.cc:101: if (!zero) On 2015/10/30 15:33:01, tommi wrote: > it feels a bit strange to check this flag twice in the same function. what > about: > > ----- > if (zero) { > if (count_frames_ == 0) { > // <comment> > all_zero_ = true; > } > } else { > all_zero_ = false; > } > > ++count_frames_; > ----- > > Btw, what is the expected value of all_zero_ at the end of this function if zero > is true but count_frames_ != 0? (can it be whatever?) thanks, it reads much better. if zero is true but count_frames_ != 0, that means there are some 0s at the beginning of a repetition, e.g. 0 0 0 1 2 3 | 0 0 0 1 2 3 This is still a valid 6-long repetition. Therefore at the beginning 0s, we allow count_frames_ to grow but all_zero_ will true until we meet "1" https://codereview.chromium.org/1357013006/diff/410001/content/renderer/media... File content/renderer/media/audio_repetition_detector_unittest.cc (right): https://codereview.chromium.org/1357013006/diff/410001/content/renderer/media... content/renderer/media/audio_repetition_detector_unittest.cc:37: int GetCount(int look_back_ms) { On 2015/10/30 15:33:01, tommi wrote: > nit: const? true! https://codereview.chromium.org/1357013006/diff/410001/content/renderer/media... content/renderer/media/audio_repetition_detector_unittest.cc:39: if (it == counters_.end()) On 2015/10/30 15:33:01, tommi wrote: > nit: could shorten to > return it == counters_.end() ? 0 : it->second; yes of course https://codereview.chromium.org/1357013006/diff/410001/content/renderer/media... content/renderer/media/audio_repetition_detector_unittest.cc:91: const size_t kNumChannels = 2; On 2015/10/30 15:33:01, tommi wrote: > nit: static const size_t kNumChannels = 2; Ok. I did not really use static on const in these situations. Now I know.
https://codereview.chromium.org/1357013006/diff/410001/content/renderer/media... File content/renderer/media/audio_repetition_detector.cc (right): https://codereview.chromium.org/1357013006/diff/410001/content/renderer/media... content/renderer/media/audio_repetition_detector.cc:26: // Avoid duplications in |look_back_times| if any. On 2015/10/30 20:17:05, minyue wrote: > On 2015/10/30 15:33:01, tommi wrote: > > Since we always create a copy here, would it make sense to expect > > look_back_times to be sorted already? > > (and have a DCHECK for that) > > being sorted or not is not essential (except for the fact that we need to find > out |max_look_back_ms_|), the goal is to avoid duplication. The reason or > std::sort is because std::unique needs to go with std::sort to avoid > duplication. And unique + sort is known as a most efficient way to remove > duplication. > > We may consider DCHECK of duplication. But I am not aware of a light-weighted > check for duplication. Thus DCHECK of duplication may be of the same complexity > as just removing the duplication when there is (removing it is a almost free > lunch) I'm in favor of DCHECK-ing that it's sorted and no duplicates. You should be able to check the current value is larger than the previous.
On 2015/11/02 09:12:11, Henrik Grunell wrote: > https://codereview.chromium.org/1357013006/diff/410001/content/renderer/media... > File content/renderer/media/audio_repetition_detector.cc (right): > > https://codereview.chromium.org/1357013006/diff/410001/content/renderer/media... > content/renderer/media/audio_repetition_detector.cc:26: // Avoid duplications in > |look_back_times| if any. > On 2015/10/30 20:17:05, minyue wrote: > > On 2015/10/30 15:33:01, tommi wrote: > > > Since we always create a copy here, would it make sense to expect > > > look_back_times to be sorted already? > > > (and have a DCHECK for that) > > > > being sorted or not is not essential (except for the fact that we need to find > > out |max_look_back_ms_|), the goal is to avoid duplication. The reason or > > std::sort is because std::unique needs to go with std::sort to avoid > > duplication. And unique + sort is known as a most efficient way to remove > > duplication. > > > > We may consider DCHECK of duplication. But I am not aware of a light-weighted > > check for duplication. Thus DCHECK of duplication may be of the same > complexity > > as just removing the duplication when there is (removing it is a almost free > > lunch) > > I'm in favor of DCHECK-ing that it's sorted and no duplicates. You should be > able to check the current value is larger than the previous. But my argument is that requiring the list to be sorted is a bit too demanding. Why would we ask user to sort the list, if the elements in the list are logically independent?
On 2015/11/02 09:24:10, minyue wrote: > On 2015/11/02 09:12:11, Henrik Grunell wrote: > > > https://codereview.chromium.org/1357013006/diff/410001/content/renderer/media... > > File content/renderer/media/audio_repetition_detector.cc (right): > > > > > https://codereview.chromium.org/1357013006/diff/410001/content/renderer/media... > > content/renderer/media/audio_repetition_detector.cc:26: // Avoid duplications > in > > |look_back_times| if any. > > On 2015/10/30 20:17:05, minyue wrote: > > > On 2015/10/30 15:33:01, tommi wrote: > > > > Since we always create a copy here, would it make sense to expect > > > > look_back_times to be sorted already? > > > > (and have a DCHECK for that) > > > > > > being sorted or not is not essential (except for the fact that we need to > find > > > out |max_look_back_ms_|), the goal is to avoid duplication. The reason or > > > std::sort is because std::unique needs to go with std::sort to avoid > > > duplication. And unique + sort is known as a most efficient way to remove > > > duplication. > > > > > > We may consider DCHECK of duplication. But I am not aware of a > light-weighted > > > check for duplication. Thus DCHECK of duplication may be of the same > > complexity > > > as just removing the duplication when there is (removing it is a almost free > > > lunch) > > > > I'm in favor of DCHECK-ing that it's sorted and no duplicates. You should be > > able to check the current value is larger than the previous. > > But my argument is that requiring the list to be sorted is a bit too demanding. > Why would we ask user to sort the list, if the elements in the list are > logically independent? If it's too much hassle to require that and that the number of elements is small, then it's fine. If the number of elements is big, then you could e.g. look into passing a non-const pointer to the vector to the ctor and mutate it directly. But again, if it's small and this is infrequent, then no biggie.
On 2015/11/02 10:00:55, tommi wrote: > On 2015/11/02 09:24:10, minyue wrote: > > On 2015/11/02 09:12:11, Henrik Grunell wrote: > > > > > > https://codereview.chromium.org/1357013006/diff/410001/content/renderer/media... > > > File content/renderer/media/audio_repetition_detector.cc (right): > > > > > > > > > https://codereview.chromium.org/1357013006/diff/410001/content/renderer/media... > > > content/renderer/media/audio_repetition_detector.cc:26: // Avoid > duplications > > in > > > |look_back_times| if any. > > > On 2015/10/30 20:17:05, minyue wrote: > > > > On 2015/10/30 15:33:01, tommi wrote: > > > > > Since we always create a copy here, would it make sense to expect > > > > > look_back_times to be sorted already? > > > > > (and have a DCHECK for that) > > > > > > > > being sorted or not is not essential (except for the fact that we need to > > find > > > > out |max_look_back_ms_|), the goal is to avoid duplication. The reason or > > > > std::sort is because std::unique needs to go with std::sort to avoid > > > > duplication. And unique + sort is known as a most efficient way to remove > > > > duplication. > > > > > > > > We may consider DCHECK of duplication. But I am not aware of a > > light-weighted > > > > check for duplication. Thus DCHECK of duplication may be of the same > > > complexity > > > > as just removing the duplication when there is (removing it is a almost > free > > > > lunch) > > > > > > I'm in favor of DCHECK-ing that it's sorted and no duplicates. You should be > > > able to check the current value is larger than the previous. > > > > But my argument is that requiring the list to be sorted is a bit too > demanding. > > Why would we ask user to sort the list, if the elements in the list are > > logically independent? > > If it's too much hassle to require that and that the number of elements is > small, then it's fine. If the number of elements is big, then you could e.g. > look into passing a non-const pointer to the vector to the ctor and mutate it > directly. But again, if it's small and this is infrequent, then no biggie. The number is actually small (a few tens at the most) and it is very infrequent (since in ctor).
The CQ bit was checked by tommi@chromium.org
lgtm
The patchset sent to the CQ was uploaded after l-g-t-m from hlundin@chromium.org, ajm@chromium.org, rkaplow@chromium.org, grunell@chromium.org Link to the patchset: https://codereview.chromium.org/1357013006/#ps450001 (title: "on tommi's comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1357013006/450001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1357013006/450001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Patchset #22 (id:430002) has been deleted
The CQ bit was checked by minyue@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from grunell@chromium.org, ajm@chromium.org, tommi@chromium.org, rkaplow@chromium.org, hlundin@chromium.org Link to the patchset: https://codereview.chromium.org/1357013006/#ps480001 (title: "rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1357013006/480001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1357013006/480001
Message was sent while issue was closed.
Committed patchset #22 (id:480001)
Message was sent while issue was closed.
Patchset 22 (id:??) landed as https://crrev.com/6a05781d7334b28a75b3321301a8927b5cd7dc36 Cr-Commit-Position: refs/heads/master@{#358555} |
