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

webrtc/modules/audio_processing/logging/aec_logging_file_handling.c

+/*

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

Why is this a C and not a C++ file? If it was C++, you could use DCHECK.

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

Do you mean I should rename it to aec_logging_file_handling.cc ? I'm happy with
that, my only concern was that it was (initially) meant to be used  from .c
files beneath aec/, and that those due to the .c extension would be built using
a C compiler.
Do we always build using the C++ compiler (regardless of code being in a .c
file)? In that case I'll happily change the name if everyone is ok with that.

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

Yes.
We compile as C or C++ depending on the file extension. But a C++ compilation
unit can easily make its functions callable from C by declaring them 'extern
"C"'. Having a C++ interface implies being implemented in C++, but the reverse
is not true.

We have a lot of C files that would be slightly better off as C++ files, but it
takes a little bit of effort to do the conversion, mostly because the C++
compiler warns about some things the C compiler doesn't. Thus it makes sense to
do the conversion if it would make the present CL simpler. And since this is a
new file, it makes sense to make it C++ from the beginning.

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

Done.

+ *  Copyright (c) 2015 The WebRTC project authors. All Rights Reserved.
+ *
+ *  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.
+ */
+
+
+#ifdef WEBRTC_AEC_DEBUG_DUMP

[the sun, 2015/08/11 08:48:26]

The includes don't need to be within conditionals (here and elsewhere).

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

Done.

+#include <assert.h>
+#include <stdio.h>
+#include <stdint.h>

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

nit: blank line after this.

stdint.h before stdio.h.

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

Done.

+#include "webrtc/common_audio/wav_file.h"
+#include "webrtc/typedefs.h"
+#include "aec_logging_file_handling.h"

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

Full path.

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

Done.

+#endif
+
+// To enable AEC logging, run this command from trunk/ :
+// python webrtc/build/gyp_webrtc -Daec_debug_dump=1
+#ifdef WEBRTC_AEC_DEBUG_DUMP

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

Lines 19 and 23 cancel each other out. (In other words, can't you just let one
#ifdef cover the whole file?)

But in cases where an entire compilation unit is present or not conditional on a
preprocessor constant, it might be cleaner to use a condition in the .gyp/.gn
files instead.

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

Thanks! I removed the first #ifdef pair, so it is now fixed.

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

Done.

+// Open a new Wav file for writing. If it was already open with a different
+// sample frequency, close it first.

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

Move this documentation to the header. In particular, the user wants to know
under which conditions the file will be reopened. Not needed here.

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

Done.

+void WebRtcAec_ReopenWav(rtc_WavWriter** wav_file,
+                         const char* const name,

[the sun, 2015/08/11 08:48:26]

No need to make the pointer const, just the stuff pointed to.

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

Done.

+                         const int seq1,
+                         const int seq2,

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

Better names than seq1, seq2. It's not clear what these are. Perhaps
instance_counter and process_rate?

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

Fully agree on that! This was the original code that I just moved from
aec_core.c. But of course I should have changed that immediately.

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

Done.

+                         const int sample_rate) {

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

Drop const from by-value parameters.

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

Done.

+  int written ATTRIBUTE_UNUSED;
+  char filename[64];
+  if (*wav_file) {
+    if (rtc_WavSampleRate(*wav_file) == sample_rate)
+      return;
+    rtc_WavClose(*wav_file);
+  }
+  written = snprintf(filename, sizeof(filename), "%s%d-%d.wav",
+                     name, seq1, seq2);
+  assert(written >= 0);                        // no output error
+  assert((size_t)written < sizeof(filename));  // buffer was large enough

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

Capitals and periods (and below).

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

I'm not sure what you refer to. Could you please be more specific?

[hlundin-webrtc, 2015/08/13 06:19:02]

He refers to the somewhat picky style rule saying that comments should be "as
readable as narrative text, with proper capitalization and punctuation". While
the guide also says that "shorter comments, such as comments at the end of a
line of code, can sometimes be less formal, but you should be consistent with
your style", I think it is safe to say that WebRTC style is to be formal about
those as well.
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Punctuation,_...

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

This is probably the part of our coding style that I disagree with the most. It
just makes no sense to require full sentences in end-of-line comments. However,
it also doesn't make much sense to rebel against it when it's the existing
style, so I mostly just grumble and move on when I encounter this issue. But one
day, when the revolution comes, we'll take to the streets armed with sharpened
perl and sed scripts, and it shall be glorious!

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

Done.

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

Acknowledged.

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

Done.

+  *wav_file = rtc_WavOpen(filename, sample_rate, 1);
+}
+
+
+// Open a new file for writing, named with a instance specific
+// suffix

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

Move this to the header.

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

Done.

+void WebRtcAec_RawFileOpen(FILE** file,
+                           const char * const name,
+                           const int16_t instanceCtr) {

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

Drop const and instance_counter

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

Done.

+int written ATTRIBUTE_UNUSED;

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

Indent.

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

Done.

+  char filename[64];
+
+  written = sprintf(filename, "%s_%d.dat", name, instanceCtr);
+  assert(written >= 0);                        // no output error
+  assert((size_t)written < sizeof(filename));  // buffer was large enough

[the sun, 2015/08/11 08:48:26]

We should never knowingly introduce buffer overruns, even if this code isn't
intended to be in release builds. Take a look at webrtc/base/stringutils.h for
portable versions of C string functions.

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

As far as I can see, the functions in stringutils.h uses templates and therefore
cannot be used from C without wrapping (I think). For now I changed to use
snprintf instead and once starting to implement the wrapped C++ variant I'll
instead transition to using the functions stringutils.h. Does that make sense?

[the sun, 2015/08/24 11:04:46]

The implementation is C++ now, correct? Can you use stringutils.h instead?

[peah-webrtc, 2015/08/25 05:05:28]

Done.

+
+  *file = fopen(filename, "wb");
+}
+
+
+#endif  // WEBRTC_AEC_DEBUG_DUMP