From aaeae3d04d1b757b6392a442e16ebeba3a135c49 Mon Sep 17 00:00:00 2001 From: anshephe <115161257+anshephe@users.noreply.github.com> Date: Mon, 6 Jul 2026 10:22:47 +0000 Subject: [PATCH 01/19] Changes to the way flushing is handled, in particular flushing state changes --- direct-rialto/AampPlayerStateMachine.cpp | 108 +++++++++-- direct-rialto/AampPlayerStateMachine.h | 20 +- direct-rialto/AampRialtoPlayer.cpp | 153 ++++++--------- direct-rialto/AampRialtoPlayer.h | 12 +- docs/rialto-integration/draw_state_machine.py | 17 +- .../AampPlayerStateMachineTestCases.cpp | 180 +++++++++++++++++- .../AampRialtoPlayerTestCases.cpp | 82 ++++---- 7 files changed, 420 insertions(+), 152 deletions(-) mode change 100644 => 100755 docs/rialto-integration/draw_state_machine.py diff --git a/direct-rialto/AampPlayerStateMachine.cpp b/direct-rialto/AampPlayerStateMachine.cpp index 0b21729646..3a46e4a029 100644 --- a/direct-rialto/AampPlayerStateMachine.cpp +++ b/direct-rialto/AampPlayerStateMachine.cpp @@ -116,11 +116,14 @@ class FlushingState final : public IPlayerState /// After a flush new init fragments arrive, restarting source attachment. std::unique_ptr onSourceAttaching() override; - /// Rialto sends PLAYING while flushing (e.g. after seek completes). - std::unique_ptr onPlaybackStarted() override; - - /// Rialto sends PAUSED while flushing (e.g. seek with keepPaused=1). - std::unique_ptr onPlaybackPaused() override; + // onPlaybackStarted() and onPlaybackPaused() are intentionally NOT + // overridden here. When Rialto sends PLAYING or PAUSED during a flush + // (delayed ack for a play() that was in-flight when Flush() started), + // PlayerStateMachine::onPlaybackStarted/Paused() intercepts the call, + // updates m_preFlushStateId, and returns without transitioning. The + // machine stays in FLUSHING until onFlushComplete() fires — keeping + // WaitForFlushToComplete() correctly blocked until all sources confirm + // flushed. See PlayerStateMachine::onPlaybackStarted/Paused() below. }; class StoppedState final : public IPlayerState @@ -212,16 +215,6 @@ std::unique_ptr FlushingState::onSourceAttaching() return std::make_unique(); } -std::unique_ptr FlushingState::onPlaybackStarted() -{ - return std::make_unique(); -} - -std::unique_ptr FlushingState::onPlaybackPaused() -{ - return std::make_unique(); -} - // ============================================================================ // PlayerStateMachine // ============================================================================ @@ -275,17 +268,100 @@ void PlayerStateMachine::onAllSourcesAttached() void PlayerStateMachine::onPlaybackStarted() { + { + // Edge-case race: a delayed PLAYING ack for a play() that was in-flight + // when Flush() started. Update m_preFlushStateId so onFlushComplete() + // will restore to PLAYING, but keep the machine in FLUSHING — this is + // the key advantage over immediately transitioning: WaitForFlushToComplete() + // (predicate: state != FLUSHING) stays blocked until all sources confirm + // flushed, preventing Configure() or a new Flush() from racing with + // in-flight OnSourceFlushed callbacks. + std::lock_guard lock(m_mutex); + if (m_state->id() == PlayerStateId::FLUSHING) + { + AAMPLOG_INFO("PlayerState: onPlaybackStarted during FLUSHING - " + "updating pre-flush state to PLAYING, " + "state remains FLUSHING until all sources flush"); + m_preFlushStateId = PlayerStateId::PLAYING; + return; + } + } dispatch(&IPlayerState::onPlaybackStarted); } void PlayerStateMachine::onPlaybackPaused() { + { + // Edge-case race: same reasoning as onPlaybackStarted above. + std::lock_guard lock(m_mutex); + if (m_state->id() == PlayerStateId::FLUSHING) + { + AAMPLOG_INFO("PlayerState: onPlaybackPaused during FLUSHING - " + "updating pre-flush state to PAUSED, " + "state remains FLUSHING until all sources flush"); + m_preFlushStateId = PlayerStateId::PAUSED; + return; + } + } dispatch(&IPlayerState::onPlaybackPaused); } void PlayerStateMachine::onFlush() { - dispatch(&IPlayerState::onFlush); + // Save the current state before entering FLUSHING so that + // onFlushComplete() can restore it when all sources report flushed. + std::lock_guard lock(m_mutex); + m_preFlushStateId = m_state->id(); + AAMPLOG_INFO("PlayerState: saving pre-flush state '%s' (id=%d)", + m_state->name(), static_cast(m_preFlushStateId)); + auto next = m_state->onFlush(); + if (next) + { + AAMPLOG_MIL("PlayerState: %s -> %s", + m_state->name(), next->name()); + m_state = std::move(next); + } +} + +void PlayerStateMachine::onFlushComplete() +{ + std::lock_guard lock(m_mutex); + + if (m_state->id() != PlayerStateId::FLUSHING) + { + // The edge-case race: onPlaybackStarted/Paused already exited + // FLUSHING before all sources confirmed flushed. This call is a + // no-op — the correct post-flush state was already applied. + AAMPLOG_INFO("PlayerState: onFlushComplete ignored - current state is '" + "%s', not FLUSHING (edge-case race already resolved)", + m_state->name()); + return; + } + + std::unique_ptr restored; + switch (m_preFlushStateId) + { + case PlayerStateId::PLAYING: + restored = std::make_unique(); + break; + case PlayerStateId::PAUSED: + restored = std::make_unique(); + break; + case PlayerStateId::SOURCES_ATTACHED: + restored = std::make_unique(); + break; + default: + AAMPLOG_WARN( + "PlayerState: onFlushComplete - unexpected pre-flush state %d," + " restoring SOURCES_ATTACHED as safe fallback", + static_cast(m_preFlushStateId)); + restored = std::make_unique(); + break; + } + + AAMPLOG_MIL("PlayerState: %s -> %s (flush complete; pre-flush state restored)", + m_state->name(), restored->name()); + m_state = std::move(restored); } void PlayerStateMachine::onStop() diff --git a/direct-rialto/AampPlayerStateMachine.h b/direct-rialto/AampPlayerStateMachine.h index d11bbf54ec..aa5959dcca 100644 --- a/direct-rialto/AampPlayerStateMachine.h +++ b/direct-rialto/AampPlayerStateMachine.h @@ -115,6 +115,12 @@ class IPlayerState /// Fired when Flush() is called. virtual std::unique_ptr onFlush() { return nullptr; } + /// Fired when all sources confirm SourceFlushedEvent, completing the + /// flush cycle. Only meaningful from FLUSHING; ignored from all other + /// states so the edge-case race (Rialto PLAYING arriving before + /// onFlushComplete) cannot cause a double-transition. + virtual std::unique_ptr onFlushComplete() { return nullptr; } + /// Valid from any state — transitions to STOPPED. /// Non-inline: body defined in .cpp after StoppedState is complete. virtual std::unique_ptr onStop(); @@ -185,6 +191,14 @@ class PlayerStateMachine /// @see IPlayerState::onFlush void onFlush(); + /// Fired when all sources report SourceFlushedEvent. + /// + /// Restores the state that was current when onFlush() was called + /// (PLAYING → PLAYING, PAUSED → PAUSED, SOURCES_ATTACHED → + /// SOURCES_ATTACHED). If the machine has already left FLUSHING + /// via the edge-case onPlaybackStarted/Paused path, this is a no-op. + void onFlushComplete(); + /// @see IPlayerState::onStop void onStop(); @@ -198,8 +212,12 @@ class PlayerStateMachine /// Execute a handler, apply the returned transition, and log MIL. void dispatch(std::unique_ptr (IPlayerState::*handler)()); - mutable std::mutex m_mutex; + mutable std::mutex m_mutex; std::unique_ptr m_state; + + /// State saved by onFlush() so that onFlushComplete() can restore it. + /// Only meaningful while the machine is in FLUSHING; undefined otherwise. + PlayerStateId m_preFlushStateId{PlayerStateId::IDLE}; }; #endif // AAMP_PLAYER_STATE_MACHINE_H diff --git a/direct-rialto/AampRialtoPlayer.cpp b/direct-rialto/AampRialtoPlayer.cpp index 5b4c689fdf..381e7bbba9 100644 --- a/direct-rialto/AampRialtoPlayer.cpp +++ b/direct-rialto/AampRialtoPlayer.cpp @@ -378,25 +378,13 @@ bool AampRialtoPlayer::ShouldRecreatePipeline( void AampRialtoPlayer::WaitForFlushToComplete() { std::unique_lock lock(m_flushMutex); + // Block until the state machine leaves FLUSHING. onFlushComplete() (called + // from OnSourceFlushed when all sources confirm flushed) transitions the + // state and then notifies m_flushCv, so this predicate will always become + // true once the flush cycle ends. m_flushCv.wait(lock, [this]() { - // Done immediately if the state machine has already left FLUSHING. - bool done = (m_stateMachine.currentState() != PlayerStateId::FLUSHING); - if (!done) - { - // Still FLUSHING: done once no source reports isFlushing(). - bool anyFlushing = false; - for (const auto &s : m_sources) - { - if (s && s->isFlushing()) - { - anyFlushing = true; - break; - } - } - done = !anyFlushing; - } - return done; + return m_stateMachine.currentState() != PlayerStateId::FLUSHING; }); } @@ -939,7 +927,11 @@ void AampRialtoPlayer::CheckAllSourcesAttached() return; } - if (m_allSourcesAttachedFlag.load(std::memory_order_relaxed)) + // Guard: allSourcesAttached() must be called exactly once per pipeline + // session. The machine is in SOURCES_ATTACHING only while waiting for + // all sources to register; once it advances past that state the call + // has already been issued. + if (m_stateMachine.currentState() != PlayerStateId::SOURCES_ATTACHING) { return; } @@ -961,6 +953,11 @@ void AampRialtoPlayer::CheckAllSourcesAttached() else { m_stateMachine.onAllSourcesAttached(); + // seq_cst store: pairs with the seq_cst load in Stream() so that + // either Stream() sees the flag and calls play() itself, or this + // function sees m_playRequested=true and calls it here. The state + // machine transition above uses a mutex (not seq_cst) so it cannot + // substitute for this atomic rendezvous. m_allSourcesAttachedFlag.store(true, std::memory_order_seq_cst); if (m_playRequested.load(std::memory_order_seq_cst)) @@ -1054,27 +1051,23 @@ void AampRialtoPlayer::Stream() // store before it reads m_playRequested (and vice-versa). m_playRequested.store(true, std::memory_order_seq_cst); - if (m_allSourcesAttachedFlag.load(std::memory_order_seq_cst)) + // If a flush is in progress, play() will be issued by OnSourceFlushed() + // once all sources confirm flushed and onFlushComplete() restores state. + // Checking state is safe because FLUSHING is only set by Flush() (AAMP + // thread) and only cleared by onFlushComplete() or the edge-case + // onPlaybackStarted/Paused path (both protected by the state machine + // mutex). + if (m_stateMachine.currentState() == PlayerStateId::FLUSHING) { - // Guard: if any source is still flushing, do not call play() - // yet. setSourcePosition() (which emits the GStreamer SEGMENT - // event) is called from OnSourceFlushed(), which fires after - // the server confirms the flush. Issuing play() before that - // point leaves the pipeline without a SEGMENT event and causes - // frames at large live-stream PTS values to be silently dropped. - // OnSourceFlushed() will issue the deferred play() once all - // sources report flushing complete. - for (const auto &source : m_sources) - { - if (source && source->isFlushing()) - { - AAMPLOG_INFO("deferring play() - source %d still flushing", - source->sourceId()); - AAMPLOG_INFO("EXIT"); - return; - } - } + AAMPLOG_INFO("deferring play() - flush in progress"); + AAMPLOG_INFO("EXIT"); + return; + } + // seq_cst load: pairs with the seq_cst store in CheckAllSourcesAttached() + // to guarantee one side always calls play(). + if (m_allSourcesAttachedFlag.load(std::memory_order_seq_cst)) + { // allSourcesAttached() already completed before this call - // promote to PLAYING immediately. bool async = false; @@ -1130,8 +1123,13 @@ void AampRialtoPlayer::Flush(double position, int rate, bool shouldTearDown) { AAMPLOG_INFO("ENTRY position=%f rate=%d shouldTearDown=%d", position, rate, shouldTearDown); - // Stage latest flush parameters first so re-entrant Flush() calls while - // FLUSHING can still retarget the pending segment start and applied rate. + // If a previous flush is still in progress, block until it completes + // before starting a new one. This keeps the flush cycle atomic: each + // Flush() call owns a single FLUSHING → cycle and + // removes the complexity of updating parameters mid-flush. + // Configure() follows the same pattern. + WaitForFlushToComplete(); + const int64_t posNs = static_cast(position * kNsPerSecond); m_pendingFlushPositionNs.store(posNs, std::memory_order_relaxed); m_pendingFlushRate.store(rate, std::memory_order_relaxed); @@ -1144,6 +1142,7 @@ void AampRialtoPlayer::Flush(double position, int rate, bool shouldTearDown) // // This differs from GStreamer which skips flush in non-PLAYING/PAUSED states. // Rialto needs to flush in states like SOURCES_ATTACHED to set up positions. + // WaitForFlushToComplete() above guarantees we are never in FLUSHING here. const PlayerStateId state = m_stateMachine.currentState(); const bool isPlayingPausedOrAttached = (state == PlayerStateId::PLAYING || state == PlayerStateId::SOURCES_ATTACHED || @@ -1158,35 +1157,6 @@ void AampRialtoPlayer::Flush(double position, int rate, bool shouldTearDown) AAMPLOG_INFO("EXIT - teardown requested"); return; } - - if (state == PlayerStateId::FLUSHING) - { - AAMPLOG_INFO("Flush requested while already FLUSHING - updated pending position/rate only (position=%f rate=%d shouldTearDown=%d)", - position, rate, shouldTearDown); - - // If some sources have already reported flushed, they will not emit another - // SourceFlushedEvent during this flush cycle. Re-apply the updated pending - // position/rate so all sources stay in sync. - if (m_pipeline) - { - for (const auto &s : m_sources) - { - if (s && s->isAttached() && !s->isFlushing()) - { - if (!m_pipeline->setSourcePosition( - s->sourceId(), posNs, /*resetTime=*/true, - computeAppliedRate(rate))) - { - AAMPLOG_WARN("setSourcePosition failed for sourceId=%d", - s->sourceId()); - } - } - } - } - - AAMPLOG_INFO("EXIT - already flushing"); - return; - } // Wake any in-flight data so it abandons the current batch. for (auto &source : m_sources) @@ -1800,9 +1770,11 @@ void AampRialtoPlayer::OnPlaybackState(firebolt::rialto::PlaybackState state) case firebolt::rialto::PlaybackState::PLAYING: { m_stateMachine.onPlaybackStarted(); - // If PLAYING arrives while a flush is still in progress the state - // machine has just left FLUSHING. Wake any thread blocked in - // WaitForFlushToComplete() so it can re-evaluate its predicate. + // Edge-case race: if PLAYING arrives before onFlushComplete() fires + // (a delayed ack for a play() that was in-flight when Flush() was + // called), the state machine has just transitioned out of FLUSHING. + // Wake any thread blocked in WaitForFlushToComplete() so it can + // re-evaluate its predicate. m_flushCv.notify_all(); // Clear injectionGated so inject threads resume blocking @@ -1952,29 +1924,30 @@ void AampRialtoPlayer::OnSourceFlushed(int32_t sourceId) m_rate.store(pendingRate, std::memory_order_relaxed); AAMPLOG_INFO("All sources flushed - committed playback rate=%d", pendingRate); + + // Restore the pre-flush state (PLAYING, PAUSED, or SOURCES_ATTACHED) + // before notifying WaitForFlushToComplete(), which unblocks as soon + // as the state leaves FLUSHING. + m_stateMachine.onFlushComplete(); m_flushCv.notify_all(); - } - // If Stream() was called while this source was still flushing, - // play() was deferred to avoid issuing it before setSourcePosition() - // sends the GStreamer SEGMENT event. Now that this source's - // setSourcePosition() has been sent, check whether all sources have - // finished flushing and, if so, issue the deferred play(). - if (allSourcesFlushed && - m_playRequested.load(std::memory_order_seq_cst) && - m_allSourcesAttachedFlag.load(std::memory_order_seq_cst)) - { - AAMPLOG_INFO("All sources flushed - issuing deferred play()"); - bool async = false; - if (!m_pipeline->play(async)) + // Issue play() only if the restored state is PLAYING. + // This correctly handles seek-while-paused: the pre-flush state was + // PAUSED so the pipeline stays paused after the flush. + if (m_stateMachine.currentState() == PlayerStateId::PLAYING) { - AAMPLOG_ERR("play() failed after flush"); + AAMPLOG_INFO("All sources flushed - issuing play() (pre-flush state was PLAYING)"); + bool async = false; + if (!m_pipeline->play(async)) + { + AAMPLOG_ERR("play() failed after flush"); + } + } + else + { + AAMPLOG_INFO("All sources flushed - not issuing play() (restored state=%s)", + m_stateMachine.currentStateName()); } - } - else if (!allSourcesFlushed) - { - AAMPLOG_INFO("source %d still flushing - play() still deferred", - sourceId); } } else diff --git a/direct-rialto/AampRialtoPlayer.h b/direct-rialto/AampRialtoPlayer.h index 5d11a40b17..ae5e98e652 100644 --- a/direct-rialto/AampRialtoPlayer.h +++ b/direct-rialto/AampRialtoPlayer.h @@ -444,8 +444,16 @@ class AampRialtoPlayer : public StreamSink, public IDirectRialtoCC /// transitions PAUSED→PLAYING only after all sources are registered. std::atomic m_playRequested{false}; - /// Set by CheckAllSourcesAttached() after allSourcesAttached() succeeds. - /// Stream() reads this to decide whether it can call play() immediately. + /// Tracks whether allSourcesAttached() was successfully sent for the + /// current pipeline session. Reset to false on pipeline rebuild. + /// + /// IMPORTANT — seq_cst rendezvous with m_playRequested: + /// Stream() stores m_playRequested=true (seq_cst) THEN loads this flag + /// (seq_cst). CheckAllSourcesAttached() stores this flag=true (seq_cst) + /// THEN loads m_playRequested (seq_cst). The seq_cst total order + /// guarantees one side always observes the other's write and calls + /// play(). A mutex-based state read does NOT participate in that total + /// order and cannot substitute for this atomic. std::atomic m_allSourcesAttachedFlag{false}; /// Cached subtitle mute state. Set by SetSubtitleMute() and re-applied diff --git a/docs/rialto-integration/draw_state_machine.py b/docs/rialto-integration/draw_state_machine.py old mode 100644 new mode 100755 index 0df3b72466..929238169a --- a/docs/rialto-integration/draw_state_machine.py +++ b/docs/rialto-integration/draw_state_machine.py @@ -86,14 +86,21 @@ def _require_graphviz(): ("PLAYING", "onFlush", "flush() + setSourcePosition()", "FLUSHING"), ("PAUSED", "onFlush", "flush() + setSourcePosition()", "FLUSHING"), + # Normal flush exit: all sources confirm SourceFlushedEvent. + # onFlushComplete() restores the state that was active when onFlush() was + # called. play() is issued only when the restored state is PLAYING. + ("FLUSHING", "onFlushComplete [pre=PLAYING]", "play()", "PLAYING"), + ("FLUSHING", "onFlushComplete [pre=PAUSED]", "", "PAUSED"), + ("FLUSHING", "onFlushComplete [pre=SOURCES_ATTACHED]","", "SOURCES_ATTACHED"), + # After flush + re-configure, new init fragments re-drive attachment. ("FLUSHING", "onSourceAttaching", "attachSource()", "SOURCES_ATTACHING"), - # FlushingState responds to Rialto playback state notifications. - # This prevents the state machine from staying stuck in FLUSHING when - # Rialto sends PLAYING or PAUSED during a flush/seek operation. - ("FLUSHING", "onPlaybackStarted", "Rialto sends PLAYING", "PLAYING"), - ("FLUSHING", "onPlaybackPaused", "Rialto sends PAUSED", "PAUSED"), + # Note: onPlaybackStarted/Paused during FLUSHING (delayed ack for a + # play() that was in-flight when Flush() started) update m_preFlushStateId + # but do NOT transition state. The machine stays in FLUSHING until + # onFlushComplete() fires, keeping WaitForFlushToComplete() correctly + # blocked until all sources confirm flushed. # ── Stop — valid from any non-terminal state ─────────────────────────── ("PIPELINE_CREATED", "onStop", "stop()", "STOPPED"), diff --git a/test/utests/tests/AampPlayerStateMachineTests/AampPlayerStateMachineTestCases.cpp b/test/utests/tests/AampPlayerStateMachineTests/AampPlayerStateMachineTestCases.cpp index be68c1973e..8be6428288 100644 --- a/test/utests/tests/AampPlayerStateMachineTests/AampPlayerStateMachineTestCases.cpp +++ b/test/utests/tests/AampPlayerStateMachineTests/AampPlayerStateMachineTestCases.cpp @@ -194,24 +194,34 @@ TEST_F(AampPlayerStateMachineTest, OnSourceAttaching_FromFlushing_TransitionsToS } /** - * @test FLUSHING + onPlaybackStarted → PLAYING. + * @test FLUSHING + onPlaybackStarted updates pre-flush state to PLAYING but + * does NOT exit FLUSHING. The machine only leaves FLUSHING via + * onFlushComplete(), which then restores to PLAYING. This keeps + * WaitForFlushToComplete() correctly blocked until all sources confirm + * flushed, preventing Configure() from racing with in-flight callbacks. */ TEST_F(AampPlayerStateMachineTest, - OnPlaybackStarted_FromFlushing_TransitionsToPlaying) + OnPlaybackStarted_FromFlushing_UpdatesPreFlushStateAndStaysFlushing) { m_sm.onPipelineLoaded(); m_sm.onSourceAttaching(); m_sm.onAllSourcesAttached(); m_sm.onFlush(); - m_sm.onPlaybackStarted(); + ASSERT_EQ(m_sm.currentState(), PlayerStateId::FLUSHING); + + m_sm.onPlaybackStarted(); // delayed ack: updates pre-flush, stays FLUSHING + EXPECT_EQ(m_sm.currentState(), PlayerStateId::FLUSHING); + + m_sm.onFlushComplete(); // sources all flushed: restores to PLAYING EXPECT_EQ(m_sm.currentState(), PlayerStateId::PLAYING); } /** - * @test FLUSHING + onPlaybackPaused → PAUSED. + * @test FLUSHING + onPlaybackPaused updates pre-flush state to PAUSED but + * does NOT exit FLUSHING. onFlushComplete() then restores to PAUSED. */ TEST_F(AampPlayerStateMachineTest, - OnPlaybackPaused_FromFlushing_TransitionsToPaused) + OnPlaybackPaused_FromFlushing_UpdatesPreFlushStateAndStaysFlushing) { m_sm.onPipelineLoaded(); m_sm.onSourceAttaching(); @@ -219,7 +229,12 @@ TEST_F(AampPlayerStateMachineTest, m_sm.onPlaybackStarted(); m_sm.onPlaybackPaused(); m_sm.onFlush(); - m_sm.onPlaybackPaused(); + ASSERT_EQ(m_sm.currentState(), PlayerStateId::FLUSHING); + + m_sm.onPlaybackPaused(); // delayed ack: updates pre-flush, stays FLUSHING + EXPECT_EQ(m_sm.currentState(), PlayerStateId::FLUSHING); + + m_sm.onFlushComplete(); // sources all flushed: restores to PAUSED EXPECT_EQ(m_sm.currentState(), PlayerStateId::PAUSED); } @@ -471,3 +486,156 @@ TEST_F(AampPlayerStateMachineTest, ConcurrentEvents_DoNotCrash) // Any valid state is acceptable; the test passes if there is no crash. SUCCEED(); } + +// =========================================================================== +// onFlushComplete — pre-flush state restoration +// =========================================================================== + +/** + * @test FLUSHING (pre-flush=PLAYING) + onFlushComplete → PLAYING. + * + * When a seek completes and sources report flushed, the state machine + * must restore the pre-flush PLAYING state, not wait for Rialto PLAYING. + */ +TEST_F(AampPlayerStateMachineTest, OnFlushComplete_FromFlushing_PreFlushPlaying_RestoresPlaying) +{ + m_sm.onPipelineLoaded(); + m_sm.onSourceAttaching(); + m_sm.onAllSourcesAttached(); + m_sm.onPlaybackStarted(); // pre-flush state = PLAYING + m_sm.onFlush(); + ASSERT_EQ(m_sm.currentState(), PlayerStateId::FLUSHING); + + m_sm.onFlushComplete(); + EXPECT_EQ(m_sm.currentState(), PlayerStateId::PLAYING); +} + +/** + * @test FLUSHING (pre-flush=PAUSED) + onFlushComplete → PAUSED. + * + * A seek while paused must restore PAUSED without issuing play(). + */ +TEST_F(AampPlayerStateMachineTest, OnFlushComplete_FromFlushing_PreFlushPaused_RestoresPaused) +{ + m_sm.onPipelineLoaded(); + m_sm.onSourceAttaching(); + m_sm.onAllSourcesAttached(); + m_sm.onPlaybackStarted(); + m_sm.onPlaybackPaused(); // pre-flush state = PAUSED + m_sm.onFlush(); + ASSERT_EQ(m_sm.currentState(), PlayerStateId::FLUSHING); + + m_sm.onFlushComplete(); + EXPECT_EQ(m_sm.currentState(), PlayerStateId::PAUSED); +} + +/** + * @test FLUSHING (pre-flush=SOURCES_ATTACHED) + onFlushComplete → + * SOURCES_ATTACHED. + * + * Flush can be called from SOURCES_ATTACHED (e.g. initial position setup). + */ +TEST_F(AampPlayerStateMachineTest, OnFlushComplete_FromFlushing_PreFlushSourcesAttached_RestoresSourcesAttached) +{ + m_sm.onPipelineLoaded(); + m_sm.onSourceAttaching(); + m_sm.onAllSourcesAttached(); // pre-flush state = SOURCES_ATTACHED + m_sm.onFlush(); + ASSERT_EQ(m_sm.currentState(), PlayerStateId::FLUSHING); + + m_sm.onFlushComplete(); + EXPECT_EQ(m_sm.currentState(), PlayerStateId::SOURCES_ATTACHED); +} + +/** + * @test onFlushComplete while NOT in FLUSHING is silently ignored. + * + * The race where Rialto already emitted PLAYING before all sources + * flushed can leave the machine in PLAYING when onFlushComplete fires. + * The call must be a no-op. + */ +TEST_F(AampPlayerStateMachineTest, OnFlushComplete_WhileNotFlushing_IsIgnored) +{ + m_sm.onPipelineLoaded(); + m_sm.onSourceAttaching(); + m_sm.onAllSourcesAttached(); + m_sm.onPlaybackStarted(); // state = PLAYING (not FLUSHING) + + m_sm.onFlushComplete(); // must be a no-op + EXPECT_EQ(m_sm.currentState(), PlayerStateId::PLAYING); +} + +/** + * @test Pre-flush state is updated across successive flushes. + * + * After PLAYING→FLUSHING→onFlushComplete→PLAYING→PAUSED→FLUSHING→ + * onFlushComplete, the state must be PAUSED (second pre-flush wins). + */ +TEST_F(AampPlayerStateMachineTest, OnFlushComplete_SuccessiveFlushes_TracksLatestPreFlushState) +{ + m_sm.onPipelineLoaded(); + m_sm.onSourceAttaching(); + m_sm.onAllSourcesAttached(); + m_sm.onPlaybackStarted(); // PLAYING + + // First flush cycle: from PLAYING + m_sm.onFlush(); + m_sm.onFlushComplete(); + ASSERT_EQ(m_sm.currentState(), PlayerStateId::PLAYING); + + m_sm.onPlaybackPaused(); // PAUSED + + // Second flush cycle: from PAUSED + m_sm.onFlush(); + m_sm.onFlushComplete(); + EXPECT_EQ(m_sm.currentState(), PlayerStateId::PAUSED); +} + +/** + * @test FLUSHING + onPlaybackStarted → stays FLUSHING, pre-flush state + * updated to PLAYING (edge-case race handler). + * + * When a Rialto PLAYING notification arrives during the narrow window + * between flushSource() sending the IPC command and onFlushComplete() + * being called, the machine stays in FLUSHING (WaitForFlushToComplete + * must not unblock prematurely) and updates the pre-flush state so that + * onFlushComplete() restores to PLAYING. + */ +TEST_F(AampPlayerStateMachineTest, OnPlaybackStarted_FromFlushing_EdgeCaseRace_StaysFlushing) +{ + m_sm.onPipelineLoaded(); + m_sm.onSourceAttaching(); + m_sm.onAllSourcesAttached(); + m_sm.onPlaybackStarted(); + m_sm.onFlush(); + ASSERT_EQ(m_sm.currentState(), PlayerStateId::FLUSHING); + + m_sm.onPlaybackStarted(); // delayed ack: state stays FLUSHING + EXPECT_EQ(m_sm.currentState(), PlayerStateId::FLUSHING); + + m_sm.onFlushComplete(); // sources flushed: restores to PLAYING + EXPECT_EQ(m_sm.currentState(), PlayerStateId::PLAYING); +} + +/** + * @test FLUSHING + onPlaybackPaused → PAUSED (edge-case race handler). + */ +TEST_F(AampPlayerStateMachineTest, OnPlaybackPaused_FromFlushing_EdgeCaseRace_TransitionsToPaused) +{ + // This test is subsumed by + // OnPlaybackPaused_FromFlushing_UpdatesPreFlushStateAndStaysFlushing. + // Kept here to verify that successive flush+PlaybackPaused sequences + // are handled correctly when pre-flush state starts as PLAYING. + m_sm.onPipelineLoaded(); + m_sm.onSourceAttaching(); + m_sm.onAllSourcesAttached(); + m_sm.onPlaybackStarted(); // pre-flush state will be PLAYING + m_sm.onFlush(); + ASSERT_EQ(m_sm.currentState(), PlayerStateId::FLUSHING); + + m_sm.onPlaybackPaused(); // overrides pre-flush state to PAUSED + EXPECT_EQ(m_sm.currentState(), PlayerStateId::FLUSHING); + + m_sm.onFlushComplete(); // restores to PAUSED (updated pre-flush state) + EXPECT_EQ(m_sm.currentState(), PlayerStateId::PAUSED); +} diff --git a/test/utests/tests/AampRialtoPlayerTests/AampRialtoPlayerTestCases.cpp b/test/utests/tests/AampRialtoPlayerTests/AampRialtoPlayerTestCases.cpp index 6efc42841f..76bb2c620e 100644 --- a/test/utests/tests/AampRialtoPlayerTests/AampRialtoPlayerTestCases.cpp +++ b/test/utests/tests/AampRialtoPlayerTests/AampRialtoPlayerTestCases.cpp @@ -2877,13 +2877,17 @@ TEST_F(AampRialtoPlayerWithDemuxTest, /** * @brief Regression: Bug A from L2 TESTDATA0 rewind failure. * - * The sequence Flush(shouldTearDown=false) → Configure(audio=INVALID) - * previously left the state machine stuck in FLUSHING, unable to - * respond to Rialto's playback state callbacks. + * The sequence Flush(shouldTearDown=false) → OnSourceFlushed (all) → + * Configure(audio=INVALID) → Rialto PLAYING previously left the state + * machine stuck in FLUSHING when the Configure() did not rebuild the + * pipeline. * - * Fix: FlushingState now handles onPlaybackStarted() and - * onPlaybackPaused(), so it can transition to PLAYING or PAUSED - * when Rialto sends those notifications. + * Fix: onFlushComplete() is now called from OnSourceFlushed() once all + * sources confirm flushed. It restores the pre-flush state + * (SOURCES_ATTACHED in this case) so the pipeline reuse path in + * Configure() finds a non-FLUSHING state and, crucially, the subsequent + * Rialto PLAYING notification transitions SOURCES_ATTACHED → PLAYING via + * the normal SourcesAttachedState::onPlaybackStarted() path. */ EXPECT_CALL(*m_mockFactory, createMediaPipeline(_, _)).Times(1); Configure(FORMAT_ISO_BMFF, FORMAT_ISO_BMFF); @@ -2906,29 +2910,36 @@ TEST_F(AampRialtoPlayerWithDemuxTest, << "Precondition: Flush(shouldTearDown=false) must move state to FLUSHING"; // Simulate Rialto confirming the flush for all attached sources. - // OnSourceFlushed commits m_pendingFlushRate → m_rate and unblocks - // WaitForFlushToComplete() so the subsequent Configure() can proceed. + // The last OnSourceFlushed call triggers onFlushComplete(), which + // restores the pre-flush state (SOURCES_ATTACHED) and notifies + // WaitForFlushToComplete() so Configure() can proceed. EXPECT_CALL(*m_mockPipelinePtr, setSourcePosition(_, _, /*resetTime=*/true, _, _)) .Times(3) .WillRepeatedly(Return(true)); PostSourceFlushed(/*sourceId=*/0); // video PostSourceFlushed(/*sourceId=*/1); // inband CC subtitle - PostSourceFlushed(/*sourceId=*/2); // audio + PostSourceFlushed(/*sourceId=*/2); // audio — triggers onFlushComplete() + + // After all sources have flushed, onFlushComplete() restores the + // pre-flush state. The pre-flush state was SOURCES_ATTACHED. + EXPECT_EQ(m_player->GetCurrentPlayerState(), PlayerStateId::SOURCES_ATTACHED) + << "onFlushComplete() must restore pre-flush state SOURCES_ATTACHED"; // Trickplay Configure: audio → FORMAT_INVALID, no pipeline recreation. + // The pipeline reuse path does not touch the state machine, so state + // remains SOURCES_ATTACHED. m_player->Configure(FORMAT_ISO_BMFF, FORMAT_INVALID, FORMAT_INVALID, /*bESChangeStatus=*/false, /*setReadyAfterPipelineCreation=*/false); - // State machine stays in FLUSHING (no pipeline recreation, no state change). - EXPECT_EQ(m_player->GetCurrentPlayerState(), PlayerStateId::FLUSHING) - << "State machine stays in FLUSHING when Configure() doesn't recreate pipeline"; + EXPECT_EQ(m_player->GetCurrentPlayerState(), PlayerStateId::SOURCES_ATTACHED) + << "Pipeline reuse must not change the state from SOURCES_ATTACHED"; - // Verify the FIX: FlushingState responds to Rialto PLAYING callback. + // Verify the normal transition: SOURCES_ATTACHED responds to Rialto PLAYING. PostPlaybackState(firebolt::rialto::PlaybackState::PLAYING); EXPECT_EQ(m_player->GetCurrentPlayerState(), PlayerStateId::PLAYING) - << "FlushingState must transition to PLAYING when Rialto sends PLAYING"; + << "SOURCES_ATTACHED must transition to PLAYING when Rialto sends PLAYING"; } TEST_F(AampRialtoPlayerTest, @@ -3139,43 +3150,50 @@ static void SetupCapabilities( bool videoMaster); TEST_F(AampRialtoPlayerTest, - Flush_AlreadyFlushing_SkipsSecondPipelineFlushButUpdatesRateAndPosition) + Flush_SequentialFlushes_SecondFlushIssuesNewPipelineFlushWithNewPositionAndRate) { /** - * @brief A second Flush() while already in FLUSHING should not re-issue - * pipeline flush IPC, but must still update staged position/rate. + * @brief Flush() calls WaitForFlushToComplete() at entry, so a second + * Flush() issued after all sources confirm flushed from the first + * cycle issues a fresh pipeline flush IPC with its own position + * and rate. + * + * This replaces the old re-entrant-flush test. With the blocking design + * there is no re-entrant path: the second Flush() simply waits for the + * first cycle to finish, then starts its own. */ Configure(FORMAT_ISO_BMFF, FORMAT_INVALID); m_player->SetStreamCaps(eMEDIATYPE_VIDEO, MakeVideoH264CodecInfo()); - // First flush transitions SOURCES_ATTACHED -> FLUSHING. - // Configure(FORMAT_ISO_BMFF, FORMAT_INVALID) attaches two sources: - // video (sourceId=0) and inband CC subtitle (sourceId=1). Each - // attached source issues one pipeline flush IPC on the first Flush(). + // First flush: two sources (video=0, subtitle=1). EXPECT_CALL(*m_mockPipelinePtr, flush(_, _, _)).Times(2); m_player->Flush(/*position=*/10.0, /*rate=*/2, /*shouldTearDown=*/false); + ASSERT_EQ(m_player->GetCurrentPlayerState(), PlayerStateId::FLUSHING); - ASSERT_EQ(m_player->GetCurrentPlayerState(), PlayerStateId::FLUSHING) - << "Precondition: first Flush() must put player into FLUSHING"; + // Complete first flush cycle: both sources report flushed. + ON_CALL(*m_mockPipelinePtr, setSourcePosition(_, _, _, _, _)) + .WillByDefault(Return(true)); + PostSourceFlushed(/*sourceId=*/0); + PostSourceFlushed(/*sourceId=*/1); // onFlushComplete() restores pre-flush state - // Re-entrant flush while FLUSHING: no second pipeline flush command. - m_player->Flush(/*position=*/33.0, /*rate=*/-4, /*shouldTearDown=*/false); + ASSERT_NE(m_player->GetCurrentPlayerState(), PlayerStateId::FLUSHING) + << "First flush cycle must be complete before second Flush() call"; - EXPECT_EQ(m_player->GetCurrentPlayerState(), PlayerStateId::FLUSHING) - << "Re-entrant Flush() must keep player in FLUSHING"; + // Second flush: fresh IPC to all attached sources with new position. + EXPECT_CALL(*m_mockPipelinePtr, flush(_, _, _)).Times(2); + m_player->Flush(/*position=*/33.0, /*rate=*/-4, /*shouldTearDown=*/false); + ASSERT_EQ(m_player->GetCurrentPlayerState(), PlayerStateId::FLUSHING); - // Both sources (video=0, subtitle=1) report flushed; setSourcePosition - // is called for each with the position from the second (pending) flush. - // The rate is committed only after all sources have reported flushed. + // Complete second flush cycle; setSourcePosition must use the new position. EXPECT_CALL(*m_mockPipelinePtr, setSourcePosition(_, testing::Ge(33'000'000'000LL), /*resetTime=*/true, _, _)) .Times(2) .WillRepeatedly(Return(true)); PostSourceFlushed(/*sourceId=*/0); - PostSourceFlushed(/*sourceId=*/1); // inband CC subtitle source + PostSourceFlushed(/*sourceId=*/1); - // GetPositionMilliseconds() must use latest rate from second flush. + // GetPositionMilliseconds() must use rate from the second flush (-4). ON_CALL(*m_mockSources[eMEDIATYPE_VIDEO], firstPtsMs()) .WillByDefault(Return(0LL)); EXPECT_CALL(*m_mockPipelinePtr, getPosition(_)) From 74158c40153566a4f1e57e6a768fa72fd4d34fa7 Mon Sep 17 00:00:00 2001 From: anshephe <115161257+anshephe@users.noreply.github.com> Date: Mon, 6 Jul 2026 15:16:33 +0000 Subject: [PATCH 02/19] Further changes to state machine --- direct-rialto/AampPlayerStateMachine.cpp | 137 ++++++++++++------ direct-rialto/AampPlayerStateMachine.h | 21 +-- direct-rialto/AampRialtoPlayer.cpp | 127 +++++++++++----- direct-rialto/AampRialtoPlayer.h | 13 +- docs/rialto-integration/draw_state_machine.py | 46 +++--- .../fakes/FakeAampPlayerStateMachine.cpp | 15 +- .../AampPlayerStateMachineTestCases.cpp | 112 +++++++++----- .../AampRialtoPlayerTestCases.cpp | 15 +- 8 files changed, 317 insertions(+), 169 deletions(-) diff --git a/direct-rialto/AampPlayerStateMachine.cpp b/direct-rialto/AampPlayerStateMachine.cpp index 3a46e4a029..bdd6a59362 100644 --- a/direct-rialto/AampPlayerStateMachine.cpp +++ b/direct-rialto/AampPlayerStateMachine.cpp @@ -41,7 +41,8 @@ class SourcesAttachedState; class PlayingState; class PausedState; class FlushingState; -class StoppedState; +class StoppedState; // forward decl retained; Stop() calls WaitForFlushToComplete + // so onStop() should never be dispatched from FLUSHING class ErrorState; // ============================================================================ @@ -55,6 +56,7 @@ class IdleState final : public IPlayerState const char *name() const override { return "IDLE"; } std::unique_ptr onPipelineLoaded() override; + std::unique_ptr onReconfigure() override; // first-tune reset }; class PipelineCreatedState final : public IPlayerState @@ -64,6 +66,9 @@ class PipelineCreatedState final : public IPlayerState const char *name() const override { return "PIPELINE_CREATED"; } std::unique_ptr onSourceAttaching() override; + std::unique_ptr onStop() override; + std::unique_ptr onError() override; + std::unique_ptr onReconfigure() override; }; class SourcesAttachingState final : public IPlayerState @@ -73,6 +78,9 @@ class SourcesAttachingState final : public IPlayerState const char *name() const override { return "SOURCES_ATTACHING"; } std::unique_ptr onAllSourcesAttached() override; + std::unique_ptr onStop() override; + std::unique_ptr onError() override; + std::unique_ptr onReconfigure() override; }; class SourcesAttachedState final : public IPlayerState @@ -83,6 +91,9 @@ class SourcesAttachedState final : public IPlayerState std::unique_ptr onPlaybackStarted() override; std::unique_ptr onFlush() override; + std::unique_ptr onStop() override; + std::unique_ptr onError() override; + std::unique_ptr onReconfigure() override; }; class PlayingState final : public IPlayerState @@ -93,8 +104,12 @@ class PlayingState final : public IPlayerState std::unique_ptr onPlaybackPaused() override; std::unique_ptr onFlush() override; - // Tolerate a duplicate PLAYING notification without transitioning. - std::unique_ptr onPlaybackStarted() override { return nullptr; } + std::unique_ptr onStop() override; + std::unique_ptr onError() override; + std::unique_ptr onReconfigure() override; + // Duplicate onPlaybackStarted while already PLAYING is handled at the + // PlayerStateMachine level (see onPlaybackStarted() below) so dispatch() + // never sees an unexpected null for this event. }; class PausedState final : public IPlayerState @@ -105,6 +120,9 @@ class PausedState final : public IPlayerState std::unique_ptr onPlaybackStarted() override; std::unique_ptr onFlush() override; + std::unique_ptr onStop() override; + std::unique_ptr onError() override; + std::unique_ptr onReconfigure() override; }; class FlushingState final : public IPlayerState @@ -113,8 +131,7 @@ class FlushingState final : public IPlayerState PlayerStateId id() const override { return PlayerStateId::FLUSHING; } const char *name() const override { return "FLUSHING"; } - /// After a flush new init fragments arrive, restarting source attachment. - std::unique_ptr onSourceAttaching() override; + std::unique_ptr onError() override; // onPlaybackStarted() and onPlaybackPaused() are intentionally NOT // overridden here. When Rialto sends PLAYING or PAUSED during a flush @@ -124,14 +141,23 @@ class FlushingState final : public IPlayerState // machine stays in FLUSHING until onFlushComplete() fires — keeping // WaitForFlushToComplete() correctly blocked until all sources confirm // flushed. See PlayerStateMachine::onPlaybackStarted/Paused() below. + // + // onStop() is not overridden: Stop() calls WaitForFlushToComplete() + // before dispatching onStop(), so FLUSHING is never the current state + // when onStop is dispatched. + // + // onReconfigure() is not overridden: Configure() calls Stop() first, + // which waits for flush to complete before onReconfigure() is fired. }; +// StoppedState is removed; onStop() now transitions directly to IDLE. +// The forward decl above is kept so the compiler does not complain if any +// stale reference exists during incremental builds. class StoppedState final : public IPlayerState { public: PlayerStateId id() const override { return PlayerStateId::STOPPED; } const char *name() const override { return "STOPPED"; } - // Re-configure (re-tune) resets to IDLE; handled by base onReconfigure(). }; class ErrorState final : public IPlayerState @@ -139,27 +165,13 @@ class ErrorState final : public IPlayerState public: PlayerStateId id() const override { return PlayerStateId::ERROR; } const char *name() const override { return "ERROR"; } - // Re-configure after error resets to IDLE; handled by base onReconfigure(). -}; - -// ============================================================================ -// IPlayerState base implementations for cross-state events -// ============================================================================ - -std::unique_ptr IPlayerState::onStop() -{ - return std::make_unique(); -} - -std::unique_ptr IPlayerState::onError() -{ - return std::make_unique(); -} -std::unique_ptr IPlayerState::onReconfigure() -{ - return std::make_unique(); -} + std::unique_ptr onStop() override; + std::unique_ptr onReconfigure() override; + // onError() is not overridden: already in ERROR; a second fatal + // notification would produce a no-op (dispatch warns) rather than + // a spurious self-transition. +}; // ============================================================================ // Concrete state transition bodies @@ -210,10 +222,34 @@ std::unique_ptr PausedState::onFlush() return std::make_unique(); } -std::unique_ptr FlushingState::onSourceAttaching() -{ - return std::make_unique(); -} +// onStop — valid from any active state (except FLUSHING; Stop waits for +// flush before dispatching, and IDLE has nothing to stop). +std::unique_ptr PipelineCreatedState::onStop() { return std::make_unique(); } +std::unique_ptr SourcesAttachingState::onStop() { return std::make_unique(); } +std::unique_ptr SourcesAttachedState::onStop() { return std::make_unique(); } +std::unique_ptr PlayingState::onStop() { return std::make_unique(); } +std::unique_ptr PausedState::onStop() { return std::make_unique(); } +std::unique_ptr ErrorState::onStop() { return std::make_unique(); } + +// onError — valid from any state in which a Rialto FAILURE notification +// can arrive (FLUSHING included; IDLE and ERROR itself are excluded). +std::unique_ptr PipelineCreatedState::onError() { return std::make_unique(); } +std::unique_ptr SourcesAttachingState::onError() { return std::make_unique(); } +std::unique_ptr SourcesAttachedState::onError() { return std::make_unique(); } +std::unique_ptr PlayingState::onError() { return std::make_unique(); } +std::unique_ptr PausedState::onError() { return std::make_unique(); } +std::unique_ptr FlushingState::onError() { return std::make_unique(); } + +// onReconfigure — Configure() fires this on every tune, including first tune +// from IDLE, retune from any active state, and recovery after ERROR. +// Not valid from FLUSHING (Configure calls Stop first, which waits for flush). +std::unique_ptr IdleState::onReconfigure() { return std::make_unique(); } +std::unique_ptr PipelineCreatedState::onReconfigure() { return std::make_unique(); } +std::unique_ptr SourcesAttachingState::onReconfigure() { return std::make_unique(); } +std::unique_ptr SourcesAttachedState::onReconfigure() { return std::make_unique(); } +std::unique_ptr PlayingState::onReconfigure() { return std::make_unique(); } +std::unique_ptr PausedState::onReconfigure() { return std::make_unique(); } +std::unique_ptr ErrorState::onReconfigure() { return std::make_unique(); } // ============================================================================ // PlayerStateMachine @@ -239,7 +275,8 @@ const char *PlayerStateMachine::currentStateName() const } void PlayerStateMachine::dispatch( - std::unique_ptr (IPlayerState::*handler)()) + std::unique_ptr (IPlayerState::*handler)(), + const char *eventName) { std::lock_guard lock(m_mutex); auto next = (m_state.get()->*handler)(); @@ -249,33 +286,36 @@ void PlayerStateMachine::dispatch( m_state->name(), next->name()); m_state = std::move(next); } + else + { + AAMPLOG_WARN("PlayerState: event '%s' has no transition from state '%s' - ignored", + eventName, m_state->name()); + } } void PlayerStateMachine::onPipelineLoaded() { - dispatch(&IPlayerState::onPipelineLoaded); + dispatch(&IPlayerState::onPipelineLoaded, "onPipelineLoaded"); } void PlayerStateMachine::onSourceAttaching() { - dispatch(&IPlayerState::onSourceAttaching); + dispatch(&IPlayerState::onSourceAttaching, "onSourceAttaching"); } void PlayerStateMachine::onAllSourcesAttached() { - dispatch(&IPlayerState::onAllSourcesAttached); + dispatch(&IPlayerState::onAllSourcesAttached, "onAllSourcesAttached"); } void PlayerStateMachine::onPlaybackStarted() { { - // Edge-case race: a delayed PLAYING ack for a play() that was in-flight - // when Flush() started. Update m_preFlushStateId so onFlushComplete() - // will restore to PLAYING, but keep the machine in FLUSHING — this is - // the key advantage over immediately transitioning: WaitForFlushToComplete() - // (predicate: state != FLUSHING) stays blocked until all sources confirm - // flushed, preventing Configure() or a new Flush() from racing with - // in-flight OnSourceFlushed callbacks. + // Intercept two cases before calling dispatch() so it never receives + // an expected null return: + // + // 1. FLUSHING: delayed PLAYING ack — update pre-flush state only. + // 2. PLAYING: duplicate PLAYING notification — silently tolerated. std::lock_guard lock(m_mutex); if (m_state->id() == PlayerStateId::FLUSHING) { @@ -285,8 +325,13 @@ void PlayerStateMachine::onPlaybackStarted() m_preFlushStateId = PlayerStateId::PLAYING; return; } + if (m_state->id() == PlayerStateId::PLAYING) + { + AAMPLOG_INFO("PlayerState: duplicate onPlaybackStarted in PLAYING - ignored"); + return; + } } - dispatch(&IPlayerState::onPlaybackStarted); + dispatch(&IPlayerState::onPlaybackStarted, "onPlaybackStarted"); } void PlayerStateMachine::onPlaybackPaused() @@ -303,7 +348,7 @@ void PlayerStateMachine::onPlaybackPaused() return; } } - dispatch(&IPlayerState::onPlaybackPaused); + dispatch(&IPlayerState::onPlaybackPaused, "onPlaybackPaused"); } void PlayerStateMachine::onFlush() @@ -366,15 +411,15 @@ void PlayerStateMachine::onFlushComplete() void PlayerStateMachine::onStop() { - dispatch(&IPlayerState::onStop); + dispatch(&IPlayerState::onStop, "onStop"); } void PlayerStateMachine::onError() { - dispatch(&IPlayerState::onError); + dispatch(&IPlayerState::onError, "onError"); } void PlayerStateMachine::onReconfigure() { - dispatch(&IPlayerState::onReconfigure); + dispatch(&IPlayerState::onReconfigure, "onReconfigure"); } diff --git a/direct-rialto/AampPlayerStateMachine.h b/direct-rialto/AampPlayerStateMachine.h index aa5959dcca..e66de3d875 100644 --- a/direct-rialto/AampPlayerStateMachine.h +++ b/direct-rialto/AampPlayerStateMachine.h @@ -121,17 +121,14 @@ class IPlayerState /// onFlushComplete) cannot cause a double-transition. virtual std::unique_ptr onFlushComplete() { return nullptr; } - /// Valid from any state — transitions to STOPPED. - /// Non-inline: body defined in .cpp after StoppedState is complete. - virtual std::unique_ptr onStop(); + /// Override in concrete states where Stop is a valid transition. + virtual std::unique_ptr onStop() { return nullptr; } - /// Valid from any state — transitions to ERROR. - /// Non-inline: body defined in .cpp after ErrorState is complete. - virtual std::unique_ptr onError(); + /// Override in concrete states where a fatal Error is a valid transition. + virtual std::unique_ptr onError() { return nullptr; } - /// Valid from any state — transitions back to IDLE for a re-tune. - /// Non-inline: body defined in .cpp after IdleState is complete. - virtual std::unique_ptr onReconfigure(); + /// Override in concrete states where a re-tune (Reconfigure) is valid. + virtual std::unique_ptr onReconfigure() { return nullptr; } }; // --------------------------------------------------------------------------- @@ -210,7 +207,11 @@ class PlayerStateMachine private: /// Execute a handler, apply the returned transition, and log MIL. - void dispatch(std::unique_ptr (IPlayerState::*handler)()); + /// Logs a WARN when the handler returns nullptr (no transition defined + /// for this event from the current state) so unexpected events are visible + /// in production logs. + void dispatch(std::unique_ptr (IPlayerState::*handler)(), + const char *eventName); mutable std::mutex m_mutex; std::unique_ptr m_state; diff --git a/direct-rialto/AampRialtoPlayer.cpp b/direct-rialto/AampRialtoPlayer.cpp index 381e7bbba9..4560981a27 100644 --- a/direct-rialto/AampRialtoPlayer.cpp +++ b/direct-rialto/AampRialtoPlayer.cpp @@ -378,10 +378,10 @@ bool AampRialtoPlayer::ShouldRecreatePipeline( void AampRialtoPlayer::WaitForFlushToComplete() { std::unique_lock lock(m_flushMutex); - // Block until the state machine leaves FLUSHING. onFlushComplete() (called - // from OnSourceFlushed when all sources confirm flushed) transitions the - // state and then notifies m_flushCv, so this predicate will always become - // true once the flush cycle ends. + // Block until the state machine leaves FLUSHING. Holds m_flushMutex + // while the predicate is evaluated so that the optional claim below is + // atomic with the check: no concurrent caller can pass the predicate + // and also claim FLUSHING before us. m_flushCv.wait(lock, [this]() { return m_stateMachine.currentState() != PlayerStateId::FLUSHING; @@ -484,7 +484,7 @@ void AampRialtoPlayer::Configure( // normal playback flow; clearing the stored protection params here would // discard them before AttachVideoSource / AttachAudioSource can use them // to call createSession(). ClearProtectionEvent() handles teardown. - // NOTE: m_pendingFlushPositionNs is intentionally NOT reset here. + // NOTE: m_pendingPositionNs is intentionally NOT reset here. // Flush() may be called before Configure() to pre-stage the seek position; // clearing it here would discard that staged value before sources attach. m_playRequested.store(false, std::memory_order_relaxed); @@ -861,7 +861,7 @@ void AampRialtoPlayer::AttachSource( auto result = source.attachOrUpdate( *m_pipeline, codecInfo, m_drmBridge.get(), - m_pendingFlushPositionNs.load(std::memory_order_relaxed), + m_pendingPositionNs.load(std::memory_order_relaxed), m_pendingProtection[static_cast(type)], computeAppliedRate( m_pendingFlushRate.load(std::memory_order_relaxed))); @@ -1053,10 +1053,8 @@ void AampRialtoPlayer::Stream() // If a flush is in progress, play() will be issued by OnSourceFlushed() // once all sources confirm flushed and onFlushComplete() restores state. - // Checking state is safe because FLUSHING is only set by Flush() (AAMP - // thread) and only cleared by onFlushComplete() or the edge-case - // onPlaybackStarted/Paused path (both protected by the state machine - // mutex). + // FLUSHING is set inside m_flushMutex in Flush() and cleared by + // onFlushComplete() (both protected by the state machine mutex). if (m_stateMachine.currentState() == PlayerStateId::FLUSHING) { AAMPLOG_INFO("deferring play() - flush in progress"); @@ -1089,6 +1087,14 @@ void AampRialtoPlayer::Stream() void AampRialtoPlayer::Stop(bool keepLastFrame) { AAMPLOG_INFO("ENTRY keepLastFrame=%d", keepLastFrame); + + // Wait for any in-progress flush cycle to complete before stopping. + // This ensures the state machine has already exited FLUSHING so that + // onStop() transitions from a well-defined state (PLAYING, PAUSED, + // SOURCES_ATTACHED, etc.) and the FLUSHING → STOPPED arc is never + // reached. + WaitForFlushToComplete(); + StopProgressTimer(); if (m_monitorAV) { @@ -1123,41 +1129,86 @@ void AampRialtoPlayer::Flush(double position, int rate, bool shouldTearDown) { AAMPLOG_INFO("ENTRY position=%f rate=%d shouldTearDown=%d", position, rate, shouldTearDown); - // If a previous flush is still in progress, block until it completes - // before starting a new one. This keeps the flush cycle atomic: each - // Flush() call owns a single FLUSHING → cycle and - // removes the complexity of updating parameters mid-flush. - // Configure() follows the same pattern. + // Step 1: Wait for any previous flush cycle to complete (no claim yet). + // The teardown check below reads the current state AFTER any in-flight + // flush has restored it (e.g. a seek from PLAYING restores to PLAYING). WaitForFlushToComplete(); - const int64_t posNs = static_cast(position * kNsPerSecond); - m_pendingFlushPositionNs.store(posNs, std::memory_order_relaxed); - m_pendingFlushRate.store(rate, std::memory_order_relaxed); - - // shouldTearDown controls recovery behavior when NOT in - // PLAYING/PAUSED/SOURCES_ATTACHED states. - // - PLAYING/PAUSED/SOURCES_ATTACHED: Always proceed with flush (shouldTearDown ignored) - // - Other states + shouldTearDown=true: Call Stop() for recovery/cleanup - // - Other states + shouldTearDown=false: Proceed with flush normally - // - // This differs from GStreamer which skips flush in non-PLAYING/PAUSED states. - // Rialto needs to flush in states like SOURCES_ATTACHED to set up positions. - // WaitForFlushToComplete() above guarantees we are never in FLUSHING here. - const PlayerStateId state = m_stateMachine.currentState(); - const bool isPlayingPausedOrAttached = (state == PlayerStateId::PLAYING || - state == PlayerStateId::SOURCES_ATTACHED || - state == PlayerStateId::PAUSED); + // Step 2: Decide whether to tear down or flush based on current state. + // This check happens BEFORE claiming FLUSHING so that Stop() — which + // also calls WaitForFlushToComplete() at its start — does not deadlock + // on a FLUSHING state we have already claimed. + const PlayerStateId preFlushState = m_stateMachine.currentState(); + const bool isPlayingPausedOrAttached = + (preFlushState == PlayerStateId::PLAYING || + preFlushState == PlayerStateId::SOURCES_ATTACHED || + preFlushState == PlayerStateId::PAUSED); if (!isPlayingPausedOrAttached && shouldTearDown) { - // Not in PLAYING/PAUSED/SOURCES_ATTACHED and shouldTearDown=true -> tear down for recovery. - AAMPLOG_WARN("Player state %s is not PLAYING/PAUSED/SOURCES_ATTACHED and shouldTearDown=true - calling Stop(true)", - m_stateMachine.currentStateName()); + // Player is not in a flushable state; recover by stopping. + // Stop() will call WaitForFlushToComplete() which returns immediately + // since we have not claimed FLUSHING. + AAMPLOG_WARN("Player was not in PLAYING/PAUSED/SOURCES_ATTACHED " + "(pre-flush state=%d) and shouldTearDown=true - calling Stop(true)", + static_cast(preFlushState)); Stop(true); AAMPLOG_INFO("EXIT - teardown requested"); return; } - + + if (!isPlayingPausedOrAttached) + { + // Not in a flushable state and no teardown requested. + // Stage the parameters (covers the pre-Configure() seek-position + // pre-staging path) and commit the rate, but do not attempt a full + // flush cycle — onFlush() would be a no-op on the state machine. + // + // Still wake any blocked inject threads (sources may exist but not + // yet be Rialto-attached in the deferred-attachment path) and, on + // trickplay exit (rate == 1), clear any audio EOS set during trickplay + // entry so injection can resume once the pipeline is ready. + const int64_t posNs = static_cast(position * kNsPerSecond); + m_pendingPositionNs.store(posNs, std::memory_order_relaxed); + m_pendingFlushRate.store(rate, std::memory_order_relaxed); + m_rate.store(rate, std::memory_order_relaxed); + + for (auto &source : m_sources) + { + if (source) + { + source->invalidateGeneration(); + if (rate == AAMP_NORMAL_PLAY_RATE) + { + auto &st = source->state(); + std::lock_guard lock(st.mu); + st.eos = false; + } + } + } + + AAMPLOG_INFO("Flush() in non-flushable state %d (shouldTearDown=false): " + "parameters staged, rate=%d committed, no flush cycle started", + static_cast(preFlushState), rate); + AAMPLOG_INFO("EXIT"); + return; + } + + // Step 3: Claim FLUSHING atomically while holding m_flushMutex so that + // any concurrent WaitForFlushToComplete() caller (e.g. Configure()) sees + // FLUSHING and blocks until onFlushComplete() signals completion. + // Flush() and Stop()/Configure() are always on the AAMP control thread, + // so no TOCTOU risk exists between step 2 and step 3. + { + std::lock_guard lock(m_flushMutex); + m_stateMachine.onFlush(); + } + + // Stage flush parameters now that FLUSHING is claimed. + const int64_t posNs = static_cast(position * kNsPerSecond); + m_pendingPositionNs.store(posNs, std::memory_order_relaxed); + m_pendingFlushRate.store(rate, std::memory_order_relaxed); + // Wake any in-flight data so it abandons the current batch. for (auto &source : m_sources) { @@ -1217,8 +1268,6 @@ void AampRialtoPlayer::Flush(double position, int rate, bool shouldTearDown) // invalidateGeneration() (called above), so the next injection into // the video source establishes the fresh segment-start baseline. - m_stateMachine.onFlush(); - AAMPLOG_INFO("EXIT"); } @@ -1897,7 +1946,7 @@ void AampRialtoPlayer::OnSourceFlushed(int32_t sourceId) // the SEGMENT event - leaving the pipeline's EOS state intact // and causing an immediate END_OF_STREAM on the next play(). const int64_t posNs = - m_pendingFlushPositionNs.load(std::memory_order_relaxed); + m_pendingPositionNs.load(std::memory_order_relaxed); const int pendingRate = m_pendingFlushRate.load(std::memory_order_relaxed); if (m_pipeline && diff --git a/direct-rialto/AampRialtoPlayer.h b/direct-rialto/AampRialtoPlayer.h index ae5e98e652..6ff0189b3b 100644 --- a/direct-rialto/AampRialtoPlayer.h +++ b/direct-rialto/AampRialtoPlayer.h @@ -432,12 +432,23 @@ class AampRialtoPlayer : public StreamSink, public IDirectRialtoCC * m_rate has been committed from m_pendingFlushRate before * Configure() evaluates ShouldRecreatePipeline(). */ + /** + * @brief Block until any in-progress flush cycle completes. + * + * Acquires m_flushMutex and waits on m_flushCv until the state machine + * is no longer FLUSHING. + * + * Used by Configure(), Stop(), and (via a separate claim step) Flush(). + * The claim-FLUSHING step in Flush() is done in a separate locked section + * after the teardown check so that Stop() — which also calls this — never + * deadlocks on a FLUSHING state claimed by the same Flush() call. + */ void WaitForFlushToComplete(); /// Position (ns) stored by Flush(); used to set the initial GStreamer /// segment via setSourcePosition() once each source is attached. /// -1 means no flush position has been set yet. - std::atomic m_pendingFlushPositionNs{-1}; + std::atomic m_pendingPositionNs{-1}; /// Set by Stream(); cleared once play() is issued. Lets us defer the /// play() call until after allSourcesAttached() so the Rialto server diff --git a/docs/rialto-integration/draw_state_machine.py b/docs/rialto-integration/draw_state_machine.py index 929238169a..7e479355d2 100755 --- a/docs/rialto-integration/draw_state_machine.py +++ b/docs/rialto-integration/draw_state_machine.py @@ -68,7 +68,6 @@ def _require_graphviz(): ("PLAYING", "Server confirmed PLAYING"), ("PAUSED", "Server confirmed PAUSED"), ("FLUSHING", "Flush() in progress; awaiting new segments"), - ("STOPPED", "Stop() was called"), ("ERROR", "Server reported a fatal error"), ] @@ -94,7 +93,10 @@ def _require_graphviz(): ("FLUSHING", "onFlushComplete [pre=SOURCES_ATTACHED]","", "SOURCES_ATTACHED"), # After flush + re-configure, new init fragments re-drive attachment. - ("FLUSHING", "onSourceAttaching", "attachSource()", "SOURCES_ATTACHING"), + # Note: there is NO direct FLUSHING→SOURCES_ATTACHING transition via + # onSourceAttaching. A pipeline rebuild always goes through onReconfigure + # (FLUSHING→IDLE) before sources re-attach. onSourceAttaching from + # FLUSHING is unreachable and will produce a WARN log if it ever fires. # Note: onPlaybackStarted/Paused during FLUSHING (delayed ack for a # play() that was in-flight when Flush() started) update m_preFlushStateId @@ -102,13 +104,15 @@ def _require_graphviz(): # onFlushComplete() fires, keeping WaitForFlushToComplete() correctly # blocked until all sources confirm flushed. - # ── Stop — valid from any non-terminal state ─────────────────────────── - ("PIPELINE_CREATED", "onStop", "stop()", "STOPPED"), - ("SOURCES_ATTACHING", "onStop", "stop()", "STOPPED"), - ("SOURCES_ATTACHED", "onStop", "stop()", "STOPPED"), - ("PLAYING", "onStop", "stop()", "STOPPED"), - ("PAUSED", "onStop", "stop()", "STOPPED"), - ("FLUSHING", "onStop", "stop()", "STOPPED"), + # ── Stop — valid from all active states except FLUSHING (Stop() waits for + # flush to complete before dispatching onStop, so FLUSHING is never the + # current state) and IDLE (nothing to stop). + ("PIPELINE_CREATED", "onStop", "stop()", "IDLE"), + ("SOURCES_ATTACHING", "onStop", "stop()", "IDLE"), + ("SOURCES_ATTACHED", "onStop", "stop()", "IDLE"), + ("PLAYING", "onStop", "stop()", "IDLE"), + ("PAUSED", "onStop", "stop()", "IDLE"), + ("ERROR", "onStop", "stop()", "IDLE"), # ── Error — valid from any non-terminal state ────────────────────────── ("PIPELINE_CREATED", "onError", "FAILURE notification", "ERROR"), @@ -118,15 +122,16 @@ def _require_graphviz(): ("PAUSED", "onError", "FAILURE notification", "ERROR"), ("FLUSHING", "onError", "FAILURE notification", "ERROR"), - # ── Reconfigure (re-tune) — valid from every state ──────────────────── + # ── Reconfigure (re-tune) — valid from IDLE (first tune) and all active + # states except FLUSHING (Configure() calls Stop() first, which waits for + # flush to complete, so FLUSHING is never current when onReconfigure fires). ("IDLE", "onReconfigure", "re-tune", "IDLE"), ("PIPELINE_CREATED", "onReconfigure", "re-tune", "IDLE"), ("SOURCES_ATTACHING", "onReconfigure", "re-tune", "IDLE"), ("SOURCES_ATTACHED", "onReconfigure", "re-tune", "IDLE"), ("PLAYING", "onReconfigure", "re-tune", "IDLE"), ("PAUSED", "onReconfigure", "re-tune", "IDLE"), - ("FLUSHING", "onReconfigure", "re-tune", "IDLE"), - ("STOPPED", "onReconfigure", "re-tune", "IDLE"), + ("ERROR", "onReconfigure", "re-tune", "IDLE"), ] @@ -141,7 +146,6 @@ def _require_graphviz(): "PLAYING": ("#F3E5F5", "#6A1B9A"), # purple "PAUSED": ("#FBE9E7", "#BF360C"), # deep-orange "FLUSHING": ("#EFEBE9", "#4E342E"), # brown - "STOPPED": ("#FAFAFA", "#616161"), # grey "ERROR": ("#FFEBEE", "#C62828"), # red } @@ -165,7 +169,6 @@ def _require_graphviz(): "PLAYING": "#F3E5F5", "PAUSED": "#FBE9E7", "FLUSHING": "#EFEBE9", - "STOPPED": "#FAFAFA", "ERROR": "#FFEBEE", } @@ -178,7 +181,7 @@ def _require_graphviz(): PUML_DEFAULT_EDGE_COLOUR = "#333333" # States that are rendered as end-states (double circle) in PlantUML. -TERMINAL_STATES = {"STOPPED", "ERROR"} +TERMINAL_STATES = {"ERROR"} def build_plantuml(show_reconfigure: bool = True) -> str: @@ -255,7 +258,7 @@ def build_graph(show_reconfigure: bool = True): # Nodes for state, tooltip in STATES: fill, border = STATE_COLOURS.get(state, ("#FFFFFF", "#333333")) - shape = "doublecircle" if state in ("STOPPED", "ERROR") else "box" + shape = "doublecircle" if state in ("ERROR",) else "box" style = "filled,rounded" dot.node( state, @@ -279,7 +282,12 @@ def build_graph(show_reconfigure: bool = True): continue colour = EDGE_COLOURS.get(event, DEFAULT_EDGE_COLOUR) label = f"{event}\\n[{action}]" if action else event - is_self = (src == dst) + is_self = (src == dst) + is_dashed = event in ("onStop", "onError", "onReconfigure") + # Cross-cutting edges (onStop/onError/onReconfigure) must not affect + # the rank computation or they pull ERROR/IDLE to extreme + # positions, causing long edges to be routed through unrelated nodes + # and misplacing their labels. dot.edge( src, dst, label=label, @@ -288,8 +296,8 @@ def build_graph(show_reconfigure: bool = True): fontname="Helvetica", fontsize="9", penwidth="1.5" if not is_self else "1.0", - style="dashed" if event in ("onStop", "onError", "onReconfigure") else "solid", - constraint="false" if is_self else "true", + style="dashed" if is_dashed else "solid", + constraint="false" if (is_self or is_dashed) else "true", ) return dot diff --git a/test/utests/fakes/FakeAampPlayerStateMachine.cpp b/test/utests/fakes/FakeAampPlayerStateMachine.cpp index c27e438453..748b68da0d 100644 --- a/test/utests/fakes/FakeAampPlayerStateMachine.cpp +++ b/test/utests/fakes/FakeAampPlayerStateMachine.cpp @@ -29,16 +29,6 @@ #include "AampPlayerStateMachine.h" -// --------------------------------------------------------------------------- -// IPlayerState — non-inline virtual methods declared in the header -// (The fake does not use IPlayerState objects, but the vtable symbols must -// be present to satisfy the linker.) -// --------------------------------------------------------------------------- - -std::unique_ptr IPlayerState::onStop() { return nullptr; } -std::unique_ptr IPlayerState::onError() { return nullptr; } -std::unique_ptr IPlayerState::onReconfigure() { return nullptr; } - // --------------------------------------------------------------------------- // PlayerStateMachine — enum-driven transition table // --------------------------------------------------------------------------- @@ -147,5 +137,8 @@ void PlayerStateMachine::onReconfigure() } void PlayerStateMachine::dispatch( - std::unique_ptr (IPlayerState::* /*handler*/)()) {} + std::unique_ptr (IPlayerState::*handler)(), + const char *eventName) +{ +} diff --git a/test/utests/tests/AampPlayerStateMachineTests/AampPlayerStateMachineTestCases.cpp b/test/utests/tests/AampPlayerStateMachineTests/AampPlayerStateMachineTestCases.cpp index 8be6428288..0dfda9a564 100644 --- a/test/utests/tests/AampPlayerStateMachineTests/AampPlayerStateMachineTestCases.cpp +++ b/test/utests/tests/AampPlayerStateMachineTests/AampPlayerStateMachineTestCases.cpp @@ -181,16 +181,20 @@ TEST_F(AampPlayerStateMachineTest, OnFlush_FromPaused_TransitionsToFlushing) } /** - * @test FLUSHING + onSourceAttaching → SOURCES_ATTACHING (re-attach after seek). + * @test onSourceAttaching from FLUSHING is ignored (no valid transition). + * + * There is no direct FLUSHING→SOURCES_ATTACHING path; a pipeline rebuild + * always goes through onReconfigure (→IDLE) first. The dispatch layer + * emits a WARN and leaves the machine in FLUSHING. */ -TEST_F(AampPlayerStateMachineTest, OnSourceAttaching_FromFlushing_TransitionsToSourcesAttaching) +TEST_F(AampPlayerStateMachineTest, OnSourceAttaching_FromFlushing_IsIgnored) { m_sm.onPipelineLoaded(); m_sm.onSourceAttaching(); m_sm.onAllSourcesAttached(); m_sm.onFlush(); - m_sm.onSourceAttaching(); - EXPECT_EQ(m_sm.currentState(), PlayerStateId::SOURCES_ATTACHING); + m_sm.onSourceAttaching(); // no transition defined — dispatch warns + EXPECT_EQ(m_sm.currentState(), PlayerStateId::FLUSHING); } /** @@ -239,47 +243,48 @@ TEST_F(AampPlayerStateMachineTest, } // =========================================================================== -// Cross-state events: onStop (valid from any non-terminal state) +// Cross-state events: onStop — Stop() waits for FLUSHING before dispatching +// so FLUSHING is never the source state in production; onStop() goes to IDLE. // =========================================================================== /** - * @test onStop from PIPELINE_CREATED → STOPPED. + * @test onStop from PIPELINE_CREATED → IDLE. */ -TEST_F(AampPlayerStateMachineTest, OnStop_FromPipelineCreated_TransitionsToStopped) +TEST_F(AampPlayerStateMachineTest, OnStop_FromPipelineCreated_TransitionsToIdle) { m_sm.onPipelineLoaded(); m_sm.onStop(); - EXPECT_EQ(m_sm.currentState(), PlayerStateId::STOPPED); + EXPECT_EQ(m_sm.currentState(), PlayerStateId::IDLE); } /** - * @test onStop from SOURCES_ATTACHING → STOPPED. + * @test onStop from SOURCES_ATTACHING → IDLE. */ -TEST_F(AampPlayerStateMachineTest, OnStop_FromSourcesAttaching_TransitionsToStopped) +TEST_F(AampPlayerStateMachineTest, OnStop_FromSourcesAttaching_TransitionsToIdle) { m_sm.onPipelineLoaded(); m_sm.onSourceAttaching(); m_sm.onStop(); - EXPECT_EQ(m_sm.currentState(), PlayerStateId::STOPPED); + EXPECT_EQ(m_sm.currentState(), PlayerStateId::IDLE); } /** - * @test onStop from PLAYING → STOPPED. + * @test onStop from PLAYING → IDLE. */ -TEST_F(AampPlayerStateMachineTest, OnStop_FromPlaying_TransitionsToStopped) +TEST_F(AampPlayerStateMachineTest, OnStop_FromPlaying_TransitionsToIdle) { m_sm.onPipelineLoaded(); m_sm.onSourceAttaching(); m_sm.onAllSourcesAttached(); m_sm.onPlaybackStarted(); m_sm.onStop(); - EXPECT_EQ(m_sm.currentState(), PlayerStateId::STOPPED); + EXPECT_EQ(m_sm.currentState(), PlayerStateId::IDLE); } /** - * @test onStop from PAUSED → STOPPED. + * @test onStop from PAUSED → IDLE. */ -TEST_F(AampPlayerStateMachineTest, OnStop_FromPaused_TransitionsToStopped) +TEST_F(AampPlayerStateMachineTest, OnStop_FromPaused_TransitionsToIdle) { m_sm.onPipelineLoaded(); m_sm.onSourceAttaching(); @@ -287,20 +292,25 @@ TEST_F(AampPlayerStateMachineTest, OnStop_FromPaused_TransitionsToStopped) m_sm.onPlaybackStarted(); m_sm.onPlaybackPaused(); m_sm.onStop(); - EXPECT_EQ(m_sm.currentState(), PlayerStateId::STOPPED); + EXPECT_EQ(m_sm.currentState(), PlayerStateId::IDLE); } /** - * @test onStop from FLUSHING → STOPPED. + * @test onStop from FLUSHING is ignored — FlushingState has no onStop override. + * + * Stop() always calls WaitForFlushToComplete() before dispatching onStop(), + * so FLUSHING is never the current state when onStop is dispatched in + * production. The dispatch layer emits a WARN and the machine stays in + * FLUSHING; it is not a valid no-arg transition here. */ -TEST_F(AampPlayerStateMachineTest, OnStop_FromFlushing_TransitionsToStopped) +TEST_F(AampPlayerStateMachineTest, OnStop_FromFlushing_IsIgnored) { m_sm.onPipelineLoaded(); m_sm.onSourceAttaching(); m_sm.onAllSourcesAttached(); m_sm.onFlush(); - m_sm.onStop(); - EXPECT_EQ(m_sm.currentState(), PlayerStateId::STOPPED); + m_sm.onStop(); // no transition defined — dispatch warns + EXPECT_EQ(m_sm.currentState(), PlayerStateId::FLUSHING); } // =========================================================================== @@ -346,6 +356,42 @@ TEST_F(AampPlayerStateMachineTest, OnError_FromPaused_TransitionsToError) EXPECT_EQ(m_sm.currentState(), PlayerStateId::ERROR); } +/** + * @test onError from PIPELINE_CREATED → ERROR. + */ +TEST_F(AampPlayerStateMachineTest, OnError_FromPipelineCreated_TransitionsToError) +{ + m_sm.onPipelineLoaded(); + m_sm.onError(); + EXPECT_EQ(m_sm.currentState(), PlayerStateId::ERROR); +} + +/** + * @test onError from SOURCES_ATTACHING → ERROR. + */ +TEST_F(AampPlayerStateMachineTest, OnError_FromSourcesAttaching_TransitionsToError) +{ + m_sm.onPipelineLoaded(); + m_sm.onSourceAttaching(); + m_sm.onError(); + EXPECT_EQ(m_sm.currentState(), PlayerStateId::ERROR); +} + +/** + * @test onStop from ERROR → IDLE. + */ +TEST_F(AampPlayerStateMachineTest, OnStop_FromError_TransitionsToIdle) +{ + m_sm.onPipelineLoaded(); + m_sm.onSourceAttaching(); + m_sm.onAllSourcesAttached(); + m_sm.onPlaybackStarted(); + m_sm.onError(); + ASSERT_EQ(m_sm.currentState(), PlayerStateId::ERROR); + m_sm.onStop(); + EXPECT_EQ(m_sm.currentState(), PlayerStateId::IDLE); +} + /** * @test onError from FLUSHING → ERROR. */ @@ -385,17 +431,6 @@ TEST_F(AampPlayerStateMachineTest, OnReconfigure_FromPlaying_TransitionsToIdle) EXPECT_EQ(m_sm.currentState(), PlayerStateId::IDLE); } -/** - * @test onReconfigure from STOPPED → IDLE. - */ -TEST_F(AampPlayerStateMachineTest, OnReconfigure_FromStopped_TransitionsToIdle) -{ - m_sm.onPipelineLoaded(); - m_sm.onStop(); - m_sm.onReconfigure(); - EXPECT_EQ(m_sm.currentState(), PlayerStateId::IDLE); -} - /** * @test onReconfigure from ERROR → IDLE. */ @@ -408,16 +443,21 @@ TEST_F(AampPlayerStateMachineTest, OnReconfigure_FromError_TransitionsToIdle) } /** - * @test onReconfigure from FLUSHING → IDLE. + * @test onReconfigure from FLUSHING is ignored — FlushingState has no + * onReconfigure override. + * + * Configure() calls Stop() first, which calls WaitForFlushToComplete(), + * so the machine has already left FLUSHING before onReconfigure is fired. + * The dispatch layer emits a WARN and the machine stays in FLUSHING. */ -TEST_F(AampPlayerStateMachineTest, OnReconfigure_FromFlushing_TransitionsToIdle) +TEST_F(AampPlayerStateMachineTest, OnReconfigure_FromFlushing_IsIgnored) { m_sm.onPipelineLoaded(); m_sm.onSourceAttaching(); m_sm.onAllSourcesAttached(); m_sm.onFlush(); - m_sm.onReconfigure(); - EXPECT_EQ(m_sm.currentState(), PlayerStateId::IDLE); + m_sm.onReconfigure(); // no transition defined — dispatch warns + EXPECT_EQ(m_sm.currentState(), PlayerStateId::FLUSHING); } // =========================================================================== diff --git a/test/utests/tests/AampRialtoPlayerTests/AampRialtoPlayerTestCases.cpp b/test/utests/tests/AampRialtoPlayerTests/AampRialtoPlayerTestCases.cpp index 76bb2c620e..eee59be66e 100644 --- a/test/utests/tests/AampRialtoPlayerTests/AampRialtoPlayerTestCases.cpp +++ b/test/utests/tests/AampRialtoPlayerTests/AampRialtoPlayerTestCases.cpp @@ -2994,9 +2994,9 @@ TEST_F(AampRialtoPlayerTest, // Flush() with shouldTearDown=true should call Stop() even in IDLE state. m_player->Flush(/*position=*/10.0, /*rate=*/1, /*shouldTearDown=*/true); - // Verify player transitions to STOPPED state. - EXPECT_EQ(m_player->GetCurrentPlayerState(), PlayerStateId::STOPPED) - << "Player must transition to STOPPED after Flush(shouldTearDown=true)"; + // Stop() transitions to IDLE; PlayerStateId::STOPPED is unreachable. + EXPECT_EQ(m_player->GetCurrentPlayerState(), PlayerStateId::IDLE) + << "Player remains in IDLE after Flush(shouldTearDown=true) with no pipeline"; } TEST_F(AampRialtoPlayerTest, @@ -3031,9 +3031,9 @@ TEST_F(AampRialtoPlayerWithDemuxTest, Configure(); m_player->Stop(false); - // Verify player is in STOPPED state. - ASSERT_EQ(m_player->GetCurrentPlayerState(), PlayerStateId::STOPPED) - << "Precondition: player must be in STOPPED state after Stop()"; + // onStop() transitions to IDLE; PlayerStateId::STOPPED is unreachable. + ASSERT_EQ(m_player->GetCurrentPlayerState(), PlayerStateId::IDLE) + << "Precondition: player must be in IDLE state after Stop()"; // Expect Stop() to be called again when shouldTearDown=true. EXPECT_CALL(*m_mockPipelinePtr, stop()).Times(1); @@ -3051,7 +3051,8 @@ TEST_F(AampRialtoPlayerWithDemuxTest, Configure(); m_player->Stop(false); - ASSERT_EQ(m_player->GetCurrentPlayerState(), PlayerStateId::STOPPED); + // onStop() transitions to IDLE; PlayerStateId::STOPPED is unreachable. + ASSERT_EQ(m_player->GetCurrentPlayerState(), PlayerStateId::IDLE); // Expect NO additional stop() call. EXPECT_CALL(*m_mockPipelinePtr, stop()).Times(0); From 0ed4d96c51a9363ca665c97eeee69a3d00336554 Mon Sep 17 00:00:00 2001 From: anshephe <115161257+anshephe@users.noreply.github.com> Date: Mon, 6 Jul 2026 16:41:38 +0000 Subject: [PATCH 03/19] Now calls play if the stream() call was deferred dut to flushing --- direct-rialto/AampRialtoPlayer.cpp | 21 ++++-- .../AampRialtoPlayerTestCases.cpp | 75 +++++++++++++++++++ 2 files changed, 91 insertions(+), 5 deletions(-) diff --git a/direct-rialto/AampRialtoPlayer.cpp b/direct-rialto/AampRialtoPlayer.cpp index 4560981a27..618b8cbe00 100644 --- a/direct-rialto/AampRialtoPlayer.cpp +++ b/direct-rialto/AampRialtoPlayer.cpp @@ -963,6 +963,7 @@ void AampRialtoPlayer::CheckAllSourcesAttached() if (m_playRequested.load(std::memory_order_seq_cst)) { AAMPLOG_INFO("play() deferred by Stream() - issuing now"); + m_playRequested.store(false, std::memory_order_relaxed); bool async = false; if (!m_pipeline->play(async)) { @@ -1068,6 +1069,7 @@ void AampRialtoPlayer::Stream() { // allSourcesAttached() already completed before this call - // promote to PLAYING immediately. + m_playRequested.store(false, std::memory_order_relaxed); bool async = false; if (!m_pipeline->play(async)) { @@ -1980,12 +1982,21 @@ void AampRialtoPlayer::OnSourceFlushed(int32_t sourceId) m_stateMachine.onFlushComplete(); m_flushCv.notify_all(); - // Issue play() only if the restored state is PLAYING. - // This correctly handles seek-while-paused: the pre-flush state was - // PAUSED so the pipeline stays paused after the flush. - if (m_stateMachine.currentState() == PlayerStateId::PLAYING) + // Issue play() if the restored state is PLAYING (seek-while-playing) + // or if Stream() was called while the flush was in progress + // (e.g. Stream() deferred from SOURCES_ATTACHED or a flush that + // preceded allSourcesAttached()). m_playRequested is reset here so + // it is not treated as stale on a subsequent flush cycle. + // seek-while-paused is correctly handled: Stream() is not called in + // that path so m_playRequested remains false and play() is not issued. + if (m_stateMachine.currentState() == PlayerStateId::PLAYING || + m_playRequested.load(std::memory_order_seq_cst)) { - AAMPLOG_INFO("All sources flushed - issuing play() (pre-flush state was PLAYING)"); + AAMPLOG_INFO("All sources flushed - issuing play() " + "(state=%s playRequested=%d)", + m_stateMachine.currentStateName(), + m_playRequested.load(std::memory_order_relaxed)); + m_playRequested.store(false, std::memory_order_relaxed); bool async = false; if (!m_pipeline->play(async)) { diff --git a/test/utests/tests/AampRialtoPlayerTests/AampRialtoPlayerTestCases.cpp b/test/utests/tests/AampRialtoPlayerTests/AampRialtoPlayerTestCases.cpp index eee59be66e..dd650f8b41 100644 --- a/test/utests/tests/AampRialtoPlayerTests/AampRialtoPlayerTestCases.cpp +++ b/test/utests/tests/AampRialtoPlayerTests/AampRialtoPlayerTestCases.cpp @@ -973,6 +973,81 @@ TEST_F(AampRialtoPlayerWithDemuxTest, Stream_CallsPlay) m_player->Stream(); } +TEST_F(AampRialtoPlayerWithDemuxTest, + Stream_DeferredDuringFlush_OnSourceFlushedIssuesPlay) +{ + /** + * @brief Regression: when Stream() is called while a flush is in + * progress, m_playRequested is set but play() is deferred. + * OnSourceFlushed() completing the flush cycle must issue + * play() because m_playRequested is true, even though the + * restored pre-flush state is SOURCES_ATTACHED (not PLAYING). + */ + Configure(); + SendVideoInitFragment(); + SendAudioInitFragment(); + + ASSERT_EQ(m_player->GetCurrentPlayerState(), PlayerStateId::SOURCES_ATTACHED); + + // Flush: state → FLUSHING, pre-flush state = SOURCES_ATTACHED. + m_player->Flush(/*position=*/5.0, /*rate=*/1, /*shouldTearDown=*/false); + ASSERT_EQ(m_player->GetCurrentPlayerState(), PlayerStateId::FLUSHING) + << "Precondition: Flush() must move state to FLUSHING"; + + // Stream() while FLUSHING sets m_playRequested=true and returns early + // without calling play(). play() must be issued exactly once — + // by OnSourceFlushed() once all sources confirm the flush. + EXPECT_CALL(*m_mockPipelinePtr, play(_)).Times(1).WillOnce(Return(true)); + m_player->Stream(); + + // Simulate Rialto confirming the flush for all attached sources. + // PostSourceFlushed for the last source detects allSourcesFlushed, + // calls onFlushComplete() (restoring SOURCES_ATTACHED), then checks + // m_playRequested=true and issues play(). + PostSourceFlushed(/*sourceId=*/0); // video + PostSourceFlushed(/*sourceId=*/1); // inband CC subtitle + PostSourceFlushed(/*sourceId=*/2); // audio — triggers play() via m_playRequested +} + +TEST_F(AampRialtoPlayerWithDemuxTest, + Stream_PlayRequestReset_SubsequentFlushWhilePausedNoPlay) +{ + /** + * @brief After Stream() successfully issues play(), m_playRequested is + * reset to false. A subsequent seek-while-paused flush must not + * spuriously re-issue play() when OnSourceFlushed restores PAUSED. + */ + Configure(); + SendVideoInitFragment(); + SendAudioInitFragment(); + + // Stream(): sources already attached, play() issued immediately and + // m_playRequested reset to false. + EXPECT_CALL(*m_mockPipelinePtr, play(_)).Times(1).WillOnce(Return(true)); + m_player->Stream(); + ::testing::Mock::VerifyAndClearExpectations(m_mockPipelinePtr); + + // Rialto confirms PLAYING, then user pauses. + PostPlaybackState(firebolt::rialto::PlaybackState::PLAYING); + ASSERT_EQ(m_player->GetCurrentPlayerState(), PlayerStateId::PLAYING); + PostPlaybackState(firebolt::rialto::PlaybackState::PAUSED); + ASSERT_EQ(m_player->GetCurrentPlayerState(), PlayerStateId::PAUSED); + + // Seek-while-paused: Flush() → FLUSHING, pre-flush state = PAUSED. + m_player->Flush(/*position=*/10.0, /*rate=*/1, /*shouldTearDown=*/false); + ASSERT_EQ(m_player->GetCurrentPlayerState(), PlayerStateId::FLUSHING); + + // play() must NOT be called: m_playRequested was reset when Stream() + // issued play(), and the restored post-flush state is PAUSED. + EXPECT_CALL(*m_mockPipelinePtr, play(_)).Times(0); + PostSourceFlushed(/*sourceId=*/0); // video + PostSourceFlushed(/*sourceId=*/1); // inband CC subtitle + PostSourceFlushed(/*sourceId=*/2); // audio — triggers onFlushComplete() → PAUSED + + EXPECT_EQ(m_player->GetCurrentPlayerState(), PlayerStateId::PAUSED) + << "Seek-while-paused flush must restore PAUSED, not resume play"; +} + TEST_F(AampRialtoPlayerWithDemuxTest, Stop_CallsPipelineStop) { Configure(); From 9474a09527be29ba3b50fed4b867473b689c453b Mon Sep 17 00:00:00 2001 From: anshephe <115161257+anshephe@users.noreply.github.com> Date: Tue, 7 Jul 2026 09:19:39 +0000 Subject: [PATCH 04/19] Test code to see if trickplay on linear works if we unblock the StreamSink injector --- priv_aamp.cpp | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/priv_aamp.cpp b/priv_aamp.cpp index 2767c9cd72..84d49a16cf 100644 --- a/priv_aamp.cpp +++ b/priv_aamp.cpp @@ -10583,6 +10583,15 @@ void PrivateInstanceAAMP::StopTrackInjection(AampMediaType type) { AAMPLOG_TRACE("PrivateInstanceAAMP: for type %s", GetMediaTypeName(type) ); std::lock_guard guard(mLock); + // Direct Rialto blocks the injector thread(s) whilst waiting for NeedData, + // this call releases the thread + // TODO: Is this the correct approach, does Resume now need to be called + // or is a new API needed on StreamSink + StreamSink *sink = AampStreamSinkManager::GetInstance().GetStreamSink(this); + if (sink) + { + sink->NotifyInjectorToPause(); + } mTrackInjectionBlocked[type] = true; } AAMPLOG_TRACE ("PrivateInstanceAAMP::Exit. type = %d", (int) type); From 1445e2c4d240fce8cbc094eb7d8c4e21bde69d73 Mon Sep 17 00:00:00 2001 From: anshephe <115161257+anshephe@users.noreply.github.com> Date: Tue, 7 Jul 2026 11:35:56 +0000 Subject: [PATCH 05/19] Moved where StopProgressTimer is called in Configure, as it was not being stopped for trickplay --- direct-rialto/AampRialtoPlayer.cpp | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/direct-rialto/AampRialtoPlayer.cpp b/direct-rialto/AampRialtoPlayer.cpp index 618b8cbe00..83493ac362 100644 --- a/direct-rialto/AampRialtoPlayer.cpp +++ b/direct-rialto/AampRialtoPlayer.cpp @@ -402,6 +402,8 @@ void AampRialtoPlayer::Configure( // flushing so m_rate is committed before ShouldRecreatePipeline reads it. WaitForFlushToComplete(); + StopProgressTimer(); + // Guard: skip teardown and recreation when the pipeline can be reused. // Rialto does not support dynamic source management, so any change to // the source set requires a full rebuild. The exception is audio going @@ -459,7 +461,6 @@ void AampRialtoPlayer::Configure( // (re-tune or first tune). This resets to IDLE from whatever previous // state the player was in. m_stateMachine.onReconfigure(); - StopProgressTimer(); // Reset first-frame flag so the new tune session forwards the initial // PLAYING notification correctly. @@ -2055,6 +2056,7 @@ void AampRialtoPlayer::StartProgressTimer() return; } + AAMPLOG_INFO("Starting progress timer %dms", intervalMs); m_progressTimer->start(intervalMs, [this]() { this->OnProgressTimerTick(); }); @@ -2064,6 +2066,7 @@ void AampRialtoPlayer::StopProgressTimer() { if (m_progressTimer) { + AAMPLOG_INFO("Stopping progress timer"); m_progressTimer->stop(); } } From ff897d1580c483cd93d98e45cbbe083a98af5f80 Mon Sep 17 00:00:00 2001 From: anshephe <115161257+anshephe@users.noreply.github.com> Date: Tue, 7 Jul 2026 13:09:50 +0000 Subject: [PATCH 06/19] Trying a flush after sending EOS to audio --- direct-rialto/AampRialtoPlayer.cpp | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/direct-rialto/AampRialtoPlayer.cpp b/direct-rialto/AampRialtoPlayer.cpp index 83493ac362..9b72a98ec1 100644 --- a/direct-rialto/AampRialtoPlayer.cpp +++ b/direct-rialto/AampRialtoPlayer.cpp @@ -422,6 +422,10 @@ void AampRialtoPlayer::Configure( AAMPLOG_INFO("Audio going FORMAT_INVALID (trickplay) - " "signalling EOS on audio source, no pipeline recreation"); EndOfStreamReached(eMEDIATYPE_AUDIO); + + double position = m_pendingPositionNs.load(std::memory_order_relaxed); + int rate = m_rate.load(std::memory_order_relaxed); + Flush(position, rate, false); } else if (m_sources[eMEDIATYPE_AUDIO] && audioFormat != FORMAT_INVALID) From af30e990026da11efd58cb8cd514a3e30a4e5d32 Mon Sep 17 00:00:00 2001 From: anshephe <115161257+anshephe@users.noreply.github.com> Date: Tue, 7 Jul 2026 15:35:07 +0000 Subject: [PATCH 07/19] See if callingSetSourcePosition before flushing has an affect --- direct-rialto/AampRialtoPlayer.cpp | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/direct-rialto/AampRialtoPlayer.cpp b/direct-rialto/AampRialtoPlayer.cpp index 9b72a98ec1..944ac3f39f 100644 --- a/direct-rialto/AampRialtoPlayer.cpp +++ b/direct-rialto/AampRialtoPlayer.cpp @@ -1242,6 +1242,14 @@ void AampRialtoPlayer::Flush(double position, int rate, bool shouldTearDown) { if (source && source->isAttached()) { + int32_t sourceId = source->sourceId(); + if (!m_pipeline->setSourcePosition( + sourceId, posNs, /*resetTime=*/true, + computeAppliedRate(rate))) + { + AAMPLOG_WARN("setSourcePosition failed for sourceId=%d", + sourceId); + } anyAttachedSource = true; source->setFlushing(true); source->flushSource(*m_pipeline, posNs); From 57d6f3d591e6c78d0ca1de201030327812b27fd7 Mon Sep 17 00:00:00 2001 From: anshephe <115161257+anshephe@users.noreply.github.com> Date: Wed, 8 Jul 2026 11:50:50 +0000 Subject: [PATCH 08/19] Revert "Trying a flush after sending EOS to audio" This reverts commit ff897d1580c483cd93d98e45cbbe083a98af5f80. --- direct-rialto/AampRialtoPlayer.cpp | 4 ---- 1 file changed, 4 deletions(-) diff --git a/direct-rialto/AampRialtoPlayer.cpp b/direct-rialto/AampRialtoPlayer.cpp index 944ac3f39f..1ec3bdff43 100644 --- a/direct-rialto/AampRialtoPlayer.cpp +++ b/direct-rialto/AampRialtoPlayer.cpp @@ -422,10 +422,6 @@ void AampRialtoPlayer::Configure( AAMPLOG_INFO("Audio going FORMAT_INVALID (trickplay) - " "signalling EOS on audio source, no pipeline recreation"); EndOfStreamReached(eMEDIATYPE_AUDIO); - - double position = m_pendingPositionNs.load(std::memory_order_relaxed); - int rate = m_rate.load(std::memory_order_relaxed); - Flush(position, rate, false); } else if (m_sources[eMEDIATYPE_AUDIO] && audioFormat != FORMAT_INVALID) From 9555522c3b8bcc58a0047ad7de91fc048821b06d Mon Sep 17 00:00:00 2001 From: anshephe <115161257+anshephe@users.noreply.github.com> Date: Wed, 8 Jul 2026 14:53:38 +0000 Subject: [PATCH 09/19] Don't call setSourcePosition on audio/subtitles when trickplay (Trying to match non-direct rialto behaviour) --- direct-rialto/AampRialtoPlayer.cpp | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-) diff --git a/direct-rialto/AampRialtoPlayer.cpp b/direct-rialto/AampRialtoPlayer.cpp index 1ec3bdff43..796cb73be9 100644 --- a/direct-rialto/AampRialtoPlayer.cpp +++ b/direct-rialto/AampRialtoPlayer.cpp @@ -1238,14 +1238,6 @@ void AampRialtoPlayer::Flush(double position, int rate, bool shouldTearDown) { if (source && source->isAttached()) { - int32_t sourceId = source->sourceId(); - if (!m_pipeline->setSourcePosition( - sourceId, posNs, /*resetTime=*/true, - computeAppliedRate(rate))) - { - AAMPLOG_WARN("setSourcePosition failed for sourceId=%d", - sourceId); - } anyAttachedSource = true; source->setFlushing(true); source->flushSource(*m_pipeline, posNs); @@ -1961,6 +1953,7 @@ void AampRialtoPlayer::OnSourceFlushed(int32_t sourceId) const int pendingRate = m_pendingFlushRate.load(std::memory_order_relaxed); if (m_pipeline && + !source->state().eos && !m_pipeline->setSourcePosition( sourceId, posNs, /*resetTime=*/true, computeAppliedRate(pendingRate))) From c17e8fcd28b3b6a39094e21003b76dbc7687149a Mon Sep 17 00:00:00 2001 From: anshephe <115161257+anshephe@users.noreply.github.com> Date: Wed, 8 Jul 2026 15:55:21 +0000 Subject: [PATCH 10/19] Previous didn't have an affect as flush is called before configure, changed the code to check the rate --- direct-rialto/AampRialtoPlayer.cpp | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/direct-rialto/AampRialtoPlayer.cpp b/direct-rialto/AampRialtoPlayer.cpp index 796cb73be9..f63b72acaf 100644 --- a/direct-rialto/AampRialtoPlayer.cpp +++ b/direct-rialto/AampRialtoPlayer.cpp @@ -1952,8 +1952,10 @@ void AampRialtoPlayer::OnSourceFlushed(int32_t sourceId) m_pendingPositionNs.load(std::memory_order_relaxed); const int pendingRate = m_pendingFlushRate.load(std::memory_order_relaxed); + const bool setPosition = (pendingRate == 1) || + (sourceId == m_sources[eMEDIATYPE_VIDEO]->sourceId()); if (m_pipeline && - !source->state().eos && + setPosition && !m_pipeline->setSourcePosition( sourceId, posNs, /*resetTime=*/true, computeAppliedRate(pendingRate))) From a370d1eb825864614ba1ea65ab36c40f8a68bb5c Mon Sep 17 00:00:00 2001 From: anshephe <115161257+anshephe@users.noreply.github.com> Date: Thu, 9 Jul 2026 08:45:55 +0000 Subject: [PATCH 11/19] When all sources flushed only csll play() if play requested, and state is not PLAYING --- direct-rialto/AampRialtoPlayer.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/direct-rialto/AampRialtoPlayer.cpp b/direct-rialto/AampRialtoPlayer.cpp index f63b72acaf..174853cec7 100644 --- a/direct-rialto/AampRialtoPlayer.cpp +++ b/direct-rialto/AampRialtoPlayer.cpp @@ -1993,7 +1993,7 @@ void AampRialtoPlayer::OnSourceFlushed(int32_t sourceId) // it is not treated as stale on a subsequent flush cycle. // seek-while-paused is correctly handled: Stream() is not called in // that path so m_playRequested remains false and play() is not issued. - if (m_stateMachine.currentState() == PlayerStateId::PLAYING || + if (m_stateMachine.currentState() != PlayerStateId::PLAYING && m_playRequested.load(std::memory_order_seq_cst)) { AAMPLOG_INFO("All sources flushed - issuing play() " From 4cdc7d83e3e7d42201404a93274cdf8662bc33f3 Mon Sep 17 00:00:00 2001 From: anshephe <115161257+anshephe@users.noreply.github.com> Date: Thu, 9 Jul 2026 09:32:13 +0000 Subject: [PATCH 12/19] Avoid calling play in Stream() if already PLAYING --- direct-rialto/AampRialtoPlayer.cpp | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/direct-rialto/AampRialtoPlayer.cpp b/direct-rialto/AampRialtoPlayer.cpp index 174853cec7..0f0dc30d84 100644 --- a/direct-rialto/AampRialtoPlayer.cpp +++ b/direct-rialto/AampRialtoPlayer.cpp @@ -1068,13 +1068,16 @@ void AampRialtoPlayer::Stream() // to guarantee one side always calls play(). if (m_allSourcesAttachedFlag.load(std::memory_order_seq_cst)) { - // allSourcesAttached() already completed before this call - - // promote to PLAYING immediately. m_playRequested.store(false, std::memory_order_relaxed); - bool async = false; - if (!m_pipeline->play(async)) + if (m_stateMachine.currentState() != PlayerStateId::PLAYING) { - AAMPLOG_ERR("play() failed"); + // allSourcesAttached() already completed before this call - + // promote to PLAYING immediately. + bool async = false; + if (!m_pipeline->play(async)) + { + AAMPLOG_ERR("play() failed"); + } } } else From 61edf4b63a8c9cde822e825cfbcccd85e4ba2791 Mon Sep 17 00:00:00 2001 From: anshephe <115161257+anshephe@users.noreply.github.com> Date: Thu, 9 Jul 2026 12:41:32 +0000 Subject: [PATCH 13/19] Trying only sending eos for subtitles based on video (which appears to be the case of what is happening in live trickplay) --- direct-rialto/AampRialtoPlayer.cpp | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/direct-rialto/AampRialtoPlayer.cpp b/direct-rialto/AampRialtoPlayer.cpp index 0f0dc30d84..a42d14d788 100644 --- a/direct-rialto/AampRialtoPlayer.cpp +++ b/direct-rialto/AampRialtoPlayer.cpp @@ -1023,19 +1023,15 @@ void AampRialtoPlayer::EndOfStreamReached(AampMediaType type) // a. Content with no subtitle tracks (e.g. an ad): subtitle source was // created from a saved header but will receive no data. - if (type == eMEDIATYPE_AUDIO) + if (type == eMEDIATYPE_VIDEO) { - auto *audioSrc = m_sources[eMEDIATYPE_AUDIO].get(); auto *subtitleSrc = m_sources[eMEDIATYPE_SUBTITLE].get(); - const bool audioAtEos = audioSrc && audioSrc->state().eos; - - if (audioAtEos - && subtitleSrc + if (subtitleSrc && subtitleSrc->isAttached() && !subtitleSrc->state().eos) { - AAMPLOG_INFO("auto-signaling subtitle EOS: audio at EOS"); + AAMPLOG_INFO("auto-signaling subtitle EOS: video at EOS"); subtitleSrc->signalEos(m_pipeline.get()); } } From e6078596a4007c0887770929db13041df55d6af2 Mon Sep 17 00:00:00 2001 From: anshephe <115161257+anshephe@users.noreply.github.com> Date: Wed, 15 Jul 2026 16:27:54 +0000 Subject: [PATCH 14/19] In Flush trying the Rialto setPosition API to handle the flush --- direct-rialto/AampRialtoPlayer.cpp | 99 +++++++++++++++++++++++------- 1 file changed, 77 insertions(+), 22 deletions(-) diff --git a/direct-rialto/AampRialtoPlayer.cpp b/direct-rialto/AampRialtoPlayer.cpp index a42d14d788..45f687f89a 100644 --- a/direct-rialto/AampRialtoPlayer.cpp +++ b/direct-rialto/AampRialtoPlayer.cpp @@ -1229,41 +1229,33 @@ void AampRialtoPlayer::Flush(double position, int rate, bool shouldTearDown) if (m_pipeline) { - bool anyAttachedSource = false; - // Mark each attached source as flushing *before* sending the - // IPC flush command so OnSourceFlushed() can call - // setSourcePosition() once the server confirms the flush. - for (auto &source : m_sources) - { - if (source && source->isAttached()) - { - anyAttachedSource = true; - source->setFlushing(true); - source->flushSource(*m_pipeline, posNs); - } - } - - // If no source is attached, there will be no SourceFlushedEvent to - // commit the pending rate. Commit it immediately so position - // calculations after Flush() observe the requested trickplay rate. - if (!anyAttachedSource) + // Perform a pipeline-level flushing seek. Rialto will emit + // PlaybackState::SEEK_DONE when the seek completes; OnPlaybackState() + // handles rate commit, per-source segment position (for trickplay + // applied_rate), state machine restoration, and flush-CV signalling. + AAMPLOG_INFO("Issuing pipeline setPosition posNs=%" PRId64, posNs); + if (!m_pipeline->setPosition(posNs)) { + AAMPLOG_WARN("setPosition failed for posNs=%" PRId64 + " - committing rate immediately and exiting FLUSHING", posNs); m_rate.store( m_pendingFlushRate.load(std::memory_order_relaxed), std::memory_order_relaxed); - AAMPLOG_INFO("No attached sources to flush - committed playback rate=%d", - m_rate.load(std::memory_order_relaxed)); + m_stateMachine.onFlushComplete(); + m_flushCv.notify_all(); } } else { - // With no pipeline there can be no SourceFlushedEvent callback, so - // commit the pending rate immediately. + // With no pipeline there can be no SEEK_DONE callback, so commit + // the pending rate immediately and exit FLUSHING. m_rate.store( m_pendingFlushRate.load(std::memory_order_relaxed), std::memory_order_relaxed); AAMPLOG_INFO("No pipeline during flush - committed playback rate=%d", m_rate.load(std::memory_order_relaxed)); + m_stateMachine.onFlushComplete(); + m_flushCv.notify_all(); } // m_firstPtsMs is reset automatically on each source by @@ -1890,6 +1882,69 @@ void AampRialtoPlayer::OnPlaybackState(firebolt::rialto::PlaybackState state) case firebolt::rialto::PlaybackState::FAILURE: m_stateMachine.onError(); break; + case firebolt::rialto::PlaybackState::SEEKING: + AAMPLOG_INFO("SEEKING notification received (state=%s)", + m_stateMachine.currentStateName()); + break; + case firebolt::rialto::PlaybackState::SEEK_DONE: + { + // Ignore SEEK_DONE not originating from a Flush()-initiated seek. + // setPosition() is also called by SeekStreamSink() without + // entering FLUSHING; that path needs no flush-completion work. + if (m_stateMachine.currentState() != PlayerStateId::FLUSHING) + { + AAMPLOG_INFO("SEEK_DONE received outside FLUSHING (state=%s) - ignored", + m_stateMachine.currentStateName()); + break; + } + + const int64_t posNs = + m_pendingPositionNs.load(std::memory_order_relaxed); + const int pendingRate = + m_pendingFlushRate.load(std::memory_order_relaxed); + +#if 0 + // Apply per-source segment position so the GStreamer + // segment event carries the correct applied_rate for trickplay. + // resetTime=false: the pipeline-level setPosition() already + // flushed source buffers; we only update the segment rate. + for (auto &source : m_sources) + { + if (source && source->isAttached() && m_pipeline) + { + const bool shouldSetPosition = + (pendingRate == AAMP_NORMAL_PLAY_RATE) || + (source.get() == m_sources[eMEDIATYPE_VIDEO].get()); + if (shouldSetPosition && + !m_pipeline->setSourcePosition( + source->sourceId(), posNs, + /*resetTime=*/false, + computeAppliedRate(pendingRate))) + { + AAMPLOG_WARN("setSourcePosition failed for " + "sourceId=%d after SEEK_DONE", + source->sourceId()); + } + } + } +#endif + + m_rate.store(pendingRate, std::memory_order_relaxed); + AAMPLOG_INFO("SEEK_DONE: committed playback rate=%d", pendingRate); + + // Restore the pre-flush state before notifying + // WaitForFlushToComplete(). + m_stateMachine.onFlushComplete(); + m_flushCv.notify_all(); + + // Rialto will emit PLAYING automatically after SEEK_DONE. + // Clear m_playRequested so it is not treated as stale on a + // subsequent flush cycle; the automatic PLAYING notification + // drives the state machine and first-frame callbacks. + m_playRequested.store(false, std::memory_order_relaxed); + + break; + } default: break; } From db6f70d3f3f6b161d2657901e266e81d7e4254ad Mon Sep 17 00:00:00 2001 From: anshephe <115161257+anshephe@users.noreply.github.com> Date: Thu, 16 Jul 2026 11:35:18 +0000 Subject: [PATCH 15/19] SEEK_DONE now calls play() if a call to Stream deferred it --- direct-rialto/AampRialtoPlayer.cpp | 33 ++++++++++++++++++++++++------ 1 file changed, 27 insertions(+), 6 deletions(-) diff --git a/direct-rialto/AampRialtoPlayer.cpp b/direct-rialto/AampRialtoPlayer.cpp index 45f687f89a..5dabab154e 100644 --- a/direct-rialto/AampRialtoPlayer.cpp +++ b/direct-rialto/AampRialtoPlayer.cpp @@ -1049,8 +1049,8 @@ void AampRialtoPlayer::Stream() // store before it reads m_playRequested (and vice-versa). m_playRequested.store(true, std::memory_order_seq_cst); - // If a flush is in progress, play() will be issued by OnSourceFlushed() - // once all sources confirm flushed and onFlushComplete() restores state. + // If a flush is in progress, play() will be issued by the SEEK_DONE + // handler in OnPlaybackState() once onFlushComplete() restores state. // FLUSHING is set inside m_flushMutex in Flush() and cleared by // onFlushComplete() (both protected by the state machine mutex). if (m_stateMachine.currentState() == PlayerStateId::FLUSHING) @@ -1937,12 +1937,33 @@ void AampRialtoPlayer::OnPlaybackState(firebolt::rialto::PlaybackState state) m_stateMachine.onFlushComplete(); m_flushCv.notify_all(); - // Rialto will emit PLAYING automatically after SEEK_DONE. - // Clear m_playRequested so it is not treated as stale on a - // subsequent flush cycle; the automatic PLAYING notification - // drives the state machine and first-frame callbacks. + // Issue play() if Stream() was called while the flush was in + // progress and the restored state is not already PLAYING. + // Rialto also emits PLAYING automatically after SEEK_DONE for + // seek-while-playing; a redundant play() call is harmless. + // seek-while-paused is handled correctly: Stream() is not + // called in that path so m_playRequested stays false. + const bool shouldPlay = + m_stateMachine.currentState() != PlayerStateId::PLAYING && + m_playRequested.load(std::memory_order_seq_cst); m_playRequested.store(false, std::memory_order_relaxed); + if (shouldPlay) + { + AAMPLOG_INFO("SEEK_DONE: issuing play() (state=%s)", + m_stateMachine.currentStateName()); + bool async = false; + if (!m_pipeline->play(async)) + { + AAMPLOG_ERR("play() failed after SEEK_DONE"); + } + } + else + { + AAMPLOG_INFO("SEEK_DONE: not issuing play() (state=%s)", + m_stateMachine.currentStateName()); + } + break; } default: From a1967108306bdf9bb3b58db7c8ae6986429112d3 Mon Sep 17 00:00:00 2001 From: anshephe <115161257+anshephe@users.noreply.github.com> Date: Thu, 16 Jul 2026 16:25:44 +0000 Subject: [PATCH 16/19] Cleanup of unused code, and comments Enabled applied_rate setting following Flush --- direct-rialto/AampPlayerStateMachine.cpp | 4 +- direct-rialto/AampPlayerStateMachine.h | 4 +- .../AampRialtoMediaPipelineClient.cpp | 9 +- direct-rialto/AampRialtoPlayer.cpp | 121 +++--------------- 4 files changed, 22 insertions(+), 116 deletions(-) diff --git a/direct-rialto/AampPlayerStateMachine.cpp b/direct-rialto/AampPlayerStateMachine.cpp index bdd6a59362..6d0b80fbc8 100644 --- a/direct-rialto/AampPlayerStateMachine.cpp +++ b/direct-rialto/AampPlayerStateMachine.cpp @@ -375,8 +375,8 @@ void PlayerStateMachine::onFlushComplete() if (m_state->id() != PlayerStateId::FLUSHING) { // The edge-case race: onPlaybackStarted/Paused already exited - // FLUSHING before all sources confirmed flushed. This call is a - // no-op — the correct post-flush state was already applied. + // FLUSHING before SEEK_DONE arrived. This call is a no-op — + // the correct post-flush state was already applied. AAMPLOG_INFO("PlayerState: onFlushComplete ignored - current state is '" "%s', not FLUSHING (edge-case race already resolved)", m_state->name()); diff --git a/direct-rialto/AampPlayerStateMachine.h b/direct-rialto/AampPlayerStateMachine.h index e66de3d875..4dd6630dba 100644 --- a/direct-rialto/AampPlayerStateMachine.h +++ b/direct-rialto/AampPlayerStateMachine.h @@ -115,7 +115,7 @@ class IPlayerState /// Fired when Flush() is called. virtual std::unique_ptr onFlush() { return nullptr; } - /// Fired when all sources confirm SourceFlushedEvent, completing the + /// Fired when PlaybackState::SEEK_DONE is received, completing the /// flush cycle. Only meaningful from FLUSHING; ignored from all other /// states so the edge-case race (Rialto PLAYING arriving before /// onFlushComplete) cannot cause a double-transition. @@ -188,7 +188,7 @@ class PlayerStateMachine /// @see IPlayerState::onFlush void onFlush(); - /// Fired when all sources report SourceFlushedEvent. + /// Fired when PlaybackState::SEEK_DONE is received from Rialto. /// /// Restores the state that was current when onFlush() was called /// (PLAYING → PLAYING, PAUSED → PAUSED, SOURCES_ATTACHED → diff --git a/direct-rialto/AampRialtoMediaPipelineClient.cpp b/direct-rialto/AampRialtoMediaPipelineClient.cpp index 269ef8440c..8f1d59e552 100644 --- a/direct-rialto/AampRialtoMediaPipelineClient.cpp +++ b/direct-rialto/AampRialtoMediaPipelineClient.cpp @@ -163,12 +163,9 @@ void AampRialtoMediaPipelineClient::notifyPlaybackError( void AampRialtoMediaPipelineClient::notifySourceFlushed(int32_t sourceId) { - AAMPLOG_INFO("ENTRY sourceId=%d", sourceId); - if (m_sourceFlushedCallback) - { - m_sourceFlushedCallback(sourceId); - } - AAMPLOG_INFO("EXIT"); + // Flush() now uses pipeline-level setPosition(); completion is signalled + // via PlaybackState::SEEK_DONE, not per-source SourceFlushedEvents. + AAMPLOG_INFO("sourceId=%d - no-op", sourceId); } void AampRialtoMediaPipelineClient::notifyPlaybackInfo( diff --git a/direct-rialto/AampRialtoPlayer.cpp b/direct-rialto/AampRialtoPlayer.cpp index 5dabab154e..323445c14e 100644 --- a/direct-rialto/AampRialtoPlayer.cpp +++ b/direct-rialto/AampRialtoPlayer.cpp @@ -598,10 +598,6 @@ void AampRialtoPlayer::Configure( [this](int32_t sid) { OnBufferUnderflow(sid); }); - m_client->SetSourceFlushedCallback( - [this](int32_t sid) { - OnSourceFlushed(sid); - }); // Advance state machine: pipeline is now created and loaded. m_stateMachine.onPipelineLoaded(); @@ -1903,31 +1899,24 @@ void AampRialtoPlayer::OnPlaybackState(firebolt::rialto::PlaybackState state) const int pendingRate = m_pendingFlushRate.load(std::memory_order_relaxed); -#if 0 - // Apply per-source segment position so the GStreamer + // Apply video-source segment position so the GStreamer // segment event carries the correct applied_rate for trickplay. // resetTime=false: the pipeline-level setPosition() already // flushed source buffers; we only update the segment rate. - for (auto &source : m_sources) + auto appliedRate = computeAppliedRate(pendingRate); + if (appliedRate != AAMP_NORMAL_PLAY_RATE) { - if (source && source->isAttached() && m_pipeline) - { - const bool shouldSetPosition = - (pendingRate == AAMP_NORMAL_PLAY_RATE) || - (source.get() == m_sources[eMEDIATYPE_VIDEO].get()); - if (shouldSetPosition && - !m_pipeline->setSourcePosition( - source->sourceId(), posNs, + auto *videoSource = m_sources[eMEDIATYPE_VIDEO].get(); + if (!m_pipeline->setSourcePosition( + videoSource->sourceId(), posNs, /*resetTime=*/false, computeAppliedRate(pendingRate))) - { - AAMPLOG_WARN("setSourcePosition failed for " - "sourceId=%d after SEEK_DONE", - source->sourceId()); - } + { + AAMPLOG_WARN("setSourcePosition failed for " + "sourceId=%d after SEEK_DONE", + videoSource->sourceId()); } } -#endif m_rate.store(pendingRate, std::memory_order_relaxed); AAMPLOG_INFO("SEEK_DONE: committed playback rate=%d", pendingRate); @@ -2011,91 +2000,11 @@ double AampRialtoPlayer::computeAppliedRate(int candidateRate) const void AampRialtoPlayer::OnSourceFlushed(int32_t sourceId) { - AAMPLOG_INFO("ENTRY sourceId=%d", sourceId); - - auto *source = findSourceByRialtoId(sourceId); - if (source) - { - source->setFlushing(false); - // Send the SEGMENT event now that the flush is confirmed by the - // server. setSourcePosition() was previously called inside - // flushSource() before SourceFlushedEvent, but if the server - // was still processing the flush at that point it could discard - // the SEGMENT event - leaving the pipeline's EOS state intact - // and causing an immediate END_OF_STREAM on the next play(). - const int64_t posNs = - m_pendingPositionNs.load(std::memory_order_relaxed); - const int pendingRate = - m_pendingFlushRate.load(std::memory_order_relaxed); - const bool setPosition = (pendingRate == 1) || - (sourceId == m_sources[eMEDIATYPE_VIDEO]->sourceId()); - if (m_pipeline && - setPosition && - !m_pipeline->setSourcePosition( - sourceId, posNs, /*resetTime=*/true, - computeAppliedRate(pendingRate))) - { - AAMPLOG_WARN("setSourcePosition failed for sourceId=%d", - sourceId); - } - - bool allSourcesFlushed = true; - for (const auto &s : m_sources) - { - if (s && s->isFlushing()) - { - allSourcesFlushed = false; - break; - } - } - - if (allSourcesFlushed) - { - m_rate.store(pendingRate, std::memory_order_relaxed); - AAMPLOG_INFO("All sources flushed - committed playback rate=%d", - pendingRate); - - // Restore the pre-flush state (PLAYING, PAUSED, or SOURCES_ATTACHED) - // before notifying WaitForFlushToComplete(), which unblocks as soon - // as the state leaves FLUSHING. - m_stateMachine.onFlushComplete(); - m_flushCv.notify_all(); - - // Issue play() if the restored state is PLAYING (seek-while-playing) - // or if Stream() was called while the flush was in progress - // (e.g. Stream() deferred from SOURCES_ATTACHED or a flush that - // preceded allSourcesAttached()). m_playRequested is reset here so - // it is not treated as stale on a subsequent flush cycle. - // seek-while-paused is correctly handled: Stream() is not called in - // that path so m_playRequested remains false and play() is not issued. - if (m_stateMachine.currentState() != PlayerStateId::PLAYING && - m_playRequested.load(std::memory_order_seq_cst)) - { - AAMPLOG_INFO("All sources flushed - issuing play() " - "(state=%s playRequested=%d)", - m_stateMachine.currentStateName(), - m_playRequested.load(std::memory_order_relaxed)); - m_playRequested.store(false, std::memory_order_relaxed); - bool async = false; - if (!m_pipeline->play(async)) - { - AAMPLOG_ERR("play() failed after flush"); - } - } - else - { - AAMPLOG_INFO("All sources flushed - not issuing play() (restored state=%s)", - m_stateMachine.currentStateName()); - } - } - } - else - { - AAMPLOG_WARN("unknown sourceId=%d - ignoring source-flushed notification", - sourceId); - } - - AAMPLOG_INFO("EXIT"); + // Flush() now uses pipeline-level setPosition(); flush completion is + // driven by PlaybackState::SEEK_DONE, not per-source SourceFlushedEvents. + // This callback should not be reached. + AAMPLOG_WARN("OnSourceFlushed called unexpectedly for sourceId=%d " + "- flush is driven by setPosition/SEEK_DONE", sourceId); } void AampRialtoPlayer::StartProgressTimer() From 2f9f1466add57eb0ebb7f409f1de8494c0e03e9a Mon Sep 17 00:00:00 2001 From: anshephe <115161257+anshephe@users.noreply.github.com> Date: Fri, 17 Jul 2026 10:18:17 +0000 Subject: [PATCH 17/19] Dispatch IStreamSinkNotifiable void callbacks via ScheduleAsyncTask MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit All void methods in PrivateInstanceAAMPNotifiable now post their work to the AAMP scheduler thread rather than calling PrivateInstanceAAMP directly from the Rialto callback thread. This eliminates the AB/BA deadlock where the AAMP main thread holds mLock or mStreamLock waiting for a Rialto call to return while the Rialto callback thread simultaneously tries to acquire the same lock through NotifyFirstFrameReceived, NotifyEOSReached, NotifySpeedChanged, etc. The two return-value methods (GetState, GetProgressReportIntervalSeconds) remain synchronous — GetState reads a std::atomic and the config read in GetProgressReportIntervalSeconds does not contend with Rialto locks. Test coverage: all 15 PrivateInstanceAAMPNotifiable tests pass. Tests updated to expect ScheduleAsyncTask calls and execute tasks inline to verify correct argument forwarding. New tests added for NotifyBufferUnderflow (both monitor-enabled and monitor-disabled paths) and SendMonitorAvEvent. --- .../PrivateInstanceAAMPNotifiable.cpp | 154 +++++++++++++++--- ...PrivateInstanceAAMPNotifiableTestCases.cpp | 141 ++++++++++++++-- 2 files changed, 263 insertions(+), 32 deletions(-) diff --git a/direct-rialto/PrivateInstanceAAMPNotifiable.cpp b/direct-rialto/PrivateInstanceAAMPNotifiable.cpp index 9a001e1935..b334f5cd6d 100644 --- a/direct-rialto/PrivateInstanceAAMPNotifiable.cpp +++ b/direct-rialto/PrivateInstanceAAMPNotifiable.cpp @@ -27,6 +27,67 @@ #include "AampLogManager.h" #include +// --------------------------------------------------------------------------- +// Parameter structs for async task dispatch. +// +// Each void IStreamSinkNotifiable method schedules its work on the AAMP +// scheduler thread via ScheduleAsyncTask rather than calling PrivateInstanceAAMP +// directly. This breaks the potential AB/BA deadlock where the Rialto +// callback thread holds a Rialto lock and tries to acquire an AAMP lock +// (mLock / mStreamLock) while the AAMP thread holds the same AAMP lock and +// waits for a Rialto call to return. +// +// Methods with extra parameters beyond the AAMP pointer use a heap-allocated +// struct carried through the void * arg. Methods with no extra parameters +// pass m_aamp directly as the arg. +// --------------------------------------------------------------------------- + +namespace { + +struct NotifyFirstFrameReceivedArgs +{ + PrivateInstanceAAMP *aamp; + unsigned long ccDecoderHandle; +}; + +struct NotifyFirstBufferProcessedArgs +{ + PrivateInstanceAAMP *aamp; + std::string videoRectangle; +}; + +struct MonitorProgressArgs +{ + PrivateInstanceAAMP *aamp; + bool sync; + bool beginningOfStream; +}; + +struct NotifySpeedChangedArgs +{ + PrivateInstanceAAMP *aamp; + float rate; + bool changeState; +}; + +struct NotifyBufferUnderflowArgs +{ + PrivateInstanceAAMP *aamp; + AampMediaType type; +}; + +struct SendMonitorAvEventArgs +{ + PrivateInstanceAAMP *aamp; + std::string status; + int64_t videoPositionMs; + int64_t audioPositionMs; + uint64_t timeInStateMs; + uint64_t droppedFrames; +}; + +} // namespace + PrivateInstanceAAMPNotifiable::PrivateInstanceAAMPNotifiable( PrivateInstanceAAMP *aamp) noexcept : m_aamp{aamp} @@ -44,45 +105,75 @@ void PrivateInstanceAAMPNotifiable::NotifyFirstFrameReceived( unsigned long ccDecoderHandle) { AAMPLOG_TRACE("ccDecoderHandle=%lu", ccDecoderHandle); - m_aamp->NotifyFirstFrameReceived(ccDecoderHandle); + auto *args = new NotifyFirstFrameReceivedArgs{m_aamp, ccDecoderHandle}; + m_aamp->ScheduleAsyncTask([](void *p) -> int { + auto *a = static_cast(p); + a->aamp->NotifyFirstFrameReceived(a->ccDecoderHandle); + delete a; + return 0; + }, args, "NotifyFirstFrameReceived"); } void PrivateInstanceAAMPNotifiable::NotifyFirstBufferProcessed( const std::string &videoRectangle) { AAMPLOG_TRACE("videoRectangle=%s", videoRectangle.c_str()); - m_aamp->NotifyFirstBufferProcessed(videoRectangle); + auto *args = new NotifyFirstBufferProcessedArgs{m_aamp, videoRectangle}; + m_aamp->ScheduleAsyncTask([](void *p) -> int { + auto *a = static_cast(p); + a->aamp->NotifyFirstBufferProcessed(a->videoRectangle); + delete a; + return 0; + }, args, "NotifyFirstBufferProcessed"); } void PrivateInstanceAAMPNotifiable::NotifyFirstVideoFrameDisplayed() { AAMPLOG_TRACE("NotifyFirstVideoFrameDisplayed"); - m_aamp->NotifyFirstVideoFrameDisplayed(); + m_aamp->ScheduleAsyncTask([](void *p) -> int { + static_cast(p)->NotifyFirstVideoFrameDisplayed(); + return 0; + }, m_aamp, "NotifyFirstVideoFrameDisplayed"); } void PrivateInstanceAAMPNotifiable::LogFirstFrame() { AAMPLOG_TRACE("LogFirstFrame"); - m_aamp->LogFirstFrame(); + m_aamp->ScheduleAsyncTask([](void *p) -> int { + static_cast(p)->LogFirstFrame(); + return 0; + }, m_aamp, "LogFirstFrame"); } void PrivateInstanceAAMPNotifiable::LogTuneComplete() { AAMPLOG_TRACE("LogTuneComplete"); - m_aamp->LogTuneComplete(); + m_aamp->ScheduleAsyncTask([](void *p) -> int { + static_cast(p)->LogTuneComplete(); + return 0; + }, m_aamp, "LogTuneComplete"); } void PrivateInstanceAAMPNotifiable::NotifyEOSReached() { AAMPLOG_TRACE("NotifyEOSReached"); - m_aamp->NotifyEOSReached(); + m_aamp->ScheduleAsyncTask([](void *p) -> int { + static_cast(p)->NotifyEOSReached(); + return 0; + }, m_aamp, "NotifyEOSReached"); } void PrivateInstanceAAMPNotifiable::MonitorProgress( bool sync, bool beginningOfStream) { AAMPLOG_TRACE("sync=%d beginningOfStream=%d", sync, beginningOfStream); - m_aamp->MonitorProgress(sync, beginningOfStream); + auto *args = new MonitorProgressArgs{m_aamp, sync, beginningOfStream}; + m_aamp->ScheduleAsyncTask([](void *p) -> int { + auto *a = static_cast(p); + a->aamp->MonitorProgress(a->sync, a->beginningOfStream); + delete a; + return 0; + }, args, "MonitorProgress"); } double PrivateInstanceAAMPNotifiable::GetProgressReportIntervalSeconds() @@ -105,7 +196,13 @@ void PrivateInstanceAAMPNotifiable::NotifySpeedChanged( float rate, bool changeState) { AAMPLOG_TRACE("rate=%f changeState=%d", rate, changeState); - m_aamp->NotifySpeedChanged(rate, changeState); + auto *args = new NotifySpeedChangedArgs{m_aamp, rate, changeState}; + m_aamp->ScheduleAsyncTask([](void *p) -> int { + auto *a = static_cast(p); + a->aamp->NotifySpeedChanged(a->rate, a->changeState); + delete a; + return 0; + }, args, "NotifySpeedChanged"); } AAMPPlayerState PrivateInstanceAAMPNotifiable::GetState() @@ -118,17 +215,23 @@ AAMPPlayerState PrivateInstanceAAMPNotifiable::GetState() void PrivateInstanceAAMPNotifiable::NotifyBufferUnderflow(AampMediaType type) { AAMPLOG_TRACE("type=%d", static_cast(type)); - if (m_aamp->mConfig->IsConfigSet(eAAMPConfig_EnableAampUnderflowMonitor)) - { - AAMPLOG_INFO( - "Underflow will be handled by AampUnderflowMonitor, " - "skipping retune for mediaType=%d", - static_cast(type)); - } - else - { - m_aamp->ScheduleRetune(eGST_ERROR_UNDERFLOW, type); - } + auto *args = new NotifyBufferUnderflowArgs{m_aamp, type}; + m_aamp->ScheduleAsyncTask([](void *p) -> int { + auto *a = static_cast(p); + if (a->aamp->mConfig->IsConfigSet(eAAMPConfig_EnableAampUnderflowMonitor)) + { + AAMPLOG_INFO( + "Underflow will be handled by AampUnderflowMonitor, " + "skipping retune for mediaType=%d", + static_cast(a->type)); + } + else + { + a->aamp->ScheduleRetune(eGST_ERROR_UNDERFLOW, a->type); + } + delete a; + return 0; + }, args, "NotifyBufferUnderflow"); } void PrivateInstanceAAMPNotifiable::SendMonitorAvEvent( @@ -142,6 +245,15 @@ void PrivateInstanceAAMPNotifiable::SendMonitorAvEvent( " timeInStateMs=%" PRIu64 " dropped=%" PRIu64, status.c_str(), videoPositionMs, audioPositionMs, timeInStateMs, droppedFrames); - m_aamp->SendMonitorAvEvent( - status, videoPositionMs, audioPositionMs, timeInStateMs, droppedFrames); + auto *args = new SendMonitorAvEventArgs{ + m_aamp, status, videoPositionMs, audioPositionMs, + timeInStateMs, droppedFrames}; + m_aamp->ScheduleAsyncTask([](void *p) -> int { + auto *a = static_cast(p); + a->aamp->SendMonitorAvEvent( + a->status, a->videoPositionMs, a->audioPositionMs, + a->timeInStateMs, a->droppedFrames); + delete a; + return 0; + }, args, "SendMonitorAvEvent"); } diff --git a/test/utests/tests/PrivateInstanceAAMPNotifiableTests/PrivateInstanceAAMPNotifiableTestCases.cpp b/test/utests/tests/PrivateInstanceAAMPNotifiableTests/PrivateInstanceAAMPNotifiableTestCases.cpp index ac5d02b17c..127d0969fa 100644 --- a/test/utests/tests/PrivateInstanceAAMPNotifiableTests/PrivateInstanceAAMPNotifiableTestCases.cpp +++ b/test/utests/tests/PrivateInstanceAAMPNotifiableTests/PrivateInstanceAAMPNotifiableTestCases.cpp @@ -79,8 +79,14 @@ TEST_F(PrivateInstanceAAMPNotifiableTest, } TEST_F(PrivateInstanceAAMPNotifiableTest, - NotifySpeedChanged_ForwardsRateAndChangeState) + NotifySpeedChanged_SchedulesTaskThatForwardsToAamp) { + EXPECT_CALL(*g_mockPrivateInstanceAAMP, + ScheduleAsyncTask(_, _, std::string("NotifySpeedChanged"))) + .WillOnce([](IdleTask task, void *arg, std::string) -> int { + task(arg); + return 0; + }); EXPECT_CALL(*g_mockPrivateInstanceAAMP, NotifySpeedChanged(2.0f, true)); m_notifiable->NotifySpeedChanged(2.0f, true); @@ -91,44 +97,85 @@ TEST_F(PrivateInstanceAAMPNotifiableTest, // =========================================================================== TEST_F(PrivateInstanceAAMPNotifiableTest, - NotifyFirstFrameReceived_ForwardsWithoutCrash) + NotifyFirstFrameReceived_SchedulesTaskThatCallsAamp) { - // NotifyFirstFrameReceived in the fake calls SetState internally; - // set up the mock to handle GetState/SetState calls. + // The fake's NotifyFirstFrameReceived calls SetState(PLAYING) when state + // is not IDLE and mFirstVideoFrameDisplayedEnabled is false (the default). EXPECT_CALL(*g_mockPrivateInstanceAAMP, GetState()) - .WillRepeatedly(Return(eSTATE_IDLE)); - EXPECT_CALL(*g_mockPrivateInstanceAAMP, SetState(_, _)).Times(testing::AnyNumber()); + .WillRepeatedly(Return(eSTATE_PLAYING)); + EXPECT_CALL(*g_mockPrivateInstanceAAMP, SetState(eSTATE_PLAYING, _)); + EXPECT_CALL(*g_mockPrivateInstanceAAMP, + ScheduleAsyncTask(_, _, std::string("NotifyFirstFrameReceived"))) + .WillOnce([](IdleTask task, void *arg, std::string) -> int { + task(arg); + return 0; + }); m_notifiable->NotifyFirstFrameReceived(42); } TEST_F(PrivateInstanceAAMPNotifiableTest, - NotifyFirstBufferProcessed_ForwardsWithoutCrash) + NotifyFirstBufferProcessed_SchedulesTask) { + EXPECT_CALL(*g_mockPrivateInstanceAAMP, + ScheduleAsyncTask(_, _, std::string("NotifyFirstBufferProcessed"))) + .WillOnce([](IdleTask task, void *arg, std::string) -> int { + task(arg); + return 0; + }); + m_notifiable->NotifyFirstBufferProcessed("0,0,1920,1080"); } TEST_F(PrivateInstanceAAMPNotifiableTest, - LogFirstFrame_ForwardsWithoutCrash) + LogFirstFrame_SchedulesTask) { + EXPECT_CALL(*g_mockPrivateInstanceAAMP, + ScheduleAsyncTask(_, _, std::string("LogFirstFrame"))) + .WillOnce([](IdleTask task, void *arg, std::string) -> int { + task(arg); + return 0; + }); + m_notifiable->LogFirstFrame(); } TEST_F(PrivateInstanceAAMPNotifiableTest, - LogTuneComplete_ForwardsWithoutCrash) + LogTuneComplete_SchedulesTask) { + EXPECT_CALL(*g_mockPrivateInstanceAAMP, + ScheduleAsyncTask(_, _, std::string("LogTuneComplete"))) + .WillOnce([](IdleTask task, void *arg, std::string) -> int { + task(arg); + return 0; + }); + m_notifiable->LogTuneComplete(); } TEST_F(PrivateInstanceAAMPNotifiableTest, - NotifyEOSReached_ForwardsWithoutCrash) + NotifyEOSReached_SchedulesTask) { + EXPECT_CALL(*g_mockPrivateInstanceAAMP, + ScheduleAsyncTask(_, _, std::string("NotifyEOSReached"))) + .WillOnce([](IdleTask task, void *arg, std::string) -> int { + task(arg); + return 0; + }); + m_notifiable->NotifyEOSReached(); } TEST_F(PrivateInstanceAAMPNotifiableTest, - MonitorProgress_ForwardsWithoutCrash) + MonitorProgress_SchedulesTask) { + EXPECT_CALL(*g_mockPrivateInstanceAAMP, + ScheduleAsyncTask(_, _, std::string("MonitorProgress"))) + .WillOnce([](IdleTask task, void *arg, std::string) -> int { + task(arg); + return 0; + }); + m_notifiable->MonitorProgress(true, false); } @@ -177,6 +224,78 @@ TEST_F(PrivateInstanceAAMPNotifiableTest, EXPECT_DOUBLE_EQ(m_notifiable->GetProgressReportIntervalSeconds(), 1.5); } +// =========================================================================== +// NotifyBufferUnderflow +// =========================================================================== + +TEST_F(PrivateInstanceAAMPNotifiableTest, + NotifyBufferUnderflow_SchedulesTaskThatCallsScheduleRetune) +{ + // Use a PrivateInstanceAAMP with a non-null config so the task lambda can + // call mConfig->IsConfigSet without crashing. + AampConfig config; + PrivateInstanceAAMP aampWithConfig(&config); + PrivateInstanceAAMPNotifiable notifiable(&aampWithConfig); + + // Underflow monitor disabled: ScheduleRetune should be invoked (no-op in fake). + EXPECT_CALL(*g_mockAampConfig, + IsConfigSet(eAAMPConfig_EnableAampUnderflowMonitor)) + .WillOnce(Return(false)); + EXPECT_CALL(*g_mockPrivateInstanceAAMP, + ScheduleAsyncTask(_, _, std::string("NotifyBufferUnderflow"))) + .WillOnce([](IdleTask task, void *arg, std::string) -> int { + task(arg); + return 0; + }); + + notifiable.NotifyBufferUnderflow(eMEDIATYPE_VIDEO); +} + +TEST_F(PrivateInstanceAAMPNotifiableTest, + NotifyBufferUnderflow_WhenMonitorEnabled_SchedulesTaskThatSkipsRetune) +{ + AampConfig config; + PrivateInstanceAAMP aampWithConfig(&config); + PrivateInstanceAAMPNotifiable notifiable(&aampWithConfig); + + // Underflow monitor enabled: ScheduleRetune must NOT be called. + EXPECT_CALL(*g_mockAampConfig, + IsConfigSet(eAAMPConfig_EnableAampUnderflowMonitor)) + .WillOnce(Return(true)); + EXPECT_CALL(*g_mockPrivateInstanceAAMP, + ScheduleAsyncTask(_, _, std::string("NotifyBufferUnderflow"))) + .WillOnce([](IdleTask task, void *arg, std::string) -> int { + task(arg); + return 0; + }); + // ScheduleRetune is a no-op fake; verify it is NOT reached by ensuring + // no unexpected calls surface (NiceMock handles this silently). + + notifiable.NotifyBufferUnderflow(eMEDIATYPE_AUDIO); +} + +// =========================================================================== +// SendMonitorAvEvent +// =========================================================================== + +TEST_F(PrivateInstanceAAMPNotifiableTest, + SendMonitorAvEvent_SchedulesTask) +{ + EXPECT_CALL(*g_mockPrivateInstanceAAMP, + ScheduleAsyncTask(_, _, std::string("SendMonitorAvEvent"))) + .WillOnce([](IdleTask task, void *arg, std::string) -> int { + task(arg); + return 0; + }); + + // The fake's SendMonitorAvEvent is a no-op; verify no crash and correct dispatch. + m_notifiable->SendMonitorAvEvent("ok", 1000, 999, 5000, 0); +} + +// =========================================================================== +// GetProgressReportIntervalSeconds +// =========================================================================== + TEST_F(PrivateInstanceAAMPNotifiableTest, GetProgressReportIntervalSeconds_WithNullConfig_ReturnsZero) { From c3b60bb52c8531ae0dbf1810f00ca16eede377dd Mon Sep 17 00:00:00 2001 From: anshephe <115161257+anshephe@users.noreply.github.com> Date: Fri, 17 Jul 2026 15:00:40 +0000 Subject: [PATCH 18/19] Extended the implementation of setPosition, to add the states SEEKING and SEEK_DONE. ALso clearing the playing and eos flags replicating a flush Changed addSegment and haveData, as the simulator was behaving as if the segment was ready to be processed on the call to addSegment which isn't correct. It is released to Rialto on the haveData call. Reworked the needData notification, to consider request_ids before a flush/setPosition to be stale and ignored. Also it seemed that it was sending all sources the next needData for each haveData response rather than limiting it to the specific source. These changes need further examination --- test/rialto/RialtoSimulator.cpp | 329 ++++++++++++++++++++++++-------- 1 file changed, 249 insertions(+), 80 deletions(-) diff --git a/test/rialto/RialtoSimulator.cpp b/test/rialto/RialtoSimulator.cpp index 60717bb685..486b891699 100644 --- a/test/rialto/RialtoSimulator.cpp +++ b/test/rialto/RialtoSimulator.cpp @@ -108,6 +108,7 @@ class SimMediaPipeline : public IMediaPipeline , m_basePositionNs(0) , m_basePositionSet(false) , m_needDataRequestId(1) + , m_generation(0) , m_stopRequested(false) , m_eosSourceCount(0) , m_eosNotified(false) @@ -243,9 +244,68 @@ class SimMediaPipeline : public IMediaPipeline bool setPosition(int64_t position) override { RIALTO_SIM_LOG("setPosition: position=%ld", static_cast(position)); + + // Real Rialto reports SEEKING as soon as the seek is accepted, i.e. + // before the flush/reposition work below has happened, then does + // that work, then reports SEEK_DONE once it has completed. Send + // SEEKING first so observers see the same ordering. + auto client = m_client.lock(); + if (client) + { + RIALTO_SIM_LOG("setPosition: notifying SEEKING"); + client->notifyPlaybackState(PlaybackState::SEEKING); + } + + // setPosition() models a pipeline-wide flushing seek: a real Rialto + // server treats it as an implicit Flush() of every source followed + // by setSourcePosition() to the new position. Reset the same + // play-readiness/EOS/backpressure state that flush() resets (see + // flush() above for why every source, not just one, must + // re-buffer), so a stale ready/EOS source from before the seek + // can't make maybeStartPlayback() fire PLAYING before the + // post-seek data has actually arrived. + m_eosSourceCount.store(0, std::memory_order_relaxed); + m_eosNotified.store(false, std::memory_order_relaxed); + m_playing = false; + { + std::lock_guard lock(m_trackMutex); + for (auto &entry : m_injectedDurationNs) + { + entry.second = 0; + } + m_readySources.clear(); + m_eosSources.clear(); + m_pendingSegments.clear(); + + // As with flush(), any needData request issued before this seek + // is now stale. Bump the generation and drop the outstanding + // request bookkeeping so a late haveData() for one of them is + // ignored instead of being applied to the post-seek state. + m_generation.fetch_add(1, std::memory_order_relaxed); + m_requestIdToSource.clear(); + } + + // Restart the needData pump immediately, as flush() does, instead of + // waiting on requests that have just been invalidated above. + if (m_allSourcesAttached) + { + startNeedDataPump(); + } + + // Apply the new position, as setSourcePosition() would after a flush. m_basePositionNs.store(position, std::memory_order_relaxed); m_basePositionSet.store(true, std::memory_order_relaxed); m_playStartTime = std::chrono::steady_clock::now(); + + // AampRialtoPlayer's Flush() blocks on SEEK_DONE (via its state + // machine) to restore state and commit the pending rate/position, + // so this notification must be sent for every setPosition() call — + // not just flush-initiated ones — or that wait never completes. + if (client) + { + RIALTO_SIM_LOG("setPosition: notifying SEEK_DONE"); + client->notifyPlaybackState(PlaybackState::SEEK_DONE); + } return true; } @@ -280,22 +340,77 @@ class SimMediaPipeline : public IMediaPipeline RIALTO_SIM_LOG("haveData: status=%d requestId=%u", static_cast(status), needDataRequestId); - // Resolve which source this response belongs to. + // Resolve which source this response belongs to, and pick up any + // segment data staged by addSegment() for this request. int32_t sourceId = -1; + bool requestKnown = false; + uint64_t requestGeneration = 0; + PendingSegmentData pending; + bool havePending = false; { std::lock_guard lock(m_trackMutex); auto it = m_requestIdToSource.find(needDataRequestId); if (it != m_requestIdToSource.end()) { - sourceId = it->second; + sourceId = it->second.sourceId; + requestGeneration = it->second.generation; + requestKnown = true; m_requestIdToSource.erase(it); } + auto pendingIt = m_pendingSegments.find(needDataRequestId); + if (pendingIt != m_pendingSegments.end()) + { + pending = pendingIt->second; + havePending = true; + m_pendingSegments.erase(pendingIt); + } + } + + // A flush()/setPosition() that happened after this request was issued + // bumps the generation and abandons every outstanding requestId: such + // a fresh pump has already been sent, so a (possibly late-arriving) + // response for the old requestId must not be applied to playback + // state or trigger another round of needData. Any segments staged + // against it were already discarded above. + if (!requestKnown || requestGeneration != m_generation.load(std::memory_order_relaxed)) + { + RIALTO_SIM_LOG("haveData: ignoring stale/unknown requestId=%u", + needDataRequestId); + return true; + } + + // Apply the effect of the segments addSegment() staged for this + // request now that the client has confirmed the request is + // complete. Real Rialto only considers a request's data delivered + // once haveData() is called for it, so the base-position update and + // the duration accounting that gates PLAYING must happen here, not + // eagerly in addSegment(). + if (havePending) + { + if (sourceId < 0) + { + sourceId = pending.sourceId; + } + if (pending.firstTimeStampNs >= 0 && + !m_basePositionSet.load(std::memory_order_relaxed)) + { + m_basePositionNs.store(pending.firstTimeStampNs, + std::memory_order_relaxed); + m_basePositionSet.store(true, std::memory_order_relaxed); + m_playStartTime = std::chrono::steady_clock::now(); + } + if (pending.sourceId >= 0 && pending.totalDurationNs > 0) + { + accumulateInjectedDuration(pending.sourceId, + pending.totalDurationNs); + } + maybeStartPlayback(); } - // Schedule the next needData even while paused: a real - // GStreamer/Rialto pipeline accepts data in PAUSED state (appSrc - // queues continue to buffer). Without this, an inject thread - // waiting in injectOneSample() for hasPending can block forever + // Schedule the next needData for this specific source even while + // paused: a real GStreamer/Rialto pipeline accepts data in PAUSED + // state (appSrc queues continue to buffer). Without this, an inject + // thread waiting in injectOneSample() for hasPending can block forever // after a seek arrives shortly after EOS, because StopInjectLoop // (called during TeardownStream) hangs waiting for inject threads // that will never exit, and Flush/invalidateGeneration is never @@ -303,7 +418,7 @@ class SimMediaPipeline : public IMediaPipeline if (status == MediaSourceStatus::OK && !m_stopRequested.load(std::memory_order_relaxed)) { - scheduleNextNeedData(); + scheduleNextNeedData(sourceId); } else if (status == MediaSourceStatus::EOS) { @@ -378,28 +493,29 @@ class SimMediaPipeline : public IMediaPipeline AddSegmentStatus addSegment(uint32_t needDataRequestId, const std::unique_ptr &mediaSegment) override { - // Capture the first segment PTS as the base position for - // simulated playback. Subsequent segments do NOT override — - // position advances by elapsed wall-clock time from this base. - if (mediaSegment && !m_basePositionSet.load(std::memory_order_relaxed)) + // Real Rialto does not consider a needData request's data delivered + // until the client calls haveData() for that requestId — addSegment() + // only stages the segment into the pipeline's shared buffer. Mirror + // that here: record the derived fields we need (source, accumulated + // duration, first PTS) against the requestId, and apply their effect + // on playback state in haveData() instead of immediately. The + // sample payload itself is never used by the simulator, so it is not + // stored. + if (mediaSegment) { - int64_t pts = mediaSegment->getTimeStamp(); - if (pts > 0) + std::lock_guard lock(m_trackMutex); + PendingSegmentData &pending = m_pendingSegments[needDataRequestId]; + pending.sourceId = mediaSegment->getId(); + pending.totalDurationNs += mediaSegment->getDuration(); + if (pending.firstTimeStampNs < 0) { - m_basePositionNs.store(pts, std::memory_order_relaxed); - m_basePositionSet.store(true, std::memory_order_relaxed); - m_playStartTime = std::chrono::steady_clock::now(); + int64_t pts = mediaSegment->getTimeStamp(); + if (pts > 0) + { + pending.firstTimeStampNs = pts; + } } } - - // Accumulate injected duration per source to gate the - // transition to PLAYING (subtitle tracks are ignored). - if (mediaSegment) - { - accumulateInjectedDuration(mediaSegment->getId(), - mediaSegment->getDuration()); - maybeStartPlayback(); - } return AddSegmentStatus::OK; } @@ -464,9 +580,27 @@ class SimMediaPipeline : public IMediaPipeline } m_readySources.clear(); m_eosSources.clear(); + m_pendingSegments.clear(); + + // Any needData request issued before this flush is now stale: a + // real Rialto server abandons in-flight requests on a flushing + // seek and issues fresh ones. Bump the generation and drop the + // bookkeeping for outstanding requests so a late haveData() for + // one of them is ignored rather than applied to (or triggering + // another data pump for) the post-flush state. + m_generation.fetch_add(1, std::memory_order_relaxed); + m_requestIdToSource.clear(); } m_playing = false; + // Restart the needData pump immediately so the client gets fresh + // requests rather than waiting on ones that have just been + // invalidated above. + if (m_allSourcesAttached) + { + startNeedDataPump(); + } + // Notify the client that the flush completed so AampRialtoPlayer // calls setSourcePosition (matching real Rialto server behavior). if (auto client = m_client.lock()) @@ -513,6 +647,25 @@ class SimMediaPipeline : public IMediaPipeline } private: + // Segment data staged by addSegment() for a given needDataRequestId, + // consumed by haveData() once the client confirms that request is + // complete. Only the derived fields needed to update playback state + // are kept — the sample payload itself is never used by the simulator. + struct PendingSegmentData + { + int32_t sourceId = -1; + int64_t totalDurationNs = 0; + int64_t firstTimeStampNs = -1; + }; + + // Bookkeeping for an outstanding notifyNeedMediaData() request, tagged + // with the pump generation it was issued under (see m_generation). + struct RequestInfo + { + int32_t sourceId = -1; + uint64_t generation = 0; + }; + int64_t getCurrentPositionNs() const { int64_t base = m_basePositionNs.load(std::memory_order_relaxed); @@ -527,47 +680,39 @@ class SimMediaPipeline : public IMediaPipeline return base + static_cast(elapsedNs); } - // Largest amount of injected-but-not-yet-played media held across the - // non-subtitle sources, in the pipeline (restamped) timebase. Used to - // model buffer-fill backpressure: injected duration accumulates as - // segments arrive, while playback drains it at 1x wall-clock time. - // Backpressure is only meaningful while the pipeline is PLAYING: during - // preroll/seek/flush the pipeline buffers freely to (re)reach the play - // threshold, so report no backpressure when not playing to avoid - // starving the pipeline (and deadlocking, since buffered would never - // drain while paused). + // Amount of injected-but-not-yet-played media held for a single + // non-subtitle source, in the pipeline (restamped) timebase. Used to + // model per-track buffer-fill backpressure: injected duration + // accumulates as segments arrive, while playback drains it at 1x + // wall-clock time. Backpressure is only meaningful while the pipeline + // is PLAYING: during preroll/seek/flush the pipeline buffers freely to + // (re)reach the play threshold, so report no backpressure when not + // playing to avoid starving the pipeline (and deadlocking, since + // buffered would never drain while paused). Subtitle sources are never + // gated (see kMinPlayDurationNs). // Caller must hold m_trackMutex. - int64_t maxBufferedAheadNsLocked() const + int64_t bufferedAheadNsLocked(int32_t sourceId) const { if (!m_playing || !m_basePositionSet.load(std::memory_order_relaxed)) { return 0; } - int64_t playedNs = 0; + auto typeIt = m_sourceTypes.find(sourceId); + if (typeIt == m_sourceTypes.end() || + typeIt->second == MediaSourceType::SUBTITLE) { - auto elapsed = std::chrono::steady_clock::now() - m_playStartTime; - playedNs = std::chrono::duration_cast( - elapsed).count(); + return 0; } - int64_t maxBuffered = 0; - for (const auto &entry : m_sourceTypes) + auto it = m_injectedDurationNs.find(sourceId); + if (it == m_injectedDurationNs.end()) { - if (entry.second == MediaSourceType::SUBTITLE) - { - continue; - } - auto it = m_injectedDurationNs.find(entry.first); - if (it == m_injectedDurationNs.end()) - { - continue; - } - int64_t buffered = it->second - playedNs; - if (buffered > maxBuffered) - { - maxBuffered = buffered; - } + return 0; } - return maxBuffered; + auto elapsed = std::chrono::steady_clock::now() - m_playStartTime; + int64_t playedNs = std::chrono::duration_cast( + elapsed).count(); + int64_t buffered = it->second - playedNs; + return buffered > 0 ? buffered : 0; } void startNeedDataPump() @@ -580,10 +725,11 @@ class SimMediaPipeline : public IMediaPipeline std::vector> sends; { std::lock_guard lock(m_trackMutex); + uint64_t generation = m_generation.load(std::memory_order_relaxed); for (int32_t sourceId : m_attachedSources) { uint32_t reqId = m_needDataRequestId++; - m_requestIdToSource[reqId] = sourceId; + m_requestIdToSource[reqId] = RequestInfo{sourceId, generation}; sends.emplace_back(sourceId, reqId); } } @@ -595,26 +741,41 @@ class SimMediaPipeline : public IMediaPipeline } } - void scheduleNextNeedData() + void scheduleNextNeedData(int32_t sourceId) { - std::thread([this]() { - // Pace data requests to model pipeline buffer backpressure: - // wait until the buffered (injected-but-not-played) media drops - // below the high-water mark before asking for more. This keeps - // injection at ~real time instead of draining AAMP's local TSB - // as fast as fragments can be produced. + const uint64_t expectedGeneration = m_generation.load(std::memory_order_relaxed); + std::thread([this, sourceId, expectedGeneration]() { + // Pace data requests to model per-track buffer backpressure: wait + // until this source's buffered (injected-but-not-played) media + // drops below the high-water mark before asking it for more. + // This keeps injection at ~real time instead of draining AAMP's + // local TSB as fast as fragments can be produced, and — since + // each source is paced independently — a fast source can't have + // its next request blocked on a slower sibling source. for (;;) { if (m_stopRequested.load(std::memory_order_relaxed)) { return; } + if (m_generation.load(std::memory_order_relaxed) != expectedGeneration) + { + // flush()/setPosition() has already restarted the pump + // with a fresh generation since this response was + // received; that pump already re-requested data for + // every source, so sending another round here would + // just create a duplicate outstanding request. + RIALTO_SIM_LOG("scheduleNextNeedData: aborting sourceId=%d - generation changed", + sourceId); + return; + } int64_t buffered = 0; { std::lock_guard lock(m_trackMutex); - buffered = maxBufferedAheadNsLocked(); + buffered = bufferedAheadNsLocked(sourceId); } - RIALTO_SIM_LOG("DBG scheduleNextNeedData buffered=%lld high=%lld playing=%d baseSet=%d", + RIALTO_SIM_LOG("DBG scheduleNextNeedData sourceId=%d buffered=%lld high=%lld playing=%d baseSet=%d", + sourceId, static_cast(buffered), static_cast(kBufferHighWaterNs), m_playing.load(std::memory_order_relaxed) ? 1 : 0, @@ -625,28 +786,31 @@ class SimMediaPipeline : public IMediaPipeline } std::this_thread::sleep_for(std::chrono::milliseconds(50)); } + + // Re-check right before sending: the pacing loop may have exited + // on the very iteration a flush()/setPosition() bumped the + // generation. + if (m_generation.load(std::memory_order_relaxed) != expectedGeneration) + { + return; + } + auto client = m_client.lock(); if (!client) { return; } - std::vector> sends; + uint32_t reqId; { std::lock_guard lock(m_trackMutex); - for (int32_t sourceId : m_attachedSources) - { - uint32_t reqId = m_needDataRequestId++; - m_requestIdToSource[reqId] = sourceId; - sends.emplace_back(sourceId, reqId); - } + reqId = m_needDataRequestId++; + m_requestIdToSource[reqId] = RequestInfo{sourceId, expectedGeneration}; } - for (const auto &send : sends) + if (!m_stopRequested.load(std::memory_order_relaxed)) { - if (m_stopRequested.load(std::memory_order_relaxed)) - { - break; - } - client->notifyNeedMediaData(send.first, kNeedDataFrameCount, send.second, nullptr); + RIALTO_SIM_LOG("notifyNeedMediaData: sourceId=%d requestId=%u (re-request)", + sourceId, reqId); + client->notifyNeedMediaData(sourceId, kNeedDataFrameCount, reqId, nullptr); } }).detach(); } @@ -830,6 +994,10 @@ class SimMediaPipeline : public IMediaPipeline std::atomic m_basePositionSet; std::chrono::steady_clock::time_point m_playStartTime; std::atomic m_needDataRequestId; + // Bumped whenever flush()/setPosition() abandons in-flight needData + // requests, so haveData() can recognise and ignore responses for + // requests issued under an earlier (now-stale) generation. + std::atomic m_generation; std::atomic m_stopRequested; std::atomic m_eosSourceCount; std::atomic m_eosNotified; @@ -840,7 +1008,8 @@ class SimMediaPipeline : public IMediaPipeline std::map m_injectedDurationNs; std::set m_readySources; std::set m_eosSources; - std::map m_requestIdToSource; + std::map m_requestIdToSource; + std::map m_pendingSegments; }; // =========================================================================== From 1a7573642a85f0dc9864396dbba84bd38d86a27e Mon Sep 17 00:00:00 2001 From: anshephe <115161257+anshephe@users.noreply.github.com> Date: Fri, 17 Jul 2026 17:24:31 +0000 Subject: [PATCH 19/19] Fix AampRialtoPlayer tests to match SEEK_DONE-driven flush completion MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Flush() was reworked to use a pipeline-level setPosition() + SEEK_DONE notification instead of per-source flush() + notifySourceFlushed(), but 11 tests (including 2 new ones added by this branch) still drove flush completion via the now-dead notifySourceFlushed()/OnSourceFlushed() path, so their EXPECT_CALL assertions on pipeline->flush()/setSourcePosition() were never satisfied. Rewired all affected tests to call PostPlaybackState(SEEK_DONE) and assert on setPosition()/setSourcePosition() per the new implementation, correcting two assertions that encoded the old (now incorrect) resetTime/rate semantics. Also added the state-machine onStop/onReconfigure test cases that were missing per-state coverage for the new individual overrides, and removed the now-unused PostSourceFlushed test helper. Files changed: - test/utests/tests/AampRialtoPlayerTests/AampRialtoPlayerTestCases.cpp — rewired 11 tests to the SEEK_DONE flow; removed dead helper - test/utests/tests/AampPlayerStateMachineTests/AampPlayerStateMachineTestCases.cpp — added 5 missing onStop/onReconfigure per-state tests --- .../AampPlayerStateMachineTestCases.cpp | 59 +++++ .../AampRialtoPlayerTestCases.cpp | 210 +++++++++--------- 2 files changed, 158 insertions(+), 111 deletions(-) diff --git a/test/utests/tests/AampPlayerStateMachineTests/AampPlayerStateMachineTestCases.cpp b/test/utests/tests/AampPlayerStateMachineTests/AampPlayerStateMachineTestCases.cpp index 0dfda9a564..ff8b8cc6bc 100644 --- a/test/utests/tests/AampPlayerStateMachineTests/AampPlayerStateMachineTestCases.cpp +++ b/test/utests/tests/AampPlayerStateMachineTests/AampPlayerStateMachineTestCases.cpp @@ -268,6 +268,18 @@ TEST_F(AampPlayerStateMachineTest, OnStop_FromSourcesAttaching_TransitionsToIdle EXPECT_EQ(m_sm.currentState(), PlayerStateId::IDLE); } +/** + * @test onStop from SOURCES_ATTACHED → IDLE. + */ +TEST_F(AampPlayerStateMachineTest, OnStop_FromSourcesAttached_TransitionsToIdle) +{ + m_sm.onPipelineLoaded(); + m_sm.onSourceAttaching(); + m_sm.onAllSourcesAttached(); + m_sm.onStop(); + EXPECT_EQ(m_sm.currentState(), PlayerStateId::IDLE); +} + /** * @test onStop from PLAYING → IDLE. */ @@ -418,6 +430,39 @@ TEST_F(AampPlayerStateMachineTest, OnReconfigure_FromIdle_StaysIdle) EXPECT_EQ(m_sm.currentState(), PlayerStateId::IDLE); } +/** + * @test onReconfigure from PIPELINE_CREATED → IDLE. + */ +TEST_F(AampPlayerStateMachineTest, OnReconfigure_FromPipelineCreated_TransitionsToIdle) +{ + m_sm.onPipelineLoaded(); + m_sm.onReconfigure(); + EXPECT_EQ(m_sm.currentState(), PlayerStateId::IDLE); +} + +/** + * @test onReconfigure from SOURCES_ATTACHING → IDLE. + */ +TEST_F(AampPlayerStateMachineTest, OnReconfigure_FromSourcesAttaching_TransitionsToIdle) +{ + m_sm.onPipelineLoaded(); + m_sm.onSourceAttaching(); + m_sm.onReconfigure(); + EXPECT_EQ(m_sm.currentState(), PlayerStateId::IDLE); +} + +/** + * @test onReconfigure from SOURCES_ATTACHED → IDLE. + */ +TEST_F(AampPlayerStateMachineTest, OnReconfigure_FromSourcesAttached_TransitionsToIdle) +{ + m_sm.onPipelineLoaded(); + m_sm.onSourceAttaching(); + m_sm.onAllSourcesAttached(); + m_sm.onReconfigure(); + EXPECT_EQ(m_sm.currentState(), PlayerStateId::IDLE); +} + /** * @test onReconfigure from PLAYING → IDLE. */ @@ -431,6 +476,20 @@ TEST_F(AampPlayerStateMachineTest, OnReconfigure_FromPlaying_TransitionsToIdle) EXPECT_EQ(m_sm.currentState(), PlayerStateId::IDLE); } +/** + * @test onReconfigure from PAUSED → IDLE. + */ +TEST_F(AampPlayerStateMachineTest, OnReconfigure_FromPaused_TransitionsToIdle) +{ + m_sm.onPipelineLoaded(); + m_sm.onSourceAttaching(); + m_sm.onAllSourcesAttached(); + m_sm.onPlaybackStarted(); + m_sm.onPlaybackPaused(); + m_sm.onReconfigure(); + EXPECT_EQ(m_sm.currentState(), PlayerStateId::IDLE); +} + /** * @test onReconfigure from ERROR → IDLE. */ diff --git a/test/utests/tests/AampRialtoPlayerTests/AampRialtoPlayerTestCases.cpp b/test/utests/tests/AampRialtoPlayerTests/AampRialtoPlayerTestCases.cpp index 228f6e2f5f..eddd6c4181 100644 --- a/test/utests/tests/AampRialtoPlayerTests/AampRialtoPlayerTestCases.cpp +++ b/test/utests/tests/AampRialtoPlayerTests/AampRialtoPlayerTestCases.cpp @@ -442,15 +442,6 @@ class AampRialtoPlayerTest : public ::testing::Test client->notifyBufferUnderflow(sourceId); } - /// Post a Rialto source-flushed notification via the captured client. - void PostSourceFlushed(int32_t sourceId) - { - auto client = m_capturedClient.lock(); - ASSERT_NE(client, nullptr) - << "Configure() must be called before PostSourceFlushed"; - client->notifySourceFlushed(sourceId); - } - /// Call Configure() with specific formats. /// Recreate m_mockPipeline with fresh default behaviours. /// Must be called before every player Configure() so the factory lambda @@ -974,12 +965,12 @@ TEST_F(AampRialtoPlayerWithDemuxTest, Stream_CallsPlay) } TEST_F(AampRialtoPlayerWithDemuxTest, - Stream_DeferredDuringFlush_OnSourceFlushedIssuesPlay) + Stream_DeferredDuringFlush_SeekDoneIssuesPlay) { /** * @brief Regression: when Stream() is called while a flush is in * progress, m_playRequested is set but play() is deferred. - * OnSourceFlushed() completing the flush cycle must issue + * The SEEK_DONE handler completing the flush cycle must issue * play() because m_playRequested is true, even though the * restored pre-flush state is SOURCES_ATTACHED (not PLAYING). */ @@ -990,23 +981,21 @@ TEST_F(AampRialtoPlayerWithDemuxTest, ASSERT_EQ(m_player->GetCurrentPlayerState(), PlayerStateId::SOURCES_ATTACHED); // Flush: state → FLUSHING, pre-flush state = SOURCES_ATTACHED. + EXPECT_CALL(*m_mockPipelinePtr, setPosition(_)).WillOnce(Return(true)); m_player->Flush(/*position=*/5.0, /*rate=*/1, /*shouldTearDown=*/false); ASSERT_EQ(m_player->GetCurrentPlayerState(), PlayerStateId::FLUSHING) << "Precondition: Flush() must move state to FLUSHING"; // Stream() while FLUSHING sets m_playRequested=true and returns early // without calling play(). play() must be issued exactly once — - // by OnSourceFlushed() once all sources confirm the flush. + // by the SEEK_DONE handler once the flush cycle completes. EXPECT_CALL(*m_mockPipelinePtr, play(_)).Times(1).WillOnce(Return(true)); m_player->Stream(); - // Simulate Rialto confirming the flush for all attached sources. - // PostSourceFlushed for the last source detects allSourcesFlushed, - // calls onFlushComplete() (restoring SOURCES_ATTACHED), then checks - // m_playRequested=true and issues play(). - PostSourceFlushed(/*sourceId=*/0); // video - PostSourceFlushed(/*sourceId=*/1); // inband CC subtitle - PostSourceFlushed(/*sourceId=*/2); // audio — triggers play() via m_playRequested + // Simulate Rialto confirming the pipeline-level flushing seek completed. + // OnPlaybackState(SEEK_DONE) calls onFlushComplete() (restoring + // SOURCES_ATTACHED), then checks m_playRequested=true and issues play(). + PostPlaybackState(firebolt::rialto::PlaybackState::SEEK_DONE); } TEST_F(AampRialtoPlayerWithDemuxTest, @@ -1015,7 +1004,7 @@ TEST_F(AampRialtoPlayerWithDemuxTest, /** * @brief After Stream() successfully issues play(), m_playRequested is * reset to false. A subsequent seek-while-paused flush must not - * spuriously re-issue play() when OnSourceFlushed restores PAUSED. + * spuriously re-issue play() when SEEK_DONE restores PAUSED. */ Configure(); SendVideoInitFragment(); @@ -1034,15 +1023,14 @@ TEST_F(AampRialtoPlayerWithDemuxTest, ASSERT_EQ(m_player->GetCurrentPlayerState(), PlayerStateId::PAUSED); // Seek-while-paused: Flush() → FLUSHING, pre-flush state = PAUSED. + EXPECT_CALL(*m_mockPipelinePtr, setPosition(_)).WillOnce(Return(true)); m_player->Flush(/*position=*/10.0, /*rate=*/1, /*shouldTearDown=*/false); ASSERT_EQ(m_player->GetCurrentPlayerState(), PlayerStateId::FLUSHING); // play() must NOT be called: m_playRequested was reset when Stream() // issued play(), and the restored post-flush state is PAUSED. EXPECT_CALL(*m_mockPipelinePtr, play(_)).Times(0); - PostSourceFlushed(/*sourceId=*/0); // video - PostSourceFlushed(/*sourceId=*/1); // inband CC subtitle - PostSourceFlushed(/*sourceId=*/2); // audio — triggers onFlushComplete() → PAUSED + PostPlaybackState(firebolt::rialto::PlaybackState::SEEK_DONE); // triggers onFlushComplete() → PAUSED EXPECT_EQ(m_player->GetCurrentPlayerState(), PlayerStateId::PAUSED) << "Seek-while-paused flush must restore PAUSED, not resume play"; @@ -1248,19 +1236,24 @@ TEST_F(AampRialtoPlayerWithDemuxTest, // =========================================================================== TEST_F(AampRialtoPlayerWithDemuxTest, - Flush_CallsPipelineFlushForEachAttachedSource) + Flush_CallsPipelineSetPositionAtFlushPosition) { + /** + * @brief Flush() now performs a pipeline-level flushing seek via + * setPosition() instead of the old per-source flush() IPC. + * position=5.0s -> 5,000,000,000ns. + */ Configure(); SendVideoInitFragment(); SendAudioInitFragment(); - // Source IDs: video=0, inband CC subtitle=1 (deferred, drained with - // video in SendVideoInitFragment), audio=2. - EXPECT_CALL(*m_mockPipelinePtr, flush(0, true, _)).Times(1); // video - EXPECT_CALL(*m_mockPipelinePtr, flush(1, true, _)).Times(1); // CC subtitle - EXPECT_CALL(*m_mockPipelinePtr, flush(2, true, _)).Times(1); // audio + EXPECT_CALL(*m_mockPipelinePtr, flush(_, _, _)).Times(0); + EXPECT_CALL(*m_mockPipelinePtr, setPosition(5'000'000'000LL)) + .WillOnce(Return(true)); m_player->Flush(5.0, 1, false); + + EXPECT_EQ(m_player->GetCurrentPlayerState(), PlayerStateId::FLUSHING); } TEST_F(AampRialtoPlayerTest, @@ -3014,14 +3007,14 @@ TEST_F(AampRialtoPlayerWithDemuxTest, /** * @brief Regression: Bug A from L2 TESTDATA0 rewind failure. * - * The sequence Flush(shouldTearDown=false) → OnSourceFlushed (all) → + * The sequence Flush(shouldTearDown=false) → SEEK_DONE → * Configure(audio=INVALID) → Rialto PLAYING previously left the state * machine stuck in FLUSHING when the Configure() did not rebuild the * pipeline. * - * Fix: onFlushComplete() is now called from OnSourceFlushed() once all - * sources confirm flushed. It restores the pre-flush state - * (SOURCES_ATTACHED in this case) so the pipeline reuse path in + * Fix: onFlushComplete() is now called from the SEEK_DONE handler once + * the pipeline-level flushing seek completes. It restores the pre-flush + * state (SOURCES_ATTACHED in this case) so the pipeline reuse path in * Configure() finds a non-FLUSHING state and, crucially, the subsequent * Rialto PLAYING notification transitions SOURCES_ATTACHED → PLAYING via * the normal SourcesAttachedState::onPlaybackStarted() path. @@ -3042,23 +3035,21 @@ TEST_F(AampRialtoPlayerWithDemuxTest, // Move state machine to FLUSHING via a flush without teardown. // shouldTearDown=false allows flush to proceed even when not in PLAYING/PAUSED. + EXPECT_CALL(*m_mockPipelinePtr, setPosition(_)).WillOnce(Return(true)); m_player->Flush(/*position=*/0.0, /*rate=*/-2, /*shouldTearDown=*/false); ASSERT_EQ(m_player->GetCurrentPlayerState(), PlayerStateId::FLUSHING) << "Precondition: Flush(shouldTearDown=false) must move state to FLUSHING"; - // Simulate Rialto confirming the flush for all attached sources. - // The last OnSourceFlushed call triggers onFlushComplete(), which - // restores the pre-flush state (SOURCES_ATTACHED) and notifies - // WaitForFlushToComplete() so Configure() can proceed. - EXPECT_CALL(*m_mockPipelinePtr, - setSourcePosition(_, _, /*resetTime=*/true, _, _)) - .Times(3) - .WillRepeatedly(Return(true)); - PostSourceFlushed(/*sourceId=*/0); // video - PostSourceFlushed(/*sourceId=*/1); // inband CC subtitle - PostSourceFlushed(/*sourceId=*/2); // audio — triggers onFlushComplete() + // Simulate Rialto confirming the pipeline-level flushing seek completed. + // This fixture does not stub isVideoMaster(), so computeAppliedRate() + // falls back to 1.0 (normal) and the SEEK_DONE handler does not issue the + // extra video-source setSourcePosition() call. onFlushComplete() still + // restores the pre-flush state and notifies WaitForFlushToComplete() so + // Configure() can proceed. + EXPECT_CALL(*m_mockPipelinePtr, setSourcePosition(_, _, _, _, _)).Times(0); + PostPlaybackState(firebolt::rialto::PlaybackState::SEEK_DONE); - // After all sources have flushed, onFlushComplete() restores the + // After the flushing seek completes, onFlushComplete() restores the // pre-flush state. The pre-flush state was SOURCES_ATTACHED. EXPECT_EQ(m_player->GetCurrentPlayerState(), PlayerStateId::SOURCES_ATTACHED) << "onFlushComplete() must restore pre-flush state SOURCES_ATTACHED"; @@ -3216,10 +3207,11 @@ TEST_F(AampRialtoPlayerWithDemuxTest, ASSERT_EQ(m_player->GetCurrentPlayerState(), PlayerStateId::PLAYING) << "Precondition: player must be in PLAYING state"; - // Player is in PLAYING state; expect normal flush() calls, NOT stop(). + // Player is in PLAYING state; expect the pipeline-level flushing seek, + // NOT stop() and NOT the old per-source flush() IPC. EXPECT_CALL(*m_mockPipelinePtr, stop()).Times(0); - EXPECT_CALL(*m_mockPipelinePtr, flush(_, _, _)) - .Times(::testing::AtLeast(1)); + EXPECT_CALL(*m_mockPipelinePtr, flush(_, _, _)).Times(0); + EXPECT_CALL(*m_mockPipelinePtr, setPosition(_)).WillOnce(Return(true)); m_player->Flush(/*position=*/20.0, /*rate=*/1, /*shouldTearDown=*/true); @@ -3271,10 +3263,11 @@ TEST_F(AampRialtoPlayerWithDemuxTest, ASSERT_EQ(m_player->GetCurrentPlayerState(), PlayerStateId::PLAYING); - // Player is in PLAYING state; expect normal flush() calls, NOT stop(). + // Player is in PLAYING state; expect the pipeline-level flushing seek, + // NOT stop() and NOT the old per-source flush() IPC. EXPECT_CALL(*m_mockPipelinePtr, stop()).Times(0); - EXPECT_CALL(*m_mockPipelinePtr, flush(_, _, _)) - .Times(::testing::AtLeast(1)); + EXPECT_CALL(*m_mockPipelinePtr, flush(_, _, _)).Times(0); + EXPECT_CALL(*m_mockPipelinePtr, setPosition(_)).WillOnce(Return(true)); m_player->Flush(/*position=*/15.0, /*rate=*/1, /*shouldTearDown=*/false); @@ -3288,13 +3281,13 @@ static void SetupCapabilities( bool videoMaster); TEST_F(AampRialtoPlayerTest, - Flush_SequentialFlushes_SecondFlushIssuesNewPipelineFlushWithNewPositionAndRate) + Flush_SequentialFlushes_SecondFlushIssuesNewPipelineSetPositionWithNewPositionAndRate) { /** * @brief Flush() calls WaitForFlushToComplete() at entry, so a second - * Flush() issued after all sources confirm flushed from the first - * cycle issues a fresh pipeline flush IPC with its own position - * and rate. + * Flush() issued after SEEK_DONE completes the first cycle issues + * a fresh pipeline-level setPosition() with its own position and + * rate. * * This replaces the old re-entrant-flush test. With the blocking design * there is no re-entrant path: the second Flush() simply waits for the @@ -3303,33 +3296,27 @@ TEST_F(AampRialtoPlayerTest, Configure(FORMAT_ISO_BMFF, FORMAT_INVALID); m_player->SetStreamCaps(eMEDIATYPE_VIDEO, MakeVideoH264CodecInfo()); - // First flush: two sources (video=0, subtitle=1). - EXPECT_CALL(*m_mockPipelinePtr, flush(_, _, _)).Times(2); + // First flush: pipeline-level seek to the requested position. + EXPECT_CALL(*m_mockPipelinePtr, setPosition(10'000'000'000LL)) + .WillOnce(Return(true)); m_player->Flush(/*position=*/10.0, /*rate=*/2, /*shouldTearDown=*/false); ASSERT_EQ(m_player->GetCurrentPlayerState(), PlayerStateId::FLUSHING); - // Complete first flush cycle: both sources report flushed. - ON_CALL(*m_mockPipelinePtr, setSourcePosition(_, _, _, _, _)) - .WillByDefault(Return(true)); - PostSourceFlushed(/*sourceId=*/0); - PostSourceFlushed(/*sourceId=*/1); // onFlushComplete() restores pre-flush state - + // Complete first flush cycle via SEEK_DONE. Capabilities are not + // stubbed here, so computeAppliedRate() falls back to 1.0 (normal) and + // no extra setSourcePosition() call is issued. + PostPlaybackState(firebolt::rialto::PlaybackState::SEEK_DONE); ASSERT_NE(m_player->GetCurrentPlayerState(), PlayerStateId::FLUSHING) << "First flush cycle must be complete before second Flush() call"; - // Second flush: fresh IPC to all attached sources with new position. - EXPECT_CALL(*m_mockPipelinePtr, flush(_, _, _)).Times(2); + // Second flush: fresh pipeline-level seek with the new position/rate. + EXPECT_CALL(*m_mockPipelinePtr, setPosition(33'000'000'000LL)) + .WillOnce(Return(true)); m_player->Flush(/*position=*/33.0, /*rate=*/-4, /*shouldTearDown=*/false); ASSERT_EQ(m_player->GetCurrentPlayerState(), PlayerStateId::FLUSHING); - // Complete second flush cycle; setSourcePosition must use the new position. - EXPECT_CALL(*m_mockPipelinePtr, - setSourcePosition(_, testing::Ge(33'000'000'000LL), - /*resetTime=*/true, _, _)) - .Times(2) - .WillRepeatedly(Return(true)); - PostSourceFlushed(/*sourceId=*/0); - PostSourceFlushed(/*sourceId=*/1); + // Complete second flush cycle. + PostPlaybackState(firebolt::rialto::PlaybackState::SEEK_DONE); // GetPositionMilliseconds() must use rate from the second flush (-4). ON_CALL(*m_mockSources[eMEDIATYPE_VIDEO], firstPtsMs()) @@ -3340,11 +3327,12 @@ TEST_F(AampRialtoPlayerTest, } TEST_F(AampRialtoPlayerWithDemuxTest, - Flush_MultiSource_CommitsRateAfterAllSourcesFlushed) + Flush_CommitsRateOnlyAfterSeekDone) { /** - * @brief Flush() stages pending rate immediately, but active playback rate - * must change only after every attached source reports flushed. + * @brief Flush() stages the pending rate immediately, but the active + * playback rate must change only once SEEK_DONE confirms the + * pipeline-level flushing seek completed - not while FLUSHING. */ Configure(); SendVideoInitFragment(); @@ -3358,27 +3346,20 @@ TEST_F(AampRialtoPlayerWithDemuxTest, SetupCapabilities(m_mockCapabilitiesFactory, /*querySucceeds=*/true, /*videoMaster=*/false); - m_player->Flush(/*position=*/12.0, /*rate=*/-4, /*shouldTearDown=*/false); - ON_CALL(*m_mockPipelinePtr, getPosition(_)) .WillByDefault(DoAll(SetArgReferee<0>(500'000'000LL), Return(true))); ON_CALL(*m_mockSources[eMEDIATYPE_VIDEO], firstPtsMs()) .WillByDefault(Return(0LL)); - // Configure() attaches three sources: video (id=0), inband CC subtitle - // (id=1), and audio (id=2). Active rate must not commit until every - // source has reported flushed. - - // Video flushed -> rate still uncommitted (subtitle + audio pending). - PostSourceFlushed(/*sourceId=*/0); - EXPECT_EQ(m_player->GetPositionMilliseconds(), 500LL); + EXPECT_CALL(*m_mockPipelinePtr, setPosition(_)).WillOnce(Return(true)); + m_player->Flush(/*position=*/12.0, /*rate=*/-4, /*shouldTearDown=*/false); + ASSERT_EQ(m_player->GetCurrentPlayerState(), PlayerStateId::FLUSHING); - // Inband CC subtitle flushed -> rate still uncommitted (audio pending). - PostSourceFlushed(/*sourceId=*/1); + // Still FLUSHING: the pending rate (-4) must not yet be active. EXPECT_EQ(m_player->GetPositionMilliseconds(), 500LL); - // Audio flushed (last source) -> pending rate commits to active rate. - PostSourceFlushed(/*sourceId=*/2); + // SEEK_DONE commits the pending rate as a single event. + PostPlaybackState(firebolt::rialto::PlaybackState::SEEK_DONE); EXPECT_EQ(m_player->GetPositionMilliseconds(), -2000LL); } @@ -3512,53 +3493,62 @@ TEST_F(AampRialtoPlayerTest, } TEST_F(AampRialtoPlayerTest, - OnSourceFlushed_FlushAtRate2_NotVideoMaster_UsesStoredRateInSetSourcePosition) + SeekDone_FlushAtRate2_NotVideoMaster_UsesStoredRateInSetSourcePosition) { /** - * @brief OnSourceFlushed must forward appliedRate == stored rate when the - * platform reports isVideoMaster==false. + * @brief SEEK_DONE must forward appliedRate == stored rate to the video + * source's setSourcePosition() when the platform reports + * isVideoMaster==false. */ SetupCapabilities(m_mockCapabilitiesFactory, /*querySucceeds=*/true, /*videoMaster=*/false); Configure(FORMAT_ISO_BMFF, FORMAT_INVALID); - // Attach source first so Flush() can mark it flushing. + // Attach source first so the video source exists for setSourcePosition(). m_player->SetStreamCaps(eMEDIATYPE_VIDEO, MakeVideoH264CodecInfo()); + EXPECT_CALL(*m_mockPipelinePtr, setPosition(_)).WillOnce(Return(true)); m_player->Flush(10.0, 2, /*shouldTearDown=*/false); + ASSERT_EQ(m_player->GetCurrentPlayerState(), PlayerStateId::FLUSHING); + // appliedRate=2.0 != normal, so the SEEK_DONE handler issues the extra + // video-source setSourcePosition() call (resetTime=false - the + // pipeline-level setPosition() already flushed the source buffers). EXPECT_CALL(*m_mockPipelinePtr, setSourcePosition(_, testing::Ge(10'000'000'000LL), - /*resetTime=*/true, 2.0, _)) + /*resetTime=*/false, 2.0, _)) .WillOnce(Return(true)); - PostSourceFlushed(/*sourceId=*/0); + PostPlaybackState(firebolt::rialto::PlaybackState::SEEK_DONE); } TEST_F(AampRialtoPlayerTest, - OnSourceFlushed_FlushAtRate2_VideoMaster_UsesRate1InSetSourcePosition) + SeekDone_FlushAtRate2_VideoMaster_SkipsSetSourcePosition) { /** - * @brief OnSourceFlushed must forward appliedRate == 1.0 when the platform - * reports isVideoMaster==true. + * @brief When the platform reports isVideoMaster==true, computeAppliedRate() + * collapses to AAMP_NORMAL_PLAY_RATE (1.0), so the SEEK_DONE handler must + * NOT issue the extra setSourcePosition() call - the pipeline-level + * setPosition() already established the correct (normal-rate) segment. */ SetupCapabilities(m_mockCapabilitiesFactory, /*querySucceeds=*/true, /*videoMaster=*/true); Configure(FORMAT_ISO_BMFF, FORMAT_INVALID); - // Attach source first so Flush() can mark it flushing. + // Attach source first so the video source exists for setSourcePosition(). m_player->SetStreamCaps(eMEDIATYPE_VIDEO, MakeVideoH264CodecInfo()); + EXPECT_CALL(*m_mockPipelinePtr, setPosition(_)).WillOnce(Return(true)); m_player->Flush(10.0, 2, /*shouldTearDown=*/false); + ASSERT_EQ(m_player->GetCurrentPlayerState(), PlayerStateId::FLUSHING); - EXPECT_CALL(*m_mockPipelinePtr, - setSourcePosition(_, testing::Ge(10'000'000'000LL), - /*resetTime=*/true, 1.0, _)) - .WillOnce(Return(true)); + EXPECT_CALL(*m_mockPipelinePtr, setSourcePosition(_, _, _, _, _)).Times(0); + + PostPlaybackState(firebolt::rialto::PlaybackState::SEEK_DONE); - PostSourceFlushed(/*sourceId=*/0); + EXPECT_EQ(m_player->GetCurrentPlayerState(), PlayerStateId::SOURCES_ATTACHED); } // =========================================================================== @@ -3847,10 +3837,11 @@ TEST_F(AampRialtoPlayerWithDemuxTest, ASSERT_EQ(m_player->GetCurrentPlayerState(), PlayerStateId::PLAYING); // Flush at trickplay rate — moves to FLUSHING and sets m_pendingFlushRate. + EXPECT_CALL(*m_mockPipelinePtr, setPosition(_)).WillOnce(Return(true)); m_player->Flush(10.0, 4, /*shouldTearDown=*/false); ASSERT_EQ(m_player->GetCurrentPlayerState(), PlayerStateId::FLUSHING); - // At this point m_rate has NOT been committed yet (sources are flushing). + // At this point m_rate has NOT been committed yet (awaiting SEEK_DONE). // Launch Configure() on a background thread; it should block in // WaitForFlushToComplete(). std::atomic configureFinished{false}; @@ -3874,13 +3865,10 @@ TEST_F(AampRialtoPlayerWithDemuxTest, std::this_thread::yield(); } EXPECT_FALSE(configureFinished.load(std::memory_order_acquire)) - << "Configure() must block while sources are still flushing"; + << "Configure() must block while the flush is still in progress"; - // Complete the flush cycle by posting SourceFlushed for all sources. - // Video source has sourceId=0, CC subtitle=1, audio=2. - PostSourceFlushed(0); - PostSourceFlushed(1); - PostSourceFlushed(2); + // Complete the flush cycle by posting SEEK_DONE. + PostPlaybackState(firebolt::rialto::PlaybackState::SEEK_DONE); // Configure() should now unblock. configureThread.join();