Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2328433003:
Adds logging of native audio levels and UMA stats to track issues (Closed)
|
Created:
4 years, 3 months ago by henrika_webrtc Modified:
4 years, 1 month ago CC:
webrtc-reviews_webrtc.org Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionAdds logging of native audio levels and UMA stats to track issues.
This changes added a simple measurement of levels "close to the audio hardware"
both for playout and for recording. These levels are logged once each 10 seconds.
It also adds WebRTC.Audio.RecordedOnlyZeros UMA stat and it is updated at
destuction. It will report true iff all reported recording leves are zero.
BUG=NONE
R=peah@webrtc.org
Committed: https://chromium.googlesource.com/external/webrtc/+/f06f35a161b753696103615d8a40532c4fb4266a
Patch Set 1 #Patch Set 2 : Added UMA #Patch Set 3 : Minor improvements #
Total comments: 8
Patch Set 4 : rebased #
Total comments: 11
Messages
Total messages: 17 (8 generated)
Description was changed from ========== Adds logging of native audio levels and UMA stats to track issues BUG= ========== to ========== Adds logging of native audio levels and UMA stats to track issues BUG=NONE ==========
henrika@webrtc.org changed reviewers: + peah@webrtc.org
PTAL
Description was changed from ========== Adds logging of native audio levels and UMA stats to track issues BUG=NONE ========== to ========== Adds logging of native audio levels and UMA stats to track issues. This changes added a simple measurement of levels "close to the audio hardware" both for playout and for recording. These levels are logged once each 10 seconds. It also adds WebRTC.Audio.RecordedOnlyZeros UMA stat and it is updated at destuction. It will report true iff all reported recording leves are zero. BUG=NONE ==========
Nice! I have some questions though. One general question is whether this is allready done in other places. I know that the levels are logged in other places in the code. How does this compare to those? Does it make sense to have both? https://codereview.webrtc.org/2328433003/diff/40001/webrtc/modules/audio_devi... File webrtc/modules/audio_device/audio_device_buffer.cc (right): https://codereview.webrtc.org/2328433003/diff/40001/webrtc/modules/audio_devi... webrtc/modules/audio_device/audio_device_buffer.cc:354: task_queue_.PostTask(rtc::Bind(&AudioDeviceBuffer::UpdatePlayStats, this, Would it not be better to post the task once every 50 frames and for the posted UpdatePlayStats task then always do all the computations, instead of as now do post the task each frame and only do the actual computations for one frame out of all those 50. I don't know the overhead of the PostTask and Bind calls as well as the actial UpdatePlayStats call but if seems better to update and check the frame counter before posting the task, rather than to do it inside the task. https://codereview.webrtc.org/2328433003/diff/40001/webrtc/modules/audio_devi... webrtc/modules/audio_device/audio_device_buffer.cc:354: task_queue_.PostTask(rtc::Bind(&AudioDeviceBuffer::UpdatePlayStats, this, How critical is the complexity here? Would it be ok to compute the level estimation for all frames? https://codereview.webrtc.org/2328433003/diff/40001/webrtc/modules/audio_devi... webrtc/modules/audio_device/audio_device_buffer.cc:487: static_cast<int16_t*>(const_cast<void*>(audio_buffer)), Do we know that these samples are always int16_t samples? https://codereview.webrtc.org/2328433003/diff/40001/webrtc/modules/audio_devi... webrtc/modules/audio_device/audio_device_buffer.cc:505: static_cast<int16_t*>(const_cast<void*>(audio_buffer)), Do we know that these samples are always int16_t samples?
On 2016/09/09 09:06:27, peah-webrtc wrote: > Nice! I have some questions though. > > One general question is whether this is allready done in other places. I know > that the levels are logged in other places in the code. How does this compare to > those? Does it make sense to have both? > > https://codereview.webrtc.org/2328433003/diff/40001/webrtc/modules/audio_devi... > File webrtc/modules/audio_device/audio_device_buffer.cc (right): > > https://codereview.webrtc.org/2328433003/diff/40001/webrtc/modules/audio_devi... > webrtc/modules/audio_device/audio_device_buffer.cc:354: > task_queue_.PostTask(rtc::Bind(&AudioDeviceBuffer::UpdatePlayStats, this, > Would it not be better to post the task once every 50 frames and for the posted > UpdatePlayStats task then always do all the computations, instead of as now do > post the task each frame and only do the actual computations for one frame out > of all those 50. > > I don't know the overhead of the PostTask and Bind calls as well as the actial > UpdatePlayStats call but if seems better to update and check the frame counter > before posting the task, rather than to do it inside the task. > > https://codereview.webrtc.org/2328433003/diff/40001/webrtc/modules/audio_devi... > webrtc/modules/audio_device/audio_device_buffer.cc:354: > task_queue_.PostTask(rtc::Bind(&AudioDeviceBuffer::UpdatePlayStats, this, > How critical is the complexity here? Would it be ok to compute the level > estimation for all frames? > > https://codereview.webrtc.org/2328433003/diff/40001/webrtc/modules/audio_devi... > webrtc/modules/audio_device/audio_device_buffer.cc:487: > static_cast<int16_t*>(const_cast<void*>(audio_buffer)), > Do we know that these samples are always int16_t samples? > > https://codereview.webrtc.org/2328433003/diff/40001/webrtc/modules/audio_devi... > webrtc/modules/audio_device/audio_device_buffer.cc:505: > static_cast<int16_t*>(const_cast<void*>(audio_buffer)), > Do we know that these samples are always int16_t samples? lgtm after offline discussions. Great change!
https://codereview.webrtc.org/2328433003/diff/40001/webrtc/modules/audio_devi... File webrtc/modules/audio_device/audio_device_buffer.cc (right): https://codereview.webrtc.org/2328433003/diff/40001/webrtc/modules/audio_devi... webrtc/modules/audio_device/audio_device_buffer.cc:354: task_queue_.PostTask(rtc::Bind(&AudioDeviceBuffer::UpdatePlayStats, this, Referring to off-line discussion. https://codereview.webrtc.org/2328433003/diff/40001/webrtc/modules/audio_devi... webrtc/modules/audio_device/audio_device_buffer.cc:354: task_queue_.PostTask(rtc::Bind(&AudioDeviceBuffer::UpdatePlayStats, this, Referring to off-line discussion. https://codereview.webrtc.org/2328433003/diff/40001/webrtc/modules/audio_devi... webrtc/modules/audio_device/audio_device_buffer.cc:487: static_cast<int16_t*>(const_cast<void*>(audio_buffer)), Yes. But I can add some more stuff around it. https://codereview.webrtc.org/2328433003/diff/40001/webrtc/modules/audio_devi... webrtc/modules/audio_device/audio_device_buffer.cc:505: static_cast<int16_t*>(const_cast<void*>(audio_buffer)), Yes.
The CQ bit was checked by henrika@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from peah@webrtc.org Link to the patchset: https://codereview.webrtc.org/2328433003/#ps60001 (title: "rebased")
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 ========== Adds logging of native audio levels and UMA stats to track issues. This changes added a simple measurement of levels "close to the audio hardware" both for playout and for recording. These levels are logged once each 10 seconds. It also adds WebRTC.Audio.RecordedOnlyZeros UMA stat and it is updated at destuction. It will report true iff all reported recording leves are zero. BUG=NONE ========== to ========== Adds logging of native audio levels and UMA stats to track issues. This changes added a simple measurement of levels "close to the audio hardware" both for playout and for recording. These levels are logged once each 10 seconds. It also adds WebRTC.Audio.RecordedOnlyZeros UMA stat and it is updated at destuction. It will report true iff all reported recording leves are zero. BUG=NONE R=peah@webrtc.org Committed: https://crrev.com/f06f35a161b753696103615d8a40532c4fb4266a Cr-Commit-Position: refs/heads/master@{#14160} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/f06f35a161b753696103615d8a40532c4fb4266a Cr-Commit-Position: refs/heads/master@{#14160}
Message was sent while issue was closed.
Description was changed from ========== Adds logging of native audio levels and UMA stats to track issues. This changes added a simple measurement of levels "close to the audio hardware" both for playout and for recording. These levels are logged once each 10 seconds. It also adds WebRTC.Audio.RecordedOnlyZeros UMA stat and it is updated at destuction. It will report true iff all reported recording leves are zero. BUG=NONE R=peah@webrtc.org Committed: https://crrev.com/f06f35a161b753696103615d8a40532c4fb4266a Cr-Commit-Position: refs/heads/master@{#14160} ========== to ========== Adds logging of native audio levels and UMA stats to track issues. This changes added a simple measurement of levels "close to the audio hardware" both for playout and for recording. These levels are logged once each 10 seconds. It also adds WebRTC.Audio.RecordedOnlyZeros UMA stat and it is updated at destuction. It will report true iff all reported recording leves are zero. BUG=NONE R=peah@webrtc.org Committed: https://chromium.googlesource.com/external/webrtc/+/f06f35a161b753696103615d8... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) manually as f06f35a161b753696103615d8a40532c4fb4266a (presubmit successful).
Message was sent while issue was closed.
tommi@webrtc.org changed reviewers: + tommi@webrtc.org
Message was sent while issue was closed.
https://codereview.chromium.org/2328433003/diff/60001/webrtc/modules/audio_de... File webrtc/modules/audio_device/audio_device_buffer.cc (right): https://codereview.chromium.org/2328433003/diff/60001/webrtc/modules/audio_de... webrtc/modules/audio_device/audio_device_buffer.cc:103: RTC_LOGGED_HISTOGRAM_BOOLEAN("WebRTC.Audio.RecordedOnlyZeros", since this is only logged in certain cases, there's no comparison to see when rec_callbacks_ or num_stats_reports_ is 0, which could be useful to know the ratio between the two (or if this dtor actually ever runs before stats gathering is turned off). https://codereview.chromium.org/2328433003/diff/60001/webrtc/modules/audio_de... webrtc/modules/audio_device/audio_device_buffer.cc:104: static_cast<int>(num_stat_reports_ == num_rec_level_is_zero_)); fix indent https://codereview.chromium.org/2328433003/diff/60001/webrtc/modules/audio_de... webrtc/modules/audio_device/audio_device_buffer.cc:286: audio_buffer, num_samples)); isn't this a bug? audio_buffer will for sure not be correct when the task actually runs, it might not even be valid. https://codereview.chromium.org/2328433003/diff/60001/webrtc/modules/audio_de... webrtc/modules/audio_device/audio_device_buffer.cc:356: &play_buffer_[0], num_samples_out)); this looks like a race. We're holding a lock here, not running on the task queue, write to play_buffer_[0] and then post that pointer... UpdatePlayStats doesn't hold the same lock but even if it did, it looks to me like what would be in that buffer isn't guaranteed to be what it was when the call to PostTask was made. https://codereview.chromium.org/2328433003/diff/60001/webrtc/modules/audio_de... webrtc/modules/audio_device/audio_device_buffer.cc:447: rec_callbacks_ = 0; what if any of these are not 0 already? Are we missing out on reporting (see dtor)? https://codereview.chromium.org/2328433003/diff/60001/webrtc/modules/audio_de... webrtc/modules/audio_device/audio_device_buffer.cc:466: RTC_DCHECK(task_queue_.IsCurrent()); see above. if this runs on the task queue, it looks to me like it will contend with the audio callback thread(s) https://codereview.chromium.org/2328433003/diff/60001/webrtc/modules/audio_de... webrtc/modules/audio_device/audio_device_buffer.cc:484: RTC_DCHECK(task_queue_.IsCurrent()); same here
Message was sent while issue was closed.
Thanks Tommi! Posted some initial comments. Let me upload a new change and take your comments into account. I have done other changes in this area since the CL landed. Hence, will start from scratch in separate issue and refer to this one. Hope that is OK. https://codereview.chromium.org/2328433003/diff/60001/webrtc/modules/audio_de... File webrtc/modules/audio_device/audio_device_buffer.cc (right): https://codereview.chromium.org/2328433003/diff/60001/webrtc/modules/audio_de... webrtc/modules/audio_device/audio_device_buffer.cc:286: audio_buffer, num_samples)); Correct. It has been fixed since this CL landed. I no longer pass the audio buffer but do the level calculation on the audio thread and post the result. https://codereview.chromium.org/2328433003/diff/60001/webrtc/modules/audio_de... webrtc/modules/audio_device/audio_device_buffer.cc:356: &play_buffer_[0], num_samples_out)); Agree. Has been changed since this CL landed. https://codereview.chromium.org/2328433003/diff/60001/webrtc/modules/audio_de... webrtc/modules/audio_device/audio_device_buffer.cc:466: RTC_DCHECK(task_queue_.IsCurrent()); On 2016/10/24 12:19:14, tommi (webrtc) wrote: > see above. if this runs on the task queue, it looks to me like it will contend > with the audio callback thread(s) Acknowledged. https://codereview.chromium.org/2328433003/diff/60001/webrtc/modules/audio_de... webrtc/modules/audio_device/audio_device_buffer.cc:484: RTC_DCHECK(task_queue_.IsCurrent()); On 2016/10/24 12:19:14, tommi (webrtc) wrote: > same here Acknowledged. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
