Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2813593003:
Add TimestampUnwrapper and generalize the code (Closed)
|
Created:
3 years, 8 months ago by hlundin-webrtc Modified:
3 years, 8 months ago Reviewers:
kwiberg-webrtc CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com Target Ref:
refs/heads/master Project:
webrtc Visibility:
Public. |
DescriptionAdd TimestampUnwrapper and generalize the code
BUG=webrtc:7467
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_chromium_rel_ng;master.tryserver.chromium.mac:mac_chromium_rel_ng,ios-device;master.tryserver.chromium.win:win_chromium_rel_ng;master.tryserver.chromium.android:android_compile_dbg,linux_android_rel_ng
Review-Url: https://codereview.webrtc.org/2813593003
Cr-Commit-Position: refs/heads/master@{#17833}
Committed: https://chromium.googlesource.com/external/webrtc/+/06863c9ce5ae397ec8eb6eed1ccf2bd315469bf0
Patch Set 1 #
Total comments: 26
Patch Set 2 : Update after first review #
Total comments: 4
Patch Set 3 : Remove ctor #
Messages
Total messages: 17 (8 generated)
henrik.lundin@webrtc.org changed reviewers: + kwiberg@webrtc.org
kwiberg@, ptal.
https://codereview.webrtc.org/2813593003/diff/1/webrtc/modules/include/module... File webrtc/modules/include/module_common_types.h (right): https://codereview.webrtc.org/2813593003/diff/1/webrtc/modules/include/module... webrtc/modules/include/module_common_types.h:487: if (static_cast<U>(value - prev_value) == kBreakpoint) { This static_cast is unnecessary, right? https://codereview.webrtc.org/2813593003/diff/1/webrtc/modules/include/module... webrtc/modules/include/module_common_types.h:491: static_cast<U>(value - prev_value) < kBreakpoint; And this one? https://codereview.webrtc.org/2813593003/diff/1/webrtc/modules/include/module... webrtc/modules/include/module_common_types.h:492: } Why do you need IsNewer(t1,t2)=true when t1>t2 and |t1-t2| = kBreakpoint. This function would be *much* simpler otherwise: return value - prev_value < prev_value - value; https://codereview.webrtc.org/2813593003/diff/1/webrtc/modules/include/module... webrtc/modules/include/module_common_types.h:520: static_assert(!std::numeric_limits<U>::is_signed, "U must be unsigned"); Move the static_assert to just after line 517? https://codereview.webrtc.org/2813593003/diff/1/webrtc/modules/include/module... webrtc/modules/include/module_common_types.h:526: return value; This suggests that last_value_ should be Optional. https://codereview.webrtc.org/2813593003/diff/1/webrtc/modules/include/module... webrtc/modules/include/module_common_types.h:529: static_cast<int64_t>(std::numeric_limits<U>::max()) + 1; static_assert(safe_cmp::Lt(std::numeric_limits<U>::max(), std::numeric_limits<int64_t>::max()), ""); Or maybe just limit U to 32 bits---the rest of the arithmetic seems to assume that last_value_ will never overflow. https://codereview.webrtc.org/2813593003/diff/1/webrtc/modules/include/module... webrtc/modules/include/module_common_types.h:540: } Hmm. The delta you're trying to compute, is it value - cropped_last + k * kMaxPlusOne with k picked from {-1, 0, 1} so that abs(delta) is as small as possible and last_value_ + delta >= 0? https://codereview.webrtc.org/2813593003/diff/1/webrtc/modules/include/module... webrtc/modules/include/module_common_types.h:556: int64_t last_value_; Initialize to -1 here, and get rid of the constructor? Or rather, make this an Optional<int64_t> and you won't have to initialize... https://codereview.webrtc.org/2813593003/diff/1/webrtc/modules/module_common_... File webrtc/modules/module_common_types_unittest.cc (right): https://codereview.webrtc.org/2813593003/diff/1/webrtc/modules/module_common_... webrtc/modules/module_common_types_unittest.cc:182: EXPECT_EQ(0x0, unwrapper.Unwrap(0x0000)); 0x0 and 0x0000 are both colloquially known as "0". https://codereview.webrtc.org/2813593003/diff/1/webrtc/modules/module_common_... webrtc/modules/module_common_types_unittest.cc:199: const int64_t kMaxIncrease = 0x80000000 - 1; Shouldn't either this one or kMinIncrease be 0x80000000? https://codereview.webrtc.org/2813593003/diff/1/webrtc/modules/module_common_... webrtc/modules/module_common_types_unittest.cc:216: for (int i = kNumWraps * 2; i >= 0; --i) { You don't use i in the loop body, so you don't have to count backwards.
Thanks! Updated and uploaded. https://codereview.webrtc.org/2813593003/diff/1/webrtc/modules/include/module... File webrtc/modules/include/module_common_types.h (right): https://codereview.webrtc.org/2813593003/diff/1/webrtc/modules/include/module... webrtc/modules/include/module_common_types.h:487: if (static_cast<U>(value - prev_value) == kBreakpoint) { On 2017/04/11 11:57:34, kwiberg-webrtc wrote: > This static_cast is unnecessary, right? Done. https://codereview.webrtc.org/2813593003/diff/1/webrtc/modules/include/module... webrtc/modules/include/module_common_types.h:491: static_cast<U>(value - prev_value) < kBreakpoint; On 2017/04/11 11:57:34, kwiberg-webrtc wrote: > And this one? No, it is needed. If value is small, and prev_value is large, you will get a negative value. The cast gives us the positive distance between the two "across the wrap-around" instead of the negative value. https://codereview.webrtc.org/2813593003/diff/1/webrtc/modules/include/module... webrtc/modules/include/module_common_types.h:492: } On 2017/04/11 11:57:34, kwiberg-webrtc wrote: > Why do you need IsNewer(t1,t2)=true when t1>t2 and |t1-t2| = kBreakpoint. This > function would be *much* simpler otherwise: > > return value - prev_value < prev_value - value; I'm simply maintaining legacy here. https://codereview.webrtc.org/2813593003/diff/1/webrtc/modules/include/module... webrtc/modules/include/module_common_types.h:520: static_assert(!std::numeric_limits<U>::is_signed, "U must be unsigned"); On 2017/04/11 11:57:34, kwiberg-webrtc wrote: > Move the static_assert to just after line 517? Done. https://codereview.webrtc.org/2813593003/diff/1/webrtc/modules/include/module... webrtc/modules/include/module_common_types.h:526: return value; On 2017/04/11 11:57:34, kwiberg-webrtc wrote: > This suggests that last_value_ should be Optional. Done. https://codereview.webrtc.org/2813593003/diff/1/webrtc/modules/include/module... webrtc/modules/include/module_common_types.h:529: static_cast<int64_t>(std::numeric_limits<U>::max()) + 1; On 2017/04/11 11:57:34, kwiberg-webrtc wrote: > static_assert(safe_cmp::Lt(std::numeric_limits<U>::max(), > std::numeric_limits<int64_t>::max()), ""); > > Or maybe just limit U to 32 bits---the rest of the arithmetic seems to assume > that last_value_ will never overflow. I went for limiting the number of bits. https://codereview.webrtc.org/2813593003/diff/1/webrtc/modules/include/module... webrtc/modules/include/module_common_types.h:540: } On 2017/04/11 11:57:34, kwiberg-webrtc wrote: > Hmm. The delta you're trying to compute, is it > > value - cropped_last + k * kMaxPlusOne > > with k picked from {-1, 0, 1} so that abs(delta) is as small as possible and > last_value_ + delta >= 0? Eeeeh, possibly. I'm just holding on to the legacy while trying to generalize it to avoid code duplication. https://codereview.webrtc.org/2813593003/diff/1/webrtc/modules/include/module... webrtc/modules/include/module_common_types.h:556: int64_t last_value_; On 2017/04/11 11:57:34, kwiberg-webrtc wrote: > Initialize to -1 here, and get rid of the constructor? Or rather, make this an > Optional<int64_t> and you won't have to initialize... Done. https://codereview.webrtc.org/2813593003/diff/1/webrtc/modules/module_common_... File webrtc/modules/module_common_types_unittest.cc (right): https://codereview.webrtc.org/2813593003/diff/1/webrtc/modules/module_common_... webrtc/modules/module_common_types_unittest.cc:182: EXPECT_EQ(0x0, unwrapper.Unwrap(0x0000)); On 2017/04/11 11:57:34, kwiberg-webrtc wrote: > 0x0 and 0x0000 are both colloquially known as "0". That's not how we speak back home... https://codereview.webrtc.org/2813593003/diff/1/webrtc/modules/module_common_... webrtc/modules/module_common_types_unittest.cc:199: const int64_t kMaxIncrease = 0x80000000 - 1; On 2017/04/11 11:57:34, kwiberg-webrtc wrote: > Shouldn't either this one or kMinIncrease be 0x80000000? No, if you make a step that is 0x80000000 either way, it will be interpreted as back-tracking. https://codereview.webrtc.org/2813593003/diff/1/webrtc/modules/module_common_... webrtc/modules/module_common_types_unittest.cc:216: for (int i = kNumWraps * 2; i >= 0; --i) { On 2017/04/11 11:57:34, kwiberg-webrtc wrote: > You don't use i in the loop body, so you don't have to count backwards. Done. https://codereview.webrtc.org/2813593003/diff/20001/webrtc/modules/include/mo... File webrtc/modules/include/module_common_types.h (right): https://codereview.webrtc.org/2813593003/diff/20001/webrtc/modules/include/mo... webrtc/modules/include/module_common_types.h:26: #include "webrtc/modules/video_coding/codecs/h264/include/h264_globals.h" git cl format did this
lgtm https://codereview.webrtc.org/2813593003/diff/1/webrtc/modules/include/module... File webrtc/modules/include/module_common_types.h (right): https://codereview.webrtc.org/2813593003/diff/1/webrtc/modules/include/module... webrtc/modules/include/module_common_types.h:491: static_cast<U>(value - prev_value) < kBreakpoint; On 2017/04/11 14:18:08, hlundin-webrtc wrote: > On 2017/04/11 11:57:34, kwiberg-webrtc wrote: > > And this one? > > No, it is needed. If value is small, and prev_value is large, you will get a > negative value. The cast gives us the positive distance between the two "across > the wrap-around" instead of the negative value. Aha. If U is smaller than int, that subtraction will be done with ints. Got it. https://codereview.webrtc.org/2813593003/diff/1/webrtc/modules/include/module... webrtc/modules/include/module_common_types.h:492: } On 2017/04/11 14:18:08, hlundin-webrtc wrote: > On 2017/04/11 11:57:34, kwiberg-webrtc wrote: > > Why do you need IsNewer(t1,t2)=true when t1>t2 and |t1-t2| = kBreakpoint. This > > function would be *much* simpler otherwise: > > > > return value - prev_value < prev_value - value; > > I'm simply maintaining legacy here. Acknowledged. https://codereview.webrtc.org/2813593003/diff/1/webrtc/modules/include/module... webrtc/modules/include/module_common_types.h:540: } On 2017/04/11 14:18:08, hlundin-webrtc wrote: > On 2017/04/11 11:57:34, kwiberg-webrtc wrote: > > Hmm. The delta you're trying to compute, is it > > > > value - cropped_last + k * kMaxPlusOne > > > > with k picked from {-1, 0, 1} so that abs(delta) is as small as possible and > > last_value_ + delta >= 0? > > Eeeeh, possibly. I'm just holding on to the legacy while trying to generalize it > to avoid code duplication. Acknowledged. https://codereview.webrtc.org/2813593003/diff/1/webrtc/modules/module_common_... File webrtc/modules/module_common_types_unittest.cc (right): https://codereview.webrtc.org/2813593003/diff/1/webrtc/modules/module_common_... webrtc/modules/module_common_types_unittest.cc:199: const int64_t kMaxIncrease = 0x80000000 - 1; On 2017/04/11 14:18:08, hlundin-webrtc wrote: > On 2017/04/11 11:57:34, kwiberg-webrtc wrote: > > Shouldn't either this one or kMinIncrease be 0x80000000? > > No, if you make a step that is 0x80000000 either way, it will be interpreted as > back-tracking. Acknowledged. https://codereview.webrtc.org/2813593003/diff/20001/webrtc/modules/include/mo... File webrtc/modules/include/module_common_types.h (right): https://codereview.webrtc.org/2813593003/diff/20001/webrtc/modules/include/mo... webrtc/modules/include/module_common_types.h:26: #include "webrtc/modules/video_coding/codecs/h264/include/h264_globals.h" On 2017/04/11 14:18:08, hlundin-webrtc wrote: > git cl format did this Yes, I've noticed that it's started sorting #includes. It causes some noise in the diffs, but it'll die down eventually. https://codereview.webrtc.org/2813593003/diff/20001/webrtc/modules/include/mo... webrtc/modules/include/module_common_types.h:525: Unwrapper() = default; You don't need this line.
Description was changed from ========== Add TimestampUnwrapper and generalize the code BUG=webrtc:7467 ========== to ========== Add TimestampUnwrapper and generalize the code BUG=webrtc:7467 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_chromium_rel_ng;master.tryserver.chromium.mac:mac_chromium_rel_ng,ios-device;master.tryserver.chromium.win:win_chromium_rel_ng;master.tryserver.chromium.android:android_compile_dbg,linux_android_rel_ng ==========
Thanks! https://codereview.webrtc.org/2813593003/diff/20001/webrtc/modules/include/mo... File webrtc/modules/include/module_common_types.h (right): https://codereview.webrtc.org/2813593003/diff/20001/webrtc/modules/include/mo... webrtc/modules/include/module_common_types.h:525: Unwrapper() = default; On 2017/04/12 08:39:14, kwiberg-webrtc wrote: > You don't need this line. Done.
The CQ bit was checked by henrik.lundin@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from kwiberg@webrtc.org Link to the patchset: https://codereview.webrtc.org/2813593003/#ps40001 (title: "Remove ctor")
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: ios32_sim_ios9_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios32_sim_ios9_dbg/buil...)
The CQ bit was checked by henrik.lundin@webrtc.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1493015571299410,
"parent_rev": "25ab4b2a58e4c49bb7856275fa4dbec9693e2db3", "commit_rev":
"06863c9ce5ae397ec8eb6eed1ccf2bd315469bf0"}
Message was sent while issue was closed.
Description was changed from ========== Add TimestampUnwrapper and generalize the code BUG=webrtc:7467 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_chromium_rel_ng;master.tryserver.chromium.mac:mac_chromium_rel_ng,ios-device;master.tryserver.chromium.win:win_chromium_rel_ng;master.tryserver.chromium.android:android_compile_dbg,linux_android_rel_ng ========== to ========== Add TimestampUnwrapper and generalize the code BUG=webrtc:7467 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_chromium_rel_ng;master.tryserver.chromium.mac:mac_chromium_rel_ng,ios-device;master.tryserver.chromium.win:win_chromium_rel_ng;master.tryserver.chromium.android:android_compile_dbg,linux_android_rel_ng Review-Url: https://codereview.webrtc.org/2813593003 Cr-Commit-Position: refs/heads/master@{#17833} Committed: https://chromium.googlesource.com/external/webrtc/+/06863c9ce5ae397ec8eb6eed1... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/external/webrtc/+/06863c9ce5ae397ec8eb6eed1... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
