dev sync#1490
Merged
Merged
Conversation
…than simply testing what is present
Co-authored-by: Copilot <copilot@github.com>
…0 commandments") (#1437) Reason for Change: Adds an ABR/LL live playback normative specification v1, a companion conceptual overview, and a set of AI review prompt templates intended to drive spec-first, evidence-based compliance reviews and instrumentation planning. - Introduces abr-normative-v1.md defining required ABR + latency-control behavior and terms. - Adds an initial abr-conceptual-oveview-v1.md to explain the “why” behind the rules in a human-readable form. - Adds abr-documentation/ai-review-prompts/ templates for spec-to-code reviews, PR diff compliance checks, runtime log validation, and instrumentation/assertion planning. --------- Signed-off-by: Philip Stroffolino <philip_stroffolino@cable.comcast.com> Co-authored-by: benmesander <bmesan@protonmail.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
…_test_agent Segment L1 instructions and create custom test engineer agent
* VPAAMP-309: Harden AAMPGstPlayer against null pointer dereference Reason for Change: Guard against null aamp, privateContext, mDRMLicenseManager, mConfig and playerInstance in: constructor setEncryption call, NeedData, EnoughData, HandleOnGstDecodeErrorCb, HandleOnGstPtsErrorCb, HandleOnGstBufferUnderflowCb, HandleBusMessage, ChangeAamp, and StartMonitorAvTimer. Signed-off-by: Philip Stroffolino <philip_stroffolino@cable.comcast.com> * VPAAMP-309: Add L1 null-guard tests for AAMPGstPlayer Tests cover all 9 guarded code paths introduced in the hardening commit: SetEncryptedAamp (null aamp, null mDRMLicenseManager) ChangeAamp (null newAamp) StartMonitorAvTimer (null aamp) HandleOnGstDecodeErrorCb (null aamp) HandleOnGstPtsErrorCb (null aamp) HandleOnGstBufferUnderflowCb (null privateContext, null aamp) NeedDataCb (null aamp) EnoughDataCb (null aamp) HandleBusMessage (null aamp) Callbacks are invoked via the public InterfacePlayerRDK callback holders (OnGstDecodeErrorCb, NeedDataCb, busMessageCallback, etc.) which are populated by RegisterBusCb during AAMPGstPlayer construction. Signed-off-by: Philip Stroffolino <philip_stroffolino@cable.comcast.com> * VPAAMP-309: Fix three review findings from null-guard work 1. Guard InitializePlayerConfigs against null aamp/mConfig before any member access; the previous constructor guard was added after the call, leaving a guaranteed dereference on the null path. 2. Add NeedData_NullPrivateContext and EnoughData_NullPrivateContext L1 tests to cover the privateContext == nullptr branch of those callbacks, which was guarded but not tested. 3. Harden NullGuardTests TearDown: delete mPlayer first (while all mocks are still live) and guard with nullptr check so early ASSERT_* exits do not leak the player or access torn-down mocks. Signed-off-by: Philip Stroffolino <philip_stroffolino@cable.comcast.com> --------- Signed-off-by: Philip Stroffolino <philip_stroffolino@cable.comcast.com>
Reason for change: reverts a previously added guard in AampStreamSinkManager::SetEncryptedHeaders that caused a crash on VOD after reboot. The removed block skipped processing if the player was not in mActiveGstPlayersMap; with single-pipeline mode, this could prevent legitimate encrypted-header setup when no active player is yet registered (e.g., right after reboot/VOD start), leading to downstream issues Test Procedure: See ticket Risks: low Signed-off-by: James Lofthouse <james_lofthouse@comcast.com> Co-authored-by: pstroffolino <Philip_Stroffolino@cable.comcast.com>
#1453) Reason for Change: A disabled track (deselected audio, inactive subtitle) must not drive a forward period transition. The EOS check loop in HandleSeekEOSAndPeriodTransition now requires mMediaStreamContext[i]->enabled in addition to the existing NULL and mPlayRate guards. Without this guard, a track whose context is non-NULL but whose enabled flag is false (e.g. during a mid-playback audio track switch, or when no subtitle stream is active) could set switchToNextPeriod=true and advance the period index spuriously. Unit test: HandleSeekEOS_DisabledTrack_NoPeriodTransition verifies that disabled+eos tracks return false / leave period index unchanged. * Apply suggestions from code review Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
* VPLAY-12792:Problems with ad progress in 6.6_p1v Reason for change:Fix ReportAdProgress for split-period ads: when an ad exceeds its period boundary and overflows into the next, use cumulative ad duration instead of basePeriodOffset to correctly anchor the start position of the subsequent ad Use mCurPlayingBreakId instead of mBasePeriodId and not to call the new logic when adIdx == 0. optimization of fragmentTime and mStartTimeOfFirstPTS calculation Test Procedure:Refer Jira Ticket Risks: Medium Signed-off-by: varshnie <varshniblue14@gmail.com> --------- Signed-off-by: varshnie <varshniblue14@gmail.com> Signed-off-by: Vinish100 <vinish.balan@gmail.com> Co-authored-by: Vinish100 <vinish.balan@gmail.com> Co-authored-by: pstroffolino <Philip_Stroffolino@cable.comcast.com>
Reason for Change: updates aampcli manifest-refresh logging to avoid misleading zero-length manifest output when the player is inactive or in error state Risk: Low. aampcli specific logging change Test Guidance: play bad linear content (i.e. missing media initialization segments), and confirm a meaningful error is displayed, instead of Manifest length [0]
Reason for Change: Stage 3 completes the rename work begun in stages 1 and 2. No behavioural change — pure mechanical rename, lock-scope tidy, and comment cleanup. Risk: low Test Guidance: regression only --------- Co-authored-by: pstroffolino <Philip_Stroffolino@cable.comcast.com>
Reason for change: Revert DELI A-25590 behavior that stops StreamSink when EAS content reaches end-of-stream. This causes crashes in VIPA when app pauses at EOS during concurrent stop operations. The original fix ( calling stop) was specific to XRE X1 platforms and is not required for VIPA. Test Procedure: Priority: P2 Risks:Low Co-authored-by: shripad bankar <Shripad_Bankar@cable.comcast.com>
#1474)" (#1475) This reverts commit 86b2393. Reason for Change: avoid crash Removes the block in PrivateInstanceAAMP::NotifyEOSReached() that calls sink->Stop(false) for ContentType_EAS. This eliminates the triggering condition entirely — no EOS-driven concurrent Stop is ever issued for EAS content. Note that we have upcoming #1452 which narrows the exposure to crash sequence, but does not completely eliminate need for #1471 With only #1452's mutex in place, Stop() and Pause() will be serialized. But serialization only means one finishes before the other starts. If Stop() wins the race and runs first, it tears down the pipeline via playerInstance->Stop(). When Stop() releases the mutex, Pause() acquires it and then calls playerInstance->Pause() on a pipeline that is already stopped or in an invalid GStreamer state. That is still a crash-prone call, and there is no longer a null guard in Pause() to bail out early. So #1452 narrows the window but does not close it for the specific EAS EOS scenario.
#1455) * VPAAMP-345 subtitle SkipFragments result must not overwrite A/V remaining-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. * test(VPAAMP-345): fix SeekInPeriod subtitle regression test - 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.
#1454) * VPAAMP-346 restore period state when UpdateTrackInfo fails in HandleSeekEOSAndPeriodTransition Reason for Change: plug edge case involving UpdateTrackInfo failure handling Test Guidance: basic regression + l1 tests. Look for new telemetry. Ideally we'll add l2 test allowing the Change Overview: 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. * fix(VPAAMP-346): complete period-transition rollback in HandleSeekEOSAndPeriodTransition 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. * fix(VPAAMP-346): harden StreamSelection null context guard and use virtual 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 * fix(VPAAMP-346): provide UpdateTrackInfo vtable entry in fake and mock 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 * fix(VPAAMP-346): AAMPLOG_ERR and telemetry on UpdateTrackInfo rollback 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. * Reason for Change: Changed armp->mAppName to aamp->GetAppName() Signed-off-by: Philip Stroffolino <philip_stroffolino@cable.comcast.com> --------- Signed-off-by: Philip Stroffolino <philip_stroffolino@cable.comcast.com>
…1442) * VPAAMP-245: Negative Latency value logged while HLS linear playback w/o TSB Test Guidance: refer ticket Risk: Low Reason for change: EndPos is set to -1 for TSB-less playback before latency calculation. This caused latency to be negative, since HLS latency is calculated as EndPos - CurrPos. So, moved this tsbless block after latency calculation. Also, fixed start and end values in aamp pos logging. * Rename format_pos lambda to formatPos and val parameter to valMs for camelCase consistency * Add regression test for VPAAMP-245: TSB-less linear HLS latency non-negative Replace (valMs == -1.0) with (valMs < 0) in formatPos lambda: direct floating-point equality against a sentinel is fragile, and culledSeconds can legitimately go negative via the HLS PDT path in UpdateCullingState (negative culledSec values are explicitly allowed there), so < 0 is both safer and more correct. Remove redundant explicit casts in AAMPLOG_MIL call: (double) casts on videoBufferedDuration, audioBufferedDuration, and latency/1000.0 are unnecessary (division by a double literal already produces double), and the (long long) cast on videoPTS is redundant as it is already long long. --------- Signed-off-by: psiva01 <sivasubramanian.patchaiperumal@ltts.com> Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: psiva01 <214551386+psiva01@users.noreply.github.com> Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> Co-authored-by: pstroffolino <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
8 participants
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.
No description provided.