Support 8 and 32 bit formats for Mac Channel Swizzler.

Support 8 and 32 bit formats for Mac Channel Swizzler.


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

[fbarchard, 2009-10-08 21:35:57]

8 and 32 bit sound for completeness.  (not useful)

[awong, 2009-10-08 22:07:53]

2 comments.

http://codereview.chromium.org/261028/diff/1/2
File media/audio/mac/audio_output_mac.cc (right):

http://codereview.chromium.org/261028/diff/1/2#newcode175
Line 175: for (size_t i = 0; i < filled; i += sizeof(aac), b += 6) {
Maybe do also 
  b += arraysize(aac)
?

I think it's arraysize. Might be ARRAYSIZE.

http://codereview.chromium.org/261028/diff/1/2#newcode228
Line 228: }
Do we need to explicitly fail on an unepxected format?  Or will the bottom code
handle it safely (if not gracefully)?

[fbarchard, 2009-10-08 23:25:33]

constant for number of channels

http://codereview.chromium.org/261028/diff/1/2
File media/audio/mac/audio_output_mac.cc (right):

http://codereview.chromium.org/261028/diff/1/2#newcode175
Line 175: for (size_t i = 0; i < filled; i += sizeof(aac), b += 6) {
I dont know about that, but lets try a constant.

http://codereview.chromium.org/261028/diff/1/2#newcode228
Line 228: }
Swizzling is not all that important, so I think its better to just pass it thru.
For depth, we only support 8,16 or 32, but really we only support 16.  The most
likely way that this will change is if we add uncompressed support (PCM).
We dont really support anything but 1,2 or 6 channels.  3,4,5 channels are
uncommon.  7 and 8 are somewhat common for stereos, but not for file formats. 
5.1 comes from DVD/movies/trailers.  But its likely we can support 7 and 8
channels in the future by just supporting the first 6 channels and passing the
others thru as is.  So this code will only change a little when we do that.
We can hopefully remove this function, or make it common, when we update ffmpeg
and take the new aac and ogg modules that have the proper layout.

[awong, 2009-10-08 23:45:16]

LGTM

[fbarchard, 2009-10-09 00:12:53]

missing { fixed in CL28487.

[Mark Mentovai, 2009-10-09 00:54:14]

http://codereview.chromium.org/261028/diff/3004/3005
File media/audio/mac/audio_output_mac.cc (right):

http://codereview.chromium.org/261028/diff/3004/3005#newcode172
Line 172: const int kNumSurroundChannels = 6;
This needs to be static or in an anonymous namespace.

[awong, 2009-10-09 00:56:58]

http://codereview.chromium.org/261028/diff/3004/3005
File media/audio/mac/audio_output_mac.cc (right):

http://codereview.chromium.org/261028/diff/3004/3005#newcode172
Line 172: const int kNumSurroundChannels = 6;
On 2009/10/09 00:54:14, Mark Mentovai wrote:
> This needs to be static or in an anonymous namespace.

...I don't think it does.  In C++, (as opposed to C), const on the global scope
implies internal linkage.

http://stackoverflow.com/questions/998425/why-does-const-imply-internal-linka...
http://bytes.com/topic/c/answers/62013-const-has-internal-linkage-c

[Mark Mentovai, 2009-10-09 00:59:21]

Ah, well color me wrong then!

Mark

ajwong wrote:
> http://codereview.chromium.org/261028/diff/3004/3005
> File media/audio/mac/audio_output_mac.cc (right):
>
> http://codereview.chromium.org/261028/diff/3004/3005#newcode172
> Line 172: const int kNumSurroundChannels =3D 6;
> On 2009/10/09 00:54:14, Mark Mentovai wrote:
>>
>> This needs to be static or in an anonymous namespace.
>
> ...I don't think it does. =A0In C++, (as opposed to C), const on the
> global scope implies internal linkage.
>
> http://stackoverflow.com/questions/998425/why-does-const-imply-internal-l=
inkage-in-c-when-it-doesnt-in-c
> http://bytes.com/topic/c/answers/62013-const-has-internal-linkage-c
>
> http://codereview.chromium.org/261028

[fbarchard, 2009-10-09 01:14:26]

an extremely private version of swizzler.

[awong, 2009-10-09 01:25:50]

One style nit

-You friendly readability reviewer.

http://codereview.chromium.org/261028/diff/8001/8002
File media/audio/mac/audio_output_mac.cc (right):

http://codereview.chromium.org/261028/diff/8001/8002#newcode187
Line 187: }
Hah...that's getting pretty overkill.  But if you add an anonymous namespace,
end it with a namespace comment.  2 spaces, then // namespace like follows:

}  // namespace

[awong, 2009-10-09 01:27:13]

LGTM btw.