Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(7)

Issue 2979833002: Add a histogram metric tracking for how long audio RTP packets are sent (Closed)

Created:
3 years, 5 months ago by saza WebRTC
Modified:
3 years, 5 months ago
Reviewers:
ossu, pbos-webrtc
CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com, the sun, stefan-webrtc, mflodman, tlegrand-webrtc, henrika_webrtc
Target Ref:
refs/heads/master
Project:
webrtc
Visibility:
Public.

Description

Adds a histogram metric tracking for how long audio RTP packets are sent through streams related to a call object. The Call object does not know directly when packets pass through it, only which AudioSendStreams are used. Each AudioSendStream has a pointer to the Transport object through which its packets are send. This CL: By registering an internal wrapper class, TimedTransport, the AudioSendStream can stay up-to-date on when packets have passed through its Transport. This lifetime (as an interval) is then queried by the Call when the AudioSendStream is destroyed. When Call is destroyed, all streams are guaranteed to have been destroyed and hence Call is up-to-date on packet activity. The class TimeInterval keeps the code in Call and AudioSendStream smaller, with fewer get methods in their APIs and less code for updating values. Also modifies the unit test for AudioSendStream: it previously enforced that the stream registers (with its channel proxy) the same transport that it was constructed with. BUG=webrtc:7882 Review-Url: https://codereview.webrtc.org/2979833002 Cr-Commit-Position: refs/heads/master@{#19087} Committed: https://chromium.googlesource.com/external/webrtc/+/c58f8c096213a5e97554a1576cc4b51d1a98c4c4

Patch Set 1 #

Total comments: 20

Patch Set 2 : Adjust for comments. #

Total comments: 1
Unified diffs Side-by-side diffs Delta from patch set Stats (+223 lines, -2 lines) Patch
M webrtc/audio/BUILD.gn View 1 2 chunks +3 lines, -0 lines 0 comments Download
M webrtc/audio/audio_send_stream.h View 1 3 chunks +7 lines, -0 lines 0 comments Download
M webrtc/audio/audio_send_stream.cc View 1 3 chunks +36 lines, -2 lines 0 comments Download
A webrtc/audio/time_interval.h View 1 1 chunk +65 lines, -0 lines 1 comment Download
A webrtc/audio/time_interval.cc View 1 1 chunk +56 lines, -0 lines 0 comments Download
A webrtc/audio/time_interval_unittest.cc View 1 1 chunk +48 lines, -0 lines 0 comments Download
M webrtc/call/call.cc View 1 4 chunks +8 lines, -0 lines 0 comments Download

Messages

Total messages: 47 (27 generated)
saza WebRTC
ptal ossu@, would like general feedback. Thrilled to hear if I made some part overly ...
3 years, 5 months ago (2017-07-13 06:36:08 UTC) #8
henrika_webrtc
Discussed offline. I am not the most suitable reviewer for rtc_base. Member of webrtc-core should ...
3 years, 5 months ago (2017-07-13 08:13:26 UTC) #9
pbos-webrtc
https://codereview.webrtc.org/2979833002/diff/1/webrtc/call/call.cc File webrtc/call/call.cc (right): https://codereview.webrtc.org/2979833002/diff/1/webrtc/call/call.cc#newcode516 webrtc/call/call.cc:516: RTC_HISTOGRAM_COUNTS_100000( Is this code actually intending to track sent ...
3 years, 5 months ago (2017-07-13 16:59:45 UTC) #11
saza WebRTC
On 2017/07/13 16:59:45, pbos-webrtc wrote: > https://codereview.webrtc.org/2979833002/diff/1/webrtc/call/call.cc > File webrtc/call/call.cc (right): > > https://codereview.webrtc.org/2979833002/diff/1/webrtc/call/call.cc#newcode516 > ...
3 years, 5 months ago (2017-07-14 08:26:02 UTC) #12
saza WebRTC
On 2017/07/13 16:59:45, pbos-webrtc wrote: > https://codereview.webrtc.org/2979833002/diff/1/webrtc/call/call.cc > File webrtc/call/call.cc (right): > > https://codereview.webrtc.org/2979833002/diff/1/webrtc/call/call.cc#newcode516 > ...
3 years, 5 months ago (2017-07-14 10:35:34 UTC) #13
ossu
Looking pretty good so far! I've added a bunch of comments below. I was also ...
3 years, 5 months ago (2017-07-14 11:13:16 UTC) #15
ossu
On 2017/07/14 10:35:34, Sam Zackrisson WebRTC wrote: > On 2017/07/13 16:59:45, pbos-webrtc wrote: > > ...
3 years, 5 months ago (2017-07-14 11:18:18 UTC) #16
saza WebRTC
On 2017/07/14 11:18:18, ossu wrote: > On 2017/07/14 10:35:34, Sam Zackrisson WebRTC wrote: > > ...
3 years, 5 months ago (2017-07-14 12:46:38 UTC) #17
saza WebRTC
On 2017/07/14 11:13:16, ossu wrote: > A bunch of good remarks. I agree with and ...
3 years, 5 months ago (2017-07-14 15:04:00 UTC) #18
ossu
On 2017/07/14 15:04:00, Sam Zackrisson WebRTC wrote: > On 2017/07/14 11:13:16, ossu wrote: > > ...
3 years, 5 months ago (2017-07-14 15:37:32 UTC) #19
pbos-webrtc
https://codereview.webrtc.org/2979833002/diff/1/webrtc/rtc_base/time_interval.h File webrtc/rtc_base/time_interval.h (right): https://codereview.webrtc.org/2979833002/diff/1/webrtc/rtc_base/time_interval.h#newcode49 webrtc/rtc_base/time_interval.h:49: void Extend(int64_t time); Should Extend not be able to ...
3 years, 5 months ago (2017-07-14 17:04:01 UTC) #20
saza WebRTC
Updated after comments by ossu@ and pbos@. I still can't find out if it is ...
3 years, 5 months ago (2017-07-17 14:27:30 UTC) #23
ossu
lgtm! On 2017/07/17 14:27:30, Sam Zackrisson WebRTC wrote: > Updated after comments by ossu@ and ...
3 years, 5 months ago (2017-07-17 16:02:58 UTC) #24
pbos-webrtc
On 2017/07/17 16:02:58, ossu wrote: > lgtm! > > On 2017/07/17 14:27:30, Sam Zackrisson WebRTC ...
3 years, 5 months ago (2017-07-17 22:38:15 UTC) #25
ossu
On 2017/07/17 22:38:15, pbos-webrtc wrote: > On 2017/07/17 16:02:58, ossu wrote: > > lgtm! > ...
3 years, 5 months ago (2017-07-18 09:14:19 UTC) #32
saza WebRTC
On 2017/07/18 09:14:19, ossu wrote: > On 2017/07/17 22:38:15, pbos-webrtc wrote: > > On 2017/07/17 ...
3 years, 5 months ago (2017-07-18 09:39:17 UTC) #34
saza WebRTC
https://codereview.webrtc.org/2979833002/diff/200001/webrtc/audio/time_interval.h File webrtc/audio/time_interval.h (right): https://codereview.webrtc.org/2979833002/diff/200001/webrtc/audio/time_interval.h#newcode47 webrtc/audio/time_interval.h:47: // Take the convex hull with another interval. Original ...
3 years, 5 months ago (2017-07-18 09:39:28 UTC) #35
pbos-webrtc
webrtc/call lgtm
3 years, 5 months ago (2017-07-18 15:41:06 UTC) #36
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/2979833002/200001
3 years, 5 months ago (2017-07-19 07:36:14 UTC) #44
commit-bot: I haz the power
3 years, 5 months ago (2017-07-19 07:39:27 UTC) #47
Message was sent while issue was closed.
Committed patchset #2 (id:200001) as
https://chromium.googlesource.com/external/webrtc/+/c58f8c096213a5e97554a1576...

Powered by Google App Engine
This is Rietveld 408576698