Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1446173002:
Fetch goog_jitter_received stats for receive stream in WebRTC perf browser test. (Closed)
|
Created:
5 years, 1 month ago by Solis Modified:
5 years, 1 month ago CC:
chromium-reviews, posciak+watch_chromium.org, feature-media-reviews_chromium.org, tnakamura+watch_chromium.org, phoglund+watch_chromium.org, mcasas+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFetch goog_jitter_received stats for receive stream in WebRTC perf browser test.
Since the default voice send channel has been removed, this statistic will no longer be fetched by calling MaybePrintResultsForAudioSend() on the SSRC in the receiver tab of the one sender/one receiver perf test.
BUG=chromium:547661
Committed: https://crrev.com/cdc49ef70f07ad42de07276b8e2a4fc90872cdc0
Cr-Commit-Position: refs/heads/master@{#360046}
Patch Set 1 #
Total comments: 4
Messages
Total messages: 17 (3 generated)
solenberg@chromium.org changed reviewers: + hta@chromium.org, phoglund@chromium.org
Description was changed from ========== Fetch goog_jitter_received stats for receive stream in WebRTC perf browser test. BUG=chromium:547661 ========== to ========== Fetch goog_jitter_received stats for receive stream in WebRTC perf browser test. Since the default voice send channel has been removed, this statistic will no longer be fetched by calling MaybePrintResultsForAudioSend() on the SSRC in the receiver tab of the one sender/one receiver perf test. BUG=chromium:547661 ==========
https://codereview.chromium.org/1446173002/diff/1/chrome/browser/media/webrtc... File chrome/browser/media/webrtc_browsertest_perf.cc (right): https://codereview.chromium.org/1446173002/diff/1/chrome/browser/media/webrtc... chrome/browser/media/webrtc_browsertest_perf.cc:72: "audio_tx", modifier, "goog_jitter_recv", value, "ms", false); Then you should remove this, no?
https://codereview.chromium.org/1446173002/diff/1/chrome/browser/media/webrtc... File chrome/browser/media/webrtc_browsertest_perf.cc (right): https://codereview.chromium.org/1446173002/diff/1/chrome/browser/media/webrtc... chrome/browser/media/webrtc_browsertest_perf.cc:72: "audio_tx", modifier, "goog_jitter_recv", value, "ms", false); On 2015/11/16 14:06:35, phoglund_chrome wrote: > Then you should remove this, no? Nope, that's the estimated jitter for send streams (as reported back by receiver through SSRC recever reports).
Ok, ship it. lgtm
https://codereview.chromium.org/1446173002/diff/1/chrome/browser/media/webrtc... File chrome/browser/media/webrtc_browsertest_perf.cc (right): https://codereview.chromium.org/1446173002/diff/1/chrome/browser/media/webrtc... chrome/browser/media/webrtc_browsertest_perf.cc:72: "audio_tx", modifier, "goog_jitter_recv", value, "ms", false); On 2015/11/16 14:46:15, Solis wrote: > On 2015/11/16 14:06:35, phoglund_chrome wrote: > > Then you should remove this, no? > > Nope, that's the estimated jitter for send streams (as reported back by receiver > through SSRC recever reports). We should consider calling this something else though; it's maybe a bit confusing that there are two goog_jitter_recv in different buckets. Maybe send_estimated_goog_jitter_recv?...
https://codereview.chromium.org/1446173002/diff/1/chrome/browser/media/webrtc... File chrome/browser/media/webrtc_browsertest_perf.cc (right): https://codereview.chromium.org/1446173002/diff/1/chrome/browser/media/webrtc... chrome/browser/media/webrtc_browsertest_perf.cc:72: "audio_tx", modifier, "goog_jitter_recv", value, "ms", false); On 2015/11/16 14:57:58, phoglund_chrome wrote: > On 2015/11/16 14:46:15, Solis wrote: > > On 2015/11/16 14:06:35, phoglund_chrome wrote: > > > Then you should remove this, no? > > > > Nope, that's the estimated jitter for send streams (as reported back by > receiver > > through SSRC recever reports). > > We should consider calling this something else though; it's maybe a bit > confusing that there are two goog_jitter_recv in different buckets. Maybe > send_estimated_goog_jitter_recv?... It is confusing for me as well. Which is why I added hta@ to this CL. Not sure how much of a standard the goog_ stats are, how swiftly they can be changed and such...
On 2015/11/16 15:03:10, Solis wrote: > https://codereview.chromium.org/1446173002/diff/1/chrome/browser/media/webrtc... > File chrome/browser/media/webrtc_browsertest_perf.cc (right): > > https://codereview.chromium.org/1446173002/diff/1/chrome/browser/media/webrtc... > chrome/browser/media/webrtc_browsertest_perf.cc:72: "audio_tx", modifier, > "goog_jitter_recv", value, "ms", false); > On 2015/11/16 14:57:58, phoglund_chrome wrote: > > On 2015/11/16 14:46:15, Solis wrote: > > > On 2015/11/16 14:06:35, phoglund_chrome wrote: > > > > Then you should remove this, no? > > > > > > Nope, that's the estimated jitter for send streams (as reported back by > > receiver > > > through SSRC recever reports). > > > > We should consider calling this something else though; it's maybe a bit > > confusing that there are two goog_jitter_recv in different buckets. Maybe > > send_estimated_goog_jitter_recv?... > > It is confusing for me as well. Which is why I added hta@ to this CL. > > Not sure how much of a standard the goog_ stats are, how swiftly they can be > changed and such... They don't change often. Maybe it should switch name in the implementation as well, but for now you can just change goog_jitter_recv (we can name it anything we want, it's just been the name of the stat by convention so far).
On 2015/11/16 15:15:43, phoglund_chrome wrote: > On 2015/11/16 15:03:10, Solis wrote: > > > https://codereview.chromium.org/1446173002/diff/1/chrome/browser/media/webrtc... > > File chrome/browser/media/webrtc_browsertest_perf.cc (right): > > > > > https://codereview.chromium.org/1446173002/diff/1/chrome/browser/media/webrtc... > > chrome/browser/media/webrtc_browsertest_perf.cc:72: "audio_tx", modifier, > > "goog_jitter_recv", value, "ms", false); > > On 2015/11/16 14:57:58, phoglund_chrome wrote: > > > On 2015/11/16 14:46:15, Solis wrote: > > > > On 2015/11/16 14:06:35, phoglund_chrome wrote: > > > > > Then you should remove this, no? > > > > > > > > Nope, that's the estimated jitter for send streams (as reported back by > > > receiver > > > > through SSRC recever reports). > > > > > > We should consider calling this something else though; it's maybe a bit > > > confusing that there are two goog_jitter_recv in different buckets. Maybe > > > send_estimated_goog_jitter_recv?... > > > > It is confusing for me as well. Which is why I added hta@ to this CL. > > > > Not sure how much of a standard the goog_ stats are, how swiftly they can be > > changed and such... > > They don't change often. Maybe it should switch name in the implementation as > well, but for now you can just change goog_jitter_recv (we can name it anything > we want, it's just been the name of the stat by convention so far). Hmm, well, the full name is audio_tx_sendonly / goog_jitter_recv (the other one audio_rx_recvonly / goog_jitter_recv). Changing the name seems redundant in that perspective. "goog_jitter_recv" is good because it matches the stat but the "recv" part is confusing since the metric is really about the traffic an endpoint is sending. If you still want me to change one of the names I'd rather change that for audio_rx, since the time series there is already broken.
In the stats specification, the name is simply "jitter", and it exists on RTCInboundRTPStreamStats. For an outgoing local stream, you would find the jitter by looking for the stream stats with id = associateStatsId of the outgoing stream, which will have isRemote = true. On Mon, Nov 16, 2015 at 5:24 PM, <solenberg@chromium.org> wrote: > On 2015/11/16 15:15:43, phoglund_chrome wrote: > >> On 2015/11/16 15:03:10, Solis wrote: >> > >> > > > https://codereview.chromium.org/1446173002/diff/1/chrome/browser/media/webrtc... > >> > File chrome/browser/media/webrtc_browsertest_perf.cc (right): >> > >> > >> > > > https://codereview.chromium.org/1446173002/diff/1/chrome/browser/media/webrtc... > >> > chrome/browser/media/webrtc_browsertest_perf.cc:72: "audio_tx", >> modifier, >> > "goog_jitter_recv", value, "ms", false); >> > On 2015/11/16 14:57:58, phoglund_chrome wrote: >> > > On 2015/11/16 14:46:15, Solis wrote: >> > > > On 2015/11/16 14:06:35, phoglund_chrome wrote: >> > > > > Then you should remove this, no? >> > > > >> > > > Nope, that's the estimated jitter for send streams (as reported >> back by >> > > receiver >> > > > through SSRC recever reports). >> > > >> > > We should consider calling this something else though; it's maybe a >> bit >> > > confusing that there are two goog_jitter_recv in different buckets. >> Maybe >> > > send_estimated_goog_jitter_recv?... >> > >> > It is confusing for me as well. Which is why I added hta@ to this CL. >> > >> > Not sure how much of a standard the goog_ stats are, how swiftly they >> can be >> > changed and such... >> > > They don't change often. Maybe it should switch name in the implementation >> as >> well, but for now you can just change goog_jitter_recv (we can name it >> > anything > >> we want, it's just been the name of the stat by convention so far). >> > > Hmm, well, the full name is audio_tx_sendonly / goog_jitter_recv (the > other one > audio_rx_recvonly / goog_jitter_recv). Changing the name seems redundant > in that > perspective. > > "goog_jitter_recv" is good because it matches the stat but the "recv" part > is > confusing since the metric is really about the traffic an endpoint is > sending. > If you still want me to change one of the names I'd rather change that for > audio_rx, since the time series there is already broken. > > https://codereview.chromium.org/1446173002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Fair enough. You have some options, including leaving it as is. All of them seem equally good/bad so it's your call.
On 2015/11/16 18:36:12, phoglund_chrome wrote: > Fair enough. You have some options, including leaving it as is. All of them seem > equally good/bad so it's your call. Then I'm keeping goog_jitter_recv, since that is the name used in webrtc-internals.
The CQ bit was checked by solenberg@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1446173002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1446173002/1
Message was sent while issue was closed.
Committed patchset #1 (id:1)
Message was sent while issue was closed.
Patchset 1 (id:??) landed as https://crrev.com/cdc49ef70f07ad42de07276b8e2a4fc90872cdc0 Cr-Commit-Position: refs/heads/master@{#360046} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
