Minor changes to ARAB and ARAD necessary for OLA

Minor changes to ARAB and ARAD necessary for OLA

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=19687

[kylep, 2009-06-30 23:18:24]

Some cleanup; mostly changing to all virtual methods, forward declarations,
ignoring sample rate and converting sample bits to sample bytes.

[scherkus (not reviewing), 2009-06-30 23:22:52]

few nits

http://codereview.chromium.org/150140/diff/1/4
File media/filters/audio_renderer_algorithm_base.cc (right):

http://codereview.chromium.org/150140/diff/1/4#newcode34
Line 34: DCHECK(callback);
I would add the following:
DCHECK_EQ(sample_bits % 8, 0) << "some message explaining why we only support 8,
16, 24, 32, ...";

http://codereview.chromium.org/150140/diff/1/5
File media/filters/audio_renderer_algorithm_base.h (right):

http://codereview.chromium.org/150140/diff/1/5#newcode85
Line 85: virtual int sample_bytes();
I would expand on the comments here since sample_bytes() isn't totally
self-explanatory

even something simple such as "Number of bytes per sample per channel"

http://codereview.chromium.org/150140/diff/1/2
File media/filters/audio_renderer_algorithm_default.cc (right):

http://codereview.chromium.org/150140/diff/1/2#newcode7
Line 7: #include "media/base/data_buffer.h"
alphabetize includes

[kylep, 2009-06-30 23:36:58]

http://codereview.chromium.org/150140/diff/1/4
File media/filters/audio_renderer_algorithm_base.cc (right):

http://codereview.chromium.org/150140/diff/1/4#newcode34
Line 34: DCHECK(callback);
On 2009/06/30 23:22:52, scherkus wrote:
> I would add the following:
> DCHECK_EQ(sample_bits % 8, 0) << "some message explaining why we only support
8,
> 16, 24, 32, ...";

Done.

http://codereview.chromium.org/150140/diff/1/5
File media/filters/audio_renderer_algorithm_base.h (right):

http://codereview.chromium.org/150140/diff/1/5#newcode85
Line 85: virtual int sample_bytes();
On 2009/06/30 23:22:52, scherkus wrote:
> I would expand on the comments here since sample_bytes() isn't totally
> self-explanatory
> 
> even something simple such as "Number of bytes per sample per channel"

Done.

http://codereview.chromium.org/150140/diff/1/2
File media/filters/audio_renderer_algorithm_default.cc (right):

http://codereview.chromium.org/150140/diff/1/2#newcode7
Line 7: #include "media/base/data_buffer.h"
On 2009/06/30 23:22:52, scherkus wrote:
> alphabetize includes

Done.

[fbarchard, 2009-06-30 23:42:17]

lgtm. just questions and info

http://codereview.chromium.org/150140/diff/1/5
File media/filters/audio_renderer_algorithm_base.h (right):

http://codereview.chromium.org/150140/diff/1/5#newcode39
Line 39: class Buffer;
why are you forward declaring these?

http://codereview.chromium.org/150140/diff/13/16
File media/filters/audio_renderer_algorithm_base.cc (right):

http://codereview.chromium.org/150140/diff/13/16#newcode39
Line 39: sample_bytes_ = sample_bits / 8;
More of a nit/FYI...
As these are signed integers, this can generate more than you'd hope.  A sign
check, and branch for handling of negatives. Same with % 8.  Or sometimes the
compile will just do the divide instruction for new CPUs, because its optimized
for divide by power of 2... 1 cycle per log2(value).
If/when you care about code size/performance, then sample_bits & 7 and
samples_bits >> 3 will produce a single instruction.
Or you can use unsigned int, or for visual C, the assume() directive.

[scherkus (not reviewing), 2009-06-30 23:44:46]

On 2009/06/30 23:42:17, fbarchard wrote:
> lgtm. just questions and info
> 
> http://codereview.chromium.org/150140/diff/1/5
> File media/filters/audio_renderer_algorithm_base.h (right):
> 
> http://codereview.chromium.org/150140/diff/1/5#newcode39
> Line 39: class Buffer;
> why are you forward declaring these?
> 
> http://codereview.chromium.org/150140/diff/13/16
> File media/filters/audio_renderer_algorithm_base.cc (right):
> 
> http://codereview.chromium.org/150140/diff/13/16#newcode39
> Line 39: sample_bytes_ = sample_bits / 8;
> More of a nit/FYI...
> As these are signed integers, this can generate more than you'd hope.  A sign
> check, and branch for handling of negatives. Same with % 8.  Or sometimes the
> compile will just do the divide instruction for new CPUs, because its
optimized
> for divide by power of 2... 1 cycle per log2(value).
> If/when you care about code size/performance, then sample_bits & 7 and
> samples_bits >> 3 will produce a single instruction.
> Or you can use unsigned int, or for visual C, the assume() directive.

I wouldn't worry about it... dividing by 8 is more readable and this code is
executed once.

[scherkus (not reviewing), 2009-06-30 23:46:52]

http://codereview.chromium.org/150140/diff/1/5
File media/filters/audio_renderer_algorithm_base.h (right):

http://codereview.chromium.org/150140/diff/1/5#newcode39
Line 39: class Buffer;
On 2009/06/30 23:42:17, fbarchard wrote:
> why are you forward declaring these?

helps reduce header file chaining

Buffer is used below, although I suppose DataBuffer isn't and could be removed

[kylep, 2009-06-30 23:49:10]

On 2009/06/30 23:44:46, scherkus wrote:
> On 2009/06/30 23:42:17, fbarchard wrote:
> > lgtm. just questions and info
> > 
> > http://codereview.chromium.org/150140/diff/1/5
> > File media/filters/audio_renderer_algorithm_base.h (right):
> > 
> > http://codereview.chromium.org/150140/diff/1/5#newcode39
> > Line 39: class Buffer;
> > why are you forward declaring these?

Google's C++ style guide says to avoid including files in the .h file so include
ordering doesn't become a problem. So I moved the includes to the .cc file and
added the forward declarations.
> > 
> > http://codereview.chromium.org/150140/diff/13/16
> > File media/filters/audio_renderer_algorithm_base.cc (right):
> > 
> > http://codereview.chromium.org/150140/diff/13/16#newcode39
> > Line 39: sample_bytes_ = sample_bits / 8;
> > More of a nit/FYI...
> > As these are signed integers, this can generate more than you'd hope.  A
sign
> > check, and branch for handling of negatives. Same with % 8.  Or sometimes
the
> > compile will just do the divide instruction for new CPUs, because its
> optimized
> > for divide by power of 2... 1 cycle per log2(value).
> > If/when you care about code size/performance, then sample_bits & 7 and
> > samples_bits >> 3 will produce a single instruction.
> > Or you can use unsigned int, or for visual C, the assume() directive.
> 
> I wouldn't worry about it... dividing by 8 is more readable and this code is
> executed once.

Sooo... leave it? Also, there is a DCHECK for positive sample bits (we shouldn't
support negative sample bits, should we? ~^)

[kylep, 2009-06-30 23:51:01]

On 2009/06/30 23:46:52, scherkus wrote:
> http://codereview.chromium.org/150140/diff/1/5
> File media/filters/audio_renderer_algorithm_base.h (right):
> 
> http://codereview.chromium.org/150140/diff/1/5#newcode39
> Line 39: class Buffer;
> On 2009/06/30 23:42:17, fbarchard wrote:
> > why are you forward declaring these?
> 
> helps reduce header file chaining
> 
> Buffer is used below, although I suppose DataBuffer isn't and could be removed

DataBuffer is used in a function declaration. Does that not count?

[scherkus (not reviewing), 2009-07-01 00:03:21]

On 2009/06/30 23:51:01, kylep wrote:
> On 2009/06/30 23:46:52, scherkus wrote:
> > http://codereview.chromium.org/150140/diff/1/5
> > File media/filters/audio_renderer_algorithm_base.h (right):
> > 
> > http://codereview.chromium.org/150140/diff/1/5#newcode39
> > Line 39: class Buffer;
> > On 2009/06/30 23:42:17, fbarchard wrote:
> > > why are you forward declaring these?
> > 
> > helps reduce header file chaining
> > 
> > Buffer is used below, although I suppose DataBuffer isn't and could be
removed
> 
> DataBuffer is used in a function declaration. Does that not count?

whoops... ctrl+f not so easy...

LGTM!!!