VPLAY-11582 ProcessFragmentChunk() uses timescale of segment downloaded #900

jfagunde · 2026-01-23T10:14:26Z

Reason for Change: Avoid potential edge case and make code correct

Summary of Changes:

Use timescale of fragment injected in ProcessFragmentChunk()
Verify timescale used by InjectFragment() in L1 tests

Test Procedure: L1 tests should pass and no AAMP TSB + CDA regressions

Risk: Low

Reason for Change: Avoid potential edge case and make code correct Summary of Changes: * Use timescale of fragment injected in ProcessFragmentChunk() * Verify timescale used by InjectFragment() in L1 tests Test Procedure: L1 tests should pass and no AAMP TSB + CDA regressions Risk: Low

Copilot

Pull request overview

This pull request fixes a timing correctness issue in the AAMP player's fragment processing logic. The fix ensures that ProcessFragmentChunk() uses the timescale associated with the specific fragment being injected, rather than relying on global timescales. This is critical for Time-Shift Buffer (TSB) scenarios where segments from different sources (e.g., ads vs. base content) may have different timescales.

Changes:

Modified ProcessFragmentChunk() in streamabstraction.cpp to prioritize the cached fragment's timescale over global timescales
Added comprehensive L1 parameterized tests to verify correct timescale usage in both primary and fallback scenarios
Improved error logging when fallback to global timescale is needed

Reviewed changes

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

File	Description
streamabstraction.cpp	Updated `ProcessFragmentChunk()` to use `cachedFragment->timeScale` as the primary source, with fallback to global timescales when it's 0
test/utests/tests/MediaTrackTests/MediaTrackTests.cpp	Added parameterized test suite with 5 test cases covering different timescale scenarios (video, audio, 100ns units, and fallback cases)

DomSyna

Not looked at tests yet, just thought better to iron out code changes as they may impact tests

streamabstraction.cpp

Copilot

Pull request overview

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

Copilot · 2026-01-23T11:54:49Z

test/utests/tests/MediaTrackTests/MediaTrackTests.cpp

+	// Set up mock to return the global timescale (used as fallback when cached fragment timescale is 0)
+	EXPECT_CALL(*g_mockPrivateInstanceAAMP, GetVidTimeScale())
+		.WillRepeatedly(Return(testData.globalTimeScale));


The comment states "used as fallback when cached fragment timescale is 0" but the production code at streamabstraction.cpp:1040-1043 does NOT implement this fallback. The mock for GetVidTimeScale() is set up but will never be called because the production code returns early when timeScale is 0. This mock setup is misleading and the comment should be corrected.

Copilot · 2026-01-23T11:54:49Z

test/utests/tests/MediaTrackTests/MediaTrackTests.cpp

+	{0, 90000, 90000},        // Fragment timescale 0, global 90000 -> use global
+	{0, 48000, 48000},        // Fragment timescale 0, global 48000 -> use global


These test cases will FAIL because they expect ParseChunkData to be called with the global timescale when cachedFragment->timeScale is 0, but the production code at streamabstraction.cpp:1040-1043 returns early without calling ParseChunkData. Either these test cases need to be removed, or the production code needs to implement the fallback behavior as the old code did (using GetVidTimeScale/GetAudTimeScale/GetSubTimeScale based on track type).

Copilot · 2026-01-23T11:54:49Z

streamabstraction.cpp

-			AAMPLOG_WARN("[%s] Invalid play context maybe test setup, timeScale=%d", name, timeScale);
-		}
+		AAMPLOG_ERR("[%s] Cached fragment timescale is 0, fragment URI: %s", name, cachedFragment->uri.c_str());
+		return true;


Returning 'true' (success) when timeScale is 0 is semantically incorrect. The function ProcessFragmentChunk returns true to indicate success in processing a chunk, but in this case the chunk is not processed at all. Consider returning 'false' to indicate failure, or implement the fallback logic to use global timescales as the old code did. This silent failure could mask real issues where fragments legitimately have timeScale set to 0.

Suggested change

return true;

return false;

Copilot · 2026-01-23T11:54:49Z

test/utests/tests/MediaTrackTests/MediaTrackTests.cpp

+ * @brief Test that ProcessFragmentChunk uses the cached fragment's timescale
+ *
+ * This test verifies that when processing fragment chunks, the timescale from
+ * the cached fragment is used (if set), rather than falling back to global
+ * timescales. This is critical for TSB (Time-Shift Buffer) scenarios where:
+ * - The segment being downloaded at the live edge may be from an ad with one timescale
+ * - The segment being injected from TSB may be from base content with a different timescale
+ *
+ * The test would fail if ProcessFragmentChunk used global timescales instead of the
+ * cached fragment's timescale, as the ParseChunkData mock expects the exact timescale
+ * value from the cached fragment.


The test documentation states "rather than falling back to global timescales", but the test data at lines 927-928 actually expects fallback to global timescales when cachedFragment->timeScale is 0. This is contradictory and confusing. The test intention needs to be clarified: should fallback be supported or not? The PR title and description suggest using the fragment's timescale, which implies no fallback, but the test cases suggest fallback should work.

Suggested change

* @brief Test that ProcessFragmentChunk uses the cached fragment's timescale

*

* This test verifies that when processing fragment chunks, the timescale from

* the cached fragment is used (if set), rather than falling back to global

* timescales. This is critical for TSB (Time-Shift Buffer) scenarios where:

* - The segment being downloaded at the live edge may be from an ad with one timescale

* - The segment being injected from TSB may be from base content with a different timescale

*

* The test would fail if ProcessFragmentChunk used global timescales instead of the

* cached fragment's timescale, as the ParseChunkData mock expects the exact timescale

* value from the cached fragment.

* @brief Test that ProcessFragmentChunk prefers the cached fragment's timescale

*

* This test verifies that when processing fragment chunks, the timescale from

* the cached fragment is used when it is non-zero, and that when the cached

* fragment's timescale is zero, ProcessFragmentChunk falls back to the global

* timescale from the AAMP instance. This is critical for TSB (Time-Shift Buffer)

* scenarios where:

* - The segment being downloaded at the live edge may be from an ad with one timescale

* - The segment being injected from TSB may be from base content with a different timescale

*

* The test would fail if ProcessFragmentChunk did not select the correct effective

* timescale (cached fragment's timescale when non-zero, otherwise the global

* timescale), as the ParseChunkData mock expects the timescale value provided

* by the test parameters.

Copilot · 2026-01-23T11:54:50Z

streamabstraction.cpp

+
+	// Use the timescale stored in the cached fragment, which represents the timescale
+	// of the segment being injected. This is critical when using TSB, as the segment
+	// being downloaded at the live edge may have a different timescale (e.g., an ad)
+	// than the segment being injected from TSB (e.g., base content).
+	uint32_t timeScale = cachedFragment->timeScale;
 	if(!timeScale)
 	{
-		//FIX-ME-Read from MPD INSTEAD
-		if(pContext)
-		{
-			timeScale = pContext->GetCurrPeriodTimeScale();
-			if(!timeScale)
-			{
-				timeScale = 10000000.0;
-				AAMPLOG_WARN("[%s] Empty timeScale!!! Using default timeScale=%d", name, timeScale);
-			}
-		}
-		else
-		{
-			timeScale = 1000.0;
-			AAMPLOG_WARN("[%s] Invalid play context maybe test setup, timeScale=%d", name, timeScale);
-		}
+		AAMPLOG_ERR("[%s] Cached fragment timescale is 0, fragment URI: %s", name, cachedFragment->uri.c_str());
+		return true;


The new implementation removes all fallback logic that existed in the old code. The old code had a comprehensive fallback chain: track-type-specific global timescale → period timescale → hardcoded default (10000000 or 1000). While the PR description states the goal is to use the fragment's timescale, removing ALL fallback mechanisms could cause issues if cachedFragment->timeScale is legitimately 0 due to incomplete initialization or parsing errors. Consider at minimum keeping the track-type-specific global timescale fallback (GetVidTimeScale/GetAudTimeScale/GetSubTimeScale) as the first fallback option.

Copilot · 2026-01-23T11:54:50Z

test/utests/tests/MediaTrackTests/MediaTrackTests.cpp

+ * cached fragment's timescale, as the ParseChunkData mock expects the exact timescale
+ * value from the cached fragment.
+ */
+TEST_P(MediaTrackProcessFragmentChunkTimescaleTests, UseCachedFragmentTimescaleInParseChunkData)


The PR description states "Verify timescale used by InjectFragment() in L1 tests" but the tests are actually for ProcessFragmentChunk(), not InjectFragment(). While InjectFragment() internally calls ProcessFragmentChunk() (when in chunk mode), the test naming and PR description should be consistent about which function is being tested. The test name "UseCachedFragmentTimescaleInParseChunkData" correctly indicates it's testing ProcessFragmentChunk's behavior.

Copilot · 2026-01-23T11:54:50Z

streamabstraction.cpp

+		AAMPLOG_ERR("[%s] Cached fragment timescale is 0, fragment URI: %s", name, cachedFragment->uri.c_str());
+		return true;


Critical bug: The code returns early when timeScale is 0 without processing the chunk, but the test cases expect fallback to global timescale. Test cases at MediaTrackTests.cpp:927-928 expect that when cachedFragment->timeScale is 0, the code should fall back to using GetVidTimeScale() (for video) or equivalent global timescales. However, this implementation logs an error and returns true without calling ParseChunkData at all, which means the fragment chunk is silently skipped. Either:

The tests need to be updated to remove the fallback test cases (lines 927-928), OR

The production code needs to implement the fallback behavior by calling GetVidTimeScale()/GetAudTimeScale()/GetSubTimeScale() based on track type when cachedFragment->timeScale is 0, as the old code did.

Suggested change

AAMPLOG_ERR("[%s] Cached fragment timescale is 0, fragment URI: %s", name, cachedFragment->uri.c_str());

return true;

// Fallback to global track-specific timescale when the cached fragment

// timescale is zero. This matches legacy behavior and unit test

// expectations (see MediaTrackTests.cpp).

switch (type)

{

case eTRACK_VIDEO:

timeScale = GetVidTimeScale();

break;

case eTRACK_AUDIO:

timeScale = GetAudTimeScale();

break;

case eTRACK_SUBTITLE:

timeScale = GetSubTimeScale();

break;

default:

timeScale = 0;

break;

}

if (!timeScale)

{

AAMPLOG_ERR("[%s] Cached fragment timescale is 0 and no valid global "

"timescale available, fragment URI: %s", name,

cachedFragment->uri.c_str());

return true;

}

AAMPLOG_WARN("[%s] Cached fragment timescale is 0, using global "

"timescale %u for fragment URI: %s", name, timeScale,

cachedFragment->uri.c_str());

Copilot · 2026-01-23T11:54:50Z

test/utests/tests/MediaTrackTests/MediaTrackTests.cpp

+	// When cached fragment timescale is 0, fall back to global timescale
+	{0, 90000, 90000},        // Fragment timescale 0, global 90000 -> use global
+	{0, 48000, 48000},        // Fragment timescale 0, global 48000 -> use global


This comment suggests fallback behavior is expected and supported, but the production code at streamabstraction.cpp:1040-1043 does not implement fallback - it returns early with an error. This creates confusion about whether timeScale=0 should be treated as an error condition or as a valid scenario requiring fallback to global timescales.

Copilot · 2026-01-23T11:54:50Z

test/utests/tests/MediaTrackTests/MediaTrackTests.cpp

+	// This tests that ProcessFragmentChunk uses cachedFragment->timeScale when set,
+	// or falls back to global timescale when cachedFragment->timeScale is 0


The comment at line 912 states "or falls back to global timescale when cachedFragment->timeScale is 0", but the production code at streamabstraction.cpp:1040-1043 does NOT implement this fallback behavior. Instead, it logs an error and returns early. Either this comment needs to be updated to reflect that fallback is NOT supported, or the production code needs to be changed to implement the fallback. This mismatch will cause test failures for the test cases at lines 927-928.

Suggested change

// This tests that ProcessFragmentChunk uses cachedFragment->timeScale when set,

// or falls back to global timescale when cachedFragment->timeScale is 0

// This tests that ProcessFragmentChunk uses cachedFragment->timeScale when set.

// Note: Fallback to a global timescale when cachedFragment->timeScale is 0 is not implemented.

jfagunde requested a review from a team as a code owner January 23, 2026 10:14

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

Copilot started reviewing on behalf of jfagunde January 23, 2026 10:14 View session

Copilot AI reviewed Jan 23, 2026

View reviewed changes

DomSyna reviewed Jan 23, 2026

View reviewed changes

streamabstraction.cpp Outdated Show resolved Hide resolved

streamabstraction.cpp Outdated Show resolved Hide resolved

jfagunde added 2 commits January 23, 2026 12:09

Remove left over, commented out code used for debug

b7a892f

Remove the code that tries to handle timescale=0

d446a63

Copilot AI review requested due to automatic review settings January 23, 2026 11:49

Copilot started reviewing on behalf of jfagunde January 23, 2026 11:49 View session

jfagunde marked this pull request as draft January 23, 2026 11:51

Copilot AI reviewed Jan 23, 2026

View reviewed changes

		{0, 90000, 90000}, // Fragment timescale 0, global 90000 -> use global
		{0, 48000, 48000}, // Fragment timescale 0, global 48000 -> use global

		AAMPLOG_ERR("[%s] Cached fragment timescale is 0, fragment URI: %s", name, cachedFragment->uri.c_str());
		return true;

-		AAMPLOG_ERR("[%s] Cached fragment timescale is 0, fragment URI: %s", name, cachedFragment->uri.c_str());
-		return true;
+		// Fallback to global track-specific timescale when the cached fragment
+		// timescale is zero. This matches legacy behavior and unit test
+		// expectations (see MediaTrackTests.cpp).
+		switch (type)
+		{
+			case eTRACK_VIDEO:
+				timeScale = GetVidTimeScale();
+				break;
+			case eTRACK_AUDIO:
+				timeScale = GetAudTimeScale();
+				break;
+			case eTRACK_SUBTITLE:
+				timeScale = GetSubTimeScale();
+				break;
+			default:
+				timeScale = 0;
+				break;
+		}
+		if (!timeScale)
+		{
+			AAMPLOG_ERR("[%s] Cached fragment timescale is 0 and no valid global "
+					"timescale available, fragment URI: %s", name,
+					cachedFragment->uri.c_str());
+			return true;
+		}
+		AAMPLOG_WARN("[%s] Cached fragment timescale is 0, using global "
+				"timescale %u for fragment URI: %s", name, timeScale,
+				cachedFragment->uri.c_str());

		// This tests that ProcessFragmentChunk uses cachedFragment->timeScale when set,
		// or falls back to global timescale when cachedFragment->timeScale is 0

VPLAY-11582 ProcessFragmentChunk() uses timescale of segment downloaded #900

Are you sure you want to change the base?

VPLAY-11582 ProcessFragmentChunk() uses timescale of segment downloaded #900

Conversation

jfagunde commented Jan 23, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

DomSyna left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Jan 23, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 23, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 23, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 23, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 23, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 23, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 23, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 23, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 23, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants