VPLAY-12359 suboptimal UTCTiming behavior #853

sayanurag · 2026-01-14T13:58:21Z

Reason for change: Updates UTCTiming logic to skip sync if interval hasn’t elapsed. Add Configs to control the interval
Test Procedure: updated in ticket
Risks: Low

Copilot

Pull request overview

This PR optimizes UTC time synchronization behavior for DASH manifests by implementing interval-based sync throttling. The changes prevent excessive network calls to UTC time servers by skipping synchronization attempts until a minimum interval has elapsed, while maintaining the ability to use cached offset values.

Changes:

Added TimeSyncClient struct to track UTC sync state (last sync time, offset, and sync status)
Introduced two new configuration options: UTCSyncOnStartup (boolean) and UTCSyncMinIntervalSec/UTCSyncOnErrorRetrySec (integers)
Modified FindServerUTCTime to check elapsed time since last sync before making network calls

Reviewed changes

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

Show a summary per file

File	Description
fragmentcollector_mpd.h	Added TimeSyncClient struct definition and member variable to track sync state
fragmentcollector_mpd.cpp	Updated FindServerUTCTime logic to conditionally sync based on elapsed interval
AampDefine.h	Added default constant definitions for UTC sync intervals
AampConfig.h	Added new config enum entries for UTC sync control
AampConfig.cpp	Registered new config options in lookup tables with defaults

fragmentcollector_mpd.h

fragmentcollector_mpd.cpp

AampDefine.h

fragmentcollector_mpd.cpp

fragmentcollector_mpd.h

fragmentcollector_mpd.cpp

Copilot

Pull request overview

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

fragmentcollector_mpd.h

fragmentcollector_mpd.cpp

fragmentcollector_mpd.h

fragmentcollector_mpd.cpp

AampConfig.h

fragmentcollector_mpd.h

Reason for change: Updates UTCTiming logic to skip sync if interval hasn’t elapsed. Add Configs to control the interval Test Procedure: updated in ticket Risks: Low Signed-off-by: Anurag Krishnan <[email protected]>

deprecated eAAMPConfig_UTCSyncOnErrorRetrySec fixed bug with config order causing runtime crash added some doxygen comments Signed-off-by: Philip Stroffolino <[email protected]>

Copilot

Pull request overview

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

fragmentcollector_mpd.h

fragmentcollector_mpd.cpp

AampConfig.cpp

AampConfig.h

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

Copilot

Pull request overview

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

fragmentcollector_mpd.cpp

AampConfig.cpp

fragmentcollector_mpd.h

fragmentcollector_mpd.cpp

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

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

Copilot

Pull request overview

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

fragmentcollector_mpd.h

fragmentcollector_mpd.cpp

AampConfig.cpp

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

Merge branch 'feature/VPLAY-12359_UTCTiming' of https://github.com/rdkcentral/aamp into feature/VPLAY-12359_UTCTiming Signed-off-by: Philip Stroffolino <[email protected]>

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

Copilot

Pull request overview

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

fragmentcollector_mpd.cpp

fragmentcollector_mpd.h

AampConfig.cpp

Copilot · 2026-01-14T22:48:54Z

fragmentcollector_mpd.h

+ * - lastOffset: Cached time delta (in seconds) between local and server time.
+ * - hasSynced: Flag indicating whether at least one successful sync has occurred.
+ */
+struct TimeSyncClient


Missing include directive for the fixed-width integer types. If the recommendation to use 'int64_t' instead of 'long long' is followed (as suggested in another comment), this header file should include '' to make those types available.

Add: #include

fragmentcollector_mpd.h

Copilot · 2026-01-14T22:48:55Z

fragmentcollector_mpd.cpp

-						mLocalUtcTime = GetNetworkTime(ServerUrl, &http_error, aamp->GetNetworkProxy());
-						if(mLocalUtcTime > 0 )
+						bool shouldSyncOnStartup = !mTimeSyncClient.hasSynced && ISCONFIGSET(eAAMPConfig_UTCSyncOnStartup);
+						bool intervalElapsed = false;


The logic flow has a subtle issue: when shouldSyncOnStartup is true, intervalElapsed is never calculated, which is fine. However, the condition on line 4517 uses OR logic (shouldSyncOnStartup || intervalElapsed), which means intervalElapsed could be used uninitialized if shouldSyncOnStartup is false and the calculation on line 4515 doesn't execute due to the if statement on line 4512.

Wait - looking more carefully, if shouldSyncOnStartup is false (line 4512 condition), then we enter the block and calculate intervalElapsed. So intervalElapsed is always initialized before use on line 4517. However, this is not immediately clear from the code structure.

Consider declaring intervalElapsed with an initial value for clarity: 'bool intervalElapsed = false;' to make the initialization explicit and improve readability.

fragmentcollector_mpd.h

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

Copilot

Pull request overview

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

fragmentcollector_mpd.h

Copilot · 2026-01-15T01:29:55Z

fragmentcollector_mpd.cpp

-							double currentTime = (double)aamp_GetCurrentTimeMS() / 1000;
-							mDeltaTime =  mLocalUtcTime - currentTime;
-							hasServerUtcTime = true;
+							const double elapsed = (double)(aamp_GetCurrentTimeMS() - mTimeSyncClient.lastSync) / 1000;


The magic number 1000 is used for millisecond-to-second conversion. Consider defining a named constant like MS_IN_SEC or MILLISECONDS_PER_SECOND to improve code readability and maintainability, as this pattern appears multiple times in the codebase.

fragmentcollector_mpd.cpp

AampDefine.h

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

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

Copilot

Pull request overview

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

Comments suppressed due to low confidence (1)

fragmentcollector_mpd.h:1

The struct members should use fixed-width integer types for lastSync to ensure consistent size across platforms. According to the custom C++ guidelines for ctypes interoperability, use int64_t instead of long long.

/*

Copilot · 2026-01-15T02:37:17Z

AampConfig.cpp

 	{DEFAULT_MIN_BUFFER_LOW_LATENCY,"lowLatencyMinBuffer",eAAMPConfig_LowLatencyMinBuffer,true, eCONFIG_RANGE_LLDBUFFER},
 	{DEFAULT_TARGET_BUFFER_LOW_LATENCY,"lowLatencyTargetBuffer",eAAMPConfig_LowLatencyTargetBuffer,true, eCONFIG_RANGE_LLDBUFFER},
-	{GST_BW_TO_BUFFER_FACTOR,"bandwidthToBufferFactor", eAAMPConfig_BWToGstBufferFactor,true},
+	{GST_BW_TO_BUFFER_FACTOR,"bandwidthToBufferFactor", eAAMPConfig_BWToGstBufferFactor,true}


The removal of the trailing comma from the last entry in mConfigLookupTableFloat is inconsistent with C++11 best practices, which allow and encourage trailing commas in initializer lists for easier maintenance. Consider keeping the trailing comma to simplify future additions.

Copilot · 2026-01-15T02:37:17Z

test/utests/fakes/FakeFragmentCollector_MPD.cpp

 #include "fragmentcollector_mpd.h"
 #include "MockStreamAbstractionAAMP_MPD.h"

+TimeSyncClient::TimeSyncClient(){}


The empty constructor implementation should have the braces on separate lines or use = default to be consistent with modern C++ style. Consider either formatting as TimeSyncClient::TimeSyncClient() {} with a space, or using the default constructor syntax.

Copilot · 2026-01-15T02:37:18Z

fragmentcollector_mpd.cpp

-							double currentTime = (double)aamp_GetCurrentTimeMS() / 1000;
-							mDeltaTime =  mLocalUtcTime - currentTime;
-							hasServerUtcTime = true;
+							const double elapsed = (double)(aamp_GetCurrentTimeMS() - mTimeSyncClient.lastSync) / 1000;


The calculation converts to double and then divides by 1000. This can be optimized by performing the division on the integer value first to minimize precision loss, or using a more explicit conversion: const double elapsed = static_cast<double>(aamp_GetCurrentTimeMS() - mTimeSyncClient.lastSync) / 1000.0;. Also prefer static_cast over C-style casts per C++ Core Guidelines.

Copilot · 2026-01-15T02:37:18Z

fragmentcollector_mpd.cpp

+							if(mLocalUtcTime > 0)
+							{
+								mTimeSyncClient.lastSync = aamp_GetCurrentTimeMS();
+								mDeltaTime =  mLocalUtcTime - (double)mTimeSyncClient.lastSync / 1000;


Use static_cast<double> instead of C-style cast (double) to follow C++ Core Guidelines and modern C++ best practices.

Copilot · 2026-01-15T02:37:18Z

test/utests/tests/StreamAbstractionAAMP_MPD/FunctionalTests.cpp

+		{eAAMPConfig_MaxFragmentChunkCached, DEFAULT_CACHED_FRAGMENT_CHUNKS_PER_TRACK},
+		{eAAMPConfig_UTCSyncMinIntervalSec, DEFAULT_UTC_SYNC_MIN_INTERVAL_SEC}


The new UTC sync functionality introduced in FindServerUTCTime lacks dedicated test coverage. The existing FindServerUTCTimeTest should be extended to verify the interval-based throttling behavior, including scenarios where sync is skipped due to the minimum interval not being elapsed, and where cached offset values are used.

@copilot open a new pull request to apply changes based on this feedback

Copilot · 2026-01-15T02:40:54Z

@pstroffolino I've opened a new pull request, #858, to work on those changes. Once the pull request is ready, I'll request review from you.

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

…kcentral/aamp into feature/VPLAY-12359_UTCTiming

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

Copilot

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.

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

Copilot

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.

Copilot · 2026-01-15T04:01:26Z

test/utests/tests/StreamAbstractionAAMP_MPD/FunctionalTests.cpp

+ */
+TEST_F(StreamAbstractionAAMP_MPDTest, FindServerUTCTime_SyncOnStartup)
+{
+	GTEST_SKIP();


All five new UTC sync test cases are skipped (lines 3034, 3086, 3145, 3207, 3266). These tests cover critical behavior including startup sync, interval-based skip logic, and cached offset usage. They should be enabled and properly integrated into the test suite to validate the new UTC synchronization logic.

Copilot · 2026-01-15T04:01:27Z

fragmentcollector_mpd.h

+	bool hasSynced; /**< Flag indicating whether at least one successful sync has occurred. */
+
+	/**
+	 * @brief Constructor initializes lastSync with current time and resets other members.


The documentation states the constructor "initializes lastSync with current time" but doesn't mention that this initialization happens via aamp_GetCurrentTimeMS() which creates a side effect during construction by making a system call. This should be documented, or consider using a factory method/explicit initialization instead of doing this work in the constructor to follow RAII best practices.

Suggested change

* @brief Constructor initializes lastSync with current time and resets other members.

* @brief Constructor initializes lastSync with current time and resets other members.

*

* This initialization obtains the current time via a call to

* aamp_GetCurrentTimeMS(), which may perform a system call to

* query the system clock. Callers should be aware that constructing

* TimeSyncClient can therefore incur this side effect.

Copilot · 2026-01-15T04:01:27Z

test/utests/fakes/FakeFragmentCollector_MPD.cpp

 #include "fragmentcollector_mpd.h"
 #include "MockStreamAbstractionAAMP_MPD.h"

+TimeSyncClient::TimeSyncClient(){}


The fake TimeSyncClient constructor is empty, which means member variables (lastSync, lastOffset, hasSynced) are left uninitialized. This creates undefined behavior when tests access these members. The fake should either match the real implementation or explicitly initialize members to known values for deterministic test behavior.

Suggested change

TimeSyncClient::TimeSyncClient(){}

TimeSyncClient::TimeSyncClient()

: lastSync(0)

, lastOffset(0)

, hasSynced(false)

{

}

sayanurag requested a review from a team as a code owner January 14, 2026 13:58

Copilot AI review requested due to automatic review settings January 14, 2026 13:58

Copilot started reviewing on behalf of sayanurag January 14, 2026 13:58 View session

sayanurag force-pushed the feature/VPLAY-12359_UTCTiming branch from a1b2712 to 4781ef1 Compare January 14, 2026 14:01

Copilot AI reviewed Jan 14, 2026

View reviewed changes

sayanurag force-pushed the feature/VPLAY-12359_UTCTiming branch from 4781ef1 to d8c11d9 Compare January 14, 2026 14:05

Copilot AI review requested due to automatic review settings January 14, 2026 14:41

sayanurag force-pushed the feature/VPLAY-12359_UTCTiming branch from d8c11d9 to aaff6b2 Compare January 14, 2026 14:41

Copilot started reviewing on behalf of sayanurag January 14, 2026 14:41 View session

Copilot AI reviewed Jan 14, 2026

View reviewed changes

VPLAY-12359 suboptimal UTCTiming behavior

a8e0d9f

Reason for change: Updates UTCTiming logic to skip sync if interval hasn’t elapsed. Add Configs to control the interval Test Procedure: updated in ticket Risks: Low Signed-off-by: Anurag Krishnan <[email protected]>

sayanurag force-pushed the feature/VPLAY-12359_UTCTiming branch from aaff6b2 to a8e0d9f Compare January 14, 2026 15:03

Reason for Change:

e57c144

deprecated eAAMPConfig_UTCSyncOnErrorRetrySec fixed bug with config order causing runtime crash added some doxygen comments Signed-off-by: Philip Stroffolino <[email protected]>

Copilot AI review requested due to automatic review settings January 14, 2026 19:36

Copilot started reviewing on behalf of pstroffolino January 14, 2026 19:36 View session

Copilot AI reviewed Jan 14, 2026

View reviewed changes

fragmentcollector_mpd.h Outdated Show resolved Hide resolved

fragmentcollector_mpd.cpp Outdated Show resolved Hide resolved

AampConfig.cpp Outdated Show resolved Hide resolved

AampConfig.h Outdated Show resolved Hide resolved

Apply suggestions from code review

63776aa

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

Copilot AI review requested due to automatic review settings January 14, 2026 20:36

Copilot started reviewing on behalf of pstroffolino January 14, 2026 20:36 View session

Copilot AI reviewed Jan 14, 2026

View reviewed changes

fragmentcollector_mpd.cpp Outdated Show resolved Hide resolved

AampConfig.cpp Outdated Show resolved Hide resolved

fragmentcollector_mpd.h Outdated Show resolved Hide resolved

fragmentcollector_mpd.cpp Outdated Show resolved Hide resolved

pstroffolino and others added 2 commits January 14, 2026 15:44

Reason for Change: additional comments

2721698

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

Apply suggestions from code review

8f74660

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

Copilot AI review requested due to automatic review settings January 14, 2026 21:03

Copilot started reviewing on behalf of pstroffolino January 14, 2026 21:04 View session

Copilot AI reviewed Jan 14, 2026

View reviewed changes

fragmentcollector_mpd.h Outdated Show resolved Hide resolved

fragmentcollector_mpd.cpp Outdated Show resolved Hide resolved

fragmentcollector_mpd.cpp Outdated Show resolved Hide resolved

AampConfig.cpp Outdated Show resolved Hide resolved

pstroffolino and others added 3 commits January 14, 2026 16:18

Readon for Change: more better comments

e14c9ec

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

Reason for Change:

4c48ceb

Merge branch 'feature/VPLAY-12359_UTCTiming' of https://github.com/rdkcentral/aamp into feature/VPLAY-12359_UTCTiming Signed-off-by: Philip Stroffolino <[email protected]>

Apply suggestions from code review

7d42196

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

Copilot AI review requested due to automatic review settings January 14, 2026 21:30

Copilot started reviewing on behalf of pstroffolino January 14, 2026 21:31 View session

Apply suggestions from code review

517d732

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

Copilot AI review requested due to automatic review settings January 14, 2026 22:43

Copilot started reviewing on behalf of pstroffolino January 14, 2026 22:43 View session

Copilot AI reviewed Jan 14, 2026

View reviewed changes

pstroffolino and others added 2 commits January 14, 2026 17:52

Reason for Change: more nitpicks

12f9951

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

Merge branch 'dev_sprint_25_2' into feature/VPLAY-12359_UTCTiming

0746e42

Copilot AI review requested due to automatic review settings January 15, 2026 01:26

Copilot started reviewing on behalf of pstroffolino January 15, 2026 01:26 View session

Copilot AI reviewed Jan 15, 2026

View reviewed changes

pstroffolino and others added 2 commits January 14, 2026 21:33

Reason for Change: fix l1tests

4301e0e

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

Apply suggestions from code review

b2d00e2

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

Copilot AI review requested due to automatic review settings January 15, 2026 02:34

Copilot started reviewing on behalf of pstroffolino January 15, 2026 02:35 View session

Copilot AI reviewed Jan 15, 2026

View reviewed changes

Copilot AI mentioned this pull request Jan 15, 2026

Add interval-based throttling tests for FindServerUTCTime #858

Open

pstroffolino and others added 3 commits January 14, 2026 22:29

Reason for Test: new utests

e4cdd35

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

Merge branch 'feature/VPLAY-12359_UTCTiming' of https://github.com/rd…

80af1c4

…kcentral/aamp into feature/VPLAY-12359_UTCTiming

Update fragmentcollector_mpd.h

0d6425a

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

Copilot AI review requested due to automatic review settings January 15, 2026 03:55

Copilot started reviewing on behalf of pstroffolino January 15, 2026 03:56 View session

Copilot AI reviewed Jan 15, 2026

View reviewed changes

Update AampDefine.h

b3faaf0

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

Copilot AI review requested due to automatic review settings January 15, 2026 03:57

Copilot started reviewing on behalf of pstroffolino January 15, 2026 03:58 View session

Update fragmentcollector_mpd.cpp

79ac33a

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

pstroffolino merged commit 5a409f0 into dev_sprint_25_2 Jan 15, 2026
6 of 7 checks passed

pstroffolino deleted the feature/VPLAY-12359_UTCTiming branch January 15, 2026 03:59

Copilot AI reviewed Jan 15, 2026

View reviewed changes

		{eAAMPConfig_MaxFragmentChunkCached, DEFAULT_CACHED_FRAGMENT_CHUNKS_PER_TRACK},
		{eAAMPConfig_UTCSyncMinIntervalSec, DEFAULT_UTC_SYNC_MIN_INTERVAL_SEC}

-	 * @brief Constructor initializes lastSync with current time and resets other members.
+	 * @brief Constructor initializes lastSync with current time and resets other members.
+	 *
+	 * This initialization obtains the current time via a call to
+	 * aamp_GetCurrentTimeMS(), which may perform a system call to
+	 * query the system clock. Callers should be aware that constructing
+	 * TimeSyncClient can therefore incur this side effect.

-TimeSyncClient::TimeSyncClient(){}
+TimeSyncClient::TimeSyncClient()
+	: lastSync(0)
+	, lastOffset(0)
+	, hasSynced(false)
+{
+}

VPLAY-12359 suboptimal UTCTiming behavior #853

VPLAY-12359 suboptimal UTCTiming behavior #853

Uh oh!

Conversation

sayanurag commented Jan 14, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

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!

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!

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!

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!

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!

Uh oh!

Uh oh!

Copilot AI Jan 14, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI Jan 14, 2026

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!

Uh oh!

Copilot AI Jan 15, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!