Fix errors in comments. Make comments match code.

Fix errors in comments. Make comments match code.

Fix the DCHECK in ValidateLayout for symmetric channel layouts.

Add 'const' to methods that don't mutate class members.

Include "base/macros.h" for the DISALLOW_COPY_AND_ASSIGN macro.

R=dalecurtis@chromium.org

Committed: https://crrev.com/80aa80baf86aed95ff718325f1d70ad39fbe9865
Cr-Commit-Position: refs/heads/master@{#300612}

[wtc, 2014-10-21 20:42:37]

Hi Dale,

I read media/base/channel_mixer.* to understand how it works. Here are the fixes
for some small problems I noticed while reading the code. Please review. Thanks.

https://codereview.chromium.org/645853011/diff/1/media/base/channel_layout.h
File media/base/channel_layout.h (right):

https://codereview.chromium.org/645853011/diff/1/media/base/channel_layout.h#...
media/base/channel_layout.h:129: // from 0 to
ChannelLayoutToChannelCount(layout) - 1.

The original upper bound is too loose. This is the tight upper bound.

https://codereview.chromium.org/645853011/diff/1/media/base/channel_mixer.cc
File media/base/channel_mixer.cc (right):

https://codereview.chromium.org/645853011/diff/1/media/base/channel_mixer.cc#...
media/base/channel_mixer.cc:51: (ChannelOrder(layout, RIGHT_OF_CENTER) >= 0))

Please check this change carefully. I don't understand the original DCHECK. I
believe the intention was to DCHECK these four conditions separately.

p == q is a shorthand for (p && q) || (!p && !q), i.e., p and q should be both
true or both false.

https://codereview.chromium.org/645853011/diff/1/media/base/channel_mixer.cc#...
media/base/channel_mixer.cc:291: // TODO(wtc): Not sure about these values?

I'm actually not sure about several other scale factors used in the mixing. I
only marked some of them with TODO comments. Please let me know if you think it
is correct to use kEqualPowerScale in these cases.

[DaleCurtis, 2014-10-21 20:52:04]

https://codereview.chromium.org/645853011/diff/1/media/base/channel_mixer.cc
File media/base/channel_mixer.cc (right):

https://codereview.chromium.org/645853011/diff/1/media/base/channel_mixer.cc#...
media/base/channel_mixer.cc:51: (ChannelOrder(layout, RIGHT_OF_CENTER) >= 0))
On 2014/10/21 20:42:37, wtc wrote:
> 
> Please check this change carefully. I don't understand the original DCHECK. I
> believe the intention was to DCHECK these four conditions separately.
> 
> p == q is a shorthand for (p && q) || (!p && !q), i.e., p and q should be both
> true or both false.

The original DCHECK just checks to see if we have a left channel that we also
have a right channel.  It seems all you've done here is break it out into
separate conditions.

What do you think was wrong with the original check?

https://codereview.chromium.org/645853011/diff/1/media/base/channel_mixer.cc#...
media/base/channel_mixer.cc:291: // TODO(wtc): Not sure about these values?
On 2014/10/21 20:42:37, wtc wrote:
> 
> I'm actually not sure about several other scale factors used in the mixing. I
> only marked some of them with TODO comments. Please let me know if you think
it
> is correct to use kEqualPowerScale in these cases.

These values are chosen based on discussions with crogers@ who has extensive
mixing experience (though is no longer at Google).  It'd be nice to double check
them for sure.  Note my TODOs above :)

https://codereview.chromium.org/645853011/diff/1/media/base/channel_mixer.cc#...
media/base/channel_mixer.cc:296: // TODO(wtc): How do we know center exists?
CENTER == mono, which every layout has.

https://codereview.chromium.org/645853011/diff/1/media/base/channel_mixer.cc#...
media/base/channel_mixer.cc:310: // TODO(wtc): why isn't the scale 1?
Because we may have just mixed a bunch of other channels above into the MONO
channel.

[wtc, 2014-10-21 21:16:59]

Dale: thank you for the review. I removed the TODO comments.

https://codereview.chromium.org/645853011/diff/1/media/base/channel_mixer.cc
File media/base/channel_mixer.cc (right):

https://codereview.chromium.org/645853011/diff/1/media/base/channel_mixer.cc#...
media/base/channel_mixer.cc:51: (ChannelOrder(layout, RIGHT_OF_CENTER) >= 0))
On 2014/10/21 20:52:03, DaleCurtis wrote:
>
> What do you think was wrong with the original check?

Here is an example that the original check considers symmetric: a channel layout
with these three channels:

  LEFT, RIGHT, SIDE_LEFT

This channel layout is not symmetric because it has SIDE_LEFT but not
SIDE_RIGHT. However, it satisfies the original DCHECK because of the use of ||
in the original DCHECK.

https://codereview.chromium.org/645853011/diff/1/media/base/channel_mixer.cc#...
media/base/channel_mixer.cc:291: // TODO(wtc): Not sure about these values?

I will remove this TODO comment. I elaborate on this issue below.

https://codereview.chromium.org/645853011/diff/1/media/base/channel_mixer.cc#...
media/base/channel_mixer.cc:296: // TODO(wtc): How do we know center exists?

On 2014/10/21 20:52:03, DaleCurtis wrote:
> CENTER == mono, which every layout has.

Not sure what you meant here. In any case, I came up with this argument:

  Every layout except mono has LEFT and RIGHT.
  Therefore, if a layout doesn't have LEFT, it
  must be mono. Mono has CENTER.

Is this correct?

https://codereview.chromium.org/645853011/diff/1/media/base/channel_mixer.cc#...
media/base/channel_mixer.cc:310: // TODO(wtc): why isn't the scale 1?
On 2014/10/21 20:52:03, DaleCurtis wrote:
> Because we may have just mixed a bunch of other channels above into the MONO
> channel.

Understood. I will remove this TODO comment.

This case is very similar to the case of mixing BACK_CENTER into CENTER (lines
281-283), on which you have a TODO comment.

kEqualPowerScale is 1/sqrt(2), which implies two channels are mixed into one. If
we are mixing N channels into the MONO channel, it seems that 1/sqrt(N) would be
more appropriate. Since we don't know what N is, I don't know if we can blindly
use 1/sqrt(2).

Does this make sense?

[DaleCurtis, 2014-10-21 21:22:59]

https://codereview.chromium.org/645853011/diff/1/media/base/channel_mixer.cc
File media/base/channel_mixer.cc (right):

https://codereview.chromium.org/645853011/diff/1/media/base/channel_mixer.cc#...
media/base/channel_mixer.cc:51: (ChannelOrder(layout, RIGHT_OF_CENTER) >= 0))
On 2014/10/21 21:16:58, wtc wrote:
> On 2014/10/21 20:52:03, DaleCurtis wrote:
> >
> > What do you think was wrong with the original check?
> 
> Here is an example that the original check considers symmetric: a channel
layout
> with these three channels:
> 
>   LEFT, RIGHT, SIDE_LEFT
> 
> This channel layout is not symmetric because it has SIDE_LEFT but not
> SIDE_RIGHT. However, it satisfies the original DCHECK because of the use of ||
> in the original DCHECK.

Ah, whoops, yeah I see what you mean. You're approach is fine, but I'd just
remove the print text in favor of a comment as well as switch to DCHECK_EQ().

https://codereview.chromium.org/645853011/diff/1/media/base/channel_mixer.cc#...
media/base/channel_mixer.cc:296: // TODO(wtc): How do we know center exists?
On 2014/10/21 21:16:58, wtc wrote:
> 
> On 2014/10/21 20:52:03, DaleCurtis wrote:
> > CENTER == mono, which every layout has.
> 
> Not sure what you meant here. In any case, I came up with this argument:
> 
>   Every layout except mono has LEFT and RIGHT.
>   Therefore, if a layout doesn't have LEFT, it
>   must be mono. Mono has CENTER.
> 
> Is this correct?

Yes, that's what I was trying to say.

https://codereview.chromium.org/645853011/diff/1/media/base/channel_mixer.cc#...
media/base/channel_mixer.cc:310: // TODO(wtc): why isn't the scale 1?
On 2014/10/21 21:16:58, wtc wrote:
> On 2014/10/21 20:52:03, DaleCurtis wrote:
> > Because we may have just mixed a bunch of other channels above into the MONO
> > channel.
> 
> Understood. I will remove this TODO comment.
> 
> This case is very similar to the case of mixing BACK_CENTER into CENTER (lines
> 281-283), on which you have a TODO comment.
> 
> kEqualPowerScale is 1/sqrt(2), which implies two channels are mixed into one.
If
> we are mixing N channels into the MONO channel, it seems that 1/sqrt(N) would
be
> more appropriate. Since we don't know what N is, I don't know if we can
blindly
> use 1/sqrt(2).
> 
> Does this make sense?

Yeah, certainly there are some odd edge cases here and I appreciate a fresh set
of eyes on it.

[wtc, 2014-10-21 21:48:50]

Dale: please review patch set 3. Thanks.

https://codereview.chromium.org/645853011/diff/1/media/base/channel_mixer.cc
File media/base/channel_mixer.cc (right):

https://codereview.chromium.org/645853011/diff/1/media/base/channel_mixer.cc#...
media/base/channel_mixer.cc:51: (ChannelOrder(layout, RIGHT_OF_CENTER) >= 0))
On 2014/10/21 21:22:59, DaleCurtis wrote:
> On 2014/10/21 21:16:58, wtc wrote:
> > On 2014/10/21 20:52:03, DaleCurtis wrote:
> > >
> > > What do you think was wrong with the original check?
> > 
> > Here is an example that the original check considers symmetric: a channel
> layout
> > with these three channels:
> > 
> >   LEFT, RIGHT, SIDE_LEFT
> > 
> > This channel layout is not symmetric because it has SIDE_LEFT but not
> > SIDE_RIGHT. However, it satisfies the original DCHECK because of the use of
||
> > in the original DCHECK.
> 
> Ah, whoops, yeah I see what you mean. You're approach is fine, but I'd just
> remove the print text in favor of a comment as well as switch to DCHECK_EQ().

Done.

[DaleCurtis, 2014-10-21 21:52:09]

lgtm, but go ahead and set BUG=none or remove it from the description for
cleanliness.

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

The CQ bit was checked by wtc@chromium.org

[commit-bot: I haz the power, 2014-10-21 23:38:41]

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

[commit-bot: I haz the power, 2014-10-22 01:49:31]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2014-10-22 04:05:12]

Patchset 3 (id:??) landed as
https://crrev.com/80aa80baf86aed95ff718325f1d70ad39fbe9865
Cr-Commit-Position: refs/heads/master@{#300612}