Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2888383002:
Stop source and fire MediaStreamTrack ended event if missing audio input callbacks are detected. (Closed)
|
Created:
3 years, 7 months ago by Henrik Grunell Modified:
3 years, 7 months ago CC:
chromium-reviews, mlamouri+watch-content_chromium.org, imcheng+watch_chromium.org, feature-media-reviews_chromium.org, posciak+watch_chromium.org, chfremer+watch_chromium.org, jam, avayvod+watch_chromium.org, darin-cc_chromium.org, jasonroberts+watch_google.com, xjz+watch_chromium.org, mfoltz+watch_chromium.org, isheriff+watch_chromium.org, miu+watch_chromium.org, o1ka, Max Morin Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionStop source and fire MediaStreamTrack ended event if missing audio input callbacks are detected.
This way the javascript application is informed when there's a problem with the input device. Since the detection is in the renderer process we would also catch any other issues in the pipeline, including on the process border (i.e. the socket pair), should it render the same type of symptom.
* Detection is done in AudioInputDevice as this corresponds to a physical device which is what we want to detect missing callbacks for.
* When detected we call CaptureError() on the capturer, which stops the source and puts the track in ended state and fires the ended event.
* The time with missing callbacks before detecting is chosen to 12 seconds, so that other components down the stack have a chance to defer start if needed and report startup success stats. Specifically the Mac implementation. See also code comment in the CL.
* UMA stats is added; reporting a boolean when stopping.
Tested manually by altering a Linux build with https://codereview.chromium.org/2891303002/ and using a WebRTC test page with the MediaStreamTrack.onended set to log that the event was fired.
BUG=722335
TEST=See above.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
Review-Url: https://codereview.chromium.org/2888383002
Cr-Commit-Position: refs/heads/master@{#474383}
Committed: https://chromium.googlesource.com/chromium/src/+/dc44616c932d390c6ee11c2eb5931d51e7df91a7
Patch Set 1 #
Total comments: 2
Patch Set 2 : Moved detection to AudioInputDevice. Cleaned up. #
Total comments: 4
Patch Set 3 : Code review (tommi@) #Patch Set 4 : Added WebRTC logging in LocalMediaStreamAudioSource::OnCaptureError(). #
Total comments: 11
Patch Set 5 : Code review (ossu@ and tommi@). #
Total comments: 4
Patch Set 6 : Code review (ossu@). #Patch Set 7 : Storing TimeTicks' internal value instead. #
Total comments: 2
Patch Set 8 : Code review (isherman@), reduced wait time, fixed init of a variable. #Patch Set 9 : Revert changes in audio_low_latency_input_mac.cc. #Patch Set 10 : Fixed typo. #Patch Set 11 : Added missing include and moved back an include. #Patch Set 12 : Changing to post task instead of atomic operation since Atomic64 exists not on 32-bit platforms. #
Total comments: 4
Patch Set 13 : Code review (ossu@). #Patch Set 14 : Removed dependency on test CL. #
Messages
Total messages: 60 (34 generated)
Description was changed from ========== Stop source and fire MediaStreamTrack.onended event if no input callbacks are detected. BUG=722335 ========== to ========== Stop source and fire MediaStreamTrack.onended event if no input callbacks are detected. BUG=722335 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ==========
Description was changed from ========== Stop source and fire MediaStreamTrack.onended event if no input callbacks are detected. BUG=722335 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ========== to ========== Stop source and fire MediaStreamTrack ended event if no input callbacks are detected. BUG=722335 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ==========
Seems straightforward enough. Great! :) https://codereview.chromium.org/2888383002/diff/1/content/renderer/media/webr... File content/renderer/media/webrtc/processed_local_audio_source.cc (right): https://codereview.chromium.org/2888383002/diff/1/content/renderer/media/webr... content/renderer/media/webrtc/processed_local_audio_source.cc:281: base::subtle::Release_Store(&input_callback_is_active_, true); Perhaps just store last_callback_time_ atomically? Don't think tracking activity with a bool as well adds any functionality.
https://codereview.chromium.org/2888383002/diff/1/content/renderer/media/webr... File content/renderer/media/webrtc/processed_local_audio_source.cc (right): https://codereview.chromium.org/2888383002/diff/1/content/renderer/media/webr... content/renderer/media/webrtc/processed_local_audio_source.cc:281: base::subtle::Release_Store(&input_callback_is_active_, true); On 2017/05/18 13:38:54, ossu-chromium wrote: > Perhaps just store last_callback_time_ atomically? Don't think tracking activity > with a bool as well adds any functionality. Yeah, this is going away. The CL is not ready for review yet. :)
drive-by https://codereview.chromium.org/2888383002/diff/20001/media/audio/audio_input... File media/audio/audio_input_device.cc (right): https://codereview.chromium.org/2888383002/diff/20001/media/audio/audio_input... media/audio/audio_input_device.cc:310: base::TimeDelta::FromSeconds(kMissingCallbacksTimeBeforeErrorSeconds)) {} https://codereview.chromium.org/2888383002/diff/20001/media/audio/audio_input... media/audio/audio_input_device.cc:311: callback_->OnCaptureError("Detected missing callbacks."); what about something like "No audio received from audio capture device"? As is, the error message reads a little too much from the perspective of the code here, where 'callbacks' means a particular thing. Up at the JS layer, an error like "Detected missing callbacks" could be confusing.
Description was changed from ========== Stop source and fire MediaStreamTrack ended event if no input callbacks are detected. BUG=722335 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ========== to ========== Stop source and fire MediaStreamTrack ended event if missing audio input callbacks are detected. This way the javascript application is informed when there's a problem with the input device. * Detection is done in AudioInputDevice as this corresponds to a physical device which is what we want to detect missing callbacks for. * When detected we call CaptureError() on the capturer, which stops the source and puts the track in ended state and fires the ended event. * The time with missing callbacks before detecting is chosen to 20 seconds, so that other components down the stack have a chance to try restarting resources, log and report stats. Specifically the Mac implementation. See also code comment in the CL. I've tested manually by altering a Linux build with https://codereview.chromium.org/2891303002/ and using a WebRTC test page with the MediaStreamTrack.onended set to log that the event was fired. BUG=722335 TEST=See above. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ==========
grunell@chromium.org changed reviewers: + ossu@chromium.org, tommi@chromium.org
Description was changed from ========== Stop source and fire MediaStreamTrack ended event if missing audio input callbacks are detected. This way the javascript application is informed when there's a problem with the input device. * Detection is done in AudioInputDevice as this corresponds to a physical device which is what we want to detect missing callbacks for. * When detected we call CaptureError() on the capturer, which stops the source and puts the track in ended state and fires the ended event. * The time with missing callbacks before detecting is chosen to 20 seconds, so that other components down the stack have a chance to try restarting resources, log and report stats. Specifically the Mac implementation. See also code comment in the CL. I've tested manually by altering a Linux build with https://codereview.chromium.org/2891303002/ and using a WebRTC test page with the MediaStreamTrack.onended set to log that the event was fired. BUG=722335 TEST=See above. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ========== to ========== Stop source and fire MediaStreamTrack ended event if missing audio input callbacks are detected. This way the javascript application is informed when there's a problem with the input device. * Detection is done in AudioInputDevice as this corresponds to a physical device which is what we want to detect missing callbacks for. * When detected we call CaptureError() on the capturer, which stops the source and puts the track in ended state and fires the ended event. * The time with missing callbacks before detecting is chosen to 20 seconds, so that other components down the stack have a chance to try restarting resources, log and report stats. Specifically the Mac implementation. See also code comment in the CL. Tested manually by altering a Linux build with https://codereview.chromium.org/2891303002/ and using a WebRTC test page with the MediaStreamTrack.onended set to log that the event was fired. BUG=722335 TEST=See above. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ==========
Ha, Tommi you did a drive-by at the same time as I added you as reviewer and updated the CL description. Ptal, both. https://codereview.chromium.org/2888383002/diff/20001/media/audio/audio_input... File media/audio/audio_input_device.cc (right): https://codereview.chromium.org/2888383002/diff/20001/media/audio/audio_input... media/audio/audio_input_device.cc:310: base::TimeDelta::FromSeconds(kMissingCallbacksTimeBeforeErrorSeconds)) On 2017/05/19 09:11:48, tommi (sloooow) - chröme wrote: > {} Done. https://codereview.chromium.org/2888383002/diff/20001/media/audio/audio_input... media/audio/audio_input_device.cc:311: callback_->OnCaptureError("Detected missing callbacks."); On 2017/05/19 09:11:48, tommi (sloooow) - chröme wrote: > what about something like "No audio received from audio capture device"? > As is, the error message reads a little too much from the perspective of the > code here, where 'callbacks' means a particular thing. Up at the JS layer, an > error like "Detected missing callbacks" could be confusing. Oops, forgot to update this text. Thanks, your suggestion sounds good.
Description was changed from ========== Stop source and fire MediaStreamTrack ended event if missing audio input callbacks are detected. This way the javascript application is informed when there's a problem with the input device. * Detection is done in AudioInputDevice as this corresponds to a physical device which is what we want to detect missing callbacks for. * When detected we call CaptureError() on the capturer, which stops the source and puts the track in ended state and fires the ended event. * The time with missing callbacks before detecting is chosen to 20 seconds, so that other components down the stack have a chance to try restarting resources, log and report stats. Specifically the Mac implementation. See also code comment in the CL. Tested manually by altering a Linux build with https://codereview.chromium.org/2891303002/ and using a WebRTC test page with the MediaStreamTrack.onended set to log that the event was fired. BUG=722335 TEST=See above. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ========== to ========== Stop source and fire MediaStreamTrack ended event if missing audio input callbacks are detected. This way the javascript application is informed when there's a problem with the input device. Since the detection is in the renderer process we would also catch any other issues in the pipeline, including on the process border (i.e. the socket pair), should it render the same type of symptom. * Detection is done in AudioInputDevice as this corresponds to a physical device which is what we want to detect missing callbacks for. * When detected we call CaptureError() on the capturer, which stops the source and puts the track in ended state and fires the ended event. * The time with missing callbacks before detecting is chosen to 20 seconds, so that other components down the stack have a chance to try restarting resources, log and report stats. Specifically the Mac implementation. See also code comment in the CL. Tested manually by altering a Linux build with https://codereview.chromium.org/2891303002/ and using a WebRTC test page with the MediaStreamTrack.onended set to log that the event was fired. BUG=722335 TEST=See above. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ==========
looks pretty good. just a couple of questions. https://codereview.chromium.org/2888383002/diff/60001/media/audio/audio_input... File media/audio/audio_input_device.cc (right): https://codereview.chromium.org/2888383002/diff/60001/media/audio/audio_input... media/audio/audio_input_device.cc:36: const int kMissingCallbacksTimeBeforeErrorSeconds = 20; 20 seems pretty long. In the comment, can you point out what stats we rely on for deciding on this value? Also, what about taking care of the TODO in this CL? https://codereview.chromium.org/2888383002/diff/60001/media/audio/audio_input... media/audio/audio_input_device.cc:58: DataCallbackNotificationCallback data_callback_notification_callback); yo dawg, I heard you like callbacks, so...
https://codereview.chromium.org/2888383002/diff/60001/media/audio/audio_input... File media/audio/audio_input_device.cc (right): https://codereview.chromium.org/2888383002/diff/60001/media/audio/audio_input... media/audio/audio_input_device.cc:58: DataCallbackNotificationCallback data_callback_notification_callback); On 2017/05/19 11:00:37, tommi (sloooow) - chröme wrote: > yo dawg, I heard you like callbacks, so... Yeah, this name... could it be made a bit clearer? Or shorter? :) Also, why is setting the last callback time done by calling callbacks and posting tasks? Since this (hopefully) happens many times per second, why not just set it atomically directly from the callback? https://codereview.chromium.org/2888383002/diff/60001/media/audio/audio_input... media/audio/audio_input_device.cc:311: callback_->OnCaptureError("No audio received from audio capture device."); Should we also tie this to a UMA stat, like we do for the audio inputs on Mac? Seems like a stat here would encompass all our inputs and allow us to track them all with a single stat.
Patchset #5 (id:80001) has been deleted
https://codereview.chromium.org/2888383002/diff/60001/media/audio/audio_input... File media/audio/audio_input_device.cc (right): https://codereview.chromium.org/2888383002/diff/60001/media/audio/audio_input... media/audio/audio_input_device.cc:36: const int kMissingCallbacksTimeBeforeErrorSeconds = 20; On 2017/05/19 11:00:37, tommi (sloooow) - chröme wrote: > 20 seems pretty long. In the comment, can you point out what stats we rely on > for deciding on this value? > Also, what about taking care of the TODO in this CL? Hmm, first of all the 10 seconds is really 10-15 seconds since we check every 5 seconds and 10 seconds is the minimum time. So total time is 18-23 seconds... Secondly, the startup success stats we report after 18-23 seconds isn't really what we want to report. That stat should be for the startup only, not a restart when we already have missing callbacks. we should have a separate stat for if the restart helped or not; we don't have that today. So in that light we could disregard that stats reporting. Also, I saw that there is no WebRTC logging at the startup success check, so we won't be missing out on that. Plus we log in the capturers called from this class so that should be enough. We anyway need 15 seconds to allow a restart and some more time if callbacks are back (pun intended). I've started a mail thread with henrika@ who introduced the restart and picked the values. Maybe they can be reduced, maybe restart removed altogether. Updated the text, removed todo since it will be sorted out during this review. https://codereview.chromium.org/2888383002/diff/60001/media/audio/audio_input... media/audio/audio_input_device.cc:58: DataCallbackNotificationCallback data_callback_notification_callback); On 2017/05/19 11:26:48, ossu-chromium wrote: > On 2017/05/19 11:00:37, tommi (sloooow) - chröme wrote: > > yo dawg, I heard you like callbacks, so... > > Yeah, this name... could it be made a bit clearer? Or shorter? :) > Also, why is setting the last callback time done by calling callbacks and > posting tasks? Since this (hopefully) happens many times per second, why not > just set it atomically directly from the callback? Haha, yep it's a callback to notify about a callback. Can't be clearer. :) Set it atomically would lead to hacky ways. TimeTicks internal value can be retrieved and set, but it's discouraged to use it for calculations, for good reasons. TimeTicks can't be re-created from it either. Changed the name to GotDataCallback. https://codereview.chromium.org/2888383002/diff/60001/media/audio/audio_input... media/audio/audio_input_device.cc:311: callback_->OnCaptureError("No audio received from audio capture device."); On 2017/05/19 11:26:48, ossu-chromium wrote: > Should we also tie this to a UMA stat, like we do for the audio inputs on Mac? > Seems like a stat here would encompass all our inputs and allow us to track them > all with a single stat. Thanks for reminding. Yes, we should have that. We need to compare against when no error occurred. I added a flag for if missing callbacks have been detected and report a boolean when stopping. In the future we could add stats for other capture errors, but this serves the purpose of this detection.
Description was changed from ========== Stop source and fire MediaStreamTrack ended event if missing audio input callbacks are detected. This way the javascript application is informed when there's a problem with the input device. Since the detection is in the renderer process we would also catch any other issues in the pipeline, including on the process border (i.e. the socket pair), should it render the same type of symptom. * Detection is done in AudioInputDevice as this corresponds to a physical device which is what we want to detect missing callbacks for. * When detected we call CaptureError() on the capturer, which stops the source and puts the track in ended state and fires the ended event. * The time with missing callbacks before detecting is chosen to 20 seconds, so that other components down the stack have a chance to try restarting resources, log and report stats. Specifically the Mac implementation. See also code comment in the CL. Tested manually by altering a Linux build with https://codereview.chromium.org/2891303002/ and using a WebRTC test page with the MediaStreamTrack.onended set to log that the event was fired. BUG=722335 TEST=See above. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ========== to ========== Stop source and fire MediaStreamTrack ended event if missing audio input callbacks are detected. This way the javascript application is informed when there's a problem with the input device. Since the detection is in the renderer process we would also catch any other issues in the pipeline, including on the process border (i.e. the socket pair), should it render the same type of symptom. * Detection is done in AudioInputDevice as this corresponds to a physical device which is what we want to detect missing callbacks for. * When detected we call CaptureError() on the capturer, which stops the source and puts the track in ended state and fires the ended event. * The time with missing callbacks before detecting is chosen to 20 seconds, so that other components down the stack have a chance to try restarting resources. Specifically the Mac implementation. See also code comment in the CL. Tested manually by altering a Linux build with https://codereview.chromium.org/2891303002/ and using a WebRTC test page with the MediaStreamTrack.onended set to log that the event was fired. BUG=722335 TEST=See above. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ==========
Description was changed from ========== Stop source and fire MediaStreamTrack ended event if missing audio input callbacks are detected. This way the javascript application is informed when there's a problem with the input device. Since the detection is in the renderer process we would also catch any other issues in the pipeline, including on the process border (i.e. the socket pair), should it render the same type of symptom. * Detection is done in AudioInputDevice as this corresponds to a physical device which is what we want to detect missing callbacks for. * When detected we call CaptureError() on the capturer, which stops the source and puts the track in ended state and fires the ended event. * The time with missing callbacks before detecting is chosen to 20 seconds, so that other components down the stack have a chance to try restarting resources. Specifically the Mac implementation. See also code comment in the CL. Tested manually by altering a Linux build with https://codereview.chromium.org/2891303002/ and using a WebRTC test page with the MediaStreamTrack.onended set to log that the event was fired. BUG=722335 TEST=See above. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ========== to ========== Stop source and fire MediaStreamTrack ended event if missing audio input callbacks are detected. This way the javascript application is informed when there's a problem with the input device. Since the detection is in the renderer process we would also catch any other issues in the pipeline, including on the process border (i.e. the socket pair), should it render the same type of symptom. * Detection is done in AudioInputDevice as this corresponds to a physical device which is what we want to detect missing callbacks for. * When detected we call CaptureError() on the capturer, which stops the source and puts the track in ended state and fires the ended event. * The time with missing callbacks before detecting is chosen to 20 seconds, so that other components down the stack have a chance to try restarting resources. Specifically the Mac implementation. See also code comment in the CL. * UMA stats is added; reporting a boolean when stopping. * Clarified pieces of the restart in the Mac audio implementation. Tested manually by altering a Linux build with https://codereview.chromium.org/2891303002/ and using a WebRTC test page with the MediaStreamTrack.onended set to log that the event was fired. BUG=722335 TEST=See above. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ==========
The CQ bit was checked by grunell@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
This looks generally good, but I'm concerned about the performance implications of the current implementation, especially since I believe it's running on a real-time thread. https://codereview.chromium.org/2888383002/diff/60001/media/audio/audio_input... File media/audio/audio_input_device.cc (right): https://codereview.chromium.org/2888383002/diff/60001/media/audio/audio_input... media/audio/audio_input_device.cc:58: DataCallbackNotificationCallback data_callback_notification_callback); On 2017/05/20 08:40:51, Henrik Grunell wrote: > Set it atomically would lead to hacky ways. TimeTicks internal value can be > retrieved and set, but it's discouraged to use it for calculations, for good > reasons. TimeTicks can't be re-created from it either. Alright, but this seems very wasteful. IIUC, with a 128 sample buffer and a sample rate of 48000 you'd queue 375 tasks per second, per input stream, each potentially leading to two task switches to get the I/O thread to execute. Also locks. Making this atomic could improve things, but that still has performance implications. I think your best bet would be to not update this "inter-thread" state every callback. Perhaps first accumulating, say, a second's worth of callbacks on the callback thread before pushing a new TimeTicks (or similar) across to the AudioInputDevice object. I bet that could be done by counting samples, and you wouldn't have to access the timer each callback, though that's much less of a problem than the locks and task switches. I'll give you a bonus point if you do that update atomically. :) Perhaps by utilizing TimeTicks::operator- to get a TimeDelta relative to the start of the stream and using TimeDelta::InMicroseconds (or some other precision) to get a raw integer to work with. https://codereview.chromium.org/2888383002/diff/100001/media/audio/audio_inpu... File media/audio/audio_input_device.h (right): https://codereview.chromium.org/2888383002/diff/100001/media/audio/audio_inpu... media/audio/audio_input_device.h:191: // reported when stopping. Must only be accessed on the IO thread. Aren't there annotations to enforce that? https://codereview.chromium.org/2888383002/diff/100001/media/audio/mac/audio_... File media/audio/mac/audio_low_latency_input_mac.cc (right): https://codereview.chromium.org/2888383002/diff/100001/media/audio/mac/audio_... media/audio/mac/audio_low_latency_input_mac.cc:1289: if (number_of_restart_indications_ == indications_to_trigger_restart && So, this is functionally equivalent but rephrased to be a bit simpler to read?
https://codereview.chromium.org/2888383002/diff/60001/media/audio/audio_input... File media/audio/audio_input_device.cc (right): https://codereview.chromium.org/2888383002/diff/60001/media/audio/audio_input... media/audio/audio_input_device.cc:58: DataCallbackNotificationCallback data_callback_notification_callback); On 2017/05/22 11:27:35, ossu-chromium wrote: > On 2017/05/20 08:40:51, Henrik Grunell wrote: > > Set it atomically would lead to hacky ways. TimeTicks internal value can be > > retrieved and set, but it's discouraged to use it for calculations, for good > > reasons. TimeTicks can't be re-created from it either. > > Alright, but this seems very wasteful. IIUC, with a 128 sample buffer and a > sample rate of 48000 you'd queue 375 tasks per second, per input stream, each > potentially leading to two task switches to get the I/O thread to execute. Also > locks. > > Making this atomic could improve things, but that still has performance > implications. I think your best bet would be to not update this "inter-thread" > state every callback. Perhaps first accumulating, say, a second's worth of > callbacks on the callback thread before pushing a new TimeTicks (or similar) > across to the AudioInputDevice object. I bet that could be done by counting > samples, and you wouldn't have to access the timer each callback, though that's > much less of a problem than the locks and task switches. > > I'll give you a bonus point if you do that update atomically. :) > Perhaps by utilizing TimeTicks::operator- to get a TimeDelta relative to the > start of the stream and using TimeDelta::InMicroseconds (or some other > precision) to get a raw integer to work with. > Another idea is to not use TimeTicks and have an atomic sample count and a timer checking it ~every second. In this case, we could also detect anomalies like "got 60 samples last second but expected 100".
I'll do one change to this, so hold reviewing for a little while. https://codereview.chromium.org/2888383002/diff/60001/media/audio/audio_input... File media/audio/audio_input_device.cc (right): https://codereview.chromium.org/2888383002/diff/60001/media/audio/audio_input... media/audio/audio_input_device.cc:58: DataCallbackNotificationCallback data_callback_notification_callback); On 2017/05/22 11:27:35, ossu-chromium wrote: > On 2017/05/20 08:40:51, Henrik Grunell wrote: > > Set it atomically would lead to hacky ways. TimeTicks internal value can be > > retrieved and set, but it's discouraged to use it for calculations, for good > > reasons. TimeTicks can't be re-created from it either. > > Alright, but this seems very wasteful. IIUC, with a 128 sample buffer and a > sample rate of 48000 you'd queue 375 tasks per second, per input stream, each > potentially leading to two task switches to get the I/O thread to execute. Also > locks. Agree. > Making this atomic could improve things, but that still has performance > implications. I think your best bet would be to not update this "inter-thread" > state every callback. Perhaps first accumulating, say, a second's worth of > callbacks on the callback thread before pushing a new TimeTicks (or similar) > across to the AudioInputDevice object. I bet that could be done by counting > samples, and you wouldn't have to access the timer each callback, though that's > much less of a problem than the locks and task switches. That's a reasonable approach. Changed to report once a second. Counting samples should work fine, doing that. > I'll give you a bonus point if you do that update atomically. :) > Perhaps by utilizing TimeTicks::operator- to get a TimeDelta relative to the > start of the stream and using TimeDelta::InMicroseconds (or some other > precision) to get a raw integer to work with. Done too. I'm using a reference start time, but I saw that I was wrong about TimeTicks not being possible to create from its internal value. It is possible, so the internal value could be stored atomically and TimeTicks recreated from it without doing calculations on it. I kept the original code in a patch set anyway for reference. https://codereview.chromium.org/2888383002/diff/60001/media/audio/audio_input... media/audio/audio_input_device.cc:58: DataCallbackNotificationCallback data_callback_notification_callback); On 2017/05/22 11:44:59, Max Morin wrote: > On 2017/05/22 11:27:35, ossu-chromium wrote: > > On 2017/05/20 08:40:51, Henrik Grunell wrote: > > > Set it atomically would lead to hacky ways. TimeTicks internal value can be > > > retrieved and set, but it's discouraged to use it for calculations, for good > > > reasons. TimeTicks can't be re-created from it either. > > > > Alright, but this seems very wasteful. IIUC, with a 128 sample buffer and a > > sample rate of 48000 you'd queue 375 tasks per second, per input stream, each > > potentially leading to two task switches to get the I/O thread to execute. > Also > > locks. > > > > Making this atomic could improve things, but that still has performance > > implications. I think your best bet would be to not update this "inter-thread" > > state every callback. Perhaps first accumulating, say, a second's worth of > > callbacks on the callback thread before pushing a new TimeTicks (or similar) > > across to the AudioInputDevice object. I bet that could be done by counting > > samples, and you wouldn't have to access the timer each callback, though > that's > > much less of a problem than the locks and task switches. > > > > I'll give you a bonus point if you do that update atomically. :) > > Perhaps by utilizing TimeTicks::operator- to get a TimeDelta relative to the > > start of the stream and using TimeDelta::InMicroseconds (or some other > > precision) to get a raw integer to work with. > > > > Another idea is to not use TimeTicks and have an atomic sample count and a timer > checking it ~every second. In this case, we could also detect anomalies like > "got 60 samples last second but expected 100". That's a nice idea. However, it seems dangerous since we can't assume that we get an exact amount of data during a period of time. https://codereview.chromium.org/2888383002/diff/100001/media/audio/audio_inpu... File media/audio/audio_input_device.h (right): https://codereview.chromium.org/2888383002/diff/100001/media/audio/audio_inpu... media/audio/audio_input_device.h:191: // reported when stopping. Must only be accessed on the IO thread. On 2017/05/22 11:27:35, ossu-chromium wrote: > Aren't there annotations to enforce that? Nope. https://codereview.chromium.org/2888383002/diff/100001/media/audio/mac/audio_... File media/audio/mac/audio_low_latency_input_mac.cc (right): https://codereview.chromium.org/2888383002/diff/100001/media/audio/mac/audio_... media/audio/mac/audio_low_latency_input_mac.cc:1289: if (number_of_restart_indications_ == indications_to_trigger_restart && On 2017/05/22 11:27:35, ossu-chromium wrote: > So, this is functionally equivalent but rephrased to be a bit simpler to read? Yes, exactly. I hope you agree it is. :o
grunell@chromium.org changed reviewers: + isherman@chromium.org
Ptal. Added isherman@ for histograms.xml.
Metrics LGTM https://codereview.chromium.org/2888383002/diff/140001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2888383002/diff/140001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:26680: +<histogram name="Media.Audio.Capture.DetectedMissingCallbacks" enum="Boolean"> nit: Please use a custom enumeration (just within the .xml files), with labels like "No missing callbacks" and "Had missing callbacks".
Sweet! LGTM. Oh, did you test it again after the last changes? :)
On 2017/05/23 10:33:35, ossu-chromium wrote: > Sweet! LGTM. > > Oh, did you test it again after the last changes? :) Yes and it didn't work. Buuut there's no audio input device when Chromoting... I haven't set up any virtual device. I'm testing it locally today.
Tommi could you take a look? The only remaining change I know of is reducing the wait time before signalling capture error. I'll do that change tomorrow.
Drive-by questions: Do we understand why audio input callbacks are not being invoked? Is it in the API contract for audio input devices to not deliver audio regularly? If the answer to either of these is no, maybe the solution to this problem needs to be upstream? Otherwise, it feels like we're masking-over a problem that should be fixed in the platform audio layer.
lgtm - can you run this one more time by ossu after the last changes tomorrow?
On 2017/05/23 19:47:43, miu wrote: > Drive-by questions: Do we understand why audio input callbacks are not being > invoked? This has been a problem on Mac as far as we know. See https://bugs.chromium.org/p/chromium/issues/detail?id=549021. What we know is that callbacks suddenly might not come from the OS/audio system, but we don't know why. So yes. > Is it in the API contract for audio input devices to not deliver audio > regularly? With "audio input devices", are you referring to Chromium, the OS audio system, actual devices? In Chromium, we deliver audio when the OS/audio system delivers audio. So yes in that case. > If the answer to either of these is no, maybe the solution to this > problem needs to be upstream? Otherwise, it feels like we're masking-over a > problem that should be fixed in the platform audio layer. Yes, we should not mask over anything. And I don't think we do since we now log stats, log to the WebRTC log, and inform the application what is happening. Sure, if something goes wrong in Chromiums pipeline, we want to know about that and I think actually this gives us a better chance of catching that than before.
Oskar: please take a look now again. https://codereview.chromium.org/2888383002/diff/140001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2888383002/diff/140001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:26680: +<histogram name="Media.Audio.Capture.DetectedMissingCallbacks" enum="Boolean"> On 2017/05/22 20:38:25, Ilya Sherman wrote: > nit: Please use a custom enumeration (just within the .xml files), with labels > like "No missing callbacks" and "Had missing callbacks". Done.
The CQ bit was checked by grunell@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/05/24 12:42:17, Henrik Grunell wrote: > Oskar: please take a look now again. And yes, I have tested the latest patch set again.
Description was changed from ========== Stop source and fire MediaStreamTrack ended event if missing audio input callbacks are detected. This way the javascript application is informed when there's a problem with the input device. Since the detection is in the renderer process we would also catch any other issues in the pipeline, including on the process border (i.e. the socket pair), should it render the same type of symptom. * Detection is done in AudioInputDevice as this corresponds to a physical device which is what we want to detect missing callbacks for. * When detected we call CaptureError() on the capturer, which stops the source and puts the track in ended state and fires the ended event. * The time with missing callbacks before detecting is chosen to 20 seconds, so that other components down the stack have a chance to try restarting resources. Specifically the Mac implementation. See also code comment in the CL. * UMA stats is added; reporting a boolean when stopping. * Clarified pieces of the restart in the Mac audio implementation. Tested manually by altering a Linux build with https://codereview.chromium.org/2891303002/ and using a WebRTC test page with the MediaStreamTrack.onended set to log that the event was fired. BUG=722335 TEST=See above. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ========== to ========== Stop source and fire MediaStreamTrack ended event if missing audio input callbacks are detected. This way the javascript application is informed when there's a problem with the input device. Since the detection is in the renderer process we would also catch any other issues in the pipeline, including on the process border (i.e. the socket pair), should it render the same type of symptom. * Detection is done in AudioInputDevice as this corresponds to a physical device which is what we want to detect missing callbacks for. * When detected we call CaptureError() on the capturer, which stops the source and puts the track in ended state and fires the ended event. * The time with missing callbacks before detecting is chosen to 12 seconds, so that other components down the stack have a chance to defer start if needed and report startup success stats. Specifically the Mac implementation. See also code comment in the CL. * UMA stats is added; reporting a boolean when stopping. Tested manually by altering a Linux build with https://codereview.chromium.org/2891303002/ and using a WebRTC test page with the MediaStreamTrack.onended set to log that the event was fired. BUG=722335 TEST=See above. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
Patchset #11 (id:220001) has been deleted
The CQ bit was checked by grunell@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by grunell@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/05/24 12:48:58, Henrik Grunell wrote: > On 2017/05/24 12:42:17, Henrik Grunell wrote: > > Oskar: please take a look now again. > > And yes, I have tested the latest patch set again. Great! :) LGTM. I only have a couple of tiny nits. https://codereview.chromium.org/2888383002/diff/260001/media/audio/audio_inpu... File media/audio/audio_input_device.cc (right): https://codereview.chromium.org/2888383002/diff/260001/media/audio/audio_inpu... media/audio/audio_input_device.cc:318: if (base::TimeTicks::Now() - last_callback_time_ > nit: Should probably be >=, but I'm not going to enforce it . :) https://codereview.chromium.org/2888383002/diff/260001/media/audio/audio_inpu... File media/audio/audio_input_device.h (right): https://codereview.chromium.org/2888383002/diff/260001/media/audio/audio_inpu... media/audio/audio_input_device.h:59: #include "base/atomicops.h" Isn't this and the removal on #62 the wrong way around? I.e., you're not using TimeTicks and not Atomic{32,64}, so you should include time.h and not atomicops.h.
https://codereview.chromium.org/2888383002/diff/260001/media/audio/audio_inpu... File media/audio/audio_input_device.cc (right): https://codereview.chromium.org/2888383002/diff/260001/media/audio/audio_inpu... media/audio/audio_input_device.cc:318: if (base::TimeTicks::Now() - last_callback_time_ > On 2017/05/24 15:52:00, ossu-chromium wrote: > nit: Should probably be >=, but I'm not going to enforce it . :) Maybe so, maybe so. Everything in this mechanism is low precision anyway, and the likelihood of hitting equals here... https://codereview.chromium.org/2888383002/diff/260001/media/audio/audio_inpu... File media/audio/audio_input_device.h (right): https://codereview.chromium.org/2888383002/diff/260001/media/audio/audio_inpu... media/audio/audio_input_device.h:59: #include "base/atomicops.h" On 2017/05/24 15:52:01, ossu-chromium wrote: > Isn't this and the removal on #62 the wrong way around? I.e., you're not using > TimeTicks and not Atomic{32,64}, so you should include time.h and not > atomicops.h. Indeed, forgot this. Done.
The CQ bit was checked by grunell@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tommi@chromium.org, isherman@chromium.org, ossu@chromium.org Link to the patchset: https://codereview.chromium.org/2888383002/#ps280001 (title: "Code review (ossu@).")
The CQ bit was unchecked by commit-bot@chromium.org
This CL has an open dependency (Issue 2891303002 Patch 1). Please resolve the dependency and try again. If you are sure that there is no real dependency, please use one of the options listed in https://goo.gl/9Es4OR to land the CL.
The CQ bit was checked by grunell@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tommi@chromium.org, ossu@chromium.org, isherman@chromium.org Link to the patchset: https://codereview.chromium.org/2888383002/#ps300001 (title: "Removed dependency on test CL.")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 300001, "attempt_start_ts": 1495646685653390,
"parent_rev": "715ce8330af39110b768f8b8dcfcd9e199d10714", "commit_rev":
"dc44616c932d390c6ee11c2eb5931d51e7df91a7"}
Message was sent while issue was closed.
Description was changed from ========== Stop source and fire MediaStreamTrack ended event if missing audio input callbacks are detected. This way the javascript application is informed when there's a problem with the input device. Since the detection is in the renderer process we would also catch any other issues in the pipeline, including on the process border (i.e. the socket pair), should it render the same type of symptom. * Detection is done in AudioInputDevice as this corresponds to a physical device which is what we want to detect missing callbacks for. * When detected we call CaptureError() on the capturer, which stops the source and puts the track in ended state and fires the ended event. * The time with missing callbacks before detecting is chosen to 12 seconds, so that other components down the stack have a chance to defer start if needed and report startup success stats. Specifically the Mac implementation. See also code comment in the CL. * UMA stats is added; reporting a boolean when stopping. Tested manually by altering a Linux build with https://codereview.chromium.org/2891303002/ and using a WebRTC test page with the MediaStreamTrack.onended set to log that the event was fired. BUG=722335 TEST=See above. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ========== to ========== Stop source and fire MediaStreamTrack ended event if missing audio input callbacks are detected. This way the javascript application is informed when there's a problem with the input device. Since the detection is in the renderer process we would also catch any other issues in the pipeline, including on the process border (i.e. the socket pair), should it render the same type of symptom. * Detection is done in AudioInputDevice as this corresponds to a physical device which is what we want to detect missing callbacks for. * When detected we call CaptureError() on the capturer, which stops the source and puts the track in ended state and fires the ended event. * The time with missing callbacks before detecting is chosen to 12 seconds, so that other components down the stack have a chance to defer start if needed and report startup success stats. Specifically the Mac implementation. See also code comment in the CL. * UMA stats is added; reporting a boolean when stopping. Tested manually by altering a Linux build with https://codereview.chromium.org/2891303002/ and using a WebRTC test page with the MediaStreamTrack.onended set to log that the event was fired. BUG=722335 TEST=See above. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel Review-Url: https://codereview.chromium.org/2888383002 Cr-Commit-Position: refs/heads/master@{#474383} Committed: https://chromium.googlesource.com/chromium/src/+/dc44616c932d390c6ee11c2eb593... ==========
Message was sent while issue was closed.
Committed patchset #14 (id:300001) as https://chromium.googlesource.com/chromium/src/+/dc44616c932d390c6ee11c2eb593...
Message was sent while issue was closed.
On 2017/05/24 19:12:23, commit-bot: I haz the power wrote: > Committed patchset #14 (id:300001) as > https://chromium.googlesource.com/chromium/src/+/dc44616c932d390c6ee11c2eb593... This seems to break webrtc tests on Mac, see https://build.chromium.org/p/chromium.webrtc/builders/Mac%20Tester/builds/68150. I am doing a revert to see if it helps: https://codereview.chromium.org/2901333002/. I can't do it through rietveld UI as it gives an error(probably because histograms.xml is too large).
Message was sent while issue was closed.
Patchset #15 (id:320001) has been deleted |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
