Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1488613002:
Added two more debug macros for logging scalar values to file. (Closed)
|
Created:
5 years ago by peah-webrtc Modified:
5 years ago CC:
webrtc-reviews_webrtc.org, Andrew MacDonald, tterriberry_mozilla.com, audio-team_agora.io, minyue-webrtc, the sun, aluebs-webrtc, bjornv1 Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionAdding two more debug macros for logging scalar values to file.
The two added macros simplifies the logging code when a value which is not stored in a variable should be logged.
BUG=
Committed: https://crrev.com/de0fc58784f3f27503eef37ae7f50cae0bcbe30c
Cr-Commit-Position: refs/heads/master@{#10870}
Patch Set 1 #Patch Set 2 : Corrected the function call for storing the data #
Total comments: 3
Messages
Total messages: 21 (8 generated)
Description was changed from ========== Added two more debug macros for logging scalar values to file. BUG= ========== to ========== Adding two more debug macros for logging scalar values to file. The two added macros simplifies the logging code when a value which is not stored in a variable should be logged. BUG= ==========
peah@webrtc.org changed reviewers: + henrik.lundin@webrtc.org, ivoc@webrtc.org
kwiberg@webrtc.org changed reviewers: + kwiberg@webrtc.org
https://codereview.webrtc.org/1488613002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/logging/aec_logging.h (right): https://codereview.webrtc.org/1488613002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/logging/aec_logging.h:59: Drive-by: Please consider using inline functions instead of introducing even more macros. Something like static inline void blupp(FILE* file, double data) { #ifdef BLIPP (void) fwrite(&data, sizeof(data), 1, file); #endif } ought to work fine.
On 2015/11/30 14:55:44, kwiberg-webrtc wrote: > https://codereview.webrtc.org/1488613002/diff/20001/webrtc/modules/audio_proc... > File webrtc/modules/audio_processing/logging/aec_logging.h (right): > > https://codereview.webrtc.org/1488613002/diff/20001/webrtc/modules/audio_proc... > webrtc/modules/audio_processing/logging/aec_logging.h:59: > Drive-by: Please consider using inline functions instead of introducing even > more macros. Something like > > static inline void blupp(FILE* file, double data) { > #ifdef BLIPP > (void) fwrite(&data, sizeof(data), 1, file); > #endif > } > > ought to work fine. It does, and I definitely agree that it would be better without macros. But my main concern with your proposed solution is that the file-pointers/handles cannot be #ifdef-ed out when not building for debugging. The macros really end up in thin air when not building for debugging, meaning that the file-pointers can be #ifdef-ed out as well, so that solution is from that perspective preferable. What do you think? Should we accept file pointers that are not used in the state in release builds? In that case we could go for inline functions. Otherwise we need to go for macros. In the long run, we should build a proper C++ class for this.
On 2015/11/30 20:59:19, peah-webrtc wrote: > On 2015/11/30 14:55:44, kwiberg-webrtc wrote: > > > https://codereview.webrtc.org/1488613002/diff/20001/webrtc/modules/audio_proc... > > File webrtc/modules/audio_processing/logging/aec_logging.h (right): > > > > > https://codereview.webrtc.org/1488613002/diff/20001/webrtc/modules/audio_proc... > > webrtc/modules/audio_processing/logging/aec_logging.h:59: > > Drive-by: Please consider using inline functions instead of introducing even > > more macros. Something like > > > > static inline void blupp(FILE* file, double data) { > > #ifdef BLIPP > > (void) fwrite(&data, sizeof(data), 1, file); > > #endif > > } > > > > ought to work fine. > > It does, and I definitely agree that it would be better without macros. But my > main concern with your proposed solution is that the file-pointers/handles > cannot be #ifdef-ed out when not building for debugging. The macros really end > up in thin air when not building for debugging, meaning that the file-pointers > can be #ifdef-ed out as well, so that solution is from that perspective > preferable. > > What do you think? Should we accept file pointers that are not used in the state > in release builds? In that case we could go for inline functions. Otherwise we > need to go for macros. > > In the long run, we should build a proper C++ class for this. I think your current solution is fine for two reasons: 1. It builds upon what is already in the file. 2. The unused file pointer concern. LGTM
On 2015/12/01 08:23:28, hlundin-webrtc wrote: > On 2015/11/30 20:59:19, peah-webrtc wrote: > > On 2015/11/30 14:55:44, kwiberg-webrtc wrote: > > > > > > https://codereview.webrtc.org/1488613002/diff/20001/webrtc/modules/audio_proc... > > > File webrtc/modules/audio_processing/logging/aec_logging.h (right): > > > > > > > > > https://codereview.webrtc.org/1488613002/diff/20001/webrtc/modules/audio_proc... > > > webrtc/modules/audio_processing/logging/aec_logging.h:59: > > > Drive-by: Please consider using inline functions instead of introducing even > > > more macros. Something like > > > > > > static inline void blupp(FILE* file, double data) { > > > #ifdef BLIPP > > > (void) fwrite(&data, sizeof(data), 1, file); > > > #endif > > > } > > > > > > ought to work fine. > > > > It does, and I definitely agree that it would be better without macros. But my > > main concern with your proposed solution is that the file-pointers/handles > > cannot be #ifdef-ed out when not building for debugging. The macros really end > > up in thin air when not building for debugging, meaning that the file-pointers > > can be #ifdef-ed out as well, so that solution is from that perspective > > preferable. > > > > What do you think? Should we accept file pointers that are not used in the > state > > in release builds? In that case we could go for inline functions. Otherwise we > > need to go for macros. > > > > In the long run, we should build a proper C++ class for this. Ah, I didn't think of that. LGTM for the original proposal, then, since making a proper class would probably be too much work (but I agree it is the solution to go for given time to do it right).
https://codereview.webrtc.org/1488613002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/logging/aec_logging.h (right): https://codereview.webrtc.org/1488613002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/logging/aec_logging.h:49: int32_t value_to_store = data; \ Would it be possible/desirable to merge this with the macro below by changing int32_t to auto?
https://codereview.webrtc.org/1488613002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/logging/aec_logging.h (right): https://codereview.webrtc.org/1488613002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/logging/aec_logging.h:49: int32_t value_to_store = data; \ On 2015/12/01 09:47:52, ivoc wrote: > Would it be possible/desirable to merge this with the macro below by changing > int32_t to auto? I think you are right in that it could. My only concern is that we miss the typecheck if the writing to a certain file happens in several places and may therefore write it in different formats in different places. Also, afaics the auto makes it up to the compiler to choose the format, right, and I guess that it would adapt it to the type of value being written. For instance in: int16_t luke=32768; int16_t skywalker=32768; RTC_AEC_DEBUG_RAW_WRITE_SCALAR_AUTO(luke*skywalker); I guess that that would store the value in int32, but it is less straightforward to see that. My intended use of this is to read the values into matlab using fread, and then I need to know the value type, and for me it would be easier to read the logs if I know whether it either an int32_t or a double. What do you think? Maybe the auto-scheme could be adapted to allow a simple read functionality when retrieving the stored data?
On 2015/12/01 12:21:43, peah-webrtc wrote: > https://codereview.webrtc.org/1488613002/diff/20001/webrtc/modules/audio_proc... > File webrtc/modules/audio_processing/logging/aec_logging.h (right): > > https://codereview.webrtc.org/1488613002/diff/20001/webrtc/modules/audio_proc... > webrtc/modules/audio_processing/logging/aec_logging.h:49: int32_t value_to_store > = data; \ > On 2015/12/01 09:47:52, ivoc wrote: > > Would it be possible/desirable to merge this with the macro below by changing > > int32_t to auto? > > I think you are right in that it could. My only concern is that we miss the > typecheck if the writing to a certain file happens in several places and may > therefore write it in different formats in different places. > > Also, afaics the auto makes it up to the compiler to choose the format, right, > and I guess that it would adapt it to the type of value being written. > For instance in: > int16_t luke=32768; > int16_t skywalker=32768; > RTC_AEC_DEBUG_RAW_WRITE_SCALAR_AUTO(luke*skywalker); > I guess that that would store the value in int32, but it is less straightforward > to see that. > > My intended use of this is to read the values into matlab using fread, and then > I need to know the value type, and for me it would be easier to read the logs if > I know whether it either an int32_t or a double. > > What do you think? Maybe the auto-scheme could be adapted to allow a simple read > functionality when retrieving the stored data? Right, that's a good point. I guess the current approach is fine then. LGTM.
The CQ bit was checked by peah@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1488613002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1488613002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_arm64_rel on tryserver.webrtc (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by peah@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1488613002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1488613002/20001
Message was sent while issue was closed.
Description was changed from ========== Adding two more debug macros for logging scalar values to file. The two added macros simplifies the logging code when a value which is not stored in a variable should be logged. BUG= ========== to ========== Adding two more debug macros for logging scalar values to file. The two added macros simplifies the logging code when a value which is not stored in a variable should be logged. BUG= ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Adding two more debug macros for logging scalar values to file. The two added macros simplifies the logging code when a value which is not stored in a variable should be logged. BUG= ========== to ========== Adding two more debug macros for logging scalar values to file. The two added macros simplifies the logging code when a value which is not stored in a variable should be logged. BUG= Committed: https://crrev.com/de0fc58784f3f27503eef37ae7f50cae0bcbe30c Cr-Commit-Position: refs/heads/master@{#10870} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/de0fc58784f3f27503eef37ae7f50cae0bcbe30c Cr-Commit-Position: refs/heads/master@{#10870} |
