Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
661443002:
Do not wait for the synchronization in AudioSyncReader (Closed)
|
Created:
6 years, 2 months ago by no longer working on chromium Modified:
5 years, 11 months ago CC:
chromium-reviews, posciak+watch_chromium.org, jam, mcasas+watch_chromium.org, feature-media-reviews_chromium.org, darin-cc_chromium.org, wjia+watch_chromium.org, miu+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionDo not wait for the synchronization in AudioSyncReader.
The current synchronization is problematic due to the |maximum_wait_time_| might be wrong on Mac, for example, when the buffer size has been changed to a lower value by another stream.
On the other hand, we probably should not wait for the synchronization at all, that says, if the renderer has already delivered a packet to the browser, the browser should proceed the packet instead of waiting for the newest packet.
BUG=423696
Patch Set 1 #
Total comments: 2
Messages
Total messages: 26 (2 generated)
xians@chromium.org changed reviewers: + dalecurtis@chromium.org, tommi@chromium.org
Hi, please review this CL.
On 2014/10/15 11:34:50, xians1 wrote: > Hi, please review this CL. Is there a bug filed? (if not, can you file one?)
not lgtm, I strongly think this is not the right approach. I've got an email out to you and I'll take a further look at the issue.
For others: the gist of the email is that we're always reading a previously requested buffer, not the current buffer. I.e., buffer 0 is filled before the OS level stream actually starts. Even if the initial buffer size is 512 and OSX is reading out 128 frame chunks, the next 512 frame chunk is requested as soon as the initial 512 are read; it is _not_ requested when the last 128 frame chunk is read from the 512. Long term I've always wanted to reduce the wait here to something like 2ms for all platforms, not vary it with buffer size.
You mention this only being the best case in email, but I don't see data to back this up; do you have any? Does changing the delay to 128/2 actually fix the problem? If you aren't ready in 5ms you're not going to be ready in ~1ms. Which means you're still going to glitch.
Copy the comment from another email thread: Dale: I actually thought about what you said some more and I don't think we have the right issue. Remember for audio output we actually request the first buffer ahead of the playback start. So the timeline looks like this: t=0 --> Request first buffer + Start stream. t=1 --> First buffer complete. t=2 --> OS stream actually starts. t=3 --> Read first buffer, request next buffer. t=3 + buffer size --> Read next buffer. I.e. if RTC is using a 512 sized buffer and reading it out in 128 frame chunks, the next 512 sized buffer is actually requested at the time of receipt, not when the last 128 frames is read from the larger chunk. So RTC should have plenty of time to fulfill the buffer. Xians: That is the ideal case, if things work exactly this way, we wouldn't need to AudioSyncReader::WaitUntilDataIsReady(). Think about the following cases: t=n -->WaitUntilDataIsReady() times out(5ms) (this can happen for different reasons), CoreAudio gets into under-run, and we feed 0 to CoreAudio. t=n+1 --> feeds 0 t=n+2 --> feeds 0 t=n+3 --> WaitUntilDataIsReady() waits for the synchronization of the indexing, this might put CoreAudio into another under-run. The fact is that, when webaudio is being use, the max_wait_time_ in AudioSyncReader should be 128/sample_rate/2, instead of 5ms for webrtc.
Dale, if you take a look at b/17298528, in comments #22, our users reported some AudioOutputControllerDataNotReady, but the glitches are worst than what those DataNotReady can explain, I suspect that it is because the wrong max_wait_time_ introduces glitches. comments #93 and comments #103 tell the difference when we force webaudio to use 10ms as buffer size.
Can you just send them a build with 128/2 sized delay and see if it resolves the issue?
Dale, you are right, if we change the max_wait_time to 1-2ms, things will be
worse for webrtc.
The evidence is that in comment #22, user reported:
Histogram: Media.AudioOutputControllerDataNotReady recorded 453 samples, average
= 1.4 (flags = 0x1)
0 O (0
= 0.0%)
1 ------------------------------------------------------------------------O
(405 = 89.4%) {0.0%}
These 405 DataNotReady are from webaudio stream, but webrtc renderer client is
doing things heavier than WebAudio renderer, so I am expecting much more
DataNotReady if the WebRTC stream uses the same max_wait_time.
On 2014/10/15 19:05:11, DaleCurtis wrote: > Can you just send them a build with 128/2 sized delay and see if it resolves the > issue? Before M38, WebRTC uses 128 as buffer size to open the stream, there the max_wait_time is 128/2, and the performance was worse there. We had some email discussion, I will forward it to you.
It sounds like the root problem is that your processing is taking too long in some cases, not whatever buffer size WebAudio puts into effect. Perhaps some signal can be sent to indicate that a large number of misses are occurring and processing should be reduced to ensure on-time delivery.
On 2014/10/15 20:13:32, DaleCurtis wrote: > It sounds like the root problem is that your processing is taking too long in > some cases, not whatever buffer size WebAudio puts into effect. Perhaps some > signal can be sent to indicate that a large number of misses are occurring and > processing should be reduced to ensure on-time delivery. There actually can be various reasons on why renderer could not deliver data in time, like CPU is high, certain thread(s) take all the CPU resource at certain time, webrtc renderer client takes too much time (as you said), the possibilities are there, but it is difficult to conclude which one(s) are the root cause. In b/17298528, we actually verified that the glitches went away if we change the webaudio buffer size to 10ms, see comments #93 and comments #103. Probably we should have another VC tomorrow or the day after tomorrow.
I'm not okay with any kind "if (buffer_size == 10ms) do_magic()". Things get tricky if we let the hardware size be greater than the shared memory size. I.e., if the OS is outputting 512 frames and the shared memory is sized for 128, then that means 4 back to back calls for data to WebAudio. Is the problem you're trying to fix the one where WebRTC is created, then WebAudio is created? If so is there some way to flag WebRTC as being active and then report 10ms as the minimum buffer size via preferred output parameters? Another path forward is through the eventual resolution of https://github.com/WebAudio/web-audio-api/issues/348 and allow WebAudio contexts to specify some sort of buffer size constraints.
I think your second paragraph accurately describes the problem and what we would like to do. As you know, the buffer size for WebAudio was chosen as the lowest glitch free size that would work well in DAW like scenarios. We don't want to change that. What we're seeing is that if the application is additionally using WebRTC (processing audio, encoding/decoding video + all sorts of other things) and processing video frames in a nacl plugin via ppapi (which needs to be optimized), then we start to get glitches on mb air (2+yr old). Anecdotally I've also heard about this happen on CrOS - I don't know for sure if it's the same issue but iirc we do the same thing there so I'm guessing it might be related. On Wed, Oct 15, 2014, 23:14 null <dalecurtis@chromium.org> wrote: > I'm not okay with any kind "if (buffer_size == 10ms) do_magic()". Things > get > tricky if we let the hardware size be greater than the shared memory size. > I.e., > if the OS is outputting 512 frames and the shared memory is sized for 128, > then > that means 4 back to back calls for data to WebAudio. > > Is the problem you're trying to fix the one where WebRTC is created, then > WebAudio is created? If so is there some way to flag WebRTC as being active > and > then report 10ms as the minimum buffer size via preferred output > parameters? > > Another path forward is through the eventual resolution of > https://github.com/WebAudio/web-audio-api/issues/348 and allow WebAudio > contexts > to specify some sort of buffer size constraints. > > > https://codereview.chromium.org/661443002/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
xians@chromium.org changed reviewers: + henrika@chromium.org
On 2014/10/15 21:14:00, DaleCurtis wrote: > I'm not okay with any kind "if (buffer_size == 10ms) do_magic()". Things get > tricky if we let the hardware size be greater than the shared memory size. I.e., > if the OS is outputting 512 frames and the shared memory is sized for 128, then > that means 4 back to back calls for data to WebAudio. I agree that this is not perfect because we are relying on the ReceiveWithTimeout() to break the back-to-back calls. But the true is that probably only us use webaudio + webrtc (output stream) at the same time > > Is the problem you're trying to fix the one where WebRTC is created, then > WebAudio is created? If so is there some way to flag WebRTC as being active and > then report 10ms as the minimum buffer size via preferred output parameters? > I think current situation is opposite, we create webaudio stream first, then create webrtc output stream. Anyway, relying on the orders of the streams' creation is not reliable, I don't think it is the right approach either. > Another path forward is through the eventual resolution of > https://github.com/WebAudio/web-audio-api/issues/348 and allow WebAudio contexts > to specify some sort of buffer size constraints. This will fix all the problems when it is implemented, but I am afraid the standard battle will take a long time. We can't wait for it since our users are complaining. Another way to fix the problem is to increase the default buffer size for WebAudio to 256, this solution won't be nasty but will affect all the webaudio users. While "if (buffer_size == 10ms) do_magic()" only affect the our use case. I am actually open to a better solution if there is any.
Remember any buffer size change affects the output size across the entire browser, not just whatever tab hangouts is running in. So any user who has a WebAudio application in another tab will be affected by increasing the buffer size. Likewise WebRTC is affected by any WebAudio or PPAPI streams being created at a lower buffer size. I'd be supportive of switching to 256 for the minimum OSX buffer size and letting power users override that with the --audio-buffer-size flag. It all depends on how much of an impact that would have for WebAudio. We've long had complaints of 128 being too low for lower powered machines, so it may be the best of all worlds.
On 2014/10/16 19:47:53, DaleCurtis wrote: > Remember any buffer size change affects the output size across the entire > browser, not just whatever tab hangouts is running in. > True, though AFAIK very few users open a tab for webaudio while using webrtc on another tab. > So any user who has a WebAudio application in another tab will be affected by > increasing the buffer size. Likewise WebRTC is affected by any WebAudio or > PPAPI streams being created at a lower buffer size. > It is also true, but I have to point out the negative impact of a smaller buffer size for WebRTC and PPAPI users are largely surpass the impact for WebAudio from a larger buffer size. Since for WebRTC and PPAPI users, they have glitches on their sound when buffer size gets smaller. For WebAudio users, they get a few ms more latency and some potential glitches (not verified though) when using webrtc at the same time. > I'd be supportive of switching to 256 for the minimum OSX buffer size and > letting power users override that with the --audio-buffer-size flag. It all > depends on how much of an impact that would have for WebAudio. We've long had > complaints of 128 being too low for lower powered machines, so it may be the > best of all worlds. I know I proposed this, but honestly this is the last approach I want to do, since I am big fan of low latency webaudio. It is just so cool for webaudio to provide a loopback latency like 15-20ms. I am going to discuss with Tommi and Henrik, will come back to you soon.
A second though, maybe we can change the buffer size to 256 for all sample rates including 96K. Most of webrtc users will use 44.1K or 48K. For WebAudio users who want extremely low buffer size, they can set the sample rate to 96K, which is still equivalent to about 3ms.
https://codereview.chromium.org/661443002/diff/1/content/browser/renderer_hos... File content/browser/renderer_host/media/audio_sync_reader.cc (left): https://codereview.chromium.org/661443002/diff/1/content/browser/renderer_hos... content/browser/renderer_host/media/audio_sync_reader.cc:143: while (timeout.InMicroseconds() > 0) { Dale, besides the buffer size issue, I think we still have concern on these code, is it the right thing to do to wait for the index synchronization even though the browser has got data to proceed? If you think it is, please explain why should we wait instead of passing the data to the browser?
96kHz is already set to 256 frames: https://code.google.com/p/chromium/codesearch#chromium/src/media/audio/mac/au... https://codereview.chromium.org/661443002/diff/1/content/browser/renderer_hos... File content/browser/renderer_host/media/audio_sync_reader.cc (left): https://codereview.chromium.org/661443002/diff/1/content/browser/renderer_hos... content/browser/renderer_host/media/audio_sync_reader.cc:143: while (timeout.InMicroseconds() > 0) { On 2014/10/16 20:27:56, xians1 wrote: > Dale, besides the buffer size issue, I think we still have concern on these > code, is it the right thing to do to wait for the index synchronization even > though the browser has got data to proceed? > If you think it is, please explain why should we wait instead of passing the > data to the browser? I don't understand what you're asking. The loop breaks once the right index is received. If we don't drain the sync socket and the renderer is behind by more than one buffer, we will never get the right index again.
I had forgotten about the --audio-buffer-size escape hatch. I think we should run experiments with setting the default buffer size to 256 like Dale suggests. If that fixes the glitches we've been hearing, then we should just do that. Regarding 96kHz and 256 frames; the problem we've been seeing has been due to high CPU loads. Won't a buffer size of 256@96kHz cause callbacks at the same rate as we'd get with 128@48kHz (and thus we'd be just as likely to get glitches)? If so, should that default then be changed to 512 frames? In any case, I just checked the stats and it looks like that's a corner case. On 2014/10/16 21:14:14, DaleCurtis wrote: > 96kHz is already set to 256 frames: > > https://code.google.com/p/chromium/codesearch#chromium/src/media/audio/mac/au... > > https://codereview.chromium.org/661443002/diff/1/content/browser/renderer_hos... > File content/browser/renderer_host/media/audio_sync_reader.cc (left): > > https://codereview.chromium.org/661443002/diff/1/content/browser/renderer_hos... > content/browser/renderer_host/media/audio_sync_reader.cc:143: while > (timeout.InMicroseconds() > 0) { > On 2014/10/16 20:27:56, xians1 wrote: > > Dale, besides the buffer size issue, I think we still have concern on these > > code, is it the right thing to do to wait for the index synchronization even > > though the browser has got data to proceed? > > If you think it is, please explain why should we wait instead of passing the > > data to the browser? > > I don't understand what you're asking. The loop breaks once the right index is > received. If we don't drain the sync socket and the renderer is behind by more > than one buffer, we will never get the right index again.
I will update the issue and ask that users who can reproduce try with 256.
Initial feedback indicates that 256 does work. "I made a quick test with the 256 buffer size and it worked fine. I would like to redo it though with a 128 comparison done in the same situation before giving 256 a thumbs up." |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
