Feature/vplay 12902 vpaamp 319#1580
Draft
anshephe wants to merge 135 commits into
Draft
Conversation
via Rialto Gstreamer sinks
Changed audio mime/codec values to match what rialto expects
…n't start at pts 0 Fixed play() being called too early
Replace pkg_check_modules(RIALTOCLIENT REQUIRED RialtoClient) with find_package(Rialto REQUIRED), using a new cmake/FindRialto.cmake module copied from rialto-gstreamer. The pkg-config approach requires RialtoClient.pc to be installed into the sysroot, but rialto's CMakeLists.txt only installs that file when built with NATIVE_BUILD=ON, which is not set in the Yocto cross- compilation pipeline. The find_package approach instead locates libRialtoClient.so and IMediaPipeline.h directly via find_library/ find_path, matching the mechanism already used successfully by rialto-gstreamer.
…arget
The Rialto::RialtoClient imported target created by FindRialto.cmake is
not GLOBAL, causing cmake's generation phase to fail to resolve it when
validating the link graph for targets like aampjsbindings and aamp-cli
that inherit it transitively.
Replace Rialto::RialtoClient with ${RIALTO_LIBRARIES}, which holds the
absolute path to libRialtoClient.so set directly by find_library().
This is equivalent to the previous ${RIALTOCLIENT_LINK_LIBRARIES} from
pkg_check_modules and avoids the imported target scope issue entirely.
Improve AampRialtoPlayer code and add L1 tests.
The Rialto server uses per-segment codec data (equivalent to GStreamer's
caps codec_data field) to update the downstream decoder whenever the
codec parameters change mid-stream. AampRialtoPlayer was not forwarding
this data at all, and discarded updates from subsequent init fragments.
Changes:
- direct-rialto/AampRialtoPlayer.cpp
- AttachVideoSource: call GetCodecInfo() and update m_videoCodecData,
m_videoWidth, and m_videoHeight before the early-return guard so
that a second init fragment (mid-stream codec change) refreshes the
cache even when the source is already attached.
- AttachAudioSource: same restructuring for m_audioCodecData,
m_audioSampleRate, and m_audioChannels.
- InjectSamples: call segment->setCodecData() on every MediaSegment
using the cached m_videoCodecData / m_audioCodecData, mirroring
BufferParser::addCodecDataToSegment in rialto-gstreamer.
- test/utests/tests/AampRialtoPlayerTests/AampRialtoPlayerTestCases.cpp
- Add Phase 12 tests:
- InjectSamples_VideoSegment_CarriesCodecDataFromInitFragment
- InjectSamples_AudioSegment_CarriesCodecDataFromInitFragment
- InitFragment_SecondVideoInit_UpdatesCodecDataWithoutReattaching
Implement end-to-end encrypted playback in AampRialtoPlayer without any GStreamer dependency. New files in direct-rialto/: - IDrmBridge.h: pure interface for DRM session lifecycle (DIP). - AampDrmBridge.h/.cpp: concrete implementation delegating to AAMP's DrmSessionManager / OCDM stack to obtain a Rialto media key session ID (mks_id). Changes to AampRialtoPlayer: - QueueProtectionEvent: stores protection parameters (systemId, initData, type); defers createSession() until the init fragment arrives so the license pre-fetcher has time to acquire the key. - AttachVideoSource / AttachAudioSource: call IDrmBridge::createSession() and pass hasDrm=true to Rialto's attachSource when a valid mks_id is obtained. - ClearProtectionEvent: calls IDrmBridge::clearSessions() and resets mks_id state. - InjectSamples: stamps encrypted MediaSegments with mks_id, key ID, IV, cipher mode, encryption pattern (CBCS), and subsample map. - Introduce QueuedSample struct so codec parameters and codec data are snapshotted at enqueue time, ensuring ABR changes take effect at the correct sample boundary. - Add m_attachMutex to serialise concurrent init-fragment delivery from the video and audio download threads. - SetDrmBridgeForTesting() seam for unit test injection. Tests (AampRialtoPlayerTests): - MockDrmBridge (Google Mock) and FakeAampDrmBridge (link-time stub). - Phase DRM test cases covering: deferred session creation, clearSessions on teardown, hasDrm flag on attachSource, encrypted segment annotation (mks_id, key ID, IV, CBCS pattern, subsample map, fallback whole-sample subsample). NOTE: The following shared AAMP files were also modified and should be reviewed independently per direct-rialto.instructions.md: - middleware/drm/DrmSession.h: add getMediaKeySessionId() virtual. - middleware/drm/ocdm/opencdmsessionadapter.h/.cpp: implement it. - mp4demux/MP4Demux.h/.cpp: ParseSampleEncryption takes a `next` bounds pointer; add IV/subsample OOB checks and debug logging. - AampStreamSinkManager.cpp: add MIL log lines around player creation. - CMakeLists.txt: add AampDrmBridge.cpp to LIBAAMP_SOURCES. - scripts/install_rialto.sh: clone rialto-ocdm alongside rialto.
Reason for Change: Make code easier to debug and improve its quality Summary of Changes: * New files and classes to implement the state machine * Verify changes in L1 tests * Add script to generate a graph of the state machine from the code Test Procedure: Play content using DirectRialto configuration Priority: P1 Risks: Medium
…ibrary Introduces IOpenCDM and IOpenCDMSession interfaces to remove direct calls to the OCDM C library from OCDMSessionAdapter. Two concrete implementations are provided: - OpenCDMProvider: delegates to the real OCDM C library (unchanged behaviour) - RialtoMediaKeysProvider: delegates to firebolt::rialto::IMediaKeys for the Rialto Direct path OpenCDMProviderFactory selects the correct implementation based on the eAAMPConfig_useRialtoDirect config flag. OCDMSessionAdapter is updated to use the new interfaces via m_ocdm and m_session members. Supporting mocks, fakes, and unit tests are updated accordingly.
…ayer
Introduce a narrow push-notification interface (IStreamSinkNotifiable)
so that AampRialtoPlayer can drive AAMP state without a direct
dependency on PrivateInstanceAAMP for notification calls.
Changes:
- direct-rialto/IStreamSinkNotifiable.h: new pure-virtual interface
(NotifyFirstFrameReceived, NotifyFirstBufferProcessed, LogFirstFrame,
LogTuneComplete, NotifyEOSReached, MonitorProgress, NotifySpeedChanged,
GetState)
- direct-rialto/PrivateInstanceAAMPNotifiable.{h,cpp}: thin adapter that
satisfies IStreamSinkNotifiable by forwarding to PrivateInstanceAAMP;
includes priv_aamp.h only in the .cpp; adds AAMPLOG_TRACE per method
- direct-rialto/AampRialtoPlayer.h: second constructor accepting an
injected IStreamSinkNotifiable* for testing; new members
(m_notifiable, m_notifiableAdapter, m_firstFrameNotified,
m_positionMs, m_durationMs, m_videoRectangle); OnPosition/OnDuration
private method declarations
- direct-rialto/AampRialtoPlayer.cpp: OnPlaybackState implements three
PLAYING sub-paths (initial tune, post-seek, resume-from-pause),
END_OF_STREAM, and FAILURE; OnPosition/OnDuration store ns→ms and
call MonitorProgress; SetVideoRectangle stores rect string and calls
setVideoWindow; GetVideoRectangle/GetPositionMilliseconds/
GetDurationMilliseconds return atomic values
- direct-rialto/AampRialtoMediaPipelineClient.{h,cpp}: add
PositionCallback and DurationCallback types, setters, and members;
notifyPosition/notifyDuration invoke their callbacks
- CMakeLists.txt: add PrivateInstanceAAMPNotifiable.cpp to libaamp sources
- test/utests/mocks/MockIStreamSinkNotifiable.h: GMock class for all 8
interface methods
- test/utests/tests/AampRialtoPlayerTests/: fixture uses
NiceMock<MockIStreamSinkNotifiable>; 16 new test cases covering
first-frame notifications, post-seek recovery, resume-from-pause,
EOS, position/duration callbacks, and SetVideoRectangle
- test/utests/tests/AampRialtoPlayerTests/CMakeLists.txt: add
PrivateInstanceAAMPNotifiable.cpp to AAMP_SOURCES
- test/utests/fakes/FakePrivateInstanceAAMPNotifiable.cpp: no-op stub
providing vtable for test targets that use FakeAampRialtoPlayer
- test/utests/fakes/FakeAampRialtoPlayer.cpp: include
PrivateInstanceAAMPNotifiable.h to complete unique_ptr<> destructor
opencdm_construct_session() retains the OpenCDMSessionCallbacks* pointer and fires it asynchronously from a WorkerPool thread (e.g. during opencdm_session_update). The pointer was previously taken from a stack-local variable in OpenCDMProvider::constructSession, which was already gone by the time the callbacks fired, causing a SIGILL crash ~2 seconds into DRM license processing. Fix: add an OpenCDMSessionCallbacks cCallbacks member to the heap-allocated OpenCDMSessionCallbackSet (cbCopy) and populate that instead of a local. The pointer passed to the OCDM library now lives for the full session lifetime, destroyed only when OpenCDMSessionProvider::destruct() is called. The bug was introduced in commit 8955958 when the previous approach (m_OCDMSessionCallbacks as a member of OCDMSessionAdapter) was replaced with the IOpenCDM abstraction layer.
Previously AampRialtoPlayer spawned two SourceWorker threads at construction time, possibly adding 2×8 MB of virtual address space per player instance before any tune was initiated. Workers are now created lazily on the first Configure() call and re-used across re-tunes via flush() rather than stop()/recreate(). Stop() resets the workers to null so the thread stacks are released immediately after playback ends.
…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>
…ayback The whole caching mechanism needs investigating in relation to Direct Rialto
* VPAAMP-434 DirectRialto - Update StreamSink Reason for Change: Reduce the risk when delivering DirectRialto changes Summary of Changes: - Add new DirectRialto configuration (cannot be enabled yet) - Add some virtual functions to StreamSink API - Rename AampStreamSinkManager vars and comments to make it more generic Test Procedure: No regressions, since this is pure refactoring Priority: P1 Risks: Low * Rename vars and comments to make StreamSink more generic * Rename AAMP config setting to useDirectRialto * Create a AampGstPlayer even if useDirectRialto=true * Fix L1 MediaStreamContextTests * Make AampStreamSinkManager log messages more generic * Update log line and comment to address review comments
…esh attempts to stop (#1531) * VPAAMP-477: a single curl 56 on manifest refresh causes manifest refresh attempts to stop After a download failure mMPDData->mIsLiveManifest is false (only set during successful parse), so refreshNeeded was never set and the downloader thread exited on the first refresh error. Add an else-if branch below the existing timeout/COULDNT_CONNECT fast-retry block to catch all other non-success error codes (including CURLE_RECV_ERROR / curl 56) when past the first download. Sets a 500 ms retry interval and keeps the refresh loop alive so the next manifest fetch is attempted rather than killing the downloader thread. The !firstDownload guard ensures tune-time failures still exit the loop immediately, preserving existing behaviour for initial tune failures. * VPAAMP-477: add recovery logging and fix unnecessary 500ms interval override Two follow-up improvements to the manifest refresh retry fix: 1. Add downloadFailed flag to track transitions from failure back to success. Set in both the timeout/COULDNT_CONNECT branch and the new transient-error branch; cleared with a WARN log when the next manifest fetch succeeds. This makes it easy to count recovery events in production logs independently of the per-failure curl error logs. 2. Remove the mRefreshInterval=MIN_DELAY_BETWEEN_PLAYLIST_UPDATE_MS override from the transient-error (curl 56 / non-timeout) branch. mRefreshInterval is a member that persists across iterations and is set by getMeNextManifestDownloadWaitTime() on every successful parse, so it already holds the correct MPD-derived interval. The 500ms override is appropriate for timeout/COULDNT_CONNECT (fast connectivity detection) but not for transient receive errors where the stream's own minimumUpdatePeriod should be respected. * VPAAMP-477: restore tuneUrl effective URL update dropped in previous commit The tuneUrl = mMPDData->mMPDDownloadResponse->sEffectiveUrl assignment was accidentally removed during refactoring. Without it, redirects are not followed on subsequent refreshes — the downloader keeps hitting the original URL instead of the post-redirect effective URL. * fix: use 500ms fast-retry for non-timeout manifest download errors - Change preProcessCallback type from std::function<std::string()> to std::function<std::pair<std::string,int>()> so callers can supply a specific curl/HTTP error code rather than always producing CURLE_OPERATION_TIMEDOUT - In the else-if retry block, use a local fastRetry = 500ms instead of mRefreshInterval; mRefreshInterval is not permanently overwritten so the MPD-derived update period is preserved after recovery - Update SendManifestPreProcessEvent in priv_aamp and all fakes/mocks to return pair<string,int> with CURLE_OPERATION_TIMEDOUT on empty - Add L1 regression test AampMPDDownloader_LiveRefreshRetriesWhenFailureIsCurlRecvError: injects CURLE_RECV_ERROR after first successful live fetch and asserts the downloader loop continues producing further refresh attempts
Reason for Change: To run existing L3 tests with DirectRialto Summary of Changes: - Add new RialtoSinkSetsDirectRialto config - If useRialtoSink and RialtoSinkSetsDirectRialto, set DirectRialto and other config Test Procedure: Confirm DirectRialto is enabled when running L3 tests Priority: P1 Risks: Low
Reason for Change: Comply with Position Reporting AC Test Procedure: Configure and check ProgressReport Priority: P1 Risks: Low
* Don't call AcquireLicenseCb if not registered
…diately then at the interval. If StartProgressTimer is called on an existing timer it kicks an immediate callbacl, and then proceeds the timeout at the interval
…ogic" This reverts commit 6ea896c.
This PR updates AAMP’s sample-injection and direct-Rialto playback lifecycle handling to support trickplay flows, and includes multiple L1 test updates to cover (and/or accommodate) the new behavior. Changes: Extend the StreamSink / SendStreamTransfer sample path with a morePending flag so sinks can know when they’re receiving the last sample in a batch. Improve direct-Rialto trickplay/seek behavior: progress timer rework (immediate fire + “kick”), FLUSHING-state transitions on PLAYING/PAUSED callbacks, source-flush completion handling (notifySourceFlushed → setSourcePosition), and position reporting based on first injected PTS and playback rate. Add/extend mocks, fakes, and tests for new interfaces and behaviors (ResumeTrackDownloads, first video frame displayed notification, firstPts baseline, etc.).
…/tests/ Signed-off-by: anjali-syna <206662904+anjali-syna@users.noreply.github.com>
isInbandCC() in AampRialtoSubtitleSource was previously driven by a
mutable m_inbandCC flag set as a side-effect of mapCodecToMime(). This
created a subtle ordering dependency: the flag was only set once the
source was attached (codec mapped), so GetCCHandle() could query it
before attachment and always see false.
Replace the flag with a direct format check:
bool AampRialtoSubtitleSource::isInbandCC() const
{
return m_streamFormat == FORMAT_INVALID;
}
Configure() now calls setFormat(FORMAT_INVALID) on the inband-CC source
immediately after creation (before attachment), so the flag is correct
by the time OnPlaybackState::PLAYING fires.
Related clean-ups bundled in the same logical change:
- Remove needDataBatchSize() virtual and the subtitle override (return 1).
The per-batch limit was a workaround for the flag ordering issue; it is
no longer needed.
- Move isInbandCC() to the public section of AampRialtoMediaSource so
AampRialtoPlayer::GetCCHandle() can call it via the base pointer without
a dynamic_cast to AampRialtoSubtitleSource. Remove the corresponding
#include of AampRialtoSubtitleSource.h from AampRialtoPlayer.cpp.
- Fix ShouldRecreatePipeline() to check the subtitle source format
alongside video and audio rather than silently ignoring it.
- Fix MockAampRialtoMediaSource: replace the injectSingleSampleProxy /
processDataFragmentProxy indirection (which checked mediaType() ==
eMEDIATYPE_SUBTITLE in non-virtual C++ methods — a hack) with plain
MOCK_METHOD overrides wired via ON_CALL in the fixture. Add
MOCK_METHOD for isInbandCC() and delegate it to format() == FORMAT_INVALID
so the mock matches production behaviour.
Fixes failing L1 tests:
AampRialtoPlayerTest.OnPlayingState_InbandCC_PassesNonZeroDecoderHandle
AampRialtoPlayerWithDemuxTest.OnPlaybackState_Playing_FirstTime_CallsAllFirstFrameNotifications
AampRialtoPlayerWithDemuxTest.OnPlaybackState_Playing_PostSeek_CallsNotifyFirstBufferAndFrame
AampRialtoPlayerWithDemuxTest.OnNeedMediaData_InbandCCSource_RespondsWithNoAvailableSamples
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
11 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.