Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
jfagunde wants to merge 3 commits into
base: dev_sprint_25_2
Choose a base branch
from
Draft

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b9c2d4d
VPLAY-11582 ProcessFragmentChunk() uses timescale of segment downloaded
jfagunde Jan 23, 2026
b7a892f
Remove left over, commented out code used for debug
jfagunde Jan 23, 2026
d446a63
Remove the code that tries to handle timescale=0
jfagunde Jan 23, 2026
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
36 changes: 8 additions & 28 deletions streamabstraction.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1031,36 +1031,16 @@ bool MediaTrack::ProcessFragmentChunk()
}
//Print box details
//isobuf.printBoxes();
uint32_t timeScale = 0;
if(type == eTRACK_VIDEO)
{
timeScale = aamp->GetVidTimeScale();
}
else if(type == eTRACK_AUDIO)
{
timeScale = aamp->GetAudTimeScale();
}
else if (type == eTRACK_SUBTITLE)
{
timeScale = aamp->GetSubTimeScale();
}

// 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;
Copy link

Copilot AI Jan 23, 2026

Choose a reason for hiding this comment

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

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 uses AI. Check for mistakes.
Comment on lines +1034 to +1043
Copy link

Copilot AI Jan 23, 2026

Choose a reason for hiding this comment

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

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 uses AI. Check for mistakes.
Comment on lines +1042 to +1043
Copy link

Copilot AI Jan 23, 2026

Choose a reason for hiding this comment

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

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:

  1. The tests need to be updated to remove the fallback test cases (lines 927-928), OR
  2. 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 uses AI. Check for mistakes.
}
double fpts = 0.0, fduration = 0.0;
bool ret = isobuf.ParseChunkData(name, unParsedBuffer, timeScale, parsedBufferSize, unParsedBufferSize, fpts, fduration);
Expand Down
110 changes: 110 additions & 0 deletions test/utests/tests/MediaTrackTests/MediaTrackTests.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -820,3 +820,113 @@ TEST_F(MediaTrackTests, MediaTrackConstructorChunkModeTest)
TestableMediaTrack videoTrack{eTRACK_VIDEO, mPrivateInstanceAAMP, "video", mStreamAbstractionAAMP_MPD};
EXPECT_EQ(videoTrack.GetCachedFragmentChunksSize(), kMaxFragmentChunkCached);
}

/**
* @brief Test data for ProcessFragmentChunk timescale tests
*
* This structure holds the test parameters for verifying that ProcessFragmentChunk
* uses the correct timescale from the cached fragment (critical for TSB playback
* where injected segments may have different timescales than live edge segments).
*/
struct ChunkTimescaleTestData
{
uint32_t cachedFragmentTimeScale; /**< Timescale stored in the cached fragment */
uint32_t globalTimeScale; /**< Global timescale from AAMP instance */
uint32_t expectedTimeScale; /**< Expected timescale to be used in ParseChunkData */
};

class MediaTrackProcessFragmentChunkTimescaleTests
: public MediaTrackTests,
public testing::WithParamInterface<ChunkTimescaleTestData>
{
};

/**
* @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.
Comment on lines +845 to +855
Copy link

Copilot AI Jan 23, 2026

Choose a reason for hiding this comment

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

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 uses AI. Check for mistakes.
*/
TEST_P(MediaTrackProcessFragmentChunkTimescaleTests, UseCachedFragmentTimescaleInParseChunkData)
Copy link

Copilot AI Jan 23, 2026

Choose a reason for hiding this comment

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

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 uses AI. Check for mistakes.
{
ChunkTimescaleTestData testData = GetParam();
CachedFragment* bufferedFragment{nullptr};

// Configure low-latency mode since ProcessFragmentChunk is used for chunk-based injection
SetLowLatencyMode(true);
mPrivateInstanceAAMP->rate = AAMP_NORMAL_PLAY_RATE;
mPrivateInstanceAAMP->mMediaFormat = eMEDIAFORMAT_DASH;
mStreamAbstractionAAMP_MPD->trickplayMode = false;

// 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));
Comment on lines +868 to +870
Copy link

Copilot AI Jan 23, 2026

Choose a reason for hiding this comment

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

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 uses AI. Check for mistakes.

EXPECT_CALL(*g_mockAampConfig, IsConfigSet(eAAMPConfig_OverrideMediaHeaderDuration))
.WillRepeatedly(Return(false));
EXPECT_CALL(*g_mockAampConfig, IsConfigSet(eAAMPConfig_CurlThroughput))
.WillRepeatedly(Return(false));
EXPECT_CALL(*g_mockAampConfig, IsConfigSet(eAAMPConfig_EnablePTSReStamp))
.WillRepeatedly(Return(false));
EXPECT_CALL(*g_mockAampConfig, GetConfigValue(eAAMPConfig_MaxFragmentCached))
.WillRepeatedly(Return(1));
EXPECT_CALL(*g_mockAampConfig, GetConfigValue(eAAMPConfig_MaxFragmentChunkCached))
.WillRepeatedly(Return(1));
EXPECT_CALL(*g_mockIsoBmffBuffer, parseBuffer(_, _)).WillRepeatedly(Return(true));
EXPECT_CALL(*g_mockPrivateInstanceAAMP, GetLLDashChunkMode()).WillRepeatedly(Return(true));

TestableMediaTrack videoTrack{eTRACK_VIDEO, mPrivateInstanceAAMP, "video",
mStreamAbstractionAAMP_MPD};

// First inject an init fragment (required before media fragments)
CachedFragment initFragment;
initFragment.initFragment = true;
initFragment.fragment.AppendBytes(FRAGMENT_TEST_DATA, strlen(FRAGMENT_TEST_DATA));
bufferedFragment = videoTrack.GetFetchChunkBuffer(true);
videoTrack.numberOfFragmentChunksCached = 1;
bufferedFragment->Copy(&initFragment, initFragment.fragment.GetLen());
ASSERT_TRUE(videoTrack.InjectFragment());

// Now inject a media fragment with a specific timescale
CachedFragment testFragment;
testFragment.initFragment = false;
testFragment.duration = FRAGMENT_DURATION.inSeconds();
testFragment.position = FIRST_PTS.inSeconds();
testFragment.timeScale = testData.cachedFragmentTimeScale;
testFragment.uri = "test_segment.m4s";
testFragment.fragment.AppendBytes(FRAGMENT_TEST_DATA, strlen(FRAGMENT_TEST_DATA));

bufferedFragment = videoTrack.GetFetchChunkBuffer(true);
videoTrack.numberOfFragmentChunksCached = 1;
bufferedFragment->Copy(&testFragment, testFragment.fragment.GetLen());

// Key assertion: ParseChunkData should be called with the expected timescale
// This tests that ProcessFragmentChunk uses cachedFragment->timeScale when set,
// or falls back to global timescale when cachedFragment->timeScale is 0
Comment on lines +911 to +912
Copy link

Copilot AI Jan 23, 2026

Choose a reason for hiding this comment

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

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.

Copilot uses AI. Check for mistakes.
EXPECT_CALL(*g_mockIsoBmffBuffer,
ParseChunkData(_, _, testData.expectedTimeScale, _, _, _, _))
.WillOnce(DoAll(SetArgReferee<5>(bufferedFragment->position),
SetArgReferee<6>(bufferedFragment->duration), Return(true)));

ASSERT_TRUE(videoTrack.InjectFragment());
}

ChunkTimescaleTestData chunkTimescaleTestData[] = {
// When cached fragment has a valid timescale, it should be used
{90000, 48000, 90000}, // Fragment timescale 90000 (typical video), global 48000 -> use 90000
{48000, 90000, 48000}, // Fragment timescale 48000 (typical audio), global 90000 -> use 48000
{10000000, 90000, 10000000}, // Fragment timescale 10000000 (100ns units), global 90000 -> use 10000000
// 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
Comment on lines +927 to +928
Copy link

Copilot AI Jan 23, 2026

Choose a reason for hiding this comment

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

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 uses AI. Check for mistakes.
Comment on lines +926 to +928
Copy link

Copilot AI Jan 23, 2026

Choose a reason for hiding this comment

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

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 uses AI. Check for mistakes.
};

INSTANTIATE_TEST_SUITE_P(MediaTrackTests, MediaTrackProcessFragmentChunkTimescaleTests,
ValuesIn(chunkTimescaleTestData));
Loading