Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2469023002:
Support floating-point audio output for Linux (Closed)
|
Created:
4 years, 1 month ago by Raymond Toy Modified:
4 years, 1 month ago Reviewers:
DaleCurtis CC:
chromium-reviews, darin-cc_chromium.org, feature-media-reviews_chromium.org, jam, mlamouri+watch-content_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionSupport floating-point audio output for Linux
Instead of converting the audio to 16-bit PCM, pass the audio data
directly to Pulse Audio as floating-point numbers and let Pulse Audio
do whatever is necessary.
Using the test url, a gain of 0.00002 produces output on my Z620 where
it didn't before. There is silence for a gain of 0.00001, but that
could be due to many things including pulse audio, the actual audio
driver, the audio HW, and my headphones.
BUG=659641
TEST=Run test url from bug and set gain to 0.00002 and hear output
Committed: https://crrev.com/9017c72f75614b18f1af80e90c77d172413644bc
Cr-Commit-Position: refs/heads/master@{#429301}
Patch Set 1 #
Total comments: 5
Patch Set 2 : Implement correctly #
Total comments: 10
Patch Set 3 : Address review comments #
Messages
Total messages: 19 (6 generated)
dalecurtis@chromium.org changed reviewers: + dalecurtis@chromium.org
https://codereview.chromium.org/2469023002/diff/1/content/renderer/renderer_b... File content/renderer/renderer_blink_platform_impl.cc (right): https://codereview.chromium.org/2469023002/diff/1/content/renderer/renderer_b... content/renderer/renderer_blink_platform_impl.cc:751: layout, static_cast<int>(sample_rate), 32, Drop this change. I can't guarantee it'll plumb properly. Instead set it in pulse_output/pulse_input. https://codereview.chromium.org/2469023002/diff/1/media/audio/pulse/pulse_out... File media/audio/pulse/pulse_output.cc (right): https://codereview.chromium.org/2469023002/diff/1/media/audio/pulse/pulse_out... media/audio/pulse/pulse_output.cc:155: audio_bus_->frames(), reinterpret_cast<float*>(buffer)); is buffer allocated correctly? https://codereview.chromium.org/2469023002/diff/1/media/audio/pulse/pulse_uti... File media/audio/pulse/pulse_util.cc (right): https://codereview.chromium.org/2469023002/diff/1/media/audio/pulse/pulse_uti... media/audio/pulse/pulse_util.cc:182: #if 1 #if 0?
https://codereview.chromium.org/2469023002/diff/1/media/audio/pulse/pulse_uti... File media/audio/pulse/pulse_util.cc (right): https://codereview.chromium.org/2469023002/diff/1/media/audio/pulse/pulse_uti... media/audio/pulse/pulse_util.cc:182: #if 1 On 2016/11/01 18:51:37, DaleCurtis wrote: > #if 0? This is for the input stream. I don't want to change that yet.
https://codereview.chromium.org/2469023002/diff/1/content/renderer/renderer_b... File content/renderer/renderer_blink_platform_impl.cc (right): https://codereview.chromium.org/2469023002/diff/1/content/renderer/renderer_b... content/renderer/renderer_blink_platform_impl.cc:751: layout, static_cast<int>(sample_rate), 32, On 2016/11/01 18:51:37, DaleCurtis wrote: > Drop this change. I can't guarantee it'll plumb properly. Instead set it in > pulse_output/pulse_input. You're right. It's not plumbed through.
PTAL. I think this implements this correctly. Some simple webaudio tests show that it is working, and the test from the bug allows output with a gain down to 0.00002. It seems as if there's silence for a gain of 0.00001. Don't know why that is, but I think we're doing everything we can to pass float values to pulse audio.
https://codereview.chromium.org/2469023002/diff/20001/media/audio/pulse/pulse... File media/audio/pulse/pulse_output.cc (right): https://codereview.chromium.org/2469023002/diff/20001/media/audio/pulse/pulse... media/audio/pulse/pulse_output.cc:52: // floats now. s/ now// -- no need to document the old state of the code :) https://codereview.chromium.org/2469023002/diff/20001/media/audio/pulse/pulse... media/audio/pulse/pulse_output.cc:135: Remove? https://codereview.chromium.org/2469023002/diff/20001/media/audio/pulse/pulse... media/audio/pulse/pulse_output.cc:159: // Sanitize the samples. Replace NaN with zero, and then clamp This should already be done as part of ToInterleaved, no? https://codereview.chromium.org/2469023002/diff/20001/media/audio/pulse/pulse... File media/audio/pulse/pulse_util.cc (right): https://codereview.chromium.org/2469023002/diff/20001/media/audio/pulse/pulse... media/audio/pulse/pulse_util.cc:182: #if 1 Fix? https://codereview.chromium.org/2469023002/diff/20001/media/base/audio_bus.cc File media/base/audio_bus.cc (right): https://codereview.chromium.org/2469023002/diff/20001/media/base/audio_bus.cc... media/base/audio_bus.cc:340: void AudioBus::Clamp() { No need IIRC, interleave should take care of this.
https://codereview.chromium.org/2469023002/diff/20001/media/audio/pulse/pulse... File media/audio/pulse/pulse_output.cc (right): https://codereview.chromium.org/2469023002/diff/20001/media/audio/pulse/pulse... media/audio/pulse/pulse_output.cc:52: // floats now. On 2016/11/01 23:07:53, DaleCurtis wrote: > s/ now// -- no need to document the old state of the code :) Done. https://codereview.chromium.org/2469023002/diff/20001/media/audio/pulse/pulse... media/audio/pulse/pulse_output.cc:135: On 2016/11/01 23:07:53, DaleCurtis wrote: > Remove? Done. https://codereview.chromium.org/2469023002/diff/20001/media/audio/pulse/pulse... media/audio/pulse/pulse_output.cc:159: // Sanitize the samples. Replace NaN with zero, and then clamp On 2016/11/01 23:07:53, DaleCurtis wrote: > This should already be done as part of ToInterleaved, no? Dunno. I didn't look. I was just going by the comment in line 155. https://codereview.chromium.org/2469023002/diff/20001/media/audio/pulse/pulse... File media/audio/pulse/pulse_util.cc (right): https://codereview.chromium.org/2469023002/diff/20001/media/audio/pulse/pulse... media/audio/pulse/pulse_util.cc:182: #if 1 On 2016/11/01 23:07:53, DaleCurtis wrote: > Fix? Done. Do you want float input too? Not exactly sure how to test this. https://codereview.chromium.org/2469023002/diff/20001/media/base/audio_bus.cc File media/base/audio_bus.cc (right): https://codereview.chromium.org/2469023002/diff/20001/media/base/audio_bus.cc... media/base/audio_bus.cc:340: void AudioBus::Clamp() { On 2016/11/01 23:07:53, DaleCurtis wrote: > No need IIRC, interleave should take care of this. You're right. I tested with a webaudio manual test that intentionally overflows to test clipping and the output is clipped as expected.
Update description with details of this change and indicate this is only for Linux. Otherwise lgtm, thanks for the change! Fingers crossed it doesn't explode for users :O
I was thinking of doing wasapi too in this CL. I'm fine with doing then separately. That might be better. On Nov 1, 2016 4:25 PM, <dalecurtis@chromium.org> wrote: > Update description with details of this change and indicate this is only > for > Linux. Otherwise lgtm, thanks for the change! Fingers crossed it doesn't > explode > for users :O > > https://codereview.chromium.org/2469023002/ > -- 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.
I'd do it separately. We'll need different reviewers.
Description was changed from ========== Support floating-point audio output WIP BUG=659641 ========== to ========== Support floating-point audio output for Linux Instead of converting the audio to 16-bit PCM, pass the audio data directly to Pulse Audio as floating-point numbers and let Pulse Audio do whatever is necessary. Using the test url, a gain of 0.00002 produces output on my Z620 where it didn't before. There is silence for a gain of 0.00001, but that could be due to many things including pulse audio, the actual audio driver, the audio HW, and my headphones. BUG=659641 TEST=Run test url from bug and set gain to 0.00002 and hear output ==========
Description was changed from ========== Support floating-point audio output for Linux Instead of converting the audio to 16-bit PCM, pass the audio data directly to Pulse Audio as floating-point numbers and let Pulse Audio do whatever is necessary. Using the test url, a gain of 0.00002 produces output on my Z620 where it didn't before. There is silence for a gain of 0.00001, but that could be due to many things including pulse audio, the actual audio driver, the audio HW, and my headphones. BUG=659641 TEST=Run test url from bug and set gain to 0.00002 and hear output ========== to ========== Support floating-point audio output for Linux Instead of converting the audio to 16-bit PCM, pass the audio data directly to Pulse Audio as floating-point numbers and let Pulse Audio do whatever is necessary. Using the test url, a gain of 0.00002 produces output on my Z620 where it didn't before. There is silence for a gain of 0.00001, but that could be due to many things including pulse audio, the actual audio driver, the audio HW, and my headphones. BUG=659641 TEST=Run test url from bug and set gain to 0.00002 and hear output ==========
On 2016/11/01 23:25:24, DaleCurtis wrote: > Update description with details of this change and indicate this is only for > Linux. Otherwise lgtm, thanks for the change! Fingers crossed it doesn't explode > for users :O Thanks for your help in doing this. Crossing my fingers and toes!
The CQ bit was checked by rtoy@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Support floating-point audio output for Linux Instead of converting the audio to 16-bit PCM, pass the audio data directly to Pulse Audio as floating-point numbers and let Pulse Audio do whatever is necessary. Using the test url, a gain of 0.00002 produces output on my Z620 where it didn't before. There is silence for a gain of 0.00001, but that could be due to many things including pulse audio, the actual audio driver, the audio HW, and my headphones. BUG=659641 TEST=Run test url from bug and set gain to 0.00002 and hear output ========== to ========== Support floating-point audio output for Linux Instead of converting the audio to 16-bit PCM, pass the audio data directly to Pulse Audio as floating-point numbers and let Pulse Audio do whatever is necessary. Using the test url, a gain of 0.00002 produces output on my Z620 where it didn't before. There is silence for a gain of 0.00001, but that could be due to many things including pulse audio, the actual audio driver, the audio HW, and my headphones. BUG=659641 TEST=Run test url from bug and set gain to 0.00002 and hear output ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Support floating-point audio output for Linux Instead of converting the audio to 16-bit PCM, pass the audio data directly to Pulse Audio as floating-point numbers and let Pulse Audio do whatever is necessary. Using the test url, a gain of 0.00002 produces output on my Z620 where it didn't before. There is silence for a gain of 0.00001, but that could be due to many things including pulse audio, the actual audio driver, the audio HW, and my headphones. BUG=659641 TEST=Run test url from bug and set gain to 0.00002 and hear output ========== to ========== Support floating-point audio output for Linux Instead of converting the audio to 16-bit PCM, pass the audio data directly to Pulse Audio as floating-point numbers and let Pulse Audio do whatever is necessary. Using the test url, a gain of 0.00002 produces output on my Z620 where it didn't before. There is silence for a gain of 0.00001, but that could be due to many things including pulse audio, the actual audio driver, the audio HW, and my headphones. BUG=659641 TEST=Run test url from bug and set gain to 0.00002 and hear output Committed: https://crrev.com/9017c72f75614b18f1af80e90c77d172413644bc Cr-Commit-Position: refs/heads/master@{#429301} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/9017c72f75614b18f1af80e90c77d172413644bc Cr-Commit-Position: refs/heads/master@{#429301} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
