RDKEMW-19159: Update AAMP (dsMgr Client) to Use Existing Thunder Plugin for libds Methods and Event Notifications

[nejuma1]

RDKEMW-19159: Update AAMP (dsMgr Client) to Use Existing Thunder Plugin for libds Methods and Event Notifications
Reason for change : Remove IARM/libds related calls and use firebolt instead
Priority : P1
Test Steps : Mentioned in ticket
Signed off by : nejumatn28@gmail.com

[Copilot]

🟡 Changes recommended

The current CMake/source selection and backend-selection logic are inconsistent (leading to likely link/runtime regressions) and there is a confirmed concurrency race in Firebolt event handler teardown.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

This PR introduces a Firebolt-based device-settings backend path (intended for RDK-E builds) and updates build/test wiring to select Firebolt instead of IARM/libds-related configuration for HDMI/HDCP/resolution handling and event registration.

Changes:

• Add CMAKE_USE_FIREBOLT CMake option and propagate USE_FIREBOLT compile definitions for Firebolt-only builds.
• Route PlayerExternalsRdkInterface initialization and HDMI status handling through DeviceFireboltInterface when USE_FIREBOLT is enabled.
• Add Firebolt-based HDCP + resolution querying and initialization idempotency guards in DeviceFireboltInterface.

File summaries

File	Description
test/utests/run.sh	Switch unit-test CMake invocation to enable Firebolt builds via -DCMAKE_USE_FIREBOLT=ON.
gst-plugins/CMakeLists.txt	Update IARM macro define condition to depend on CMAKE_USE_FIREBOLT.
externals/rdk/PlayerExternalsRdkInterface.h	Gate DS includes for Firebolt builds and add minimal HDCP enum + setter used by Firebolt backend.
externals/rdk/PlayerExternalsRdkInterface.cpp	Add Firebolt-only initialization path and Firebolt-specific SetHDMIStatus() implementation.
externals/rdk/IFirebolt/DeviceFireboltInterface.h	Add mutex state for one-time initialization and declare SetHDMIStatus().
externals/rdk/IFirebolt/DeviceFireboltInterface.cpp	Make Initialize idempotent and implement Firebolt HDCP/resolution query to update player state.
externals/PlayerExternalsInterface.cpp	Allow PlayerExternalsRdkInterface inclusion/instantiation under USE_FIREBOLT as well as IARM_MGR.
externals/CMakeLists.txt	Add Firebolt build option and adjust source/link selection for Firebolt vs IARM/native builds.

Review details

Suppressed comments (1)

externals/CMakeLists.txt:202

• playerfbinterface links FireboltAamp only when CMAKE_USE_FIREBOLT is set, but the RDK-V backend selection code can still instantiate DeviceFireboltInterface; if Firebolt backend sources are built for RDK-V, FireboltAamp must be linked as well.

if(CMAKE_USE_SECCLIENT OR CMAKE_USE_SECMANAGER OR CMAKE_USE_FIREBOLT)
   find_package(FireboltAamp CONFIG REQUIRED)
   target_link_libraries(playerfbinterface FireboltAamp::FireboltAamp)
endif()

• Files reviewed: 8/8 changed files
• Comments generated: 3
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

🟡 Changes recommended

There are confirmed build/behavioral issues in the new Firebolt gating and concurrency/safety handling (notably DS-event macro conflicts on Firebolt builds and a mutex-protected flag updated without locking) that should be fixed before approval.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (2)

externals/rdk/PlayerExternalsRdkInterface.h:31

• USE_DS_EVENT_SUPPORTED can still be defined in Firebolt-only builds (e.g., unit tests), but the DS/DeviceSettings headers are excluded under USE_FIREBOLT, which will cause compilation errors when the class tries to inherit DS event interfaces. Consider explicitly disabling USE_DS_EVENT_SUPPORTED when USE_FIREBOLT is enabled (or emitting a clear preprocessor error) to prevent an invalid macro combination.

#ifndef USE_FIREBOLT
#include "manager.hpp"
#include "host.hpp"
#include "videoResolution.hpp"
#include "videoOutputPort.hpp"

externals/rdk/IFirebolt/DeviceFireboltInterface.cpp:150

• m_isInitialized is protected by m_initMutex in Initialize(), but RemoveEventHandlers() updates it without taking the same lock. This creates a data race if Initialize() / RemoveEventHandlers() are called concurrently (including via the destructor), which can lead to double-registration or skipped registration.

void DeviceFireboltInterface::RemoveEventHandlers()
{
	std::lock_guard<std::mutex> lock(m_initMutex);
	m_isInitialized = false;
	//removes everything ...

• Files reviewed: 8/8 changed files
• Comments generated: 2
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

There are confirmed build-configuration and macro-gating issues (notably around Firebolt vs DS event support and IARM macro enablement) that can cause incorrect compilation paths or misleading test/build behavior.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (3)

Previously missed (2) — in code that hasn't changed since the last review.

externals/CMakeLists.txt:140

• add_definitions(-DUSE_FIREBOLT) sets a global compile definition for all targets added after this subdirectory, but this CMakeLists already wires USE_FIREBOLT into playerfbinterface via LIB_EXT_DEFINES. Keeping the definition target-scoped (and avoiding the duplicate -DUSE_FIREBOLT=1 assignment in this block) reduces the risk of unintended behavior changes in other targets.
  test/utests/run.sh:99
• The utests CMake project (test/utests/CMakeLists.txt) does not reference CMAKE_USE_FIREBOLT, so passing -DCMAKE_USE_FIREBOLT=ON here has no effect and is misleading. Either wire this cache variable into the utests build to add the appropriate compile definitions, or remove it from the script to avoid giving the impression tests are exercising the Firebolt path.

gst-plugins/CMakeLists.txt:71

• This block now checks only NOT CMAKE_USE_FIREBOLT but still prints "CMAKE_IARM_MGR set" and unconditionally injects -DIARM_MGR into CMAKE_CXX_FLAGS. That macro should be tied to whether the project is actually building with IARM support (CMAKE_IARM_MGR), otherwise native/non-IARM builds may accidentally compile code paths guarded by IARM_MGR.

if(NOT CMAKE_USE_FIREBOLT)
	message("CMAKE_IARM_MGR set")
	set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -DIARM_MGR")
endif()

• Files reviewed: 8/8 changed files
• Comments generated: 1
• Review effort level: Lite

[Copilot]

🔵 Needs a closer look

A build-flag regression can incorrectly enable IARM_MGR in non-Firebolt builds (potentially breaking native/simulator builds), and there are a couple of remaining correctness/robustness concerns called out in the PR comments.

Review details

Suppressed comments (2)

gst-plugins/CMakeLists.txt:71

• The IARM_MGR compile define is now enabled for every build where CMAKE_USE_FIREBOLT is false, which can unintentionally turn on IARM/DeviceSettings code paths in native/simulator builds (or any build that simply doesn't pass -DCMAKE_USE_FIREBOLT). This can lead to build failures due to missing IARM/DS headers/libs, and the log message is also misleading because CMAKE_IARM_MGR is no longer consulted.

if(NOT CMAKE_USE_FIREBOLT AND NOT CMAKE_PLATFORM_UBUNTU AND NOT CMAKE_SYSTEM_NAME STREQUAL Darwin)
	message("Enabling IARM_MGR")

externals/rdk/PlayerExternalsRdkInterface.h:46

• #undef USE_DS_EVENT_SUPPORTED inside a public header mutates the including translation unit’s preprocessor state, which can cause hard-to-debug behavior changes in any code included after this header. It would be safer to keep USE_DS_EVENT_SUPPORTED intact and instead gate the DS-event-specific declarations with an explicit && !defined(USE_FIREBOLT) check (or ensure the build never defines USE_DS_EVENT_SUPPORTED for Firebolt builds).

#ifdef USE_DS_EVENT_SUPPORTED
#undef USE_DS_EVENT_SUPPORTED

• Files reviewed: 2/2 changed files
• Comments generated: 0 new
• Review effort level: Lite

[Copilot]

🔵 Needs a closer look

Undefining a feature macro inside a public header can silently alter compilation in dependent translation units and should be replaced with safer conditional compilation or build-time flag control.

Review details

Suppressed comments (1)

externals/rdk/PlayerExternalsRdkInterface.h:47

• Avoid #undef USE_DS_EVENT_SUPPORTED in this header: it mutates a compile-definition that may be intentionally provided by the build, and it affects any translation unit that includes this header (e.g., externals/rdk/IIarm/DeviceIARMInterface.cpp includes this header before its #ifndef USE_DS_EVENT_SUPPORTED blocks), potentially flipping feature-flagged code silently. Prefer leaving the macro untouched and instead guard the DS-event-related inheritance/method declarations with an additional !defined(USE_FIREBOLT) check (or ensure the build never defines USE_DS_EVENT_SUPPORTED for Firebolt builds).

#ifdef USE_DS_EVENT_SUPPORTED
#undef USE_DS_EVENT_SUPPORTED
#endif

• Files reviewed: 2/2 changed files
• Comments generated: 0 new
• Review effort level: Lite

[github-advanced-security]

You are seeing this message because GitHub Code Scanning has recently been set up for this repository, or this pull request contains the workflow file for the Code Scanning tool.

What Enabling Code Scanning Means:

• The 'Security' tab will display more code scanning analysis results (e.g., for the default branch).
• Depending on your configuration and choice of analysis tool, future pull requests will be annotated with code scanning analysis results.
• You will be able to see the analysis results for the pull request's branch on this overview once the scans have completed and the checks have passed.

For more information about GitHub Code Scanning, check out the documentation.

[Copilot]

🟢 Approval recommended

No unresolved review issues remain.

Review details

• Files reviewed: 2/2 changed files
• Comments generated: 0 new
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

The RDK-E test configuration no longer enables CMAKE_USE_FIREBOLT, so the intended Firebolt path is not exercised.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

• Files reviewed: 4/4 changed files
• Comments generated: 1
• Review effort level: Lite