VPLAY-11600 IsEmptyPeriod() with checkIframe=false when using AAMP TSB #725

jfagunde · 2025-12-03T10:21:00Z

Reason for Change: Change necessary prior to fixing VPLAY-11583

Summary of Changes:

Add ShouldCheckOnlyIframeAdaptation() helper function,
which returns false for AAMP TSB
Add IsEmptyPeriod() helper function
Replace StreamAbstraction mPlayRate with the new helper functions
Improved functions comments

Test Procedure: No regressions with AAMP TSB

Risk: Low

Reason for Change: This needs fixing before we can fix VPLAY-11583 Summary of Changes: * Call IsEmptyPeriod() with checkIframe=false for AAMP TSB * Use aamp->rate instead of rate in IsEmptyPeriod() * Improve comments and parameters name for GetPeriodDuration() and GetPeriodEndTime() Test Procedure: Verify no regressions when using AAMP TSB Risk: Low Signed-off-by: Jose Fagundez <[email protected]>

…ture/VPLAY-11600

Copilot

Pull request overview

This PR refactors the logic for checking empty periods in MPEG-DASH playback to support AAMP TSB (Time Shift Buffer) functionality. The key change introduces two new helper functions that centralize the decision of whether to check only iframe adaptations when determining if a period is empty. When using local AAMP TSB, the code now checks all adaptations regardless of playback rate, since the iframe track is not used in TSB mode.

Key Changes:

Added ShouldCheckOnlyIframeAdaptation() helper to encapsulate the logic for determining whether to check only iframe adaptations based on playback rate and TSB mode
Added IsEmptyPeriod() wrapper that uses the new helper to call the underlying MPD parse helper
Replaced 30+ call sites throughout fragmentcollector_mpd.cpp to use the new centralized functions

Reviewed changes

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

File	Description
fragmentcollector_mpd.h	Added declarations for two new helper functions: `ShouldCheckOnlyIframeAdaptation()` and `IsEmptyPeriod()`
fragmentcollector_mpd.cpp	Implemented the two new helper functions and refactored all call sites to use them instead of inline checks, centralizing TSB-aware logic
AampMPDParseHelper.h	Improved documentation for `GetPeriodDuration()`, `GetPeriodEndTime()`, and `UpdateBoundaryPeriod()` with detailed parameter descriptions
AampMPDParseHelper.cpp	Updated documentation for `UpdateBoundaryPeriod()` and `GetPeriodEndTime()` to clarify parameter meanings

fragmentcollector_mpd.h

fragmentcollector_mpd.cpp

Address review comments made by Copilot Co-authored-by: Copilot <[email protected]>

Copilot

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

fragmentcollector_mpd.cpp

Address review comment made by Copilot Co-authored-by: Copilot <[email protected]>

Copilot

Pull request overview

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

AampMPDParseHelper.h

AampMPDParseHelper.cpp

fragmentcollector_mpd.h

fragmentcollector_mpd.cpp

Co-authored-by: Copilot <[email protected]>

…ture/VPLAY-11600

Copilot

Pull request overview

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

Copilot · 2025-12-05T13:23:29Z

fragmentcollector_mpd.cpp

+bool StreamAbstractionAAMP_MPD::ShouldCheckOnlyIframeAdaptation() const
+{
+	// If playing at normal rate, check all adaptations.
+	// For trick modes, check only iframe adaptations (unless using AAMP TSB)
+	bool checkOnlyIframeAdaptation = (aamp->rate != AAMP_NORMAL_PLAY_RATE);
+
+	// When using local AAMP TSB, i/frame track is not used,
+	// so check all adaptations regardless of playback rate.
+	if (aamp->IsLocalAAMPTsb())
+	{
+		checkOnlyIframeAdaptation = false;
+	}
+
+	return checkOnlyIframeAdaptation;


The ShouldCheckOnlyIframeAdaptation() helper uses aamp->rate directly, which differs from the original behavior that captured currentRate at the start of Init() (line 3198) and used that snapshot throughout the function. This change means the rate is now evaluated dynamically on each call rather than using a consistent snapshot.

If aamp->rate can change during the execution of Init() or other methods, this could lead to inconsistent behavior where different calls to IsEmptyPeriod() or GetPeriodDuration() within the same method use different rate values.

Consider one of these approaches:

If the rate is guaranteed not to change during these operations, document this assumption clearly

If the rate can change, restore the original behavior by passing the rate as a parameter to the helper functions

Add synchronization to ensure consistent rate evaluation

Since this affects TSB behavior (the primary purpose of this PR), this behavioral change should be explicitly validated.

Copilot · 2025-12-05T13:23:30Z

fragmentcollector_mpd.cpp

+bool StreamAbstractionAAMP_MPD::ShouldCheckOnlyIframeAdaptation() const
+{
+	// If playing at normal rate, check all adaptations.
+	// For trick modes, check only iframe adaptations (unless using AAMP TSB)
+	bool checkOnlyIframeAdaptation = (aamp->rate != AAMP_NORMAL_PLAY_RATE);
+
+	// When using local AAMP TSB, i/frame track is not used,
+	// so check all adaptations regardless of playback rate.
+	if (aamp->IsLocalAAMPTsb())
+	{
+		checkOnlyIframeAdaptation = false;
+	}
+
+	return checkOnlyIframeAdaptation;
+}
+
+/**
+ * @fn IsEmptyPeriod
+ * @param[in] iPeriodIndex Index of the period to check whether it is empty
+ * @retval Return true on empty Period
+ */
+bool StreamAbstractionAAMP_MPD::IsEmptyPeriod(int iPeriodIndex) const
+{
+	return mMPDParseHelper->IsEmptyPeriod(iPeriodIndex, ShouldCheckOnlyIframeAdaptation());
+}


The new helper functions ShouldCheckOnlyIframeAdaptation() and IsEmptyPeriod() lack test coverage. Given that:

These functions encapsulate critical TSB-specific logic

The PR description states "Change necessary prior to fixing VPLAY-11583"

Similar components like AampTsbReader, AampTsbDataManager have comprehensive test coverage in test/utests/tests/

Unit tests should be added to verify:

Normal playback rate returns false for ShouldCheckOnlyIframeAdaptation()

Trick mode (non-normal rate) returns true when not using AAMP TSB

AAMP TSB mode always returns false regardless of rate

IsEmptyPeriod() correctly delegates to mMPDParseHelper->IsEmptyPeriod() with the right parameter

jfagunde added 4 commits October 31, 2025 13:30

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

b065a94

…ture/VPLAY-11600

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

10582d5

…ture/VPLAY-11600

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

ef5a92d

…ture/VPLAY-11600

Copilot AI review requested due to automatic review settings December 3, 2025 10:21

Copilot started reviewing on behalf of jfagunde December 3, 2025 10:21 View session

Copilot finished reviewing on behalf of jfagunde December 3, 2025 10:23

Copilot AI reviewed Dec 3, 2025

View reviewed changes

Apply suggestions from code review

0b1c778

Address review comments made by Copilot Co-authored-by: Copilot <[email protected]>

Copilot AI review requested due to automatic review settings December 3, 2025 10:49

Copilot started reviewing on behalf of jfagunde December 3, 2025 10:49 View session

Copilot finished reviewing on behalf of jfagunde December 3, 2025 10:52

Copilot AI reviewed Dec 3, 2025

View reviewed changes

fragmentcollector_mpd.cpp Outdated Show resolved Hide resolved

Apply suggestions from code review

0054a1c

Address review comment made by Copilot Co-authored-by: Copilot <[email protected]>

Copilot AI review requested due to automatic review settings December 3, 2025 10:54

Copilot started reviewing on behalf of jfagunde December 3, 2025 10:55 View session

Copilot finished reviewing on behalf of jfagunde December 3, 2025 10:58

Copilot AI reviewed Dec 3, 2025

View reviewed changes

jfagunde and others added 3 commits December 5, 2025 12:12

Apply suggestions from code review

3f22f9e

Co-authored-by: Copilot <[email protected]>

Fix build failure and add comment

94643f1

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

7d54c6a

…ture/VPLAY-11600

jfagunde requested a review from Copilot December 5, 2025 13:19

Copilot started reviewing on behalf of jfagunde December 5, 2025 13:19 View session

Copilot finished reviewing on behalf of jfagunde December 5, 2025 13:22

Copilot AI reviewed Dec 5, 2025

View reviewed changes

Test ShouldCheckOnlyIframeAdaptation() in L1 tests

8cefb77

VPLAY-11600 IsEmptyPeriod() with checkIframe=false when using AAMP TSB #725

Are you sure you want to change the base?

VPLAY-11600 IsEmptyPeriod() with checkIframe=false when using AAMP TSB #725

Uh oh!

Conversation

jfagunde commented Dec 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

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!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Dec 5, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 5, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

jfagunde commented Dec 3, 2025 •

edited

Loading