Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10533004:
Use aligned buffer in the audio mixer. (Closed)
|
Created:
8 years, 6 months ago by enal Modified:
8 years, 6 months ago CC:
chromium-reviews, feature-media-reviews_chromium.org Base URL:
http://src.chromium.org/svn/trunk/src/ Visibility:
Public. |
DescriptionUse aligned buffer in the audio mixer.
Allows us in the future to use faster SSE/AVX intrinsics.
TEST=None, should work as expected.
Patch Set 1 #
Total comments: 1
Messages
Total messages: 12 (0 generated)
Dale, can you please take a look?
https://chromiumcodereview.appspot.com/10533004/diff/1/media/audio/audio_outp... File media/audio/audio_output_mixer.cc (right): https://chromiumcodereview.appspot.com/10533004/diff/1/media/audio/audio_outp... media/audio/audio_output_mixer.cc:23: mixer_data_(new DecoderBuffer(params_.GetBytesPerBuffer())), I'm not convinced we should be using DecoderBuffer as a general purpose aligned memory allocator. Would src/base/memory/aligned_memory.h work here instead?
On 2012/06/05 16:37:43, scherkus wrote: > https://chromiumcodereview.appspot.com/10533004/diff/1/media/audio/audio_outp... > File media/audio/audio_output_mixer.cc (right): > > https://chromiumcodereview.appspot.com/10533004/diff/1/media/audio/audio_outp... > media/audio/audio_output_mixer.cc:23: mixer_data_(new > DecoderBuffer(params_.GetBytesPerBuffer())), > I'm not convinced we should be using DecoderBuffer as a general purpose aligned > memory allocator. > > Would src/base/memory/aligned_memory.h work here instead? I thought about it. DecoderBuffer is too specialized, I'd prefer something more lightweight. We don't need reference counter. What finally bought me is the fact that it hides exact alignment amount. Of course I could directly use FF_INPUT_BUFFER_PADDING_SIZE, but resulting code is less elegant. And overhead is small enough.
On 2012/06/05 16:44:49, enal1 wrote: > On 2012/06/05 16:37:43, scherkus wrote: > > > https://chromiumcodereview.appspot.com/10533004/diff/1/media/audio/audio_outp... > > File media/audio/audio_output_mixer.cc (right): > > > > > https://chromiumcodereview.appspot.com/10533004/diff/1/media/audio/audio_outp... > > media/audio/audio_output_mixer.cc:23: mixer_data_(new > > DecoderBuffer(params_.GetBytesPerBuffer())), > > I'm not convinced we should be using DecoderBuffer as a general purpose > aligned > > memory allocator. > > > > Would src/base/memory/aligned_memory.h work here instead? > > I thought about it. DecoderBuffer is too specialized, I'd prefer something more > lightweight. We don't need reference counter. > > What finally bought me is the fact that it hides exact alignment amount. Of > course I could directly use FF_INPUT_BUFFER_PADDING_SIZE, but resulting code is > less elegant. And overhead is small enough. BTW, FF_INPUT_BUFFER_PADDING_SIZE is probably wrong -- it is 16 bytes, while AVX needs more. With DecoderBuffer I can be sure data is properly aligned.
On 2012/06/05 16:54:35, enal1 wrote: > On 2012/06/05 16:44:49, enal1 wrote: > > On 2012/06/05 16:37:43, scherkus wrote: > > > > > > https://chromiumcodereview.appspot.com/10533004/diff/1/media/audio/audio_outp... > > > File media/audio/audio_output_mixer.cc (right): > > > > > > > > > https://chromiumcodereview.appspot.com/10533004/diff/1/media/audio/audio_outp... > > > media/audio/audio_output_mixer.cc:23: mixer_data_(new > > > DecoderBuffer(params_.GetBytesPerBuffer())), > > > I'm not convinced we should be using DecoderBuffer as a general purpose > > aligned > > > memory allocator. > > > > > > Would src/base/memory/aligned_memory.h work here instead? > > > > I thought about it. DecoderBuffer is too specialized, I'd prefer something > more > > lightweight. We don't need reference counter. > > > > What finally bought me is the fact that it hides exact alignment amount. Of > > course I could directly use FF_INPUT_BUFFER_PADDING_SIZE, but resulting code > is > > less elegant. And overhead is small enough. > > BTW, FF_INPUT_BUFFER_PADDING_SIZE is probably wrong -- it is 16 bytes, while AVX > needs more. With DecoderBuffer I can be sure data is properly aligned. If we want to have a memory allocator for media purposes would it make more sense to pull out + wrap that tiny bit of code + document the memory allocation semantics instead of relying on the implementation details of DecoderBuffer?
On 2012/06/05 16:54:35, enal1 wrote: > On 2012/06/05 16:44:49, enal1 wrote: > > On 2012/06/05 16:37:43, scherkus wrote: > > > > > > https://chromiumcodereview.appspot.com/10533004/diff/1/media/audio/audio_outp... > > > File media/audio/audio_output_mixer.cc (right): > > > > > > > > > https://chromiumcodereview.appspot.com/10533004/diff/1/media/audio/audio_outp... > > > media/audio/audio_output_mixer.cc:23: mixer_data_(new > > > DecoderBuffer(params_.GetBytesPerBuffer())), > > > I'm not convinced we should be using DecoderBuffer as a general purpose > > aligned > > > memory allocator. > > > > > > Would src/base/memory/aligned_memory.h work here instead? > > > > I thought about it. DecoderBuffer is too specialized, I'd prefer something > more > > lightweight. We don't need reference counter. > > > > What finally bought me is the fact that it hides exact alignment amount. Of > > course I could directly use FF_INPUT_BUFFER_PADDING_SIZE, but resulting code > is > > less elegant. And overhead is small enough. > > BTW, FF_INPUT_BUFFER_PADDING_SIZE is probably wrong -- it is 16 bytes, while AVX > needs more. With DecoderBuffer I can be sure data is properly aligned. We're not aligning based on FF_INPUT_BUFFER_PADDING_SIZE, the av_malloc call runs posix_memalign on Linux and a hacked memalign on Windows/Mac. The buffer padding is another requirement. aligned_memory.h is only for static (compile time) alignments, you can't use it for dynamic allocations :( We could refactor the memory allocation bits of DecoderBuffer into say AlignedData and have DecoderBuffer inherit Buffer and AlignedData. Using DecoderBuffer here is definitely not ideal, but the alternative is implementing your own hacked memalign
We have 2 allocations with same requirement -- one in ffmpeg, another in chrome. It is better to reuse existing code instead of duplicating it. DecoderBuffer internally uses ffmpeg av_malloc() which guarantees alignment. Yes, maybe we can refactor ffmpeg code -- see Dale's comment. On 2012/06/05 17:20:29, DaleCurtis wrote: > On 2012/06/05 16:54:35, enal1 wrote: > > On 2012/06/05 16:44:49, enal1 wrote: > > > On 2012/06/05 16:37:43, scherkus wrote: > > > > > > > > > > https://chromiumcodereview.appspot.com/10533004/diff/1/media/audio/audio_outp... > > > > File media/audio/audio_output_mixer.cc (right): > > > > > > > > > > > > > > https://chromiumcodereview.appspot.com/10533004/diff/1/media/audio/audio_outp... > > > > media/audio/audio_output_mixer.cc:23: mixer_data_(new > > > > DecoderBuffer(params_.GetBytesPerBuffer())), > > > > I'm not convinced we should be using DecoderBuffer as a general purpose > > > aligned > > > > memory allocator. > > > > > > > > Would src/base/memory/aligned_memory.h work here instead? > > > > > > I thought about it. DecoderBuffer is too specialized, I'd prefer something > > more > > > lightweight. We don't need reference counter. > > > > > > What finally bought me is the fact that it hides exact alignment amount. Of > > > course I could directly use FF_INPUT_BUFFER_PADDING_SIZE, but resulting code > > is > > > less elegant. And overhead is small enough. > > > > BTW, FF_INPUT_BUFFER_PADDING_SIZE is probably wrong -- it is 16 bytes, while > AVX > > needs more. With DecoderBuffer I can be sure data is properly aligned. > > We're not aligning based on FF_INPUT_BUFFER_PADDING_SIZE, the av_malloc call > runs posix_memalign on Linux and a hacked memalign on Windows/Mac. The buffer > padding is another requirement. > > aligned_memory.h is only for static (compile time) alignments, you can't use it > for dynamic allocations :( > > We could refactor the memory allocation bits of DecoderBuffer into say > AlignedData and have DecoderBuffer inherit Buffer and AlignedData. > > Using DecoderBuffer here is definitely not ideal, but the alternative is > implementing your own hacked memalign
On 2012/06/05 17:20:29, DaleCurtis wrote: > On 2012/06/05 16:54:35, enal1 wrote: > > On 2012/06/05 16:44:49, enal1 wrote: > > > On 2012/06/05 16:37:43, scherkus wrote: > > > > > > > > > > https://chromiumcodereview.appspot.com/10533004/diff/1/media/audio/audio_outp... > > > > File media/audio/audio_output_mixer.cc (right): > > > > > > > > > > > > > > https://chromiumcodereview.appspot.com/10533004/diff/1/media/audio/audio_outp... > > > > media/audio/audio_output_mixer.cc:23: mixer_data_(new > > > > DecoderBuffer(params_.GetBytesPerBuffer())), > > > > I'm not convinced we should be using DecoderBuffer as a general purpose > > > aligned > > > > memory allocator. > > > > > > > > Would src/base/memory/aligned_memory.h work here instead? > > > > > > I thought about it. DecoderBuffer is too specialized, I'd prefer something > > more > > > lightweight. We don't need reference counter. > > > > > > What finally bought me is the fact that it hides exact alignment amount. Of > > > course I could directly use FF_INPUT_BUFFER_PADDING_SIZE, but resulting code > > is > > > less elegant. And overhead is small enough. > > > > BTW, FF_INPUT_BUFFER_PADDING_SIZE is probably wrong -- it is 16 bytes, while > AVX > > needs more. With DecoderBuffer I can be sure data is properly aligned. > > We're not aligning based on FF_INPUT_BUFFER_PADDING_SIZE, the av_malloc call > runs posix_memalign on Linux and a hacked memalign on Windows/Mac. The buffer > padding is another requirement. > > aligned_memory.h is only for static (compile time) alignments, you can't use it > for dynamic allocations :( > > We could refactor the memory allocation bits of DecoderBuffer into say > AlignedData and have DecoderBuffer inherit Buffer and AlignedData. > > Using DecoderBuffer here is definitely not ideal, but the alternative is > implementing your own hacked memalign Would it help to have a simple scoped aligned memory allocator that uses av_malloc/free if available and something else on platforms that don't use FFmpeg (i.e., Android)? namespace media { class Aligned { public: Aligned(size) : size_(size), data_(av_malloc(size + PADDING)) {} ~Aligned() { av_free(data_); } void* data() { return data_; } int size() { return size_; } }; } // namespace
On 2012/06/05 17:50:32, scherkus wrote: > On 2012/06/05 17:20:29, DaleCurtis wrote: > > On 2012/06/05 16:54:35, enal1 wrote: > > > On 2012/06/05 16:44:49, enal1 wrote: > > > > On 2012/06/05 16:37:43, scherkus wrote: > > > > > > > > > > > > > > > https://chromiumcodereview.appspot.com/10533004/diff/1/media/audio/audio_outp... > > > > > File media/audio/audio_output_mixer.cc (right): > > > > > > > > > > > > > > > > > > > > https://chromiumcodereview.appspot.com/10533004/diff/1/media/audio/audio_outp... > > > > > media/audio/audio_output_mixer.cc:23: mixer_data_(new > > > > > DecoderBuffer(params_.GetBytesPerBuffer())), > > > > > I'm not convinced we should be using DecoderBuffer as a general purpose > > > > aligned > > > > > memory allocator. > > > > > > > > > > Would src/base/memory/aligned_memory.h work here instead? > > > > > > > > I thought about it. DecoderBuffer is too specialized, I'd prefer something > > > more > > > > lightweight. We don't need reference counter. > > > > > > > > What finally bought me is the fact that it hides exact alignment amount. > Of > > > > course I could directly use FF_INPUT_BUFFER_PADDING_SIZE, but resulting > code > > > is > > > > less elegant. And overhead is small enough. > > > > > > BTW, FF_INPUT_BUFFER_PADDING_SIZE is probably wrong -- it is 16 bytes, while > > AVX > > > needs more. With DecoderBuffer I can be sure data is properly aligned. > > > > We're not aligning based on FF_INPUT_BUFFER_PADDING_SIZE, the av_malloc call > > runs posix_memalign on Linux and a hacked memalign on Windows/Mac. The buffer > > padding is another requirement. > > > > aligned_memory.h is only for static (compile time) alignments, you can't use > it > > for dynamic allocations :( > > > > We could refactor the memory allocation bits of DecoderBuffer into say > > AlignedData and have DecoderBuffer inherit Buffer and AlignedData. > > > > Using DecoderBuffer here is definitely not ideal, but the alternative is > > implementing your own hacked memalign > > Would it help to have a simple scoped aligned memory allocator that uses > av_malloc/free if available and something else on platforms that don't use > FFmpeg (i.e., Android)? > > namespace media { > class Aligned { > public: > Aligned(size) : size_(size), data_(av_malloc(size + PADDING)) {} > ~Aligned() { av_free(data_); } > > void* data() { return data_; } > int size() { return size_; } > }; > } // namespace Yes, that would work -- Dale suggested exactly that as proper fix for the future, see his reply earlier. Not sure if that's worth it. Our classes multiply like rabbits, and absolute majority of them is simple wrappers around one line of code doing actual work...
On 2012/06/05 17:50:32, scherkus wrote: > On 2012/06/05 17:20:29, DaleCurtis wrote: > > On 2012/06/05 16:54:35, enal1 wrote: > > > On 2012/06/05 16:44:49, enal1 wrote: > > > > On 2012/06/05 16:37:43, scherkus wrote: > > > > > > > > > > > > > > > https://chromiumcodereview.appspot.com/10533004/diff/1/media/audio/audio_outp... > > > > > File media/audio/audio_output_mixer.cc (right): > > > > > > > > > > > > > > > > > > > > https://chromiumcodereview.appspot.com/10533004/diff/1/media/audio/audio_outp... > > > > > media/audio/audio_output_mixer.cc:23: mixer_data_(new > > > > > DecoderBuffer(params_.GetBytesPerBuffer())), > > > > > I'm not convinced we should be using DecoderBuffer as a general purpose > > > > aligned > > > > > memory allocator. > > > > > > > > > > Would src/base/memory/aligned_memory.h work here instead? > > > > > > > > I thought about it. DecoderBuffer is too specialized, I'd prefer something > > > more > > > > lightweight. We don't need reference counter. > > > > > > > > What finally bought me is the fact that it hides exact alignment amount. > Of > > > > course I could directly use FF_INPUT_BUFFER_PADDING_SIZE, but resulting > code > > > is > > > > less elegant. And overhead is small enough. > > > > > > BTW, FF_INPUT_BUFFER_PADDING_SIZE is probably wrong -- it is 16 bytes, while > > AVX > > > needs more. With DecoderBuffer I can be sure data is properly aligned. > > > > We're not aligning based on FF_INPUT_BUFFER_PADDING_SIZE, the av_malloc call > > runs posix_memalign on Linux and a hacked memalign on Windows/Mac. The buffer > > padding is another requirement. > > > > aligned_memory.h is only for static (compile time) alignments, you can't use > it > > for dynamic allocations :( > > > > We could refactor the memory allocation bits of DecoderBuffer into say > > AlignedData and have DecoderBuffer inherit Buffer and AlignedData. > > > > Using DecoderBuffer here is definitely not ideal, but the alternative is > > implementing your own hacked memalign > > Would it help to have a simple scoped aligned memory allocator that uses > av_malloc/free if available and something else on platforms that don't use > FFmpeg (i.e., Android)? > > namespace media { > class Aligned { > public: > Aligned(size) : size_(size), data_(av_malloc(size + PADDING)) {} > ~Aligned() { av_free(data_); } > > void* data() { return data_; } > int size() { return size_; } > }; > } // namespace That's roughly what I was suggesting, but with the further requirement that we refactor DecoderBuffer on top it so we don't have to duplicate the testing in place there. Short term, what's enal has now is probably fine. We just need to mark it with a TODO(dalecurtis): Refactor DecoderBuffer into AlignedData.
On 2012/06/05 18:02:28, DaleCurtis wrote: > On 2012/06/05 17:50:32, scherkus wrote: > > On 2012/06/05 17:20:29, DaleCurtis wrote: > > > On 2012/06/05 16:54:35, enal1 wrote: > > > > On 2012/06/05 16:44:49, enal1 wrote: > > > > > On 2012/06/05 16:37:43, scherkus wrote: > > > > > > > > > > > > > > > > > > > > > https://chromiumcodereview.appspot.com/10533004/diff/1/media/audio/audio_outp... > > > > > > File media/audio/audio_output_mixer.cc (right): > > > > > > > > > > > > > > > > > > > > > > > > > > > https://chromiumcodereview.appspot.com/10533004/diff/1/media/audio/audio_outp... > > > > > > media/audio/audio_output_mixer.cc:23: mixer_data_(new > > > > > > DecoderBuffer(params_.GetBytesPerBuffer())), > > > > > > I'm not convinced we should be using DecoderBuffer as a general > purpose > > > > > aligned > > > > > > memory allocator. > > > > > > > > > > > > Would src/base/memory/aligned_memory.h work here instead? > > > > > > > > > > I thought about it. DecoderBuffer is too specialized, I'd prefer > something > > > > more > > > > > lightweight. We don't need reference counter. > > > > > > > > > > What finally bought me is the fact that it hides exact alignment amount. > > Of > > > > > course I could directly use FF_INPUT_BUFFER_PADDING_SIZE, but resulting > > code > > > > is > > > > > less elegant. And overhead is small enough. > > > > > > > > BTW, FF_INPUT_BUFFER_PADDING_SIZE is probably wrong -- it is 16 bytes, > while > > > AVX > > > > needs more. With DecoderBuffer I can be sure data is properly aligned. > > > > > > We're not aligning based on FF_INPUT_BUFFER_PADDING_SIZE, the av_malloc call > > > runs posix_memalign on Linux and a hacked memalign on Windows/Mac. The > buffer > > > padding is another requirement. > > > > > > aligned_memory.h is only for static (compile time) alignments, you can't use > > it > > > for dynamic allocations :( > > > > > > We could refactor the memory allocation bits of DecoderBuffer into say > > > AlignedData and have DecoderBuffer inherit Buffer and AlignedData. > > > > > > Using DecoderBuffer here is definitely not ideal, but the alternative is > > > implementing your own hacked memalign > > > > Would it help to have a simple scoped aligned memory allocator that uses > > av_malloc/free if available and something else on platforms that don't use > > FFmpeg (i.e., Android)? > > > > namespace media { > > class Aligned { > > public: > > Aligned(size) : size_(size), data_(av_malloc(size + PADDING)) {} > > ~Aligned() { av_free(data_); } > > > > void* data() { return data_; } > > int size() { return size_; } > > }; > > } // namespace > > That's roughly what I was suggesting, but with the further requirement that we > refactor DecoderBuffer on top it so we don't have to duplicate the testing in > place there. > > Short term, what's enal has now is probably fine. We just need to mark it with a > TODO(dalecurtis): Refactor DecoderBuffer into AlignedData. Another solution would be to build something similar to what av_malloc does into base/ so all of Chrome can have a general purpose dynamic aligned allocator.
On 2012/06/05 18:04:43, DaleCurtis wrote: > On 2012/06/05 18:02:28, DaleCurtis wrote: > > On 2012/06/05 17:50:32, scherkus wrote: > > > On 2012/06/05 17:20:29, DaleCurtis wrote: > > > > On 2012/06/05 16:54:35, enal1 wrote: > > > > > On 2012/06/05 16:44:49, enal1 wrote: > > > > > > On 2012/06/05 16:37:43, scherkus wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://chromiumcodereview.appspot.com/10533004/diff/1/media/audio/audio_outp... > > > > > > > File media/audio/audio_output_mixer.cc (right): > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://chromiumcodereview.appspot.com/10533004/diff/1/media/audio/audio_outp... > > > > > > > media/audio/audio_output_mixer.cc:23: mixer_data_(new > > > > > > > DecoderBuffer(params_.GetBytesPerBuffer())), > > > > > > > I'm not convinced we should be using DecoderBuffer as a general > > purpose > > > > > > aligned > > > > > > > memory allocator. > > > > > > > > > > > > > > Would src/base/memory/aligned_memory.h work here instead? > > > > > > > > > > > > I thought about it. DecoderBuffer is too specialized, I'd prefer > > something > > > > > more > > > > > > lightweight. We don't need reference counter. > > > > > > > > > > > > What finally bought me is the fact that it hides exact alignment > amount. > > > Of > > > > > > course I could directly use FF_INPUT_BUFFER_PADDING_SIZE, but > resulting > > > code > > > > > is > > > > > > less elegant. And overhead is small enough. > > > > > > > > > > BTW, FF_INPUT_BUFFER_PADDING_SIZE is probably wrong -- it is 16 bytes, > > while > > > > AVX > > > > > needs more. With DecoderBuffer I can be sure data is properly aligned. > > > > > > > > We're not aligning based on FF_INPUT_BUFFER_PADDING_SIZE, the av_malloc > call > > > > runs posix_memalign on Linux and a hacked memalign on Windows/Mac. The > > buffer > > > > padding is another requirement. > > > > > > > > aligned_memory.h is only for static (compile time) alignments, you can't > use > > > it > > > > for dynamic allocations :( > > > > > > > > We could refactor the memory allocation bits of DecoderBuffer into say > > > > AlignedData and have DecoderBuffer inherit Buffer and AlignedData. > > > > > > > > Using DecoderBuffer here is definitely not ideal, but the alternative is > > > > implementing your own hacked memalign > > > > > > Would it help to have a simple scoped aligned memory allocator that uses > > > av_malloc/free if available and something else on platforms that don't use > > > FFmpeg (i.e., Android)? > > > > > > namespace media { > > > class Aligned { > > > public: > > > Aligned(size) : size_(size), data_(av_malloc(size + PADDING)) {} > > > ~Aligned() { av_free(data_); } > > > > > > void* data() { return data_; } > > > int size() { return size_; } > > > }; > > > } // namespace > > > > That's roughly what I was suggesting, but with the further requirement that we > > refactor DecoderBuffer on top it so we don't have to duplicate the testing in > > place there. > > > > Short term, what's enal has now is probably fine. We just need to mark it with > a > > TODO(dalecurtis): Refactor DecoderBuffer into AlignedData. > > Another solution would be to build something similar to what av_malloc does into > base/ so all of Chrome can have a general purpose dynamic aligned allocator. Ok, abandoning it for now. We do not require alignment yet... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
