VPLAY-11433 : Enhancement Request: Utilize Existing Player Source Fie…

[molakalapalliharipriya]

…ld in PCTune_split to Indicate VIPA vs Non-VIPA Playback

Reason for Change : To better collect and analyze tune metrics, use the existing player source string field to differentiate VIPA Playback

Test Procedure: Tune to linear/VOD content , verify Tune log
Priority : p1
Risks: Low

[Copilot]

Pull request overview

This pull request implements VIPA (Video IP Accelerator) playback tagging in tune metrics to differentiate between VIPA and non-VIPA playback sessions. When the Firebolt SDK is enabled (via eAAMPConfig_UseFireboltSDK), the string "_VIPA" is appended to the application name in tune metric logs and JSON output.

Changes:

• Modifies TuneEnd() to append "_VIPA" suffix to appName when Firebolt SDK is enabled
• Increases buffer size for tune time prefix string to accommodate the additional suffix
• Adds comprehensive test coverage for VIPA tagging functionality

Reviewed changes

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

File	Description
AampProfiler.cpp	Adds conditional "_VIPA" suffix to appName based on Firebolt SDK configuration; increases buffer size from 64 to 128 bytes
test/utests/tests/AampProfilerTests/AampProfilerTests.cpp	Adds three test cases validating VIPA tagging behavior when enabled, disabled, and with null configuration

[Copilot]

Pull request overview

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

[Copilot]

Pull request overview

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

[Copilot]

Pull request overview

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

[Copilot]

Pull request overview

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

[Copilot]

Pull request overview

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

[Copilot]

Pull request overview

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

[Copilot]

Consider using NiceMock instead of plain MockAampConfig. The codebase convention in similar test fixtures (e.g., AampAbrTests, MediaStreamContextTests) is to use NiceMock to suppress warnings about uninteresting mock function calls. Change the declaration to: NiceMock mockConfig;

Suggested change

	MockAampConfig mockConfig;
	NiceMock<MockAampConfig> mockConfig;

[Copilot]

The test creates a local MockAampConfig on the stack but sets g_mockAampConfig to point to it. When the test exits, the local mockConfig is destroyed, leaving g_mockAampConfig pointing to deallocated stack memory. This is a use-after-free bug. The mock should be allocated on the heap with 'new' and deleted in the cleanup section, or g_mockAampConfig should be set to nullptr before the function returns.

[Copilot]

Consider using NiceMock instead of plain MockAampConfig for consistency with codebase conventions. Change to: NiceMock mockConfig;

Suggested change

	MockAampConfig mockConfig;
	NiceMock<MockAampConfig> mockConfig;

[Copilot]

The buffer size increase from 64 to 128 should be verified against actual usage. The snprintf call uses format "%s PLAYER[%d] APP: %s IP_AAMP_TUNETIME". With the addition of "_VIPA" (5 chars), worst-case scenarios should be calculated: playerActiveMode (e.g., "FOREGROUND" = 10 chars) + " PLAYER[" (8) + playerId (up to 10 digits) + "] APP: " (7) + appName (variable) + "_VIPA" (5) + " IP_AAMP_TUNETIME" (17) + null terminator (1). The new 128-byte buffer should be sufficient for typical application names, but consider documenting the expected maximum appName length or adding a check to prevent buffer overflow with unusually long application names.

[Copilot]

The tests cover the three main scenarios, but consider adding test cases for edge cases such as: 1) Very long application names that might approach the buffer limit when "_VIPA" is appended (to verify the 128-byte buffer is sufficient), 2) Application names that already contain "_VIPA" to document the behavior (potential double-tagging, though this may be intentional). These additional tests would improve robustness and document expected behavior.

Suggested change

	TEST_F(AampProfilertests, TuneEndVIPATaggingWithNullConfigAndLongAppName)
	{
	// Save the current gpGlobalConfig pointer
	AampConfig* savedConfig = gpGlobalConfig;
	// Setup: ensure gpGlobalConfig is NULL for this test
	gpGlobalConfig = nullptr;
	// Prepare TuneEndMetrics
	TuneEndMetrics metrics;
	metrics.success = 1;
	metrics.contentType = ContentType_VOD;
	metrics.streamType = 1;
	metrics.mFirstTune = true;
	metrics.mTimedMetadataStartTime = 0;
	metrics.mTimedMetadataDuration = 0;
	metrics.mTuneAttempts = 1;
	metrics.mTotalTime = 0;
	// Create a very long application name to approach typical buffer limits
	std::string appName(130, 'A'); // longer than 128 to exercise edge behavior
	std::string playerActiveMode = "FOREGROUND";
	int playerId = 1;
	bool playerPreBuffered = false;
	unsigned int durationSeconds = 100;
	bool interfaceWifi = true;
	std::string failureReason = "";
	std::string tuneMetricDataStr = "";
	// Call TuneBegin first to enable profiling
	profileEvent->TuneBegin();
	// Call TuneEnd which should NOT append "_VIPA" when gpGlobalConfig is NULL,
	// even for very long application names
	profileEvent->TuneEnd(metrics, appName, playerActiveMode, playerId,
	playerPreBuffered, durationSeconds, interfaceWifi,
	failureReason, &tuneMetricDataStr);
	if (!tuneMetricDataStr.empty())
	{
	// Ensure no VIPA suffix is added when configuration is NULL
	EXPECT_TRUE(tuneMetricDataStr.find("_VIPA") == std::string::npos);
	// The original long app name should still appear in the metrics
	EXPECT_TRUE(tuneMetricDataStr.find(appName) != std::string::npos);
	}
	// Cleanup: restore the original gpGlobalConfig
	gpGlobalConfig = savedConfig;
	}
	TEST_F(AampProfilertests, TuneEndVIPATaggingWithNullConfigAndExistingVIPASuffix)
	{
	// Save the current gpGlobalConfig pointer
	AampConfig* savedConfig = gpGlobalConfig;
	// Setup: ensure gpGlobalConfig is NULL for this test
	gpGlobalConfig = nullptr;
	// Prepare TuneEndMetrics
	TuneEndMetrics metrics;
	metrics.success = 1;
	metrics.contentType = ContentType_VOD;
	metrics.streamType = 1;
	metrics.mFirstTune = true;
	metrics.mTimedMetadataStartTime = 0;
	metrics.mTimedMetadataDuration = 0;
	metrics.mTuneAttempts = 1;
	metrics.mTotalTime = 0;
	// Application name that already contains the VIPA suffix
	std::string appName = "TestApp_VIPA";
	std::string playerActiveMode = "FOREGROUND";
	int playerId = 1;
	bool playerPreBuffered = false;
	unsigned int durationSeconds = 100;
	bool interfaceWifi = true;
	std::string failureReason = "";
	std::string tuneMetricDataStr = "";
	// Call TuneBegin first to enable profiling
	profileEvent->TuneBegin();
	// Call TuneEnd which should NOT append an additional "_VIPA" when
	// gpGlobalConfig is NULL, even if the name already contains "_VIPA"
	profileEvent->TuneEnd(metrics, appName, playerActiveMode, playerId,
	playerPreBuffered, durationSeconds, interfaceWifi,
	failureReason, &tuneMetricDataStr);
	if (!tuneMetricDataStr.empty())
	{
	// We expect to see the original app name with single "_VIPA"
	EXPECT_TRUE(tuneMetricDataStr.find("TestApp_VIPA") != std::string::npos);
	// But no evidence of double-tagging
	EXPECT_TRUE(tuneMetricDataStr.find("TestApp_VIPA_VIPA") == std::string::npos);
	}
	// Cleanup: restore the original gpGlobalConfig
	gpGlobalConfig = savedConfig;
	}

[Copilot]

The addition of the "_VIPA" suffix to the application name is not documented in the function's header comment block (lines 220-287). Since this is a significant change to the tune metrics format that could affect downstream metric collection and analysis systems, consider adding documentation to explain that VIPA (Video Intelligence Platform Architecture) playback is indicated by appending "_VIPA" to the application name when eAAMPConfig_UseFireboltSDK is enabled.