|
|
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, stefan-webrtc, mflodman, tlegrand-webrtc, henrika_webrtc Target Ref:
refs/heads/master Project:
webrtc Visibility:
Public. |
DescriptionAdds 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
Messages
Total messages: 47 (27 generated)
Description was changed from ========== 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. BUG=webrtc:7882 ========== to ========== 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 ==========
saza@webrtc.org changed reviewers: + henrika@webrtc.org, ossu@webrtc.org, pbos@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/...
Description was changed from ========== 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 ========== to ========== 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 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_clang_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?)) win_clang_rel on master.tryserver.webrtc (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?)) win_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?)) win_rel on master.tryserver.webrtc (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?)) win_x64_clang_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?)) win_x64_clang_rel on master.tryserver.webrtc (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?)) win_x64_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?)) win_x64_rel on master.tryserver.webrtc (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?))
ptal ossu@, would like general feedback. Thrilled to hear if I made some part overly complicated. henrika@ and pbos@, mainly want input on your OWNER directories, webrtc/rtc_base and webrtc/call respectively, unless you want to spend more time on this. Thanks!
Discussed offline. I am not the most suitable reviewer for rtc_base. Member of webrtc-core should be used instead.
henrika@webrtc.org changed reviewers: - henrika@webrtc.org
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 RTP from multiple send streams? Otherwise this could be done entirely in AudioSendStream? Would it be better for the transport object to track for how long each payload type is sent, construct the lifetime through there and log lifetime for both audio or video? E.g. on destruction, report for how long each payload type is sent. Also, what should happen when a stream is inactive for 18/20 minutes in the middle? Is that something we care about? This stats will report that it's sending for 20 minutes.
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 > webrtc/call/call.cc:516: RTC_HISTOGRAM_COUNTS_100000( > Is this code actually intending to track sent RTP from multiple send streams? > Otherwise this could be done entirely in AudioSendStream? > > Would it be better for the transport object to track for how long each payload > type is sent, construct the lifetime through there and log lifetime for both > audio or video? E.g. on destruction, report for how long each payload type is > sent. > > Also, what should happen when a stream is inactive for 18/20 minutes in the > middle? Is that something we care about? This stats will report that it's > sending for 20 minutes.
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 > webrtc/call/call.cc:516: RTC_HISTOGRAM_COUNTS_100000( > Is this code actually intending to track sent RTP from multiple send streams? > Otherwise this could be done entirely in AudioSendStream? > > Would it be better for the transport object to track for how long each payload > type is sent, construct the lifetime through there and log lifetime for both > audio or video? E.g. on destruction, report for how long each payload type is > sent. > > Also, what should happen when a stream is inactive for 18/20 minutes in the > middle? Is that something we care about? This stats will report that it's > sending for 20 minutes. Good questions. Thank you! The goal is to get these metrics as close to the actual calls as possible. 1. I think we do want to track multiple streams, assuming that during calls, they can be created and destroyed. The lifetime of a single stream will not reflect that of the actual call as well as the aggregate of all streams. 2. I'm not caught up on the packet sending workings, but doesn't the same problem arise for transport objects? Is the Call object guaranteed that all of its audio send streams will use the same transport, even during (say) a network switch? 3. As for inactive streams, as long as they pick up sending again they indicate that their owner (Call object) is still ongoing.
saza@webrtc.org changed reviewers: + tommi@webrtc.org
Looking pretty good so far! I've added a bunch of comments below. I was also a bit dubious as to whether the TimedTransport should change an external TimeInterval rather than keep one internally. It does seem the more practical route though, in case Transports change over the life of an AudioSendStream. Do they? https://codereview.webrtc.org/2979833002/diff/1/webrtc/audio/audio_send_strea... File webrtc/audio/audio_send_stream.cc (right): https://codereview.webrtc.org/2979833002/diff/1/webrtc/audio/audio_send_strea... webrtc/audio/audio_send_stream.cc:165: stream->timed_send_transport_adapter_ = I think you can do .reset(new TimedTransport(...)) here. https://codereview.webrtc.org/2979833002/diff/1/webrtc/audio/audio_send_stream.h File webrtc/audio/audio_send_stream.h (right): https://codereview.webrtc.org/2979833002/diff/1/webrtc/audio/audio_send_strea... webrtc/audio/audio_send_stream.h:80: const rtc::TimeInterval* GetActiveLifetime() const; Why not return as a const& instead? Can it be null? https://codereview.webrtc.org/2979833002/diff/1/webrtc/audio/audio_send_strea... File webrtc/audio/audio_send_stream_unittest.cc (left): https://codereview.webrtc.org/2979833002/diff/1/webrtc/audio/audio_send_strea... webrtc/audio/audio_send_stream_unittest.cc:236: EXPECT_CALL(*channel_proxy_, RegisterExternalTransport(nullptr)).Times(1); This is odd. Why would the test want to set the external transport to nullptr (rather than DeRegistering the external transport)? Also, with a nullptr external transport, I'd expect we wouldn't want to wrap it with a TimedTransport. https://codereview.webrtc.org/2979833002/diff/1/webrtc/rtc_base/time_interval.cc File webrtc/rtc_base/time_interval.cc (right): https://codereview.webrtc.org/2979833002/diff/1/webrtc/rtc_base/time_interval... webrtc/rtc_base/time_interval.cc:16: TimeInterval::TimeInterval() {} = default instead of {} https://codereview.webrtc.org/2979833002/diff/1/webrtc/rtc_base/time_interval... webrtc/rtc_base/time_interval.cc:17: TimeInterval::~TimeInterval() {} Same here. https://codereview.webrtc.org/2979833002/diff/1/webrtc/rtc_base/time_interval... webrtc/rtc_base/time_interval.cc:24: if (!first_ || time < *first_) { Just a minor detail, but first_ and last_ aren't both individually optional. You'll either have an interval with both first and last points (possibly equal) or no interval. I'd probably restructure it as: In the header: struct Interval { int64_t first, last; }; rtc::Optional<Interval> interval_; Here: if (!interval_) { interval_.emplace{{time, time}); } else { ... comparisons go here ... } This probably save a whole eight bytes of memory per TimeInterval as well. :) 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... webrtc/rtc_base/time_interval.h:11: #ifndef WEBRTC_RTC_BASE_TIME_INTERVAL_H_ I'm not sure if this class belongs in rtc_base just yet. Since it's got a quite narrow scope and is only used by Call and AudioSendStream, maybe put it in call/ for now? If it's needed elsewhere later, it can be moved to rtc_base then, possibly being adapted for a wider scope of use. https://codereview.webrtc.org/2979833002/diff/1/webrtc/rtc_base/time_interval... File webrtc/rtc_base/time_interval_unittest.cc (right): https://codereview.webrtc.org/2979833002/diff/1/webrtc/rtc_base/time_interval... webrtc/rtc_base/time_interval_unittest.cc:20: Thread::SleepMs(100); Use a fake clock for testing instead. This is prone to going flaky (and will make the test always take more time than strictly necessary). With the fake timer, you should be able to make the test below even stricter: e.g. if 100 ms has elapsed, make sure the interval reports exactly 100 ms and not 99 or 101.
On 2017/07/14 10:35:34, Sam Zackrisson WebRTC wrote: > 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 > > webrtc/call/call.cc:516: RTC_HISTOGRAM_COUNTS_100000( > > Is this code actually intending to track sent RTP from multiple send streams? > > Otherwise this could be done entirely in AudioSendStream? > > > > Would it be better for the transport object to track for how long each payload > > type is sent, construct the lifetime through there and log lifetime for both > > audio or video? E.g. on destruction, report for how long each payload type is > > sent. > > > > Also, what should happen when a stream is inactive for 18/20 minutes in the > > middle? Is that something we care about? This stats will report that it's > > sending for 20 minutes. > > Good questions. Thank you! > The goal is to get these metrics as close to the actual calls as possible. > > 3. As for inactive streams, as long as they pick up sending again they indicate > that their owner (Call object) is still ongoing. What if they don't? What if the user is muted and isn't sending audio at all? Would this still capture what you want to know, or does it need to be augmented with information from the receiving side as well?
On 2017/07/14 11:18:18, ossu wrote: > On 2017/07/14 10:35:34, Sam Zackrisson WebRTC wrote: > > 3. As for inactive streams, as long as they pick up sending again they > indicate > > that their owner (Call object) is still ongoing. > > What if they don't? What if the user is muted and isn't sending audio at all? > Would this still capture what you want to know, or does it need to be augmented > with information from the receiving side as well? I'd say yes, we still want to measure the time RTP packets are going out. You raise a very good point about muting. We could... 1. collect all four stats (sent/received audio/video) and also report their joint active time, 2. only track when the first packet is sent and report the time between this and the destruction of the object, or 3. define some minimum packet rate as active and track the total time streams are active. I think the first approach would give the most accurate measure. 2 assumes clients won't e.g. let a peer connection linger after a call until the application is closed, and 3 feels too far off the original feature of interest.
On 2017/07/14 11:13:16, ossu wrote: > A bunch of good remarks. I agree with and will add a patch for all of these comments. Two things: > https://codereview.webrtc.org/2979833002/diff/1/webrtc/rtc_base/time_interval... > webrtc/rtc_base/time_interval.cc:24: if (!first_ || time < *first_) { > Just a minor detail, but first_ and last_ aren't both individually optional. > You'll either have an interval with both first and last points (possibly equal) > or no interval. I'd probably restructure it as: ... What'd be the best accessor methods in this case? Same ones? (First(), Last(), constructing Optional<int64_t>s for each call) Making the Interval struct public and returning it? Not sure if TimeInterval::Interval feels very clear, but it probably leads to the least amount of code. > https://codereview.webrtc.org/2979833002/diff/1/webrtc/audio/audio_send_strea... > webrtc/audio/audio_send_stream_unittest.cc:236: EXPECT_CALL(*channel_proxy_, > RegisterExternalTransport(nullptr)).Times(1); > This is odd. Why would the test want to set the external transport to nullptr[..]? This assumption on the interface is made in multiple tests (e.g. call_unittests), and adding MockTransports (as we discussed offline) is too much work for this CL. I'll look into addressing this in a separate issue and, for now, will probably have AudioSendStream propagate nullptr if you agree: if (new_config.send_transport) { do_the_thing_with_the(..send_transport); } else { use_a_nullptr; }
On 2017/07/14 15:04:00, Sam Zackrisson WebRTC wrote: > On 2017/07/14 11:13:16, ossu wrote: > > A bunch of good remarks. > > https://codereview.webrtc.org/2979833002/diff/1/webrtc/audio/audio_send_strea... > > webrtc/audio/audio_send_stream_unittest.cc:236: EXPECT_CALL(*channel_proxy_, > > RegisterExternalTransport(nullptr)).Times(1); > > This is odd. Why would the test want to set the external transport to > nullptr[..]? > This assumption on the interface is made in multiple tests (e.g. > call_unittests), and > adding MockTransports (as we discussed offline) is too much work for this CL. > I'll > look into addressing this in a separate issue and, for now, will probably have > AudioSendStream propagate nullptr if you agree: > if (new_config.send_transport) { > do_the_thing_with_the(..send_transport); > } else { use_a_nullptr; } Oh, alright! https://codereview.webrtc.org/2979833002/diff/1/webrtc/rtc_base/time_interval.cc File webrtc/rtc_base/time_interval.cc (right): https://codereview.webrtc.org/2979833002/diff/1/webrtc/rtc_base/time_interval... webrtc/rtc_base/time_interval.cc:24: if (!first_ || time < *first_) { Rietveld top-tip: If you reply to the comments individually on the diff pages, rather than in the main reply message, you'll get neat threads that are easier to follow! I've tried to sort-of hack your message back into this comment thread. It probably won't work well. :) On 2017/07/14 15:04:00, Sam Zackrisson WebRTC wrote: > What'd be the best accessor methods in this case? > Same ones? > (First(), Last(), constructing Optional<int64_t>s for each call) > Making the Interval struct public and returning it? > Not sure if TimeInterval::Interval feels very clear, but it probably leads to > the least amount of code. Hmm... You _could_ just have First() and Last() return int64_t and in them DCHECK that the internal interval_ is set... though Optional will also check that for you. In a way, accessing TimeInterval::First() when Empty() is as wrong as accessing operator* on an Optional that's empty. I.e. there's no particular reason to signal nonexistence of First() both through Empty() and the through return value of First().
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... webrtc/rtc_base/time_interval.h:49: void Extend(int64_t time); Should Extend not be able to take another time interval? Then you can do: sent_rtp_audio_timer_ms_.Extend(*audio_send_stream->GetActiveLifetime()); In call.cc, without checking empty, etc. That would just be done inside Extend.
Patchset #2 (id:20001) has been deleted
Patchset #2 (id:40001) has been deleted
Updated after comments by ossu@ and pbos@. I still can't find out if it is always the same transport, throughout calls, for (voice) media. There's support for changing them, but I can't yet tell if that code path is ever taken. https://codereview.webrtc.org/2979833002/diff/1/webrtc/audio/audio_send_strea... File webrtc/audio/audio_send_stream.cc (right): https://codereview.webrtc.org/2979833002/diff/1/webrtc/audio/audio_send_strea... webrtc/audio/audio_send_stream.cc:165: stream->timed_send_transport_adapter_ = On 2017/07/14 11:13:15, ossu wrote: > I think you can do .reset(new TimedTransport(...)) here. Done. https://codereview.webrtc.org/2979833002/diff/1/webrtc/audio/audio_send_stream.h File webrtc/audio/audio_send_stream.h (right): https://codereview.webrtc.org/2979833002/diff/1/webrtc/audio/audio_send_strea... webrtc/audio/audio_send_stream.h:80: const rtc::TimeInterval* GetActiveLifetime() const; On 2017/07/14 11:13:15, ossu wrote: > Why not return as a const& instead? Can it be null? Done. https://codereview.webrtc.org/2979833002/diff/1/webrtc/audio/audio_send_strea... File webrtc/audio/audio_send_stream_unittest.cc (left): https://codereview.webrtc.org/2979833002/diff/1/webrtc/audio/audio_send_strea... webrtc/audio/audio_send_stream_unittest.cc:236: EXPECT_CALL(*channel_proxy_, RegisterExternalTransport(nullptr)).Times(1); On 2017/07/14 11:13:15, ossu wrote: > This is odd. Why would the test want to set the external transport to nullptr > (rather than DeRegistering the external transport)? Also, with a nullptr > external transport, I'd expect we wouldn't want to wrap it with a > TimedTransport. Done. https://codereview.webrtc.org/2979833002/diff/1/webrtc/rtc_base/time_interval.cc File webrtc/rtc_base/time_interval.cc (right): https://codereview.webrtc.org/2979833002/diff/1/webrtc/rtc_base/time_interval... webrtc/rtc_base/time_interval.cc:16: TimeInterval::TimeInterval() {} On 2017/07/14 11:13:15, ossu wrote: > = default instead of {} Done. https://codereview.webrtc.org/2979833002/diff/1/webrtc/rtc_base/time_interval... webrtc/rtc_base/time_interval.cc:17: TimeInterval::~TimeInterval() {} On 2017/07/14 11:13:15, ossu wrote: > Same here. Done. https://codereview.webrtc.org/2979833002/diff/1/webrtc/rtc_base/time_interval... webrtc/rtc_base/time_interval.cc:24: if (!first_ || time < *first_) { On 2017/07/14 15:37:32, ossu wrote: > Rietveld top-tip: If you reply to the comments individually on the diff pages, > rather than in the main reply message, you'll get neat threads that are easier > to follow! I've tried to sort-of hack your message back into this comment > thread. It probably won't work well. :) > > On 2017/07/14 15:04:00, Sam Zackrisson WebRTC wrote: > > What'd be the best accessor methods in this case? > > Same ones? > > (First(), Last(), constructing Optional<int64_t>s for each call) > > Making the Interval struct public and returning it? > > Not sure if TimeInterval::Interval feels very clear, but it probably leads to > > the least amount of code. > > Hmm... You _could_ just have First() and Last() return int64_t and in them > DCHECK that the internal interval_ is set... though Optional will also check > that for you. In a way, accessing TimeInterval::First() when Empty() is as wrong > as accessing operator* on an Optional that's empty. I.e. there's no particular > reason to signal nonexistence of First() both through Empty() and the through > return value of First(). Oh, thanks for the tip! 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... webrtc/rtc_base/time_interval.h:11: #ifndef WEBRTC_RTC_BASE_TIME_INTERVAL_H_ On 2017/07/14 11:13:15, ossu wrote: > I'm not sure if this class belongs in rtc_base just yet. Since it's got a quite > narrow scope and is only used by Call and AudioSendStream, maybe put it in call/ > for now? If it's needed elsewhere later, it can be moved to rtc_base then, > possibly being adapted for a wider scope of use. Done. https://codereview.webrtc.org/2979833002/diff/1/webrtc/rtc_base/time_interval... webrtc/rtc_base/time_interval.h:49: void Extend(int64_t time); On 2017/07/14 17:04:01, pbos-webrtc wrote: > Should Extend not be able to take another time interval? Then you can do: > > sent_rtp_audio_timer_ms_.Extend(*audio_send_stream->GetActiveLifetime()); > > In call.cc, without checking empty, etc. That would just be done inside Extend. Done. https://codereview.webrtc.org/2979833002/diff/1/webrtc/rtc_base/time_interval... File webrtc/rtc_base/time_interval_unittest.cc (right): https://codereview.webrtc.org/2979833002/diff/1/webrtc/rtc_base/time_interval... webrtc/rtc_base/time_interval_unittest.cc:20: Thread::SleepMs(100); On 2017/07/14 11:13:15, ossu wrote: > Use a fake clock for testing instead. This is prone to going flaky (and will > make the test always take more time than strictly necessary). > > With the fake timer, you should be able to make the test below even stricter: > e.g. if 100 ms has elapsed, make sure the interval reports exactly 100 ms and > not 99 or 101. Done.
lgtm! On 2017/07/17 14:27:30, Sam Zackrisson WebRTC wrote: > Updated after comments by ossu@ and pbos@. > > I still can't find out if it is always the same transport, throughout calls, for > (voice) media. There's support for changing them, but I can't yet tell if that > code path is ever taken. I think it should be fine either way, since you're updating the timed interval in the AudioSendStream. It's a biit icky, but it should be fine. :) https://codereview.webrtc.org/2979833002/diff/60001/webrtc/audio/time_interval.h File webrtc/audio/time_interval.h (right): https://codereview.webrtc.org/2979833002/diff/60001/webrtc/audio/time_interva... webrtc/audio/time_interval.h:47: // Extend the interval with another interval. It may be prudent to note here that this interval is extended with the times (first and last) of other_interval and not with the length of the other interval. (So extending an interval with itself will not double the length). Though maybe that's obvious?
On 2017/07/17 16:02:58, ossu wrote: > lgtm! > > On 2017/07/17 14:27:30, Sam Zackrisson WebRTC wrote: > > Updated after comments by ossu@ and pbos@. > > > > I still can't find out if it is always the same transport, throughout calls, > for > > (voice) media. There's support for changing them, but I can't yet tell if that > > code path is ever taken. > > I think it should be fine either way, since you're updating the timed interval > in the AudioSendStream. It's a biit icky, but it should be fine. :) > > https://codereview.webrtc.org/2979833002/diff/60001/webrtc/audio/time_interval.h > File webrtc/audio/time_interval.h (right): > > https://codereview.webrtc.org/2979833002/diff/60001/webrtc/audio/time_interva... > webrtc/audio/time_interval.h:47: // Extend the interval with another interval. > It may be prudent to note here that this interval is extended with the times > (first and last) of other_interval and not with the length of the other > interval. (So extending an interval with itself will not double the length). > Though maybe that's obvious? I'm confused. https://codereview.webrtc.org/2979833002/diff/60001/webrtc/call/call.cc doesn't show the full diff, do you have any idea why? I would expect this to show new Extend calls, and be based on the base, not previous patchset. Did something change? Am I wielding Rietveld wrong? I'd like to see the full diff to review this..?
Patchset #3 (id:80001) has been deleted
Patchset #4 (id:120001) has been deleted
Patchset #2 (id:60001) has been deleted
Patchset #2 (id:100001) has been deleted
Patchset #2 (id:140001) has been deleted
Patchset #2 (id:160001) has been deleted
On 2017/07/17 22:38:15, pbos-webrtc wrote: > On 2017/07/17 16:02:58, ossu wrote: > > lgtm! > > > > On 2017/07/17 14:27:30, Sam Zackrisson WebRTC wrote: > > > Updated after comments by ossu@ and pbos@. > > > > > > I still can't find out if it is always the same transport, throughout calls, > > for > > > (voice) media. There's support for changing them, but I can't yet tell if > that > > > code path is ever taken. > > > > I think it should be fine either way, since you're updating the timed interval > > in the AudioSendStream. It's a biit icky, but it should be fine. :) > > > > > https://codereview.webrtc.org/2979833002/diff/60001/webrtc/audio/time_interval.h > > File webrtc/audio/time_interval.h (right): > > > > > https://codereview.webrtc.org/2979833002/diff/60001/webrtc/audio/time_interva... > > webrtc/audio/time_interval.h:47: // Extend the interval with another interval. > > It may be prudent to note here that this interval is extended with the times > > (first and last) of other_interval and not with the length of the other > > interval. (So extending an interval with itself will not double the length). > > Though maybe that's obvious? > > I'm confused. > https://codereview.webrtc.org/2979833002/diff/60001/webrtc/call/call.cc doesn't > show the full diff, do you have any idea why? > > I would expect this to show new Extend calls, and be based on the base, not > previous patchset. Did something change? Am I wielding Rietveld wrong? I'd like > to see the full diff to review this..? Seems like saza@ has removed old patchsets, probably confusing Rietveld. At this point, patchset 60001 doesn't even exist. Rietveld top-tip #2: Don't do that! :) IMO, removing patchsets is fine to clean stuff up before you send the CL out for review. After that, it only confuses reviewers and runs the risk of removing comments and other useful context from the review.
Patchset #2 (id:180001) has been deleted
On 2017/07/18 09:14:19, ossu wrote: > On 2017/07/17 22:38:15, pbos-webrtc wrote: > > On 2017/07/17 16:02:58, ossu wrote: > > > lgtm! > > > > > > On 2017/07/17 14:27:30, Sam Zackrisson WebRTC wrote: > > > > Updated after comments by ossu@ and pbos@. > > > > > > > > I still can't find out if it is always the same transport, throughout > calls, > > > for > > > > (voice) media. There's support for changing them, but I can't yet tell if > > that > > > > code path is ever taken. > > > > > > I think it should be fine either way, since you're updating the timed > interval > > > in the AudioSendStream. It's a biit icky, but it should be fine. :) > > > > > > > > > https://codereview.webrtc.org/2979833002/diff/60001/webrtc/audio/time_interval.h > > > File webrtc/audio/time_interval.h (right): > > > > > > > > > https://codereview.webrtc.org/2979833002/diff/60001/webrtc/audio/time_interva... > > > webrtc/audio/time_interval.h:47: // Extend the interval with another > interval. > > > It may be prudent to note here that this interval is extended with the times > > > (first and last) of other_interval and not with the length of the other > > > interval. (So extending an interval with itself will not double the length). > > > Though maybe that's obvious? > > > > I'm confused. > > https://codereview.webrtc.org/2979833002/diff/60001/webrtc/call/call.cc > doesn't > > show the full diff, do you have any idea why? > > > > I would expect this to show new Extend calls, and be based on the base, not > > previous patchset. Did something change? Am I wielding Rietveld wrong? I'd > like > > to see the full diff to review this..? > > Seems like saza@ has removed old patchsets, probably confusing Rietveld. At this > point, patchset 60001 doesn't even exist. > > Rietveld top-tip #2: Don't do that! :) > IMO, removing patchsets is fine to clean stuff up before you send the CL out for > review. After that, it only confuses reviewers and runs the risk of removing > comments and other useful context from the review. pbos@'s issues stem from me having different upstream branches in my local git when submitting the patch sets. The BASE that diffs are shown against is not origin/master as I'd thought, but rather whatever branch was upstream when uploading the patch set. The patch set deleting is my attempt to rectify this. This patch set is identical to the one lgtm'd by ossu@, with one change in code and three in Rietveld: + The unified diff is the actual unified diff. - The move of TimeInterval from rtc_base to audio is not shown properly. - ossu@'s comment disappeared. I re-added it and updated the comment to better reflect what happens. I'm sorry about the mess, this was the only way I could see to get the diff-to-BASE.
https://codereview.webrtc.org/2979833002/diff/200001/webrtc/audio/time_interv... File webrtc/audio/time_interval.h (right): https://codereview.webrtc.org/2979833002/diff/200001/webrtc/audio/time_interv... webrtc/audio/time_interval.h:47: // Take the convex hull with another interval. Original line: // Extend the interval with another interval. ossu@'s comment: It may be prudent to note here that this interval is extended with the times (first and last) of other_interval and not with the length of the other interval. (So extending an interval with itself will not double the length). Though maybe that's obvious?
webrtc/call lgtm
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/...
saza@webrtc.org changed reviewers: - tommi@webrtc.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by saza@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from ossu@webrtc.org Link to the patchset: https://codereview.webrtc.org/2979833002/#ps200001 (title: "Adjust for comments.")
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": 200001, "attempt_start_ts": 1500449768667860, "parent_rev": "e1d4dcaaa95bd3fbbcfa847b520f5ad3abadb18a", "commit_rev": "c58f8c096213a5e97554a1576cc4b51d1a98c4c4"}
Message was sent while issue was closed.
Description was changed from ========== 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 ========== to ========== 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/+/c58f8c096213a5e97554a1576... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:200001) as https://chromium.googlesource.com/external/webrtc/+/c58f8c096213a5e97554a1576... |