Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1404743003:
Removed the indirect error message reporting in aec and aecm. (Closed)
|
Created:
5 years, 2 months ago by peah-webrtc Modified:
5 years, 1 month ago CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com, aluebs-webrtc, bjornv1, ivoc Base URL:
https://chromium.googlesource.com/external/webrtc.git@process_test_no_output_CL Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
Description-Removed the indirect error message reporting in aec and aecm.
-Made the component error messages generic to be an unspecified error message.
BUG=webrtc:5099
Committed: https://crrev.com/c12be3984ff49f202f873f8ecd83f0e5b9cb36c9
Cr-Commit-Position: refs/heads/master@{#10570}
Patch Set 1 #
Total comments: 17
Patch Set 2 : Removed state variable for lastError. Removed function prototypes for unused functions. Added const qualifiers. #
Total comments: 11
Patch Set 3 : Removed obsolete comments and reverted the changes in processing_component.cc as they were not need… #
Total comments: 16
Patch Set 4 : Changes in response to reviewer comments. #Patch Set 5 : Merge #Patch Set 6 : Merge #Patch Set 7 : Merge #
Total comments: 4
Patch Set 8 : Changed if-statement #Patch Set 9 : Merge from other CLs in the list #Patch Set 10 : Merge from preceeding CLs #Patch Set 11 : Merge from latest master #
Depends on Patchset: Dependent Patchsets: Messages
Total messages: 33 (12 generated)
peah@webrtc.org changed reviewers: + henrik.lundin@webrtc.org, kwiberg@webrtc.org, solenberg@google.com, solenberg@webrtc.org
This CL is a preparation for the enabling of concurrent processing of the render and capture sides of APM. The indirect error code reporting in aec and aecm is shared between the render and capture sides and in order to remove that dependency it is removed in this CL. This introduces no different behavior in APM apart from during configuring and reinitialization. The reason that it is different in those is that the error handling scheme is generic for all subcomponents in APM, and during discussions it was decided to replace the reporting of error-specific codes in those with generic error codes. The motivation for this was that 1) There seems to be no sensible way to react in response to the specific error codes. 2) This will allow the dependency between the render and capture sides in aec and aecm to be broken in a generic way where the indirect error code reporting scheme is fully replaced by a direct error reporting scheme.
https://codereview.webrtc.org/1404743003/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/aec/echo_cancellation.c (left): https://codereview.webrtc.org/1404743003/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/aec/echo_cancellation.c:195: aecpc->lastError = AEC_BAD_PARAMETER_ERROR; You should be able to remove this struct member too. https://codereview.webrtc.org/1404743003/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/aec/include/echo_cancellation.h (right): https://codereview.webrtc.org/1404743003/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/aec/include/echo_cancellation.h:249: int32_t WebRtcAec_get_error_code(void* aecInst); Remove this too. https://codereview.webrtc.org/1404743003/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/aecm/echo_control_mobile.c (left): https://codereview.webrtc.org/1404743003/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/aecm/echo_control_mobile.c:527: int32_t WebRtcAecm_get_config(void *aecmInst, AecmConfig *config) Why is this function deleted? https://codereview.webrtc.org/1404743003/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/aecm/echo_control_mobile.c (right): https://codereview.webrtc.org/1404743003/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/aecm/echo_control_mobile.c:221: int32_t err = WebRtcAecm_GetBufferFarendError(aecmInst, farend, nrOfSamples); const https://codereview.webrtc.org/1404743003/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/aecm/include/echo_control_mobile.h (right): https://codereview.webrtc.org/1404743003/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/aecm/include/echo_control_mobile.h:93: * Reports any errors that would arise if buffering a farend buffer End with . https://codereview.webrtc.org/1404743003/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/aecm/include/echo_control_mobile.h:216: int32_t WebRtcAecm_get_error_code(void *aecmInst); Remove the declaration too. https://codereview.webrtc.org/1404743003/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/echo_cancellation_impl.cc (right): https://codereview.webrtc.org/1404743003/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/echo_cancellation_impl.cc:312: int err = const
On 2015/10/14 14:34:49, hlundin-webrtc wrote: > https://codereview.webrtc.org/1404743003/diff/1/webrtc/modules/audio_processi... > File webrtc/modules/audio_processing/aec/echo_cancellation.c (left): > > https://codereview.webrtc.org/1404743003/diff/1/webrtc/modules/audio_processi... > webrtc/modules/audio_processing/aec/echo_cancellation.c:195: aecpc->lastError = > AEC_BAD_PARAMETER_ERROR; > You should be able to remove this struct member too. > > https://codereview.webrtc.org/1404743003/diff/1/webrtc/modules/audio_processi... > File webrtc/modules/audio_processing/aec/include/echo_cancellation.h (right): > > https://codereview.webrtc.org/1404743003/diff/1/webrtc/modules/audio_processi... > webrtc/modules/audio_processing/aec/include/echo_cancellation.h:249: int32_t > WebRtcAec_get_error_code(void* aecInst); > Remove this too. > > https://codereview.webrtc.org/1404743003/diff/1/webrtc/modules/audio_processi... > File webrtc/modules/audio_processing/aecm/echo_control_mobile.c (left): > > https://codereview.webrtc.org/1404743003/diff/1/webrtc/modules/audio_processi... > webrtc/modules/audio_processing/aecm/echo_control_mobile.c:527: int32_t > WebRtcAecm_get_config(void *aecmInst, AecmConfig *config) > Why is this function deleted? > > https://codereview.webrtc.org/1404743003/diff/1/webrtc/modules/audio_processi... > File webrtc/modules/audio_processing/aecm/echo_control_mobile.c (right): > > https://codereview.webrtc.org/1404743003/diff/1/webrtc/modules/audio_processi... > webrtc/modules/audio_processing/aecm/echo_control_mobile.c:221: int32_t err = > WebRtcAecm_GetBufferFarendError(aecmInst, farend, nrOfSamples); > const > > https://codereview.webrtc.org/1404743003/diff/1/webrtc/modules/audio_processi... > File webrtc/modules/audio_processing/aecm/include/echo_control_mobile.h (right): > > https://codereview.webrtc.org/1404743003/diff/1/webrtc/modules/audio_processi... > webrtc/modules/audio_processing/aecm/include/echo_control_mobile.h:93: * Reports > any errors that would arise if buffering a farend buffer > End with . > > https://codereview.webrtc.org/1404743003/diff/1/webrtc/modules/audio_processi... > webrtc/modules/audio_processing/aecm/include/echo_control_mobile.h:216: int32_t > WebRtcAecm_get_error_code(void *aecmInst); > Remove the declaration too. > > https://codereview.webrtc.org/1404743003/diff/1/webrtc/modules/audio_processi... > File webrtc/modules/audio_processing/echo_cancellation_impl.cc (right): > > https://codereview.webrtc.org/1404743003/diff/1/webrtc/modules/audio_processi... > webrtc/modules/audio_processing/echo_cancellation_impl.cc:312: int err = > const Oh, and get rid of the '-' from the CL title. (You can edit it in the web tool.)
https://codereview.webrtc.org/1404743003/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/aec/echo_cancellation.c (left): https://codereview.webrtc.org/1404743003/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/aec/echo_cancellation.c:195: aecpc->lastError = AEC_BAD_PARAMETER_ERROR; On 2015/10/14 14:34:49, hlundin-webrtc wrote: > You should be able to remove this struct member too. Good catch! https://codereview.webrtc.org/1404743003/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/aec/echo_cancellation.c:195: aecpc->lastError = AEC_BAD_PARAMETER_ERROR; On 2015/10/14 14:34:49, hlundin-webrtc wrote: > You should be able to remove this struct member too. Done. https://codereview.webrtc.org/1404743003/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/aec/include/echo_cancellation.h (right): https://codereview.webrtc.org/1404743003/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/aec/include/echo_cancellation.h:249: int32_t WebRtcAec_get_error_code(void* aecInst); On 2015/10/14 14:34:49, hlundin-webrtc wrote: > Remove this too. Done. https://codereview.webrtc.org/1404743003/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/aecm/echo_control_mobile.c (left): https://codereview.webrtc.org/1404743003/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/aecm/echo_control_mobile.c:527: int32_t WebRtcAecm_get_config(void *aecmInst, AecmConfig *config) On 2015/10/14 14:34:49, hlundin-webrtc wrote: > Why is this function deleted? I removed it as it was not used anywhere. It is basically untested code as it is not used, and as this is a submodule we intend to replace I removed it. But I'm happy to keep it as well. https://codereview.webrtc.org/1404743003/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/aecm/echo_control_mobile.c (right): https://codereview.webrtc.org/1404743003/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/aecm/echo_control_mobile.c:221: int32_t err = WebRtcAecm_GetBufferFarendError(aecmInst, farend, nrOfSamples); On 2015/10/14 14:34:49, hlundin-webrtc wrote: > const Done. https://codereview.webrtc.org/1404743003/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/aecm/include/echo_control_mobile.h (right): https://codereview.webrtc.org/1404743003/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/aecm/include/echo_control_mobile.h:93: * Reports any errors that would arise if buffering a farend buffer On 2015/10/14 14:34:49, hlundin-webrtc wrote: > End with . Done. https://codereview.webrtc.org/1404743003/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/aecm/include/echo_control_mobile.h:216: int32_t WebRtcAecm_get_error_code(void *aecmInst); On 2015/10/14 14:34:49, hlundin-webrtc wrote: > Remove the declaration too. Done. https://codereview.webrtc.org/1404743003/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/echo_cancellation_impl.cc (right): https://codereview.webrtc.org/1404743003/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/echo_cancellation_impl.cc:312: int err = On 2015/10/14 14:34:49, hlundin-webrtc wrote: > const Done.
andrew@webrtc.org changed reviewers: + andrew@webrtc.org
Drive-by. https://codereview.webrtc.org/1404743003/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec/echo_cancellation.c (right): https://codereview.webrtc.org/1404743003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec/echo_cancellation.c:407: // Due to the initialization scheme the indirect This looks pretty direct; what do you mean? In any case, don't bother repeating it five times; just say "similar below" or something. https://codereview.webrtc.org/1404743003/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec/include/echo_cancellation.h (right): https://codereview.webrtc.org/1404743003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec/include/echo_cancellation.h:112: * int32_t return 12000-12050: error code It's a pretty strong convention in webrtc that error returns are less than zero. Do you want to keep the error codes at all? If so, I'd make them proceed downward from -1. The notion that they had to occupy a unique range is completely legacy, so don't worry about that. Should we just remove them entirely though, and return -1? https://codereview.webrtc.org/1404743003/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aecm/echo_control_mobile.c (right): https://codereview.webrtc.org/1404743003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aecm/echo_control_mobile.c:254: return -1; This should be something else. Some kind of unspecified error code? https://codereview.webrtc.org/1404743003/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/processing_component.cc (right): https://codereview.webrtc.org/1404743003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/processing_component.cc:88: return AudioProcessing::kUnspecifiedError; I think it would probably be fine to remove all component-specific error reporting, but I didn't understand from your explanation why you're only removing these ones.
https://codereview.webrtc.org/1404743003/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec/echo_cancellation.c (right): https://codereview.webrtc.org/1404743003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec/echo_cancellation.c:407: // Due to the initialization scheme the indirect On 2015/10/16 06:05:09, Andrew MacDonald wrote: > This looks pretty direct; what do you mean? > > In any case, don't bother repeating it five times; just say "similar below" or > something. Totally correct. Good find! That was a leftover comment because we decided in the last minute to change it to be direct. Will fix! https://codereview.webrtc.org/1404743003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec/echo_cancellation.c:407: // Due to the initialization scheme the indirect On 2015/10/16 06:05:09, Andrew MacDonald wrote: > This looks pretty direct; what do you mean? > > In any case, don't bother repeating it five times; just say "similar below" or > something. Done. https://codereview.webrtc.org/1404743003/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec/include/echo_cancellation.h (right): https://codereview.webrtc.org/1404743003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec/include/echo_cancellation.h:112: * int32_t return 12000-12050: error code On 2015/10/16 06:05:09, Andrew MacDonald wrote: > It's a pretty strong convention in webrtc that error returns are less than zero. > Do you want to keep the error codes at all? If so, I'd make them proceed > downward from -1. > > The notion that they had to occupy a unique range is completely legacy, so don't > worry about that. > > Should we just remove them entirely though, and return -1? That would be great! We've had discussions about the error codes, the how useful they are, and how it would be possible to react on them. If ok, I won't fix this in the scope of the current work, but I'd be happy to change it afterwards. https://codereview.webrtc.org/1404743003/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/processing_component.cc (right): https://codereview.webrtc.org/1404743003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/processing_component.cc:88: return AudioProcessing::kUnspecifiedError; On 2015/10/16 06:05:09, Andrew MacDonald wrote: > I think it would probably be fine to remove all component-specific error > reporting, but I didn't understand from your explanation why you're only > removing these ones. You are correct, this change is not really needed at this point, as the GetHandleError in echo_cancellation_impl_ has been properly changed to handle it. I'll revert this. https://codereview.webrtc.org/1404743003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/processing_component.cc:88: return AudioProcessing::kUnspecifiedError; On 2015/10/16 06:05:09, Andrew MacDonald wrote: > I think it would probably be fine to remove all component-specific error > reporting, but I didn't understand from your explanation why you're only > removing these ones. Done.
lgtm https://codereview.webrtc.org/1404743003/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec/include/echo_cancellation.h (right): https://codereview.webrtc.org/1404743003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec/include/echo_cancellation.h:112: * int32_t return 12000-12050: error code On 2015/10/16 08:14:39, peah-webrtc wrote: > On 2015/10/16 06:05:09, Andrew MacDonald wrote: > > It's a pretty strong convention in webrtc that error returns are less than > zero. > > Do you want to keep the error codes at all? If so, I'd make them proceed > > downward from -1. > > > > The notion that they had to occupy a unique range is completely legacy, so > don't > > worry about that. > > > > Should we just remove them entirely though, and return -1? > > That would be great! We've had discussions about the error codes, the how useful > they are, and how it would be possible to react on them. > > If ok, I won't fix this in the scope of the current work, but I'd be happy to > change it afterwards. That's fine. I think if you intend to remove them you can leave the codes as-is, but if you decide to keep them it would be great if you could change them to be non-unique and descending from -1 in a soon-ish CL. Thanks! https://codereview.webrtc.org/1404743003/diff/40001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec/echo_cancellation_internal.h (right): https://codereview.webrtc.org/1404743003/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec/echo_cancellation_internal.h:59: Extra blank line here now.
lgtm https://codereview.webrtc.org/1404743003/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/aecm/echo_control_mobile.c (left): https://codereview.webrtc.org/1404743003/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/aecm/echo_control_mobile.c:527: int32_t WebRtcAecm_get_config(void *aecmInst, AecmConfig *config) On 2015/10/14 17:57:36, peah-webrtc wrote: > On 2015/10/14 14:34:49, hlundin-webrtc wrote: > > Why is this function deleted? > > I removed it as it was not used anywhere. It is basically untested code as it is > not used, and as this is a submodule we intend to replace I removed it. But I'm > happy to keep it as well. OK. So it is slightly out-of-scope for this CL. But I don't mind you removing that code now.
lgtm % nits https://codereview.webrtc.org/1404743003/diff/40001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec/echo_cancellation.c (right): https://codereview.webrtc.org/1404743003/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec/echo_cancellation.c:266: // farend signal. nit: "farend" -> "far-end" or "far end" https://codereview.webrtc.org/1404743003/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec/echo_cancellation.c:267: int32_t WebRtcAec_GetBufferFarendError(void* aecInst, I don't see where this function is used outside of this file. Why did you promote it to be part of the interface? Is it in a different CL? https://codereview.webrtc.org/1404743003/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec/echo_cancellation.c:296: error_code = WebRtcAec_GetBufferFarendError(aecInst, farend, nrOfSamples); nit: int32_t on same line. https://codereview.webrtc.org/1404743003/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec/echo_cancellation.c:381: } else { What's the point of trying to process if we already got a "bad parameter warning"? https://codereview.webrtc.org/1404743003/diff/40001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec/include/echo_cancellation.h (right): https://codereview.webrtc.org/1404743003/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec/include/echo_cancellation.h:158: * int32_t return 12000-12050: error code nit: remove the second "return" https://codereview.webrtc.org/1404743003/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec/include/echo_cancellation.h:180: * int32_t return 12000-12050: error code no, it's not int32_t, here and below also, alignment of "0: OK" formatting in echo_control_mobile.h looks fine, do the same here
solenberg@webrtc.org changed reviewers: - solenberg@google.com
Description was changed from ========== -Removed the indirect error message reporting in aec and aecm. -Made the component error messages generic to be an unspecified error message. BUG= ========== to ========== -Removed the indirect error message reporting in aec and aecm. -Made the component error messages generic to be an unspecified error message. BUG=webrtc:5099 ==========
https://codereview.webrtc.org/1404743003/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec/include/echo_cancellation.h (right): https://codereview.webrtc.org/1404743003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec/include/echo_cancellation.h:112: * int32_t return 12000-12050: error code On 2015/10/16 15:47:05, Andrew MacDonald wrote: > On 2015/10/16 08:14:39, peah-webrtc wrote: > > On 2015/10/16 06:05:09, Andrew MacDonald wrote: > > > It's a pretty strong convention in webrtc that error returns are less than > > zero. > > > Do you want to keep the error codes at all? If so, I'd make them proceed > > > downward from -1. > > > > > > The notion that they had to occupy a unique range is completely legacy, so > > don't > > > worry about that. > > > > > > Should we just remove them entirely though, and return -1? > > > > That would be great! We've had discussions about the error codes, the how > useful > > they are, and how it would be possible to react on them. > > > > If ok, I won't fix this in the scope of the current work, but I'd be happy to > > change it afterwards. > > That's fine. I think if you intend to remove them you can leave the codes as-is, > but if you decide to keep them it would be great if you could change them to be > non-unique and descending from -1 in a soon-ish CL. Thanks! Acknowledged. https://codereview.webrtc.org/1404743003/diff/40001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec/echo_cancellation.c (right): https://codereview.webrtc.org/1404743003/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec/echo_cancellation.c:266: // farend signal. On 2015/10/21 12:09:26, the sun wrote: > nit: "farend" -> "far-end" or "far end" Done. https://codereview.webrtc.org/1404743003/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec/echo_cancellation.c:267: int32_t WebRtcAec_GetBufferFarendError(void* aecInst, On 2015/10/21 12:09:26, the sun wrote: > I don't see where this function is used outside of this file. Why did you > promote it to be part of the interface? Is it in a different CL? Good catch! It will, however, be used in the next CL. https://codereview.webrtc.org/1404743003/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec/echo_cancellation.c:296: error_code = WebRtcAec_GetBufferFarendError(aecInst, farend, nrOfSamples); On 2015/10/21 12:09:27, the sun wrote: > nit: int32_t on same line. Done. https://codereview.webrtc.org/1404743003/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec/echo_cancellation.c:381: } else { On 2015/10/21 12:09:26, the sun wrote: > What's the point of trying to process if we already got a "bad parameter > warning"? That is a very good question! This was how it was done before and I cannot answer that. But it seems like a good candidate for something to fix. But I'd prefer not to do it in the scope of this CL. https://codereview.webrtc.org/1404743003/diff/40001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec/echo_cancellation_internal.h (right): https://codereview.webrtc.org/1404743003/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec/echo_cancellation_internal.h:59: On 2015/10/16 15:47:05, Andrew MacDonald wrote: > Extra blank line here now. Done. https://codereview.webrtc.org/1404743003/diff/40001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec/include/echo_cancellation.h (right): https://codereview.webrtc.org/1404743003/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec/include/echo_cancellation.h:158: * int32_t return 12000-12050: error code On 2015/10/21 12:09:27, the sun wrote: > nit: remove the second "return" Done. https://codereview.webrtc.org/1404743003/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec/include/echo_cancellation.h:180: * int32_t return 12000-12050: error code On 2015/10/21 12:09:27, the sun wrote: > no, it's not int32_t, here and below > > also, alignment of "0: OK" > > formatting in echo_control_mobile.h looks fine, do the same here Done.
https://codereview.webrtc.org/1404743003/diff/40001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec/echo_cancellation.c (right): https://codereview.webrtc.org/1404743003/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec/echo_cancellation.c:381: } else { On 2015/10/23 09:31:15, peah-webrtc wrote: > On 2015/10/21 12:09:26, the sun wrote: > > What's the point of trying to process if we already got a "bad parameter > > warning"? > > That is a very good question! This was how it was done before and I cannot > answer that. But it seems like a good candidate for something to fix. But I'd > prefer not to do it in the scope of this CL. Because the audio device might actually provide a delay value out of our buffer range (this has happened in the past). It might still be a good idea to process the signal, if the value is bogus for example.
https://codereview.webrtc.org/1404743003/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/aecm/echo_control_mobile.c (left): https://codereview.webrtc.org/1404743003/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/aecm/echo_control_mobile.c:527: int32_t WebRtcAecm_get_config(void *aecmInst, AecmConfig *config) On 2015/10/20 13:01:54, hlundin-webrtc wrote: > On 2015/10/14 17:57:36, peah-webrtc wrote: > > On 2015/10/14 14:34:49, hlundin-webrtc wrote: > > > Why is this function deleted? > > > > I removed it as it was not used anywhere. It is basically untested code as it > is > > not used, and as this is a submodule we intend to replace I removed it. But > I'm > > happy to keep it as well. > > OK. So it is slightly out-of-scope for this CL. But I don't mind you removing > that code now. Acknowledged. https://codereview.webrtc.org/1404743003/diff/40001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec/echo_cancellation.c (right): https://codereview.webrtc.org/1404743003/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec/echo_cancellation.c:381: } else { On 2015/10/24 00:27:08, Andrew MacDonald wrote: > On 2015/10/23 09:31:15, peah-webrtc wrote: > > On 2015/10/21 12:09:26, the sun wrote: > > > What's the point of trying to process if we already got a "bad parameter > > > warning"? > > > > That is a very good question! This was how it was done before and I cannot > > answer that. But it seems like a good candidate for something to fix. But I'd > > prefer not to do it in the scope of this CL. > > Because the audio device might actually provide a delay value out of our buffer > range (this has happened in the past). It might still be a good idea to process > the signal, if the value is bogus for example. Acknowledged.
lgtm https://codereview.webrtc.org/1404743003/diff/120001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/aec/echo_cancellation.c (right): https://codereview.webrtc.org/1404743003/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/aec/echo_cancellation.c:272: if (farend == NULL) if (!farend) https://codereview.webrtc.org/1404743003/diff/120001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/aec/include/echo_cancellation.h (right): https://codereview.webrtc.org/1404743003/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/aec/include/echo_cancellation.h:112: * 12000-12050: error code As Andrew said, this deviates from how WebRTC functions usually report errors. And it's OK to keep it for now if you promise to fix it in a follow-up CL.
https://codereview.webrtc.org/1404743003/diff/120001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/aec/echo_cancellation.c (right): https://codereview.webrtc.org/1404743003/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/aec/echo_cancellation.c:272: if (farend == NULL) On 2015/10/27 09:11:01, kwiberg-webrtc wrote: > if (!farend) Done. https://codereview.webrtc.org/1404743003/diff/120001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/aec/include/echo_cancellation.h (right): https://codereview.webrtc.org/1404743003/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/aec/include/echo_cancellation.h:112: * 12000-12050: error code On 2015/10/27 09:11:01, kwiberg-webrtc wrote: > As Andrew said, this deviates from how WebRTC functions usually report errors. > And it's OK to keep it for now if you promise to fix it in a follow-up CL. I'll put it on the todo list.
The CQ bit was checked by peah@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from solenberg@webrtc.org, henrik.lundin@webrtc.org, andrew@webrtc.org, kwiberg@webrtc.org Link to the patchset: https://codereview.webrtc.org/1404743003/#ps160001 (title: "Merge from other CLs in the list")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1404743003/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1404743003/160001
The CQ bit was unchecked by peah@webrtc.org
The CQ bit was checked by peah@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from solenberg@webrtc.org, henrik.lundin@webrtc.org, kwiberg@webrtc.org, andrew@webrtc.org Link to the patchset: https://codereview.webrtc.org/1404743003/#ps180001 (title: "Merge from preceeding CLs")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1404743003/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1404743003/180001
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
The patchset sent to the CQ was uploaded after l-g-t-m from solenberg@webrtc.org, henrik.lundin@webrtc.org, kwiberg@webrtc.org, andrew@webrtc.org Link to the patchset: https://codereview.webrtc.org/1404743003/#ps200001 (title: "Merge from latest master")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1404743003/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1404743003/200001
Message was sent while issue was closed.
Committed patchset #11 (id:200001)
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/c12be3984ff49f202f873f8ecd83f0e5b9cb36c9 Cr-Commit-Position: refs/heads/master@{#10570} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
