Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2475453003:
Support floating-point audio output for Windows7+ (Closed)
|
Created:
4 years, 1 month ago by Raymond Toy Modified:
4 years, 1 month ago CC:
chromium-reviews, feature-media-reviews_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionSupport floating-point audio output for Windows7+
Instead of converting audio to 16-bit PCM, pass the audio data
as floating-point directly on the low-latency path (WASAPI)
BUG=659641
TEST=Run test url from bug and set gain to 0.00002 and hear output
Committed: https://crrev.com/70fe19f8a20761c939444f2d3b8c6d1daf13c975
Cr-Commit-Position: refs/heads/master@{#429928}
Patch Set 1 #Patch Set 2 : git cl format #
Total comments: 4
Patch Set 3 : Address review comments #Patch Set 4 : Remove old comment and unused variable. #Patch Set 5 : Fixup #Messages
Total messages: 29 (13 generated)
rtoy@chromium.org changed reviewers: + dalecurtis@chromium.org
PTAL or suggest another reviewer.
dalecurtis@chromium.org changed reviewers: + tommi@chromium.org
+tommi, lgtm though. That was way easier than expected. :o
lgtm! Adding henrika@ to cc as FYI https://codereview.chromium.org/2475453003/diff/20001/media/audio/win/audio_l... File media/audio/win/audio_low_latency_output_win.cc (right): https://codereview.chromium.org/2475453003/diff/20001/media/audio/win/audio_l... media/audio/win/audio_low_latency_output_win.cc:110: format->wBitsPerSample = 32; nit: Add a constant at the top of the file for this? Just so that readers will be more aware of where this value needs to be set. https://codereview.chromium.org/2475453003/diff/20001/media/audio/win/audio_l... media/audio/win/audio_low_latency_output_win.cc:118: format_.SubFormat = KSDATAFORMAT_SUBTYPE_IEEE_FLOAT; that's it? :-|
https://codereview.chromium.org/2475453003/diff/20001/media/audio/win/audio_l... File media/audio/win/audio_low_latency_output_win.cc (right): https://codereview.chromium.org/2475453003/diff/20001/media/audio/win/audio_l... media/audio/win/audio_low_latency_output_win.cc:110: format->wBitsPerSample = 32; On 2016/11/02 20:23:04, tommi (chrömium) wrote: > nit: Add a constant at the top of the file for this? Just so that readers will > be more aware of where this value needs to be set. Done. Please up choose a good name if you don't like kBitsPerFloatSample. https://codereview.chromium.org/2475453003/diff/20001/media/audio/win/audio_l... media/audio/win/audio_low_latency_output_win.cc:118: format_.SubFormat = KSDATAFORMAT_SUBTYPE_IEEE_FLOAT; On 2016/11/02 20:23:04, tommi (chrömium) wrote: > that's it? :-| I think so. :-) I can hear output now that wasn't heard before on my windows machine.
The CQ bit was checked by rtoy@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: Try jobs failed on following builders: win_clang on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
lgtm
LGTM :-)
The CQ bit was checked by rtoy@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dalecurtis@chromium.org Link to the patchset: https://codereview.chromium.org/2475453003/#ps60001 (title: "Remove old comment and unused variable.")
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: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
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...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
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.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Support floating-point audio output for Windows7+ Instead of converting audio to 16-bit PCM, pass the audio data as floating-point directly on the low-latency path (WASAPI) BUG=659641 TEST=Run test url from bug and set gain to 0.00002 and hear output ========== to ========== Support floating-point audio output for Windows7+ Instead of converting audio to 16-bit PCM, pass the audio data as floating-point directly on the low-latency path (WASAPI) BUG=659641 TEST=Run test url from bug and set gain to 0.00002 and hear output Committed: https://crrev.com/70fe19f8a20761c939444f2d3b8c6d1daf13c975 Cr-Commit-Position: refs/heads/master@{#429928} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/70fe19f8a20761c939444f2d3b8c6d1daf13c975 Cr-Commit-Position: refs/heads/master@{#429928}
Message was sent while issue was closed.
On 2016/11/04 17:36:44, commit-bot: I haz the power wrote: > Patchset 4 (id:??) landed as > https://crrev.com/70fe19f8a20761c939444f2d3b8c6d1daf13c975 > Cr-Commit-Position: refs/heads/master@{#429928} I believe this breaks https://build.chromium.org/p/chromium.gpu.fyi/builders/Win7%20Release%20%28AM... [4648:3596:1104/114818:FATAL:audio_low_latency_output_win.cc(558)] Check failed: num_filled_bytes <= packet_size_bytes_ (3528 vs. 1764) Can this be reverted / fixed?
Message was sent while issue was closed.
A revert of this CL (patchset #4 id:60001) has been created in https://codereview.chromium.org/2478893002/ by jmadill@chromium.org. The reason for reverting is: Seems to have broken Windows GPU bots. Examples: https://build.chromium.org/p/chromium.gpu.fyi/builders/Win7%20x64%20Release%2... https://build.chromium.org/p/chromium.gpu.fyi/builders/Win7%20Release%20%28AM... https://build.chromium.org/p/chromium.gpu.fyi/builders/Win8%20Release%20%28NV... .
Message was sent while issue was closed.
On 2016/11/04 20:33:21, Jamie Madill wrote: > A revert of this CL (patchset #4 id:60001) has been created in > https://codereview.chromium.org/2478893002/ by mailto:jmadill@chromium.org. > > The reason for reverting is: Seems to have broken Windows GPU bots. Examples: > > https://build.chromium.org/p/chromium.gpu.fyi/builders/Win7%20x64%20Release%2... > https://build.chromium.org/p/chromium.gpu.fyi/builders/Win7%20Release%20%28AM... > https://build.chromium.org/p/chromium.gpu.fyi/builders/Win8%20Release%20%28NV... > . I think this is because of packet_size_bytes_ = params.GetBytesPerBuffer(); GetBytesPerBuffer() doesn't use the new bits_per_sample. I'll upload a new CL. |
