VPLAY-12274 ewma algorithm testing

[pstroffolino]

VPLAY-12274 ewma algorithm testing

Reason for Change: test code for profiling abr algorithms

[Copilot]

Pull request overview

This PR adds a new standalone throughput estimator tool for profiling and testing EWMA (Exponentially Weighted Moving Average) algorithms used in Adaptive Bitrate (ABR) streaming. The tool downloads media segments repeatedly to collect baseline performance data and test throughput estimation algorithms.

Key changes:

• Implementation of EWMA-based throughput estimation for ABR
• Mid-download underflow prediction logic
• Performance profiling capabilities for network downloads

Reviewed changes

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

Show a summary per file

File	Description
throughput_estimator/throughput_estimator/main.cpp	New C++17 profiling tool implementing EWMA throughput estimation, transfer statistics tracking, and mid-download abort logic for testing ABR algorithms
throughput_estimator/throughput_estimator.xcodeproj/xcuserdata/.../*.plist	User-specific Xcode configuration files (should not be committed)
throughput_estimator/throughput_estimator.xcodeproj/xcuserdata/.../Breakpoints_v2.xcbkptlist	User-specific Xcode breakpoint configuration (should not be committed)
throughput_estimator/throughput_estimator.xcodeproj/project.xcworkspace/xcuserdata/.../UserInterfaceState.xcuserstate	User-specific Xcode UI state file (should not be committed)
throughput_estimator/throughput_estimator.xcodeproj/xcshareddata/xcschemes/throughput_estimator.xcscheme	Xcode build scheme configuration for the throughput_estimator target
throughput_estimator/throughput_estimator.xcodeproj/project.xcworkspace/contents.xcworkspacedata	Xcode workspace metadata file
throughput_estimator/throughput_estimator.xcodeproj/project.pbxproj	Xcode project file defining build settings, targets, and dependencies

Files not reviewed (1)

• throughput_estimator/throughput_estimator.xcodeproj/project.xcworkspace/contents.xcworkspacedata: Language not supported

[Copilot]

Pull request overview

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

Files not reviewed (1)

• throughput_estimator/throughput_estimator.xcodeproj/project.xcworkspace/contents.xcworkspacedata: Language not supported

[Copilot]

Pull request overview

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

Files not reviewed (1)

• throughput_estimator/throughput_estimator.xcodeproj/project.xcworkspace/contents.xcworkspacedata: Language not supported

[Copilot]

Pull request overview

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

Files not reviewed (1)

• throughput_estimator/throughput_estimator.xcodeproj/project.xcworkspace/contents.xcworkspacedata: Language not supported

[Copilot]

Pull request overview

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

Files not reviewed (1)

• throughput_estimator/throughput_estimator.xcodeproj/project.xcworkspace/contents.xcworkspacedata: Language not supported

[Copilot]

The DownloadContext class stores a raw FILE pointer as a member without ownership semantics. This violates RAII principles and makes the class difficult to use safely. The class should either own the FILE* (and close it in the destructor) or use a non-owning observer pattern with clear documentation. Consider using std::ofstream instead or at minimum, document the ownership semantics clearly.

[Copilot]

Pull request overview

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

Files not reviewed (1)

• throughput_estimator/throughput_estimator.xcodeproj/project.xcworkspace/contents.xcworkspacedata: Language not supported

[Copilot]

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

[Copilot]

Pull request overview

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

Files not reviewed (1)

• throughput_estimator/throughput_estimator.xcodeproj/project.xcworkspace/contents.xcworkspacedata: Language not supported

[Copilot]

The copyright year should be 2026 to match the other new files in this PR. Currently shows 2023.

Suggested change

	# Copyright 2023 RDK Management
	# Copyright 2026 RDK Management

[Copilot]

The 'LastUpgradeCheck' is set to 2620, which appears to be an invalid Xcode version number. Xcode versions follow a pattern like 1420 (for Xcode 14.2). This should be corrected to a valid version number.

Suggested change

	LastUpgradeCheck = 2620;
	LastUpgradeCheck = 1420;

[Copilot]

This comment contains inconsistent formatting. There's a space after the opening brace on line 83 ("{ // compute derived values"), but on line 64 the same pattern uses a different style. Consider using consistent comment placement throughout the file - either always on the same line after braces or always on a separate line before.

Suggested change

	{ // compute derived values
	{
	// compute derived values

[Copilot]

The macOS deployment target is set to 26.2 in the Release configuration as well, which is invalid. This should be a proper macOS version number like 11.0 or later.

Suggested change

	MACOSX_DEPLOYMENT_TARGET = 26.2;
	MACOSX_DEPLOYMENT_TARGET = 11.0;

[Copilot]

The variable 'BLEND_WEIGHT_HARMONIC' should be declared as 'constexpr' instead of 'const' since it's a compile-time constant. This is a C++11 best practice that allows the value to be used in more contexts and communicates intent more clearly.

[Copilot]

The global file stream 'f_EWMA' violates the principle of avoiding global mutable state. Consider refactoring this to be passed as a parameter or encapsulated in a class to improve testability and maintainability.

[Copilot]

Pull request overview

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

Files not reviewed (1)

• throughput_estimator/throughput_estimator.xcodeproj/project.xcworkspace/contents.xcworkspacedata: Language not supported

[Copilot]

Missing test coverage for boundary conditions. The MedianTest should include edge cases such as negative values, very large values, NaN, and infinity to ensure robust handling of edge cases in the GetMedian function.

[Copilot]

Empty test main function. The NetworkBandwidthEstimatorTests.cpp file only contains a test main() function but no actual test cases. Either add test cases to this file or remove it if all tests are in FunctionalTests.cpp, as Google Test can auto-generate the main function.

[Copilot]

Avoid reinterpret_cast when static_cast is sufficient. The conversion from void* to DownloadContext* should use static_cast instead of reinterpret_cast, as the pointer type is known at compile time and this cast is type-safe.

Suggested change

	DownloadContext *context = reinterpret_cast<DownloadContext*>(clientp);
	DownloadContext *context = static_cast<DownloadContext*>(clientp);

[Copilot]

Hardcoded development team identifier. The DEVELOPMENT_TEAM is set to "C9XTLYAG8J" which appears to be a personal or specific team identifier. This should either be removed to allow developers to use their own team settings, or moved to a local configuration file that is not committed to version control.

Suggested change

DEVELOPMENT_TEAM = C9XTLYAG8J;

[Copilot]

Duplicate comment detected. This line is identical to line 111 and should be removed to maintain clean documentation.

Suggested change

// Harmonic mean over last N samples.

[Copilot]

Inconsistent namespace qualification. The function uses std::size_t for some variables (lines 61, 62) but size_t without std:: for others (line 119, 121, 122). Maintain consistency by using std::size_t throughout the function.

[Copilot]

Avoid reinterpret_cast when static_cast is sufficient. The conversion from void* to DownloadContext* should use static_cast instead of reinterpret_cast, as the pointer type is known at compile time and this cast is type-safe.

Suggested change

	DownloadContext *context = reinterpret_cast<DownloadContext*>(clientp);
	DownloadContext *context = static_cast<DownloadContext*>(clientp);

[Copilot]

The vector is resized unnecessarily. Using emplace_back followed by erase when history exceeds MAX_HISTORY is inefficient. Consider using a circular buffer or std::deque, or at minimum, check the size before emplacing to avoid the grow-then-shrink pattern on every insertion once the history is full.

Suggested change

	m_history.emplace_back(sample);
	if (m_history.size() > MAX_HISTORY)
	{ // Trim history to avoid unbounded growth
	m_history.erase(m_history.begin(), m_history.begin() + (m_history.size() - MAX_HISTORY));
	}
	// Trim history to avoid unbounded growth before inserting new sample
	if( m_history.size() == MAX_HISTORY )
	{
	m_history.erase(m_history.begin());
	}
	m_history.emplace_back(sample);

[Copilot]

Prefer std::size_t over size_t for consistency with modern C++ style. While both are equivalent, std::size_t makes the namespace explicit and is preferred in modern C++ codebases.

[Copilot]

C-style array size calculation is error-prone. Replace sizeof(test_data)/sizeof(test_data[0]) with std::size(test_data) from <iterator> (C++17) or use a range-based for loop to iterate over the array directly, which is safer and more modern.

Suggested change

	for( int i=0; i<sizeof(test_data)/sizeof(test_data[0]); i++ )
	{
	EXPECT_NEAR( test_data[i].timeToFirstByteSeconds, networkBandwidthEstimator.GetTimeToFirstByteSeconds(), epsilon );
	EXPECT_NEAR( test_data[i].throughputBytesPerSecond, networkBandwidthEstimator.GetThroughputBytesPerSecond(), epsilon );
	EXPECT_NEAR( test_data[i].predictedDownloadTimeSeconds, networkBandwidthEstimator.GetPredictedDownloadTimeSeconds(segment_size_bytes), epsilon );
	CurlInfo curlInfo;
	curlInfo.m_size_download_bytes = test_data[i].download_bytes;
	curlInfo.m_total_time_seconds = test_data[i].total_time_seconds;
	curlInfo.m_time_to_first_byte_seconds = test_data[i].time_to_first_byte_seconds;
	for( const auto &data : test_data )
	{
	EXPECT_NEAR( data.timeToFirstByteSeconds, networkBandwidthEstimator.GetTimeToFirstByteSeconds(), epsilon );
	EXPECT_NEAR( data.throughputBytesPerSecond, networkBandwidthEstimator.GetThroughputBytesPerSecond(), epsilon );
	EXPECT_NEAR( data.predictedDownloadTimeSeconds, networkBandwidthEstimator.GetPredictedDownloadTimeSeconds(segment_size_bytes), epsilon );
	CurlInfo curlInfo;
	curlInfo.m_size_download_bytes = data.download_bytes;
	curlInfo.m_total_time_seconds = data.total_time_seconds;
	curlInfo.m_time_to_first_byte_seconds = data.time_to_first_byte_seconds;

[Copilot]

Pull request overview

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

[Copilot]

The member variable names use m_ prefix with snake_case (m_ewma_short_window_weight, m_ewma_bytes_per_second, m_dltotal, m_dlnow_prev, m_time_prev). According to the coding guidelines, member variables should use the m prefix with camelCase. Consider renaming to: mEwmaShortWindowWeight, mEwmaBytesPerSecond, mDlTotal, mDlNowPrev, mTimePrev.

Suggested change

	static constexpr double m_ewma_short_window_weight = 0.4;
	double m_ewma_bytes_per_second = 0.0;
	std::size_t m_dltotal = 0;
	std::size_t m_dlnow_prev = 0;
	double m_time_prev = 0.0;
	static constexpr double mEwmaShortWindowWeight = 0.4;
	double mEwmaBytesPerSecond = 0.0;
	std::size_t mDlTotal = 0;
	std::size_t mDlNowPrev = 0;
	double mTimePrev = 0.0;

[Copilot]

The local variable names use snake_case (delta_bytes, delta_time) which is inconsistent with the project's camelCase convention for variables. According to the coding guidelines, variables should use camelCase. Consider renaming to deltaBytes and deltaTime for consistency.

Suggested change

	const size_t delta_bytes = dlnow - m_dlnow_prev;
	const double delta_time = now - m_time_prev;
	m_dltotal = dltotal;
	if( delta_bytes > 0 )
	{ // some data has trickled in
	if( delta_time > epsilon )
	{
	const double Bps = static_cast<double>(delta_bytes)/delta_time;
	const size_t deltaBytes = dlnow - m_dlnow_prev;
	const double deltaTime = now - m_time_prev;
	m_dltotal = dltotal;
	if( deltaBytes > 0 )
	{ // some data has trickled in
	if( deltaTime > epsilon )
	{
	const double Bps = static_cast<double>(deltaBytes) / deltaTime;

[Copilot]

The local variable names payloadBytesPerSecond and EWMA_min use inconsistent casing. According to the coding guidelines, variables should use camelCase. Consider renaming EWMA_min to ewmaMin for consistency with the camelCase convention.

Suggested change

	double EWMA_min = (m_EWMA_fast_BytesPerSecond > 0.0 && m_EWMA_slow_BytesPerSecond > 0.0)
	? std::min(m_EWMA_fast_BytesPerSecond, m_EWMA_slow_BytesPerSecond)
	: std::max(m_EWMA_fast_BytesPerSecond, m_EWMA_slow_BytesPerSecond);
	if( EWMA_min > 0.0 )
	{
	if (m_harmonic_BytesPerSecond > 0.0)
	{
	return BLEND_WEIGHT_HARMONIC * m_harmonic_BytesPerSecond + (1.0 - BLEND_WEIGHT_HARMONIC) * EWMA_min;
	}
	else
	{
	return EWMA_min;
	double ewmaMin = (m_EWMA_fast_BytesPerSecond > 0.0 && m_EWMA_slow_BytesPerSecond > 0.0)
	? std::min(m_EWMA_fast_BytesPerSecond, m_EWMA_slow_BytesPerSecond)
	: std::max(m_EWMA_fast_BytesPerSecond, m_EWMA_slow_BytesPerSecond);
	if( ewmaMin > 0.0 )
	{
	if (m_harmonic_BytesPerSecond > 0.0)
	{
	return BLEND_WEIGHT_HARMONIC * m_harmonic_BytesPerSecond + (1.0 - BLEND_WEIGHT_HARMONIC) * ewmaMin;
	}
	else
	{
	return ewmaMin;

[Copilot]

The empty constructor body is unnecessary. Since the constructor does nothing, it can be removed entirely or declared with = default in the class definition, which is more explicit about its intent and follows modern C++ best practices.

Suggested change

	FunctionalTests()
	{
	}
	};
	FunctionalTests() = default;
	};

[Copilot]

The CurlInfo struct uses built-in types (std::size_t and double) instead of fixed-width integer types. According to the coding guidelines, when defining data structures that may be accessed from other languages (like Python's ctypes), use fixed-width integer types from cstdint header (e.g., uint64_t, uint32_t) to ensure consistent memory layout across platforms. Consider using uint64_t for m_size_download_bytes if this structure might be used for cross-language interoperability.

[Copilot]

The variable names in this test use snake_case (test_data, download_bytes, total_time_seconds, time_to_first_byte_seconds) which is inconsistent with the project's camelCase convention for variables. According to the coding guidelines, variables should use camelCase. Consider renaming to testData, downloadBytes, totalTimeSeconds, timeToFirstByteSeconds.

[Copilot]

Using C-style array size calculation with sizeof is error-prone and not modern C++. The code uses sizeof(test_data)/sizeof(test_data[0]) which can be replaced with std::size(test_data) (C++17) or the simpler range-based for loop. Consider refactoring to: for(const auto& test : test_data) to improve readability and type safety.

[pstroffolino]

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

[Copilot]

The constructor and destructor declarations are redundant. Since both are defaulted in the implementation file (lines 217, 219), you can use the = default syntax directly in the header or simply omit them if they would be implicitly generated with the same behavior. This reduces unnecessary declarations and improves maintainability.

Suggested change

	DownloadContext();
	~DownloadContext();
	DownloadContext() = default;
	~DownloadContext() = default;

[Copilot]

The member variable names use m_ prefix with snake_case or inconsistent patterns (m_history, m_estimated_TTFB_seconds, m_EWMA_fast_BytesPerSecond, m_EWMA_slow_BytesPerSecond, m_harmonic_BytesPerSecond). According to the coding guidelines, member variables should use the m prefix with camelCase. Consider standardizing to: mHistory, mEstimatedTtfbSeconds, mEwmaFastBytesPerSecond, mEwmaSlowBytesPerSecond, mHarmonicBytesPerSecond.

Suggested change

	std::vector<Sample> m_history;
	// Robust per-request overhead Time to First Byte (TTFB) estimate
	double m_estimated_TTFB_seconds = 0.0; // median TTFB - computed brute force
	// Robust throughput estimates (bytes/s)
	double m_EWMA_fast_BytesPerSecond = 0.0; // reacts quickly
	double m_EWMA_slow_BytesPerSecond = 0.0; // stable
	double m_harmonic_BytesPerSecond = 0.0; // conservative
	std::vector<Sample> mHistory;
	// Robust per-request overhead Time to First Byte (TTFB) estimate
	double mEstimatedTtfbSeconds = 0.0; // median TTFB - computed brute force
	// Robust throughput estimates (bytes/s)
	double mEwmaFastBytesPerSecond = 0.0; // reacts quickly
	double mEwmaSlowBytesPerSecond = 0.0; // stable
	double mHarmonicBytesPerSecond = 0.0; // conservative

[Copilot]

The local variable names use snake_case (payload_bytes_per_second) which is inconsistent with the project's camelCase convention for variables. According to the coding guidelines, variables should use camelCase. Consider renaming to payloadBytesPerSecond for consistency.

[Copilot]

Pull request overview

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

[Copilot]

There is a duplicate comment on line 112. The comment "Harmonic mean over last N samples" appears on both line 111 and line 112. Remove the duplicate on line 111.

Suggested change

// Harmonic mean over last N samples

[Copilot]

The includes for iostream, cstdio, cstdlib, ctime, and string appear to be unused in this file. Unused includes should be removed to minimize compilation dependencies and improve build times.

Suggested change

	#include <cstdio>
	#include <cstdlib>
	#include <iostream>
	#include <numeric>
	#include <optional>
	#include <string>
	#include <ctime>
	#include <numeric>
	#include <optional>

[Copilot]

The function signature uses 'void' in the parameter list, which is C-style syntax. In modern C++, empty parentheses should be used instead. Change 'GetCurrentTimeMonotonicSeconds( void )' to 'GetCurrentTimeMonotonicSeconds()'.

Suggested change

	double GetCurrentTimeMonotonicSeconds( void );
	double GetCurrentTimeMonotonicSeconds();

[Copilot]

The member variables use raw 'size_t' type. For structures that may be used across language boundaries or need guaranteed sizes, consider using fixed-width types from <cstdint> such as 'uint64_t' instead of 'size_t' to ensure consistent memory layout across platforms.

[Copilot]

The test uses the DownloadContext class defined in the production code being tested, but the xferinfo callback function and related test infrastructure reference DownloadContext without showing how it's instantiated or used in the actual production code. The test appears to be testing implementation rather than behavior through the public API. Consider whether this test should be structured differently to test the component's public interface more directly.

[Copilot]

Pull request overview

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

[Copilot]

Consider using auto for type deduction: auto bytesPerSecond = static_cast(delta_bytes)/delta_time;

[Copilot]

Consider using auto for type deduction: auto upper = values[mid]; auto lower = values[mid - 1];

[Copilot]

Consider using auto for type deduction to improve readability: auto ewma_min = ...

[Copilot]

Missing Doxygen documentation for the clientp parameter. All parameters should be documented with @param tags for consistency with the project's documentation standards.

[Copilot]

Consider using auto for type deduction: auto remaining_bytes = m_dltotal - m_dlnow_prev;

[Copilot]

Consider using auto for type deduction: auto payloadBytesPerSecond = m_history[i].GetPayloadBytesPerSecond();

[Copilot]

Consider using a range-based for loop instead of index-based iteration for better readability and to follow modern C++ practices. For example: for (const auto& data : test_data) { /* use data instead of test_data[i] */ }

[Copilot]

The naming convention violates the project's C++ standards. Member variables should be prefixed with 'm' (e.g., mCurlInfo) rather than using m_ prefix. The project uses camelCase for member variables with 'm' prefix, not snake_case with m_ prefix.

[Copilot]

The constructor should be marked as explicit to prevent implicit conversions, following modern C++ best practices. Change to: explicit Sample(const CurlInfo &curlInfo);

[Copilot]

Consider using auto for type deduction: auto throughput = GetThroughputBytesPerSecond();