Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2527193002:
Use the new interface to get APM stats (Closed)
|
Created:
4 years ago by Ivo-OOO until feb 6 Modified:
4 years ago Reviewers:
hbos_chromium CC:
chromium-reviews, mlamouri+watch-content_chromium.org, imcheng+watch_chromium.org, posciak+watch_chromium.org, avayvod+watch_chromium.org, jam, feature-media-reviews_chromium.org, darin-cc_chromium.org, mcasas+watch+vc_chromium.org, jasonroberts+watch_google.com, xjz+watch_chromium.org, isheriff+watch_chromium.org, miu+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionUse the new statics interface to get APM stats from WebRTC
This also includes the new residual echo likelihood stat.
BUG=chromium:668632, webrtc:6525
Committed: https://crrev.com/b5bc4a4e2967f50790fb378c14d1af668b350448
Cr-Commit-Position: refs/heads/master@{#434493}
Patch Set 1 #
Total comments: 7
Patch Set 2 : Added TODO. #
Messages
Total messages: 18 (10 generated)
ivoc@chromium.org changed reviewers: + hbos@webrtc.org
Hi Henrik, here's a small CL to make use of the new interface to get stats from APM. PTAL.
Description was changed from ========== Use the new interface to get APM stats This also includes the new residual echo likelihood stat. BUG=webrtc:6525 ========== to ========== Use the new interface to get APM stats This also includes the new residual echo likelihood stat. BUG=chromium:668632,webrtc:6525 ==========
hbos@chromium.org changed reviewers: + hbos@chromium.org
https://codereview.chromium.org/2527193002/diff/1/content/renderer/media/medi... File content/renderer/media/media_stream_audio_processor_options.cc (left): https://codereview.chromium.org/2527193002/diff/1/content/renderer/media/medi... content/renderer/media/media_stream_audio_processor_options.cc:497: stats->aec_quality_min = -1.0f; I think that defaulting values to negative, whether here or in [1], is problematic. If webrtc::AudioProcessorInterface::AudioProcessorStats's stats can be optional, we should change them to rtc::Optional. The old stats collector (statscollector.h) just copies values blindly. The new stats collector (rtcstatscollector.h) supports leaving stats values undefined. But to do so it needs to know if a value is undefined, and Optional is better than some arbitrary/undocumented negative value. Could you do a follow-up WebRTC CL that indicate that optional statistics are optional? (Might be messy rolling into chromium) Alternatively, we could make "void MediaStreamAudioProcessor::GetStats" (overriding webrtc::AudioProcessor::GetStats) return false if stats are not available, if it's an all-or-nothing OP. Is it? Or, if undefined values are unexpected, DCHECK. [1] https://cs.chromium.org/chromium/src/third_party/webrtc/modules/audio_process... https://codereview.chromium.org/2527193002/diff/1/content/renderer/media/medi... File content/renderer/media/media_stream_audio_processor_options.cc (right): https://codereview.chromium.org/2527193002/diff/1/content/renderer/media/medi... content/renderer/media/media_stream_audio_processor_options.cc:489: apm_stats.echo_return_loss_enhancement.instant(); The new stats collector is only concerned with echoReturnLoss and echoReturnLossEnhancement out of all audio statistics: https://w3c.github.io/webrtc-stats/#dom-rtcmediastreamtrackstats-echoreturnloss These are calculated in decibels. Are these values using the same units?
hbos@chromium.org changed reviewers: - hbos@webrtc.org
Description was changed from ========== Use the new interface to get APM stats This also includes the new residual echo likelihood stat. BUG=chromium:668632,webrtc:6525 ========== to ========== Use the new statics interface to get APM stats from WebRTC This also includes the new residual echo likelihood stat. BUG=chromium:668632,webrtc:6525 ==========
https://codereview.chromium.org/2527193002/diff/1/content/renderer/media/medi... File content/renderer/media/media_stream_audio_processor_options.cc (left): https://codereview.chromium.org/2527193002/diff/1/content/renderer/media/medi... content/renderer/media/media_stream_audio_processor_options.cc:497: stats->aec_quality_min = -1.0f; On 2016/11/25 10:04:05, hbos_chromium wrote: > I think that defaulting values to negative, whether here or in [1], is > problematic. If webrtc::AudioProcessorInterface::AudioProcessorStats's stats can > be optional, we should change them to rtc::Optional. > > The old stats collector (statscollector.h) just copies values blindly. The new > stats collector (rtcstatscollector.h) supports leaving stats values undefined. > But to do so it needs to know if a value is undefined, and Optional is better > than some arbitrary/undocumented negative value. > > Could you do a follow-up WebRTC CL that indicate that optional statistics are > optional? (Might be messy rolling into chromium) > > Alternatively, we could make "void MediaStreamAudioProcessor::GetStats" > (overriding webrtc::AudioProcessor::GetStats) return false if stats are not > available, if it's an all-or-nothing OP. Is it? > > Or, if undefined values are unexpected, DCHECK. > > [1] > https://cs.chromium.org/chromium/src/third_party/webrtc/modules/audio_process... I agree that this way of using default values is not very nice, and using rtc::Optional sounds much better. I can do that in a follow-up CL (not sure if I have time for it right away). The reason the stats can be undefined is that the AEC can be disabled, so in that case an Optional is indeed a much nicer way to signal that. https://codereview.chromium.org/2527193002/diff/1/content/renderer/media/medi... File content/renderer/media/media_stream_audio_processor_options.cc (right): https://codereview.chromium.org/2527193002/diff/1/content/renderer/media/medi... content/renderer/media/media_stream_audio_processor_options.cc:489: apm_stats.echo_return_loss_enhancement.instant(); On 2016/11/25 10:04:05, hbos_chromium wrote: > The new stats collector is only concerned with echoReturnLoss and > echoReturnLossEnhancement out of all audio statistics: > https://w3c.github.io/webrtc-stats/#dom-rtcmediastreamtrackstats-echoreturnloss > > These are calculated in decibels. Are these values using the same units? These are in exactly the same units, in fact the audio_processing->GetStatistics() function calls the old functions internally. The reason for this change is that we want to get rid of direct pointers to the AEC outside of APM, since it makes the APM/AEC locking scheme more complicated than it needs to be.
lgtm https://codereview.chromium.org/2527193002/diff/1/content/renderer/media/medi... File content/renderer/media/media_stream_audio_processor_options.cc (left): https://codereview.chromium.org/2527193002/diff/1/content/renderer/media/medi... content/renderer/media/media_stream_audio_processor_options.cc:497: stats->aec_quality_min = -1.0f; On 2016/11/25 10:40:44, Ivo wrote: > On 2016/11/25 10:04:05, hbos_chromium wrote: > > I think that defaulting values to negative, whether here or in [1], is > > problematic. If webrtc::AudioProcessorInterface::AudioProcessorStats's stats > can > > be optional, we should change them to rtc::Optional. > > > > The old stats collector (statscollector.h) just copies values blindly. The new > > stats collector (rtcstatscollector.h) supports leaving stats values undefined. > > But to do so it needs to know if a value is undefined, and Optional is better > > than some arbitrary/undocumented negative value. > > > > Could you do a follow-up WebRTC CL that indicate that optional statistics are > > optional? (Might be messy rolling into chromium) > > > > Alternatively, we could make "void MediaStreamAudioProcessor::GetStats" > > (overriding webrtc::AudioProcessor::GetStats) return false if stats are not > > available, if it's an all-or-nothing OP. Is it? > > > > Or, if undefined values are unexpected, DCHECK. > > > > [1] > > > https://cs.chromium.org/chromium/src/third_party/webrtc/modules/audio_process... > > I agree that this way of using default values is not very nice, and using > rtc::Optional sounds much better. I can do that in a follow-up CL (not sure if I > have time for it right away). > > The reason the stats can be undefined is that the AEC can be disabled, so in > that case an Optional is indeed a much nicer way to signal that. That's fine, add a TODO comment? :) https://codereview.chromium.org/2527193002/diff/1/content/renderer/media/medi... File content/renderer/media/media_stream_audio_processor_options.cc (right): https://codereview.chromium.org/2527193002/diff/1/content/renderer/media/medi... content/renderer/media/media_stream_audio_processor_options.cc:489: apm_stats.echo_return_loss_enhancement.instant(); On 2016/11/25 10:40:44, Ivo wrote: > On 2016/11/25 10:04:05, hbos_chromium wrote: > > The new stats collector is only concerned with echoReturnLoss and > > echoReturnLossEnhancement out of all audio statistics: > > > https://w3c.github.io/webrtc-stats/#dom-rtcmediastreamtrackstats-echoreturnloss > > > > These are calculated in decibels. Are these values using the same units? > > These are in exactly the same units, in fact the > audio_processing->GetStatistics() function calls the old functions internally. > > The reason for this change is that we want to get rid of direct pointers to the > AEC outside of APM, since it makes the APM/AEC locking scheme more complicated > than it needs to be. Acknowledged.
Thanks! https://codereview.chromium.org/2527193002/diff/1/content/renderer/media/medi... File content/renderer/media/media_stream_audio_processor_options.cc (left): https://codereview.chromium.org/2527193002/diff/1/content/renderer/media/medi... content/renderer/media/media_stream_audio_processor_options.cc:497: stats->aec_quality_min = -1.0f; On 2016/11/25 12:51:39, hbos_chromium wrote: > On 2016/11/25 10:40:44, Ivo wrote: > > On 2016/11/25 10:04:05, hbos_chromium wrote: > > > I think that defaulting values to negative, whether here or in [1], is > > > problematic. If webrtc::AudioProcessorInterface::AudioProcessorStats's stats > > can > > > be optional, we should change them to rtc::Optional. > > > > > > The old stats collector (statscollector.h) just copies values blindly. The > new > > > stats collector (rtcstatscollector.h) supports leaving stats values > undefined. > > > But to do so it needs to know if a value is undefined, and Optional is > better > > > than some arbitrary/undocumented negative value. > > > > > > Could you do a follow-up WebRTC CL that indicate that optional statistics > are > > > optional? (Might be messy rolling into chromium) > > > > > > Alternatively, we could make "void MediaStreamAudioProcessor::GetStats" > > > (overriding webrtc::AudioProcessor::GetStats) return false if stats are not > > > available, if it's an all-or-nothing OP. Is it? > > > > > > Or, if undefined values are unexpected, DCHECK. > > > > > > [1] > > > > > > https://cs.chromium.org/chromium/src/third_party/webrtc/modules/audio_process... > > > > I agree that this way of using default values is not very nice, and using > > rtc::Optional sounds much better. I can do that in a follow-up CL (not sure if > I > > have time for it right away). > > > > The reason the stats can be undefined is that the AEC can be disabled, so in > > that case an Optional is indeed a much nicer way to signal that. > > That's fine, add a TODO comment? :) Done.
The CQ bit was checked by ivoc@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from hbos@chromium.org Link to the patchset: https://codereview.chromium.org/2527193002/#ps20001 (title: "Added TODO.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 20001, "attempt_start_ts": 1480080160727380,
"parent_rev": "d9c5104d8965a579d248d6e8f7417f4e4202ed4f", "commit_rev":
"3ce2a40979ef5e16855f01f86073a9f8fab80d94"}
Message was sent while issue was closed.
Description was changed from ========== Use the new statics interface to get APM stats from WebRTC This also includes the new residual echo likelihood stat. BUG=chromium:668632,webrtc:6525 ========== to ========== Use the new statics interface to get APM stats from WebRTC This also includes the new residual echo likelihood stat. BUG=chromium:668632,webrtc:6525 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Use the new statics interface to get APM stats from WebRTC This also includes the new residual echo likelihood stat. BUG=chromium:668632,webrtc:6525 ========== to ========== Use the new statics interface to get APM stats from WebRTC This also includes the new residual echo likelihood stat. BUG=chromium:668632,webrtc:6525 Committed: https://crrev.com/b5bc4a4e2967f50790fb378c14d1af668b350448 Cr-Commit-Position: refs/heads/master@{#434493} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/b5bc4a4e2967f50790fb378c14d1af668b350448 Cr-Commit-Position: refs/heads/master@{#434493} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
