Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2582043002:
Fixing init time error in smoothing filter. (Closed)
|
Created:
4 years ago by minyue-webrtc Modified:
3 years, 11 months ago CC:
webrtc-reviews_webrtc.org, kwiberg-webrtc, Andrew MacDonald, henrika_webrtc, tlegrand-webrtc, tterriberry_mozilla.com, audio-team_agora.io, hlundin-webrtc, peah-webrtc, the sun, aluebs-webrtc, bjornv1 Target Ref:
refs/heads/master Project:
webrtc Visibility:
Public. |
DescriptionFixing init time error in smoothing filter.
BUG=webrtc:6909, webrtc:6303
TBR=tina.legrand@webrtc.org
Review-Url: https://codereview.webrtc.org/2582043002
Cr-Commit-Position: refs/heads/master@{#15817}
Committed: https://chromium.googlesource.com/external/webrtc/+/7667db4a74ce630c19fff9e0ff953d1990b6301d
Patch Set 1 #Patch Set 2 : fixing #
Total comments: 1
Patch Set 3 : reducing scope #
Total comments: 4
Patch Set 4 : small fixes #
Total comments: 6
Patch Set 5 : on Ivo's comments #
Messages
Total messages: 28 (15 generated)
Description was changed from ========== Fixing smoothing filter related errors. BUG=webrtc:6909, webrtc:6303 ========== to ========== Fixing smoothing filter related errors. BUG=webrtc:6909, webrtc:6303 ==========
minyue@webrtc.org changed reviewers: + michaelt@webrtc.org
hi Michael, Could you take a look at this?
Hi Michael, PTAL again. https://codereview.webrtc.org/2582043002/diff/20001/webrtc/common_audio/smoot... File webrtc/common_audio/smoothing_filter.cc (right): https://codereview.webrtc.org/2582043002/diff/20001/webrtc/common_audio/smoot... webrtc/common_audio/smoothing_filter.cc:17: SmoothingFilterImpl::SmoothingFilterImpl(int init_time_ms, const Clock* clock) this was an old error, no actual problem, but bad style.
Description was changed from ========== Fixing smoothing filter related errors. BUG=webrtc:6909, webrtc:6303 ========== to ========== Fixing init time error in smoothing filter. BUG=webrtc:6909, webrtc:6303 ==========
Hi Michael, As said, we'd like to reduce the scope of the this CL. PTAL again. https://codereview.webrtc.org/2582043002/diff/40001/webrtc/common_audio/smoot... File webrtc/common_audio/smoothing_filter.cc (right): https://codereview.webrtc.org/2582043002/diff/40001/webrtc/common_audio/smoot... webrtc/common_audio/smoothing_filter.cc:20: // alpha(n) = exp(pow(init_factor_, n)), has to be -pow(..., will change https://codereview.webrtc.org/2582043002/diff/40001/webrtc/common_audio/smoot... webrtc/common_audio/smoothing_filter.cc:24: -1.0f / init_time_ms_)), This is an error that makes init_factor_ different from the appendix. The code is right, but not the comments. https://codereview.webrtc.org/2582043002/diff/40001/webrtc/common_audio/smoot... webrtc/common_audio/smoothing_filter.cc:83: // alpha(n) = exp(pow(init_factor_, n)), has to be -pow(... https://codereview.webrtc.org/2582043002/diff/40001/webrtc/common_audio/smoot... File webrtc/common_audio/smoothing_filter.h (right): https://codereview.webrtc.org/2582043002/diff/40001/webrtc/common_audio/smoot... webrtc/common_audio/smoothing_filter.h:62: rtc::Optional<int64_t> init_end_time_ms_; this is a refactor to make the code cleaner.
minyue@webrtc.org changed reviewers: + ivoc@webrtc.org
Hi Ivo, Would you help review this CL? This is to fix an error in a recently introduced smoothing filter. The error is that when init_time_ms is 0 or 1, the equation does not hold any longer. I also found errors in the comment. Fixed altogether.
I didn't check the math, since I'm not familiar enough with this class. I did have a few comments/questions, see below. https://codereview.webrtc.org/2582043002/diff/60001/webrtc/common_audio/smoot... File webrtc/common_audio/smoothing_filter.cc (right): https://codereview.webrtc.org/2582043002/diff/60001/webrtc/common_audio/smoot... webrtc/common_audio/smoothing_filter.cc:23: init_factor_(init_time_ms_ == 0 ? 0.0f : pow(init_time_ms_, Since this is a float we can use powf instead of pow (which is for double precision). https://codereview.webrtc.org/2582043002/diff/60001/webrtc/common_audio/smoot... webrtc/common_audio/smoothing_filter.cc:29: : init_time_ms_ - The "1.0f / " part is gone now, is that on purpose? (I don't understand the class well enough to judge) https://codereview.webrtc.org/2582043002/diff/60001/webrtc/common_audio/smoot... webrtc/common_audio/smoothing_filter.cc:90: } else if (init_factor_ == 1.0f) { This happens when init_time_ms_ == 1, right? Is the pow operation precise enough for this to be an exact equality instead of checking it is within some range?
Thanks for the comments! see the new patch set. https://codereview.webrtc.org/2582043002/diff/60001/webrtc/common_audio/smoot... File webrtc/common_audio/smoothing_filter.cc (right): https://codereview.webrtc.org/2582043002/diff/60001/webrtc/common_audio/smoot... webrtc/common_audio/smoothing_filter.cc:23: init_factor_(init_time_ms_ == 0 ? 0.0f : pow(init_time_ms_, On 2016/12/27 16:09:51, ivoc wrote: > Since this is a float we can use powf instead of pow (which is for double > precision). oh, cool, I did not know powf. Thanks! https://codereview.webrtc.org/2582043002/diff/60001/webrtc/common_audio/smoot... webrtc/common_audio/smoothing_filter.cc:29: : init_time_ms_ - On 2016/12/27 16:09:51, ivoc wrote: > The "1.0f / " part is gone now, is that on purpose? (I don't understand the > class well enough to judge) it is intended. Otherwise, 1/0.0f will look ugly. https://codereview.webrtc.org/2582043002/diff/60001/webrtc/common_audio/smoot... webrtc/common_audio/smoothing_filter.cc:90: } else if (init_factor_ == 1.0f) { On 2016/12/27 16:09:51, ivoc wrote: > This happens when init_time_ms_ == 1, right? Is the pow operation precise enough > for this to be an exact equality instead of checking it is within some range? according to the sum of geometric series: alpha^n, if alpha = 1, the equation on line 93 will fail, otherwise, line 93 can handle it. Since init_factor = pow(init_time, -1.0f / init_time); the only case that it can equal 1.0f is when init_time = 1, or init_time = inf. since init_time is int. it is indeed better to branch upon init_time.
Great, lgtm code-wise.
The CQ bit was checked by minyue@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/12/28 08:46:33, ivoc wrote: > Great, lgtm code-wise. Hi Michael, I submit this CL now. If you find any error when you come back from holiday, I can continue to fix.
The CQ bit was checked by minyue@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: presubmit on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/11787)
Description was changed from ========== Fixing init time error in smoothing filter. BUG=webrtc:6909, webrtc:6303 ========== to ========== Fixing init time error in smoothing filter. BUG=webrtc:6909, webrtc:6303 TBR=tina.legrand@webrtc.org ==========
minyue@webrtc.org changed reviewers: + tina.legrand@webrtc.org
On 2016/12/28 10:05:26, commit-bot: I haz the power wrote: > Try jobs failed on following builders: > presubmit on master.tryserver.webrtc (JOB_FAILED, > http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/11787) +Tina, Hi Tina, I add you in TBR for owner's approve. This is to fix a bug. Michael has discussed offline with me and Ivo has reviewed it. To avoid blocking the fix, I would like to submit it now. If you find any issue later, I can revert it and/or continue fix.
The CQ bit was checked by minyue@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
CQ is committing da patch.
Bot data: {"patchset_id": 80001, "attempt_start_ts": 1482922547100900,
"parent_rev": "bee671297a84409682ff59a46a7d9327a00e714f", "commit_rev":
"7667db4a74ce630c19fff9e0ff953d1990b6301d"}
Message was sent while issue was closed.
Description was changed from ========== Fixing init time error in smoothing filter. BUG=webrtc:6909, webrtc:6303 TBR=tina.legrand@webrtc.org ========== to ========== Fixing init time error in smoothing filter. BUG=webrtc:6909, webrtc:6303 TBR=tina.legrand@webrtc.org Review-Url: https://codereview.webrtc.org/2582043002 Cr-Commit-Position: refs/heads/master@{#15817} Committed: https://chromium.googlesource.com/external/webrtc/+/7667db4a74ce630c19fff9e0f... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/external/webrtc/+/7667db4a74ce630c19fff9e0f... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
