Revert "Revert "Revert "RDKEMW-10536 : Revert RDKEMW-9633 and RDKEMW-… by utkarshece14 · Pull Request #338 · rdkcentral/entservices-inputoutput

utkarshece14 · 2025-12-11T13:04:24Z

…9955"" (…"

This reverts commit e469f8b.

…9955"" (…" This reverts commit e469f8b.

Copilot

Pull request overview

This PR reverts a previous revert, effectively re-introducing advanced PQ (Picture Quality) parameter support that was part of RDKEMW-9633 and RDKEMW-9955. The changes add a new V2 API for handling picture quality parameters with enhanced context-aware capabilities, async processing support, and improved parameter management.

Key Changes

Introduces V2 methods for advanced PQ parameter handling with context-based capabilities
Adds async worker thread mechanism for processing multiple parameter updates
Extends API version to 1.1.0 with new getter/setter/reset methods for advanced PQ features (AI Super Resolution, MEMC, SDR Gamma, etc.)

Reviewed changes

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

File	Description
AVOutput/CHANGELOG.md	Adds version 1.2.1 and 1.2.0 changelog entries for PQ parameter fixes and additions
AVOutput/AVOutputTVHelper.cpp	Implements V2 parameter handling functions, async worker thread, JSON-based context management, and extended PQ parameter support
AVOutput/AVOutputTV.h	Declares new V2 API methods, adds member variables for capability tracking, and defines thread pool infrastructure
AVOutput/AVOutput.cpp	Updates API version number to 1.1.0 with new metadata structure

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

Copilot · 2025-12-11T13:08:48Z

 * Changes in CHANGELOG should be updated when commits are added to the main or release branches. There should be one CHANGELOG entry per JIRA Ticket. This is not enforced on sprint branches since there could be multiple changes for the same JIRA ticket during development. 
+## [1.2.1] - 2025-11-01
+### Fixed
+- Fixed PQMOde index issue for Advance PQParams


Spelling error: "PQMOde" should be "PQMode" (capitalization inconsistency). The 'O' should be lowercase.

Suggested change

- Fixed PQMOde index issue for Advance PQParams

- Fixed PQMode index issue for Advance PQParams

Copilot · 2025-12-11T13:08:49Z

            case tvColorTemp_COLD: return "Cold";
 	    case tvColorTemp_USER : return "UserDefined";
-            default : return "Max";
+            default : return "Standard";


The default return value for an unknown ColorTemp has been changed from "Max" to "Standard". While this may be intentional, ensure this change is consistent with the expected behavior and won't break existing functionality that might depend on "Max" as a fallback value.

Suggested change

default : return "Standard";

default : return "Max";

Copilot · 2025-12-11T13:08:49Z

+            paramUpdateQueue.push([this, action, tr181ParamName, parameters, pqParamIndex, level]() {
+                updateAVoutputTVParamV2Implementation(action, tr181ParamName, parameters, pqParamIndex, level);


Potential thread safety issue: The parameters JsonObject is captured by value in the lambda (line 3370), which is good. However, if the JsonObject contains references to external data or if the underlying implementation doesn't properly deep-copy, there could be race conditions. Ensure that JsonObject's copy constructor properly deep-copies all data.

Suggested change

paramUpdateQueue.push([this, action, tr181ParamName, parameters, pqParamIndex, level]() {

updateAVoutputTVParamV2Implementation(action, tr181ParamName, parameters, pqParamIndex, level);

JsonObject parametersCopy = parameters; // Ensure this is a deep copy

paramUpdateQueue.push([this, action, tr181ParamName, parametersCopy, pqParamIndex, level]() {

updateAVoutputTVParamV2Implementation(action, tr181ParamName, parametersCopy, pqParamIndex, level);

Copilot · 2025-12-11T13:08:49Z

+		std::mutex queueMutex;
+		std::condition_variable queueCondition;
+		std::thread workerThread;
+		std::atomic<bool> shouldStopWorker{false};


Inconsistent spacing in member variable initialization. Line 521 uses {false} with braces, while other atomic members in the codebase might use = false. Consider using consistent initialization style throughout the class.

Suggested change

std::atomic<bool> shouldStopWorker{false};

std::atomic<bool> shouldStopWorker = false;

Copilot · 2025-12-11T13:08:50Z

+		// Thread pool for non-blocking parameter updates
+		std::queue<std::function<void()>> paramUpdateQueue;
+		std::mutex queueMutex;
+		std::condition_variable queueCondition;
+		std::thread workerThread;


Missing documentation for the thread pool and worker thread mechanism. The public member variables (lines 517-521) for the worker thread, queue, mutex, and condition variable should have comments explaining their purpose, thread safety guarantees, and lifecycle management.

Suggested change

// Thread pool for non-blocking parameter updates

std::queue<std::function<void()>> paramUpdateQueue;

std::mutex queueMutex;

std::condition_variable queueCondition;

std::thread workerThread;

// Thread pool for non-blocking parameter updates

/**

* @brief Queue of parameter update tasks to be processed by the worker thread.

* Thread Safety: Access to this queue is protected by queueMutex.

* Lifecycle: The queue exists for the lifetime of the AVOutputTV instance.

*/

std::queue<std::function<void()>> paramUpdateQueue;

/**

* @brief Mutex to protect access to paramUpdateQueue.

* Thread Safety: Must be locked whenever paramUpdateQueue is accessed or modified.

* Lifecycle: Exists for the lifetime of the AVOutputTV instance.

*/

std::mutex queueMutex;

/**

* @brief Condition variable to notify the worker thread of new tasks or shutdown.

* Thread Safety: Used in conjunction with queueMutex.

* Lifecycle: Exists for the lifetime of the AVOutputTV instance.

*/

std::condition_variable queueCondition;

/**

* @brief Worker thread that processes parameter update tasks from the queue.

* Lifecycle: Started during initialization and joined during destruction.

*/

std::thread workerThread;

/**

* @brief Flag to signal the worker thread to stop processing and exit.

* Thread Safety: Atomic to allow safe concurrent access from multiple threads.

* Lifecycle: Set to true during shutdown to terminate the worker thread.

*/

Copilot · 2025-12-11T13:08:50Z

+		tvContextCaps_t* m_backlightCaps = nullptr;
+		tvError_t m_backlightStatus = tvERROR_NONE;
+
+		int m_maxBrightness = 0;
+		tvContextCaps_t* m_brightnessCaps = nullptr;
+		tvError_t m_brightnessStatus = tvERROR_NONE;
+
+		int m_maxContrast = 0;
+		tvContextCaps_t* m_contrastCaps = nullptr;
+		tvError_t m_contrastStatus = tvERROR_NONE;
+
+		int m_maxSharpness = 0;
+		tvContextCaps_t* m_sharpnessCaps = nullptr;
+		tvError_t m_sharpnessStatus = tvERROR_NONE;
+
+		int m_maxSaturation = 0;
+		tvContextCaps_t* m_saturationCaps = nullptr;
+		tvError_t m_saturationStatus = tvERROR_NONE;
+
+		int m_maxHue = 0;
+		tvContextCaps_t* m_hueCaps = nullptr;
+		tvError_t m_hueStatus = tvERROR_NONE;
+
+		int m_maxlowLatencyState = 0;
+		tvContextCaps_t* m_lowLatencyStateCaps = nullptr;
+		tvError_t m_lowLatencyStateStatus = tvERROR_NONE;
+
+		int m_maxPrecisionDetail = 0;
+		tvContextCaps_t* m_precisionDetailCaps = nullptr;
+		tvError_t m_precisionDetailStatus = tvERROR_NONE;
+
+		int m_maxLocalContrastEnhancement = 0;
+		tvContextCaps_t* m_localContrastEnhancementCaps = nullptr;
+		tvError_t m_localContrastEnhancementStatus = tvERROR_NONE;
+
+		int m_maxMPEGNoiseReduction = 0;
+		tvContextCaps_t* m_MPEGNoiseReductionCaps = nullptr;
+		tvError_t m_MPEGNoiseReductionStatus = tvERROR_NONE;
+
+		int m_maxDigitalNoiseReduction = 0;
+		tvContextCaps_t* m_digitalNoiseReductionCaps = nullptr;
+		tvError_t m_digitalNoiseReductionStatus = tvERROR_NONE;
+
+		int m_maxAISuperResolution = 0;
+		tvContextCaps_t* m_AISuperResolutionCaps = nullptr;
+		tvError_t m_AISuperResolutionStatus = tvERROR_NONE;
+
+		int m_maxMEMC = 0;
+		tvContextCaps_t* m_MEMCCaps = nullptr;
+		tvError_t m_MEMCStatus = tvERROR_NONE;
+
+		tvColorTemp_t* m_colortemp = nullptr;
+        size_t m_numColortemp = 0;
+        tvContextCaps_t* m_colortempCaps = nullptr;
+		tvError_t m_colorTempStatus = tvERROR_NONE;
+
+		tvDisplayMode_t* m_aspectRatio = nullptr;
+		size_t m_numAspectRatio = 0;
+		tvContextCaps_t* m_aspectRatioCaps = nullptr;
+		tvError_t m_aspectRatioStatus = tvERROR_NONE;
+
+		tvDimmingMode_t* m_dimmingModes = nullptr;
+        size_t m_numdimmingModes = 0;
+        tvContextCaps_t* m_dimmingModeCaps = nullptr;
+		tvError_t m_dimmingModeStatus = tvERROR_NONE;
+
+        tvPQModeIndex_t* m_pictureModes = nullptr;
+        size_t m_numPictureModes = 0;
+        tvContextCaps_t* m_pictureModeCaps = nullptr;
+		tvError_t m_pictureModeStatus = tvERROR_NONE;
+
+        tvBacklightMode_t* m_backlightModes = nullptr;
+        size_t m_numBacklightModes = 0;
+        tvContextCaps_t* m_backlightModeCaps = nullptr;
+		tvError_t m_backlightModeStatus = tvERROR_NONE;
+
+		tvSdrGamma_t* m_sdrGammaModes = nullptr;
+		size_t m_numsdrGammaModes = 0;
+		tvContextCaps_t* m_sdrGammaModeCaps = nullptr;
+		tvError_t m_sdrGammaModeStatus = tvERROR_NONE;
+		void getSdrGammaStringFromEnum(tvSdrGamma_t value, std::string& str);
+
+		int m_numHalMatrixPoints = 0;
+		int m_rgbMin = 0;
+		int m_rgbMax = 0;
+		int m_numUiMatrixPoints = 0;
+		double* m_uiMatrixPositions = nullptr;
+		tvContextCaps_t* m_multiPointWBCaps = nullptr;
+		tvError_t m_multiPointWBStatus = tvERROR_NONE;
+
+		tvDVCalibrationSettings_t* m_minValues;
+		tvDVCalibrationSettings_t* m_maxValues;
+		tvContextCaps_t* m_DVCalibrationCaps = nullptr;
+		tvError_t m_DVCalibrationStatus = tvERROR_NONE;
+
+		int m_maxCmsHue = 0;
+		int m_maxCmsSaturation = 0;
+		int m_maxCmsLuma = 0;
+		size_t m_numColor = 0;
+		size_t m_numComponent = 0;
+		tvDataComponentColor_t* m_cmsColorArr;
+		tvComponentType_t* m_cmsComponentArr;
+		std::vector<std::string> m_cmsColorList;
+		std::vector<std::string> m_cmsComponentList;
+		std::unordered_map<std::string, int> m_cmsIndexMap;
+		tvContextCaps_t* m_cmsCaps = nullptr;
+		tvError_t m_cmsStatus = tvERROR_NONE;


Memory management concern: Multiple capability structures are stored as raw pointers (e.g., m_backlightCaps, m_brightnessCaps, etc.) without clear ownership semantics. Ensure these pointers are properly managed - either document that they're owned by the HAL and don't need deallocation, or implement proper cleanup in the destructor.

Copilot · 2025-12-11T13:08:50Z

+
+        syncWBParams();
+
+        //Ambient Bakclight Mode


Spelling error: "Bakclight" should be "Backlight". There's a typo with the letters 'c' and 'k' swapped.

Suggested change

//Ambient Bakclight Mode

//Ambient Backlight Mode

Copilot · 2025-12-11T13:08:51Z

+        } else {
+
+            // Capture parameters by value for thread safety
+            std::lock_guard<std::mutex> lock(queueMutex);
+            paramUpdateQueue.push([this, action, tr181ParamName, parameters, pqParamIndex, level]() {
+                updateAVoutputTVParamV2Implementation(action, tr181ParamName, parameters, pqParamIndex, level);
+            });
+            queueCondition.notify_one();
+
+            // Return immediately - operation queued successfully
+            return 0;
+        }


The updateAVoutputTVParamV2 function queues tasks for asynchronous execution when there are multiple contexts (validContexts.size() > 1), but returns 0 immediately. This means the caller cannot determine if the actual operation succeeded or failed. Consider returning a status that indicates the operation was queued, or providing a callback mechanism for async results.

Copilot · 2025-12-11T13:08:51Z

+                }
+            }
+            LOGINFO("Exit: %s, Return Value: %d", __FUNCTION__, ret);
+            return (ret < 0) ? -1 : 0;


The CMS parameter update uses bitwise OR to accumulate error codes (lines 3447, 3453, 3459, 3464, 3469), but then checks if ret < 0 at line 3509. Since the accumulated errors are positive bit flags (1, 2, 4, 8, 16), the condition (ret < 0) will never be true. This means the function will always return 0 even when errors occurred. The return logic should be: return (ret != 0) ? -1 : 0;

Suggested change

return (ret < 0) ? -1 : 0;

return (ret != 0) ? -1 : 0;

Copilot · 2025-12-11T13:08:51Z

+    void AVOutputTV::paramUpdateWorker() {
+        while (!shouldStopWorker) {
+            std::unique_lock<std::mutex> lock(queueMutex);
+
+            // Wait for work or stop signal
+            queueCondition.wait(lock, [this] {
+                return !paramUpdateQueue.empty() || shouldStopWorker;
+            });
+
+            if (shouldStopWorker) {
+                break;
+            }
+
+            // Process all queued updates
+            while (!paramUpdateQueue.empty() && !shouldStopWorker) {
+                auto task = paramUpdateQueue.front();
+                paramUpdateQueue.pop();
+                lock.unlock();
+                // Execute the task
+                task();
+                lock.lock();
+            }
+        }
+    }


Potential resource leak concern: The worker thread is started somewhere (not visible in this diff), but there's no explicit cleanup mechanism shown. Ensure that the destructor properly signals shouldStopWorker, notifies the condition variable, and joins the worker thread to prevent resource leaks and ensure clean shutdown.

Revert "Revert "Revert "RDKEMW-10536 : Revert RDKEMW-9633 and RDKEMW-…

9adf337

…9955"" (…" This reverts commit e469f8b.

utkarshece14 requested a review from a team as a code owner December 11, 2025 13:04

Copilot AI review requested due to automatic review settings December 11, 2025 13:04

Copilot started reviewing on behalf of utkarshece14 December 11, 2025 13:04 View session

Copilot AI reviewed Dec 11, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Revert "Revert "Revert "RDKEMW-10536 : Revert RDKEMW-9633 and RDKEMW-…#338

Revert "Revert "Revert "RDKEMW-10536 : Revert RDKEMW-9633 and RDKEMW-…#338
utkarshece14 wants to merge 1 commit into
support/AVOutput_syncfrom
revert-336-revert-311-revert-300-feature/RDKEMW-10536

utkarshece14 commented Dec 11, 2025

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Dec 11, 2025

Uh oh!

Copilot AI Dec 11, 2025

Uh oh!

Copilot AI Dec 11, 2025

Uh oh!

Copilot AI Dec 11, 2025

Uh oh!

Copilot AI Dec 11, 2025

Uh oh!

Copilot AI Dec 11, 2025

Uh oh!

Copilot AI Dec 11, 2025

Uh oh!

Copilot AI Dec 11, 2025

Uh oh!

Copilot AI Dec 11, 2025

Uh oh!

Copilot AI Dec 11, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

	- Fixed PQMOde index issue for Advance PQParams
	- Fixed PQMode index issue for Advance PQParams

		paramUpdateQueue.push([this, action, tr181ParamName, parameters, pqParamIndex, level]() {
		updateAVoutputTVParamV2Implementation(action, tr181ParamName, parameters, pqParamIndex, level);

-            paramUpdateQueue.push([this, action, tr181ParamName, parameters, pqParamIndex, level]() {
-                updateAVoutputTVParamV2Implementation(action, tr181ParamName, parameters, pqParamIndex, level);
+            JsonObject parametersCopy = parameters; // Ensure this is a deep copy
+            paramUpdateQueue.push([this, action, tr181ParamName, parametersCopy, pqParamIndex, level]() {
+                updateAVoutputTVParamV2Implementation(action, tr181ParamName, parametersCopy, pqParamIndex, level);

	std::atomic<bool> shouldStopWorker{false};
	std::atomic<bool> shouldStopWorker = false;

-		// Thread pool for non-blocking parameter updates
-		std::queue<std::function<void()>> paramUpdateQueue;
-		std::mutex queueMutex;
-		std::condition_variable queueCondition;
-		std::thread workerThread;
+		// Thread pool for non-blocking parameter updates
+		/**
+		 * @brief Queue of parameter update tasks to be processed by the worker thread.
+		 * Thread Safety: Access to this queue is protected by queueMutex.
+		 * Lifecycle: The queue exists for the lifetime of the AVOutputTV instance.
+		 */
+		std::queue<std::function<void()>> paramUpdateQueue;
+		/**
+		 * @brief Mutex to protect access to paramUpdateQueue.
+		 * Thread Safety: Must be locked whenever paramUpdateQueue is accessed or modified.
+		 * Lifecycle: Exists for the lifetime of the AVOutputTV instance.
+		 */
+		std::mutex queueMutex;
+		/**
+		 * @brief Condition variable to notify the worker thread of new tasks or shutdown.
+		 * Thread Safety: Used in conjunction with queueMutex.
+		 * Lifecycle: Exists for the lifetime of the AVOutputTV instance.
+		 */
+		std::condition_variable queueCondition;
+		/**
+		 * @brief Worker thread that processes parameter update tasks from the queue.
+		 * Lifecycle: Started during initialization and joined during destruction.
+		 */
+		std::thread workerThread;
+		/**
+		 * @brief Flag to signal the worker thread to stop processing and exit.
+		 * Thread Safety: Atomic to allow safe concurrent access from multiple threads.
+		 * Lifecycle: Set to true during shutdown to terminate the worker thread.
+		 */

Uh oh!

Conversation

utkarshece14 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 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants