Fix errors in comments. Make comments match code.

media/base/channel_mixer.cc

Index: media/base/channel_mixer.cc
diff --git a/media/base/channel_mixer.cc b/media/base/channel_mixer.cc
index 4c5179b40a80e579c11a864617c6c8cf610bc0ba..2ef73ce63930c98e83862669f88a4e43606d8aab 100644
--- a/media/base/channel_mixer.cc
+++ b/media/base/channel_mixer.cc
@@ -38,20 +38,21 @@ static void ValidateLayout(ChannelLayout layout) {
   // Symmetry allows simplifying the matrix building code by allowing us to
   // assume that if one channel of a pair exists, the other will too.
   if (channel_count > 1) {
-    DCHECK((ChannelOrder(layout, LEFT) >= 0 &&
-            ChannelOrder(layout, RIGHT) >= 0) ||
-           (ChannelOrder(layout, SIDE_LEFT) >= 0 &&
-            ChannelOrder(layout, SIDE_RIGHT) >= 0) ||
-           (ChannelOrder(layout, BACK_LEFT) >= 0 &&
-            ChannelOrder(layout, BACK_RIGHT) >= 0) ||
-           (ChannelOrder(layout, LEFT_OF_CENTER) >= 0 &&
-            ChannelOrder(layout, RIGHT_OF_CENTER) >= 0))
+    DCHECK((ChannelOrder(layout, LEFT) >= 0) ==
+           (ChannelOrder(layout, RIGHT) >= 0))
+        << "Non-symmetric channel layout encountered.";
+    DCHECK((ChannelOrder(layout, SIDE_LEFT) >= 0) ==
+           (ChannelOrder(layout, SIDE_RIGHT) >= 0))
+        << "Non-symmetric channel layout encountered.";
+    DCHECK((ChannelOrder(layout, BACK_LEFT) >= 0) ==
+           (ChannelOrder(layout, BACK_RIGHT) >= 0))
+        << "Non-symmetric channel layout encountered.";
+    DCHECK((ChannelOrder(layout, LEFT_OF_CENTER) >= 0) ==
+           (ChannelOrder(layout, RIGHT_OF_CENTER) >= 0))

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

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.

[DaleCurtis, 2014/10/21 20:52:03]

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?

[wtc, 2014/10/21 21:16:58]

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.

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

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().

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

Done.

         << "Non-symmetric channel layout encountered.";
   } else {
     DCHECK_EQ(layout, CHANNEL_LAYOUT_MONO);
   }
-
-  return;
 }
 
 class MatrixBuilder {
@@ -96,19 +97,18 @@ class MatrixBuilder {
 
   // Helper methods for managing unaccounted input channels.
   void AccountFor(Channels ch);
-  bool IsUnaccounted(Channels ch);
+  bool IsUnaccounted(Channels ch) const;
 
   // Helper methods for checking if |ch| exists in either |input_layout_| or
   // |output_layout_| respectively.
-  bool HasInputChannel(Channels ch);
-  bool HasOutputChannel(Channels ch);
+  bool HasInputChannel(Channels ch) const;
+  bool HasOutputChannel(Channels ch) const;
 
   // Helper methods for updating |matrix_| with the proper value for
   // mixing |input_ch| into |output_ch|.  MixWithoutAccounting() does not
   // remove the channel from |unaccounted_inputs_|.
   void Mix(Channels input_ch, Channels output_ch, float scale);
-  void MixWithoutAccounting(Channels input_ch, Channels output_ch,
-                                          float scale);
+  void MixWithoutAccounting(Channels input_ch, Channels output_ch, float scale);
 
   DISALLOW_COPY_AND_ASSIGN(MatrixBuilder);
 };
@@ -219,8 +219,8 @@ bool MatrixBuilder::CreateTransformationMatrix(
   // Mix back LR into: side LR || back center || front LR || front center.
   if (IsUnaccounted(BACK_LEFT)) {
     if (HasOutputChannel(SIDE_LEFT)) {
-      // If we have side LR, mix back LR into side LR, but instead if the input
-      // doesn't have side LR (but output does) copy back LR to side LR.
+      // If the input has side LR, mix back LR into side LR, but instead if the
+      // input doesn't have side LR (but output does) copy back LR to side LR.
       float scale = HasInputChannel(SIDE_LEFT) ? kEqualPowerScale : 1;
       Mix(BACK_LEFT, SIDE_LEFT, scale);
       Mix(BACK_RIGHT, SIDE_RIGHT, scale);
@@ -242,8 +242,8 @@ bool MatrixBuilder::CreateTransformationMatrix(
   // Mix side LR into: back LR || back center || front LR || front center.
   if (IsUnaccounted(SIDE_LEFT)) {
     if (HasOutputChannel(BACK_LEFT)) {
-      // If we have back LR, mix side LR into back LR, but instead if the input
-      // doesn't have back LR (but output does) copy side LR to back LR.
+      // If the input has back LR, mix side LR into back LR, but instead if the
+      // input doesn't have back LR (but output does) copy side LR to back LR.
       float scale = HasInputChannel(BACK_LEFT) ? kEqualPowerScale : 1;
       Mix(SIDE_LEFT, BACK_LEFT, scale);
       Mix(SIDE_RIGHT, BACK_RIGHT, scale);
@@ -284,20 +284,22 @@ bool MatrixBuilder::CreateTransformationMatrix(
     }
   }
 
-  // Mix LR of center into: front center || front LR.
+  // Mix LR of center into: front LR || front center.
   if (IsUnaccounted(LEFT_OF_CENTER)) {
     if (HasOutputChannel(LEFT)) {
       // Mix LR of center into front LR.
+      // TODO(wtc): Not sure about these values?

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

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:03]

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 :)

[wtc, 2014/10/21 21:16:58]

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

       Mix(LEFT_OF_CENTER, LEFT, kEqualPowerScale);
       Mix(RIGHT_OF_CENTER, RIGHT, kEqualPowerScale);
     } else {
       // Mix LR of center into front center.
+      // TODO(wtc): How do we know center exists?

[DaleCurtis, 2014/10/21 20:52:03]

CENTER == mono, which every layout has. 

[wtc, 2014/10/21 21:16:58]

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?

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

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

       Mix(LEFT_OF_CENTER, CENTER, kEqualPowerScale);
       Mix(RIGHT_OF_CENTER, CENTER, kEqualPowerScale);
     }
   }
 
-  // Mix LFE into: front LR || front center.
+  // Mix LFE into: front center || front LR.
   if (IsUnaccounted(LFE)) {
     if (!HasOutputChannel(CENTER)) {
       // Mix LFE into front LR.
@@ -305,6 +307,7 @@ bool MatrixBuilder::CreateTransformationMatrix(
       Mix(LFE, RIGHT, kEqualPowerScale);
     } else {
       // Mix LFE into front center.
+      // TODO(wtc): why isn't the scale 1?

[DaleCurtis, 2014/10/21 20:52:03]

Because we may have just mixed a bunch of other channels above into the MONO
channel.

[wtc, 2014/10/21 21:16:58]

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]

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

       Mix(LFE, CENTER, kEqualPowerScale);
     }
   }
@@ -373,16 +376,16 @@ void MatrixBuilder::AccountFor(Channels ch) {
       unaccounted_inputs_.begin(), unaccounted_inputs_.end(), ch));
 }
 
-bool MatrixBuilder::IsUnaccounted(Channels ch) {
+bool MatrixBuilder::IsUnaccounted(Channels ch) const {
   return std::find(unaccounted_inputs_.begin(), unaccounted_inputs_.end(),
                    ch) != unaccounted_inputs_.end();
 }
 
-bool MatrixBuilder::HasInputChannel(Channels ch) {
+bool MatrixBuilder::HasInputChannel(Channels ch) const {
   return ChannelOrder(input_layout_, ch) >= 0;
 }
 
-bool MatrixBuilder::HasOutputChannel(Channels ch) {
+bool MatrixBuilder::HasOutputChannel(Channels ch) const {
   return ChannelOrder(output_layout_, ch) >= 0;
 }