Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1769373003:
AudioBus: Add a ToInterleavedFloat() method to AudioBus (Closed)
|
Created:
4 years, 9 months ago by eklavyamirani Modified:
4 years, 4 months ago Reviewers:
*mcasas CC:
chromium-reviews, mlamouri+watch-content_chromium.org, imcheng+watch_chromium.org, posciak+watch_chromium.org, avayvod+watch_chromium.org, jam, mcasas+watch_chromium.org, feature-media-reviews_chromium.org, darin-cc_chromium.org, jasonroberts+watch_google.com, mkwst+moarreviews-renderer_chromium.org, xjz+watch_chromium.org, isheriff+watch_chromium.org, miu+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAudioBus: Add a ToInterleavedFloat() method to AudioBus
Bug = 580391
R = mcasas@chromium.org
Patch Set 1 #
Total comments: 12
Patch Set 2 #
Total comments: 5
Messages
Total messages: 16 (5 generated)
Looking good, some comments. https://codereview.chromium.org/1769373003/diff/1/content/renderer/media/audi... File content/renderer/media/audio_track_recorder.cc (right): https://codereview.chromium.org/1769373003/diff/1/content/renderer/media/audi... content/renderer/media/audio_track_recorder.cc:81: audio_bus->ToInterleavedFloat(0, 0, audio_bus->frames(), Please inline this call and remove ToInterleave() altogether. https://codereview.chromium.org/1769373003/diff/1/media/base/audio_bus.cc File media/base/audio_bus.cc (right): https://codereview.chromium.org/1769373003/diff/1/media/base/audio_bus.cc#new... media/base/audio_bus.cc:304: float* buffer) const { DCHECK_EQ(num_channels, channels()); ? Also check the arguments with a CheckOverflow() like in l.317. https://codereview.chromium.org/1769373003/diff/1/media/base/audio_bus.cc#new... media/base/audio_bus.cc:305: for (int ch = 0; ch < this->channels(); ++ch) { No |this|. https://codereview.chromium.org/1769373003/diff/1/media/base/audio_bus_unitte... File media/base/audio_bus_unittest.cc (right): https://codereview.chromium.org/1769373003/diff/1/media/base/audio_bus_unitte... media/base/audio_bus_unittest.cc:269: -0.5, 0, 1.0, 0, 0}; This needs formatting, use git cl format before uploading. https://codereview.chromium.org/1769373003/diff/1/media/base/audio_bus_unitte... media/base/audio_bus_unittest.cc:381: TEST_F(AudioBusTest, ToInterleavedFloat) { Add a comment like in l.341 https://codereview.chromium.org/1769373003/diff/1/media/base/audio_bus_unitte... media/base/audio_bus_unittest.cc:389: float test_array[arraysize(kTestVectorFloat)]; You might want to add a SCOPED_TRACE("float"); for this scope.
The CQ bit was checked by mcasas@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1769373003/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1769373003/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: The author eklavyamirani@gmail.com has not signed Google Contributor License Agreement. Please visit https://cla.developers.google.com to sign and manage CLA.
eklavyamirani@gmail.com changed required reviewers: + mcasas@chromium.org
Will publish the updated CL shortly. https://codereview.chromium.org/1769373003/diff/1/content/renderer/media/audi... File content/renderer/media/audio_track_recorder.cc (right): https://codereview.chromium.org/1769373003/diff/1/content/renderer/media/audi... content/renderer/media/audio_track_recorder.cc:81: audio_bus->ToInterleavedFloat(0, 0, audio_bus->frames(), On 2016/03/09 00:24:18, mcasas wrote: > Please inline this call and remove ToInterleave() > altogether. Done. https://codereview.chromium.org/1769373003/diff/1/media/base/audio_bus.cc File media/base/audio_bus.cc (right): https://codereview.chromium.org/1769373003/diff/1/media/base/audio_bus.cc#new... media/base/audio_bus.cc:305: for (int ch = 0; ch < this->channels(); ++ch) { On 2016/03/09 00:24:18, mcasas wrote: > No |this|. Done. https://codereview.chromium.org/1769373003/diff/1/media/base/audio_bus_unitte... File media/base/audio_bus_unittest.cc (right): https://codereview.chromium.org/1769373003/diff/1/media/base/audio_bus_unitte... media/base/audio_bus_unittest.cc:269: -0.5, 0, 1.0, 0, 0}; On 2016/03/09 00:24:18, mcasas wrote: > This needs formatting, use > git cl format > before uploading. Done. https://codereview.chromium.org/1769373003/diff/1/media/base/audio_bus_unitte... media/base/audio_bus_unittest.cc:381: TEST_F(AudioBusTest, ToInterleavedFloat) { On 2016/03/09 00:24:18, mcasas wrote: > Add a comment like in l.341 Done. https://codereview.chromium.org/1769373003/diff/1/media/base/audio_bus_unitte... media/base/audio_bus_unittest.cc:389: float test_array[arraysize(kTestVectorFloat)]; On 2016/03/09 00:24:18, mcasas wrote: > You might want to add a > SCOPED_TRACE("float"); > for this scope. Done. Is this to output information when the tests run?
The CQ bit was checked by eklavyamirani@gmail.com
The CQ bit was unchecked by eklavyamirani@gmail.com
On 2016/03/15 03:48:13, eklavyamirani wrote: > Will publish the updated CL shortly. > > https://codereview.chromium.org/1769373003/diff/1/content/renderer/media/audi... > File content/renderer/media/audio_track_recorder.cc (right): > > https://codereview.chromium.org/1769373003/diff/1/content/renderer/media/audi... > content/renderer/media/audio_track_recorder.cc:81: > audio_bus->ToInterleavedFloat(0, 0, audio_bus->frames(), > On 2016/03/09 00:24:18, mcasas wrote: > > Please inline this call and remove ToInterleave() > > altogether. > > Done. > > https://codereview.chromium.org/1769373003/diff/1/media/base/audio_bus.cc > File media/base/audio_bus.cc (right): > > https://codereview.chromium.org/1769373003/diff/1/media/base/audio_bus.cc#new... > media/base/audio_bus.cc:305: for (int ch = 0; ch < this->channels(); ++ch) { > On 2016/03/09 00:24:18, mcasas wrote: > > No |this|. > > Done. > > https://codereview.chromium.org/1769373003/diff/1/media/base/audio_bus_unitte... > File media/base/audio_bus_unittest.cc (right): > > https://codereview.chromium.org/1769373003/diff/1/media/base/audio_bus_unitte... > media/base/audio_bus_unittest.cc:269: -0.5, 0, 1.0, 0, 0}; > On 2016/03/09 00:24:18, mcasas wrote: > > This needs formatting, use > > git cl format > > before uploading. > > Done. > > https://codereview.chromium.org/1769373003/diff/1/media/base/audio_bus_unitte... > media/base/audio_bus_unittest.cc:381: TEST_F(AudioBusTest, ToInterleavedFloat) { > On 2016/03/09 00:24:18, mcasas wrote: > > Add a comment like in l.341 > > Done. > > https://codereview.chromium.org/1769373003/diff/1/media/base/audio_bus_unitte... > media/base/audio_bus_unittest.cc:389: float > test_array[arraysize(kTestVectorFloat)]; > On 2016/03/09 00:24:18, mcasas wrote: > > You might want to add a > > SCOPED_TRACE("float"); > > for this scope. > > Done. > > Is this to output information when the tests run? Any updates here?
https://codereview.chromium.org/1769373003/diff/20001/media/base/audio_bus_un... File media/base/audio_bus_unittest.cc (right): https://codereview.chromium.org/1769373003/diff/20001/media/base/audio_bus_un... media/base/audio_bus_unittest.cc:394: ASSERT_EQ(memcmp(test_array, kTestVectorFloat, Fixed the formatting on this in my local commit. Amend the commit posted here, or it goes as a separate patch?
On 2016/04/04 08:11:58, eklavyamirani wrote: > https://codereview.chromium.org/1769373003/diff/20001/media/base/audio_bus_un... > File media/base/audio_bus_unittest.cc (right): > > https://codereview.chromium.org/1769373003/diff/20001/media/base/audio_bus_un... > media/base/audio_bus_unittest.cc:394: ASSERT_EQ(memcmp(test_array, > kTestVectorFloat, > Fixed the formatting on this in my local commit. Amend the commit posted here, > or it goes as a separate patch? Upload as a new patch set plz (don't remove PSs w/ comment, essentially)
Apologies for the delay, didn't see it! https://codereview.chromium.org/1769373003/diff/1/media/base/audio_bus_unitte... File media/base/audio_bus_unittest.cc (right): https://codereview.chromium.org/1769373003/diff/1/media/base/audio_bus_unitte... media/base/audio_bus_unittest.cc:389: float test_array[arraysize(kTestVectorFloat)]; On 2016/03/15 03:48:13, eklavyamirani wrote: > On 2016/03/09 00:24:18, mcasas wrote: > > You might want to add a > > SCOPED_TRACE("float"); > > for this scope. > > Done. > > Is this to output information when the tests run? Yes. https://codereview.chromium.org/1769373003/diff/20001/media/base/audio_bus.cc File media/base/audio_bus.cc (right): https://codereview.chromium.org/1769373003/diff/20001/media/base/audio_bus.cc... media/base/audio_bus.cc:304: int num_samples, This parameter is called |int num_channels| in the method declaration. Only one is correct, and beyond that, if you allow the caller to specify |num_samples|, then this method should be suffixed Partial like in l.317. Also try to follow the other ToInterleaved*() variable naming convention, e.g. |dest|, |start_frame| etc. https://codereview.chromium.org/1769373003/diff/20001/media/base/audio_bus_un... File media/base/audio_bus_unittest.cc (right): https://codereview.chromium.org/1769373003/diff/20001/media/base/audio_bus_un... media/base/audio_bus_unittest.cc:382: // into a single linear output buffer correctly s/ToInterleavedFloat/ToInterleavedFloat()/ Classes like AudioBus do not need to be specified |as_variable|. Period at the end. https://codereview.chromium.org/1769373003/diff/20001/media/base/audio_bus_un... media/base/audio_bus_unittest.cc:394: ASSERT_EQ(memcmp(test_array, kTestVectorFloat, On 2016/04/04 08:11:58, eklavyamirani wrote: > Fixed the formatting on this in my local commit. Amend the commit posted here, > or it goes as a separate patch? Sorry, yes, when you have addressed the reviewers' comments, upload a new patch, then "Publish and mail comments" please. https://codereview.chromium.org/1769373003/diff/20001/media/cast/sender/audio... File media/cast/sender/audio_encoder.cc (right): https://codereview.chromium.org/1769373003/diff/20001/media/cast/sender/audio... media/cast/sender/audio_encoder.cc:286: DCHECK_EQ(audio_bus->channels(), num_channels_); nit: Expected value goes first, and I think in this case that's |num_channels_|.
On 2016/04/11 23:47:08, mcasas wrote: > Apologies for the delay, didn't see it! > > https://codereview.chromium.org/1769373003/diff/1/media/base/audio_bus_unitte... > File media/base/audio_bus_unittest.cc (right): > > https://codereview.chromium.org/1769373003/diff/1/media/base/audio_bus_unitte... > media/base/audio_bus_unittest.cc:389: float > test_array[arraysize(kTestVectorFloat)]; > On 2016/03/15 03:48:13, eklavyamirani wrote: > > On 2016/03/09 00:24:18, mcasas wrote: > > > You might want to add a > > > SCOPED_TRACE("float"); > > > for this scope. > > > > Done. > > > > Is this to output information when the tests run? > > Yes. > > https://codereview.chromium.org/1769373003/diff/20001/media/base/audio_bus.cc > File media/base/audio_bus.cc (right): > > https://codereview.chromium.org/1769373003/diff/20001/media/base/audio_bus.cc... > media/base/audio_bus.cc:304: int num_samples, > This parameter is called |int num_channels| in the > method declaration. Only one is correct, and beyond > that, if you allow the caller to specify |num_samples|, > then this method should be suffixed Partial like > in l.317. > > Also try to follow the other ToInterleaved*() variable > naming convention, e.g. |dest|, |start_frame| etc. > > https://codereview.chromium.org/1769373003/diff/20001/media/base/audio_bus_un... > File media/base/audio_bus_unittest.cc (right): > > https://codereview.chromium.org/1769373003/diff/20001/media/base/audio_bus_un... > media/base/audio_bus_unittest.cc:382: // into a single linear output buffer > correctly > s/ToInterleavedFloat/ToInterleavedFloat()/ > > Classes like AudioBus do not need to be > specified |as_variable|. > > Period at the end. > > https://codereview.chromium.org/1769373003/diff/20001/media/base/audio_bus_un... > media/base/audio_bus_unittest.cc:394: ASSERT_EQ(memcmp(test_array, > kTestVectorFloat, > On 2016/04/04 08:11:58, eklavyamirani wrote: > > Fixed the formatting on this in my local commit. Amend the commit posted here, > > or it goes as a separate patch? > > Sorry, yes, when you have addressed the > reviewers' comments, upload a new patch, > then "Publish and mail comments" please. > > https://codereview.chromium.org/1769373003/diff/20001/media/cast/sender/audio... > File media/cast/sender/audio_encoder.cc (right): > > https://codereview.chromium.org/1769373003/diff/20001/media/cast/sender/audio... > media/cast/sender/audio_encoder.cc:286: DCHECK_EQ(audio_bus->channels(), > num_channels_); > nit: Expected value goes first, and I > think in this case that's |num_channels_|. eklavyamirani@ are you planning to continue with this CL? You were almost there! :)
On 2016/05/12 16:42:58, mcasas wrote: > On 2016/04/11 23:47:08, mcasas wrote: > > Apologies for the delay, didn't see it! > > > > > https://codereview.chromium.org/1769373003/diff/1/media/base/audio_bus_unitte... > > File media/base/audio_bus_unittest.cc (right): > > > > > https://codereview.chromium.org/1769373003/diff/1/media/base/audio_bus_unitte... > > media/base/audio_bus_unittest.cc:389: float > > test_array[arraysize(kTestVectorFloat)]; > > On 2016/03/15 03:48:13, eklavyamirani wrote: > > > On 2016/03/09 00:24:18, mcasas wrote: > > > > You might want to add a > > > > SCOPED_TRACE("float"); > > > > for this scope. > > > > > > Done. > > > > > > Is this to output information when the tests run? > > > > Yes. > > > > https://codereview.chromium.org/1769373003/diff/20001/media/base/audio_bus.cc > > File media/base/audio_bus.cc (right): > > > > > https://codereview.chromium.org/1769373003/diff/20001/media/base/audio_bus.cc... > > media/base/audio_bus.cc:304: int num_samples, > > This parameter is called |int num_channels| in the > > method declaration. Only one is correct, and beyond > > that, if you allow the caller to specify |num_samples|, > > then this method should be suffixed Partial like > > in l.317. > > > > Also try to follow the other ToInterleaved*() variable > > naming convention, e.g. |dest|, |start_frame| etc. > > > > > https://codereview.chromium.org/1769373003/diff/20001/media/base/audio_bus_un... > > File media/base/audio_bus_unittest.cc (right): > > > > > https://codereview.chromium.org/1769373003/diff/20001/media/base/audio_bus_un... > > media/base/audio_bus_unittest.cc:382: // into a single linear output buffer > > correctly > > s/ToInterleavedFloat/ToInterleavedFloat()/ > > > > Classes like AudioBus do not need to be > > specified |as_variable|. > > > > Period at the end. > > > > > https://codereview.chromium.org/1769373003/diff/20001/media/base/audio_bus_un... > > media/base/audio_bus_unittest.cc:394: ASSERT_EQ(memcmp(test_array, > > kTestVectorFloat, > > On 2016/04/04 08:11:58, eklavyamirani wrote: > > > Fixed the formatting on this in my local commit. Amend the commit posted > here, > > > or it goes as a separate patch? > > > > Sorry, yes, when you have addressed the > > reviewers' comments, upload a new patch, > > then "Publish and mail comments" please. > > > > > https://codereview.chromium.org/1769373003/diff/20001/media/cast/sender/audio... > > File media/cast/sender/audio_encoder.cc (right): > > > > > https://codereview.chromium.org/1769373003/diff/20001/media/cast/sender/audio... > > media/cast/sender/audio_encoder.cc:286: DCHECK_EQ(audio_bus->channels(), > > num_channels_); > > nit: Expected value goes first, and I > > think in this case that's |num_channels_|. > > eklavyamirani@ are you planning to continue with this CL? > You were almost there! :) https://codereview.chromium.org/2024993004/ addressed the issue, so I'm going to close this CL. eklavyamirani@: thanks! |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
