Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(8)

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

Can't Edit
Can't Publish+Mail
Start Review
Created:
1 week, 3 days ago by saza WebRTC(OOO until July 31)
Modified:
3 days, 18 hours ago
Reviewers:
ossu, pbos-webrtc
CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com, the sun (OOO until July 24th), stefan-webrtc, mflodman_OOO, tlegrand-webrtc, henrika_webrtc (OOO Aug 14)
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
Commit queue not available (can’t edit this change).

Messages

Total messages: 47 (27 generated)
saza WebRTC(OOO until July 31)
ptal ossu@, would like general feedback. Thrilled to hear if I made some part overly ...
1 week, 2 days ago (2017-07-13 06:36:08 UTC) #8
henrika_webrtc (OOO Aug 14)
Discussed offline. I am not the most suitable reviewer for rtc_base. Member of webrtc-core should ...
1 week, 2 days 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 ...
1 week, 2 days ago (2017-07-13 16:59:45 UTC) #11
saza WebRTC(OOO until July 31)
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 > ...
1 week, 1 day ago (2017-07-14 08:26:02 UTC) #12
saza WebRTC(OOO until July 31)
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 > ...
1 week, 1 day 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 ...
1 week, 1 day 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: > > ...
1 week, 1 day ago (2017-07-14 11:18:18 UTC) #16
saza WebRTC(OOO until July 31)
On 2017/07/14 11:18:18, ossu wrote: > On 2017/07/14 10:35:34, Sam Zackrisson WebRTC wrote: > > ...
1 week, 1 day ago (2017-07-14 12:46:38 UTC) #17
saza WebRTC(OOO until July 31)
On 2017/07/14 11:13:16, ossu wrote: > A bunch of good remarks. I agree with and ...
1 week, 1 day 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: > > ...
1 week, 1 day 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 ...
1 week, 1 day ago (2017-07-14 17:04:01 UTC) #20
saza WebRTC(OOO until July 31)
Updated after comments by ossu@ and pbos@. I still can't find out if it is ...
5 days, 12 hours 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 ...
5 days, 10 hours 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 ...
5 days, 3 hours 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! > ...
4 days, 17 hours ago (2017-07-18 09:14:19 UTC) #32
saza WebRTC(OOO until July 31)
On 2017/07/18 09:14:19, ossu wrote: > On 2017/07/17 22:38:15, pbos-webrtc wrote: > > On 2017/07/17 ...
4 days, 16 hours ago (2017-07-18 09:39:17 UTC) #34
saza WebRTC(OOO until July 31)
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 ...
4 days, 16 hours ago (2017-07-18 09:39:28 UTC) #35
pbos-webrtc
webrtc/call lgtm
4 days, 10 hours 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 days, 18 hours ago (2017-07-19 07:36:14 UTC) #44
commit-bot: I haz the power
3 days, 18 hours 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...
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 25c286973