RDKEMW-16104: Updated the Framerates 59 and 23 to fix crash issue by yuvaramachandran-gurusamy · Pull Request #234 · rdkcentral/devicesettings

yuvaramachandran-gurusamy · 2026-03-26T11:32:16Z

RDKEMW-16104: Add dsVIDEO_FRAMERATE_59 and dsVIDEO_FRAMERATE_23 enums

Signed-off-by: Yuvaramachandran Gurusamy [yuvaramachandran_gurusamy@comcast.com]

Copilot

Pull request overview

Adds support for additional video frame rate enums (59 and 23) in the Device Settings FrameRate wrapper to address a crash caused by missing enum-to-name/value mappings.

Changes:

Introduces FrameRate::k59 and FrameRate::k23 constants in the public header.
Extends internal _values / _names lookup tables to include 59 and 23 and adds an upper-bound guard in isValid().
Adjusts constant assignments in ds/frameRate.cpp for new frame rate IDs.

Reviewed changes

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

File	Description
ds/include/frameRate.hpp	Exposes new `FrameRate` constants for 59 and 23.
ds/frameRate.cpp	Updates frame rate lookup tables and validity gating; adds new constant mappings.

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

ds/include/frameRate.hpp

ds/frameRate.cpp

Copilot · 2026-03-26T11:38:34Z

ds/frameRate.cpp

+			200,
+			239.79,
+			240,


The 239.x entry here looks incorrect/inconsistent: elsewhere dsVIDEO_FRAMERATE_239dot76 is rendered as "239.76" (see ds/hdmiIn.cpp), but this mapping table uses 239.79. This can cause incorrect reporting/selection for that frame rate; align the value/name with 239.76.

Copilot · 2026-03-26T11:38:34Z

ds/frameRate.cpp

+			"120",
+			"200",
+			"239.79",
+			"240",


This string table uses "239.79" which conflicts with the dsVIDEO_FRAMERATE_239dot76 naming/usage elsewhere (rendered as "239.76"). Keeping these consistent avoids confusing UI/log output and prevents mismatches between string<->id conversions.

ds/frameRate.cpp

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Copilot

Pull request overview

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

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

Copilot · 2026-03-26T11:45:52Z

ds/include/frameRate.hpp

+	static const int k59;        //!< Indicates video frame rate of 59 fps.
+	static const int k23;        //!< Indicates video frame rate of 23 fps.


The docstrings for the newly added frame rates are incorrect: k59 should describe 59 fps (not 59.94), and k23 should describe 23 fps (currently also says 59.94). Please correct these comments so they match the actual enum/value meaning.

Copilot · 2026-03-26T11:45:52Z

ds/frameRate.cpp

+#define MAX_VIDEO_FRAMERATE_SUPPORTED 18
+
 namespace {
-	const float _values[] = {
+	const float _values[MAX_VIDEO_FRAMERATE_SUPPORTED] = {


MAX_VIDEO_FRAMERATE_SUPPORTED is hardcoded to 18 while the rest of the code (e.g., FrameRate::kMax / config loading) uses dsVIDEO_FRAMERATE_MAX. Please avoid duplicated sources of truth by deriving this size from dsVIDEO_FRAMERATE_MAX (or from the array size) and consider adding a compile-time check to ensure the arrays stay in sync with the enum range.

Copilot · 2026-03-26T11:45:52Z

ds/frameRate.cpp

+			200,
+			239.79,
+			240,


The mapping for the 239.xx frame rate appears inconsistent: this table uses 239.79 (value and string), but elsewhere the enum is named dsVIDEO_FRAMERATE_239dot76 and is rendered as "239.76" (see ds/hdmiIn.cpp). Please align _values/_names with the actual enum meaning to avoid incorrect reporting/selection of this frame rate.

Copilot

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

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

Copilot · 2026-03-26T14:11:50Z

ds/frameRate.cpp

+			120,
+			200,
+			239.76,
+			240,
+			59,
+			23,
+			0


PR title/description says this change is about adding 59 and 23 fps enums, but this hunk also changes the 239.xx mapping (239.79 -> 239.76) and adds a new hard limit + sentinel entries in the lookup tables. Please confirm these extra behavior changes are intended and update the PR description accordingly so reviewers/release notes capture them.

Copilot

Pull request overview

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

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

Copilot · 2026-03-26T14:24:15Z

ds/frameRate.cpp

+	else if (_value == 59.94) {
 		_id = dsVIDEO_FRAMERATE_59dot94;
-	}
+	}		


FrameRate(float) still only recognizes up to 59.94 and will throw IllegalArgumentException for newly added framerates like 59.0 and 23.0 (even though FrameRate::k59/k23 are now part of the public API). Add explicit handling for 59 and 23 (and ensure _name resolves correctly for those IDs).

Suggested change

}

}

else if (_value == 59.0) {

_id = dsVIDEO_FRAMERATE_59;

}

else if (_value == 23.0) {

_id = dsVIDEO_FRAMERATE_23;

}

Copilot · 2026-03-26T14:24:15Z

ds/frameRate.cpp

 FrameRate::FrameRate(float value) : _value(value){
 	if (_value == 24.0) {
 		_id = dsVIDEO_FRAMERATE_24;
 	}
-	else if (_value == 24.0) {
+	else if (_value == 25.0) {


There are existing Boost tests for frame-rate constants via VideoOutputPortConfig, but the updated FrameRate(float) mapping logic (including the corrected 25/30/60 branches and the new 59/23 support) is not covered. Add unit tests that construct FrameRate from float values and assert the expected ID/name for supported framerates.

yuvaramachandran-gurusamy added 3 commits March 26, 2026 16:56

RDKEMW-16104: Update frameRate.cpp

72b1cdc

RDKEMW-16104: Update frameRate.hpp

944f98f

RDKEMW-16104: Update frameRate.cpp

58fee4f

yuvaramachandran-gurusamy requested a review from a team as a code owner March 26, 2026 11:32

Copilot AI review requested due to automatic review settings March 26, 2026 11:32

Copilot started reviewing on behalf of yuvaramachandran-gurusamy March 26, 2026 11:32 View session

RDKEMW-16104: Update frameRate.cpp

11ff2b3

Copilot AI reviewed Mar 26, 2026

View reviewed changes

RDKEMW-16104: Update frameRate.cpp

f634dd4

Copilot AI review requested due to automatic review settings March 26, 2026 11:41

Copilot started reviewing on behalf of yuvaramachandran-gurusamy March 26, 2026 11:41 View session

Apply suggestions from code review

91970cb

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Copilot AI reviewed Mar 26, 2026

View reviewed changes

yuvaramachandran-gurusamy added 2 commits March 26, 2026 17:39

RDKEMW-16104: Update frameRate.cpp

e2fbae1

RDKEMW-16104: Update frameRate.cpp

ce867c2

Copilot AI review requested due to automatic review settings March 26, 2026 14:06

Copilot started reviewing on behalf of yuvaramachandran-gurusamy March 26, 2026 14:06 View session

Copilot AI reviewed Mar 26, 2026

View reviewed changes

RDKEMW-16104: Update frameRate.cpp

fdeb379

Copilot AI review requested due to automatic review settings March 26, 2026 14:18

Copilot started reviewing on behalf of yuvaramachandran-gurusamy March 26, 2026 14:19 View session

Copilot AI reviewed Mar 26, 2026

View reviewed changes

apatel859 approved these changes Mar 26, 2026

View reviewed changes

apatel859 merged commit e9b7f48 into support/8.5.3.0 Mar 26, 2026
8 checks passed

github-actions bot locked and limited conversation to collaborators Mar 26, 2026

+,
+.79,
+,

		static const int k59; //!< Indicates video frame rate of 59 fps.
		static const int k23; //!< Indicates video frame rate of 23 fps.

+,
+,
+.76,
+,
+,
+,
+

-	}
+	}
+	else if (_value == 59.0) {
+		_id = dsVIDEO_FRAMERATE_59;
+	}
+	else if (_value == 23.0) {
+		_id = dsVIDEO_FRAMERATE_23;
+	}

Conversation

yuvaramachandran-gurusamy commented Mar 26, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Copilot AI Mar 26, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 26, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Mar 26, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 26, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 26, 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 Mar 26, 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 Mar 26, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 26, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants