Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10829183:
Introduce AudioBus to replace std::vector<float*>. (Closed)
|
Created:
8 years, 4 months ago by DaleCurtis Modified:
8 years, 4 months ago CC:
chromium-reviews, feature-media-reviews_chromium.org, tommi (sloooow) - chröme Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionIntroduce AudioBus to replace std::vector<float*> within
AudioRenderSink::RenderCallback.
The first step towards unifying our pipeline on float. Adds a
new AudioBus class for managing audio channel data. Includes
helper methods for shared memory transfers and common tasks
such as zeroing the audio data. Inspired by WebKit's AudioBus.
BUG=114700
TEST=Unit tests.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=150660
Patch Set 1 : Cleanup. #
Total comments: 34
Patch Set 2 : Comments. #
Total comments: 28
Patch Set 3 : Mo' Comments! #Patch Set 4 : Line endings? #
Messages
Total messages: 17 (0 generated)
PTAL. A preliminary look at what this looks like can be seen here https://chromiumcodereview.appspot.com/10823175 (only enough files to get media_unittests to compile) cc: henrika, tommi since they are both about to head out on vacation.
On 2012/08/04 02:34:03, DaleCurtis wrote: > PTAL. A preliminary look at what this looks like can be seen here > https://chromiumcodereview.appspot.com/10823175 (only enough files to get > media_unittests to compile) > > cc: henrika, tommi since they are both about to head out on vacation. Forgot to mention, this patch set assumes we'll memcpy into and out of the shared memory. If it's kosher to wrap a portion of the shared memory, we can lose the overhead of the memcpy on the renderer side.
nice! mostly nits http://codereview.chromium.org/10829183/diff/5001/media/base/audio_bus.cc File media/base/audio_bus.cc (right): http://codereview.chromium.org/10829183/diff/5001/media/base/audio_bus.cc#new... media/base/audio_bus.cc:39: DCHECK_EQ(reinterpret_cast<uintptr_t>(audio_data_vector_[i]) & idea: add static IsAligned() function at top by constant for a more readable DCHECK http://codereview.chromium.org/10829183/diff/5001/media/base/audio_bus.h File media/base/audio_bus.h (right): http://codereview.chromium.org/10829183/diff/5001/media/base/audio_bus.h#newc... media/base/audio_bus.h:21: ~AudioBus(); if you only return scoped_ptr<> this can be private w/ scoped_ptr<AudioBus> a friend class I notice in 10823175 that you're passing raw pointers, do doing this will forbid those areas of code from being able to delete the pointer http://codereview.chromium.org/10829183/diff/5001/media/base/audio_bus.h#newc... media/base/audio_bus.h:27: // AudioParameters structure. Additionally sets |channel_layout_|. nit: I'd just move this up by the other Create() and have one comment for both http://codereview.chromium.org/10829183/diff/5001/media/base/audio_bus.h#newc... media/base/audio_bus.h:30: // Creates a new AudioBus and from an existing channel vector. s/and// I'd doc that this isn't ownership transfer nor a copy and that the lifetime of the float* inside |audio_data_vector| must outlive this object http://codereview.chromium.org/10829183/diff/5001/media/base/audio_bus.h#newc... media/base/audio_bus.h:33: const std::vector<float*>& audio_data_vector); s/audio_data_vector/channel_data? http://codereview.chromium.org/10829183/diff/5001/media/base/audio_bus.h#newc... media/base/audio_bus.h:35: // Returns a raw pointer to internal data for reconstruction of an AudioBus I'd drop reference to shared memory -- that's something content/ specific http://codereview.chromium.org/10829183/diff/5001/media/base/audio_bus.h#newc... media/base/audio_bus.h:38: void* audio_data() { s/audio_// here + below (no need to prefix methods w/ audio for a class that has audio in the name) http://codereview.chromium.org/10829183/diff/5001/media/base/audio_bus.h#newc... media/base/audio_bus.h:39: DCHECK(audio_data_.get()); I see what you mean about the DCHECK() -- I would de-inline and move to .cc http://codereview.chromium.org/10829183/diff/5001/media/base/audio_bus.h#newc... media/base/audio_bus.h:53: const float* channel(int channel) const { what's up with the overloaded const version? where are we using const AudioBus objects? http://codereview.chromium.org/10829183/diff/5001/media/base/audio_bus.h#newc... media/base/audio_bus.h:62: ChannelLayout channel_layout() const { return channel_layout_; } I don't see channel layout used in 10823175 -- can we remove until needed? http://codereview.chromium.org/10829183/diff/5001/media/base/audio_bus.h#newc... media/base/audio_bus.h:71: void Zero() { Zero(frames_); } instead of overload (generally avoided), how about ZeroAll OR change Zero(frames) to ZeroFrames() http://codereview.chromium.org/10829183/diff/5001/media/base/audio_bus.h#newc... media/base/audio_bus.h:75: AudioBus(int frames, const std::vector<float*>& audio_data_vector); ditto http://codereview.chromium.org/10829183/diff/5001/media/base/audio_bus.h#newc... media/base/audio_bus.h:78: scoped_ptr_malloc<float, base::ScopedPtrAlignedFree> audio_data_; similar to above this can be data_ if you want http://codereview.chromium.org/10829183/diff/5001/media/base/audio_bus.h#newc... media/base/audio_bus.h:82: std::vector<float*> audio_data_vector_; similar to above this can be channel_data_
Excellent work, this looks great. As a little background, the class this was roughly based on in WebKit has been extremely helpful as a high-level abstraction for moving audio data around. http://codereview.chromium.org/10829183/diff/5001/media/base/audio_bus.cc File media/base/audio_bus.cc (right): http://codereview.chromium.org/10829183/diff/5001/media/base/audio_bus.cc#new... media/base/audio_bus.cc:19: // Choose a size such that each channel is aligned by kChannelAlignment. do we want sanity checking DCHECKS, etc. on channels and frames (negative values, wildly high values?) http://codereview.chromium.org/10829183/diff/5001/media/base/audio_bus.cc#new... media/base/audio_bus.cc:22: audio_data_size_ = sizeof(float) * channels * aligned_frames; The security guys seemed keen on using size_t for all of this kind of stuff: http://trac.webkit.org/changeset/93749 Not sure if this is an issue, but you could talk with Chris Palmer... Seems like we'd at least want sanity check for negative value if we're using int... http://codereview.chromium.org/10829183/diff/5001/media/base/audio_bus.cc#new... media/base/audio_bus.cc:27: // Seperate audio data out into channels for easy lookup later. typo: Seperate
http://codereview.chromium.org/10829183/diff/5001/media/base/audio_bus.cc File media/base/audio_bus.cc (right): http://codereview.chromium.org/10829183/diff/5001/media/base/audio_bus.cc#new... media/base/audio_bus.cc:19: // Choose a size such that each channel is aligned by kChannelAlignment. On 2012/08/06 19:40:26, Chris Rogers wrote: > do we want sanity checking DCHECKS, etc. on channels and frames (negative > values, wildly high values?) Done. http://codereview.chromium.org/10829183/diff/5001/media/base/audio_bus.cc#new... media/base/audio_bus.cc:22: audio_data_size_ = sizeof(float) * channels * aligned_frames; On 2012/08/06 19:40:26, Chris Rogers wrote: > The security guys seemed keen on using size_t for all of this kind of stuff: > http://trac.webkit.org/changeset/93749 > > Not sure if this is an issue, but you could talk with Chris Palmer... > > Seems like we'd at least want sanity check for negative value if we're using > int... Used CHECK() above plus limits to ensure this can't happen. http://codereview.chromium.org/10829183/diff/5001/media/base/audio_bus.cc#new... media/base/audio_bus.cc:27: // Seperate audio data out into channels for easy lookup later. On 2012/08/06 19:40:26, Chris Rogers wrote: > typo: Seperate Done. http://codereview.chromium.org/10829183/diff/5001/media/base/audio_bus.cc#new... media/base/audio_bus.cc:39: DCHECK_EQ(reinterpret_cast<uintptr_t>(audio_data_vector_[i]) & On 2012/08/06 18:55:34, scherkus wrote: > idea: add static IsAligned() function at top by constant for a more readable > DCHECK Done. http://codereview.chromium.org/10829183/diff/5001/media/base/audio_bus.h File media/base/audio_bus.h (right): http://codereview.chromium.org/10829183/diff/5001/media/base/audio_bus.h#newc... media/base/audio_bus.h:21: ~AudioBus(); On 2012/08/06 18:55:34, scherkus wrote: > if you only return scoped_ptr<> this can be private w/ scoped_ptr<AudioBus> a > friend class > > I notice in 10823175 that you're passing raw pointers, do doing this will forbid > those areas of code from being able to delete the pointer Nice! http://codereview.chromium.org/10829183/diff/5001/media/base/audio_bus.h#newc... media/base/audio_bus.h:27: // AudioParameters structure. Additionally sets |channel_layout_|. On 2012/08/06 18:55:34, scherkus wrote: > nit: I'd just move this up by the other Create() and have one comment for both Done. http://codereview.chromium.org/10829183/diff/5001/media/base/audio_bus.h#newc... media/base/audio_bus.h:30: // Creates a new AudioBus and from an existing channel vector. On 2012/08/06 18:55:34, scherkus wrote: > s/and// > > I'd doc that this isn't ownership transfer nor a copy and that the lifetime of > the float* inside |audio_data_vector| must outlive this object Done. http://codereview.chromium.org/10829183/diff/5001/media/base/audio_bus.h#newc... media/base/audio_bus.h:33: const std::vector<float*>& audio_data_vector); On 2012/08/06 18:55:34, scherkus wrote: > s/audio_data_vector/channel_data? Done. http://codereview.chromium.org/10829183/diff/5001/media/base/audio_bus.h#newc... media/base/audio_bus.h:35: // Returns a raw pointer to internal data for reconstruction of an AudioBus On 2012/08/06 18:55:34, scherkus wrote: > I'd drop reference to shared memory -- that's something content/ specific Done. http://codereview.chromium.org/10829183/diff/5001/media/base/audio_bus.h#newc... media/base/audio_bus.h:38: void* audio_data() { On 2012/08/06 18:55:34, scherkus wrote: > s/audio_// here + below (no need to prefix methods w/ audio for a class that has > audio in the name) Done. http://codereview.chromium.org/10829183/diff/5001/media/base/audio_bus.h#newc... media/base/audio_bus.h:39: DCHECK(audio_data_.get()); On 2012/08/06 18:55:34, scherkus wrote: > I see what you mean about the DCHECK() -- I would de-inline and move to .cc Done. http://codereview.chromium.org/10829183/diff/5001/media/base/audio_bus.h#newc... media/base/audio_bus.h:53: const float* channel(int channel) const { On 2012/08/06 18:55:34, scherkus wrote: > what's up with the overloaded const version? where are we using const AudioBus > objects? In AudioUtil::InterleaveFloatToInt, see: http://codereview.chromium.org/10823175/diff/1/media/audio/audio_util.cc SincResampler/MultiChannelResampler should actually take a const ref as input too. Possibly other use cases I'll find during conversion. In any case, I ended up not needing the overload, since technically channel() can be marked const. http://codereview.chromium.org/10829183/diff/5001/media/base/audio_bus.h#newc... media/base/audio_bus.h:62: ChannelLayout channel_layout() const { return channel_layout_; } On 2012/08/06 18:55:34, scherkus wrote: > I don't see channel layout used in 10823175 -- can we remove until needed? Done. http://codereview.chromium.org/10829183/diff/5001/media/base/audio_bus.h#newc... media/base/audio_bus.h:71: void Zero() { Zero(frames_); } On 2012/08/06 18:55:34, scherkus wrote: > instead of overload (generally avoided), how about ZeroAll OR change > Zero(frames) to ZeroFrames() Done. http://codereview.chromium.org/10829183/diff/5001/media/base/audio_bus.h#newc... media/base/audio_bus.h:75: AudioBus(int frames, const std::vector<float*>& audio_data_vector); On 2012/08/06 18:55:34, scherkus wrote: > ditto Done. http://codereview.chromium.org/10829183/diff/5001/media/base/audio_bus.h#newc... media/base/audio_bus.h:78: scoped_ptr_malloc<float, base::ScopedPtrAlignedFree> audio_data_; On 2012/08/06 18:55:34, scherkus wrote: > similar to above this can be data_ if you want Done. http://codereview.chromium.org/10829183/diff/5001/media/base/audio_bus.h#newc... media/base/audio_bus.h:82: std::vector<float*> audio_data_vector_; On 2012/08/06 18:55:34, scherkus wrote: > similar to above this can be channel_data_ Done.
lgtm w/ nits! nice to see this coming together! http://codereview.chromium.org/10829183/diff/4004/media/base/audio_bus.cc File media/base/audio_bus.cc (right): http://codereview.chromium.org/10829183/diff/4004/media/base/audio_bus.cc#new... media/base/audio_bus.cc:18: static inline bool IsAligned(void* ptr) { don't needlessly inline -- this is for a DCHECK() in run-once code and furthermore only in the wrapping case http://codereview.chromium.org/10829183/diff/4004/media/base/audio_bus.cc#new... media/base/audio_bus.cc:24: // Sanity check values, use CHECK to prevent any overflow attempts. comment ain't needed http://codereview.chromium.org/10829183/diff/4004/media/base/audio_bus.cc#new... media/base/audio_bus.cc:29: CHECK_LT(limits::kMaxSamplesPerPacket * limits::kMaxChannels, this CHECK seems redundant given: * kMaxSamplesPerPacket = 192000 * kMaxChannels = 32 That brings us to 6144000, which is well under 2147483648! http://codereview.chromium.org/10829183/diff/4004/media/base/audio_bus.h File media/base/audio_bus.h (right): http://codereview.chromium.org/10829183/diff/4004/media/base/audio_bus.h#newc... media/base/audio_bus.h:17: // Scoped container for "busing" audio channel data around. I'd expand this a tiny bit more to talk about memory details -- you mention the 16-byte aligned-ness by channel() -- that seems to be pertinent information for the class-level docs! http://codereview.chromium.org/10829183/diff/4004/media/base/audio_bus.h#newc... media/base/audio_bus.h:21: // channels() and frames_per_buffer() if an AudioParameters class is provided. s/class/object/ http://codereview.chromium.org/10829183/diff/4004/media/base/audio_bus.h#newc... media/base/audio_bus.h:32: // between two AudioBus object created with the same parameters. data_size() s/object/objects/ http://codereview.chromium.org/10829183/diff/4004/media/base/audio_bus.h#newc... media/base/audio_bus.h:45: void ZeroFrames(int frames) const; the const attribute here is completely misleading as the data contained by this class is getting modified where do we need constness? if it's because of passing by const-ref I would say don't pass by const-ref (AudioBus lives its life as a pointer, either in scoped_ptr<T> form or T* form) http://codereview.chromium.org/10829183/diff/4004/media/base/audio_bus.h#newc... media/base/audio_bus.h:46: void Zero() const { ZeroFrames(frames_); } nit: I'd deinline this -- it's technically not a simple getter http://codereview.chromium.org/10829183/diff/4004/media/base/audio_bus.h#newc... media/base/audio_bus.h:59: // Vector of pointers to each channel's audio data. with channel_data_ I think this comment is no longer being helpful :) http://codereview.chromium.org/10829183/diff/4004/media/base/audio_bus_unitte... File media/base/audio_bus_unittest.cc (right): http://codereview.chromium.org/10829183/diff/4004/media/base/audio_bus_unitte... media/base/audio_bus_unittest.cc:12: // Use a buffer size which is intentionally not a multiple of 16. nit: "See kChannelAlignment in audio_bus.cc" ?
Great stuff Dale. Added some nits for the unit test. Glad to to see this class being created. Thanks. http://codereview.chromium.org/10829183/diff/4004/media/base/audio_bus_unitte... File media/base/audio_bus_unittest.cc (right): http://codereview.chromium.org/10829183/diff/4004/media/base/audio_bus_unitte... media/base/audio_bus_unittest.cc:38: // is aligned properly. Can you be more specific than "properly"? Makes it easier to read. http://codereview.chromium.org/10829183/diff/4004/media/base/audio_bus_unitte... media/base/audio_bus_unittest.cc:74: TEST_F(AudioBusTest, Wrap) { Short comment for this test would be nice. http://codereview.chromium.org/10829183/diff/4004/media/base/audio_bus_unitte... media/base/audio_bus_unittest.cc:88: scoped_ptr<AudioBus> bus = AudioBus::Create(kChannels, kFrameCount); Nit, why not bus1 and bus2? http://codereview.chromium.org/10829183/diff/4004/media/base/audio_bus_unitte... media/base/audio_bus_unittest.cc:104: TEST_F(AudioBusTest, Zero) { Comment?
crogers: any more comments? http://codereview.chromium.org/10829183/diff/4004/media/base/audio_bus.cc File media/base/audio_bus.cc (right): http://codereview.chromium.org/10829183/diff/4004/media/base/audio_bus.cc#new... media/base/audio_bus.cc:18: static inline bool IsAligned(void* ptr) { On 2012/08/07 02:51:16, scherkus wrote: > don't needlessly inline -- this is for a DCHECK() in run-once code and > furthermore only in the wrapping case Done. http://codereview.chromium.org/10829183/diff/4004/media/base/audio_bus.cc#new... media/base/audio_bus.cc:24: // Sanity check values, use CHECK to prevent any overflow attempts. On 2012/08/07 02:51:16, scherkus wrote: > comment ain't needed Done. http://codereview.chromium.org/10829183/diff/4004/media/base/audio_bus.cc#new... media/base/audio_bus.cc:29: CHECK_LT(limits::kMaxSamplesPerPacket * limits::kMaxChannels, On 2012/08/07 02:51:16, scherkus wrote: > this CHECK seems redundant given: > * kMaxSamplesPerPacket = 192000 > * kMaxChannels = 32 > > That brings us to 6144000, which is well under 2147483648! Correct, but this part of the code doesn't know that; i.e., not checking this means we assume those values will always multiply out to be less than INT_MAX. No reason for it to be a CHECK vs DCHECK though. http://codereview.chromium.org/10829183/diff/4004/media/base/audio_bus.h File media/base/audio_bus.h (right): http://codereview.chromium.org/10829183/diff/4004/media/base/audio_bus.h#newc... media/base/audio_bus.h:17: // Scoped container for "busing" audio channel data around. On 2012/08/07 02:51:16, scherkus wrote: > I'd expand this a tiny bit more to talk about memory details -- you mention the > 16-byte aligned-ness by channel() -- that seems to be pertinent information for > the class-level docs! Done. http://codereview.chromium.org/10829183/diff/4004/media/base/audio_bus.h#newc... media/base/audio_bus.h:21: // channels() and frames_per_buffer() if an AudioParameters class is provided. On 2012/08/07 02:51:16, scherkus wrote: > s/class/object/ Done. http://codereview.chromium.org/10829183/diff/4004/media/base/audio_bus.h#newc... media/base/audio_bus.h:32: // between two AudioBus object created with the same parameters. data_size() On 2012/08/07 02:51:16, scherkus wrote: > s/object/objects/ Done. http://codereview.chromium.org/10829183/diff/4004/media/base/audio_bus.h#newc... media/base/audio_bus.h:45: void ZeroFrames(int frames) const; On 2012/08/07 02:51:16, scherkus wrote: > the const attribute here is completely misleading as the data contained by this > class is getting modified > > where do we need constness? if it's because of passing by const-ref I would say > don't pass by const-ref (AudioBus lives its life as a pointer, either in > scoped_ptr<T> form or T* form) We don't for this method, I just went overboard with const'ing. I think supporting const ref is good since it indicates that the provided AudioBus is merely a source. Since the only change necessary for this is a "const float* channel(x)" method, I think it's worth having. As such I've added that method back from the previous patch set. http://codereview.chromium.org/10829183/diff/4004/media/base/audio_bus.h#newc... media/base/audio_bus.h:46: void Zero() const { ZeroFrames(frames_); } On 2012/08/07 02:51:16, scherkus wrote: > nit: I'd deinline this -- it's technically not a simple getter Done. http://codereview.chromium.org/10829183/diff/4004/media/base/audio_bus.h#newc... media/base/audio_bus.h:59: // Vector of pointers to each channel's audio data. On 2012/08/07 02:51:16, scherkus wrote: > with channel_data_ I think this comment is no longer being helpful :) Done. http://codereview.chromium.org/10829183/diff/4004/media/base/audio_bus_unitte... File media/base/audio_bus_unittest.cc (right): http://codereview.chromium.org/10829183/diff/4004/media/base/audio_bus_unitte... media/base/audio_bus_unittest.cc:12: // Use a buffer size which is intentionally not a multiple of 16. On 2012/08/07 02:51:16, scherkus wrote: > nit: "See kChannelAlignment in audio_bus.cc" ? Done. http://codereview.chromium.org/10829183/diff/4004/media/base/audio_bus_unitte... media/base/audio_bus_unittest.cc:38: // is aligned properly. On 2012/08/07 09:52:55, henrika wrote: > Can you be more specific than "properly"? Makes it easier to read. Done. http://codereview.chromium.org/10829183/diff/4004/media/base/audio_bus_unitte... media/base/audio_bus_unittest.cc:74: TEST_F(AudioBusTest, Wrap) { On 2012/08/07 09:52:55, henrika wrote: > Short comment for this test would be nice. Done. http://codereview.chromium.org/10829183/diff/4004/media/base/audio_bus_unitte... media/base/audio_bus_unittest.cc:88: scoped_ptr<AudioBus> bus = AudioBus::Create(kChannels, kFrameCount); On 2012/08/07 09:52:55, henrika wrote: > Nit, why not bus1 and bus2? Done. http://codereview.chromium.org/10829183/diff/4004/media/base/audio_bus_unitte... media/base/audio_bus_unittest.cc:104: TEST_F(AudioBusTest, Zero) { On 2012/08/07 09:52:55, henrika wrote: > Comment? Done.
LGTM. Question: what is the next step when this CL has landed?
On 2012/08/07 18:36:34, henrika wrote: > LGTM. > > Question: what is the next step when this CL has landed? henrika: refactor the world!!!!
one small step for man... On Tue, Aug 7, 2012 at 8:39 PM, <scherkus@chromium.org> wrote: > On 2012/08/07 18:36:34, henrika wrote: > >> LGTM. >> > > Question: what is the next step when this CL has landed? >> > > henrika: refactor the world!!!! > > http://codereview.chromium.**org/10829183/<http://codereview.chromium.org/108... >
On 2012/08/07 18:44:43, henrika wrote: > one small step for man... > > > On Tue, Aug 7, 2012 at 8:39 PM, <mailto:scherkus@chromium.org> wrote: > > > On 2012/08/07 18:36:34, henrika wrote: > > > >> LGTM. > >> > > > > Question: what is the next step when this CL has landed? > >> > > > > henrika: refactor the world!!!! > > > > > http://codereview.chromium.**org/10829183/%3Chttp://codereview.chromium.org/1...> > > Refactor the world indeed! WIP here: https://chromiumcodereview.appspot.com/10823175
LGTM
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dalecurtis@chromium.org/10829183/12005
Try job failure for 10829183-12005 (retry) on android for steps "compile, build" (clobber build). It's a second try, previously, steps "compile, build" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dalecurtis@chromium.org/10829183/12005
Change committed as 150660 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
