Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2690793002:
Add basic resample support to WASAPIAudioInputStream. (Closed)
|
Created:
3 years, 10 months ago by tommi (sloooow) - chröme Modified:
3 years, 10 months ago CC:
asvitkine+watch_chromium.org, chromium-reviews, feature-media-reviews_chromium.org, Max Morin, o1ka Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd basic resample support to WASAPIAudioInputStream.
After removing the legacy Wave capture implementation, we're seeing
frequent failures on Windows due to "FORMAT_NOT_SUPPORTED".
It seems we still have places that hardcode a specific sample rate instead of querying.
This CL adds basic support for resampling to see how much that buys us while we track down the code that still requires this.
Notably we don't support cases where there are mismatches in the number of channels etc,
so there will possibly still be cases where a user has a mono mic but the calling code has hardcoded requiring stereo input.
BUG=684741
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
Review-Url: https://codereview.chromium.org/2690793002
Cr-Commit-Position: refs/heads/master@{#451461}
Committed: https://chromium.googlesource.com/chromium/src/+/3d875dfaa149b616ef07c753b4ad55777b636c7e
Patch Set 1 #
Total comments: 15
Patch Set 2 : Use AudioConverter for converting instead of the multi channel resampler. #
Total comments: 5
Patch Set 3 : Add FIFO in cases where we don't get an exact buffer match for resampling #
Total comments: 4
Patch Set 4 : Switch to AudioBlockFifo #Patch Set 5 : Rebase #
Total comments: 3
Patch Set 6 : Add AudioBlockFifo::PushSilence #
Total comments: 1
Patch Set 7 : Fix comment #
Total comments: 12
Patch Set 8 : Use closest match bits-per-sample, add DCHECKs and address other comments #
Total comments: 3
Patch Set 9 : Add check for bitness #Patch Set 10 : Add check for unsupported channel layout #
Messages
Total messages: 53 (19 generated)
Description was changed from ========== Add basic resample support to WASAPIAudioInputStream. After removing the legacy Wave capture implementation, we're seeing frequent failures on Windows due to "FORMAT_NOT_SUPPORTED". It seems we still have places that hardcode a specific sample rate instead of querying. This CL adds basic support for resampling to see how much that buys us while we track down the code that still requires this. Notably we don't support cases where there are mismatches in the number of channels etc, so there will possibly still be cases where a user has a mono mic but the calling code has hardcoded requiring stereo input. BUG=684741 ========== to ========== Add basic resample support to WASAPIAudioInputStream. After removing the legacy Wave capture implementation, we're seeing frequent failures on Windows due to "FORMAT_NOT_SUPPORTED". It seems we still have places that hardcode a specific sample rate instead of querying. This CL adds basic support for resampling to see how much that buys us while we track down the code that still requires this. Notably we don't support cases where there are mismatches in the number of channels etc, so there will possibly still be cases where a user has a mono mic but the calling code has hardcoded requiring stereo input. BUG=684741 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ==========
tommi@chromium.org changed reviewers: + dalecurtis@chromium.org, jwd@chromium.org
dale - main review jwd - histograms.xml (one more enum addition to a recently reviewed change)
The CQ bit was checked by tommi@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.
https://codereview.chromium.org/2690793002/diff/1/media/audio/win/audio_low_l... File media/audio/win/audio_low_latency_input_win.cc (right): https://codereview.chromium.org/2690793002/diff/1/media/audio/win/audio_low_l... media/audio/win/audio_low_latency_input_win.cc:127: if (SUCCEEDED(hr) && resampler_.get() != nullptr) just "&& resampler_" it has safe-bool conversion built in these days. https://codereview.chromium.org/2690793002/diff/1/media/audio/win/audio_low_l... media/audio/win/audio_low_latency_input_win.cc:320: // Allocate a buffer with a size that enables us to take care of cases like: You might consider that instead of uint8_t array + resampler you use a AudioBlockFIFO + Resampler here; this way the resampler can pull from subsequent buffers if it needs to. https://codereview.chromium.org/2690793002/diff/1/media/audio/win/audio_low_l... media/audio/win/audio_low_latency_input_win.cc:430: if (resampler_.get()) { Ditto. https://codereview.chromium.org/2690793002/diff/1/media/audio/win/audio_low_l... media/audio/win/audio_low_latency_input_win.cc:607: if (hr == S_FALSE && closest_match->nChannels == format_.nChannels) { If you use a AudioConverter it'll handle mismatched channel counts for you. https://codereview.chromium.org/2690793002/diff/1/media/audio/win/audio_low_l... media/audio/win/audio_low_latency_input_win.cc:619: resampler_.reset(new MultiChannelResampler( I think this works, though not how we've done it in the past see AudioBufferConverter. Will take a closer look later, have to run now. https://codereview.chromium.org/2690793002/diff/1/media/audio/win/audio_low_l... media/audio/win/audio_low_latency_input_win.cc:790: resample_bus_->CopyTo(audio_bus); Probably want a DCHECK this isn't called twice without resample_bus_ being refilled.
https://codereview.chromium.org/2690793002/diff/1/media/audio/win/audio_low_l... File media/audio/win/audio_low_latency_input_win.cc (right): https://codereview.chromium.org/2690793002/diff/1/media/audio/win/audio_low_l... media/audio/win/audio_low_latency_input_win.cc:607: if (hr == S_FALSE && closest_match->nChannels == format_.nChannels) { On 2017/02/13 at 19:40:42, DaleCurtis wrote: > If you use a AudioConverter it'll handle mismatched channel counts for you. You should also sanity check the closest match sample rate here. I.e. that it isn't 0 or something outside the bound of our limits::kMin/MaxSampleRate. https://codereview.chromium.org/2690793002/diff/1/media/audio/win/audio_low_l... media/audio/win/audio_low_latency_input_win.cc:619: resampler_.reset(new MultiChannelResampler( On 2017/02/13 at 19:40:42, DaleCurtis wrote: > I think this works, though not how we've done it in the past see AudioBufferConverter. Will take a closer look later, have to run now. Over a long enough time frame I think this might lose a sample every now and then. I.e. Resample() will consume all but the last frame and then the next FromInterleaved() call will overwrite it. To make this correct I think you need to use the AudioBlockFifo as suggested above. E.g., if the desired is 44.1kHz/1024 frames and the closest match is 16kHz, I think you'll lose the fractional part on occasion.
histograms LGTM
Use AudioConverter for converting instead of the multi channel resampler.
https://codereview.chromium.org/2690793002/diff/20001/media/audio/win/audio_l... File media/audio/win/audio_low_latency_input_win.cc (right): https://codereview.chromium.org/2690793002/diff/20001/media/audio/win/audio_l... media/audio/win/audio_low_latency_input_win.cc:441: LOG(ERROR) << "Failed to convert enough samples."; Won't this trample whatever you have in audio_bus_? Did the parameters I suggested in the last comment trigger this path? I suspect you either need an outbound fifo or to issue multiple OnData() calls here. https://codereview.chromium.org/2690793002/diff/20001/media/audio/win/audio_l... media/audio/win/audio_low_latency_input_win.cc:631: GuessChannelLayout(closest_match->nChannels), Need to check the result of this that it's not CHANNEL_LAYOUT_UNSUPPORTED. https://codereview.chromium.org/2690793002/diff/20001/media/audio/win/audio_l... media/audio/win/audio_low_latency_input_win.cc:633: // We need to be careful here to not pick the closest wBitsPerSample I think this statement is false, or at least should instead be up with the FromInterleaved() call, the bits_per_sample field here is effectively dead. You probably just want to make sure this is 8, 16, or 32 and fail otherwise.
Add FIFO in cases where we don't get an exact buffer match for resampling
Hey Dale - I switched to AudioConverter and added a FIFO in cases such as you mentioned (converting from 16kHz to 44.1kHz @1024 frames). https://codereview.chromium.org/2690793002/diff/1/media/audio/win/audio_low_l... File media/audio/win/audio_low_latency_input_win.cc (right): https://codereview.chromium.org/2690793002/diff/1/media/audio/win/audio_low_l... media/audio/win/audio_low_latency_input_win.cc:127: if (SUCCEEDED(hr) && resampler_.get() != nullptr) On 2017/02/13 19:40:42, DaleCurtis wrote: > just "&& resampler_" it has safe-bool conversion built in these days. Done. https://codereview.chromium.org/2690793002/diff/1/media/audio/win/audio_low_l... media/audio/win/audio_low_latency_input_win.cc:320: // Allocate a buffer with a size that enables us to take care of cases like: On 2017/02/13 19:40:42, DaleCurtis wrote: > You might consider that instead of uint8_t array + resampler you use a > AudioBlockFIFO + Resampler here; this way the resampler can pull from subsequent > buffers if it needs to. Acknowledged. https://codereview.chromium.org/2690793002/diff/1/media/audio/win/audio_low_l... media/audio/win/audio_low_latency_input_win.cc:430: if (resampler_.get()) { On 2017/02/13 19:40:42, DaleCurtis wrote: > Ditto. Done. https://codereview.chromium.org/2690793002/diff/1/media/audio/win/audio_low_l... media/audio/win/audio_low_latency_input_win.cc:607: if (hr == S_FALSE && closest_match->nChannels == format_.nChannels) { On 2017/02/13 23:59:00, DaleCurtis wrote: > On 2017/02/13 at 19:40:42, DaleCurtis wrote: > > If you use a AudioConverter it'll handle mismatched channel counts for you. > > You should also sanity check the closest match sample rate here. I.e. that it > isn't 0 or something outside the bound of our limits::kMin/MaxSampleRate. Done. Also switched to AudioConverter. https://codereview.chromium.org/2690793002/diff/1/media/audio/win/audio_low_l... media/audio/win/audio_low_latency_input_win.cc:619: resampler_.reset(new MultiChannelResampler( On 2017/02/13 23:59:00, DaleCurtis wrote: > On 2017/02/13 at 19:40:42, DaleCurtis wrote: > > I think this works, though not how we've done it in the past see > AudioBufferConverter. Will take a closer look later, have to run now. > > Over a long enough time frame I think this might lose a sample every now and > then. I.e. Resample() will consume all but the last frame and then the next > FromInterleaved() call will overwrite it. To make this correct I think you need > to use the AudioBlockFifo as suggested above. > > E.g., if the desired is 44.1kHz/1024 frames and the closest match is 16kHz, I > think you'll lose the fractional part on occasion. I tried several ways of reproducing that, including recording at 16kHz and converting to 44.1kHz/1024 for 10 minutes, but was unable to reproduce the problem (ProvideInput was called for all buffers). Any further hints on how to test that? https://codereview.chromium.org/2690793002/diff/1/media/audio/win/audio_low_l... media/audio/win/audio_low_latency_input_win.cc:790: resample_bus_->CopyTo(audio_bus); On 2017/02/13 19:40:42, DaleCurtis wrote: > Probably want a DCHECK this isn't called twice without resample_bus_ being > refilled. Done.
https://codereview.chromium.org/2690793002/diff/1/media/audio/win/audio_low_l... File media/audio/win/audio_low_latency_input_win.cc (right): https://codereview.chromium.org/2690793002/diff/1/media/audio/win/audio_low_l... media/audio/win/audio_low_latency_input_win.cc:619: resampler_.reset(new MultiChannelResampler( On 2017/02/16 17:08:47, tommi (krómíum) wrote: > On 2017/02/13 23:59:00, DaleCurtis wrote: > > On 2017/02/13 at 19:40:42, DaleCurtis wrote: > > > I think this works, though not how we've done it in the past see > > AudioBufferConverter. Will take a closer look later, have to run now. > > > > Over a long enough time frame I think this might lose a sample every now and > > then. I.e. Resample() will consume all but the last frame and then the next > > FromInterleaved() call will overwrite it. To make this correct I think you > need > > to use the AudioBlockFifo as suggested above. > > > > E.g., if the desired is 44.1kHz/1024 frames and the closest match is 16kHz, I > > think you'll lose the fractional part on occasion. > > I tried several ways of reproducing that, including recording at 16kHz and > converting to 44.1kHz/1024 for 10 minutes, but was unable to reproduce the > problem (ProvideInput was called for all buffers). Any further hints on how to > test that? Oh please ignore this, I did more testing and figured out what I was missing.
I still strongly recommend you switch to an AudioBlockFifo for the capture buffers, it will make the code much simpler and should be trivial to do. Just directly replace capture_buffer above with an class member AudioBlockFIFO fifo_, delete capture_buffer, capture_buffer_size. l.386..l.392 become Push() calls; when not resampling, it's just OnData(fifo_->Consume()), when resampling it's converter_->Resample(resampled_audio_bus_)+ OnData(resampled_audio_bus_) with ProvideInput doing a fifo_->Consume()->CopyTo(provide_input_bus). This removes a memcpy from the standard path, has the same number of copies as your current approach when resampling, and avoids needing to check for fractional buffer ratios. https://codereview.chromium.org/2690793002/diff/40001/media/audio/win/audio_l... File media/audio/win/audio_low_latency_input_win.cc (right): https://codereview.chromium.org/2690793002/diff/40001/media/audio/win/audio_l... media/audio/win/audio_low_latency_input_win.cc:689: // Check if we'll need to inject an intermediery buffer to avoid You could also determine this by checking if |new_frames_per_buffer| has a fractional part. https://codereview.chromium.org/2690793002/diff/40001/media/audio/win/audio_l... media/audio/win/audio_low_latency_input_win.cc:694: DCHECK(buffer_ratio <= buffer_ratio2); DCHECK_LE().
(Also I think you missed some comments from Ps#2, though I'm not sure if your last comment was a request to wait until you made another round of changes :)
Thanks Dale - indeed I missed those comments. Acking those below and I'll upload a new patch tomorrow and ping back. https://codereview.chromium.org/2690793002/diff/20001/media/audio/win/audio_l... File media/audio/win/audio_low_latency_input_win.cc (right): https://codereview.chromium.org/2690793002/diff/20001/media/audio/win/audio_l... media/audio/win/audio_low_latency_input_win.cc:441: LOG(ERROR) << "Failed to convert enough samples."; On 2017/02/16 02:03:36, DaleCurtis wrote: > Won't this trample whatever you have in audio_bus_? Did the parameters I > suggested in the last comment trigger this path? I suspect you either need an > outbound fifo or to issue multiple OnData() calls here. Yes indeed it would. I wasn't able to repro this at the time, never saw the error and forgot about it :-/ Incidentally, when I was thinking about how to handle this case, I looked around for a similar case and saw what we're doing in the speech code: https://cs.chromium.org/chromium/src/content/browser/speech/speech_recognizer... Are we converting the same data twice there? https://codereview.chromium.org/2690793002/diff/20001/media/audio/win/audio_l... media/audio/win/audio_low_latency_input_win.cc:633: // We need to be careful here to not pick the closest wBitsPerSample On 2017/02/16 02:03:36, DaleCurtis wrote: > I think this statement is false, or at least should instead be up with the > FromInterleaved() call, the bits_per_sample field here is effectively dead. You > probably just want to make sure this is 8, 16, or 32 and fail otherwise. Oh interesting, thanks for pointing that out (and I'm happy I wrote this down, because it was unexpected to me as I hit the error). I'll take a look again to see what I missed.
Switch to AudioBlockFifo
Rebase
The CQ bit was checked by tommi@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...
Switched to AudioBlockFifo now. ptal. It's indeed, much simpler, but there's one particular case I'm wondering about right now. See comment. https://codereview.chromium.org/2690793002/diff/40001/media/audio/win/audio_l... File media/audio/win/audio_low_latency_input_win.cc (right): https://codereview.chromium.org/2690793002/diff/40001/media/audio/win/audio_l... media/audio/win/audio_low_latency_input_win.cc:689: // Check if we'll need to inject an intermediery buffer to avoid On 2017/02/16 19:59:48, DaleCurtis wrote: > You could also determine this by checking if |new_frames_per_buffer| has a > fractional part. Of course! done. https://codereview.chromium.org/2690793002/diff/40001/media/audio/win/audio_l... media/audio/win/audio_low_latency_input_win.cc:694: DCHECK(buffer_ratio <= buffer_ratio2); On 2017/02/16 19:59:48, DaleCurtis wrote: > DCHECK_LE(). Now removed https://codereview.chromium.org/2690793002/diff/80001/media/audio/win/audio_l... File media/audio/win/audio_low_latency_input_win.cc (right): https://codereview.chromium.org/2690793002/diff/80001/media/audio/win/audio_l... media/audio/win/audio_low_latency_input_win.cc:399: memset(data_ptr, 0, num_frames_to_read * frame_size_); I'm looking for a way to test this right now, so it worries me a little. I don't suppose there's a way to inject zeroed frames into the fifo_?
https://codereview.chromium.org/2690793002/diff/80001/media/audio/win/audio_l... File media/audio/win/audio_low_latency_input_win.cc (right): https://codereview.chromium.org/2690793002/diff/80001/media/audio/win/audio_l... media/audio/win/audio_low_latency_input_win.cc:399: memset(data_ptr, 0, num_frames_to_read * frame_size_); On 2017/02/17 at 17:09:21, tommi (krómíum) wrote: > I'm looking for a way to test this right now, so it worries me a little. > I don't suppose there's a way to inject zeroed frames into the fifo_? Not currently, but feel free to add PushSilence()
Add AudioBlockFifo::PushSilence
https://codereview.chromium.org/2690793002/diff/80001/media/audio/win/audio_l... File media/audio/win/audio_low_latency_input_win.cc (right): https://codereview.chromium.org/2690793002/diff/80001/media/audio/win/audio_l... media/audio/win/audio_low_latency_input_win.cc:399: memset(data_ptr, 0, num_frames_to_read * frame_size_); On 2017/02/17 17:25:18, DaleCurtis wrote: > On 2017/02/17 at 17:09:21, tommi (krómíum) wrote: > > I'm looking for a way to test this right now, so it worries me a little. > > I don't suppose there's a way to inject zeroed frames into the fifo_? > > Not currently, but feel free to add PushSilence() Done.
https://codereview.chromium.org/2690793002/diff/100001/media/base/audio_block... File media/base/audio_block_fifo.h (right): https://codereview.chromium.org/2690793002/diff/100001/media/base/audio_block... media/base/audio_block_fifo.h:32: // Pushes zeroed out interleaved audio data to the FIFO. oops, will s/interleaved audio data/frames
Fix comment
lgtm % some nits! https://codereview.chromium.org/2690793002/diff/120001/media/audio/win/audio_... File media/audio/win/audio_low_latency_input_win.cc (right): https://codereview.chromium.org/2690793002/diff/120001/media/audio/win/audio_... media/audio/win/audio_low_latency_input_win.cc:7: #include <math.h> cmath? for modf? http://en.cppreference.com/w/cpp/numeric/math/modf https://codereview.chromium.org/2690793002/diff/120001/media/audio/win/audio_... media/audio/win/audio_low_latency_input_win.cc:614: // We need to be careful here to not pick the closest wBitsPerSample Missed feedback from previous patchset? Re: dead parameter; just needs to be 8, 16, 32. https://codereview.chromium.org/2690793002/diff/120001/media/audio/win/audio_... media/audio/win/audio_low_latency_input_win.cc:641: format_.nChannels * Replace terms with * format_.nBlockAlign? https://codereview.chromium.org/2690793002/diff/120001/media/audio/win/audio_... media/audio/win/audio_low_latency_input_win.cc:649: modf(new_frames_per_buffer, &new_frames_per_buffer) != 0.0; std:: ?
Use closest match bits-per-sample, add DCHECKs and address other comments
https://codereview.chromium.org/2690793002/diff/120001/media/audio/win/audio_... File media/audio/win/audio_low_latency_input_win.cc (right): https://codereview.chromium.org/2690793002/diff/120001/media/audio/win/audio_... media/audio/win/audio_low_latency_input_win.cc:7: #include <math.h> On 2017/02/17 18:23:23, DaleCurtis wrote: > cmath? for modf? > > http://en.cppreference.com/w/cpp/numeric/math/modf Done. https://codereview.chromium.org/2690793002/diff/120001/media/audio/win/audio_... media/audio/win/audio_low_latency_input_win.cc:614: // We need to be careful here to not pick the closest wBitsPerSample On 2017/02/17 18:23:23, DaleCurtis wrote: > Missed feedback from previous patchset? Re: dead parameter; just needs to be 8, > 16, 32. Ah, sorry, yes. Removed the comment and now use the closest_match bitrate. Also added DCHECKs for GuessChannelLayout. https://codereview.chromium.org/2690793002/diff/120001/media/audio/win/audio_... media/audio/win/audio_low_latency_input_win.cc:641: format_.nChannels * On 2017/02/17 18:23:23, DaleCurtis wrote: > Replace terms with * format_.nBlockAlign? Done. https://codereview.chromium.org/2690793002/diff/120001/media/audio/win/audio_... media/audio/win/audio_low_latency_input_win.cc:649: modf(new_frames_per_buffer, &new_frames_per_buffer) != 0.0; On 2017/02/17 18:23:23, DaleCurtis wrote: > std:: ? Done.
The CQ bit was checked by tommi@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jwd@chromium.org, dalecurtis@chromium.org Link to the patchset: https://codereview.chromium.org/2690793002/#ps140001 (title: "Use closest match bits-per-sample, add DCHECKs and address other comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Still a couple of potential failures, but are more theoretical, so up to you. I.e., no idea if the bits per sample can be anything other than 8, 16, 32 -- 24 generally means 32 as well, so it might not be worth checking. Also no idea if WASAPI has input devices with > 8 channels. https://codereview.chromium.org/2690793002/diff/120001/media/audio/win/audio_... File media/audio/win/audio_low_latency_input_win.cc (right): https://codereview.chromium.org/2690793002/diff/120001/media/audio/win/audio_... media/audio/win/audio_low_latency_input_win.cc:614: // We need to be careful here to not pick the closest wBitsPerSample On 2017/02/17 at 22:21:27, tommi (krómíum) wrote: > On 2017/02/17 18:23:23, DaleCurtis wrote: > > Missed feedback from previous patchset? Re: dead parameter; just needs to be 8, > > 16, 32. > > Ah, sorry, yes. Removed the comment and now use the closest_match bitrate. > Also added DCHECKs for GuessChannelLayout. You do need to check that it's 8, 16, or 32, otherwise we don't have FromInterleaved() support which result in silence being injected during the deinterleave step. https://codereview.chromium.org/2690793002/diff/140001/media/audio/win/audio_... File media/audio/win/audio_low_latency_input_win.cc (right): https://codereview.chromium.org/2690793002/diff/140001/media/audio/win/audio_... media/audio/win/audio_low_latency_input_win.cc:612: DCHECK_NE(CHANNEL_LAYOUT_UNSUPPORTED, input_layout); This will cause CHECK() failures. Instead you either want to return an error or translate unsupported to DISCRETE with the given channel count; you can only reach this point if someone tries to hook up a > 8 channel input device FWIW.
The CQ bit was unchecked by tommi@chromium.org
On 2017/02/17 22:29:23, DaleCurtis wrote: > Still a couple of potential failures, but are more theoretical, so up to you. > I.e., no idea if the bits per sample can be anything other than 8, 16, 32 -- 24 > generally means 32 as well, so it might not be worth checking. Also no idea if > WASAPI has input devices with > 8 channels. Looking into it now (will probably finish it tomorrow). Btw, this whole conversion business wouldn't be necessary if all the code first queried for the preferred parameters and then used that. At the moment, the conversion is needed for cast and pepper afaik. Once those have been updated to do as the rest of the code does, we shouldn't need this functionality. See e.g. https://cs.chromium.org/chromium/src/content/renderer/pepper/pepper_platform_... > https://codereview.chromium.org/2690793002/diff/120001/media/audio/win/audio_... > File media/audio/win/audio_low_latency_input_win.cc (right): > > https://codereview.chromium.org/2690793002/diff/120001/media/audio/win/audio_... > media/audio/win/audio_low_latency_input_win.cc:614: // We need to be careful > here to not pick the closest wBitsPerSample > On 2017/02/17 at 22:21:27, tommi (krómíum) wrote: > > On 2017/02/17 18:23:23, DaleCurtis wrote: > > > Missed feedback from previous patchset? Re: dead parameter; just needs to be > 8, > > > 16, 32. > > > > Ah, sorry, yes. Removed the comment and now use the closest_match bitrate. > > Also added DCHECKs for GuessChannelLayout. > > You do need to check that it's 8, 16, or 32, otherwise we don't have > FromInterleaved() support which result in silence being injected during the > deinterleave step. > > https://codereview.chromium.org/2690793002/diff/140001/media/audio/win/audio_... > File media/audio/win/audio_low_latency_input_win.cc (right): > > https://codereview.chromium.org/2690793002/diff/140001/media/audio/win/audio_... > media/audio/win/audio_low_latency_input_win.cc:612: > DCHECK_NE(CHANNEL_LAYOUT_UNSUPPORTED, input_layout); > This will cause CHECK() failures. Instead you either want to return an error or > translate unsupported to DISCRETE with the given channel count; you can only > reach this point if someone tries to hook up a > 8 channel input device FWIW.
https://codereview.chromium.org/2690793002/diff/120001/media/audio/win/audio_... File media/audio/win/audio_low_latency_input_win.cc (right): https://codereview.chromium.org/2690793002/diff/120001/media/audio/win/audio_... media/audio/win/audio_low_latency_input_win.cc:614: // We need to be careful here to not pick the closest wBitsPerSample On 2017/02/17 22:29:23, DaleCurtis wrote: > On 2017/02/17 at 22:21:27, tommi (krómíum) wrote: > > On 2017/02/17 18:23:23, DaleCurtis wrote: > > > Missed feedback from previous patchset? Re: dead parameter; just needs to be > 8, > > > 16, 32. > > > > Ah, sorry, yes. Removed the comment and now use the closest_match bitrate. > > Also added DCHECKs for GuessChannelLayout. > > You do need to check that it's 8, 16, or 32, otherwise we don't have > FromInterleaved() support which result in silence being injected during the > deinterleave step. iow, 24 is not supported? I'll add the checks. I did test with 8, 16 and 32 btw. https://codereview.chromium.org/2690793002/diff/140001/media/audio/win/audio_... File media/audio/win/audio_low_latency_input_win.cc (right): https://codereview.chromium.org/2690793002/diff/140001/media/audio/win/audio_... media/audio/win/audio_low_latency_input_win.cc:612: DCHECK_NE(CHANNEL_LAYOUT_UNSUPPORTED, input_layout); On 2017/02/17 22:29:23, DaleCurtis wrote: > This will cause CHECK() failures. Instead you either want to return an error or > translate unsupported to DISCRETE with the given channel count; you can only > reach this point if someone tries to hook up a > 8 channel input device FWIW. Will add check. Hmm... gives me an idea to test. I think I have a device that has more channels (albeit Firewire).
https://codereview.chromium.org/2690793002/diff/120001/media/audio/win/audio_... File media/audio/win/audio_low_latency_input_win.cc (right): https://codereview.chromium.org/2690793002/diff/120001/media/audio/win/audio_... media/audio/win/audio_low_latency_input_win.cc:614: // We need to be careful here to not pick the closest wBitsPerSample On 2017/02/17 at 23:14:02, tommi (krómíum) wrote: > On 2017/02/17 22:29:23, DaleCurtis wrote: > > On 2017/02/17 at 22:21:27, tommi (krómíum) wrote: > > > On 2017/02/17 18:23:23, DaleCurtis wrote: > > > > Missed feedback from previous patchset? Re: dead parameter; just needs to be > > 8, > > > > 16, 32. > > > > > > Ah, sorry, yes. Removed the comment and now use the closest_match bitrate. > > > Also added DCHECKs for GuessChannelLayout. > > > > You do need to check that it's 8, 16, or 32, otherwise we don't have > > FromInterleaved() support which result in silence being injected during the > > deinterleave step. > > iow, 24 is not supported? I'll add the checks. I did test with 8, 16 and 32 btw. 24, if it's actually packed into 3 bytes is not supported. I don't think I've ever seen it done that way, 4 bytes is supported for 24bit.
Add check for bitness
Add check for unsupported channel layout
https://codereview.chromium.org/2690793002/diff/120001/media/audio/win/audio_... File media/audio/win/audio_low_latency_input_win.cc (right): https://codereview.chromium.org/2690793002/diff/120001/media/audio/win/audio_... media/audio/win/audio_low_latency_input_win.cc:614: // We need to be careful here to not pick the closest wBitsPerSample On 2017/02/17 22:29:23, DaleCurtis wrote: > On 2017/02/17 at 22:21:27, tommi (krómíum) wrote: > > On 2017/02/17 18:23:23, DaleCurtis wrote: > > > Missed feedback from previous patchset? Re: dead parameter; just needs to be > 8, > > > 16, 32. > > > > Ah, sorry, yes. Removed the comment and now use the closest_match bitrate. > > Also added DCHECKs for GuessChannelLayout. > > You do need to check that it's 8, 16, or 32, otherwise we don't have > FromInterleaved() support which result in silence being injected during the > deinterleave step. Done. https://codereview.chromium.org/2690793002/diff/140001/media/audio/win/audio_... File media/audio/win/audio_low_latency_input_win.cc (right): https://codereview.chromium.org/2690793002/diff/140001/media/audio/win/audio_... media/audio/win/audio_low_latency_input_win.cc:612: DCHECK_NE(CHANNEL_LAYOUT_UNSUPPORTED, input_layout); On 2017/02/17 22:29:23, DaleCurtis wrote: > This will cause CHECK() failures. Instead you either want to return an error or > translate unsupported to DISCRETE with the given channel count; you can only > reach this point if someone tries to hook up a > 8 channel input device FWIW. I added a check for channel layout of the closest match to the IsSupportedFormatForConversion() method (just added). We won't attempt conversion if we get CHANNEL_LAYOUT_UNSUPPORTED.
The CQ bit was checked by tommi@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: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by tommi@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dalecurtis@chromium.org, jwd@chromium.org Link to the patchset: https://codereview.chromium.org/2690793002/#ps180001 (title: "Add check for unsupported channel layout")
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": 180001, "attempt_start_ts": 1487422903412160,
"parent_rev": "17c78241a37810aff596974cfdec60b01bc0f4c5", "commit_rev":
"3d875dfaa149b616ef07c753b4ad55777b636c7e"}
Message was sent while issue was closed.
Description was changed from ========== Add basic resample support to WASAPIAudioInputStream. After removing the legacy Wave capture implementation, we're seeing frequent failures on Windows due to "FORMAT_NOT_SUPPORTED". It seems we still have places that hardcode a specific sample rate instead of querying. This CL adds basic support for resampling to see how much that buys us while we track down the code that still requires this. Notably we don't support cases where there are mismatches in the number of channels etc, so there will possibly still be cases where a user has a mono mic but the calling code has hardcoded requiring stereo input. BUG=684741 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ========== to ========== Add basic resample support to WASAPIAudioInputStream. After removing the legacy Wave capture implementation, we're seeing frequent failures on Windows due to "FORMAT_NOT_SUPPORTED". It seems we still have places that hardcode a specific sample rate instead of querying. This CL adds basic support for resampling to see how much that buys us while we track down the code that still requires this. Notably we don't support cases where there are mismatches in the number of channels etc, so there will possibly still be cases where a user has a mono mic but the calling code has hardcoded requiring stereo input. BUG=684741 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel Review-Url: https://codereview.chromium.org/2690793002 Cr-Commit-Position: refs/heads/master@{#451461} Committed: https://chromium.googlesource.com/chromium/src/+/3d875dfaa149b616ef07c753b4ad... ==========
Message was sent while issue was closed.
Committed patchset #10 (id:180001) as https://chromium.googlesource.com/chromium/src/+/3d875dfaa149b616ef07c753b4ad... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
