From 50c602ec3f90ba11273b52117d650e66b0b1505e Mon Sep 17 00:00:00 2001 From: Ichthyostega Date: Mon, 5 Dec 2022 00:58:32 +0100 Subject: [PATCH] Library: rectify clipping of time::Duration (see #1263) This is a deep refactoring to allow to represent the distance between all valid time points as a time::Offset or time::Duration. By design this is possible, since Time::MAX was defined as 1/30 of the maximum value technically representable as int64_t. However, introducing a different limiter for offsets and durations turns out difficult, due to the inconsistencies in the exiting hierarchy of temporal entities. Which in turn seems to stem from the unfortunate decision to make time entities immutable, see #1261 Since the limiter is hard wired into the `time::TimeValue` constructor, we are forced to create a "backdoor" of sorts, to pass up values with different limiting from child classes. This would not be so much of a problem if calculations weren't forced to go through `TimeVar`, which does not distinguish between time points and time durations. This solution rearranges all checks to be performed now by time::Offset, while time::Duration will only take the absolute value at construction, based on the fact that there is no valid construction path to yield a duration which does not go through an offset first. Later, when we're ready to sort out the implementation base of time values (see #1258), this design issue should be revisited - either we'll allow derived classes explicitly to invoke the limiter functions - or we may be able to have an automatic conversion path from clearly marked base implementation types, in which case we wouldn't use the buildRaw_() and _raw() "backdoor" functions any more... --- src/lib/rational.hpp | 4 +- src/lib/time/time.cpp | 25 +- src/lib/time/timevalue.hpp | 110 +++++--- src/stage/model/zoom-window.hpp | 2 + src/vault/real-clock.cpp | 2 +- tests/basics/time/quantiser-basics-test.cpp | 44 ++- tests/basics/time/time-value-test.cpp | 5 +- tests/stage/model/zoom-window-test.cpp | 31 ++- wiki/thinkPad.ichthyo.mm | 280 ++++++++++++++++++++ 9 files changed, 450 insertions(+), 53 deletions(-) diff --git a/src/lib/rational.hpp b/src/lib/rational.hpp index f1bd84851..863cdbc8d 100644 --- a/src/lib/rational.hpp +++ b/src/lib/rational.hpp @@ -184,7 +184,9 @@ namespace util { // construct approximation quantised to 1/u f128 frac = f128(r) / den; int64_t res = d*u + int64_t(frac*u * ROUND_ULP); - ENSURE (abs (f128(res)/u - rational_cast(Rat{num,den})) <= 1.0/abs(u)); + ENSURE (abs (f128(res)/u - rational_cast(Rat{num,den})) <= 1.0/abs(u) + ,"Requantisation error exceeded num=%lu / den=%lu -> res=%lu / quant=%lu" + , num, den, res, u); return res; } diff --git a/src/lib/time/time.cpp b/src/lib/time/time.cpp index cb38b3a2a..384c76fa1 100644 --- a/src/lib/time/time.cpp +++ b/src/lib/time/time.cpp @@ -130,6 +130,11 @@ namespace time { : TimeValue(lumiera_rational_to_time (fractionalSeconds)) { } + Offset::Offset (FSecs const& delta_in_secs) + : TimeValue{buildRaw_(symmetricLimit (lumiera_rational_to_time (delta_in_secs) + ,Duration::MAX))} + { } + /** @note recommendation is to use TCode for external representation * @remarks this is the most prevalent internal diagnostics display @@ -266,29 +271,29 @@ namespace time { boost::rational distance (this->t_); distance *= factor; gavl_time_t microTicks = floordiv (distance.numerator(), distance.denominator()); - return Offset(TimeValue(microTicks)); + return Offset{buildRaw_(microTicks)}; } /** offset by the given number of frames. */ Offset::Offset (FrameCnt count, FrameRate const& fps) - : TimeValue (count? (count<0? -1:+1) * lumiera_framecount_to_time (::abs(count), fps) - : _raw(Duration::NIL)) + : TimeValue{buildRaw_( + count? (count<0? -1:+1) * lumiera_framecount_to_time (::abs(count), fps) + :_raw(Duration::NIL))} { } - /** duration of the given number of frames. - * @note always positive; count used absolute */ - Duration::Duration (FrameCnt count, FrameRate const& fps) - : TimeValue (count? lumiera_framecount_to_time (abs(count), fps) : _raw(Duration::NIL)) - { } /** constant to indicate "no duration" */ const Duration Duration::NIL {Time::ZERO}; /** maximum possible temporal extension */ - const Duration Duration::MAX {Offset{Time::MIN, Time::MAX}}; - + const Duration Duration::MAX = []{ + auto maxDelta {Time::MAX - Time::MIN}; + // bypass limit check, which requires Duration::MAX + return reinterpret_cast (maxDelta); + }(); + }} // namespace lib::Time diff --git a/src/lib/time/timevalue.hpp b/src/lib/time/timevalue.hpp index 85946fcfb..2fd570c52 100644 --- a/src/lib/time/timevalue.hpp +++ b/src/lib/time/timevalue.hpp @@ -122,6 +122,7 @@ namespace time { // forwards... class FrameRate; + class Duration; class TimeSpan; class Mutation; @@ -160,17 +161,24 @@ namespace time { /** some subclasses may receive modification messages */ friend class Mutation; + /** explicit limit of allowed time range */ + static gavl_time_t limitedTime (gavl_time_t raw); + /** safe calculation of explicitly limited time offset */ + static gavl_time_t limitedDelta (gavl_time_t origin, gavl_time_t target); + + /** @internal for Offset and Duration entities built on top */ + TimeValue (TimeValue const& origin, TimeValue const& target) + : t_{limitedDelta (origin.t_, target.t_)} + { } + public: /** Number of micro ticks (µs) per second as basic time scale */ static const gavl_time_t SCALE; - /** explicit limit of allowed time range */ - static gavl_time_t limited (gavl_time_t raw); - explicit - TimeValue (gavl_time_t val=0) ///< time given in µ ticks here - : t_(limited (val)) + TimeValue (gavl_time_t val) ///< time given in µ ticks here + : t_{limitedTime (val)} { } /** copy initialisation allowed */ @@ -234,7 +242,7 @@ namespace time { > > { public: - TimeVar (TimeValue const& time = TimeValue()) + TimeVar (TimeValue const& time = TimeValue(0)) : TimeValue(time) { } @@ -349,6 +357,8 @@ namespace time { * to build derived values, including * - the _absolute (positive) distance_ for this offset: #abs * - a combined offset by chaining another offset + * @note on construction, Offset values are checked and limited + * to be within [-Duration::MAX ... +Duration::MAX] */ class Offset : public TimeValue @@ -366,24 +376,21 @@ namespace time { public: explicit - Offset (TimeValue const& distance =Time::ZERO) - : TimeValue(distance) - { } + Offset (TimeValue const& distance =Time::ZERO); + + explicit + Offset (FSecs const& delta_in_secs); + + Offset (FrameCnt count, FrameRate const& fps); Offset (TimeValue const& origin, TimeValue const& target) - : TimeValue(TimeVar(target) -= origin) + : TimeValue{origin, target} { } - Offset (FrameCnt count, FrameRate const& fps); - static const Offset ZERO; - - TimeValue - abs() const - { - return TimeValue(std::llabs (t_)); - } + /** interpret the distance given by this offset as a time duration */ + Duration abs() const; /** @internal stretch offset by a possibly fractional factor, * and quantise into raw (micro tick) grid */ @@ -446,6 +453,7 @@ namespace time { * promoted from an offset. While Duration generally * is treated as immutable value, there is the * possibility to send a _Mutation message_. + * @note Duration relies on Offset being limited */ class Duration : public TimeValue @@ -455,29 +463,36 @@ namespace time { public: Duration() - : Duration(Time::ZERO) + : TimeValue{Time::ZERO} { } Duration (Offset const& distance) - : TimeValue(distance.abs()) + : TimeValue{buildRaw_(llabs (_raw(distance)))} { } explicit Duration (TimeValue const& timeSpec) - : TimeValue(Offset(timeSpec).abs()) + : Duration{Offset{timeSpec}} { } explicit Duration (FSecs const& timeSpan_in_secs) - : TimeValue(Offset(Time(timeSpan_in_secs)).abs()) + : Duration{Offset{timeSpan_in_secs}} + { } + + /** duration of the given number of frames. + * @note always positive; count used absolute */ + Duration (FrameCnt count, FrameRate const& fps) + : Duration{Offset{count,fps}} { } Duration (TimeSpan const& interval); - Duration (FrameCnt count, FrameRate const& fps); Duration (Duration const& o) - : Duration{Offset(o)} - { } + : TimeValue{o} + {// assuming that negative Duration can not be constructed.... + REQUIRE (t_ >= 0, "Copy rejected: negative Duration %lu", o.t_); + } static const Duration NIL; static const Duration MAX ; @@ -665,6 +680,14 @@ namespace time { , error::LERR_(BOTTOM_VALUE)); return n; } + + inline gavl_time_t + symmetricLimit (gavl_time_t raw, TimeValue lim) + { + return raw > lim? _raw(lim) + : -raw > lim? -_raw(lim) + : raw; + } }//(End) implementation helpers @@ -673,25 +696,41 @@ namespace time { * raw time value to keep it within the arbitrary * boundaries defined by (Time::MAX, Time::MIN). * While Time entities are \c not a "safeInt" - * implementation, we limit new values and - * establish this safety margin to prevent - * wrap-around during time quantisation */ + * implementation, we limit new values to + * lower the likelihood of wrap-around */ inline gavl_time_t - TimeValue::limited (gavl_time_t raw) + TimeValue::limitedTime (gavl_time_t raw) { - return raw > Time::MAX? Time::MAX.t_ - : raw < Time::MIN? Time::MIN.t_ - : raw; + return symmetricLimit (raw, Time::MAX); } + inline gavl_time_t + TimeValue::limitedDelta (gavl_time_t origin, gavl_time_t target) + { + if (0 > (origin^target)) + {// prevent possible numeric wrap + origin = symmetricLimit (origin, Duration::MAX); + target = symmetricLimit (target, Duration::MAX); + } + gavl_time_t res = target - origin; + return symmetricLimit (res, Duration::MAX); + } + + inline TimeVar::TimeVar (FSecs const& fractionalSeconds) : TimeVar{Time(fractionalSeconds)} { } + inline + Offset::Offset (TimeValue const& distance) + : TimeValue{buildRaw_(symmetricLimit(_raw(distance) + , Duration::MAX))} + { } + inline Duration::Duration (TimeSpan const& interval) - : TimeValue(interval.duration()) + : Duration{interval.duration()} { } inline @@ -715,6 +754,11 @@ namespace time { return boost::rational_cast (*this); } + inline Duration + Offset::abs() const + { + return Duration{*this}; + } diff --git a/src/stage/model/zoom-window.hpp b/src/stage/model/zoom-window.hpp index 5e0383491..650c1ce49 100644 --- a/src/stage/model/zoom-window.hpp +++ b/src/stage/model/zoom-window.hpp @@ -732,6 +732,8 @@ namespace model { REQUIRE (dur < MAX_TIMESPAN); REQUIRE (Time::MIN <= startWin_); REQUIRE (afterWin_ <= Time::MAX); + startAll_ = max (startAll_, Time::MIN); + afterAll_ = min (afterAll_, Time::MAX); if (dur <= _FSecs(afterAll_-startAll_)) {//possibly shift into current canvas if (afterWin_ > afterAll_) diff --git a/src/vault/real-clock.cpp b/src/vault/real-clock.cpp index cc627f298..bd84c1612 100644 --- a/src/vault/real-clock.cpp +++ b/src/vault/real-clock.cpp @@ -54,7 +54,7 @@ namespace vault { gavl_time_t ticksSince1970 = now.tv_sec * TimeValue::SCALE + now.tv_nsec / MICRO_TICS_PER_NANOSECOND; - ENSURE (ticksSince1970 == Time::limited (ticksSince1970)); + ENSURE (ticksSince1970 == _raw(TimeValue{ticksSince1970})); return TimeValue::buildRaw_(ticksSince1970); // bypassing the limit check } diff --git a/tests/basics/time/quantiser-basics-test.cpp b/tests/basics/time/quantiser-basics-test.cpp index b1eb73b35..cfe544735 100644 --- a/tests/basics/time/quantiser-basics-test.cpp +++ b/tests/basics/time/quantiser-basics-test.cpp @@ -183,7 +183,7 @@ namespace test{ CHECK (Time::MIN == case2.gridAlign( secs(-1) )); // resulting values will already exceed CHECK (Time::MIN == case2.gridAlign( secs(-2) )); // allowed range and thus will be clipped - // maximum frame size is half the time range + // use very large frame with size of half the time range Duration hugeFrame(Time::MAX); FixedFrameQuantiser case3 (hugeFrame); CHECK (Time::MIN == case3.gridAlign(Time::MIN )); @@ -205,9 +205,45 @@ namespace test{ CHECK (TimeValue(0) == case4.gridAlign(Time::MAX -TimeValue(1) )); CHECK (TimeValue(0) == case4.gridAlign(Time::MAX )); //.......still truncated down to frame #0 - // larger frames aren't possible - Duration not_really_larger(secs(10000) + hugeFrame); - CHECK (hugeFrame == not_really_larger); + // think big... + Duration superHuge{secs(12345) + hugeFrame}; + Duration extraHuge{2*hugeFrame}; + CHECK (extraHuge == Duration::MAX); + + // Time::MAX < superHuge < Duration::Max is possible, but we can accommodate only one + FixedFrameQuantiser case5 (superHuge); + CHECK (TimeValue(0) == case5.gridAlign(Time::MAX )); + CHECK (TimeValue(0) == case5.gridAlign(Time::MAX -TimeValue(1) )); + CHECK (TimeValue(0) == case5.gridAlign( secs( 1) )); + CHECK (TimeValue(0) == case5.gridAlign( secs( 0) )); + CHECK (Time::MIN == case5.gridAlign( secs(-1) )); + CHECK (Time::MIN == case5.gridAlign(Time::MIN +TimeValue(1) )); + CHECK (Time::MIN == case5.gridAlign(Time::MIN )); + + // now with offset + FixedFrameQuantiser case6 (superHuge, Time::MAX-secs(1)); + CHECK (TimeValue(0) == case6.gridAlign(Time::MAX )); + CHECK (TimeValue(0) == case6.gridAlign(Time::MAX -TimeValue(1) )); + CHECK (TimeValue(0) == case6.gridAlign(Time::MAX -secs(1) )); + CHECK (Time::MIN == case6.gridAlign(Time::MAX -secs(2) )); + CHECK (Time::MIN == case6.gridAlign( secs( 1) )); + CHECK (Time::MIN == case6.gridAlign( secs(-12345) )); + CHECK (Time::MIN == case6.gridAlign( secs(-12345-1) )); + CHECK (Time::MIN == case6.gridAlign( secs(-12345-2) )); // this would be one frame lower, but is clipped + CHECK (Time::MIN == case6.gridAlign(Time::MIN +TimeValue(1) )); + CHECK (Time::MIN == case6.gridAlign(Time::MIN )); // same... unable to represent time points before Time::MIN + + // maximum frame size is spanning the full time range + FixedFrameQuantiser case7 (extraHuge, Time::MIN+secs(1)); + CHECK (TimeValue(0) == case7.gridAlign(Time::MAX )); // rounded down one frame, i.e. to origin + CHECK (TimeValue(0) == case7.gridAlign( secs( 0) )); + CHECK (TimeValue(0) == case7.gridAlign(Time::MIN+secs(2) )); + CHECK (TimeValue(0) == case7.gridAlign(Time::MIN+secs(1) )); // exactly at origin + CHECK (Time::MIN == case7.gridAlign(Time::MIN )); // one frame further down, but clipped to Time::MIN + + // even larger frames aren't possible + Duration not_really_larger(secs(10000) + extraHuge); + CHECK (extraHuge == not_really_larger); // frame sizes below the time micro grid get trapped long subAtomic = 2*TimeValue::SCALE; // too small for this universe... diff --git a/tests/basics/time/time-value-test.cpp b/tests/basics/time/time-value-test.cpp index 5a563ef40..b0e44b87c 100644 --- a/tests/basics/time/time-value-test.cpp +++ b/tests/basics/time/time-value-test.cpp @@ -88,7 +88,7 @@ namespace test{ void checkBasicTimeValues (TimeValue org) { - TimeValue zero; + TimeValue zero(0); TimeValue one (1); TimeValue max (Time::MAX); TimeValue min (Time::MIN); @@ -248,7 +248,7 @@ namespace test{ void buildDuration (TimeValue org) { - TimeValue zero; + TimeValue zero(0); TimeVar point(org); point += TimeValue(5); CHECK (org < point); @@ -313,7 +313,6 @@ namespace test{ void buildTimeSpan (TimeValue org) { - TimeValue zero; TimeValue five(5); TimeSpan interval (Time(org), Duration(Offset (org,five))); diff --git a/tests/stage/model/zoom-window-test.cpp b/tests/stage/model/zoom-window-test.cpp index f7e562d70..993d75e9b 100644 --- a/tests/stage/model/zoom-window-test.cpp +++ b/tests/stage/model/zoom-window-test.cpp @@ -533,7 +533,7 @@ namespace test { CHECK (negaTime < Time::ZERO); Duration& evilDuration = reinterpret_cast (negaTime); // attempt fabricate a subverted TimeSpan CHECK (evilDuration < Time::ZERO); // ...sneak in a negative value - CHECK (TimeSpan(_t(20), evilDuration) == TimeSpan(_t(20),_t(30))); // .....yet won't make it get past Duration copy ctor! +// CHECK (TimeSpan(_t(20), evilDuration) == TimeSpan(_t(20),_t(30))); // .....yet won't make it get past Duration copy ctor! } @@ -601,6 +601,7 @@ namespace test { Rat poison{_raw(Time::MAX)-101010101010101010, _raw(Time::MAX)+23}; CHECK (0 < poison and poison < 1); + /*--Test-1-----------*/ win.setMetric (poison); // inject an evil new value for the metric CHECK (win.visible() == win.overallSpan()); // however, nothing happens CHECK (win.visible().duration() == _t(23)); // since the window is confined to overall canvas size @@ -613,6 +614,7 @@ namespace test { CHECK (win.overallSpan().duration() == Time::MAX); CHECK (win.visible().duration() == _t(23)); // while the visible part remains unaltered + /*--Test-2-----------*/ win.setMetric (poison); // Now attempt again to poison the zoom calculations... CHECK (win.overallSpan().duration() == Time::MAX); // overall canvas unchanged CHECK (win.visible().duration() == TimeValue{856350691}); // visible window expanded (a zoom-out, as required) @@ -636,6 +638,7 @@ namespace test { CHECK (win.overallSpan().duration() == TimeValue{307445734561825860}); CHECK (win.visible().duration() == TimeValue{856350691}); + /*--Test-3-----------*/ win.setVisiblePos (poison); // Yet another way to sneak in our toxic value... CHECK (win.overallSpan().start() == Time::ZERO); CHECK (win.overallSpan().duration() == TimeValue{307445734561825860}); // However, all base values turn out unaffected @@ -652,6 +655,31 @@ namespace test { CHECK (win.px_per_sec() == 575000000_r/856350691); // metric and pixel width are retained CHECK (win.pxWidth() == 575); + + + win.setOverallStart(Time::MAX - TimeValue(23)); // preparation for Test-4 : shift canvas to end of time + CHECK (win.overallSpan() == win.visible()); // consequence: window has been capped to canvas size + CHECK (win.overallSpan().start() == TimeValue{307445734561825572}); // window now also located at extreme values + CHECK (win.overallSpan().end() == TimeValue{307445734561825860}); + CHECK (win.overallSpan().duration() == TimeValue{288}); // window (and canvas) were expanded to comply to maximum zoom factor + CHECK (win.px_per_sec() == 17968750_r/9); // zoom factor was then slightly reduced to match next pixel boundary + CHECK (win.pxWidth() == 575); // established pixel size was retained + SHOW_EXPR(win.overallSpan()); + SHOW_EXPR(_raw(win.overallSpan().start())); + SHOW_EXPR(_raw(win.overallSpan().end())); + SHOW_EXPR(_raw(win.overallSpan().duration())); + SHOW_EXPR(_raw(win.visible().duration())); + + /*--Test-4-----------*/ + win.setVisiblePos(Time{Time::MIN + TimeValue(13)}); // Test: implicitly provoke poisonous factor through extreme offset + + SHOW_EXPR(win.overallSpan()); + SHOW_EXPR(_raw(win.overallSpan().start())); + SHOW_EXPR(_raw(win.overallSpan().end())); + SHOW_EXPR(_raw(win.overallSpan().duration())); + SHOW_EXPR(_raw(win.visible().duration())); + SHOW_EXPR(win.px_per_sec()); + SHOW_EXPR(win.pxWidth()); } @@ -662,6 +690,7 @@ namespace test { safeguard_extremeZoomOut() { // SHOW_EXPR(win.overallSpan()); +// SHOW_EXPR(_raw(win.overallSpan().duration())); // SHOW_EXPR(_raw(win.visible().duration())); // SHOW_EXPR(win.px_per_sec()); // SHOW_EXPR(win.pxWidth()); diff --git a/wiki/thinkPad.ichthyo.mm b/wiki/thinkPad.ichthyo.mm index 377101fb3..a18f551bd 100644 --- a/wiki/thinkPad.ichthyo.mm +++ b/wiki/thinkPad.ichthyo.mm @@ -40893,6 +40893,286 @@ + + + + + + + + + + + + +

+ also offensichtlich erkennt boost::rational die Möglichkeit, den gemeinsamen Faktor 1e6 aus Zähler und Nenner wegzukürzen. Da wir aber hier einen FSec-Wert als Offset zugeben, haben wir wenig Möglichkeiten, das zu vergiften +

+ + + + + + + + + + + + + + + + + + + + +

+ rein numerisch würde das gehen, da ich Time::MAX | MIN sinnigerweise auf INT_MAX / 30 gesetzt habe. Das war vorausschauend.... +

+ + + + + + + + + + + + + +

+ es untergräbt gradezu den Sinn dedizierter Zeit-Entitäten +

+ + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + +
    +
  • + auf den ersten Blick darf es nicht aussehen wie "aha, und hier kann ich alles machen" +
    • +
  • + so wie der Zugang beschrieben ist, ist er völlig logisch und konsistent +
    • +
  • + die erweiterte Möglichkeit erschließt sich erst dem aufmerksamen Leser +
    • +
  • + diese erweiterte Möglichkeit erweist sich aber letztlich als nichts anderes als die Konsequenz der formalen Definition +
    • +
+ + + + + + + + + + + + + +
    +
  • + das wäre dann ein ctor mit einem 2. Argument +
    • +
  • + wer so einen ctor aufruft, weiß was er tut +
    • +
  • + das könnte auch später in eine MicroTic-Basisklasse übernommen werden +
    • +
+ + + + + + + + + + + + + +

+ Das versaut den bisher sehr sauberen Code von TimeValue, und läd gradezu dazu ein, hier beliebige Werte zu konstruieren. Im Hinblick darauf, daß ich umgestalten möchte TimeValue ⟶ MicroTicks, würde damit die Daseinsberechtigung untergraben, denn man kann nun nicht mehr sicher sein, daß MicroTicks ein sicherer Wert ist. +

+ + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + +

+ ...ganz dunkel kommt mir die Erinnerung an eine „kognitive Dissonanz“ — die ich dann schell unter den Teppich gekehrt hatte, indem ich sie mit einem Assert dokumentierte.... +

+ + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + +

+ und die unterliegende lumiera_time_of_gridpoint weicht da ebenfalls auffällig ab... +

+ + + + + + + + + + + +

+ ....also ein Feature, das zwar auf theoretischer Basis entwickelt wurde, aber nur im testgetriebenen Kontext; hier wohl entstanden aus der implementierungsmäßigen Symmetrie zu Grid::gridPoint(n) +

+ + + + + + + + + + + + + + + + + + + + + + + + + + + +