Feature/vplay 12250 vpaamp 966 a

[anshephe]

No description provided.

[Copilot]

Pull request overview

This PR extends the closed-captions manager layer with an explicit “handle invalidation” API intended to clear stored backend handles when the handle owner is destroyed, and adds a helper to check whether the CC manager singleton already exists without instantiating it.

Changes:

• Added PlayerCCManagerBase::InvalidateHandle(void*) and implemented it in the Rialto and direct-Rialto CC manager subclasses.
• Added PlayerCCManager::HasInstance() to query whether the singleton has been created.
• Updated CC unit-test CMake includes to include the closedcaptions/direct-rialto headers.

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
test/utests/tests/ClosedCaptionsTests/CMakeLists.txt	Adds include path for direct-Rialto CC headers in unit tests.
closedcaptions/rialto/PlayerRialtoCCManager.h	Declares InvalidateHandle() override for Rialto CC manager.
closedcaptions/rialto/PlayerRialtoCCManager.cpp	Implements InvalidateHandle() for Rialto handle clearing.
closedcaptions/PlayerCCManager.h	Adds base InvalidateHandle() API and introduces PlayerCCManager::HasInstance().
closedcaptions/PlayerCCManager.cpp	Implements PlayerCCManager::HasInstance().
closedcaptions/direct-rialto/PlayerDirectRialtoCCManager.h	Declares InvalidateHandle() override for direct-Rialto CC manager.
closedcaptions/direct-rialto/PlayerDirectRialtoCCManager.cpp	Implements InvalidateHandle() for direct-Rialto handle clearing.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[github-advanced-security]

You are seeing this message because GitHub Code Scanning has recently been set up for this repository, or this pull request contains the workflow file for the Code Scanning tool.

What Enabling Code Scanning Means:

• The 'Security' tab will display more code scanning analysis results (e.g., for the default branch).
• Depending on your configuration and choice of analysis tool, future pull requests will be annotated with code scanning analysis results.
• You will be able to see the analysis results for the pull request's branch on this overview once the scans have completed and the checks have passed.

For more information about GitHub Code Scanning, check out the documentation.

[Copilot]

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.

Suppressed comments (5)

closedcaptions/rialto/PlayerRialtoCCManager.cpp:114

• The comment states the atomic handle "can safely race" with SetTrack/StartRendering/StopRendering. Atomicity only makes the pointer value updates race-free; it does not make dereferencing the handle safe if the underlying GstElement can be destroyed concurrently (use-after-free is still possible if another thread loads the pointer before invalidation). Either add real lifetime synchronization or adjust the comment/contract to reflect the requirement on callers.

	// mSubtitleControlHandle is atomic, so this can safely race with
	// Initialize() / SetTrack() / StartRendering() / StopRendering().

closedcaptions/direct-rialto/PlayerDirectRialtoCCManager.cpp:124

• The comment claims m_control being atomic makes it safe to race with SetTrack/StartRendering/StopRendering. Atomicity only prevents data races on the pointer value; it does not prevent another thread from calling through a pointer that has been invalidated/freed concurrently (use-after-free risk remains). Consider adding synchronization/lifetime management (e.g., shared ownership or a mutex around both invalidate and use), or at least avoid stating this is safe.

	// m_control is atomic, so this can safely race with Initialize() /
	// SetTrack() / StartRendering() / StopRendering() without m_idLock.

closedcaptions/PlayerCCManager.h:80

• The InvalidateHandle() doc promises that invalidation means the handle "can never be used after" the underlying object is freed, but clearing an atomic/raw pointer cannot provide that guarantee on its own (another thread may have already loaded the pointer and still dereference it). The contract should be weakened to state the required external synchronization / lifetime management (e.g., stop concurrent users, or use ref-counting) rather than implying this prevents use-after-free by itself.

	 *        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).

closedcaptions/direct-rialto/PlayerDirectRialtoCCManager.cpp:130

• InvalidateHandle() is new behavior but there are no unit tests covering it (e.g., that invalidating the currently-stored handle clears it and prevents subsequent SetTrack/StartRendering/StopRendering from calling through it, while invalidating a different handle is a no-op). There is already a PlayerDirectRialtoCCManagerTests suite, so this behavior should be exercised there.

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<IDirectRialtoCC *>(handle);
	if (expected != nullptr && m_control.compare_exchange_strong(expected, nullptr))
	{
		MW_LOG_WARN("handle=%p invalidated ahead of Release()", handle);
	}
}

closedcaptions/PlayerCCManager.cpp:869

• HasInstance() is newly introduced but there are no unit tests covering its behavior (including the common patterns: returns false before GetInstance(), true after GetInstance(), and false again after DestroyInstance()). The existing ClosedCaptionsTests suite already exercises the singleton lifecycle, so this should be added there to prevent regressions.

/**
 *  @brief Check whether the singleton has already been created
 */
bool PlayerCCManager::HasInstance()
{
	return mInstance != NULL;
}