Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2017443003:
Implement timestamp translation/filter in VideoCapturer. (Closed)
|
Created:
4 years, 7 months ago by nisse-webrtc Modified:
4 years, 6 months ago Reviewers:
qiangchen, tommi, Stefan, sprang_webrtc, stefan-webrtc, magjed_webrtc, mflodman, pbos-webrtc, pthatcher1 CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com, the sun, danilchap, sprang_webrtc Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionImplement timestamp translation/filter in VideoCapturer.
Use in AndroidVideoCapturer.
BUG=webrtc:5740
Committed: https://crrev.com/191b359d0de8cb9d2f05e708bd19bf66c1624805
Cr-Commit-Position: refs/heads/master@{#13254}
Patch Set 1 #Patch Set 2 : Fix android build. #
Total comments: 8
Patch Set 3 : Fix offset update equation, and nits. #
Total comments: 16
Patch Set 4 : Minor improvements to comments, formatting, etc. #Patch Set 5 : Fixes for tests. #Patch Set 6 : Rebase. #
Total comments: 18
Patch Set 7 : Add filter reset. Improve comments and naming. #Patch Set 8 : Simplify filter reset. Drop hack to detect if camera time and system time are the same. #
Total comments: 4
Patch Set 9 : Reset logic based on error between estimate and measurement. Deleted obsolete test. #Patch Set 10 : Revert unused VideoFrameFactory changes. Add missing braces. #Patch Set 11 : Add * operators in comment equations. #Patch Set 12 : Implement timestamp clipping. #Patch Set 13 : Formatting. #
Total comments: 12
Patch Set 14 : Parameterize tests of timestamp filter. #
Total comments: 1
Patch Set 15 : Moved timestamp filtering logic to a separate class. #
Total comments: 2
Patch Set 16 : Comment and format fixes. #include <algorithms>. #Patch Set 17 : Undo left-over change to videocapturer_unittest.cc. #Patch Set 18 : Rebase. #Patch Set 19 : Add explicit destructor, required by chromium style. #Patch Set 20 : Add missing include of math.h. #
Messages
Total messages: 77 (23 generated)
nisse@webrtc.org changed reviewers: + holmer@chromium.org, magjed@webrtc.org, pbos@webrtc.org
Please have a first look. This is the timestamp translation filter. I think it's both pretty simple and hopefully good enough. This doesn't quite work though, it fails WebRtcVideoEngine2Test.PropagatesInputFrameTimestamp and at least for testing, it's no good to have the VideoCapturer read the clock using rtc::TimeMicros. And it generally needs unit tests, suggestions welcome.
stefan@webrtc.org changed reviewers: + qiangchen@chromium.org, stefan@webrtc.org
+qiangchen who this likely is interesting to as well.
I think I found a math error in the code. Can you double check that? https://codereview.webrtc.org/2017443003/diff/20001/webrtc/media/base/videoca... File webrtc/media/base/videocapturer.cc (right): https://codereview.webrtc.org/2017443003/diff/20001/webrtc/media/base/videoca... webrtc/media/base/videocapturer.cc:278: offset_us_ += diff_us / frames_seen_; Can you double check the correctness of the math behind? Here frames_seen_ is a variable, not a constant. Suppose we have two arrays of observed value of x_k and y_k. This would be offset_us_ = (y_1-x_1) + (y_2 - x_2)/2 + (y_3 - x_3)/3 ... Not average of (y_k - x_k). based on your comment above I think the code should be if (frames_seen_ < kWindowSize) { offset_us_ *= frames_seen_; ++ frames_seen_; offset_us_ /= frames_seen_; offset_us_ += diff_us /frames_seen_; } else { offset_us_ = offset_us_ * rate + diff_us * (1-rate); } where |rate| is a pre-determined parameter in the exponential weighted average formula.
https://codereview.webrtc.org/2017443003/diff/20001/webrtc/media/base/videoca... File webrtc/media/base/videocapturer.cc (right): https://codereview.webrtc.org/2017443003/diff/20001/webrtc/media/base/videoca... webrtc/media/base/videocapturer.cc:266: static const int64_t kDelayLimit = 50 * rtc::kNumMicrosecsPerMillisec; Maybe just kDelayLimitUs = 50000? Easier to read, IMO. https://codereview.webrtc.org/2017443003/diff/20001/webrtc/media/base/videoca... webrtc/media/base/videocapturer.cc:276: frames_seen_++; ++frames_seen_ https://codereview.webrtc.org/2017443003/diff/20001/webrtc/media/base/videoca... webrtc/media/base/videocapturer.cc:278: offset_us_ += diff_us / frames_seen_; On 2016/05/26 20:01:35, qiangchenC wrote: > Can you double check the correctness of the math behind? > > > Here frames_seen_ is a variable, not a constant. > Suppose we have two arrays of observed value of x_k and y_k. > > This would be > offset_us_ = (y_1-x_1) + (y_2 - x_2)/2 + (y_3 - x_3)/3 ... > > Not average of (y_k - x_k). > > > based on your comment above I think the code should be > if (frames_seen_ < kWindowSize) { > offset_us_ *= frames_seen_; > ++ frames_seen_; > offset_us_ /= frames_seen_; > offset_us_ += diff_us /frames_seen_; > } else { > offset_us_ = offset_us_ * rate + diff_us * (1-rate); > } > > where |rate| is a pre-determined parameter in the exponential weighted average > formula. That's what I would have expected too.
On 2016/05/26 20:01:35, qiangchenC wrote: > I think I found a math error in the code. > Can you double check that? > > https://codereview.webrtc.org/2017443003/diff/20001/webrtc/media/base/videoca... > File webrtc/media/base/videocapturer.cc (right): > > https://codereview.webrtc.org/2017443003/diff/20001/webrtc/media/base/videoca... > webrtc/media/base/videocapturer.cc:278: offset_us_ += diff_us / frames_seen_; > Can you double check the correctness of the math behind? > > > Here frames_seen_ is a variable, not a constant. > Suppose we have two arrays of observed value of x_k and y_k. > > This would be > offset_us_ = (y_1-x_1) + (y_2 - x_2)/2 + (y_3 - x_3)/3 ... > > Not average of (y_k - x_k). Good catch, thanks! The update should be offset_us_ += (diff_us - offset_us_) / frames_seen_; which I think is simplest way of doing an incremental plain average. I really like this form, because we switch from plain averaging to exponential averaging by just letting frames_seen stick at the maximum value (feel free to suggest improvements to the naming). I guess I was confused by my naming of the input as "diff", when in fact one more diff operation is required... And the code really needs unit tests.
Fixed the error in the averaging. I wonder how to go about fixing (and extending) the WebRtcVideoEngine2Test.PropagatesInputFrameTimestamp test. Should the UpdateOffset and AdaptFrame method get system time as input, instead of calling rtc::TimeMicros? Would at least make testing simpler. But it won't help OnFrameCapture. Maybe we really need the fake clock thing, to let the test case pass in capture_time with a givn interval, with timestamps matching rtc::TimeMicros, without actually sleeping? https://codereview.webrtc.org/2017443003/diff/20001/webrtc/media/base/videoca... File webrtc/media/base/videocapturer.cc (right): https://codereview.webrtc.org/2017443003/diff/20001/webrtc/media/base/videoca... webrtc/media/base/videocapturer.cc:266: static const int64_t kDelayLimit = 50 * rtc::kNumMicrosecsPerMillisec; On 2016/05/27 00:49:10, stefan-webrtc (holmer) wrote: > Maybe just kDelayLimitUs = 50000? Easier to read, IMO. Done. https://codereview.webrtc.org/2017443003/diff/20001/webrtc/media/base/videoca... webrtc/media/base/videocapturer.cc:276: frames_seen_++; On 2016/05/27 00:49:10, stefan-webrtc (holmer) wrote: > ++frames_seen_ Done. https://codereview.webrtc.org/2017443003/diff/20001/webrtc/media/base/videoca... webrtc/media/base/videocapturer.cc:278: offset_us_ += diff_us / frames_seen_; On 2016/05/26 20:01:35, qiangchenC wrote: > Can you double check the correctness of the math behind? > > > Here frames_seen_ is a variable, not a constant. > Suppose we have two arrays of observed value of x_k and y_k. > > This would be > offset_us_ = (y_1-x_1) + (y_2 - x_2)/2 + (y_3 - x_3)/3 ... > > Not average of (y_k - x_k). > > > based on your comment above I think the code should be > if (frames_seen_ < kWindowSize) { > offset_us_ *= frames_seen_; > ++ frames_seen_; > offset_us_ /= frames_seen_; > offset_us_ += diff_us /frames_seen_; > } else { > offset_us_ = offset_us_ * rate + diff_us * (1-rate); > } > > where |rate| is a pre-determined parameter in the exponential weighted average > formula. Fixed.
lgtm https://codereview.webrtc.org/2017443003/diff/20001/webrtc/media/base/videoca... File webrtc/media/base/videocapturer.cc (right): https://codereview.webrtc.org/2017443003/diff/20001/webrtc/media/base/videoca... webrtc/media/base/videocapturer.cc:278: offset_us_ += diff_us / frames_seen_; On 2016/05/27 09:47:32, nisse-webrtc wrote: > On 2016/05/26 20:01:35, qiangchenC wrote: > > Can you double check the correctness of the math behind? > > > > > > Here frames_seen_ is a variable, not a constant. > > Suppose we have two arrays of observed value of x_k and y_k. > > > > This would be > > offset_us_ = (y_1-x_1) + (y_2 - x_2)/2 + (y_3 - x_3)/3 ... > > > > Not average of (y_k - x_k). > > > > > > based on your comment above I think the code should be > > if (frames_seen_ < kWindowSize) { > > offset_us_ *= frames_seen_; > > ++ frames_seen_; > > offset_us_ /= frames_seen_; > > offset_us_ += diff_us /frames_seen_; > > } else { > > offset_us_ = offset_us_ * rate + diff_us * (1-rate); > > } > > > > where |rate| is a pre-determined parameter in the exponential weighted average > > formula. > > Fixed. Acknowledged.
I would have used the Clock/SimulatedClock for testing.
On 2016/05/27 16:21:25, stefan-webrtc (holmer) wrote: > I would have used the Clock/SimulatedClock for testing. For cricket code, we use ClockInterface/FakeClock (or for old cricket code Timing). We should really unify all of these!
pthatcher@webrtc.org changed reviewers: + pthatcher@webrtc.org
https://codereview.webrtc.org/2017443003/diff/40001/webrtc/api/java/jni/andro... File webrtc/api/java/jni/androidvideocapturer_jni.cc (right): https://codereview.webrtc.org/2017443003/diff/40001/webrtc/api/java/jni/andro... webrtc/api/java/jni/androidvideocapturer_jni.cc:183: int64_t time_us; Can you be a little more specific about what the timestamp is? Capture time? https://codereview.webrtc.org/2017443003/diff/40001/webrtc/media/base/videoca... File webrtc/media/base/videocapturer.cc (right): https://codereview.webrtc.org/2017443003/diff/40001/webrtc/media/base/videoca... webrtc/media/base/videocapturer.cc:256: // application reads the system clock earlier. You could pass in a cricket::ClockInterface and use a cricket::FakeClock. But passing in a current time into UpdateOffset sounds cleaner to me anyway. https://codereview.webrtc.org/2017443003/diff/40001/webrtc/media/base/videoca... webrtc/media/base/videocapturer.cc:268: diff_us = 0; {}s please https://codereview.webrtc.org/2017443003/diff/40001/webrtc/media/base/videoca... webrtc/media/base/videocapturer.cc:276: ++frames_seen_; {}s here too https://codereview.webrtc.org/2017443003/diff/40001/webrtc/media/base/videoca... webrtc/media/base/videocapturer.cc:278: offset_us_ += (diff_us - offset_us_) / frames_seen_; Oh wow. I'm going to have to come back to this when I have more time. https://codereview.webrtc.org/2017443003/diff/40001/webrtc/media/base/videoca... File webrtc/media/base/videocapturer.h (right): https://codereview.webrtc.org/2017443003/diff/40001/webrtc/media/base/videoca... webrtc/media/base/videocapturer.h:230: // timestamp translated into the same scale as rtc::TimeMicros. Then why not call it capture_time_us? And why require passing both in? Shouldn't passing one in be sufficient? https://codereview.webrtc.org/2017443003/diff/40001/webrtc/media/base/videoca... webrtc/media/base/videocapturer.h:291: void UpdateOffset(int64_t capture_time_us); This method could use a comment explaining what it does. It's not obvious looking at it. https://codereview.webrtc.org/2017443003/diff/40001/webrtc/media/base/videoca... webrtc/media/base/videocapturer.h:321: int64_t offset_us_; This could use a little more clarity on what it means. https://codereview.webrtc.org/2017443003/diff/40001/webrtc/media/base/videofr... File webrtc/media/base/videoframefactory.h (right): https://codereview.webrtc.org/2017443003/diff/40001/webrtc/media/base/videofr... webrtc/media/base/videoframefactory.h:54: int64_t timestamp_offset_us_; This could use a comment about what effect is has on frames.
On 2016/05/27 16:21:25, stefan-webrtc (holmer) wrote: > I would have used the Clock/SimulatedClock for testing. After discussion with pbos, I wrote another cl to fake rtc::Time*. Not yet sure if it's the right tool for testing this code, though. See https://codereview.webrtc.org/2016863003/
I've addressed the simpler formatting and comment suggestions. https://codereview.webrtc.org/2017443003/diff/40001/webrtc/api/java/jni/andro... File webrtc/api/java/jni/androidvideocapturer_jni.cc (right): https://codereview.webrtc.org/2017443003/diff/40001/webrtc/api/java/jni/andro... webrtc/api/java/jni/androidvideocapturer_jni.cc:183: int64_t time_us; On 2016/05/27 22:47:43, pthatcher1 wrote: > Can you be a little more specific about what the timestamp is? Capture time? I changed it to translated_capture_time_us. Better? https://codereview.webrtc.org/2017443003/diff/40001/webrtc/media/base/videoca... File webrtc/media/base/videocapturer.cc (right): https://codereview.webrtc.org/2017443003/diff/40001/webrtc/media/base/videoca... webrtc/media/base/videocapturer.cc:268: diff_us = 0; On 2016/05/27 22:47:44, pthatcher1 wrote: > {}s please Done. https://codereview.webrtc.org/2017443003/diff/40001/webrtc/media/base/videoca... webrtc/media/base/videocapturer.cc:276: ++frames_seen_; On 2016/05/27 22:47:44, pthatcher1 wrote: > {}s here too Done. https://codereview.webrtc.org/2017443003/diff/40001/webrtc/media/base/videoca... File webrtc/media/base/videocapturer.h (right): https://codereview.webrtc.org/2017443003/diff/40001/webrtc/media/base/videoca... webrtc/media/base/videocapturer.h:230: // timestamp translated into the same scale as rtc::TimeMicros. On 2016/05/27 22:47:44, pthatcher1 wrote: > Then why not call it capture_time_us? > > And why require passing both in? Shouldn't passing one in be sufficient? |time_us| is an output argument, the output of the timestamp translation. The nanosecond unit on the input argument, |capture_time_ns|, is kept for now only to make the change a little smaller; it would be more consistent to use microsecond unit here too. The output argument is that time translated into rtc::TimeMicros timescale. To eliminate the call to rtc::TimeMicros from the VideoCapturer class (using global state complicates testing), I'm considering adding a second input argument, which is the current rtc::TimeMicros, or, preferably, the value as rtc::TimeMicros read as soon as possibly when the application gets the frame. To recap, the model here is that the capture timestamp has pretty good accuracy, in its own unknown timescale, while reading rtc::Time may exhibit jitter of several ms due to scheduling or poor accuracy in the OS clock. https://codereview.webrtc.org/2017443003/diff/40001/webrtc/media/base/videoca... webrtc/media/base/videocapturer.h:291: void UpdateOffset(int64_t capture_time_us); On 2016/05/27 22:47:44, pthatcher1 wrote: > This method could use a comment explaining what it does. It's not obvious > looking at it. Since it's a private method, I put the explanatory comment in the implementation file. I'm adding a short comment in the header too. https://codereview.webrtc.org/2017443003/diff/40001/webrtc/media/base/videoca... webrtc/media/base/videocapturer.h:321: int64_t offset_us_; On 2016/05/27 22:47:44, pthatcher1 wrote: > This could use a little more clarity on what it means. I'm adding a short comment. https://codereview.webrtc.org/2017443003/diff/40001/webrtc/media/base/videofr... File webrtc/media/base/videoframefactory.h (right): https://codereview.webrtc.org/2017443003/diff/40001/webrtc/media/base/videofr... webrtc/media/base/videoframefactory.h:54: int64_t timestamp_offset_us_; On 2016/05/27 22:47:44, pthatcher1 wrote: > This could use a comment about what effect is has on frames. Done.
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/patch-status/2017443003/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2017443003/80001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_compile_x86_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_compile_x86_dbg...) ios64_gn_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios64_gn_dbg/builds/157) ios64_gn_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios64_gn_rel/builds/152) ios64_sim_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios64_sim_dbg/builds/7882) ios_api_framework on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_api_framework/build...) ios_arm64_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_dbg/builds/10219) ios_arm64_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_rel/builds/10143) ios_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_dbg/builds/15436) linux_libfuzzer_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_libfuzzer_rel/bui...) linux_ubsan on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_ubsan/builds/2481) linux_ubsan_vptr on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_ubsan_vptr/builds...) mac_compile_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_compile_dbg/builds/...) mac_gn_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_gn_dbg/builds/3582) mac_gn_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_gn_rel/builds/9801)
Another update, where tests hopefully pass. Changes: 1. I disable use of the timestamp translator on the path involving OnCapturedFrame and VideoFrameFactory. This makes the old test pass with no changes. I'd like to ultimately delete this code path; if you think this choice is right, I'll also revert the changes to VideoFrameFactory (the SetTimestampOffset method). 2. I added current system time as an input to AdaptFrame and UpdateOffset in VideoCapturer. 3. I added unittests. After this cl is landed, I intend to reorganize the chrome code to delete its VideoFrameFactory and make it use the new methods, including the timestamp translator. Another question: It would make sense to reset the filter when the capturer is stopped and later restarted. But I don't see any method in the base class which are called at that time (except the wrapper StartCapturing, which I think isn't intended to stay in the base class). Am I missing something? Otherwise I guess I can add a check like if (std::abs(diff_us - offset_us_) > kSomeThreshold) frames_seen_ = 0; as mentioned in a TODO comment.
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/patch-status/2017443003/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2017443003/100001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_x64_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_x64_rel/builds/15134)
https://codereview.webrtc.org/2017443003/diff/100001/webrtc/media/base/videoc... File webrtc/media/base/videocapturer.cc (right): https://codereview.webrtc.org/2017443003/diff/100001/webrtc/media/base/videoc... webrtc/media/base/videocapturer.cc:270: } Why do we have to do this? https://codereview.webrtc.org/2017443003/diff/100001/webrtc/media/base/videoc... webrtc/media/base/videocapturer.cc:274: // frames_seen_ to zero. I'd suggest using cusum change detection to detect this, as we do in timestamp_extrapolator.cc. Look for TimestampExtrapolator::DelayChangeDetection(). I do think that it would be a good idea to have detection like that as it isn't unlikely that something could happen... https://codereview.webrtc.org/2017443003/diff/100001/webrtc/media/base/videoc... webrtc/media/base/videocapturer.cc:292: int64_t* time_us) { Should this be called something else? system_capture_time_us? https://codereview.webrtc.org/2017443003/diff/100001/webrtc/media/base/videoc... webrtc/media/base/videocapturer.cc:335: // TODO(nisse): Disable use of the timestamp Should this be fixed now? https://codereview.webrtc.org/2017443003/diff/100001/webrtc/media/base/videoc... File webrtc/media/base/videocapturer.h (right): https://codereview.webrtc.org/2017443003/diff/100001/webrtc/media/base/videoc... webrtc/media/base/videocapturer.h:230: // timestamp translated into the same scale as rtc::TimeMicros. Comment on system_time_us to make it clear that it should correspond to the system time of the capture. https://codereview.webrtc.org/2017443003/diff/100001/webrtc/media/base/videoc... webrtc/media/base/videocapturer.h:322: unsigned frames_seen_; int
https://codereview.webrtc.org/2017443003/diff/100001/webrtc/media/base/videoc... File webrtc/media/base/videocapturer.cc (right): https://codereview.webrtc.org/2017443003/diff/100001/webrtc/media/base/videoc... webrtc/media/base/videocapturer.cc:270: } On 2016/06/08 11:28:42, stefan-webrtc (holmer) wrote: > Why do we have to do this? The above? I'd like the filtering to add jitter in the case the incoming timestamps are already system time. An alternative might be to let applications which use system time somehow bypass the translation. Which sounds like the right thing, but I'm afraid it's not so easy. The application might not really know, and if we start depending on having system time, we might not want to trust the application to get it right. https://codereview.webrtc.org/2017443003/diff/100001/webrtc/media/base/videoc... webrtc/media/base/videocapturer.cc:274: // frames_seen_ to zero. On 2016/06/08 11:28:42, stefan-webrtc (holmer) wrote: > I'd suggest using cusum change detection to detect this, as we do in > timestamp_extrapolator.cc. Look for > TimestampExtrapolator::DelayChangeDetection(). > > I do think that it would be a good idea to have detection like that as it isn't > unlikely that something could happen... I think cusum is overkill (and when possible, I try to avoid algorithms with complicated thresholds). Do you think a simple comparison to a limit of a few 100 ms will be good enough? I'm adding that (even though I'd prefer an explicit reset on capture start). My thinking is that if we're more than 500 ms off, whatever the reason (camera being reset, or application not getting scheduled), we're having some kind of glitch anyway and resetting the translation shouldn't make things noticably worse. While if we have a camera time jump less than 500 ms, converging gradually over the next few hundred frames should also give a tolerable experience. A slightly different approach could be to remember the previous camera timestamp. If the camera time is earlier than then previous time, or more than 0.5 s later, reset the filter, regardless of whether or not the current offset happens to be close. Under the theory that this filter really shouldn't hold on to historic data. https://codereview.webrtc.org/2017443003/diff/100001/webrtc/media/base/videoc... webrtc/media/base/videocapturer.cc:292: int64_t* time_us) { On 2016/06/08 11:28:42, stefan-webrtc (holmer) wrote: > Should this be called something else? system_capture_time_us? Maybe. In some of the callers, I use the name translated_camera_time_us. Is that better, or do you prefer system_capture_time_us? https://codereview.webrtc.org/2017443003/diff/100001/webrtc/media/base/videoc... webrtc/media/base/videocapturer.cc:335: // TODO(nisse): Disable use of the timestamp On 2016/06/08 11:28:42, stefan-webrtc (holmer) wrote: > Should this be fixed now? The comment is perhaps badly phrased, I'm trying to make it clearer. The translation *is* now disabled in this case. My plan is that as soon as this cl is landed, Chrome should be updated to use this interface and purged from all usage of VideoFrameFactory and SignalFrameCaptured. And at that point, this function becomes mostly irrelevant, and we should figure out how to schedule its removal (together with VideoFrameFactory and SignalFrameCaptured). https://codereview.webrtc.org/2017443003/diff/100001/webrtc/media/base/videoc... File webrtc/media/base/videocapturer.h (right): https://codereview.webrtc.org/2017443003/diff/100001/webrtc/media/base/videoc... webrtc/media/base/videocapturer.h:230: // timestamp translated into the same scale as rtc::TimeMicros. On 2016/06/08 11:28:42, stefan-webrtc (holmer) wrote: > Comment on system_time_us to make it clear that it should correspond to the > system time of the capture. I've expanded the comments here. https://codereview.webrtc.org/2017443003/diff/100001/webrtc/media/base/videoc... webrtc/media/base/videocapturer.h:322: unsigned frames_seen_; On 2016/06/08 11:28:42, stefan-webrtc (holmer) wrote: > int Done.
https://codereview.webrtc.org/2017443003/diff/100001/webrtc/media/base/videoc... File webrtc/media/base/videocapturer.cc (right): https://codereview.webrtc.org/2017443003/diff/100001/webrtc/media/base/videoc... webrtc/media/base/videocapturer.cc:270: } On 2016/06/09 10:02:58, nisse-webrtc wrote: > On 2016/06/08 11:28:42, stefan-webrtc (holmer) wrote: > > Why do we have to do this? > > The above? I'd like the filtering to add jitter in the case the incoming > timestamps are already system time. > > An alternative might be to let applications which use system time somehow bypass > the translation. Which sounds like the right thing, but I'm afraid it's not so > easy. The application might not really know, and if we start depending on having > system time, we might not want to trust the application to get it right. But if it's using system time, shouldn't diff_us be very very close to zero? The jitter added should be super small after filtering. It seems like it will mostly complicate the code with another threshold, and at the same time it may hide bugs in the filtering since only a few cameras may actually run the filtering code. https://codereview.webrtc.org/2017443003/diff/100001/webrtc/media/base/videoc... webrtc/media/base/videocapturer.cc:274: // frames_seen_ to zero. On 2016/06/09 10:02:58, nisse-webrtc wrote: > On 2016/06/08 11:28:42, stefan-webrtc (holmer) wrote: > > I'd suggest using cusum change detection to detect this, as we do in > > timestamp_extrapolator.cc. Look for > > TimestampExtrapolator::DelayChangeDetection(). > > > > I do think that it would be a good idea to have detection like that as it > isn't > > unlikely that something could happen... > > I think cusum is overkill (and when possible, I try to avoid algorithms with > complicated thresholds). Do you think a simple comparison to a limit of a few > 100 ms will be good enough? I'm adding that (even though I'd prefer an explicit > reset on capture start). > > My thinking is that if we're more than 500 ms off, whatever the reason (camera > being reset, or application not getting scheduled), we're having some kind of > glitch anyway and resetting the translation shouldn't make things noticably > worse. While if we have a camera time jump less than 500 ms, converging > gradually over the next few hundred frames should also give a tolerable > experience. > > A slightly different approach could be to remember the previous camera > timestamp. If the camera time is earlier than then previous time, or more than > 0.5 s later, reset the filter, regardless of whether or not the current offset > happens to be close. Under the theory that this filter really shouldn't hold on > to historic data. I think the latter approach sounds pretty good then. As you say, the filter will adapt to the change, so if it was small enough we should recover fairly quickly. https://codereview.webrtc.org/2017443003/diff/100001/webrtc/media/base/videoc... webrtc/media/base/videocapturer.cc:292: int64_t* time_us) { On 2016/06/09 10:02:58, nisse-webrtc wrote: > On 2016/06/08 11:28:42, stefan-webrtc (holmer) wrote: > > Should this be called something else? system_capture_time_us? > > Maybe. In some of the callers, I use the name translated_camera_time_us. Is that > better, or do you prefer system_capture_time_us? Both work for me. https://codereview.webrtc.org/2017443003/diff/100001/webrtc/media/base/videoc... webrtc/media/base/videocapturer.cc:335: // TODO(nisse): Disable use of the timestamp On 2016/06/09 10:02:58, nisse-webrtc wrote: > On 2016/06/08 11:28:42, stefan-webrtc (holmer) wrote: > > Should this be fixed now? > > The comment is perhaps badly phrased, I'm trying to make it clearer. The > translation *is* now disabled in this case. My plan is that as soon as this cl > is landed, Chrome should be updated to use this interface and purged from all > usage of VideoFrameFactory and SignalFrameCaptured. > > And at that point, this function becomes mostly irrelevant, and we should figure > out how to schedule its removal (together with VideoFrameFactory and > SignalFrameCaptured). Acknowledged.
https://codereview.webrtc.org/2017443003/diff/100001/webrtc/media/base/videoc... File webrtc/media/base/videocapturer.cc (right): https://codereview.webrtc.org/2017443003/diff/100001/webrtc/media/base/videoc... webrtc/media/base/videocapturer.cc:270: } On 2016/06/09 11:59:09, stefan-webrtc (holmer) wrote: > On 2016/06/09 10:02:58, nisse-webrtc wrote: > > On 2016/06/08 11:28:42, stefan-webrtc (holmer) wrote: > > > Why do we have to do this? > > > > The above? I'd like the filtering to add jitter in the case the incoming > > timestamps are already system time. > > > > An alternative might be to let applications which use system time somehow > bypass > > the translation. Which sounds like the right thing, but I'm afraid it's not so > > easy. The application might not really know, and if we start depending on > having > > system time, we might not want to trust the application to get it right. > > But if it's using system time, shouldn't diff_us be very very close to zero? The > jitter added should be super small after filtering. It seems like it will mostly > complicate the code with another threshold, and at the same time it may hide > bugs in the filtering since only a few cameras may actually run the filtering > code. I think you're right, it's unlikely to make any noticable difference. Deleting it for now, we could add it back later if need arises. https://codereview.webrtc.org/2017443003/diff/100001/webrtc/media/base/videoc... webrtc/media/base/videocapturer.cc:274: // frames_seen_ to zero. On 2016/06/09 11:59:09, stefan-webrtc (holmer) wrote: > On 2016/06/09 10:02:58, nisse-webrtc wrote: > > On 2016/06/08 11:28:42, stefan-webrtc (holmer) wrote: > > > I'd suggest using cusum change detection to detect this, as we do in > > > timestamp_extrapolator.cc. Look for > > > TimestampExtrapolator::DelayChangeDetection(). > > > > > > I do think that it would be a good idea to have detection like that as it > > isn't > > > unlikely that something could happen... > > > > I think cusum is overkill (and when possible, I try to avoid algorithms with > > complicated thresholds). Do you think a simple comparison to a limit of a few > > 100 ms will be good enough? I'm adding that (even though I'd prefer an > explicit > > reset on capture start). > > > > My thinking is that if we're more than 500 ms off, whatever the reason (camera > > being reset, or application not getting scheduled), we're having some kind of > > glitch anyway and resetting the translation shouldn't make things noticably > > worse. While if we have a camera time jump less than 500 ms, converging > > gradually over the next few hundred frames should also give a tolerable > > experience. > > > > A slightly different approach could be to remember the previous camera > > timestamp. If the camera time is earlier than then previous time, or more than > > 0.5 s later, reset the filter, regardless of whether or not the current offset > > happens to be close. Under the theory that this filter really shouldn't hold > on > > to historic data. > > I think the latter approach sounds pretty good then. As you say, the filter will > adapt to the change, so if it was small enough we should recover fairly quickly. I changed it to the second approach changed, i.e., onyl check for jumps in the camera time.
https://codereview.webrtc.org/2017443003/diff/140001/webrtc/media/base/videoc... File webrtc/media/base/videocapturer.cc (right): https://codereview.webrtc.org/2017443003/diff/140001/webrtc/media/base/videoc... webrtc/media/base/videocapturer.cc:263: || camera_time_us > prev_camera_time_us_ + kResetLimitUs) { Hm, what if we have only 1 fps input? I think we need to be comparing camera_time_us - prev_camera_time_us_ to system_time_us - prev_system_time_us_.
https://codereview.webrtc.org/2017443003/diff/140001/webrtc/media/base/videoc... File webrtc/media/base/videocapturer.cc (right): https://codereview.webrtc.org/2017443003/diff/140001/webrtc/media/base/videoc... webrtc/media/base/videocapturer.cc:263: || camera_time_us > prev_camera_time_us_ + kResetLimitUs) { On 2016/06/10 13:13:31, stefan-webrtc (holmer) wrote: > Hm, what if we have only 1 fps input? Then we'll ignore the camera timestamp and use only system time. I'm thinking that high accuracy for the frame timestamps is beneficial mainly for high-fps video, so I wouldn't expect it to be a problem. Or do you think jitter on the system time reading would cause any noticable problem in a 1 fps (or 2 fps) video stream? > I think we need to be comparing > camera_time_us - prev_camera_time_us_ to system_time_us - prev_system_time_us_. If we want to avoid resetting the filter in the case that (1) there are largish intervals between frames, and (2) the camera time still is consistent with no jumps, then I think it's better to go back to comparing std::abs(diff_us - offset_us_) like in an earlier patch set. Then we don't need any additional state just for the reset logic. (And to repeat, I would prefer to not have any automatic reset logic here, and instead do a reset on StartCapturing. But that seems tricky to do with the current organization of the interface).
https://codereview.webrtc.org/2017443003/diff/140001/webrtc/media/base/videoc... File webrtc/media/base/videocapturer.cc (right): https://codereview.webrtc.org/2017443003/diff/140001/webrtc/media/base/videoc... webrtc/media/base/videocapturer.cc:263: || camera_time_us > prev_camera_time_us_ + kResetLimitUs) { On 2016/06/13 06:52:49, nisse-webrtc wrote: > On 2016/06/10 13:13:31, stefan-webrtc (holmer) wrote: > > Hm, what if we have only 1 fps input? > > Then we'll ignore the camera timestamp and use only system time. I'm thinking > that high accuracy for the frame timestamps is beneficial mainly for high-fps > video, so I wouldn't expect it to be a problem. Or do you think jitter on the > system time reading would cause any noticable problem in a 1 fps (or 2 fps) > video stream? This is probably true, but it seems a bit ugly to me to have an arbitrary threshold when we no longer care about the capture timestamp and instead reset the filter on every frame. > > > I think we need to be comparing > > camera_time_us - prev_camera_time_us_ to system_time_us - > prev_system_time_us_. > > If we want to avoid resetting the filter in the case that (1) there are largish > intervals between frames, and (2) the camera time still is consistent with no > jumps, then I think it's better to go back to comparing > > std::abs(diff_us - offset_us_) > > like in an earlier patch set. Then we don't need any additional state just for > the reset logic. (And to repeat, I would prefer to not have any automatic reset > logic here, and instead do a reset on StartCapturing. But that seems tricky to > do with the current organization of the interface). Ok, maybe the abs diff is better then. I'm fine with that.
https://codereview.webrtc.org/2017443003/diff/140001/webrtc/media/base/videoc... File webrtc/media/base/videocapturer.cc (right): https://codereview.webrtc.org/2017443003/diff/140001/webrtc/media/base/videoc... webrtc/media/base/videocapturer.cc:263: || camera_time_us > prev_camera_time_us_ + kResetLimitUs) { On 2016/06/13 08:56:01, stefan-webrtc (holmer) wrote: > On 2016/06/13 06:52:49, nisse-webrtc wrote: > > On 2016/06/10 13:13:31, stefan-webrtc (holmer) wrote: > > > Hm, what if we have only 1 fps input? > > > > Then we'll ignore the camera timestamp and use only system time. I'm thinking > > that high accuracy for the frame timestamps is beneficial mainly for high-fps > > video, so I wouldn't expect it to be a problem. Or do you think jitter on the > > system time reading would cause any noticable problem in a 1 fps (or 2 fps) > > video stream? > > This is probably true, but it seems a bit ugly to me to have an arbitrary > threshold when we no longer care about the capture timestamp and instead reset > the filter on every frame. > > > > > > I think we need to be comparing > > > camera_time_us - prev_camera_time_us_ to system_time_us - > > prev_system_time_us_. > > > > If we want to avoid resetting the filter in the case that (1) there are > largish > > intervals between frames, and (2) the camera time still is consistent with no > > jumps, then I think it's better to go back to comparing > > > > std::abs(diff_us - offset_us_) > > > > like in an earlier patch set. Then we don't need any additional state just for > > the reset logic. (And to repeat, I would prefer to not have any automatic > reset > > logic here, and instead do a reset on StartCapturing. But that seems tricky to > > do with the current organization of the interface). > > Ok, maybe the abs diff is better then. I'm fine with that. I've changed the reset logic back. Also added a log message when reset happens.
lgtm
Peter and peter: Please have a look, I'd like to get this landed in reasonable time before vacation.
nisse@webrtc.org changed reviewers: + mflodman@webrtc.org
+ mflodman Discussed with pbos. There are two potential problems with the filter: 1. There's no guarantee that translated timestamps are monotonic. 2. There's no guarantee that translated timestamps are not in the future. For (1), I don't think that's a big problem. I haven't really analyzed the conditions under which this can happen, but I'd expect it to be rare. And occasional frames arriving with timestamps out of order has to be handled elsewhere anyway, for the case of switching sources. For (2), I'm pretty sure it can happen consistently if the camera clock has a large drift in the wrong direction, resulting in translated timestamps a few ms into the future. Unclear what assumptions elsewhere in the code this might violate, but it does breaks a quite reasonable expectations. If needed, (2) can be fixed with a post filter running with a longer time constant. Not trivial, but not extremely complicated either (estimate mean and stddev of system_time_us - *translated_camera_time, subtract to get some reasonable margin), and clip the rare remaining timestamps that belong to the future. Or extend the current filter to also get c_1 coefficient (to reduce the static bias which we otherwise get from camera clock drift). But my current feeling is that a post filter is preferable because it makes things simpler, and because it seems good to be able to run it with a longer time constant.
On 2016/06/14 10:56:42, nisse-webrtc wrote: > For (2), I'm pretty sure it can happen consistently if the camera clock has a > large drift in the wrong direction, resulting in translated timestamps a few ms > into the future. Unclear what assumptions elsewhere in the code this might > violate, but it does breaks a quite reasonable expectations. If the camera clock is drifting, the translated timestamp gets a bias. For the k:th frame up to k == 100, the bias is half the camera clock's drift over a time period of k frames. Positive or negative depending on the whether the camera clock runs at a too high or too low frequency. Later on, the bias converges exponentially to equal the camera clock's drift over 100 frames, which should be pretty small. Discussed offline with sprang, and decided to start with a simple clipping mechanism after the filter. In the case the camera clock drifts in the direction which pushes timestamps into the future, the effect will be to initially stick to the system clock only, and take advantage of the supposedly higher accuracy of the camera clock only after things have settled, which may take a few seconds. Only if this turns out to not be good enough, extend the estimator to compute the estimated drift (c_1 coefficient) and noise level.
On 2016/06/15 13:25:12, nisse-webrtc wrote: > Discussed offline with sprang, and decided to start with a simple clipping > mechanism after the filter. Implemented now. Only a dozen lines of code, plus test cases which get a bit more complicated.
Description was changed from ========== Implement timestamp translation/filter in VideoCapturer. Use in AndroidVideoCapturer and WebRtcVideoFrameFactory. BUG=webrtc:5740 ========== to ========== Implement timestamp translation/filter in VideoCapturer. Use in AndroidVideoCapturer. BUG=webrtc:5740 ==========
sprang@webrtc.org changed reviewers: + sprang@webrtc.org
Just some minor comments https://codereview.webrtc.org/2017443003/diff/240001/webrtc/media/base/videoc... File webrtc/media/base/videocapturer.cc (right): https://codereview.webrtc.org/2017443003/diff/240001/webrtc/media/base/videoc... webrtc/media/base/videocapturer.cc:283: } Wow, quite extensive commenting there! Guess it will likely be great for posterity though, thanks! :) https://codereview.webrtc.org/2017443003/diff/240001/webrtc/media/base/videoc... File webrtc/media/base/videocapturer.h (right): https://codereview.webrtc.org/2017443003/diff/240001/webrtc/media/base/videoc... webrtc/media/base/videocapturer.h:284: // Access intended only for testing. Can't we use friend class to expose this instead, or do we really hate those? https://codereview.webrtc.org/2017443003/diff/240001/webrtc/media/base/videoc... File webrtc/media/base/videocapturer_unittest.cc (right): https://codereview.webrtc.org/2017443003/diff/240001/webrtc/media/base/videoc... webrtc/media/base/videocapturer_unittest.cc:818: const int nframes = 2 * window_size; Think also these should have kConstantVariable formatting. And preferreably kNumFrames instead of nFrames https://codereview.webrtc.org/2017443003/diff/240001/webrtc/media/base/videoc... webrtc/media/base/videocapturer_unittest.cc:879: TEST_F(VideoCapturerTest, AttenuateTimestampJitterNegDrift) { A lot of code duplication here. Could you extract most of this logic to a helper class that takes rel_freq_error as a parameter and condition the bias_us checks on rel_freq_error > 0 (if that is what's going on?) The you could also easily have test cases for large and small negative and positive drift, plus a zero drift just for the sake of it. wdyt?
https://codereview.webrtc.org/2017443003/diff/240001/webrtc/media/base/videoc... File webrtc/media/base/videocapturer.h (right): https://codereview.webrtc.org/2017443003/diff/240001/webrtc/media/base/videoc... webrtc/media/base/videocapturer.h:284: // Access intended only for testing. On 2016/06/17 13:10:52, språng wrote: > Can't we use friend class to expose this instead, or do we really hate those? I have no strong opinion on that (and not much experience with the C++ "friend" feature). But to me, a protected non-virtual inline method seems very cheap in all respects. A friend declaration referencing some class in the testsuite (if that's what you are suggesting?) seems kind-of ugly. https://codereview.webrtc.org/2017443003/diff/240001/webrtc/media/base/videoc... File webrtc/media/base/videocapturer_unittest.cc (right): https://codereview.webrtc.org/2017443003/diff/240001/webrtc/media/base/videoc... webrtc/media/base/videocapturer_unittest.cc:818: const int nframes = 2 * window_size; On 2016/06/17 13:10:52, språng wrote: > Think also these should have kConstantVariable formatting. > And preferreably kNumFrames instead of nFrames CamelCaseEvenForLocalVariables? Sure, I can do that, even if I find it ugly. No new patchset at the moment, though. https://codereview.webrtc.org/2017443003/diff/240001/webrtc/media/base/videoc... webrtc/media/base/videocapturer_unittest.cc:879: TEST_F(VideoCapturerTest, AttenuateTimestampJitterNegDrift) { On 2016/06/17 13:10:52, språng wrote: > A lot of code duplication here. Could you extract most of this logic to a helper > class that takes rel_freq_error as a parameter and condition the bias_us checks > on rel_freq_error > 0 (if that is what's going on?) > > The you could also easily have test cases for large and small negative and > positive drift, plus a zero drift just for the sake of it. wdyt? Makes some sense, I can give it a try. In general, I'm a lot more tolerant to code duplication in test code than in other code. E.g, there might be a need to do something special for one particular test, and that gets more difficult if it's down in some shared helper.
https://codereview.webrtc.org/2017443003/diff/240001/webrtc/media/base/videoc... File webrtc/media/base/videocapturer.h (right): https://codereview.webrtc.org/2017443003/diff/240001/webrtc/media/base/videoc... webrtc/media/base/videocapturer.h:284: // Access intended only for testing. On 2016/06/17 13:43:01, nisse-webrtc wrote: > On 2016/06/17 13:10:52, språng wrote: > > Can't we use friend class to expose this instead, or do we really hate those? > > I have no strong opinion on that (and not much experience with the C++ "friend" > feature). But to me, a protected non-virtual inline method seems very cheap in > all respects. A friend declaration referencing some class in the testsuite (if > that's what you are suggesting?) seems kind-of ugly. Friend class isn't pretty, but exposing and commenting that it's only for test is no fun either imo. But I'm not religious about it either, so do as you like https://codereview.webrtc.org/2017443003/diff/240001/webrtc/media/base/videoc... File webrtc/media/base/videocapturer_unittest.cc (right): https://codereview.webrtc.org/2017443003/diff/240001/webrtc/media/base/videoc... webrtc/media/base/videocapturer_unittest.cc:818: const int nframes = 2 * window_size; On 2016/06/17 13:43:01, nisse-webrtc wrote: > On 2016/06/17 13:10:52, språng wrote: > > Think also these should have kConstantVariable formatting. > > And preferreably kNumFrames instead of nFrames > > CamelCaseEvenForLocalVariables? Sure, I can do that, even if I find it ugly. No > new patchset at the moment, though. Makes it clear which ones are just named constant and which are actual variables. And I for one like the kConstantValue better than chromes CONSTANT_VALUE, if I can add some fuel to the flame war :P https://codereview.webrtc.org/2017443003/diff/240001/webrtc/media/base/videoc... webrtc/media/base/videocapturer_unittest.cc:879: TEST_F(VideoCapturerTest, AttenuateTimestampJitterNegDrift) { On 2016/06/17 13:43:01, nisse-webrtc wrote: > On 2016/06/17 13:10:52, språng wrote: > > A lot of code duplication here. Could you extract most of this logic to a > helper > > class that takes rel_freq_error as a parameter and condition the bias_us > checks > > on rel_freq_error > 0 (if that is what's going on?) > > > > The you could also easily have test cases for large and small negative and > > positive drift, plus a zero drift just for the sake of it. wdyt? > > Makes some sense, I can give it a try. > > In general, I'm a lot more tolerant to code duplication in test code than in > other code. E.g, there might be a need to do something special for one > particular test, and that gets more difficult if it's down in some shared > helper. Agree, some code duplication in tests are fine. In this case it's not trivial code though, and especially if makes it easy to add more test varieties I think it's worth a refactoring.
https://codereview.webrtc.org/2017443003/diff/240001/webrtc/media/base/videoc... File webrtc/media/base/videocapturer_unittest.cc (right): https://codereview.webrtc.org/2017443003/diff/240001/webrtc/media/base/videoc... webrtc/media/base/videocapturer_unittest.cc:818: const int nframes = 2 * window_size; On 2016/06/17 13:10:52, språng wrote: > Think also these should have kConstantVariable formatting. > And preferreably kNumFrames instead of nFrames Done. https://codereview.webrtc.org/2017443003/diff/240001/webrtc/media/base/videoc... webrtc/media/base/videocapturer_unittest.cc:879: TEST_F(VideoCapturerTest, AttenuateTimestampJitterNegDrift) { On 2016/06/17 13:10:52, språng wrote: > A lot of code duplication here. Could you extract most of this logic to a helper > class that takes rel_freq_error as a parameter and condition the bias_us checks > on rel_freq_error > 0 (if that is what's going on?) > > The you could also easily have test cases for large and small negative and > positive drift, plus a zero drift just for the sake of it. wdyt? Done. https://codereview.webrtc.org/2017443003/diff/260001/webrtc/media/base/videoc... File webrtc/media/base/videocapturer.cc (right): https://codereview.webrtc.org/2017443003/diff/260001/webrtc/media/base/videoc... webrtc/media/base/videocapturer.cc:289: clip_bias_us_ = 0; It might make sense to initialize it to a positive value (say, 10 ms or so), to make it constant in the common case.
Consider breaking out this to a separate TimestampAligner class, perhaps in webrtc/base, we might want to align other times and not just camera timestamps (such as remote timestamps). Up to you though.
On 2016/06/21 12:18:31, pbos-webrtc wrote: > Consider breaking out this to a separate TimestampAligner class, perhaps in > webrtc/base, we might want to align other times and not just camera timestamps > (such as remote timestamps). Up to you though. Done. Is base/timestampaligner.h a good place? It's not intended as a public api, but the presubmit script thinks that it is.
Is there a test that makes sense that verifies that we're handling the case well where the capturer timestamp is more precise than the system clock? https://codereview.webrtc.org/2017443003/diff/280001/webrtc/base/timestampali... File webrtc/base/timestampaligner_unittest.cc (right): https://codereview.webrtc.org/2017443003/diff/280001/webrtc/base/timestampali... webrtc/base/timestampaligner_unittest.cc:20: // by exponential averaging with weight 1/|window_size| for each new sample. " / "
On 2016/06/21 14:49:50, pbos-webrtc wrote: > Is there a test that makes sense that verifies that we're handling the case well > where the capturer timestamp is more precise than the system clock? That's what the TimestampAligner tests are about, they add 5 ms jitter on the system clock input (system_measured_us), and verify that the jitter is reduced on the translated timestamp output.
https://codereview.webrtc.org/2017443003/diff/280001/webrtc/base/timestampali... File webrtc/base/timestampaligner_unittest.cc (right): https://codereview.webrtc.org/2017443003/diff/280001/webrtc/base/timestampali... webrtc/base/timestampaligner_unittest.cc:20: // by exponential averaging with weight 1/|window_size| for each new sample. On 2016/06/21 14:49:50, pbos-webrtc wrote: > " / " Done. Also updated the invalid "VideoCapturer" reference below.
lgtm
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/patch-status/2017443003/320001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_clang_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_clang_dbg/build...) android_gn_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_gn_rel/builds/1...) ios64_gn_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios64_gn_dbg/builds/692) ios64_gn_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios64_gn_rel/builds/692) ios64_sim_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios64_sim_dbg/builds/8436) ios_api_framework on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_api_framework/build...) ios_arm64_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_dbg/builds/10755) ios_arm64_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_rel/builds/10676) ios_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_dbg/builds/15991) ios_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_rel/builds/14629) linux_asan on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_asan/builds/15638) linux_gn_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_gn_rel/builds/11854) linux_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_rel/builds/16088) linux_tsan2 on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_tsan2/builds/13091) linux_ubsan on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_ubsan/builds/2977) linux_ubsan_vptr on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_ubsan_vptr/builds...) mac_compile_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_compile_dbg/builds/...) mac_gn_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_gn_rel/builds/10358) mac_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_rel/builds/16077)
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/patch-status/2017443003/340001
nisse@webrtc.org changed reviewers: + tommi@webrtc.org
Tommi: Please have a look. OWNER's approval needed.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios_arm64_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_dbg/builds/10756) ios_arm64_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_rel/builds/10677) mac_gn_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_gn_rel/builds/10359)
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/patch-status/2017443003/360001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios_arm64_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_dbg/builds/10757) ios_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_dbg/builds/15993) ios_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_rel/builds/14631)
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/patch-status/2017443003/380001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Rs lgtm
The CQ bit was checked by nisse@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from qiangchen@chromium.org, stefan@webrtc.org, sprang@webrtc.org Link to the patchset: https://codereview.webrtc.org/2017443003/#ps380001 (title: "Add missing include of math.h.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2017443003/380001
Message was sent while issue was closed.
Description was changed from ========== Implement timestamp translation/filter in VideoCapturer. Use in AndroidVideoCapturer. BUG=webrtc:5740 ========== to ========== Implement timestamp translation/filter in VideoCapturer. Use in AndroidVideoCapturer. BUG=webrtc:5740 ==========
Message was sent while issue was closed.
Committed patchset #20 (id:380001)
Message was sent while issue was closed.
Description was changed from ========== Implement timestamp translation/filter in VideoCapturer. Use in AndroidVideoCapturer. BUG=webrtc:5740 ========== to ========== Implement timestamp translation/filter in VideoCapturer. Use in AndroidVideoCapturer. BUG=webrtc:5740 Committed: https://crrev.com/191b359d0de8cb9d2f05e708bd19bf66c1624805 Cr-Commit-Position: refs/heads/master@{#13254} ==========
Message was sent while issue was closed.
Patchset 20 (id:??) landed as https://crrev.com/191b359d0de8cb9d2f05e708bd19bf66c1624805 Cr-Commit-Position: refs/heads/master@{#13254} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
