Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2290203002:
Delete Timing class, timing.h, and update all users. (Closed)
|
Created:
4 years, 3 months ago by nisse-webrtc Modified:
4 years, 3 months ago CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com, the sun Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionDelete Timing class, timing.h, and update all users.
BUG=webrtc:6324
Committed: https://crrev.com/cdf37a929336e2ca1e0bcdd5e858ffc75b2b317e
Cr-Commit-Position: refs/heads/master@{#14203}
Patch Set 1 #
Total comments: 6
Patch Set 2 : Use rtc:kNumNanosecsPerSec. #Patch Set 3 : New helper rtc::WallTimeSeconds. #
Total comments: 7
Patch Set 4 : Delete obsolete test. Fix unit error. #
Total comments: 4
Patch Set 5 : Rename to rtc::TimeUTCMicros, and use it. #
Total comments: 5
Patch Set 6 : Rebase. #Patch Set 7 : Address nits. #
Total comments: 4
Patch Set 8 : Address hbos' comments. #Patch Set 9 : Fix copy-paste error in rtpdataengine.cc. #
Messages
Total messages: 45 (17 generated)
nisse@webrtc.org changed reviewers: + deadbeef@webrtc.org, hbos@webrtc.org, perkj@webrtc.org
I've noticed yet another timing class, the mostly unused Timing class. I'd like
to delete it. Some questions:
What's the purpose of the SendDataMultipleClocks test? Can it be deleted now
that rtpdataengine uses the global rtc::TimeNanos clock?
What about the GetTimeNow/WallTimeNow function, wrapping gettimeofday (or
whatever is needed on non-posix platforms)? It would make some sense to me to
move into timeutils.{h,cc}, possibly under the name rtc::WallClockTimeNow. I
guess the StatsCollector really wants wall clock time?
There are half a dozen calls to gettimeofday in the webrtc, but most seem to be
in OS-specific code.
Hmh, SendDataMultipleClocks uses two different timers simultaneously. This is not possible with a global singleton clock. Perhaps there is still need to have this interface? https://codereview.webrtc.org/2290203002/diff/1/webrtc/api/statscollector.cc File webrtc/api/statscollector.cc (right): https://codereview.webrtc.org/2290203002/diff/1/webrtc/api/statscollector.cc#... webrtc/api/statscollector.cc:385: // TODO(nisse): Helper function belongs in timeutils.h. I think this TODO should be fixed in this CL before landing it. https://codereview.webrtc.org/2290203002/diff/1/webrtc/media/base/rtpdataengi... File webrtc/media/base/rtpdataengine.cc (right): https://codereview.webrtc.org/2290203002/diff/1/webrtc/media/base/rtpdataengi... webrtc/media/base/rtpdataengine.cc:309: double now = rtc::TimeMicros() * 1e-6; Prefer using (defining a new if you have to) constant from timeutils.h for conversion. Perhaps add a rtc::TimeSeconds()? https://codereview.webrtc.org/2290203002/diff/1/webrtc/media/base/rtpdataengi... File webrtc/media/base/rtpdataengine_unittest.cc (right): https://codereview.webrtc.org/2290203002/diff/1/webrtc/media/base/rtpdataengi... webrtc/media/base/rtpdataengine_unittest.cc:60: clock_.SetTimeNanos(now * 1e9); Use a constant from timeutils.h.
On 2016/08/30 12:56:40, hbos wrote: > Hmh, SendDataMultipleClocks uses two different timers simultaneously. This is > not possible with a global singleton clock. Perhaps there is still need to have > this interface? Try using ClockInterface the way Timing was previously used in this case.
On 2016/08/30 13:00:37, hbos wrote: > On 2016/08/30 12:56:40, hbos wrote: > > Hmh, SendDataMultipleClocks uses two different timers simultaneously. This is > > not possible with a global singleton clock. Perhaps there is still need to > have > > this interface? I doubt it is useful. I wonder who to ask to make sure, most work on rtpdataengine seem to predate git history. > Try using ClockInterface the way Timing was previously used in this case. Not sure what you mean. One could switch to Clock (webrtc/system_wrappers/include/clock.h), but I'd prefer not to create more dependences on that. In my opinion, we should need only a single global clock, and then for testing we have the ScopedFakeClock hack. There can be nothing good in having multiple clock instances, with different parts of the code using different clocks.
Addressed the simplest of the comments. Haven't yet investigated the bot failures, but the look genuine, and I can reproduce locally. https://codereview.webrtc.org/2290203002/diff/1/webrtc/api/statscollector.cc File webrtc/api/statscollector.cc (right): https://codereview.webrtc.org/2290203002/diff/1/webrtc/api/statscollector.cc#... webrtc/api/statscollector.cc:385: // TODO(nisse): Helper function belongs in timeutils.h. On 2016/08/30 12:56:40, hbos wrote: > I think this TODO should be fixed in this CL before landing it. Agreed. We only have to settle on a name. https://codereview.webrtc.org/2290203002/diff/1/webrtc/media/base/rtpdataengi... File webrtc/media/base/rtpdataengine.cc (right): https://codereview.webrtc.org/2290203002/diff/1/webrtc/media/base/rtpdataengi... webrtc/media/base/rtpdataengine.cc:309: double now = rtc::TimeMicros() * 1e-6; On 2016/08/30 12:56:40, hbos wrote: > Prefer using (defining a new if you have to) constant from timeutils.h for > conversion. Perhaps add a rtc::TimeSeconds()? I'm considering that. But nothing else in timeutils.h use floating point, so a double rtc::TimeSeconds would be a bit odd. I haven't investigated how the time is used in RtpDataEngine, but I think I'd prefer to switch it to using integer time, in us or ns units. https://codereview.webrtc.org/2290203002/diff/1/webrtc/media/base/rtpdataengi... File webrtc/media/base/rtpdataengine_unittest.cc (right): https://codereview.webrtc.org/2290203002/diff/1/webrtc/media/base/rtpdataengi... webrtc/media/base/rtpdataengine_unittest.cc:60: clock_.SetTimeNanos(now * 1e9); On 2016/08/30 12:56:40, hbos wrote: > Use a constant from timeutils.h. Done.
On 2016/08/30 13:11:32, nisse-webrtc wrote: > On 2016/08/30 13:00:37, hbos wrote: > > On 2016/08/30 12:56:40, hbos wrote: > > Try using ClockInterface the way Timing was previously used in this case. > > Not sure what you mean. One could switch to Clock > (webrtc/system_wrappers/include/clock.h), but I'd prefer not to create more > dependences on that. Hah, there are even more clocks? Let's not depend on that. :) To clarify, for rtpdataengine.[h/cc]: Instead of rtc::Timing* timing_ and timing_->TimerNow() you could use rtc::ClockInterface* clock_ and clock_->TimeNanos() / static_cast<double>(kNumNanosecsPerSec). This being the same interface that rtc::TimeNanos() uses (g_clock) in timeutils.h. Then you get the same behavior as before without depending on the class you want to remove. > In my opinion, we should need only a single global clock, and then for testing > we have the ScopedFakeClock hack. There can be nothing good in having multiple > clock instances, with different parts of the code using different clocks. The test is verifying that the timestamps are based on the timers. dmc1/params1/timing1 looks independent from dmc2/params2/timing2 though, perhaps the two can be split up into two different tests? That way you can rely on a a ScopedFakeClock in the unittest. If this is a meaningful test. On 2016/08/30 13:20:25, nisse-webrtc wrote: > On 2016/08/30 12:56:40, hbos wrote: > > I think this TODO should be fixed in this CL before landing it. > > Agreed. We only have to settle on a name. > rtc::WallClockTimeNow sounds good to me.
nisse@webrtc.org changed reviewers: + pthatcher@webrtc.org - deadbeef@webrtc.org
Peter, do you know the rtpdataengine code? I have two questions: 1. Does the SendDataMultipleClocks unit test exercise any useful feature, or is it good enough for the rtpdataengine to work with a single global clock? 2. Is there any good reason why it uses double (seconds) to represent times internally? That seems unusual within webrtc.
https://codereview.webrtc.org/2290203002/diff/40001/webrtc/base/timeutils.cc File webrtc/base/timeutils.cc (right): https://codereview.webrtc.org/2290203002/diff/40001/webrtc/base/timeutils.cc#... webrtc/base/timeutils.cc:189: double WallClockSeconds() { Why don't we call this TimeDouble and use TimeNanos to calculate it? That would work better with the rest of the file, especially SetClockForTesting. And you wouldn't need to duplicate code here between POSIX and WIN. And you don't treat MAC the same way.
https://codereview.webrtc.org/2290203002/diff/40001/webrtc/api/statscollector.cc File webrtc/api/statscollector.cc (right): https://codereview.webrtc.org/2290203002/diff/40001/webrtc/api/statscollector... webrtc/api/statscollector.cc:468: double time_now = GetTimeNow(); hbos: This is the only call to GetTimeNow. For testing, would it work to move everything but this call to a helper method that takes the time as an input argument (and hence doesn't depend on any global state), and test that helper method? Then we wouldn't need any hacks in statscollector_unittests to fake this clock, and we wouldn't need the GetTimeNow virtual method. https://codereview.webrtc.org/2290203002/diff/40001/webrtc/base/timeutils.cc File webrtc/base/timeutils.cc (right): https://codereview.webrtc.org/2290203002/diff/40001/webrtc/base/timeutils.cc#... webrtc/base/timeutils.cc:189: double WallClockSeconds() { On 2016/08/31 00:10:17, pthatcher1 wrote: > Why don't we call this TimeDouble and use TimeNanos to calculate it? That would > work better with the rest of the file, especially SetClockForTesting. Because TimeNanos and related functions return monotonic time with an arbitrary epoch (clock_gettime(CLOCK_MONOTONIC, ...) on posix), while WallClockSeconds uses gettimeofday, so it gets time relative to the unix epoch, and will jump backwards or forwards if the system clock is modified using settimeofday. So they're very different. "WallClock" in the name is intended to suggest this difference. I'm open to renaming it or moving it elsewhere (before this cl it was the static method rtc::Timing::WallTimeNow, defined in timing.cc). > And you wouldn't need to duplicate code here between POSIX and WIN. And you > don't treat MAC the same way. As for Macs, my understanding is that macs are posix enough to have gettimeofday, so they don't need their own case here. (Maybe they have clock_gettime too? I really don't know why the current SystemTimeNanos needs to use low-level mach calls, but I guess clock_gettime for some reason is worse).
The CQ bit was checked by nisse@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/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_gyp_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_gyp_dbg/builds/95)
Hi, I've deleted the obsolete test, and fixed a unit bug in my change to statscollector. I have a couple of new questions. 1. The only test case failing was P2PTestConductor.GetGcmRecv, is that as expected? 2. Perhaps there should be a unit suffix on GetTimeNow. 3. If statscollector doesn't really want double (seconds) precision, there's little point in having the WallClock method return that. I'd suggest a primitive producing an int64_t with number of microseconds since the unix epoch (not nanoseconds, because (i) we won't get that precision anyway, and (ii) we would introduce a silly year 2292 problem). And a convenience helper returning millis, if that's what the statcollector uses. 4. Patch now fails and needs a rebase, because the new file rtcstatscollector also uses timing.h. I think this use is invalid, it's used in order to cache results for a limited time. This logic ought to use system monotonic time (i.e., rtc::TimeMicros, *not* wall clock time). Otherwise it will get confused if settimeofday is ever called. 5. And on second though, that applies to the caching logic in StatsCollector::UpdateStats as well. times are used for two purposes, first for the caching logic, and then they're also attached to reports like // FYI - for remote reports, the timestamp will be overwritten later. report->set_timestamp(stats_gathering_started_); The former must use rtc::TimeMicros (system monotonic time). For the second, I'm not sure. hbos, what are the report timestamps used for? If they're presented to some user, or sent to remote systems, using system monotonic time is useless, and in that case, one would need to read *both* clocks, monotonic time, *and* wall clock time. Let's discuss when you get to the office. I'd propose doing a separate cl to fix the caching logic, and land that first.
On 2016/08/31 07:11:22, nisse-webrtc wrote: > Hi, I've deleted the obsolete test, and fixed a unit bug in my change to > statscollector. I have a couple of new questions. > > 1. The only test case failing was P2PTestConductor.GetGcmRecv, is that as > expected? > > 2. Perhaps there should be a unit suffix on GetTimeNow. SGTM. > 3. If statscollector doesn't really want double (seconds) precision, there's > little point in having the WallClock method return that. I'd suggest a primitive > producing an int64_t with number of microseconds since the unix epoch (not > nanoseconds, because (i) we won't get that precision anyway, and (ii) we would > introduce a silly year 2292 problem). And a convenience helper returning millis, > if that's what the statcollector uses. I'll make the new RTCStatsCollector and RTCStats use int64_t microsecond timestamps, but I don't think we want to touch the old stats timestamps, would require lots of changes, including outside the webrtc repo. > 4. Patch now fails and needs a rebase, because the new file rtcstatscollector > also uses timing.h. I think this use is invalid, it's used in order to cache > results for a limited time. This logic ought to use system monotonic time (i.e., > rtc::TimeMicros, *not* wall clock time). Otherwise it will get confused if > settimeofday is ever called. Addressed in https://codereview.webrtc.org/2299643002/. > 5. And on second though, that applies to the caching logic in > StatsCollector::UpdateStats as well. times are used for two purposes, first for > the caching logic, and then they're also attached to reports like > > // FYI - for remote reports, the timestamp will be overwritten later. > report->set_timestamp(stats_gathering_started_); > > The former must use rtc::TimeMicros (system monotonic time). For the second, I'm > not sure. hbos, what are the report timestamps used for? If they're presented to > some user, or sent to remote systems, using system monotonic time is useless, > and in that case, one would need to read *both* clocks, monotonic time, *and* > wall clock time. Let's discuss when you get to the office. > > I'd propose doing a separate cl to fix the caching logic, and land that first. I'd have to dig deeper, I didn't write the old stats collector code.
https://codereview.webrtc.org/2290203002/diff/60001/webrtc/media/base/rtpdata... File webrtc/media/base/rtpdataengine.cc (right): https://codereview.webrtc.org/2290203002/diff/60001/webrtc/media/base/rtpdata... webrtc/media/base/rtpdataengine.cc:309: double now = rtc::TimeMicros() * 1e-6; rtc::TimeMicros() / static_cast<double>(kNumMicrosecsPerSec) https://codereview.webrtc.org/2290203002/diff/60001/webrtc/media/base/rtpdata... File webrtc/media/base/rtpdataengine_unittest.cc (left): https://codereview.webrtc.org/2290203002/diff/60001/webrtc/media/base/rtpdata... webrtc/media/base/rtpdataengine_unittest.cc:334: // Should bump timestamp by 90000 because the clock rate is 90khz. I'll leave it to pthatcher, but this doesn't look like a pointless test to me. If we want to keep a version of it, it could be tweaked to use ClockInterface instead, or perhaps it could be written differently so that two clocks are not necessary simultaneously and ScopedFakeClock can be used.
pthatcher@webrtc.org changed reviewers: + deadbeef@webrtc.org
I think Taylor would be a good reviewer for this, since he did a lot of good work with the clocks and timing recently. Can you wait until he gets back next week? https://codereview.webrtc.org/2290203002/diff/40001/webrtc/api/statscollector.cc File webrtc/api/statscollector.cc (right): https://codereview.webrtc.org/2290203002/diff/40001/webrtc/api/statscollector... webrtc/api/statscollector.cc:468: double time_now = GetTimeNow(); On 2016/08/31 06:34:41, nisse-webrtc wrote: > hbos: This is the only call to GetTimeNow. > > For testing, would it work to move everything but this call to a helper method > that takes the time as an input argument (and hence doesn't depend on any global > state), and test that helper method? Then we wouldn't need any hacks in > statscollector_unittests to fake this clock, and we wouldn't need the GetTimeNow > virtual method. I think using a ScopedFakeClock is easy. Why not just use that, like in the RtpDataEngine unittest? https://codereview.webrtc.org/2290203002/diff/40001/webrtc/base/timeutils.cc File webrtc/base/timeutils.cc (right): https://codereview.webrtc.org/2290203002/diff/40001/webrtc/base/timeutils.cc#... webrtc/base/timeutils.cc:189: double WallClockSeconds() { On 2016/08/31 06:34:41, nisse-webrtc wrote: > On 2016/08/31 00:10:17, pthatcher1 wrote: > > Why don't we call this TimeDouble and use TimeNanos to calculate it? That > would > > work better with the rest of the file, especially SetClockForTesting. > > Because TimeNanos and related functions return monotonic time with an arbitrary > epoch (clock_gettime(CLOCK_MONOTONIC, ...) on posix), while WallClockSeconds > uses gettimeofday, so it gets time relative to the unix epoch, and will jump > backwards or forwards if the system clock is modified using settimeofday. > > So they're very different. "WallClock" in the name is intended to suggest this > difference. I'm open to renaming it or moving it elsewhere (before this cl it > was the static method rtc::Timing::WallTimeNow, defined in timing.cc). > > > And you wouldn't need to duplicate code here between POSIX and WIN. And you > > don't treat MAC the same way. > > As for Macs, my understanding is that macs are posix enough to have > gettimeofday, so they don't need their own case here. (Maybe they have > clock_gettime too? I really don't know why the current SystemTimeNanos needs to > use low-level mach calls, but I guess clock_gettime for some reason is worse). But why would anything want non-monotonic time? It looks like only the StatsCollector is using it. Is that intentional? Why would we want non-monotonic timestamps in the stats? It sounds like it would drive people using the stats crazy to have non-monotonic values. https://codereview.webrtc.org/2290203002/diff/60001/webrtc/media/base/rtpdata... File webrtc/media/base/rtpdataengine_unittest.cc (left): https://codereview.webrtc.org/2290203002/diff/60001/webrtc/media/base/rtpdata... webrtc/media/base/rtpdataengine_unittest.cc:334: // Should bump timestamp by 90000 because the clock rate is 90khz. On 2016/08/31 14:43:13, hbos wrote: > I'll leave it to pthatcher, but this doesn't look like a pointless test to me. > > If we want to keep a version of it, it could be tweaked to use ClockInterface > instead, or perhaps it could be written differently so that two clocks are not > necessary simultaneously and ScopedFakeClock can be used. I think SendData is sufficient. I don't think we need SendDataMultipleClocks.
This isn't urgent, so let's wait for Taylor's feedback. https://codereview.webrtc.org/2290203002/diff/40001/webrtc/api/statscollector.cc File webrtc/api/statscollector.cc (right): https://codereview.webrtc.org/2290203002/diff/40001/webrtc/api/statscollector... webrtc/api/statscollector.cc:468: double time_now = GetTimeNow(); On 2016/08/31 18:19:46, pthatcher1 wrote: > On 2016/08/31 06:34:41, nisse-webrtc wrote: > > hbos: This is the only call to GetTimeNow. > > > > For testing, would it work to move everything but this call to a helper method > > that takes the time as an input argument (and hence doesn't depend on any > global > > state), and test that helper method? Then we wouldn't need any hacks in > > statscollector_unittests to fake this clock, and we wouldn't need the > GetTimeNow > > virtual method. > > I think using a ScopedFakeClock is easy. Why not just use that, like in the > RtpDataEngine unittest? We'd need a separate hook to fake utc time. I don't think it worth it, because it's only a single use, and this code should be obsoleted by the new code under webrtc/stats (with tests which don't try to mock the UTC clock). https://codereview.webrtc.org/2290203002/diff/40001/webrtc/base/timeutils.cc File webrtc/base/timeutils.cc (right): https://codereview.webrtc.org/2290203002/diff/40001/webrtc/base/timeutils.cc#... webrtc/base/timeutils.cc:189: double WallClockSeconds() { On 2016/08/31 18:19:46, pthatcher1 wrote: > On 2016/08/31 06:34:41, nisse-webrtc wrote: > > On 2016/08/31 00:10:17, pthatcher1 wrote: > > > Why don't we call this TimeDouble and use TimeNanos to calculate it? That > > would > > > work better with the rest of the file, especially SetClockForTesting. > > > > Because TimeNanos and related functions return monotonic time with an > arbitrary > > epoch (clock_gettime(CLOCK_MONOTONIC, ...) on posix), while WallClockSeconds > > uses gettimeofday, so it gets time relative to the unix epoch, and will jump > > backwards or forwards if the system clock is modified using settimeofday. > > > > So they're very different. "WallClock" in the name is intended to suggest this > > difference. I'm open to renaming it or moving it elsewhere (before this cl it > > was the static method rtc::Timing::WallTimeNow, defined in timing.cc). > > > > > And you wouldn't need to duplicate code here between POSIX and WIN. And you > > > don't treat MAC the same way. > > > > As for Macs, my understanding is that macs are posix enough to have > > gettimeofday, so they don't need their own case here. (Maybe they have > > clock_gettime too? I really don't know why the current SystemTimeNanos needs > to > > use low-level mach calls, but I guess clock_gettime for some reason is worse). > > But why would anything want non-monotonic time? It looks like only the > StatsCollector is using it. Is that intentional? Why would we want > non-monotonic timestamps in the stats? It sounds like it would drive people > using the stats crazy to have non-monotonic values. UTC has the advantage that you can compare the timestamps on stats from different devices. And it's *usually* monotonic. Ultimately, I think these stats are exposed in the javascript APIs, I'll have to leave to hbos to answer what's really required there. https://codereview.webrtc.org/2290203002/diff/60001/webrtc/media/base/rtpdata... File webrtc/media/base/rtpdataengine_unittest.cc (left): https://codereview.webrtc.org/2290203002/diff/60001/webrtc/media/base/rtpdata... webrtc/media/base/rtpdataengine_unittest.cc:334: // Should bump timestamp by 90000 because the clock rate is 90khz. On 2016/08/31 18:19:46, pthatcher1 wrote: > On 2016/08/31 14:43:13, hbos wrote: > > I'll leave it to pthatcher, but this doesn't look like a pointless test to me. > > > > If we want to keep a version of it, it could be tweaked to use ClockInterface > > instead, or perhaps it could be written differently so that two clocks are not > > necessary simultaneously and ScopedFakeClock can be used. > > I think SendData is sufficient. I don't think we need SendDataMultipleClocks. Good, deleted in current patchset.
On 2016/09/01 09:05:37, nisse-webrtc wrote: > Ultimately, I think these stats are exposed in the javascript APIs, I'll have to > leave to hbos to answer what's really required there. Discussed with hbos off-line. The javascript API:s require UTC (which could be debated). I guess it would be possible to let the javascript layer try to translate from monotonic time to UTC, but I think it's simpler and cleaner to attach an UTC time when the report is created. When is Taylor expected back at the office?
lgtm https://codereview.webrtc.org/2290203002/diff/80001/webrtc/api/statscollector.cc File webrtc/api/statscollector.cc (right): https://codereview.webrtc.org/2290203002/diff/80001/webrtc/api/statscollector... webrtc/api/statscollector.cc:472: LOG(LS_INFO) << "UpdateStats: time_now: " << time_now; nit: The two colons seem odd, could change it to "time_now = ". https://codereview.webrtc.org/2290203002/diff/80001/webrtc/media/base/rtpdata... File webrtc/media/base/rtpdataengine_unittest.cc (left): https://codereview.webrtc.org/2290203002/diff/80001/webrtc/media/base/rtpdata... webrtc/media/base/rtpdataengine_unittest.cc:354: } I'm no expert here but I agree that this test seems pointless, since no production code was actually constructing RtpDataEngines with different timings.
lgtm % comment below https://codereview.webrtc.org/2290203002/diff/80001/webrtc/media/base/rtpdata... File webrtc/media/base/rtpdataengine.cc (right): https://codereview.webrtc.org/2290203002/diff/80001/webrtc/media/base/rtpdata... webrtc/media/base/rtpdataengine.cc:309: double now = rtc::TimeMicros() * 1e-6; use the correct const.
Description was changed from ========== Delete Timing class, timing.h, and update all users. BUG= ========== to ========== Delete Timing class, timing.h, and update all users. BUG=webrtc:6324 ==========
Rebased, and nits addressed. I've noticed that there seems to be a couple of uses of base/timing.h in Chrome. Those have to be fixed before landing. https://codereview.webrtc.org/2290203002/diff/80001/webrtc/api/statscollector.cc File webrtc/api/statscollector.cc (right): https://codereview.webrtc.org/2290203002/diff/80001/webrtc/api/statscollector... webrtc/api/statscollector.cc:472: LOG(LS_INFO) << "UpdateStats: time_now: " << time_now; On 2016/09/07 18:39:17, Taylor Brandstetter wrote: > nit: The two colons seem odd, could change it to "time_now = ". Done. https://codereview.webrtc.org/2290203002/diff/80001/webrtc/media/base/rtpdata... File webrtc/media/base/rtpdataengine.cc (right): https://codereview.webrtc.org/2290203002/diff/80001/webrtc/media/base/rtpdata... webrtc/media/base/rtpdataengine.cc:309: double now = rtc::TimeMicros() * 1e-6; On 2016/09/08 06:07:03, perkj_webrtc wrote: > use the correct const. Done. (Then needs a static cast to get floating point rather than integer division).
lgtm https://codereview.webrtc.org/2290203002/diff/120001/webrtc/api/statscollecto... File webrtc/api/statscollector.cc (right): https://codereview.webrtc.org/2290203002/diff/120001/webrtc/api/statscollecto... webrtc/api/statscollector.cc:472: LOG(LS_INFO) << "UpdateStats: time_now = " << time_now; Do we really want this log? Its LS_INFO, but it's still spammy. https://codereview.webrtc.org/2290203002/diff/120001/webrtc/base/timeutils.h File webrtc/base/timeutils.h (right): https://codereview.webrtc.org/2290203002/diff/120001/webrtc/base/timeutils.h#... webrtc/base/timeutils.h:111: // Returns number of microseconds since January 1, 1970, UTC. Comment about it being the wall clock and that it could change, e.g. not monotonically increasing, don't use for measuring time between two calls.
https://codereview.webrtc.org/2290203002/diff/120001/webrtc/api/statscollecto... File webrtc/api/statscollector.cc (right): https://codereview.webrtc.org/2290203002/diff/120001/webrtc/api/statscollecto... webrtc/api/statscollector.cc:472: LOG(LS_INFO) << "UpdateStats: time_now = " << time_now; On 2016/09/08 07:50:43, hbos wrote: > Do we really want this log? Its LS_INFO, but it's still spammy. Probably not. I'll remove it. https://codereview.webrtc.org/2290203002/diff/120001/webrtc/base/timeutils.h File webrtc/base/timeutils.h (right): https://codereview.webrtc.org/2290203002/diff/120001/webrtc/base/timeutils.h#... webrtc/base/timeutils.h:111: // Returns number of microseconds since January 1, 1970, UTC. On 2016/09/08 07:50:43, hbos wrote: > Comment about it being the wall clock and that it could change, e.g. not > monotonically increasing, don't use for measuring time between two calls. I've added a comment on proper and improper use.
lgtm
The CQ bit was checked by nisse@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from deadbeef@webrtc.org, perkj@webrtc.org, hbos@webrtc.org Link to the patchset: https://codereview.webrtc.org/2290203002/#ps140001 (title: "Address hbos' comments.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_ubsan_vptr on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_ubsan_vptr/builds...) win_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_dbg/builds/11183)
The CQ bit was checked by nisse@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from hbos@webrtc.org, deadbeef@webrtc.org, perkj@webrtc.org, pthatcher@webrtc.org Link to the patchset: https://codereview.webrtc.org/2290203002/#ps160001 (title: "Fix copy-paste error in rtpdataengine.cc.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_arm64_rel on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL) android_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL) android_rel on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by nisse@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
Message was sent while issue was closed.
Description was changed from ========== Delete Timing class, timing.h, and update all users. BUG=webrtc:6324 ========== to ========== Delete Timing class, timing.h, and update all users. BUG=webrtc:6324 ==========
Message was sent while issue was closed.
Committed patchset #9 (id:160001)
Message was sent while issue was closed.
Description was changed from ========== Delete Timing class, timing.h, and update all users. BUG=webrtc:6324 ========== to ========== Delete Timing class, timing.h, and update all users. BUG=webrtc:6324 Committed: https://crrev.com/cdf37a929336e2ca1e0bcdd5e858ffc75b2b317e Cr-Commit-Position: refs/heads/master@{#14203} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/cdf37a929336e2ca1e0bcdd5e858ffc75b2b317e Cr-Commit-Position: refs/heads/master@{#14203} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
