VPLAY-12682:SoC-independent underflow detection - follow-up improvements

[varshnie]

Reason for change:follow-up improvements for AampUnderflowMonitor
Risks: Medium

Signed-off-by: varshnie varshniblue14@gmail.com

[Copilot]

Pull request overview

This pull request implements follow-up improvements to the AampUnderflowMonitor feature, focusing on lifecycle management changes and code simplification. However, the PR introduces critical thread-safety issues by removing mutex protections that were essential for safe concurrent access.

Changes:

• Moved AampUnderflowMonitor initialization from lazy creation in StartUnderflowMonitor to eager creation in StreamAbstractionAAMP constructor
• Removed mUnderflowMonitorMutex from StreamAbstractionAAMP class and all lifecycle method synchronization
• Cached raw pointers in AampUnderflowMonitor::Run() method to avoid repeated mutex locking
• Renamed parameter from bufferingStopped to bufferingStart for clarity
• Added config check wrapper around SetBufferingState logic
• Moved StartUnderflowMonitor calls from central TuneHelper to protocol-specific Start() methods

Reviewed changes

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

Show a summary per file

File	Description
streamabstraction.cpp	Moved monitor initialization to constructor; removed mutex protections from lifecycle methods; simplified StartUnderflowMonitor
StreamAbstractionAAMP.h	Removed mUnderflowMonitorMutex member variable
priv_aamp.h	Renamed parameter bufferingStopped to bufferingStart in SendBufferChangeEvent
priv_aamp.cpp	Updated parameter name; added ReportBufferEvent config check wrapping SetBufferingState logic; removed monitor startup from TuneHelper
AampUnderflowMonitor.h	Removed @fn tags from Doxygen comments
AampUnderflowMonitor.cpp	Cached mAamp and mStream pointers locally in Run(); removed mutex protections from multiple pointer accesses
fragmentcollector_progressive.cpp	Added StartUnderflowMonitor call to protocol-specific Start() method
fragmentcollector_mpd.cpp	Added StartUnderflowMonitor call to protocol-specific Start() method
fragmentcollector_hls.cpp	Added StartUnderflowMonitor call to protocol-specific Start() method
AampConfig.cpp	Added documentation comment for enableAampUnderflowMonitor config option

Comments suppressed due to low confidence (1)

streamabstraction.cpp:3043

• Missing synchronization: The removal of the mutex lock from StopUnderflowMonitor creates a race condition with StartUnderflowMonitor and IsUnderflowMonitorRunning(). Without the mutex, concurrent access to mUnderflowMonitor (checking, calling methods, and resetting) is not thread-safe.

Re-introduce the mUnderflowMonitorMutex to protect access to mUnderflowMonitor in all three lifecycle methods.

	if (mUnderflowMonitor)
	{
		mUnderflowMonitor->Stop();
		mUnderflowMonitor.reset();
		AAMPLOG_INFO("Stopped AampUnderflowMonitor for video");
	}

[Copilot]

Thread safety issue: The call to interruptibleMsSleep is not protected by the mutex. After checking "if (!aampPtr)" at line 145, the cached pointer is used without protection. If Stop() is called concurrently on another thread after the check but before the sleep call, this could result in accessing a deleted PrivateInstanceAAMP object.

The original code correctly wrapped this call in a mutex-protected block to ensure thread-safe access to mAamp.

Suggested change

	if (!aampPtr) return;
	aampPtr->interruptibleMsSleep(100);
	{
	std::lock_guard<std::mutex> lock(mMutex);
	if (!aampPtr) return;
	aampPtr->interruptibleMsSleep(100);
	}

[Copilot]

Thread safety issue: Direct access to cached pointer without mutex protection. The pattern at lines 249-250 is inconsistent with the mutex-protected access pattern used elsewhere in this method (e.g., lines 210-212). If Stop() is called concurrently after the check at line 249, the cached aampPtr could be stale while the actual mAamp member has been nullified.

All accesses to aampPtr that call methods should be protected by the mMutex lock.

[Copilot]

Thread safety issue: The call to interruptibleMsSleep is not protected by the mutex, similar to the issue at lines 145-146. The cached pointer aampPtr could be stale if Stop() is called concurrently.

The original code correctly wrapped this call in a mutex-protected block.

[Copilot]

Inconsistent indentation: There is an extra space before "aampPtr->" on this line. It should align with the surrounding code.

Suggested change

	aampPtr->interruptibleMsSleep(sleepMs);
	aampPtr->interruptibleMsSleep(sleepMs);

[Copilot]

Critical race condition: Caching raw pointers from member variables at the start of Run() defeats the thread-safety mechanism. If Stop() is called on another thread after these pointers are cached but before they're used, the cached pointers could point to deleted objects since Stop() sets mAamp and mStream to nullptr under mutex lock at line 105-106.

The original code properly checked these pointers under mutex protection before each use. The new approach removes this protection, creating a use-after-free vulnerability. The checks like "if (!aampPtr) return;" throughout the method cannot prevent this race since aampPtr is the cached copy, not the actual member variable that Stop() nullifies.

Revert to checking mAamp and mStream under mutex lock before each use, as in the original implementation.

[Copilot]

Potential issue: Creating the AampUnderflowMonitor in the StreamAbstractionAAMP constructor may be premature. The monitor is only started after the stream's Start() method is called (as seen in fragmentcollector_*.cpp), and requires a valid video track to function properly (checked in StartUnderflowMonitor at line 3015).

If the monitor constructor performs any initialization that depends on the stream being fully set up, or if there are resource allocation concerns, consider deferring creation to the first call to StartUnderflowMonitor, similar to the original lazy initialization pattern.

Suggested change

	if(GETCONFIGVALUE(eAAMPConfig_EnableAampUnderflowMonitor))
	{
	if(!mUnderflowMonitor)
	{
	mUnderflowMonitor = std::make_unique<AampUnderflowMonitor>(this, aamp);
	}
	}

[Copilot]

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated 6 comments.

[Copilot]

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated 4 comments.

[Copilot]

Stop() no longer nulls mAamp/mStream, but Run() still contains checks/commentary that imply pointers can become null during shutdown (if (!mAamp || !mStream) return; // Stop() was called). Consider either restoring pointer nullification after join() (if you want pointer-null checks to be meaningful) or removing the pointer-null shutdown path and relying purely on mRunning + join for termination to keep the shutdown contract consistent.