Skip to content
Merged
Show file tree
Hide file tree
Changes from 9 commits
Commits
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
13 changes: 9 additions & 4 deletions AampConfig.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -368,7 +368,8 @@ static const ConfigLookupEntryBool mConfigLookupTableBool[AAMPCONFIG_BOOL_COUNT]
{false, "curlThroughput", eAAMPConfig_CurlThroughput, false },
{false, "useFireboltSDK", eAAMPConfig_UseFireboltSDK, false},
{true, "enableChunkInjection", eAAMPConfig_EnableChunkInjection, true},
{false, "debugChunkTransfer", eAAMPConfig_DebugChunkTransfer, false}
{false, "debugChunkTransfer", eAAMPConfig_DebugChunkTransfer, false},
{true, "utcSyncOnStartup", eAAMPConfig_UTCSyncOnStartup, true}
};

#define CONFIG_INT_ALIAS_COUNT 2
Expand Down Expand Up @@ -468,9 +469,13 @@ static const ConfigLookupEntryInt mConfigLookupTableInt[AAMPCONFIG_INT_COUNT+CON
{DEFAULT_MONITOR_AV_JUMP_THRESHOLD_MS,"monitorAVJumpThreshold",eAAMPConfig_MonitorAVJumpThreshold,true,eCONFIG_RANGE_MONITOR_AVSYNC_JUMP_THRESHOLD },
{DEFAULT_PROGRESS_LOGGING_DIVISOR,"progressLoggingDivisor",eAAMPConfig_ProgressLoggingDivisor,false},
{DEFAULT_MONITOR_AV_REPORTING_INTERVAL, "monitorAVReportingInterval", eAAMPConfig_MonitorAVReportingInterval, false},
// aliases, kept for backwards compatibility
{DEFAULT_UTC_SYNC_MIN_INTERVAL,"utcSyncMinIntervalSec",eAAMPConfig_UTCSyncMinIntervalSec,true },

// Add new integer config entries above this line, before the aliases section.
//
// Aliases, kept for backwards compatibility
{DEFAULT_INIT_BITRATE,"defaultBitrate",eAAMPConfig_DefaultBitrate,true },
{DEFAULT_INIT_BITRATE_4K,"defaultBitrate4K",eAAMPConfig_DefaultBitrate4K,true },
{DEFAULT_INIT_BITRATE_4K,"defaultBitrate4K",eAAMPConfig_DefaultBitrate4K,true }
};

/**
Expand All @@ -495,7 +500,7 @@ static const ConfigLookupEntryFloat mConfigLookupTableFloat[AAMPCONFIG_FLOAT_COU
{DEFAULT_NORMAL_RATE_CORRECTION_SPEED,"normalLatencyCorrectionPlaybackRate",eAAMPConfig_NormalLatencyCorrectionPlaybackRate,false},
{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}
Copy link

Copilot AI Jan 15, 2026

Choose a reason for hiding this comment

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

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 generated this review using guidance from repository custom instructions.
};

/**
Expand Down
2 changes: 2 additions & 0 deletions AampConfig.h
Original file line number Diff line number Diff line change
Expand Up @@ -214,6 +214,7 @@ typedef enum
eAAMPConfig_UseFireboltSDK, /**< Config to use Firebolt SDK for license Acquisition */
eAAMPConfig_EnableChunkInjection, /**< Config to enable chunk injection for low latency DASH */
eAAMPConfig_DebugChunkTransfer, /**< app-managed chunked transfer protocol */
eAAMPConfig_UTCSyncOnStartup, /**< Perform sync at startup */
eAAMPConfig_BoolMaxValue /**< Max value of bool config always last element */

} AAMPConfigSettingBool;
Expand Down Expand Up @@ -311,6 +312,7 @@ typedef enum
eAAMPConfig_MonitorAVJumpThreshold, /**< configures threshold aligned audio,video positions advancing together by unexpectedly large delta to be reported as jump in milliseconds*/
eAAMPConfig_ProgressLoggingDivisor, /**< Divisor to avoid printing the progress report too frequently in the log */
eAAMPConfig_MonitorAVReportingInterval, /**< Timeout in milliseconds for reporting MonitorAV events */
eAAMPConfig_UTCSyncMinIntervalSec, /**< Minimum interval between sync attempts */
eAAMPConfig_IntMaxValue /**< Max value of int config always last element*/
} AAMPConfigSettingInt;
#define AAMPCONFIG_INT_COUNT (eAAMPConfig_IntMaxValue)
Expand Down
2 changes: 2 additions & 0 deletions AampDefine.h
Original file line number Diff line number Diff line change
Expand Up @@ -143,6 +143,8 @@
#define DEFAULT_MONITOR_AV_JUMP_THRESHOLD_MS 100 /**< default jump threshold to MonitorAV reporting */
#define DEFAULT_MAX_DOWNLOAD_BUFFER 10 /**< Default maximum download buffer in seconds, this can be used to limit player download job scheduling for DASH */
#define DEFAULT_MONITOR_AV_REPORTING_INTERVAL 1000 /**< time interval in ms for MonitorAV reporting */
#define DEFAULT_UTC_SYNC_MIN_INTERVAL 60 /**< Minimum interval between sync attempts */


// We can enable the following once we have a thread monitoring video PTS progress and triggering subtec clock fast update when we detect video freeze. Disabled it for now for brute force fast refresh..
//#define SUBTEC_VARIABLE_CLOCK_UPDATE_RATE /* enable this to make the clock update rate dynamic*/
Expand Down
42 changes: 34 additions & 8 deletions fragmentcollector_mpd.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -4468,11 +4468,13 @@ void StreamAbstractionAAMP_MPD::FindPeriodGapsAndReport()
}
}

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

/**
* @brief Read UTCTiming _element_
* @retval Return true if UTCTiming _element_ is available in the manifest
*/
bool StreamAbstractionAAMP_MPD::FindServerUTCTime(Node* root)
bool StreamAbstractionAAMP_MPD::FindServerUTCTime(Node* root)
{
bool hasServerUtcTime = false;
if( root )
Expand Down Expand Up @@ -4505,16 +4507,40 @@ bool StreamAbstractionAAMP_MPD::FindServerUTCTime(Node* root)
aamp_ResolveURL(ServerUrl, aamp->GetManifestUrl(), valueCopy.c_str(), false);
}

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

Copilot AI Jan 14, 2026

Choose a reason for hiding this comment

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

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.

Copilot uses AI. Check for mistakes.
if( !shouldSyncOnStartup )
{
double currentTime = (double)aamp_GetCurrentTimeMS() / 1000;
mDeltaTime = mLocalUtcTime - currentTime;
hasServerUtcTime = true;
const double elapsed = (double)(aamp_GetCurrentTimeMS() - mTimeSyncClient.lastSync) / 1000;
Copy link

Copilot AI Jan 15, 2026

Choose a reason for hiding this comment

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

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.

Copilot uses AI. Check for mistakes.
Copy link

Copilot AI Jan 15, 2026

Choose a reason for hiding this comment

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

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 generated this review using guidance from repository custom instructions.
intervalElapsed = elapsed >= GETCONFIGVALUE(eAAMPConfig_UTCSyncMinIntervalSec);
}
else
if (shouldSyncOnStartup || intervalElapsed)
{
mLocalUtcTime = GetNetworkTime(ServerUrl, &http_error, aamp->GetNetworkProxy());
if(mLocalUtcTime > 0)
{
mTimeSyncClient.lastSync = aamp_GetCurrentTimeMS();
mDeltaTime = mLocalUtcTime - (double)mTimeSyncClient.lastSync / 1000;
Copy link

Copilot AI Jan 15, 2026

Choose a reason for hiding this comment

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

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

Copilot generated this review using guidance from repository custom instructions.
mTimeSyncClient.lastOffset = mDeltaTime;
mTimeSyncClient.hasSynced = true;
hasServerUtcTime = true;
}
else
{
if (!mTimeSyncClient.hasSynced)
{
AAMPLOG_ERR("Failed initial read of timeServer [%s] RetCode[%d]", ServerUrl.c_str(), http_error);
}
else
{
AAMPLOG_WARN("Failed to refresh timeServer [%s] RetCode[%d]", ServerUrl.c_str(), http_error);
}
}
Comment on lines 4528 to 4538
Copy link

Copilot AI Jan 14, 2026

Choose a reason for hiding this comment

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

Potential issue: If the first sync attempt fails (mLocalUtcTime <= 0), and this happens during startup with eAAMPConfig_UTCSyncOnStartup enabled, the code will not set hasServerUtcTime to true. However, on subsequent manifest refreshes, if hasSynced is still false and UTCSyncOnStartup is true, the code will attempt sync again. But if the startup sync is disabled or the interval hasn't elapsed, the player will operate without UTC sync indefinitely until an interval elapses. Consider whether a retry mechanism or fallback to system time should be added for the initial sync failure case.

Copilot uses AI. Check for mistakes.
}
else if(mTimeSyncClient.hasSynced)
{
AAMPLOG_ERR("Failed to read timeServer [%s] RetCode[%d]",ServerUrl.c_str(),http_error);
mDeltaTime = mTimeSyncClient.lastOffset;
hasServerUtcTime = true;
}
break;
}
Expand Down
40 changes: 40 additions & 0 deletions fragmentcollector_mpd.h
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,45 @@ struct ProfileInfo
int representationIndex;
};

/**
* @struct TimeSyncClient
*
* @brief Maintains state for periodic synchronization of the local clock
* with a remote UTC time server, used in DASH manifest processing.
*
* This struct tracks the last successful synchronization time and the
* cached offset between the local system clock and the server's UTC time.
* It supports logic to determine when a new synchronization request should
* be made based on elapsed time and configuration.
*
* Members:
* - lastSync: Timestamp (milliseconds since epoch) of the last successful sync.
* - lastOffset: Cached time delta (in seconds) between local and server time.
* - hasSynced: Flag indicating whether at least one successful sync has occurred.
*/
struct TimeSyncClient
Copy link

Copilot AI Jan 14, 2026

Choose a reason for hiding this comment

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

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

Copilot generated this review using guidance from repository custom instructions.
{
/**
* @brief UTC time in milliseconds since epoch when time was last synchronized with time server.
*/
long long lastSync;

/**
* @brief Current delta (seconds) between local and server time.
*/
double lastOffset;

/**
* @brief True if time has been synchronized at least once.
*/
bool hasSynced;

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

Copilot AI Jan 15, 2026

Choose a reason for hiding this comment

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

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 uses AI. Check for mistakes.
*/
TimeSyncClient();
};

class AampDashWorkerJob : public aamp::AampTrackWorkerJob
{
private:
Expand Down Expand Up @@ -1244,6 +1283,7 @@ class StreamAbstractionAAMP_MPD : public StreamAbstractionAAMP
bool mShortAdOffsetCalc;
AampTime mNextPts; /*For PTS restamping*/
bool mIsFinalFirstPTS; /**< Flag to indicate if the first PTS is final or not */
TimeSyncClient mTimeSyncClient;
};

#endif //FRAGMENTCOLLECTOR_MPD_H_
Expand Down
Loading