VPAAMP-684: Fix Tech Fault error on rewind from live edge into CDAI ads

fragmentcollector_mpd.cpp

@@ -10630,18 +10630,34 @@ void StreamAbstractionAAMP_MPD::FetcherLoop()
 				if (AdState::OUTSIDE_ADBREAK != mCdaiObject->mAdState)
 				{
 					Period2AdData &curPeriod = mCdaiObject->mPeriodMap[mBasePeriodId];
+					bool breakDownloaderLoop = false;
 					if ((mPlayRate < AAMP_RATE_PAUSE && mBasePeriodOffset <= 0) ||
 						(mPlayRate > AAMP_RATE_PAUSE && curPeriod.filled && curPeriod.duration <= (uint64_t)(mBasePeriodOffset * 1000)))
 					{
 						AAMPLOG_INFO("[CDAI]: BasePeriod[%s] completed @%lf. Changing to next ", mBasePeriodId.c_str(), mBasePeriodOffset);
-						break;
+						breakDownloaderLoop = true;
 					}
 					else if (lastPrdOffset != mBasePeriodOffset && AdState::IN_ADBREAK_AD_NOT_PLAYING == mCdaiObject->mAdState)
 					{
 						// In adbreak, but somehow Ad is not playing. Need to check whether the position reached the next Ad start.
 						adStateChanged = onAdEvent(AdEvent::BASE_OFFSET_CHANGE);
 						if (adStateChanged)
-							break;
+							breakDownloaderLoop = true;
+					}
+					// State is not OUTSIDE_ADBREAK, Ad is either ended or not playing
+					// Wait for scheduled fragments to be downloaded and cached before moving to next period.
+					if (breakDownloaderLoop)
+					{
+						aamp->GetAampTrackWorkerManager()->WaitForCompletionWithTimeout(MAX_WAIT_TIMEOUT_MS, [this]() {
+							if (mIsLiveManifest)
+							{
+								if (eAAMPSTATUS_OK != UpdateMPD())
+								{
+									AAMPLOG_DEBUG("Failed to refresh MPD");
+								}
+							}
+						});
+						break;
 					}
 					lastPrdOffset = mBasePeriodOffset;
 				}

test/utests/fakes/FakeAampTrackWorkerManager.cpp

@@ -19,6 +19,12 @@
 
 #include "AampTrackWorkerManager.hpp"
 
+/**
+ * @brief Global counter incremented each time WaitForCompletionWithTimeout is called.
+ *        Tests can reset this to 0 before invoking FetcherLoop and then check it afterwards.
+ */
+int g_waitForCompletionWithTimeoutCallCount = 0;
+
 namespace aamp
 {
 	/**
@@ -92,6 +98,7 @@ namespace aamp
 	 */
 	void AampTrackWorkerManager::WaitForCompletionWithTimeout(int timeout, std::function<void()> onTimeout)
 	{
+		g_waitForCompletionWithTimeoutCallCount++;
 	}
 
 	/**

test/utests/tests/StreamAbstractionAAMP_MPD/FetcherLoopTests.cpp

@@ -36,6 +36,7 @@
 #include "MockAampStreamSinkManager.h"
 #include "MockTSBSessionManager.h"
 #include "MockAdManager.h"
+#include "MockAampTimeBasedBufferManager.h"
 #include "AampTrackWorker.hpp"
 
 using ::testing::_;
@@ -166,6 +167,21 @@ class FetcherLoopTests : public ::testing::Test
 			mIsFogTSB = value;
 		}
 
+		void SetPlayRate(float rate)
+		{
+			mPlayRate = rate;
+		}
+
+		void SetBasePeriodOffset(double offset)
+		{
+			mBasePeriodOffset = offset;
+		}
+
+		void SetBasePeriodId(const std::string &id)
+		{
+			mBasePeriodId = id;
+		}
+
 		bool InvokeHandleSeekEOSAndPeriodTransition(double remainingSeek, bool skipToEnd)
 		{
 			return HandleSeekEOSAndPeriodTransition(remainingSeek, skipToEnd);
@@ -3327,4 +3343,179 @@ TEST_F(FetcherLoopTests, SegmentBase_SkipFragments_FloatingPointEpsilon_CrossesF
 
 	EXPECT_EQ(ctx->fragmentIndex, 1);
 	EXPECT_NEAR(ctx->fragmentTime, 2.0, 0.001);
+}
+
+// ─── VPAAMP-684 Unit Tests ────────────────────────────────────────────────────
+//
+// These tests verify the VPAAMP-684 fix: before breaking out of the segment
+// downloader loop when an ad period is complete, FetcherLoop must call
+// WaitForCompletionWithTimeout() so that any in-flight fragment downloads finish
+// before the period transition.  The global counter
+// g_waitForCompletionWithTimeoutCallCount (defined in
+// FakeAampTrackWorkerManager.cpp) is incremented by the fake stub and checked
+// after InvokeFetcherLoop() returns.
+//
+// Both tests use a LIVE manifest (type="dynamic") which is the primary scenario
+// described in VPAAMP-684: rewinding from live edge into CDAI ads.
+// With mIsLiveManifest=true the WaitForCompletionWithTimeout callback would also
+// call UpdateMPD() to refresh the manifest before the period transition.
+
+extern int g_waitForCompletionWithTimeoutCallCount;
+
+/**
+ * @brief VPAAMP-684 – live rewind: period completion in CDAI adbreak calls
+ *        WaitForCompletionWithTimeout before breaking.
+ *
+ * Scenario (live stream):
+ *   - Dynamic (live) two-period manifest, CDAI enabled, ad state IN_ADBREAK_AD_PLAYING.
+ *   - Play rate is negative (rewind, -2×) — user has rewound from live edge into an ad.
+ *   - mBasePeriodOffset is set to –0.1 (≤ 0), satisfying
+ *     (mPlayRate < AAMP_RATE_PAUSE && mBasePeriodOffset <= 0).
+ *   - mIsLiveManifest=true means UpdateMPD() executes inside the inner loop
+ *     AND the WaitForCompletionWithTimeout callback would call UpdateMPD() to
+ *     refresh the manifest before the period transition (not verified here
+ *     because the fake stub does not invoke the callback).
+ *
+ * Expected (post-fix): WaitForCompletionWithTimeout() is called at least once
+ * and FetcherLoop terminates without hanging.
+ */
+TEST_F(FetcherLoopTests, VPAAMP684_CDAI_LiveRewindPeriodComplete_WaitsForWorkers)
+{
+	// mLiveManifest is type="dynamic" — sets mIsLiveManifest=true inside Init().
+	// Seek to 10 s so Init places the playhead inside p0 (0–30 s), ensuring
+	// mBasePeriodId="p0" and mCurrentPeriodIdx=0.  Without this, NEW_NORMAL on a
+	// live manifest seeks to the live edge (p1), which would cause
+	// SelectSourceOrAdPeriod to detect a period change and reset mBasePeriodOffset.
+	std::string fragmentUrl = std::string(TEST_BASE_URL) + std::string("video_p0_init.mp4");
+	EXPECT_CALL(*g_mockMediaStreamContext, CacheFragment(fragmentUrl, _, _, _, _, true, _, _, _))
+		.WillRepeatedly(Return(true));
+
+	AAMPStatusType status = InitializeMPD(mLiveManifest, eTUNETYPE_SEEK, 10.0, AAMP_NORMAL_PLAY_RATE);
+	ASSERT_EQ(status, eAAMPSTATUS_OK);
+
+	// Prevent AdvanceTrack from being called: reporting IsFull()=true makes the
+	// inner loop skip the per-track download branch and jump straight to the CDAI
+	// period check, avoiding an infinite spin calling PushNextFragment.
+	g_mockAampTimeBasedBufferManager =
+		std::make_shared<NiceMock<aamp::MockAampTimeBasedBufferManager>>();
+	ON_CALL(*g_mockAampTimeBasedBufferManager, IsFull()).WillByDefault(Return(true));
+
+	// Set CDAI state: inside an ad break, the ad is actively playing.
+	auto cdaiObj = mTestableStreamAbstractionAAMP_MPD->GetCDAIObject();
+	cdaiObj->mAdState = AdState::IN_ADBREAK_AD_PLAYING;
+	// Provide a Period2AdData entry so the mPeriodMap lookup inside FetcherLoop
+	// does not insert a default entry with undefined content.
+	cdaiObj->mPeriodMap["p0"] = Period2AdData();
+
+	// Switch to rewind; place the downloader position at ≤ 0 so the condition
+	//   (mPlayRate < AAMP_RATE_PAUSE && mBasePeriodOffset <= 0)
+	// is satisfied on the very first inner-loop iteration.
+	mTestableStreamAbstractionAAMP_MPD->SetPlayRate(-2.0f);
+	mTestableStreamAbstractionAAMP_MPD->SetBasePeriodOffset(-0.1);
+	mTestableStreamAbstractionAAMP_MPD->SetBasePeriodId("p0");
+
+	// Keep downloads enabled throughout so the loop does not exit via the
+	// disabled path before reaching the CDAI check.
+	// For live, UpdateMPD() is executed each inner-loop iteration; the
+	// GetManifest mock (set up in InitializeMPD with WillRepeatedly) handles
+	// the repeated manifest-refresh calls transparently.
+	EXPECT_CALL(*g_mockPrivateInstanceAAMP, DownloadsAreEnabled())
+		.Times(AnyNumber())
+		.WillRepeatedly(Return(true));
+	EXPECT_CALL(*g_mockPrivateInstanceAAMP, IsLocalAAMPTsbInjection()).WillRepeatedly(Return(false));
+	EXPECT_CALL(*g_mockPrivateInstanceAAMP, GetTSBSessionManager()).WillRepeatedly(Return(nullptr));
+
+	g_waitForCompletionWithTimeoutCallCount = 0;
+
+	mTestableStreamAbstractionAAMP_MPD->InvokeInitializeWorkers();
+	mTestableStreamAbstractionAAMP_MPD->InvokeFetcherLoop();
+
+	// Core assertion: the VPAAMP-684 fix must call WaitForCompletionWithTimeout
+	// before breaking out of the inner loop, giving in-flight worker-thread
+	// downloads a chance to finish before the period transition.
+	EXPECT_GE(g_waitForCompletionWithTimeoutCallCount, 1)
+		<< "WaitForCompletionWithTimeout was not called; VPAAMP-684 fix may be missing";
+
+	g_mockAampTimeBasedBufferManager.reset();
+}
+
+/**
+ * @brief VPAAMP-684 – live forward play: period duration exhausted in CDAI
+ *        adbreak calls WaitForCompletionWithTimeout before breaking.
+ *
+ * Scenario (live stream):
+ *   - Dynamic (live) two-period manifest, CDAI enabled, ad state IN_ADBREAK_AD_PLAYING.
+ *   - Play rate is normal (1×).
+ *   - The period is marked filled=true with a 30-second duration.
+ *   - mBasePeriodOffset is set to 30.1 s (30 100 ms > 30 000 ms), satisfying
+ *     (mPlayRate > AAMP_RATE_PAUSE && curPeriod.filled
+ *      && curPeriod.duration <= mBasePeriodOffset*1000).
+ *   - DownloadsAreEnabled() returns false on the third call so FetcherLoop
+ *     exits cleanly through the outer-loop guard after the CDAI break, instead
+ *     of entering a second SelectSourceOrAdPeriod which would attempt a period
+ *     change that requires extra state setup.
+ *
+ * Expected (post-fix): WaitForCompletionWithTimeout() is called at least once
+ * and FetcherLoop terminates without hanging.
+ */
+TEST_F(FetcherLoopTests, VPAAMP684_CDAI_LiveForwardPlayPeriodComplete_WaitsForWorkers)
+{
+	// mLiveManifest is type="dynamic" — two periods p0 (0–30 s) and p1 (30 s+).
+	// Seek to 10 s to land inside p0, so mBasePeriodId="p0" and
+	// mCurrentPeriodIdx=0 after Init.  A eTUNETYPE_NEW_NORMAL on a live manifest
+	// would jump to the live edge (p1) instead, conflicting with the "p0" state
+	// we configure below and causing SelectSourceOrAdPeriod to reset the offset.
+	std::string fragmentUrl = std::string(TEST_BASE_URL) + std::string("video_p0_init.mp4");
+	EXPECT_CALL(*g_mockMediaStreamContext, CacheFragment(fragmentUrl, _, _, _, _, true, _, _, _))
+		.WillRepeatedly(Return(true));
+
+	AAMPStatusType status = InitializeMPD(mLiveManifest, eTUNETYPE_SEEK, 10.0, AAMP_NORMAL_PLAY_RATE);
+	ASSERT_EQ(status, eAAMPSTATUS_OK);
+
+	// Prevent AdvanceTrack from being called by making the time-based buffer
+	// report full, so the inner loop reaches the CDAI check without downloading.
+	g_mockAampTimeBasedBufferManager =
+		std::make_shared<NiceMock<aamp::MockAampTimeBasedBufferManager>>();
+	ON_CALL(*g_mockAampTimeBasedBufferManager, IsFull()).WillByDefault(Return(true));
+
+	// Set CDAI state: inside an ad break, the ad is actively playing.
+	auto cdaiObj = mTestableStreamAbstractionAAMP_MPD->GetCDAIObject();
+	cdaiObj->mAdState = AdState::IN_ADBREAK_AD_PLAYING;
+
+	// Mark period p0 as fully filled with a 30-second ad (30 000 ms).
+	// Condition fired:
+	//   curPeriod.filled (true) && curPeriod.duration (30000) <= 30.1*1000 (30100)
+	cdaiObj->mPeriodMap["p0"] = Period2AdData(true /*filled*/, "p0", 30000 /*ms*/, {});
+
+	// Downloader position is 100 ms past the end of the ad period.
+	mTestableStreamAbstractionAAMP_MPD->SetBasePeriodOffset(30.1);
+	mTestableStreamAbstractionAAMP_MPD->SetBasePeriodId("p0");
+
+	// DownloadsAreEnabled() call sequence:
+	//   Call 1 – outer loop guard (iteration 1)         → true  (run inner loop)
+	//   Call 2 – inner loop guard                       → true  (reach CDAI check)
+	//   Call 3 – outer loop guard (iteration 2, after   → false (exit cleanly before
+	//             mIterPeriodIndex++ makes it 1 and           SelectSourceOrAdPeriod
+	//             SelectSourceOrAdPeriod would try a          attempts a period change)
+	//             live period change with IN_ADBREAK state)
+	int downloadCallCount = 0;
+	EXPECT_CALL(*g_mockPrivateInstanceAAMP, DownloadsAreEnabled())
+		.Times(AnyNumber())
+		.WillRepeatedly([&downloadCallCount]() {
+			return ++downloadCallCount <= 2;
+		});
+	EXPECT_CALL(*g_mockPrivateInstanceAAMP, IsLocalAAMPTsbInjection()).WillRepeatedly(Return(false));
+	EXPECT_CALL(*g_mockPrivateInstanceAAMP, GetTSBSessionManager()).WillRepeatedly(Return(nullptr));
+
+	g_waitForCompletionWithTimeoutCallCount = 0;
+
+	mTestableStreamAbstractionAAMP_MPD->InvokeInitializeWorkers();
+	mTestableStreamAbstractionAAMP_MPD->InvokeFetcherLoop();
+
+	// Core assertion: the VPAAMP-684 fix must call WaitForCompletionWithTimeout
+	// before breaking out of the inner loop.
+	EXPECT_GE(g_waitForCompletionWithTimeoutCallCount, 1)
+		<< "WaitForCompletionWithTimeout was not called; VPAAMP-684 fix may be missing";
+
+	g_mockAampTimeBasedBufferManager.reset();
 }