Feature/rdkemw 8587 dlsym by suppal045 · Pull Request #186 · rdkcentral/devicesettings

suppal045 · 2025-12-11T16:17:19Z

No description provided.

* RDKEMW-4848: pass the proper handle to dsEnableHDCP

Copilot

Pull request overview

This PR refactors the dynamic library loading mechanism for device settings configuration to use a shared library handle instead of opening/closing the library multiple times. The changes introduce a centralized getDLInstance() and releaseDLInstance() pattern, and update all config loading methods to accept a handle parameter.

Key Changes

Centralized dynamic library handle management with thread-safe singleton pattern
Updated all config load methods to accept a void* pDLHandle parameter
Added support for FrontPanelConfig in the initialization flow
Removed the parse_opt_flag() function and associated delay logic

Reviewed changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated 6 comments.

Show a summary per file

File	Description
ds/manager.cpp	Added getDLInstance/releaseDLInstance functions, refactored searchConfigs to use provided handle, updated Initialize/load methods to pass handle to configs, removed parse_opt_flag function and delay logic
ds/include/manager.hpp	Updated searchConfigs declaration to include pDLHandle parameter, removed parse_opt_flag declaration
ds/videoOutputPortConfig.hpp	Updated load method signature to accept pDLHandle parameter
ds/videoOutputPortConfig.cpp	Refactored load to accept handle parameter, moved symbol search variables into conditional block, added dynamic vs old config loading logic
ds/videoDeviceConfig.hpp	Updated load method signature to accept pDLHandle parameter
ds/videoDeviceConfig.cpp	Refactored load to accept handle parameter, added dynamic vs old config loading logic
ds/audioOutputPortConfig.hpp	Updated load method signature to accept pDLHandle parameter
ds/audioOutputPortConfig.cpp	Refactored load to accept handle parameter, added dynamic vs old config loading logic
ds/include/frontPanelConfig.hpp	Moved load method from private to public, added m_isFPConfigLoaded member, updated load signature to accept pDLHandle
ds/frontPanelConfig.cpp	Removed load call from getInstance, added load validation checks with m_isFPConfigLoaded, refactored load to accept handle parameter and support dynamic/old config loading

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-12-11T16:21:04Z

ds/frontPanelConfig.cpp

-														  configuration.pKIndicators[i].colorMode));
-				}
-		}
+    if (pDLHandle)


In the FrontPanelConfig::load method, the check for pDLHandle uses a different null comparison pattern (pDLHandle without nullptr comparison) compared to other config files which consistently use "nullptr != pDLHandle". Consider using "nullptr != pDLHandle" for consistency with the rest of the codebase.

Suggested change

if (pDLHandle)

if (nullptr != pDLHandle)

Copilot · 2025-12-11T16:21:04Z

ds/frontPanelConfig.cpp

+    }
+    m_isFPConfigLoaded = true;
 	INT_INFO("Exit function\n");
+    return;


The explicit return statement at the end of a void function is unnecessary. Since this function returns void, the return statement at line 554 can be removed as the function will automatically return at its end.

Suggested change

return;

Copilot · 2025-12-11T16:21:05Z

ds/videoDeviceConfig.cpp

+
+    if ( false == isDynamicConfigLoad)
 	{
 		INT_INFO("Read Old Configs\n");


The log message format for old config loading is inconsistent. In videoDeviceConfig.cpp line 164 it says "Read Old Configs" while other files (videoOutputPortConfig.cpp line 435, audioOutputPortConfig.cpp line 235, frontPanelConfig.cpp line 482) consistently use "Using OLD config loading". Consider updating this message to match the consistent pattern used elsewhere.

Suggested change

INT_INFO("Read Old Configs\n");

INT_INFO("Using OLD config loading\n");

Copilot · 2025-12-11T16:21:05Z

ds/manager.cpp

+void* getDLInstance()
+{
+    std::lock_guard<std::mutex> lock(gDLMutex);
+    dlerror(); // clear old error
+    if (nullptr == gDLHandle){
+        gDLHandle = dlopen(RDK_DSHAL_NAME, RTLD_LAZY);
+    }
+    INT_INFO("%d:%s: DL Instance '%s'\n", __LINE__, __func__, (nullptr == gDLHandle ? "NULL" : "Valid"));
+    return gDLHandle;
+}


The getDLInstance function does not log or handle the error when dlopen fails. The old implementation would log the error message from dlerror(), but this new implementation only logs whether the handle is NULL or Valid. Consider logging the actual error message from dlerror() when dlopen fails to aid in debugging issues with loading the dynamic library.

Copilot · 2025-12-11T16:21:06Z

ds/manager.cpp

+    void* pDLHandle = getDLInstance();
+	device::AudioOutputPortConfig::getInstance().load(pDLHandle);
+	device::VideoOutputPortConfig::getInstance().load(pDLHandle);
+	device::VideoDeviceConfig::getInstance().load(pDLHandle);


The Manager::load() method calls load for AudioOutputPortConfig, VideoOutputPortConfig, and VideoDeviceConfig, but does not call FrontPanelConfig::getInstance().load(pDLHandle). This is inconsistent with Manager::Initialize() which calls load on all four config classes including FrontPanelConfig. This inconsistency could lead to FrontPanelConfig not being properly loaded when Manager::load() is called.

Suggested change

device::VideoDeviceConfig::getInstance().load(pDLHandle);

device::VideoDeviceConfig::getInstance().load(pDLHandle);

device::FrontPanelConfig::getInstance().load(pDLHandle);

Copilot

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

Copilot

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

Reason for change: consume the config variables using dlsym() in MW. Test Procedure: refer RDKEMW-8587 Risks: High Signed-off-by:gsanto722 <grandhi_santoshkumar@comcast.com>

Signed-off-by: grandhi_santoshkumar <grandhi_santoshkumar@comcast.com>

Reason for change: consume the config variables using dlsym() in MW. Test Procedure: refer RDKEMW-8587 Risks: High Signed-off-by:gsanto722 <grandhi_santoshkumar@comcast.com>

Reason for change: consume the config variables using dlsym() in MW. Test Procedure: refer RDKEMW-8587 Risks: High Signed-off-by: grandhi_santoshkumar <grandhi_santoshkumar@comcast.com>

Reason for change: consume the config variables using dlsym() in MW. Test Procedure: refer RDKEMW-8587 Risks: High Signed-off-by:gsanto722 <grandhi_santoshkumar@comcast.com>

Copilot

Pull request overview

Copilot reviewed 31 out of 31 changed files in this pull request and generated 22 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-02-23T16:58:59Z

rpc/srv/dsConfigs.c

+
+    INT_INFO("Entering capabilityType = 0x%08X\n", capabilityType);
+    dlerror(); /* clear old error */
+    pDLHandle = dlopen(RDK_DSHAL_NAME, RTLD_LAZY);


Missing error handling: The function doesn't check for dlopen failures. If pDLHandle is NULL (dlopen failed), the code will pass NULL to LoadDLSymbols, which will fail silently and fall back to static configurations. Consider logging dlerror() message when dlopen fails to help diagnose library loading issues.

Suggested change

pDLHandle = dlopen(RDK_DSHAL_NAME, RTLD_LAZY);

pDLHandle = dlopen(RDK_DSHAL_NAME, RTLD_LAZY);

if (NULL == pDLHandle) {

const char* dlErrMsg = dlerror();

if (dlErrMsg != NULL) {

INT_ERROR("Failed to load '%s': %s\n", RDK_DSHAL_NAME, dlErrMsg);

} else {

INT_ERROR("Failed to load '%s': unknown dlopen error\n", RDK_DSHAL_NAME);

}

}

Copilot · 2026-02-23T16:59:00Z

ds/videoDeviceConfig.cpp

+#define DEBUG 1 // Using for dumpconfig 



Undefined preprocessor macro: The code references DEBUG macro at line 38, but this macro is defined locally in this file. However, DEBUG is commonly a predefined macro in many build systems. Consider using a more specific name like DS_VIDEO_DEVICE_DEBUG to avoid conflicts.

Suggested change

#define DEBUG 1 // Using for dumpconfig

// Local debug flag for VideoDeviceConfig; avoid clobbering global DEBUG.

#ifndef DS_VIDEO_DEVICE_DEBUG

#define DS_VIDEO_DEVICE_DEBUG 1 // Using for dumpconfig

#endif

// Preserve existing behavior: only define DEBUG here if not provided by build system.

#ifndef DEBUG

#define DEBUG DS_VIDEO_DEVICE_DEBUG

#endif

Copilot · 2026-02-23T16:59:00Z

ds/videoOutputPortType.cpp

+                                 : dsVIDEOPORT_TYPE_INTERNAL;
+    ret = dsGetVideoPort(portType, 0, &handle);
+    if ((ret == dsERR_NONE) && (handle != -1)) {
+        ret = dsEnableHDCP(handle, contentProtect, hdcpKey, keySize);


Changed API signature: The dsEnableHDCP function call changed from passing a port type (dsVideoPortType_t) to passing a handle (intptr_t). This is a breaking API change that needs to be verified against the HAL library implementation. Ensure that the HAL library's dsEnableHDCP function signature matches this new usage.

Copilot · 2026-02-23T16:59:00Z

rpc/srv/dsDisplay.c

    errno_t rc = -1;
    dsVideoPortResolution_t *edidResn = NULL;
-    dsVideoPortResolution_t *presolution = NULL;
+    dsVideoPortResolution_t *presolution = NULL, *kVidoeResolutionsSettings = NULL;


Spelling error in variable name: kVidoeResolutionsSettings should be kVideoResolutionsSettings (misspelled "Video" as "Vidoe"). This variable appears multiple times in this function.

Copilot · 2026-02-23T16:59:01Z

rpc/srv/dsVideoPort.c

-            componentPortPresent = true;;
+            if (typeConfigs[i].typeId == dsVIDEOPORT_TYPE_COMPONENT)
+            {
+                componentPortPresent = true;


Unnecessary double semicolon at the end of the statement. Remove one semicolon.

Copilot · 2026-02-23T16:59:04Z

ds/videoOutputPortConfig.cpp

@@ -259,88 +257,207 @@ List<VideoResolution>  VideoOutputPortConfig::getSupportedResolutions(bool isIgn
 		for (VideoResolution resolution : tmpsupportedResolutions){
 			_supportedResolutions.push_back(resolution);
 		}
+		cout << "[DsMgr] _supportedResolutions cache after update, size: " << _supportedResolutions.size() << endl;
+		for (size_t idx = 0; idx < _supportedResolutions.size(); idx++) {
+			const VideoResolution& res = _supportedResolutions[idx];
+			cout << "[DsMgr] [" << idx << "] " << res.getName() 
+			     << " pixelRes=" << res.getPixelResolution().getId()
+			     << " aspectRatio=" << res.getAspectRatio().getId()
+			     << " frameRate=" << res.getFrameRate().getId() << endl;
+		}


Debugging code left in production: The cout statements (lines 220-230, 260-267) appear to be debug code that should be removed or replaced with proper logging macros (INT_INFO/INT_DEBUG) for consistency with the rest of the codebase.

Copilot · 2026-02-23T16:59:04Z

rpc/srv/dsVideoPortConfig.c

+    return numElements;
+}
+
+int dsLoadVideoOutputPortConfig(const videoPortConfigs_t* dynamicVideoPortConfigs)


API signature mismatch: The header file declares dsLoadVideoOutputPortConfig as returning void (line 53 of dsVideoPortConfig.h), but the implementation returns int. This inconsistency will cause compilation errors or undefined behavior.

Copilot · 2026-02-23T16:59:05Z

rpc/srv/dsVideoDeviceConfig.c

+    return numElements;
+}
+
+int dsLoadVideoDeviceConfig(const videoDeviceConfig_t* dynamicVideoDeviceConfigs)


API signature mismatch: The header file declares dsLoadVideoDeviceConfig as returning void (line 48 of dsVideoDeviceConfig.h), but the implementation returns int. This inconsistency will cause compilation errors or undefined behavior.

Copilot · 2026-02-23T16:59:05Z

rpc/srv/dsAudioConfig.c

+	int kPortSize;
+}audioConfigsLocal_t;
+
+static audioConfigsLocal_t audioConfiguration = {0};


Potential race condition: The global static variable audioConfiguration is accessed and modified without any mutex protection. Multiple threads could potentially call dsLoadAudioOutputPortConfig or the getter functions concurrently, leading to data races. Consider adding mutex protection around accesses to this shared state.

Copilot · 2026-02-23T16:59:05Z

rpc/srv/dsVideoPortConfig.c

+int dsLoadVideoOutputPortConfig(const videoPortConfigs_t* dynamicVideoPortConfigs)
+{
+    int configSize, portSize, resolutionSize, defaultResIndex;
+    const dsVideoPortTypeConfig_t* videoPortConfigs;
+    const dsVideoPortPortConfig_t* videoPortPorts;
+    const dsVideoPortResolution_t* videoPortResolutions;
+    bool isDynamic;
+
+    INT_INFO("Using '%s' config\n", dynamicVideoPortConfigs ? "dynamic" : "static");
+
+    // Set up parameters based on config source
+    if (NULL != dynamicVideoPortConfigs) {
+        configSize = *(dynamicVideoPortConfigs->pKVideoPortConfigs_size);
+        portSize = *(dynamicVideoPortConfigs->pKVideoPortPorts_size);
+        resolutionSize = *(dynamicVideoPortConfigs->pKResolutionsSettings_size);
+        defaultResIndex = *(dynamicVideoPortConfigs->pKDefaultResIndex);
+        videoPortConfigs = dynamicVideoPortConfigs->pKVideoPortConfigs;
+        videoPortPorts = dynamicVideoPortConfigs->pKVideoPortPorts;
+        videoPortResolutions = dynamicVideoPortConfigs->pKVideoPortResolutionsSettings;
+        isDynamic = true;
+    } else {
+        configSize = dsUTL_DIM(kConfigs);
+        portSize = dsUTL_DIM(kPorts);
+        resolutionSize = dsUTL_DIM(kResolutions);
+        defaultResIndex = kDefaultResIndex;
+        videoPortConfigs = kConfigs;
+        videoPortPorts = kPorts;
+        videoPortResolutions = kResolutions;
+        isDynamic = false;
+    }
+
+    // Allocate and copy video port type configs
+    if (allocateAndCopyVideoPortConfigs(videoPortConfigs, configSize, isDynamic) == -1) {
+        INT_ERROR("Failed to allocate video port configs\n");
+        return -1;
+    }
+
+    // Allocate and copy video port ports
+    if (allocateAndCopyVideoPortPorts(videoPortPorts, portSize, isDynamic) == -1) {
+        INT_ERROR("Failed to allocate video port ports\n");
+        // Clean up previously allocated memory
+        if (videoPortConfiguration.pKVideoPortConfigs != NULL) {
+            free(videoPortConfiguration.pKVideoPortConfigs);
+            videoPortConfiguration.pKVideoPortConfigs = NULL;
+        }
+        return -1;
+    }
+
+    // Allocate and copy video port resolutions
+    if (allocateAndCopyVideoPortResolutions(videoPortResolutions, resolutionSize, isDynamic) == -1) {
+        INT_ERROR("Failed to allocate video port resolutions\n");
+        // Clean up previously allocated memory
+        if (videoPortConfiguration.pKVideoPortConfigs != NULL) {
+            free(videoPortConfiguration.pKVideoPortConfigs);
+            videoPortConfiguration.pKVideoPortConfigs = NULL;
+        }
+        if (videoPortConfiguration.pKVideoPortPorts != NULL) {
+            free(videoPortConfiguration.pKVideoPortPorts);
+            videoPortConfiguration.pKVideoPortPorts = NULL;
+        }
+        return -1;
+    }
+
+    INT_INFO("Store sizes configSize =%d, portSize =%d, resolutionSize = %d\n", configSize, portSize, resolutionSize);
+    videoPortConfiguration.kDefaultResIndex = defaultResIndex;
+    INT_INFO("Store sizes videoPortConfiguration.kDefaultResIndex = %d\n", videoPortConfiguration.kDefaultResIndex);
+
+    INT_INFO("Store sizes configSize =%d, portSize =%d, resolutionSize = %d\n", configSize, portSize, resolutionSize);
+    videoPortConfiguration.kVideoPortConfigs_size = configSize;
+    videoPortConfiguration.kVideoPortPorts_size = portSize;
+    videoPortConfiguration.kResolutionsSettings_size = resolutionSize;
+    INT_INFO("Store sizes kVideoPortConfigs_size = %d\n", videoPortConfiguration.kVideoPortConfigs_size);
+    INT_INFO("Store sizes kVideoPortPorts_size = %d\n", videoPortConfiguration.kVideoPortPorts_size);
+    INT_INFO("Store sizes kResolutionsSettings_size = %d\n", videoPortConfiguration.kResolutionsSettings_size);
+
+    INT_INFO("VideoPort Config[%p] ConfigSize[%d] Ports[%p] PortSize[%d] Resolutions[%p] ResolutionSize[%d] DefaultResIndex[%d]\n",
+            videoPortConfiguration.pKVideoPortConfigs,
+            videoPortConfiguration.kVideoPortConfigs_size,
+            videoPortConfiguration.pKVideoPortPorts,
+            videoPortConfiguration.kVideoPortPorts_size,
+            videoPortConfiguration.pKVideoPortResolutionsSettings,
+            videoPortConfiguration.kResolutionsSettings_size,
+            videoPortConfiguration.kDefaultResIndex);
+    videoPortDumpconfig(&videoPortConfiguration);
+    return 0;
+}


Potential double-free vulnerability: If dsLoadVideoOutputPortConfig is called multiple times, the previously allocated memory will be leaked before being replaced with new allocations. Additionally, there's no check to prevent double-initialization. Consider adding a flag to track initialization state and free existing memory before reallocating.

Copilot

Pull request overview

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

Comments suppressed due to low confidence (1)

rpc/srv/dsVideoPort.c:1968

Potential NULL pointer dereference. If kVidoeResolutionsSettings is NULL (which can happen if dsGetVideoPortResolutions fails), the code will attempt to dereference it at line 1959 and in the loop at line 1963. Add a NULL check after line 1946.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-02-24T12:52:47Z

rpc/srv/dsVideoPort.c

+    dsGetVideoPortPortConfigs(&numPorts, &kVideoPortPorts);
+
+    // Print kVideoPortPorts array
+    INT_INFO("[DsMgr] ========== Dumping kVideoPortPorts Array in _GetVideoPortType ==========\r\n");
+    INT_INFO("[DsMgr] Total ports retrieved: %d\r\n", numPorts);
+    for (int k = 0; k < numPorts; k++) {
+        const dsVideoPortPortConfig_t *port = &kVideoPortPorts[k];
+        INT_INFO("[DsMgr] [%d] type=%d, index=%d, connectedAOP_type=%d, connectedAOP_index=%d, defaultResolution=%s\r\n",
+                 k, port->id.type, port->id.index, 
+                 port->connectedAOP.type, port->connectedAOP.index, 
+                 port->defaultResolution ? port->defaultResolution : "NULL");
+    }
+    INT_INFO("[DsMgr] ========== End of kVideoPortPorts Dump ==========\r\n");

-    numPorts = dsUTL_DIM(kSupportedPortTypes);
    for(i=0; i< numPorts; i++)
    {
-		dsGetVideoPort(kPorts[i].id.type, kPorts[i].id.index, &halhandle);
+		dsGetVideoPort(kVideoPortPorts[i].id.type, kVideoPortPorts[i].id.index, &halhandle);
 		if (handle == halhandle)
 		{
-			return kPorts[i].id.type;
+			return kVideoPortPorts[i].id.type;
 		}
 	}


Potential NULL pointer dereference in error path. If kVideoPortPorts is NULL (line 1708), the code continues to iterate over numPorts (line 1724) and dereference kVideoPortPorts[i] (line 1726), which would cause a crash. Add a NULL check before the loop.

Copilot · 2026-02-24T12:52:47Z

rpc/srv/dsVideoPort.c

+       		  {
+        	      int numResolutions = 0;
+        	      dsVideoPortResolution_t* resolutions = NULL;
+        	      int defaultIndex = 0;
+        	      dsGetVideoPortResolutions(&numResolutions, &resolutions);
+        	      dsGetDefaultResolutionIndex(&defaultIndex);
+        	      if (resolutions && defaultIndex >= 0 && defaultIndex < numResolutions) {
+        	          return resolution.assign(resolutions[defaultIndex].name);
+        	      }
+       		  }


Potential NULL pointer dereference. If resolutions is NULL or defaultIndex is out of bounds, the code will attempt to access resolutions[defaultIndex].name which could crash. Add proper NULL and bounds checking before accessing the array.

Copilot · 2026-02-24T12:52:47Z

rpc/srv/dsVideoPort.c

    return eRet;
 }

+//This funcation does not have any caller.


Typo in comment: "funcation" should be "function".

Copilot · 2026-02-24T12:52:48Z

rpc/srv/dsDisplay.c

@@ -604,14 +620,14 @@ static void filterEDIDResolution(intptr_t handle, dsDisplayEDID_t *edid)
        edid->numOfSupportedResolution = 0;
        for (size_t i = 0; i < iCount; i++)
        {
-            presolution = &kResolutions[i];
+            presolution = &kVidoeResolutionsSettings[i];
            for (size_t j = 0; j < edidData->numOfSupportedResolution; j++)
            {    
                edidResn = &(edidData->suppResolutionList[j]);

                if (0 == (strcmp(presolution->name,edidResn->name)))
                {
-                    edid->suppResolutionList[edid->numOfSupportedResolution] = kResolutions[i];
+                    edid->suppResolutionList[edid->numOfSupportedResolution] = kVidoeResolutionsSettings[i];
                    edid->numOfSupportedResolution++;
                    numOfSupportedResolution++;
                    INT_DEBUG("[DsMgr] presolution->name : %s, resolution count : %d\r\n",presolution->name,numOfSupportedResolution);


Potential NULL pointer dereference. If kVidoeResolutionsSettings is NULL (which can happen if dsGetVideoPortResolutions fails), the code will attempt to dereference it at line 623 and line 630. Add a NULL check after line 598 before using the pointer.

Copilot · 2026-02-24T12:52:48Z

rpc/srv/dsDisplay.c

+    dsGetVideoPortPortConfigs(&numPorts, &kVideoPortPorts);
+
+    // Print kVideoPortPorts array
+    INT_INFO("[DsMgr] ========== Dumping kVideoPortPorts Array ==========\r\n");
+    INT_INFO("[DsMgr] Total ports retrieved: %d\r\n", numPorts);
+    for (int k = 0; k < numPorts; k++) {
+        const dsVideoPortPortConfig_t *port = &kVideoPortPorts[k];
+        INT_INFO("[DsMgr] [%d] type=%d, index=%d, connectedAOP_type=%d, connectedAOP_index=%d, defaultResolution=%s\r\n",
+                 k, port->id.type, port->id.index, 
+                 port->connectedAOP.type, port->connectedAOP.index, 
+                 port->defaultResolution);
+    }
+    INT_INFO("[DsMgr] ========== End of kVideoPortPorts Dump ==========\r\n");

-    numPorts = dsUTL_DIM(kSupportedPortTypes);
    for(i=0; i< numPorts; i++)
    {
-        dsGetDisplay(kPorts[i].id.type, kPorts[i].id.index, &halhandle);
+        dsGetDisplay(kVideoPortPorts[i].id.type, kVideoPortPorts[i].id.index, &halhandle);
        if (handle == halhandle)
        {
-            return kPorts[i].id.type;
+            return kVideoPortPorts[i].id.type;
        }
    }


Potential NULL pointer dereference. If kVideoPortPorts is NULL (which can happen if dsGetVideoPortPortConfigs fails), the code will attempt to dereference it in the loop at line 684. Add a NULL check after line 668.

Copilot · 2026-02-24T12:52:48Z

rpc/srv/dsDisplay.c

@@ -590,7 +592,21 @@ static void filterEDIDResolution(intptr_t handle, dsDisplayEDID_t *edid)
    if(_VPortType == dsVIDEOPORT_TYPE_HDMI)
    { 
        INT_DEBUG("EDID for HDMI Port\r\n");    
-        size_t iCount = dsUTL_DIM(kResolutions);
+        //size_t iCount = dsUTL_DIM(kResolutions);
+        int iCount = 0;
+        //Get details from libds
+        dsGetVideoPortResolutions(&iCount, &kVidoeResolutionsSettings);
+
+        // Print kVidoeResolutionsSettings array
+        INT_INFO("[DsMgr] ========== Dumping kVidoeResolutionsSettings Array ==========\r\n");
+        INT_INFO("[DsMgr] Total resolutions retrieved: %d\r\n", iCount);
+        for (int k = 0; k < iCount; k++) {
+            dsVideoPortResolution_t *res = &kVidoeResolutionsSettings[k];
+            INT_INFO("[DsMgr] [%d] name=%s, pixelRes=%d, aspectRatio=%d, stereoMode=%d, frameRate=%d, interlaced=%d\r\n",
+                     k, res->name, res->pixelResolution, res->aspectRatio, 
+                     res->stereoScopicMode, res->frameRate, res->interlaced);
+        }
+        INT_INFO("[DsMgr] ========== End of kVidoeResolutionsSettings Dump ==========\r\n");

        /*Initialize the struct*/
        memset(edidData,0,sizeof(*edidData));
@@ -604,14 +620,14 @@ static void filterEDIDResolution(intptr_t handle, dsDisplayEDID_t *edid)
        edid->numOfSupportedResolution = 0;
        for (size_t i = 0; i < iCount; i++)
        {
-            presolution = &kResolutions[i];
+            presolution = &kVidoeResolutionsSettings[i];
            for (size_t j = 0; j < edidData->numOfSupportedResolution; j++)
            {    
                edidResn = &(edidData->suppResolutionList[j]);

                if (0 == (strcmp(presolution->name,edidResn->name)))
                {
-                    edid->suppResolutionList[edid->numOfSupportedResolution] = kResolutions[i];
+                    edid->suppResolutionList[edid->numOfSupportedResolution] = kVidoeResolutionsSettings[i];


Spelling error: "kVidoeResolutionsSettings" should be "kVideoResolutionsSettings" (missing 'i' in "Video"). This variable name appears multiple times and should be corrected for consistency.

Copilot · 2026-02-24T12:52:49Z

rpc/srv/dsAudio.c

+    dsGetAudioPortConfigs(&numPorts, &kAudioPorts);

    for(i=0; i< numPorts; i++)
    {
-        if(dsGetAudioPort (kPorts[i].id.type, kPorts[i].id.index, &halhandle) == dsERR_NONE) {
+        if(dsGetAudioPort (kAudioPorts[i].id.type, kAudioPorts[i].id.index, &halhandle) == dsERR_NONE) {
            if (handle == halhandle)
            {
-                return kPorts[i].id.type;
+                return kAudioPorts[i].id.type;
            }
        }
    }


Potential NULL pointer dereference. If kAudioPorts is NULL (which can happen if dsGetAudioPortConfigs fails), the code will attempt to dereference it in the loop at line 3804. Add a NULL check after line 3800.

Copilot · 2026-02-24T12:52:49Z

ds/videoDevice.cpp

 * @return None.
 */
-VideoDevice::VideoDevice(int id)
+VideoDevice::VideoDevice(int id): _dfc(0)


Inconsistent member initialization order. The member _dfc is initialized in the initializer list, but _handle is assigned in the constructor body. For consistency and to avoid potential issues with initialization order, consider initializing all members in the initializer list or all in the constructor body. Note that the original code didn't initialize _dfc at all, so this change is adding initialization.

Copilot · 2026-02-24T12:52:49Z

rpc/srv/dsVideoPort.c

+    dsGetVideoPortPortConfigs(&numPorts, &kVideoPortPorts);
+
+    // Print kVideoPortPorts array
+    INT_INFO("[DsMgr] ========== Dumping kVideoPortPorts Array in dsGetDefaultPortHandle ==========\r\n");
+    INT_INFO("[DsMgr] Total ports retrieved: %d\r\n", numPorts);
+    for (int k = 0; k < numPorts; k++) {
+        const dsVideoPortPortConfig_t *port = &kVideoPortPorts[k];
+        INT_INFO("[DsMgr] [%d] type=%d, index=%d, connectedAOP_type=%d, connectedAOP_index=%d, defaultResolution=%s\r\n",
+                 k, port->id.type, port->id.index, 
+                 port->connectedAOP.type, port->connectedAOP.index, 
+                 port->defaultResolution ? port->defaultResolution : "NULL");
+    }
+    INT_INFO("[DsMgr] ========== End of kVideoPortPorts Dump ==========\r\n");
+
    for(i=0; i< numPorts; i++)
    {
-        dsGetVideoPort(kPorts[i].id.type, kPorts[i].id.index, &halhandle);
-        if (dsVIDEOPORT_TYPE_HDMI == kPorts[i].id.type ||
-             dsVIDEOPORT_TYPE_INTERNAL == kPorts[i].id.type)
+        dsGetVideoPort(kVideoPortPorts[i].id.type, kVideoPortPorts[i].id.index, &halhandle);
+        if (dsVIDEOPORT_TYPE_HDMI == kVideoPortPorts[i].id.type ||
+             dsVIDEOPORT_TYPE_INTERNAL == kVideoPortPorts[i].id.type)
        {
            return halhandle;
        }


Potential NULL pointer dereference. If kVideoPortPorts is NULL (which can happen if dsGetVideoPortPortConfigs fails to retrieve data), the code will attempt to dereference it in the loop at line 2452. Add a NULL check after line 2436.

Copilot · 2026-02-24T12:52:49Z

rpc/srv/dsVideoPortConfig.c

+    INT_INFO("\n=============== Starting to Dump VideoPort Configs ===============\n");
+
+    int configSize = -1, portSize = -1, resolutionSize = -1;
+    if (( NULL != config->pKVideoPortConfigs ) && ( NULL != config->pKVideoPortPorts ) && ( nullptr != config->pKVideoPortResolutionsSettings ))


The NULL check uses C-style nullptr in a C file. In C, NULL should be used instead of nullptr, which is a C++ keyword.

Copilot

Pull request overview

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

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-02-26T15:54:23Z

rpc/srv/dsAudioConfig.c

+    audioConfiguration.pKAudioConfigs = (dsAudioTypeConfig_t*)malloc(numElements * sizeof(dsAudioTypeConfig_t));
+    if (audioConfiguration.pKAudioConfigs == NULL) {
+        INT_ERROR("Failed to allocate memory for %s audio configs\n", configType);
+        return -1;
+    }
+
+    memcpy(audioConfiguration.pKAudioConfigs, source, numElements * sizeof(dsAudioTypeConfig_t));
+    INT_INFO("Allocated and copied %d audio configs (%s)", numElements, configType);
+    return numElements;


allocateAndCopyAudioConfigs() does a raw memcpy() of dsAudioTypeConfig_t. Those structs contain pointer fields (e.g., name, encodings, compressions, stereoModes in the existing configs), and dsLoadConfigs() closes the HAL dlopen() handle after loading. That makes these pointers dangle (use-after-dlclose) when the configs are later consumed. Deep-copy all pointer members (and any pointed-to arrays) into DSMgr-owned memory, or keep the HAL library loaded for the lifetime of the copied configs.

Copilot · 2026-02-26T15:54:23Z

rpc/srv/dsVideoPortConfig.c

+        if (videoPortConfiguration.pKVideoPortResolutionsSettings != NULL) {
+            for (int i = 0; i < videoPortConfiguration.kResolutionsSettings_size; ++i) {
+                if (videoPortConfiguration.pKVideoPortResolutionsSettings[i].name != NULL) {
+                    free(videoPortConfiguration.pKVideoPortResolutionsSettings[i].name);
+                }
+            }
+            free(videoPortConfiguration.pKVideoPortResolutionsSettings);
+            videoPortConfiguration.pKVideoPortResolutionsSettings = NULL;
+        }


The error cleanup path attempts to free(videoPortConfiguration.pKVideoPortResolutionsSettings[i].name), but this file explicitly documents that dsVideoPortResolution_t::name is a fixed array (see the comment in allocateAndCopyVideoPortResolutions()), not heap memory. This would be an invalid free if the loop ever runs. Remove the per-element free and only free(pKVideoPortResolutionsSettings) (and make sure kResolutionsSettings_size is set before using it for cleanup).

Copilot · 2026-02-26T15:54:23Z

rpc/srv/dsVideoPortConfig.c

+        if (videoPortConfiguration.pKVideoPortResolutionsSettings != NULL) {
+            for (int i = 0; i < resolutionSize; ++i) {
+                if (videoPortConfiguration.pKVideoPortResolutionsSettings[i].name != NULL) {
+                    free(videoPortConfiguration.pKVideoPortResolutionsSettings[i].name);
+                }
+            }
+            free(videoPortConfiguration.pKVideoPortResolutionsSettings);
+            videoPortConfiguration.pKVideoPortResolutionsSettings = NULL;
+        }


This second cleanup block also calls free(videoPortConfiguration.pKVideoPortResolutionsSettings[i].name), but dsVideoPortResolution_t::name is treated as a fixed array elsewhere in this file. This would be an invalid free. Only free the containing pKVideoPortResolutionsSettings allocation (and do not attempt to free name).

Copilot · 2026-02-26T15:54:24Z

rpc/srv/dsVideoPortConfig.c

+void dsVideoPortConfigFree(void)
+{
+    INT_INFO("Freeing VideoPort configuration resources\n");
+    // Free video port configs
+    if (videoPortConfiguration.pKVideoPortConfigs != NULL) {
+        free(videoPortConfiguration.pKVideoPortConfigs);
+        videoPortConfiguration.pKVideoPortConfigs = NULL;
+        INT_INFO("Freed pKVideoPortConfigs\n");
+    }
+
+    // Free video port ports
+    if (videoPortConfiguration.pKVideoPortPorts != NULL) {
+        free(videoPortConfiguration.pKVideoPortPorts);
+        videoPortConfiguration.pKVideoPortPorts = NULL;
+        INT_INFO("Freed pKVideoPortPorts\n");
+    }
+
+    // Free video port resolutions
+    if (videoPortConfiguration.pKVideoPortResolutionsSettings != NULL) {
+        free(videoPortConfiguration.pKVideoPortResolutionsSettings);
+        videoPortConfiguration.pKVideoPortResolutionsSettings = NULL;
+        INT_INFO("Freed pKVideoPortResolutionsSettings\n");
+    }


dsVideoPortConfigFree() frees only the top-level arrays but never frees the per-entry heap allocations created earlier (strdup() for dsVideoPortTypeConfig_t::name and dsVideoPortPortConfig_t::defaultResolution). This leaks memory every time configs are loaded/freed. Free those duplicated strings (iterate the arrays) before freeing the arrays themselves.

Copilot · 2026-02-26T15:54:24Z

rpc/srv/dsVideoDeviceConfig.c

+void videoDeviceDumpconfig(videoDeviceConfigLocal_t *config)
+{
+    if (NULL == config) {
+        INT_ERROR("Video config is NULL\n");
+        return;
+    }
+    if ( -1 == access("/opt/dsMgrDumpDeviceConfigs", F_OK) ) {
+        INT_INFO("Dumping of Device configs is disabled\n");
+        return;
+    }


videoDeviceDumpconfig() uses access() but this file does not include a header that declares it (e.g., <unistd.h>). Since the build compiles *.c with $(CXX), this is typically a compile error. Add the proper include in this file.

Copilot · 2026-02-26T15:54:25Z

ds/videoOutputPortConfig.cpp

+        for (const VideoResolution& resolution : _supportedResolutions) {
+            tmpsupportedResolutions.push_back(resolution);


getSupportedResolutions() iterates _supportedResolutions without holding gSupportedResolutionsMutex, while other code updates _supportedResolutions under that mutex. This introduces a data race and potential iterator invalidation/crash. Take the same mutex while copying _supportedResolutions into tmpsupportedResolutions (or provide a thread-safe snapshot accessor).

Suggested change

for (const VideoResolution& resolution : _supportedResolutions) {

tmpsupportedResolutions.push_back(resolution);

{

std::lock_guard<std::mutex> lock(gSupportedResolutionsMutex);

for (const VideoResolution& resolution : _supportedResolutions) {

tmpsupportedResolutions.push_back(resolution);

}

Copilot · 2026-02-26T15:54:25Z

ds/dslogger.cpp


    if (nullptr != logCb) {
-        logCb(priority, tmp_buff);
+        logCb(priority, formatted);


When a log callback is registered (logCb != nullptr), ds_log() forwards only the formatted message string and drops fileName, lineNum, and func. Since the logging macros now always supply this metadata, callback users will lose critical context. Consider including the same prefix in the string passed to logCb (consistent with the stderr path).

Suggested change

logCb(priority, formatted);

char fullMsg[MAX_LOG_BUFF] = {'\0'};

snprintf(fullMsg,

sizeof(fullMsg),

"[DS][%d] %s [%s:%d] %s: %s",

(int)syscall(SYS_gettid),

levelMap[static_cast<int>(priority)],

fileName,

lineNum,

func,

formatted);

logCb(priority, fullMsg);

Copilot · 2026-02-26T15:54:25Z

rpc/srv/dsVideoPortConfig.c

+                    if (videoPortConfiguration.pKVideoPortConfigs[j].name != NULL)
+                    {
+                        free((void*)videoPortConfiguration.pKVideoPortConfigs[j].name);
+                        videoPortConfiguration.pKVideoPortConfigs[j].name = "NULL";
+                    }
+                }


On strdup failure cleanup, the code sets videoPortConfiguration.pKVideoPortConfigs[j].name = "NULL" after freeing. Assigning a string literal here makes the pointer non-NULL but not owned, which is error-prone (and can lead to freeing a literal if future cleanup code is added). Prefer setting it to NULL after free().

Copilot · 2026-02-26T15:54:26Z

ds/manager.cpp

+    bool needInit = false;
+
+    {std::lock_guard<std::mutex> lock(gManagerInitMutex);
+        printf("Entering %s count %d with thread id %lu\n",__FUNCTION__,IsInitialized,pthread_self());
+        if (IsInitialized == 0) {
+            needInit = true;
+        }
+        IsInitialized++;
+	}


Manager::Initialize() increments IsInitialized under the mutex before performing the one-time initialization work. A second thread entering Initialize() while the first is still initializing will see IsInitialized != 0, skip initialization (needInit == false), and return even though the system may not be ready yet. Consider holding the lock until initialization completes, or introduce a separate "initialization in progress/complete" state (e.g., std::once_flag / call_once, or a condition variable) so other threads block until the first initialization finishes successfully.

Copilot · 2026-02-26T15:54:26Z

ds/videoOutputPortConfig.hpp

 public:
+	std::vector<VideoResolution> 		_supportedResolutions;
+	std::vector<VideoResolution> 		_originalSupportedResolutions;
 	static VideoOutputPortConfig & getInstance();


_supportedResolutions and _originalSupportedResolutions are exposed as public mutable members. This breaks encapsulation and makes it easy to bypass the existing mutex protection (gSupportedResolutionsMutex), which can lead to data races (as already happens elsewhere). Prefer keeping these vectors private and exposing a const, thread-safe accessor (e.g., returns a copy/snapshot or a const& guarded by internal locking).

Copilot

Pull request overview

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

Comments suppressed due to low confidence (2)

rpc/srv/dsDisplay.c:586

filterEDIDResolution allocates edidData but never checks whether malloc succeeded. Also the current NULL check is on edid (with a comment about malloc failure), so a NULL edidData would still be dereferenced by memset/memcpy_s, leading to a crash. Add an explicit if (edidData == NULL) guard (and keep the edid == NULL guard separately).
rpc/srv/dsVideoPort.c:1973
getPixelResolution() overwrites Resn on every loop iteration and always returns Resn->pixelResolution after the loop. If resolution does not match any entry, this returns the last resolution’s pixelResolution rather than the default/fallback resolution, which is incorrect behavior. Track whether a match was found and return the default when not found.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-03-02T17:36:47Z

rpc/srv/dsVideoPort.c

+
+    if (_dsGetVideoPortPortConfigs(&numPorts, &pVideoPortPorts) != dsERR_NONE) {
+        INT_ERROR("Failed to get video port port configurations\n");
+        return NULL;
+    }


dsGetDefaultPortHandle() returns NULL on error, but the return type is intptr_t (an integer type). Returning 0 (or NULL_HANDLE) is clearer and avoids pointer/integer mismatch warnings.

Copilot · 2026-03-02T17:36:48Z

rpc/srv/dsAudioConfig.c

+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include "dsserverlogger.h"
+#include "dsTypes.h"
+#include "dsError.h"
+#include "dsAudioConfig.h"
+#include "dsAudioSettings.h"
+
+
+#ifdef __cplusplus
+extern "C" {
+#endif
+
+typedef struct audioConfigsLocal
+{
+	dsAudioTypeConfig_t  *pKAudioConfigs;
+	dsAudioPortConfig_t  *pKAudioPorts;
+	int kConfigSize;
+	int kPortSize;
+}audioConfigsLocal_t;
+
+static audioConfigsLocal_t audioConfiguration = {0};
+
+void audioDumpconfig(audioConfigsLocal_t *config)
+{
+    if (NULL == config) {
+        INT_ERROR("Audio config is NULL\n");
+        return;
+    }
+    if ( -1 == access("/opt/dsMgrDumpDeviceConfigs", F_OK) ) {
+        INT_INFO("Dumping of Device configs is disabled\n");
+        return;
+    }
+
+    int configSize = -1, portSize = -1;


This file uses access(..., F_OK) but does not include <unistd.h>, which can trigger implicit-declaration warnings (or build failures under stricter flags). Include <unistd.h> (and any needed feature macros) where access is used.

Copilot · 2026-03-02T17:36:48Z

ds/manager.cpp

+    bool needInit = false;
+
+    {std::lock_guard<std::mutex> lock(gManagerInitMutex);
+        printf("Entering %s count %d with thread id %lu\n",__FUNCTION__,IsInitialized,pthread_self());
+        if (IsInitialized == 0) {
+            needInit = true;
+        }
+        IsInitialized++;
+	}


Manager::Initialize() increments IsInitialized and allows subsequent callers to return immediately (needInit == false) even while the first thread is still performing initialization. That creates a race where clients observe the manager as “initialized” before init completes. Consider using std::call_once, or keep a separate “initializing/initialized” state with a condition variable so other threads wait for completion (and only increment the refcount after successful init).

Copilot · 2026-03-02T17:36:48Z

ds/manager.cpp

+dsError_t initializeFunctionWithRetry(const char* functionName, 
+                                   std::function<dsError_t()> initFunc) 
+{
+	dsError_t err = dsERR_GENERAL;
+	unsigned int retryCount = 0;
+	unsigned int maxRetries = 25;
+
+	do {
+		err = initFunc();
+		printf("Manager::Initialize:%s result :%d retryCount :%d\n", 
+				functionName, err, retryCount);
+		if (dsERR_NONE == err) break;
+		usleep(100000);
+	} while (retryCount++ < maxRetries);
+
+	return err;
+}


initializeFunctionWithRetry() retries unconditionally for any error code. This can add ~2.5s delay on permanent failures (e.g., NOT_SUPPORTED, INVALID_PARAM) and makes failures slower to surface. If the intent is only to wait for service readiness, limit retries to dsERR_INVALID_STATE (or a clearly defined retryable set) and fail fast for other errors.

Copilot · 2026-03-02T17:36:49Z

ds/dslogger.cpp

+int ds_log(LogLevel priority, const char* fileName, int lineNum, const char *func, const char *format, ...)
 {
-    char tmp_buff[MAX_LOG_BUFF] = {'\0'};
-
-    int offset = snprintf(tmp_buff, MAX_LOG_BUFF, "[%s:%d] ", fileName, lineNum);
+    char formatted[MAX_LOG_BUFF] = {'\0'};
+    enum LogLevel {INFO_LEVEL = 0, WARN_LEVEL, ERROR_LEVEL, DEBUG_LEVEL, TRACE_LEVEL};
+    const char *levelMap[] = { "INFO", "WARN", "ERROR", "DEBUG", "TRACE"};



ds_log() indexes levelMap with priority without validating the value. If an out-of-range value is ever passed (e.g., from external callbacks), this becomes an out-of-bounds read. Add a bounds check (and/or clamp to a default string) and remove the redundant inner enum LogLevel declaration to avoid confusion/shadowing.

Copilot · 2026-03-02T17:36:49Z

ds/videoOutputPortType.cpp

+    intptr_t handle = -1;
+
+    // Assuming isHDMIOutPortPresent() will only be 'true' for Source devices
+    dsVideoPortType_t portType = device::Host::getInstance().isHDMIOutPortPresent()
+                                 ? dsVIDEOPORT_TYPE_HDMI
+                                 : dsVIDEOPORT_TYPE_INTERNAL;
+    ret = dsGetVideoPort(portType, 0, &handle);
+    if ((ret == dsERR_NONE) && (handle != -1)) {


enabledHDCP() treats -1 as an invalid handle, but elsewhere in the codebase NULL_HANDLE is defined as 0 for intptr_t handles. If dsGetVideoPort() returns 0 on failure, this check would incorrectly treat it as valid. Prefer initializing to 0 and validating against 0/NULL_HANDLE (or use the HAL-defined invalid-handle contract).

Suggested change

intptr_t handle = -1;

// Assuming isHDMIOutPortPresent() will only be 'true' for Source devices

dsVideoPortType_t portType = device::Host::getInstance().isHDMIOutPortPresent()

? dsVIDEOPORT_TYPE_HDMI

: dsVIDEOPORT_TYPE_INTERNAL;

ret = dsGetVideoPort(portType, 0, &handle);

if ((ret == dsERR_NONE) && (handle != -1)) {

intptr_t handle = 0;

// Assuming isHDMIOutPortPresent() will only be 'true' for Source devices

dsVideoPortType_t portType = device::Host::getInstance().isHDMIOutPortPresent()

? dsVIDEOPORT_TYPE_HDMI

: dsVIDEOPORT_TYPE_INTERNAL;

ret = dsGetVideoPort(portType, 0, &handle);

if ((ret == dsERR_NONE) && (handle != 0)) {

Copilot · 2026-03-02T17:36:49Z

ds/host.cpp

+        {
+            cout << " Exception Thrown in getAudioPortHandle().. returning NULL...: " << e.what() << "\n";
+        }
+
+        return NULL;
+   }


getAudioPortHandle() returns NULL but the return type is intptr_t. Return 0 (or a named invalid-handle constant) to avoid pointer/integer mismatch and make the invalid-handle semantics explicit.

Copilot

Pull request overview

Copilot reviewed 41 out of 41 changed files in this pull request and generated 3 comments.

Comments suppressed due to low confidence (1)

rpc/srv/dsDisplay.c:586

edidData is allocated with malloc() but the subsequent NULL-check tests edid instead of edidData. If malloc fails, this will dereference NULL in the following memset/memcpy_s. Check edidData == NULL (and return early) before using it.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-03-03T02:29:39Z

ds/manager.cpp


 int Manager::IsInitialized = 0;   //!< Indicates the application has initialized with devicettings modules.
 static std::mutex gManagerInitMutex;
+static dsError_t initializeFunctionWithRetry(const char* functionName, std::function<dsError_t()> initFunc);


initializeFunctionWithRetry is forward-declared as static (internal linkage) but defined later without static, which will fail to compile due to mismatched linkage. Make the definition static as well, or drop the static from the forward declaration (and keep only one consistent declaration/definition).

Suggested change

static dsError_t initializeFunctionWithRetry(const char* functionName, std::function<dsError_t()> initFunc);

dsError_t initializeFunctionWithRetry(const char* functionName, std::function<dsError_t()> initFunc);

Copilot · 2026-03-03T02:29:40Z

rpc/srv/dsAudioConfig.c

+        // Deep copy connectedVOPs array if present
+        // Note: We need to know the size of connectedVOPs array
+        // For now, assuming it's NULL or a single element (common case)
+        // A proper implementation would need size information
+        if (src->connectedVOPs != NULL) {
+            dest->connectedVOPs = (dsVideoPortPortId_t*)malloc(sizeof(dsVideoPortPortId_t));
+            if (dest->connectedVOPs == NULL) {
+                INT_ERROR("Failed to allocate connectedVOPs for audio port %d\n", i);
+                // Cleanup previously allocated items
+                for (int j = 0; j < i; j++) {
+                    free((void*)audioConfiguration.pKAudioPorts[j].connectedVOPs);
+                }
+                free(audioConfiguration.pKAudioPorts);
+                audioConfiguration.pKAudioPorts = NULL;
+                return -1;
+            }
+            memcpy((void*)dest->connectedVOPs, src->connectedVOPs, sizeof(dsVideoPortPortId_t));
+        } else {


connectedVOPs is being deep-copied as a single dsVideoPortPortId_t, but in the existing static configs it's an array (e.g. connectedVOPs[...][dsVIDEOPORT_TYPE_MAX]). This will truncate the data and can lead to out-of-bounds reads when code expects the full array. Allocate/copy the full array size (or introduce/consume an explicit length field) instead of assuming a single element.

Copilot · 2026-03-03T02:29:40Z

ds/host.cpp

+            cout << " Exception Thrown in getAudioPortHandle().. returning NULL...: " << e.what() << "\n";
+        }
+
+        return NULL;


getAudioPortHandle() returns intptr_t but returns NULL on failure. Return 0 (or a dedicated invalid-handle value) to avoid pointer-to-integer conversion warnings and make the failure value explicit.

Suggested change

cout << " Exception Thrown in getAudioPortHandle().. returning NULL...: " << e.what() << "\n";

}

return NULL;

cout << " Exception Thrown in getAudioPortHandle().. returning 0 (invalid handle)...: " << e.what() << "\n";

}

return 0;

Copilot

Pull request overview

Copilot reviewed 42 out of 42 changed files in this pull request and generated 3 comments.

Comments suppressed due to low confidence (1)

ds/frontPanelConfig.cpp:103

FrontPanelConfig::getInstance() now initializes the HAL but no longer loads the static/dynamic indicator/textDisplay configs, so _indicators/_textDisplays remain empty and subsequent getIndicator/getTextDisplay calls will throw "Bad indicator name". Either restore a load step after successful dsFPInit (for the static default) or make accessors fail with a clear "config not loaded" error until load() is called.

            errorCode = dsFPInit();
            if (dsERR_NONE == errorCode)
            {
                _singleton.m_isFPInitialized = true;
                INT_INFO("dsFPInit success\n");
            }
            else{
                INT_ERROR("dsFPInit failed with error[%d]. Retrying... (%d/20)\n", errorCode, retryCount);
                usleep(50000); // Sleep for 50ms before retrying

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-03-03T06:44:44Z

ds/videoOutputPortConfig.hpp

 public:
+    std::vector<VideoResolution> 		_originalSupportedResolutions;
 	static VideoOutputPortConfig & getInstance();


Making _originalSupportedResolutions a public mutable member exposes internal state and lets callers modify the config cache (and bypass locking). Prefer keeping it private and exposing a const accessor (e.g., getOriginalSupportedResolutions() returning const std::vector<VideoResolution>&).

Copilot · 2026-03-03T06:44:44Z

rpc/srv/dsVideoPort.c

+    if (_dsGetVideoPortPortConfigs(&numPorts, &pVideoPortPorts) != dsERR_NONE) {
+        INT_ERROR("Failed to get video port port configurations\n");
+        return NULL;
+    }


dsGetDefaultPortHandle() returns NULL on error even though the return type is intptr_t. Since callers treat this as a handle and don't check for failure in some paths, returning 0 can lead to HAL calls with an invalid handle. Prefer returning an explicit invalid sentinel (e.g., -1) and updating call sites to guard against it, or propagate the error to the caller instead of returning a handle.

Copilot · 2026-03-03T06:44:45Z

ds/dslogger.cpp

    if (nullptr != logCb) {
-        logCb(priority, tmp_buff);
+        logCb(priority, formatted);
    } else {
-        return printf("%s\n", tmp_buff);
+        fprintf(stderr, "[DS][%d] %s [%s:%d] %s: %s \n",
+                (int)syscall(SYS_gettid),
+                levelMap[static_cast<int>(priority)],
+                fileName,
+                lineNum,
+                func,
+                formatted);


When a log callback is registered, ds_log forwards only the formatted message to logCb and drops the level + file/line/function context that callers expect from the INT_* macros. This is a functional regression for callback consumers; consider building the full prefixed message (including level + [file:line] func) and passing that to the callback too. Also, levelMap[static_cast<int>(priority)] has no bounds check, so an invalid priority can read out of bounds; clamp/validate before indexing.

RDKEMW-11168, RDKEMW-4848: pass the proper handle to dsEnableHDCP (#182)

d3a2867

* RDKEMW-4848: pass the proper handle to dsEnableHDCP

suppal045 requested a review from a team as a code owner December 11, 2025 16:17

Copilot AI review requested due to automatic review settings December 11, 2025 16:17

Copilot started reviewing on behalf of suppal045 December 11, 2025 16:17 View session

Copilot AI reviewed Dec 11, 2025

View reviewed changes

Copilot AI review requested due to automatic review settings December 12, 2025 22:58

Copilot AI reviewed Dec 12, 2025

View reviewed changes

Copilot AI review requested due to automatic review settings December 16, 2025 16:47

Copilot AI reviewed Dec 16, 2025

View reviewed changes

santoshcomcast added 21 commits January 6, 2026 10:48

RDKEMW-8587: consume the config variables using dlsym() in MW.

6abd424

Reason for change: consume the config variables using dlsym() in MW. Test Procedure: refer RDKEMW-8587 Risks: High Signed-off-by:gsanto722 <grandhi_santoshkumar@comcast.com>

fix build issue

59bbd5c

fix build issue

b08a24d

Signed-off-by: grandhi_santoshkumar <grandhi_santoshkumar@comcast.com>

addressed review comments

9cc6b5f

fix build issue

ade5a61

fix build issue

6ac6e50

fix build issue

2dc944b

fix build issue

f6f3d48

fix build issue

441361b

fix coverity issue and removed unwanted code.

8d36de9

Fix build issue.

c676229

Addressed review comments.

968901d

Addressed review comments.

513b3c7

addressed review comments.

fb4785d

RDKEMW-8587: consume the config variables using dlsym() in MW.

35661b4

Reason for change: consume the config variables using dlsym() in MW. Test Procedure: refer RDKEMW-8587 Risks: High Signed-off-by:gsanto722 <grandhi_santoshkumar@comcast.com>

RDKEMW-8587: consume the config variables using dlsym() in MW.

4c52eeb

Reason for change: consume the config variables using dlsym() in MW. Test Procedure: refer RDKEMW-8587 Risks: High Signed-off-by:gsanto722 <grandhi_santoshkumar@comcast.com>

fix build issue

e645685

RDKEMW-8587: consume the config variables using dlsym() in MW.

db4fa61

Reason for change: consume the config variables using dlsym() in MW. Test Procedure: refer RDKEMW-8587 Risks: High Signed-off-by:gsanto722 <grandhi_santoshkumar@comcast.com>

RDKEMW-8587: consume the config variables using dlsym() in MW.

f5838f4

Reason for change: consume the config variables using dlsym() in MW. Test Procedure: refer RDKEMW-8587 Risks: High Signed-off-by:gsanto722 <grandhi_santoshkumar@comcast.com>

RDKEMW-8587: consume the config variables using dlsym() in MW.

82e1bf4

Reason for change: consume the config variables using dlsym() in MW. Test Procedure: refer RDKEMW-8587 Risks: High Signed-off-by: grandhi_santoshkumar <grandhi_santoshkumar@comcast.com>

RDKEMW-8587: consume the config variables using dlsym() in MW.

2ee6669

Reason for change: consume the config variables using dlsym() in MW. Test Procedure: refer RDKEMW-8587 Risks: High Signed-off-by:gsanto722 <grandhi_santoshkumar@comcast.com>

Fix build errors.

31735bd

Copilot AI review requested due to automatic review settings February 23, 2026 16:51

Copilot started reviewing on behalf of santoshcomcast February 23, 2026 16:51 View session

Copilot AI reviewed Feb 23, 2026

View reviewed changes

santoshcomcast and others added 2 commits February 23, 2026 18:11

fix build issue

f2369dd

Merge branch 'develop' into feature/RDKEMW-8587-dlsym

ea8f943

Copilot AI review requested due to automatic review settings February 24, 2026 12:46

Copilot started reviewing on behalf of santoshcomcast February 24, 2026 12:47 View session

Copilot AI reviewed Feb 24, 2026

View reviewed changes

santoshcomcast added 2 commits February 25, 2026 17:25

addresss all review comments

4534399

address copilt review and fix setresolution issue

0cdf7e0

Copilot AI review requested due to automatic review settings February 26, 2026 15:43

Copilot started reviewing on behalf of santoshcomcast February 26, 2026 15:43 View session

Copilot AI reviewed Feb 26, 2026

View reviewed changes

santoshcomcast added 2 commits February 26, 2026 16:50

add use of orginal resolutions

55f0c79

address all the review comments

07a40d8

Copilot AI review requested due to automatic review settings March 2, 2026 17:26

Copilot started reviewing on behalf of santoshcomcast March 2, 2026 17:27 View session

Copilot AI reviewed Mar 2, 2026

View reviewed changes

santoshcomcast added 2 commits March 2, 2026 18:47

address the copilot review

88716e5

fix native build issue

d074e1d

Copilot AI review requested due to automatic review settings March 3, 2026 02:22

Copilot started reviewing on behalf of santoshcomcast March 3, 2026 02:22 View session

Copilot AI reviewed Mar 3, 2026

View reviewed changes

santoshcomcast added 2 commits March 3, 2026 03:24

address copilot review comments

0493378

fix native build issue

ffe1449

Copilot AI review requested due to automatic review settings March 3, 2026 06:36

Copilot started reviewing on behalf of santoshcomcast March 3, 2026 06:37 View session

Copilot AI reviewed Mar 3, 2026

View reviewed changes

addressing copilot review

69f17ed

	INT_INFO("Read Old Configs\n");
	INT_INFO("Using OLD config loading\n");

	device::VideoDeviceConfig::getInstance().load(pDLHandle);
	device::VideoDeviceConfig::getInstance().load(pDLHandle);
	device::FrontPanelConfig::getInstance().load(pDLHandle);

-    pDLHandle = dlopen(RDK_DSHAL_NAME, RTLD_LAZY);
+    pDLHandle = dlopen(RDK_DSHAL_NAME, RTLD_LAZY);
+    if (NULL == pDLHandle) {
+        const char* dlErrMsg = dlerror();
+        if (dlErrMsg != NULL) {
+            INT_ERROR("Failed to load '%s': %s\n", RDK_DSHAL_NAME, dlErrMsg);
+        } else {
+            INT_ERROR("Failed to load '%s': unknown dlopen error\n", RDK_DSHAL_NAME);
+        }
+    }

-#define DEBUG 1 // Using for dumpconfig
+// Local debug flag for VideoDeviceConfig; avoid clobbering global DEBUG.
+#ifndef DS_VIDEO_DEVICE_DEBUG
+#define DS_VIDEO_DEVICE_DEBUG 1 // Using for dumpconfig
+#endif
+// Preserve existing behavior: only define DEBUG here if not provided by build system.
+#ifndef DEBUG
+#define DEBUG DS_VIDEO_DEVICE_DEBUG
+#endif

		for (const VideoResolution& resolution : _supportedResolutions) {
		tmpsupportedResolutions.push_back(resolution);

	static dsError_t initializeFunctionWithRetry(const char* functionName, std::function<dsError_t()> initFunc);
	dsError_t initializeFunctionWithRetry(const char* functionName, std::function<dsError_t()> initFunc);

Conversation

suppal045 commented Dec 11, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Key Changes

Reviewed changes

Uh oh!

Copilot AI Dec 11, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 11, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 11, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 11, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 11, 2025

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

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Feb 23, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 23, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 23, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 23, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 23, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 23, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 23, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 23, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 23, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 23, 2026

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 24, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 24, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 24, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 24, 2026

Choose a reason for hiding this comment

Uh oh!