VPAAMP-205 eliminate recursion and other concerns in HandleSeekEOSAndPeriodTransition#1365
Merged
Merged
Conversation
pstroffolino
force-pushed
the
feature/VPAAMP-205
branch
from
b39bbf6 to
dc62415
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
This PR removes recursive period-by-period seek handling in SeekInPeriod() for DASH MPD playback by introducing an iterative loop and a helper to perform period transitions, reducing stack-growth risk when seeks span many periods.
Changes:
- Added
HandleSeekEOSAndPeriodTransition()to switch to the next playable period when a seek crosses a period boundary. - Refactored
SeekInPeriod()to an iterative loop that repeatedly skips fragments and advances periods until the seek remainder is consumed or no further periods exist. - Added a new functional test to validate that
mFirstPTSupdates correctly when a seek crosses from one period to the next.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
fragmentcollector_mpd.cpp |
Introduces the period-transition helper and refactors SeekInPeriod() from recursive-style behavior to an iterative loop. |
fragmentcollector_mpd.h |
Declares the new HandleSeekEOSAndPeriodTransition() helper. |
test/utests/tests/StreamAbstractionAAMP_MPD/FunctionalTests.cpp |
Adds a test wrapper class and a new test covering SeekInPeriod() across two periods. |
pstroffolino
changed the title
pstroffolino
force-pushed
the
feature/VPAAMP-205
branch
2 times, most recently
from
c9d1075 to
825e7e7
Compare
pstroffolino
changed the title
pstroffolino
force-pushed
the
feature/VPAAMP-205
branch
from
110e483 to
d81a95e
Compare
Convert SeekInPeriod from a recursive design into an iterative while-loop. HandleSeekEOSAndPeriodTransition now only handles the period state transition and returns true/false; SeekInPeriod drives the loop, calling SkipFragments and HandleSeekEOSAndPeriodTransition until no further period transition is needed or no playable period remains. This eliminates the theoretical unbounded stack growth that arose when a seek remainder spanned multiple consecutive short periods.
…sition/SeekInPeriod 1. Add 'enabled' guard to EOS check (HandleSeekEOSAndPeriodTransition) A disabled/unselected track (e.g. alternate audio, subtitle when off) may carry stale eos=true from a prior operation and must not trigger a spurious period transition. Guard with mMediaStreamContext[i]->enabled, matching the established pattern used throughout fragmentcollector_mpd.cpp. 2. Capture remaining seek from primary video track only (SeekInPeriod) The subtitle SkipFragments call omits skipToEnd and can return a different remainder than A/V. Since subtitle is last in the loop (VIDEO=0, AUDIO=1, SUBTITLE=2), it previously overwrote trackRemainingSeek on every seek with subtitles active, driving incorrect period transitions and carry-over offsets. Now: subtitle result is discarded; trackRemainingSeek is set from video, or from the first enabled non-subtitle track if video is absent. 3. Save/restore period state around UpdateTrackInfo (HandleSeekEOSAndPeriodTransition) Five members (mCurrentPeriodIdx, mCurrentPeriod, mBasePeriodId, mPeriodStartTime/Duration/EndTime) were mutated before UpdateTrackInfo was called. On failure the object was left in a half-switched state (period index updated, track info not). Save the previous values and restore them on failure so the player remains in a consistent state.
pstroffolino
force-pushed
the
feature/VPAAMP-205
branch
from
d81a95e to
ed43c64
Compare
…essage - Replace 0.0-sentinel track selection with an explicit primaryCaptured flag so the first enabled non-subtitle track (video, else audio) is deterministically chosen as the period-transition remainder, avoiding ambiguity when the primary track legitimately returns 0.0. - Replace NULL with nullptr in HandleSeekEOSAndPeriodTransition guard. - Clarify log message: 'caller will re-run SkipFragments on the new period' instead of the misleading 'calling SkipFragments'.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…d/unused skipToEnd is accepted for API consistency with SeekInPeriod and reserved for future direction-aware empty-period handling, but is not read inside the function body. Comment out the parameter name in the definition to silence -Wunused-parameter without removing the parameter, and update the Doxygen @param to explain the rationale.
DomSyna
pushed a commit
that referenced
this pull request
Apr 29, 2026
…PeriodTransition (#1365) * VPAAMP-205 Eliminate recursion in HandleSeekEOSAndPeriodTransition Reason for Change: Convert SeekInPeriod from a recursive design into an iterative while-loop. HandleSeekEOSAndPeriodTransition now only handles the period state transition and returns true/false; SeekInPeriod drives the loop, calling SkipFragments and HandleSeekEOSAndPeriodTransition until no further period transition is needed or no playable period remains. This eliminates the theoretical unbounded stack growth that arose when a seek remainder spanned multiple consecutive short periods. * VPAAMP-205 Fix three correctness issues in HandleSeekEOSAndPeriodTransition/SeekInPeriod 1. Add 'enabled' guard to EOS check (HandleSeekEOSAndPeriodTransition) A disabled/unselected track (e.g. alternate audio, subtitle when off) may carry stale eos=true from a prior operation and must not trigger a spurious period transition. Guard with mMediaStreamContext[i]->enabled, matching the established pattern used throughout fragmentcollector_mpd.cpp. 2. Capture remaining seek from primary video track only (SeekInPeriod) The subtitle SkipFragments call omits skipToEnd and can return a different remainder than A/V. Since subtitle is last in the loop (VIDEO=0, AUDIO=1, SUBTITLE=2), it previously overwrote trackRemainingSeek on every seek with subtitles active, driving incorrect period transitions and carry-over offsets. Now: subtitle result is discarded; trackRemainingSeek is set from video, or from the first enabled non-subtitle track if video is absent. 3. Save/restore period state around UpdateTrackInfo (HandleSeekEOSAndPeriodTransition) Five members (mCurrentPeriodIdx, mCurrentPeriod, mBasePeriodId, mPeriodStartTime/Duration/EndTime) were mutated before UpdateTrackInfo was called. On failure the object was left in a half-switched state (period index updated, track info not). Save the previous values and restore them on failure so the player remains in a consistent state. * VPAAMP-205 SeekInPeriod: use primaryCaptured flag, nullptr, fix log message - Replace 0.0-sentinel track selection with an explicit primaryCaptured flag so the first enabled non-subtitle track (video, else audio) is deterministically chosen as the period-transition remainder, avoiding ambiguity when the primary track legitimately returns 0.0. - Replace NULL with nullptr in HandleSeekEOSAndPeriodTransition guard. - Clarify log message: 'caller will re-run SkipFragments on the new period' instead of the misleading 'calling SkipFragments'. - skipToEnd is accepted for API consistency with SeekInPeriod and reserved for future direction-aware empty-period handling, but is not read inside the function body. Comment out the parameter name in the definition to silence -Wunused-parameter without removing the parameter, and update the Doxygen @param to explain the rationale. --------- Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Abhi-jith-S
pushed a commit
that referenced
this pull request
May 12, 2026
…PeriodTransition (#1365) * VPAAMP-205 Eliminate recursion in HandleSeekEOSAndPeriodTransition Reason for Change: Convert SeekInPeriod from a recursive design into an iterative while-loop. HandleSeekEOSAndPeriodTransition now only handles the period state transition and returns true/false; SeekInPeriod drives the loop, calling SkipFragments and HandleSeekEOSAndPeriodTransition until no further period transition is needed or no playable period remains. This eliminates the theoretical unbounded stack growth that arose when a seek remainder spanned multiple consecutive short periods. * VPAAMP-205 Fix three correctness issues in HandleSeekEOSAndPeriodTransition/SeekInPeriod 1. Add 'enabled' guard to EOS check (HandleSeekEOSAndPeriodTransition) A disabled/unselected track (e.g. alternate audio, subtitle when off) may carry stale eos=true from a prior operation and must not trigger a spurious period transition. Guard with mMediaStreamContext[i]->enabled, matching the established pattern used throughout fragmentcollector_mpd.cpp. 2. Capture remaining seek from primary video track only (SeekInPeriod) The subtitle SkipFragments call omits skipToEnd and can return a different remainder than A/V. Since subtitle is last in the loop (VIDEO=0, AUDIO=1, SUBTITLE=2), it previously overwrote trackRemainingSeek on every seek with subtitles active, driving incorrect period transitions and carry-over offsets. Now: subtitle result is discarded; trackRemainingSeek is set from video, or from the first enabled non-subtitle track if video is absent. 3. Save/restore period state around UpdateTrackInfo (HandleSeekEOSAndPeriodTransition) Five members (mCurrentPeriodIdx, mCurrentPeriod, mBasePeriodId, mPeriodStartTime/Duration/EndTime) were mutated before UpdateTrackInfo was called. On failure the object was left in a half-switched state (period index updated, track info not). Save the previous values and restore them on failure so the player remains in a consistent state. * VPAAMP-205 SeekInPeriod: use primaryCaptured flag, nullptr, fix log message - Replace 0.0-sentinel track selection with an explicit primaryCaptured flag so the first enabled non-subtitle track (video, else audio) is deterministically chosen as the period-transition remainder, avoiding ambiguity when the primary track legitimately returns 0.0. - Replace NULL with nullptr in HandleSeekEOSAndPeriodTransition guard. - Clarify log message: 'caller will re-run SkipFragments on the new period' instead of the misleading 'calling SkipFragments'. - skipToEnd is accepted for API consistency with SeekInPeriod and reserved for future direction-aware empty-period handling, but is not read inside the function body. Comment out the parameter name in the definition to silence -Wunused-parameter without removing the parameter, and update the Doxygen @param to explain the rationale. --------- Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
varatharajan568
pushed a commit
that referenced
this pull request
May 20, 2026
…PeriodTransition (#1365) * VPAAMP-205 Eliminate recursion in HandleSeekEOSAndPeriodTransition Reason for Change: Convert SeekInPeriod from a recursive design into an iterative while-loop. HandleSeekEOSAndPeriodTransition now only handles the period state transition and returns true/false; SeekInPeriod drives the loop, calling SkipFragments and HandleSeekEOSAndPeriodTransition until no further period transition is needed or no playable period remains. This eliminates the theoretical unbounded stack growth that arose when a seek remainder spanned multiple consecutive short periods. * VPAAMP-205 Fix three correctness issues in HandleSeekEOSAndPeriodTransition/SeekInPeriod 1. Add 'enabled' guard to EOS check (HandleSeekEOSAndPeriodTransition) A disabled/unselected track (e.g. alternate audio, subtitle when off) may carry stale eos=true from a prior operation and must not trigger a spurious period transition. Guard with mMediaStreamContext[i]->enabled, matching the established pattern used throughout fragmentcollector_mpd.cpp. 2. Capture remaining seek from primary video track only (SeekInPeriod) The subtitle SkipFragments call omits skipToEnd and can return a different remainder than A/V. Since subtitle is last in the loop (VIDEO=0, AUDIO=1, SUBTITLE=2), it previously overwrote trackRemainingSeek on every seek with subtitles active, driving incorrect period transitions and carry-over offsets. Now: subtitle result is discarded; trackRemainingSeek is set from video, or from the first enabled non-subtitle track if video is absent. 3. Save/restore period state around UpdateTrackInfo (HandleSeekEOSAndPeriodTransition) Five members (mCurrentPeriodIdx, mCurrentPeriod, mBasePeriodId, mPeriodStartTime/Duration/EndTime) were mutated before UpdateTrackInfo was called. On failure the object was left in a half-switched state (period index updated, track info not). Save the previous values and restore them on failure so the player remains in a consistent state. * VPAAMP-205 SeekInPeriod: use primaryCaptured flag, nullptr, fix log message - Replace 0.0-sentinel track selection with an explicit primaryCaptured flag so the first enabled non-subtitle track (video, else audio) is deterministically chosen as the period-transition remainder, avoiding ambiguity when the primary track legitimately returns 0.0. - Replace NULL with nullptr in HandleSeekEOSAndPeriodTransition guard. - Clarify log message: 'caller will re-run SkipFragments on the new period' instead of the misleading 'calling SkipFragments'. - skipToEnd is accepted for API consistency with SeekInPeriod and reserved for future direction-aware empty-period handling, but is not read inside the function body. Comment out the parameter name in the definition to silence -Wunused-parameter without removing the parameter, and update the Doxygen @param to explain the rationale. --------- Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
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: HandleSeekEOSAndPeriodTransition() performs a recursive call back into SeekInPeriod(). If a seek remainder spans many periods, this can recurse once per period and risks unnecessary stack growth.
Risk: Low
Test Guidance: requires basic regression-only, but may want to consider having the developer generate a new stress test.