VPLAY-12568:Implement cancelReservation UVE-API by varshnie · Pull Request #1007 · rdkcentral/aamp

varshnie · 2026-02-10T09:05:09Z

Reason for change:cancelReservation UVE call
Test Procedure: Refer jira ticket
Priority: P1

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

Copilot

Pull request overview

Implements a new cancelReservation API surface for UVE by plumbing a CancelReservation() call from JS → PlayerInstanceAAMP → PrivateInstanceAAMP → CDAI ad manager (MPD).

Changes:

Added CancelReservation() to PlayerInstanceAAMP and PrivateInstanceAAMP public APIs.
Exposed cancelReservation() to JavaScript via jsmediaplayer.cpp.
Added CancelReservation() to CDAI interfaces and MPD ad manager classes.

Reviewed changes

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

Show a summary per file

File	Description
`priv_aamp.h`	Declares `PrivateInstanceAAMP::CancelReservation()` API.
`priv_aamp.cpp`	Forwards cancel request to `mCdaiObject`.
`main_aamp.h`	Declares `PlayerInstanceAAMP::CancelReservation()` public API.
`main_aamp.cpp`	Forwards cancel request to private AAMP instance.
`jsbindings/jsmediaplayer.cpp`	Adds JS-callable `cancelReservation()` binding and registers it.
`AdManagerBase.h`	Extends CDAI base interface with `CancelReservation()`.
`admanager_mpd.h`	Declares MPD overrides for `CancelReservation()`.
`admanager_mpd.cpp`	Implements MPD forwarding, but leaves private implementation as TODO/empty.

priv_aamp.cpp

main_aamp.cpp

Copilot · 2026-02-10T09:13:38Z

jsbindings/jsmediaplayer.cpp

+    std::string playingReservationId = aamp_JSValueToCString(ctx, arguments[0], exception);
+    std::string cancelAtReservationId = aamp_JSValueToCString(ctx, arguments[1], exception);
+


aamp_JSValueToCString() allocates with new[] and requires the caller to free (typically via SAFE_DELETE_ARRAY). Here the returned char* is implicitly converted into std::string and never freed, leaking memory on every cancelReservation() call. Please copy into std::string and then free the original buffer (or introduce a helper that returns std::string safely).

Suggested change

std::string playingReservationId = aamp_JSValueToCString(ctx, arguments[0], exception);

std::string cancelAtReservationId = aamp_JSValueToCString(ctx, arguments[1], exception);

char *playingReservationIdCStr = aamp_JSValueToCString(ctx, arguments[0], exception);

std::string playingReservationId;

if (playingReservationIdCStr)

{

playingReservationId.assign(playingReservationIdCStr);

}

SAFE_DELETE_ARRAY(playingReservationIdCStr);

char *cancelAtReservationIdCStr = aamp_JSValueToCString(ctx, arguments[1], exception);

std::string cancelAtReservationId;

if (cancelAtReservationIdCStr)

{

cancelAtReservationId.assign(cancelAtReservationIdCStr);

}

SAFE_DELETE_ARRAY(cancelAtReservationIdCStr);

Copilot · 2026-02-10T09:13:38Z

jsbindings/jsmediaplayer.cpp

+    if (argumentCount < 2)
+    {
+        // Throw a JS exception: 2 arguments required
+        JSStringRef errorStr = JSStringCreateWithUTF8CString("cancelReservation requires 2 string arguments");
+        *exception = JSValueMakeString(ctx, errorStr);
+        JSStringRelease(errorStr);
+        return JSValueMakeUndefined(ctx);
+    }
+
+    std::string playingReservationId = aamp_JSValueToCString(ctx, arguments[0], exception);
+    std::string cancelAtReservationId = aamp_JSValueToCString(ctx, arguments[1], exception);
+
+    AAMPMediaPlayer_JS *privObj = (AAMPMediaPlayer_JS *) JSObjectGetPrivate(thisObject);
+    if (privObj && privObj->_aamp)
+    {
+        privObj->_aamp->CancelReservation(playingReservationId, cancelAtReservationId);
+    }
+


This JS API’s argument/object validation and exception handling is inconsistent with other bindings in this file: it only checks argumentCount < 2 (allowing extra args), does not validate privObj/_aamp with aamp_GetException(), and sets *exception to a raw string rather than using aamp_GetException(AAMPJS_INVALID_ARGUMENT/...). Please follow the existing pattern (e.g., notifyReservationCompletion) so callers get a proper JS exception and consistent behavior.

Suggested change

if (argumentCount < 2)

{

// Throw a JS exception: 2 arguments required

JSStringRef errorStr = JSStringCreateWithUTF8CString("cancelReservation requires 2 string arguments");

*exception = JSValueMakeString(ctx, errorStr);

JSStringRelease(errorStr);

return JSValueMakeUndefined(ctx);

}

std::string playingReservationId = aamp_JSValueToCString(ctx, arguments[0], exception);

std::string cancelAtReservationId = aamp_JSValueToCString(ctx, arguments[1], exception);

AAMPMediaPlayer_JS *privObj = (AAMPMediaPlayer_JS *) JSObjectGetPrivate(thisObject);

if (privObj && privObj->_aamp)

{

privObj->_aamp->CancelReservation(playingReservationId, cancelAtReservationId);

}

AAMPMediaPlayer_JS *privObj = (AAMPMediaPlayer_JS *) JSObjectGetPrivate(thisObject);

if (!privObj || !privObj->_aamp)

{

LOG_ERROR_EX("JSObjectGetPrivate returned NULL!");

*exception = aamp_GetException(ctx, AAMPJS_MISSING_OBJECT,

"Can only call cancelReservation() on instances of AAMPPlayer");

return JSValueMakeUndefined(ctx);

}

if (argumentCount != 2)

{

*exception = aamp_GetException(ctx, AAMPJS_INVALID_ARGUMENT,

"cancelReservation requires 2 string arguments");

return JSValueMakeUndefined(ctx);

}

if (!JSValueIsString(ctx, arguments[0]) || !JSValueIsString(ctx, arguments[1]))

{

*exception = aamp_GetException(ctx, AAMPJS_INVALID_ARGUMENT,

"cancelReservation arguments must be strings");

return JSValueMakeUndefined(ctx);

}

std::string playingReservationId = aamp_JSValueToCString(ctx, arguments[0], exception);

std::string cancelAtReservationId = aamp_JSValueToCString(ctx, arguments[1], exception);

if (exception && *exception)

{

return JSValueMakeUndefined(ctx);

}

privObj->_aamp->CancelReservation(playingReservationId, cancelAtReservationId);

main_aamp.h

AdManagerBase.h

Copilot · 2026-02-10T09:13:39Z

admanager_mpd.cpp

+    // TODO: Actual ad truncation logic, e.g.:
+    // Find the ad by playingReservationId, mark as truncated at cancelAtReservationId.


PrivateCDAIObjectMPD::CancelReservation is currently an empty TODO, so the newly exposed CancelReservation API will have no effect for DASH streams. Either implement the actual cancellation/truncation behavior (and any required locking/state updates) or return/log an explicit “not supported/unimplemented” outcome so callers can handle it correctly.

Suggested change

// TODO: Actual ad truncation logic, e.g.:

// Find the ad by playingReservationId, mark as truncated at cancelAtReservationId.

AAMPLOG_WARN(

"[CDAI] CancelReservation not supported for DASH. "

"playingReservationId=%s, cancelAtReservationId=%s",

playingReservationId.c_str(),

cancelAtReservationId.c_str());

admanager_mpd.h

priv_aamp.h

Copilot

Pull request overview

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

Copilot · 2026-02-10T11:00:19Z

test/utests/fakes/FakePrivateInstanceAAMP.cpp

+void PrivateInstanceAAMP::CancelReservation(const std::string& playingReservationId, const std::string& cancelAtReservationId) { }
+{
+


CancelReservation in this fake has a syntax/brace issue: the function is defined with an empty body on the signature line and then followed by an extra standalone { ... } block. This will not compile. Also, to match the pattern used by NotifyReservationComplete, this fake should forward the call to g_mockPrivateInstanceAAMP when it is set.

Suggested change

void PrivateInstanceAAMP::CancelReservation(const std::string& playingReservationId, const std::string& cancelAtReservationId) { }

{

void PrivateInstanceAAMP::CancelReservation(const std::string& playingReservationId, const std::string& cancelAtReservationId)

{

if (g_mockPrivateInstanceAAMP != nullptr)

{

g_mockPrivateInstanceAAMP->CancelReservation(playingReservationId, cancelAtReservationId);

}

test/utests/fakes/FakeAdManager.cpp

Copilot · 2026-02-10T11:00:20Z

admanager_mpd.h

 	*/
 	AdNode(bool invalid, bool placed, bool resolved, std::string adId, std::string url, uint64_t duration,
-		std::string basePeriodId, int basePeriodOffset, MPD* mpd)
+		std::string basePeriodId, int basePeriodOffset, MPD* mpd, std::string cancelAtReservationId)


The AdNode constructor signature was changed to require an additional cancelAtReservationId argument, which breaks existing call sites that construct AdNode with the previous parameter list (e.g., multiple emplace_back(false, false, false, ...) usages in unit tests). To avoid widespread churn and keep backward compatibility, consider adding a default value for the new parameter or providing an overload that preserves the old signature.

Suggested change

std::string basePeriodId, int basePeriodOffset, MPD* mpd, std::string cancelAtReservationId)

std::string basePeriodId, int basePeriodOffset, MPD* mpd, std::string cancelAtReservationId = std::string())

jsbindings/jsmediaplayer.cpp

Copilot · 2026-02-10T11:00:21Z

admanager_mpd.cpp

+    std::lock_guard<std::mutex> lock(mDaiMtx); // Ensure thread safety if ad state is shared
+
+    // Log the action for audit/debug
+    AAMPLOG_INFO("[CDAI] PrivateCDAIObjectMPD::CancelReservation called: playingReservationId=%s, cancelAtReservationId=%s",
+        playingReservationId.c_str(), cancelAtReservationId.c_str());
+
+	// Apply cancellation to the currently playing ad in the current break
+	if (mCurAds && mCurAdIdx >= 0 && mCurAdIdx < (int)mCurAds->size())
+	{
+		mCurAds->at(mCurAdIdx).cancelAtReservationId = cancelAtReservationId;
+		AAMPLOG_INFO("[CDAI]AD playing under reservationId=%s will be truncated at %s.",
+			playingReservationId.c_str(), cancelAtReservationId.c_str());
+	}
+	else
+    {
+        AAMPLOG_WARN("[CDAI] PrivateCDAIObjectMPD::CancelReservation: No current ads list!");
+    }


PrivateCDAIObjectMPD::CancelReservation does not use playingReservationId for any validation/routing (it is only logged). As written, any cancel request will apply to whatever ad is currently pointed to by mCurAds/mCurAdIdx, which can cancel the wrong reservation if multiple breaks/ads are possible. Please either validate that the request matches the currently playing reservation/break or locate the correct ad/break by ID before mutating state.

Suggested change

std::lock_guard<std::mutex> lock(mDaiMtx); // Ensure thread safety if ad state is shared

// Log the action for audit/debug

AAMPLOG_INFO("[CDAI] PrivateCDAIObjectMPD::CancelReservation called: playingReservationId=%s, cancelAtReservationId=%s",

playingReservationId.c_str(), cancelAtReservationId.c_str());

// Apply cancellation to the currently playing ad in the current break

if (mCurAds && mCurAdIdx >= 0 && mCurAdIdx < (int)mCurAds->size())

{

mCurAds->at(mCurAdIdx).cancelAtReservationId = cancelAtReservationId;

AAMPLOG_INFO("[CDAI]AD playing under reservationId=%s will be truncated at %s.",

playingReservationId.c_str(), cancelAtReservationId.c_str());

}

else

{

AAMPLOG_WARN("[CDAI] PrivateCDAIObjectMPD::CancelReservation: No current ads list!");

}

std::lock_guard<std::mutex> lock(mDaiMtx); // Ensure thread safety if ad state is shared

// Log the action for audit/debug

AAMPLOG_INFO("[CDAI] PrivateCDAIObjectMPD::CancelReservation called: playingReservationId=%s, cancelAtReservationId=%s",

playingReservationId.c_str(), cancelAtReservationId.c_str());

// Validate that the playing reservation exists

if (!isAdBreakObjectExist(playingReservationId))

{

AAMPLOG_WARN("[CDAI] CancelReservation: playingReservationId %s not found",

playingReservationId.c_str());

return;

}

AdBreakObject &abObj = mAdBreaks[playingReservationId];

// Ensure the playing reservation has ads associated with it

if (!abObj.ads)

{

AAMPLOG_WARN("[CDAI] CancelReservation: No ads associated with playingReservationId %s",

playingReservationId.c_str());

return;

}

// Ensure that the playing reservation corresponds to the currently active ad break

if (abObj.ads != mCurAds)

{

AAMPLOG_WARN("[CDAI] CancelReservation: playingReservationId %s is not the currently active ad break",

playingReservationId.c_str());

return;

}

// Apply cancellation to the currently playing ad in the current break

if (mCurAds && mCurAdIdx >= 0 && mCurAdIdx < (int)mCurAds->size())

{

mCurAds->at(mCurAdIdx).cancelAtReservationId = cancelAtReservationId;

AAMPLOG_INFO("[CDAI] AD playing under reservationId=%s will be truncated at %s.",

playingReservationId.c_str(), cancelAtReservationId.c_str());

}

else

{

AAMPLOG_WARN("[CDAI] CancelReservation: No current ad to cancel for playingReservationId %s",

playingReservationId.c_str());

}

Copilot · 2026-02-10T11:00:21Z

admanager_mpd.cpp

+    std::lock_guard<std::mutex> lock(mDaiMtx); // Ensure thread safety if ad state is shared
+
+    // Log the action for audit/debug
+    AAMPLOG_INFO("[CDAI] PrivateCDAIObjectMPD::CancelReservation called: playingReservationId=%s, cancelAtReservationId=%s",
+        playingReservationId.c_str(), cancelAtReservationId.c_str());
+
+	// Apply cancellation to the currently playing ad in the current break
+	if (mCurAds && mCurAdIdx >= 0 && mCurAdIdx < (int)mCurAds->size())
+	{
+		mCurAds->at(mCurAdIdx).cancelAtReservationId = cancelAtReservationId;
+		AAMPLOG_INFO("[CDAI]AD playing under reservationId=%s will be truncated at %s.",
+			playingReservationId.c_str(), cancelAtReservationId.c_str());
+	}
+	else
+    {
+        AAMPLOG_WARN("[CDAI] PrivateCDAIObjectMPD::CancelReservation: No current ads list!");
+    }


cancelAtReservationId is assigned on the current AdNode, but there is no corresponding read/use of this field anywhere else in the codebase, so calling CancelReservation currently has no effect on ad playback/truncation behavior. If the intent is to truncate/stop playback at the requested reservation, the cancellation signal needs to be integrated into the ad state machine (for example, into termination/placement logic such as CheckForAdTerminate).

Suggested change

std::lock_guard<std::mutex> lock(mDaiMtx); // Ensure thread safety if ad state is shared

// Log the action for audit/debug

AAMPLOG_INFO("[CDAI] PrivateCDAIObjectMPD::CancelReservation called: playingReservationId=%s, cancelAtReservationId=%s",

playingReservationId.c_str(), cancelAtReservationId.c_str());

// Apply cancellation to the currently playing ad in the current break

if (mCurAds && mCurAdIdx >= 0 && mCurAdIdx < (int)mCurAds->size())

{

mCurAds->at(mCurAdIdx).cancelAtReservationId = cancelAtReservationId;

AAMPLOG_INFO("[CDAI]AD playing under reservationId=%s will be truncated at %s.",

playingReservationId.c_str(), cancelAtReservationId.c_str());

}

else

{

AAMPLOG_WARN("[CDAI] PrivateCDAIObjectMPD::CancelReservation: No current ads list!");

}

std::lock_guard<std::mutex> lock(mDaiMtx); // Ensure thread safety if ad state is shared

// Log the action for audit/debug. Note: actual truncation logic is not implemented

// in the current ad state machine; this call is effectively a no-op.

AAMPLOG_INFO("[CDAI] PrivateCDAIObjectMPD::CancelReservation called: playingReservationId=%s, cancelAtReservationId=%s",

playingReservationId.c_str(), cancelAtReservationId.c_str());

admanager_mpd.cpp

Copilot

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

Copilot

Pull request overview

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

Copilot · 2026-02-12T08:13:04Z

main_aamp.cpp

+void PlayerInstanceAAMP::CancelReservation(const std::string& playingReservationId, const std::string& cancelAtReservationId)
+{
+    if (aamp)
+    {
+        aamp->CancelReservation(playingReservationId, cancelAtReservationId);
+    }
+}


PlayerInstanceAAMP::CancelReservation is indented with spaces, while the rest of this file predominantly uses hard tabs. Please reformat this block to keep indentation consistent with the repository’s style guidelines.

Copilot · 2026-02-12T08:13:05Z

admanager_mpd.h

 	int          basePeriodOffset; /**< Offset of the base period at the beginning of the Ad in milliseconds */
 	MPD*         mpd;              /**< Pointer to the MPD object */
+	std::string  cancelAtReservationId;


AdNode::cancelAtReservationId was added without a field description, while the other AdNode members use /**< ... */ documentation. Please add a brief comment describing what this value represents and when it is set/used.

Copilot · 2026-02-12T08:13:05Z

AdManagerBase.h

+	/**
+	 * @fn cancelReservation
+	 * @brief Cancel ad reservation
+	 * @param[in] playingReservationId The reservation identifier which is currently playing
+	 * @param[in] cancelAtReservationId The reservation identifier which needs to be cancelled
+	 */
+	virtual void CancelReservation(const std::string& playingReservationId, const std::string& cancelAtReservationId) {}


In AdManagerBase.h the Doxygen tag says @fn cancelReservation, but the API method is CancelReservation. This makes the generated docs inconsistent and harder to search; please align the @fn name with the actual method signature/casing.

admanager_mpd.cpp

Copilot · 2026-02-12T08:13:06Z

admanager_mpd.cpp

+
+	// Validate that the request targets the currently playing reservation/break
+	if (!currentBreakId.empty() && (playingReservationId == currentBreakId) &&
+		mCurAds && mCurAdIdx >= 0 && mCurAdIdx < (int)mCurAds->size())


The bounds check mixes signed/unsigned and uses (int)mCurAds->size(). Since mCurAdIdx is an int, a safer/clearer check is mCurAdIdx >= 0 && static_cast<size_t>(mCurAdIdx) < mCurAds->size(). This avoids truncation if size() exceeds INT_MAX and removes the C-style cast.

Suggested change

mCurAds && mCurAdIdx >= 0 && mCurAdIdx < (int)mCurAds->size())

mCurAds && mCurAdIdx >= 0 &&

static_cast<size_t>(mCurAdIdx) < mCurAds->size())

jsbindings/jsmediaplayer.cpp

Copilot · 2026-02-12T08:13:06Z

admanager_mpd.cpp

+ * @brief Cancel the reservation for the ad break
+ * @param[in] playingReservationId Ad break ID to be cancelled
+ * @param[in] cancelAtReservationId Ad break ID at which the cancellation should happen


The parameter docs for PrivateCDAIObjectMPD::CancelReservation are misleading: playingReservationId is described as “Ad break ID to be cancelled”, but the implementation treats it as the currently playing break ID to validate the request. Please update the comment to match the actual behavior/meaning.

Suggested change

* @brief Cancel the reservation for the ad break

* @param[in] playingReservationId Ad break ID to be cancelled

* @param[in] cancelAtReservationId Ad break ID at which the cancellation should happen

* @brief Cancel (truncate) the currently playing ad break at a later reservation

* @param[in] playingReservationId ID of the ad break that is currently playing;

* used to validate that the cancellation request targets the active break

* @param[in] cancelAtReservationId ID of the ad break at which the currently

* playing ad should be cancelled/truncated

admanager_mpd.cpp

jsbindings/jsmediaplayer.cpp

admanager_mpd.cpp

Vinish100 · 2026-02-13T08:52:21Z

admanager_mpd.cpp

+		for (const auto &entry : mAdBreaks)
+		{
+			const AdBreakObject &abObj = entry.second;
+			if (abObj.ads && (abObj.ads.get() == mCurAds.get()))


abObj.ads.get() == mCurAds.get()) not required.
This adbreak should be the current one getting placed. We should check PlacementObj and confirm this adbreak is currently in progress

Vinish100 · 2026-02-13T08:53:48Z

admanager_mpd.cpp

+	if (!currentBreakId.empty() && (playingReservationId == currentBreakId) &&
+		mCurAds && mCurAdIdx >= 0 && mCurAdIdx < (int)mCurAds->size())
+	{
+		mCurAds->at(mCurAdIdx).cancelAtReservationId = cancelAtReservationId;


The cancelAtReservationId needs to be updated in adbreak and not ads

admanager_mpd.h

Reason for change:cancelReservation UVE call Test Procedure: Refer jira ticket Priority: P1 Signed-off-by: varshnie <varshniblue14@gmail.com>

Copilot

Pull request overview

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

Copilot · 2026-02-13T14:27:50Z

admanager_mpd.cpp

+	const bool isPlacementInProgress = (!mPlacementObj.pendingAdbrkId.empty());
+	const bool isTargetCurrentPlacement = (isPlacementInProgress && (playingReservationId == mPlacementObj.pendingAdbrkId));
+
+	if (isTargetCurrentPlacement && mCurAds && mCurAdIdx >= 0 && mCurAdIdx < (int)mCurAds->size())


The condition mCurAdIdx >= 0 is redundant. In C++, comparing an unsigned type (from mCurAds->size() which returns size_t) with a signed int can lead to unexpected behavior if mCurAdIdx is negative. Since the right side of the condition already checks the upper bound using a cast to int, the left side should either consistently use unsigned types, or the variable mCurAdIdx should be validated for negativity separately. According to the test case CancelReservation_OutOfRangeIndex_NoChange, a value of 5 (positive but out of range) is tested, but negative values are not explicitly handled or tested.

Suggested change

if (isTargetCurrentPlacement && mCurAds && mCurAdIdx >= 0 && mCurAdIdx < (int)mCurAds->size())

if (isTargetCurrentPlacement

&& mCurAds

&& mCurAdIdx >= 0

&& static_cast<size_t>(mCurAdIdx) < mCurAds->size())

Copilot · 2026-02-13T14:27:50Z

jsbindings/jsmediaplayer.cpp

+	char* playingReservationId_c = aamp_JSValueToCString(ctx, arguments[0], exception);
+	char* cancelAtReservationId_c = aamp_JSValueToCString(ctx, arguments[1], exception);
+
+	std::string playingReservationId = playingReservationId_c ? playingReservationId_c : "";
+	std::string cancelAtReservationId = cancelAtReservationId_c ? cancelAtReservationId_c : "";
+
+	SAFE_DELETE_ARRAY(playingReservationId_c);
+	SAFE_DELETE_ARRAY(cancelAtReservationId_c);


According to the AAMP codebase modernization guidelines, C-style manual memory management with raw pointers should be avoided in favor of modern C++ smart pointers and RAII. The code uses aamp_JSValueToCString which returns a raw char* pointer and then manually deallocates it using SAFE_DELETE_ARRAY. While this pattern is consistent with other parts of the JavaScript bindings in this file, for new code, consider using std::unique_ptr<char[]> with a custom deleter or wrapping the allocation in an RAII guard to ensure exception safety and clearer ownership semantics.

Copilot · 2026-02-13T14:27:51Z

AdManagerBase.h

 	virtual void NotifyReservationComplete(const std::string& reservationId) {}
+
+	/**
+	 * @fn cancelReservation


The Doxygen documentation uses @fn tag unnecessarily. Since this documentation is directly above the function declaration, the @fn tag should be removed. Only @brief is needed when the documentation is adjacent to the declaration.

Suggested change

* @fn cancelReservation

Copilot · 2026-02-13T14:27:51Z

priv_aamp.cpp

+ * @brief Cancel ad reservation
+ * @param[in] playingReservationId The reservation identifier of the currently playing ad, if any. This can be used by the CDAIObject to determine if the cancellation is for an ad that is currently playing.
+ * @param[in] cancelAtReservationId The reservation identifier of the ad reservation to be cancelled. This is the reservation that the CDAIObject should cancel. If this matches the playingReservationId, it indicates that the currently playing ad reservation should be cancelled.


The documentation's description of the parameters is unclear and potentially contradictory. Lines 9250-9251 state that playingReservationId is "of the currently playing ad, if any" and that if it "matches the playingReservationId" (likely meant to say "matches the cancelAtReservationId"), it indicates the currently playing ad should be cancelled. However, based on the implementation in admanager_mpd.cpp (lines 1931-1957), the function sets cancelAtReservationId as a marker on the ad break identified by playingReservationId, suggesting it's used to mark when to cancel in the future, not that the currently playing ad should be cancelled immediately. The documentation should clarify the intended behavior and align with the implementation.

Suggested change

* @brief Cancel ad reservation

* @param[in] playingReservationId The reservation identifier of the currently playing ad, if any. This can be used by the CDAIObject to determine if the cancellation is for an ad that is currently playing.

* @param[in] cancelAtReservationId The reservation identifier of the ad reservation to be cancelled. This is the reservation that the CDAIObject should cancel. If this matches the playingReservationId, it indicates that the currently playing ad reservation should be cancelled.

* @brief Request cancellation of an ad reservation

* @param[in] playingReservationId The reservation identifier of the ad break

* that is currently playing (if any). The

* CDAIObject uses this to locate the active

* ad reservation whose behavior should be

* modified.

* @param[in] cancelAtReservationId The reservation identifier that marks

* when cancellation should take effect.

* Implementations (for example,

* admanager_mpd) may use this as a marker

* on the ad break identified by

* playingReservationId to indicate the

* point in the ad sequence at which

* playback should be cancelled and return

* to content. If this value is the same as

* playingReservationId, it indicates that

* cancellation should occur at the current

* reservation rather than a future one.

test/utests/tests/AdManagerMPDTests/FunctionalTests.cpp

Copilot · 2026-02-13T14:27:53Z

priv_aamp.cpp

+    if (mCdaiObject) {
+        mCdaiObject->CancelReservation(playingReservationId, cancelAtReservationId);
+    }


Inconsistent indentation: lines 9255-9257 use spaces for indentation while the surrounding code (lines 9258-9261) uses tabs. According to the project's coding guidelines, indentation should use hard tabs (4-space width). Lines 9255-9257 should be converted to use tabs to maintain consistency with the rest of the codebase.

Suggested change

if (mCdaiObject) {

mCdaiObject->CancelReservation(playingReservationId, cancelAtReservationId);

}

if (mCdaiObject) {

mCdaiObject->CancelReservation(playingReservationId, cancelAtReservationId);

}

Copilot · 2026-02-13T14:27:53Z

test/utests/tests/AdManagerMPDTests/FunctionalTests.cpp

+ * @brief CancelReservation should set cancelAtReservationId on the current ad.
+ */
+TEST_F(AdManagerMPDTests, CancelReservation_SetsCancelIdOnCurrentAd)


The test name CancelReservation_SetsCancelIdOnCurrentAd is slightly misleading. According to the implementation (admanager_mpd.cpp line 1947), the cancellation marker is set at the AdBreak level (not on the current ad/AdNode), as stated in the comment at line 1945. A more accurate test name would be CancelReservation_SetsCancelIdOnAdBreak or CancelReservation_SetsCancelIdOnCurrentAdBreak to reflect that the marker applies to the entire ad break.

Suggested change

* @brief CancelReservation should set cancelAtReservationId on the current ad.

*/

TEST_F(AdManagerMPDTests, CancelReservation_SetsCancelIdOnCurrentAd)

* @brief CancelReservation should set cancelAtReservationId on the current ad break.

*/

TEST_F(AdManagerMPDTests, CancelReservation_SetsCancelIdOnAdBreak)

Copilot · 2026-02-13T14:27:53Z

admanager_mpd.cpp

+	const bool isPlacementInProgress = (!mPlacementObj.pendingAdbrkId.empty());
+	const bool isTargetCurrentPlacement = (isPlacementInProgress && (playingReservationId == mPlacementObj.pendingAdbrkId));
+
+	if (isTargetCurrentPlacement && mCurAds && mCurAdIdx >= 0 && mCurAdIdx < (int)mCurAds->size())


Missing test coverage for the case where mCurAds is nullptr. The implementation at line 1943 checks mCurAds before dereferencing, but there's no test case that validates the behavior when this condition is false. Adding a test case where mCurAds is null would improve coverage and ensure the guard condition works as expected.

Copilot · 2026-02-13T14:27:54Z

admanager_mpd.cpp

+	const bool isPlacementInProgress = (!mPlacementObj.pendingAdbrkId.empty());
+	const bool isTargetCurrentPlacement = (isPlacementInProgress && (playingReservationId == mPlacementObj.pendingAdbrkId));


Missing test coverage for the case where mPlacementObj.pendingAdbrkId is empty (no placement in progress). This is one of the conditions checked at lines 1940-1941. A test case covering this scenario would ensure the function correctly ignores cancellation requests when there's no active placement.

Copilot · 2026-02-13T14:27:54Z

test/utests/tests/AdManagerMPDTests/FunctionalTests.cpp

+}
+
+/**
+ * @brief CancelReservation should set cancelAtReservationId on the current ad.


The comment states "CancelReservation should set cancelAtReservationId on the current ad" but the implementation sets it on the AdBreakObject (line 1947 in admanager_mpd.cpp), not on an individual ad (AdNode). The comment should be corrected to say "CancelReservation should set cancelAtReservationId on the current ad break" to accurately reflect what the test validates.

Suggested change

* @brief CancelReservation should set cancelAtReservationId on the current ad.

* @brief CancelReservation should set cancelAtReservationId on the current ad break.

Copilot AI review requested due to automatic review settings February 10, 2026 09:05

varshnie requested a review from a team as a code owner February 10, 2026 09:05

Copilot started reviewing on behalf of varshnie February 10, 2026 09:05 View session

Copilot AI reviewed Feb 10, 2026

View reviewed changes

varshnie force-pushed the feature/VPLAY-12568 branch from ffdd52b to 23db3c1 Compare February 10, 2026 09:39

Copilot AI review requested due to automatic review settings February 10, 2026 10:54

varshnie force-pushed the feature/VPLAY-12568 branch from 23db3c1 to b755876 Compare February 10, 2026 10:54

Copilot started reviewing on behalf of varshnie February 10, 2026 10:54 View session

Copilot AI reviewed Feb 10, 2026

View reviewed changes

varshnie force-pushed the feature/VPLAY-12568 branch from b755876 to 183997a Compare February 11, 2026 06:49

Copilot AI review requested due to automatic review settings February 11, 2026 15:41

varshnie force-pushed the feature/VPLAY-12568 branch from 183997a to 324e893 Compare February 11, 2026 15:41

Copilot started reviewing on behalf of varshnie February 11, 2026 15:42 View session

Copilot AI reviewed Feb 11, 2026

View reviewed changes

varshnie force-pushed the feature/VPLAY-12568 branch from 324e893 to d054ea6 Compare February 12, 2026 07:42

Copilot AI review requested due to automatic review settings February 12, 2026 08:07

varshnie force-pushed the feature/VPLAY-12568 branch from d054ea6 to d116573 Compare February 12, 2026 08:07

Copilot started reviewing on behalf of varshnie February 12, 2026 08:08 View session

varshnie force-pushed the feature/VPLAY-12568 branch from d116573 to f1d9403 Compare February 12, 2026 08:10

Copilot AI reviewed Feb 12, 2026

View reviewed changes

Vinish100 reviewed Feb 13, 2026

View reviewed changes

varshnie force-pushed the feature/VPLAY-12568 branch from f1d9403 to 74c3559 Compare February 13, 2026 14:00

VPLAY-12568:Implement cancelReservation UVE-API

ac8b88f

Reason for change:cancelReservation UVE call Test Procedure: Refer jira ticket Priority: P1 Signed-off-by: varshnie <varshniblue14@gmail.com>

Copilot AI review requested due to automatic review settings February 13, 2026 14:21

varshnie force-pushed the feature/VPLAY-12568 branch from 74c3559 to ac8b88f Compare February 13, 2026 14:21

Copilot started reviewing on behalf of varshnie February 13, 2026 14:22 View session

Copilot AI reviewed Feb 13, 2026

View reviewed changes

		std::string playingReservationId = aamp_JSValueToCString(ctx, arguments[0], exception);
		std::string cancelAtReservationId = aamp_JSValueToCString(ctx, arguments[1], exception);

-    std::string playingReservationId = aamp_JSValueToCString(ctx, arguments[0], exception);
-    std::string cancelAtReservationId = aamp_JSValueToCString(ctx, arguments[1], exception);
+    char *playingReservationIdCStr = aamp_JSValueToCString(ctx, arguments[0], exception);
+    std::string playingReservationId;
+    if (playingReservationIdCStr)
+    {
+        playingReservationId.assign(playingReservationIdCStr);
+    }
+    SAFE_DELETE_ARRAY(playingReservationIdCStr);
+    char *cancelAtReservationIdCStr = aamp_JSValueToCString(ctx, arguments[1], exception);
+    std::string cancelAtReservationId;
+    if (cancelAtReservationIdCStr)
+    {
+        cancelAtReservationId.assign(cancelAtReservationIdCStr);
+    }
+    SAFE_DELETE_ARRAY(cancelAtReservationIdCStr);

		// TODO: Actual ad truncation logic, e.g.:
		// Find the ad by playingReservationId, mark as truncated at cancelAtReservationId.

		void PrivateInstanceAAMP::CancelReservation(const std::string& playingReservationId, const std::string& cancelAtReservationId) { }
		{

	std::string basePeriodId, int basePeriodOffset, MPD* mpd, std::string cancelAtReservationId)
	std::string basePeriodId, int basePeriodOffset, MPD* mpd, std::string cancelAtReservationId = std::string())

	mCurAds && mCurAdIdx >= 0 && mCurAdIdx < (int)mCurAds->size())
	mCurAds && mCurAdIdx >= 0 &&
	static_cast<size_t>(mCurAdIdx) < mCurAds->size())

- * @brief Cancel the reservation for the ad break
- * @param[in] playingReservationId Ad break ID to be cancelled
- * @param[in] cancelAtReservationId Ad break ID at which the cancellation should happen
+ * @brief Cancel (truncate) the currently playing ad break at a later reservation
+ * @param[in] playingReservationId ID of the ad break that is currently playing;
+ *            used to validate that the cancellation request targets the active break
+ * @param[in] cancelAtReservationId ID of the ad break at which the currently
+ *            playing ad should be cancelled/truncated

- * @brief Cancel ad reservation
- * @param[in] playingReservationId The reservation identifier of the currently playing ad, if any. This can be used by the CDAIObject to determine if the cancellation is for an ad that is currently playing.
- * @param[in] cancelAtReservationId The reservation identifier of the ad reservation to be cancelled. This is the reservation that the CDAIObject should cancel. If this matches the playingReservationId, it indicates that the currently playing ad reservation should be cancelled.
+ * @brief Request cancellation of an ad reservation
+ * @param[in] playingReservationId The reservation identifier of the ad break
+ *                                 that is currently playing (if any). The
+ *                                 CDAIObject uses this to locate the active
+ *                                 ad reservation whose behavior should be
+ *                                 modified.
+ * @param[in] cancelAtReservationId The reservation identifier that marks
+ *                                  when cancellation should take effect.
+ *                                  Implementations (for example,
+ *                                  admanager_mpd) may use this as a marker
+ *                                  on the ad break identified by
+ *                                  playingReservationId to indicate the
+ *                                  point in the ad sequence at which
+ *                                  playback should be cancelled and return
+ *                                  to content. If this value is the same as
+ *                                  playingReservationId, it indicates that
+ *                                  cancellation should occur at the current
+ *                                  reservation rather than a future one.

		const bool isPlacementInProgress = (!mPlacementObj.pendingAdbrkId.empty());
		const bool isTargetCurrentPlacement = (isPlacementInProgress && (playingReservationId == mPlacementObj.pendingAdbrkId));

	* @brief CancelReservation should set cancelAtReservationId on the current ad.
	* @brief CancelReservation should set cancelAtReservationId on the current ad break.

Conversation

varshnie commented Feb 10, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Copilot AI Feb 10, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 10, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Copilot AI Feb 10, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Feb 10, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI Feb 10, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI Feb 10, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 10, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Feb 12, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 12, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 12, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Copilot AI Feb 12, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI Feb 12, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Vinish100 Feb 13, 2026

Choose a reason for hiding this comment

Uh oh!

Vinish100 Feb 13, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview