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: This changeset deprecates libds/IARM calls in RDK-E and replaces it with firebolt path.
Priority : P1
Test Steps: Mentioned in ticket
Signed off by : Nejuma T N nejumatn28@gmail.com

[Copilot]

🟡 Changes recommended

There are confirmed build/config regressions (CMake Darwin comparison bug, unit tests forcing Firebolt SDK, and SetUseFireBoltSDK() no longer influencing backend selection) that should be fixed before approval.

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

Pull request overview

This PR updates the player externals/device-settings integration to support RDK-E “Firebolt-only” builds by routing libds/IARM-dependent behavior through an existing Thunder/Firebolt path, and adjusts build/test configuration accordingly.

Changes:

• Introduces a Firebolt-based SetHDMIStatus() path (HDCP + resolution) and adds initialization guards for Firebolt event registration.
• Updates build logic to disable/avoid IARM on Firebolt and native (Ubuntu/Darwin) builds, and to include Firebolt sources/defines when enabled.
• Minor GObject property-setting refactors to set multiple properties in one call.

File summaries

File	Description
test/utests/run.sh	Switches unit-test CMake invocation to enable Firebolt builds.
InterfacePlayerRDK.cpp	Refactors decryptor property assignment to use g_object_set().
gst-plugins/CMakeLists.txt	Adjusts when IARM_MGR is enabled based on Firebolt/native build flags.
externals/rdk/PlayerExternalsRdkInterface.h	Adds Firebolt build guards and minimal DS type definitions for RDK-E.
externals/rdk/PlayerExternalsRdkInterface.cpp	Adds Firebolt-only initialization and routes SetHDMIStatus() to Firebolt backend when applicable.
externals/rdk/IFirebolt/DeviceFireboltInterface.h	Adds mutex/initialization flags and declares SetHDMIStatus().
externals/rdk/IFirebolt/DeviceFireboltInterface.cpp	Adds initialization idempotency and implements Firebolt SetHDMIStatus().
externals/PlayerExternalsInterfaceBase.h	Consolidates g_object_get() calls for source width/height reads.
externals/PlayerExternalsInterface.cpp	Ensures the RDK externals interface is used for USE_FIREBOLT builds too.
externals/CMakeLists.txt	Adds CMAKE_USE_FIREBOLT option, routes externals sources/defines, and links FireboltAamp when needed.

Review details

• Files reviewed: 10/10 changed files
• Comments generated: 5
• 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

Build configuration, backend selection, initialization, synchronization, and HDCP error handling issues remain unresolved.

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

Review details

Suppressed comments (7)

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

externals/rdk/IFirebolt/DeviceFireboltInterface.cpp:92

• This flag is set even when either registration method has just logged a subscription failure. Because the next Initialize() returns when m_isInitialized is true, a transiently unavailable Firebolt service permanently prevents retrying the DS/network subscriptions, so the promised event notifications can be lost for the lifetime of the process. Track registration success (or return a status from the registration methods) and only mark the interface initialized after successful registration.

InterfacePlayerRDK.cpp:5058

• g_object_set() is not an atomic multi-property transaction; it invokes the property setters and notification processing sequentially. Therefore this does not remove the use-after-free window described by the new comment—callbacks can still run after the first property is set and before the second. Freeze notifications and/or hold the required object references around the update, or remove the atomicity claim if that lifetime hazard is not applicable.

 					// Set both properties atomically to avoid potential use-after-free if property notifications trigger unrefs
					g_object_set(msg->src,
					             privatePlayer->mPlayerName.c_str(), pInterfacePlayerRDK->mDRMSessionManager,
					             "drm-session-manager", pInterfacePlayerRDK->mEncrypt,
					             NULL);

externals/rdk/IFirebolt/DeviceFireboltInterface.cpp:250

• The new SetHDMIStatus() path writes m_displayWidth and m_displayHeight, while ResolutionHandlerFirebolt() can write the same fields from another Firebolt callback. Firebolt is configured with a worker pool, and neither SetResolution() nor GetDisplayResolution() synchronizes this state, so concurrent callbacks/readers can race and observe a mixed width/height pair. Protect the shared display state with one mutex or serialize all Firebolt callbacks.

    // Query current resolution via Firebolt Device.videoResolution (xrn:firebolt:capability:device:info)
    auto resolutionResult = Firebolt::IFireboltAampAccessor::Instance().DeviceInterface().videoResolution();
    if (resolutionResult)
    {
        int width  = resolutionResult.value()[0];
        int height = resolutionResult.value()[1];
        pInstance->SetResolution(width, height);
        MW_LOG_INFO("DeviceFirebolt SetHDMIStatus: Resolution [%d][%d]", width, height);

externals/rdk/IFirebolt/DeviceFireboltInterface.cpp:209

• This production Firebolt HDCP/resolution path has no coverage: the existing PlayerExternalsRdkTests target compiles FakePlayerExternalsRdkInterface.cpp, not this implementation, so it does not exercise either Firebolt query, error path, or state update. Add tests with a mocked Firebolt Device API before relying on this path.

void DeviceFireboltInterface::SetHDMIStatus()
{
	std::unique_lock<std::mutex> lock(m_hdmiStatusMutex, std::try_to_lock);
    if (!lock.owns_lock()) {
        MW_LOG_WARN("DeviceFirebolt SetHDMIStatus: Already in progress on another thread, skipping");

externals/rdk/PlayerExternalsRdkInterface.cpp:95

• On non-Firebolt RDK-V builds this drops m_use_firebolt_sdk from backend selection. SetUseFireBoltSDK(true) still sets the flag and calls Initialize() (385-392), but this path now returns/chooses IARM whenever the process is not containerized, so the public switch can never select Firebolt. Preserve the flag in the early-return checks and final selection, or remove the setter and its contract.

        if(m_initialized == InitState::FIREBOLT && useFireboltInContainer)

test/utests/run.sh:96

• This unconditionally enables the Firebolt build for the macOS unit-test configuration. That takes the CMAKE_USE_FIREBOLT branch, which requires FireboltAamp CONFIG and compiles RDK-specific Firebolt sources; the previous macOS setup did not require that SDK. Standard macOS test runs will therefore fail at configure unless the RDK-E Firebolt SDK is installed; enable this option only for configurations that provide the SDK.

    PKG_CONFIG_PATH=/Library/Frameworks/GStreamer.framework/Versions/1.0/lib/pkgconfig:${PLAYER_DIR}/.libs/lib/pkgconfig:/usr/local/lib/pkgconfig:$PKG_CONFIG_PATH cmake -DCOVERAGE_ENABLED=ON -DCMAKE_BUILD_TYPE=Debug -DCMAKE_RDKE_TEST_RUN=$rdke_build -DCMAKE_USE_FIREBOLT=ON ../

test/utests/run.sh:99

• These commands replace the old test flag with CMAKE_USE_FIREBOLT, but test/utests/CMakeLists.txt is an independent UnitTests project and neither includes externals/CMakeLists.txt nor consumes this variable. Consequently the RDK-E run still builds the standalone fake in PlayerExternalsRdkTests and never compiles or exercises DeviceFireboltInterface.cpp; this new flag is a no-op for the tests. Wire the option into the test targets or add a dedicated Firebolt test configuration.

    PKG_CONFIG_PATH=/Library/Frameworks/GStreamer.framework/Versions/1.0/lib/pkgconfig:${PLAYER_DIR}/.libs/lib/pkgconfig:/usr/local/lib/pkgconfig:$PKG_CONFIG_PATH cmake -DCOVERAGE_ENABLED=ON -DCMAKE_BUILD_TYPE=Debug -DCMAKE_RDKE_TEST_RUN=$rdke_build -DCMAKE_USE_FIREBOLT=ON ../
elif [[ "$OSTYPE" == "linux"* ]]; then
    echo "PLAYER DIR[${PLAYER_DIR}]"
    PKG_CONFIG_PATH=${PLAYER_DIR}/.libs/lib/pkgconfig cmake --no-warn-unused-cli -DCMAKE_INSTALL_PREFIX=${PLAYER_DIR}/.libs -DCMAKE_PLATFORM_UBUNTU=1 -DCOVERAGE_ENABLED=ON -DCMAKE_LIBRARY_PATH=${PLAYER_DIR}/.libs/lib -DCMAKE_EXPORT_COMPILE_COMMANDS:BOOL=TRUE -DCMAKE_BUILD_TYPE:STRING=Debug -DCMAKE_C_COMPILER:FILEPATH=/usr/bin/gcc -DCMAKE_CXX_COMPILER:FILEPATH=/usr/bin/g++ -DCMAKE_RDKE_TEST_RUN=$rdke_build -DCMAKE_USE_FIREBOLT=ON -S../ -B$PWD -G "Unix Makefiles"

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

[Copilot]

🟡 Changes recommended

Four moderate issues remain unresolved.

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

Review details

Suppressed comments (3)

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

externals/rdk/IFirebolt/DeviceFireboltInterface.cpp:92

• m_isInitialized is set unconditionally after the registration calls, even when either subscription fails (the registration methods only log errors and return). If Firebolt is unavailable during startup, later Initialize() calls take the early-return path and never retry, leaving the HDCP/resolution or network notifications permanently unregistered. Track registration success and only mark the interface initialized when the required subscriptions succeed, or allow a failed initialization to retry.

externals/CMakeLists.txt:204

• Adding CMAKE_IARM_MGR here makes every plain RDK-V/IARM build require find_package(FireboltAamp CONFIG REQUIRED), even when CMAKE_USE_FIREBOLT is off; the Firebolt sources are also now unconditionally compiled in the RDK-V branch above. This is a new configure/build dependency for platforms that previously built with only IARM/DeviceSettings. Gate the Firebolt backend and package lookup on an explicit feature/availability check, or otherwise preserve a plain-IARM build without FireboltAamp.

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

externals/rdk/IFirebolt/DeviceFireboltInterface.cpp:210

• Using try_to_lock drops an HDMI update whenever another query is in progress, and this path does not schedule a retry. Back-to-back HDCP and resolution notifications can therefore leave the player with stale state after the first query completes. Serialize the calls or coalesce a follow-up update instead of returning without processing the event.

	std::unique_lock<std::mutex> lock(m_hdmiStatusMutex, std::try_to_lock);
    if (!lock.owns_lock()) {
        MW_LOG_WARN("DeviceFirebolt SetHDMIStatus: Already in progress on another thread, skipping");
        return;

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

[Copilot]

🟡 Changes recommended

Unresolved synchronization and initialization/state-handling issues remain, along with missing Firebolt-path test coverage.

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

Review details

Suppressed comments (1)

externals/rdk/IFirebolt/DeviceFireboltInterface.cpp:241

• When hdcp() fails, this branch only logs the error and leaves the previously cached HDCP protocol/enabled state untouched. After a prior 2.2 result, a Firebolt outage can therefore keep isHDCPConnection2_2() true and allow UHD decryption despite the current protection state; clear the state and reset the protocol to a non-2.2 value on this failure path.

    else
    {
        MW_LOG_ERR("DeviceFirebolt SetHDMIStatus: Failed to query HDCP: %d", static_cast<int>(hdcpResult.error()));
    }

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

[dp0000]

approved