Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
600143002:
Adding new function ReadFrames() that returns the audio frame in planar (Closed)
|
Created:
6 years, 2 months ago by pwestin(chromium) Modified:
6 years, 1 month ago Reviewers:
DaleCurtis CC:
chromium-reviews, feature-media-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionAdding new function ReadFrames() that returns the audio frame in planar
format int32.
BUG=
Committed: https://crrev.com/34e0ac22e25cfac606296646ecb765fa4bd3b54e
Cr-Commit-Position: refs/heads/master@{#297340}
Patch Set 1 #
Total comments: 5
Patch Set 2 : Changed to signed 32 bit interleaved mode. #Patch Set 3 : Updated description #Patch Set 4 : Adding trim_start_. #
Total comments: 4
Patch Set 5 : Draft of unittest #Patch Set 6 : Addressing Dales comments #Patch Set 7 : Functioning unittests #
Total comments: 8
Patch Set 8 : Added a template function to make the code have less repeated code #Patch Set 9 : Actually include the template function #
Total comments: 4
Patch Set 10 : Fixing nits #Patch Set 11 : Move files to lib target #Messages
Total messages: 14 (2 generated)
pwestin@chromium.org changed reviewers: + dalecurtis@chromium.org
On 2014/09/24 21:41:27, pwestin(chromium) wrote: > mailto:pwestin@chromium.org changed reviewers: > + mailto:dalecurtis@chromium.org Dale; please have a quick look at this CL to see if it matches what you thought; if it does I'll add unit test for it.
Rough comments only for now. I'll take a look at your conversion methods more closely later. https://codereview.chromium.org/600143002/diff/1/media/base/audio_buffer.cc File media/base/audio_buffer.cc (right): https://codereview.chromium.org/600143002/diff/1/media/base/audio_buffer.cc#n... media/base/audio_buffer.cc:248: static const int32 kUint8Bias = 128; remove capital U or capitalize int above. https://codereview.chromium.org/600143002/diff/1/media/base/audio_buffer.cc#n... media/base/audio_buffer.cc:251: return static_cast<int32>(value) << 8; These are mislabeled, you're actually converting to S24. https://codereview.chromium.org/600143002/diff/1/media/base/audio_buffer.cc#n... media/base/audio_buffer.cc:262: std::vector<int32> AudioBuffer::ReadFrames() { All these conversion methods need to take trim_start into account. https://codereview.chromium.org/600143002/diff/1/media/base/audio_buffer.cc#n... media/base/audio_buffer.cc:267: if (sample_format_ == kSampleFormatU8) { It may be worth finally adding in FFmpeg's super optimized libavresample or at least some shim to get their sample format converters. We don't use it today since it's a lot of extra code, but a shim approach could be profitable if you're on a constrained system, third_party/libavresample. https://codereview.chromium.org/600143002/diff/1/media/base/audio_buffer.h File media/base/audio_buffer.h (right): https://codereview.chromium.org/600143002/diff/1/media/base/audio_buffer.h#ne... media/base/audio_buffer.h:79: std::vector<int32> ReadFrames(); We don't have a SampleFormat for planar 24 bit, only interleaved 32 bit. Which do you actually need? Also, I was thinking this would be two methods like: void ReadFramesInterleaved(int frames_to_copy, SampleFormat output_format, void* dest); void ReadFramesPlanar(int frames_to_copy, int plane_length, SampleFormat output_format, void* dest); Note, you'll need to be careful about adjusted_frame_count_ and trim_start_. Implementing a generic method may be overkill if you only expect to output 24-bit PCM, so I'd be fine with something like: void ReadFramesPlanarS24(int frames_to_copy, int plane_length, int32* dest); If you always know your input sample format I'd be fine with CHECK_EQ()'ing that and titling the method appropriately.
On 2014/09/24 22:30:17, DaleCurtis wrote: > Rough comments only for now. I'll take a look at your conversion methods more > closely later. > > https://codereview.chromium.org/600143002/diff/1/media/base/audio_buffer.cc > File media/base/audio_buffer.cc (right): > > https://codereview.chromium.org/600143002/diff/1/media/base/audio_buffer.cc#n... > media/base/audio_buffer.cc:248: static const int32 kUint8Bias = 128; > remove capital U or capitalize int above. > > https://codereview.chromium.org/600143002/diff/1/media/base/audio_buffer.cc#n... > media/base/audio_buffer.cc:251: return static_cast<int32>(value) << 8; > These are mislabeled, you're actually converting to S24. > > https://codereview.chromium.org/600143002/diff/1/media/base/audio_buffer.cc#n... > media/base/audio_buffer.cc:262: std::vector<int32> AudioBuffer::ReadFrames() { > All these conversion methods need to take trim_start into account. > > https://codereview.chromium.org/600143002/diff/1/media/base/audio_buffer.cc#n... > media/base/audio_buffer.cc:267: if (sample_format_ == kSampleFormatU8) { > It may be worth finally adding in FFmpeg's super optimized libavresample or at > least some shim to get their sample format converters. We don't use it today > since it's a lot of extra code, but a shim approach could be profitable if > you're on a constrained system, third_party/libavresample. > > https://codereview.chromium.org/600143002/diff/1/media/base/audio_buffer.h > File media/base/audio_buffer.h (right): > > https://codereview.chromium.org/600143002/diff/1/media/base/audio_buffer.h#ne... > media/base/audio_buffer.h:79: std::vector<int32> ReadFrames(); > We don't have a SampleFormat for planar 24 bit, only interleaved 32 bit. Which > do you actually need? Also, I was thinking this would be two methods like: > > void ReadFramesInterleaved(int frames_to_copy, SampleFormat output_format, void* > dest); > void ReadFramesPlanar(int frames_to_copy, int plane_length, SampleFormat > output_format, void* dest); > > Note, you'll need to be careful about adjusted_frame_count_ and trim_start_. > Implementing a generic method may be overkill if you only expect to output > 24-bit PCM, so I'd be fine with something like: > > void ReadFramesPlanarS24(int frames_to_copy, int plane_length, int32* dest); > > If you always know your input sample format I'd be fine with CHECK_EQ()'ing that > and titling the method appropriately. PTAL
Can you add some unittests for this? You might consider adding a perftest if that's something you care about. https://codereview.chromium.org/600143002/diff/60001/media/base/audio_buffer.cc File media/base/audio_buffer.cc (right): https://codereview.chromium.org/600143002/diff/60001/media/base/audio_buffer.... media/base/audio_buffer.cc:258: ? value * ~std::numeric_limits<int32>::min() I don't think this does what you want; e.g., ~(-5) == 4, instead try -value without the ~ on the min. https://codereview.chromium.org/600143002/diff/60001/media/base/audio_buffer.... media/base/audio_buffer.cc:307: dest_data[ch + (i * dest_data_frame_size)] = A paired incrementing offset is more efficient than computing this multiplication every time, see AudioBus::FromInterleavedInternal. Minor in any case, so up to you. https://codereview.chromium.org/600143002/diff/60001/media/base/audio_buffer.... media/base/audio_buffer.cc:320: dest_data[ch + (i * dest_data_frame_size)] = Ditto. https://codereview.chromium.org/600143002/diff/60001/media/base/audio_buffer.... media/base/audio_buffer.cc:325: NOTREACHED(); If you prefer you could use case statement since it explictly has to list all possible types.
On 2014/09/25 22:30:44, DaleCurtis wrote: > Can you add some unittests for this? You might consider adding a perftest if > that's something you care about. > > https://codereview.chromium.org/600143002/diff/60001/media/base/audio_buffer.cc > File media/base/audio_buffer.cc (right): > > https://codereview.chromium.org/600143002/diff/60001/media/base/audio_buffer.... > media/base/audio_buffer.cc:258: ? value * ~std::numeric_limits<int32>::min() > I don't think this does what you want; e.g., ~(-5) == 4, instead try -value > without the ~ on the min. > > https://codereview.chromium.org/600143002/diff/60001/media/base/audio_buffer.... > media/base/audio_buffer.cc:307: dest_data[ch + (i * dest_data_frame_size)] = > A paired incrementing offset is more efficient than computing this > multiplication every time, see AudioBus::FromInterleavedInternal. Minor in any > case, so up to you. > > https://codereview.chromium.org/600143002/diff/60001/media/base/audio_buffer.... > media/base/audio_buffer.cc:320: dest_data[ch + (i * dest_data_frame_size)] = > Ditto. > > https://codereview.chromium.org/600143002/diff/60001/media/base/audio_buffer.... > media/base/audio_buffer.cc:325: NOTREACHED(); > If you prefer you could use case statement since it explictly has to list all > possible types. PTAL added unittest and did some more cleanup and took you advice making it more efficient
https://codereview.chromium.org/600143002/diff/120001/media/base/audio_buffer.cc File media/base/audio_buffer.cc (right): https://codereview.chromium.org/600143002/diff/120001/media/base/audio_buffer... media/base/audio_buffer.cc:258: DCHECK_LE(frames_to_copy, channel_count_ * adjusted_frame_count_); frames_to_copy must be per channel. https://codereview.chromium.org/600143002/diff/120001/media/base/audio_buffer... media/base/audio_buffer.cc:260: You'll need to update your frames to copy in the interleaved case to account for multiple channels. Instead of implementing four different methods you could use a single template function like so: template <class Target, typename Converter> void InterleaveToS32(const std::vector<uint8*>& channel_data, int frames_to_copy, int trim_start, int32* dest) { const int total_frames = frames_to_copy * channel_data.size(); for (int ch = 0; ch < channels; ++ch) { const Target* source_data = reinterpret_cast<const Target*>(channel_data_[ch]) + trim_start; for (int i = 0, offset = ch; i < total_frames; ++i, offset += channel_data.size()) { dest_data[offset] = Converter(source_data[i]); } } } You would call it Interleaved: InterleaveToS32<int16*, ConvertS16ToS32>(channel_data_, frames_to_copy * channels, trim_start_, dest_data); Planar: InterleaveToS32<float*, ConvertF32ToS32>(channel_data_, frames_to_copy, trim_start_, dest_data); https://codereview.chromium.org/600143002/diff/120001/media/base/audio_buffer... media/base/audio_buffer.cc:271: for (int i = 0; i < frames_to_copy; ++i) { Typically drop {} for one line for loops. Ditto for the ones below. https://codereview.chromium.org/600143002/diff/120001/media/base/audio_buffer... media/base/audio_buffer.cc:274: } break; break; should be inside {}. What does clang-format do with this?
PTAL https://codereview.chromium.org/600143002/diff/120001/media/base/audio_buffer.cc File media/base/audio_buffer.cc (right): https://codereview.chromium.org/600143002/diff/120001/media/base/audio_buffer... media/base/audio_buffer.cc:258: DCHECK_LE(frames_to_copy, channel_count_ * adjusted_frame_count_); On 2014/09/26 23:02:41, DaleCurtis wrote: > frames_to_copy must be per channel. Done. https://codereview.chromium.org/600143002/diff/120001/media/base/audio_buffer... media/base/audio_buffer.cc:260: On 2014/09/26 23:02:41, DaleCurtis wrote: > You'll need to update your frames to copy in the interleaved case to account for > multiple channels. Instead of implementing four different methods you could use > a single template function like so: > > template <class Target, typename Converter> > void InterleaveToS32(const std::vector<uint8*>& channel_data, > int frames_to_copy, > int trim_start, > int32* dest) { > const int total_frames = frames_to_copy * channel_data.size(); > for (int ch = 0; ch < channels; ++ch) { > const Target* source_data = > reinterpret_cast<const Target*>(channel_data_[ch]) + trim_start; > for (int i = 0, offset = ch; i < total_frames; > ++i, offset += channel_data.size()) { > dest_data[offset] = Converter(source_data[i]); > } > } > } > > You would call it > > Interleaved: > InterleaveToS32<int16*, ConvertS16ToS32>(channel_data_, frames_to_copy * > channels, trim_start_, dest_data); > > Planar: > InterleaveToS32<float*, ConvertF32ToS32>(channel_data_, frames_to_copy, > trim_start_, dest_data); Done. https://codereview.chromium.org/600143002/diff/120001/media/base/audio_buffer... media/base/audio_buffer.cc:271: for (int i = 0; i < frames_to_copy; ++i) { On 2014/09/26 23:02:41, DaleCurtis wrote: > Typically drop {} for one line for loops. Ditto for the ones below. Done. https://codereview.chromium.org/600143002/diff/120001/media/base/audio_buffer... media/base/audio_buffer.cc:274: } break; On 2014/09/26 23:02:41, DaleCurtis wrote: > break; should be inside {}. What does clang-format do with this? Done.
lgtm % nits. https://codereview.chromium.org/600143002/diff/160001/media/base/audio_buffer.cc File media/base/audio_buffer.cc (right): https://codereview.chromium.org/600143002/diff/160001/media/base/audio_buffer... media/base/audio_buffer.cc:266: ++i, offset += channel_data.size()) {} since the for() spans multiple lines. https://codereview.chromium.org/600143002/diff/160001/media/base/audio_buffer... File media/base/audio_buffer_unittest.cc (right): https://codereview.chromium.org/600143002/diff/160001/media/base/audio_buffer... media/base/audio_buffer_unittest.cc:502: const int frames = kSampleRate / 10; Use / 100 here, no need for 4800 frames.
PTAL, thanks for all your hand-holding on this one https://codereview.chromium.org/600143002/diff/160001/media/base/audio_buffer.cc File media/base/audio_buffer.cc (right): https://codereview.chromium.org/600143002/diff/160001/media/base/audio_buffer... media/base/audio_buffer.cc:266: ++i, offset += channel_data.size()) On 2014/09/29 23:01:32, DaleCurtis wrote: > {} since the for() spans multiple lines. Done. https://codereview.chromium.org/600143002/diff/160001/media/base/audio_buffer... File media/base/audio_buffer_unittest.cc (right): https://codereview.chromium.org/600143002/diff/160001/media/base/audio_buffer... media/base/audio_buffer_unittest.cc:502: const int frames = kSampleRate / 10; On 2014/09/29 23:01:33, DaleCurtis wrote: > Use / 100 here, no need for 4800 frames. Done.
The CQ bit was checked by pwestin@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/600143002/180001
Message was sent while issue was closed.
Committed patchset #10 (id:180001) as b7f61804740aae8c4d401752d6ba5b702a7509ec
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/34e0ac22e25cfac606296646ecb765fa4bd3b54e Cr-Commit-Position: refs/heads/master@{#297340} |
