Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2957073002:
Add received audio and video call duration metrics based on packets. (Closed)
|
Created:
3 years, 5 months ago by saza WebRTC Modified:
3 years, 5 months ago CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com, the sun, mflodman Target Ref:
refs/heads/master Project:
webrtc Visibility:
Public. |
DescriptionAdd received audio/video call duration metrics based on packets.
Tracks time between first and last audio and packets to successfully pass through Call object's DeliverRtp method, timed with packet timestamps.
BUG=webrtc:7882
Review-Url: https://codereview.webrtc.org/2957073002
Cr-Commit-Position: refs/heads/master@{#18881}
Committed: https://chromium.googlesource.com/external/webrtc/+/746749237ab5e34bd6bfa9cc0da63fffce528901
Patch Set 1 : Add audio metric. #
Total comments: 4
Patch Set 2 : Correct for comments. Use a more predictable arrival time. #Patch Set 3 : Add corresponding call duration metric for received video rtp packets. #Patch Set 4 : Extend unittest to verify that video metric is properly logged. #Messages
Total messages: 36 (21 generated)
Description was changed from ========== Add received audio call duration metric based on packets. BUG=webrtc:7882 ========== to ========== Add received audio call duration metric based on packets. Tracks time between first and last audio packet to successfully pass through Call object's DeliverRtp method, timed with packet timestamps. BUG=webrtc:7882 ==========
saza@webrtc.org changed reviewers: + aleloi@webrtc.org, asapersson@webrtc.org
https://codereview.webrtc.org/2957073002/diff/1/webrtc/call/call.cc File webrtc/call/call.cc (right): https://codereview.webrtc.org/2957073002/diff/1/webrtc/call/call.cc#newcode328 webrtc/call/call.cc:328: I think the rtp_audio_ms_ counters should be moved here, because they are accessed in the same pattern as the RateCounters. The synchronization comment applies to them too. https://codereview.webrtc.org/2957073002/diff/1/webrtc/call/call.cc#newcode344 webrtc/call/call.cc:344: int64_t last_received_rtp_audio_ms_ = -1; There is a type rtc::Optional from webrtc/base/optional.h. Can you use that instead of a special value to distinguish between 'no value yet' and valid value? https://codereview.webrtc.org/2957073002/diff/1/webrtc/call/call.cc#newcode1329 webrtc/call/call.cc:1329: first_received_rtp_audio_ms_ = packet_time.timestamp; Other places in this file use brackets for single statement if:s. Please add brackets for consistency. (There is no style guide rule for this AFAIK).
Patchset #2 (id:20001) has been deleted
Patchset #2 (id:40001) has been deleted
Patchset #2 (id:60001) has been deleted
lgtm, but you have to get approval from someone who understands the call code
saza@webrtc.org changed reviewers: + stefan@webrtc.org - asapersson@webrtc.org
The CQ bit was checked by saza@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
PTAL https://codereview.webrtc.org/2957073002/diff/1/webrtc/call/call.cc File webrtc/call/call.cc (right): https://codereview.webrtc.org/2957073002/diff/1/webrtc/call/call.cc#newcode1329 webrtc/call/call.cc:1329: first_received_rtp_audio_ms_ = packet_time.timestamp; On 2017/06/27 14:13:37, aleloi wrote: > Other places in this file use brackets for single statement if:s. Please add > brackets for consistency. (There is no style guide rule for this AFAIK). Ok, I'll add them (for safety, if nothing else). The file is inconsistent though, e.g. lines 1106-1116, 1196, 1258, 1353, and 1365 omit braces. Yeah, style guide leaves it up to the project. https://google.github.io/styleguide/cppguide.html#Conditionals
PTAL
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
holmer@google.com changed reviewers: + asapersson@webrtc.org, holmer@google.com
Åsa, can you take a look? In particular I'm curious to know how this compares to the implementation for video. Do we track this for video too? I think it would be good to have it work the same way. Sam, wouldn't it make more sense to track this on each audio stream instead?
On 2017/06/29 12:24:27, holmer wrote: > Åsa, can you take a look? In particular I'm curious to know how this compares to > the implementation for video. Do we track this for video too? I think it would > be good to have it work the same way. > > Sam, wouldn't it make more sense to track this on each audio stream instead? We want to measure actual call durations, but many current metrics are biased toward short lifetimes because of short-lived temporary objects (e.g. as part of call setup). Wouldn't tracking stream lifetimes have the same problem if several streams are set up / torn down in sequence during a call? Or is stream lifetime a useful metric in some other situation?
lgtm For video, the lifetime of the streams are currently tracked but I think it would be good if the actual media duration was also tracked (per call or/and stream). We could add it for video later.
On 2017/06/29 14:40:36, åsapersson wrote: > lgtm > > For video, the lifetime of the streams are currently tracked but I think it > would be good if the actual media duration was also tracked (per call or/and > stream). We could add it for video later. Or perhaps we can do it in this CL? Seems like a simple thing to extend it :)
On 2017/06/29 14:47:48, stefan-webrtc wrote: > On 2017/06/29 14:40:36, åsapersson wrote: > > lgtm > > > > For video, the lifetime of the streams are currently tracked but I think it > > would be good if the actual media duration was also tracked (per call or/and > > stream). We could add it for video later. > > Or perhaps we can do it in this CL? Seems like a simple thing to extend it :) I'll add it for video receive duration, per call. Per-stream media durations do not lie as close to actual call lengths.
Patchset #3 (id:100001) has been deleted
The CQ bit was checked by saza@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
Description was changed from ========== Add received audio call duration metric based on packets. Tracks time between first and last audio packet to successfully pass through Call object's DeliverRtp method, timed with packet timestamps. BUG=webrtc:7882 ========== to ========== Add received audio/video call duration metrics based on packets. Tracks time between first and last audio and packets to successfully pass through Call object's DeliverRtp method, timed with packet timestamps. BUG=webrtc:7882 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Looks good, but perhaps also verify this in the tests here: https://cs.chromium.org/chromium/src/third_party/webrtc/video/end_to_end_test...
lgtm
The CQ bit was checked by saza@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from aleloi@webrtc.org, asapersson@webrtc.org Link to the patchset: https://codereview.webrtc.org/2957073002/#ps140001 (title: "Extend unittest to verify that video metric is properly logged.")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
CQ is committing da patch.
Bot data: {"patchset_id": 140001, "attempt_start_ts": 1499150821423280,
"parent_rev": "2a2b297aa6acce292f3f19b6ebe3cf4af5e014b5", "commit_rev":
"746749237ab5e34bd6bfa9cc0da63fffce528901"}
Message was sent while issue was closed.
Description was changed from ========== Add received audio/video call duration metrics based on packets. Tracks time between first and last audio and packets to successfully pass through Call object's DeliverRtp method, timed with packet timestamps. BUG=webrtc:7882 ========== to ========== Add received audio/video call duration metrics based on packets. Tracks time between first and last audio and packets to successfully pass through Call object's DeliverRtp method, timed with packet timestamps. BUG=webrtc:7882 Review-Url: https://codereview.webrtc.org/2957073002 Cr-Commit-Position: refs/heads/master@{#18881} Committed: https://chromium.googlesource.com/external/webrtc/+/746749237ab5e34bd6bfa9cc0... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:140001) as https://chromium.googlesource.com/external/webrtc/+/746749237ab5e34bd6bfa9cc0...
Message was sent while issue was closed.
A revert of this CL (patchset #4 id:140001) has been created in https://codereview.webrtc.org/2972613002/ by saza@webrtc.org. The reason for reverting is: The following, seemingly related, unit tests crash on Android32 (M Nexus5X). org.webrtc.PeerConnectionTest#testCompleteSession org.webrtc.PeerConnectionTest#testDataChannelOnlySession A Windows build fails with a mysterious compile error..
Message was sent while issue was closed.
On 2017/07/04 08:11:35, Sam Zackrisson WebRTC wrote: > A revert of this CL (patchset #4 id:140001) has been created in > https://codereview.webrtc.org/2972613002/ by mailto:saza@webrtc.org. > > The reason for reverting is: The following, seemingly related, unit tests crash > on Android32 (M Nexus5X). > org.webrtc.PeerConnectionTest#testCompleteSession > org.webrtc.PeerConnectionTest#testDataChannelOnlySession > > A Windows build fails with a mysterious compile error.. This CL was relanded shortly after the revert, as it was determined to not cause the crashes. New CL: https://codereview.webrtc.org/2970793003/ |
