Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10701049:
Add MultiChannelResampler wrapper for SincResampler. (Closed)
|
Created:
8 years, 5 months ago by DaleCurtis Modified:
8 years, 5 months ago CC:
chromium-reviews, feature-media-reviews_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdd MultiChannelResampler wrapper for SincResampler.
Wraps the SincResampler for multichannel use. Can't land until after
https://chromiumcodereview.appspot.com/10702050/ lands.
Since the SincResampler unit tests focus on accuracy, the multi channel
resampling tests just ensure the wrapper functions as expected with a
single resampling frequency and a constant fill value.
BUG=133637
TEST=MultiChannelResampler Unittests.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=146254
Patch Set 1 : Initial Commit! #
Total comments: 57
Patch Set 2 : Comments! #
Total comments: 7
Patch Set 3 : Comments. Fixes. #Patch Set 4 : Take advantage of ChunkSize(). Reduce memcpy() by 1. #Patch Set 5 : Rebase. #
Messages
Total messages: 13 (0 generated)
PTAL. fischman: Since this is just a wrapper without audio specifics, please do a full review. crogers: Ditto.
Lots of comments but only a few threads interwoven among them :) https://chromiumcodereview.appspot.com/10701049/diff/5005/media/base/multi_ch... File media/base/multi_channel_resampler.cc (right): https://chromiumcodereview.appspot.com/10701049/diff/5005/media/base/multi_ch... media/base/multi_channel_resampler.cc:33: // Clean up |resampler_audio_data_|. With my scoped* suggestions in the .h file, this dtor should become empty. https://chromiumcodereview.appspot.com/10701049/diff/5005/media/base/multi_ch... media/base/multi_channel_resampler.cc:46: // We need to ensure that SincResampler only calls ProvideInput once for each An alternative, of course, is to allow SR to call PI as many times as it wants to satisfy the entire number_of_frames in one shot, and manage the buffering in this class (by tracking per-channel the first not-yet-handed-to-SR offset in the buffered audio data (and asking for more multichannel data when a channel's offset reaches its current buffered size). I actually suspect that would result in clearer code so if I were you I'd try that out and see whether my suspicion is correct. Your choice. https://chromiumcodereview.appspot.com/10701049/diff/5005/media/base/multi_ch... media/base/multi_channel_resampler.cc:93: DCHECK_EQ(channel_index_, callback_count_++); Bug: this line is compiled out of Release builds, so your ++ will disappear. https://chromiumcodereview.appspot.com/10701049/diff/5005/media/base/multi_ch... File media/base/multi_channel_resampler.h (right): https://chromiumcodereview.appspot.com/10701049/diff/5005/media/base/multi_ch... media/base/multi_channel_resampler.h:15: class MEDIA_EXPORT MultiChannelResampler Add a class-level comment (what this does, how it's useful). https://chromiumcodereview.appspot.com/10701049/diff/5005/media/base/multi_ch... media/base/multi_channel_resampler.h:18: // Abstract base-class for a pulling data into the resampler. english https://chromiumcodereview.appspot.com/10701049/diff/5005/media/base/multi_ch... media/base/multi_channel_resampler.h:19: class MultiChannelAudioSourceProvider { Is this a provider of "audio source"s or a provider of audio? (I suspect you want one of s/Source/Data/ s/Source// s/Provider// ) https://chromiumcodereview.appspot.com/10701049/diff/5005/media/base/multi_ch... media/base/multi_channel_resampler.h:23: virtual void ProvideInput(const std::vector<float*>& audio_data, |audio_data|'s format needs description. My guess is that the float* array has number_of_frames elements, and the outer vector has one element per channel(?). Any reason to mix styles instead of going with float** or vector<vector<float> > (my pref is for the latter, esp. if you can typedef your way to clarity typedef std::vector<float> SingleChannelSample; typedef std::vector<SingleChannelAudioData> MultiChannelSample; (modulo better names; I don't know the difference between sample & frame)) https://chromiumcodereview.appspot.com/10701049/diff/5005/media/base/multi_ch... media/base/multi_channel_resampler.h:23: virtual void ProvideInput(const std::vector<float*>& audio_data, When your interface is just one (or just a few) methods with no state, it can be clearer & more concise to use a callback: typedef base::Callback<void(const std::vector<float*>&, int)> DataProviderCB; The same applies to this class itself (being a Provider for the single-channel resampler). https://chromiumcodereview.appspot.com/10701049/diff/5005/media/base/multi_ch... media/base/multi_channel_resampler.h:24: int number_of_frames) = 0; fwiw, with a type of int, I think |frames| is clear enough (here and below) https://chromiumcodereview.appspot.com/10701049/diff/5005/media/base/multi_ch... media/base/multi_channel_resampler.h:33: // TODO(dalecurtis): Should provider be a const ref? Need to make ProvideInput TODO goes away with my callback suggestion above. https://chromiumcodereview.appspot.com/10701049/diff/5005/media/base/multi_ch... media/base/multi_channel_resampler.h:35: MultiChannelResampler(MultiChannelAudioSourceProvider* provider, generally output params follow input params, so this should be the last param, not the first https://chromiumcodereview.appspot.com/10701049/diff/5005/media/base/multi_ch... media/base/multi_channel_resampler.h:36: double scale_factor, int number_of_channels); s/scale_factor/io_sample_rate_ratio/ is clearer? https://chromiumcodereview.appspot.com/10701049/diff/5005/media/base/multi_ch... media/base/multi_channel_resampler.h:36: double scale_factor, int number_of_channels); FWIW, number_of_channels is implicit in the Provider call API, so you could drop it from here and lazily create resamplers_ on the first invocation of the provider API, and then DCHECK_EQ(resamplers_.size(), audio_data.size()); in the Provider. https://chromiumcodereview.appspot.com/10701049/diff/5005/media/base/multi_ch... media/base/multi_channel_resampler.h:43: int number_of_channels_; const https://chromiumcodereview.appspot.com/10701049/diff/5005/media/base/multi_ch... media/base/multi_channel_resampler.h:45: // Index of the channel currently being processed. This and the next two members are used mostly for sanity-checking. If you take my suggestion above to replace the provider interface with a callback, then you get to Bind the expected params into the callback and avoid the need for these explicit members. IOW the only reason you want these members now is to track state as control flows between different methods of this class, but those methods are all invoked in a single stack and you can curry these expectations into the relevant functions when you use base::Bind. https://chromiumcodereview.appspot.com/10701049/diff/5005/media/base/multi_ch... media/base/multi_channel_resampler.h:59: std::vector<SincResampler*> resamplers_; ScopedVector https://chromiumcodereview.appspot.com/10701049/diff/5005/media/base/multi_ch... media/base/multi_channel_resampler.h:63: std::vector<float*> resampler_audio_data_; vector<scoped_array<float> > would be clearer about who owns what. https://chromiumcodereview.appspot.com/10701049/diff/5005/media/base/multi_ch... media/base/multi_channel_resampler.h:66: // called once for each channel, starting with the first channel. Each time s/starting with the first channel/in channel order/ https://chromiumcodereview.appspot.com/10701049/diff/5005/media/base/multi_ch... File media/base/multi_channel_resampler_unittest.cc (right): https://chromiumcodereview.appspot.com/10701049/diff/5005/media/base/multi_ch... media/base/multi_channel_resampler_unittest.cc:17: static const float kScaleFactor = 44100.0 / 48000.0; probably need 'f's to disambiguate double-vs-float (MSVS is pickier than clang/gcc IIRC) https://chromiumcodereview.appspot.com/10701049/diff/5005/media/base/multi_ch... media/base/multi_channel_resampler_unittest.cc:26: static const float kFillValue = 0.1; needs 'f' suffix https://chromiumcodereview.appspot.com/10701049/diff/5005/media/base/multi_ch... media/base/multi_channel_resampler_unittest.cc:28: // Chosen arbitrarily on what each resampler reported during testing. s/on/based on/ https://chromiumcodereview.appspot.com/10701049/diff/5005/media/base/multi_ch... media/base/multi_channel_resampler_unittest.cc:34: extra \n https://chromiumcodereview.appspot.com/10701049/diff/5005/media/base/multi_ch... media/base/multi_channel_resampler_unittest.cc:112: std::vector<float*> audio_data_; scoped* to avoid dtor work https://chromiumcodereview.appspot.com/10701049/diff/5005/media/base/multi_ch... media/base/multi_channel_resampler_unittest.cc:117: TEST_F(MultiChannelProviderTest, MultiChannel_1_HighLatency) { Any reason not to use parameterized gtest?
Thanks for the fast review Ami! Just a couple comment responses here, no changes yet. http://codereview.chromium.org/10701049/diff/5005/media/base/multi_channel_re... File media/base/multi_channel_resampler.cc (right): http://codereview.chromium.org/10701049/diff/5005/media/base/multi_channel_re... media/base/multi_channel_resampler.cc:46: // We need to ensure that SincResampler only calls ProvideInput once for each On 2012/06/30 20:12:14, Ami Fischman wrote: > An alternative, of course, is to allow SR to call PI as many times as it wants > to satisfy the entire number_of_frames in one shot, and manage the buffering in > this class (by tracking per-channel the first not-yet-handed-to-SR offset in the > buffered audio data (and asking for more multichannel data when a channel's > offset reaches its current buffered size). > > I actually suspect that would result in clearer code so if I were you I'd try > that out and see whether my suspicion is correct. Your choice. Yeah, I'm not super happy with the chunking design here since it relies on internal details of SincResampler. That's how it's implemented in WebKit though. I didn't go the internal management route because that means reallocating buffers on every call unless I fix number_of_frames at construction which is contrary (and a little confusing due to scale_factor / SincResampler implementation) to the RenderCallback design. http://codereview.chromium.org/10701049/diff/5005/media/base/multi_channel_re... media/base/multi_channel_resampler.cc:93: DCHECK_EQ(channel_index_, callback_count_++); On 2012/06/30 20:12:14, Ami Fischman wrote: > Bug: this line is compiled out of Release builds, so your ++ will disappear. Whoops! I wonder if that's the source of my memory corruption in the CL that ties this all together. http://codereview.chromium.org/10701049/diff/5005/media/base/multi_channel_re... File media/base/multi_channel_resampler.h (right): http://codereview.chromium.org/10701049/diff/5005/media/base/multi_channel_re... media/base/multi_channel_resampler.h:23: virtual void ProvideInput(const std::vector<float*>& audio_data, On 2012/06/30 20:12:14, Ami Fischman wrote: > |audio_data|'s format needs description. > My guess is that the float* array has number_of_frames elements, and the outer > vector has one element per channel(?). Any reason to mix styles instead of > going with float** or vector<vector<float> > (my pref is for the latter, esp. > if you can typedef your way to clarity > typedef std::vector<float> SingleChannelSample; > typedef std::vector<SingleChannelAudioData> MultiChannelSample; > > (modulo better names; I don't know the difference between sample & frame)) I'm keeping style with AudioRendererSink::RenderCallback, since that will be the primary caller of this API: http://git.chromium.org/gitweb/?p=chromium.git&a=blob&f=media/base/audio_rend... Forgot about callbacks while doing the port, that's a much better fit! http://codereview.chromium.org/10701049/diff/5005/media/base/multi_channel_re... media/base/multi_channel_resampler.h:24: int number_of_frames) = 0; On 2012/06/30 20:12:14, Ami Fischman wrote: > fwiw, with a type of int, I think |frames| is clear enough (here and below) Ditto. I prefer frames as well, but am going for consistency unless you think that's overrated. http://codereview.chromium.org/10701049/diff/5005/media/base/multi_channel_re... media/base/multi_channel_resampler.h:63: std::vector<float*> resampler_audio_data_; On 2012/06/30 20:12:14, Ami Fischman wrote: > vector<scoped_array<float> > would be clearer about who owns what. I don't think I can pass that along via RenderCallback style interface unfortunately.
http://codereview.chromium.org/10701049/diff/5005/media/base/multi_channel_re... File media/base/multi_channel_resampler.cc (right): http://codereview.chromium.org/10701049/diff/5005/media/base/multi_channel_re... media/base/multi_channel_resampler.cc:46: // We need to ensure that SincResampler only calls ProvideInput once for each On 2012/07/01 23:27:46, DaleCurtis wrote: > I didn't go the internal management route because that means reallocating > buffers on every call unless I fix number_of_frames at construction which is > contrary (and a little confusing due to scale_factor / SincResampler > implementation) to the RenderCallback design. I don't follow. Why would you need to reallocate on every call? http://codereview.chromium.org/10701049/diff/5005/media/base/multi_channel_re... File media/base/multi_channel_resampler.h (right): http://codereview.chromium.org/10701049/diff/5005/media/base/multi_channel_re... media/base/multi_channel_resampler.h:24: int number_of_frames) = 0; On 2012/07/01 23:27:46, DaleCurtis wrote: > On 2012/06/30 20:12:14, Ami Fischman wrote: > > fwiw, with a type of int, I think |frames| is clear enough (here and below) > > Ditto. I prefer frames as well, but am going for consistency unless you think > that's overrated. Yep, in this case. (note, also, consistency can work the other way; you can change the other places to match). But it's obviously a pretty low-importance bit... http://codereview.chromium.org/10701049/diff/5005/media/base/multi_channel_re... media/base/multi_channel_resampler.h:63: std::vector<float*> resampler_audio_data_; On 2012/07/01 23:27:46, DaleCurtis wrote: > On 2012/06/30 20:12:14, Ami Fischman wrote: > > vector<scoped_array<float> > would be clearer about who owns what. > > I don't think I can pass that along via RenderCallback style interface > unfortunately. Ouch (great example of how API choices propagate...). You could re-create a vector of raw ptrs per call, or you could have a vector<scoped_array<>> as a deleter_, but probably neither is better/clearer than just adding a comment here that this class owns the pointed-to arrays. (and possibly extend the TODO on the upstream Render() that threatens to convert to a callback that something with clearer ownership semantics would be nicer).
http://codereview.chromium.org/10701049/diff/5005/media/base/multi_channel_re... File media/base/multi_channel_resampler.cc (right): http://codereview.chromium.org/10701049/diff/5005/media/base/multi_channel_re... media/base/multi_channel_resampler.cc:46: // We need to ensure that SincResampler only calls ProvideInput once for each On 2012/07/01 23:44:44, Ami Fischman wrote: > On 2012/07/01 23:27:46, DaleCurtis wrote: > > I didn't go the internal management route because that means reallocating > > buffers on every call unless I fix number_of_frames at construction which is > > contrary (and a little confusing due to scale_factor / SincResampler > > implementation) to the RenderCallback design. > > I don't follow. Why would you need to reallocate on every call? If SR calls PI as many times as it wants for a single channel we have to buffer the data for all the other channels (at the time of call) for as many times as SR needs to call. There's no way to get just a single channel of audio data back with the current RenderCallback design. I could lazy-initialize and resize on the first channel assuming every call to MCR::PI would ask for the same amount. That's not very clean either though. Am I missing something? http://codereview.chromium.org/10701049/diff/5005/media/base/multi_channel_re... File media/base/multi_channel_resampler.h (right): http://codereview.chromium.org/10701049/diff/5005/media/base/multi_channel_re... media/base/multi_channel_resampler.h:24: int number_of_frames) = 0; On 2012/07/01 23:44:44, Ami Fischman wrote: > On 2012/07/01 23:27:46, DaleCurtis wrote: > > On 2012/06/30 20:12:14, Ami Fischman wrote: > > > fwiw, with a type of int, I think |frames| is clear enough (here and below) > > > > Ditto. I prefer frames as well, but am going for consistency unless you think > > that's overrated. > > Yep, in this case. > (note, also, consistency can work the other way; you can change the other places > to match). > But it's obviously a pretty low-importance bit... sgtm. http://codereview.chromium.org/10701049/diff/5005/media/base/multi_channel_re... media/base/multi_channel_resampler.h:63: std::vector<float*> resampler_audio_data_; On 2012/07/01 23:44:44, Ami Fischman wrote: > On 2012/07/01 23:27:46, DaleCurtis wrote: > > On 2012/06/30 20:12:14, Ami Fischman wrote: > > > vector<scoped_array<float> > would be clearer about who owns what. > > > > I don't think I can pass that along via RenderCallback style interface > > unfortunately. > > Ouch (great example of how API choices propagate...). > You could re-create a vector of raw ptrs per call, or you could have a > vector<scoped_array<>> as a deleter_, but probably neither is better/clearer > than just adding a comment here that this class owns the pointed-to arrays. > (and possibly extend the TODO on the upstream Render() that threatens to convert > to a callback that something with clearer ownership semantics would be nicer). Actually, I don't know what I was talking about. I don't have to pass this memory anywhere it's used only inside this class. I'll change it to vector<scoped...
http://codereview.chromium.org/10701049/diff/5005/media/base/multi_channel_re... File media/base/multi_channel_resampler.cc (right): http://codereview.chromium.org/10701049/diff/5005/media/base/multi_channel_re... media/base/multi_channel_resampler.cc:33: // Clean up |resampler_audio_data_|. On 2012/06/30 20:12:14, Ami Fischman wrote: > With my scoped* suggestions in the .h file, this dtor should become empty. Removed clean up of resamplers_ http://codereview.chromium.org/10701049/diff/5005/media/base/multi_channel_re... media/base/multi_channel_resampler.cc:46: // We need to ensure that SincResampler only calls ProvideInput once for each On 2012/07/02 17:43:45, DaleCurtis wrote: > On 2012/07/01 23:44:44, Ami Fischman wrote: > > On 2012/07/01 23:27:46, DaleCurtis wrote: > > > I didn't go the internal management route because that means reallocating > > > buffers on every call unless I fix number_of_frames at construction which is > > > contrary (and a little confusing due to scale_factor / SincResampler > > > implementation) to the RenderCallback design. > > > > I don't follow. Why would you need to reallocate on every call? > > If SR calls PI as many times as it wants for a single channel we have to buffer > the data for all the other channels (at the time of call) for as many times as > SR needs to call. There's no way to get just a single channel of audio data back > with the current RenderCallback design. > > I could lazy-initialize and resize on the first channel assuming every call to > MCR::PI would ask for the same amount. That's not very clean either though. > > Am I missing something? After discussion and partial implementation this is a bit tricky and certainly more complicated than the current implementation since we'd reallocate buffers, track both read pointers and write pointers in a secondary vector<float*> for calling into read_cb_. Keeping the current implementation for now. http://codereview.chromium.org/10701049/diff/5005/media/base/multi_channel_re... media/base/multi_channel_resampler.cc:93: DCHECK_EQ(channel_index_, callback_count_++); On 2012/07/01 23:27:46, DaleCurtis wrote: > On 2012/06/30 20:12:14, Ami Fischman wrote: > > Bug: this line is compiled out of Release builds, so your ++ will disappear. > > Whoops! I wonder if that's the source of my memory corruption in the CL that > ties this all together. Actually this doesn't matter as it's only used for DCHECK'ing anyways. I can move it out if you prefer, but I think it makes sense to keep in the DCHECK since it's unused elsewhere. http://codereview.chromium.org/10701049/diff/5005/media/base/multi_channel_re... File media/base/multi_channel_resampler.h (right): http://codereview.chromium.org/10701049/diff/5005/media/base/multi_channel_re... media/base/multi_channel_resampler.h:15: class MEDIA_EXPORT MultiChannelResampler On 2012/06/30 20:12:14, Ami Fischman wrote: > Add a class-level comment (what this does, how it's useful). Done. http://codereview.chromium.org/10701049/diff/5005/media/base/multi_channel_re... media/base/multi_channel_resampler.h:18: // Abstract base-class for a pulling data into the resampler. On 2012/06/30 20:12:14, Ami Fischman wrote: > english Removed. http://codereview.chromium.org/10701049/diff/5005/media/base/multi_channel_re... media/base/multi_channel_resampler.h:19: class MultiChannelAudioSourceProvider { On 2012/06/30 20:12:14, Ami Fischman wrote: > Is this a provider of "audio source"s or a provider of audio? > (I suspect you want one of > s/Source/Data/ > s/Source// > s/Provider// > ) Just ReadCB now. http://codereview.chromium.org/10701049/diff/5005/media/base/multi_channel_re... media/base/multi_channel_resampler.h:23: virtual void ProvideInput(const std::vector<float*>& audio_data, On 2012/07/01 23:27:46, DaleCurtis wrote: > On 2012/06/30 20:12:14, Ami Fischman wrote: > > |audio_data|'s format needs description. > > My guess is that the float* array has number_of_frames elements, and the outer > > vector has one element per channel(?). Any reason to mix styles instead of > > going with float** or vector<vector<float> > (my pref is for the latter, esp. > > if you can typedef your way to clarity > > typedef std::vector<float> SingleChannelSample; > > typedef std::vector<SingleChannelAudioData> MultiChannelSample; > > > > (modulo better names; I don't know the difference between sample & frame)) > > I'm keeping style with AudioRendererSink::RenderCallback, since that will be the > primary caller of this API: > > http://git.chromium.org/gitweb/?p=chromium.git&a=blob&f=media/base/audio_rend... > > Forgot about callbacks while doing the port, that's a much better fit! Converted to Callback, kept RenderCallback style. http://codereview.chromium.org/10701049/diff/5005/media/base/multi_channel_re... media/base/multi_channel_resampler.h:24: int number_of_frames) = 0; On 2012/07/02 17:43:45, DaleCurtis wrote: > On 2012/07/01 23:44:44, Ami Fischman wrote: > > On 2012/07/01 23:27:46, DaleCurtis wrote: > > > On 2012/06/30 20:12:14, Ami Fischman wrote: > > > > fwiw, with a type of int, I think |frames| is clear enough (here and > below) > > > > > > Ditto. I prefer frames as well, but am going for consistency unless you > think > > > that's overrated. > > > > Yep, in this case. > > (note, also, consistency can work the other way; you can change the other > places > > to match). > > But it's obviously a pretty low-importance bit... > > sgtm. Done. http://codereview.chromium.org/10701049/diff/5005/media/base/multi_channel_re... media/base/multi_channel_resampler.h:33: // TODO(dalecurtis): Should provider be a const ref? Need to make ProvideInput On 2012/06/30 20:12:14, Ami Fischman wrote: > TODO goes away with my callback suggestion above. Removed. http://codereview.chromium.org/10701049/diff/5005/media/base/multi_channel_re... media/base/multi_channel_resampler.h:35: MultiChannelResampler(MultiChannelAudioSourceProvider* provider, On 2012/06/30 20:12:14, Ami Fischman wrote: > generally output params follow input params, so this should be the last param, > not the first These are all input parameters? I moved the CB to the end though assuming that's what you're referring to. http://codereview.chromium.org/10701049/diff/5005/media/base/multi_channel_re... media/base/multi_channel_resampler.h:36: double scale_factor, int number_of_channels); On 2012/06/30 20:12:14, Ami Fischman wrote: > FWIW, number_of_channels is implicit in the Provider call API, so you could drop > it from here and lazily create resamplers_ on the first invocation of the > provider API, and then > DCHECK_EQ(resamplers_.size(), audio_data.size()); > in the Provider. Doesn't really save anything and makes the already complicated resample() call more complicated. Kept as is, but dropped the channel_count_ / channel_index stuff. http://codereview.chromium.org/10701049/diff/5005/media/base/multi_channel_re... media/base/multi_channel_resampler.h:43: int number_of_channels_; On 2012/06/30 20:12:14, Ami Fischman wrote: > const Removed. http://codereview.chromium.org/10701049/diff/5005/media/base/multi_channel_re... media/base/multi_channel_resampler.h:45: // Index of the channel currently being processed. On 2012/06/30 20:12:14, Ami Fischman wrote: > This and the next two members are used mostly for sanity-checking. If you take > my suggestion above to replace the provider interface with a callback, then you > get to Bind the expected params into the callback and avoid the need for these > explicit members. > > IOW the only reason you want these members now is to track state as control > flows between different methods of this class, but those methods are all invoked > in a single stack and you can curry these expectations into the relevant > functions when you use base::Bind. I was able to bind away channel_index_, but I don't see how the other two can go away (assuming we keep the sanity checks). http://codereview.chromium.org/10701049/diff/5005/media/base/multi_channel_re... media/base/multi_channel_resampler.h:59: std::vector<SincResampler*> resamplers_; On 2012/06/30 20:12:14, Ami Fischman wrote: > ScopedVector Done. http://codereview.chromium.org/10701049/diff/5005/media/base/multi_channel_re... media/base/multi_channel_resampler.h:63: std::vector<float*> resampler_audio_data_; On 2012/07/02 17:43:45, DaleCurtis wrote: > On 2012/07/01 23:44:44, Ami Fischman wrote: > > On 2012/07/01 23:27:46, DaleCurtis wrote: > > > On 2012/06/30 20:12:14, Ami Fischman wrote: > > > > vector<scoped_array<float> > would be clearer about who owns what. > > > > > > I don't think I can pass that along via RenderCallback style interface > > > unfortunately. > > > > Ouch (great example of how API choices propagate...). > > You could re-create a vector of raw ptrs per call, or you could have a > > vector<scoped_array<>> as a deleter_, but probably neither is better/clearer > > than just adding a comment here that this class owns the pointed-to arrays. > > (and possibly extend the TODO on the upstream Render() that threatens to > convert > > to a callback that something with clearer ownership semantics would be nicer). > > Actually, I don't know what I was talking about. I don't have to pass this > memory anywhere it's used only inside this class. I'll change it to > vector<scoped... Actually, that's not true, I remember what I was talking about :) This needs to be float* for when the MCR::ReadCB is invoked on the caller. I'll update the comment. http://codereview.chromium.org/10701049/diff/5005/media/base/multi_channel_re... media/base/multi_channel_resampler.h:66: // called once for each channel, starting with the first channel. Each time On 2012/06/30 20:12:14, Ami Fischman wrote: > s/starting with the first channel/in channel order/ Done. http://codereview.chromium.org/10701049/diff/5005/media/base/multi_channel_re... File media/base/multi_channel_resampler_unittest.cc (right): http://codereview.chromium.org/10701049/diff/5005/media/base/multi_channel_re... media/base/multi_channel_resampler_unittest.cc:17: static const float kScaleFactor = 44100.0 / 48000.0; On 2012/06/30 20:12:14, Ami Fischman wrote: > probably need 'f's to disambiguate double-vs-float (MSVS is pickier than > clang/gcc IIRC) Done. http://codereview.chromium.org/10701049/diff/5005/media/base/multi_channel_re... media/base/multi_channel_resampler_unittest.cc:26: static const float kFillValue = 0.1; On 2012/06/30 20:12:14, Ami Fischman wrote: > needs 'f' suffix Done. http://codereview.chromium.org/10701049/diff/5005/media/base/multi_channel_re... media/base/multi_channel_resampler_unittest.cc:28: // Chosen arbitrarily on what each resampler reported during testing. On 2012/06/30 20:12:14, Ami Fischman wrote: > s/on/based on/ Done. http://codereview.chromium.org/10701049/diff/5005/media/base/multi_channel_re... media/base/multi_channel_resampler_unittest.cc:34: On 2012/06/30 20:12:14, Ami Fischman wrote: > extra \n Done. http://codereview.chromium.org/10701049/diff/5005/media/base/multi_channel_re... media/base/multi_channel_resampler_unittest.cc:112: std::vector<float*> audio_data_; On 2012/06/30 20:12:14, Ami Fischman wrote: > scoped* to avoid dtor work Can't due to RenderCallback interface. http://codereview.chromium.org/10701049/diff/5005/media/base/multi_channel_re... media/base/multi_channel_resampler_unittest.cc:117: TEST_F(MultiChannelProviderTest, MultiChannel_1_HighLatency) { On 2012/06/30 20:12:14, Ami Fischman wrote: > Any reason not to use parameterized gtest? Done.
LGTM % nits http://codereview.chromium.org/10701049/diff/11002/media/base/multi_channel_r... File media/base/multi_channel_resampler.cc (right): http://codereview.chromium.org/10701049/diff/11002/media/base/multi_channel_r... media/base/multi_channel_resampler.cc:22: resamplers_.push_back(new SincResampler( multi-line for bodies require braces http://codereview.chromium.org/10701049/diff/11002/media/base/multi_channel_r... media/base/multi_channel_resampler.cc:23: base::Bind( could go on prev line http://codereview.chromium.org/10701049/diff/11002/media/base/multi_channel_r... media/base/multi_channel_resampler.cc:86: DCHECK_EQ(channel, callback_count_++); I think it's poor form to have a variable that sits at 0 for the lifetime of the process in Release. You might just promote this to a CHECK (with a comment explaining you want the side-effect, always). http://codereview.chromium.org/10701049/diff/11002/media/base/multi_channel_r... media/base/multi_channel_resampler.cc:89: memcpy(destination, resampler_audio_data_[channel], It'd be nicer to avoid this copy, but I guess that'd require overhauling SR's API.
Thanks! This patch set also fixes chunking to be based on kBlockSize / io_sample_rate_ratio and changes the unit test to hit this case. Will wait for crogers approval and SincResampler to land. http://codereview.chromium.org/10701049/diff/11002/media/base/multi_channel_r... File media/base/multi_channel_resampler.cc (right): http://codereview.chromium.org/10701049/diff/11002/media/base/multi_channel_r... media/base/multi_channel_resampler.cc:22: resamplers_.push_back(new SincResampler( On 2012/07/03 18:53:12, Ami Fischman wrote: > multi-line for bodies require braces Done. http://codereview.chromium.org/10701049/diff/11002/media/base/multi_channel_r... media/base/multi_channel_resampler.cc:23: base::Bind( On 2012/07/03 18:53:12, Ami Fischman wrote: > could go on prev line Done. http://codereview.chromium.org/10701049/diff/11002/media/base/multi_channel_r... media/base/multi_channel_resampler.cc:86: DCHECK_EQ(channel, callback_count_++); On 2012/07/03 18:53:12, Ami Fischman wrote: > I think it's poor form to have a variable that sits at 0 for the lifetime of the > process in Release. You might just promote this to a CHECK (with a comment > explaining you want the side-effect, always). Switched to DCHECK and moved ++ out.
LGTM
On 2012/07/09 22:28:51, Chris Rogers wrote: > LGTM Now updated to take advantage of SincResampler::ChunkSize() and removes one memcpy; which is a 50% improvement in the common stereo case! :)
Still LGTM On 2012/07/10 22:23:16, DaleCurtis wrote: > On 2012/07/09 22:28:51, Chris Rogers wrote: > > LGTM > > Now updated to take advantage of SincResampler::ChunkSize() and removes one > memcpy; which is a 50% improvement in the common stereo case! :)
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dalecurtis@chromium.org/10701049/11010
Change committed as 146254 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
