Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
11889041:
Fix VirtualAudioInputStream callback timing issues. (Closed)
|
Created:
7 years, 11 months ago by justinlin Modified:
7 years, 11 months ago CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, jam, feature-media-reviews_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionFix VirtualAudioInputStream callback timing issues.
- While mirroring is activated, the VirtualAudioInputStream drives the audio, so we need to account
for the time spent in AudioConverters as well as imprecisions from PostDelayedTask when scheduling callbacks to not slow video playback down.
- Need to use microseconds for the buffer size because for 44.1kHz, we are asked to use a 440 frame buffer.
BUG=168532
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=177570
Patch Set 1 #
Total comments: 4
Patch Set 2 : comments #
Total comments: 10
Patch Set 3 : Increase buffer size to 2048 #Patch Set 4 : update comment #
Total comments: 1
Patch Set 5 : remove buffer size change #
Total comments: 2
Patch Set 6 : Use microseconds for buffer length because for some reason input params specify a 440 frame buffer … #
Total comments: 2
Patch Set 7 : nit #
Messages
Total messages: 22 (0 generated)
https://codereview.chromium.org/11889041/diff/1/content/renderer/media/audio_... File content/renderer/media/audio_renderer_mixer_manager.cc (right): https://codereview.chromium.org/11889041/diff/1/content/renderer/media/audio_... content/renderer/media/audio_renderer_mixer_manager.cc:63: hardware_sample_rate_, 16, 2048); You'll need to test this on mac/win and make sure there are no issues with HTML5 audio/video. Specifically playback must continue to be smooth on say http://xorax.sea/snowboard.mp4 https://codereview.chromium.org/11889041/diff/1/media/audio/virtual_audio_inp... File media/audio/virtual_audio_input_stream.cc (right): https://codereview.chromium.org/11889041/diff/1/media/audio/virtual_audio_inp... media/audio/virtual_audio_input_stream.cc:146: if (delay < base::TimeDelta()) I'd worry about drift over time here. Can you limit this calculation to the delta between ReadAudio() calls? I.e. reset next_read_time_ based on base::Time::Now() in each call.
https://codereview.chromium.org/11889041/diff/1/content/renderer/media/audio_... File content/renderer/media/audio_renderer_mixer_manager.cc (right): https://codereview.chromium.org/11889041/diff/1/content/renderer/media/audio_... content/renderer/media/audio_renderer_mixer_manager.cc:63: hardware_sample_rate_, 16, 2048); On 2013/01/15 21:00:08, DaleCurtis wrote: > You'll need to test this on mac/win and make sure there are no issues with HTML5 > audio/video. Specifically playback must continue to be smooth on say > http://xorax.sea/snowboard.mp4 OK, will test. https://codereview.chromium.org/11889041/diff/1/media/audio/virtual_audio_inp... File media/audio/virtual_audio_input_stream.cc (right): https://codereview.chromium.org/11889041/diff/1/media/audio/virtual_audio_inp... media/audio/virtual_audio_input_stream.cc:146: if (delay < base::TimeDelta()) On 2013/01/15 21:00:08, DaleCurtis wrote: > I'd worry about drift over time here. Can you limit this calculation to the > delta between ReadAudio() calls? I.e. reset next_read_time_ based on > base::Time::Now() in each call. I tested this and actually the comment I left there isn't quite right. PostDelayedTask actually contributes to the imprecision as well. I think a combination of the time spent in audio conversions and the imprecision of PostDelayedTask pushes it just over the edge to produce a pretty noticeable framerate drop (i.e. the posttask delay ends up fluctuating between 8 and 9ms on my machine if we do account for both things). Accounting for only the audio conversions is not enough. As for drift towards the delay getting shorter and shorter and eventually using up all the cpu, how about we just reset the next read time in that case. We'll get a video slow down but at least it won't end up spinning.
https://codereview.chromium.org/11889041/diff/8001/content/renderer/media/aud... File content/renderer/media/audio_renderer_mixer_manager.cc (right): https://codereview.chromium.org/11889041/diff/8001/content/renderer/media/aud... content/renderer/media/audio_renderer_mixer_manager.cc:56: #if defined(OS_WIN) || defined(OS_MACOSX) Remove #if, since this would seem to apply to all platforms. Or, am I missing something? https://codereview.chromium.org/11889041/diff/8001/media/audio/virtual_audio_... File media/audio/virtual_audio_input_stream.cc (right): https://codereview.chromium.org/11889041/diff/8001/media/audio/virtual_audio_... media/audio/virtual_audio_input_stream.cc:143: // Need to account for time spent here due to renderer side mixing as well as You'll want to do almost exactly what's in content/browser/renderer_host/media/web_contents_video_capture_device.cc, lines 888 through 908. I went through a few iterations of this scheduling problem, and I believe what's true for video is true for audio here. In particular: 1. The goal is to target regular intervals of time. Missed tasks shouldn't recover by immediately posting a task with zero delay. Instead, just schedule for the next one to take place when it would have normally occurred. This should be better for the downstream code to keep audio and video in-sync. 2. Accounts for clock skew and major system time changes.
https://codereview.chromium.org/11889041/diff/8001/content/renderer/media/aud... File content/renderer/media/audio_renderer_mixer_manager.cc (right): https://codereview.chromium.org/11889041/diff/8001/content/renderer/media/aud... content/renderer/media/audio_renderer_mixer_manager.cc:58: // changes. WebRTC audio mirroring will use the input device's buffer size to WebRTC doesn't use this code?
https://codereview.chromium.org/11889041/diff/8001/content/renderer/media/aud... File content/renderer/media/audio_renderer_mixer_manager.cc (right): https://codereview.chromium.org/11889041/diff/8001/content/renderer/media/aud... content/renderer/media/audio_renderer_mixer_manager.cc:56: #if defined(OS_WIN) || defined(OS_MACOSX) On 2013/01/15 23:28:35, Yuri wrote: > Remove #if, since this would seem to apply to all platforms. Or, am I > missing something? Renderer side mixing is only turned on for OSX/Win right now, so you won't see any of these problems on Linux yet. https://codereview.chromium.org/11889041/diff/8001/content/renderer/media/aud... content/renderer/media/audio_renderer_mixer_manager.cc:58: // changes. WebRTC audio mirroring will use the input device's buffer size to On 2013/01/15 23:30:03, DaleCurtis wrote: > WebRTC doesn't use this code? Done. We'd typically use audio mirroring with webrtc, but right it's not directly tied to that. https://codereview.chromium.org/11889041/diff/8001/media/audio/virtual_audio_... File media/audio/virtual_audio_input_stream.cc (right): https://codereview.chromium.org/11889041/diff/8001/media/audio/virtual_audio_... media/audio/virtual_audio_input_stream.cc:143: // Need to account for time spent here due to renderer side mixing as well as On 2013/01/15 23:28:35, Yuri wrote: > You'll want to do almost exactly what's in > content/browser/renderer_host/media/web_contents_video_capture_device.cc, lines > 888 through 908. I went through a few iterations of this scheduling problem, > and I believe what's true for video is true for audio here. In particular: > > 1. The goal is to target regular intervals of time. Missed tasks shouldn't > recover by immediately posting a task with zero delay. Instead, just schedule > for the next one to take place when it would have normally occurred. This > should be better for the downstream code to keep audio and video in-sync. > > 2. Accounts for clock skew and major system time changes. The problem is there isn't a mechanism to skip ahead like you could can when capturing snapshots. You have to read all the frames here, so if we get behind, we have to catch up by scheduling the next callback sooner. This could degenerate (I think that was Dale's concern), so in that case we just schedule it at the regular time, but video/audio will end up slowing down, but everything would probably be already pretty slow in that case.
Tested on Windows and Mac with some of the webm files in that xorax.sea directory and video played back remained as smooth as in Canary Chrome from what I could tell. I picked 1024 as the size because the default mic input sample rate on mac is 96000hz, so we need at least a 960 frame buffer size, and I'm not sure users will usually have mics that go higher than that. PTAL. On 2013/01/15 23:52:45, justinlin wrote: > https://codereview.chromium.org/11889041/diff/8001/content/renderer/media/aud... > File content/renderer/media/audio_renderer_mixer_manager.cc (right): > > https://codereview.chromium.org/11889041/diff/8001/content/renderer/media/aud... > content/renderer/media/audio_renderer_mixer_manager.cc:56: #if defined(OS_WIN) > || defined(OS_MACOSX) > On 2013/01/15 23:28:35, Yuri wrote: > > Remove #if, since this would seem to apply to all platforms. Or, am I > > missing something? > > Renderer side mixing is only turned on for OSX/Win right now, so you won't see > any of these problems on Linux yet. > > https://codereview.chromium.org/11889041/diff/8001/content/renderer/media/aud... > content/renderer/media/audio_renderer_mixer_manager.cc:58: // changes. WebRTC > audio mirroring will use the input device's buffer size to > On 2013/01/15 23:30:03, DaleCurtis wrote: > > WebRTC doesn't use this code? > > Done. We'd typically use audio mirroring with webrtc, but right it's not > directly tied to that. > > https://codereview.chromium.org/11889041/diff/8001/media/audio/virtual_audio_... > File media/audio/virtual_audio_input_stream.cc (right): > > https://codereview.chromium.org/11889041/diff/8001/media/audio/virtual_audio_... > media/audio/virtual_audio_input_stream.cc:143: // Need to account for time spent > here due to renderer side mixing as well as > On 2013/01/15 23:28:35, Yuri wrote: > > You'll want to do almost exactly what's in > > content/browser/renderer_host/media/web_contents_video_capture_device.cc, > lines > > 888 through 908. I went through a few iterations of this scheduling problem, > > and I believe what's true for video is true for audio here. In particular: > > > > 1. The goal is to target regular intervals of time. Missed tasks shouldn't > > recover by immediately posting a task with zero delay. Instead, just schedule > > for the next one to take place when it would have normally occurred. This > > should be better for the downstream code to keep audio and video in-sync. > > > > 2. Accounts for clock skew and major system time changes. > > The problem is there isn't a mechanism to skip ahead like you could can when > capturing snapshots. You have to read all the frames here, so if we get behind, > we have to catch up by scheduling the next callback sooner. This could > degenerate (I think that was Dale's concern), so in that case we just schedule > it at the regular time, but video/audio will end up slowing down, but everything > would probably be already pretty slow in that case.
https://codereview.chromium.org/11889041/diff/8001/content/renderer/media/aud... File content/renderer/media/audio_renderer_mixer_manager.cc (right): https://codereview.chromium.org/11889041/diff/8001/content/renderer/media/aud... content/renderer/media/audio_renderer_mixer_manager.cc:56: #if defined(OS_WIN) || defined(OS_MACOSX) On 2013/01/15 23:52:45, justinlin wrote: > On 2013/01/15 23:28:35, Yuri wrote: > > Remove #if, since this would seem to apply to all platforms. Or, am I > > missing something? > > Renderer side mixing is only turned on for OSX/Win right now, so you won't see > any of these problems on Linux yet. It's a flag for multiple platforms, enabled by default for mac/win, and disabled (but can be enabled) in linux. Reference: http://code.google.com/searchframe#OAMlx_jo-ck/src/content/renderer/render_vi... Perhaps Dale could chime in on the right approach here? https://codereview.chromium.org/11889041/diff/8001/media/audio/virtual_audio_... File media/audio/virtual_audio_input_stream.cc (right): https://codereview.chromium.org/11889041/diff/8001/media/audio/virtual_audio_... media/audio/virtual_audio_input_stream.cc:143: // Need to account for time spent here due to renderer side mixing as well as On 2013/01/15 23:52:45, justinlin wrote: > The problem is there isn't a mechanism to skip ahead like you could can when > capturing snapshots. You have to read all the frames here, so if we get behind, > we have to catch up by scheduling the next callback sooner. This could > degenerate (I think that was Dale's concern), so in that case we just schedule > it at the regular time, but video/audio will end up slowing down, but everything > would probably be already pretty slow in that case. Okay. I guess it doesn't matter either way, then. I'm fine with the way you've got it here. BTW--Now you got me thinking of a new, killer feature for Chrome: A way to playback all media faster simply by pulling audio at a faster rate. ;-) I don't know about you, but I'd love to watch Youtube videos at 115-120% of normal speed. I used to do this with a homebrew DVR I put together, and watched so much more TV in so much less time.
Did you test with 192kHz output? I'd worry that 1024 might not work well in that case. Yuri, you can already set the playback rate on YouTube videos if you're using the html5 player :)
Thanks, you're right, it didn't work well with 192kHz. We need a buffer size of 2048. Local playback was still smooth (watched avatar and buck). PTAL. https://codereview.chromium.org/11889041/diff/8001/content/renderer/media/aud... File content/renderer/media/audio_renderer_mixer_manager.cc (right): https://codereview.chromium.org/11889041/diff/8001/content/renderer/media/aud... content/renderer/media/audio_renderer_mixer_manager.cc:56: #if defined(OS_WIN) || defined(OS_MACOSX) On 2013/01/16 03:43:03, Yuri wrote: > On 2013/01/15 23:52:45, justinlin wrote: > > On 2013/01/15 23:28:35, Yuri wrote: > > > Remove #if, since this would seem to apply to all platforms. Or, am I > > > missing something? > > > > Renderer side mixing is only turned on for OSX/Win right now, so you won't see > > any of these problems on Linux yet. > > It's a flag for multiple platforms, enabled by default for mac/win, and disabled > (but can be enabled) in linux. > > Reference: > http://code.google.com/searchframe#OAMlx_jo-ck/src/content/renderer/render_vi... > > Perhaps Dale could chime in on the right approach here? I added a TODO to do this if we don't get to renderer side device changes + some mirroring code changes first (we would be able to update params on renderer side too in that case, and I think we can avoid resampling altogether). https://codereview.chromium.org/11889041/diff/8001/media/audio/virtual_audio_... File media/audio/virtual_audio_input_stream.cc (right): https://codereview.chromium.org/11889041/diff/8001/media/audio/virtual_audio_... media/audio/virtual_audio_input_stream.cc:143: // Need to account for time spent here due to renderer side mixing as well as On 2013/01/16 03:43:03, Yuri wrote: > On 2013/01/15 23:52:45, justinlin wrote: > > The problem is there isn't a mechanism to skip ahead like you could can when > > capturing snapshots. You have to read all the frames here, so if we get > behind, > > we have to catch up by scheduling the next callback sooner. This could > > degenerate (I think that was Dale's concern), so in that case we just schedule > > it at the regular time, but video/audio will end up slowing down, but > everything > > would probably be already pretty slow in that case. > > Okay. I guess it doesn't matter either way, then. I'm fine with the way you've > got it here. > > BTW--Now you got me thinking of a new, killer feature for Chrome: A way to > playback all media faster simply by pulling audio at a faster rate. ;-) I > don't know about you, but I'd love to watch Youtube videos at 115-120% of normal > speed. I used to do this with a homebrew DVR I put together, and watched so > much more TV in so much less time. Yea, cool idea :).
lgtm
On 2013/01/16 21:06:30, Yuri wrote: > lgtm For the record what kind of latency increase does this cause? You can check this by building the pyautolib target and running ./chrome/test/functional/media/audio_latency_perf.py
Hm.. these are the results I get: Macbook Pro (@44.1kHz): 11ms -> 138ms Windows Desktop (@44.1kHz: 75ms -> 185ms Is this an acceptable latency increase for html5 video? On 2013/01/16 21:37:25, DaleCurtis wrote: > On 2013/01/16 21:06:30, Yuri wrote: > > lgtm > > For the record what kind of latency increase does this cause? You can check this > by building the pyautolib target and running > ./chrome/test/functional/media/audio_latency_perf.py
https://codereview.chromium.org/11889041/diff/19004/content/renderer/media/au... File content/renderer/media/audio_renderer_mixer_manager.cc (right): https://codereview.chromium.org/11889041/diff/19004/content/renderer/media/au... content/renderer/media/audio_renderer_mixer_manager.cc:64: buffer_size = std::max(hardware_buffer_size_, 2048); Why not make this a function of hardware_sample_rate_? Meaning, assume we want a 10ms buffer, then: buffer_size = std::max(hardware_buffer_size_, hardware_sample_rate_ / 100);
Hmm, that's a lot larger than I thought it would be. An alternate solution is to reenable blocking shared memory. Crap. I'll need to discuss with Chris, we've got a couple issues around non-blocking shared memory and I'm starting to think it's not worth the trouble anymore.
On 2013/01/16 23:09:23, DaleCurtis wrote: > Hmm, that's a lot larger than I thought it would be. An alternate solution is to > reenable blocking shared memory. Crap. I'll need to discuss with Chris, we've > got a couple issues around non-blocking shared memory and I'm starting to think > it's not worth the trouble anymore. OK, yea it did seem like a large increase. Blocking shared memory would work too. Would that be easy to merge into M25? I can start to look at the renderer side device changes since that seems like really the best way overall. I removed the buffer size change from this CL, could you ptal the other part? We'd need that fix too anyway.
On Wed, Jan 16, 2013 at 11:06 AM, <dalecurtis@chromium.org> wrote: > Yuri, you can already set the playback rate on YouTube videos if you're > using > the html5 player :) > Ah! Just switched the dogfood on. Absolutely <3 this feature! Wish they had 1.1X, 1.25X and 1.75X choice as well. Will provide feedback... -Yuri
Well, we've only done blocking shared memory on Windows, I don't think we'll add it for Mac. I wonder if there are other solutions we can consider here. Otherwise we can try merging the buffer change and living with M25 having bad latency for Html5. https://codereview.chromium.org/11889041/diff/19007/media/audio/virtual_audio... File media/audio/virtual_audio_input_stream.cc (right): https://codereview.chromium.org/11889041/diff/19007/media/audio/virtual_audio... media/audio/virtual_audio_input_stream.cc:151: next_read_time_ = base::Time::Now(); I'd save base::Time::Now() from the first call instead of calling it twice. This still seems like it might be prone to cycles. Have you tried logging how frequently callbacks fall behind?
Sorry for the delay. Ran into another bug while verifying. For some reason videos were playing slightly faster when I set my system settings to 44.1kHz. Turns out it's because we're asked to use a 440 frame buffer (because of webRTC?), so the division in VAIS to get the buffer duration ends up truncating and we end up posting tasks every 9ms, when it should be every ~9.9ms. PTAL. https://codereview.chromium.org/11889041/diff/19007/media/audio/virtual_audio... File media/audio/virtual_audio_input_stream.cc (right): https://codereview.chromium.org/11889041/diff/19007/media/audio/virtual_audio... media/audio/virtual_audio_input_stream.cc:151: next_read_time_ = base::Time::Now(); On 2013/01/17 00:10:51, DaleCurtis wrote: > I'd save base::Time::Now() from the first call instead of calling it twice. This > still seems like it might be prone to cycles. Have you tried logging how > frequently callbacks fall behind? Done. Updated this: delay should be set to the buffer duration (i.e. we'll just schedule it at normal time if we're really behind). Yea, I put logging to see how often this happens and it's pretty much almost always behind by a little. Roughly ~200us due to posttask's imprecision and ~400us-ish for the time in the audio conversions.
lgtm % naming nit. https://codereview.chromium.org/11889041/diff/26001/media/audio/virtual_audio... File media/audio/virtual_audio_input_stream.h (right): https://codereview.chromium.org/11889041/diff/26001/media/audio/virtual_audio... media/audio/virtual_audio_input_stream.h:79: base::TimeDelta buffer_duration_us_; As a time delta this doesn't have units.
https://codereview.chromium.org/11889041/diff/26001/media/audio/virtual_audio... File media/audio/virtual_audio_input_stream.h (right): https://codereview.chromium.org/11889041/diff/26001/media/audio/virtual_audio... media/audio/virtual_audio_input_stream.h:79: base::TimeDelta buffer_duration_us_; On 2013/01/17 22:34:21, DaleCurtis wrote: > As a time delta this doesn't have units. Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/justinlin@chromium.org/11889041/33001 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
