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..0adfa466 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 *) {} + /** * @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..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"); @@ -118,6 +118,18 @@ void PlayerDirectRialtoCCManager::Release(int id) } } +void PlayerDirectRialtoCCManager::InvalidateHandle(void *handle) +{ + // 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); + } +} + + int PlayerDirectRialtoCCManager::SetTrack( const std::string &track, CCFormat format) { @@ -127,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; @@ -135,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 936f1f50..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 @@ -63,6 +64,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; @@ -98,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 db48d6d9..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; @@ -105,6 +105,20 @@ void PlayerRialtoCCManager::Release(int id) return; } +/** + * @brief Clear mSubtitleControlHandle if it currently equals handle + */ +void PlayerRialtoCCManager::InvalidateHandle(void *handle) +{ + // 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); + } +} + /** * @brief Set CC track */ @@ -117,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'. @@ -137,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 { @@ -154,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 { @@ -172,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 bbbd9882..71c8d9d7 100644 --- a/closedcaptions/rialto/PlayerRialtoCCManager.h +++ b/closedcaptions/rialto/PlayerRialtoCCManager.h @@ -31,6 +31,7 @@ #include #include +#include #include /** @@ -48,6 +49,9 @@ class PlayerRialtoCCManager : public PlayerCCManagerBase */ void Release(int iID) override; + /// @copydoc PlayerCCManagerBase::InvalidateHandle + void InvalidateHandle(void *handle) override; + /** * @fn GetId * @return int - unique ID @@ -119,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}; 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})