VPLAY-10081:Problems with ad progress beacons by varshnie · Pull Request #782 · rdkcentral/aamp

varshnie · 2025-12-19T14:16:04Z

Reason for change:Ad position fix
Test Procedure:Refer Jira Ticket
Risks: Medium

Vinish100

Need L1

fragmentcollector_mpd.cpp

Copilot

Pull request overview

This pull request addresses issues with ad progress beacons (VPLAY-10081) by fixing the calculation of ad position timing. The fix ensures that mStartTimeOfFirstPTS is correctly set during ad playback to reflect the actual start time of ads within an ad break.

Key Changes

Sets mStartTimeOfFirstPTS to match fragmentTime when playing ads within an ad break (line 7668)
Adds logic to calculate mStartTimeOfFirstPTS for ads during discontinuity detection in two separate conditional branches (lines 9331-9339 and 9355-9363)

fragmentcollector_mpd.cpp

fragmentcollector_mpd.h

Copilot

Pull request overview

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

fragmentcollector_mpd.cpp

Reason for change:Ad position fix Test Procedure:Refer Jira Ticket Risks: Medium Signed-off-by: varshnie <varshniblue14@gmail.com>

Copilot

Pull request overview

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

Copilot · 2026-01-20T09:29:51Z

fragmentcollector_mpd.cpp

 				pMediaStreamContext->fragmentTime = mPeriodStartTime;
 				// For playing an ad in a ad break, we should update fragmentTime to PeriodStartTime + basePeriodOffset of ad;
-				if (mCdaiObject->mAdState == AdState::IN_ADBREAK_AD_PLAYING && mCdaiObject->mCurAdIdx > 0 )
+				if (mCdaiObject && mCdaiObject->mAdState == AdState::IN_ADBREAK_AD_PLAYING && mCdaiObject->mCurAdIdx > 0 


The bounds check for mCurAdIdx should include '== 0' case for completeness. The current check 'mCdaiObject->mCurAdIdx > 0' excludes index 0, which may be a valid ad index. Consider whether the bounds check should be 'mCdaiObject->mCurAdIdx >= 0' to include all valid indices, or if index 0 should be excluded with proper documentation explaining why.

Suggested change

if (mCdaiObject && mCdaiObject->mAdState == AdState::IN_ADBREAK_AD_PLAYING && mCdaiObject->mCurAdIdx > 0

if (mCdaiObject && mCdaiObject->mAdState == AdState::IN_ADBREAK_AD_PLAYING

Copilot · 2026-01-20T09:29:51Z

fragmentcollector_mpd.cpp

+		if (mCdaiObject && mCdaiObject->mAdState == AdState::IN_ADBREAK_AD_PLAYING && mCdaiObject->mCurAdIdx > 0
+	 		&& mCdaiObject->mCurAdIdx < mCdaiObject->mCurAds->size())


The bounds check for mCurAdIdx should include '== 0' case for completeness. The current check 'mCdaiObject->mCurAdIdx > 0' excludes index 0, which may be a valid ad index. Consider whether the bounds check should be 'mCdaiObject->mCurAdIdx >= 0' to include all valid indices, or if index 0 should be excluded with proper documentation explaining why.

Suggested change

if (mCdaiObject && mCdaiObject->mAdState == AdState::IN_ADBREAK_AD_PLAYING && mCdaiObject->mCurAdIdx > 0

&& mCdaiObject->mCurAdIdx < mCdaiObject->mCurAds->size())

if (mCdaiObject && mCdaiObject->mAdState == AdState::IN_ADBREAK_AD_PLAYING &&

mCdaiObject->mCurAds && mCdaiObject->mCurAdIdx < mCdaiObject->mCurAds->size())

Copilot · 2026-01-20T09:29:51Z

fragmentcollector_mpd.cpp

 				//the periodStartOffset should now be relative to the Availability Start Time.
 				mMediaStreamContext[i]->periodStartOffset = mPeriodStartTime;
-				if (mCdaiObject->mAdState == AdState::IN_ADBREAK_AD_PLAYING && mCdaiObject->mCurAdIdx > 0)
+				if (mCdaiObject != nullptr &&mCdaiObject->mAdState == AdState::IN_ADBREAK_AD_PLAYING && mCdaiObject->mCurAdIdx > 0)


Missing space after 'nullptr'. The condition should be formatted as 'mCdaiObject != nullptr && mCdaiObject->mAdState' for consistency with coding style that places spaces around operators.

Suggested change

if (mCdaiObject != nullptr &&mCdaiObject->mAdState == AdState::IN_ADBREAK_AD_PLAYING && mCdaiObject->mCurAdIdx > 0)

if (mCdaiObject != nullptr && mCdaiObject->mAdState == AdState::IN_ADBREAK_AD_PLAYING && mCdaiObject->mCurAdIdx > 0)

Copilot · 2026-01-20T09:29:52Z

fragmentcollector_mpd.cpp

 				//the periodStartOffset should now be relative to the Availability Start Time.
 				mMediaStreamContext[i]->periodStartOffset = mPeriodStartTime;
-				if (mCdaiObject->mAdState == AdState::IN_ADBREAK_AD_PLAYING && mCdaiObject->mCurAdIdx > 0)
+				if (mCdaiObject != nullptr &&mCdaiObject->mAdState == AdState::IN_ADBREAK_AD_PLAYING && mCdaiObject->mCurAdIdx > 0)


The bounds check for mCurAdIdx should include '== 0' case for completeness. The current check 'mCdaiObject->mCurAdIdx > 0' excludes index 0, which may be a valid ad index. Consider whether the bounds check should be 'mCdaiObject->mCurAdIdx >= 0' to include all valid indices, or if index 0 should be excluded with proper documentation explaining why.

Suggested change

if (mCdaiObject != nullptr &&mCdaiObject->mAdState == AdState::IN_ADBREAK_AD_PLAYING && mCdaiObject->mCurAdIdx > 0)

if (mCdaiObject != nullptr

&& mCdaiObject->mAdState == AdState::IN_ADBREAK_AD_PLAYING

&& mCdaiObject->mCurAdIdx >= 0

&& static_cast<size_t>(mCdaiObject->mCurAdIdx) < mCdaiObject->mCurAds->size())

varshnie requested a review from a team as a code owner December 19, 2025 14:16

varshnie force-pushed the feature/VPLAY-10081 branch 2 times, most recently from 5c727cb to 5e102f3 Compare January 5, 2026 13:53

Vinish100 requested changes Jan 6, 2026

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

fragmentcollector_mpd.cpp Outdated Show resolved Hide resolved

Vinish100 requested a review from Copilot January 6, 2026 06:24

Copilot started reviewing on behalf of Vinish100 January 6, 2026 06:24 View session

Copilot AI reviewed Jan 6, 2026

View reviewed changes

varshnie force-pushed the feature/VPLAY-10081 branch from 5e102f3 to fe46a02 Compare January 6, 2026 10:48

Vinish100 reviewed Jan 7, 2026

View reviewed changes

fragmentcollector_mpd.cpp Outdated Show resolved Hide resolved

varshnie force-pushed the feature/VPLAY-10081 branch from fe46a02 to 1df7952 Compare January 9, 2026 06:54

Vinish100 reviewed Jan 12, 2026

View reviewed changes

fragmentcollector_mpd.cpp Show resolved Hide resolved

fragmentcollector_mpd.cpp Outdated Show resolved Hide resolved

fragmentcollector_mpd.h Show resolved Hide resolved

fragmentcollector_mpd.h Outdated Show resolved Hide resolved

Vinish100 requested a review from Copilot January 12, 2026 09:50

Copilot started reviewing on behalf of Vinish100 January 12, 2026 09:51 View session

Copilot AI reviewed Jan 12, 2026

View reviewed changes

fragmentcollector_mpd.cpp Outdated Show resolved Hide resolved

fragmentcollector_mpd.cpp Outdated Show resolved Hide resolved

fragmentcollector_mpd.cpp Show resolved Hide resolved

fragmentcollector_mpd.cpp Outdated Show resolved Hide resolved

fragmentcollector_mpd.cpp Show resolved Hide resolved

varshnie force-pushed the feature/VPLAY-10081 branch 3 times, most recently from f803381 to 9d69e8e Compare January 13, 2026 06:58

VPLAY-10081:Problems with ad progress beacons

299956a

Reason for change:Ad position fix Test Procedure:Refer Jira Ticket Risks: Medium Signed-off-by: varshnie <varshniblue14@gmail.com>

Copilot AI review requested due to automatic review settings January 20, 2026 09:25

varshnie force-pushed the feature/VPLAY-10081 branch from 9d69e8e to 299956a Compare January 20, 2026 09:25

Copilot started reviewing on behalf of varshnie January 20, 2026 09:26 View session

Copilot AI reviewed Jan 20, 2026

View reviewed changes

Merge branch 'dev_sprint_25_2' into feature/VPLAY-10081

b794e57

pstroffolino merged commit 8455be2 into dev_sprint_25_2 Jan 21, 2026
5 of 6 checks passed

pstroffolino deleted the feature/VPLAY-10081 branch January 21, 2026 18:12

	if (mCdaiObject && mCdaiObject->mAdState == AdState::IN_ADBREAK_AD_PLAYING && mCdaiObject->mCurAdIdx > 0
	if (mCdaiObject && mCdaiObject->mAdState == AdState::IN_ADBREAK_AD_PLAYING

		if (mCdaiObject && mCdaiObject->mAdState == AdState::IN_ADBREAK_AD_PLAYING && mCdaiObject->mCurAdIdx > 0
		&& mCdaiObject->mCurAdIdx < mCdaiObject->mCurAds->size())

	if (mCdaiObject != nullptr &&mCdaiObject->mAdState == AdState::IN_ADBREAK_AD_PLAYING && mCdaiObject->mCurAdIdx > 0)
	if (mCdaiObject != nullptr && mCdaiObject->mAdState == AdState::IN_ADBREAK_AD_PLAYING && mCdaiObject->mCurAdIdx > 0)

Conversation

varshnie commented Dec 19, 2025

Uh oh!

Vinish100 left a comment

Choose a reason for hiding this comment

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

Key Changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Jan 20, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 20, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 20, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 20, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants