Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
187493002:
Cast: Resample input audio to the configured sample rate (Closed)
|
Created:
6 years, 9 months ago by Alpha Left Google Modified:
6 years, 9 months ago CC:
chromium-reviews, feature-media-reviews_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionCast: Resample input audio to the configured sample rate
We configure opus to encode at 48khz. But on some platforms like Linux
the incoming data is at 44.1khz. We need to resample the data before
giving it to the cast library.
Fixing this allows us to have proper audio with a receiver.
BUG=349295
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=255893
Patch Set 1 #
Total comments: 4
Patch Set 2 : nits #
Total comments: 2
Patch Set 3 : resample correctly #
Total comments: 11
Patch Set 4 : preroll #
Total comments: 1
Patch Set 5 : fixed perf #
Total comments: 3
Patch Set 6 : no Pass() #Patch Set 7 : upload again #Messages
Total messages: 26 (0 generated)
I think there are files missing from this review, like cast_rtp_stream.h?
lgtm Never mind my last message. I see now the class is fully-contained in the module. Please address before commit: https://codereview.chromium.org/187493002/diff/1/chrome/renderer/media/cast_r... File chrome/renderer/media/cast_rtp_stream.cc (right): https://codereview.chromium.org/187493002/diff/1/chrome/renderer/media/cast_r... chrome/renderer/media/cast_rtp_stream.cc:214: resampler_->Resample(output_bus->frames(), output_bus.get()); nit: Add a comment that the resampler will invoke CastAudioSink::ProvideData() and consume all input data. https://codereview.chromium.org/187493002/diff/1/chrome/renderer/media/cast_r... chrome/renderer/media/cast_rtp_stream.cc:277: int output_channels_; nit: The two output_xxx_ members should be const.
https://codereview.chromium.org/187493002/diff/1/chrome/renderer/media/cast_r... File chrome/renderer/media/cast_rtp_stream.cc (right): https://codereview.chromium.org/187493002/diff/1/chrome/renderer/media/cast_r... chrome/renderer/media/cast_rtp_stream.cc:214: resampler_->Resample(output_bus->frames(), output_bus.get()); On 2014/03/05 20:59:00, miu wrote: > nit: Add a comment that the resampler will invoke CastAudioSink::ProvideData() > and consume all input data. Done. https://codereview.chromium.org/187493002/diff/1/chrome/renderer/media/cast_r... chrome/renderer/media/cast_rtp_stream.cc:277: int output_channels_; On 2014/03/05 20:59:00, miu wrote: > nit: The two output_xxx_ members should be const. Done.
The CQ bit was checked by hclam@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/hclam@chromium.org/187493002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on chromium_presubmit for step(s) presubmit http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...
The CQ bit was checked by hclam@chromium.org
The CQ bit was unchecked by hclam@chromium.org
+dalecurtis to add dependency on media/audio.
https://codereview.chromium.org/187493002/diff/20001/chrome/renderer/media/ca... File chrome/renderer/media/cast_rtp_stream.cc (right): https://codereview.chromium.org/187493002/diff/20001/chrome/renderer/media/ca... chrome/renderer/media/cast_rtp_stream.cc:217: resampler_->Resample(output_bus->frames(), output_bus.get()); While this guarantees all input data is consumed it doesn't mean you've gotten all available output frames. There may still be frames in the resampler. You'll need to do some math based on the input and output ratios to figure out how much. It'd be nice to make that calculation available on SincResampler. We have some upcoming changes from rileya@ which would use that information. An easier solution would be to use an AudioPullFifo you push the input data into and then pull from during Resample(). Note: Depending on the input sample rate you may be called back less than you expect here. I.e. the input data is 44KhZ and you want 48kHz, you're only going to get data delivered at 44kHz which may lead to stalls in your pipeline (I haven't dug into your code further to see if this is the case). https://codereview.chromium.org/187493002/diff/20001/chrome/renderer/media/ca... chrome/renderer/media/cast_rtp_stream.cc:252: input_bytes_per_frame_ = params.bits_per_sample() / 8; This is calculating bytes per channel, not bytes per frame? Rename?
Per our discussion I have updated the code to perform resampling correctly using a FIFO and buffer 10ms of data. This means the audio will be lagging at most 10ms behind but that's okay for now.
https://codereview.chromium.org/187493002/diff/40001/chrome/renderer/media/ca... File chrome/renderer/media/cast_rtp_stream.cc (right): https://codereview.chromium.org/187493002/diff/40001/chrome/renderer/media/ca... chrome/renderer/media/cast_rtp_stream.cc:212: scoped_ptr<media::AudioBus> input_bus = Don't create this every time, it can be quite expensive. |number_of_frames| should be constant, instead create it in SetFormat() below and just have a FromInterleaved here. I see you need at least one to pass to SendAudio, which kind of stinks, so choose whether it's easier avoid the allocation on output or input. https://codereview.chromium.org/187493002/diff/40001/chrome/renderer/media/ca... chrome/renderer/media/cast_rtp_stream.cc:253: scoped_ptr<media::AudioBus> ResampleData( Seems unnecessary to extract this to a new function given how little OnData() does. Up to you though. https://codereview.chromium.org/187493002/diff/40001/chrome/renderer/media/ca... chrome/renderer/media/cast_rtp_stream.cc:258: if (fifo_->frames() != fifo_->max_frames()) I think you just want to make sure you don't respond to the first callback. Like I said it's possible you get two callbacks in one request, you just won't get one at all on the next call to Resample(). https://codereview.chromium.org/187493002/diff/40001/chrome/renderer/media/ca... chrome/renderer/media/cast_rtp_stream.cc:261: scoped_ptr<media::AudioBus> output_bus( Ditto. Just return a bool if there's no data available. https://codereview.chromium.org/187493002/diff/40001/chrome/renderer/media/ca... chrome/renderer/media/cast_rtp_stream.cc:282: if (params.sample_rate() == output_sample_rate_) When and who calls this? Do you need to worry about blowing away the fifo or resampler while a callback is in progress above?
https://codereview.chromium.org/187493002/diff/40001/chrome/renderer/media/ca... File chrome/renderer/media/cast_rtp_stream.cc (right): https://codereview.chromium.org/187493002/diff/40001/chrome/renderer/media/ca... chrome/renderer/media/cast_rtp_stream.cc:212: scoped_ptr<media::AudioBus> input_bus = On 2014/03/07 01:14:03, DaleCurtis wrote: > Don't create this every time, it can be quite expensive. |number_of_frames| > should be constant, instead create it in SetFormat() below and just have a > FromInterleaved here. > > I see you need at least one to pass to SendAudio, which kind of stinks, so > choose whether it's easier avoid the allocation on output or input. I think the only way is to save on the allocation for the input. But then the code will be different to support having resampling or not. I actually prefer just doing the allocation and fix later if this turns out to be a problem for performance. https://codereview.chromium.org/187493002/diff/40001/chrome/renderer/media/ca... chrome/renderer/media/cast_rtp_stream.cc:253: scoped_ptr<media::AudioBus> ResampleData( On 2014/03/07 01:14:03, DaleCurtis wrote: > Seems unnecessary to extract this to a new function given how little OnData() > does. Up to you though. It looks cleaner to me to have a separate method for resample logic. https://codereview.chromium.org/187493002/diff/40001/chrome/renderer/media/ca... chrome/renderer/media/cast_rtp_stream.cc:258: if (fifo_->frames() != fifo_->max_frames()) On 2014/03/07 01:14:03, DaleCurtis wrote: > I think you just want to make sure you don't respond to the first callback. > Like I said it's possible you get two callbacks in one request, you just won't > get one at all on the next call to Resample(). I changed it to a preroll. https://codereview.chromium.org/187493002/diff/40001/chrome/renderer/media/ca... chrome/renderer/media/cast_rtp_stream.cc:261: scoped_ptr<media::AudioBus> output_bus( On 2014/03/07 01:14:03, DaleCurtis wrote: > Ditto. Just return a bool if there's no data available. Not sure I understand the suggestion here. https://codereview.chromium.org/187493002/diff/40001/chrome/renderer/media/ca... chrome/renderer/media/cast_rtp_stream.cc:282: if (params.sample_rate() == output_sample_rate_) On 2014/03/07 01:14:03, DaleCurtis wrote: > When and who calls this? Do you need to worry about blowing away the fifo or > resampler while a callback is in progress above? This is called once before there audio data arrives by the provider of audio data. I don't think we need to worry about parameters changed or this method called a couple times.
https://codereview.chromium.org/187493002/diff/40001/chrome/renderer/media/ca... File chrome/renderer/media/cast_rtp_stream.cc (right): https://codereview.chromium.org/187493002/diff/40001/chrome/renderer/media/ca... chrome/renderer/media/cast_rtp_stream.cc:212: scoped_ptr<media::AudioBus> input_bus = On 2014/03/07 01:34:01, Alpha wrote: > On 2014/03/07 01:14:03, DaleCurtis wrote: > > Don't create this every time, it can be quite expensive. |number_of_frames| > > should be constant, instead create it in SetFormat() below and just have a > > FromInterleaved here. > > > > I see you need at least one to pass to SendAudio, which kind of stinks, so > > choose whether it's easier avoid the allocation on output or input. > > I think the only way is to save on the allocation for the input. But then the > code will be different to support having resampling or not. I actually prefer > just doing the allocation and fix later if this turns out to be a problem for > performance. How about reflowing this block so that ResampleData() takes |audio_data| instead of an input_bus. Then you can allocate a resampler_input_ bus during SetFormat(). These callbacks happen a lot and allocating a large aligned memory block everytime in expensive. In testing elsewhere the slowdown was ~2.4x (on a z620 no less) vs not allocating everytime. https://codereview.chromium.org/187493002/diff/60001/chrome/renderer/media/ca... File chrome/renderer/media/cast_rtp_stream.cc (right): https://codereview.chromium.org/187493002/diff/60001/chrome/renderer/media/ca... chrome/renderer/media/cast_rtp_stream.cc:257: if (++input_preroll_ < kBufferAudioData) don't increment outside the conditional block otherwise for long streams you're going to overflow. Remember this will be called ~4800 times a second.
> How about reflowing this block so that ResampleData() takes |audio_data| instead > of an input_bus. Then you can allocate a resampler_input_ bus during > SetFormat(). These callbacks happen a lot and allocating a large aligned memory > block everytime in expensive. In testing elsewhere the slowdown was ~2.4x (on a > z620 no less) vs not allocating everytime. I've updated the code such that in the resampling path we have a preallocated buffer. > https://codereview.chromium.org/187493002/diff/60001/chrome/renderer/media/ca... > chrome/renderer/media/cast_rtp_stream.cc:257: if (++input_preroll_ < > kBufferAudioData) > don't increment outside the conditional block otherwise for long streams you're > going to overflow. Remember this will be called ~4800 times a second. Fixed this as well.
lgtm % nits. https://codereview.chromium.org/187493002/diff/80001/chrome/renderer/media/ca... File chrome/renderer/media/cast_rtp_stream.cc (right): https://codereview.chromium.org/187493002/diff/80001/chrome/renderer/media/ca... chrome/renderer/media/cast_rtp_stream.cc:221: number_of_channels, number_of_frames).Pass(); No .Pass necessary. https://codereview.chromium.org/187493002/diff/80001/chrome/renderer/media/ca... chrome/renderer/media/cast_rtp_stream.cc:271: output_sample_rate_ * fifo_input_bus_->frames() / sample_rate)); This should be fine for the sample rates your using, but in extreme cases, say 192kHz -> 44kHz you might overflow the AudioFifo. https://codereview.chromium.org/187493002/diff/80001/chrome/renderer/media/ca... chrome/renderer/media/cast_rtp_stream.cc:295: params.channels(), params.frames_per_buffer()).Pass(); No need for .Pass().
The CQ bit was checked by hclam@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/hclam@chromium.org/187493002/60002
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/hclam@chromium.org/187493002/60002
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on mac_rel for step(s) telemetry_perf_unittests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...
The CQ bit was checked by hclam@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/hclam@chromium.org/187493002/60002
Message was sent while issue was closed.
Change committed as 255893 |
