Skip to content

VPLAY-11951 AampTime fixed point calculation conversion gives wrong result with some input #721

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
pstroffolino wants to merge 7 commits into
base: dev_sprint_25_2
Choose a base branch
from
Open

VPLAY-11951 AampTime fixed point calculation conversion gives wrong result with some input #721

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
21becee
VPLAY-11951 AampTime fixed point calculation conversion gives wrong r…
pstroffolino Dec 1, 2025
3098d5f
Reason for Change: refinements
pstroffolino Dec 2, 2025
b6a48f5
Merge branch 'dev_sprint_25_2' into feature/VPLAY-11951
pstroffolino Dec 2, 2025
554d755
Update AampTime.h
p-bond Dec 5, 2025
94e4bde
Update AampTime.h
p-bond Dec 5, 2025
1bda00b
Update AampTime.h
p-bond Dec 5, 2025
828279e
Update AampTime.h
p-bond Dec 5, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
340 changes: 175 additions & 165 deletions AampTime.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,190 +29,200 @@ 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; }
};

/// @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 <inttypes.h>
Copy link

Copilot AI Dec 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The include directive #include <inttypes.h> is placed after the class comment and in the middle of the header structure. This include should be moved to the top of the file with the other includes (<cstdint>, <ostream>, <cmath>) for better organization and consistency with C++ header conventions.

Copilot uses AI. Check for mistakes.

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
{
int64_t retval = this->seconds();

// Fractional part
int64_t tempval = baseTime - retval * baseTimescale;

if (tempval >= ((5 * baseTimescale)/10))
{
retval += 1;
}

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);
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
AampTime(const AampTicks& time) : baseTime(time.ticks)
{
int64_t threshold = INT64_MAX / baseTimescale;
Copy link

Copilot AI Dec 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The constructor parameter time should be passed by const reference since it's not modified. Change the signature to AampTime(const AampTicks &time) to follow const-correctness principles and prevent accidental modifications.

Copilot generated this review using guidance from repository custom instructions.
if( baseTime < threshold && baseTime > -threshold )
Copy link

Copilot AI Dec 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing space after if keyword. Should be if ( instead of if( to follow the project's coding style which adds spaces after control flow keywords.

Suggested change
if( baseTime < threshold && baseTime > -threshold )
if ( baseTime < threshold && baseTime > -threshold )

Copilot uses AI. Check for mistakes.
{ // no overflow - use integer math
baseTime *= baseTimescale;
baseTime /= (int64_t)time.timescale;
}

inline AampTime operator-(const double &t) const
{
AampTime temp(*this);
temp.baseTime = baseTime - int64_t(t * baseTimescale);
return std::move(temp);
else
{ // overflow - fallback to floating point math
baseTime *= (baseTimescale / (double)time.timescale);
Copy link

Copilot AI Dec 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fallback floating-point calculation baseTime *= (baseTimescale / (double)time.timescale); performs the division first, potentially losing precision. This should be rewritten as baseTime = (int64_t)(baseTime * (double)baseTimescale / (double)time.timescale); to ensure the full precision of the multiplication is preserved before division.

Suggested change
baseTime *= (baseTimescale / (double)time.timescale);
baseTime = (int64_t)(baseTime * (double)baseTimescale / (double)time.timescale);

Copilot uses AI. Check for mistakes.
}

inline const AampTime &operator-=(const AampTime &t)
}

/// @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))
{
*this = *this - t;
return *this;
retval += 1;
}
inline const AampTime &operator-=(const double &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 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)
Expand Down
Loading
Loading