Firebolt changes - #226
Open
nejuma1 wants to merge 32 commits into
Open
Firebolt changes#226nejuma1 wants to merge 32 commits into
nejuma1 wants to merge 32 commits into
Conversation
…gin for libds Methods and Event Notifications Reason for change : Added SetHDMIStatus() function in fireboltinterface.cpp Priority: P1 Test Steps : Mentioned in ticket Signed off by : Nejuma T N <nejumatn28@gmail.com>
…gin for libds Methods and Event Notifications Reason for change : Resolve compilation errors Priority: P1 Test Steps : Mentioned in ticket Signed off by : Nejuma T N <nejumatn28@gmail.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR adds/build-gates Firebolt-only (RDK‑E) plumbing so player externals can be built without IARM/DS dependencies, and routes HDMI/HDCP status updates through the Firebolt Device API.
Changes:
- Adds
USE_FIREBOLTcompile-time gating in the RDK externals interface and Firebolt device interface to avoid DS header dependencies. - Introduces Firebolt-driven
SetHDMIStatus()flow and related state update hooks (HDCP + resolution). - Updates external build logic to (attempt to) support Firebolt-only builds and adjusts unit-test build script flags.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| test/utests/run.sh | Updates Linux utest cmake invocation (removes DS-event cmake var from Linux path). |
| externals/rdk/PlayerExternalsRdkInterface.h | Adds DS-header exclusion for Firebolt builds and introduces SetHDCPEnabled(). |
| externals/rdk/PlayerExternalsRdkInterface.cpp | Adds compile-time Firebolt vs IARM routing and Firebolt SetHDMIStatus() delegation. |
| externals/rdk/IFirebolt/DeviceFireboltInterface.h | Adds mutex + declares Firebolt SetHDMIStatus(). |
| externals/rdk/IFirebolt/DeviceFireboltInterface.cpp | Implements Firebolt SetHDMIStatus() (HDCP + resolution) and adds/changes logging. |
| externals/CMakeLists.txt | Adds CMAKE_USE_FIREBOLT option and attempts to gate IARM/DS includes/sources/linking for Firebolt-only builds. |
Suppressed comments (2)
externals/CMakeLists.txt:122
CMAKE_USE_FIREBOLTis introduced as an option, but the rest of this CMake file gates Firebolt-only behavior onUSE_FIREBOLT(e.g., include paths, sources, and FireboltAamp linking). As written, turning on-DCMAKE_USE_FIREBOLT=ONwon’t actually setUSE_FIREBOLT, so the build may still compile/link IARM/DS while also definingUSE_FIREBOLTfor compilation flags.
option(CMAKE_USE_FIREBOLT "Enable Firebolt SDK for RDK-E builds (disables IARM/DS linking)" OFF)
if(CMAKE_USE_FIREBOLT)
message("USE_FIREBOLT Set")
set(LIB_EXT_DEFINES "${LIB_EXT_DEFINES} -DUSE_FIREBOLT=1")
endif()
externals/rdk/PlayerExternalsRdkInterface.cpp:223
- This warning executes in the IARM/DeviceSettings implementation of
SetHDMIStatus()and claims the code path shouldn't run. It will spam WARN logs during normal operation on IARM builds and makes troubleshooting harder.
void PlayerExternalsRdkInterface::SetHDMIStatus()
{
MW_LOG_WARN("[FIREBOLT] This block of code shouldnt be executed!!!!!!!!!!!");
std::unique_lock<std::mutex> lock(m_hdmiStatusMutex, std::try_to_lock);
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+74
to
+78
| MW_LOG_WARN("[FIREBOLT] Initializing \n"); | ||
| // Reset before assigning new interface | ||
| m_pDeviceInterfaceBase = nullptr; | ||
| m_pDeviceInterfaceBase = DeviceFireboltInterface::GetInstance(); | ||
| DeviceFireboltInterface::Initialize(); |
Comment on lines
+105
to
+106
| MW_LOG_WARN("[FIREBOLT] Done getting interface \n"); | ||
| MW_LOG_WARN("[FIREBOLT]Next SETHDMISTATUS call should route through firebolt"); |
Comment on lines
+219
to
+224
| else if (hdcpMap.hdcp1_4) | ||
| { | ||
| pInstance->SetHDCPEnabled(true); | ||
| pInstance->setHdcpProtocol(dsHDCP_VERSION_1X); | ||
| MW_LOG_WARN("[FIREBOLT] SetHDMIStatus: HDCP 2.2 detected"); } | ||
| else |
Comment on lines
360
to
370
| void PlayerExternalsRdkInterface::SetUseFireBoltSDK(bool t_use_firebolt_sdk) | ||
| { | ||
| #ifndef USE_FIREBOLT | ||
| MW_PRE_LOGGER_LOG("old : %d, new : %d \n", m_use_firebolt_sdk, t_use_firebolt_sdk); | ||
| if(m_use_firebolt_sdk != t_use_firebolt_sdk) | ||
| { | ||
| m_use_firebolt_sdk = t_use_firebolt_sdk; | ||
| //reinitialize | ||
| m_initialized = InitState::NOT_INITIALIZED; | ||
| Initialize(); | ||
|
|
||
| } |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Suppressed comments (4)
externals/rdk/PlayerExternalsRdkInterface.cpp:78
- In the USE_FIREBOLT initialization path,
m_initializedis never set toFIREBOLT. This makes them_initialized != NOT_INITIALIZEDguard ineffective and can cause repeated re-initialization on subsequentInitialize()calls.
MW_LOG_WARN("[FIREBOLT] Initializing \n");
// Reset before assigning new interface
m_pDeviceInterfaceBase = nullptr;
m_pDeviceInterfaceBase = DeviceFireboltInterface::GetInstance();
DeviceFireboltInterface::Initialize();
externals/rdk/PlayerExternalsRdkInterface.h:44
- When
USE_FIREBOLTis enabled, DS/DeviceSettings headers are not included, but other build targets (notably unit tests) can still defineUSE_DS_EVENT_SUPPORTED, which then triggers DS-event inheritance/handlers and breaks compilation. Consider explicitly disablingUSE_DS_EVENT_SUPPORTEDfor Firebolt builds in this header to prevent accidental enablement.
#else
/* Minimal HDCP version enum - DS headers not available for RDK-E (USE_FIREBOLT) builds */
typedef enum {
dsHDCP_VERSION_1X = 0,
dsHDCP_VERSION_2X,
dsHDCP_VERSION_MAX
} dsHdcpProtocolVersion_t;
#endif
externals/rdk/IFirebolt/DeviceFireboltInterface.cpp:224
- The HDCP 1.4 branch logs "HDCP 2.2 detected" and also closes the brace on the same line, which is both misleading and hard to read.
else if (hdcpMap.hdcp1_4)
{
pInstance->SetHDCPEnabled(true);
pInstance->setHdcpProtocol(dsHDCP_VERSION_1X);
MW_LOG_WARN("[FIREBOLT] SetHDMIStatus: HDCP 2.2 detected"); }
else
externals/rdk/PlayerExternalsRdkInterface.cpp:110
- These log lines are compiled in both Firebolt and non-Firebolt builds, but they claim the call "should" route through Firebolt / "shouldn't be executed". This is misleading and will spam warnings on non-Firebolt builds. Prefer neutral logging and/or compile-time guarding of DS-event registration to non-Firebolt builds only.
MW_LOG_WARN("[FIREBOLT] Done getting interface \n");
MW_LOG_WARN("[FIREBOLT]Next SETHDMISTATUS call should route through firebolt");
SetHDMIStatus();
#ifdef USE_DS_EVENT_SUPPORTED
MW_LOG_WARN("[FIREBOLT] This block of code shouldnt be executed");
RegisterDsClientEventHandler();
| # RDK-E: Firebolt-only; no IARM or ds | ||
| message("USE_FIREBOLT set - RDK-E build: routing device APIs through Firebolt") | ||
| add_definitions(-DUSE_FIREBOLT) | ||
| set(LIB_EXT_DEFINES "${LIB_EXT_DEFINES} -DUSE_FIREBOLT=1") |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.
Suppressed comments (5)
Previously missed (1) — in code that hasn't changed since the last review.
externals/CMakeLists.txt:199
- When building with CMAKE_USE_FIREBOLT, the library links FireboltAamp, but PlayerExternalsRdkInterface.h is still only installed as a public header when CMAKE_IARM_MGR is set. This can cause Firebolt builds to omit the header from installation packages.
if(CMAKE_USE_SECCLIENT OR CMAKE_USE_SECMANAGER OR (CMAKE_USE_FIREBOLT AND NOT CMAKE_PLATFORM_UBUNTU))
externals/rdk/PlayerExternalsRdkInterface.cpp:78
- In the USE_FIREBOLT initialization path, m_initialized is never updated to FIREBOLT. This leaves the instance in NOT_INITIALIZED, so subsequent Initialize() calls will re-run initialization even though the early-return guard relies on m_initialized.
MW_LOG_WARN("[FIREBOLT] Initializing \n");
// Reset before assigning new interface
m_pDeviceInterfaceBase = nullptr;
m_pDeviceInterfaceBase = DeviceFireboltInterface::GetInstance();
DeviceFireboltInterface::Initialize();
externals/rdk/PlayerExternalsRdkInterface.cpp:106
- These WARN logs are debug-only assertions about code routing (and include an internal note about the next call). Leaving them at WARN level will add noisy/incorrect output in normal operation.
MW_LOG_WARN("[FIREBOLT] Done getting interface \n");
MW_LOG_WARN("[FIREBOLT]Next SETHDMISTATUS call should route through firebolt");
externals/rdk/IFirebolt/DeviceFireboltInterface.cpp:223
- The HDCP 1.4 branch logs "HDCP 2.2 detected" and has a closing brace on the same line as the log statement, which is both misleading and easy to miss during review.
else if (hdcpMap.hdcp1_4)
{
pInstance->SetHDCPEnabled(true);
pInstance->setHdcpProtocol(dsHDCP_VERSION_1X);
MW_LOG_WARN("[FIREBOLT] SetHDMIStatus: HDCP 2.2 detected"); }
gst-plugins/CMakeLists.txt:71
- This change makes -DIARM_MGR get added whenever CMAKE_USE_FIREBOLT is not enabled (including when CMAKE_IARM_MGR is unset). Previously this only happened when CMAKE_IARM_MGR was explicitly enabled, so this alters build behavior and the message is no longer accurate.
if(NOT CMAKE_USE_FIREBOLT)
message("CMAKE_IARM_MGR set")
set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -DIARM_MGR")
endif()
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
Suppressed comments (5)
Previously missed (1) — in code that hasn't changed since the last review.
externals/rdk/PlayerExternalsRdkInterface.cpp:109
- This message is inside
#ifdef USE_DS_EVENT_SUPPORTED, so it will run when DS event handling is enabled. The current wording ("shouldnt be executed") is misleading during normal DS-enabled builds.
MW_LOG_WARN("[FIREBOLT] This block of code shouldnt be executed");
externals/rdk/PlayerExternalsRdkInterface.cpp:78
- In the USE_FIREBOLT initialization path, m_initialized is never updated to InitState::FIREBOLT. This makes the "already inited" guard ineffective and can lead to repeated re-initialization / event re-subscription on subsequent Initialize() calls.
MW_LOG_WARN("[FIREBOLT] Initializing \n");
// Reset before assigning new interface
m_pDeviceInterfaceBase = nullptr;
m_pDeviceInterfaceBase = DeviceFireboltInterface::GetInstance();
DeviceFireboltInterface::Initialize();
externals/rdk/PlayerExternalsRdkInterface.cpp:87
- This log message is in the non-Firebolt (#else) branch, so it will execute for non-Firebolt builds. The current wording ("SHOULDN'T BE EXECUTED") is misleading and makes logs look like a fatal error during normal IARM builds.
MW_LOG_WARN("[FIREBOLT] THIS BLOCK OF CODE SHOULDN'T BE EXECUTED!!!!!");
externals/rdk/IFirebolt/DeviceFireboltInterface.cpp:223
- The HDCP 1.4 branch logs "HDCP 2.2 detected" and also has the closing brace on the same line as the log statement, which looks like a copy/paste error and makes the control flow harder to read.
else if (hdcpMap.hdcp1_4)
{
pInstance->SetHDCPEnabled(true);
pInstance->setHdcpProtocol(dsHDCP_VERSION_1X);
MW_LOG_WARN("[FIREBOLT] SetHDMIStatus: HDCP 2.2 detected"); }
gst-plugins/CMakeLists.txt:71
- This now defines
IARM_MGRfor every build where CMAKE_USE_FIREBOLT is false, even ifCMAKE_IARM_MGRwas not enabled/passed in. SinceCMAKE_IARM_MGRis still used elsewhere, this can unintentionally compile IARM-specific code paths and create inconsistent build flags between components.
if(NOT CMAKE_USE_FIREBOLT)
message("CMAKE_IARM_MGR set")
set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -DIARM_MGR")
endif()
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated no new comments.
Suppressed comments (7)
Previously missed (1) — in code that hasn't changed since the last review.
gst-plugins/CMakeLists.txt:71
- This block now keys off
NOT CMAKE_USE_FIREBOLT, but the message still saysCMAKE_IARM_MGR setand the define is-DIARM_MGR(no value), while other parts of the build use-DIARM_MGR=1. This makes configuration/debugging harder and can lead to inconsistent macro usage across targets.
if(NOT CMAKE_USE_FIREBOLT)
message("CMAKE_IARM_MGR set")
set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -DIARM_MGR")
endif()
externals/rdk/PlayerExternalsRdkInterface.cpp:78
- In the USE_FIREBOLT initialization path,
m_initializedis never updated toInitState::FIREBOLT. Because the early-return guard checksm_initialized != NOT_INITIALIZED, this causesInitialize()to re-run Firebolt initialization every time (and can lead to repeated event subscriptions / re-init behavior).
#ifdef USE_FIREBOLT
MW_LOG_WARN("[FIREBOLT]Using Firebolt \n");
//initialize only if needed
if(m_initialized != InitState::NOT_INITIALIZED)
{
MW_LOG_WARN("[FIREBOLT] Firebolt already Inited \n");
return;
}
MW_LOG_WARN("[FIREBOLT] Initializing \n");
// Reset before assigning new interface
m_pDeviceInterfaceBase = nullptr;
m_pDeviceInterfaceBase = DeviceFireboltInterface::GetInstance();
DeviceFireboltInterface::Initialize();
externals/rdk/PlayerExternalsRdkInterface.cpp:87
- The log line inside the non-Firebolt (
#else) branch says "[FIREBOLT] THIS BLOCK OF CODE SHOULDN'T BE EXECUTED". In a non-Firebolt build this branch will always execute, so this message is misleading and also emitted at WARN severity.
MW_LOG_WARN("[FIREBOLT] THIS BLOCK OF CODE SHOULDN'T BE EXECUTED!!!!!");
externals/rdk/PlayerExternalsRdkInterface.cpp:222
- The non-Firebolt
SetHDMIStatus()implementation unconditionally logs "[FIREBOLT] This block of code shouldnt be executed" even though this is the correct code path for IARM/DeviceSettings builds. This will create noisy/misleading WARN logs in normal operation.
MW_LOG_WARN("[FIREBOLT] This block of code shouldnt be executed!!!!!!!!!!!");
externals/rdk/IFirebolt/DeviceFireboltInterface.cpp:224
- In the Firebolt HDCP mapping, the
hdcp1_4branch logs "HDCP 2.2 detected" (copy/paste error) and also has the closing brace on the same line, which makes the control flow hard to read.
else if (hdcpMap.hdcp1_4)
{
pInstance->SetHDCPEnabled(true);
pInstance->setHdcpProtocol(dsHDCP_VERSION_1X);
MW_LOG_WARN("[FIREBOLT] SetHDMIStatus: HDCP 2.2 detected"); }
else
externals/PlayerExternalsInterface.cpp:45
PlayerExternalsRdkInterface.his now included unconditionally, but the class is only used whenIARM_MGRis defined. This can break simulator/fake builds (no IARM_MGR, no USE_FIREBOLT) because the header pulls in DeviceSettings/DS headers. Separately, Firebolt builds defineUSE_FIREBOLTbut do not defineIARM_MGR, so the constructor will currently select the Fake interface instead of the Firebolt-backed RDK interface.
#include "PlayerExternalsRdkInterface.h"
/**< Static variable for singleton */
std::shared_ptr<PlayerExternalsInterface> PlayerExternalsInterface::s_pPlayerOP = NULL;
/**
* @brief PlayerExternalsInterface Constructor
*/
PlayerExternalsInterface::PlayerExternalsInterface()
{
#ifdef IARM_MGR
MW_PRE_LOGGER_LOG("Device API IARM/Firebolt\n");
m_pIarmInterface = PlayerExternalsRdkInterface::GetPlayerExternalsRdkInterfaceInstance();
#else
MW_PRE_LOGGER_LOG("Device API FAKE\n");
m_pIarmInterface = std::shared_ptr<PlayerExternalsInterfaceBase>(new FakePlayerExternalsInterface());
#endif
externals/CMakeLists.txt:139
- This Firebolt/IARM selection block contradicts the earlier message that Ubuntu simulator builds should ignore
CMAKE_USE_FIREBOLT. As written,if(CMAKE_USE_FIREBOLT)will still execute on Ubuntu, adding Firebolt sources/defines, and theelse()branch will always enable IARM (and link -lIARMBus/-lds) when Firebolt is OFF — which can break simulator builds that rely on the Fake externals interface.
if(CMAKE_USE_FIREBOLT)
# RDK-E: Firebolt-only; no IARM or ds
message("USE_FIREBOLT set - RDK-E build: routing device APIs through Firebolt")
add_definitions(-DUSE_FIREBOLT)
set(LIB_EXT_DEFINES "${LIB_EXT_DEFINES} -DUSE_FIREBOLT=1")
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
Suppressed comments (5)
Previously missed (1) — in code that hasn't changed since the last review.
externals/rdk/PlayerExternalsRdkInterface.h:30
- Undefining
USE_DS_EVENT_SUPPORTEDinside a public header has translation-unit side effects (it changes the macro state for any includes that come after this header). This can cause hard-to-trace build differences. Prefer validating the combination and failing fast rather than mutating the preprocessor environment.
#ifdef USE_FIREBOLT
#undef USE_DS_EVENT_SUPPORTED
#endif
#ifndef USE_FIREBOLT
externals/CMakeLists.txt:139
- The device-API selection logic always falls back to the IARM path when
CMAKE_USE_FIREBOLTis OFF, even ifCMAKE_IARM_MGRis not set (e.g., simulator builds). This changes previous behavior and can cause unwanted linking against-lIARMBus/-lds. Also, Firebolt builds don’t defineIARM_MGR, soPlayerExternalsInterface.cppwill select the fake externals implementation despite compiling the Firebolt sources.
if(CMAKE_USE_FIREBOLT AND NOT CMAKE_PLATFORM_UBUNTU)
# RDK-E: Firebolt-only; no IARM or ds
message("USE_FIREBOLT set - RDK-E build: routing device APIs through Firebolt")
add_definitions(-DUSE_FIREBOLT)
set(LIB_EXT_DEFINES "${LIB_EXT_DEFINES} -DUSE_FIREBOLT=1")
gst-plugins/CMakeLists.txt:71
gst-pluginsnow definesIARM_MGRwheneverCMAKE_USE_FIREBOLTis false/undefined. SinceCMAKE_USE_FIREBOLTis not declared in this CMake file, it will typically be empty (treated as false), making this block run unconditionally and potentially enabling IARM-only code paths on builds that previously didn’t setCMAKE_IARM_MGR.
if(NOT CMAKE_USE_FIREBOLT)
message("CMAKE_IARM_MGR set")
set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -DIARM_MGR")
endif()
externals/rdk/PlayerExternalsRdkInterface.cpp:72
- In the
USE_FIREBOLTinitialization path,m_initializedis never set toInitState::FIREBOLT. As a result, the early-return guard (m_initialized != NOT_INITIALIZED) will never trigger, andInitialize()may re-run Firebolt initialization multiple times.
//initialize only if needed
if(m_initialized != InitState::NOT_INITIALIZED)
{
MW_LOG_WARN("[FIREBOLT] Firebolt already Inited \n");
return;
externals/rdk/IFirebolt/DeviceFireboltInterface.cpp:223
- The HDCP 1.4 branch logs "HDCP 2.2 detected" and also has a stray closing brace on the same line as the log statement, which makes the control flow harder to read and the log misleading.
else if (hdcpMap.hdcp1_4)
{
pInstance->SetHDCPEnabled(true);
pInstance->setHdcpProtocol(dsHDCP_VERSION_1X);
MW_LOG_WARN("[FIREBOLT] SetHDMIStatus: HDCP 2.2 detected"); }
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated no new comments.
Suppressed comments (11)
Previously missed (2) — in code that hasn't changed since the last review.
externals/rdk/IFirebolt/DeviceFireboltInterface.cpp:102
- RegisterDsMgrEventHandler() is called from Initialize() when an instance exists, so logging "This shouldnt be called" is misleading and can hide real issues.
This issue also appears on line 140 of the same file.
void DeviceFireboltInterface::RegisterDsMgrEventHandler()
{
MW_LOG_WARN("[FIREBOLT] This shouldnt be called!!!!!");
MW_PRE_LOGGER_LOG("Subscribing to Firebolt hdcp change event \n");
externals/rdk/PlayerExternalsRdkInterface.h:30
- Undefining USE_DS_EVENT_SUPPORTED inside a public header introduces surprising, order-dependent compile behavior for any translation unit that includes this header. Prefer making DS-event code conditional on "USE_DS_EVENT_SUPPORTED && !USE_FIREBOLT" (or handle this solely in CMake) rather than forcibly undefining macros from within the header.
#ifdef USE_FIREBOLT
#undef USE_DS_EVENT_SUPPORTED
#endif
#ifndef USE_FIREBOLT
externals/PlayerExternalsInterface.cpp:29
- Including PlayerExternalsRdkInterface.h unconditionally makes non-IARM/simulator builds fail (external DeviceSettings headers like manager.hpp are not available) and also causes USE_FIREBOLT builds to select FakePlayerExternalsInterface because the constructor only checks IARM_MGR. Gate both the include and the constructor branch on (IARM_MGR || USE_FIREBOLT).
#include "PlayerExternalsInterface.h"
#include "PlayerExternalUtils.h"
#include <utility>
#include "PlayerExternalsRdkInterface.h"
externals/rdk/PlayerExternalsRdkInterface.cpp:87
- This log line is inside the non-Firebolt compile-time branch, so it will be executed in normal (non-USE_FIREBOLT) builds. The message is misleading and makes diagnosing real execution paths harder.
MW_LOG_WARN("[FIREBOLT] THIS BLOCK OF CODE SHOULDN'T BE EXECUTED!!!!!");
externals/CMakeLists.txt:139
- The #IARM/RFC selection falls back to the IARM/DS path for CMAKE_PLATFORM_UBUNTU builds (since the condition only checks CMAKE_USE_FIREBOLT), which contradicts the earlier simulator-build message and will attempt to define IARM_MGR/link -lIARMBus/-lds on Ubuntu. Add an explicit CMAKE_PLATFORM_UBUNTU branch that keeps the fake externals path (no IARM/DS linking).
if(CMAKE_USE_FIREBOLT AND NOT CMAKE_PLATFORM_UBUNTU)
# RDK-E: Firebolt-only; no IARM or ds
message("USE_FIREBOLT set - RDK-E build: routing device APIs through Firebolt")
add_definitions(-DUSE_FIREBOLT)
set(LIB_EXT_DEFINES "${LIB_EXT_DEFINES} -DUSE_FIREBOLT=1")
externals/rdk/PlayerExternalsRdkInterface.cpp:78
- In the USE_FIREBOLT Initialize() path, m_initialized is never set to FIREBOLT. That makes the "already inited" guard ineffective and can lead to repeated initialization/event registration on subsequent Initialize() calls.
MW_LOG_WARN("[FIREBOLT] Initializing \n");
// Reset before assigning new interface
m_pDeviceInterfaceBase = nullptr;
m_pDeviceInterfaceBase = DeviceFireboltInterface::GetInstance();
DeviceFireboltInterface::Initialize();
externals/rdk/PlayerExternalsRdkInterface.cpp:110
- This log line is emitted whenever USE_DS_EVENT_SUPPORTED is enabled, including in non-Firebolt builds where RegisterDsClientEventHandler() is expected to run. The message is misleading.
#ifdef USE_DS_EVENT_SUPPORTED
MW_LOG_WARN("[FIREBOLT] This block of code shouldnt be executed");
RegisterDsClientEventHandler();
externals/rdk/PlayerExternalsRdkInterface.cpp:223
- This log line is in the non-Firebolt SetHDMIStatus() implementation, so it will run in normal builds and is misleading as written.
void PlayerExternalsRdkInterface::SetHDMIStatus()
{
MW_LOG_WARN("[FIREBOLT] This block of code shouldnt be executed!!!!!!!!!!!");
std::unique_lock<std::mutex> lock(m_hdmiStatusMutex, std::try_to_lock);
externals/rdk/IFirebolt/DeviceFireboltInterface.cpp:223
- The HDCP 1.4 branch logs "HDCP 2.2 detected" and the closing brace is on the same line as the log, which makes the output incorrect and the code harder to read.
else if (hdcpMap.hdcp1_4)
{
pInstance->SetHDCPEnabled(true);
pInstance->setHdcpProtocol(dsHDCP_VERSION_1X);
MW_LOG_WARN("[FIREBOLT] SetHDMIStatus: HDCP 2.2 detected"); }
externals/rdk/IFirebolt/DeviceFireboltInterface.cpp:145
- RemoveEventHandlers() is invoked from the destructor, so logging "This shouldnt be called" is misleading and can confuse teardown diagnostics.
void DeviceFireboltInterface::RemoveEventHandlers()
{
MW_LOG_WARN("[FIREBOLT] This shouldnt be called!!!!!");
//removes everything ...
Firebolt::IFireboltAampAccessor::Instance().DeviceInterface().unsubscribeAll();
}
gst-plugins/CMakeLists.txt:71
- The condition was changed to NOT CMAKE_USE_FIREBOLT, but the message still says "CMAKE_IARM_MGR set", which is inaccurate and can mislead during configuration.
if(NOT CMAKE_USE_FIREBOLT)
message("CMAKE_IARM_MGR set")
set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -DIARM_MGR")
endif()
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.
Suppressed comments (4)
externals/rdk/PlayerExternalsRdkInterface.cpp:78
- In the USE_FIREBOLT initialization path, m_initialized is never set to FIREBOLT. As a result, subsequent Initialize() calls will re-run initialization every time (the NOT_INITIALIZED guard never trips). Set the state after successful initialization to make the guard effective.
MW_LOG_WARN("[FIREBOLT] Initializing \n");
// Reset before assigning new interface
m_pDeviceInterfaceBase = nullptr;
m_pDeviceInterfaceBase = DeviceFireboltInterface::GetInstance();
DeviceFireboltInterface::Initialize();
externals/rdk/PlayerExternalsRdkInterface.cpp:87
- This warning message is misleading: it is compiled/executed in non-Firebolt builds, but claims the block "shouldn't be executed". It looks like leftover debug text and will create noisy/incorrect logs in normal IARM builds.
MW_LOG_WARN("[FIREBOLT] THIS BLOCK OF CODE SHOULDN'T BE EXECUTED!!!!!");
externals/rdk/PlayerExternalsRdkInterface.cpp:110
- This log line is inaccurate: RegisterDsClientEventHandler() is expected to run when USE_DS_EVENT_SUPPORTED is enabled (non-Firebolt builds), but the message says it "shouldn't be executed". Please remove or replace with an accurate, non-alarming log level/message.
#ifdef USE_DS_EVENT_SUPPORTED
MW_LOG_WARN("[FIREBOLT] This block of code shouldnt be executed");
RegisterDsClientEventHandler();
externals/rdk/IFirebolt/DeviceFireboltInterface.cpp:223
- The HDCP 1.4 path logs "HDCP 2.2 detected" and also has a stray closing brace on the same line as the log call. This will confuse diagnostics and makes the control flow harder to read.
else if (hdcpMap.hdcp1_4)
{
pInstance->SetHDCPEnabled(true);
pInstance->setHdcpProtocol(dsHDCP_VERSION_1X);
MW_LOG_WARN("[FIREBOLT] SetHDMIStatus: HDCP 2.2 detected"); }
Comment on lines
+27
to
+30
| #ifdef USE_FIREBOLT | ||
| #undef USE_DS_EVENT_SUPPORTED | ||
| #endif | ||
| #ifndef USE_FIREBOLT |
| message("CMAKE_IARM_MGR set") | ||
| set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -DIARM_MGR") | ||
| endif() | ||
| # IARM is deprecated for the Firebolt routing flow in this bran#ch. |
Comment on lines
+119
to
+124
| if(CMAKE_USE_FIREBOLT AND NOT CMAKE_PLATFORM_UBUNTU) | ||
| message("USE_FIREBOLT SET") | ||
| set(LIB_EXT_DEFINES "${LIB_EXT_DEFINES} -DUSE_FIREBOLT=1") | ||
| elseif(CMAKE_USE_FIREBOLT AND CMAKE_PLATFORM_UBUNTU) | ||
| message("CMAKE_PLATFORM_UBUNTU set - simulator build: ignoring CMAKE_USE_FIREBOLT and using fake externals interface") | ||
| endif() |
Comment on lines
+181
to
183
| if(CMAKE_USE_FIREBOLT) | ||
| # Append additional header(s) for IARM_MGR | ||
| set_property(TARGET playerfbinterface APPEND PROPERTY PUBLIC_HEADER "rdk/PlayerExternalsRdkInterface.h") |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
Suppressed comments (6)
Previously missed (1) — in code that hasn't changed since the last review.
externals/rdk/IFirebolt/DeviceFireboltInterface.cpp:244
- videoResolution() is indexed without checking its length. If the Firebolt API ever returns an unexpected vector size, this will read out of bounds and crash.
// 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_WARN("[FIREBOLT] DeviceFirebolt SetHDMIStatus: Resolution [%d][%d]", width, height);
}
externals/CMakeLists.txt:183
- This comment still refers to IARM_MGR, but the surrounding condition is now CMAKE_USE_FIREBOLT. Updating it avoids confusion when reading install header rules.
if(CMAKE_USE_FIREBOLT)
# Append additional header(s) for IARM_MGR
set_property(TARGET playerfbinterface APPEND PROPERTY PUBLIC_HEADER "rdk/PlayerExternalsRdkInterface.h")
gst-plugins/CMakeLists.txt:68
- Typo in comment: "bran#ch" → "branch".
# IARM is deprecated for the Firebolt routing flow in this bran#ch.
externals/rdk/PlayerExternalsRdkInterface.h:30
- Undefining USE_DS_EVENT_SUPPORTED inside a public header mutates the preprocessor state for the entire translation unit, which can lead to hard-to-debug build behavior. If USE_DS_EVENT_SUPPORTED must not be used with USE_FIREBOLT, fail fast with a compile-time error (or gate the DS code with an additional !USE_FIREBOLT condition).
#ifdef USE_FIREBOLT
#undef USE_DS_EVENT_SUPPORTED
#endif
#ifndef USE_FIREBOLT
externals/rdk/PlayerExternalsRdkInterface.cpp:79
- In the USE_FIREBOLT initialization path, m_initialized is never set to InitState::FIREBOLT. As a result, subsequent Initialize() calls will keep re-initializing because the NOT_INITIALIZED guard never flips.
// Reset before assigning new interface
m_pDeviceInterfaceBase = nullptr;
m_pDeviceInterfaceBase = DeviceFireboltInterface::GetInstance();
DeviceFireboltInterface::Initialize();
#else
externals/rdk/IFirebolt/DeviceFireboltInterface.cpp:223
- The HDCP 1.4 branch logs "HDCP 2.2 detected" and has a closing brace on the same line as the log, which is misleading and easy to miss during review.
else if (hdcpMap.hdcp1_4)
{
pInstance->SetHDCPEnabled(true);
pInstance->setHdcpProtocol(dsHDCP_VERSION_1X);
MW_LOG_WARN("[FIREBOLT] SetHDMIStatus: HDCP 2.2 detected"); }
Comment on lines
+54
to
+56
| #if(NOT CMAKE_USE_FIREBOLT AND NOT CMAKE_PLATFORM_UBUNTU) | ||
| #include_directories(${CMAKE_CURRENT_SOURCE_DIR}/rdk/IIarm) | ||
| #endif() |
Comment on lines
134
to
+138
| #IARM/RFC | ||
| if(CMAKE_IARM_MGR) | ||
| message("PLAYER IARM_MGR set") | ||
| set(LIB_EXT_DEFINES "${LIB_EXT_DEFINES} -DIARM_MGR=1") | ||
| if(CMAKE_USE_FIREBOLT AND NOT CMAKE_PLATFORM_UBUNTU) | ||
| # RDK-E: Firebolt-only; no IARM or ds | ||
| message("USE_FIREBOLT set - RDK-E build: routing device APIs through Firebolt") | ||
| add_definitions(-DUSE_FIREBOLT) |
| @@ -96,7 +96,7 @@ if [[ "$OSTYPE" == "darwin"* ]]; then | |||
| 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 -DUSE_DS_EVENT_SUPPORTED=ON ../ | |||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.