WebRTC Code Reviews
Help | Chromium Project | Sign in
(11124)

Issue 16459004: Allow the RTP level indicator computation to work at any sample rate. (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
5 years, 3 months ago by ajm
Modified:
5 years, 3 months ago
CC:
webrtc-reviews_webrtc.org, rillian-moz, henrika_webrtc, ajm, tterriberry, leozwang1, Bjorn
Base URL:
https://webrtc.googlecode.com/svn/trunk
Visibility:
Public.

Description

Allow the RTP level indicator computation to work at any sample rate. Break out the computation to a separate class, and call directly into this from channel.cc rather than going through AudioProcessing. This circumvents AudioProcessing's sample rate limitations. We now compute the RMS over all samples rather than downmixing to a single channel. This makes the call point in channel.cc easier, is more "correct" and should have similar (negligible) complexity. This caused slight changes in the RMS output, so the ApmTest.Process reference has been updated. Snippet of the failing output: [ RUN ] ApmTest.Process Running test 4 of 12... Value of: rms_level Actual: 27 Expected: test->rms_level() Which is: 28 Running test 5 of 12... Value of: rms_level Actual: 26 Expected: test->rms_level() Which is: 27 Running test 6 of 12... Value of: rms_level Actual: 26 Expected: test->rms_level() Which is: 27 Running test 10 of 12... Value of: rms_level Actual: 27 Expected: test->rms_level() Which is: 28 Running test 11 of 12... Value of: rms_level Actual: 26 Expected: test->rms_level() Which is: 27 Running test 12 of 12... Value of: rms_level Actual: 26 Expected: test->rms_level() Which is: 27 BUG=3290 TESTED=Chrome assert is avoided and both voe_cmd_test and apprtc produce reasonable printed out results from RMS(). R=bjornv@webrtc.org Committed: https://code.google.com/p/webrtc/source/detail?r=6056

Patch Set 1 : #

Total comments: 17

Patch Set 2 : Review comments. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+147 lines, -126 lines) Patch
M data/audio_processing/output_data_float.pb View Binary file 0 comments Download
M webrtc/modules/audio_processing/audio_processing.gypi View 1 chunk +2 lines, -0 lines 0 comments Download
M webrtc/modules/audio_processing/include/audio_processing.h View 1 chunk +1 line, -2 lines 0 comments Download
M webrtc/modules/audio_processing/level_estimator_impl.h View 2 chunks +1 line, -1 line 0 comments Download
M webrtc/modules/audio_processing/level_estimator_impl.cc View 1 2 chunks +23 lines, -99 lines 0 comments Download
A webrtc/modules/audio_processing/rms_level.h View 1 1 chunk +51 lines, -0 lines 0 comments Download
A webrtc/modules/audio_processing/rms_level.cc View 1 1 chunk +61 lines, -0 lines 0 comments Download
M webrtc/voice_engine/channel.h View 3 chunks +5 lines, -4 lines 0 comments Download
M webrtc/voice_engine/channel.cc View 1 3 chunks +3 lines, -20 lines 0 comments Download
Trybot results:

Messages

Total messages: 6 (0 generated)
ajm
http://review.webrtc.org/16459004/diff/40001/webrtc/modules/audio_processing/rms_level.cc File webrtc/modules/audio_processing/rms_level.cc (right): http://review.webrtc.org/16459004/diff/40001/webrtc/modules/audio_processing/rms_level.cc#newcode42 webrtc/modules/audio_processing/rms_level.cc:42: int RMSLevel::RMS() { This is identical to the previous ...
5 years, 3 months ago (2014-05-03 01:29:28 UTC) #1
Bjorn
lgtm % nits http://review.webrtc.org/16459004/diff/40001/webrtc/modules/audio_processing/level_estimator_impl.cc File webrtc/modules/audio_processing/level_estimator_impl.cc (right): http://review.webrtc.org/16459004/diff/40001/webrtc/modules/audio_processing/level_estimator_impl.cc#newcode27 webrtc/modules/audio_processing/level_estimator_impl.cc:27: return AudioProcessing::kNoError; Doesn't this require an ...
5 years, 3 months ago (2014-05-05 13:09:50 UTC) #2
no longer working on webrtc
Thanks Andrew for acting quickly. Some comments from me. http://review.webrtc.org/16459004/diff/40001/webrtc/modules/audio_processing/level_estimator_impl.cc File webrtc/modules/audio_processing/level_estimator_impl.cc (right): http://review.webrtc.org/16459004/diff/40001/webrtc/modules/audio_processing/level_estimator_impl.cc#newcode35 webrtc/modules/audio_processing/level_estimator_impl.cc:35: ...
5 years, 3 months ago (2014-05-05 13:26:40 UTC) #3
ajm
Karl, I'm committing this, but if you still want to have a look will address ...
5 years, 3 months ago (2014-05-05 18:21:16 UTC) #4
ajm
http://review.webrtc.org/16459004/diff/40001/webrtc/modules/audio_processing/rms_level.h File webrtc/modules/audio_processing/rms_level.h (right): http://review.webrtc.org/16459004/diff/40001/webrtc/modules/audio_processing/rms_level.h#newcode25 webrtc/modules/audio_processing/rms_level.h:25: static const int kMinLevel = 127; On 2014/05/05 13:26:41, ...
5 years, 3 months ago (2014-05-05 18:22:11 UTC) #5
no longer working on webrtc
5 years, 3 months ago (2014-05-05 18:24:06 UTC) #6
lgtm
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 245c2c2-tainted