Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2738403002:
AudioParameters::UnavailableDeviceParameters buffer size 100 ms -> 10 ms (Closed)
DescriptionCL https://codereview.chromium.org/2692203003/ makes the code to use UnavailableDeviceParameters in case there are no real output devices available.
WebAudio is unhappy when 100 ms buffer size is used for audio sink.
(CL got reverted because of that). Switching to 10 ms.
BUG=672468, 701000
Review-Url: https://codereview.chromium.org/2738403002
Cr-Commit-Position: refs/heads/master@{#456464}
Committed: https://chromium.googlesource.com/chromium/src/+/f85f4d03170997f6d8fbf843e647e58a3923fd9e
Patch Set 1 #
Total comments: 1
Patch Set 2 : Comment #Patch Set 3 : comment updated #Messages
Total messages: 40 (23 generated)
The CQ bit was checked by olka@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.
Description was changed from ========== fix BUG= ========== to ========== CL https://codereview.chromium.org/2692203003/ makes the code to use UnavailableDeviceParameters in case there are no real output devices available. WebAudio is unhappy when 100 ms buffer size is used for audio sink. (CL got reverted because of that). Switching to 10 ms. BUG=672468 ==========
olka@chromium.org changed reviewers: + tommi@chromium.org
Lgtm, one request. https://codereview.chromium.org/2738403002/diff/1/media/base/audio_parameters.cc File media/base/audio_parameters.cc (right): https://codereview.chromium.org/2738403002/diff/1/media/base/audio_parameters... media/base/audio_parameters.cc:109: media::AudioParameters::kAudioCDSampleRate / 100); Since there was an issue with this, can you add a comment?
The CQ bit was checked by olka@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tommi@chromium.org Link to the patchset: https://codereview.chromium.org/2738403002/#ps20001 (title: "Comment")
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
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by olka@chromium.org
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
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
olka@chromium.org changed reviewers: + dalecurtis@chromium.org
Oh, so tommi@ is not an owner... dalecurtis@ could you PTAL?
dalecurtis@chromium.org changed reviewers: + rtoy@chromium.org
Seems like this is a bug in WebAudio then? As part of the buffering size work the WebAudio team is working on they want to support arbitrary buffer sizes; so +rtoy to comment.
On 2017/03/10 18:16:50, DaleCurtis wrote: > Seems like this is a bug in WebAudio then? As part of the buffering size work > the WebAudio team is working on they want to support arbitrary buffer sizes; so > +rtoy to comment. It's not exactly WebAudio problem, it's reference time calculation in WebAudioMediaStreamSource (I'll write up the details in the mail thread I started earlier and I'll file a bug, just did not have time for it today). Usually WebAudio operates on buffer sizes close to/lower than 10 ms, and that calculation problem does not show up. But if we pull the graph 100 Ms at a time, reference time calculation is off, because it assumes that pulls are distributed evenly in time, but in fact we do many pulls side by side to fill up 100 Ms buffer.
Ah, so the problem is the back-to-back pulls? Can you elaborate more on this in the commit description then?
Just checking - has there been some regression wrt latency for webaudio? On Fri, Mar 10, 2017, 19:34 <olka@chromium.org> wrote: > On 2017/03/10 18:16:50, DaleCurtis wrote: > > Seems like this is a bug in WebAudio then? As part of the buffering size > work > > the WebAudio team is working on they want to support arbitrary buffer > sizes; > so > > +rtoy to comment. > > It's not exactly WebAudio problem, it's reference time calculation in > WebAudioMediaStreamSource (I'll write up the details in the mail thread I > started earlier and I'll file a bug, just did not have time for it today). > Usually WebAudio operates on buffer sizes close to/lower than 10 ms, and > that > calculation problem does not show up. But if we pull the graph 100 Ms at a > time, > reference time calculation is off, because it assumes that pulls are > distributed > evenly in time, but in fact we do many pulls side by side to fill up 100 Ms > buffer. > > > https://codereview.chromium.org/2738403002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2017/03/10 18:38:13, tommi - chröme wrote: > Just checking - has there been some regression wrt latency for webaudio? +hongchan for more info. The webaudio FIFO has changed recently and we've just discovered one possible issue (only seen so far on Android). > > On Fri, Mar 10, 2017, 19:34 <mailto:olka@chromium.org> wrote: > > > On 2017/03/10 18:16:50, DaleCurtis wrote: > > > Seems like this is a bug in WebAudio then? As part of the buffering size > > work > > > the WebAudio team is working on they want to support arbitrary buffer > > sizes; > > so > > > +rtoy to comment. > > > > It's not exactly WebAudio problem, it's reference time calculation in > > WebAudioMediaStreamSource (I'll write up the details in the mail thread I > > started earlier and I'll file a bug, just did not have time for it today). > > Usually WebAudio operates on buffer sizes close to/lower than 10 ms, and > > that > > calculation problem does not show up. But if we pull the graph 100 Ms at a > > time, > > reference time calculation is off, because it assumes that pulls are > > distributed > > evenly in time, but in fact we do many pulls side by side to fill up 100 Ms > > buffer. > > > > > > https://codereview.chromium.org/2738403002/ > > > > -- > You received this message because you are subscribed to the Google Groups > "Chromium-reviews" group. > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org.
rtoy@chromium.org changed reviewers: + hongchan@chromium.org
+hongchan for real
Description was changed from ========== CL https://codereview.chromium.org/2692203003/ makes the code to use UnavailableDeviceParameters in case there are no real output devices available. WebAudio is unhappy when 100 ms buffer size is used for audio sink. (CL got reverted because of that). Switching to 10 ms. BUG=672468 ========== to ========== CL https://codereview.chromium.org/2692203003/ makes the code to use UnavailableDeviceParameters in case there are no real output devices available. WebAudio is unhappy when 100 ms buffer size is used for audio sink. (CL got reverted because of that). Switching to 10 ms. BUG=672468,701000 ==========
The CQ bit was checked by olka@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 details are both in the mail trhead and in https://bugs.chromium.org/p/chromium/issues/detail?id=701000. Updated the comment - PTAL
lgtm, thanks for the additional comments.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by olka@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tommi@chromium.org Link to the patchset: https://codereview.chromium.org/2738403002/#ps40001 (title: "comment updated")
On 2017/03/13 19:11:06, DaleCurtis wrote: > lgtm, thanks for the additional comments. Thanks!
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": 40001, "attempt_start_ts": 1489434710265470,
"parent_rev": "35d9006b00547065e01b7e17cd9871d67deba206", "commit_rev":
"f85f4d03170997f6d8fbf843e647e58a3923fd9e"}
Message was sent while issue was closed.
Description was changed from ========== CL https://codereview.chromium.org/2692203003/ makes the code to use UnavailableDeviceParameters in case there are no real output devices available. WebAudio is unhappy when 100 ms buffer size is used for audio sink. (CL got reverted because of that). Switching to 10 ms. BUG=672468,701000 ========== to ========== CL https://codereview.chromium.org/2692203003/ makes the code to use UnavailableDeviceParameters in case there are no real output devices available. WebAudio is unhappy when 100 ms buffer size is used for audio sink. (CL got reverted because of that). Switching to 10 ms. BUG=672468,701000 Review-Url: https://codereview.chromium.org/2738403002 Cr-Commit-Position: refs/heads/master@{#456464} Committed: https://chromium.googlesource.com/chromium/src/+/f85f4d03170997f6d8fbf843e647... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/f85f4d03170997f6d8fbf843e647... |
