VPAAMP-205 eliminate recursion concerns in HandleSeekEOSAndPeriodTransition by pstroffolino · Pull Request #1375 · rdkcentral/aamp

pstroffolino · 2026-04-28T17:49:13Z

Reason for Change: This PR removes recursive period-by-period seek handling in SeekInPeriod() for DASH MPD playback by introducing an iterative loop and a helper to perform period transitions, reducing stack-growth risk when seeks span many periods.

Added HandleSeekEOSAndPeriodTransition() to switch to the next playable period when a seek crosses a period boundary.
Refactored SeekInPeriod() to an iterative loop that repeatedly skips fragments and advances periods until the seek remainder is consumed or no further periods exist.
Added a new functional test to validate that mFirstPTS updates correctly when a seek crosses from one period to the next.

Convert SeekInPeriod from a recursive design into an iterative while-loop. HandleSeekEOSAndPeriodTransition now only handles the period state transition and returns true/false; SeekInPeriod drives the loop, calling SkipFragments and HandleSeekEOSAndPeriodTransition until no further period transition is needed or no playable period remains. This eliminates the theoretical unbounded stack growth that arose when a seek remainder spanned multiple consecutive short periods.

…sition/SeekInPeriod 1. Add 'enabled' guard to EOS check (HandleSeekEOSAndPeriodTransition) A disabled/unselected track (e.g. alternate audio, subtitle when off) may carry stale eos=true from a prior operation and must not trigger a spurious period transition. Guard with mMediaStreamContext[i]->enabled, matching the established pattern used throughout fragmentcollector_mpd.cpp. 2. Capture remaining seek from primary video track only (SeekInPeriod) The subtitle SkipFragments call omits skipToEnd and can return a different remainder than A/V. Since subtitle is last in the loop (VIDEO=0, AUDIO=1, SUBTITLE=2), it previously overwrote trackRemainingSeek on every seek with subtitles active, driving incorrect period transitions and carry-over offsets. Now: subtitle result is discarded; trackRemainingSeek is set from video, or from the first enabled non-subtitle track if video is absent. 3. Save/restore period state around UpdateTrackInfo (HandleSeekEOSAndPeriodTransition) Five members (mCurrentPeriodIdx, mCurrentPeriod, mBasePeriodId, mPeriodStartTime/Duration/EndTime) were mutated before UpdateTrackInfo was called. On failure the object was left in a half-switched state (period index updated, track info not). Save the previous values and restore them on failure so the player remains in a consistent state.

…essage - Replace 0.0-sentinel track selection with an explicit primaryCaptured flag so the first enabled non-subtitle track (video, else audio) is deterministically chosen as the period-transition remainder, avoiding ambiguity when the primary track legitimately returns 0.0. - Replace NULL with nullptr in HandleSeekEOSAndPeriodTransition guard. - Clarify log message: 'caller will re-run SkipFragments on the new period' instead of the misleading 'calling SkipFragments'.

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

@param

…d/unused skipToEnd is accepted for API consistency with SeekInPeriod and reserved for future direction-aware empty-period handling, but is not read inside the function body. Comment out the parameter name in the definition to silence -Wunused-parameter without removing the parameter, and update the Doxygen @param to explain the rationale.

Copilot

Pull request overview

This PR refactors DASH multi-period seek handling to avoid recursion by introducing an iterative seek loop and a helper to transition between playable periods when a seek crosses a period boundary.

Changes:

Added HandleSeekEOSAndPeriodTransition() to detect seek-driven EOS and advance to the next playable period.
Refactored SeekInPeriod() from a recursive approach into an iterative loop that repeatedly applies SkipFragments() and transitions periods as needed.
Added/updated a functional test to validate mFirstPTS updates correctly across a period boundary during seek.

Reviewed changes

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

File	Description
`fragmentcollector_mpd.cpp`	Implements the new helper and iterative seek loop; adjusts EOS detection and period-switch state handling.
`fragmentcollector_mpd.h`	Updates API documentation for the new helper and clarifies parameter usage.
`test/utests/tests/StreamAbstractionAAMP_MPD/FunctionalTests.cpp`	Adds a functional test covering `mFirstPTS` update when a seek crosses from one period to the next.

Copilot · 2026-04-28T18:01:51Z

 	MediaStreamContext *pMediaStreamContext = static_cast<MediaStreamContext *>(track);
+	(void)pMediaStreamContext;


In this test, pMediaStreamContext is created via static_cast but never used (it’s immediately void-cast). If the cast isn’t needed, consider removing the variable; if the intent is to validate the type, prefer a runtime-checked cast (e.g., dynamic_cast + ASSERT) and then use it for an assertion that matters to the test.

Suggested change

MediaStreamContext *pMediaStreamContext = static_cast<MediaStreamContext *>(track);

(void)pMediaStreamContext;

Copilot · 2026-04-28T18:01:52Z

+	// Check whether any enabled track hit EOS during the seek. Only enabled tracks
+	// participate in playback; a disabled or unselected track (e.g. an alternate audio
+	// track, or a subtitle track when subtitles are off) may carry stale eos=true from
+	// a prior operation and must not trigger a spurious period transition.
+	// DASH spec requires all tracks in a period to end at the same presentation time,
+	// so any single enabled track reaching EOS is sufficient.
+	// The mPlayRate and remainingSeek guards prevent false positives:
 	//   - mPlayRate >= AAMP_RATE_PAUSE: for reverse playback (mPlayRate < 0), EOS means
 	//     beginning-of-period, not end-of-period, so we must not advance to the next period.
 	//   - remainingSeek >= 0: a negative remainder also indicates the seek overshot backward
 	//     rather than forward, so no forward period transition is appropriate.
 	for (int i = 0; i < mNumberOfTracks; i++)
 	{
-		if (mMediaStreamContext[i] != NULL && mMediaStreamContext[i]->eos && (mPlayRate >= AAMP_RATE_PAUSE) &&  remainingSeek >= 0)
+		if (mMediaStreamContext[i] != nullptr && mMediaStreamContext[i]->enabled && mMediaStreamContext[i]->eos && (mPlayRate >= AAMP_RATE_PAUSE) &&  remainingSeek >= 0)
 		{
 			switchToNextPeriod = true;


HandleSeekEOSAndPeriodTransition() decides to advance periods when any enabled track has eos=true. This includes subtitles, but the main fetch loop’s EOS gating ignores subtitles (it only checks video/audio; see fragmentcollector_mpd.cpp:10338-10341). If subtitle segments end earlier (or are missing) than A/V, a seek could incorrectly advance to the next period. Consider restricting the EOS check here to video/audio (and any other A/V-carrying tracks you intend) rather than iterating all tracks indiscriminately.

Copilot · 2026-04-28T18:01:52Z

+		AAMPLOG_WARN("SeekInPeriod: UpdateTrackInfo failed while switching to period %d, restoring previous period state", mCurrentPeriodIdx);
+		mCurrentPeriodIdx = savedPeriodIdx;
+		mCurrentPeriod    = savedPeriod;
+		mBasePeriodId     = savedBasePeriodId;
+		mPeriodStartTime  = savedPeriodStart;
+		mPeriodDuration   = savedPeriodDuration;
+		mPeriodEndTime    = savedPeriodEnd;
 		return false;


In the UpdateTrackInfo failure path, the code restores period index/pointers/timing fields but does not restore other state mutated during the attempted switch (e.g., StreamSelection() effects such as enabled flags/mNumberOfTracks, and mUpdateStreamInfo=true). After restoring the old period, the object may still reflect the new period’s stream selection, leaving state inconsistent for subsequent operations. Consider either (a) delaying StreamSelection/mUpdateStreamInfo until after UpdateTrackInfo succeeds, or (b) saving/restoring those additional fields and/or rerunning StreamSelection() for the restored period before returning.

Copilot · 2026-04-28T18:01:52Z

 	}

-	AAMPLOG_INFO("SeekInPeriod: Switched to period %d, calling SkipFragments with remaining seek %lf", mCurrentPeriodIdx, remainingSeek);
+AAMPLOG_INFO("SeekInPeriod: Switched to period %d; caller will re-run SkipFragments on the new period with remaining seek %lf", mCurrentPeriodIdx, remainingSeek);


The AAMPLOG_INFO call is not indented inside the function body, which breaks the surrounding indentation/style consistency and makes the block harder to scan. Please indent it to match the other statements in this scope.

Suggested change

AAMPLOG_INFO("SeekInPeriod: Switched to period %d; caller will re-run SkipFragments on the new period with remaining seek %lf", mCurrentPeriodIdx, remainingSeek);

AAMPLOG_INFO("SeekInPeriod: Switched to period %d; caller will re-run SkipFragments on the new period with remaining seek %lf", mCurrentPeriodIdx, remainingSeek);

pstroffolino and others added 7 commits April 28, 2026 13:44

Update test/utests/tests/StreamAbstractionAAMP_MPD/FunctionalTests.cpp

7396ccd

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Apply suggestions from code review

6a77320

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Update fragmentcollector_mpd.cpp

27c9028

pstroffolino requested a review from a team as a code owner April 28, 2026 17:49

pstroffolino changed the title ~~VPAAMP-205 eliminate recession concerns in HandleSeekEOSAndPeriodTransition~~ VPAAMP-205 eliminate recursion concerns in HandleSeekEOSAndPeriodTransition Apr 28, 2026

pstroffolino requested review from Copilot and removed request for a team April 28, 2026 17:49

Copilot started reviewing on behalf of pstroffolino April 28, 2026 17:54 View session

Copilot AI reviewed Apr 28, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

VPAAMP-205 eliminate recursion concerns in HandleSeekEOSAndPeriodTransition#1375

VPAAMP-205 eliminate recursion concerns in HandleSeekEOSAndPeriodTransition#1375
pstroffolino wants to merge 7 commits intosupport/2.11.1_8.4_vipafrom
feature/VPAAMP-205-support

pstroffolino commented Apr 28, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Apr 28, 2026

Uh oh!

Copilot AI Apr 28, 2026

Uh oh!

Copilot AI Apr 28, 2026

Uh oh!

Copilot AI Apr 28, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

		MediaStreamContext pMediaStreamContext = static_cast<MediaStreamContext >(track);
		(void)pMediaStreamContext;

	AAMPLOG_INFO("SeekInPeriod: Switched to period %d; caller will re-run SkipFragments on the new period with remaining seek %lf", mCurrentPeriodIdx, remainingSeek);
	AAMPLOG_INFO("SeekInPeriod: Switched to period %d; caller will re-run SkipFragments on the new period with remaining seek %lf", mCurrentPeriodIdx, remainingSeek);

Conversation

pstroffolino commented Apr 28, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Apr 28, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 28, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 28, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 28, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants