VPAAMP-205: fix reverse trickplay oscillation - guard against download-loop-exit re-entry#1443
Open
pstroffolino wants to merge 2 commits into
Open
VPAAMP-205: fix reverse trickplay oscillation - guard against download-loop-exit re-entry#1443pstroffolino wants to merge 2 commits into
pstroffolino wants to merge 2 commits into
Conversation
…d-loop-exit re-entry Root cause (Scenario A) ----------------------- ec74c60 oscillation guard only covers Scenario B: All Ads Finished fires inside SelectSourceOrAdPeriod's own onAdEvent(DEFAULT) call, where snapPlayingBreakId captures mCurPlayingBreakId before the transition clears it. In Scenario A (the device failure observed in QA), All Ads Finished fires from the segment download loop before SelectSourceOrAdPeriod is entered. mCurPlayingBreakId is already empty. snapPlayingBreakId captures empty string, the ec74c60 guard never fires, and the first onAdEvent(DEFAULT) call immediately re-enters the same adbreak via CheckForAdStart (rate<0, key==end). Repeated oscillation cycles fed GStreamer mismatched init segments -> error 80:1. Fix --- Add mLastCompletedBreakId to PrivateCDAIObjectMPD. Set it before clearing mCurPlayingBreakId in all three onAdEvent clear sites (ADBREAK ENDED, BUG path, All Ads Finished). Persists across the download-loop / SelectSourceOrAdPeriod boundary. In SelectSourceOrAdPeriod, before the first onAdEvent call, check mLastCompletedBreakId against mPeriodMap[mBasePeriodId].adBreakId. On match, skip the probe (skipProbeForDownloadLoopExit=true, adStateChanged=false) and consume mLastCompletedBreakId as a one-shot. The ec74c60 guard is retained for Scenario B. Back-to-back adbreak detection is unaffected. Unit test --------- ReverseTrickPlay_AllAdsFinished_InDownloadLoop_NoOscillation (FetcherLoopTests.cpp): Simulates post-download-loop state: mAdState=OUTSIDE_ADBREAK, mCurPlayingBreakId empty, mLastCompletedBreakId=p1, mPeriodMap[p0].adBreakId=p1. Asserts CheckForAdStart is not called. FAILS on pre-fix code, PASSES with fix.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR addresses CDAI reverse trickplay oscillation by persisting the last completed ad break across the download-loop/period-selection boundary so SelectSourceOrAdPeriod can avoid re-entering the same ad break.
Changes:
- Adds
mLastCompletedBreakIdto CDAI MPD state. - Uses that ID to skip a reverse trickplay end-of-period ad probe when it would re-enter the just-completed break.
- Adds a regression test for the download-loop “All Ads Finished” scenario.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
admanager_mpd.h |
Adds CDAI state for tracking the last completed ad break. |
fragmentcollector_mpd.cpp |
Updates ad completion handling and reverse probe guard logic. |
test/utests/tests/StreamAbstractionAAMP_MPD/FetcherLoopTests.cpp |
Adds regression coverage for the reverse trickplay oscillation scenario. |
Comments suppressed due to low confidence (2)
fragmentcollector_mpd.cpp:12348
- This persists mLastCompletedBreakId regardless of playback direction, but the new guard only consumes it during reverse playback. If this recovery path runs for a non-reverse transition, the stale ID can remain and suppress a later legitimate reverse ad-start probe for the same mapped break; consider recording it only for the reverse download-loop scenario or clearing it for non-reverse paths.
mCdaiObject->mLastCompletedBreakId = mCdaiObject->mCurPlayingBreakId;
fragmentcollector_mpd.cpp:12448
- This records the completed break for forward playback as well as reverse playback, but mLastCompletedBreakId is only consumed by the reverse guard. After a forward ad break finishes, this stale ID can remain until a later reverse SelectSourceOrAdPeriod call and incorrectly skip a valid ad-start probe when the current period maps to the same break.
mCdaiObject->mLastCompletedBreakId = mCdaiObject->mCurPlayingBreakId; // persist across FetcherLoop boundary before clearing
| std::unordered_map<std::string, AdBreakObject> mAdBreaks; /**< Periodid to adbreakobject map*/ | ||
| std::unordered_map<std::string, Period2AdData> mPeriodMap; /**< periodId to Ad map */ | ||
| std::string mCurPlayingBreakId; /**< Currently playing Ad */ | ||
| std::string mLastCompletedBreakId; /**< ID of the adbreak most recently cleared from mCurPlayingBreakId (set just before the clear); used by SelectSourceOrAdPeriod to guard against end-of-period re-entry in reverse trickplay when "All Ads Finished" fires in the download loop before SelectSourceOrAdPeriod is entered. */ |
| { | ||
| AAMPLOG_WARN("[CDAI]: ADBREAK[%s] ENDED. Playing the basePeriod[%s].", mCdaiObject->mCurPlayingBreakId.c_str(), mBasePeriodId.c_str()); | ||
| mCdaiObject->mAdBreaks[mCdaiObject->mCurPlayingBreakId].mAdFailed = false; | ||
| mCdaiObject->mLastCompletedBreakId = mCdaiObject->mCurPlayingBreakId; |
Comment on lines
+2980
to
+2981
| * @brief VPAAMP-205 regression: "All Ads Finished" fired in the download loop | ||
| * (Scenario A) — mLastCompletedBreakId guard. |
pstroffolino
changed the title
Adds kCdaiAdBreakAtIndex0Manifest and test ReverseTrickPlay_AdBreakAtPeriodIndex0_FogTopology_NoOscillation. Why existing tests do not go RED --------------------------------- kCdaiRewindManifest has adbreak on period 'p1' (index 1). After adbreak 'p1' completes in reverse, prevPId='p0'. mPeriodMap['p0'].adBreakId is empty, so CheckForAdStart returns -1 -- no oscillation on either pre-fix or post-fix code. The oscillation condition requires mPeriodMap[mBasePeriodId].adBreakId == mLastCompletedBreakId. This occurs when the adbreak period is at index 0 in the manifest: the prevPId loop breaks at index 0 immediately, prevPId stays '', and mBasePeriodId is not updated from the adbreak period ID. With mBasePeriodId == adBreakId and mPeriodMap[adBreakId].adBreakId == adBreakId, the condition fires. This is the exact FOG/TSB live-edge topology: the most-recent period (live edge) is always at index 0, and the SCTE35 fires at the live edge. The tester's 100% reproducible failure (watch ad break on live channel with FOG/TSB, then REW) maps directly to this case. New manifest kCdaiAdBreakAtIndex0Manifest: s1 (index 0, 60 s, carries adbreak), s2 (index 1, 30 s, older source content). New test setup mirrors post-download-loop state: mAdState=OUTSIDE_ADBREAK, mCurPlayingBreakId='', mLastCompletedBreakId='s1', mPeriodMap['s1'].adBreakId='s1', mPlayRate=-12, adStateChanged=true. Pre-fix: snapPlayingBreakId='' != 's1', ec74c60 guard bypassed, onAdEvent runs, CheckForAdStart finds adbreak 's1' via (rate<0)&&(key==end) => RED. Post-fix: mLastCompletedBreakId='s1' == mPeriodMap['s1'].adBreakId='s1', skipProbeForDownloadLoopExit=true, CheckForAdStart never called => GREEN.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Reason for Change: new tests and regression fix
Root cause (Scenario A)
ec74c60 oscillation guard only covers Scenario B: All Ads Finished fires inside SelectSourceOrAdPeriod's own onAdEvent(DEFAULT) call, where snapPlayingBreakId captures mCurPlayingBreakId before the transition clears it.
In Scenario A (the device failure observed in QA), All Ads Finished fires from the segment download loop before SelectSourceOrAdPeriod is entered. mCurPlayingBreakId is already empty. snapPlayingBreakId captures empty string, the ec74c60 guard never fires, and the first onAdEvent(DEFAULT) call immediately re-enters the same adbreak via CheckForAdStart (rate<0, key==end). Repeated oscillation cycles fed GStreamer mismatched init segments -> error 80:1.
Fix
Add mLastCompletedBreakId to PrivateCDAIObjectMPD. Set it before clearing mCurPlayingBreakId in all three onAdEvent clear sites (ADBREAK ENDED, BUG path, All Ads Finished). Persists across the download-loop / SelectSourceOrAdPeriod boundary.
In SelectSourceOrAdPeriod, before the first onAdEvent call, check mLastCompletedBreakId against mPeriodMap[mBasePeriodId].adBreakId. On match, skip the probe (skipProbeForDownloadLoopExit=true, adStateChanged=false) and consume mLastCompletedBreakId as a one-shot. The ec74c60 guard is retained for Scenario B. Back-to-back adbreak detection is unaffected.
Unit test
ReverseTrickPlay_AllAdsFinished_InDownloadLoop_NoOscillation (FetcherLoopTests.cpp): Simulates post-download-loop state: mAdState=OUTSIDE_ADBREAK, mCurPlayingBreakId empty, mLastCompletedBreakId=p1, mPeriodMap[p0].adBreakId=p1. Asserts CheckForAdStart is not called. FAILS on pre-fix code, PASSES with fix.