VPAAMP 568 inconsistent cc behaviour when CC and OOB subs are available#1629
Merged
pstroffolino merged 12 commits intoJun 29, 2026
Merged
Conversation
jfagunde
reviewed
Jun 19, 2026
jfagunde
approved these changes
Jun 25, 2026
…C_behaviout_on_Xumo
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>
…C_behaviout_on_Xumo
…C_behaviout_on_Xumo
pstroffolino
approved these changes
Jun 29, 2026
pstroffolino
deleted the
feature/VPAAMP-568_Inconsistent_CC_behaviout_on_Xumo
branch
jfagunde
added a commit
that referenced
this pull request
Jul 2, 2026
* VPAAMP-512: Fix EOS marking spam in FetcherLoop (#1550) Reason for Change: state check to avoid logging Audio/Video EOS Marked multiple times Risk: Low * RDKEMW-19029: Observed Video Freeze & Fast-Forward Effect After Chann… (#1548) * RDKEMW-19029: Observed Video Freeze & Fast-Forward Effect After Channel Change Reason for change: Added retry logic to AAMP downloads when curl returns error 55 (CURL_SEND_ERROR) Risks: Low Test Procedure: Refer jira ticket Priority: P0 Signed-off-by: Gnanesha <gnaani82@gmail.com> * Apply suggestions from code review Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> * RDKEMW-19029: Added L1 test AampCurlDownloader_Retry_SendError Signed-off-by: psiva01 <sivasubramanian.patchaiperumal@ltts.com> --------- Signed-off-by: Gnanesha <gnaani82@gmail.com> Signed-off-by: psiva01 <sivasubramanian.patchaiperumal@ltts.com> Co-authored-by: Sivasubramanian Patchaiperumal <sivasubramanian.patchaiperumal@ltts.com> Co-authored-by: pstroffolino <Philip_Stroffolino@cable.comcast.com> Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> * VPAAMP-615 - Download stops when playing from TSB and fragment Cache is full (#1640) * VPAAMP-615 - Download stops when playing from TSB and fragment Cache is full The scenario occurred after tuning to channel and performing pause-play-pause sequence. If the fragment cache is full, the while loop will spin indefinitely preventing download. Fix: When playing from TSB any wait for free cache segment is bypassed Also adds L1 microtest to verify the fix Test Instructions: - Refer ticket Risks Low * Simplifying the comments --------- Co-authored-by: pstroffolino <Philip_Stroffolino@cable.comcast.com> * VPAAMP-565: Deprecate ceaFormat from AAMP Config. (#1654) * VPAAMP-565: Deprecate ceaFormat from AAMP Config. Reason for Change: Deprecated ceaFormat from AAMP Test Procedure: see ticket Priority: P1 Risks: Low * VPAAMP-565: Deprecate ceaFormat from AAMP Config. Reason for Change: Deprecated ceaFormat from AAMP Test Procedure: see ticket Priority: P1 Risks: Low --------- Co-authored-by: pstroffolino <Philip_Stroffolino@cable.comcast.com> * VPAAMP-473:[Timeshift DAI] Place CDAI ads for cold CDVR and iVOD (#1652) * VPAAMP-473:[Timeshift DAI] Place CDAI ads for cold CDVR and iVOD Reason for change:Cold CDVR/static manifests never refresh, so ad placement was never triggered after fulfillment, causing playback to freeze waiting for ads that were never mapped.Fixed by immediately calling ad placement as soon as an ad is resolved, instead of waiting for a manifest refresh that never comes. Risks: p1 Signed-off-by: varshnie <varshniblue14@gmail.com> * VPAAMP-473:[Timeshift DAI] Place CDAI ads for cold CDVR and iVOD Reason for change: Fix L1 failures Test Procedure: L1 should pass Risks: None Signed-off-by: Vinish100 <vinish.balan@gmail.com> --------- Signed-off-by: varshnie <varshniblue14@gmail.com> Signed-off-by: Vinish100 <vinish.balan@gmail.com> Co-authored-by: Vinish K B <vinish.balan@gmail.com> Co-authored-by: pstroffolino <Philip_Stroffolino@cable.comcast.com> * VPAAMP-394: Enable CDAI ads for CDVR and iVOD content. (#1653) Reason for change: Invoke FoundEventBreak for both static and dynamic manifests when CDAI is enabled to support CDAI on cold CDVR. Avoid calling onAdEvent from init() to prevent processing DAI events during the tune-time period for a new tune.Added L1s. Test Procedure: Validate CDAI ads are played in CDVR and iVOD content Priority: P1 Signed-off-by: srikanthreddybijjam-comcast <srikanthreddybijjam.2000@gmail.com> Co-authored-by: vinodkadungoth <vinodkadungoth@gmail.com> Co-authored-by: pstroffolino <Philip_Stroffolino@cable.comcast.com> * VPAAMP-621 Fix AAMP L1 test build failure on Ubuntu (#1645) Reason for Change: Fix AAMP L1 test build failure on Ubuntu Summary of Changes: - Update the script to install the middleware interfaces Test Procedure: Build AAMP L1 tests Priority: P2 Risks: Low * Revert "VPAAMP-512: Fix EOS marking spam in FetcherLoop (#1550)" (#1661) This reverts commit cce5e9b. * VPAAMP-559: Fix memory retention after local TSB I-frame extraction (#1658) Fixes steady memory growth in the local TSB trickplay path by ensuring ISO-BMFF segments truncated to a single I-frame do not retain their original over-allocated std::vector capacity, and simplifies the MP4 demuxer trickmode sample handling accordingly. * VPAAMP-507:[Timeshift DAI] PostrollAD support for CDVR (#1657) Reason for change: post roll cdai support on static manifest Risks: p1 This PR adds support for CDAI post-roll ads on cold-CDVR/iVOD (static MPD) so that, after the post-roll ad finishes, playback correctly transitions to EOS instead of attempting a manifest refresh, and queued CDAI ad events are flushed before the app receives EOS Signed-off-by: varshnie varshniblue14@gmail.com Co-authored-by: pstroffolino <Philip_Stroffolino@cable.comcast.com> * VPAAMP-363 address race conditions specific to DASH SegmentBase format (#1639) * test: L1 regression test for SegmentBase ABR switch byte-range base offset Add DownloadFragment_SegmentBase_ABRSwitch_UsesIdxBaseOffset to FragmentDownloadTests to deterministically cover the race introduced by VPAAMP-363 removing the serial-download guard for SegmentBase streams. Before the fix, DownloadFragment computed the byte range for an ABR-switched SegmentBase segment as (0 + 1 + first_offset), which landed inside the moov/SIDX prefix and returned duration=0 from the IsoBmff parser. This produced before=0/after=0 in RestampPts log, causing intermittent failure in test_8003_0[...main-segmentbase.mpd] (PTS actual 0s, expected 76800s). The test sets mIdxBaseOffset=1000, fragmentDescriptor.Bandwidth=5000000 (1080p), and passes a stale dlInfo with bandwidth=1400000 (480p) and an empty uriList. DownloadFragment executes the ABR switch range computation then returns false before issuing any network request (no mocks required). Assert: dlInfo->range == "17384-29671" (mIdxBaseOffset + ref[0].size as start, + ref[1].size - 1 as end) and fragmentDurationSec == 2.0f. * fix: thread mIdxBaseOffset through SegmentBase IDX load/clear/use VPAAMP-363 removed the DashParallelFragDownload=false override from SkipFragments for SegmentBase streams, enabling parallel downloads. This introduced a race: a stale ABR download job could call DownloadFragment while the FetcherLoop had already advanced to a new profile. The ABR switch branch recomputed fragmentOffset as (0 + 1 + first_offset), landing inside the moov/SIDX prefix and fetching garbage. The IsoBmff parser returned duration=0, producing before=0/after=0 in RestampPts log and causing intermittent failure in test_8003_0[...main-segmentbase.mpd] (PTS actual 0s, expected 76800s). Changes: - MediaStreamContext.h: add mIdxBaseOffset{0} (uint64_t) field alongside IDX; tracks the byte position of segment 0 in the file for the currently loaded IDX profile. Add mIdxMutex (std::mutex) to protect IDX and mIdxBaseOffset against concurrent access by FetcherLoop (writer) and download worker threads (reader). - fragmentcollector_mpd.cpp (FetchFragment lazy-load): after computing fragmentOffset = end_of_SIDX + 1 + first_offset, snapshot it into mIdxBaseOffset so DownloadFragment can use it on ABR switches without recomputing from zero. - fragmentcollector_mpd.cpp (FetchAndInjectInitialization): clear mIdxBaseOffset=0 inside the mIdxMutex scope alongside the existing ClearAndRelease(IDX), keeping the two values consistent. - MediaStreamContext.cpp (DownloadFragment ABR switch): replace the old fragmentOffset=0+1+first_offset recomputation with dlInfo->fragmentOffset = mIdxBaseOffset, then accumulate referenced_size values from ParseSegmentIndexBox to reach the correct segment start byte. * fix: address review comments - Lock mIdxMutex around IDX offset computation in PushNextFragment - Correct comment wording in MediaStreamContext (missed -> landed inside) - Normalize test indentation and replace std::cbegin/cend with pointer arithmetic * fix: snapshot IDX under mIdxMutex in PushNextFragment segment-fetch block The outer IDX.empty() check and both ParseSegmentIndexBox calls in the segment-fetch block of PushNextFragment ran without mIdxMutex, exposing a check-then-act race with FetchAndInjectInitialization (worker thread) which calls ClearAndRelease(IDX) under the mutex: Thread A (FetcherLoop): IDX.empty() -> false Thread B (worker): ClearAndRelease(IDX) <- IDX freed Thread A (FetcherLoop): IDX.data() dereference <- use-after-free Fix: take a local idxSnapshot copy of IDX under mIdxMutex before the segment-fetch block, then parse from the snapshot. The mutex is released before FetchFragment so we never hold it across blocking network I/O. Addresses review comment on feature/vpaamp-363-follow-up PR #1533. * fix: address PushNextFragment review comments (IDX load and EOS clear) Two issues raised in code review on PushNextFragment SegmentBase path: 1. mIdxMutex held across LoadIDX network I/O (lines R1629-R1632) LoadIDX calls GetFile() which performs network I/O and can block for hundreds of milliseconds. Holding mIdxMutex across this call stalls any concurrent thread needing the mutex (e.g. DownloadFragment during an ABR switch). Fix: download into a local loadedIdx buffer with no lock held, then move into IDX under mIdxMutex only if IDX is still empty (matching the pattern in SkipFragments). 2. mIdxBaseOffset not reset on EOS ClearAndRelease (lines R1750-R1754) When the SIDX is exhausted, IDX is cleared but mIdxBaseOffset was left at its last value. A subsequent ABR-switch job reading mIdxBaseOffset before the next IDX load would see a stale base offset. Fix: reset mIdxBaseOffset = 0 alongside ClearAndRelease(IDX) under mIdxMutex. * Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> * fix: VPAAMP-614 three remaining SegmentBase race conditions 1. PushNextFragment IDX.empty() TOCTOU (line ~1614): Read IDX.empty() without mIdxMutex exposed a check-then-act race against FetchAndInjectInitialization's ClearAndRelease. Fix: use the shouldLoadIdx pattern (same as SkipFragments) so the read is always under the lock. 2. FetchAndInjectInitialization atomicity (line ~8825): fragmentOffset=0 was written outside mIdxMutex, so a concurrent DownloadFragment ABR-switch thread could read IDX (non-empty) and then race to use fragmentOffset=0, computing a byte range starting at byte 0 of the media file (moov box) -> GStreamer error 80:1 on production VOD content. Fix: move fragmentOffset=0 inside the existing mIdxMutex critical section alongside ClearAndRelease(IDX) and mIdxBaseOffset=0. 3. Stop() path data race (line ~11208): ClearAndRelease(track->IDX) was called with no lock held, creating a potential use-after-free if PushNextFragment/DownloadFragment were concurrently dereferencing IDX.data(). Fix: wrap in mIdxMutex. Tests: three regression tests added to FragmentDownloadTests.cpp - SegmentBase_FetchAndInjectInit_ResetsAreAtomic - SegmentBase_IDXEmptyCheck_MutexGuard_NoStaleRead - SegmentBase_StopPath_ClearAndRelease_IsLocked * fix: VPAAMP-614 run SegmentBase init synchronously to prevent GStreamer invalid-data error For SegmentBase content the FetcherLoop returns immediately after SubmitJob() and races ahead: it loads IDX and submits media segment[0] to the worker queue while the async init download is still in flight. fragmentOffset is mutated on the FetcherLoop thread concurrently with the worker thread's DownloadFragment path, and GStreamer receives media data before the init segment, producing the 'file is invalid' error (code 80:1). Detect SegmentBase init via the non-empty byte-range on an init segment (segmentBaseInit = isInitializationSegment && !range.empty()) and fall through to the existing synchronous Execute() path for that case only. All other downloads (SegmentTemplate init, media segments) continue to use the parallel worker as before. * fix: VPAAMP-614 extend SegmentBase synchronous fix to all downloads Original fix only covered init segments (isInitializationSegment && !range.empty()) but SegmentBase media segments also need protection. The race condition occurs when FetcherLoop continues after SubmitJob returns and can race ahead with IDX loading and subsequent segment submissions while async downloads are still in flight. Extend the condition to cover all SegmentBase content (!range.empty()) so both init and media segments download synchronously, guaranteeing strict ordering and preventing GStreamer from receiving media data before init data. --------- Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> * VPAAMP-454 Enable AAMP L1 AampGstPlayer tests (#1644) * VPAAMP-454 Enable AAMP L1 AampGstPlayer tests Reason for Change: Protect against regressions in AampGstPlayer code Summary of Changes: - Add AampGstPlayer L1 test directory - Add L1 AampGstPlayerMain file - Remove middleware code from L1 AampGstPlayer CMake file - Delete tests that verified middleware code - Update existing L1 tests - Add middleware fakes Test Procedure: Run AAMP L1 tests Priority: P1 Risks: Low * Update copyright year to 2026 for new files added * Apply Copilot suggestions from code review Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> * Do not initialize mProtectionLock for InterfacePlayerRDK fake --------- Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> * VPAAMP-580 Disable LatencyMonitor earlier in pause and trickplay paths (#1663) * VPAAMP-580 Disable LatencyMonitor earlier in pause and trickplay paths Reason for Change: Required for DirectRialto Summary of Changes: - Disable LatencyMonitor earlier in SetRateInternal(0) - Disable LatencyMonitor in SetRateInternal() for trickmodes Test Procedure: Check LatencyMonitor during pause and trick modes Priority: P1 Risks: Low * Add WARN when pipeline is running but position does not change * Verify changes in L1 test --------- Co-authored-by: pstroffolino <Philip_Stroffolino@cable.comcast.com> * VPAAMP 568 inconsistent cc behaviour when CC and OOB subs are available (#1629) This PR addresses the issue of inconsistent CC behaviour on return to normal play after trick / seek where the app selected CC track is dropped in favour of the default TTML track with DASH streams containing both CC and OOB subs With fix, we skip OOB subtitle scoring on seek/trickplay resume when the app explicitly selected a CC track. --------- Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> * VPAAMP-490: Prioritize latencyParams present in manifest over init config (#1545) * VPAAMP-490: Update latencyParams present in manifest as stream settings Reason for change: When latencyParams are present in the manifest, they should take precedence over the default values provided. Risks: Low Test Procedure: Test with LLD streams Priority: P1 Change-Id: I6a1efdee4c654012377604c31e9a221cd70aea68 Signed-off-by: Nandakishor U M <nu641001@gmail.com> * Reason for Change: l1test fix Signed-off-by: Philip Stroffolino <philip_stroffolino@cable.comcast.com> --------- Signed-off-by: Nandakishor U M <nu641001@gmail.com> Signed-off-by: Philip Stroffolino <philip_stroffolino@cable.comcast.com> Co-authored-by: pstroffolino <Philip_Stroffolino@cable.comcast.com> * VPAAMP-491: Use ProducerReferenceClock over UTC for calculating live latency (#1656) * VPAAMP-491: Use ProducerReferenceClock over UTC for calculating live latency Reason for change: Switch to using ProducerReferenceClock for more accurate live latency calculation. Risks: Low Test Procedure: Test with LLD Streams Priority: P1 Change-Id: Iee1920db51edff829e55aae7aa2c01292a34cf3a Signed-off-by: Nandakishor U M <nu641001@gmail.com> * VPAAMP-491: Apply suggestions from code review Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> Change-Id: I21a54e7dc03b862a37f363b64df5b5d73a1bea27 * Apply suggestions from code review Changing log level to WARN as it is called once Co-authored-by: Nandu <nu641001@gmail.com> * VPAAMP-491: Fix L1 Change-Id: Ic11555497dec8fdb145d95d2f44e296d6b4b20ba Signed-off-by: Nandakishor U M <nu641001@gmail.com> --------- Signed-off-by: Nandakishor U M <nu641001@gmail.com> Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> * Fix build failure --------- Signed-off-by: Gnanesha <gnaani82@gmail.com> Signed-off-by: psiva01 <sivasubramanian.patchaiperumal@ltts.com> Signed-off-by: varshnie <varshniblue14@gmail.com> Signed-off-by: Vinish100 <vinish.balan@gmail.com> Signed-off-by: srikanthreddybijjam-comcast <srikanthreddybijjam.2000@gmail.com> Signed-off-by: varshnie varshniblue14@gmail.com Signed-off-by: Nandakishor U M <nu641001@gmail.com> Signed-off-by: Philip Stroffolino <philip_stroffolino@cable.comcast.com> Co-authored-by: pstroffolino <Philip_Stroffolino@cable.comcast.com> Co-authored-by: Gnanesha <gnaani82@gmail.com> Co-authored-by: Sivasubramanian Patchaiperumal <sivasubramanian.patchaiperumal@ltts.com> Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> Co-authored-by: hthakkar-synamedia <115170760+hthakkar-synamedia@users.noreply.github.com> Co-authored-by: varshnie <125456267+varshnie@users.noreply.github.com> Co-authored-by: Vinish K B <vinish.balan@gmail.com> Co-authored-by: srikanthreddybijjam-comcast <srikanthreddybijjam.2000@gmail.com> Co-authored-by: vinodkadungoth <vinodkadungoth@gmail.com> Co-authored-by: DomSyna <192202460+DomSyna@users.noreply.github.com> Co-authored-by: Nandu <nu641001@gmail.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.
This PR addresses the issue of inconsistent CC behaviour on return to normal play after trick / seek where the app selected CC track is dropped in favour of the default TTML track with DASH streams containing both CC and OOB subs.