Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2132613002:
Adds data logging in native AudioDeviceBuffer class (Closed)
|
Created:
4 years, 5 months ago by henrika_webrtc Modified:
4 years, 5 months ago Reviewers:
stefan-webrtc CC:
webrtc-reviews_webrtc.org Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionAdds data logging in native AudioDeviceBuffer class.
Goal is to provide periodic logging of most essential audio parameters
for playout and recording sides. It will allow us to track if the native audio layer is working as intended.
BUG=NONE
Committed: https://crrev.com/348e411dd27e6dbe9b84b27ce46e9b7c657c1eae
Cr-Commit-Position: refs/heads/master@{#13440}
Patch Set 1 #
Total comments: 10
Patch Set 2 : Feedback from Stefan #Patch Set 3 : nit #
Total comments: 4
Patch Set 4 : Fixes build issues #Patch Set 5 : Added delta time to logs #Patch Set 6 : nits #Patch Set 7 : Special log for Windows #Patch Set 8 : Added comments for Windows TaskQueue #Patch Set 9 : Removed logging in TaskQueue for Windows #Patch Set 10 : rebased #
Messages
Total messages: 32 (14 generated)
Description was changed from ========== Adds data logging in native AudioDeviceBuffer class BUG= ========== to ========== Adds data logging in native AudioDeviceBuffer class. Goal is to provide periodic logging of most essential audio parameters for playout and recording sides. It will allow us to track if the native audio layer is working as intended. BUG=NONE ==========
henrika@webrtc.org changed reviewers: + stefan@webrtc.org
Example of output: 07-07 16:18:45.642 15948 15987 I libjingle: (audio_device_buffer.cc:413): [PLAY:10 sec@48kHz] callbacks: 1003, samples: 481440, rate: 48144 07-07 16:18:55.648 15948 15987 I libjingle: (audio_device_buffer.cc:405): [REC:10 sec@48kHz] callbacks: 1000, samples: 480000, rate: 48000 07-07 16:18:55.649 15948 15987 I libjingle: (audio_device_buffer.cc:413): [PLAY:10 sec@48kHz] callbacks: 1000, samples: 480000, rate: 48000 07-07 16:19:05.658 15948 15987 I libjingle: (audio_device_buffer.cc:405): [REC:10 sec@48kHz] callbacks: 1002, samples: 480960, rate: 48096 07-07 16:19:05.659 15948 15987 I libjingle: (audio_device_buffer.cc:413): [PLAY:10 sec@48kHz] callbacks: 1000, samples: 480000, rate: 48000 07-07 16:19:15.666 15948 15987 I libjingle: (audio_device_buffer.cc:405): [REC:10 sec@48kHz] callbacks: 1000, samples: 480000, rate: 48000 07-07 16:19:15.667 15948 15987 I libjingle: (audio_device_buffer.cc:413): [PLAY:10 sec@48kHz] callbacks: 1000, samples: 479520, rate: 47952 07-07 16:19:25.674 15948 15987 I libjingle: (audio_device_buffer.cc:405): [REC:10 sec@48kHz] callbacks: 1001, samples: 480480, rate: 48048 07-07 16:19:25.674 15948 15987 I libjingle: (audio_device_buffer.cc:413): [PLAY:10 sec@48kHz] callbacks: 1002, samples: 480960, rate: 48096 07-07 16:19:35.677 15948 15987 I libjingle: (audio_device_buffer.cc:405): [REC:10 sec@48kHz] callbacks: 1001, samples: 480480, rate: 48048 07-07 16:19:35.677 15948 15987 I libjingle: (audio_device_buffer.cc:413): [PLAY:10 sec@48kHz] callbacks: 998, samples: 479040, rate: 47904
Stefan, it would be great if you could take a look at this one. Thanks ;-)
https://codereview.webrtc.org/2132613002/diff/1/webrtc/modules/audio_device/a... File webrtc/modules/audio_device/audio_device_buffer.cc (right): https://codereview.webrtc.org/2132613002/diff/1/webrtc/modules/audio_device/a... webrtc/modules/audio_device/audio_device_buffer.cc:90: if (!timer_has_started_) { Is this accessed on a single thread? I don't know this code well, so I can't determine if this is safe or not. Maybe it's a good idea to add a thread checker now already just to be sure? https://codereview.webrtc.org/2132613002/diff/1/webrtc/modules/audio_device/a... webrtc/modules/audio_device/audio_device_buffer.cc:401: int32_t next_callback_time = rtc::Time32() + kTimerIntervalInMilliseconds; I think it'd be better to use TimeMillis() and int64_t's instead. https://codereview.webrtc.org/2132613002/diff/1/webrtc/modules/audio_device/a... webrtc/modules/audio_device/audio_device_buffer.cc:419: last_rec_callbacks_ = rec_callbacks_; As mentioned offline, I think you have to protect these with a lock, otherwise tsan this will most certainly complain. https://codereview.webrtc.org/2132613002/diff/1/webrtc/modules/audio_device/a... webrtc/modules/audio_device/audio_device_buffer.cc:424: int32_t time_to_wait_ms = next_callback_time - rtc::Time32(); same here. https://codereview.webrtc.org/2132613002/diff/1/webrtc/modules/audio_device/a... File webrtc/modules/audio_device/audio_device_buffer.h (right): https://codereview.webrtc.org/2132613002/diff/1/webrtc/modules/audio_device/a... webrtc/modules/audio_device/audio_device_buffer.h:87: std::unique_ptr<rtc::TaskQueue> task_queue_; Any point in allocating this dynamically?
Comments only. Will add thread checker next. https://codereview.webrtc.org/2132613002/diff/1/webrtc/modules/audio_device/a... File webrtc/modules/audio_device/audio_device_buffer.cc (right): https://codereview.webrtc.org/2132613002/diff/1/webrtc/modules/audio_device/a... webrtc/modules/audio_device/audio_device_buffer.cc:90: if (!timer_has_started_) { It is only accessed on one thread but it is a good idea to do that anyhow. Thanks. https://codereview.webrtc.org/2132613002/diff/1/webrtc/modules/audio_device/a... webrtc/modules/audio_device/audio_device_buffer.cc:401: int32_t next_callback_time = rtc::Time32() + kTimerIntervalInMilliseconds; On 2016/07/07 15:22:13, stefan-webrtc (holmer) wrote: > I think it'd be better to use TimeMillis() and int64_t's instead. Done. https://codereview.webrtc.org/2132613002/diff/1/webrtc/modules/audio_device/a... webrtc/modules/audio_device/audio_device_buffer.cc:419: last_rec_callbacks_ = rec_callbacks_; It actually does not complain. At least not in the tests we run. But let me add a thread checker and upload a new version. https://codereview.webrtc.org/2132613002/diff/1/webrtc/modules/audio_device/a... webrtc/modules/audio_device/audio_device_buffer.cc:424: int32_t time_to_wait_ms = next_callback_time - rtc::Time32(); On 2016/07/07 15:22:13, stefan-webrtc (holmer) wrote: > same here. Done. https://codereview.webrtc.org/2132613002/diff/1/webrtc/modules/audio_device/a... File webrtc/modules/audio_device/audio_device_buffer.h (right): https://codereview.webrtc.org/2132613002/diff/1/webrtc/modules/audio_device/a... webrtc/modules/audio_device/audio_device_buffer.h:87: std::unique_ptr<rtc::TaskQueue> task_queue_; Actually not. Will change.
Comments only. Will add thread checker next.
Added thread checker and used it in methods that are essential to this CL only. Will expand later. Moved all access of new members to task queue to avoid using locks. Hope you like it ;-) PTAL
Looks good, but you have some build errors. https://codereview.webrtc.org/2132613002/diff/40001/webrtc/modules/audio_devi... File webrtc/modules/audio_device/audio_device_buffer.cc (right): https://codereview.webrtc.org/2132613002/diff/40001/webrtc/modules/audio_devi... webrtc/modules/audio_device/audio_device_buffer.cc:295: rtc::Bind(&AudioDeviceBuffer::UpdateRecStats, this, nSamples)); Nice https://codereview.webrtc.org/2132613002/diff/40001/webrtc/modules/audio_devi... webrtc/modules/audio_device/audio_device_buffer.cc:420: << "rate: " << rate; Should you also log how much system time actually passed since the last call to LogStats()? The time may not be accurate, so you may want to know if 5, 10 or 15 ms has passed.
lgtm
Fixing build issues. Will add one extra log as Stefan suggests next. https://codereview.webrtc.org/2132613002/diff/40001/webrtc/modules/audio_devi... File webrtc/modules/audio_device/audio_device_buffer.cc (right): https://codereview.webrtc.org/2132613002/diff/40001/webrtc/modules/audio_devi... webrtc/modules/audio_device/audio_device_buffer.cc:295: rtc::Bind(&AudioDeviceBuffer::UpdateRecStats, this, nSamples)); Thanks! https://codereview.webrtc.org/2132613002/diff/40001/webrtc/modules/audio_devi... webrtc/modules/audio_device/audio_device_buffer.cc:420: << "rate: " << rate; Good idea. Let me fix that. Assuming you mean seconds and not milliseconds ;-) Note that I try to derive the exact waiting time below to compensate for any deviations. Will add anyhow.
The CQ bit was checked by henrika@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from stefan@webrtc.org Link to the patchset: https://codereview.webrtc.org/2132613002/#ps100001 (title: "nits")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_drmemory_light on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_drmemory_light/buil...)
The CQ bit was checked by henrika@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by henrika@webrtc.org
The CQ bit was checked by henrika@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from stefan@webrtc.org Link to the patchset: https://codereview.webrtc.org/2132613002/#ps120001 (title: "Special log for Windows")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_drmemory_light on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_drmemory_light/buil...)
The CQ bit was checked by henrika@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from stefan@webrtc.org Link to the patchset: https://codereview.webrtc.org/2132613002/#ps180001 (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 data logging in native AudioDeviceBuffer class. Goal is to provide periodic logging of most essential audio parameters for playout and recording sides. It will allow us to track if the native audio layer is working as intended. BUG=NONE ========== to ========== Adds data logging in native AudioDeviceBuffer class. Goal is to provide periodic logging of most essential audio parameters for playout and recording sides. It will allow us to track if the native audio layer is working as intended. BUG=NONE ==========
Message was sent while issue was closed.
Committed patchset #10 (id:180001)
Message was sent while issue was closed.
Description was changed from ========== Adds data logging in native AudioDeviceBuffer class. Goal is to provide periodic logging of most essential audio parameters for playout and recording sides. It will allow us to track if the native audio layer is working as intended. BUG=NONE ========== to ========== Adds data logging in native AudioDeviceBuffer class. Goal is to provide periodic logging of most essential audio parameters for playout and recording sides. It will allow us to track if the native audio layer is working as intended. BUG=NONE Committed: https://crrev.com/348e411dd27e6dbe9b84b27ce46e9b7c657c1eae Cr-Commit-Position: refs/heads/master@{#13440} ==========
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/348e411dd27e6dbe9b84b27ce46e9b7c657c1eae Cr-Commit-Position: refs/heads/master@{#13440}
Message was sent while issue was closed.
A revert of this CL (patchset #10 id:180001) has been created in https://codereview.webrtc.org/2139233002/ by sprang@webrtc.org. The reason for reverting is: Seems to break things upstream.. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
