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

pstroffolino · 2025-12-01T21:31:29Z

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

…esult 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 <[email protected]>

Copilot

Pull request overview

This PR addresses overflow issues in the AampTime class's fixed-point timescale conversion. When converting from AampTicks to AampTime, large tick values multiplied by the nanosecond base timescale could overflow, producing incorrect results. The fix adds overflow detection and falls back to floating-point arithmetic when overflow is detected.

Key Changes

Modified the AampTime(AampTicks&) constructor to detect integer overflow during timescale conversion
Added runtime overflow check that falls back to double-precision arithmetic when multiplication would overflow
Added new L1 test testTimescaleConversion to validate the fix with values that previously caused overflow

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 9 comments.

File	Description
test/utests/tests/AampTimeTests/validateAampTimeOverloads.cpp	Adds new test case to validate timescale conversion with large tick values that trigger overflow handling
AampTime.h	Implements overflow detection in AampTicks constructor and fallback to floating-point arithmetic; includes code reformatting for consistency

test/utests/tests/AampTimeTests/validateAampTimeOverloads.cpp

AampTime.h

test/utests/tests/AampTimeTests/validateAampTimeOverloads.cpp

AampTime.h

test/utests/tests/AampTimeTests/validateAampTimeOverloads.cpp

AampTime.h

Signed-off-by: Philip Stroffolino <[email protected]>

Copilot

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 8 comments.

AampTime.h

Copilot · 2025-12-02T17:11:04Z

test/utests/tests/AampTimeTests/validateAampTimeOverloads.cpp

+	}
+}
+
+TEST_F(validateAampTimeOverloads, testTimescaleConversionWithOverflow)


[nitpick] The test name testTimescaleConversionWithOverflow doesn't follow the existing naming convention in this test file. Consider renaming to testAampTicksConstructorWithOverflow or testTimescaleConversionLargeValues to be more consistent with the existing test naming pattern.

Suggested change

TEST_F(validateAampTimeOverloads, testTimescaleConversionWithOverflow)

TEST_F(validateAampTimeOverloads, testTimescaleConversionLargeValues)

Copilot · 2025-12-02T17:11:04Z

test/utests/tests/AampTimeTests/validateAampTimeOverloads.cpp

+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);
+	}
+
+	{
+		int64_t rawTicks = -927996007213;
+		uint32_t scale = 240000;
+		AampTicks ticks(rawTicks, scale);
+		AampTime t(ticks);
+		EXPECT_NEAR(t.inSeconds(), -3866650.03005416, 0.001);
+	}
+}


The test coverage is missing an edge case: values that are exactly at the threshold boundary (INT64_MAX / baseTimescale and its negative). Testing these boundary values would ensure the overflow detection logic correctly handles the transition between the integer and floating-point paths. Consider adding test cases with rawTicks = 9223372036 and rawTicks = -9223372036 with an appropriate timescale.

AampTime.h

Copilot · 2025-12-02T17:11:05Z

AampTime.h

 /// @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>


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 · 2025-12-02T17:11:05Z

test/utests/tests/AampTimeTests/validateAampTimeOverloads.cpp

+		uint32_t scale = 240000;
+		AampTicks ticks(rawTicks, scale);
+		AampTime t(ticks);
+		EXPECT_NEAR(t.inSeconds(), 3866650.03005416, 0.001);


The tolerance of 0.001 seconds in the overflow test case is relatively large (1 millisecond) compared to the 0.000001 used in the no-overflow test. Given that the expected result is 3866650.03005416 seconds, this tolerance allows for significant deviation.

While this may be acceptable for the floating-point fallback path, it should be documented why such a large tolerance is needed, or the tolerance should be tightened if the floating-point calculation can be made more accurate (as suggested for line 78 in AampTime.h).

AampTime.h

Co-authored-by: Copilot <[email protected]>

Copilot

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.

Copilot · 2025-12-05T14:31:30Z

AampTime.h

-			return std::move(temp);
+		else
+		{ // overflow - fallback to floating point math
+			baseTime *= (baseTimescale / (double)time.timescale);


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 · 2025-12-05T14:31:30Z

AampTime.h

+	AampTime(const AampTicks& time) : baseTime(time.ticks)
+	{
+		int64_t threshold = INT64_MAX / baseTimescale;
+		if( baseTime < threshold && baseTime > -threshold )


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 · 2025-12-05T14:31:30Z

test/utests/tests/AampTimeTests/validateAampTimeOverloads.cpp

+		AampTime t(ticks);
+		EXPECT_NEAR(t.inSeconds(), 3866650.03005416, 0.001);
+	}
+


The trailing whitespace on this otherwise empty line should be removed. Most modern coding standards and tools discourage trailing whitespace.

Suggested change

Copilot · 2025-12-05T14:31:31Z

test/utests/tests/AampTimeTests/validateAampTimeOverloads.cpp

+		uint32_t scale = 240000;
+		AampTicks ticks(rawTicks, scale);
+		AampTime t(ticks);
+		EXPECT_NEAR(t.inSeconds(), 3866650.03005416, 0.001);


[nitpick] The test tolerance of 0.001 seconds (1 millisecond) is quite loose for a fixed-point time representation that claims nanosecond precision. While this may be acceptable for the overflow case where floating-point fallback is used, consider documenting why this reduced precision is acceptable, or tightening the tolerance if the implementation can support it.

Copilot · 2025-12-05T14:31:31Z

AampTime.h

+	/// @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;


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 AI review requested due to automatic review settings December 1, 2025 21:31

pstroffolino requested a review from a team as a code owner December 1, 2025 21:31

Copilot started reviewing on behalf of pstroffolino December 1, 2025 21:32 View session

Copilot finished reviewing on behalf of pstroffolino December 1, 2025 21:34

Copilot AI reviewed Dec 1, 2025

View reviewed changes

pstroffolino and others added 2 commits December 1, 2025 23:04

Reason for Change: refinements

3098d5f

Signed-off-by: Philip Stroffolino <[email protected]>

Merge branch 'dev_sprint_25_2' into feature/VPLAY-11951

b6a48f5

Copilot AI review requested due to automatic review settings December 2, 2025 17:06

Copilot started reviewing on behalf of pstroffolino December 2, 2025 17:06 View session

Copilot finished reviewing on behalf of pstroffolino December 2, 2025 17:09

Copilot AI reviewed Dec 2, 2025

View reviewed changes

Update AampTime.h

554d755

Co-authored-by: Copilot <[email protected]>

Copilot AI review requested due to automatic review settings December 5, 2025 14:28

p-bond and others added 2 commits December 5, 2025 14:28

Update AampTime.h

94e4bde

Co-authored-by: Copilot <[email protected]>

Update AampTime.h

1bda00b

Co-authored-by: Copilot <[email protected]>

Copilot started reviewing on behalf of p-bond December 5, 2025 14:28 View session

Update AampTime.h

828279e

Co-authored-by: Copilot <[email protected]>

Copilot finished reviewing on behalf of p-bond December 5, 2025 14:30

Copilot AI reviewed Dec 5, 2025

View reviewed changes

	TEST_F(validateAampTimeOverloads, testTimescaleConversionWithOverflow)
	TEST_F(validateAampTimeOverloads, testTimescaleConversionLargeValues)

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

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

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

Are you sure you want to change the base?

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

Conversation

pstroffolino commented Dec 1, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Key Changes

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Copilot AI Dec 2, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 2, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Copilot AI Dec 2, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 2, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Dec 5, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 5, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 5, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 5, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 5, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants