Fix AudioBuffer verification tests.

Fix AudioBuffer verification tests.

The previous tests were not properly accounting for the removed
segments in the AudioBus.  The offset specified was not taken
into consideration when determing the iteration end point.

BUG=none
TEST=manual inspection of tests.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=268401

[DaleCurtis, 2014-05-05 18:44:49]

You were right, the tests were not correct.  Good catch!

[wolenetz, 2014-05-05 20:28:45]

Looking pretty good.

https://codereview.chromium.org/261333002/diff/1/media/base/audio_buffer_unit...
File media/base/audio_buffer_unittest.cc (right):

https://codereview.chromium.org/261333002/diff/1/media/base/audio_buffer_unit...
media/base/audio_buffer_unittest.cc:18: float start_offset,
Is there really a difference between start and start_offset? (Why do we need to
split out start_offset from start?)

https://codereview.chromium.org/261333002/diff/1/media/base/audio_buffer_unit...
media/base/audio_buffer_unittest.cc:142: EXPECT_EQ(timestamp + trim_duration,
buffer->timestamp());
nit: versus TrimStart, TrimRange(0,...) does not update timestamp because it is
only used in cases where the timestamp shouldn't need updating? This seems a
little too tightly bound to the current caller
(AudioDiscardHelper::ProcessBuffers()), which (upon success) replaces the
buffer's timestamp with it's own calculation, after any TrimRange() may or may
not have updated it.
Suggestion: Make TrimRange() more general purpose/intuitive by having it update
the buffer timestamp in TrimStart cases (possibly by just detecting such cases
and deferring to TrimStart instead of existing internal TrimRange() logic?)

[DaleCurtis, 2014-05-05 21:28:49]

https://codereview.chromium.org/261333002/diff/1/media/base/audio_buffer_unit...
File media/base/audio_buffer_unittest.cc (right):

https://codereview.chromium.org/261333002/diff/1/media/base/audio_buffer_unit...
media/base/audio_buffer_unittest.cc:18: float start_offset,
On 2014/05/05 20:28:45, wolenetz wrote:
> Is there really a difference between start and start_offset? (Why do we need
to
> split out start_offset from start?)

Yes.  |start| is used to calculate a new |v| at the first frame of each channel.
 However if it's computed using |start_offset| it'll be completely wrong since
|start_offset| wasn't what was used to construct the buffer.

https://codereview.chromium.org/261333002/diff/1/media/base/audio_buffer_unit...
media/base/audio_buffer_unittest.cc:142: EXPECT_EQ(timestamp + trim_duration,
buffer->timestamp());
On 2014/05/05 20:28:45, wolenetz wrote:
> nit: versus TrimStart, TrimRange(0,...) does not update timestamp because it
is
> only used in cases where the timestamp shouldn't need updating? This seems a
> little too tightly bound to the current caller
> (AudioDiscardHelper::ProcessBuffers()), which (upon success) replaces the
> buffer's timestamp with it's own calculation, after any TrimRange() may or may
> not have updated it.
> Suggestion: Make TrimRange() more general purpose/intuitive by having it
update
> the buffer timestamp in TrimStart cases (possibly by just detecting such cases
> and deferring to TrimStart instead of existing internal TrimRange() logic?)

No, callers can call TrimStart() if they want the timestamp updated. This
provides a way to Trim from the beginning without updating the timestamp.

[wolenetz, 2014-05-05 22:16:29]

lgtm % nit:

https://codereview.chromium.org/261333002/diff/1/media/base/audio_buffer_unit...
File media/base/audio_buffer_unittest.cc (right):

https://codereview.chromium.org/261333002/diff/1/media/base/audio_buffer_unit...
media/base/audio_buffer_unittest.cc:18: float start_offset,
On 2014/05/05 21:28:50, DaleCurtis wrote:
> On 2014/05/05 20:28:45, wolenetz wrote:
> > Is there really a difference between start and start_offset? (Why do we need
> to
> > split out start_offset from start?)
> 
> Yes.  |start| is used to calculate a new |v| at the first frame of each
channel.
>  However if it's computed using |start_offset| it'll be completely wrong since
> |start_offset| wasn't what was used to construct the buffer.

nit: Hmm. Then maybe lift the constant 'start_offset + ' out of the inner loop
into the calculation of v in the outer loop?

[DaleCurtis, 2014-05-05 22:54:44]

https://codereview.chromium.org/261333002/diff/1/media/base/audio_buffer_unit...
File media/base/audio_buffer_unittest.cc (right):

https://codereview.chromium.org/261333002/diff/1/media/base/audio_buffer_unit...
media/base/audio_buffer_unittest.cc:18: float start_offset,
On 2014/05/05 22:16:29, wolenetz wrote:
> On 2014/05/05 21:28:50, DaleCurtis wrote:
> > On 2014/05/05 20:28:45, wolenetz wrote:
> > > Is there really a difference between start and start_offset? (Why do we
need
> > to
> > > split out start_offset from start?)
> > 
> > Yes.  |start| is used to calculate a new |v| at the first frame of each
> channel.
> >  However if it's computed using |start_offset| it'll be completely wrong
since
> > |start_offset| wasn't what was used to construct the buffer.
> 
> nit: Hmm. Then maybe lift the constant 'start_offset + ' out of the inner loop
> into the calculation of v in the outer loop?

Done.

[DaleCurtis, 2014-05-05 22:55:08]

The CQ bit was checked by dalecurtis@chromium.org

[commit-bot: I haz the power, 2014-05-05 22:55:28]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/dalecurtis@chromium.org/261333002/20001

[commit-bot: I haz the power, 2014-05-06 03:42:10]

Change committed as 268401