Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2445363003:
Improvements in how WebRTC.Audio.RecordedOnlyZeros is added as histogram (Closed)
|
Created:
4 years, 1 month ago by henrika_webrtc Modified:
4 years, 1 month ago Reviewers:
tommi CC:
webrtc-reviews_webrtc.org Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionImprovements in how WebRTC.Audio.RecordedOnlyZeros is added as histogram.
Contains fixes for a non-perfect implementation in https://codereview.webrtc.org/2328433003/
Summary:
Adds WebRTC.Audio.RecordedOnlyZeros UMA stat when recording stops if:
- All level estimates during the audio session were zero, and
- If the audio session was longer than 10 seconds.
Adds four simple methods to the AudioDeviceBuffer (ADB) class to allow the ADM
to update the ADB about when media starts and stops in both directions.
Moves any "critical" parst out frome the timer (based on task queue) and ensures
that it only does trivial logging tasks.
The task queue is now owned by a unique pointer to improve control of when it
starts and stops.
Adds time measurements (for logging) of both total time playing out and total
recording time. Units are in milliseconds.
BUG=webrtc:6592
Committed: https://crrev.com/ba156cfe96d738ff4c04537bd4c23335aaace11f
Cr-Commit-Position: refs/heads/master@{#14854}
Patch Set 1 #Patch Set 2 : cleanup #
Total comments: 42
Patch Set 3 : Feedback from tommi@ #Patch Set 4 : Fixes failing ADM unittests #Patch Set 5 : Removes usage of invalid pointer to TaskQueue #Patch Set 6 : Final cleanup #Patch Set 7 : nits #
Total comments: 15
Patch Set 8 : Feedback from tommi@ #Patch Set 9 : More lambdas #
Total comments: 2
Patch Set 10 : Added TODO(henrika) #
Messages
Total messages: 22 (9 generated)
Description was changed from ========== Improvements in how WebRTC.Audio.RecordedOnlyZeros is added as histogram BUG= ========== to ========== Improvements in how WebRTC.Audio.RecordedOnlyZeros is added as histogram. Contains fixes for a non-perfect implementation in https://codereview.webrtc.org/2328433003/ BUG=webrtc:6592 ==========
henrika@webrtc.org changed reviewers: + tommi@webrtc.org
Description was changed from ========== Improvements in how WebRTC.Audio.RecordedOnlyZeros is added as histogram. Contains fixes for a non-perfect implementation in https://codereview.webrtc.org/2328433003/ BUG=webrtc:6592 ========== to ========== Improvements in how WebRTC.Audio.RecordedOnlyZeros is added as histogram. Contains fixes for a non-perfect implementation in https://codereview.webrtc.org/2328433003/ Summary: Adds WebRTC.Audio.RecordedOnlyZeros UMA stat when recording stops if: - All level estimates during the audio session were zero, and - If the audio session was longer than 10 seconds. Adds four simple methods to the AudioDeviceBuffer (ADB) class to allow the ADM to update the ABD about when media starts and stops in both directions. Moves any "critical" parst out frome the timer (based on task queue) and ensures that it only does trivial logging tasks. The task queue is now owned by a unique pointer to improve control o when it starts and stops. Adds time measurements (for logging) of both total time playing out and total recording time. Units are in milliseconds. BUG=webrtc:6592 ==========
PTAL
Description was changed from ========== Improvements in how WebRTC.Audio.RecordedOnlyZeros is added as histogram. Contains fixes for a non-perfect implementation in https://codereview.webrtc.org/2328433003/ Summary: Adds WebRTC.Audio.RecordedOnlyZeros UMA stat when recording stops if: - All level estimates during the audio session were zero, and - If the audio session was longer than 10 seconds. Adds four simple methods to the AudioDeviceBuffer (ADB) class to allow the ADM to update the ABD about when media starts and stops in both directions. Moves any "critical" parst out frome the timer (based on task queue) and ensures that it only does trivial logging tasks. The task queue is now owned by a unique pointer to improve control o when it starts and stops. Adds time measurements (for logging) of both total time playing out and total recording time. Units are in milliseconds. BUG=webrtc:6592 ========== to ========== Improvements in how WebRTC.Audio.RecordedOnlyZeros is added as histogram. Contains fixes for a non-perfect implementation in https://codereview.webrtc.org/2328433003/ Summary: Adds WebRTC.Audio.RecordedOnlyZeros UMA stat when recording stops if: - All level estimates during the audio session were zero, and - If the audio session was longer than 10 seconds. Adds four simple methods to the AudioDeviceBuffer (ADB) class to allow the ADM to update the ADB about when media starts and stops in both directions. Moves any "critical" parst out frome the timer (based on task queue) and ensures that it only does trivial logging tasks. The task queue is now owned by a unique pointer to improve control o when it starts and stops. Adds time measurements (for logging) of both total time playing out and total recording time. Units are in milliseconds. BUG=webrtc:6592 ==========
Description was changed from ========== Improvements in how WebRTC.Audio.RecordedOnlyZeros is added as histogram. Contains fixes for a non-perfect implementation in https://codereview.webrtc.org/2328433003/ Summary: Adds WebRTC.Audio.RecordedOnlyZeros UMA stat when recording stops if: - All level estimates during the audio session were zero, and - If the audio session was longer than 10 seconds. Adds four simple methods to the AudioDeviceBuffer (ADB) class to allow the ADM to update the ADB about when media starts and stops in both directions. Moves any "critical" parst out frome the timer (based on task queue) and ensures that it only does trivial logging tasks. The task queue is now owned by a unique pointer to improve control o when it starts and stops. Adds time measurements (for logging) of both total time playing out and total recording time. Units are in milliseconds. BUG=webrtc:6592 ========== to ========== Improvements in how WebRTC.Audio.RecordedOnlyZeros is added as histogram. Contains fixes for a non-perfect implementation in https://codereview.webrtc.org/2328433003/ Summary: Adds WebRTC.Audio.RecordedOnlyZeros UMA stat when recording stops if: - All level estimates during the audio session were zero, and - If the audio session was longer than 10 seconds. Adds four simple methods to the AudioDeviceBuffer (ADB) class to allow the ADM to update the ADB about when media starts and stops in both directions. Moves any "critical" parst out frome the timer (based on task queue) and ensures that it only does trivial logging tasks. The task queue is now owned by a unique pointer to improve control of when it starts and stops. Adds time measurements (for logging) of both total time playing out and total recording time. Units are in milliseconds. BUG=webrtc:6592 ==========
Here are some thoughts about this cl but I also took a look at a few other things that were there before and I think we can improve. https://codereview.webrtc.org/2445363003/diff/20001/webrtc/modules/audio_devi... File webrtc/modules/audio_device/audio_device_buffer.cc (right): https://codereview.webrtc.org/2445363003/diff/20001/webrtc/modules/audio_devi... webrtc/modules/audio_device/audio_device_buffer.cc:76: AudioDeviceBuffer::~AudioDeviceBuffer() { add checks for !playing_ and !recording_? https://codereview.webrtc.org/2445363003/diff/20001/webrtc/modules/audio_devi... webrtc/modules/audio_device/audio_device_buffer.cc:85: audio_transport_cb_ = audio_callback; is it allowed to call RegisterAudioCallback while the callback is active? https://codereview.webrtc.org/2445363003/diff/20001/webrtc/modules/audio_devi... webrtc/modules/audio_device/audio_device_buffer.cc:85: audio_transport_cb_ = audio_callback; first: RTC_DCHECK(!audio_transport_cb_ || !audio_callback); https://codereview.webrtc.org/2445363003/diff/20001/webrtc/modules/audio_devi... webrtc/modules/audio_device/audio_device_buffer.cc:89: int32_t AudioDeviceBuffer::InitPlayout() { can we remove this function? https://codereview.webrtc.org/2445363003/diff/20001/webrtc/modules/audio_devi... webrtc/modules/audio_device/audio_device_buffer.cc:96: if (playing_) { what about RTC_DCHECK(!playing_) ? https://codereview.webrtc.org/2445363003/diff/20001/webrtc/modules/audio_devi... webrtc/modules/audio_device/audio_device_buffer.cc:104: StartTimer(); it looks like every time StartTimer is called, you first check if task_queue_ is not null. What about moving this check into StartTimer and don't do anything if task_queue_ is already valid? https://codereview.webrtc.org/2445363003/diff/20001/webrtc/modules/audio_devi... webrtc/modules/audio_device/audio_device_buffer.cc:116: if (recording_) { RTC_DCHECK(!recording_)? https://codereview.webrtc.org/2445363003/diff/20001/webrtc/modules/audio_devi... webrtc/modules/audio_device/audio_device_buffer.cc:131: if (!playing_) { RTC_DCHECK(playing_) ? https://codereview.webrtc.org/2445363003/diff/20001/webrtc/modules/audio_devi... webrtc/modules/audio_device/audio_device_buffer.cc:157: LOG(INFO) << "total_diff_time: " << total_diff_time; can all of these be merged to one LOG statement? I wonder what the difference would be for this .obj file in terms of size if LOG(INFO) was made to be a no-op in release builds. Seems like a lot of debug logging that's not useful unless someone's actually debugging. https://codereview.webrtc.org/2445363003/diff/20001/webrtc/modules/audio_devi... webrtc/modules/audio_device/audio_device_buffer.cc:168: if (!recording_) { RTC_DCHECK(recording_) ? https://codereview.webrtc.org/2445363003/diff/20001/webrtc/modules/audio_devi... webrtc/modules/audio_device/audio_device_buffer.cc:191: LOG(INFO) << "HISTOGRAM(WebRTC.Audio.RecordedOnlyZeros): " << only_zeros; fix spaces https://codereview.webrtc.org/2445363003/diff/20001/webrtc/modules/audio_devi... webrtc/modules/audio_device/audio_device_buffer.cc:218: int32_t AudioDeviceBuffer::SetRecordingChannels(size_t channels) { See also question elsewhere about the rec_channels lambda. Can we restrict when this method is allowed to be called? Specifically, is it possible that we can make it so that calling this method is only allowed when *not* recording? If so, then we don't need a lock for rec_channels_ or rec_bytes_per_sample_. https://codereview.webrtc.org/2445363003/diff/20001/webrtc/modules/audio_devi... webrtc/modules/audio_device/audio_device_buffer.cc:306: return rec_channels_; why do we do this? It looks like we're grabbing the lock twice inside this function and might get two different values. Wouldn't it be better to simply fetch the value once and use the fetched value in both places? https://codereview.webrtc.org/2445363003/diff/20001/webrtc/modules/audio_devi... webrtc/modules/audio_device/audio_device_buffer.cc:315: LOG(LS_INFO) << "Size of recording buffer: " << rec_buffer_.size(); fyi - logging from within performance sensitive callbacks can be expensive. Amongst other things, it will grab a global lock for stdio, which means that this can contend with any other thread that is logging or writing to stdout. Additionally it grabs the global g_log_crit lock, which is held while any potential log sink's OnLogMessage method is called. https://codereview.webrtc.org/2445363003/diff/20001/webrtc/modules/audio_devi... webrtc/modules/audio_device/audio_device_buffer.cc:350: return rec_bytes_per_sample_; nit: it seems unnecessarily fancy to use a lambda here. We could just have a scope and cache the value locally. https://codereview.webrtc.org/2445363003/diff/20001/webrtc/modules/audio_devi... webrtc/modules/audio_device/audio_device_buffer.cc:410: if (res != 0) { at this point, we don't need to hold |lock| but we actually hold it for the rest of the function. Can we add a scope around the usage of audio_transport_cb_? https://codereview.webrtc.org/2445363003/diff/20001/webrtc/modules/audio_devi... webrtc/modules/audio_device/audio_device_buffer.cc:429: max_abs, num_samples_out)); fix indent (maybe run git cl format?) https://codereview.webrtc.org/2445363003/diff/20001/webrtc/modules/audio_devi... webrtc/modules/audio_device/audio_device_buffer.cc:444: LOG(INFO) << "StartTimer"; do we need all of this logging in release builds? https://codereview.webrtc.org/2445363003/diff/20001/webrtc/modules/audio_devi... webrtc/modules/audio_device/audio_device_buffer.cc:454: if (task_queue_) { check isn't necessary (reset() handles it) https://codereview.webrtc.org/2445363003/diff/20001/webrtc/modules/audio_devi... File webrtc/modules/audio_device/audio_device_buffer.h (right): https://codereview.webrtc.org/2445363003/diff/20001/webrtc/modules/audio_devi... webrtc/modules/audio_device/audio_device_buffer.h:16: #include "webrtc/base/task_queue.h" can you fwd declare TaskQueue now instead of including here? https://codereview.webrtc.org/2445363003/diff/20001/webrtc/modules/audio_devi... webrtc/modules/audio_device/audio_device_buffer.h:41: void StartPlayout(); we have the following three states: construct / destruct init / (no uninitialize function afaict) start / stop Do we need all of them?
Great feedback. Covered most of it and added some references to upcoming CLs where I plan to attack the locks in this class. PTAL https://codereview.webrtc.org/2445363003/diff/20001/webrtc/modules/audio_devi... File webrtc/modules/audio_device/audio_device_buffer.cc (right): https://codereview.webrtc.org/2445363003/diff/20001/webrtc/modules/audio_devi... webrtc/modules/audio_device/audio_device_buffer.cc:76: AudioDeviceBuffer::~AudioDeviceBuffer() { On 2016/10/25 16:43:22, tommi (webrtc) wrote: > add checks for !playing_ and !recording_? Done. https://codereview.webrtc.org/2445363003/diff/20001/webrtc/modules/audio_devi... webrtc/modules/audio_device/audio_device_buffer.cc:85: audio_transport_cb_ = audio_callback; Long story since this API goes way back when the idea that all APIs should be as flexible as possible. Today, we support starting media without any callback registered. The native layer will still run but no "connection" to WebRTC exists (all will be silent but audio is active in the background). Not used by any client AFAIK and no unit test exists to force it. I have https://bugs.chromium.org/p/webrtc/issues/detail?id=6560 where my plan is to improve more in this class. If you don't mind, could we postpone some parts from this CL to ensure that it does not change too much? I will then come back and make more changes inline with this proposal. https://codereview.webrtc.org/2445363003/diff/20001/webrtc/modules/audio_devi... webrtc/modules/audio_device/audio_device_buffer.cc:85: audio_transport_cb_ = audio_callback; We actually have unit tests today for the ADM (which owns this class) that uses nullptr as input here do deregister. See next comment as well. https://codereview.webrtc.org/2445363003/diff/20001/webrtc/modules/audio_devi... webrtc/modules/audio_device/audio_device_buffer.cc:89: int32_t AudioDeviceBuffer::InitPlayout() { On 2016/10/25 16:43:22, tommi (webrtc) wrote: > can we remove this function? Done. https://codereview.webrtc.org/2445363003/diff/20001/webrtc/modules/audio_devi... webrtc/modules/audio_device/audio_device_buffer.cc:96: if (playing_) { On 2016/10/25 16:43:23, tommi (webrtc) wrote: > what about RTC_DCHECK(!playing_) ? Done. https://codereview.webrtc.org/2445363003/diff/20001/webrtc/modules/audio_devi... webrtc/modules/audio_device/audio_device_buffer.cc:104: StartTimer(); Excellent idea ;-) https://codereview.webrtc.org/2445363003/diff/20001/webrtc/modules/audio_devi... webrtc/modules/audio_device/audio_device_buffer.cc:116: if (recording_) { On 2016/10/25 16:43:22, tommi (webrtc) wrote: > RTC_DCHECK(!recording_)? Done. https://codereview.webrtc.org/2445363003/diff/20001/webrtc/modules/audio_devi... webrtc/modules/audio_device/audio_device_buffer.cc:131: if (!playing_) { On 2016/10/25 16:43:22, tommi (webrtc) wrote: > RTC_DCHECK(playing_) ? Done. https://codereview.webrtc.org/2445363003/diff/20001/webrtc/modules/audio_devi... webrtc/modules/audio_device/audio_device_buffer.cc:157: LOG(INFO) << "total_diff_time: " << total_diff_time; On 2016/10/25 16:43:22, tommi (webrtc) wrote: > can all of these be merged to one LOG statement? > > I wonder what the difference would be for this .obj file in terms of size if > LOG(INFO) was made to be a no-op in release builds. Seems like a lot of debug > logging that's not useful unless someone's actually debugging. Done. https://codereview.webrtc.org/2445363003/diff/20001/webrtc/modules/audio_devi... webrtc/modules/audio_device/audio_device_buffer.cc:168: if (!recording_) { On 2016/10/25 16:43:22, tommi (webrtc) wrote: > RTC_DCHECK(recording_) ? Done. https://codereview.webrtc.org/2445363003/diff/20001/webrtc/modules/audio_devi... webrtc/modules/audio_device/audio_device_buffer.cc:191: LOG(INFO) << "HISTOGRAM(WebRTC.Audio.RecordedOnlyZeros): " << only_zeros; On 2016/10/25 16:43:22, tommi (webrtc) wrote: > fix spaces Done. https://codereview.webrtc.org/2445363003/diff/20001/webrtc/modules/audio_devi... webrtc/modules/audio_device/audio_device_buffer.cc:218: int32_t AudioDeviceBuffer::SetRecordingChannels(size_t channels) { I am working on a related CL which will improve how locks are handled in this class. Adding restrictions can be tricky since changes on the fly can sometimes be required when e.g. devices are plugged in and out. https://codereview.webrtc.org/2445363003/diff/20001/webrtc/modules/audio_devi... webrtc/modules/audio_device/audio_device_buffer.cc:306: return rec_channels_; I use the lambda based on feedback from kwiberg@ in an earlier CL. I only take it once in this method and the idea was to avoid an empty declaration only. https://codereview.webrtc.org/2445363003/diff/20001/webrtc/modules/audio_devi... webrtc/modules/audio_device/audio_device_buffer.cc:315: LOG(LS_INFO) << "Size of recording buffer: " << rec_buffer_.size(); Wow, didn't know that. It is a very, very rare event and only happens the first time we set the size or if e.g. the sample rate of a device changes. May I keep it? https://codereview.webrtc.org/2445363003/diff/20001/webrtc/modules/audio_devi... webrtc/modules/audio_device/audio_device_buffer.cc:350: return rec_bytes_per_sample_; LOL, I did initially but kwiberg@ wanted me to use a lambda. https://codereview.webrtc.org/2445363003/diff/20001/webrtc/modules/audio_devi... webrtc/modules/audio_device/audio_device_buffer.cc:410: if (res != 0) { As mentioned; I am planning a separate "improve locking in the ADB class" CL but this proposal is too good to resist so I am adding it ;-) https://codereview.webrtc.org/2445363003/diff/20001/webrtc/modules/audio_devi... webrtc/modules/audio_device/audio_device_buffer.cc:429: max_abs, num_samples_out)); For some reason git cl format does not work on this CL. Don't know the exact reason. I always use it. Will change. I created a new CL and patched it with this one and did git cl format again. Did not find any other misses than those you pointed out already. https://codereview.webrtc.org/2445363003/diff/20001/webrtc/modules/audio_devi... webrtc/modules/audio_device/audio_device_buffer.cc:444: LOG(INFO) << "StartTimer"; Removed https://codereview.webrtc.org/2445363003/diff/20001/webrtc/modules/audio_devi... webrtc/modules/audio_device/audio_device_buffer.cc:454: if (task_queue_) { Thanks! https://codereview.webrtc.org/2445363003/diff/20001/webrtc/modules/audio_devi... File webrtc/modules/audio_device/audio_device_buffer.h (right): https://codereview.webrtc.org/2445363003/diff/20001/webrtc/modules/audio_devi... webrtc/modules/audio_device/audio_device_buffer.h:16: #include "webrtc/base/task_queue.h" On 2016/10/25 16:43:23, tommi (webrtc) wrote: > can you fwd declare TaskQueue now instead of including here? Done. https://codereview.webrtc.org/2445363003/diff/20001/webrtc/modules/audio_devi... webrtc/modules/audio_device/audio_device_buffer.h:41: void StartPlayout(); Good point. Calls to init have been around for a long time. Will see if I can remove them without touching too much code... It worked well and I was able to remove both Init methods since they serve no purpose.
I had to remove some DCHECKs and use return instead due to how the existing unit tests for the ADM are designed. If not, I would have to modify unit tests and implementations on all platforms. The current design is that you can call Stop(), Stop() without Start() in between. I simply can't start making such changes in this CL.
Now done some final fixes that should remove any race conditions and improve the overall state. - The task queue is now a member of the ADB class (not a unique pointer). - New states results in: + logging and timer starts + logging keeps going, or + logging is stopped.
Hence, please check again Tommi ;-)
https://codereview.webrtc.org/2445363003/diff/120001/webrtc/modules/audio_dev... File webrtc/modules/audio_device/audio_device_buffer.cc (right): https://codereview.webrtc.org/2445363003/diff/120001/webrtc/modules/audio_dev... webrtc/modules/audio_device/audio_device_buffer.cc:94: if (playing_) { I had a comment on RTC_DCHECK(!playing_) here before, which was marked as 'done'. I'm not sure what changed though. Do we need this check? https://codereview.webrtc.org/2445363003/diff/120001/webrtc/modules/audio_dev... webrtc/modules/audio_device/audio_device_buffer.cc:99: task_queue_.PostTask(rtc::Bind(&AudioDeviceBuffer::ResetPlayStats, this)); fyi - you can use lambdas instead of rtc::Bind(). E.g. task_queue_.PostTask([this] { ResetPlayStats(); }); https://codereview.webrtc.org/2445363003/diff/120001/webrtc/modules/audio_dev... webrtc/modules/audio_device/audio_device_buffer.cc:114: if (recording_) { same here (see RTC_DCHECK comment above) https://codereview.webrtc.org/2445363003/diff/120001/webrtc/modules/audio_dev... webrtc/modules/audio_device/audio_device_buffer.cc:136: if (!playing_) { here too https://codereview.webrtc.org/2445363003/diff/120001/webrtc/modules/audio_dev... webrtc/modules/audio_device/audio_device_buffer.cc:173: if (!recording_) { and here. I'll have another pass at the previous comments tomorrow, just to see if there's anything that was missed so that I don't have to review all again https://codereview.webrtc.org/2445363003/diff/120001/webrtc/modules/audio_dev... File webrtc/modules/audio_device/audio_device_buffer.h (right): https://codereview.webrtc.org/2445363003/diff/120001/webrtc/modules/audio_dev... webrtc/modules/audio_device/audio_device_buffer.h:30: kLogStart = 0, LOG_START, LOG_STOP, LOG_ACTIVE, What about moving this enum into the class? As is, it's webrtc::LOG_START, which implies a much greater scope than just AudioDeviceBuffer. https://codereview.webrtc.org/2445363003/diff/120001/webrtc/modules/audio_dev... webrtc/modules/audio_device/audio_device_buffer.h:90: // state = |kLogStart| => members are initialized and the timer starts. and update documentation to ALL_CAPS style
PTAL https://codereview.webrtc.org/2445363003/diff/120001/webrtc/modules/audio_dev... File webrtc/modules/audio_device/audio_device_buffer.cc (right): https://codereview.webrtc.org/2445363003/diff/120001/webrtc/modules/audio_dev... webrtc/modules/audio_device/audio_device_buffer.cc:94: if (playing_) { I have a TODO to clean these things up but today the owning ADM supports a wide range of calling sequences, e.g. Start,Start,Stop,Stop and adding a strict check here will fail. Hence, to avoid wide-spread changes I would like to stick with the current approach. https://codereview.webrtc.org/2445363003/diff/120001/webrtc/modules/audio_dev... webrtc/modules/audio_device/audio_device_buffer.cc:99: task_queue_.PostTask(rtc::Bind(&AudioDeviceBuffer::ResetPlayStats, this)); It's magic ;-) https://codereview.webrtc.org/2445363003/diff/120001/webrtc/modules/audio_dev... webrtc/modules/audio_device/audio_device_buffer.cc:114: if (recording_) { See above. https://codereview.webrtc.org/2445363003/diff/120001/webrtc/modules/audio_dev... webrtc/modules/audio_device/audio_device_buffer.cc:136: if (!playing_) { dito https://codereview.webrtc.org/2445363003/diff/120001/webrtc/modules/audio_dev... webrtc/modules/audio_device/audio_device_buffer.cc:173: if (!recording_) { dito https://codereview.webrtc.org/2445363003/diff/120001/webrtc/modules/audio_dev... File webrtc/modules/audio_device/audio_device_buffer.h (right): https://codereview.webrtc.org/2445363003/diff/120001/webrtc/modules/audio_dev... webrtc/modules/audio_device/audio_device_buffer.h:30: kLogStart = 0, Will fix. It was actually not clear to me that we prefer capital letters. But I will change that part as well. https://codereview.webrtc.org/2445363003/diff/120001/webrtc/modules/audio_dev... webrtc/modules/audio_device/audio_device_buffer.h:90: // state = |kLogStart| => members are initialized and the timer starts. On 2016/10/27 17:38:24, tommi (webrtc) wrote: > and update documentation to ALL_CAPS style Done.
lgtm. for those things that I mentioned and you'd like to do later, it would be good to leave a todo in the code so we don't forget. https://codereview.webrtc.org/2445363003/diff/120001/webrtc/modules/audio_dev... File webrtc/modules/audio_device/audio_device_buffer.cc (right): https://codereview.webrtc.org/2445363003/diff/120001/webrtc/modules/audio_dev... webrtc/modules/audio_device/audio_device_buffer.cc:94: if (playing_) { On 2016/10/31 12:23:06, henrika_webrtc wrote: > I have a TODO to clean these things up but today the owning ADM supports a wide > range of calling sequences, e.g. Start,Start,Stop,Stop and adding a strict check > here will fail. Hence, to avoid wide-spread changes I would like to stick with > the current approach. OK, that wasn't clear to me. If you could leave a todo comment here for now, that would be good. https://codereview.webrtc.org/2445363003/diff/160001/webrtc/modules/audio_dev... File webrtc/modules/audio_device/audio_device_buffer.cc (right): https://codereview.webrtc.org/2445363003/diff/160001/webrtc/modules/audio_dev... webrtc/modules/audio_device/audio_device_buffer.cc:481: // set to kLogStart. Hence, first printed log will be after ~10 seconds. LOG_START
Thanks for tons of valuable feedback! https://codereview.webrtc.org/2445363003/diff/160001/webrtc/modules/audio_dev... File webrtc/modules/audio_device/audio_device_buffer.cc (right): https://codereview.webrtc.org/2445363003/diff/160001/webrtc/modules/audio_dev... webrtc/modules/audio_device/audio_device_buffer.cc:481: // set to kLogStart. Hence, first printed log will be after ~10 seconds. On 2016/10/31 14:18:03, tommi (webrtc) wrote: > LOG_START Done.
The CQ bit was checked by henrika@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from tommi@webrtc.org Link to the patchset: https://codereview.webrtc.org/2445363003/#ps180001 (title: "Added TODO(henrika)")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
Message was sent while issue was closed.
Description was changed from ========== Improvements in how WebRTC.Audio.RecordedOnlyZeros is added as histogram. Contains fixes for a non-perfect implementation in https://codereview.webrtc.org/2328433003/ Summary: Adds WebRTC.Audio.RecordedOnlyZeros UMA stat when recording stops if: - All level estimates during the audio session were zero, and - If the audio session was longer than 10 seconds. Adds four simple methods to the AudioDeviceBuffer (ADB) class to allow the ADM to update the ADB about when media starts and stops in both directions. Moves any "critical" parst out frome the timer (based on task queue) and ensures that it only does trivial logging tasks. The task queue is now owned by a unique pointer to improve control of when it starts and stops. Adds time measurements (for logging) of both total time playing out and total recording time. Units are in milliseconds. BUG=webrtc:6592 ========== to ========== Improvements in how WebRTC.Audio.RecordedOnlyZeros is added as histogram. Contains fixes for a non-perfect implementation in https://codereview.webrtc.org/2328433003/ Summary: Adds WebRTC.Audio.RecordedOnlyZeros UMA stat when recording stops if: - All level estimates during the audio session were zero, and - If the audio session was longer than 10 seconds. Adds four simple methods to the AudioDeviceBuffer (ADB) class to allow the ADM to update the ADB about when media starts and stops in both directions. Moves any "critical" parst out frome the timer (based on task queue) and ensures that it only does trivial logging tasks. The task queue is now owned by a unique pointer to improve control of when it starts and stops. Adds time measurements (for logging) of both total time playing out and total recording time. Units are in milliseconds. BUG=webrtc:6592 ==========
Message was sent while issue was closed.
Committed patchset #10 (id:180001)
Message was sent while issue was closed.
Description was changed from ========== Improvements in how WebRTC.Audio.RecordedOnlyZeros is added as histogram. Contains fixes for a non-perfect implementation in https://codereview.webrtc.org/2328433003/ Summary: Adds WebRTC.Audio.RecordedOnlyZeros UMA stat when recording stops if: - All level estimates during the audio session were zero, and - If the audio session was longer than 10 seconds. Adds four simple methods to the AudioDeviceBuffer (ADB) class to allow the ADM to update the ADB about when media starts and stops in both directions. Moves any "critical" parst out frome the timer (based on task queue) and ensures that it only does trivial logging tasks. The task queue is now owned by a unique pointer to improve control of when it starts and stops. Adds time measurements (for logging) of both total time playing out and total recording time. Units are in milliseconds. BUG=webrtc:6592 ========== to ========== Improvements in how WebRTC.Audio.RecordedOnlyZeros is added as histogram. Contains fixes for a non-perfect implementation in https://codereview.webrtc.org/2328433003/ Summary: Adds WebRTC.Audio.RecordedOnlyZeros UMA stat when recording stops if: - All level estimates during the audio session were zero, and - If the audio session was longer than 10 seconds. Adds four simple methods to the AudioDeviceBuffer (ADB) class to allow the ADM to update the ADB about when media starts and stops in both directions. Moves any "critical" parst out frome the timer (based on task queue) and ensures that it only does trivial logging tasks. The task queue is now owned by a unique pointer to improve control of when it starts and stops. Adds time measurements (for logging) of both total time playing out and total recording time. Units are in milliseconds. BUG=webrtc:6592 Committed: https://crrev.com/ba156cfe96d738ff4c04537bd4c23335aaace11f Cr-Commit-Position: refs/heads/master@{#14854} ==========
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/ba156cfe96d738ff4c04537bd4c23335aaace11f Cr-Commit-Position: refs/heads/master@{#14854} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
