From 8e782cde3169a24a92dc42df2b8a00f5cdc68285 Mon Sep 17 00:00:00 2001 From: anshephe <115161257+anshephe@users.noreply.github.com> Date: Fri, 7 Aug 2026 14:22:39 +0000 Subject: [PATCH 1/3] Add InvalidateHandle() to decouple CC handle lifetime from refcount PlayerCCManagerBase::GetId()/Release() implements a usage-counter pattern that is correct for PlayerSubtecCCManager's shared, out-of-process connection, but was reused as-is for the Rialto and direct-Rialto backends, where the "handle" is actually a per-pipeline object owned by a single playback session. In multi-pipeline mode this can leave a dangling handle in the singleton after its owning session is destroyed, since the refcount only tracks session count, not the validity of a specific handle. - Add PlayerCCManagerBase::InvalidateHandle(void*), a no-op by default, that a handle owner can call to clear its handle from the singleton ahead of Release(), independent of the GetId()/Release() count. - Add PlayerCCManager::HasInstance() so callers can check for an existing singleton without creating one. - Override InvalidateHandle() in PlayerDirectRialtoCCManager and PlayerRialtoCCManager to clear m_control/mSubtitleControlHandle only when it matches the given handle. This is a minimal mitigation (Option A); wiring InvalidateHandle() into PlayerRialtoCCManager's GStreamer-side handle owner, and a larger structural fix (shared preferences singleton + per-session state), are tracked separately as follow-up work. --- closedcaptions/PlayerCCManager.cpp | 8 ++++++++ closedcaptions/PlayerCCManager.h | 20 +++++++++++++++++++ .../PlayerDirectRialtoCCManager.cpp | 11 ++++++++++ .../PlayerDirectRialtoCCManager.h | 3 +++ .../rialto/PlayerRialtoCCManager.cpp | 13 ++++++++++++ closedcaptions/rialto/PlayerRialtoCCManager.h | 3 +++ 6 files changed, 58 insertions(+) diff --git a/closedcaptions/PlayerCCManager.cpp b/closedcaptions/PlayerCCManager.cpp index bfd0f689..ef09609c 100644 --- a/closedcaptions/PlayerCCManager.cpp +++ b/closedcaptions/PlayerCCManager.cpp @@ -860,6 +860,14 @@ PlayerCCManagerBase *PlayerCCManager::GetInstance() return mInstance; } +/** + * @brief Check whether the singleton has already been created + */ +bool PlayerCCManager::HasInstance() +{ + return mInstance != NULL; +} + /** * @brief Reset the state. */ diff --git a/closedcaptions/PlayerCCManager.h b/closedcaptions/PlayerCCManager.h index af4683cb..28690413 100644 --- a/closedcaptions/PlayerCCManager.h +++ b/closedcaptions/PlayerCCManager.h @@ -71,6 +71,17 @@ class PlayerCCManagerBase */ virtual void Release(int iID) = 0; + /** + * @brief Clear the stored control handle if it currently equals handle. + * Called by the handle owner's destructor so a handle can never be + * used after the object it points to is freed, independent of + * whether the GetId()/Release() usage count has reached zero (it + * may not have, if another session is still registered - see + * multi-pipeline mode). + * @param[in] handle - the handle being invalidated + */ + virtual void InvalidateHandle(void *handle) {} + /** * @fn SetStatus * @@ -274,6 +285,15 @@ class PlayerCCManager */ static PlayerCCManagerBase * GetInstance(); + /** + * @fn HasInstance + * @brief Check whether GetInstance() has already created the singleton, + * without creating it as a side effect. + * + * @return bool - true if an instance exists + */ + static bool HasInstance(); + /** * @fn SetRialto * @brief Configure which CC manager subclass GetInstance() will create. diff --git a/closedcaptions/direct-rialto/PlayerDirectRialtoCCManager.cpp b/closedcaptions/direct-rialto/PlayerDirectRialtoCCManager.cpp index 35e73268..6cd7a92a 100755 --- a/closedcaptions/direct-rialto/PlayerDirectRialtoCCManager.cpp +++ b/closedcaptions/direct-rialto/PlayerDirectRialtoCCManager.cpp @@ -118,6 +118,17 @@ void PlayerDirectRialtoCCManager::Release(int id) } } +void PlayerDirectRialtoCCManager::InvalidateHandle(void *handle) +{ + std::lock_guard lock(m_idLock); + if (handle != nullptr && handle == static_cast(m_control)) + { + MW_LOG_WARN("handle=%p invalidated ahead of Release()", handle); + m_control = nullptr; + } +} + + int PlayerDirectRialtoCCManager::SetTrack( const std::string &track, CCFormat format) { diff --git a/closedcaptions/direct-rialto/PlayerDirectRialtoCCManager.h b/closedcaptions/direct-rialto/PlayerDirectRialtoCCManager.h index 936f1f50..cc61da76 100755 --- a/closedcaptions/direct-rialto/PlayerDirectRialtoCCManager.h +++ b/closedcaptions/direct-rialto/PlayerDirectRialtoCCManager.h @@ -63,6 +63,9 @@ class PlayerDirectRialtoCCManager : public PlayerCCManagerBase /// @copydoc PlayerCCManagerBase::Release void Release(int iID) override; + /// @copydoc PlayerCCManagerBase::InvalidateHandle + void InvalidateHandle(void *handle) override; + /// @copydoc PlayerCCManagerBase::SetTrack int SetTrack(const std::string &track, CCFormat format = eCLOSEDCAPTION_FORMAT_DEFAULT) override; diff --git a/closedcaptions/rialto/PlayerRialtoCCManager.cpp b/closedcaptions/rialto/PlayerRialtoCCManager.cpp index db48d6d9..5fbb57ba 100644 --- a/closedcaptions/rialto/PlayerRialtoCCManager.cpp +++ b/closedcaptions/rialto/PlayerRialtoCCManager.cpp @@ -105,6 +105,19 @@ void PlayerRialtoCCManager::Release(int id) return; } +/** + * @brief Clear mSubtitleControlHandle if it currently equals handle + */ +void PlayerRialtoCCManager::InvalidateHandle(void *handle) +{ + std::lock_guard lock(mIdLock); + if (handle != nullptr && handle == mSubtitleControlHandle) + { + MW_LOG_WARN("PlayerRialtoCCManager::handle:%p invalidated ahead of Release()", handle); + mSubtitleControlHandle = nullptr; + } +} + /** * @brief Set CC track */ diff --git a/closedcaptions/rialto/PlayerRialtoCCManager.h b/closedcaptions/rialto/PlayerRialtoCCManager.h index bbbd9882..9bf6374b 100644 --- a/closedcaptions/rialto/PlayerRialtoCCManager.h +++ b/closedcaptions/rialto/PlayerRialtoCCManager.h @@ -48,6 +48,9 @@ class PlayerRialtoCCManager : public PlayerCCManagerBase */ void Release(int iID) override; + /// @copydoc PlayerCCManagerBase::InvalidateHandle + void InvalidateHandle(void *handle) override; + /** * @fn GetId * @return int - unique ID From d5aebe3fd3f1af0450c8043efa69185a6c4d1b4f Mon Sep 17 00:00:00 2001 From: anshephe <115161257+anshephe@users.noreply.github.com> Date: Fri, 7 Aug 2026 14:51:36 +0000 Subject: [PATCH 2/3] Added missing header --- test/utests/tests/ClosedCaptionsTests/CMakeLists.txt | 1 + 1 file changed, 1 insertion(+) diff --git a/test/utests/tests/ClosedCaptionsTests/CMakeLists.txt b/test/utests/tests/ClosedCaptionsTests/CMakeLists.txt index 296294f6..e7b23601 100644 --- a/test/utests/tests/ClosedCaptionsTests/CMakeLists.txt +++ b/test/utests/tests/ClosedCaptionsTests/CMakeLists.txt @@ -30,6 +30,7 @@ include_directories(${PLAYER_ROOT}/playerJsonObject) include_directories(${PLAYER_ROOT}/playerLogManager) include_directories(${PLAYER_ROOT}/mp4demux) include_directories(${PLAYER_ROOT}/closedcaptions/rialto) +include_directories(${PLAYER_ROOT}/closedcaptions/direct-rialto) include_directories(${GTEST_INCLUDE_DIRS}) include_directories(${GMOCK_INCLUDE_DIRS}) include_directories(${GLIB_INCLUDE_DIRS}) From a4e89ac135a171774c5d7b32221e4e9987539fb2 Mon Sep 17 00:00:00 2001 From: anshephe <115161257+anshephe@users.noreply.github.com> Date: Fri, 7 Aug 2026 16:09:50 +0000 Subject: [PATCH 3/3] Updated with review comments --- closedcaptions/PlayerCCManager.h | 2 +- .../PlayerDirectRialtoCCManager.cpp | 26 +++++++++++-------- .../PlayerDirectRialtoCCManager.h | 7 +++-- .../rialto/PlayerRialtoCCManager.cpp | 24 ++++++++++------- closedcaptions/rialto/PlayerRialtoCCManager.h | 5 +++- 5 files changed, 39 insertions(+), 25 deletions(-) diff --git a/closedcaptions/PlayerCCManager.h b/closedcaptions/PlayerCCManager.h index 28690413..0adfa466 100644 --- a/closedcaptions/PlayerCCManager.h +++ b/closedcaptions/PlayerCCManager.h @@ -80,7 +80,7 @@ class PlayerCCManagerBase * multi-pipeline mode). * @param[in] handle - the handle being invalidated */ - virtual void InvalidateHandle(void *handle) {} + virtual void InvalidateHandle(void *) {} /** * @fn SetStatus diff --git a/closedcaptions/direct-rialto/PlayerDirectRialtoCCManager.cpp b/closedcaptions/direct-rialto/PlayerDirectRialtoCCManager.cpp index 6cd7a92a..03ade0e9 100755 --- a/closedcaptions/direct-rialto/PlayerDirectRialtoCCManager.cpp +++ b/closedcaptions/direct-rialto/PlayerDirectRialtoCCManager.cpp @@ -65,10 +65,10 @@ int PlayerDirectRialtoCCManager::Initialize(void *handle) MW_LOG_INFO("ENTRY handle=%p", handle); auto *newControl = static_cast(handle); - const bool changedHandle = (newControl != m_control); + const bool changedHandle = (newControl != m_control.load()); m_control = newControl; - if (m_control == nullptr) + if (newControl == nullptr) { MW_LOG_WARN("Initialize called with null handle"); MW_LOG_INFO("EXIT"); @@ -120,11 +120,12 @@ void PlayerDirectRialtoCCManager::Release(int id) void PlayerDirectRialtoCCManager::InvalidateHandle(void *handle) { - std::lock_guard lock(m_idLock); - if (handle != nullptr && handle == static_cast(m_control)) + // m_control is atomic, so this can safely race with Initialize() / + // SetTrack() / StartRendering() / StopRendering() without m_idLock. + auto *expected = static_cast(handle); + if (expected != nullptr && m_control.compare_exchange_strong(expected, nullptr)) { MW_LOG_WARN("handle=%p invalidated ahead of Release()", handle); - m_control = nullptr; } } @@ -138,7 +139,8 @@ int PlayerDirectRialtoCCManager::SetTrack( MW_LOG_INFO("track=\"%s\" format=%d", track.c_str(), static_cast(format)); - if (m_control == nullptr) + IDirectRialtoCC *control = m_control.load(); + if (control == nullptr) { MW_LOG_INFO("No control handle — track cached"); return 0; @@ -146,31 +148,33 @@ int PlayerDirectRialtoCCManager::SetTrack( const std::string identifier = mapTrackIdentifier(track, format); MW_LOG_INFO("setTextTrackIdentifier=\"%s\"", identifier.c_str()); - m_control->setTextTrackIdentifier(identifier); + control->setTextTrackIdentifier(identifier); return 0; } void PlayerDirectRialtoCCManager::StartRendering() { MW_LOG_INFO("ENTRY — unmuting CC"); - if (m_control == nullptr) + IDirectRialtoCC *control = m_control.load(); + if (control == nullptr) { MW_LOG_WARN("No control handle — cannot unmute"); return; } - m_control->setCCMute(false); + control->setCCMute(false); MW_LOG_INFO("EXIT"); } void PlayerDirectRialtoCCManager::StopRendering() { MW_LOG_INFO("ENTRY — muting CC"); - if (m_control == nullptr) + IDirectRialtoCC *control = m_control.load(); + if (control == nullptr) { MW_LOG_WARN("No control handle — cannot mute"); return; } - m_control->setCCMute(true); + control->setCCMute(true); MW_LOG_INFO("EXIT"); } diff --git a/closedcaptions/direct-rialto/PlayerDirectRialtoCCManager.h b/closedcaptions/direct-rialto/PlayerDirectRialtoCCManager.h index cc61da76..de90a460 100755 --- a/closedcaptions/direct-rialto/PlayerDirectRialtoCCManager.h +++ b/closedcaptions/direct-rialto/PlayerDirectRialtoCCManager.h @@ -44,6 +44,7 @@ #include "PlayerCCManager.h" #include "IDirectRialtoCC.h" +#include #include #include @@ -101,8 +102,10 @@ class PlayerDirectRialtoCCManager : public PlayerCCManagerBase CCFormat format); /// Non-owning pointer to the AampRialtoPlayer CC control interface. - /// Null until Initialize() is called (first PLAYING state). - IDirectRialtoCC *m_control{nullptr}; + /// Null until Initialize() is called (first PLAYING state). Atomic since + /// InvalidateHandle() may run concurrently with the other accessors from + /// the handle owner's destructor. + std::atomic m_control{nullptr}; /// Guards mId / mIdSet. std::mutex m_idLock; diff --git a/closedcaptions/rialto/PlayerRialtoCCManager.cpp b/closedcaptions/rialto/PlayerRialtoCCManager.cpp index 5fbb57ba..f8814355 100644 --- a/closedcaptions/rialto/PlayerRialtoCCManager.cpp +++ b/closedcaptions/rialto/PlayerRialtoCCManager.cpp @@ -35,7 +35,7 @@ int PlayerRialtoCCManager::Initialize(void * handle) { MW_LOG_INFO("PlayerRialtoCCManager::Initialize(%p) called", handle); - bool changedHandle = (handle != mSubtitleControlHandle); + bool changedHandle = (handle != mSubtitleControlHandle.load()); mSubtitleControlHandle = handle; @@ -110,11 +110,12 @@ void PlayerRialtoCCManager::Release(int id) */ void PlayerRialtoCCManager::InvalidateHandle(void *handle) { - std::lock_guard lock(mIdLock); - if (handle != nullptr && handle == mSubtitleControlHandle) + // mSubtitleControlHandle is atomic, so this can safely race with + // Initialize() / SetTrack() / StartRendering() / StopRendering(). + void *expected = handle; + if (expected != nullptr && mSubtitleControlHandle.compare_exchange_strong(expected, nullptr)) { MW_LOG_WARN("PlayerRialtoCCManager::handle:%p invalidated ahead of Release()", handle); - mSubtitleControlHandle = nullptr; } } @@ -130,7 +131,8 @@ int PlayerRialtoCCManager::SetTrack(const std::string &track, const CCFormat for MW_LOG_INFO("PlayerRialtoCCManager::set track \"%s\"", track.c_str()); - if (nullptr != mSubtitleControlHandle) + void *handle = mSubtitleControlHandle.load(); + if (nullptr != handle) { // We expect 'track' to have an alphabetic prefix. If it does not, // add one based on 'format'. @@ -150,7 +152,7 @@ int PlayerRialtoCCManager::SetTrack(const std::string &track, const CCFormat for MW_LOG_INFO("PlayerRialtoCCManager::set track (modified) \"%s\"", textTrackIdentifier.c_str()); - g_object_set(mSubtitleControlHandle, "text-track-identifier", textTrackIdentifier.c_str(), NULL); + g_object_set(handle, "text-track-identifier", textTrackIdentifier.c_str(), NULL); } else { @@ -167,9 +169,10 @@ void PlayerRialtoCCManager::StartRendering() { MW_LOG_INFO("PlayerRialtoCCManager::unmuting"); - if (nullptr != mSubtitleControlHandle) + void *handle = mSubtitleControlHandle.load(); + if (nullptr != handle) { - g_object_set(mSubtitleControlHandle, "mute", FALSE, NULL); + g_object_set(handle, "mute", FALSE, NULL); } else { @@ -185,9 +188,10 @@ void PlayerRialtoCCManager::StopRendering() { MW_LOG_INFO("PlayerRialtoCCManager::muting"); - if (nullptr != mSubtitleControlHandle) + void *handle = mSubtitleControlHandle.load(); + if (nullptr != handle) { - g_object_set(mSubtitleControlHandle, "mute", TRUE, NULL); + g_object_set(handle, "mute", TRUE, NULL); } else { diff --git a/closedcaptions/rialto/PlayerRialtoCCManager.h b/closedcaptions/rialto/PlayerRialtoCCManager.h index 9bf6374b..71c8d9d7 100644 --- a/closedcaptions/rialto/PlayerRialtoCCManager.h +++ b/closedcaptions/rialto/PlayerRialtoCCManager.h @@ -31,6 +31,7 @@ #include #include +#include #include /** @@ -122,7 +123,9 @@ class PlayerRialtoCCManager : public PlayerCCManagerBase void ResetState() override; private: - void *mSubtitleControlHandle{nullptr}; + /// GstElement* decoder handle. Atomic since InvalidateHandle() may run + /// concurrently with the other accessors from the handle owner's destructor. + std::atomic mSubtitleControlHandle{nullptr}; std::mutex mIdLock{}; int mId{0};