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

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:
2 months, 2 weeks ago by saza WebRTC
Modified:
2 months, 1 week 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
Commit queue not available (can’t edit this change).

Messages

Total messages: 47 (27 generated)
saza WebRTC
ptal ossu@, would like general feedback. Thrilled to hear if I made some part overly ...
2 months, 2 weeks 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 ...
2 months, 2 weeks 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 ...
2 months, 1 week 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 > ...
2 months, 1 week 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 > ...
2 months, 1 week 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 ...
2 months, 1 week 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: > > ...
2 months, 1 week 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: > > ...
2 months, 1 week 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 ...
2 months, 1 week 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: > > ...
2 months, 1 week 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 ...
2 months, 1 week 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 ...
2 months, 1 week 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 ...
2 months, 1 week 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 ...
2 months, 1 week 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! > ...
2 months, 1 week 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 ...
2 months, 1 week 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 ...
2 months, 1 week ago (2017-07-18 09:39:28 UTC) #35
pbos-webrtc
webrtc/call lgtm
2 months, 1 week 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
2 months, 1 week ago (2017-07-19 07:36:14 UTC) #44
commit-bot: I haz the power
2 months, 1 week 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 b40b6558b