dev sync#1459
Merged
Merged
Conversation
…than simply testing what is present
Reason for Change: Fix static loop counter in HybridABRManager and mutex unlock-before-read in updateProfile - Bug #1: Replace static int loop with per-instance mRampupFromSteadyStateLoop in HybridABRManager::CheckRampupFromSteadyState, matching the new ABR implementation. - Bug #2: Move lock.unlock() after all mProfiles reads in updateProfile() for both legacy ABRManager and new abr/abr.cpp to prevent races during period transitions. - Add unit tests for per-instance loop independence and updateProfile iframe selection. * Reason for Change: fixed test to cover intent Signed-off-by: Philip Stroffolino <philip_stroffolino@cable.comcast.com> --------- Signed-off-by: Philip Stroffolino <philip_stroffolino@cable.comcast.com> Co-authored-by: Philip Stroffolino <philip_stroffolino@cable.comcast.com>
…es without need to run external tool (#1328) Reason for change: Mechanism added to create JSON persona automatically based on curl profiling. - JSON creation is triggered once stop API is received, - Network Persona utests added. - Add NetPersonaFitter::ResetForTesting() (AAMP_TEST_BUILD-guarded) so each unit test starts from a clean singleton state, eliminating order-dependence under --gtest_shuffle or sharding. - Wire ResetForTesting() into NetPersonaFitterTest::SetUp(); remove the now-redundant conditional guard in EmptyDataReturnsFalse. - Narrow FlushCsv() FileState mutex scope to cover only CSV writes; move persona fitter calls outside the lock to avoid unnecessary contention (NetPersonaFitter has its own internal mutex). Risks: Medium Priority: P1 Signed-off-by: Rajat <emailofrajatyadav@gmail.com> Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --------- Signed-off-by: Rajat <emailofrajatyadav@gmail.com> Co-authored-by: pstroffolino <Philip_Stroffolino@cable.comcast.com> Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…D guard (#1349) Reason for Change: The test build does not define AAMP_TEST_BUILD, so the method was compiled out and caused a build error in NetPersonaFitterTestCases.cpp. Remove the ifdef guards — ResetForTesting() is test-only by convention and inert in production (never called outside test code).
…plied to real AAMP playback (#1325) * VPAAMP-145 abrsim - network persona-based induced latency patterns applied to real AAMP playback Reason for Change: help isolate abrsim integration issues from true aamp behavior, and open door to additional abr testing with real aamp in simulator and devices - support for network persona changes - spread induced delays to so that aamp's interruption logic works. Optimization to minimize impact from this path in production code. - critical bug fix avoiding deadlock, plus patch to reflect latency correctly in httpRequestEnd telemetry - move lazy loading of network persona to curlDownloader. Since every tune starts with manifest download, this ensures that all downloads (even first manifest download) honor simulated latency throttling. - fixed budget and early exit for networkTimeout enforcement --------- Signed-off-by: Philip Stroffolino <philip_stroffolino@cable.comcast.com>
…1350) Reason for Change: Eliminate unnecessary deep copy of fragment data (2-7 MB for UHD) in CacheTsbFragment by using move assignment instead of Copy(). The move assignment operator already exists on CachedFragment. Also add null guard for GetFetchChunkBuffer return value to prevent potential null pointer dereference. (bug discovered while doing above) LL-DASH chunk mode callers that need the fragment for both CacheTsbFragment and EnqueueWrite now create a separate copy before moving into the cache. Add unit test validating move semantics transfer ownership correctly. Also: Refactoring to enforce std::move use at compile time. Update for old (misleading) comment. Test Guidance: regression + no test breakage --------- Signed-off-by: Philip Stroffolino <philip_stroffolino@cable.comcast.com> Co-authored-by: pstroffolino <Philip_Stroffolino@cable.comcast.com>
…1305) on linear channel on performing multiple times trickplay/Seek" (#1302) Reason for Change: Fixes potential race in teardown/restart flow observed under trickplay * Move cJSON_CreateObject outside mutex * Reason for Change: NULL -> nullptr * AampProfiler: OOM guard + narrow mutex critical sections in TuneBegin/GetTelemetryParam - Guard against cJSON_CreateObject() returning NULL (OOM): keep existing telemetryParam intact and log an error rather than leaving it null and crashing callers like SetLatencyParam that dereference it without a guard. - Move cJSON_Delete() and cJSON_PrintUnformatted() outside the discontinuityParamMutex critical section: swap the pointer under the lock, then free/log the old tree after release to reduce lock contention. --------- Signed-off-by: Philip Stroffolino <philip_stroffolino@cable.comcast.com>
Reason for Change: Replace per-sample copies with zero-copy sample views backed by shared, move-owned fragment buffers. Test Guidance: regression only Risk: Low --------- Co-authored-by: pstroffolino <Philip_Stroffolino@cable.comcast.com>
… event with speed=1 emitted during rewind speed change (e.g., -12x to -30x" (#1351) Reason for Change: Problem Statement: During trickplay (rate ≠ 1), calling Seek() to a specific position unconditionally reset aamp->rate to AAMP_NORMAL_PLAY_RATE and fired a NotifySpeedChanged(1) event. This spurious speed=1 notification was incorrect — the player should resume trickplay at the original rate after a positional seek. Fix — main_aamp.cpp SeekInternal() (active-playback seek path) and SeekAfterPrepared() (deferred seek path for eSTATE_INITIALIZED/eSTATE_PREPARING) now both apply the same conditional logic: Seek typeRate after seekNotifySpeedChanged fired?eTUNETYPE_SEEK (positional)preservedNoeTUNETYPE_SEEKTOLIVEreset to 1YeseTUNETYPE_SEEKTOENDreset to 1Yes Previously only SeekInternal() had the guard; SeekAfterPrepared() was still unconditional. Test Guidance: refer ticket Risk: Low Co-authored-by: pstroffolino <Philip_Stroffolino@cable.comcast.com>
* VPAAMP-201 Move SpeedCache to common AampSpeedCache.h header Reason for Change: SpeedCache was defined identically in three places: - priv_aamp.h - abr/abr.cpp - support/aampabr/HybridABRManager.cpp Introduce AampSpeedCache.h as the single canonical definition. Replace each duplicate with an #include "AampSpeedCache.h". Also add the include to abr/abr.h and HybridABRManager.h so their CheckLLDashABRSpeedStoreSize declarations are self-contained and callers no longer need to pull in priv_aamp.h for the struct.
…nsumers Reason for Change: Reintroduces feature, with accommodations to avoid FOG build failure. This reverts commit d09a62c. This PR reintroduces a shared SpeedCache definition by moving the duplicated struct into a common AampSpeedCache.h header and updating ABR-related components to include it, as part of preparing for a later reintroduction of the prior change. Changes: Add new common header AampSpeedCache.h containing struct SpeedCache. Replace duplicated SpeedCache struct definitions in ABR/AAMP code with includes of the new header. Add conditional/fallback SpeedCache definition in HybridABRManager.h for header-only consumers. And follow-up changes: - Install AampSpeedCache.h as a public header (CMakeLists.txt) so that consumers of abr/abr.h find it on the installed include path - Remove redundant #include "AampSpeedCache.h" from HybridABRManager.cpp; SpeedCache is already provided via HybridABRManager.h - Drop broken #else branch in HybridABRManager.h that unconditionally included AampSpeedCache.h on toolchains lacking __has_include support; the inline fallback definition now covers that case correctly --------- Signed-off-by: Philip Stroffolino <philip_stroffolino@cable.comcast.com>
…h calculations (#1342) Reason for Change: Fix integer truncation in legacy HybridABRManager bandwidth calculations Issue 1 — HybridABRManager.cpp: CheckAbrThresholdSize now computes (bufferlen * 8000L) / downloadTimeMs instead of (bufferlen / downloadTimeMs) * 8000, preventing truncation to zero for small fragments. This was already fixed in abr.cpp Issue 2 — Same file: CheckLLDashABRSpeedStoreSize now computes (total_dl_diff * 8000L) / time_diff instead of (total_dl_diff / time_diff) * 8000. This was already fixed in abr.cpp Tests — HybridAbrTests.cpp: 4 new tests covering small-fragment and large-fragment cases for both functions. Adjust fragmentDurationMs from 4000 to 1500 so the test verifies the multiply-before-divide calculation without triggering the guard clause that resets downloadbps to currentProfilebps when downloadTimeMs < fragmentDurationMs/2. Signed-off-by: Philip Stroffolino <philip_stroffolino@cable.comcast.com> --------- Signed-off-by: Philip Stroffolino <philip_stroffolino@cable.comcast.com> Co-authored-by: pstroffolino <Philip_Stroffolino@cable.comcast.com>
…cy and new abr (#1348) Reason for Change: Fix FragmentfailureRampdown to exclude iframe tracks from selection Filter iframe tracks from availableProfiles in FragmentfailureRampdown() in both ABRManager (abr/abr.cpp) and HybridABRManager using erase-remove, matching the pattern used by every other ABR function. This prevents an iframe track bandwidth from being selected as a rampdown target or as the fallback lowest profile. Add 6 L1 tests covering iframe filtering, normal rampdown, and fallback behavior for both ABR implementations. --------- Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Co-authored-by: Philip Stroffolino <philip_stroffolino@cable.comcast.com>
…k in VOD assets (#1367) * VPLAY-13232 Observing abnormal position value jump after CDAI ad break in VOD assets Cherry-picked from support/2.11.1_8.4_vipa (#1329). If the current SkipFragments loop reaches EOS and another playable period is available, switch to that period and return true so the seek can be retried. This path is required to update mFirstPTS correctly; otherwise, mFirstPTS may be updated incorrectly and cause a position jump. - Added HandleSeekEOSAndPeriodTransition() and invoked it from SeekInPeriod() - Added clarifying comments for the EOS/direction guards - Updated MPD functional test harness with a two-period regression test * VPLAY-13232 Fix CacheFragment mock call to use 9-param signature The cherry-picked test was written against the support/2.11.1_8.4_vipa branch where CacheFragment had 11 parameters (added pto and overWriteTrackId). dev_sprint_25_2 has the 9-parameter signature; drop the extra two matchers. * VPLAY-13232 Fix unexpected GetTSBSessionManager call in L1_SeekInPeriod_TwoPeriods_UpdatesFirstPTS g_mockPrivateInstanceAAMP is a StrictMock; any unexpected call fails the test. dev_sprint_25_2 calls GetTSBSessionManager() in the SkipFragments path, unlike the support branch where the test was authored. Uncomment the already-present expectation to allow the call. --------- Co-authored-by: vinodkadungoth <33543349+vinodkadungoth@users.noreply.github.com>
…ipa (#1374) Reason for Change: Apply the same trickplay-seek rate fix to SeekAfterPrepared() that was already applied to SeekInternal() in PR #1351. When the deferred seek path is taken (eSTATE_INITIALIZED / eSTATE_PREPARING), positional seeks now preserve the trickplay rate and skip the spurious speed=1 notification, consistent with the SeekInternal() behaviour. - Add three unit tests covering the trickplay seek scenarios: * SeekInternal_TrickplayPositionSeek_PreservesRateNoSpeedEvent * SeekInternal_TrickplaySeekToLive_ResetsRateAndNotifies * SeekInternal_TrickplaySeekToEnd_ResetsRateAndNotifies
…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>
Reason for change:Add dynamic latency threshold restoration: tighten min/target/max back toward config defaults one step at a time when the buffer consistently maintains above danger buffer of stable sec(300s) after a rebuffering event. Risks: Medium Test Guidance: refer ticket Signed-off-by: varshnie varshniblue14@gmail.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>
Reason for Change: Changes required for DirectRialto implementation Summary of Changes: - Update versions of Rialto and Protobuff installed with AAMP - Install Rialto header files, even if not using 'rialto' option - Add hint on where to find the Rialto library - Add automake and libtool dependencies - Address review comments - Make cmake less verbose, addressing one comment from Copilot - rialto-ocdm no longer needed - Fix gstreamer spelling - Install package autoconf Test Procedure: Build AAMP with rialto option Priority: P1 Risks: Low --------- Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Reason for change: Set default drm as widevine as part of VPAAMP-31 Test Procedure: updated in ticket Risks: Medium Signed-off-by: Reshma-JO07 <sreshmaraphaelk@gmail.com> Co-authored-by: pstroffolino <Philip_Stroffolino@cable.comcast.com>
Reason for Change: update instruction hints in substantive ways, intended to give more meaningful copilot review feedback * Add milestone to logging guidance * overhaul of commenting & documentation guidelines --------- Co-authored-by: pstroffolino <Philip_Stroffolino@cable.comcast.com>
Reason for Change: auto-install gstreamer via homebrew when framework is absent
install_pkgs_fn now checks for the GStreamer macOS framework at
/Library/Frameworks/GStreamer.framework/Versions/1.0/lib/pkgconfig.
If the framework is not present it installs gstreamer and
gst-plugins-base via homebrew so that the cmake configure step has a
valid pkg-config provider for gstreamer-1.0 and gstreamer-app-1.0.
Previously, neither package was in the homebrew install list, causing
silent cmake failures on machines without the standalone framework.
* build(osx): detect gstreamer from framework or homebrew in cmake PKG_CONFIG_PATH
aampcli_install_build_darwin_fn was unconditionally prepending the
GStreamer framework pkgconfig path to PKG_CONFIG_PATH with no existence
check. If the framework was not installed (e.g. gstreamer installed via
homebrew instead), cmake would fail with the opaque error:
Package 'gstreamer-1.0' not found
Now the function:
1. Uses the framework path if /Library/Frameworks/GStreamer.framework
/Versions/1.0/lib/pkgconfig exists (existing behaviour preserved).
2. Falls back to brew --prefix gstreamer and brew --prefix
gst-plugins-base when the framework is absent.
3. Fails early with a clear, actionable error message if neither
location is found, rather than propagating to cmake.
* scripts: scope _GST_* variables as local in install functions
Reason for Change: Fix BoS edge case identified in review of PR #1345 Summary of Changes: - Ensure progress event is always sent before BOS notification - Test edge case in L1 test Test Procedure: L1 test PrivAampTests should pass Priority: P1 Risks: Low Co-authored-by: pstroffolino <Philip_Stroffolino@cable.comcast.com>
Reason for Change: apply identical framework-vs-homebrew detection logic to run.sh so it behaves consistently with install_aampcli.sh Signed-off-by: Philip Stroffolino <philip_stroffolino@cable.comcast.com>
) Reason for change: lstring::equal(const char*) could read past the null terminator of cstring when cstring was a strict prefix of the lstring content, causing a crash in HLS manifest parsing. Reproduce: attrName = KEYFORMATVERSIONS= (len=18), cstring = "KEYFORMAT" (9 chars) Changes: - Fix equalsCString (was equal(const char*)): index-based loop; never reads past cstring null terminator. Regression: KEYFORMATVERSIONS vs KEYFORMAT no longer crashes. - Fix SubStringMatch: add n>=len guard to prevent OOB read on lstring. - Fix removePrefix(const char*): same OOB guard. - Fix substr: return empty lstring on negative or out-of-range offset instead of invoking UB through pointer arithmetic / size_t wraparound. - Rename equal(const char*) -> equalsCString for clarity. - Rename equal(const lstring&) -> isSameView; add comment documenting that this is pointer-identity (not content equality), intentionally used as a fast init-fragment-change check in fragmentcollector_hls. - Add equalsContent(const lstring&) for true content equality. - Add LSTRING_ENABLE_LEGACY_EQUAL backward-compat wrappers. - Refactor atof(): remove try/catch + stdexcept; accumulate directly into double to avoid integer overflow UB; best-effort stop on second decimal point rather than failing entire conversion. - constexpr/noexcept cleanup; fix include guard (was reserved __name__); inline constexpr CHAR_* constants; add data() alias; clamp getLen() to INT_MAX; fix GetAttributeValueString to require closing quote. - lstringTests: new suites for equalsCString, isSameView, SubStringMatch, removePrefix, substr covering OOB regression scenarios. - lstringTests CMakeLists: drop heavyweight fakes library and CommonTestIncludes (which dragged in GStreamer); provide minimal FakeLstringDeps.cpp stub instead. Fixes Mac build without GStreamer. - fragmentcollector_hls: rename all .equal() call sites. Signed-off-by: pstroffolino <Philip_Stroffolino@cable.comcast.com> Co-authored-by: Philip Stroffolino <philip_stroffolino@cable.comcast.com>
#1504) Reason for Change: modernize PrivateInstanceAAMP’s “GStreamer operation” locking by replacing the manual SyncBegin()/SyncEnd() pair with an RAII-style SyncLock() that returns a std::unique_lock<std::recursive_mutex>, reducing the risk of mismatched lock/unlock calls across the player and collectors. Changes: Replace SyncBegin()/SyncEnd() with [[nodiscard]] SyncLock() in PrivateInstanceAAMP (header + implementation). Update call sites across player/collectors/DRM and unit-test fakes/mocks to use scoped RAII locking. Adjust unit tests to validate the new API entry point (SyncLock()). --------- Co-authored-by: pstroffolino <Philip_Stroffolino@cable.comcast.com>
…und for the fragment cache (#1478) Reason for Change: Before this change, the mCachedFragment ring buffer array in MediaTrack was sized by DEFAULT_LLD_CACHED_FRAGMENTS_PER_TRACK, but neither AampConfig nor SetCachedFragmentSize enforced that value as an upper bound. An integrator calling PlayerInstanceAAMP::SetDownloadBufferSize(N) with N > DEFAULT_LLD_CACHED_FRAGMENTS_PER_TRACK would not be rejected by the config layer, and a subsequent SetCachedFragmentSize(N) call would silently reject the resize (due to the size <= maxLLDCachedFragmentsPerTrack guard) while the array remained at its original capacity. This left a silent config/behaviour mismatch. This PR introduces MAX_CACHED_FRAGMENTS_PER_TRACK as a named compile-time hard cap, aligns the config range validation with it, and resizes the array to that cap so valid values in the full accepted range can actually be used.
…exceeds threshold (#1484) * VPAAMP-295: Add trickplay live-point bypass when accumulated latency exceeds threshold Reason for change: When a live stream accumulates >= 10s of target-latency increments, ie to final target reaches 16sec (e.g. due to repeated rebuffering), the live-point guard in SetRateInternal was preventing fast-forward trick-play even though the player was significantly behind the live edge. Risks: Low Test Procedure: Test with L1 unit tests (PrivAampTests, PlayerInstanceAAMPTests — 481 + 342 tests all passing); manual LLD stream playback to verify fast-forward unblocked after sustained latency accumulation Priority: P1 --------- Signed-off-by: Nandakishor U M <nu641001@gmail.com> Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> Co-authored-by: pstroffolino <Philip_Stroffolino@cable.comcast.com>
…ert (#1452) Reason for Change: Fix crash in AAMPGstPlayer::Pause Summary of Changes: - Add protection against null playerInstance to AAMPGstPlayer::Pause - Lock sync mutex in AAMPGstPlayer::Stop - Test changes in L1 tests - Add mocks for SyncBegin and SyncEnd Test Procedure: Send EAS alerts and confirm there is no crash Priority: P0 Risks: Low * Fix L1 tests failures by using NiceMock instead of StrictMock * Print log lines when enter and exit Pause() * Delete the playerInstance check from Pause() * Revert unnecessary change to Pause()
……" (#1496)" (#1497) Reason for Change: Updates PrivateInstanceAAMP::MonitorProgress() to prevent negative live-latency values during TSB-less linear HLS playback by deferring the start/end = -1 (XRE sentinel) assignment until after HLS latency is computed, and adds a unit regression test to guard against recurrence. Changes: Move the TSB-less linear start/end = -1 override to after live latency calculation in MonitorProgress(). Add a regression unit test validating GetCurrentLatencyMs() is never negative for TSB-less linear HLS playback.
…TYPE_VIDEO] and mMediaStreamContext[eMEDIATYPE_AUDIO] (#1487) Reason for Change: Replace the runtime guards and the regression test with comments at each site that document the invariant and the reasoning, so reviewers and future authors understand why the code is safe without dead guard code. --------- Signed-off-by: Philip Stroffolino <philip_stroffolino@cable.comcast.com>
Reason for Change: routine sprint bump of IP_AAMP_TUNETIME version Signed-off-by: psiva01 <sivasubramanian.patchaiperumal@ltts.com>
…egments (#1494) Reason for Change: Add monitorMp4Integrity: detect and dump corrupt MP4 segments Introduces eAAMPConfig_MonitorMp4Integrity (bool, default false). When enabled alongside useMp4Demux=true, every downloaded video/audio segment (including init segments) is parsed by a persistent per-track Mp4Demux instance. The per-track validator preserves init-segment state so encrypted streams do not produce false-positive MP4_PARSE_ERROR_INVALID_IV_SIZE errors on media segments. On parse failure the segment is logged at WARN level with the Mp4ParseError code and written to harvestPath (or /tmp as fallback) using the server-side filename. Files changed: AampConfig.h - add eAAMPConfig_MonitorMp4Integrity enum value AampConfig.cpp - add bool lookup table entry for monitorMp4Integrity priv_aamp.h - add CheckSegmentIntegrity() declaration and mVideoIntegrityValidator / mAudioIntegrityValidator unique_ptr<Mp4Demux> members priv_aamp.cpp - implement CheckSegmentIntegrity(); hook into GetFile() after harvest block for VIDEO/AUDIO/INIT media types Test Guidance: enable eAAMPConfig_MonitorMp4Integrity - see ticket for details Risk: Low
…nseManager::handleLicenseResponse (#1513) Reason for change: Deprecated deadcode Test Procedure: Refer jira ticket VPAAMP-416 Priority: P2 Signed-off-by: srikanthreddybijjam-comcast <srikanthreddybijjam.2000@gmail.com>
Reason for Change: consolidates the legacy live-latency “rate correction worker thread” logic into the unified AampLatencyMonitor flow, updating both production code paths and related unit tests to use EnableLatencyMonitor() / IsLatencyMonitorEnabled() rather than direct mDisableRateCorrection state Test Procedure: Refer jira ticket VPAAMP-28 Priority: P2 Signed-off-by: srikanthreddybijjam-comcast <srikanthreddybijjam.2000@gmail.com> --------- Signed-off-by: srikanthreddybijjam-comcast <srikanthreddybijjam.2000@gmail.com> Co-authored-by: pstroffolino <Philip_Stroffolino@cable.comcast.com>
* VPAAMP-396 DirectRialto - Update Mp4Demux Reason for Change: Deliver to dev sprint Mp4Demux changes made for POC Summary of Changes: - Ignore MP4 boxes of unknown type - Add aditional log lines to Mp4Demux - Verify this change in L1 tests Test Procedure: EsdsReadLenTests L1 tests should pass Priority: P2 Risks: Low * Print WARN when skipping an unknown MP4 box and remove added ERR * Add optional error detail string to setParseError()
|
You are seeing this message because GitHub Code Scanning has recently been set up for this repository, or this pull request contains the workflow file for the Code Scanning tool. What Enabling Code Scanning Means:
For more information about GitHub Code Scanning, check out the documentation. |
Reason for Change: Fixes subtitle rendering regressions after SSAI ad breaks for HLS/TS when eAAMPConfig_HlsTsEnablePTSReStamp is enabled by correcting how WebVTT timing is restamped and by resetting TS demuxer rollover state across discontinuities. Fix RestampSubtitle LOCAL parsing for non-standard CDN VTT format Some HLS CDN WebVTT segments place MPEGTS on a pseudo-cue settings line rather than in the X-TIMESTAMP-MAP header: WEBVTT X-TIMESTAMP-MAP=LOCAL:00:00:00.000 00:00:00.000 --> 00:00:00.420 ...,MPEGTS:3944266080 00:00:00.000 --> ... (real cue with text) The previous code used strchr(localPtr, ',') which searched past the line-ending newline, finding the comma inside the pseudo-cue settings. This caused localStr to absorb the pseudo-cue timing line (with an embedded newline), placing MPEGTS at the end of a cue settings line in the output instead of in the X-TIMESTAMP-MAP header: AFTER: X-TIMESTAMP-MAP=LOCAL:00:00:00.000,MPEGTS:160347600 The fix: limit the comma search to the current line by also checking for a preceding newline. If the comma comes after a newline (or there is no comma on the LOCAL line), extract LOCAL up to the newline only. MPEGTS is still found correctly via extractFieldPtr which scans the full headerView (including the pseudo-cue content). Fix RestampSubtitle test build: move to protected, expose via using Use explicit wrapper in TestableMediaTrack for RestampSubtitle - fragmentcollector_hls.cpp: fix misleading comment at SetSubtitlePtsOffset call site; ptsOffsetSecs is passed in seconds and the callee handles the unit conversion (ns for Rialto, ms for other sinks) - tsprocessor.cpp: remove commented-out per-packet AAMPLOG_TRACE; dead code in a hot loop with no runtime path to re-enable it Test Guidance: refer ticket Risk: Medium --------- Co-authored-by: pstroffolino <Philip_Stroffolino@cable.comcast.com>
* VPAAMP-413 Speed up L1 tests Reason for Change: Speed up L1 tests Summary of Changes: - Modify run.sh to print list of slow tests that take longer than 1s - Make AampCurlDownloader delay between 502 retries configurable - Modify various L1 tests to make them quicker Test Procedure: L1 tests should pass and slow tests printed Priority: P2 Risks: Low * Add script to list slow tests * Delete L1 tests wrongly added as part of the rebase * Address review comments
…s and documentation (#1491) Reason for change: Modified res override with dns Test Procedure: Refer jira ticket VPAAMP-102 Priority: P1 Signed-off-by: srikanthreddybijjam-comcast <srikanthreddybijjam.2000@gmail.com> Co-authored-by: pstroffolino <Philip_Stroffolino@cable.comcast.com>
* VPAAMP-20: Align Copilot instructions with C++17 and risk-based testing policy - Validate and correct applyTo globs (normalize l1-oracle-design, add description to js.instructions.md). - Remove active C++20 guidance (concepts, ranges, std::span, fold_left). Mark C++20 features as deferred, not current. - Clarify that new production code and new L1 test code must target C++17. - Add explicit modernization scope discipline: encourage local safe improvements, discourage opportunistic repository-wide rewrites. - Soften exception guidance: conservative usage on embedded / playback / hot paths, consistent with surrounding code, no throws across C ABI. - Replace absolute testing rules with risk- and contract-based guidance aligned with the existing L1 oracle and validity instructions. * Address medium-priority issues. Known trade-offs intentionally left unchanged: - `aamp.instructions.md` and `legacy-cpp-patterns.instructions.md` remain broadly scoped because they provide repository context rather than narrow file-style rules. - Some repeated testing guidance remains where reinforcement is useful for Copilot behaviour. - `auto` guidance remains principles-based to avoid creating artificial style disputes. --------- Co-authored-by: pstroffolino <Philip_Stroffolino@cable.comcast.com>
Reason for Change: persona was being logged unconditionally and unnecessarily as warning for every download. Test Guidance: check logs Risk: None Signed-off-by: Philip Stroffolino <philip_stroffolino@cable.comcast.com>
…ate event (#1485) * VPAAMP-261: AAMP_EVENT_TUNE_TIME_METRICS dispatched before STATE_CHANGED(PLAYING) Reason for change: Sent the tunetime metrics event from the progress callback to time it always after the player has moved to PLAYING state. In TuneFailure case, it will be sent during Stop call. PLAYING state event is used by VIPA and EPG to measure total tune time and hence should not be delayed. Test Procedure: Confirm from logs that tune time metrics event is sent after state=PLAYING event in tune success. Ensure the event is still sent in tune fail case Risks: Low Signed-off-by: Vinish100 <vinish.balan@gmail.com> * VPAAMP-261: address code review comments - priv_aamp.cpp: move mTuneMetricDataPending.load() guard inside SendTuneMetricsEvent() so the check lives in one place; remove redundant outer guard at the progress-event call site - priv_aamp.cpp: collapse if(eventAvailStatus){store(true)} patterns at LogTuneFailure and LogTuneComplete sites to store(eventAvailStatus) - priv_aamp.cpp: add centralised AAMPLOG_MIL state-transition log in SetState(); remove per-site redundant log at first-frame call site - AampEvent.h / AampEvent.cpp / FakeAampEvent.cpp: change TuneTimeMetricsEvent tuneMetricData parameter to rvalue reference (&&) - AampProfiler.h: expand GetTuneTimeMetricAsJson @return doc comment to describe the JSON object and all its keys * VPAAMP-261: fix TuneTimeMetricsEvent test call site for rvalue ref parameter std::move() the lvalue timeMetricData at the call site in AampEventTests SetUp() to match the updated TuneTimeMetricsEvent constructor signature which takes tuneMetricData as std::string &&. * VPAAMP-261: fix TuneTimeMetricsEventTest failures after rvalue ref change After the constructor was changed to take tuneMetricData as std::string&&, std::move(timeMetricData) in SetUp() left the member in a moved-from (empty) state, causing both Constructor and GetTuneMetricsData assertions to compare against "" instead of the expected value. Pass std::string{timeMetricData} instead: constructs an rvalue temporary that satisfies the && parameter while keeping the member intact for assertions. --------- Signed-off-by: Vinish100 <vinish.balan@gmail.com> Co-authored-by: Philip Stroffolino <philip_stroffolino@cable.comcast.com>
…1515) * VPAAMP-415 Replace L1 tests mocks raw pointers with shared pointers Reason for Change: Reduce the chances of leaks or undefined behaviour Summary of Changes: - Replace raw pointers g_mock** with smart pointers - L1 test only changes Test Procedure: L1 tests should pass Priority: P1 Risks: Low * Undo changes to middleware and AampGstPlayer L1 tests * Second try at undoing changes to AampGstPlayer L1 tests * Deleted L1 StreamAbstractionAAMP_MPD_Tests unintentionally added * Undo changes made to L1 InterfacePlayerRDKTests * Address PR comments about raw pointers * Fix PrivAampTests build failure --------- Co-authored-by: pstroffolino <Philip_Stroffolino@cable.comcast.com>
…1462) * VPAAMP-363: L1 tests for parallel init segment ordering regression Add three GoogleTest cases to AampTrackWorkerTests/FunctionalTests.cpp to cover the race condition introduced by PR 114108 (RDKAAMP-4072): InitJob_HighPriority_ExecutesBeforeEnqueuedMediaJobs Verifies that SubmitJob(..., highPriority=true) places the init segment job at the front of the per-track worker queue, executing before any already-queued media jobs. This is the protection mechanism that FetchFragment relies on when profileChanged=true. InitJob_LowPriority_ExecutesAfterEnqueuedMediaJobs Documents the ordering hazard on the DetectDiscontinuityAndFetchInit path where profileChanged has already been cleared (highPriority=false). The init job goes to push_back and executes after queued media jobs. A code fix must ensure init always carries high priority on this path. StoppedWorker_SubmitInitJob_FutureIsInvalid Verifies that a job submitted to a stopped worker is silently dropped (returned future is invalid). Documents the stop/retune race: if StopWorker() wins, the init segment is lost and the decoder is never initialised for that tune attempt. * test: RDKAAMP-4072 deterministic SegmentBase profileChanged regression test Add FetcherLoopTests::SegmentBase_WaitForFreeFragmentFails_ProfileChangedMustBeCleared to the StreamAbstractionAAMP_MPD suite. Why the previously committed AampTrackWorker L1 tests do NOT catch the bug: The three worker tests validate AampTrackWorker::SubmitJob priority ordering in isolation. The worker mechanism is correct; the bug is in the CALLER (FetchAndInjectInitialization, SegmentBase path) which leaves profileChanged true when WaitForFreeFragmentAvailable(0) returns false. The worker is never invoked on that path. Why the new test FAILS on unpatched code / PASSES with the fix: - Uses mSegmentBaseManifest (SegmentBase, not SegmentTemplate) so that FetchAndInjectInitialization enters the only branch where profileChanged is conditionally cleared. - Sets numberOfFragmentsCached == GetCachedFragmentSize() to force WaitForFreeFragmentAvailable(0) to return false. - Asserts EXPECT_FALSE(videoTrack->profileChanged) after the call. - Without fix: profileChanged stays true -> FAIL. - With fix (else-branch added): profileChanged is cleared -> PASS. Changes: - Added mSegmentBaseManifest static constexpr (VOD SegmentBase manifest). - Added InvokeFetchAndInjectInitialization() to TestableStreamAbstractionAAMP_MPD. - Added regression test. * fix: RDKAAMP-4072 clear profileChanged on SegmentBase WaitForFreeFragment failure In FetchAndInjectInitialization, the SegmentBase path called WaitForFreeFragmentAvailable(0) and only cleared profileChanged inside the success branch. When the ring buffer was full (WaitForFreeFragmentAvailable returned false) there was no else-branch, so profileChanged remained true. On the next pass through the FetcherLoop every OnFragmentDownloadComplete callback saw profileChanged=true and re-invoked FetchAndInjectInitialization, which failed again for the same reason. The init segment was never delivered to the decoder, AAMP_EVENT_TUNED never fired, and the application observed a ~20-second silence / hang. The SegmentTemplate and SegmentList-with-sourceURL paths were unaffected because they clear profileChanged unconditionally. Fix: add an else-branch that clears profileChanged when WaitForFreeFragmentAvailable(0) returns false, allowing the FetcherLoop to drain the ring buffer and re-attempt the init fetch on the next profileChanged cycle rather than spinning indefinitely. * test: fix SegmentBase regression test — add LoadIDX mock expectation SegmentBase streams call LoadIDX to fetch and parse the Segment Index box (SIDX, pointed to by indexRange). The test used StrictMock so the unexpected LoadIDX call caused an immediate failure before the profileChanged assertion was ever reached. Add EXPECT_CALL(*g_mockPrivateInstanceAAMP, LoadIDX(...)) using the same sidxBox lambda pattern already used by other SegmentBase tests in this file, so InitializeMPD completes normally and the regression assertion is exercised. * test/prod: fix ASSERT_TRUE anti-pattern and DRY profileChanged assignment FunctionalTests.cpp: replace ASSERT_TRUE(future.wait_for(...) == ready) with ASSERT_EQ so GoogleTest prints actual vs expected future_status on timeout failures (L1 anti-pattern: EXPECT_TRUE with comparison operators). fragmentcollector_mpd.cpp (~line 8538): hoist the unconditional profileChanged = false out of the if/else branches into a single statement after the WaitForFreeFragmentAvailable block, removing the duplicated assignment and obsolete else-branch (DRY). * fix: clear profileChanged on SegmentList-range WaitForFreeFragment failure Mirror the RDKAAMP-4072 fix applied to the SegmentBase path: move profileChanged = false outside the WaitForFreeFragmentAvailable(0) block in the SegmentList-with-byte-range init segment sub-path. Previously, if the ring buffer was full and WaitForFreeFragmentAvailable(0) returned false, profileChanged was left true, causing OnFragmentDownloadComplete to re-invoke FetchAndInjectInitialization on every subsequent media segment — an infinite silent-skip loop identical to the SegmentBase bug. * Reason for Change: remove not-needed parallel download disable for segment base Signed-off-by: Philip Stroffolino <philip_stroffolino@cable.comcast.com> * VPAAMP-437: protect IDX buffer with mIdxMutex for parallel download thread safety SegmentBase streams set DashParallelFragDownload=false to avoid a race on the IDX (sidx) buffer between the FetcherLoop writer and download worker thread readers. Now that the false-override is removed (parallel download re-enabled for SegmentBase), introduce std::mutex mIdxMutex on MediaStreamContext to protect IDX access across threads. Write side (FetcherLoop / init path - all in fragmentcollector_mpd.cpp): - SkipFragments lazy LoadIDX call - SkipFragments ClearAndRelease on EOS - FetchAndInjectInitialization ClearAndRelease on profile change Read side (download worker thread - MediaStreamContext.cpp): - DownloadFragment bandwidth-change range recompute block The mutex is uncontested on every normal segment download; it is only contested during the narrow window where a live manifest refresh clears and reloads IDX while a worker is mid-ABR-switch range recompute. * fix: pass .get() to %p format specifier for shared_ptr in fake * fix: restore missing closing brace on GetPeriodEndTime() in FetcherLoopTests * fix: restore missing closing brace on HandleSeekEOS_UpdateTrackInfoFails test --------- 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
20 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.