VPLAY-12274 standalone tool to measure CDN performant tool

[pstroffolino]

VPLAY-12274 standalone tool to measure CDN performant tool

Reason for Change: Split off from new class and l1 tests introduced in same ticket, this adds a simple test app that repeatedly downloads a given segment to build up set of recorded download timings that can be used for analysis and simulation.

[Copilot]

Pull request overview

This PR introduces a standalone CDN performance measurement tool along with a new NetworkBandwidthEstimator class for adaptive bitrate (ABR) streaming. The tool repeatedly downloads segments to collect timing data for analysis and simulation of network conditions.

Key Changes:

• New NetworkBandwidthEstimator class implementing EWMA-based throughput estimation with harmonic mean blending
• Standalone throughput_estimator tool for CDN performance measurement
• Comprehensive L1 functional tests validating median calculation, throughput prediction, and mid-download monitoring

Reviewed changes

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

Show a summary per file

File	Description
abr/NetworkBandwidthEstimator.h	Header defining bandwidth estimation classes with EWMA filters and download context tracking
abr/NetworkBandwidthEstimator.cpp	Implementation of throughput estimation algorithms using median TTFB and harmonic mean blending
throughput_estimator/main.cpp	Standalone tool using libcurl to repeatedly download segments and generate CSV performance data
test/utests/tests/NetworkBandwidthEstimator/FunctionalTests.cpp	Comprehensive tests for median calculation, throughput prediction, and download monitoring
test/utests/tests/NetworkBandwidthEstimator/NetworkBandwidthEstimatorTests.cpp	Google Test main entry point for the test suite
test/utests/tests/NetworkBandwidthEstimator/CMakeLists.txt	Build configuration for NetworkBandwidthEstimator unit tests
test/utests/tests/CMakeLists.txt	Updated to include NetworkBandwidthEstimator test subdirectory
CMakeLists.txt	Added NetworkBandwidthEstimator source files to main build

[Copilot]

Using sizeof with an array divided by sizeof of element to compute array size is a legacy C pattern. Modern C++ should use std::size() or define the array with std::array to get compile-time size checking.

[Copilot]

The variable name 'f_EWMA' does not follow modern C++ naming conventions. According to project guidelines, it should use camelCase for variables. Consider renaming to 'ewmaFile' or 'fEwma'.

[Copilot]

The variable name 'f_ABR' does not follow modern C++ naming conventions. According to project guidelines, it should use camelCase for variables. Consider renaming to 'abrFile' or 'fAbr'.

Suggested change

	std::ofstream f_ABR(pathABR);
	if( !f_ABR )
	{
	std::cerr << "unable to open " << pathABR << "\n";
	return EXIT_FAILURE;
	}
	f_ABR << std::fixed << std::setprecision(16);
	std::ofstream abrFile(pathABR);
	if( !abrFile )
	{
	std::cerr << "unable to open " << pathABR << "\n";
	return EXIT_FAILURE;
	}
	abrFile << std::fixed << std::setprecision(16);

[Copilot]

The clientp parameter should be checked for nullptr before dereferencing. While unlikely in this controlled context, defensive programming is important for robustness.

Suggested change

	{
	{
	if (!clientp)
	{
	// No download context available; skip progress handling
	return 0;
	}

[Copilot]

The return value of std::strtoull should be validated for errors. Check errno or compare the result with ULLONG_MAX to handle conversion errors properly. Additionally, the nullptr check for the second parameter is not sufficient for error detection.

[Copilot]

Unused parameters should be removed from the function signature or marked with the [[maybe_unused]] attribute to clarify intent. The parameters ptr and userdata are explicitly cast to void but not used.

Suggested change

	static size_t write_callback(char *ptr, size_t size, size_t nmemb, void *userdata)
	{ // stub to avoid spewing download contents to console log
	(void)ptr;
	(void)userdata;
	static size_t write_callback([[maybe_unused]] char *ptr, size_t size, size_t nmemb, [[maybe_unused]] void *userdata)
	{ // stub to avoid spewing download contents to console log

[Copilot]

This global variable should be avoided. Global state makes the code harder to test and reason about. Consider passing this as a parameter to the xferinfo callback through the clientp pointer, or encapsulating it within a context structure.

[Copilot]

Missing cleanup of curl handle. If curl_easy_init fails, the function returns EXIT_FAILURE without closing the file streams f_EWMA and f_ABR that were successfully opened.

Suggested change

	std::cerr << "curl_easy_init failed\n";
	std::cerr << "curl_easy_init failed\n";
	f_ABR.close();
	f_EWMA.close();
	return EXIT_FAILURE;

[Copilot]

The use of C-style casts with reinterpret_cast can be error-prone. Consider using static_cast if the pointer type is known, or add a nullptr check before the cast for safety.

Suggested change

	DownloadContext *context = reinterpret_cast<DownloadContext*>(clientp);
	if (clientp == nullptr)
	{
	return 0; // continue transfer; no context available
	}
	DownloadContext *context = static_cast<DownloadContext*>(clientp);

[Copilot]

The copyright year 2023 appears outdated for new code added in 2025/2026. Consider updating to the current year to accurately reflect when this test was created.

Suggested change

	# Copyright 2023 RDK Management
	# Copyright 2026 RDK Management

[Copilot]

Pull request overview

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

[Copilot]

The file stream f_EWMA is opened but there's no guarantee it will be properly closed if an error occurs later in the loop. While std::ofstream's destructor will close it, the global nature means it stays open across iterations. Consider using RAII by scoping the file stream within a block or using std::unique_ptr with a custom deleter to ensure proper resource cleanup in all execution paths.

[Copilot]

The write_callback function parameters use void casts to suppress unused parameter warnings. Modern C++ offers clearer alternatives: use [[maybe_unused]] attribute or simply omit parameter names. This is consistent with how the test file handles the same situation.

[Copilot]

Global mutable state should be avoided as it creates tight coupling and makes testing difficult. The global std::ofstream f_EWMA is used across multiple functions, violating the principle of encapsulation. Consider passing file streams as parameters to functions or encapsulating this functionality within a class that manages the file I/O. This would improve testability and make the code's dependencies more explicit.

[Copilot]

The hardcoded URLs and sizes in the variation array appear to be Sky CDN production URLs for specific content. This is problematic for several reasons: 1) Production URLs may change or become unavailable, breaking the tool. 2) It's unclear if this creates load on production infrastructure. 3) It's not configurable for other use cases. Consider making these configurable via command-line arguments, a configuration file, or environment variables to improve the tool's flexibility and avoid hardcoding external dependencies.

Suggested change

	};
	};
	// Allow overriding the default URLs and sizes via environment variables.
	// For each representation "LE<n>", the following variables are consulted:
	// THROUGHPUT_URL_LE<n> - override the download URL
	// THROUGHPUT_SIZE_LE<n> - override the expected segment size in bytes
	for( int iRepOverride = 0; iRepOverride < 6; iRepOverride++ )
	{
	const char *repId = variation[iRepOverride].rep;
	if( repId != NULL )
	{
	std::string urlEnvName = std::string("THROUGHPUT_URL_") + repId;
	std::string sizeEnvName = std::string("THROUGHPUT_SIZE_") + repId;
	const char *urlEnvValue = std::getenv( urlEnvName.c_str() );
	if( urlEnvValue != NULL && urlEnvValue[0] != '\0' )
	{
	// Use URL from environment instead of the compiled-in default.
	variation[iRepOverride].url = urlEnvValue;
	}
	const char *sizeEnvValue = std::getenv( sizeEnvName.c_str() );
	if( sizeEnvValue != NULL && sizeEnvValue[0] != '\0' )
	{
	char *endPtr = NULL;
	unsigned long long parsedSize = std::strtoull( sizeEnvValue, &endPtr, 10 );
	if( endPtr != sizeEnvValue && *endPtr == '\0' && parsedSize > 0ULL )
	{
	variation[iRepOverride].size = static_cast<size_t>( parsedSize );
	}
	}
	}
	}

[Copilot]

The magic number 30 for iteration_count should be a named constant or configurable parameter. This would make the tool more flexible for different testing scenarios and make the code's intent clearer. Consider using a const or constexpr variable with a descriptive name like DEFAULT_ITERATION_COUNT or making it configurable via command-line argument.

[Copilot]

The variable 'path' is read from the environment but never validated for null before use on line 121. If the environment variable 'outpath' is not set, this will lead to undefined behavior when constructing the std::string. Add validation to check if path is null and provide an appropriate error message.

Suggested change

	const char* path = std::getenv("outpath");
	const char* path = std::getenv("outpath");
	if (path == nullptr || path[0] == '\0')
	{
	std::cerr << "Error: environment variable 'outpath' is not set or is empty. "
	<< "Please set 'outpath' to a valid output directory or file path."
	<< std::endl;
	return EXIT_FAILURE;
	}

[Copilot]

Commented-out code should be removed. If this alternative implementation for reading configuration from environment variables is no longer needed, delete it. If it might be useful in the future, document why in a comment and consider using conditional compilation or a command-line argument approach instead.

Suggested change

	/*
	const char* size_env = std::getenv("size");
	if (!path || !url || !size_env)
	{
	std::cerr << "Please set env: outpath, url, and size\n";
	return EXIT_FAILURE;
	}
	const size_t segment_size_bytes = static_cast<size_t>(std::strtoull(size_env, nullptr, 10));
	if (segment_size_bytes == 0)
	{
	std::cerr << "Invalid 'size' value\n";
	return EXIT_FAILURE;
	}
	*/

[Copilot]

The magic number 6 is repeated in the loop condition and in the variation array size. Use the array size directly (std::size(variation) or sizeof(variation)/sizeof(variation[0])) or declare it as a named constant to ensure consistency and prevent bugs if the array size changes.

[Copilot]

The error message is missing a return statement. When curl_easy_init() fails, the code prints an error but continues execution, leading to undefined behavior as curl will be nullptr. Add 'return EXIT_FAILURE;' after the error message to properly terminate the program.

Suggested change

	std::cerr << "curl_easy_init failed\n";
	std::cerr << "curl_easy_init failed\n";
	return EXIT_FAILURE;

[Copilot]

The anonymous struct used for the variation array should be given a proper name or replaced with a std::array of a named struct type. This improves code clarity, enables reuse, and follows modern C++ best practices. Consider defining a struct like 'RepresentationVariant' or 'TestSegment' with clear semantic meaning.