VPAAMP-205 eliminate recursion concerns in HandleSeekEOSAndPeriodTransition

fragmentcollector_mpd.cpp

@@ -2287,21 +2287,24 @@ bool StreamAbstractionAAMP_MPD::PushNextFragment( class MediaStreamContext *pMed
  * @brief If SkipFragments reaches EOS and an additional playable period is available, switch to the next period.
  */
 bool StreamAbstractionAAMP_MPD::HandleSeekEOSAndPeriodTransition(double remainingSeek,
-	bool skipToEnd)
+	bool /*skipToEnd*/) // reserved for future use; see header for rationale
 {
 	bool switchToNextPeriod = false;
 
-	// Check whether any track hit EOS during the seek. Any enabled track reaching EOS
-	// is sufficient to trigger a period transition: DASH spec requires all tracks in a
-	// period to end at the same presentation time, so this is safe for well-formed content.
-	// The mPlayRate and remainingSeek guards (comment 2 & 3) prevent false positives:
+	// 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;
 			break;
@@ -2326,6 +2329,15 @@ bool StreamAbstractionAAMP_MPD::HandleSeekEOSAndPeriodTransition(double remainin
 		return false;
 	}
 
+	// Save current period state so we can restore it if UpdateTrackInfo fails,
+	// leaving the object in a consistent state rather than half-switched.
+	const int      savedPeriodIdx      = mCurrentPeriodIdx;
+	IPeriod       *savedPeriod         = mCurrentPeriod;
+	std::string    savedBasePeriodId   = mBasePeriodId;
+	const double   savedPeriodStart    = mPeriodStartTime;
+	const double   savedPeriodDuration = mPeriodDuration;
+	const double   savedPeriodEnd      = mPeriodEndTime;
+
 	mCurrentPeriodIdx = nextPeriodIdx;
 	mCurrentPeriod = mpd->GetPeriods().at(mCurrentPeriodIdx);
 	mBasePeriodId = mCurrentPeriod->GetId();
@@ -2338,18 +2350,19 @@ bool StreamAbstractionAAMP_MPD::HandleSeekEOSAndPeriodTransition(double remainin
 	AAMPStatusType ret = UpdateTrackInfo(true, true);
 	if (ret != eAAMPSTATUS_OK)
 	{
-		AAMPLOG_WARN("SeekInPeriod: UpdateTrackInfo failed while switching to period %d", mCurrentPeriodIdx);
+		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;
 	}
 
-	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);
 
-	// TODO: This recursive call works correctly for the expected case where remainingSeek
-	// is fully absorbed by the next period (recursion depth = 1). On content with many
-	// consecutive short periods it could theoretically recurse once per period. A future
-	// refactor should convert SeekInPeriod into an iterative loop (driven by the bool
-	// return value of this function) to eliminate the theoretical stack-growth risk.
-	SeekInPeriod(remainingSeek, skipToEnd);
+	// Caller (SeekInPeriod) will invoke SkipFragments on the new period in its loop.
 	return true;
 }
 
@@ -2359,31 +2372,50 @@ bool StreamAbstractionAAMP_MPD::HandleSeekEOSAndPeriodTransition(double remainin
  */
 void StreamAbstractionAAMP_MPD::SeekInPeriod( double seekPositionSeconds, bool skipToEnd)
 {
-	double trackRemainingSeek = 0.0;
-	for (int i = 0; i < mNumberOfTracks; i++)
-	{
-		if (!mMediaStreamContext[i])
+	// Iterative loop: advance through periods until the seek remainder is fully consumed
+	// or no further playable period is available. HandleSeekEOSAndPeriodTransition updates
+	// mCurrentPeriodIdx and associated state when a transition is needed, and returns true
+	// to signal that SkipFragments should be run again on the new period.
+	while (true)
+	{
+		// Capture remaining seek from the primary (video) track, or the first enabled
+		// non-subtitle track if video is absent. Subtitle is intentionally excluded:
+		// it is called without skipToEnd and may return a different remainder than A/V,
+		// which would drive an incorrect period transition or carry-over seek offset.
+		double trackRemainingSeek = 0.0;
+		bool primaryCaptured = false;
+		for (int i = 0; i < mNumberOfTracks; i++)
 		{
-			continue;
-		}
+			if (!mMediaStreamContext[i])
+			{
+				continue;
+			}
 
-		if (eMEDIATYPE_SUBTITLE == i)
-		{
-			double skipTime = seekPositionSeconds;
-			trackRemainingSeek = SkipFragments(mMediaStreamContext[i], skipTime, true);
+			if (eMEDIATYPE_SUBTITLE == i)
+			{
+				double skipTime = seekPositionSeconds;
+				SkipFragments(mMediaStreamContext[i], skipTime, true);
+			}
+			else
+			{
+				double remaining = SkipFragments(mMediaStreamContext[i], seekPositionSeconds, true, skipToEnd);
+				// Capture the first enabled non-subtitle track as the period-transition
+				// primary: video (index 0) if enabled, otherwise audio (index 1).
+				// Using a flag rather than a 0.0 sentinel avoids ambiguity when the
+				// primary track legitimately returns a remainder of exactly 0.0.
+				if (!primaryCaptured && mMediaStreamContext[i]->enabled)
+				{
+					trackRemainingSeek = remaining;
+					primaryCaptured = true;
+				}
+			}
 		}
-		else
+		if (!HandleSeekEOSAndPeriodTransition(trackRemainingSeek, skipToEnd))
 		{
-			trackRemainingSeek = SkipFragments(mMediaStreamContext[i], seekPositionSeconds, true, skipToEnd);
+			break;
 		}
-
+		seekPositionSeconds = trackRemainingSeek;
 	}
-	// trackRemainingSeek holds the SkipFragments return value of the last processed
-	// track (comment 1). All non-subtitle tracks seek to the same seekPositionSeconds so
-	// their remaining-seek values are expected to converge. HandleSeekEOSAndPeriodTransition
-	// only uses this value for a sign check (>= 0) and as the carry-over seek offset into
-	// the next period, so using the last track's value is acceptable in practice.
-	HandleSeekEOSAndPeriodTransition(trackRemainingSeek, skipToEnd);
 }
 
 /**
@@ -3637,7 +3669,7 @@ AAMPStatusType StreamAbstractionAAMP_MPD::InitTsbReader(TuneType tuneType)
 			{
 				for (int i = 0; i < mNumberOfTracks; i++)
 				{
-					if (mMediaStreamContext[i] != NULL)
+					if (mMediaStreamContext[i] != nullptr)
 					{
 						mMediaStreamContext[i]->SetLocalTSBInjection(true);
 					}

fragmentcollector_mpd.h

@@ -922,7 +922,9 @@ class StreamAbstractionAAMP_MPD : public StreamAbstractionAAMP
 	/**
 	 * @fn HandleSeekEOSAndPeriodTransition
 	 * @param remainingSeek remaining seek time after skipping fragments
-	 * @param skipToEnd true when seek operation is a seek-to-end
+	 * @param skipToEnd reserved for future use (e.g. direction-aware empty-period
+	 *                  skipping); currently unused inside this function — the caller
+	 *                  (SeekInPeriod) uses it when invoking SkipFragments directly.
 	 * @return true if period switched; false otherwise
 	 */
 	bool HandleSeekEOSAndPeriodTransition(double remainingSeek, bool skipToEnd);

test/utests/tests/StreamAbstractionAAMP_MPD/FunctionalTests.cpp

@@ -3942,6 +3942,7 @@ R"(<?xml version="1.0" encoding="utf-8"?>
 	MediaTrack *track = mStreamAbstractionAAMP_MPD->GetMediaTrack(eTRACK_VIDEO);
 	ASSERT_NE(track, nullptr);
 	MediaStreamContext *pMediaStreamContext = static_cast<MediaStreamContext *>(track);
+	(void)pMediaStreamContext;
 	//EXPECT_CALL(*g_mockPrivateInstanceAAMP, GetTSBSessionManager()).WillRepeatedly(Return(nullptr));
 
 	// Reset mFirstPTS before seek
@@ -3952,7 +3953,7 @@ R"(<?xml version="1.0" encoding="utf-8"?>
 	// with remaining seek value and still update mFirstPTS from SkipFragments path.
 	const double seekPositionSeconds = 12.0;
 	TestableFunctionalStreamAbstractionAAMP_MPD *testableStreamAbstractionAAMP_MPD =
-    dynamic_cast<TestableFunctionalStreamAbstractionAAMP_MPD *>(mStreamAbstractionAAMP_MPD);
+		dynamic_cast<TestableFunctionalStreamAbstractionAAMP_MPD *>(mStreamAbstractionAAMP_MPD);
 	ASSERT_NE(testableStreamAbstractionAAMP_MPD, nullptr);
 	testableStreamAbstractionAAMP_MPD->CallSeekInPeriod(seekPositionSeconds);
 	// Verify: mFirstPTS updated after period transition and remaining-seek skip in period-2.