Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2325563002:
New method TimestampAligner::TranslateTimestamp (Closed)
|
Created:
4 years, 3 months ago by nisse-webrtc Modified:
4 years, 3 months ago CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionNew method TimestampAligner::TranslateTimestamp
Also enforce a minimum inter-frame interval of 1 ms,
fix a bug in the clipping logic, and improve comments.
BUG=webrtc:5740
Committed: https://crrev.com/a075848ebddd90956187935e01d792043dce6ce4
Cr-Commit-Position: refs/heads/master@{#14206}
Patch Set 1 #
Total comments: 6
Patch Set 2 : Make translated timestamps monotonous over reset. #
Total comments: 11
Patch Set 3 : Demote UpdateOffset and ClipTimestamp to protected. Added ClipToMonotonous test. #
Total comments: 9
Patch Set 4 : Log (unlikely) duplicate timestamps. Improve comments. #
Total comments: 9
Patch Set 5 : Minimum inter-frame interval 1ms. Fix clipping logic. #
Messages
Total messages: 22 (6 generated)
nisse@webrtc.org changed reviewers: + noahric@chromium.org, perkj@webrtc.org
Please have a look. Should eliminate duplicate timestamps.
https://codereview.webrtc.org/2325563002/diff/1/webrtc/base/timestampaligner.cc File webrtc/base/timestampaligner.cc (right): https://codereview.webrtc.org/2325563002/diff/1/webrtc/base/timestampaligner.... webrtc/base/timestampaligner.cc:96: // timestamp, time_us, equal to system_time_us. Which also ensures nit: |time_us| https://codereview.webrtc.org/2325563002/diff/1/webrtc/base/timestampaligner.h File webrtc/base/timestampaligner.h (right): https://codereview.webrtc.org/2325563002/diff/1/webrtc/base/timestampaligner.... webrtc/base/timestampaligner.h:22: class TimestampAligner { Can you add a comment about how to use this. And maybe- make the UpdateOffset and ClipTimestamp protected for testing and create new public helper method that give you the timestamp that should be used by a frame. https://codereview.webrtc.org/2325563002/diff/1/webrtc/base/timestampaligner.... webrtc/base/timestampaligner.h:31: // Clip timestamp, it should larger than the previous timestamp but please clarify this sentence. change should to to will be?
https://codereview.webrtc.org/2325563002/diff/1/webrtc/base/timestampaligner.cc File webrtc/base/timestampaligner.cc (right): https://codereview.webrtc.org/2325563002/diff/1/webrtc/base/timestampaligner.... webrtc/base/timestampaligner.cc:98: RTC_DCHECK_EQ(time_us, system_time_us); On second look, it appears this expectation is not correct. AdaptFrame, both in cricket::VideoCapturer and AndroidVideoTrackSource, calls UpdateOffset but not ClipTimestamp, in the case that frame_wanted is false. Which I think makes sense. So back to the drawing board, in particular, it's no longer correct to clear prev_translated_time at filter reset. https://codereview.webrtc.org/2325563002/diff/1/webrtc/base/timestampaligner.h File webrtc/base/timestampaligner.h (right): https://codereview.webrtc.org/2325563002/diff/1/webrtc/base/timestampaligner.... webrtc/base/timestampaligner.h:22: class TimestampAligner { On 2016/09/08 07:25:23, perkj_webrtc wrote: > Can you add a comment about how to use this. > > And maybe- make the UpdateOffset and ClipTimestamp protected for testing and > create new public helper method that give you the timestamp that should be used > by a frame. Regardless of my other comment, I agree it makes sense with an all-in-one function, which could be called TranslateTimestamp. https://codereview.webrtc.org/2325563002/diff/1/webrtc/base/timestampaligner.... webrtc/base/timestampaligner.h:31: // Clip timestamp, it should larger than the previous timestamp but On 2016/09/08 07:25:23, perkj_webrtc wrote: > please clarify this sentence. > > change should to to will be? Done.
Timestamps should now be monotonous also over reset. Also: Added a more user-friendly helper method TranslateTimestamp. Dropped the useless test fixture from the unittests.
https://codereview.webrtc.org/2325563002/diff/20001/webrtc/base/timestampalig... File webrtc/base/timestampaligner.cc (right): https://codereview.webrtc.org/2325563002/diff/20001/webrtc/base/timestampalig... webrtc/base/timestampaligner.cc:92: clip_bias_us_ = 0; can this change be tested? https://codereview.webrtc.org/2325563002/diff/20001/webrtc/base/timestampalig... File webrtc/base/timestampaligner.h (right): https://codereview.webrtc.org/2325563002/diff/20001/webrtc/base/timestampalig... webrtc/base/timestampaligner.h:21: class TimestampAligner { Add comment about the purpose of this class please. https://codereview.webrtc.org/2325563002/diff/20001/webrtc/base/timestampalig... webrtc/base/timestampaligner.h:27: // Recommended function for code outside of webrtc. |camera_time_us| Why only outside webrtc and what does that mean? Why not always use this? https://codereview.webrtc.org/2325563002/diff/20001/webrtc/base/timestampalig... webrtc/base/timestampaligner.h:31: // frame is received. It may have poor accuracy due to poor /s frame is received -> frame is captured https://codereview.webrtc.org/2325563002/diff/20001/webrtc/base/timestampalig... webrtc/base/timestampaligner.h:37: int64_t UpdateOffset(int64_t camera_time_us, int64_t system_time_us); make private ? https://codereview.webrtc.org/2325563002/diff/20001/webrtc/base/timestampalig... webrtc/base/timestampaligner.h:41: int64_t ClipTimestamp(int64_t filtered_time_us, int64_t system_time_us); make private ?
https://codereview.webrtc.org/2325563002/diff/20001/webrtc/base/timestampalig... File webrtc/base/timestampaligner.cc (right): https://codereview.webrtc.org/2325563002/diff/20001/webrtc/base/timestampalig... webrtc/base/timestampaligner.cc:92: clip_bias_us_ = 0; On 2016/09/08 13:41:05, perkj_webrtc wrote: > can this change be tested? I'm adding a test for the clipping logic, but it doesn't check this change in particular. At least, I think the new logic to initialize prev_translated_time_us_ to the minimum value and never reset it, is clearer than the old version. https://codereview.webrtc.org/2325563002/diff/20001/webrtc/base/timestampalig... File webrtc/base/timestampaligner.h (right): https://codereview.webrtc.org/2325563002/diff/20001/webrtc/base/timestampalig... webrtc/base/timestampaligner.h:21: class TimestampAligner { On 2016/09/08 13:41:05, perkj_webrtc wrote: > Add comment about the purpose of this class please. Done. https://codereview.webrtc.org/2325563002/diff/20001/webrtc/base/timestampalig... webrtc/base/timestampaligner.h:31: // frame is received. It may have poor accuracy due to poor On 2016/09/08 13:41:05, perkj_webrtc wrote: > /s frame is received -> frame is captured Done. https://codereview.webrtc.org/2325563002/diff/20001/webrtc/base/timestampalig... webrtc/base/timestampaligner.h:37: int64_t UpdateOffset(int64_t camera_time_us, int64_t system_time_us); On 2016/09/08 13:41:05, perkj_webrtc wrote: > make private ? Done. (Or rather protected, to make it available for the unit tests). https://codereview.webrtc.org/2325563002/diff/20001/webrtc/base/timestampalig... webrtc/base/timestampaligner.h:41: int64_t ClipTimestamp(int64_t filtered_time_us, int64_t system_time_us); On 2016/09/08 13:41:05, perkj_webrtc wrote: > make private ? Same here.
Very cool :) I especially like the new API, reads much clearer to me. https://codereview.webrtc.org/2325563002/diff/40001/webrtc/base/timestampalig... File webrtc/base/timestampaligner.cc (right): https://codereview.webrtc.org/2325563002/diff/40001/webrtc/base/timestampalig... webrtc/base/timestampaligner.cc:81: // otherwise very rare. The threshold of 300 ms should make this I'd remove the "otherwise very rare"; I wouldn't expect it to happen if the underlying capturer never changes, but capturer changes oftentimes result in clock source changes. https://codereview.webrtc.org/2325563002/diff/40001/webrtc/base/timestampalig... webrtc/base/timestampaligner.cc:106: time_us = prev_translated_time_us_ + 1; I'd bump it 1ms, just to make sure it's monotonic at lower accuracy. https://codereview.webrtc.org/2325563002/diff/40001/webrtc/base/timestampalig... webrtc/base/timestampaligner.cc:112: // timestamp several times, violating strict monotonicity. Since we can detect that, consider logging when it happens (if time_us <= prev_translated_time_us after this block runs). https://codereview.webrtc.org/2325563002/diff/40001/webrtc/base/timestampalig... File webrtc/base/timestampaligner.h (right): https://codereview.webrtc.org/2325563002/diff/40001/webrtc/base/timestampalig... webrtc/base/timestampaligner.h:59: // State for the ClipTimestamp methos, applied after the filter. "methods"
https://codereview.webrtc.org/2325563002/diff/40001/webrtc/base/timestampalig... File webrtc/base/timestampaligner.cc (right): https://codereview.webrtc.org/2325563002/diff/40001/webrtc/base/timestampalig... webrtc/base/timestampaligner.cc:81: // otherwise very rare. The threshold of 300 ms should make this On 2016/09/09 23:37:55, noahric wrote: > I'd remove the "otherwise very rare"; I wouldn't expect it to happen if the > underlying capturer never changes, but capturer changes oftentimes result in > clock source changes. Done. https://codereview.webrtc.org/2325563002/diff/40001/webrtc/base/timestampalig... webrtc/base/timestampaligner.cc:106: time_us = prev_translated_time_us_ + 1; On 2016/09/09 23:37:55, noahric wrote: > I'd bump it 1ms, just to make sure it's monotonic at lower accuracy. This makes sense at first. But should we then also change the test to if (time_us < prev_translated_time_us_ + rtc::kNumMicrosecsPerMillisec)? So I'm thinking that after all, it may be better to leave to downstream code which require larger intervals (say, e.g, unique rtp timestamps) do whatever they need? Relying on larger intervals here seems a bit brittle. What do you think Per, is it a good idea to increase the minimum frame interval here, from 1 us to some larger constant? My thinking about the current +1 code was not to do some general sanity check on inter-frame timing, but to make things simpler for code which wants to use the timestamp as an identifier, and therefore need to reject frames with duplicate timestamps. https://codereview.webrtc.org/2325563002/diff/40001/webrtc/base/timestampalig... webrtc/base/timestampaligner.cc:112: // timestamp several times, violating strict monotonicity. On 2016/09/09 23:37:55, noahric wrote: > Since we can detect that, consider logging when it happens (if time_us <= > prev_translated_time_us after this block runs). Done. Added a DCHECK_GE to check that time_us >= prev_translated_us_, and then another check and log message for equality. https://codereview.webrtc.org/2325563002/diff/40001/webrtc/base/timestampalig... File webrtc/base/timestampaligner.h (right): https://codereview.webrtc.org/2325563002/diff/40001/webrtc/base/timestampalig... webrtc/base/timestampaligner.h:59: // State for the ClipTimestamp methos, applied after the filter. On 2016/09/09 23:37:55, noahric wrote: > "methods" Corrected to "method" (singular).
lgtm https://codereview.webrtc.org/2325563002/diff/40001/webrtc/base/timestampalig... File webrtc/base/timestampaligner.cc (right): https://codereview.webrtc.org/2325563002/diff/40001/webrtc/base/timestampalig... webrtc/base/timestampaligner.cc:106: time_us = prev_translated_time_us_ + 1; On 2016/09/12 06:58:22, nisse-webrtc wrote: > On 2016/09/09 23:37:55, noahric wrote: > > I'd bump it 1ms, just to make sure it's monotonic at lower accuracy. > > This makes sense at first. > > But should we then also change the test to > if (time_us < prev_translated_time_us_ + rtc::kNumMicrosecsPerMillisec)? > > So I'm thinking that after all, it may be better to leave to downstream code > which require larger intervals (say, e.g, unique rtp timestamps) do whatever > they need? Relying on larger intervals here seems a bit brittle. > > What do you think Per, is it a good idea to increase the minimum frame interval > here, from 1 us to some larger constant? > > My thinking about the current +1 code was not to do some general sanity check on > inter-frame timing, but to make things simpler for code which wants to use the > timestamp as an identifier, and therefore need to reject frames with duplicate > timestamps. We regardless need to drop frames with old ts in lower layers since each capturer runs its own TimeStampAligner. But I don't see why we can not have the minimun diff be 1ms regardless. https://codereview.webrtc.org/2325563002/diff/60001/webrtc/api/androidvideotr... File webrtc/api/androidvideotracksource.cc (right): https://codereview.webrtc.org/2325563002/diff/60001/webrtc/api/androidvideotr... webrtc/api/androidvideotracksource.cc:248: width, height, camera_time_us * rtc::kNumNanosecsPerMicrosec, use translated_camera_time_us here? https://codereview.webrtc.org/2325563002/diff/60001/webrtc/base/timestampalig... File webrtc/base/timestampaligner.h (right): https://codereview.webrtc.org/2325563002/diff/60001/webrtc/base/timestampalig... webrtc/base/timestampaligner.h:23: // Frame timestamps in webrtc should rtc::TimeMicros (system monotonic should use https://codereview.webrtc.org/2325563002/diff/60001/webrtc/base/timestampalig... webrtc/base/timestampaligner.h:50: // timestamp but no larger than |system_time_us|. nit: Can you refrase this to say that the return value is min(previouse_ts + the offset, system_time_us) https://codereview.webrtc.org/2325563002/diff/60001/webrtc/base/timestampalig... webrtc/base/timestampaligner.h:60: nit: remove empty line. https://codereview.webrtc.org/2325563002/diff/60001/webrtc/media/base/videoca... File webrtc/media/base/videocapturer.cc (right): https://codereview.webrtc.org/2325563002/diff/60001/webrtc/media/base/videoca... webrtc/media/base/videocapturer.cc:238: width, height, camera_time_us * rtc::kNumNanosecsPerMicrosec, use translated time?
Description was changed from ========== Make TimestampAligner::ClipTimestamp produce strictly monotonous timestamps. Also clarify a few comments. BUG=webrtc:5740 ========== to ========== New method TimestampAligner::TranslateTimestamp Also enforce a minimum inter-frame interval of 1 ms, fix a bug in the clipping logic, and improve comments. BUG=webrtc:5740 ==========
New version. Also found and fixed a bug in the clipping logic, have to apply the clip bias before checking for monotonicity. https://codereview.webrtc.org/2325563002/diff/60001/webrtc/api/androidvideotr... File webrtc/api/androidvideotracksource.cc (right): https://codereview.webrtc.org/2325563002/diff/60001/webrtc/api/androidvideotr... webrtc/api/androidvideotracksource.cc:248: width, height, camera_time_us * rtc::kNumNanosecsPerMicrosec, On 2016/09/12 09:33:46, perkj_webrtc wrote: > use translated_camera_time_us here? No, camera_time:us is supposedly of better accuracy. And we also allow translated_camera_time_us to be NULL (for the OnFrameCaptured path, which hasn't been deleted yet). https://codereview.webrtc.org/2325563002/diff/60001/webrtc/base/timestampalig... File webrtc/base/timestampaligner.h (right): https://codereview.webrtc.org/2325563002/diff/60001/webrtc/base/timestampalig... webrtc/base/timestampaligner.h:23: // Frame timestamps in webrtc should rtc::TimeMicros (system monotonic On 2016/09/12 09:33:46, perkj_webrtc wrote: > should use Done. https://codereview.webrtc.org/2325563002/diff/60001/webrtc/base/timestampalig... webrtc/base/timestampaligner.h:50: // timestamp but no larger than |system_time_us|. On 2016/09/12 09:33:46, perkj_webrtc wrote: > nit: Can you refrase this to say that the return value is min(previouse_ts + the > offset, system_time_us) It's a bit more complicated than that, but I've given it another try. https://codereview.webrtc.org/2325563002/diff/60001/webrtc/base/timestampalig... webrtc/base/timestampaligner.h:60: On 2016/09/12 09:33:46, perkj_webrtc wrote: > nit: remove empty line. Done. I was thinking of that line as a heading for the next two members. Don't know if there are any formatting conventions here, but I think it's clear enough without the empty line.
Noah: Do you have any additional comments? Per: You may want to have another look at the new clip logic. The problem in the previous patchset was that we first made timestamps monotonic, and then subtracted the clip_bias_us, possibly losing monotonicity.
On 2016/09/13 13:13:57, nisse-webrtc wrote: > Noah: Do you have any additional comments? > > Per: You may want to have another look at the new clip logic. The problem in the > previous patchset was that we first made timestamps monotonic, and then > subtracted the clip_bias_us, possibly losing monotonicity. Nope, I'm all good :)
The CQ bit was checked by nisse@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from perkj@webrtc.org Link to the patchset: https://codereview.webrtc.org/2325563002/#ps80001 (title: "Minimum inter-frame interval 1ms. Fix clipping logic.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
still lgtm
Message was sent while issue was closed.
Description was changed from ========== New method TimestampAligner::TranslateTimestamp Also enforce a minimum inter-frame interval of 1 ms, fix a bug in the clipping logic, and improve comments. BUG=webrtc:5740 ========== to ========== New method TimestampAligner::TranslateTimestamp Also enforce a minimum inter-frame interval of 1 ms, fix a bug in the clipping logic, and improve comments. BUG=webrtc:5740 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== New method TimestampAligner::TranslateTimestamp Also enforce a minimum inter-frame interval of 1 ms, fix a bug in the clipping logic, and improve comments. BUG=webrtc:5740 ========== to ========== New method TimestampAligner::TranslateTimestamp Also enforce a minimum inter-frame interval of 1 ms, fix a bug in the clipping logic, and improve comments. BUG=webrtc:5740 Committed: https://crrev.com/a075848ebddd90956187935e01d792043dce6ce4 Cr-Commit-Position: refs/heads/master@{#14206} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/a075848ebddd90956187935e01d792043dce6ce4 Cr-Commit-Position: refs/heads/master@{#14206} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
