Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1130063002:
Allowing a custom audio buffer size in WebRtcAudioCapturer (Closed)
|
Created:
5 years, 7 months ago by Charlie Modified:
5 years, 7 months ago CC:
chromium-reviews, darin-cc_chromium.org, feature-media-reviews_chromium.org, jam, juberti2, mcasas+watch_chromium.org, mkwst+moarreviews-renderer_chromium.org, mlamouri+watch-content_chromium.org, posciak+watch_chromium.org, rkc, wjia+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAllowing a custom audio buffer size in WebRtcAudioCapturer.
Committed: https://crrev.com/bdc452b641f8379e00523e1340f71a46f5621be6
Cr-Commit-Position: refs/heads/master@{#330192}
Patch Set 1 #Patch Set 2 : #
Total comments: 2
Patch Set 3 : #
Total comments: 26
Patch Set 4 : #Patch Set 5 : #
Total comments: 2
Patch Set 6 : #Patch Set 7 : Merging to head #
Messages
Total messages: 27 (6 generated)
ckehoe@chromium.org changed reviewers: + juberti@chromium.org, tommi@chromium.org
I haven't tested this yet, but in the interest of staying on track for M44, please let me know if this looks reasonable. https://codereview.chromium.org/1130063002/diff/20001/content/renderer/media/... File content/renderer/media/webrtc_audio_capturer.cc (right): https://codereview.chromium.org/1130063002/diff/20001/content/renderer/media/... content/renderer/media/webrtc_audio_capturer.cc:95: explicit TrackWrapper(WebRtcLocalAudioTrack* track) : track_(track) {} Fixing linter error. https://codereview.chromium.org/1130063002/diff/20001/media/audio/audio_manag... File media/audio/audio_manager_base.cc (right): https://codereview.chromium.org/1130063002/diff/20001/media/audio/audio_manag... media/audio/audio_manager_base.cc:203: LOG(INFO) << "Creating a new AudioInputStream with buffer size = " Disregard this statement. It's a temporary testing tool.
The CQ bit was checked by ckehoe@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1130063002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
Adding some comments. The overall method looks OK but it feels a bit hacky to use "0 <=> don't use the buffer size parameter". Then, "if (!buffer_size)" should be read as "if user has not set a buffer size via constraint" and that is not clear as I see it. Could you make some improvements in this area. Perhaps by defining const int kDontUseBufferSizeParameter = 0. Also, some test, or demo page driving this code change would be nice. And what about sanity tests here? Assume that the user selects 10000000000 as buffer size.
On 2015/05/07 08:10:12, henrika wrote: > Adding some comments. The overall method looks OK but it feels a bit hacky to > use "0 <=> don't use the buffer size parameter". Then, "if (!buffer_size)" > should be read as "if user has not set a buffer size via constraint" and that is > not clear as I see it. Could you make some improvements in this area. Perhaps by > defining const int kDontUseBufferSizeParameter = 0. > > Also, some test, or demo page driving this code change would be nice. > > And what about sanity tests here? Assume that the user selects 10000000000 as > buffer size. Thanks Henrik. I made some changes to hopefully address your comments. Can you be more specific about what kind of test I should add? What exactly would the test verify? There doesn't seem to be any logic in WebRtcAudioCapturerTest that deals with the buffer size: https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/m... I could instead add a demo page using the parameter, if that would be sufficient. Currently I'm just verifying this by reading the log message I added, and looking at the drop in CPU usage when I increase the buffer size. I will keep an eye on my email and try to be available early tomorrow morning, if that helps. I'd be great to get this checked into M44. Do I understand correctly that Tommi's OWNERS approval is required?
On 2015/05/13 22:58:34, Charlie wrote: > On 2015/05/07 08:10:12, henrika wrote: > > Adding some comments. The overall method looks OK but it feels a bit hacky to > > use "0 <=> don't use the buffer size parameter". Then, "if (!buffer_size)" > > should be read as "if user has not set a buffer size via constraint" and that > is > > not clear as I see it. Could you make some improvements in this area. Perhaps > by > > defining const int kDontUseBufferSizeParameter = 0. > > > > Also, some test, or demo page driving this code change would be nice. > > > > And what about sanity tests here? Assume that the user selects 10000000000 as > > buffer size. > > Thanks Henrik. I made some changes to hopefully address your comments. > > Can you be more specific about what kind of test I should add? What exactly > would the test verify? There doesn't seem to be any logic in > WebRtcAudioCapturerTest that deals with the buffer size: > > https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/m... > > I could instead add a demo page using the parameter, if that would be > sufficient. Currently I'm just verifying this by reading the log message I > added, and looking at the drop in CPU usage when I increase the buffer size. > > I will keep an eye on my email and try to be available early tomorrow morning, > if that helps. I'd be great to get this checked into M44. Do I understand > correctly that Tommi's OWNERS approval is required? Also, I should mention that the discussion on the w3c thread is trending towards calling it simply "latency" and giving the value in seconds: https://lists.w3.org/Archives/Public/public-media-capture/2015May/0008.html
+Henrik again who somehow got taken off CC On Wed, May 13, 2015, 4:21 PM <ckehoe@chromium.org> wrote: > On 2015/05/13 22:58:34, Charlie wrote: > > On 2015/05/07 08:10:12, henrika wrote: > > > Adding some comments. The overall method looks OK but it feels a bit > > hacky > to > > > use "0 <=> don't use the buffer size parameter". Then, "if > > (!buffer_size)" > > > should be read as "if user has not set a buffer size via constraint" > and > that > > is > > > not clear as I see it. Could you make some improvements in this area. > Perhaps > > by > > > defining const int kDontUseBufferSizeParameter = 0. > > > > > > Also, some test, or demo page driving this code change would be nice. > > > > > > And what about sanity tests here? Assume that the user selects > > 10000000000 > as > > > buffer size. > > > Thanks Henrik. I made some changes to hopefully address your comments. > > > Can you be more specific about what kind of test I should add? What > > exactly > > would the test verify? There doesn't seem to be any logic in > > WebRtcAudioCapturerTest that deals with the buffer size: > > > > https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/m... > > > I could instead add a demo page using the parameter, if that would be > > sufficient. Currently I'm just verifying this by reading the log message > I > > added, and looking at the drop in CPU usage when I increase the buffer > > size. > > > I will keep an eye on my email and try to be available early tomorrow > > morning, > > if that helps. I'd be great to get this checked into M44. Do I understand > > correctly that Tommi's OWNERS approval is required? > > Also, I should mention that the discussion on the w3c thread is trending > towards > calling it simply "latency" and giving the value in seconds: > > https://lists.w3.org/Archives/Public/public-media-capture/2015May/0008.html > > > https://codereview.chromium.org/1130063002/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
henrika@chromium.org changed reviewers: + henrika@chromium.org - juberti@chromium.org
henrika replaced juberti as reviewer, juberti is now on cc
LGTM with some minor comments. Note that I am not an owner. Given that a new constraint is needed to activate the new code, I think it is OK to TBR tommi (who is OOO today and tomorrow). I understand you want to land this before Friday if possible.
https://codereview.chromium.org/1130063002/diff/40001/content/renderer/media/... File content/renderer/media/webrtc_audio_capturer.cc (right): https://codereview.chromium.org/1130063002/diff/40001/content/renderer/media/... content/renderer/media/webrtc_audio_capturer.cc:28: const char kAudioLatency[] = "latency"; Is it clear from this name that this is for the capture side? Also, note that the actual latency will be larger since more parts are added in the complete audio path. You are in fact setting the native buffer size in milliseconds and not the capture latency. Just FYI. As an example, using 10ms will lead to different actual delays and the end result will also very between platforms. https://codereview.chromium.org/1130063002/diff/40001/content/renderer/media/... content/renderer/media/webrtc_audio_capturer.cc:29: const double kMinAudioLatency = 0; Add unit. https://codereview.chromium.org/1130063002/diff/40001/content/renderer/media/... content/renderer/media/webrtc_audio_capturer.cc:32: const int kDontUseBufferSizeParameter = 0; Is it used? https://codereview.chromium.org/1130063002/diff/40001/content/renderer/media/... content/renderer/media/webrtc_audio_capturer.cc:210: double buffer_size_seconds = 0; is it really in seconds!? Should it not be in milliseconds or do you allow a 60 seconds long delay? https://codereview.chromium.org/1130063002/diff/40001/content/renderer/media/... content/renderer/media/webrtc_audio_capturer.cc:216: if (buffer_size > 0) > kDontUse... https://codereview.chromium.org/1130063002/diff/40001/content/renderer/media/... content/renderer/media/webrtc_audio_capturer.cc:336: if (buffer_size == kDontUseBufferSizeParameter) Make a clear comment about what happens here and what controls this change. https://codereview.chromium.org/1130063002/diff/40001/content/renderer/media/... File content/renderer/media/webrtc_audio_capturer.h (right): https://codereview.chromium.org/1130063002/diff/40001/content/renderer/media/... content/renderer/media/webrtc_audio_capturer.h:142: // buffer_size is optional. Set to 0 to let it be chosen automatically. Perhaps mention kDontUseBufferSizeParameter https://codereview.chromium.org/1130063002/diff/40001/media/audio/audio_manag... File media/audio/audio_manager_base.cc (right): https://codereview.chromium.org/1130063002/diff/40001/media/audio/audio_manag... media/audio/audio_manager_base.cc:203: LOG(INFO) << "Creating a new AudioInputStream with buffer size = " DLOG?
https://codereview.chromium.org/1130063002/diff/40001/content/renderer/media/... File content/renderer/media/webrtc_audio_capturer.cc (right): https://codereview.chromium.org/1130063002/diff/40001/content/renderer/media/... content/renderer/media/webrtc_audio_capturer.cc:29: const double kMinAudioLatency = 0; It's not clear if this is a buffer size or time value. https://codereview.chromium.org/1130063002/diff/40001/content/renderer/media/... content/renderer/media/webrtc_audio_capturer.cc:32: const int kDontUseBufferSizeParameter = 0; From the name, it sounds like this should be a bool but the type is int, which is confusing. What about kInvalidBufferSize? Or perhaps this constant isn't necessary? I think I would prefer to see a 0 in the code in order to understand what's being compared instead of a constant name since I would probably always go and check what the magical value would be. https://codereview.chromium.org/1130063002/diff/40001/content/renderer/media/... content/renderer/media/webrtc_audio_capturer.cc:203: if (media::ToAudioSampleRate(sample_rate, &asr)) { nit: to keep the change simple, can you revert the cosmetic changes? https://codereview.chromium.org/1130063002/diff/40001/content/renderer/media/... content/renderer/media/webrtc_audio_capturer.cc:212: constraints_, kAudioLatency, &buffer_size_seconds); seconds doesn't feel right. also I think you need to check the return value of this call. If the call succeeds and the buffer size is not within the valid range, I would prefer to treat that as the buffer size not being set instead of clamping. https://codereview.chromium.org/1130063002/diff/40001/content/renderer/media/... content/renderer/media/webrtc_audio_capturer.cc:213: buffer_size_seconds = cc::MathUtil::ClampToRange( with the above, I think that would also remove the dependency on MathUtil? https://codereview.chromium.org/1130063002/diff/40001/media/audio/audio_manag... File media/audio/audio_manager_base.cc (right): https://codereview.chromium.org/1130063002/diff/40001/media/audio/audio_manag... media/audio/audio_manager_base.cc:203: LOG(INFO) << "Creating a new AudioInputStream with buffer size = " yeah please change this to VLOG(1) or DVLOG(1)
Responding now to comments that need discussion. I'll make all the requested code changes next. https://codereview.chromium.org/1130063002/diff/40001/content/renderer/media/... File content/renderer/media/webrtc_audio_capturer.cc (right): https://codereview.chromium.org/1130063002/diff/40001/content/renderer/media/... content/renderer/media/webrtc_audio_capturer.cc:28: const char kAudioLatency[] = "latency"; On 2015/05/14 15:19:51, henrika wrote: > Is it clear from this name that this is for the capture side? Also, note that > the actual latency will be larger since more parts are added in the complete > audio path. You are in fact setting the native buffer size in milliseconds and > not the capture latency. Just FYI. As an example, using 10ms will lead to > different actual delays and the end result will also very between platforms. The JS will look like this: navigator.webkitGetUserMedia( { 'audio' : { 'optional' : [ { 'latency' : 0.3 } ] } }, onAudioReady, onAudioError); We are requesting a audio input MediaStream and specifying the latency we can tolerate. This is an optional constraint, and since it's processed directly by the WebRtcAudioCapturer, it will be ignored if specified elsewhere. The w3c thread seems to be converging on calling this simply "latency": https://lists.w3.org/Archives/Public/public-media-capture/2015May/0008.html As for the total latency, it seems that this would depend rather unpredictably on the client's processing speed. Once the buffer size is set, the rest of the stream should hand the audio along with the same buffer size unless instructed otherwise. (Do you know whether this is the case?) So the main source of delay should be how long the driver accumulates audio before passing it along. https://codereview.chromium.org/1130063002/diff/40001/content/renderer/media/... content/renderer/media/webrtc_audio_capturer.cc:32: const int kDontUseBufferSizeParameter = 0; On 2015/05/14 16:29:26, tommi wrote: > From the name, it sounds like this should be a bool but the type is int, which > is confusing. What about kInvalidBufferSize? Or perhaps this constant isn't > necessary? I think I would prefer to see a 0 in the code in order to understand > what's being compared instead of a constant name since I would probably always > go and check what the magical value would be. I think Henrik was suggesting that we "tag" the zero with the constant name to explain what is going on. But I agree that an explicit zero with a comment seems better. https://codereview.chromium.org/1130063002/diff/40001/content/renderer/media/... content/renderer/media/webrtc_audio_capturer.cc:210: double buffer_size_seconds = 0; On 2015/05/14 15:19:51, henrika wrote: > is it really in seconds!? Should it not be in milliseconds or do you allow a 60 > seconds long delay? I can drop the max to 10 seconds if you want. I don't see much harm in allowing that. https://codereview.chromium.org/1130063002/diff/40001/content/renderer/media/... content/renderer/media/webrtc_audio_capturer.cc:212: constraints_, kAudioLatency, &buffer_size_seconds); On 2015/05/14 16:29:26, tommi wrote: > seconds doesn't feel right. also I think you need to check the return value of > this call. If the call succeeds and the buffer size is not within the valid > range, I would prefer to treat that as the buffer size not being set instead of > clamping. On the w3c thread it was pointed out that milliseconds aren't really used anywhere else. I also don't like seconds as a unit here, though. Actually the logic already does what you want if this value isn't set. Since it's initialized to zero, the value will stay there. And zero is our magic value that says we need to figure out the buffer size for ourselves. Setting an explicit minimum (like 10ms) wouldn't preserve the current behavior, since right now we use the hardware buffer size if it is smaller. https://codereview.chromium.org/1130063002/diff/40001/content/renderer/media/... content/renderer/media/webrtc_audio_capturer.cc:213: buffer_size_seconds = cc::MathUtil::ClampToRange( On 2015/05/14 16:29:25, tommi wrote: > with the above, I think that would also remove the dependency on MathUtil? Well, we still need to clamp the value if the user specifies something out of range. Also I think this isn't a binary dependency, since the function is a template that just calls std::min and std::max.
https://codereview.chromium.org/1130063002/diff/40001/content/renderer/media/... File content/renderer/media/webrtc_audio_capturer.cc (right): https://codereview.chromium.org/1130063002/diff/40001/content/renderer/media/... content/renderer/media/webrtc_audio_capturer.cc:29: const double kMinAudioLatency = 0; On 2015/05/14 15:19:51, henrika wrote: > Add unit. Done. https://codereview.chromium.org/1130063002/diff/40001/content/renderer/media/... content/renderer/media/webrtc_audio_capturer.cc:32: const int kDontUseBufferSizeParameter = 0; On 2015/05/14 16:46:10, Charlie wrote: > On 2015/05/14 16:29:26, tommi wrote: > > From the name, it sounds like this should be a bool but the type is int, which > > is confusing. What about kInvalidBufferSize? Or perhaps this constant isn't > > necessary? I think I would prefer to see a 0 in the code in order to > understand > > what's being compared instead of a constant name since I would probably always > > go and check what the magical value would be. > > I think Henrik was suggesting that we "tag" the zero with the constant name to > explain what is going on. But I agree that an explicit zero with a comment seems > better. Done. https://codereview.chromium.org/1130063002/diff/40001/content/renderer/media/... content/renderer/media/webrtc_audio_capturer.cc:203: if (media::ToAudioSampleRate(sample_rate, &asr)) { On 2015/05/14 16:29:26, tommi wrote: > nit: to keep the change simple, can you revert the cosmetic changes? Done. https://codereview.chromium.org/1130063002/diff/40001/content/renderer/media/... content/renderer/media/webrtc_audio_capturer.cc:212: constraints_, kAudioLatency, &buffer_size_seconds); On 2015/05/14 16:46:10, Charlie wrote: > On 2015/05/14 16:29:26, tommi wrote: > > seconds doesn't feel right. also I think you need to check the return value of > > this call. If the call succeeds and the buffer size is not within the valid > > range, I would prefer to treat that as the buffer size not being set instead > of > > clamping. > > On the w3c thread it was pointed out that milliseconds aren't really used > anywhere else. I also don't like seconds as a unit here, though. > > Actually the logic already does what you want if this value isn't set. Since > it's initialized to zero, the value will stay there. And zero is our magic value > that says we need to figure out the buffer size for ourselves. Setting an > explicit minimum (like 10ms) wouldn't preserve the current behavior, since right > now we use the hardware buffer size if it is smaller. Fixed to ignore out-of-range values. https://codereview.chromium.org/1130063002/diff/40001/content/renderer/media/... content/renderer/media/webrtc_audio_capturer.cc:216: if (buffer_size > 0) On 2015/05/14 15:19:51, henrika wrote: > > kDontUse... Done. https://codereview.chromium.org/1130063002/diff/40001/content/renderer/media/... content/renderer/media/webrtc_audio_capturer.cc:336: if (buffer_size == kDontUseBufferSizeParameter) On 2015/05/14 15:19:51, henrika wrote: > Make a clear comment about what happens here and what controls this change. Done. https://codereview.chromium.org/1130063002/diff/40001/content/renderer/media/... File content/renderer/media/webrtc_audio_capturer.h (right): https://codereview.chromium.org/1130063002/diff/40001/content/renderer/media/... content/renderer/media/webrtc_audio_capturer.h:142: // buffer_size is optional. Set to 0 to let it be chosen automatically. On 2015/05/14 15:19:51, henrika wrote: > Perhaps mention kDontUseBufferSizeParameter I think Tommi prefers the explicit zero.
Thanks Tommi and Henrik for the review. I think I addressed your comments. Absent any objections, I'll be checking this in tomorrow morning (Fri 5/15 between 9am and noon PDT). Putting tommi@ as TBR for OWNERS approval, as he is OOO and we need this in M44.
Henrik - are there any cases where an input stream can be shared? I've got one nit but the cl otherwise lgtm. https://codereview.chromium.org/1130063002/diff/80001/content/renderer/media/... File content/renderer/media/webrtc_audio_capturer.cc (right): https://codereview.chromium.org/1130063002/diff/80001/content/renderer/media/... content/renderer/media/webrtc_audio_capturer.cc:219: device_info_.device.input.sample_rate * buffer_size_ms / 1000; Can we move this calculation into an else clause of the if() statement? As is, it's not clear if you actually want to do this calculation when buffer_size_ms is 0 or if it's a bug (I do see the comment above though, but this calculation should not be done in that case I think).
On 2015/05/15 06:55:26, tommi wrote: > Henrik - are there any cases where an input stream can be shared? Not as far as I know. However (I don't know all the details) there is a unique type of correlation between in and out on Mac but I assume that Charlie has tested on all platforms. Also, as I have mentioned earlier, I am not convinced that there will be any CPU gain on Windows using this approach since the native audio engine will still run using its lowest possible native size. At least that is what Dale found when he tried to to similar adjustments on the output side to save resources.
On 2015/05/15 09:25:39, henrika wrote: > On 2015/05/15 06:55:26, tommi wrote: > > Henrik - are there any cases where an input stream can be shared? > > Not as far as I know. However (I don't know all the details) there is a unique > type of correlation between in and out on Mac but I assume that Charlie has > tested on all platforms. Also, as I have mentioned earlier, I am not convinced > that there will be any CPU gain on Windows using this approach since the native > audio engine will still run using its lowest possible native size. At least that > is what Dale found when he tried to to similar adjustments on the output side to > save resources. Yes, it sounds like this will help on Linux and Mac, but not Windows. That's a whole other can of worms, unfortunately. Thanks Tommi for the review while you're OOO!
Submitting after one last test. https://codereview.chromium.org/1130063002/diff/80001/content/renderer/media/... File content/renderer/media/webrtc_audio_capturer.cc (right): https://codereview.chromium.org/1130063002/diff/80001/content/renderer/media/... content/renderer/media/webrtc_audio_capturer.cc:219: device_info_.device.input.sample_rate * buffer_size_ms / 1000; On 2015/05/15 06:55:25, tommi wrote: > Can we move this calculation into an else clause of the if() statement? As is, > it's not clear if you actually want to do this calculation when buffer_size_ms > is 0 or if it's a bug (I do see the comment above though, but this calculation > should not be done in that case I think). Done.
The CQ bit was checked by ckehoe@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from henrika@chromium.org, tommi@chromium.org Link to the patchset: https://codereview.chromium.org/1130063002/#ps120001 (title: "Merging to head")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1130063002/120001
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/bdc452b641f8379e00523e1340f71a46f5621be6 Cr-Commit-Position: refs/heads/master@{#330192} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
