VPLAY-11126 [AAMP TSB] TsbReader waits for fragments in a tight loop by hthakkar-synamedia · Pull Request #606 · rdkcentral/aamp

hthakkar-synamedia · 2025-10-21T17:25:31Z

Reason for Change: When TsbReader tries to read the fragments that are not downloaded yet by the FetcherLoop, it enters into a quick tight loop. Fix here implements producer consumer design and wait for a conditional variable until new fragment is available.
Added L1 microtest
Test Procedure: - Refer Ticket
Risks:low

Reason for Change: When TsbReader tries to read the fragments that are not downloaded yet by the FetcherLoop, it enters into a quick tight loop. Fix here implements producer consumer design and wait for a conditional variable until new fragment is available. Test Procedure: - Refer Ticket Risks:low

test/utests/fakes/FakeTSBSessionManager.cpp

test/utests/tests/FragmentCollectorMpdTests/FragmentCollectorMpdTestCases.cpp

AampTSBSessionManager.cpp

AampTSBSessionManager.h

fragmentcollector_mpd.cpp

Reason for Change: When TsbReader tries to read the fragments that are not downloaded yet by the FetcherLoop, it enters into a quick tight loop. Fix here implements producer consumer design and wait for a conditional variable until new fragment is available. Test Procedure: - Refer Ticket Risks:low

hthakkar-synamedia · 2025-10-23T12:47:06Z

Addressed the comments

hthakkar-synamedia

Addressed the comments

AampTSBSessionManager.cpp

fragmentcollector_mpd.cpp

Reason for Change: When TsbReader tries to read the fragments that are not downloaded yet by the FetcherLoop, it enters into a quick tight loop. Fix here implements producer consumer design and wait for a conditional variable until new fragment is available. Test Procedure: - Refer Ticket Risks:low

hthakkar-synamedia

fixed typo and modified the RaiseNewVideoTsbContentNotification function

fragmentcollector_mpd.cpp

AampTSBSessionManager.cpp

AampTSBSessionManager.h

priv_aamp.cpp

Reason for Change: When TsbReader tries to read the fragments that are not downloaded yet by the FetcherLoop, it enters into a quick tight loop. Fix here implements producer consumer design and wait for a conditional variable until new fragment is available. Test Procedure: - Refer Ticket Risks:low

Copilot

Pull Request Overview

This PR enhances the TSB (Time Shift Buffer) fragment fetching mechanism by adding a return value to AdvanceTsbFetch to indicate whether a segment was successfully cached, and implements a condition variable-based waiting mechanism to efficiently handle scenarios where no new fragments are available in the TSB.

Key Changes:

Modified AdvanceTsbFetch to return bool indicating if a fragment was cached
Added condition variable mNewVideoTsbContentCV to signal when new video content is written to TSB
Implemented WaitForNewVideoTsbFragment() to block until new content is available or downloads are disabled
Replaced polling with event-driven waiting in the TsbReader loop

Reviewed Changes

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

Show a summary per file

File	Description
fragmentcollector_mpd.h	Updated method signature and documentation for `AdvanceTsbFetch` to return bool
fragmentcollector_mpd.cpp	Implemented return value logic, added segment tracking, and replaced sleep with condition variable waits
AampTSBSessionManager.h	Added new methods and members for condition variable-based notification
AampTSBSessionManager.cpp	Implemented wait/notify mechanism and initialized new member variable
priv_aamp.cpp	Added notification to unblock waiting threads when downloads are disabled
test/utests/tests/FragmentCollectorMpdTests/FragmentCollectorMpdTestCases.cpp	Updated tests to capture and verify the return value
test/utests/fakes/FakeTSBSessionManager.cpp	Added stub implementations for new public methods

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

fragmentcollector_mpd.cpp

Reason for Change: When TsbReader tries to read the fragments that are not downloaded yet by the FetcherLoop, it enters into a quick tight loop. Fix here implements producer consumer design and wait for a conditional variable until new fragment is available. Test Procedure: - Refer Ticket Risks:low

Reason for Change: When TsbReader tries to read the fragments that are not downloaded yet by the FetcherLoop, it enters into a quick tight loop. Fix here implements producer consumer design and wait for a conditional variable until new fragment is available. Test Procedure: - Refer Ticket Risks:low Merge branch 'dev_sprint_25_2' of github.com:rdkcentral/aamp into feature/VPLAY-11126

Copilot

Pull Request Overview

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

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

fragmentcollector_mpd.cpp

AampTSBSessionManager.cpp

fragmentcollector_mpd.cpp

Reason for Change: When TsbReader tries to read the fragments that are not downloaded yet by the FetcherLoop, it enters into a quick tight loop. Fix here implements producer consumer design and wait for a conditional variable until new fragment is available. Test Procedure: - Refer Ticket Risks:low

AampTSBSessionManager.cpp

fragmentcollector_mpd.cpp

…ture/VPLAY-11126

Reason for Change: When TsbReader tries to read the fragments that are not downloaded yet by the FetcherLoop, it enters into a quick tight loop. Fix here implements producer consumer design and wait for a conditional variable until new fragment is available. Test Procedure: - Refer Ticket Risks:low

AampTSBSessionManager.cpp

Avoid race where the TsbReader thread reads predicate between flag being set and the notify firing. Before joining tsbReaderThreadID, NotifyVideoTsbWaiters() is now called, avoiding potential deadlock: Avoid tight loop and log spam when rate <= AAMP_NORMAL_PLAY_RATE and no segment is found. Signed-off-by: Philip Stroffolino <philip_stroffolino@cable.comcast.com>

Copilot

Pull request overview

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

test/utests/tests/AampTsbSessionManagerTests_Mocked/FunctionalTests.cpp

test/utests/fakes/FakeTSBSessionManager.cpp

fragmentcollector_mpd.cpp

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

Copilot

Pull request overview

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

Copilot · 2026-03-02T00:53:40Z

fragmentcollector_mpd.cpp

+					if (segmentFound)
+					{
+						aamp->interruptibleMsSleep(50);				//To Avoid tight loop adding a small delay
+					}
+					// AAMP could reach the end of the TSB only when doing FF (rate > AAMP_NORMAL_PLAY_RATE)
+					else if (aamp->rate > AAMP_NORMAL_PLAY_RATE)
+					{
+						// All the segments in TSB have been sent to gstreamer; avoid indefinite
+						// blocking in WaitForVideoTsbContentOrAbort by using a bounded, interruptible
+						// sleep before retrying the fetch loop.
+						aamp->interruptibleMsSleep(50);
+					}
+					else
+					{
+						// Unexpected: no segment found at normal/slow rate outside of EOS. Sleep to
+						// avoid a tight loop and throttle log output until the condition resolves.
+						AAMPLOG_TRACE("No segment found for rate <= AAMP_NORMAL_PLAY_RATE, retrying");
+						aamp->interruptibleMsSleep(50);


The PR description says TsbReader will block on a condition variable until new fragments are available, but this loop still only polls with interruptibleMsSleep(50) and never calls AampTSBSessionManager::WaitForVideoTsbContentOrAbort(). As a result, the newly added Wait/Notify mechanism is unused in production and the tight-loop/polling behavior remains (the comment even references WaitForVideoTsbContentOrAbort without actually using it). Consider calling WaitForVideoTsbContentOrAbort() when no segments were cached and the cache is not full, and ensure the wait can be interrupted on abort/stop paths.

is this valid issue? If so, how important to address?

I do not think this is a valid issue

test/utests/tests/AampTsbSessionManagerTests_Mocked/FunctionalTests.cpp

Copilot

Pull request overview

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

test/utests/tests/AampTsbSessionManagerTests_Mocked/FunctionalTests.cpp

fragmentcollector_mpd.cpp

Copilot · 2026-03-02T01:59:24Z

fragmentcollector_mpd.cpp

+		// Unblock TsbReader if it is waiting for new video TSB content (e.g. during FF
+		// on local AAMP TSB where DisableDownloads is not called and therefore
+		// NotifyVideoTsbWaiters is not triggered via the downloads-disabled path).
+		if (AampTSBSessionManager *tsbMgr = aamp->GetTSBSessionManager())
+		{
+			tsbMgr->NotifyVideoTsbWaiters();
+		}


This stop-path comment says it is unblocking TsbReader waiting on WaitForVideoTsbContentOrAbort(), but StreamAbstractionAAMP_MPD::TsbReader() never calls WaitForVideoTsbContentOrAbort() (it only uses WaitForManifestUpdate() / WaitForCachedFragmentChunkInjected() / sleeps). Either wire TsbReader() up to the new condition variable as intended, or adjust/remove this comment + notification so it matches actual behavior.

valid issue?

Signed-off-by: Philip Stroffolino <philip_stroffolino@cable.comcast.com>

Copilot

Pull request overview

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

test/utests/tests/AampTsbSessionManagerTests_Mocked/FunctionalTests.cpp

AampTSBSessionManager.h

AampTSBSessionManager.cpp

…ture/VPLAY-11126

… feature/VPLAY-11126

Copilot

Pull request overview

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

Copilot · 2026-03-02T15:02:53Z

test/utests/tests/AampTsbSessionManagerTests_Mocked/FunctionalTests.cpp

+	// Brief sleep to allow thread to enter waiting state
+	// Note: This is a timing assumption, but std::thread provides no "is waiting" status check
+	std::this_thread::sleep_for(std::chrono::milliseconds(10));


This test relies on a fixed sleep to “let the consumer thread enter waiting state”. Fixed timing sleeps can be flaky across different CI loads and platforms. Prefer a deterministic handshake (e.g., a promise/condition_variable that the consumer signals after it is ready to wait) so the test does not depend on scheduling.

Copilot · 2026-03-02T15:02:53Z

test/utests/tests/AampTsbSessionManagerTests_Mocked/FunctionalTests.cpp

+	std::thread consumerThread([this]() {
+		mAampTSBSessionManager->WaitForVideoTsbContentOrAbort();
+	});
+
+	// Brief sleep to allow thread to enter waiting state
+	// Note: This is a timing assumption, but std::thread provides no "is waiting" status check
+	std::this_thread::sleep_for(std::chrono::milliseconds(10));
+
+	mAampTSBSessionManager->NotifyVideoTsbWaiter();
+
+	consumerThread.join();


If WaitForVideoTsbContentOrAbort() fails to unblock, this join will hang the entire test binary indefinitely. Consider adding a bounded wait (e.g., run the wait in a future and ASSERT that it completes within a timeout) so failures are reported rather than deadlocking CI.

Copilot

Pull request overview

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

Copilot · 2026-03-04T15:46:26Z

test/utests/tests/FragmentCollectorMpdTests/FragmentCollectorMpdTestCases.cpp

 	EXPECT_CALL(*g_mockTSBReader, IsEos()).WillOnce(Return(false));
 	EXPECT_CALL(*g_mockTSBSessionManager, PushNextTsbFragment(mediaStreamContext, _)).WillOnce(Return(true));

 	// Call the protected method through testable wrapper
-	mMpdStream->TestAdvanceTsbFetch(trackIdx, trickPlay, delta, waitForFreeFrag, bCacheFullState);
+	bool result = mMpdStream->TestAdvanceTsbFetch(trackIdx, trickPlay, delta, waitForFreeFrag, bCacheFullState);


The expectations are set on g_mockTSBReader, but GetTsbReader is configured to return a real AampTsbReader (tsbReader). As a result, the EXPECT_CALLs on g_mockTSBReader will never be satisfied and this test will fail. Return g_mockTSBReader (as a shared_ptr) from GetTsbReader or set expectations against the actual object being used.

Copilot · 2026-03-04T15:46:27Z

test/utests/tests/FragmentCollectorMpdTests/FragmentCollectorMpdTestCases.cpp

 	// Call the protected method through testable wrapper
-	mMpdStream->TestAdvanceTsbFetch(trackIdx, trickPlay, delta, waitForFreeFrag, bCacheFullState);
+	bool result = mMpdStream->TestAdvanceTsbFetch(trackIdx, trickPlay, delta, waitForFreeFrag, bCacheFullState);


This test allocates MediaStreamContext with new and never deletes it. Please switch to RAII (e.g., std::unique_ptr) or ensure it is deleted at the end of the test to avoid leaking across the unit test process.

Copilot · 2026-03-04T15:46:27Z

test/utests/tests/FragmentCollectorMpdTests/FragmentCollectorMpdTestCases.cpp

 	// Verify that PushNextTsbFragment is NOT called when track is disabled
 	EXPECT_CALL(*g_mockTSBSessionManager, PushNextTsbFragment(mediaStreamContext, _)).Times(0);

 	// Call the protected method through testable wrapper
-	mMpdStream->TestAdvanceTsbFetch(trackIdx, trickPlay, delta, waitForFreeFrag, bCacheFullState);
+	bool result = mMpdStream->TestAdvanceTsbFetch(trackIdx, trickPlay, delta, waitForFreeFrag, bCacheFullState);


Same issue as above: g_mockTSBReader has EXPECT_CALL(TrackEnabled) but GetTsbReader returns a real AampTsbReader instance (tsbReader). The TrackEnabled expectation will be unmet and the test will fail. Configure GetTsbReader to return g_mockTSBReader (or otherwise ensure expectations are set on the object actually used).

Copilot · 2026-03-04T15:46:27Z

test/utests/tests/AampTsbSessionManagerTests_Mocked/FunctionalTests.cpp

+	mAampTSBSessionManager->NotifyVideoTsbWaiter();
+
+	// Spawn a thread that waits for the notification
+	std::thread consumerThread([this]() {
+		mAampTSBSessionManager->WaitForVideoTsbContentOrAbort();
+	});
+
+	consumerThread.join();


Same as the previous test: this thread join is unbounded. Please add a bounded wait/timeout-based assertion (e.g., future.wait_for) so a regression cannot deadlock the full test suite.

hthakkar-synamedia requested review from DomSyna and jfagunde October 21, 2025 17:25

DomSyna reviewed Oct 22, 2025

View reviewed changes

hthakkar-synamedia commented Oct 23, 2025

View reviewed changes

DomSyna reviewed Oct 23, 2025

View reviewed changes

AampTSBSessionManager.cpp Outdated Show resolved Hide resolved

fragmentcollector_mpd.cpp Outdated Show resolved Hide resolved

hthakkar-synamedia commented Oct 23, 2025

View reviewed changes

jfagunde reviewed Oct 28, 2025

View reviewed changes

Copilot AI review requested due to automatic review settings October 28, 2025 18:13

Copilot AI reviewed Oct 28, 2025

View reviewed changes

fragmentcollector_mpd.cpp Outdated Show resolved Hide resolved

fragmentcollector_mpd.cpp Outdated Show resolved Hide resolved

fragmentcollector_mpd.cpp Outdated Show resolved Hide resolved

hthakkar-synamedia added 2 commits October 29, 2025 09:06

Copilot AI review requested due to automatic review settings October 29, 2025 09:55

Copilot AI reviewed Oct 29, 2025

View reviewed changes

fragmentcollector_mpd.cpp Outdated Show resolved Hide resolved

AampTSBSessionManager.cpp Outdated Show resolved Hide resolved

fragmentcollector_mpd.cpp Outdated Show resolved Hide resolved

jfagunde approved these changes Oct 31, 2025

View reviewed changes

DomSyna reviewed Nov 3, 2025

View reviewed changes

AampTSBSessionManager.cpp Outdated Show resolved Hide resolved

AampTSBSessionManager.cpp Outdated Show resolved Hide resolved

fragmentcollector_mpd.cpp Outdated Show resolved Hide resolved

hthakkar-synamedia and others added 2 commits February 10, 2026 16:38

Merge branch 'dev_sprint_25_2' of github.com:rdkcentral/aamp into fea…

081bfd4

…ture/VPLAY-11126

jfagunde reviewed Feb 19, 2026

View reviewed changes

AampTSBSessionManager.cpp Outdated Show resolved Hide resolved

AampTSBSessionManager.cpp Outdated Show resolved Hide resolved

Addressed review comments and added L1 microtests

2079958

jfagunde approved these changes Feb 20, 2026

View reviewed changes

hthakkar-synamedia marked this pull request as ready for review February 20, 2026 17:18

hthakkar-synamedia requested a review from a team as a code owner February 20, 2026 17:18

Copilot AI review requested due to automatic review settings February 20, 2026 17:18

Copilot started reviewing on behalf of hthakkar-synamedia February 20, 2026 17:18 View session

pstroffolino requested a review from Copilot February 28, 2026 06:22

Copilot started reviewing on behalf of pstroffolino February 28, 2026 06:22 View session

Copilot AI reviewed Feb 28, 2026

View reviewed changes

test/utests/tests/AampTsbSessionManagerTests_Mocked/FunctionalTests.cpp Outdated Show resolved Hide resolved

test/utests/fakes/FakeTSBSessionManager.cpp Show resolved Hide resolved

fragmentcollector_mpd.cpp Outdated Show resolved Hide resolved

pstroffolino and others added 2 commits March 1, 2026 14:24

Merge branch 'dev_sprint_25_2' into feature/VPLAY-11126

31915e3

Apply suggestions from code review

b3fa98b

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

Copilot AI review requested due to automatic review settings March 2, 2026 00:48

Copilot started reviewing on behalf of pstroffolino March 2, 2026 00:49 View session

Copilot AI reviewed Mar 2, 2026

View reviewed changes

Update fragmentcollector_mpd.cpp

e3fcc40

Copilot AI review requested due to automatic review settings March 2, 2026 01:52

Copilot started reviewing on behalf of pstroffolino March 2, 2026 01:53 View session

Copilot AI reviewed Mar 2, 2026

View reviewed changes

Reason for Change: reverted two most recent commits in this PR

c638bbf

Signed-off-by: Philip Stroffolino <philip_stroffolino@cable.comcast.com>

pstroffolino requested a review from Copilot March 2, 2026 02:19

Copilot started reviewing on behalf of pstroffolino March 2, 2026 02:19 View session

Copilot AI reviewed Mar 2, 2026

View reviewed changes

test/utests/tests/AampTsbSessionManagerTests_Mocked/FunctionalTests.cpp Outdated Show resolved Hide resolved

AampTSBSessionManager.h Outdated Show resolved Hide resolved

AampTSBSessionManager.cpp Show resolved Hide resolved

pstroffolino and others added 4 commits March 2, 2026 09:11

Merge branch 'dev_sprint_25_2' into feature/VPLAY-11126

f7f5adc

Addressing co-pilot comments

d081256

Merge branch 'dev_sprint_25_2' of github.com:rdkcentral/aamp into fea…

6055d19

…ture/VPLAY-11126

Merge branch 'feature/VPLAY-11126' of github.com:rdkcentral/aamp into…

678dc16

… feature/VPLAY-11126

Copilot AI review requested due to automatic review settings March 2, 2026 14:57

Copilot started reviewing on behalf of hthakkar-synamedia March 2, 2026 14:58 View session

Copilot AI reviewed Mar 2, 2026

View reviewed changes

hthakkar-synamedia and others added 2 commits March 2, 2026 15:30

Correcting the ERROR to ERR

05b8128

Merge branch 'dev_sprint_25_2' into feature/VPLAY-11126

c19c506

Copilot AI review requested due to automatic review settings March 4, 2026 15:40

Copilot started reviewing on behalf of pstroffolino March 4, 2026 15:41 View session

Copilot AI reviewed Mar 4, 2026

View reviewed changes

hthakkar-synamedia marked this pull request as draft March 4, 2026 16:33

Conversation

hthakkar-synamedia commented Oct 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

hthakkar-synamedia commented Oct 23, 2025

Uh oh!

hthakkar-synamedia left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

hthakkar-synamedia left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

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 Mar 2, 2026

Choose a reason for hiding this comment

Uh oh!

pstroffolino Mar 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hthakkar-synamedia Mar 2, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Copilot AI Mar 2, 2026

hthakkar-synamedia commented Oct 21, 2025 •

edited

Loading

pstroffolino Mar 2, 2026 •

edited

Loading