VPAAMP-346 restore period state when UpdateTrackInfo fails in HandleS…#1454
Merged
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR aims to make MPD period transitions safer by rolling back period state when HandleSeekEOSAndPeriodTransition() cannot complete UpdateTrackInfo(), preventing the fetch loop from continuing with a partially switched period.
Changes:
- Adds period-state snapshot/restore logic around the period switch path.
- Adds an L1 regression test that forces
UpdateTrackInfo()failure during a forward period transition.
L1 test verdict: Mostly valid, but needs revision.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
fragmentcollector_mpd.cpp |
Restores selected period identity fields when UpdateTrackInfo() fails during EOS-driven period transition. |
test/utests/tests/StreamAbstractionAAMP_MPD/FetcherLoopTests.cpp |
Adds test accessors and a regression test for failed period-switch rollback. |
…eekEOSAndPeriodTransition HandleSeekEOSAndPeriodTransition updates six period-identity members (mCurrentPeriodIdx, mCurrentPeriod, mBasePeriodId, mPeriodStartTime, mPeriodDuration, mPeriodEndTime) before calling UpdateTrackInfo. If UpdateTrackInfo returns failure (malformed period, no codec-compatible representation, null track context after live manifest refresh), the function returned false but left all six members pointing at the new, uninitialised period. The next fetcher-loop iteration would read mCurrentPeriodIdx as the new value and attempt to download fragments from a period whose tracks were never set up, producing download errors or incorrect init-segment selection. Fix: snapshot all six members before the switch. On UpdateTrackInfo failure, restore them and return false so the caller sees a clean no-op. Unit test: HandleSeekEOS_UpdateTrackInfoFails_PeriodStateRestored forces UpdateTrackInfo to fail by nulling the audio track context (UpdateTrackInfo returns MANIFEST_CONTENT_ERROR on a null context) and verifies that mCurrentPeriodIdx is restored to its pre-switch value.
…AndPeriodTransition StreamSelection() and UpdateTrackInfo() mutate per-track state (adaptationSet, representation, fragmentDescriptor.Number/Time/Bandwidth, eos, timeLineIndex, fragmentRepeatCount, scaledPTO, enabled, adaptationSetIdx, adaptationSetId, representationIndex, profileChanged) one track at a time. The previous rollback only restored the six period-identity scalars (mCurrentPeriodIdx, mCurrentPeriod, mBasePeriodId, mPeriodStartTime, mPeriodDuration, mPeriodEndTime). If UpdateTrackInfo() failed after processing video (track 0) but before processing audio (track 1), the video context was left pointing at the new period's adaptation set and representation while the period index was restored to the old period, putting the object in a state that would cause wrong-period fragment downloads on the next fetcher-loop iteration. Fix: snapshot all mutable per-track fields (via a local TrackSnapshot struct across all mMaxTracks slots) plus mNumberOfTracks, mUpdateStreamInfo, mABRMode, mStreamInfo, and mBitrateIndexVector before calling StreamSelection()/ UpdateTrackInfo(). Restore the full snapshot on UpdateTrackInfo() failure so the object is in exactly the same state as before the attempted transition. test: add HandleSeekEOS_UpdateTrackInfoFails_PeriodStateRestored assertions The existing test only checked mCurrentPeriodIdx and the boolean return value. Added four accessors to TestableStreamAbstractionAAMP_MPD (GetBasePeriodId, GetPeriodStartTime, GetPeriodDuration, GetPeriodEndTime) and extended the test to snapshot and re-assert every period-identity field plus the video track's adaptationSet pointer, representation pointer, fragmentDescriptor.Number, and eos flag after the failed transition, so an incomplete rollback that restores only the index would cause the test to fail.
pstroffolino
force-pushed
the
feature/VPAAMP-346
branch
from
057bdfc to
404ea8c
Compare
…rtual UpdateTrackInfo override in unit test - fragmentcollector_mpd.cpp: add null guard to StreamSelection second loop (mirrors the guard already present in the first loop) so a null mMediaStreamContext entry does not cause a SEGV - fragmentcollector_mpd.h: declare UpdateTrackInfo virtual so test doubles can override it without linker trickery - FetcherLoopTests.cpp: replace null-audio-context hack in HandleSeekEOS_UpdateTrackInfoFails_PeriodStateRestored with a clean TestableStreamAbstractionAAMP_MPD::UpdateTrackInfo override that returns MANIFEST_CONTENT_ERROR when mForceUpdateTrackInfoFailure is set; update all comments accordingly
UpdateTrackInfo is now virtual in StreamAbstractionAAMP_MPD. Add the corresponding entries so all test targets link cleanly: - FakeFragmentCollector_MPD.cpp: stub returning eAAMPSTATUS_OK (satisfies vtable for libfakes.a used by LocalTSBTests and others) - MockStreamAbstractionAAMP_MPD.h: MOCK_METHOD entry so NiceMock<MockStreamAbstractionAAMP_MPD> resolves the vtable slot
Replace the AAMPLOG_WARN at the UpdateTrackInfo failure site with an AAMPLOG_ERR that records both the source and target period index and ID, making the failure unambiguous in production logs regardless of log level filtering. Add a PERIOD_SWITCH_FAILED telemetry marker (guarded by AAMP_TELEMETRY_SUPPORT) carrying from_period and to_period indices so the failure rate can be tracked at scale in the telemetry pipeline.
Signed-off-by: Philip Stroffolino <philip_stroffolino@cable.comcast.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.
…eekEOSAndPeriodTransition
HandleSeekEOSAndPeriodTransition updates six period-identity members (mCurrentPeriodIdx, mCurrentPeriod, mBasePeriodId, mPeriodStartTime, mPeriodDuration, mPeriodEndTime) before calling UpdateTrackInfo. If UpdateTrackInfo returns failure (malformed period, no codec-compatible representation, null track context after live manifest refresh), the function returned false but left all six members pointing at the new, uninitialised period. The next fetcher-loop iteration would read mCurrentPeriodIdx as the new value and attempt to download fragments from a period whose tracks were never set up, producing download errors or incorrect init-segment selection.
Fix: snapshot all six members before the switch. On UpdateTrackInfo failure, restore them and return false so the caller sees a clean no-op.
Unit test: HandleSeekEOS_UpdateTrackInfoFails_PeriodStateRestored forces UpdateTrackInfo to fail by nulling the audio track context (UpdateTrackInfo returns MANIFEST_CONTENT_ERROR on a null context) and verifies that mCurrentPeriodIdx is restored to its pre-switch value.