Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1272403003:
Replaced the wav file dumping functionality in aec_core.c with the newly added corresponding macros (Closed)
|
Created:
5 years, 4 months ago by peah-webrtc Modified:
5 years, 3 months ago CC:
webrtc-reviews_webrtc.org, Andrew MacDonald, tterriberry_mozilla.com, kwiberg-webrtc, aluebs-webrtc, bjornv1 Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionAdded logging using the raw variant of the new aec logging macros
Replaced the wav file dumping functionality in aec_core.c with the newly added corresponding macros
Added macros for logging of AEC internal data
BUG=
Committed: https://crrev.com/9e69abf85eeae8b6e23170c189cf5dd155771a51
Cr-Commit-Position: refs/heads/master@{#9808}
Patch Set 1 #Patch Set 2 : Added logging using the raw variant of the new aec logging macros #
Total comments: 41
Patch Set 3 : Added support for inclusion from C++ code #
Total comments: 98
Patch Set 4 : Code update according to reviewer comments #
Total comments: 2
Patch Set 5 : Updates according to comments. Also added BUILD.gn that were previously missing from the changeset #
Total comments: 7
Patch Set 6 : Introduced DCHECKs, moved to using safe functions from stringutils.h, changed to static_casts inste… #
Total comments: 20
Patch Set 7 : Updates according to comments #Patch Set 8 : Updated the implementation of the extern C declaration #
Messages
Total messages: 35 (4 generated)
peah@webrtc.org changed reviewers: + andrew@webrtc.org, henrik.lundin@webrtc.org, kwiberg@webrtc.org
In order to be able to log internal data in the AEC I created and added a set of macros. In the same spirit, I created and replaced the function calls in aec_core.c for dumping data to wav files with a similar set of macros. I also added an example of how the non-wav file data logging macros are to be used. The main motivation by this code change is to be able to add permanent logpoints in the code that do not go into the normal binaries without cluttering the code (in the form of more #defines than necessary). I also wanted as far as possible to separate debug specific code, such as the ReopenWav function, from the non-debug code. The permanent logpoints lives/evolves together with the code and will hence simplify bug analysis by allowing more permanent bug-analysis scripts to be created and used.
peah@webrtc.org changed reviewers: + solenberg@webrtc.org
Nice. Please, see comments inline. https://codereview.webrtc.org/1272403003/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/logging/aec_logging_file_handling.c (right): https://codereview.webrtc.org/1272403003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/logging/aec_logging_file_handling.c:15: #include <stdint.h> Sort includes alphabetically. stdint < stdio https://codereview.webrtc.org/1272403003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/logging/aec_logging_file_handling.c:16: #include "webrtc/common_audio/wav_file.h" I usually insert a blank line between system files and project files. https://codereview.webrtc.org/1272403003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/logging/aec_logging_file_handling.c:18: #include "aec_logging_file_handling.h" Use full path. https://codereview.webrtc.org/1272403003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/logging/aec_logging_file_handling.c:21: // To enable AEC logging, run this command from trunk/ : This comment is the same as in the .h file. Function comments at the declaration should describe what the function does. Function comments at the definition should (if necessary) describe how it does it. http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Function_Comm... https://codereview.webrtc.org/1272403003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/logging/aec_logging_file_handling.c:26: void WebRtcAec_ReopenWav(rtc_WavWriter** wav_file, wav_file is both input and output. Input-only goes before other parameters. http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Function_Para... https://codereview.webrtc.org/1272403003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/logging/aec_logging_file_handling.c:51: int written ATTRIBUTE_UNUSED; Indentation. (Run "git cl format" on your patch.) https://codereview.webrtc.org/1272403003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/logging/aec_logging_file_handling.c:54: written = sprintf(filename, "%s_%d.dat", name, instanceCtr); snprintf? https://codereview.webrtc.org/1272403003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/logging/aec_logging_file_handling.c:58: *file = fopen(filename, "wb"); Why not return a FILE* instead, and avoid the FILE** parameter? https://codereview.webrtc.org/1272403003/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/logging/aec_logging_file_handling.h (right): https://codereview.webrtc.org/1272403003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/logging/aec_logging_file_handling.h:15: // python webrtc/build/gyp_webrtc -Daec_debug_dump=1 I would rephrase this as: To enable AEC logging, invoke GYP with -Daec_debug_dump=1. https://codereview.webrtc.org/1272403003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/logging/aec_logging_file_handling.h:22: // (Re)open wav file using the specified sample rate (Re)opens... http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Function_Comm...: 'comments should be descriptive ("Opens the file") rather than imperative ("Open the file")' https://codereview.webrtc.org/1272403003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/logging/aec_logging_file_handling.h:22: // (Re)open wav file using the specified sample rate End comment with period (.) http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Punctuation,_... https://codereview.webrtc.org/1272403003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/logging/aec_logging_file_handling.h:22: // (Re)open wav file using the specified sample rate What do the seq1 and seq2 parameters do? https://codereview.webrtc.org/1272403003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/logging/aec_logging_file_handling.h:25: const int seq1, We typically don't qualify parameters passed by value as const. That is, simply write "int seq1", "int seq2", etc. https://codereview.webrtc.org/1272403003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/logging/aec_logging_file_handling.h:29: // Open dumpfile with instance-specific filename Opens... End with . https://codereview.webrtc.org/1272403003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/logging/aec_logging_file_handling.h:31: const char * const name, Connect the type with the *. const char* const name https://codereview.webrtc.org/1272403003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/logging/aec_logging_file_handling.h:32: const int16_t instanceCtr); Drop const. https://codereview.webrtc.org/1272403003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/logging/aec_logging_file_handling.h:32: const int16_t instanceCtr); Why int16_t? Almost always use int. http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Integer_Types https://codereview.webrtc.org/1272403003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/logging/aec_logging_file_handling.h:32: const int16_t instanceCtr); Variable names should be all lowercase, with underscores between words. http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Variable_Names https://codereview.webrtc.org/1272403003/diff/40001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/logging/aec_logging.h (right): https://codereview.webrtc.org/1272403003/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/logging/aec_logging.h:2: * Copyright (c) 2013 The WebRTC project authors. All Rights Reserved. 2015 https://codereview.webrtc.org/1272403003/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/logging/aec_logging.h:15: // To enable AEC logging, run this command from trunk/ : See my other suggestion in aec_logging_file_handling.h. https://codereview.webrtc.org/1272403003/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/logging/aec_logging.h:29: // Dump wav data to file End with . (On all comments.) https://codereview.webrtc.org/1272403003/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/logging/aec_logging.h:52: rtc_WavWriteSamples(file, data, numSamples); \ Parentheses around the parameters, like you have below.
Ah, I noticed I repeated a bunch of Henrik's comments since I was looking at a different patchset. https://codereview.webrtc.org/1272403003/diff/40001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec/aec_core.c (right): https://codereview.webrtc.org/1272403003/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec/aec_core.c:1570: "aec_eFFT", nit: "aec_efft" or "aec_e_fft" https://codereview.webrtc.org/1272403003/diff/40001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/logging/aec_logging.h (right): https://codereview.webrtc.org/1272403003/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/logging/aec_logging.h:13: Remove extra blank line. https://codereview.webrtc.org/1272403003/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/logging/aec_logging.h:26: Remove extra blank line. https://codereview.webrtc.org/1272403003/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/logging/aec_logging.h:30: #define WEBRTC_AEC_DEBUG_WAV_WRITE(file, data, numSamples) 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. https://codereview.webrtc.org/1272403003/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/logging/aec_logging.h:32: // (Re)open wav file for writing using the specified sample rate Just add all these comments once on the implemented version. https://codereview.webrtc.org/1272403003/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/logging/aec_logging.h:79: Remove extra blank line. https://codereview.webrtc.org/1272403003/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/logging/aec_logging.h:87: Remove two extra blank lines. https://codereview.webrtc.org/1272403003/diff/40001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/logging/aec_logging_file_handling.c (right): https://codereview.webrtc.org/1272403003/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/logging/aec_logging_file_handling.c:15: #include <stdint.h> nit: blank line after this. stdint.h before stdio.h. https://codereview.webrtc.org/1272403003/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/logging/aec_logging_file_handling.c:18: #include "aec_logging_file_handling.h" Full path. https://codereview.webrtc.org/1272403003/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/logging/aec_logging_file_handling.c:25: // sample frequency, close it first. Move this documentation to the header. In particular, the user wants to know under which conditions the file will be reopened. Not needed here. https://codereview.webrtc.org/1272403003/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/logging/aec_logging_file_handling.c:29: const int seq2, Better names than seq1, seq2. It's not clear what these are. Perhaps instance_counter and process_rate? https://codereview.webrtc.org/1272403003/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/logging/aec_logging_file_handling.c:30: const int sample_rate) { Drop const from by-value parameters. https://codereview.webrtc.org/1272403003/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/logging/aec_logging_file_handling.c:41: assert((size_t)written < sizeof(filename)); // buffer was large enough Capitals and periods (and below). https://codereview.webrtc.org/1272403003/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/logging/aec_logging_file_handling.c:47: // suffix Move this to the header. https://codereview.webrtc.org/1272403003/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/logging/aec_logging_file_handling.c:50: const int16_t instanceCtr) { Drop const and instance_counter https://codereview.webrtc.org/1272403003/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/logging/aec_logging_file_handling.c:51: int written ATTRIBUTE_UNUSED; Indent. https://codereview.webrtc.org/1272403003/diff/40001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/logging/aec_logging_file_handling.h (right): https://codereview.webrtc.org/1272403003/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/logging/aec_logging_file_handling.h:15: // python webrtc/build/gyp_webrtc -Daec_debug_dump=1 I would instead just say: "To enable AEC logging, enable the gyp variable aec_debug_dump=1." There are multiple ways to enable gyp variables. https://codereview.webrtc.org/1272403003/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/logging/aec_logging_file_handling.h:17: #include <stdio.h> nit: blank line after this. https://codereview.webrtc.org/1272403003/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/logging/aec_logging_file_handling.h:41: Remove blank line.
https://codereview.webrtc.org/1272403003/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/logging/aec_logging_file_handling.c (right): https://codereview.webrtc.org/1272403003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/logging/aec_logging_file_handling.c:26: void WebRtcAec_ReopenWav(rtc_WavWriter** wav_file, On 2015/08/10 13:04:40, hlundin-webrtc wrote: > wav_file is both input and output. Input-only goes before other parameters. > http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Function_Para... Agreed. I actually would prefer returning a rtc_WavWriter* (and a FILE* below). https://codereview.webrtc.org/1272403003/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/logging/aec_logging_file_handling.h (right): https://codereview.webrtc.org/1272403003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/logging/aec_logging_file_handling.h:15: // python webrtc/build/gyp_webrtc -Daec_debug_dump=1 On 2015/08/10 13:04:40, hlundin-webrtc wrote: > I would rephrase this as: > To enable AEC logging, invoke GYP with -Daec_debug_dump=1. I like this better than my suggestion.
Looks good, but a few comments: https://codereview.webrtc.org/1272403003/diff/40001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/logging/aec_logging.h (right): https://codereview.webrtc.org/1272403003/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/logging/aec_logging.h:24: #endif You only define macros, so the extern "C" thingy isn't necessary. https://codereview.webrtc.org/1272403003/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/logging/aec_logging.h:33: #define WEBRTC_AEC_DEBUG_WAV_REOPEN(wavFile, name, seq1, seq2, sampleRate) 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(); https://codereview.webrtc.org/1272403003/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/logging/aec_logging.h:52: rtc_WavWriteSamples(file, data, numSamples); \ On 2015/08/10 13:04:41, hlundin-webrtc wrote: > Parentheses around the parameters, like you have below. 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). https://codereview.webrtc.org/1272403003/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/logging/aec_logging.h:53: } while (0); 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(); https://codereview.webrtc.org/1272403003/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/logging/aec_logging.h:94: #endif // WEBRTC_MODULES_AUDIO_PROCESSING_AEC_AEC_LOGGING_ 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! https://codereview.webrtc.org/1272403003/diff/40001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/logging/aec_logging_file_handling.c (right): https://codereview.webrtc.org/1272403003/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/logging/aec_logging_file_handling.c:1: /* Why is this a C and not a C++ file? If it was C++, you could use DCHECK. https://codereview.webrtc.org/1272403003/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/logging/aec_logging_file_handling.c:23: #ifdef WEBRTC_AEC_DEBUG_DUMP 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. https://codereview.webrtc.org/1272403003/diff/40001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/logging/aec_logging_file_handling.h (right): https://codereview.webrtc.org/1272403003/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/logging/aec_logging_file_handling.h:32: const int sample_rate); Marking parameters const in function declarations is just noise to callers, so there's little point in doing it. (But not entirely pointless. IIRC, gcc and clang will let you have the consts in the definition but not in the declaration, which is what you'd want, but Visual C++ complains about that. Still, they're not generally worth it.) https://codereview.webrtc.org/1272403003/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/logging/aec_logging_file_handling.h:36: const char * const name, In the night sky, stars appear surrounded by empty space on all sides, but that's just because they're far enough away that the light that you see was emitted before our style guide went into effect. https://codereview.webrtc.org/1272403003/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/logging/aec_logging_file_handling.h:37: const int16_t instanceCtr); Why not int? (That's what you always should pick unless you have a good reason.)
https://codereview.webrtc.org/1272403003/diff/40001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec/aec_core.c (right): https://codereview.webrtc.org/1272403003/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec/aec_core.c:35: #include "webrtc/modules/audio_processing/logging/aec_logging.h" include order https://codereview.webrtc.org/1272403003/diff/40001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/logging/aec_logging_file_handling.c (right): https://codereview.webrtc.org/1272403003/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/logging/aec_logging_file_handling.c:12: #ifdef WEBRTC_AEC_DEBUG_DUMP The includes don't need to be within conditionals (here and elsewhere). https://codereview.webrtc.org/1272403003/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/logging/aec_logging_file_handling.c:27: const char* const name, No need to make the pointer const, just the stuff pointed to. https://codereview.webrtc.org/1272403003/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/logging/aec_logging_file_handling.c:56: assert((size_t)written < sizeof(filename)); // buffer was large enough 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.
(Two drafts I forgot to send.) https://codereview.webrtc.org/1272403003/diff/40001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/logging/aec_logging.h (right): https://codereview.webrtc.org/1272403003/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/logging/aec_logging.h:52: rtc_WavWriteSamples(file, data, numSamples); \ On 2015/08/10 20:19:57, kwiberg-webrtc wrote: > On 2015/08/10 13:04:41, hlundin-webrtc wrote: > > Parentheses around the parameters, like you have below. > > 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). No, I can not. It was just a knee-jerk reaction from having a lot of arithmetic macros. Anyway, make all look the same. https://codereview.webrtc.org/1272403003/diff/40001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/logging/aec_logging_file_handling.h (right): https://codereview.webrtc.org/1272403003/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/logging/aec_logging_file_handling.h:37: const int16_t instanceCtr); On 2015/08/10 20:19:57, kwiberg-webrtc wrote: > Why not int? (That's what you always should pick unless you have a good reason.) Acknowledged.
Thanks for the awesome reviews!!! I have accepted almost all changes and made the corresponding changes. https://codereview.webrtc.org/1272403003/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/logging/aec_logging_file_handling.c (right): https://codereview.webrtc.org/1272403003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/logging/aec_logging_file_handling.c:15: #include <stdint.h> On 2015/08/10 13:04:40, hlundin-webrtc wrote: > Sort includes alphabetically. stdint < stdio Done. https://codereview.webrtc.org/1272403003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/logging/aec_logging_file_handling.c:16: #include "webrtc/common_audio/wav_file.h" On 2015/08/10 13:04:40, hlundin-webrtc wrote: > I usually insert a blank line between system files and project files. Done. https://codereview.webrtc.org/1272403003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/logging/aec_logging_file_handling.c:18: #include "aec_logging_file_handling.h" On 2015/08/10 13:04:40, hlundin-webrtc wrote: > Use full path. Done. https://codereview.webrtc.org/1272403003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/logging/aec_logging_file_handling.c:21: // To enable AEC logging, run this command from trunk/ : On 2015/08/10 13:04:40, hlundin-webrtc wrote: > This comment is the same as in the .h file. > > Function comments at the declaration should describe what the function does. > Function comments at the definition should (if necessary) describe how it does > it. > > http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Function_Comm... Done. https://codereview.webrtc.org/1272403003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/logging/aec_logging_file_handling.c:26: void WebRtcAec_ReopenWav(rtc_WavWriter** wav_file, On 2015/08/10 13:04:40, hlundin-webrtc wrote: > wav_file is both input and output. Input-only goes before other parameters. > http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Function_Para... Done. https://codereview.webrtc.org/1272403003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/logging/aec_logging_file_handling.c:26: void WebRtcAec_ReopenWav(rtc_WavWriter** wav_file, On 2015/08/10 15:28:44, andrew_ooo_to_aug14 wrote: > On 2015/08/10 13:04:40, hlundin-webrtc wrote: > > wav_file is both input and output. Input-only goes before other parameters. > > > http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Function_Para... > > Agreed. I actually would prefer returning a rtc_WavWriter* (and a FILE* below). I agree that that would be preferrable. My concern with that, however, is that then the calls to the macros would need to be placed within #ifdef's as the receiving output variable cannot be removed from the code using the compiler flag specific #define selections that are used in aec_logging.h. (My goal with these macros was to allow permanent logpoints in the code that are not built into the released product and neither clutters the code with extra #ifdef's). I'm not that good with macros so maybe any of you knows a way to achieve this using macros with nonvoid return values? Is it ok to keep it as it is (no pointer return values)? https://codereview.webrtc.org/1272403003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/logging/aec_logging_file_handling.c:51: int written ATTRIBUTE_UNUSED; On 2015/08/10 13:04:40, hlundin-webrtc wrote: > Indentation. (Run "git cl format" on your patch.) Done. https://codereview.webrtc.org/1272403003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/logging/aec_logging_file_handling.c:54: written = sprintf(filename, "%s_%d.dat", name, instanceCtr); On 2015/08/10 13:04:40, hlundin-webrtc wrote: > snprintf? Done. https://codereview.webrtc.org/1272403003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/logging/aec_logging_file_handling.c:58: *file = fopen(filename, "wb"); On 2015/08/10 13:04:40, hlundin-webrtc wrote: > Why not return a FILE* instead, and avoid the FILE** parameter? I agree that would be a better solution, if only it could work without extra enclosing #ifdef's (pls see reply above). Is it ok to keep it as it is? https://codereview.webrtc.org/1272403003/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/logging/aec_logging_file_handling.h (right): https://codereview.webrtc.org/1272403003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/logging/aec_logging_file_handling.h:15: // python webrtc/build/gyp_webrtc -Daec_debug_dump=1 On 2015/08/10 13:04:40, hlundin-webrtc wrote: > I would rephrase this as: > To enable AEC logging, invoke GYP with -Daec_debug_dump=1. Done. https://codereview.webrtc.org/1272403003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/logging/aec_logging_file_handling.h:15: // python webrtc/build/gyp_webrtc -Daec_debug_dump=1 On 2015/08/10 15:28:44, andrew_ooo_to_aug14 wrote: > On 2015/08/10 13:04:40, hlundin-webrtc wrote: > > I would rephrase this as: > > To enable AEC logging, invoke GYP with -Daec_debug_dump=1. > > I like this better than my suggestion. Done. https://codereview.webrtc.org/1272403003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/logging/aec_logging_file_handling.h:22: // (Re)open wav file using the specified sample rate On 2015/08/10 13:04:40, hlundin-webrtc wrote: > (Re)opens... > > http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Function_Comm...: > 'comments should be descriptive ("Opens the file") rather than imperative ("Open > the file")' Done. https://codereview.webrtc.org/1272403003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/logging/aec_logging_file_handling.h:22: // (Re)open wav file using the specified sample rate On 2015/08/10 13:04:41, hlundin-webrtc wrote: > What do the seq1 and seq2 parameters do? Done. (In response to another comment I changed those names to be more descriptive). https://codereview.webrtc.org/1272403003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/logging/aec_logging_file_handling.h:22: // (Re)open wav file using the specified sample rate On 2015/08/10 13:04:41, hlundin-webrtc wrote: > End comment with period (.) > http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Punctuation,_... Done. https://codereview.webrtc.org/1272403003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/logging/aec_logging_file_handling.h:25: const int seq1, On 2015/08/10 13:04:41, hlundin-webrtc wrote: > We typically don't qualify parameters passed by value as const. That is, simply > write "int seq1", "int seq2", etc. Done. https://codereview.webrtc.org/1272403003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/logging/aec_logging_file_handling.h:29: // Open dumpfile with instance-specific filename On 2015/08/10 13:04:41, hlundin-webrtc wrote: > Opens... > End with . Done. https://codereview.webrtc.org/1272403003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/logging/aec_logging_file_handling.h:31: const char * const name, On 2015/08/10 13:04:41, hlundin-webrtc wrote: > Connect the type with the *. > const char* const name Done. https://codereview.webrtc.org/1272403003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/logging/aec_logging_file_handling.h:32: const int16_t instanceCtr); On 2015/08/10 13:04:40, hlundin-webrtc wrote: > Variable names should be all lowercase, with underscores between words. > http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Variable_Names Done. https://codereview.webrtc.org/1272403003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/logging/aec_logging_file_handling.h:32: const int16_t instanceCtr); On 2015/08/10 13:04:41, hlundin-webrtc wrote: > Drop const. Done. https://codereview.webrtc.org/1272403003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/logging/aec_logging_file_handling.h:32: const int16_t instanceCtr); On 2015/08/10 13:04:41, hlundin-webrtc wrote: > Why int16_t? Almost always use int. > http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Integer_Types Done. https://codereview.webrtc.org/1272403003/diff/40001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec/aec_core.c (right): https://codereview.webrtc.org/1272403003/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec/aec_core.c:35: #include "webrtc/modules/audio_processing/logging/aec_logging.h" On 2015/08/11 08:48:26, the sun wrote: > include order Done. https://codereview.webrtc.org/1272403003/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec/aec_core.c:1570: "aec_eFFT", On 2015/08/10 15:25:54, andrew_ooo_to_aug14 wrote: > nit: "aec_efft" or "aec_e_fft" Done. https://codereview.webrtc.org/1272403003/diff/40001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/logging/aec_logging.h (right): https://codereview.webrtc.org/1272403003/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/logging/aec_logging.h:2: * Copyright (c) 2013 The WebRTC project authors. All Rights Reserved. On 2015/08/10 13:04:41, hlundin-webrtc wrote: > 2015 Done. https://codereview.webrtc.org/1272403003/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/logging/aec_logging.h:13: On 2015/08/10 15:25:55, andrew_ooo_to_aug14 wrote: > Remove extra blank line. Done. https://codereview.webrtc.org/1272403003/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/logging/aec_logging.h:15: // To enable AEC logging, run this command from trunk/ : On 2015/08/10 13:04:41, hlundin-webrtc wrote: > See my other suggestion in aec_logging_file_handling.h. Done. https://codereview.webrtc.org/1272403003/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/logging/aec_logging.h:24: #endif On 2015/08/10 20:19:57, kwiberg-webrtc wrote: > You only define macros, so the extern "C" thingy isn't necessary. Done. https://codereview.webrtc.org/1272403003/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/logging/aec_logging.h:26: On 2015/08/10 15:25:55, andrew_ooo_to_aug14 wrote: > Remove extra blank line. Done. https://codereview.webrtc.org/1272403003/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/logging/aec_logging.h:29: // Dump wav data to file On 2015/08/10 13:04:41, hlundin-webrtc wrote: > End with . > (On all comments.) Done. https://codereview.webrtc.org/1272403003/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/logging/aec_logging.h:30: #define WEBRTC_AEC_DEBUG_WAV_WRITE(file, data, numSamples) On 2015/08/10 15:25:55, andrew_ooo_to_aug14 wrote: > 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. Done. https://codereview.webrtc.org/1272403003/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/logging/aec_logging.h:32: // (Re)open wav file for writing using the specified sample rate On 2015/08/10 15:25:55, andrew_ooo_to_aug14 wrote: > Just add all these comments once on the implemented version. Done. https://codereview.webrtc.org/1272403003/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/logging/aec_logging.h:33: #define WEBRTC_AEC_DEBUG_WAV_REOPEN(wavFile, name, seq1, seq2, sampleRate) On 2015/08/10 20:19:57, kwiberg-webrtc wrote: > 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(); 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. https://codereview.webrtc.org/1272403003/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/logging/aec_logging.h:52: rtc_WavWriteSamples(file, data, numSamples); \ On 2015/08/10 13:04:41, hlundin-webrtc wrote: > Parentheses around the parameters, like you have below. Acknowledged. https://codereview.webrtc.org/1272403003/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/logging/aec_logging.h:52: rtc_WavWriteSamples(file, data, numSamples); \ On 2015/08/10 20:19:57, kwiberg-webrtc wrote: > On 2015/08/10 13:04:41, hlundin-webrtc wrote: > > Parentheses around the parameters, like you have below. > > 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). Please see comment below! https://codereview.webrtc.org/1272403003/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/logging/aec_logging.h:52: rtc_WavWriteSamples(file, data, numSamples); \ On 2015/08/12 11:48:18, hlundin-webrtc wrote: > On 2015/08/10 20:19:57, kwiberg-webrtc wrote: > > On 2015/08/10 13:04:41, hlundin-webrtc wrote: > > > Parentheses around the parameters, like you have below. > > > > 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). > > No, I can not. It was just a knee-jerk reaction from having a lot of arithmetic > macros. Anyway, make all look the same. 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! https://codereview.webrtc.org/1272403003/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/logging/aec_logging.h:53: } while (0); On 2015/08/10 20:19:56, kwiberg-webrtc wrote: > 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(); Great point! https://codereview.webrtc.org/1272403003/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/logging/aec_logging.h:53: } while (0); On 2015/08/10 20:19:56, kwiberg-webrtc wrote: > 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(); Done. https://codereview.webrtc.org/1272403003/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/logging/aec_logging.h:79: On 2015/08/10 15:25:54, andrew_ooo_to_aug14 wrote: > Remove extra blank line. Done. https://codereview.webrtc.org/1272403003/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/logging/aec_logging.h:87: On 2015/08/10 15:25:54, andrew_ooo_to_aug14 wrote: > Remove two extra blank lines. Done. https://codereview.webrtc.org/1272403003/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/logging/aec_logging.h:94: #endif // WEBRTC_MODULES_AUDIO_PROCESSING_AEC_AEC_LOGGING_ On 2015/08/10 20:19:57, kwiberg-webrtc wrote: > 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! 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! https://codereview.webrtc.org/1272403003/diff/40001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/logging/aec_logging_file_handling.c (right): https://codereview.webrtc.org/1272403003/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/logging/aec_logging_file_handling.c:1: /* On 2015/08/10 20:19:57, kwiberg-webrtc wrote: > Why is this a C and not a C++ file? If it was C++, you could use DCHECK. 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. https://codereview.webrtc.org/1272403003/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/logging/aec_logging_file_handling.c:12: #ifdef WEBRTC_AEC_DEBUG_DUMP On 2015/08/11 08:48:26, the sun wrote: > The includes don't need to be within conditionals (here and elsewhere). Done. https://codereview.webrtc.org/1272403003/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/logging/aec_logging_file_handling.c:15: #include <stdint.h> On 2015/08/10 15:25:55, andrew_ooo_to_aug14 wrote: > nit: blank line after this. > > stdint.h before stdio.h. Done. https://codereview.webrtc.org/1272403003/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/logging/aec_logging_file_handling.c:18: #include "aec_logging_file_handling.h" On 2015/08/10 15:25:55, andrew_ooo_to_aug14 wrote: > Full path. Done. https://codereview.webrtc.org/1272403003/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/logging/aec_logging_file_handling.c:23: #ifdef WEBRTC_AEC_DEBUG_DUMP On 2015/08/10 20:19:57, kwiberg-webrtc wrote: > 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. Thanks! I removed the first #ifdef pair, so it is now fixed. https://codereview.webrtc.org/1272403003/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/logging/aec_logging_file_handling.c:23: #ifdef WEBRTC_AEC_DEBUG_DUMP On 2015/08/10 20:19:57, kwiberg-webrtc wrote: > 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. Done. https://codereview.webrtc.org/1272403003/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/logging/aec_logging_file_handling.c:25: // sample frequency, close it first. On 2015/08/10 15:25:55, andrew_ooo_to_aug14 wrote: > Move this documentation to the header. In particular, the user wants to know > under which conditions the file will be reopened. Not needed here. Done. https://codereview.webrtc.org/1272403003/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/logging/aec_logging_file_handling.c:27: const char* const name, On 2015/08/11 08:48:26, the sun wrote: > No need to make the pointer const, just the stuff pointed to. Done. https://codereview.webrtc.org/1272403003/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/logging/aec_logging_file_handling.c:29: const int seq2, On 2015/08/10 15:25:55, andrew_ooo_to_aug14 wrote: > Better names than seq1, seq2. It's not clear what these are. Perhaps > instance_counter and process_rate? 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. https://codereview.webrtc.org/1272403003/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/logging/aec_logging_file_handling.c:29: const int seq2, On 2015/08/10 15:25:55, andrew_ooo_to_aug14 wrote: > Better names than seq1, seq2. It's not clear what these are. Perhaps > instance_counter and process_rate? Done. https://codereview.webrtc.org/1272403003/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/logging/aec_logging_file_handling.c:30: const int sample_rate) { On 2015/08/10 15:25:55, andrew_ooo_to_aug14 wrote: > Drop const from by-value parameters. Done. https://codereview.webrtc.org/1272403003/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/logging/aec_logging_file_handling.c:41: assert((size_t)written < sizeof(filename)); // buffer was large enough On 2015/08/10 15:25:55, andrew_ooo_to_aug14 wrote: > Capitals and periods (and below). I'm not sure what you refer to. Could you please be more specific? https://codereview.webrtc.org/1272403003/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/logging/aec_logging_file_handling.c:47: // suffix On 2015/08/10 15:25:55, andrew_ooo_to_aug14 wrote: > Move this to the header. Done. https://codereview.webrtc.org/1272403003/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/logging/aec_logging_file_handling.c:50: const int16_t instanceCtr) { On 2015/08/10 15:25:55, andrew_ooo_to_aug14 wrote: > Drop const and instance_counter Done. https://codereview.webrtc.org/1272403003/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/logging/aec_logging_file_handling.c:51: int written ATTRIBUTE_UNUSED; On 2015/08/10 15:25:55, andrew_ooo_to_aug14 wrote: > Indent. Done. https://codereview.webrtc.org/1272403003/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/logging/aec_logging_file_handling.c:56: assert((size_t)written < sizeof(filename)); // buffer was large enough On 2015/08/11 08:48:26, the sun wrote: > 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. 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? https://codereview.webrtc.org/1272403003/diff/40001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/logging/aec_logging_file_handling.h (right): https://codereview.webrtc.org/1272403003/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/logging/aec_logging_file_handling.h:15: // python webrtc/build/gyp_webrtc -Daec_debug_dump=1 On 2015/08/10 15:25:55, andrew_ooo_to_aug14 wrote: > I would instead just say: > "To enable AEC logging, enable the gyp variable aec_debug_dump=1." > > There are multiple ways to enable gyp variables. Done. https://codereview.webrtc.org/1272403003/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/logging/aec_logging_file_handling.h:17: #include <stdio.h> On 2015/08/10 15:25:55, andrew_ooo_to_aug14 wrote: > nit: blank line after this. Done. https://codereview.webrtc.org/1272403003/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/logging/aec_logging_file_handling.h:32: const int sample_rate); On 2015/08/10 20:19:57, kwiberg-webrtc wrote: > Marking parameters const in function declarations is just noise to callers, so > there's little point in doing it. (But not entirely pointless. IIRC, gcc and > clang will let you have the consts in the definition but not in the declaration, > which is what you'd want, but Visual C++ complains about that. Still, they're > not generally worth it.) Done. https://codereview.webrtc.org/1272403003/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/logging/aec_logging_file_handling.h:36: const char * const name, On 2015/08/10 20:19:57, kwiberg-webrtc wrote: > In the night sky, stars appear surrounded by empty space on all sides, but > that's just because they're far enough away that the light that you see was > emitted before our style guide went into effect. Wonderful comment! :-) (and of course you are right, will do the change :-) ) Done! https://codereview.webrtc.org/1272403003/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/logging/aec_logging_file_handling.h:36: const char * const name, On 2015/08/10 20:19:57, kwiberg-webrtc wrote: > In the night sky, stars appear surrounded by empty space on all sides, but > that's just because they're far enough away that the light that you see was > emitted before our style guide went into effect. Done. https://codereview.webrtc.org/1272403003/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/logging/aec_logging_file_handling.h:37: const int16_t instanceCtr); On 2015/08/10 20:19:57, kwiberg-webrtc wrote: > Why not int? (That's what you always should pick unless you have a good reason.) Done. https://codereview.webrtc.org/1272403003/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/logging/aec_logging_file_handling.h:41: On 2015/08/10 15:25:55, andrew_ooo_to_aug14 wrote: > Remove blank line. Done.
https://codereview.webrtc.org/1272403003/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/logging/aec_logging_file_handling.c (right): https://codereview.webrtc.org/1272403003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/logging/aec_logging_file_handling.c:26: void WebRtcAec_ReopenWav(rtc_WavWriter** wav_file, On 2015/08/12 20:25:51, peah-webrtc wrote: > On 2015/08/10 15:28:44, andrew_ooo_to_aug14 wrote: > > Agreed. I actually would prefer returning a rtc_WavWriter* (and a FILE* > below). > > I agree that that would be preferrable. My concern with that, however, is that > then the calls to the macros would need to be placed within #ifdef's as the > receiving output variable cannot be removed from the code using the compiler > flag specific #define selections that are used in aec_logging.h. (My goal with > these macros was to allow permanent logpoints in the code that are not built > into the released product and neither clutters the code with extra #ifdef's). > > I'm not that good with macros so maybe any of you knows a way to achieve this > using macros with nonvoid return values? > > Is it ok to keep it as it is (no pointer return values)? Ah, good point. Yes I think it's reasonable to keep it as-is.
https://codereview.webrtc.org/1272403003/diff/40001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/logging/aec_logging_file_handling.c (right): https://codereview.webrtc.org/1272403003/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/logging/aec_logging_file_handling.c:41: assert((size_t)written < sizeof(filename)); // buffer was large enough On 2015/08/12 20:25:52, peah-webrtc wrote: > On 2015/08/10 15:25:55, andrew_ooo_to_aug14 wrote: > > Capitals and periods (and below). > > I'm not sure what you refer to. Could you please be more specific? 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,_... https://codereview.webrtc.org/1272403003/diff/60001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/logging/aec_logging_file_handling.h (right): https://codereview.webrtc.org/1272403003/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/logging/aec_logging_file_handling.h:25: // Open a new Wav file for writing. If it was already open with a different Opens...
https://codereview.webrtc.org/1272403003/diff/40001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/logging/aec_logging.h (right): https://codereview.webrtc.org/1272403003/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/logging/aec_logging.h:33: #define WEBRTC_AEC_DEBUG_WAV_REOPEN(wavFile, name, seq1, seq2, sampleRate) On 2015/08/12 20:25:52, peah-webrtc wrote: > On 2015/08/10 20:19:57, kwiberg-webrtc wrote: > > 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(); > > 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. Looks good now. And yes, do {} while (0) really ought to produce no instructions at all. https://codereview.webrtc.org/1272403003/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/logging/aec_logging.h:52: rtc_WavWriteSamples(file, data, numSamples); \ On 2015/08/12 20:25:52, peah-webrtc wrote: > On 2015/08/12 11:48:18, hlundin-webrtc wrote: > > On 2015/08/10 20:19:57, kwiberg-webrtc wrote: > > > On 2015/08/10 13:04:41, hlundin-webrtc wrote: > > > > Parentheses around the parameters, like you have below. > > > > > > 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). > > > > No, I can not. It was just a knee-jerk reaction from having a lot of > arithmetic > > macros. Anyway, make all look the same. > > 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! 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. https://codereview.webrtc.org/1272403003/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/logging/aec_logging.h:94: #endif // WEBRTC_MODULES_AUDIO_PROCESSING_AEC_AEC_LOGGING_ On 2015/08/12 20:25:52, peah-webrtc wrote: > On 2015/08/10 20:19:57, kwiberg-webrtc wrote: > > 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! > > 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! 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. https://codereview.webrtc.org/1272403003/diff/40001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/logging/aec_logging_file_handling.c (right): https://codereview.webrtc.org/1272403003/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/logging/aec_logging_file_handling.c:1: /* On 2015/08/12 20:25:52, peah-webrtc wrote: > On 2015/08/10 20:19:57, kwiberg-webrtc wrote: > > Why is this a C and not a C++ file? If it was C++, you could use DCHECK. > > Do you mean I should rename it to aec_logging_file_handling.cc? Yes. > 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. 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. https://codereview.webrtc.org/1272403003/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/logging/aec_logging_file_handling.c:41: assert((size_t)written < sizeof(filename)); // buffer was large enough On 2015/08/13 06:19:02, hlundin-webrtc wrote: > On 2015/08/12 20:25:52, peah-webrtc wrote: > > On 2015/08/10 15:25:55, andrew_ooo_to_aug14 wrote: > > > Capitals and periods (and below). > > > > I'm not sure what you refer to. Could you please be more specific? > > 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,_... 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!
Updated according to comments. As far as I know, the only remaining thing is whether to use macros or inline functions. https://codereview.webrtc.org/1272403003/diff/40001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/logging/aec_logging.h (right): https://codereview.webrtc.org/1272403003/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/logging/aec_logging.h:52: rtc_WavWriteSamples(file, data, numSamples); \ On 2015/08/13 07:59:34, kwiberg-webrtc wrote: > On 2015/08/12 20:25:52, peah-webrtc wrote: > > On 2015/08/12 11:48:18, hlundin-webrtc wrote: > > > On 2015/08/10 20:19:57, kwiberg-webrtc wrote: > > > > On 2015/08/10 13:04:41, hlundin-webrtc wrote: > > > > > Parentheses around the parameters, like you have below. > > > > > > > > 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). > > > > > > No, I can not. It was just a knee-jerk reaction from having a lot of > > arithmetic > > > macros. Anyway, make all look the same. > > > > 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! > > 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. Done. https://codereview.webrtc.org/1272403003/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/logging/aec_logging.h:94: #endif // WEBRTC_MODULES_AUDIO_PROCESSING_AEC_AEC_LOGGING_ On 2015/08/13 07:59:34, kwiberg-webrtc wrote: > On 2015/08/12 20:25:52, peah-webrtc wrote: > > On 2015/08/10 20:19:57, kwiberg-webrtc wrote: > > > 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! > > > > 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! > > 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. 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. https://codereview.webrtc.org/1272403003/diff/40001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/logging/aec_logging_file_handling.c (right): https://codereview.webrtc.org/1272403003/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/logging/aec_logging_file_handling.c:1: /* On 2015/08/13 07:59:34, kwiberg-webrtc wrote: > On 2015/08/12 20:25:52, peah-webrtc wrote: > > On 2015/08/10 20:19:57, kwiberg-webrtc wrote: > > > Why is this a C and not a C++ file? If it was C++, you could use DCHECK. > > > > Do you mean I should rename it to aec_logging_file_handling.cc? > > Yes. > > > 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. > > 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. Done. https://codereview.webrtc.org/1272403003/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/logging/aec_logging_file_handling.c:41: assert((size_t)written < sizeof(filename)); // buffer was large enough On 2015/08/13 06:19:02, hlundin-webrtc wrote: > On 2015/08/12 20:25:52, peah-webrtc wrote: > > On 2015/08/10 15:25:55, andrew_ooo_to_aug14 wrote: > > > Capitals and periods (and below). > > > > I'm not sure what you refer to. Could you please be more specific? > > 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,_... Done. https://codereview.webrtc.org/1272403003/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/logging/aec_logging_file_handling.c:41: assert((size_t)written < sizeof(filename)); // buffer was large enough On 2015/08/10 15:25:55, andrew_ooo_to_aug14 wrote: > Capitals and periods (and below). Done. https://codereview.webrtc.org/1272403003/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/logging/aec_logging_file_handling.c:41: assert((size_t)written < sizeof(filename)); // buffer was large enough On 2015/08/13 07:59:34, kwiberg-webrtc wrote: > On 2015/08/13 06:19:02, hlundin-webrtc wrote: > > On 2015/08/12 20:25:52, peah-webrtc wrote: > > > On 2015/08/10 15:25:55, andrew_ooo_to_aug14 wrote: > > > > Capitals and periods (and below). > > > > > > I'm not sure what you refer to. Could you please be more specific? > > > > 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,_... > > 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! Acknowledged. https://codereview.webrtc.org/1272403003/diff/60001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/logging/aec_logging_file_handling.h (right): https://codereview.webrtc.org/1272403003/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/logging/aec_logging_file_handling.h:25: // Open a new Wav file for writing. If it was already open with a different On 2015/08/13 06:19:02, hlundin-webrtc wrote: > Opens... Done.
lgtm, but I'll let you hash out the issue of macros vs. inlines with the other reviewers. I'm fine with either solution.
Added below comment to the discussion regarding the choice between macros and inlined functions: 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? https://codereview.webrtc.org/1272403003/diff/40001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/logging/aec_logging.h (right): https://codereview.webrtc.org/1272403003/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/logging/aec_logging.h:94: #endif // WEBRTC_MODULES_AUDIO_PROCESSING_AEC_AEC_LOGGING_ On 2015/08/13 07:59:34, kwiberg-webrtc wrote: > On 2015/08/12 20:25:52, peah-webrtc wrote: > > On 2015/08/10 20:19:57, kwiberg-webrtc wrote: > > > 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! > > > > 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! > > 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. 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?
Added below comment to the discussion regarding the choice between macros and inlined functions: 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?
lgtm https://codereview.webrtc.org/1272403003/diff/40001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/logging/aec_logging.h (right): https://codereview.webrtc.org/1272403003/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/logging/aec_logging.h:94: #endif // WEBRTC_MODULES_AUDIO_PROCESSING_AEC_AEC_LOGGING_ On 2015/08/17 09:36:16, peah-webrtc wrote: > On 2015/08/13 07:59:34, kwiberg-webrtc wrote: > > On 2015/08/12 20:25:52, peah-webrtc wrote: > > > On 2015/08/10 20:19:57, kwiberg-webrtc wrote: > > > > 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! > > > > > > 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! > > > > 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. > > 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? Yes. :-)
On 2015/08/17 09:43:07, kwiberg-webrtc wrote: > lgtm > > https://codereview.webrtc.org/1272403003/diff/40001/webrtc/modules/audio_proc... > File webrtc/modules/audio_processing/logging/aec_logging.h (right): > > https://codereview.webrtc.org/1272403003/diff/40001/webrtc/modules/audio_proc... > webrtc/modules/audio_processing/logging/aec_logging.h:94: #endif // > WEBRTC_MODULES_AUDIO_PROCESSING_AEC_AEC_LOGGING_ > On 2015/08/17 09:36:16, peah-webrtc wrote: > > On 2015/08/13 07:59:34, kwiberg-webrtc wrote: > > > On 2015/08/12 20:25:52, peah-webrtc wrote: > > > > On 2015/08/10 20:19:57, kwiberg-webrtc wrote: > > > > > 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! > > > > > > > > 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! > > > > > > 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. > > > > 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? > > Yes. :-) Yes.
lgtm with a few nits. https://codereview.webrtc.org/1272403003/diff/40001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/logging/aec_logging.h (right): https://codereview.webrtc.org/1272403003/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/logging/aec_logging.h:94: #endif // WEBRTC_MODULES_AUDIO_PROCESSING_AEC_AEC_LOGGING_ On 2015/08/17 09:43:07, kwiberg-webrtc wrote: > On 2015/08/17 09:36:16, peah-webrtc wrote: > > Are everyone ok with that setup/way forward? > > Yes. :-) Yes. https://codereview.webrtc.org/1272403003/diff/80001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/logging/aec_logging_file_handling.cc (right): https://codereview.webrtc.org/1272403003/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/logging/aec_logging_file_handling.cc:17: #include "webrtc/modules/audio_processing/logging/aec_logging_file_handling.h" nit: style guide likes the associated header to be the first include (with a blank line after). https://codereview.webrtc.org/1272403003/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/logging/aec_logging_file_handling.cc:36: assert(written >= 0); Since this is C++ now, you could use DCHECKs instead of the asserts. https://codereview.webrtc.org/1272403003/diff/80001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/logging/aec_logging_file_handling.h (right): https://codereview.webrtc.org/1272403003/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/logging/aec_logging_file_handling.h:14: // To enable AEC logging, enable the gyp variable aec_debug_dump=1 Remove this; it's already mentioned in aec_logging.h.
https://codereview.webrtc.org/1272403003/diff/40001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/logging/aec_logging_file_handling.c (right): https://codereview.webrtc.org/1272403003/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/logging/aec_logging_file_handling.c:56: assert((size_t)written < sizeof(filename)); // buffer was large enough On 2015/08/12 20:25:53, peah-webrtc wrote: > On 2015/08/11 08:48:26, the sun wrote: > > 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. > > 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 implementation is C++ now, correct? Can you use stringutils.h instead? https://codereview.webrtc.org/1272403003/diff/80001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/logging/aec_logging_file_handling.cc (right): https://codereview.webrtc.org/1272403003/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/logging/aec_logging_file_handling.cc:36: assert(written >= 0); On 2015/08/17 16:37:05, Andrew MacDonald wrote: > Since this is C++ now, you could use DCHECKs instead of the asserts. +1
Hi, Again thanks for the comments. I've done changes according to the latest comments. In particular, there were substantial changes in aec_logging_file_handling.cc which was changed to reflect that it is now a C++ file. One change made apart from the comments was to change the c-style casts to static_cast. https://codereview.webrtc.org/1272403003/diff/40001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/logging/aec_logging_file_handling.c (right): https://codereview.webrtc.org/1272403003/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/logging/aec_logging_file_handling.c:56: assert((size_t)written < sizeof(filename)); // buffer was large enough On 2015/08/24 11:04:46, the sun wrote: > On 2015/08/12 20:25:53, peah-webrtc wrote: > > On 2015/08/11 08:48:26, the sun wrote: > > > 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. > > > > 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 implementation is C++ now, correct? Can you use stringutils.h instead? Done. https://codereview.webrtc.org/1272403003/diff/80001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/logging/aec_logging_file_handling.cc (right): https://codereview.webrtc.org/1272403003/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/logging/aec_logging_file_handling.cc:17: #include "webrtc/modules/audio_processing/logging/aec_logging_file_handling.h" On 2015/08/17 16:37:05, Andrew MacDonald wrote: > nit: style guide likes the associated header to be the first include (with a > blank line after). Done. https://codereview.webrtc.org/1272403003/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/logging/aec_logging_file_handling.cc:36: assert(written >= 0); On 2015/08/17 16:37:05, Andrew MacDonald wrote: > Since this is C++ now, you could use DCHECKs instead of the asserts. Done. https://codereview.webrtc.org/1272403003/diff/80001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/logging/aec_logging_file_handling.h (right): https://codereview.webrtc.org/1272403003/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/logging/aec_logging_file_handling.h:14: // To enable AEC logging, enable the gyp variable aec_debug_dump=1 On 2015/08/17 16:37:05, Andrew MacDonald wrote: > Remove this; it's already mentioned in aec_logging.h. Done.
lgtm https://codereview.webrtc.org/1272403003/diff/100001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/logging/aec_logging_file_handling.cc (right): https://codereview.webrtc.org/1272403003/diff/100001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/logging/aec_logging_file_handling.cc:47: int instance_counter, Is there a difference between instance_index (as used in _ReopenWav()) and instance_counter?
https://codereview.webrtc.org/1272403003/diff/100001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/logging/aec_logging_file_handling.cc (right): https://codereview.webrtc.org/1272403003/diff/100001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/logging/aec_logging_file_handling.cc:23: extern "C" void WebRtcAec_ReopenWav(const char* name, You only need the 'extern "C"' in the declaration, not in the definition. See e.g. https://isocpp.org/wiki/faq/mixing-c-and-cpp https://codereview.webrtc.org/1272403003/diff/100001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/logging/aec_logging_file_handling.cc:29: char filename[64]; Move declarations as far down as possible now that the code is C++. And see below about ATTRIBUTE_UNUSED. https://codereview.webrtc.org/1272403003/diff/100001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/logging/aec_logging_file_handling.cc:36: instance_index, process_rate); You appear to need to run clang-format. https://codereview.webrtc.org/1272403003/diff/100001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/logging/aec_logging_file_handling.cc:39: DCHECK(written >= 0); DCHECK_GE https://codereview.webrtc.org/1272403003/diff/100001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/logging/aec_logging_file_handling.cc:41: DCHECK(static_cast<size_t>(written) < sizeof(filename)); DCHECK_LT https://codereview.webrtc.org/1272403003/diff/100001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/logging/aec_logging_file_handling.cc:49: int written ATTRIBUTE_UNUSED; You shouldn't need the ATTRIBUTE_UNUSED. (DCHECK is written in an extra sneaky way so that you won't have to worry. See https://code.google.com/p/webrtc/source/detail?r=7869.) Also, this is C++, so you can declare it at the point of first use.
lgtm https://codereview.webrtc.org/1272403003/diff/100001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/logging/aec_logging_file_handling.cc (right): https://codereview.webrtc.org/1272403003/diff/100001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/logging/aec_logging_file_handling.cc:23: extern "C" void WebRtcAec_ReopenWav(const char* name, On 2015/08/25 09:04:43, kwiberg-webrtc wrote: > You only need the 'extern "C"' in the declaration, not in the definition. See > e.g. https://isocpp.org/wiki/faq/mixing-c-and-cpp Ah, interesting. I think we have this pattern elsewhere too. https://codereview.webrtc.org/1272403003/diff/100001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/logging/aec_logging_file_handling.cc:35: written = rtc::sprintfn(filename, ARRAY_SIZE(filename), "%s%d-%d.wav", name, sprintfn calls for a number of bytes in the second parameter. ARRAY_SIZE and sizeof will give the same result for a char array, but sizeof is more semantically correct.
Thanks again! Great feedback as always! I've revised accordingly and sent up a new patch. https://codereview.webrtc.org/1272403003/diff/100001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/logging/aec_logging_file_handling.cc (right): https://codereview.webrtc.org/1272403003/diff/100001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/logging/aec_logging_file_handling.cc:23: extern "C" void WebRtcAec_ReopenWav(const char* name, On 2015/08/25 09:04:43, kwiberg-webrtc wrote: > You only need the 'extern "C"' in the declaration, not in the definition. See > e.g. https://isocpp.org/wiki/faq/mixing-c-and-cpp Done. https://codereview.webrtc.org/1272403003/diff/100001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/logging/aec_logging_file_handling.cc:23: extern "C" void WebRtcAec_ReopenWav(const char* name, On 2015/08/25 17:16:29, Andrew MacDonald wrote: > On 2015/08/25 09:04:43, kwiberg-webrtc wrote: > > You only need the 'extern "C"' in the declaration, not in the definition. See > > e.g. https://isocpp.org/wiki/faq/mixing-c-and-cpp > > Ah, interesting. I think we have this pattern elsewhere too. Acknowledged. https://codereview.webrtc.org/1272403003/diff/100001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/logging/aec_logging_file_handling.cc:23: extern "C" void WebRtcAec_ReopenWav(const char* name, On 2015/08/25 09:04:43, kwiberg-webrtc wrote: > You only need the 'extern "C"' in the declaration, not in the definition. See > e.g. https://isocpp.org/wiki/faq/mixing-c-and-cpp Done. https://codereview.webrtc.org/1272403003/diff/100001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/logging/aec_logging_file_handling.cc:29: char filename[64]; On 2015/08/25 09:04:43, kwiberg-webrtc wrote: > Move declarations as far down as possible now that the code is C++. And see > below about ATTRIBUTE_UNUSED. Done. https://codereview.webrtc.org/1272403003/diff/100001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/logging/aec_logging_file_handling.cc:35: written = rtc::sprintfn(filename, ARRAY_SIZE(filename), "%s%d-%d.wav", name, On 2015/08/25 17:16:29, Andrew MacDonald wrote: > sprintfn calls for a number of bytes in the second parameter. ARRAY_SIZE and > sizeof will give the same result for a char array, but sizeof is more > semantically correct. Done. https://codereview.webrtc.org/1272403003/diff/100001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/logging/aec_logging_file_handling.cc:36: instance_index, process_rate); On 2015/08/25 09:04:43, kwiberg-webrtc wrote: > You appear to need to run clang-format. Done. https://codereview.webrtc.org/1272403003/diff/100001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/logging/aec_logging_file_handling.cc:39: DCHECK(written >= 0); On 2015/08/25 09:04:43, kwiberg-webrtc wrote: > DCHECK_GE Done. https://codereview.webrtc.org/1272403003/diff/100001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/logging/aec_logging_file_handling.cc:41: DCHECK(static_cast<size_t>(written) < sizeof(filename)); On 2015/08/25 09:04:43, kwiberg-webrtc wrote: > DCHECK_LT Done. https://codereview.webrtc.org/1272403003/diff/100001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/logging/aec_logging_file_handling.cc:47: int instance_counter, On 2015/08/25 08:32:35, the sun wrote: > Is there a difference between instance_index (as used in _ReopenWav()) and > instance_counter? Good point. No, there is not :-). I'll choose the instance_index for both of them, because that is really what it is. https://codereview.webrtc.org/1272403003/diff/100001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/logging/aec_logging_file_handling.cc:47: int instance_counter, On 2015/08/25 08:32:35, the sun wrote: > Is there a difference between instance_index (as used in _ReopenWav()) and > instance_counter? Done. https://codereview.webrtc.org/1272403003/diff/100001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/logging/aec_logging_file_handling.cc:49: int written ATTRIBUTE_UNUSED; On 2015/08/25 09:04:43, kwiberg-webrtc wrote: > You shouldn't need the ATTRIBUTE_UNUSED. (DCHECK is written in an extra sneaky > way so that you won't have to worry. See > https://code.google.com/p/webrtc/source/detail?r=7869.) > > Also, this is C++, so you can declare it at the point of first use. Done.
Sorry, my mistake, please hold on with the review. I just ran into an issue with the latest patch
Now the previous issue is fixed. I submitted a new patch. Please feel free to verify.
lgtm
lgtm
The CQ bit was checked by peah@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from henrik.lundin@webrtc.org, solenberg@webrtc.org Link to the patchset: https://codereview.webrtc.org/1272403003/#ps140001 (title: "Updated the implementation of the extern C declaration")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1272403003/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1272403003/140001
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/9e69abf85eeae8b6e23170c189cf5dd155771a51 Cr-Commit-Position: refs/heads/master@{#9808} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
