Add software audio mixing to the audio utils.

Add software audio mixing to the audio utils.

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

[enal1, 2012-03-08 18:50:37]

Hi Victoria,

Can you please review first mixer-related change?

[vrk (LEFT CHROMIUM), 2012-03-12 18:40:24]

Mostly looks fine; I had some questions, though.

https://chromiumcodereview.appspot.com/9641026/diff/1/media/audio/audio_util.cc
File media/audio/audio_util.cc (right):

https://chromiumcodereview.appspot.com/9641026/diff/1/media/audio/audio_util....
media/audio/audio_util.cc:244: // TODO(enal): use specialization and intrinsics.
What sort of specialization? Are intrinsics really necessary?

https://chromiumcodereview.appspot.com/9641026/diff/1/media/audio/audio_util....
media/audio/audio_util.cc:256: const int fixed_volume = static_cast<int>(volume
* 65536);
why 65536/maxval of uint16?

https://chromiumcodereview.appspot.com/9641026/diff/1/media/audio/audio_util....
media/audio/audio_util.cc:276: MixStreams<uint8, int32, -128, 127,
128>(static_cast<uint8*>(dst),
Why is the bias 128 here and 0 elsewhere?

https://chromiumcodereview.appspot.com/9641026/diff/1/media/audio/audio_util.h
File media/audio/audio_util.h (right):

https://chromiumcodereview.appspot.com/9641026/diff/1/media/audio/audio_util....
media/audio/audio_util.h:44: // Dst += Src * volume.
This function assumes |dst| and |src| have the same sample rate, right? Add
comment?

https://chromiumcodereview.appspot.com/9641026/diff/1/media/audio/audio_util_...
File media/audio/audio_util_unittest.cc (right):

https://chromiumcodereview.appspot.com/9641026/diff/1/media/audio/audio_util_...
media/audio/audio_util_unittest.cc:90: media::MixStreams(dst_u8, src_u8,
sizeof(dst_u8), sizeof(src_u8[0]), 0.5f);
It looks like you are adjusting the volume in most of these, except the tests
with "_one" appended to the name. Seems like it'd make more sense to test volume
and stream format separately.

Maybe change the u8, s16, and s32 tests to be at full volume (1.0), then have a
u8_HalfVolume, s16_QuarterVolume, s32_ThreeQuarters volume or something like
that?

[enal1, 2012-03-12 21:20:52]

Addressed everything.

https://chromiumcodereview.appspot.com/9641026/diff/1/media/audio/audio_util.cc
File media/audio/audio_util.cc (right):

https://chromiumcodereview.appspot.com/9641026/diff/1/media/audio/audio_util....
media/audio/audio_util.cc:244: // TODO(enal): use specialization and intrinsics.
On 2012/03/12 18:40:24, Victoria Kirst wrote:
> What sort of specialization? Are intrinsics really necessary?

Updated comment.

https://chromiumcodereview.appspot.com/9641026/diff/1/media/audio/audio_util....
media/audio/audio_util.cc:256: const int fixed_volume = static_cast<int>(volume
* 65536);
On 2012/03/12 18:40:24, Victoria Kirst wrote:
> why 65536/maxval of uint16?

We are replacing FP multiplication by integer one. I.e. instead of x*volume,
where volume in range 0..1, we have x*(volume * 65536) / 65536.

Standard trick used in lot of codecs. See use of fixed_volume in ScaleChannel()
and similar calculation in AdjustVOlume(), line 122.

https://chromiumcodereview.appspot.com/9641026/diff/1/media/audio/audio_util....
media/audio/audio_util.cc:276: MixStreams<uint8, int32, -128, 127,
128>(static_cast<uint8*>(dst),
On 2012/03/12 18:40:24, Victoria Kirst wrote:
> Why is the bias 128 here and 0 elsewhere?

Because we have "unsigned char" for 8 bits and "signed short/int" for 16/32
bits. I did not invent it, see similar usage through the file...

https://chromiumcodereview.appspot.com/9641026/diff/1/media/audio/audio_util....
media/audio/audio_util.cc:276: MixStreams<uint8, int32, -128, 127,
128>(static_cast<uint8*>(dst),
On 2012/03/12 18:40:24, Victoria Kirst wrote:
> Why is the bias 128 here and 0 elsewhere?

Because we have "unsigned char" for 8 bits and "signed short/int" for 16/32
bits. I did not invent it, see similar usage through the file...

https://chromiumcodereview.appspot.com/9641026/diff/1/media/audio/audio_util.h
File media/audio/audio_util.h (right):

https://chromiumcodereview.appspot.com/9641026/diff/1/media/audio/audio_util....
media/audio/audio_util.h:44: // Dst += Src * volume.
On 2012/03/12 18:40:24, Victoria Kirst wrote:
> This function assumes |dst| and |src| have the same sample rate, right? Add
> comment?

Done.

https://chromiumcodereview.appspot.com/9641026/diff/1/media/audio/audio_util_...
File media/audio/audio_util_unittest.cc (right):

https://chromiumcodereview.appspot.com/9641026/diff/1/media/audio/audio_util_...
media/audio/audio_util_unittest.cc:90: media::MixStreams(dst_u8, src_u8,
sizeof(dst_u8), sizeof(src_u8[0]), 0.5f);
On 2012/03/12 18:40:24, Victoria Kirst wrote:
> It looks like you are adjusting the volume in most of these, except the tests
> with "_one" appended to the name. Seems like it'd make more sense to test
volume
> and stream format separately.
> 
> Maybe change the u8, s16, and s32 tests to be at full volume (1.0), then have
a
> u8_HalfVolume, s16_QuarterVolume, s32_ThreeQuarters volume or something like
> that?

Added missing test and changed u8 test. Now we test size (1, 2, 4) on volume
(0.25, 1.0).

[vrk (LEFT CHROMIUM), 2012-03-20 00:27:53]

LGTM with nits

Sorry for the delay -- forgot this was still out! Feel free to ping me if you're
waiting on a CL this long in the future.

https://chromiumcodereview.appspot.com/9641026/diff/1/media/audio/audio_util.cc
File media/audio/audio_util.cc (right):

https://chromiumcodereview.appspot.com/9641026/diff/1/media/audio/audio_util....
media/audio/audio_util.cc:256: const int fixed_volume = static_cast<int>(volume
* 65536);
On 2012/03/12 21:20:53, enal1 wrote:
> On 2012/03/12 18:40:24, Victoria Kirst wrote:
> > why 65536/maxval of uint16?
> 
> We are replacing FP multiplication by integer one. I.e. instead of x*volume,
> where volume in range 0..1, we have x*(volume * 65536) / 65536.
> 
> Standard trick used in lot of codecs. See use of fixed_volume in
ScaleChannel()
> and similar calculation in AdjustVOlume(), line 122.

OK... well, it's not code that you wrote, but it'd be nice if ScaleChannel()
took the raw float volume as a parameter, then did the "volume * 65536" in the
ScaleChannel() implementation. Up to you if you want to make that change, since
I see that FoldChannels() does something different for the center channel.

https://chromiumcodereview.appspot.com/9641026/diff/1/media/audio/audio_util_...
File media/audio/audio_util_unittest.cc (right):

https://chromiumcodereview.appspot.com/9641026/diff/1/media/audio/audio_util_...
media/audio/audio_util_unittest.cc:90: media::MixStreams(dst_u8, src_u8,
sizeof(dst_u8), sizeof(src_u8[0]), 0.5f);
On 2012/03/12 21:20:53, enal1 wrote:
> On 2012/03/12 18:40:24, Victoria Kirst wrote:
> > It looks like you are adjusting the volume in most of these, except the
tests
> > with "_one" appended to the name. Seems like it'd make more sense to test
> volume
> > and stream format separately.
> > 
> > Maybe change the u8, s16, and s32 tests to be at full volume (1.0), then
have
> a
> > u8_HalfVolume, s16_QuarterVolume, s32_ThreeQuarters volume or something like
> > that?
> 
> Added missing test and changed u8 test. Now we test size (1, 2, 4) on volume
> (0.25, 1.0).

Thanks enal! Can you do the name change as well? (e.g.
MixStreams_u8_QuarterVolume instead of MixStreams_u8, MixStreams_u8_FullVolume
instead of MixStreams_u8_one).

https://chromiumcodereview.appspot.com/9641026/diff/4001/media/audio/audio_ut...
File media/audio/audio_util.cc (right):

https://chromiumcodereview.appspot.com/9641026/diff/4001/media/audio/audio_ut...
media/audio/audio_util.cc:249: static void MixStreams(Format* dst, Format* src,
int count, float volume) {
Return early for muted src?

if (volume == 0.0f)
  return;

https://chromiumcodereview.appspot.com/9641026/diff/4001/media/audio/audio_ut...
media/audio/audio_util.cc:250: if (volume == 1.0f) {
nit: It's cleaner/more concise if you push the if into the loop instead of vice
versa, e.g.

for (int i = 0; i < count; ++i) {
  Fixed src = src[i] - bias;
  if (volume != 1.0f) {
    // General case -- have to adjust volume before mixing.
    const int fixed_volume = static_cast<int>(volume * 65536);
    src = ScaleChannel<Fixed>(src, fixed_volume);
  }
  Fixed value = AddSaturated<Fixed, min_value, max_value>(dst[i] - bias, src);
  dst[i] = static_cast<Format>(value + bias);
}

https://chromiumcodereview.appspot.com/9641026/diff/4001/media/audio/audio_ut...
media/audio/audio_util.cc:276: DCHECK(volume >= 0.0f && volume <= 1.0f);
nit: two separate DCHECKs, DCHECK_GE(volume, 0.0f) and DCHECK_LE(volume, 1.0f)

(code-wise, I think what you did looks nicer, but doing DCHECK_LE/GE gives more
helpful messages if things fail!)

[scherkus (not reviewing), 2012-03-20 14:45:06]

enal: can you update the CL description with the appropriate BUG= for adding
mixing support?

[commit-bot: I haz the power, 2012-03-20 15:58:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/enal@chromium.org/9641026/10001

[commit-bot: I haz the power, 2012-03-20 17:10:25]

Change committed as 127709