From 21becee3dfd0c827ec3ab76f821add3fa27679fa Mon Sep 17 00:00:00 2001 From: Philip Stroffolino Date: Mon, 1 Dec 2025 16:28:55 -0500 Subject: [PATCH 1/6] VPLAY-11951 AampTime fixed point calculation conversion gives wrong result with some input Reason for Change: introduced overflow sanity check and fallback to floating point arithmetic Test Guidance: see new L1 test Risk: Medium Signed-off-by: Philip Stroffolino --- AampTime.h | 340 +++++++++--------- .../validateAampTimeOverloads.cpp | 11 + 2 files changed, 188 insertions(+), 163 deletions(-) diff --git a/AampTime.h b/AampTime.h index 5d8844dab..f8ddc61de 100644 --- a/AampTime.h +++ b/AampTime.h @@ -29,12 +29,12 @@ struct AampTicks { int64_t ticks; uint32_t timescale; - + /// @brief Constructor /// @param ticks /// @param timescale AampTicks(int64_t ticks, uint32_t timescale) : ticks(ticks), timescale(timescale) {} - + /// @brief Get time in milliseconds int64_t inMilli() { return (ticks * 1000) / (int64_t)timescale; } }; @@ -45,174 +45,188 @@ struct AampTicks class AampTime { - public: - typedef enum { milli = 1000, micro = 1000000, nano = 1000000000 } TimeScale; - - private: - static const uint64_t baseTimescale = nano; - int64_t baseTime; - - public: - /// @brief Constructor - /// @param seconds time in seconds, as a double - constexpr AampTime(double seconds = 0.0) : baseTime(int64_t(seconds * baseTimescale)){} - - /// @brief Copy constructor - /// @param rhs AampTime object to copy - constexpr AampTime(const AampTime& rhs) : baseTime(rhs.baseTime){} - - /// @brief Constructor - /// @param time struct containing time in ticks and timescale - /// @note This is used to convert from AampTicks to AampTime; it is lossy and cannot be converted back - constexpr AampTime(AampTicks &time) : baseTime((time.ticks * (int64_t)baseTimescale) / (int64_t)time.timescale) {} - - /// @brief Get the stored time - /// @return Time in seconds (double) - inline double inSeconds() const { return (baseTime / double(baseTimescale)); } - - /// @brief Get the stored time in seconds - /// @return Time in seconds (integer) - inline int64_t seconds() const { return (baseTime / baseTimescale); } - - /// @brief Get the stored time in milliseconds - /// @return Time in milliseconds (integer) - inline int64_t milliseconds() const { return (baseTime / (baseTimescale / milli)); } - - // Equivalent to round() but in integer domain - inline int64_t nearestSecond() const +public: + typedef enum { milli = 1000, micro = 1000000, nano = 1000000000 } TimeScale; + +private: + static const uint64_t baseTimescale = nano; + int64_t baseTime; + +public: + /// @brief Constructor + /// @param seconds time in seconds, as a double + constexpr AampTime(double seconds = 0.0) : baseTime(int64_t(seconds * baseTimescale)){} + + /// @brief Copy constructor + /// @param rhs AampTime object to copy + constexpr AampTime(const AampTime& rhs) : baseTime(rhs.baseTime){} + + /// @brief Constructor + /// @param time struct containing time in ticks and timescale + /// @note This is used to convert from AampTicks to AampTime; it is lossy and cannot be converted back + /*constexpr*/ AampTime(AampTicks &time) : baseTime(time.ticks) + { + if( baseTime!=0 ) { - int64_t retval = this->seconds(); - - // Fractional part - int64_t tempval = baseTime - retval * baseTimescale; - - if (tempval >= ((5 * baseTimescale)/10)) - { - retval += 1; + auto result = baseTime * baseTimescale; + if( baseTime == result / baseTimescale ) + { // no overflow - use integer math + baseTime = result / (int64_t)time.timescale; } - - return retval; - } - - // Overloads for comparison operators to check AampTime : AampTime and AampTime : double - // Converting (and truncating) the double to the timescale should avoid the issues around epsilon for floating point - inline bool operator==(const AampTime &rhs) const - { - if (this == &rhs) - return true; else - return (baseTime == rhs.baseTime); - } - - inline bool operator==(const double &rhs) const { return (baseTime == int64_t(rhs * baseTimescale)); } - - inline AampTime& operator=(const AampTime &rhs) - { - if (this == &rhs) - return *this; - - baseTime = rhs.baseTime; - return *this; - } - - inline AampTime& operator=(const double &rhs) - { - baseTime = int64_t(rhs * baseTimescale); - return *this; - } - - inline AampTime operator-() const - { - AampTime temp(*this); - temp.baseTime = -baseTime; - return temp; - } - - inline bool operator!=(const AampTime &rhs) const { return !(*this == rhs); } - inline bool operator!=(double &rhs) const { return !(*this == rhs); } - - inline bool operator>(const AampTime &rhs) const { return (baseTime > rhs.baseTime); } - inline bool operator>(const double &rhs) const { return (baseTime > int64_t(rhs * baseTimescale)); } - - inline bool operator<(const AampTime &rhs) const { return ((*this != rhs) && (!(*this > rhs))); } - inline bool operator<(const double &rhs) const { return ((*this != rhs) && (!(*this > rhs))); } - - inline bool operator>=(const AampTime &rhs) const { return ((*this > rhs) || (*this == rhs)); } - inline bool operator>=(double rhs) const { return ((*this > rhs) || (*this == rhs)); } - - inline bool operator<=(const AampTime &rhs) const { return ((*this < rhs) || (*this == rhs)); } - inline bool operator<=(double rhs) const { return ((*this < rhs) || (*this == rhs)); } - - inline AampTime operator+(const AampTime &t) const - { - AampTime temp(*this); - - temp.baseTime = baseTime + t.baseTime; - return temp; - } - inline AampTime operator+(const double &t) const - { - AampTime temp(*this); - - temp.baseTime = baseTime + int64_t(t * baseTimescale); - return std::move(temp); - } - - inline const AampTime &operator+=(const AampTime &t) - { - *this = *this + t; - return *this; - } - inline const AampTime &operator+=(const double &t) - { - *this = *this + t; - return *this; - } - - inline AampTime operator-(const AampTime &t) const - { - AampTime temp(*this); - - temp.baseTime = baseTime - t.baseTime; - return std::move(temp); + { // overflow - fallback to double precision math + baseTime *= (baseTimescale / (double)time.timescale); + } } - - inline AampTime operator-(const double &t) const + } + + /// @brief Get the stored time + /// @return Time in seconds (double) + inline double inSeconds() const { return (baseTime / double(baseTimescale)); } + + /// @brief Get the stored time in seconds + /// @return Time in seconds (integer) + inline int64_t seconds() const { return (baseTime / baseTimescale); } + + /// @brief Get the stored time in milliseconds + /// @return Time in milliseconds (integer) + inline int64_t milliseconds() const { return (baseTime / (baseTimescale / milli)); } + + // Equivalent to round() but in integer domain + inline int64_t nearestSecond() const + { + int64_t retval = this->seconds(); + + // Fractional part + int64_t tempval = baseTime - retval * baseTimescale; + + if (tempval >= ((5 * baseTimescale)/10)) { - AampTime temp(*this); - temp.baseTime = baseTime - int64_t(t * baseTimescale); - return std::move(temp); + retval += 1; } - - inline const AampTime &operator-=(const AampTime &t) - { - *this = *this - t; + + return retval; + } + + // Overloads for comparison operators to check AampTime : AampTime and AampTime : double + // Converting (and truncating) the double to the timescale should avoid the issues around epsilon for floating point + inline bool operator==(const AampTime &rhs) const + { + if (this == &rhs) + return true; + else + return (baseTime == rhs.baseTime); + } + + inline bool operator==(const double &rhs) const { return (baseTime == int64_t(rhs * baseTimescale)); } + + inline AampTime& operator=(const AampTime &rhs) + { + if (this == &rhs) return *this; - } - inline const AampTime &operator-=(const double &t) - { - *this = *this - t; - return *this; - } - - inline AampTime operator/(const double &t) const - { - AampTime temp(*this); - - temp.baseTime = (int64_t)((double)baseTime/t); - return std::move(temp); - } - - inline AampTime operator*(const double &t) const - { - AampTime temp(*this); - - temp.baseTime = (int64_t)((double)baseTime * t); - return std::move(temp); - } - - explicit operator double() const { return this->inSeconds(); } - explicit operator int64_t() const { return this->seconds(); } + + baseTime = rhs.baseTime; + return *this; + } + + inline AampTime& operator=(const double &rhs) + { + baseTime = int64_t(rhs * baseTimescale); + return *this; + } + + inline AampTime operator-() const + { + AampTime temp(*this); + temp.baseTime = -baseTime; + return temp; + } + + inline bool operator!=(const AampTime &rhs) const { return !(*this == rhs); } + inline bool operator!=(double &rhs) const { return !(*this == rhs); } + + inline bool operator>(const AampTime &rhs) const { return (baseTime > rhs.baseTime); } + inline bool operator>(const double &rhs) const { return (baseTime > int64_t(rhs * baseTimescale)); } + + inline bool operator<(const AampTime &rhs) const { return ((*this != rhs) && (!(*this > rhs))); } + inline bool operator<(const double &rhs) const { return ((*this != rhs) && (!(*this > rhs))); } + + inline bool operator>=(const AampTime &rhs) const { return ((*this > rhs) || (*this == rhs)); } + inline bool operator>=(double rhs) const { return ((*this > rhs) || (*this == rhs)); } + + inline bool operator<=(const AampTime &rhs) const { return ((*this < rhs) || (*this == rhs)); } + inline bool operator<=(double rhs) const { return ((*this < rhs) || (*this == rhs)); } + + inline AampTime operator+(const AampTime &t) const + { + AampTime temp(*this); + + temp.baseTime = baseTime + t.baseTime; + return temp; + } + inline AampTime operator+(const double &t) const + { + AampTime temp(*this); + + temp.baseTime = baseTime + int64_t(t * baseTimescale); + return std::move(temp); + } + + inline const AampTime &operator+=(const AampTime &t) + { + *this = *this + t; + return *this; + } + inline const AampTime &operator+=(const double &t) + { + *this = *this + t; + return *this; + } + + inline AampTime operator-(const AampTime &t) const + { + AampTime temp(*this); + + temp.baseTime = baseTime - t.baseTime; + return std::move(temp); + } + + inline AampTime operator-(const double &t) const + { + AampTime temp(*this); + temp.baseTime = baseTime - int64_t(t * baseTimescale); + return std::move(temp); + } + + inline const AampTime &operator-=(const AampTime &t) + { + *this = *this - t; + return *this; + } + inline const AampTime &operator-=(const double &t) + { + *this = *this - t; + return *this; + } + + inline AampTime operator/(const double &t) const + { + AampTime temp(*this); + + temp.baseTime = (int64_t)((double)baseTime/t); + return std::move(temp); + } + + inline AampTime operator*(const double &t) const + { + AampTime temp(*this); + + temp.baseTime = (int64_t)((double)baseTime * t); + return std::move(temp); + } + + explicit operator double() const { return this->inSeconds(); } + explicit operator int64_t() const { return this->seconds(); } }; // For those who like if (0.0 == b) diff --git a/test/utests/tests/AampTimeTests/validateAampTimeOverloads.cpp b/test/utests/tests/AampTimeTests/validateAampTimeOverloads.cpp index 2ea67939f..2444cc557 100644 --- a/test/utests/tests/AampTimeTests/validateAampTimeOverloads.cpp +++ b/test/utests/tests/AampTimeTests/validateAampTimeOverloads.cpp @@ -359,3 +359,14 @@ TEST_F(validateAampTimeOverloads, AampTicksInMilli) AampTicks ticks(5000, 1000); // 5000 ticks with a timescale of 1000 EXPECT_EQ(ticks.inMilli(), 5000); // 5000 milliseconds } + +TEST_F(validateAampTimeOverloads, testTimescaleConversion ) +{ + int64_t rawTicks = 927996007213; + uint32_t scale = 240000; + AampTicks ticks(rawTicks, scale); + + AampTime t(ticks); + double seconds = t.inSeconds(); + EXPECT_EQ( round(seconds), 3866650); +} From 3098d5f01db244fee9d69e0baa4bb92bb3f580e0 Mon Sep 17 00:00:00 2001 From: Philip Stroffolino Date: Mon, 1 Dec 2025 23:04:43 -0500 Subject: [PATCH 2/6] Reason for Change: refinements Signed-off-by: Philip Stroffolino --- AampTime.h | 23 +++++----- .../validateAampTimeOverloads.cpp | 42 +++++++++++++++---- 2 files changed, 45 insertions(+), 20 deletions(-) diff --git a/AampTime.h b/AampTime.h index f8ddc61de..f5453ae20 100644 --- a/AampTime.h +++ b/AampTime.h @@ -42,6 +42,7 @@ struct AampTicks /// @brief time class to work around the use of doubles within Aamp // While operators are overloaded for comparisons, the underlying data type is integer // But the code is tolerant of being treated as a double +#include class AampTime { @@ -64,19 +65,17 @@ class AampTime /// @brief Constructor /// @param time struct containing time in ticks and timescale /// @note This is used to convert from AampTicks to AampTime; it is lossy and cannot be converted back - /*constexpr*/ AampTime(AampTicks &time) : baseTime(time.ticks) + AampTime(AampTicks &time) : baseTime(time.ticks) { - if( baseTime!=0 ) - { - auto result = baseTime * baseTimescale; - if( baseTime == result / baseTimescale ) - { // no overflow - use integer math - baseTime = result / (int64_t)time.timescale; - } - else - { // overflow - fallback to double precision math - baseTime *= (baseTimescale / (double)time.timescale); - } + int64_t threshold = INT64_MAX / baseTimescale; + if( baseTime < threshold && baseTime > -threshold ) + { // no overflow - use integer math + baseTime *= baseTimescale; + baseTime /= (int64_t)time.timescale; + } + else + { // overflow - fallback to floating point math + baseTime *= (baseTimescale / (double)time.timescale); } } diff --git a/test/utests/tests/AampTimeTests/validateAampTimeOverloads.cpp b/test/utests/tests/AampTimeTests/validateAampTimeOverloads.cpp index 2444cc557..9bf4bbd0b 100644 --- a/test/utests/tests/AampTimeTests/validateAampTimeOverloads.cpp +++ b/test/utests/tests/AampTimeTests/validateAampTimeOverloads.cpp @@ -360,13 +360,39 @@ TEST_F(validateAampTimeOverloads, AampTicksInMilli) EXPECT_EQ(ticks.inMilli(), 5000); // 5000 milliseconds } -TEST_F(validateAampTimeOverloads, testTimescaleConversion ) -{ - int64_t rawTicks = 927996007213; - uint32_t scale = 240000; - AampTicks ticks(rawTicks, scale); +TEST_F(validateAampTimeOverloads, testTimescaleConversionNoOverflow) +{ // Small values that won't overflow + { + int64_t rawTicks = 1000; + uint32_t scale = 1000; + AampTicks ticks(rawTicks, scale); + AampTime t(ticks); + EXPECT_NEAR(t.inSeconds(), 1.0, 0.000001); + } + { + int64_t rawTicks = -1000; + uint32_t scale = 1000; + AampTicks ticks(rawTicks, scale); + AampTime t(ticks); + EXPECT_NEAR(t.inSeconds(), -1.0, 0.000001); + } +} + +TEST_F(validateAampTimeOverloads, testTimescaleConversionWithOverflow) +{ // Large values that will overflow int64_t * 1000000000 + { + int64_t rawTicks = 927996007213; + uint32_t scale = 240000; + AampTicks ticks(rawTicks, scale); + AampTime t(ticks); + EXPECT_NEAR(t.inSeconds(), 3866650.03005416, 0.001); + } - AampTime t(ticks); - double seconds = t.inSeconds(); - EXPECT_EQ( round(seconds), 3866650); + { + int64_t rawTicks = -927996007213; + uint32_t scale = 240000; + AampTicks ticks(rawTicks, scale); + AampTime t(ticks); + EXPECT_NEAR(t.inSeconds(), -3866650.03005416, 0.001); + } } From 554d755bc04a1bd4030279cb453b0cb571a053e9 Mon Sep 17 00:00:00 2001 From: p-bond <116633057+p-bond@users.noreply.github.com> Date: Fri, 5 Dec 2025 14:28:25 +0000 Subject: [PATCH 3/6] Update AampTime.h Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --- AampTime.h | 1 - 1 file changed, 1 deletion(-) diff --git a/AampTime.h b/AampTime.h index f5453ae20..608cd6524 100644 --- a/AampTime.h +++ b/AampTime.h @@ -29,7 +29,6 @@ struct AampTicks { int64_t ticks; uint32_t timescale; - /// @brief Constructor /// @param ticks /// @param timescale From 94e4bdee5c32c605c45eaf8713dd3ad1cac5f88d Mon Sep 17 00:00:00 2001 From: p-bond <116633057+p-bond@users.noreply.github.com> Date: Fri, 5 Dec 2025 14:28:33 +0000 Subject: [PATCH 4/6] Update AampTime.h Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --- AampTime.h | 1 - 1 file changed, 1 deletion(-) diff --git a/AampTime.h b/AampTime.h index 608cd6524..15fa65ed4 100644 --- a/AampTime.h +++ b/AampTime.h @@ -33,7 +33,6 @@ struct AampTicks /// @param ticks /// @param timescale AampTicks(int64_t ticks, uint32_t timescale) : ticks(ticks), timescale(timescale) {} - /// @brief Get time in milliseconds int64_t inMilli() { return (ticks * 1000) / (int64_t)timescale; } }; From 1bda00b79010282dbea9e0f6ffc33099b60b4f47 Mon Sep 17 00:00:00 2001 From: p-bond <116633057+p-bond@users.noreply.github.com> Date: Fri, 5 Dec 2025 14:28:47 +0000 Subject: [PATCH 5/6] Update AampTime.h Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --- AampTime.h | 1 - 1 file changed, 1 deletion(-) diff --git a/AampTime.h b/AampTime.h index 15fa65ed4..d50be5cc8 100644 --- a/AampTime.h +++ b/AampTime.h @@ -46,7 +46,6 @@ class AampTime { public: typedef enum { milli = 1000, micro = 1000000, nano = 1000000000 } TimeScale; - private: static const uint64_t baseTimescale = nano; int64_t baseTime; From 828279eae2af6b864073c16bf3863f265a3ba912 Mon Sep 17 00:00:00 2001 From: p-bond <116633057+p-bond@users.noreply.github.com> Date: Fri, 5 Dec 2025 14:29:38 +0000 Subject: [PATCH 6/6] Update AampTime.h Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --- AampTime.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/AampTime.h b/AampTime.h index d50be5cc8..0c98941f2 100644 --- a/AampTime.h +++ b/AampTime.h @@ -62,7 +62,7 @@ class AampTime /// @brief Constructor /// @param time struct containing time in ticks and timescale /// @note This is used to convert from AampTicks to AampTime; it is lossy and cannot be converted back - AampTime(AampTicks &time) : baseTime(time.ticks) + AampTime(const AampTicks& time) : baseTime(time.ticks) { int64_t threshold = INT64_MAX / baseTimescale; if( baseTime < threshold && baseTime > -threshold )