Expose the internal MatrixBuilder class as ChannelMixingMatrix.

Expose the internal MatrixBuilder class as ChannelMixingMatrix.

R=dalecurtis@chromium.org
BUG=none

Committed: https://crrev.com/61f68e21492ffdfc03b8a6f9fc2ef8dc60cabea2
Cr-Commit-Position: refs/heads/master@{#300962}

[DaleCurtis, 2014-10-23 16:49:05]

https://codereview.chromium.org/672793002/diff/40001/media/base/channel_mixer...
File media/base/channel_mixer_unittest.cc (right):

https://codereview.chromium.org/672793002/diff/40001/media/base/channel_mixer...
media/base/channel_mixer_unittest.cc:27: // TODO(wtc): why do we only test up to
CHANNEL_LAYOUT_STEREO_DOWNMIX?
Hmm, I don't remember, may be a bug. What happens if you change it to MAX? Is
there any git blame info on that line?

https://codereview.chromium.org/672793002/diff/40001/media/base/channel_mixin...
File media/base/channel_mixing_matrix_unittest.cc (right):

https://codereview.chromium.org/672793002/diff/40001/media/base/channel_mixin...
media/base/channel_mixing_matrix_unittest.cc:97:
EXPECT_EQ(static_cast<float>(M_SQRT1_2), matrix[0][0]);
You may need EXPECT_FLOAT_EQ all over to get past all the try bots, FYI.

https://codereview.chromium.org/672793002/diff/60001/media/base/channel_mixer...
File media/base/channel_mixer_unittest.cc (right):

https://codereview.chromium.org/672793002/diff/60001/media/base/channel_mixer...
media/base/channel_mixer_unittest.cc:22: TEST(ChannelMixerTest,
ConstructAllPossibleLayouts) {
Delete this test now? I think it's sufficient to test in Matrix builder.

[wtc, 2014-10-23 16:55:13]

Dale: thanks for the review! I need to go to a meeting now. I will address your
review comments after 11:00 am.

https://codereview.chromium.org/672793002/diff/80001/media/base/channel_mixin...
File media/base/channel_mixing_matrix_unittest.cc (right):

https://codereview.chromium.org/672793002/diff/80001/media/base/channel_mixin...
media/base/channel_mixing_matrix_unittest.cc:110: // Output: mono CENTER  0.707 
0.707  1       0.707  0.707      0.707

Dale: I added graphical representations of the matrices. I must admit I find the
sole 1 scaling factor for CENTER in this matrix very surprising, although it is
the expected result of our algorithm.

[DaleCurtis, 2014-10-23 17:15:27]

https://codereview.chromium.org/672793002/diff/80001/media/base/channel_mixin...
File media/base/channel_mixing_matrix_unittest.cc (right):

https://codereview.chromium.org/672793002/diff/80001/media/base/channel_mixin...
media/base/channel_mixing_matrix_unittest.cc:110: // Output: mono CENTER  0.707 
0.707  1       0.707  0.707      0.707
On 2014/10/23 16:55:13, wtc wrote:
> 
> Dale: I added graphical representations of the matrices. I must admit I find
the
> sole 1 scaling factor for CENTER in this matrix very surprising, although it
is
> the expected result of our algorithm.

By all means, please analyze :) I intuitively chose that value because the
center channel in 5.1 is generally vocals, etc, other material which should be
featured more prominently than surrounds.

Based on listening to a few 5.1 -> mono down mixes it sounds good too, but I'd
very much welcome a new set of ears on it!

[wtc, 2014-10-23 18:39:46]

Dale: please review patch set 6. Thanks.

https://codereview.chromium.org/672793002/diff/40001/media/base/channel_mixer...
File media/base/channel_mixer_unittest.cc (right):

https://codereview.chromium.org/672793002/diff/40001/media/base/channel_mixer...
media/base/channel_mixer_unittest.cc:27: // TODO(wtc): why do we only test up to
CHANNEL_LAYOUT_STEREO_DOWNMIX?

On 2014/10/23 16:49:05, DaleCurtis wrote:
> Hmm, I don't remember, may be a bug. What happens if you change it to MAX?

If I change it to MAX, this CHECK will fail:

  // Stereo down mix should never be the output layout.
  CHECK_NE(output_layout, CHANNEL_LAYOUT_STEREO_DOWNMIX);

I can fix the CHECK failure by skipping the test if output_layout ==
CHANNEL_LAYOUT_STEREO_DOWNMIX on line 35.

> Is there any git blame info on that line?

Yes. You added it in the initial revision of this file:

https://chromiumcodereview.appspot.com/11150034

https://codereview.chromium.org/672793002/diff/40001/media/base/channel_mixin...
File media/base/channel_mixing_matrix_unittest.cc (right):

https://codereview.chromium.org/672793002/diff/40001/media/base/channel_mixin...
media/base/channel_mixing_matrix_unittest.cc:97:
EXPECT_EQ(static_cast<float>(M_SQRT1_2), matrix[0][0]);

On 2014/10/23 16:49:05, DaleCurtis wrote:
> You may need EXPECT_FLOAT_EQ all over to get past all the try bots, FYI.

Done.

EXPECT_EQ also works on all the try bots. I think it's because we assign the
exact same expression to the matrix elements.

Should I also use EXPECT_FLOAT_EQ with the other three values (0.0f, 0.5f,
1.0f)? Or is EXPECT_EQ fine because these three values can be represented
exactly as binary fractions?

https://codereview.chromium.org/672793002/diff/60001/media/base/channel_mixer...
File media/base/channel_mixer_unittest.cc (right):

https://codereview.chromium.org/672793002/diff/60001/media/base/channel_mixer...
media/base/channel_mixer_unittest.cc:22: TEST(ChannelMixerTest,
ConstructAllPossibleLayouts) {

On 2014/10/23 16:49:05, DaleCurtis wrote:
> Delete this test now? I think it's sufficient to test in Matrix builder.

You are probably right. This test does test a little bit more code than the
Matrix builder test because this test calls mixer.Transform(). So it seems
useful to keep this test to verify that none of the CHECKs or DCHECKs will fail
and valgrind/ASAN report no memory errors. What do you think?

https://codereview.chromium.org/672793002/diff/80001/media/base/channel_mixin...
File media/base/channel_mixing_matrix_unittest.cc (right):

https://codereview.chromium.org/672793002/diff/80001/media/base/channel_mixin...
media/base/channel_mixing_matrix_unittest.cc:110: // Output: mono CENTER  0.707 
0.707  1       0.707  0.707      0.707

On 2014/10/23 17:15:27, DaleCurtis wrote:
>
> By all means, please analyze :) I intuitively chose that value because the
> center channel in 5.1 is generally vocals, etc, other material which should be
> featured more prominently than surrounds.

That's a good rationale and it's supported by your listening tests. I was
worried that the center channel uses the 1.0 scale simply because the output
layout also has a center channel.

[DaleCurtis, 2014-10-23 19:22:48]

lgtm

[DaleCurtis, 2014-10-23 19:23:50]

https://codereview.chromium.org/672793002/diff/40001/media/base/channel_mixer...
File media/base/channel_mixer_unittest.cc (right):

https://codereview.chromium.org/672793002/diff/40001/media/base/channel_mixer...
media/base/channel_mixer_unittest.cc:27: // TODO(wtc): why do we only test up to
CHANNEL_LAYOUT_STEREO_DOWNMIX?
On 2014/10/23 18:39:45, wtc wrote:
> 
> On 2014/10/23 16:49:05, DaleCurtis wrote:
> > Hmm, I don't remember, may be a bug. What happens if you change it to MAX?
> 
> If I change it to MAX, this CHECK will fail:
> 
>   // Stereo down mix should never be the output layout.
>   CHECK_NE(output_layout, CHANNEL_LAYOUT_STEREO_DOWNMIX);
> 
> I can fix the CHECK failure by skipping the test if output_layout ==
> CHANNEL_LAYOUT_STEREO_DOWNMIX on line 35.
> 
> > Is there any git blame info on that line?
> 
> Yes. You added it in the initial revision of this file:
> 
> https://chromiumcodereview.appspot.com/11150034

Haha, whoops. Thanks for fixing :)

https://codereview.chromium.org/672793002/diff/40001/media/base/channel_mixin...
File media/base/channel_mixing_matrix_unittest.cc (right):

https://codereview.chromium.org/672793002/diff/40001/media/base/channel_mixin...
media/base/channel_mixing_matrix_unittest.cc:97:
EXPECT_EQ(static_cast<float>(M_SQRT1_2), matrix[0][0]);
On 2014/10/23 18:39:45, wtc wrote:
> 
> On 2014/10/23 16:49:05, DaleCurtis wrote:
> > You may need EXPECT_FLOAT_EQ all over to get past all the try bots, FYI.
> 
> Done.
> 
> EXPECT_EQ also works on all the try bots. I think it's because we assign the
> exact same expression to the matrix elements.
> 
> Should I also use EXPECT_FLOAT_EQ with the other three values (0.0f, 0.5f,
> 1.0f)? Or is EXPECT_EQ fine because these three values can be represented
> exactly as binary fractions?

I'd just wait and see if any trybots actually fail.

https://codereview.chromium.org/672793002/diff/60001/media/base/channel_mixer...
File media/base/channel_mixer_unittest.cc (right):

https://codereview.chromium.org/672793002/diff/60001/media/base/channel_mixer...
media/base/channel_mixer_unittest.cc:22: TEST(ChannelMixerTest,
ConstructAllPossibleLayouts) {
On 2014/10/23 18:39:45, wtc wrote:
> 
> On 2014/10/23 16:49:05, DaleCurtis wrote:
> > Delete this test now? I think it's sufficient to test in Matrix builder.
> 
> You are probably right. This test does test a little bit more code than the
> Matrix builder test because this test calls mixer.Transform(). So it seems
> useful to keep this test to verify that none of the CHECKs or DCHECKs will
fail
> and valgrind/ASAN report no memory errors. What do you think?
> 

Ah, yeah that might be worth it then.

[wtc, 2014-10-23 19:24:00]

https://codereview.chromium.org/672793002/diff/100001/media/base/channel_mixe...
File media/base/channel_mixer_unittest.cc (right):

https://codereview.chromium.org/672793002/diff/100001/media/base/channel_mixe...
media/base/channel_mixer_unittest.cc:165: static float kFiveOneToMonoValues[] =
{ 0.1f, 0.2f, 0.0f, 0.4f, 0.5f, 0.6f };

Nit: when these inputs are mixed and scaled by 1/sqrt(2), the result is 1.27279,
which is outside the [-1.0, 1.0] range. I assume if this happens in reality the
output will be clipped to 1.0?

I only considered this issue because clipping avoidance is the reason we use the
0.5 scale in the stereo to mono case. I wonder if we should do the same when
down mixing from 5.1 to mono.

[DaleCurtis, 2014-10-23 19:27:55]

https://codereview.chromium.org/672793002/diff/100001/media/base/channel_mixe...
File media/base/channel_mixer_unittest.cc (right):

https://codereview.chromium.org/672793002/diff/100001/media/base/channel_mixe...
media/base/channel_mixer_unittest.cc:165: static float kFiveOneToMonoValues[] =
{ 0.1f, 0.2f, 0.0f, 0.4f, 0.5f, 0.6f };
On 2014/10/23 19:24:00, wtc wrote:
> 
> Nit: when these inputs are mixed and scaled by 1/sqrt(2), the result is
1.27279,
> which is outside the [-1.0, 1.0] range. I assume if this happens in reality
the
> output will be clipped to 1.0?
> 
> I only considered this issue because clipping avoidance is the reason we use
the
> 0.5 scale in the stereo to mono case. I wonder if we should do the same when
> down mixing from 5.1 to mono.
> 

Clipping avoidance would be nice, but I wouldn't worry about it in this patch
set. I'd include such fixups as a consequence of a larger listening-test based
analysis.

Indeed, in Chrome, values are clipped when interleaving to integer output, which
is used for all our platform drivers.

[wtc, 2014-10-23 21:00:36]

The CQ bit was checked by wtc@chromium.org

[commit-bot: I haz the power, 2014-10-23 21:04:34]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/672793002/120001

[commit-bot: I haz the power, 2014-10-23 22:16:01]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2014-10-23 22:16:41]

Patchset 7 (id:??) landed as
https://crrev.com/61f68e21492ffdfc03b8a6f9fc2ef8dc60cabea2
Cr-Commit-Position: refs/heads/master@{#300962}