VPAAMP-345 subtitle SkipFragments result must not overwrite A/V remai…#1455
Open
pstroffolino wants to merge 2 commits into
Open
VPAAMP-345 subtitle SkipFragments result must not overwrite A/V remai…#1455pstroffolino wants to merge 2 commits into
pstroffolino wants to merge 2 commits into
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes VPAAMP-345 in the DASH (MPD) stream abstraction: SeekInPeriod() previously overwrote the A/V SkipFragments() remaining-seek value with the subtitle track’s remaining-seek (processed last), which could incorrectly block or distort forward period transitions when subtitle segment boundaries differ from A/V.
Changes:
- Update
StreamAbstractionAAMP_MPD::SeekInPeriod()to discard the subtitle track’sSkipFragments()return value so period-transition decisions use only A/V remaining-seek. - Update the in-code comment to reflect the new “A/V-only” remaining-seek behavior.
- Add a unit test intended to guard the behavior (but it currently does not exercise a subtitle-present scenario).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
fragmentcollector_mpd.cpp |
Prevents subtitle SkipFragments() remaining-seek from overwriting the A/V remaining-seek used for period transition gating/carry-over. |
test/utests/tests/StreamAbstractionAAMP_MPD/FetcherLoopTests.cpp |
Adds a SeekInPeriod()-related test and exposes a wrapper to invoke SeekInPeriod() from the fixture. |
…ning-seek SeekInPeriod iterates all tracks in VIDEO/AUDIO/SUBTITLE order, assigning the SkipFragments return value to trackRemainingSeek for each iteration. When a subtitle track is present, its return value overwrites the A/V value as the last write. HandleSeekEOSAndPeriodTransition receives this subtitle-derived value as the carry-over seek offset and for the sign check that gates forward period transition. Subtitle segment grids are often coarser than A/V (e.g. WebVTT on 10-second boundaries vs 2-second A/V segments). When the two grids disagree, the subtitle remaining-seek may be negative (seek overshot) even when the A/V seek correctly spans the period boundary, preventing a valid period transition. Conversely, a subtitle returning a positive remainder could carry an incorrect seek offset into the next period. Fix: discard the subtitle SkipFragments return value; trackRemainingSeek is updated only from A/V tracks. Unit test: SeekInPeriod_SubtitleResultNotUsedForPeriodTransition verifies the A/V-only seek path is not regressed.
- Remove unused GetNumberOfTracks() and GetMediaStreamContextAt()
accessor helpers from TestableStreamAbstractionAAMP_MPD inner class.
- Add mAVVodManifest: two-period static VOD with video + audio, no
subtitle adaptation set, 15 x 2 s segments per period (timescale
2500, d=5000, startNumber=1 in both periods).
- Rewrite SeekInPeriod_SubtitleResultNotUsedForPeriodTransition:
* Init with mAVVodManifest so A/V tracks have non-null representations.
* Call SetNumberOfTracks(3) to inject the subtitle slot into the
SeekInPeriod loop without a real subtitle pipeline.
* mMediaStreamContext[eTRACK_SUBTITLE] is allocated but representation
is null; SkipFragments returns 0.0 immediately from the null-rep guard,
which is the corrupting value on a pre-fix build.
* Seek to 35 s (5 s past the 30 s period 0 boundary).
* Assert mCurrentPeriodIdx==1 (period transition occurred).
* Assert fragmentDescriptor.Number==3: post-fix carry-over of 5 s skips
two 2 s segments in period 1, landing at segment 3. Pre-fix carry-over
of 0 s leaves Number at 1, causing the assertion to fail.
pstroffolino
force-pushed
the
feature/VPAAMP-345
branch
from
8fd7456 to
8379826
Compare
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.
…ning-seek
SeekInPeriod iterates all tracks in VIDEO/AUDIO/SUBTITLE order, assigning the SkipFragments return value to trackRemainingSeek for each iteration. When a subtitle track is present, its return value overwrites the A/V value as the last write. HandleSeekEOSAndPeriodTransition receives this subtitle-derived value as the carry-over seek offset and for the sign check that gates forward period transition.
Subtitle segment grids are often coarser than A/V (e.g. WebVTT on 10-second boundaries vs 2-second A/V segments). When the two grids disagree, the subtitle remaining-seek may be negative (seek overshot) even when the A/V seek correctly spans the period boundary, preventing a valid period transition. Conversely, a subtitle returning a positive remainder could carry an incorrect seek offset into the next period.
Fix: discard the subtitle SkipFragments return value; trackRemainingSeek is updated only from A/V tracks.
Unit test: SeekInPeriod_SubtitleResultNotUsedForPeriodTransition verifies the A/V-only seek path is not regressed.