Fix errors in comments. Make comments match code.

media/base/channel_mixer.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 // MSVC++ requires this to be set before any other includes to get M_SQRT1_2.
 #define _USE_MATH_DEFINES
 
 #include "media/base/channel_mixer.h"
 
 #include <algorithm>
@@ skipping 20 matching lines @@
   // Verify there's at least one channel.  Should always be true here by virtue
   // of not being one of the invalid layouts, but lets double check to be sure.
   int channel_count = ChannelLayoutToChannelCount(layout);
   DCHECK_GT(channel_count, 0);
 
   // If we have more than one channel, verify a symmetric layout for sanity.
   // The unit test will verify all possible layouts, so this can be a DCHECK.
   // 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 {
  public:
   MatrixBuilder(ChannelLayout input_layout, int input_channels,
                 ChannelLayout output_layout, int output_channels)
       : input_layout_(input_layout),
         input_channels_(input_channels),
         output_layout_(output_layout),
         output_channels_(output_channels) {
@@ skipping 24 matching lines @@
   int input_channels_;
   ChannelLayout output_layout_;
   int output_channels_;
 
   // Helper variable for tracking which inputs are currently unaccounted,
   // should be empty after construction completes.
   std::vector<Channels> unaccounted_inputs_;
 
   // 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);
 };
 
 ChannelMixer::ChannelMixer(ChannelLayout input_layout,
                            ChannelLayout output_layout) {
   Initialize(input_layout,
              ChannelLayoutToChannelCount(input_layout),
              output_layout,
              ChannelLayoutToChannelCount(output_layout));
@@ skipping 90 matching lines @@
     // When up mixing from mono, just do a copy to front LR.
     float scale =
         (input_layout_ == CHANNEL_LAYOUT_MONO) ? 1 : kEqualPowerScale;
     MixWithoutAccounting(CENTER, LEFT, scale);
     Mix(CENTER, RIGHT, scale);
   }
 
   // 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);
     } else if (HasOutputChannel(BACK_CENTER)) {
       // Mix back LR into back center.
       Mix(BACK_LEFT, BACK_CENTER, kEqualPowerScale);
       Mix(BACK_RIGHT, BACK_CENTER, kEqualPowerScale);
     } else if (output_layout_ > CHANNEL_LAYOUT_MONO) {
       // Mix back LR into front LR.
       Mix(BACK_LEFT, LEFT, kEqualPowerScale);
       Mix(BACK_RIGHT, RIGHT, kEqualPowerScale);
     } else {
       // Mix back LR into front center.
       Mix(BACK_LEFT, CENTER, kEqualPowerScale);
       Mix(BACK_RIGHT, CENTER, kEqualPowerScale);
     }
   }
 
   // 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);
     } else if (HasOutputChannel(BACK_CENTER)) {
       // Mix side LR into back center.
       Mix(SIDE_LEFT, BACK_CENTER, kEqualPowerScale);
       Mix(SIDE_RIGHT, BACK_CENTER, kEqualPowerScale);
     } else if (output_layout_ > CHANNEL_LAYOUT_MONO) {
       // Mix side LR into front LR.
       Mix(SIDE_LEFT, LEFT, kEqualPowerScale);
@@ skipping 20 matching lines @@
       // TODO(dalecurtis): Not sure about these values?
       MixWithoutAccounting(BACK_CENTER, LEFT, kEqualPowerScale);
       Mix(BACK_CENTER, RIGHT, kEqualPowerScale);
     } else {
       // Mix back center into front center.
       // TODO(dalecurtis): Not sure about these values?
       Mix(BACK_CENTER, CENTER, kEqualPowerScale);
     }
   }
 
-  // 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.
       MixWithoutAccounting(LFE, LEFT, kEqualPowerScale);
       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);
     }
   }
 
   // All channels should now be accounted for.
   DCHECK(unaccounted_inputs_.empty());
 
   // See if the output |matrix_| is simply a remapping matrix.  If each input
   // channel maps to a single output channel we can simply remap.  Doing this
   // programmatically is less fragile than logic checks on channel mappings.
@@ skipping 48 matching lines @@
       }
     }
   }
 }
 
 void MatrixBuilder::AccountFor(Channels ch) {
   unaccounted_inputs_.erase(std::find(
       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;
 }
 
 void MatrixBuilder::Mix(Channels input_ch, Channels output_ch, float scale) {
   MixWithoutAccounting(input_ch, output_ch, scale);
   AccountFor(input_ch);
 }
 
 void MatrixBuilder::MixWithoutAccounting(Channels input_ch, Channels output_ch,
                                          float scale) {
   int input_ch_index = ChannelOrder(input_layout_, input_ch);
   int output_ch_index = ChannelOrder(output_layout_, output_ch);
 
   DCHECK(IsUnaccounted(input_ch));
   DCHECK_GE(input_ch_index, 0);
   DCHECK_GE(output_ch_index, 0);
 
   DCHECK_EQ((*matrix_)[output_ch_index][input_ch_index], 0);
   (*matrix_)[output_ch_index][input_ch_index] = scale;
 }
 
 }  // namespace media