Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1469803004:
Don't overwrite audio stats when they're not available. (Closed)
|
Created:
5 years ago by Andrew MacDonald Modified:
5 years ago CC:
niklas.enbom, peah-webrtc, qiang.lu, tterriberry_mozilla.com, webrtc-reviews_webrtc.org, yujie_mao (webrtc) Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionDon't overwrite audio stats when they're not available.
Chromium implements AudioProcessorInterface::GetStats(), but other
clients may not. The existing stats were getting overwritten with
default AudioProcessorStats values in that case.
Now, we only overwrite the stats if the track has an
AudioProcessorInterface. Also, move signal level out of
SetAudioProcessingStats() to avoid the "don't set if it's -1" pattern.
Committed: https://crrev.com/2fe1cb0f0acb66e6f8df47365aac816cb69eb911
Cr-Commit-Position: refs/heads/master@{#10831}
Patch Set 1 #Patch Set 2 : Move audio level to list. #
Total comments: 8
Patch Set 3 : Add DCHECK for audio level. #Messages
Total messages: 21 (7 generated)
Description was changed from ========== Don't overwrite audio stats when they're not available. Chromium implements AudioProcessorInterface::GetStats(), but other native clients may not. The existing stats were getting overwritten with default AudioProcessorStats values in that case. Now, we only overwrite the stats if the track has an AudioProcessorInterface. Also, move signal level out of SetAudioProcessingStats() to avoid the "don't set if it's -1" pattern. ========== to ========== Don't overwrite audio stats when they're not available. Chromium implements AudioProcessorInterface::GetStats(), but other clients may not. The existing stats were getting overwritten with default AudioProcessorStats values in that case. Now, we only overwrite the stats if the track has an AudioProcessorInterface. Also, move signal level out of SetAudioProcessingStats() to avoid the "don't set if it's -1" pattern. ==========
andrew@webrtc.org changed reviewers: + solenberg@webrtc.org, tommi@webrtc.org
https://codereview.webrtc.org/1469803004/diff/20001/talk/app/webrtc/statscoll... File talk/app/webrtc/statscollector.cc (right): https://codereview.webrtc.org/1469803004/diff/20001/talk/app/webrtc/statscoll... talk/app/webrtc/statscollector.cc:191: { StatsReport::kStatsValueNameAudioInputLevel, info.audio_level}, this doesn't handle (or rather, ignore) negative signal values like the current implementation does. Intentional?
https://codereview.webrtc.org/1469803004/diff/20001/talk/app/webrtc/statscoll... File talk/app/webrtc/statscollector.cc (right): https://codereview.webrtc.org/1469803004/diff/20001/talk/app/webrtc/statscoll... talk/app/webrtc/statscollector.cc:191: { StatsReport::kStatsValueNameAudioInputLevel, info.audio_level}, On 2015/11/24 20:29:59, tommi (webrtc) wrote: > this doesn't handle (or rather, ignore) negative signal values like the current > implementation does. Intentional? Yes. I assumed (perhaps incorrectly) that this was only needed to workaround GetSignalLevel failing on line 895 in the original code. Effectively, the signal level stat continued to work outside of Chrome because of this, but the audio processing stats didn't. I thought it best to make the behavior consistent. Do you know of any other reason why we would get a negative signal level here that we shouldn't report?
https://codereview.webrtc.org/1469803004/diff/20001/talk/app/webrtc/statscoll... File talk/app/webrtc/statscollector.cc (right): https://codereview.webrtc.org/1469803004/diff/20001/talk/app/webrtc/statscoll... talk/app/webrtc/statscollector.cc:191: { StatsReport::kStatsValueNameAudioInputLevel, info.audio_level}, On 2015/11/24 22:43:54, Andrew MacDonald wrote: > On 2015/11/24 20:29:59, tommi (webrtc) wrote: > > this doesn't handle (or rather, ignore) negative signal values like the > current > > implementation does. Intentional? > > Yes. I assumed (perhaps incorrectly) that this was only needed to workaround > GetSignalLevel failing on line 895 in the original code. Effectively, the signal > level stat continued to work outside of Chrome because of this, but the audio > processing stats didn't. > > I thought it best to make the behavior consistent. Do you know of any other > reason why we would get a negative signal level here that we shouldn't report? AFAICT VoiceSenderInfo::audio_level cannot be negative now that we fetch it from AudioSendStream (which doesn't early-out while collecting stats). The default AudioSendStream::Stats object sets audio_level to -1, but we should never have a default object in practice. https://codereview.webrtc.org/1469803004/diff/20001/talk/app/webrtc/statscoll... talk/app/webrtc/statscollector.cc:898: if (track->GetSignalLevel(&signal_level)) AudioTrackInterface::GetSignalLevel() doesn't have a real ´ implementation (not in WebRTC, not in chromium/src/chrome). It will always return false.
https://codereview.webrtc.org/1469803004/diff/20001/talk/app/webrtc/statscoll... File talk/app/webrtc/statscollector.cc (right): https://codereview.webrtc.org/1469803004/diff/20001/talk/app/webrtc/statscoll... talk/app/webrtc/statscollector.cc:191: { StatsReport::kStatsValueNameAudioInputLevel, info.audio_level}, On 2015/11/25 13:02:17, the sun wrote: > On 2015/11/24 22:43:54, Andrew MacDonald wrote: > > On 2015/11/24 20:29:59, tommi (webrtc) wrote: > > > this doesn't handle (or rather, ignore) negative signal values like the > > current > > > implementation does. Intentional? > > > > Yes. I assumed (perhaps incorrectly) that this was only needed to workaround > > GetSignalLevel failing on line 895 in the original code. Effectively, the > signal > > level stat continued to work outside of Chrome because of this, but the audio > > processing stats didn't. > > > > I thought it best to make the behavior consistent. Do you know of any other > > reason why we would get a negative signal level here that we shouldn't report? > > > AFAICT VoiceSenderInfo::audio_level cannot be negative now that we fetch it from > AudioSendStream (which doesn't early-out while collecting stats). The default > AudioSendStream::Stats object sets audio_level to -1, but we should never have a > default object in practice. Thanks Fredrik. Can we have a DCHECK() here then that info.audio_level >= 0? https://codereview.webrtc.org/1469803004/diff/20001/talk/app/webrtc/statscoll... talk/app/webrtc/statscollector.cc:898: if (track->GetSignalLevel(&signal_level)) On 2015/11/25 13:02:17, the sun wrote: > AudioTrackInterface::GetSignalLevel() doesn't have a real ´ implementation (not > in WebRTC, not in chromium/src/chrome). It will always return false. Here it is: https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/m...
https://codereview.webrtc.org/1469803004/diff/20001/talk/app/webrtc/statscoll... File talk/app/webrtc/statscollector.cc (right): https://codereview.webrtc.org/1469803004/diff/20001/talk/app/webrtc/statscoll... talk/app/webrtc/statscollector.cc:191: { StatsReport::kStatsValueNameAudioInputLevel, info.audio_level}, On 2015/11/25 13:11:09, tommi (webrtc) wrote: > Thanks Fredrik. Can we have a DCHECK() here then that info.audio_level >= 0? Sure, done.
lgtm https://codereview.webrtc.org/1469803004/diff/20001/talk/app/webrtc/statscoll... File talk/app/webrtc/statscollector.cc (right): https://codereview.webrtc.org/1469803004/diff/20001/talk/app/webrtc/statscoll... talk/app/webrtc/statscollector.cc:898: if (track->GetSignalLevel(&signal_level)) On 2015/11/25 13:11:09, tommi (webrtc) wrote: > On 2015/11/25 13:02:17, the sun wrote: > > AudioTrackInterface::GetSignalLevel() doesn't have a real ´ implementation > (not > > in WebRTC, not in chromium/src/chrome). It will always return false. > > Here it is: > https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/m... Ah, thanks
oops, sorry, thought I lgtm-ed it in my last reply.
On 2015/11/26 16:47:40, tommi (webrtc) wrote: > oops, sorry, thought I lgtm-ed it in my last reply. No worries. Thanks guys!
The CQ bit was checked by andrew@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1469803004/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1469803004/40001
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) android_rel on tryserver.webrtc (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by andrew@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1469803004/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1469803004/40001
Message was sent while issue was closed.
Description was changed from ========== Don't overwrite audio stats when they're not available. Chromium implements AudioProcessorInterface::GetStats(), but other clients may not. The existing stats were getting overwritten with default AudioProcessorStats values in that case. Now, we only overwrite the stats if the track has an AudioProcessorInterface. Also, move signal level out of SetAudioProcessingStats() to avoid the "don't set if it's -1" pattern. ========== to ========== Don't overwrite audio stats when they're not available. Chromium implements AudioProcessorInterface::GetStats(), but other clients may not. The existing stats were getting overwritten with default AudioProcessorStats values in that case. Now, we only overwrite the stats if the track has an AudioProcessorInterface. Also, move signal level out of SetAudioProcessingStats() to avoid the "don't set if it's -1" pattern. ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Don't overwrite audio stats when they're not available. Chromium implements AudioProcessorInterface::GetStats(), but other clients may not. The existing stats were getting overwritten with default AudioProcessorStats values in that case. Now, we only overwrite the stats if the track has an AudioProcessorInterface. Also, move signal level out of SetAudioProcessingStats() to avoid the "don't set if it's -1" pattern. ========== to ========== Don't overwrite audio stats when they're not available. Chromium implements AudioProcessorInterface::GetStats(), but other clients may not. The existing stats were getting overwritten with default AudioProcessorStats values in that case. Now, we only overwrite the stats if the track has an AudioProcessorInterface. Also, move signal level out of SetAudioProcessingStats() to avoid the "don't set if it's -1" pattern. Committed: https://crrev.com/2fe1cb0f0acb66e6f8df47365aac816cb69eb911 Cr-Commit-Position: refs/heads/master@{#10831} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/2fe1cb0f0acb66e6f8df47365aac816cb69eb911 Cr-Commit-Position: refs/heads/master@{#10831} |
