VPLAY-12313: Implement notifyReservationComplete API #883

srikanthreddybijjam-comcast · 2026-01-21T08:54:38Z

Reason for change: Added notifyReservationAPI
Risks: Low
Test Procedure: Refer jira ticket VPLAY-12313
Priority: P1

Copilot

Pull request overview

This pull request implements the notifyReservationComplete API to notify AAMP when ad reservation is complete for a given reservation ID and time. The API is exposed through the JavaScript bindings and flows through the PlayerInstanceAAMP and PrivateInstanceAAMP layers down to the ad manager implementation.

Changes:

Added notifyReservationComplete API across the public interface chain (JS bindings → PlayerInstanceAAMP → PrivateInstanceAAMP → PrivateCDAIObjectMPD)
Implemented reservation completion logic in PrivateCDAIObjectMPD to mark ad breaks as placed and resolved
Added corresponding fake implementation for testing framework

Reviewed changes

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

Show a summary per file

File	Description
jsbindings/jsmediaplayer.cpp	Added JavaScript binding to call notifyReservationComplete API with reservation ID and time parameters
main_aamp.h	Declared public API method in PlayerInstanceAAMP interface
main_aamp.cpp	Implemented forwarding wrapper from PlayerInstanceAAMP to PrivateInstanceAAMP
priv_aamp.h	Declared API method in PrivateInstanceAAMP class
priv_aamp.cpp	Implemented logging in PrivateInstanceAAMP (missing propagation to CDAI object)
admanager_mpd.h	Declared notifyReservationComplete in PrivateCDAIObjectMPD class
admanager_mpd.cpp	Implemented reservation completion logic with mutex protection, marking ad breaks as placed/resolved
test/utests/fakes/FakePlayerInstanceAamp.cpp	Added fake implementation stub for testing

Copilot · 2026-01-21T09:01:19Z

test/utests/fakes/FakePlayerInstanceAamp.cpp

 	void PlayerInstanceAAMP::SetContentProtectionDataUpdateTimeout(int timeout) {  }
 	void PlayerInstanceAAMP::ProcessContentProtectionDataConfig(const char *jsonbuffer) {  }
 	void PlayerInstanceAAMP::SetRuntimeDRMConfigSupport(bool DynamicDRMSupported) {  }
+	void PlayerInstanceAAMP::notifyReservationComplete(const std::string& reservationId, long time) {  }


Use fixed-width integer type for cross-platform consistency: The parameter 'time' is declared as 'long', which has platform-dependent size. This should use a fixed-width type like int64_t or uint64_t from <cstdint> to ensure consistent memory layout across platforms and match the updated signature of the actual implementation.

Copilot · 2026-01-21T09:01:19Z

priv_aamp.cpp

+ */
+void PrivateInstanceAAMP::notifyReservationComplete(const std::string& reservationId, long time)
+{
+	AAMPLOG_INFO("[AAMP] Reservation complete for reservationId: %s at time: %ld", reservationId.c_str(), time);


Missing implementation - method only logs and does not propagate to the CDAI subsystem: The PrivateInstanceAAMP::notifyReservationComplete method only logs the reservation completion but does not forward the call to the mCdaiObject where the actual implementation exists (in PrivateCDAIObjectMPD::notifyReservationComplete at admanager_mpd.cpp line 1881). Following the existing pattern used by other methods like SetAlternateContents (see priv_aamp.cpp line 9771), this method should check if mCdaiObject exists and is of type CDAIObjectMPD, then call the corresponding method on the CDAI object to properly handle the reservation completion. Without this, the reservation state in the ad manager is never updated, making this API non-functional.

Suggested change

AAMPLOG_INFO("[AAMP] Reservation complete for reservationId: %s at time: %ld", reservationId.c_str(), time);

AAMPLOG_INFO("[AAMP] Reservation complete for reservationId: %s at time: %ld", reservationId.c_str(), time);

// Forward reservation completion to CDAI subsystem for DASH streams

if (mCdaiObject && (mMediaFormat == eMEDIAFORMAT_DASH))

{

PrivateCDAIObjectMPD *cdaiObjectMpd = dynamic_cast<PrivateCDAIObjectMPD *>(mCdaiObject);

if (cdaiObjectMpd)

{

cdaiObjectMpd->notifyReservationComplete(reservationId, time);

}

}

Copilot · 2026-01-21T09:01:19Z

admanager_mpd.cpp

+ * @param[in] reservationId Ad break ID
+ * @param[in] time Time to mark the reservation complete
+ */
+void PrivateCDAIObjectMPD::notifyReservationComplete(const std::string& reservationId, long time)


Use fixed-width integer type for cross-platform consistency: The parameter 'time' is declared as 'long', which has platform-dependent size. This should use a fixed-width type like int64_t or uint64_t from <cstdint> to ensure consistent memory layout across platforms. This applies to both the method signature and all logging statements using the %ld format specifier (which should be changed to the appropriate macro like PRId64 or PRIu64).

Copilot · 2026-01-21T09:01:20Z

priv_aamp.h

+	 * @brief Notify AAMP that ad reservation is complete for a given reservationId and time
+	 * @param[in] reservationId The reservation identifier
+	 * @param[in] time The Reservation complete time in milliseconds


Unclear parameter semantics: The documentation states the 'time' parameter is "The Reservation complete time in milliseconds", but it's unclear what this time represents. Is it a timestamp (e.g., epoch time), an offset from the start of the stream, a duration, or something else? The implementation logs it but doesn't use it for any logic. The documentation should clarify: (1) What is the base/origin of this time value? (2) How should it be used by the ad manager implementation? (3) What happens if the time parameter doesn't match expected boundaries (e.g., negative, zero, or beyond the ad break duration)?

Suggested change

* @brief Notify AAMP that ad reservation is complete for a given reservationId and time

* @param[in] reservationId The reservation identifier

* @param[in] time The Reservation complete time in milliseconds

* @brief Notify AAMP that an ad reservation has completed.

*

* This API is typically called by an external ad subsystem when playback

* has reached the end of a reserved ad break.

*

* @param[in] reservationId Unique identifier of the reservation / ad break.

* @param[in] time Playback position, in milliseconds, at which the

* reservation completed. The value is an offset from the start

* of the main content timeline (i.e., 0 = stream start) and is

* expected to be monotonically increasing while the stream

* plays.

*

* @note AAMP core currently uses this value only for logging and

* diagnostics; it does not affect playback state or scheduling

* logic. No validation is performed on the value, and negative,

* zero, or out-of-range values are accepted but should be avoided

* by callers. Ad manager implementations may use this value for

* correlating with their own timelines or reporting, but should

* treat obviously invalid values as unspecified.

Copilot · 2026-01-21T09:01:20Z

admanager_mpd.cpp

+		AdBreakObject& abObj = mAdBreaks[reservationId];
+		abObj.mAdBreakPlaced = true;
+		abObj.resolved = true;
+		AAMPLOG_INFO("[CDAI] Marked reservation complete for adBreakId: %s at time: %ld", reservationId.c_str(), time);


Unused parameter: The 'time' parameter is logged but not used in any logic within this method. The implementation sets mAdBreakPlaced and resolved flags, and potentially changes mAdState, but none of these operations use the time value. If the time parameter is meant to validate or constrain the reservation completion (e.g., to trim ads or verify timing), that logic is missing. Either implement the intended behavior using this parameter, or clarify in documentation why it's only logged and when/how it might be used in future.

Copilot · 2026-01-21T09:01:22Z

main_aamp.cpp

+void PlayerInstanceAAMP::notifyReservationComplete(const std::string& reservationId, long time)
+{
+	aamp->notifyReservationComplete(reservationId, time);
+}


Missing test coverage: The newly added public API PlayerInstanceAAMP::notifyReservationComplete lacks unit tests. According to the testing guidelines, all public functions require unit tests. Test cases should be added to verify that the call is properly forwarded from PlayerInstanceAAMP to the underlying PrivateInstanceAAMP (aamp) object. Tests should be placed in test/utests/tests/PlayerInstanceAAMP/ following the existing test pattern.

Copilot · 2026-01-21T09:01:22Z

priv_aamp.cpp

+void PrivateInstanceAAMP::notifyReservationComplete(const std::string& reservationId, long time)
+{
+	AAMPLOG_INFO("[AAMP] Reservation complete for reservationId: %s at time: %ld", reservationId.c_str(), time);
+}


Missing test coverage: The newly added public API PrivateInstanceAAMP::notifyReservationComplete lacks unit tests. According to the testing guidelines, all public functions require unit tests. Once the implementation is fixed to properly forward to the CDAI object, test cases should be added to verify: (1) the call is properly forwarded to mCdaiObject when it exists and is a CDAIObjectMPD, (2) graceful handling when mCdaiObject is null, and (3) behavior when mCdaiObject is not a CDAIObjectMPD type. Tests should be placed in test/utests/tests/PrivAampTests/ following the existing test pattern.

Copilot · 2026-01-21T09:01:22Z

priv_aamp.h

+	 * @param[in] reservationId The reservation identifier
+	 * @param[in] time The Reservation complete time in milliseconds
+	 */
+	void notifyReservationComplete(const std::string& reservationId, long time);


Use fixed-width integer type for cross-platform consistency: The parameter 'time' is declared as 'long', which has platform-dependent size (32 bits on some platforms, 64 bits on others). Since this API may be called from JavaScript bindings where ctypes is used, it should use a fixed-width type like int64_t or uint64_t from <cstdint> to ensure consistent memory layout across platforms. The documentation states this is in milliseconds, so int64_t would be appropriate if it can be negative, or uint64_t if it's always non-negative.

Copilot · 2026-01-21T09:01:23Z

main_aamp.cpp

+/**
+ *  @brief Notify AAMP that ad reservation is complete for a given reservationId and time
+ */
+void PlayerInstanceAAMP::notifyReservationComplete(const std::string& reservationId, long time)


Use fixed-width integer type for cross-platform consistency: The parameter 'time' is declared as 'long', which has platform-dependent size. Since this API is called from JavaScript bindings where ctypes is used, it should use a fixed-width type like int64_t or uint64_t from <cstdint> to ensure consistent memory layout across platforms. This applies to both the method signature and the corresponding forwarding call to aamp->notifyReservationComplete.

Copilot · 2026-01-21T09:01:23Z

admanager_mpd.cpp

+	}
+	else
+	{
+			   AAMPLOG_WARN("[CDAI] notifyReservationComplete: adBreakId %s not found", reservationId.c_str());


Inconsistent indentation: Line 1899 has excessive indentation (appears to have extra tabs/spaces) compared to the surrounding code at the same nesting level. The AAMPLOG_WARN line should be aligned with the opening brace on line 1898.

Suggested change

AAMPLOG_WARN("[CDAI] notifyReservationComplete: adBreakId %s not found", reservationId.c_str());

AAMPLOG_WARN("[CDAI] notifyReservationComplete: adBreakId %s not found", reservationId.c_str());

Copilot

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

…AAMP Reason for change: Added API and callbacks to aamp. Test Procedure: Refer jira ticket VPLAY-12313 Priority: P1 Signed-off-by: srikanthreddybijjam-comcast <srikanthreddybijjam.2000@gmail.com>

Copilot

Pull request overview

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

Copilot · 2026-01-23T07:24:04Z

AdManagerBase.h

+	/**
+	 * @brief Mark reservation as complete for a given reservationId and time
+	 * @param[in] reservationId The reservation identifier
+	 * @param[in] time The Reservation complete time in milliseconds
+	 */
+	virtual void notifyReservationComplete(const std::string& reservationId, long time) {}


CDAIObject::notifyReservationComplete takes long time even though the doc says milliseconds. Since this method is part of the interface and is called across components (and from JS), using long can be non-portable. Prefer a fixed-width type (e.g., int64_t) to ensure consistent behavior on 32-bit and 64-bit platforms.

Copilot · 2026-01-23T07:24:05Z

main_aamp.h

+	/**
+	 * @brief Notify AAMP that ad reservation is complete for a given reservationId and time
+	 * @param[in] reservationId The reservation identifier
+	 * @param[in] time The Reservation complete time in milliseconds
+	 */
+	void notifyReservationComplete(const std::string& reservationId, long time);


PlayerInstanceAAMP methods in this header use PascalCase (e.g., Tune, Stop, SetRate). The newly added notifyReservationComplete is lowerCamelCase, which is inconsistent with the existing public API naming and the project C++ naming guidelines. Consider renaming to NotifyReservationComplete (and updating callers) for consistency before this API becomes widely used.

Copilot · 2026-01-23T07:24:05Z

priv_aamp.h

+	/**
+	 * @brief Notify AAMP that ad reservation is complete for a given reservationId and time
+	 * @param[in] reservationId The reservation identifier
+	 * @param[in] time The Reservation complete time in milliseconds
+	 */
+	void notifyReservationComplete(const std::string& reservationId, long time);


PrivateInstanceAAMP already exposes similar notification-style APIs with PascalCase (e.g., NotifySpeedChanged, NotifyOnEnteringLive). The new notifyReservationComplete deviates from that pattern. Consider renaming it to NotifyReservationComplete to keep notification APIs consistent and easier to discover.

Copilot · 2026-01-23T07:24:05Z

admanager_mpd.cpp

+	if (isAdBreakObjectExist(reservationId))
+	{
+		AdBreakObject& abObj = mAdBreaks[reservationId];
+		//abObj.mAdBreakPlaced = true;


Please avoid leaving commented-out code in new logic (//abObj.mAdBreakPlaced = true;). Either remove it or implement the intended behavior, so the reservation-complete semantics are unambiguous.

Suggested change

//abObj.mAdBreakPlaced = true;

Copilot · 2026-01-23T07:24:05Z

main_aamp.h

+	 */
+	void notifyReservationComplete(const std::string& reservationId, long time);


The API uses long time for a millisecond timestamp. long is not a fixed-width type and can be 32-bit on some platforms, which risks truncation for large timestamps. Consider using a fixed-width integer (e.g., int64_t) consistently across PlayerInstanceAAMP/PrivateInstanceAAMP/CDAIObject interfaces and updating log formatting (PRId64) where needed.

Copilot · 2026-01-23T07:24:06Z

admanager_mpd.cpp

+/**
+ * @brief Mark the reservation as complete for the ad break
+ * @param[in] reservationId Ad break ID
+ * @param[in] time Time to mark the reservation complete
+ */
+void PrivateCDAIObjectMPD::notifyReservationComplete(const std::string& reservationId, long time)
+{
+	std::lock_guard<std::mutex> lock(mDaiMtx);
+	if (isAdBreakObjectExist(reservationId))
+	{
+		AdBreakObject& abObj = mAdBreaks[reservationId];
+		//abObj.mAdBreakPlaced = true;
+		abObj.resolved = true;
+		AAMPLOG_INFO("[CDAI] Marked reservation complete for adBreakId: %s at time: %ld", reservationId.c_str(), time);
+		if (!abObj.ads || abObj.ads->empty())
+		{
+			AAMPLOG_INFO("[CDAI] Ad break %s is empty. No ads to play.", reservationId.c_str());
+			AbortWaitForNextAdResolved();
+			mAdFulfillCV.notify_all();
+		}
+	}
+	else
+	{
+		AAMPLOG_WARN("[CDAI] notifyReservationComplete: adBreakId %s not found", reservationId.c_str());
+	}
+}


PrivateCDAIObjectMPD::notifyReservationComplete introduces new state transitions (marking ad break resolved, handling empty breaks, notifying condition variables). There are existing AdManagerMPD unit tests, but none cover this new path. Please add tests to validate: (1) resolved flag set when break exists, (2) no-op + warning when break is missing, and (3) empty-break path triggers the expected wake-ups.

srikanthreddybijjam-comcast requested a review from a team as a code owner January 21, 2026 08:54

Copilot AI review requested due to automatic review settings January 21, 2026 08:54

Copilot started reviewing on behalf of srikanthreddybijjam-comcast January 21, 2026 08:55 View session

Copilot AI reviewed Jan 21, 2026

View reviewed changes

srikanthreddybijjam-comcast force-pushed the feature/VPLAY-12313 branch from 436371c to 8bf6ca1 Compare January 23, 2026 05:27

VPLAY-12313: VPLAY-12174: Implement notifyReservationComplete API in …

358ecd6

…AAMP Reason for change: Added API and callbacks to aamp. Test Procedure: Refer jira ticket VPLAY-12313 Priority: P1 Signed-off-by: srikanthreddybijjam-comcast <srikanthreddybijjam.2000@gmail.com>

Copilot AI review requested due to automatic review settings January 23, 2026 07:15

srikanthreddybijjam-comcast force-pushed the feature/VPLAY-12313 branch from 8bf6ca1 to 358ecd6 Compare January 23, 2026 07:15

Copilot started reviewing on behalf of srikanthreddybijjam-comcast January 23, 2026 07:15 View session

Copilot AI reviewed Jan 23, 2026

View reviewed changes

-	AAMPLOG_INFO("[AAMP] Reservation complete for reservationId: %s at time: %ld", reservationId.c_str(), time);
+	AAMPLOG_INFO("[AAMP] Reservation complete for reservationId: %s at time: %ld", reservationId.c_str(), time);
+	// Forward reservation completion to CDAI subsystem for DASH streams
+	if (mCdaiObject && (mMediaFormat == eMEDIAFORMAT_DASH))
+	{
+		PrivateCDAIObjectMPD *cdaiObjectMpd = dynamic_cast<PrivateCDAIObjectMPD *>(mCdaiObject);
+		if (cdaiObjectMpd)
+		{
+			cdaiObjectMpd->notifyReservationComplete(reservationId, time);
+		}
+	}

-	 * @brief Notify AAMP that ad reservation is complete for a given reservationId and time
-	 * @param[in] reservationId The reservation identifier
-	 * @param[in] time The Reservation complete time in milliseconds
+	 * @brief Notify AAMP that an ad reservation has completed.
+	 *
+	 * This API is typically called by an external ad subsystem when playback
+	 * has reached the end of a reserved ad break.
+	 *
+	 * @param[in] reservationId Unique identifier of the reservation / ad break.
+	 * @param[in] time Playback position, in milliseconds, at which the
+	 *            reservation completed. The value is an offset from the start
+	 *            of the main content timeline (i.e., 0 = stream start) and is
+	 *            expected to be monotonically increasing while the stream
+	 *            plays.
+	 *
+	 * @note AAMP core currently uses this value only for logging and
+	 *       diagnostics; it does not affect playback state or scheduling
+	 *       logic. No validation is performed on the value, and negative,
+	 *       zero, or out-of-range values are accepted but should be avoided
+	 *       by callers. Ad manager implementations may use this value for
+	 *       correlating with their own timelines or reporting, but should
+	 *       treat obviously invalid values as unspecified.

	AAMPLOG_WARN("[CDAI] notifyReservationComplete: adBreakId %s not found", reservationId.c_str());
	AAMPLOG_WARN("[CDAI] notifyReservationComplete: adBreakId %s not found", reservationId.c_str());

		*/
		void notifyReservationComplete(const std::string& reservationId, long time);

VPLAY-12313: Implement notifyReservationComplete API #883

Are you sure you want to change the base?

VPLAY-12313: Implement notifyReservationComplete API #883

Uh oh!

Conversation

srikanthreddybijjam-comcast commented Jan 21, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Jan 21, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 21, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 21, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 21, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 21, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 21, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 21, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 21, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 21, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 21, 2026

Choose a reason for hiding this comment

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 Jan 23, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 23, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 23, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 23, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 23, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 23, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants