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]

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]

Pull request overview

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

[Copilot]

Pull request overview

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

[Copilot]

Pull request overview

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

[Copilot]

Pull request overview

Copilot reviewed 18 out of 19 changed files in this pull request and generated 2 comments.

Comments suppressed due to low confidence (1)

test/utests/fakes/FakeAampUnderflowMonitor.cpp:54

• This fake file defines the production AampUnderflowMonitor methods (ctor/dtor/Start/Stop/Run). Because the fakes library is linked by most unit test executables, any test that also compiles the real AampUnderflowMonitor.cpp will hit ODR / multiple-definition linker failures. Consider renaming this to a separate fake class (instead of redefining AampUnderflowMonitor), or moving it out of the always-linked fakes library so only specific tests override the implementation.

#include "AampUnderflowMonitor.h"
#include "MockAampUnderflowMonitor.h"

MockAampUnderflowMonitor *g_mockAampUnderflowMonitor = nullptr;

AampUnderflowMonitor::AampUnderflowMonitor(StreamAbstractionAAMP* stream, PrivateInstanceAAMP* aamp) : mStream(stream), mAamp(aamp)
{
}

AampUnderflowMonitor::~AampUnderflowMonitor()
{
}

void AampUnderflowMonitor::Start()
{
    mRunning.store(true);
    if(g_mockAampUnderflowMonitor != nullptr)
    {
        g_mockAampUnderflowMonitor->Start();
    }
}

void AampUnderflowMonitor::Stop()
{
    mRunning.store(false);
    if(g_mockAampUnderflowMonitor != nullptr)
    {
        g_mockAampUnderflowMonitor->Stop();
    }
}

void AampUnderflowMonitor::Run()
{
}

[Copilot]

Pull request overview

Copilot reviewed 20 out of 21 changed files in this pull request and generated 1 comment.

[Copilot]

Pull request overview

Copilot reviewed 18 out of 19 changed files in this pull request and generated 5 comments.

Comments suppressed due to low confidence (1)

StreamAbstractionAAMP.h:2042

• mUnderflowMonitor is a std::unique_ptr that is started/stopped and also reset in the implementation, and the class-level mutex that previously serialized these operations was removed. Given the monitor pointer can now be reset, this member needs explicit synchronization (or a stable lifetime guarantee) to avoid races when accessed from multiple threads via the public Start/Stop/IsRunning wrappers.

protected:
	/**
	 * Underflow monitor instance owned by Stream; manages detection and
	 * handling of underflow conditions.
	 */
	std::unique_ptr<class AampUnderflowMonitor> mUnderflowMonitor;

[Copilot]

Pull request overview

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

[Copilot]

Pull request overview

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

[Copilot]

Pull request overview

Copilot reviewed 18 out of 20 changed files in this pull request and generated 5 comments.

[Copilot]

StopUnderflowMonitor() is invoked after DisableDownloads() and other teardown steps. While the underflow monitor thread is still running it can call ResumeTrackDownloads()/SetBufferingState(), which can counteract Stop()’s attempt to quiesce downloads and increase teardown-time races. Consider stopping the underflow monitor at the beginning of Stop() (before DisableDownloads and before track teardown), and ideally keying off mUnderflowMonitor’s existence rather than a separate config check.

[Copilot]

StopUnderflowMonitor() happens after DisableDownloads()/AbortWaitForNextAdResolved() in this shutdown path. Since the monitor thread may call ResumeTrackDownloads()/SetBufferingState while Stop() is in progress, it’s safer to stop/join the monitor thread at the start of Stop() (before disabling downloads / tearing down tracks), and to guard on mUnderflowMonitor being non-null.

[Copilot]

This note says StopUnderflowMonitor “must be called before Stop()”, but the current shutdown flow calls StopUnderflowMonitor from within the various Stop() implementations (and not necessarily before Stop()). Please update the note to match the actual lifecycle contract (e.g., “Stop() will stop the monitor; ensure it is stopped before destruction”), or enforce it by stopping the monitor in the StreamAbstractionAAMP destructor as a safety net.

Suggested change

	* @note Must be called before Stop() or before destroying the StreamAbstractionAAMP
	* instance. The underflow monitor holds raw pointers back into the stream and
	* the AAMP instance; stopping it joins the background thread and prevents any
	* use-after-free that would otherwise arise during teardown.
	* @note Stops the underflow monitor thread and joins it. The monitor holds
	* raw pointers back into the stream and the AAMP instance, so it must
	* be stopped (either explicitly via this method or implicitly from
	* Stop()) before destroying the StreamAbstractionAAMP instance to
	* avoid use-after-free during teardown.

[Copilot]

This loop busy-waits with std::this_thread::yield() for up to 1s, which can burn CPU (especially since the test fakes’ interruptibleMsSleep() is a no-op and the monitor thread can spin). Consider using a small sleep_for (e.g., 1–5ms) or a condition_variable/latch to wait for the expected call.

Suggested change

	std::this_thread::yield();
	std::this_thread::sleep_for(std::chrono::milliseconds(1));

[Copilot]

This second busy-wait loop also yields in a tight loop for up to 1s. To keep the unit test suite CPU-friendly and reduce flakiness under load, prefer a blocking primitive (condition_variable) or at least a small sleep_for in the polling loop.

Suggested change

	std::this_thread::yield();
	std::this_thread::sleep_for(std::chrono::milliseconds(1));