From 38837da65e5e3ebf0d0048531e6beeaec8ec0dc6 Mon Sep 17 00:00:00 2001 From: Ichthyostega Date: Mon, 17 Feb 2020 02:36:54 +0100 Subject: [PATCH] Timehandling: choose safer representation for fractional seconds (closes #939) When drafting the time handling framework some years ago, I foresaw the possible danger of mixing up numbers relating to fractional seconds, with other plain numbers intended as frame counts or as micro ticks. Thus I deliberately picked an incompatible integer type for FSecs = boost::rational However, using long is problematic in itself, since its actual bit length is not fixed, and especially on 32bit platforms long is quite surprisingly defined to be the same as int. However, meanwhile, using the new C++ features, I have blocked pretty much any possible implicit conversion path, requiring explicit conversions in the relevant ctor invocations. So, after weighting in the alternatives, FSecs is now defined as boost::rational. --- src/lib/time/grid.hpp | 15 +- src/lib/time/time.cpp | 38 ++++- src/lib/time/timecode.cpp | 4 +- src/lib/time/timevalue.hpp | 29 ++-- tests/basics/time/quantiser-basics-test.cpp | 8 +- .../asset/meta/time-grid-basics-test.cpp | 18 +-- wiki/thinkPad.ichthyo.mm | 150 ++++++++++++++++-- 7 files changed, 214 insertions(+), 48 deletions(-) diff --git a/src/lib/time/grid.hpp b/src/lib/time/grid.hpp index 50854ea30..f87dc0793 100644 --- a/src/lib/time/grid.hpp +++ b/src/lib/time/grid.hpp @@ -24,7 +24,7 @@ /** @file grid.hpp ** definition of a time grid abstraction for time and timecode handling. ** This interface is the foundation to deal with _quantised_ (grid aligned) - ** time values, as is essential for handling of timecode data. + ** time values, which is essential for handling of timecode data. */ @@ -43,12 +43,13 @@ namespace time { /** * Abstraction of a value alignment grid. * Such a grid has an underlying scale (origin and measurement) - * and is comprised of distinct grid intervals, which can be addressed - * by a ordering number, centred at origin with interval number zero. - * The typical example is a 25fps time frame grid, but indeed the - * spacing of the intervals is not necessarily constant. An entity - * defining such a grid provides functions to calculate the - * grid coordinates and to convert back to plain values. + * and is comprised of consecutive grid intervals, joined at the + * _grid points._ These can be addressed by an ordering number, + * centred at origin with grid point number zero. + * The classical example is a 25fps time frame grid, but in fact + * the length of the intervals is not necessarily constant. An + * entity defining such a grid provides functions to calculate + * the grid coordinates and to convert back to plain values. * This includes a way of rounding to the next lower * grid point, usable for grid aligning values. * diff --git a/src/lib/time/time.cpp b/src/lib/time/time.cpp index 76a988e5d..ce05c2cb6 100644 --- a/src/lib/time/time.cpp +++ b/src/lib/time/time.cpp @@ -55,6 +55,7 @@ extern "C" { #include #include #include +#include #include #include @@ -73,6 +74,9 @@ namespace error = lumiera::error; namespace lib { +namespace meta { + extern const std::string FAILURE_INDICATOR; +} namespace time { @@ -175,17 +179,30 @@ namespace time { } + namespace { + template + string + renderFraction (INT const& frac, Literal postfx) noexcept + try { + std::ostringstream buffer; + if (1 == frac.denominator() or 0 == frac.numerator()) + buffer << frac.numerator() << postfx; + else + buffer << frac < (numerator())+"FPS" - : lexical_cast (numerator()) - + "/" - + lexical_cast (denominator()) - + "FPS"; + return renderFraction (*this, "FPS"); } + /** @internal backdoor to sneak in a raw time value * bypassing any normalisation and limiting */ TimeValue @@ -247,6 +264,13 @@ namespace time { }} // namespace lib::Time +namespace util { + string + StringConv::invoke (lib::time::FSecs val) noexcept + { + return lib::time::renderFraction (val, "sec"); + } +} // namespace util @@ -285,7 +309,7 @@ lumiera_tmpbuf_print_time (gavl_time_t time) gavl_time_t lumiera_rational_to_time (FSecs const& fractionalSeconds) { - return rational_cast (lib::time::TimeValue::SCALE * fractionalSeconds); + return rational_cast (fractionalSeconds * int{lib::time::TimeValue::SCALE}); } gavl_time_t @@ -304,7 +328,7 @@ lumiera_frame_duration (FrameRate const& fps) throw error::Logic ("Impossible to quantise to an zero spaced frame grid" , error::LUMIERA_ERROR_BOTTOM_VALUE); - FSecs duration = rational_cast (1/fps); + FSecs duration = 1 / fps; return lumiera_rational_to_time (duration); } diff --git a/src/lib/time/timecode.cpp b/src/lib/time/timecode.cpp index 2e30a9e79..a1f4ebee6 100644 --- a/src/lib/time/timecode.cpp +++ b/src/lib/time/timecode.cpp @@ -122,14 +122,14 @@ namespace time { static regex fracSecs_parser ("(?:^|[^\\./\\d\\-])(\\-?\\d+)(?:([\\-\\+]\\d+)?/(\\d+))?sec"); //__no leading[./-\d] number [+-] number '/' number 'sec' - #define SUB_EXPR(N) lexical_cast (match[N]) + #define SUB_EXPR(N) lexical_cast (match[N]) smatch match; if (regex_search (seconds, match, fracSecs_parser)) if (match[2].matched) { // complete spec with all parts FSecs fractionalPart (SUB_EXPR(2), SUB_EXPR(3)); - long fullSeconds (SUB_EXPR(1)); + int fullSeconds (SUB_EXPR(1)); return grid.timeOf (fullSeconds + fractionalPart); } else diff --git a/src/lib/time/timevalue.hpp b/src/lib/time/timevalue.hpp index 0755e4969..36c50c515 100644 --- a/src/lib/time/timevalue.hpp +++ b/src/lib/time/timevalue.hpp @@ -205,7 +205,7 @@ namespace time { /** rational representation of fractional seconds * @warning do not mix up gavl_time_t and FSecs */ - typedef boost::rational FSecs; + typedef boost::rational FSecs; /** @@ -550,12 +550,10 @@ namespace time { class FrameRate : public boost::rational { - typedef boost::rational IFrac; - public: FrameRate (uint fps) ; FrameRate (uint num, uint denom); - FrameRate (IFrac const& fractionalRate); + FrameRate (boost::rational const& fractionalRate); // standard copy acceptable; @@ -576,7 +574,7 @@ namespace time { inline FSecs operator/ (int n, FrameRate rate) { - return n / boost::rational_cast (rate); + return FSecs{ n*rate.denominator(), rate.numerator()}; } @@ -621,17 +619,17 @@ namespace time { inline FrameRate::FrameRate (uint fps) - : IFrac (__ensure_nonzero(fps)) + : boost::rational (__ensure_nonzero(fps)) { } inline FrameRate::FrameRate (uint num, uint denom) - : IFrac (__ensure_nonzero(num), denom) + : boost::rational (__ensure_nonzero(num), denom) { } inline - FrameRate::FrameRate (IFrac const& fractionalRate) - : IFrac (__ensure_nonzero(fractionalRate)) + FrameRate::FrameRate (boost::rational const& fractionalRate) + : boost::rational (__ensure_nonzero(fractionalRate)) { } inline double @@ -654,5 +652,16 @@ namespace util { return 0 == dur; } + // repeated or forward declaration, see meta/util.hpp + template + struct StringConv; + + /** specialisation: render fractional seconds (for diagnostics) */ + template<> + struct StringConv + { + static std::string + invoke (lib::time::FSecs) noexcept; + }; } -#endif +#endif /*LIB_TIME_TIMEVALUE_H*/ diff --git a/tests/basics/time/quantiser-basics-test.cpp b/tests/basics/time/quantiser-basics-test.cpp index e37445409..b1eb73b35 100644 --- a/tests/basics/time/quantiser-basics-test.cpp +++ b/tests/basics/time/quantiser-basics-test.cpp @@ -43,8 +43,8 @@ namespace test{ namespace { - const uint MAX_FRAMES = 25*500; - const uint DIRT_GRAIN = 50; + const int MAX_FRAMES = 25*500; + const int DIRT_GRAIN = 50; const FSecs F25(1,25); // duration of one PAL frame @@ -91,10 +91,10 @@ namespace test{ { FixedFrameQuantiser fixQ(25); - uint frames = (rand() % MAX_FRAMES); + int frames = (rand() % MAX_FRAMES); FSecs dirt = (F25 / (2 + rand() % DIRT_GRAIN)); - Time rawTime = Time(frames*F25) + Duration(dirt); ////////////////TICKET #939 : should better use 64bit base type for FSecs ?? + Time rawTime = Time(frames*F25) + Duration(dirt); CHECK (Time( frames *F25) <= rawTime); CHECK (Time((frames+1)*F25) > rawTime); diff --git a/tests/core/steam/asset/meta/time-grid-basics-test.cpp b/tests/core/steam/asset/meta/time-grid-basics-test.cpp index 54f77303e..70578ebdc 100644 --- a/tests/core/steam/asset/meta/time-grid-basics-test.cpp +++ b/tests/core/steam/asset/meta/time-grid-basics-test.cpp @@ -52,8 +52,8 @@ namespace test { namespace { // Test definitions... - const Time testOrigin (12,34); - const FrameRate testFps (5,6); + const Time TEST_ORIGIN (12,34); + const FrameRate TEST_FPS (5,6); const uint MAX_FRAMES = 1000; const uint DIRT_GRAIN = 50; @@ -89,8 +89,8 @@ namespace test { CHECK ( spec.origin == TimeValue(0)); CHECK (!spec.predecessor); - spec.fps = testFps; - spec.origin = testOrigin; + spec.fps = TEST_FPS; + spec.origin = TEST_ORIGIN; PGrid myGrid = spec.commit(); CHECK (myGrid); @@ -101,15 +101,15 @@ namespace test { int randomFrame = (rand() % MAX_FRAMES); Time point (myGrid->timeOf (randomFrame)); - CHECK (point == testOrigin + randomFrame * testFps.duration()); + CHECK (point == TEST_ORIGIN + randomFrame * TEST_FPS.duration()); - uint fract = 2 + rand() % DIRT_GRAIN; - FSecs dirt = rational_cast (1 / testFps / fract); - ASSERT (Time(dirt) < testFps.duration()); + int fract = 2 + rand() % DIRT_GRAIN; + FSecs dirt = (1/TEST_FPS) / fract; + ASSERT (Time(dirt) < TEST_FPS.duration()); ASSERT (0 < dirt); Time dirty(point + Time(dirt)); - CHECK (point == testOrigin + myGrid->gridAlign(dirty)); + CHECK (point == TEST_ORIGIN + myGrid->gridAlign(dirty)); } diff --git a/wiki/thinkPad.ichthyo.mm b/wiki/thinkPad.ichthyo.mm index fa8b7458e..2509c69fc 100644 --- a/wiki/thinkPad.ichthyo.mm +++ b/wiki/thinkPad.ichthyo.mm @@ -44967,8 +44967,7 @@ früher war das so eine typische "nörgel"-Warnung, die man unter den Teppich kehren konnte: 'ey, der Compiler bekommt es ja trotzdem richtig hin.

- - + @@ -44989,8 +44988,7 @@ - - + @@ -45363,9 +45361,144 @@ - - - + + + + + + + + +

+ als "workaround" hatte ich boost::rational<long> genommen +

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

+ und wir brauchen die definitiv zum sinnvollen Rechenen mit Zeiten auf micro-Scala +

+ + + + + + + + +

+ Der kritsche Fall ist nämlich, wenn wir mit FSecs anfangen, und dann irgendwo in der Rechnung mal Time::SCALE multiplizieren, um auf die µ-Skala zu wechseln. Am Ende der Rechnung würde dann typischerweise ein rational_cast stehen. Damit das funktioniert, muß vor allem der Zähler des Bruches die volle Zeitskala unterstützen. Daher sind 64bit zwingend +

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

+ weil 64bit -> 32bit eine narrowing conversion ist, die zumindest eine Warnung erzeugt +

+ + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + @@ -45406,8 +45539,7 @@ WICHTIG: keine vorgreifende Infos publizieren!!!!!

- - +