Feature/rdkemw 8587 dlsym

[suppal045]

No description provided.

[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]

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]

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]

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]

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]

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.

[Copilot]

Pull request overview

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

Comments suppressed due to low confidence (1)

rpc/srv/dsVideoPort.c:1963

• getPixelResolution() returns Resn->pixelResolution even when iCount == 0 or when resolution isn't found; in both cases Resn will point at the first (zero-initialized) element, causing an incorrect pixelResolution to be returned. Add an explicit fallback (e.g., default resolution) when iCount==0 or no match is found.

	dsVideoPortResolution_t kVidoeResolutionsSettings[16] = {};
	int iCount = 0;
	dsGetVideoPortResolutions(&iCount, kVidoeResolutionsSettings);
	
	// Print kVidoeResolutionsSettings array
	INT_INFO("[DsMgr] ========== Dumping kVidoeResolutionsSettings Array in getPixelResolution ==========\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");
	
  	dsVideoPortResolution_t *Resn = &kVidoeResolutionsSettings[0]; 
	
	for (int i = 0; i < iCount; i++)
	{
		Resn = &kVidoeResolutionsSettings[i];
		if (resolution.compare(Resn->name) == 0 )
		{
			break;
		}
	}
	return Resn->pixelResolution;
}

---

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

[Copilot]

The unconditional INT_INFO dumps for every port/resolution will be very noisy in production and can impact performance. Consider gating these dumps behind INT_DEBUG, a runtime flag, or the existing /opt/dsMgrDumpDeviceConfigs switch used elsewhere.

[Copilot]

kVideoPortPorts is a fixed-size array of 16, but getVideoPortVPorts(&numPorts, ...) can return more than 16 ports. Both the dump loop and the lookup loop will read past the stack buffer. Clamp numPorts to the array capacity (or switch to a sized allocation).

[Copilot]

#include <fstream> appears unused in this file (no std::ifstream/ofstream usage). If it's not needed, remove it to avoid extra compile time and warnings.

Suggested change

#include <fstream>

[Copilot]

In the port dump, port->defaultResolution is logged with %s without a NULL check. If any port has a NULL defaultResolution this will crash. Use a ternary fallback (as done elsewhere) or skip printing when NULL.

Suggested change

	port->defaultResolution);
	port->defaultResolution ? port->defaultResolution : "NULL");

[Copilot]

using namespace std; in a public header (manager.hpp) pollutes the global namespace for every translation unit that includes it and can cause name collisions. Remove it and qualify symbols with std:: instead (or keep using directives inside .cpp files only).

Suggested change

using namespace std;

[Copilot]

<dlfcn.h> is included here but there are no dlopen/dlsym usages in this file. If it's not needed, remove it to avoid unnecessary dependencies and warnings.

Suggested change

#include <dlfcn.h>

[Copilot]

kVidoeResolutionsSettings is a fixed-size array of 16, but dsGetVideoPortResolutions(&iCount, ...) can return more than 16. The dump loop and search loop iterate to iCount, which will read past the end of the buffer. Clamp iCount to the array capacity or switch to a sized allocation/two-pass query.

[Copilot]

Typo in comment: "funcation" -> "function".

Suggested change

	//This funcation does not have any caller.
	//This function does not have any caller.

[Copilot]

getVideoPortVPorts() fills pPorts[i] for _vPorts.size() entries with no way for callers to specify their buffer capacity; RPC callers currently allocate 16 entries. This can overflow the caller buffer. Add a capacity parameter (or two-pass sizing) and clamp writes to the provided capacity.

[Copilot]

kVideoPortPorts is a fixed-size array of 16, but getVideoPortVPorts(&numPorts, ...) can return more than 16 ports, after which the log loop and the lookup loop will read past the end of the stack buffer. Clamp numPorts to the array capacity (or call once with NULL to get the required size and allocate accordingly).

[Copilot]

Pull request overview

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

---

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

[Copilot]

kVidoeResolutionsSettings[0] is dereferenced even if dsGetVideoPortResolutions returns iCount <= 0 or a NULL pointer. If iCount is 0/-1, this is an out-of-bounds/null dereference. Add a guard (e.g., if (!kVidoeResolutionsSettings || iCount <= 0) return <fallback>;) before accessing index 0 and before iterating/logging.

[Copilot]

INT_INFO(... defaultResolution=%s ...) passes port->defaultResolution directly, but this field can be NULL (other code paths already treat it as optional). Passing NULL to %s is undefined and can crash. Use a ternary (port->defaultResolution ? port->defaultResolution : "NULL").

Suggested change

	k, port->id.type, port->id.index,
	port->connectedAOP.type, port->connectedAOP.index,
	port->defaultResolution);
	k, port->id.type, port->id.index,
	port->connectedAOP.type, port->connectedAOP.index,
	port->defaultResolution ? port->defaultResolution : "NULL");

[Copilot]

This file uses access(..., F_OK) but doesn’t include <unistd.h>. Please include it directly here to avoid implicit-declaration / missing-symbol issues when headers change or compilers are stricter.

[Copilot]

This file calls access(..., F_OK) but does not include <unistd.h> (where access/F_OK are declared). Add the include to make the dependency explicit.

[Copilot]

This function logs the entire port-config array at INFO level every time _GetVideoPortType() runs. This is likely a hot path and will spam logs / add overhead in production. Consider gating this behind INT_DEBUG, the existing /opt/dsMgrDumpDeviceConfigs flag, or a build-time debug switch.

Suggested change

	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");
	INT_DEBUG("[DsMgr] ========== Dumping kVideoPortPorts Array in _GetVideoPortType ==========\r\n");
	INT_DEBUG("[DsMgr] Total ports retrieved: %d\r\n", numPorts);
	for (int k = 0; k < numPorts; k++) {
	const dsVideoPortPortConfig_t *port = &kVideoPortPorts[k];
	INT_DEBUG("[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_DEBUG("[DsMgr] ========== End of kVideoPortPorts Dump ==========\r\n");

[Copilot]

iCount is an int populated by dsGetVideoPortResolutions, but the subsequent loop uses size_t i < iCount. If iCount is negative (e.g., -1 when configs aren’t loaded), it will be converted to a huge size_t and the loop will walk off into memory; additionally kVidoeResolutionsSettings isn’t checked for NULL before dereference. Use an int loop variable and guard if (!kVidoeResolutionsSettings || iCount <= 0) { ... } before iterating.

[Copilot]

This unconditional INFO-level dump of every resolution retrieved from config can be very noisy (and filterEDIDResolution() is invoked on display/EDID flows). Consider moving these to INT_DEBUG and/or gating behind the existing /opt/dsMgrDumpDeviceConfigs flag to avoid log spam in normal operation.

Suggested change

	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");
	INT_DEBUG("[DsMgr] ========== Dumping kVidoeResolutionsSettings Array ==========\r\n");
	INT_DEBUG("[DsMgr] Total resolutions retrieved: %d\r\n", iCount);
	for (int k = 0; k < iCount; k++) {
	dsVideoPortResolution_t *res = &kVidoeResolutionsSettings[k];
	INT_DEBUG("[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_DEBUG("[DsMgr] ========== End of kVidoeResolutionsSettings Dump ==========\r\n");

[Copilot]

allocateAndCopyVideoPortConfigs() overwrites allocatedVideoPortConfigs on each call without freeing any previous allocation. If configs can be reloaded (e.g., dsLoadConfigs() called more than once), this leaks memory and leaves stale pointers in videoPortConfiguration. Consider freeing any existing allocation (and resetting globals) before allocating new memory, or use realloc with careful error handling.

[Copilot]

dsGetDefaultResolutionIndex is implemented as int dsGetDefaultResolutionIndex(void) (returns a value), but the public header declares void dsGetDefaultResolutionIndex(int* outDefaultIndex) and call sites pass an output pointer. This is undefined behavior and will also leave callers’ defaultIndex uninitialized/incorrect. Make the implementation match the header (set the out param), or change the header + all callers to the return-value form consistently.

[Copilot]

This code calls dsGetDefaultResolutionIndex(&defaultIndex), but the current implementation in dsVideoPortConfig.c is a no-arg function returning int. With the current mismatch, defaultIndex may stay at its initialized value and the selected resolution can be wrong. Align the API so the default index is reliably returned here.

Suggested change

	int defaultIndex = 0;
	dsGetVideoPortResolutions(&numResolutions, &resolutions);
	dsGetDefaultResolutionIndex(&defaultIndex);
	int defaultIndex = dsGetDefaultResolutionIndex();
	dsGetVideoPortResolutions(&numResolutions, &resolutions);

[Copilot]

Pull request overview

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

---

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

[Copilot]

API design inconsistency. The function dsGetVideoDeviceConfigs returns a non-const pointer (dsVideoConfig_t**) at line 56, which allows callers to modify the internal configuration. For consistency with other getter functions and to prevent unintended modifications, consider changing the signature to return const dsVideoConfig_t** instead.

Suggested change

	void dsGetVideoDeviceConfigs(int* outConfigSize, dsVideoConfig_t** outConfigs);
	void dsGetVideoDeviceConfigs(int* outConfigSize, const dsVideoConfig_t** outConfigs);

[Copilot]

Duplicate type definition. The videoDeviceConfig_t struct is defined in both dsConfigs.h (lines 58-62) and dsVideoDeviceConfig.h (lines 37-41) with identical structure. This duplication can lead to maintenance issues. Consider defining it in only one header file (preferably dsVideoDeviceConfig.h) and having dsConfigs.h include that header instead.

[Copilot]

Critical bug: Dereferencing uninitialized pointers. The pointers pKVideoPortConfigs_size, pKVideoPortPorts_size, and pKResolutionsSettings_size in videoPortConfiguration are never allocated or initialized before being dereferenced at these lines. This will cause crashes or undefined behavior. These pointers need to be allocated before use.

[Copilot]

Critical bug: Dereferencing uninitialized pointers. The pointers pKConfigSize and pKPortSize in audioConfiguration are never allocated or initialized before being dereferenced at lines 179-180 and later at lines 182-185, 194, 204. This will cause crashes or undefined behavior. These pointers need to be allocated before use.

Suggested change

	// Store sizes for getter functions
	// Store sizes for getter functions
	if (audioConfiguration.pKConfigSize == NULL) {
	audioConfiguration.pKConfigSize = (int *)malloc(sizeof(int));
	if (audioConfiguration.pKConfigSize == NULL) {
	INT_ERROR("Failed to allocate memory for audio configuration size\n");
	return;
	}
	}
	if (audioConfiguration.pKPortSize == NULL) {
	audioConfiguration.pKPortSize = (int *)malloc(sizeof(int));
	if (audioConfiguration.pKPortSize == NULL) {
	INT_ERROR("Failed to allocate memory for audio port size\n");
	return;
	}
	}

[Copilot]

Spelling error in comment. "VIdeo" should be "Video" (capital I should be lowercase).

Suggested change

	cout << "VIdeo Outport Exception Thrown. ..."<<endl;
	cout << "Video Outport Exception Thrown. ..."<<endl;

[Copilot]

Duplicate type definition. The videoPortConfigs_t struct is defined in both dsConfigs.h (lines 47-56) and dsVideoPortConfig.h (lines 37-46) with identical structure. This duplication can lead to maintenance issues. Consider defining it in only one header file (preferably dsVideoPortConfig.h) and having dsConfigs.h include that header instead.

[Copilot]

Inconsistent NULL checking syntax. This file uses both NULL (C-style, line 48) and nullptr (C++-style, line 50). This is a C file, so use NULL consistently throughout.

Suggested change

	if (nullptr == config) {
	if (NULL == config) {

[Copilot]

Potential memory leak. If the second allocation for audio ports (line 155) fails after the first allocation for audio configs (line 147) succeeded, or vice versa in the static configuration path, the previously allocated memory is not freed. Add proper cleanup on failure.

Suggested change

	if(ret == -1)
	{
	INT_ERROR("failed to create dynamic memory\n");
	}
	// Reading Audio port configs
	portSize = (dynamicAudioConfigs->pKPortSize) ? *(dynamicAudioConfigs->pKPortSize) : -1;
	ret = allocateAndCopyAudioPorts(dynamicAudioConfigs->pKAudioPorts, portSize, true);
	if(ret == -1)
	{
	INT_ERROR("failed to create dynamic memory\n");
	}
	}
	else {
	// Using static configuration
	configSize = dsUTL_DIM(kConfigs);
	ret = allocateAndCopyAudioConfigs(kConfigs, configSize, false);
	if(ret == -1)
	{
	INT_ERROR("failed to create dynamic memory\n");
	}
	portSize = dsUTL_DIM(kPorts);
	ret = allocateAndCopyAudioPorts(kPorts, portSize, false);
	if(ret == -1)
	{
	INT_ERROR("failed to create dynamic memory\n");
	if (ret == -1)
	{
	INT_ERROR("failed to create dynamic memory for audio type configs\n");
	// Nothing to clean up yet in this path
	return;
	}
	// Reading Audio port configs
	portSize = (dynamicAudioConfigs->pKPortSize) ? *(dynamicAudioConfigs->pKPortSize) : -1;
	ret = allocateAndCopyAudioPorts(dynamicAudioConfigs->pKAudioPorts, portSize, true);
	if (ret == -1)
	{
	INT_ERROR("failed to create dynamic memory for audio port configs\n");
	// Clean up previously allocated audio type configs in dynamic path
	if (audioConfiguration.pKAudioConfigs != NULL) {
	free(audioConfiguration.pKAudioConfigs);
	audioConfiguration.pKAudioConfigs = NULL;
	}
	if (audioConfiguration.pKConfigSize != NULL) {
	*(audioConfiguration.pKConfigSize) = 0;
	}
	return;
	}
	}
	else {
	// Using static configuration
	configSize = dsUTL_DIM(kConfigs);
	ret = allocateAndCopyAudioConfigs(kConfigs, configSize, false);
	if (ret == -1)
	{
	INT_ERROR("failed to create dynamic memory for audio type configs\n");
	// No dynamic memory to free in static configuration path
	return;
	}
	portSize = dsUTL_DIM(kPorts);
	ret = allocateAndCopyAudioPorts(kPorts, portSize, false);
	if (ret == -1)
	{
	INT_ERROR("failed to create dynamic memory for audio port configs\n");
	// Leave audio type configs as-is; reset ports to safe state
	audioConfiguration.pKAudioPorts = NULL;
	if (audioConfiguration.pKPortSize != NULL) {
	*(audioConfiguration.pKPortSize) = 0;
	}
	return;

[Copilot]

Potential buffer overflow risk. The code uses static_cast<int>(priority) as an array index into levelMap without bounds checking. If an invalid priority value is passed (e.g., greater than TRACE_LEVEL), this will result in an out-of-bounds array access. Add bounds checking before indexing the array.

Suggested change

	fprintf(stderr, "[DS][%d] %s [%s:%d] %s: %s \n",
	(int)syscall(SYS_gettid),
	levelMap[static_cast<int>(priority)],
	int levelIndex = static_cast<int>(priority);
	const char *levelStr = "UNKNOWN";
	if (levelIndex >= INFO_LEVEL && levelIndex <= TRACE_LEVEL)
	{
	levelStr = levelMap[levelIndex];
	}
	fprintf(stderr, "[DS][%d] %s [%s:%d] %s: %s \n",
	(int)syscall(SYS_gettid),
	levelStr,

[Copilot]

Possible unintended behavior when m_isFPConfigLoaded is true. The function returns early if config is already loaded (line 418-420), but there's no indication whether this is intentional (idempotent loading) or an error condition. If loading twice is an error, the error message should be more explicit. If it's intentional, consider using INT_INFO instead of INT_ERROR, or add a comment explaining the behavior.

Suggested change

	if (( false == m_isFPInitialized) || (true == m_isFPConfigLoaded)) {
	INT_ERROR("'%s'", (!m_isFPInitialized) ? "Front Panel not initialized" : "Front Panel Config already loaded");
	return;
	}
	if (false == m_isFPInitialized) {
	INT_ERROR("'%s'", "Front Panel not initialized");
	return;
	}
	// It is intentional that load() is idempotent: if config is already loaded,
	// treat this as a no-op and log at info level rather than as an error.
	if (true == m_isFPConfigLoaded) {
	INT_INFO("'%s'", "Front Panel Config already loaded, skipping reload");
	return;
	}

[Copilot]

Pull request overview

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

---

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

[Copilot]

Duplicate type definition. The videoDeviceConfig_t structure is defined in both videoDeviceConfig.hpp (lines 38-42) and dsVideoDeviceConfig.h (lines 37-41). This could lead to confusion and type mismatches. Consolidate to a single definition or ensure they're truly different types with different purposes.

Suggested change

	#include <vector>
	typedef struct videoDeviceConfig
	{
	dsVideoConfig_t *pKVideoDeviceConfigs;
	int *pKVideoDeviceConfigs_size;
	}videoDeviceConfig_t;
	#include "dsVideoDeviceConfig.h"
	#include <vector>

[Copilot]

API signature change may break existing code. The dsEnableHDCP function signature changed from taking a port type (dsVideoPortType_t) to taking a handle (intptr_t). This is a breaking API change. Ensure that all callers of this function throughout the codebase have been updated to pass a handle instead of a port type, or that this is the only usage of this API.

[Copilot]

Duplicate type definition and naming inconsistency. The dlSymbolLookup type is defined in both ds/include/manager.hpp (lines 173-176) and rpc/srv/dsConfigs.c (lines 49-52), with slightly different naming (struct vs typedef struct). The function LoadDLSymbols is also duplicated in both files with different signatures (bool vs int return type). Consolidate these to a common header to avoid inconsistencies.

Suggested change

	} dlSymbolLookup_t;
	static int LoadDLSymbols(void* pDLHandle, const dlSymbolLookup_t* symbols, int numberOfSymbols)
	} dsConfigs_dlSymbolLookup_t;
	static int LoadDLSymbols(void* pDLHandle, const dsConfigs_dlSymbolLookup_t* symbols, int numberOfSymbols)

[Copilot]

Inconsistent return type. Function is declared with void return type (line 50) but returns a value here. Should be return statement without a value, or the function should be declared to return int to match the error checking pattern in the caller (dsConfigs.c line 111).

[Copilot]

Inconsistent return type. Function is declared with void return type (line 50) but returns a value here. Should be return statement without a value, or the function should be declared to return int to match the error checking pattern in the caller (dsConfigs.c line 111).

[Copilot]

Missing function declaration. The function dsAudioConfigFree is called in dsMgr.c line 149, but it's not declared in the dsAudioConfig.h header file. Add the function declaration to the header file to ensure proper API visibility.

[Copilot]

Duplicate type definition. The videoPortConfigs_t structure is defined both in videoOutputPortConfig.hpp (lines 39-47) and in dsVideoPortConfig.h (lines 37-46), with slightly different field names (pKConfigs vs pKVideoPortConfigs). This could lead to confusion and type mismatches. Consolidate to a single definition or ensure they're truly different types with different purposes.

[Copilot]

Duplicate type definition. The audioConfigs_t structure is defined in both audioOutputPortConfig.hpp (lines 41-47) and dsAudioConfig.h (lines 37-43). This could lead to confusion and type mismatches. Consolidate to a single definition or ensure they're truly different types with different purposes.

Suggested change

	typedef struct audioConfigs
	{
	const dsAudioTypeConfig_t *pKConfigs;
	const dsAudioPortConfig_t *pKPorts;
	int *pKConfigSize;
	int *pKPortSize;
	}audioConfigs_t;
	typedef struct audioConfigs audioConfigs_t;

[Copilot]

Debug macro definition left in production code. Line 38 defines DEBUG macro but it's not used conditionally in the dumpconfig function that follows. If this is meant for conditional compilation, wrap the dumpconfig function calls with #ifdef DEBUG guards. Otherwise, remove this definition to avoid confusion.

Suggested change

#define DEBUG 1 // Using for dumpconfig

[Copilot]

Inconsistent NULL checking style. This line uses nullptr (C++ keyword) in C code, while other NULL checks in the same function use NULL (lines 55, 66 use NULL elsewhere). In C code, use NULL consistently instead of nullptr.

[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]

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]

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]

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]

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

[Copilot]

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

[Copilot]

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]

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]

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]

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]

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.