Replaced the wav file dumping functionality in aec_core.c with the newly added corresponding macros

webrtc/modules/audio_processing/logging/aec_logging.h

+/*
+ *  Copyright (c) 2013 The WebRTC project authors. All Rights Reserved.

[hlundin-webrtc, 2015/08/10 13:04:41]

2015

[peah-webrtc, 2015/08/12 20:25:52]

Done.

+ *
+ *  Use of this source code is governed by a BSD-style license
+ *  that can be found in the LICENSE file in the root of the source
+ *  tree. An additional intellectual property rights grant can be found
+ *  in the file PATENTS.  All contributing project authors may
+ *  be found in the AUTHORS file in the root of the source tree.
+ */
+
+#ifndef WEBRTC_MODULES_AUDIO_PROCESSING_AEC_AEC_LOGGING_
+#define WEBRTC_MODULES_AUDIO_PROCESSING_AEC_AEC_LOGGING_
+

[Andrew MacDonald, 2015/08/10 15:25:55]

Remove extra blank line.

[peah-webrtc, 2015/08/12 20:25:52]

Done.

+
+// To enable AEC logging, run this command from trunk/ :

[hlundin-webrtc, 2015/08/10 13:04:41]

See my other suggestion in aec_logging_file_handling.h.

[peah-webrtc, 2015/08/12 20:25:52]

Done.

+// python webrtc/build/gyp_webrtc.py -Daec_debug_dump=1
+#ifdef WEBRTC_AEC_DEBUG_DUMP
+#include <stdio.h>
+#include "webrtc/modules/audio_processing/logging/aec_logging_file_handling.h"
+#endif
+
+#ifdef __cplusplus
+extern "C" {
+#endif

[kwiberg-webrtc, 2015/08/10 20:19:57]

You only define macros, so the extern "C" thingy isn't necessary.

[peah-webrtc, 2015/08/12 20:25:52]

Done.

+
+

[Andrew MacDonald, 2015/08/10 15:25:55]

Remove extra blank line.

[peah-webrtc, 2015/08/12 20:25:52]

Done.

+#ifndef WEBRTC_AEC_DEBUG_DUMP
+
+// Dump wav data to file

[hlundin-webrtc, 2015/08/10 13:04:41]

End with .
(On all comments.)

[peah-webrtc, 2015/08/12 20:25:51]

Done.

+#define WEBRTC_AEC_DEBUG_WAV_WRITE(file, data, numSamples)

[Andrew MacDonald, 2015/08/10 15:25:55]

I know this creates inconsistencies, but we've been using the prefix RTC_ rather
than WEBRTC_ for new precompiler definitions. You could do that here as well if
you want.

[peah-webrtc, 2015/08/12 20:25:52]

Done.

+
+// (Re)open wav file for writing using the specified sample rate

[Andrew MacDonald, 2015/08/10 15:25:55]

Just add all these comments once on the implemented version.

[peah-webrtc, 2015/08/12 20:25:52]

Done.

+#define WEBRTC_AEC_DEBUG_WAV_REOPEN(wavFile, name, seq1, seq2, sampleRate)

[kwiberg-webrtc, 2015/08/10 20:19:57]

Macros that should expand to empty statements should expand to something like
"do {} while (0)". Otherwise, the compiler won't catch missing semicolons, which
can be bad:

  if (strange)
    WARN(12)
  if (!permission)
    return -1;
  PrivilegedOperation();

[peah-webrtc, 2015/08/12 20:25:52]

Done!
Did I do it right now? One concern I have is whether the compiler will always be
able to optimize that so that the code is removed in the build. But it should
always be able to I guess.

[kwiberg-webrtc, 2015/08/13 07:59:34]

Looks good now. And yes, do {} while (0) really ought to produce no instructions
at all.

+
+// Close wav file
+#define WEBRTC_AEC_DEBUG_WAV_CLOSE(wavFile)
+
+// Dump raw data to file
+#define WEBRTC_AEC_DEBUG_RAW_WRITE(file, data, dataSize)
+
+// Open raw data file for writing using the specified sample rate
+#define WEBRTC_AEC_DEBUG_RAW_OPEN(file, name, instanceCtr)
+
+// Close raw data file
+#define WEBRTC_AEC_DEBUG_RAW_CLOSE(file)
+
+#else  // WEBRTC_AEC_DEBUG_DUMP
+
+// Dump wav data to file
+#define WEBRTC_AEC_DEBUG_WAV_WRITE(file, data, numSamples)  \
+  do {                                                      \
+    rtc_WavWriteSamples(file, data, numSamples);            \

[hlundin-webrtc, 2015/08/10 13:04:41]

Parentheses around the parameters, like you have below.

[kwiberg-webrtc, 2015/08/10 20:19:57]

Can you give an example invocation of the macro where this would be necessary? I
think the commas and parentheses of a function call have sufficiently low
"precedence" that you won't find any. I'm not 100% sure, though, so if you have
such an example I'd love to see it, but otherwise the parentheses are
unnecessary here (and below).

[hlundin-webrtc, 2015/08/12 11:48:18]

No, I can not. It was just a knee-jerk reaction from having a lot of arithmetic
macros. Anyway, make all look the same.

[peah-webrtc, 2015/08/12 20:25:51]

Please see comment below!

[peah-webrtc, 2015/08/12 20:25:52]

I think you are both correct. I've been taught to always use parentheses as a
rule when using macro inputs to avoid unwanted macro behavior and to be
consistent.
However, I think that in this case, neither of the macros need those
parentheses, so I could remove them in all places.

For now, I added them to the missing place. But if you want, I'd be happy to
remove them! Pls let me know what you think!

[peah-webrtc, 2015/08/12 20:25:52]

Acknowledged.

[kwiberg-webrtc, 2015/08/13 07:59:34]

I'd be happier if you removed them, since they clutter up the code. Since
shotgunning the code with parentheses doesn't protect you from all the mistakes
you can do with macro parameter expansion anyway, it makes little sense to
pretend this one simple rule can save you from having to pay close attention.

[peah-webrtc, 2015/08/13 13:37:39]

Done.

+  } while (0);

[kwiberg-webrtc, 2015/08/10 20:19:56]

Better to leave the semicolon out of the macro definition. That way, the
compiler will force the caller to write her own semicolon, preventing
obscenities such as

  if (bored)
    LOG()
  TheNextThing();

[peah-webrtc, 2015/08/12 20:25:51]

Great point!

[peah-webrtc, 2015/08/12 20:25:52]

Done.

+
+// (Re)open wav file for writing using the specified sample rate
+#define WEBRTC_AEC_DEBUG_WAV_REOPEN(wavFile, name, seq1, seq2, sampleRate) \
+  do {                                                                  \
+    WebRtcAec_ReopenWav((wavFile), (name), (seq1), (seq2), (sampleRate)); \
+  } while (0);
+
+// Close wav file
+#define WEBRTC_AEC_DEBUG_WAV_CLOSE(wavFile)     \
+  do {                                          \
+    rtc_WavClose((wavFile));                    \
+  } while (0);
+
+// Dump raw data to file
+#define WEBRTC_AEC_DEBUG_RAW_WRITE(file, data, dataSize)  \
+  do {                                                    \
+    (void)fwrite((data), (dataSize) , 1, (file));         \
+  } while (0);
+
+// Open raw data file for writing using the specified sample rate
+#define WEBRTC_AEC_DEBUG_RAW_OPEN(file, name, instanceCtr)  \
+  do {                                                      \
+    WebRtcAec_RawFileOpen((file), (name), (instanceCtr));   \
+  } while (0);
+
+

[Andrew MacDonald, 2015/08/10 15:25:54]

Remove extra blank line.

[peah-webrtc, 2015/08/12 20:25:52]

Done.

+// Close raw data file
+#define WEBRTC_AEC_DEBUG_RAW_CLOSE(file)        \
+  do {                                          \
+    fclose((file));                             \
+  } while (0);
+
+
+

[Andrew MacDonald, 2015/08/10 15:25:54]

Remove two extra blank lines.

[peah-webrtc, 2015/08/12 20:25:52]

Done.

+#endif  // WEBRTC_AEC_DEBUG_DUMP
+
+#ifdef __cplusplus
+}
+#endif
+
+#endif  // WEBRTC_MODULES_AUDIO_PROCESSING_AEC_AEC_LOGGING_

[kwiberg-webrtc, 2015/08/10 20:19:57]

Hmm. Suggestion: Instead of 

#ifdef WEBRTC_AEC_DEBUG_DUMP
// Dumps x.
#define DUMP(x) do { do_dump(x); } while (0)
// Jumps to x.
#define JUMP(x) do { do_jump(x); } while (0)
#else
// Dumps x.
#define DUMP(x) do {} while (0)
// Jumps to x.
#define JUMP(x) do {} while (0)
#endif

why not use inline functions:

// Dumps x.
static inline void dump(int x) {
#ifdef WEBRTC_AEC_DEBUG_DUMP
  do_dump(x);
#endif
}

// Jumps to x.
static inline void jump(HyperspaceCoordinate x) {
#ifdef WEBRTC_AEC_DEBUG_DUMP
  do_jump(x);
#endif
}

Callers will get better error messages, you won't have to worry about macro
parameter expansion, and you won't have to keep two sets of declarations and
comments in sync. Also, no line-continuation backslashes!

[peah-webrtc, 2015/08/12 20:25:52]

Would it be guaranteed that those inline function calls be discarded by the
compiler if they are empty? It is up to the compiler to decide, right? It would
be nice to be able to even put the log function calls inside nested loops and
then I would not want to risk a function call in that place. But apart from that
it for sure sounds as a much more preferred solution! 

[kwiberg-webrtc, 2015/08/13 07:59:34]

There's no guarantee that the compiler will actually inline the function call,
and no guarantee that an empty inlined function will cause no code to be
generated. But as a practical matter, you can usually count on the compiler to
get this right---it's not that hard, and it help greatly for almost all
programs.

The style guide's advice on the topic is, "Be very cautious with macros. Prefer
inline functions, enums, and const variables to macros."

IMO, you should use inline functions instead of macros since there's no reason
not to in this case. If you suspect the inline functions will perform poorly
compared to macros, that would count as a reason if you measured both approaches
and confirmed your suspicion.

[peah-webrtc, 2015/08/13 13:37:39]

I think that you are correct in that all compilers should/probably will manage
that. And I'm definitely not a macro fan in general so I agree with the style
guide as well.
However, as the inlining and optimization of the empty functions is actually up
to the compiler and not something we can rely fully on, and as it is not
possible to verify (in general) how the compiler deals with this, I really think
we should use macros in this case. Otherwise that would cause people (==me at
least) to hesitate to put logpoints into critical places such as deeply into
nested loops and that would unnecessary limit the applicability of the logging
functionality.

[peah-webrtc, 2015/08/17 09:36:16]

After an offline discussion, it was concluded that in order to avoid #ifdefs
while using inline functions, a proper C++ class implementation is required for
this. 
The idea is therefore to
1) For now, continue using macros as in the latest patch (#5).
2) After this CL, begin work on a class that encapsulates the recording
functionality and file pointers, avoiding the use of #ifdefs from the product
code to the logging code, and allowing for inline functions to be used instead
of macros. The idea is then to replace the macro calls with the new function
calls.

Are everyone ok with that setup/way forward?

[kwiberg-webrtc, 2015/08/17 09:43:07]

Yes. :-)

[Andrew MacDonald, 2015/08/17 16:37:05]

Yes.