Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10824304:
Upgrade AudioBus to support wrapping, interleaving. (Closed)
|
Created:
8 years, 4 months ago by DaleCurtis Modified:
8 years, 4 months ago CC:
chromium-reviews, feature-media-reviews_chromium.org, Chris Rogers Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionUpgrade AudioBus to support wrapping, interleaving.
Introduces AudioBus::WrapMemory(...) for constructing an
AudioBus object via an existing block of memory.
Introduces AudioBus::CalculateMemorySize(...) for sizing
the shared memory without having to create an empty AudioBus.
Introduces AudioBus::FromInterleaved(...) and AudioBus::
ToInterleaved() for converting between formats. Fixes off
by one errors in our existing audio_util implmentations.
Removes InterleaveFloatToInt from audio_util.
Removes data(), data_size() methods in favor of CopyTo()
helper method combined with AudioBus::WrapMemory().
And of course adds new tests for all of the above.
This CL is an extraction of all AudioBus functionality
required to land the WIP CL which converts browser side to
use AudioBus: http://codereview.chromium.org/10832285/
BUG=114700, 120319
TEST=unit tests under ASAN.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=152494
Patch Set 1 : Lint. #
Total comments: 29
Patch Set 2 : Comments. #
Total comments: 31
Patch Set 3 : Cleanup! #
Total comments: 1
Patch Set 4 : Comments. Bugs. #
Total comments: 4
Patch Set 5 : Rebase. #
Total comments: 5
Messages
Total messages: 27 (0 generated)
PTAL guys. @henrika, I hope this incorporates what you were thinking in the FromInterleaved() ToInterleaved() methods. crogers: cc.
I somehow missed including the AudioOutputDevice changes which use the new wrapped WrapBlock, ToInterleaved, and ExpectedDataSize methods. I'll upload these tomorrow; see AudioOutputDevice and AudioDeviceThread in the WIP CL for what that looks like. http://codereview.chromium.org/10824304/diff/5001/media/base/audio_bus.h File media/base/audio_bus.h (right): http://codereview.chromium.org/10824304/diff/5001/media/base/audio_bus.h#newc... media/base/audio_bus.h:65: // Output sample rate for the frames in this buffer. Can only be used with I actually don't like this and pending discussion from our ongoing emails, hopefully we can remove it in favor of having AudioOutputController retrieve the information from AudioManager.
Looks great Dale! I added some initial comments. Did not have time to check the unit test today. Will do that after your next upload. http://codereview.chromium.org/10824304/diff/5001/media/base/audio_bus.cc File media/base/audio_bus.cc (right): http://codereview.chromium.org/10824304/diff/5001/media/base/audio_bus.cc#new... media/base/audio_bus.cc:167: void AudioBus::FromInterleaved(const void* source, int frames, Perhaps mention that these methods are related to existing methods in audio_util.cc. http://codereview.chromium.org/10824304/diff/5001/media/base/audio_bus.cc#new... media/base/audio_bus.cc:191: float v = *source16++; Is it not more clear to use static_cast<> here? http://codereview.chromium.org/10824304/diff/5001/media/base/audio_bus.h File media/base/audio_bus.h (right): http://codereview.chromium.org/10824304/diff/5001/media/base/audio_bus.h#newc... media/base/audio_bus.h:25: // channels() and frames_per_buffer() if given an AudioParameters object. nit, channels() and frames_per_buffer() can also mean your private methods with the same names. Should we add AudioParameters::channels() to be more clear? Also, the sample rate is now used as well. http://codereview.chromium.org/10824304/diff/5001/media/base/audio_bus.h#newc... media/base/audio_bus.h:38: static scoped_ptr<AudioBus> WrapBlock(int channels, int frames, void* data); I have actually never really understood why we need the Wrap methods and I can't see that they are used today in Chrome either. Care to add some info? I understand what they do; just don't know where they will be used. http://codereview.chromium.org/10824304/diff/5001/media/base/audio_bus.h#newc... media/base/audio_bus.h:43: static int ExpectedDataSize(int channels, int frames); Hope this is not a stupid comment but, "expected" means "will most likely get but we are not sure if we get exactly this size". I mean, we *will* get this size, right? I am unable to come up with a better name ;-) http://codereview.chromium.org/10824304/diff/5001/media/base/audio_bus.h#newc... media/base/audio_bus.h:54: void FromInterleaved(const void* source, int frames, int bytes_per_channel); Signature is not inline with the function definition where |bytes_per_sample| is used. bytes_per_channel does not feel correct and it might look odd to mix frames with samples. Also, you do more stuff here right? Like converting to float and scaling. Would be nice with some more comments. http://codereview.chromium.org/10824304/diff/5001/media/base/audio_bus.h#newc... media/base/audio_bus.h:65: // Output sample rate for the frames in this buffer. Can only be used with "Output sample rate": not sure if it is a good idea to use output here. Isn't the intention that we shall use the AudioBus on the input/capture side as well? http://codereview.chromium.org/10824304/diff/5001/media/base/audio_bus.h#newc... media/base/audio_bus.h:95: // Output sample rate of the data contained in the bus. ditto http://codereview.chromium.org/10824304/diff/5001/media/base/audio_bus_unitte... File media/base/audio_bus_unittest.cc (right): http://codereview.chromium.org/10824304/diff/5001/media/base/audio_bus_unitte... media/base/audio_bus_unittest.cc:47: SCOPED_TRACE(base::StringPrintf("ch=%d, i=%d", ch, i)); Cool. I have never seen this before. Thanks. http://codereview.chromium.org/10824304/diff/5001/media/base/audio_bus_unitte... media/base/audio_bus_unittest.cc:106: // Verify an AudioBus created via wrapping a memory block works as advertised. Strange sentence. Please read it again ;-)
Thanks henrika! Just some quick comments while you might still be awake :) I'll make the changes shortly. http://codereview.chromium.org/10824304/diff/5001/media/base/audio_bus.h File media/base/audio_bus.h (right): http://codereview.chromium.org/10824304/diff/5001/media/base/audio_bus.h#newc... media/base/audio_bus.h:38: static scoped_ptr<AudioBus> WrapBlock(int channels, int frames, void* data); On 2012/08/15 09:59:37, henrika wrote: > I have actually never really understood why we need the Wrap methods and I can't > see that they are used today in Chrome either. Care to add some info? I > understand what they do; just don't know where they will be used. These are necessary to save a memcpy between the AudioBus and the shared memory. The next patch set will show this done for AudioOutputDevice. Without these methods PPAPI would require both an AudioBus::FromInterleaved() and a memcpy to transfer data across the shared memory. http://codereview.chromium.org/10824304/diff/5001/media/base/audio_bus.h#newc... media/base/audio_bus.h:43: static int ExpectedDataSize(int channels, int frames); On 2012/08/15 09:59:37, henrika wrote: > Hope this is not a stupid comment but, "expected" means "will most likely get > but we are not sure if we get exactly this size". I mean, we *will* get this > size, right? I am unable to come up with a better name ;-) Agreed, I don't like it either :) Will keep thinking on it! http://codereview.chromium.org/10824304/diff/5001/media/base/audio_bus.h#newc... media/base/audio_bus.h:65: // Output sample rate for the frames in this buffer. Can only be used with On 2012/08/15 09:59:37, henrika wrote: > "Output sample rate": not sure if it is a good idea to use output here. Isn't > the intention that we shall use the AudioBus on the input/capture side as well? I'm pretty confident this can be removed now. I'll drop it in the next patch set.
https://chromiumcodereview.appspot.com/10824304/diff/5001/media/base/audio_bu... File media/base/audio_bus.cc (right): https://chromiumcodereview.appspot.com/10824304/diff/5001/media/base/audio_bu... media/base/audio_bus.cc:25: int* aligned_frames) { NULL-check aligned_frames https://chromiumcodereview.appspot.com/10824304/diff/5001/media/base/audio_bu... media/base/audio_bus.cc:43: data_size_, kChannelAlignment))); Are we now allocating more than we need to in order to be aligned since ExpectedDataSizeInternal() is adding the extra +kChannelAlignment, on top of whatever AlignedAlloc is doing (sorry I didn't look AlignedAlloc internals) https://chromiumcodereview.appspot.com/10824304/diff/5001/media/base/audio_bu... media/base/audio_bus.cc:179: channel(ch)[i] = v * (v < 0 ? -kMinScale : kMaxScale); It seems like it would be more efficient to reverse these two for() loops and avoid having to call the channel() method each time in the inner loop. You can instead loop through source8 with stride==channels() Similar for other int cases below https://chromiumcodereview.appspot.com/10824304/diff/5001/media/base/audio_bus.h File media/base/audio_bus.h (right): https://chromiumcodereview.appspot.com/10824304/diff/5001/media/base/audio_bu... media/base/audio_bus.h:43: static int ExpectedDataSize(int channels, int frames); How about CalculateSize() or CalculateDataSize() or CalculateStorageSize()
(just comments) http://codereview.chromium.org/10824304/diff/5001/media/base/audio_bus.cc File media/base/audio_bus.cc (right): http://codereview.chromium.org/10824304/diff/5001/media/base/audio_bus.cc#new... media/base/audio_bus.cc:43: data_size_, kChannelAlignment))); On 2012/08/15 19:39:31, Chris Rogers wrote: > Are we now allocating more than we need to in order to be aligned since > ExpectedDataSizeInternal() is adding the extra +kChannelAlignment, on top of > whatever AlignedAlloc is doing (sorry I didn't look AlignedAlloc internals) We're allocating more than we need since we don't require WrapBlock to be given a 16-byte aligned address. AlignedAlloc is in the 99% of cases using an aligned allocator method like posix_memalign which isn't over allocating under the hood. The only time over allocation is used is when a user manually specifies CHROME_ALLOCATOR=winheap or winlfh. In common desktop case we've already got a 16-byte alignment due to the underlying allocator, so we're not even fragmenting the heap by using AlignedAlloc. http://codereview.chromium.org/10824304/diff/5001/media/base/audio_bus.cc#new... media/base/audio_bus.cc:179: channel(ch)[i] = v * (v < 0 ? -kMinScale : kMaxScale); On 2012/08/15 19:39:31, Chris Rogers wrote: > It seems like it would be more efficient to reverse these two for() loops and > avoid having to call the channel() method each time in the inner loop. You can > instead loop through source8 with stride==channels() > > Similar for other int cases below How about iterating over i=0..frames * channels and setting channel(i % channel) instead?
PTAL. This one also removes InterleaveFloatToInt() and almost all uses of DeinterleaveAudioChannel(). http://codereview.chromium.org/10824304/diff/5001/media/base/audio_bus.cc File media/base/audio_bus.cc (right): http://codereview.chromium.org/10824304/diff/5001/media/base/audio_bus.cc#new... media/base/audio_bus.cc:25: int* aligned_frames) { On 2012/08/15 19:39:31, Chris Rogers wrote: > NULL-check aligned_frames Done. http://codereview.chromium.org/10824304/diff/5001/media/base/audio_bus.cc#new... media/base/audio_bus.cc:167: void AudioBus::FromInterleaved(const void* source, int frames, On 2012/08/15 09:59:37, henrika wrote: > Perhaps mention that these methods are related to existing methods in > audio_util.cc. Ideally those methods will go away and these will be the only ones, which is why I added a "TODO" in audio_util.cc. Adding another transient comment doesn't seem useful. http://codereview.chromium.org/10824304/diff/5001/media/base/audio_bus.cc#new... media/base/audio_bus.cc:179: channel(ch)[i] = v * (v < 0 ? -kMinScale : kMaxScale); On 2012/08/15 20:12:54, DaleCurtis wrote: > On 2012/08/15 19:39:31, Chris Rogers wrote: > > It seems like it would be more efficient to reverse these two for() loops and > > avoid having to call the channel() method each time in the inner loop. You > can > > instead loop through source8 with stride==channels() > > > > Similar for other int cases below > > How about iterating over i=0..frames * channels and setting channel(i % channel) > instead? I did some basic benchmarking and the new template version is ~twice as fast as the old one. I'll make another pass to convert this to intrinsics in the future and add a proper benchmark at that time. http://codereview.chromium.org/10824304/diff/5001/media/base/audio_bus.cc#new... media/base/audio_bus.cc:191: float v = *source16++; On 2012/08/15 09:59:37, henrika wrote: > Is it not more clear to use static_cast<> here? I think that would be over-verbose. http://codereview.chromium.org/10824304/diff/5001/media/base/audio_bus.h File media/base/audio_bus.h (right): http://codereview.chromium.org/10824304/diff/5001/media/base/audio_bus.h#newc... media/base/audio_bus.h:25: // channels() and frames_per_buffer() if given an AudioParameters object. On 2012/08/15 09:59:37, henrika wrote: > nit, channels() and frames_per_buffer() can also mean your private methods with > the same names. Should we add AudioParameters::channels() to be more clear? > > Also, the sample rate is now used as well. Reworded, sample rate dropped. http://codereview.chromium.org/10824304/diff/5001/media/base/audio_bus.h#newc... media/base/audio_bus.h:43: static int ExpectedDataSize(int channels, int frames); On 2012/08/15 19:39:31, Chris Rogers wrote: > How about CalculateSize() or CalculateDataSize() or CalculateStorageSize() Since this method is intended to be used with WrapBlock I went with BlockSize(). http://codereview.chromium.org/10824304/diff/5001/media/base/audio_bus.h#newc... media/base/audio_bus.h:54: void FromInterleaved(const void* source, int frames, int bytes_per_channel); On 2012/08/15 09:59:37, henrika wrote: > Signature is not inline with the function definition where |bytes_per_sample| is > used. bytes_per_channel does not feel correct and it might look odd to mix > frames with samples. > > Also, you do more stuff here right? Like converting to float and scaling. Would > be nice with some more comments. Done. http://codereview.chromium.org/10824304/diff/5001/media/base/audio_bus.h#newc... media/base/audio_bus.h:95: // Output sample rate of the data contained in the bus. On 2012/08/15 09:59:37, henrika wrote: > ditto Removed. http://codereview.chromium.org/10824304/diff/5001/media/base/audio_bus_unitte... File media/base/audio_bus_unittest.cc (right): http://codereview.chromium.org/10824304/diff/5001/media/base/audio_bus_unitte... media/base/audio_bus_unittest.cc:106: // Verify an AudioBus created via wrapping a memory block works as advertised. On 2012/08/15 09:59:37, henrika wrote: > Strange sentence. Please read it again ;-) Not sure what's strange about it? :) http://codereview.chromium.org/10824304/diff/4003/media/audio/audio_util.cc File media/audio/audio_util.cc (right): http://codereview.chromium.org/10824304/diff/4003/media/audio/audio_util.cc#n... media/audio/audio_util.cc:185: // TODO(dalecurtis): Delete once everywhere is using the AudioBus version. Still one caller of this: AudioFileReader which requires a bit more work than should be done in this CL to remove. I'll remove it in a followup CL.
http://codereview.chromium.org/10824304/diff/4003/media/audio/audio_output_de... File media/audio/audio_output_device.cc (right): http://codereview.chromium.org/10824304/diff/4003/media/audio/audio_output_de... media/audio/audio_output_device.cc:273: // TODO(dalecurtis): Remove this when we have float everywhere. reference bug? http://codereview.chromium.org/10824304/diff/4003/media/audio/audio_util.cc File media/audio/audio_util.cc (right): http://codereview.chromium.org/10824304/diff/4003/media/audio/audio_util.cc#n... media/audio/audio_util.cc:185: // TODO(dalecurtis): Delete once everywhere is using the AudioBus version. reference bug? http://codereview.chromium.org/10824304/diff/4003/media/base/audio_bus.cc File media/base/audio_bus.cc (right): http://codereview.chromium.org/10824304/diff/4003/media/base/audio_bus.cc#new... media/base/audio_bus.cc:25: int* aligned_frames) { alignment http://codereview.chromium.org/10824304/diff/4003/media/base/audio_bus.cc#new... media/base/audio_bus.cc:26: DCHECK(aligned_frames); dcheck not helpful here as you immediately deref http://codereview.chromium.org/10824304/diff/4003/media/base/audio_bus.cc#new... media/base/audio_bus.cc:32: return sizeof(float) * channels * *aligned_frames + kChannelAlignment; the * * is freaking me out -- want to do (*aligned_frames)? http://codereview.chromium.org/10824304/diff/4003/media/base/audio_bus.cc#new... media/base/audio_bus.cc:159: void FromInterleavedInternal(const void* src, int frames, AudioBus* dest) { static + move to top http://codereview.chromium.org/10824304/diff/4003/media/base/audio_bus.cc#new... media/base/audio_bus.cc:179: // TODO(dalecurtis): See if intrinsic optimizations help any here. can we get a bug tracking where we may want to use intrinsics? http://codereview.chromium.org/10824304/diff/4003/media/base/audio_bus.cc#new... media/base/audio_bus.cc:195: Zero(); add return after Zeroing? http://codereview.chromium.org/10824304/diff/4003/media/base/audio_bus.cc#new... media/base/audio_bus.cc:209: static void ToInterleavedInternal(const AudioBus* source, int frames, we put static fn's at the top of files http://codereview.chromium.org/10824304/diff/4003/media/base/audio_bus.h File media/base/audio_bus.h (right): http://codereview.chromium.org/10824304/diff/4003/media/base/audio_bus.h#newc... media/base/audio_bus.h:38: static scoped_ptr<AudioBus> WrapBlock(int channels, int frames, void* data); WrapData? "Block" isn't a very widely used term any restrictions on memory alignment? it's also very strange to see a memory pointer passed in w/o any size -- I'd almost encourage it for the sake of CHECK()ing As a side this API is a bit strange (call BlockSize(), allocate it however you wish, then pass the result in) but I don't have concrete suggestions as to how to improve things at the moment. One thought I had was whether this class should be a wrapper for externally allocated memory so that usage of AudioBus is consistent (i.e., replace Create() with callers allocating aligned memory alongside an AudioBus), but we don't need to figure that out in this CL. http://codereview.chromium.org/10824304/diff/4003/media/base/audio_bus.h#newc... media/base/audio_bus.h:43: static int BlockSize(int channels, int frames); try to use verbs for functions -- this should be CalculateDataSize() or similar http://codereview.chromium.org/10824304/diff/4003/media/base/audio_bus.h#newc... media/base/audio_bus.h:55: // conversion. ToInterleaved will also clip values to format range. Handles ToInterleaved() http://codereview.chromium.org/10824304/diff/4003/media/base/audio_bus.h#newc... media/base/audio_bus.h:57: void FromInterleaved(const void* source, int frames, int bytes_per_sample); at some point in time it would be *fantastic* if we used an enum for bytes_per_sample that was set to 1, 2, and 4 instead of an int we have a lot of switches for bytes_per_sample scattered around and some compile-time checking would be nice http://codereview.chromium.org/10824304/diff/4003/media/base/audio_bus.h#newc... media/base/audio_bus.h:85: void ValidateConfig(int channels, int frames); const method http://codereview.chromium.org/10824304/diff/4003/media/filters/pipeline_inte... File media/filters/pipeline_integration_test.cc (right): http://codereview.chromium.org/10824304/diff/4003/media/filters/pipeline_inte... media/filters/pipeline_integration_test.cc:260: EXPECT_EQ(GetAudioHash(), "5699a4415b620e45b9d0aae531c9df76"); ?
(just comments) http://codereview.chromium.org/10824304/diff/4003/media/base/audio_bus.h File media/base/audio_bus.h (right): http://codereview.chromium.org/10824304/diff/4003/media/base/audio_bus.h#newc... media/base/audio_bus.h:38: static scoped_ptr<AudioBus> WrapBlock(int channels, int frames, void* data); On 2012/08/16 18:58:19, scherkus wrote: > WrapData? "Block" isn't a very widely used term > > any restrictions on memory alignment? > > it's also very strange to see a memory pointer passed in w/o any size -- I'd > almost encourage it for the sake of CHECK()ing > > > As a side this API is a bit strange (call BlockSize(), allocate it however you > wish, then pass the result in) but I don't have concrete suggestions as to how > to improve things at the moment. > > One thought I had was whether this class should be a wrapper for externally > allocated memory so that usage of AudioBus is consistent (i.e., replace Create() > with callers allocating aligned memory alongside an AudioBus), but we don't need > to figure that out in this CL. Agreed, ideally we'd have no Wrap methods :) Since this is explicitly intended for wrapping shared memory. An alternative solution might be to have an AudioBusSharedMemory subclass which adds WrapSharedMemory() and CalculateSharedMemorySize(). Even without the subclass, I could name the methods in that style. I don't forsee any other callers needing this information. WDYT? There are no alignment requirements since we require return our real size + alignment leaving us room to slide the given pointer as necessary. http://codereview.chromium.org/10824304/diff/4003/media/base/audio_bus.h#newc... media/base/audio_bus.h:57: void FromInterleaved(const void* source, int frames, int bytes_per_sample); On 2012/08/16 18:58:19, scherkus wrote: > at some point in time it would be *fantastic* if we used an enum for > bytes_per_sample that was set to 1, 2, and 4 instead of an int > > we have a lot of switches for bytes_per_sample scattered around and some > compile-time checking would be nice Yeah, I've been thinking the same thing. However, a direct bytes per sample enough doesn't seem like the correct approach, but rather something like FFMpeg's AVSampleFormat. It's not obvious that bytes_per_sample == 1 means uint8 vs int8, and bytes_per_sample == 2 means int16 vs uint16, etc; e.g., the enum should be something like kSampleFormatU8, kSampleFormatS16, etc. I'll file a bug for this if this is inline with what you're thinking. http://codereview.chromium.org/10824304/diff/4003/media/filters/pipeline_inte... File media/filters/pipeline_integration_test.cc (right): http://codereview.chromium.org/10824304/diff/4003/media/filters/pipeline_inte... media/filters/pipeline_integration_test.cc:260: EXPECT_EQ(GetAudioHash(), "5699a4415b620e45b9d0aae531c9df76"); On 2012/08/16 18:58:19, scherkus wrote: > ? The old interleave/deinterleave code had an off by one error, the max value could never be 1.0 since we scaled by -min_value() instead of max_value() in the positive case which is usually (-min_value() - 1). As such fixing this changes the audio hash.
On 2012/08/16 19:50:33, DaleCurtis wrote: > (just comments) > > http://codereview.chromium.org/10824304/diff/4003/media/base/audio_bus.h > File media/base/audio_bus.h (right): > > http://codereview.chromium.org/10824304/diff/4003/media/base/audio_bus.h#newc... > media/base/audio_bus.h:38: static scoped_ptr<AudioBus> WrapBlock(int channels, > int frames, void* data); > On 2012/08/16 18:58:19, scherkus wrote: > > WrapData? "Block" isn't a very widely used term > > > > any restrictions on memory alignment? > > > > it's also very strange to see a memory pointer passed in w/o any size -- I'd > > almost encourage it for the sake of CHECK()ing > > > > > > As a side this API is a bit strange (call BlockSize(), allocate it however you > > wish, then pass the result in) but I don't have concrete suggestions as to how > > to improve things at the moment. > > > > One thought I had was whether this class should be a wrapper for externally > > allocated memory so that usage of AudioBus is consistent (i.e., replace > Create() > > with callers allocating aligned memory alongside an AudioBus), but we don't > need > > to figure that out in this CL. > > Agreed, ideally we'd have no Wrap methods :) Since this is explicitly intended > for wrapping shared memory. An alternative solution might be to have an > AudioBusSharedMemory subclass which adds WrapSharedMemory() and > CalculateSharedMemorySize(). > > Even without the subclass, I could name the methods in that style. I don't > forsee any other callers needing this information. > > WDYT? I was actually thinking the other way -- we always use AudioBus to wrap various forms of memory (flat contiguous blocks, vectors of float*, etc...) and get rid of Create(), data(), and data_size() since that's information known by client using AudioBus. > There are no alignment requirements since we require return our real size + > alignment leaving us room to slide the given pointer as necessary. Q about shared memory and pointer adjustment... when we map shared memory are we guaranteed that the resulting pointer will > http://codereview.chromium.org/10824304/diff/4003/media/base/audio_bus.h#newc... > media/base/audio_bus.h:57: void FromInterleaved(const void* source, int frames, > int bytes_per_sample); > On 2012/08/16 18:58:19, scherkus wrote: > > at some point in time it would be *fantastic* if we used an enum for > > bytes_per_sample that was set to 1, 2, and 4 instead of an int > > > > we have a lot of switches for bytes_per_sample scattered around and some > > compile-time checking would be nice > > Yeah, I've been thinking the same thing. However, a direct bytes per sample > enough doesn't seem like the correct approach, but rather something like > FFMpeg's AVSampleFormat. It's not obvious that bytes_per_sample == 1 means > uint8 vs int8, and bytes_per_sample == 2 means int16 vs uint16, etc; e.g., the > enum should be something like kSampleFormatU8, kSampleFormatS16, etc. > > I'll file a bug for this if this is inline with what you're thinking. I'd say it's worth filing a bug w/ ideas but no need to do it right away (i.e., Pri-3, no Mstone) > http://codereview.chromium.org/10824304/diff/4003/media/filters/pipeline_inte... > File media/filters/pipeline_integration_test.cc (right): > > http://codereview.chromium.org/10824304/diff/4003/media/filters/pipeline_inte... > media/filters/pipeline_integration_test.cc:260: EXPECT_EQ(GetAudioHash(), > "5699a4415b620e45b9d0aae531c9df76"); > On 2012/08/16 18:58:19, scherkus wrote: > > ? > > The old interleave/deinterleave code had an off by one error, the max value > could never be 1.0 since we scaled by -min_value() instead of max_value() in the > positive case which is usually (-min_value() - 1). As such fixing this changes > the audio hash.
On 2012/08/16 21:55:00, scherkus wrote: > On 2012/08/16 19:50:33, DaleCurtis wrote: > > (just comments) > > > > http://codereview.chromium.org/10824304/diff/4003/media/base/audio_bus.h > > File media/base/audio_bus.h (right): > > > > > http://codereview.chromium.org/10824304/diff/4003/media/base/audio_bus.h#newc... > > media/base/audio_bus.h:38: static scoped_ptr<AudioBus> WrapBlock(int channels, > > int frames, void* data); > > On 2012/08/16 18:58:19, scherkus wrote: > > > WrapData? "Block" isn't a very widely used term > > > > > > any restrictions on memory alignment? > > > > > > it's also very strange to see a memory pointer passed in w/o any size -- I'd > > > almost encourage it for the sake of CHECK()ing > > > > > > > > > As a side this API is a bit strange (call BlockSize(), allocate it however > you > > > wish, then pass the result in) but I don't have concrete suggestions as to > how > > > to improve things at the moment. > > > > > > One thought I had was whether this class should be a wrapper for externally > > > allocated memory so that usage of AudioBus is consistent (i.e., replace > > Create() > > > with callers allocating aligned memory alongside an AudioBus), but we don't > > need > > > to figure that out in this CL. > > > > Agreed, ideally we'd have no Wrap methods :) Since this is explicitly intended > > for wrapping shared memory. An alternative solution might be to have an > > AudioBusSharedMemory subclass which adds WrapSharedMemory() and > > CalculateSharedMemorySize(). > > > > Even without the subclass, I could name the methods in that style. I don't > > forsee any other callers needing this information. > > > > WDYT? > > I was actually thinking the other way -- we always use AudioBus to wrap various > forms of memory (flat contiguous blocks, vectors of float*, etc...) and get rid > of Create(), data(), and data_size() since that's information known by client > using AudioBus. > > > There are no alignment requirements since we require return our real size + > > alignment leaving us room to slide the given pointer as necessary. > > Q about shared memory and pointer adjustment... when we map shared memory are we > guaranteed that the resulting pointer will whoops hit send early... the resulting pointer will require the same amount of offset? If process A adjusts it pointer by 8 bytes but process B doesn't have to adjust at all, aren't we kinda screwed? > > > http://codereview.chromium.org/10824304/diff/4003/media/base/audio_bus.h#newc... > > media/base/audio_bus.h:57: void FromInterleaved(const void* source, int > frames, > > int bytes_per_sample); > > On 2012/08/16 18:58:19, scherkus wrote: > > > at some point in time it would be *fantastic* if we used an enum for > > > bytes_per_sample that was set to 1, 2, and 4 instead of an int > > > > > > we have a lot of switches for bytes_per_sample scattered around and some > > > compile-time checking would be nice > > > > Yeah, I've been thinking the same thing. However, a direct bytes per sample > > enough doesn't seem like the correct approach, but rather something like > > FFMpeg's AVSampleFormat. It's not obvious that bytes_per_sample == 1 means > > uint8 vs int8, and bytes_per_sample == 2 means int16 vs uint16, etc; e.g., > the > > enum should be something like kSampleFormatU8, kSampleFormatS16, etc. > > > > I'll file a bug for this if this is inline with what you're thinking. > > I'd say it's worth filing a bug w/ ideas but no need to do it right away (i.e., > Pri-3, no Mstone) > > > > http://codereview.chromium.org/10824304/diff/4003/media/filters/pipeline_inte... > > File media/filters/pipeline_integration_test.cc (right): > > > > > http://codereview.chromium.org/10824304/diff/4003/media/filters/pipeline_inte... > > media/filters/pipeline_integration_test.cc:260: EXPECT_EQ(GetAudioHash(), > > "5699a4415b620e45b9d0aae531c9df76"); > > On 2012/08/16 18:58:19, scherkus wrote: > > > ? > > > > The old interleave/deinterleave code had an off by one error, the max value > > could never be 1.0 since we scaled by -min_value() instead of max_value() in > the > > positive case which is usually (-min_value() - 1). As such fixing this changes > > the audio hash.
On 2012/08/16 21:55:00, scherkus wrote: > On 2012/08/16 19:50:33, DaleCurtis wrote: > > (just comments) > > > > http://codereview.chromium.org/10824304/diff/4003/media/base/audio_bus.h > > File media/base/audio_bus.h (right): > > > > > http://codereview.chromium.org/10824304/diff/4003/media/base/audio_bus.h#newc... > > media/base/audio_bus.h:38: static scoped_ptr<AudioBus> WrapBlock(int channels, > > int frames, void* data); > > On 2012/08/16 18:58:19, scherkus wrote: > > > WrapData? "Block" isn't a very widely used term > > > > > > any restrictions on memory alignment? > > > > > > it's also very strange to see a memory pointer passed in w/o any size -- I'd > > > almost encourage it for the sake of CHECK()ing > > > > > > > > > As a side this API is a bit strange (call BlockSize(), allocate it however > you > > > wish, then pass the result in) but I don't have concrete suggestions as to > how > > > to improve things at the moment. > > > > > > One thought I had was whether this class should be a wrapper for externally > > > allocated memory so that usage of AudioBus is consistent (i.e., replace > > Create() > > > with callers allocating aligned memory alongside an AudioBus), but we don't > > need > > > to figure that out in this CL. > > > > Agreed, ideally we'd have no Wrap methods :) Since this is explicitly intended > > for wrapping shared memory. An alternative solution might be to have an > > AudioBusSharedMemory subclass which adds WrapSharedMemory() and > > CalculateSharedMemorySize(). > > > > Even without the subclass, I could name the methods in that style. I don't > > forsee any other callers needing this information. > > > > WDYT? > > I was actually thinking the other way -- we always use AudioBus to wrap various > forms of memory (flat contiguous blocks, vectors of float*, etc...) and get rid > of Create(), data(), and data_size() since that's information known by client > using AudioBus. > > > There are no alignment requirements since we require return our real size + > > alignment leaving us room to slide the given pointer as necessary. > > Q about shared memory and pointer adjustment... when we map shared memory are we > guaranteed that the resulting pointer will whoops hit send early... the resulting pointer will require the same amount of offset? If process A adjusts it pointer by 8 bytes but process B doesn't have to adjust at all, aren't we kinda screwed? > > > http://codereview.chromium.org/10824304/diff/4003/media/base/audio_bus.h#newc... > > media/base/audio_bus.h:57: void FromInterleaved(const void* source, int > frames, > > int bytes_per_sample); > > On 2012/08/16 18:58:19, scherkus wrote: > > > at some point in time it would be *fantastic* if we used an enum for > > > bytes_per_sample that was set to 1, 2, and 4 instead of an int > > > > > > we have a lot of switches for bytes_per_sample scattered around and some > > > compile-time checking would be nice > > > > Yeah, I've been thinking the same thing. However, a direct bytes per sample > > enough doesn't seem like the correct approach, but rather something like > > FFMpeg's AVSampleFormat. It's not obvious that bytes_per_sample == 1 means > > uint8 vs int8, and bytes_per_sample == 2 means int16 vs uint16, etc; e.g., > the > > enum should be something like kSampleFormatU8, kSampleFormatS16, etc. > > > > I'll file a bug for this if this is inline with what you're thinking. > > I'd say it's worth filing a bug w/ ideas but no need to do it right away (i.e., > Pri-3, no Mstone) > > > > http://codereview.chromium.org/10824304/diff/4003/media/filters/pipeline_inte... > > File media/filters/pipeline_integration_test.cc (right): > > > > > http://codereview.chromium.org/10824304/diff/4003/media/filters/pipeline_inte... > > media/filters/pipeline_integration_test.cc:260: EXPECT_EQ(GetAudioHash(), > > "5699a4415b620e45b9d0aae531c9df76"); > > On 2012/08/16 18:58:19, scherkus wrote: > > > ? > > > > The old interleave/deinterleave code had an off by one error, the max value > > could never be 1.0 since we scaled by -min_value() instead of max_value() in > the > > positive case which is usually (-min_value() - 1). As such fixing this changes > > the audio hash.
On 2012/08/16 21:56:10, scherkus wrote: > On 2012/08/16 21:55:00, scherkus wrote: > > On 2012/08/16 19:50:33, DaleCurtis wrote: > > > (just comments) > > > > > > http://codereview.chromium.org/10824304/diff/4003/media/base/audio_bus.h > > > File media/base/audio_bus.h (right): > > > > > > > > > http://codereview.chromium.org/10824304/diff/4003/media/base/audio_bus.h#newc... > > > media/base/audio_bus.h:38: static scoped_ptr<AudioBus> WrapBlock(int > channels, > > > int frames, void* data); > > > On 2012/08/16 18:58:19, scherkus wrote: > > > > WrapData? "Block" isn't a very widely used term > > > > > > > > any restrictions on memory alignment? > > > > > > > > it's also very strange to see a memory pointer passed in w/o any size -- > I'd > > > > almost encourage it for the sake of CHECK()ing > > > > > > > > > > > > As a side this API is a bit strange (call BlockSize(), allocate it however > > you > > > > wish, then pass the result in) but I don't have concrete suggestions as to > > how > > > > to improve things at the moment. > > > > > > > > One thought I had was whether this class should be a wrapper for > externally > > > > allocated memory so that usage of AudioBus is consistent (i.e., replace > > > Create() > > > > with callers allocating aligned memory alongside an AudioBus), but we > don't > > > need > > > > to figure that out in this CL. > > > > > > Agreed, ideally we'd have no Wrap methods :) Since this is explicitly > intended > > > for wrapping shared memory. An alternative solution might be to have an > > > AudioBusSharedMemory subclass which adds WrapSharedMemory() and > > > CalculateSharedMemorySize(). > > > > > > Even without the subclass, I could name the methods in that style. I don't > > > forsee any other callers needing this information. > > > > > > WDYT? > > > > I was actually thinking the other way -- we always use AudioBus to wrap > various > > forms of memory (flat contiguous blocks, vectors of float*, etc...) and get > rid > > of Create(), data(), and data_size() since that's information known by client > > using AudioBus. Hmmm, your comments have got me rethinking some things. We can get rid of data() and data_size() in favor of a CopyTo(AudioBus* dest) helper method. We'll still need that because only the renderer side passes the wrapped AudioBus around. The browser side needs to copy the data out of an AudioBus wrapping the shared memory into the AudioBus provided by the caller to the OnMoreData() method. We need both sides to wrap the same shared memory pointer so they end up with the same layout, otherwise we run into problems like you mention below. Alternatively, if we want the wrapped shared memory passed around browser side we would need something like AudioBus::Reset(const AudioBus* src) that hollows out the existing AudioBus, but that's something we can do in another CL if we want. Given this, I don't think having the client handle allocation would do anything but make the common use case more complicated. As a bonus to removing data(), data_size() there is no longer any difference between a wrapped AudioBus and a Create()'d one. > > > > > There are no alignment requirements since we require return our real size + > > > alignment leaving us room to slide the given pointer as necessary. > > > > Q about shared memory and pointer adjustment... when we map shared memory are > we > > guaranteed that the resulting pointer will > > whoops hit send early... > > the resulting pointer will require the same amount of offset? > > If process A adjusts it pointer by 8 bytes but process B doesn't have to adjust > at all, aren't we kinda screwed? Having both sides wrap the same shared memory pointer should make this a moot point. If you're suggesting that process A and B each have different pointers to the same memory block then maybe I'm missing something about our shared memory implementation. > > > > > > > http://codereview.chromium.org/10824304/diff/4003/media/base/audio_bus.h#newc... > > > media/base/audio_bus.h:57: void FromInterleaved(const void* source, int > > frames, > > > int bytes_per_sample); > > > On 2012/08/16 18:58:19, scherkus wrote: > > > > at some point in time it would be *fantastic* if we used an enum for > > > > bytes_per_sample that was set to 1, 2, and 4 instead of an int > > > > > > > > we have a lot of switches for bytes_per_sample scattered around and some > > > > compile-time checking would be nice > > > > > > Yeah, I've been thinking the same thing. However, a direct bytes per sample > > > enough doesn't seem like the correct approach, but rather something like > > > FFMpeg's AVSampleFormat. It's not obvious that bytes_per_sample == 1 means > > > uint8 vs int8, and bytes_per_sample == 2 means int16 vs uint16, etc; e.g., > > the > > > enum should be something like kSampleFormatU8, kSampleFormatS16, etc. > > > > > > I'll file a bug for this if this is inline with what you're thinking. > > > > I'd say it's worth filing a bug w/ ideas but no need to do it right away > (i.e., > > Pri-3, no Mstone) > > > > > > > > http://codereview.chromium.org/10824304/diff/4003/media/filters/pipeline_inte... > > > File media/filters/pipeline_integration_test.cc (right): > > > > > > > > > http://codereview.chromium.org/10824304/diff/4003/media/filters/pipeline_inte... > > > media/filters/pipeline_integration_test.cc:260: EXPECT_EQ(GetAudioHash(), > > > "5699a4415b620e45b9d0aae531c9df76"); > > > On 2012/08/16 18:58:19, scherkus wrote: > > > > ? > > > > > > The old interleave/deinterleave code had an off by one error, the max value > > > could never be 1.0 since we scaled by -min_value() instead of max_value() in > > the > > > positive case which is usually (-min_value() - 1). As such fixing this > changes > > > the audio hash.
On 2012/08/16 22:58:31, DaleCurtis wrote: > On 2012/08/16 21:56:10, scherkus wrote: > > On 2012/08/16 21:55:00, scherkus wrote: > > > On 2012/08/16 19:50:33, DaleCurtis wrote: > > > > (just comments) > > > > > > > > http://codereview.chromium.org/10824304/diff/4003/media/base/audio_bus.h > > > > File media/base/audio_bus.h (right): > > > > > > > > > > > > > > http://codereview.chromium.org/10824304/diff/4003/media/base/audio_bus.h#newc... > > > > media/base/audio_bus.h:38: static scoped_ptr<AudioBus> WrapBlock(int > > channels, > > > > int frames, void* data); > > > > On 2012/08/16 18:58:19, scherkus wrote: > > > > > WrapData? "Block" isn't a very widely used term > > > > > > > > > > any restrictions on memory alignment? > > > > > > > > > > it's also very strange to see a memory pointer passed in w/o any size -- > > I'd > > > > > almost encourage it for the sake of CHECK()ing > > > > > > > > > > > > > > > As a side this API is a bit strange (call BlockSize(), allocate it > however > > > you > > > > > wish, then pass the result in) but I don't have concrete suggestions as > to > > > how > > > > > to improve things at the moment. > > > > > > > > > > One thought I had was whether this class should be a wrapper for > > externally > > > > > allocated memory so that usage of AudioBus is consistent (i.e., replace > > > > Create() > > > > > with callers allocating aligned memory alongside an AudioBus), but we > > don't > > > > need > > > > > to figure that out in this CL. > > > > > > > > Agreed, ideally we'd have no Wrap methods :) Since this is explicitly > > intended > > > > for wrapping shared memory. An alternative solution might be to have an > > > > AudioBusSharedMemory subclass which adds WrapSharedMemory() and > > > > CalculateSharedMemorySize(). > > > > > > > > Even without the subclass, I could name the methods in that style. I > don't > > > > forsee any other callers needing this information. > > > > > > > > WDYT? > > > > > > I was actually thinking the other way -- we always use AudioBus to wrap > > various > > > forms of memory (flat contiguous blocks, vectors of float*, etc...) and get > > rid > > > of Create(), data(), and data_size() since that's information known by > client > > > using AudioBus. > > Hmmm, your comments have got me rethinking some things. We can get rid of > data() > and data_size() in favor of a CopyTo(AudioBus* dest) helper method. We'll still > need that because only the renderer side passes the wrapped AudioBus around. > > The browser side needs to copy the data out of an AudioBus wrapping the shared > memory into the AudioBus provided by the caller to the OnMoreData() method. > > We need both sides to wrap the same shared memory pointer so they end up with > the same layout, otherwise we run into problems like you mention below. > > Alternatively, if we want the wrapped shared memory passed around browser side > we would need something like AudioBus::Reset(const AudioBus* src) that hollows > out the > existing AudioBus, but that's something we can do in another CL if we want. > > Given this, I don't think having the client handle allocation would do anything > but > make the common use case more complicated. As a bonus to removing data(), > data_size() > there is no longer any difference between a wrapped AudioBus and a Create()'d > one. > > > > > > > > There are no alignment requirements since we require return our real size > + > > > > alignment leaving us room to slide the given pointer as necessary. > > > > > > Q about shared memory and pointer adjustment... when we map shared memory > are > > we > > > guaranteed that the resulting pointer will > > > > whoops hit send early... > > > > the resulting pointer will require the same amount of offset? > > > > If process A adjusts it pointer by 8 bytes but process B doesn't have to > adjust > > at all, aren't we kinda screwed? > > Having both sides wrap the same shared memory pointer should make this a > moot point. If you're suggesting that process A and B each have different > pointers to the same memory block then maybe I'm missing something about > our shared memory implementation. > > > > > > > > > > > > > http://codereview.chromium.org/10824304/diff/4003/media/base/audio_bus.h#newc... > > > > media/base/audio_bus.h:57: void FromInterleaved(const void* source, int > > > frames, > > > > int bytes_per_sample); > > > > On 2012/08/16 18:58:19, scherkus wrote: > > > > > at some point in time it would be *fantastic* if we used an enum for > > > > > bytes_per_sample that was set to 1, 2, and 4 instead of an int > > > > > > > > > > we have a lot of switches for bytes_per_sample scattered around and some > > > > > compile-time checking would be nice > > > > > > > > Yeah, I've been thinking the same thing. However, a direct bytes per > sample > > > > enough doesn't seem like the correct approach, but rather something like > > > > FFMpeg's AVSampleFormat. It's not obvious that bytes_per_sample == 1 > means > > > > uint8 vs int8, and bytes_per_sample == 2 means int16 vs uint16, etc; > e.g., > > > the > > > > enum should be something like kSampleFormatU8, kSampleFormatS16, etc. > > > > > > > > I'll file a bug for this if this is inline with what you're thinking. > > > > > > I'd say it's worth filing a bug w/ ideas but no need to do it right away > > (i.e., > > > Pri-3, no Mstone) > > > > > > > > > > > > > http://codereview.chromium.org/10824304/diff/4003/media/filters/pipeline_inte... > > > > File media/filters/pipeline_integration_test.cc (right): > > > > > > > > > > > > > > http://codereview.chromium.org/10824304/diff/4003/media/filters/pipeline_inte... > > > > media/filters/pipeline_integration_test.cc:260: EXPECT_EQ(GetAudioHash(), > > > > "5699a4415b620e45b9d0aae531c9df76"); > > > > On 2012/08/16 18:58:19, scherkus wrote: > > > > > ? > > > > > > > > The old interleave/deinterleave code had an off by one error, the max > value > > > > could never be 1.0 since we scaled by -min_value() instead of max_value() > in > > > the > > > > positive case which is usually (-min_value() - 1). As such fixing this > > changes > > > > the audio hash. Turns out I am misunderstanding the shared memory implementation, it's a virtual mapping so it does seem possible that one side might not be aligned while the other is, whether that's possible in practice I don't know. I'll dig a bit.
> Turns out I am misunderstanding the shared memory implementation, it's a virtual > mapping so it does seem possible that one side might not be aligned while the > other is, whether that's possible in practice I don't know. I'll dig a bit. Time to email the wizards at chromium-dev@? Should we drop the alignment requirement for shmem?
On 2012/08/17 00:16:55, scherkus wrote: > > Turns out I am misunderstanding the shared memory implementation, it's a > virtual > > mapping so it does seem possible that one side might not be aligned while the > > other is, whether that's possible in practice I don't know. I'll dig a bit. > > Time to email the wizards at chromium-dev@? > > Should we drop the alignment requirement for shmem? Well if we want to wrap we need to instead CHECK that the shmem is always aligned (which it always will be on POSIX, NACL since mmap aligns to page boundary, same story with windows I think, but will verify with jar.) Otherwise we'll have to give up on wrapping, which isn't the end of the world.
PTAL. This should address all comments. http://codereview.chromium.org/10824304/diff/4003/media/audio/audio_output_de... File media/audio/audio_output_device.cc (right): http://codereview.chromium.org/10824304/diff/4003/media/audio/audio_output_de... media/audio/audio_output_device.cc:273: // TODO(dalecurtis): Remove this when we have float everywhere. On 2012/08/16 18:58:19, scherkus wrote: > reference bug? Done. http://codereview.chromium.org/10824304/diff/4003/media/base/audio_bus.cc File media/base/audio_bus.cc (right): http://codereview.chromium.org/10824304/diff/4003/media/base/audio_bus.cc#new... media/base/audio_bus.cc:25: int* aligned_frames) { On 2012/08/16 18:58:19, scherkus wrote: > alignment Done. http://codereview.chromium.org/10824304/diff/4003/media/base/audio_bus.cc#new... media/base/audio_bus.cc:26: DCHECK(aligned_frames); On 2012/08/16 18:58:19, scherkus wrote: > dcheck not helpful here as you immediately deref Done. http://codereview.chromium.org/10824304/diff/4003/media/base/audio_bus.cc#new... media/base/audio_bus.cc:32: return sizeof(float) * channels * *aligned_frames + kChannelAlignment; On 2012/08/16 18:58:19, scherkus wrote: > the * * is freaking me out -- want to do (*aligned_frames)? Done. http://codereview.chromium.org/10824304/diff/4003/media/base/audio_bus.cc#new... media/base/audio_bus.cc:159: void FromInterleavedInternal(const void* src, int frames, AudioBus* dest) { On 2012/08/16 18:58:19, scherkus wrote: > static + move to top Done. http://codereview.chromium.org/10824304/diff/4003/media/base/audio_bus.cc#new... media/base/audio_bus.cc:179: // TODO(dalecurtis): See if intrinsic optimizations help any here. On 2012/08/16 18:58:19, scherkus wrote: > can we get a bug tracking where we may want to use intrinsics? I think this is the last spot, I intend to remove the integer based mixing methods in favor of the AudioRendererMixer method I've already optimized. http://codereview.chromium.org/10824304/diff/4003/media/base/audio_bus.cc#new... media/base/audio_bus.cc:195: Zero(); On 2012/08/16 18:58:19, scherkus wrote: > add return after Zeroing? Done. http://codereview.chromium.org/10824304/diff/4003/media/base/audio_bus.cc#new... media/base/audio_bus.cc:209: static void ToInterleavedInternal(const AudioBus* source, int frames, On 2012/08/16 18:58:19, scherkus wrote: > we put static fn's at the top of files Done. http://codereview.chromium.org/10824304/diff/4003/media/base/audio_bus.h File media/base/audio_bus.h (right): http://codereview.chromium.org/10824304/diff/4003/media/base/audio_bus.h#newc... media/base/audio_bus.h:38: static scoped_ptr<AudioBus> WrapBlock(int channels, int frames, void* data); On 2012/08/16 19:50:33, DaleCurtis wrote: > On 2012/08/16 18:58:19, scherkus wrote: > > WrapData? "Block" isn't a very widely used term > > > > any restrictions on memory alignment? > > > > it's also very strange to see a memory pointer passed in w/o any size -- I'd > > almost encourage it for the sake of CHECK()ing > > > > > > As a side this API is a bit strange (call BlockSize(), allocate it however you > > wish, then pass the result in) but I don't have concrete suggestions as to how > > to improve things at the moment. > > > > One thought I had was whether this class should be a wrapper for externally > > allocated memory so that usage of AudioBus is consistent (i.e., replace > Create() > > with callers allocating aligned memory alongside an AudioBus), but we don't > need > > to figure that out in this CL. > > Agreed, ideally we'd have no Wrap methods :) Since this is explicitly intended > for wrapping shared memory. An alternative solution might be to have an > AudioBusSharedMemory subclass which adds WrapSharedMemory() and > CalculateSharedMemorySize(). > > Even without the subclass, I could name the methods in that style. I don't > forsee any other callers needing this information. > > WDYT? > > There are no alignment requirements since we require return our real size + > alignment leaving us room to slide the given pointer as necessary. Pared these down a bit to only the known use cases, fixed comments, cleaned up. |data| now requires kChannelAlignment, have a Q outstanding to chromium-dev about that. Renamed to WrapMemory(). http://codereview.chromium.org/10824304/diff/4003/media/base/audio_bus.h#newc... media/base/audio_bus.h:43: static int BlockSize(int channels, int frames); On 2012/08/16 18:58:19, scherkus wrote: > try to use verbs for functions -- this should be CalculateDataSize() or similar Done. http://codereview.chromium.org/10824304/diff/4003/media/base/audio_bus.h#newc... media/base/audio_bus.h:55: // conversion. ToInterleaved will also clip values to format range. Handles On 2012/08/16 18:58:19, scherkus wrote: > ToInterleaved() Done. http://codereview.chromium.org/10824304/diff/4003/media/base/audio_bus.h#newc... media/base/audio_bus.h:85: void ValidateConfig(int channels, int frames); On 2012/08/16 18:58:19, scherkus wrote: > const method Moved to static, not really any point to have it in the header.
Dale: results from the unit test on my Windows 7 machine. Note: Google Test filter = AudioBusTest* [==========] Running 8 tests from 1 test case. [----------] Global test environment set-up. [----------] 8 tests from AudioBusTest [ RUN ] AudioBusTest.Create base\audio_bus_unittest.cc(59): error: Value of: reinterpret_cast<uintptr_t>( bus->channel(i)) & AudioBus::kChannelAlignment Actual: 16 Expected: 0U Which is: 0 [ FAILED ] AudioBusTest.Create (1 ms) [ RUN ] AudioBusTest.CreateUsingAudioParameters base\audio_bus_unittest.cc(59): error: Value of: reinterpret_cast<uintptr_t>( bus->channel(i)) & AudioBus::kChannelAlignment Actual: 16 Expected: 0U Which is: 0 [ FAILED ] AudioBusTest.CreateUsingAudioParameters (0 ms) [ RUN ] AudioBusTest.WrapVector base\audio_bus_unittest.cc(59): error: Value of: reinterpret_cast<uintptr_t>( bus->channel(i)) & AudioBus::kChannelAlignment Actual: 16 Expected: 0U Which is: 0 [ FAILED ] AudioBusTest.WrapVector (1 ms) [ RUN ] AudioBusTest.WrapMemory base\audio_bus_unittest.cc(59): error: Value of: reinterpret_cast<uintptr_t>( bus->channel(i)) & AudioBus::kChannelAlignment Actual: 16 Expected: 0U Which is: 0 [ FAILED ] AudioBusTest.WrapMemory (2 ms) [ RUN ] AudioBusTest.CopyTo [ OK ] AudioBusTest.CopyTo (1716 ms) [ RUN ] AudioBusTest.Zero [ OK ] AudioBusTest.Zero (3 ms) [ RUN ] AudioBusTest.FromInterleaved [ OK ] AudioBusTest.FromInterleaved (1 ms) [ RUN ] AudioBusTest.ToInterleaved [ OK ] AudioBusTest.ToInterleaved (0 ms) [----------] 8 tests from AudioBusTest (1736 ms total) [----------] Global test environment tear-down [==========] 8 tests from 1 test case ran. (7402 ms total) [ PASSED ] 4 tests. [ FAILED ] 4 tests, listed below: [ FAILED ] AudioBusTest.Create [ FAILED ] AudioBusTest.CreateUsingAudioParameters [ FAILED ] AudioBusTest.WrapVector [ FAILED ] AudioBusTest.WrapMemory 4 FAILED TESTS YOU HAVE 11 DISABLED TESTS Press any key to continue . . .
On 2012/08/17 13:36:16, henrika wrote: LGTM assuming all tests are OK on all platforms.
http://codereview.chromium.org/10824304/diff/4005/content/renderer/media/webr... File content/renderer/media/webrtc_audio_device_impl.cc (right): http://codereview.chromium.org/10824304/diff/4005/content/renderer/media/webr... content/renderer/media/webrtc_audio_device_impl.cc:220: audio_bus->FromInterleaved(output_buffer_.get(), audio_bus->frames(), Looks really nice ;-) Just in case. Did you run the content_unittests and verify that the audio is OK for the WebRTC tests?
On 2012/08/17 13:36:16, henrika wrote: > Dale: results from the unit test on my Windows 7 machine. > > Note: Google Test filter = AudioBusTest* > [==========] Running 8 tests from 1 test case. > [----------] Global test environment set-up. > [----------] 8 tests from AudioBusTest > [ RUN ] AudioBusTest.Create > base\audio_bus_unittest.cc(59): error: Value of: reinterpret_cast<uintptr_t>( > bus->channel(i)) & AudioBus::kChannelAlignment > Actual: 16 > Expected: 0U > Which is: 0 > [ FAILED ] AudioBusTest.Create (1 ms) > [ RUN ] AudioBusTest.CreateUsingAudioParameters > base\audio_bus_unittest.cc(59): error: Value of: reinterpret_cast<uintptr_t>( > bus->channel(i)) & AudioBus::kChannelAlignment > Actual: 16 > Expected: 0U > Which is: 0 > [ FAILED ] AudioBusTest.CreateUsingAudioParameters (0 ms) > [ RUN ] AudioBusTest.WrapVector > base\audio_bus_unittest.cc(59): error: Value of: reinterpret_cast<uintptr_t>( > bus->channel(i)) & AudioBus::kChannelAlignment > Actual: 16 > Expected: 0U > Which is: 0 > [ FAILED ] AudioBusTest.WrapVector (1 ms) > [ RUN ] AudioBusTest.WrapMemory > base\audio_bus_unittest.cc(59): error: Value of: reinterpret_cast<uintptr_t>( > bus->channel(i)) & AudioBus::kChannelAlignment > Actual: 16 > Expected: 0U > Which is: 0 > [ FAILED ] AudioBusTest.WrapMemory (2 ms) > [ RUN ] AudioBusTest.CopyTo > [ OK ] AudioBusTest.CopyTo (1716 ms) > [ RUN ] AudioBusTest.Zero > [ OK ] AudioBusTest.Zero (3 ms) > [ RUN ] AudioBusTest.FromInterleaved > [ OK ] AudioBusTest.FromInterleaved (1 ms) > [ RUN ] AudioBusTest.ToInterleaved > [ OK ] AudioBusTest.ToInterleaved (0 ms) > [----------] 8 tests from AudioBusTest (1736 ms total) > > [----------] Global test environment tear-down > [==========] 8 tests from 1 test case ran. (7402 ms total) > [ PASSED ] 4 tests. > [ FAILED ] 4 tests, listed below: > [ FAILED ] AudioBusTest.Create > [ FAILED ] AudioBusTest.CreateUsingAudioParameters > [ FAILED ] AudioBusTest.WrapVector > [ FAILED ] AudioBusTest.WrapMemory > > 4 FAILED TESTS > YOU HAVE 11 DISABLED TESTS > > Press any key to continue . . . Thanks Henrik, looks like I dropped a -1 when converting kChannelAlignment to an enum. Wonder why the bots didn't hit this! I did run the content_unittests earlier, but will make do so again before submitting. I'll also run a lonely WebRTC hangout to make sure everything is okay :)
FYI CL description still mentions BlockSize() LGTM w/ nit + q http://codereview.chromium.org/10824304/diff/10031/media/base/audio_bus.cc File media/base/audio_bus.cc (right): http://codereview.chromium.org/10824304/diff/10031/media/base/audio_bus.cc#ne... media/base/audio_bus.cc:17: (AudioBus::kChannelAlignment - 1)) == 0U; left paren ( is still open, so this code should line up with the "reinterpret_cast" bit http://codereview.chromium.org/10824304/diff/10031/media/base/audio_bus.cc#ne... media/base/audio_bus.cc:25: // Choose a size such that each channel is aligned by kChannelAlignment. Do we still need the kChannelAlignment stuff if the caller must passed in an aligned pointer?
http://codereview.chromium.org/10824304/diff/10031/media/base/audio_bus.cc File media/base/audio_bus.cc (right): http://codereview.chromium.org/10824304/diff/10031/media/base/audio_bus.cc#ne... media/base/audio_bus.cc:17: (AudioBus::kChannelAlignment - 1)) == 0U; On 2012/08/17 23:46:07, scherkus wrote: > left paren ( is still open, so this code should line up with the > "reinterpret_cast" bit Done. http://codereview.chromium.org/10824304/diff/10031/media/base/audio_bus.cc#ne... media/base/audio_bus.cc:25: // Choose a size such that each channel is aligned by kChannelAlignment. On 2012/08/17 23:46:07, scherkus wrote: > Do we still need the kChannelAlignment stuff if the caller must passed in an > aligned pointer? Yes because we don't require the user to provide a frame size that will result in channel(N>1) being 16-byte aligned. I've removed the extra padding by kChannelAlignment though. I realized this calculation was slightly over done though since it was adjusting frames without taking into the bytes per sample. I've corrected this in the current patch set. http://codereview.chromium.org/10824304/diff/6009/media/base/audio_bus.cc File media/base/audio_bus.cc (right): http://codereview.chromium.org/10824304/diff/6009/media/base/audio_bus.cc#new... media/base/audio_bus.cc:146: scoped_ptr<AudioBus> AudioBus::WrapMemory(int channels, int frames, Ended up needing this for PPAPI since they don't have an AudioParameters object.
LGTM - thanks Dale! I have some bikeshedding comments, but you can ignore if you strongly prefer the current way http://codereview.chromium.org/10824304/diff/6009/media/base/audio_bus.h File media/base/audio_bus.h (right): http://codereview.chromium.org/10824304/diff/6009/media/base/audio_bus.h#newc... media/base/audio_bus.h:44: void* data); nit: my preference would be to have the void* param be first. I think it's more common and reads better this way. http://codereview.chromium.org/10824304/diff/6009/media/base/audio_bus.h#newc... media/base/audio_bus.h:54: void ToInterleaved(int frames, int bytes_per_sample, void* dest) const; nit: it would be nice to have the pointer |dest| be the first argument, matching FromInterleaved(). It's a little confusing because of this asymmetry http://codereview.chromium.org/10824304/diff/6009/media/base/audio_bus.h#newc... media/base/audio_bus.h:77: AudioBus(int channels, int frames, float* data); nit: |data| looks better as 1st argument http://codereview.chromium.org/10824304/diff/6009/media/base/audio_bus.h#newc... media/base/audio_bus.h:82: void BuildChannelData(int channels, int aligned_frame, float* data); nit: |data| looks better as 1st argument
Thanks for review Chris. The current ordering is the Chrome-style: http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi... I don't think there's a strong reason break style and write it the other way, so I'll keep it as is. On 2012/08/21 00:14:10, Chris Rogers wrote: > LGTM - thanks Dale! > > I have some bikeshedding comments, but you can ignore if you strongly prefer the > current way > > http://codereview.chromium.org/10824304/diff/6009/media/base/audio_bus.h > File media/base/audio_bus.h (right): > > http://codereview.chromium.org/10824304/diff/6009/media/base/audio_bus.h#newc... > media/base/audio_bus.h:44: void* data); > nit: my preference would be to have the void* param be first. I think it's more > common and reads better this way. > > http://codereview.chromium.org/10824304/diff/6009/media/base/audio_bus.h#newc... > media/base/audio_bus.h:54: void ToInterleaved(int frames, int bytes_per_sample, > void* dest) const; > nit: it would be nice to have the pointer |dest| be the first argument, matching > FromInterleaved(). It's a little confusing because of this asymmetry > > http://codereview.chromium.org/10824304/diff/6009/media/base/audio_bus.h#newc... > media/base/audio_bus.h:77: AudioBus(int channels, int frames, float* data); > nit: |data| looks better as 1st argument > > http://codereview.chromium.org/10824304/diff/6009/media/base/audio_bus.h#newc... > media/base/audio_bus.h:82: void BuildChannelData(int channels, int > aligned_frame, float* data); > nit: |data| looks better as 1st argument
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dalecurtis@chromium.org/10824304/6009
Change committed as 152494 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
