VPLAY-12287 - JS SetCCStatus+Stop sequence should not block on Tune by Gnanesha · Pull Request #976 · rdkcentral/aamp

Gnanesha · 2026-02-05T19:51:07Z

Reason for change: Allow Stop sequence to proceed and abort a tune. Allow JS Stop command to run if JS Tune() is in progress. Also prevent SetCCStatus JS command from blocking during Tune. This is to improve channel zapping speed, where Stop is called during prior Tune() and is currently blocked.

Test Procedure: Three fast ch changes, delay. Repeat. Check we don't see OTTPENDING or timeouts above AAMP. Check stop occurrs asynchronously if stop is sent during tune. Regression check normal tune and stop times and operation. Check CC is blanked as required after fast/slow channel changes with PG. Check we have not introduced crashes with continuous channel change.

Risk: High

Reason for change: Allow Stop sequence to proceed and abort a tune. Allow JS Stop command to run if JS Tune() is in progress. Also prevent SetCCStatus JS command from blocking during Tune. This is to improve channel zapping speed, where Stop is called during prior Tune() and is currently blocked. Test Procedure: Three fast ch changes, delay. Repeat. Check we don't see OTTPENDING or timeouts above AAMP. Check stop occurrs asynchronously if stop is sent during tune. Regression check normal tune and stop times and operation. Check CC is blanked as required after fast/slow channel changes with PG. Check we have not introduced crashes with continuous channel change. Risk: High Signed-off-by: Gavin Parry <gavin_parry@comcast.com>

Copilot

Pull request overview

This PR implements a significant concurrency improvement to allow Stop() to proceed during Tune() operations and prevent SetCCStatus from blocking during tune. The primary goal is to improve channel zapping speed by making certain operations non-blocking.

Changes:

Introduced interruptable task support in AampScheduler to allow Stop() to abort in-progress Tune operations
Added mStopInProgress flag with mutex protection in PlayerInstanceAAMP to coordinate Stop/Tune interactions
Modified SetCCStatus to use TryStreamLock() with deferred application via mApplyCachedCCStatus flag
Changed curl progress callback return values from -1 to 1 for abort signaling
Reorganized StreamLock acquisition in Stop() to protect critical sections

Reviewed changes

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

Show a summary per file

File	Description
priv_aamp.h	Added mApplyCachedCCStatus flag for deferred CC status application
priv_aamp.cpp	Implemented deferred CC status logic, modified curl callbacks, reorganized Stop() locking
main_aamp.h	Added mStopInternalMutex and mStopInProgress for Stop/Tune coordination
main_aamp.cpp	Implemented interruptable Stop logic with new mutex, added stopInProgress checks in TuneInternal
AampScheduler.h	Added mInterruptableTaskInProgress flag and updated SuspendScheduler signature
AampScheduler.cpp	Implemented interruptable task detection and conditional blocking in SuspendScheduler
downloader/AampCurlStore.cpp	Added preprocessor warning for old curl versions
downloader/AampCurlDownloader.cpp	Added commented-out test code for download throttling

Copilot · 2026-02-05T19:56:20Z

 	bool playerStartedWithTrickPlay; 			/**< To indicate player switch happened in trickplay rate */
 	bool userProfileStatus; 				/**< Select profile based on user list*/
 	bool mApplyCachedVideoMute;				/**< To apply video mute() operations if it has been cached due to tune in progress */
+	bool mApplyCachedCCStatus;				/**< To apply () operations if it has been cached due to tune in progress */


The comment for mApplyCachedCCStatus is incomplete. It states "To apply () operations if it has been cached due to tune in progress" but should specify what operation it applies (e.g., "To apply SetCCStatus operations if it has been cached due to tune in progress").

Suggested change

bool mApplyCachedCCStatus; /**< To apply () operations if it has been cached due to tune in progress */

bool mApplyCachedCCStatus; /**< To apply SetCCStatus() operations if it has been cached due to tune in progress */

Copilot · 2026-02-05T19:56:20Z

+				if (mApplyCachedCCStatus)
+				{
+					AAMPLOG_INFO("SetCCStatus CC muted %d mApplyCachedCCStatus %d", subtitles_muted, mApplyCachedCCStatus);
+					mApplyCachedCCStatus = false;
+					if (mpStreamAbstractionAAMP)
+					{
+						mpStreamAbstractionAAMP->MuteSubtitles(subtitles_muted);
+						if (HasSidecarData())
+						{ // has sidecar data
+							mpStreamAbstractionAAMP->MuteSidecarSubtitles(subtitles_muted);
+						}
+					}
+					SetSubtitleMute(subtitles_muted);
+				}


The code block at lines 5503-5516 is duplicated at lines 6111-6124. This violates the DRY (Don't Repeat Yourself) principle. Consider extracting this logic into a separate helper method such as ApplyCachedCCStatus() to improve maintainability and ensure consistency between both locations.

Copilot · 2026-02-05T19:56:21Z

+		}
+		else
+		{
+			AAMPLOG_WARN("Stop was is in progress, so tune was aborted.");


The typo "was is" should be "is". The message should read "Stop is in progress, so tune was aborted."

Suggested change

AAMPLOG_WARN("Stop was is in progress, so tune was aborted.");

AAMPLOG_WARN("Stop is in progress, so tune was aborted.");

Copilot · 2026-02-05T19:56:21Z

 	SetLocalAAMPTsbInjection(false);
 	// Stopping the playback, release all DRM context
+
+	//AcquireStreamLock();


The commented-out code "//AcquireStreamLock();" at line 7527 should be removed. Dead code should not be left in the codebase, especially when it's critical locking logic. If it's temporarily needed for reference, it should be documented with a clear explanation or tracked in version control history.

Suggested change

//AcquireStreamLock();

Copilot · 2026-02-05T19:56:21Z

+	#if 0
+	// GNP.. For testing... throttle downloads and ensure specific write callback frequency
+	CURL_EASY_SETOPT_FUNC(mCurl, CURLOPT_BUFFERSIZE, 16384L);   // default 16K. Ensure < CURLOPT_MAX_RECV_SPEED_LARGE & more write callbacks to terminate early
+	CURL_EASY_SETOPT_FUNC(mCurl, CURLOPT_MAX_RECV_SPEED_LARGE, 375000L); // bytes/second cap, if > CURLOPT_BUFFERSIZE; e.g. 375000*8= 3Mbit
+	#endif



The test code wrapped in "#if 0" should be removed before merging. Test/debug code should not be committed to production branches, even if disabled. This adds clutter and confusion.

Suggested change

#if 0

// GNP.. For testing... throttle downloads and ensure specific write callback frequency

CURL_EASY_SETOPT_FUNC(mCurl, CURLOPT_BUFFERSIZE, 16384L); // default 16K. Ensure < CURLOPT_MAX_RECV_SPEED_LARGE & more write callbacks to terminate early

CURL_EASY_SETOPT_FUNC(mCurl, CURLOPT_MAX_RECV_SPEED_LARGE, 375000L); // bytes/second cap, if > CURLOPT_BUFFERSIZE; e.g. 375000*8= 3Mbit

#endif

Copilot · 2026-02-05T19:56:22Z

+	auto playerState=GetState();
+	bool streamLockTaken=false;
+	// This process will be blocking if we've already entered a playback state.
+	// This is a workaround where SetCCStatus is called whilst Tune is in progress (StreamLock held) from an EPG Stop call.
+	// Note: CC status can be changed at any time during playback and we do not want to ignore this if StreamLock is currently taken.
+	// The alternative would be to allow TryStreamLock() to have a timeout ~1s, but this might be less predictable.
+	bool allowDeferredApplication = ((playerState == eSTATE_IDLE)         ||
+					 (playerState == eSTATE_INITIALIZING) ||
+					 (playerState == eSTATE_INITIALIZED)  ||
+					 (playerState == eSTATE_PREPARING)    ||
+					 (playerState == eSTATE_PREPARED)
+					);
+
 	subtitles_muted = !enabled;
-	if (mpStreamAbstractionAAMP)
+
+	if(allowDeferredApplication)
 	{
-		mpStreamAbstractionAAMP->MuteSubtitles(subtitles_muted);
-		if (HasSidecarData())
-		{ // has sidecar data
-			mpStreamAbstractionAAMP->MuteSidecarSubtitles(subtitles_muted);
+		streamLockTaken=TryStreamLock();
+	}
+	else
+	{
+		AcquireStreamLock();
+		streamLockTaken=true;
+	}
+
+	if (streamLockTaken)
+	{
+		if (mpStreamAbstractionAAMP)
+		{
+			mpStreamAbstractionAAMP->MuteSubtitles(subtitles_muted);
+			if (HasSidecarData())
+			{ // has sidecar data
+				mpStreamAbstractionAAMP->MuteSidecarSubtitles(subtitles_muted);
+			}
 		}
+		SetSubtitleMute(subtitles_muted);
+		ReleaseStreamLock();
+	}
+	else
+	{
+		AAMPLOG_WARN("CC status value has been cached, subtitles_muted = %d, playerState=%d", subtitles_muted, playerState);
+		mApplyCachedCCStatus=true; // can't do it now, but remember that we want to apply this


There is a potential race condition between reading GetState() at line 10893 and setting mApplyCachedCCStatus at line 10934. The player state could transition after the check but before the flag is set, leading to inconsistent behavior. Consider holding a lock that protects both the state check and the flag assignment, or checking the state again after attempting TryStreamLock().

Copilot · 2026-02-05T19:56:22Z

+					 (playerState == eSTATE_PREPARED)
+					);
+
 	subtitles_muted = !enabled;


The member variable subtitles_muted is set at line 10906 without any lock protection, but it may be accessed by other threads (e.g., during tune or playback). This creates a potential race condition. Consider protecting this assignment with appropriate synchronization, or document why it's safe if there's a guarantee that only one thread modifies it at a time.

Copilot · 2026-02-05T19:56:22Z

+		mStopInternalMutex.lock();
+		stopInProgress=mStopInProgress;	// if player stop is being called from JS thread, don't tune. StopInternal() is ok..
+						//  (mutex protected) and ensures that anything this thread does in future is undone
+
+		if ((state != eSTATE_IDLE) && (state != eSTATE_RELEASED) && (!IsOTAtoOTA) )
 		{
 			//Calling tune without closing previous tune
 			StopInternal(false);
 		}
-		aamp->getAampCacheHandler()->StartPlaylistCache();
-		aamp->Tune(mainManifestUrl, autoPlay, contentType, bFirstAttempt, bFinalAttempt, traceUUID, audioDecoderStreamSync, refreshManifestUrl, mpdStitchingMode, std::move(sid),manifestData);
+		mStopInternalMutex.unlock();
+
+		if (!stopInProgress)
+		{
+			aamp->getAampCacheHandler()->StartPlaylistCache();
+			aamp->Tune(mainManifestUrl, autoPlay, contentType, bFirstAttempt, bFinalAttempt, traceUUID, audioDecoderStreamSync, refreshManifestUrl, mpdStitchingMode, std::move(sid),manifestData);
+		}


There is potential for a race condition in the mutex usage pattern. The mStopInternalMutex is locked at line 415, the stopInProgress flag is read at line 416, then StopInternal() is called at line 422 while still holding the lock, and the mutex is unlocked at line 424. However, if stopInProgress is false and the function proceeds to call Tune(), a Stop() from another thread could set mStopInProgress=true after line 424 but before Tune() begins executing. This could lead to unexpected behavior. Consider holding the lock until after the Tune() call starts, or using a different synchronization mechanism.

Copilot · 2026-02-05T19:56:22Z

 						//Unlock so that new entries can be added to queue while function executes
 						queueLock.unlock();

+						// extensibility - add slow tasks that we want to be interruptable by an external Stop() here (e.g. seek/setrate) once they are modified for suitability
+						if ( (0 == obj.mTaskName.compare("Tune"))
+						   )
+						{
+							AAMPLOG_WARN("Scheduler found an interruptable task '%s' is about to run, taskId: %d.", obj.mTaskName.c_str(), obj.mId);
+							mInterruptableTaskInProgress=true;
+						}
+
 						AAMPLOG_WARN("SchedulerTask Execution:%s taskId:%d",obj.mTaskName.c_str(),obj.mId);
 						//Execute function
 						obj.mTask(obj.mData);
 						//May be used in a wait() in future loops, it needs to be locked
+
+						mInterruptableTaskInProgress=false;
 						queueLock.lock();


The mInterruptableTaskInProgress flag is set at line 153 and cleared at line 161 without holding mQMutex, but it is read at line 231 while holding mQMutex. While the write occurs after unlocking queueLock and before executing the task, there's still a potential race condition. If SuspendScheduler(true) is called between lines 153 and 161, it could read an inconsistent value. Consider protecting all accesses to mInterruptableTaskInProgress with mQMutex, or use std::atomic for this flag.

Suggested change

//Unlock so that new entries can be added to queue while function executes

queueLock.unlock();

// extensibility - add slow tasks that we want to be interruptable by an external Stop() here (e.g. seek/setrate) once they are modified for suitability

if ( (0 == obj.mTaskName.compare("Tune"))

)

{

AAMPLOG_WARN("Scheduler found an interruptable task '%s' is about to run, taskId: %d.", obj.mTaskName.c_str(), obj.mId);

mInterruptableTaskInProgress=true;

}

AAMPLOG_WARN("SchedulerTask Execution:%s taskId:%d",obj.mTaskName.c_str(),obj.mId);

//Execute function

obj.mTask(obj.mData);

//May be used in a wait() in future loops, it needs to be locked

mInterruptableTaskInProgress=false;

queueLock.lock();

// Mark interruptable tasks while holding the queue mutex,

// so that readers synchronized on mQMutex see a consistent state.

if ( (0 == obj.mTaskName.compare("Tune"))

)

{

AAMPLOG_WARN("Scheduler found an interruptable task '%s' is about to run, taskId: %d.", obj.mTaskName.c_str(), obj.mId);

mInterruptableTaskInProgress = true;

}

// Unlock so that new entries can be added to queue while function executes

queueLock.unlock();

AAMPLOG_WARN("SchedulerTask Execution:%s taskId:%d",obj.mTaskName.c_str(),obj.mId);

//Execute function

obj.mTask(obj.mData);

//May be used in a wait() in future loops, it needs to be locked

queueLock.lock();

// Clear the flag while holding the queue mutex to avoid races

mInterruptableTaskInProgress = false;

Copilot · 2026-02-05T19:56:23Z

 	 */
 	void ExecuteAsyncTask();

+	bool mInterruptableTaskInProgress;	/**< Special case to allow abort (stop) of allowed asynchronous tasks; access under mQMutex */


The comment at line 167 states that mInterruptableTaskInProgress should be accessed under mQMutex, but the implementation in AampScheduler.cpp does not follow this convention. The flag is written at lines 153 and 161 without holding mQMutex. Either update the comment to reflect the actual synchronization strategy (e.g., if using implicit synchronization via task execution order), or modify the implementation to protect all accesses with mQMutex as documented.

Suggested change

bool mInterruptableTaskInProgress; /**< Special case to allow abort (stop) of allowed asynchronous tasks; access under mQMutex */

bool mInterruptableTaskInProgress; /**< Special case flag to allow abort (stop) of allowed asynchronous tasks; see implementation for synchronization details */

Copilot AI review requested due to automatic review settings February 5, 2026 19:51

Copilot started reviewing on behalf of Gnanesha February 5, 2026 19:51 View session

Copilot AI reviewed Feb 5, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

VPLAY-12287 - JS SetCCStatus+Stop sequence should not block on Tune#976

VPLAY-12287 - JS SetCCStatus+Stop sequence should not block on Tune#976
Gnanesha wants to merge 1 commit intosupport/2.7.1_8_2_drmfrom
feature/VPLAY-12287_8.2_drm

Gnanesha commented Feb 5, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Feb 5, 2026

Uh oh!

Copilot AI Feb 5, 2026

Uh oh!

Copilot AI Feb 5, 2026

Uh oh!

Copilot AI Feb 5, 2026

Uh oh!

Copilot AI Feb 5, 2026

Uh oh!

Copilot AI Feb 5, 2026

Uh oh!

Copilot AI Feb 5, 2026

Uh oh!

Copilot AI Feb 5, 2026

Uh oh!

Copilot AI Feb 5, 2026

Uh oh!

Copilot AI Feb 5, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

	bool mApplyCachedCCStatus; /*< To apply () operations if it has been cached due to tune in progress /
	bool mApplyCachedCCStatus; /*< To apply SetCCStatus() operations if it has been cached due to tune in progress /

	AAMPLOG_WARN("Stop was is in progress, so tune was aborted.");
	AAMPLOG_WARN("Stop is in progress, so tune was aborted.");

	bool mInterruptableTaskInProgress; /*< Special case to allow abort (stop) of allowed asynchronous tasks; access under mQMutex /
	bool mInterruptableTaskInProgress; /*< Special case flag to allow abort (stop) of allowed asynchronous tasks; see implementation for synchronization details /

Conversation

Gnanesha commented Feb 5, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Feb 5, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 5, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 5, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 5, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 5, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 5, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 5, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 5, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 5, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 5, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants