Skip to content

RDKEMW-16714: refactor hard-coded array-based approach to a generic #239

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
apatel859 merged 13 commits into from
Apr 28, 2026
+280 −137
Merged

RDKEMW-16714: refactor hard-coded array-based approach to a generic #239

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
7c1fa73
RDKEMW-16714: refactor hard-coded array-based approach to a generic
yuvaramachandran-gurusamy Apr 15, 2026
0e49a2c
Apply suggestion from @Copilot
yuvaramachandran-gurusamy Apr 15, 2026
dc7467e
RDKEMW-16714: Address CoPilot review comments
yuvaramachandran-gurusamy Apr 15, 2026
7105957
Fix size_t/int format specifier mismatch in initializeFrameRates log …
Copilot Apr 15, 2026
9d262fe
RDKEMW-16714: Fix Float to string conversion issue
yuvaramachandran-gurusamy Apr 15, 2026
cc045a8
Merge branch 'topic/RDKEMW-16714' of github.com:rdkcentral/devicesett…
yuvaramachandran-gurusamy Apr 15, 2026
127caca
RDKEMW-16714: Update Framerates into member variables
yuvaramachandran-gurusamy Apr 17, 2026
eb61b2f
RDKEMW-16714: Removing Mutex as not needed
yuvaramachandran-gurusamy Apr 17, 2026
55153f0
Merge branch 'develop' of github.com:rdkcentral/devicesettings into t…
yuvaramachandran-gurusamy Apr 17, 2026
4919c34
RDKEMW-16714: Fix build error
yuvaramachandran-gurusamy Apr 17, 2026
3e7638c
RDKEMW-16714: Addressing CoPilot review comments
yuvaramachandran-gurusamy Apr 17, 2026
efeb453
RDKEMW-16714: Addressed Copilot review comments
yuvaramachandran-gurusamy Apr 17, 2026
dca9c29
RDKEMW-16714: Fix Framerate initialization
yuvaramachandran-gurusamy Apr 20, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
356 changes: 242 additions & 114 deletions ds/frameRate.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,136 +33,264 @@
#include "dsTypes.h"
#include "dsUtl.h"
#include "dslogger.h"

#define MAX_VIDEO_FRAMERATE_SUPPORTED 18
#include <cmath>
#include <map>
#include <limits>
#include <cstdio>
#include <unistd.h>
Comment thread
yuvaramachandran-gurusamy marked this conversation as resolved.

namespace {
const float _values[MAX_VIDEO_FRAMERATE_SUPPORTED] = {
0, //unknown
24,
25,
30,
60,
23.98,
29.97,
50,
59.94,
100,
119.88,
120,
200,
239.76,
240,
59,
23,
0 //unknown
};
const char * _names[MAX_VIDEO_FRAMERATE_SUPPORTED] = {
"UnKnown", //unknown
"24",
"25",
"30",
"60",
"23.98",
"29.97",
"50",
"59.94",
"100",
"119.88",
"120",
"200",
"239.76",
"240",
"59",
"23",
"UnKnown" //unknown
};

inline bool isValid(int id) {
if ( MAX_VIDEO_FRAMERATE_SUPPORTED <= id ) {
return false;
}
return dsVideoPortFrameRate_isValid(id);
}
typedef struct _frameRateInfo
{
std::string name;
float value;
}
FrameRateInfo;

std::map<dsVideoFrameRate_t, FrameRateInfo> _frameRates;

inline bool isValid(int id) {
bool valid = false;
valid = dsVideoPortFrameRate_isValid(id);
Comment thread
yuvaramachandran-gurusamy marked this conversation as resolved.
// now check if the frame rate id is supported in the map, as some devices may not support all frame rates defined in dsVideoFrameRate_t enum
if (valid) {
auto it = _frameRates.find(static_cast<dsVideoFrameRate_t>(id));
if (it == _frameRates.end()) {
valid = false;
INT_WARN("Frame rate id: %d is not supported in the map", id);
}
Comment thread
yuvaramachandran-gurusamy marked this conversation as resolved.
}
return valid;
}
Comment thread
yuvaramachandran-gurusamy marked this conversation as resolved.
Comment thread
yuvaramachandran-gurusamy marked this conversation as resolved.
}

namespace device {
// initializing with UNKNOWN, actual value will be set in initializeFrameRates() function
int FrameRate::kUnknown = dsVIDEO_FRAMERATE_UNKNOWN;
int FrameRate::k24 = dsVIDEO_FRAMERATE_UNKNOWN;
int FrameRate::k25 = dsVIDEO_FRAMERATE_UNKNOWN;
int FrameRate::k30 = dsVIDEO_FRAMERATE_UNKNOWN;
int FrameRate::k60 = dsVIDEO_FRAMERATE_UNKNOWN;
int FrameRate::k23dot98 = dsVIDEO_FRAMERATE_UNKNOWN;
int FrameRate::k29dot97 = dsVIDEO_FRAMERATE_UNKNOWN;
int FrameRate::k50 = dsVIDEO_FRAMERATE_UNKNOWN;
int FrameRate::k59dot94 = dsVIDEO_FRAMERATE_UNKNOWN;
int FrameRate::k100 = dsVIDEO_FRAMERATE_UNKNOWN;
int FrameRate::k119dot88 = dsVIDEO_FRAMERATE_UNKNOWN;
int FrameRate::k120 = dsVIDEO_FRAMERATE_UNKNOWN;
int FrameRate::k200 = dsVIDEO_FRAMERATE_UNKNOWN;
int FrameRate::k239dot76 = dsVIDEO_FRAMERATE_UNKNOWN;
int FrameRate::k240 = dsVIDEO_FRAMERATE_UNKNOWN;
int FrameRate::k59 = dsVIDEO_FRAMERATE_UNKNOWN;
int FrameRate::k23 = dsVIDEO_FRAMERATE_UNKNOWN;
int FrameRate::kMax = dsVIDEO_FRAMERATE_MAX;

const int FrameRate::kUnknown = dsVIDEO_FRAMERATE_UNKNOWN;
const int FrameRate::k24 = dsVIDEO_FRAMERATE_24;
const int FrameRate::k25 = dsVIDEO_FRAMERATE_25;
const int FrameRate::k30 = dsVIDEO_FRAMERATE_30;
const int FrameRate::k60 = dsVIDEO_FRAMERATE_60;
const int FrameRate::k23dot98 = dsVIDEO_FRAMERATE_23dot98;
const int FrameRate::k29dot97 = dsVIDEO_FRAMERATE_29dot97;
const int FrameRate::k50 = dsVIDEO_FRAMERATE_50;
const int FrameRate::k59dot94 = dsVIDEO_FRAMERATE_59dot94;
const int FrameRate::kMax = dsVIDEO_FRAMERATE_MAX;

const FrameRate & FrameRate::getInstance(int id)
{
if (::isValid(id)) {
return VideoOutputPortConfig::getInstance().getFrameRate(id);
}
else {
throw IllegalArgumentException();
}
}
void initializeFrameRates() {
_frameRates.clear();
_frameRates.insert({dsVIDEO_FRAMERATE_UNKNOWN, {"Unknown", 0}});

FrameRate::FrameRate(int id)
{
if (::isValid(id)) {
_value = _values[id];
_name = std::string(_names[id]);
_id = id;
}
else {
throw IllegalArgumentException();
}
INT_INFO("Initializing frame rates map with supported frame rates, count: %d", dsVIDEO_FRAMERATE_MAX);

}
for (size_t i = dsVIDEO_FRAMERATE_UNKNOWN + 1; i < dsVIDEO_FRAMERATE_MAX; i++) {
FrameRateInfo framerateInfo;
framerateInfo.name.clear();
framerateInfo.value = 0;

FrameRate::FrameRate(float value) : _value(value){
if (_value == 24.0) {
_id = dsVIDEO_FRAMERATE_24;
}
else if (_value == 25.0) {
_id = dsVIDEO_FRAMERATE_25;
}
else if (_value == 30.0) {
_id = dsVIDEO_FRAMERATE_30;
}
else if (_value == 60.0) {
_id = dsVIDEO_FRAMERATE_60;
}
else if (_value == 23.98) {
_id = dsVIDEO_FRAMERATE_23dot98;
}
else if (_value == 29.97) {
_id = dsVIDEO_FRAMERATE_29dot97;
}
else if (_value == 50.0) {
_id = dsVIDEO_FRAMERATE_50;
}
else if (_value == 59.94) {
_id = dsVIDEO_FRAMERATE_59dot94;
}
else {
throw IllegalArgumentException();
}
_name = std::string(_names[_id]);
}
switch (i) {
case dsVIDEO_FRAMERATE_24: {
framerateInfo.value = 24.0;
framerateInfo.name = "24";
FrameRate::k24 = dsVIDEO_FRAMERATE_24;
}
Comment thread
yuvaramachandran-gurusamy marked this conversation as resolved.
break;
case dsVIDEO_FRAMERATE_25: {
framerateInfo.value = 25.0;
framerateInfo.name = "25";
FrameRate::k25 = dsVIDEO_FRAMERATE_25;
}
break;
case dsVIDEO_FRAMERATE_30: {
framerateInfo.value = 30.0;
framerateInfo.name = "30";
FrameRate::k30 = dsVIDEO_FRAMERATE_30;
}
break;
case dsVIDEO_FRAMERATE_60: {
framerateInfo.value = 60.0;
framerateInfo.name = "60";
FrameRate::k60 = dsVIDEO_FRAMERATE_60;
}
break;
case dsVIDEO_FRAMERATE_23dot98: {
framerateInfo.value = 23.98;
framerateInfo.name = "23.98";
FrameRate::k23dot98 = dsVIDEO_FRAMERATE_23dot98;
}
break;
case dsVIDEO_FRAMERATE_29dot97: {
framerateInfo.value = 29.97;
framerateInfo.name = "29.97";
FrameRate::k29dot97 = dsVIDEO_FRAMERATE_29dot97;
}
break;
case dsVIDEO_FRAMERATE_50: {
framerateInfo.value = 50.0;
framerateInfo.name = "50";
FrameRate::k50 = dsVIDEO_FRAMERATE_50;
}
break;
case dsVIDEO_FRAMERATE_59dot94: {
framerateInfo.value = 59.94;
framerateInfo.name = "59.94";
FrameRate::k59dot94 = dsVIDEO_FRAMERATE_59dot94;
}
break;
case dsVIDEO_FRAMERATE_100: {
framerateInfo.value = 100.0;
framerateInfo.name = "100";
FrameRate::k100 = dsVIDEO_FRAMERATE_100;
}
break;
case dsVIDEO_FRAMERATE_119dot88: {
framerateInfo.value = 119.88;
framerateInfo.name = "119.88";
FrameRate::k119dot88 = dsVIDEO_FRAMERATE_119dot88;
}
break;
case dsVIDEO_FRAMERATE_120: {
framerateInfo.value = 120.0;
framerateInfo.name = "120";
FrameRate::k120 = dsVIDEO_FRAMERATE_120;
}
break;
case dsVIDEO_FRAMERATE_200: {
framerateInfo.value = 200.0;
framerateInfo.name = "200";
FrameRate::k200 = dsVIDEO_FRAMERATE_200;
}
break;
case dsVIDEO_FRAMERATE_239dot76: {
framerateInfo.value = 239.76;
framerateInfo.name = "239.76";
FrameRate::k239dot76 = dsVIDEO_FRAMERATE_239dot76;
}
break;
case dsVIDEO_FRAMERATE_240: {
framerateInfo.value = 240.0;
framerateInfo.name = "240";
FrameRate::k240 = dsVIDEO_FRAMERATE_240;
}
break;
#if 0 // dsVIDEO_FRAMERATE_59 and dsVIDEO_FRAMERATE_23 are not supported in all devices, this will be enabled once all devices support them
case dsVIDEO_FRAMERATE_59: {
framerateInfo.value = 59.0;
framerateInfo.name = "59";
FrameRate::k59 = dsVIDEO_FRAMERATE_59;
}
break;
case dsVIDEO_FRAMERATE_23: {
framerateInfo.value = 23.0;
framerateInfo.name = "23";
FrameRate::k23 = dsVIDEO_FRAMERATE_23;
}
break;
#endif
default: {
framerateInfo.name = "Unknown";
framerateInfo.value = 0;
INT_WARN("Invalid frame rate id: %d", static_cast<int>(i));
}
break;
}

INT_INFO("Frame rate id: %d, name: %s, value: %f", static_cast<int>(i), framerateInfo.name.c_str(), framerateInfo.value);
// inserting the frame rate info in the map with frame rate id as key and FrameRateInfo struct as value
_frameRates.insert({static_cast<dsVideoFrameRate_t>(i), framerateInfo});
}
Comment thread
yuvaramachandran-gurusamy marked this conversation as resolved.
Comment thread
yuvaramachandran-gurusamy marked this conversation as resolved.
}

FrameRate::~FrameRate() {
// TODO Auto-generated destructor stub
}
void deinitializeFrameRates() {
FrameRate::k24 = dsVIDEO_FRAMERATE_UNKNOWN;
FrameRate::k25 = dsVIDEO_FRAMERATE_UNKNOWN;
FrameRate::k30 = dsVIDEO_FRAMERATE_UNKNOWN;
FrameRate::k60 = dsVIDEO_FRAMERATE_UNKNOWN;
FrameRate::k23dot98 = dsVIDEO_FRAMERATE_UNKNOWN;
FrameRate::k29dot97 = dsVIDEO_FRAMERATE_UNKNOWN;
FrameRate::k50 = dsVIDEO_FRAMERATE_UNKNOWN;
FrameRate::k59dot94 = dsVIDEO_FRAMERATE_UNKNOWN;
FrameRate::k100 = dsVIDEO_FRAMERATE_UNKNOWN;
FrameRate::k119dot88 = dsVIDEO_FRAMERATE_UNKNOWN;
FrameRate::k120 = dsVIDEO_FRAMERATE_UNKNOWN;
FrameRate::k200 = dsVIDEO_FRAMERATE_UNKNOWN;
FrameRate::k239dot76 = dsVIDEO_FRAMERATE_UNKNOWN;
FrameRate::k240 = dsVIDEO_FRAMERATE_UNKNOWN;
FrameRate::k59 = dsVIDEO_FRAMERATE_UNKNOWN;
FrameRate::k23 = dsVIDEO_FRAMERATE_UNKNOWN;
_frameRates.clear();
Comment thread
yuvaramachandran-gurusamy marked this conversation as resolved.
Comment thread
yuvaramachandran-gurusamy marked this conversation as resolved.
}

const FrameRate & FrameRate::getInstance(int id)
{
if (::isValid(id)) {
return VideoOutputPortConfig::getInstance().getFrameRate(id);
}
else {
INT_ERROR("Frame rate id: %d is not valid", id);
throw IllegalArgumentException();
}
Comment thread
yuvaramachandran-gurusamy marked this conversation as resolved.
}

const FrameRate & FrameRate::getInstance(const std::string &name)
{
for (const auto& frameRate : _frameRates) {
if (frameRate.second.name == name) {
return VideoOutputPortConfig::getInstance().getFrameRate(frameRate.first);
}
}
INT_ERROR("Frame rate name: %s is not valid", name.c_str());
throw IllegalArgumentException();
}
Comment thread
yuvaramachandran-gurusamy marked this conversation as resolved.
Comment thread
yuvaramachandran-gurusamy marked this conversation as resolved.
Comment thread
yuvaramachandran-gurusamy marked this conversation as resolved.

}
FrameRate::FrameRate(int id)
{
if (::isValid(id)) {
auto it = _frameRates.find(static_cast<dsVideoFrameRate_t>(id));
if (it != _frameRates.end()) {
_value = it->second.value;
_name = it->second.name;
_id = id;
INT_INFO("Creating FrameRate with id: %d, name: %s, value: %f", _id, _name.c_str(), _value);
}
else {
// this should never happen as we are already validating the id, but adding this check to avoid potential crash in case of map lookup failure
INT_ERROR("Frame rate id: %d is valid but not found in the map", id);
throw IllegalArgumentException();
}
}
else {
INT_ERROR("Frame rate id: %d is not valid", id);
throw IllegalArgumentException();
}

}

FrameRate::FrameRate(float value) : _value(value){
for (const auto& frameRate : _frameRates) {
// check if the value matches with any of the supported frame rates in the map, if found initialize the FrameRate object with corresponding name and id
if (std::fabs(frameRate.second.value - value) < std::numeric_limits<float>::epsilon()) {
_id = frameRate.first;
_name = frameRate.second.name;
Comment on lines +277 to +282
Copy link

Copilot AI Apr 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FrameRate(float) compares float values using std::numeric_limits<float>::epsilon(), which is effectively an exact-equality check and is too strict for values like 23.98/29.97 that may come from calculations or external APIs with small rounding differences. Use a practical tolerance (absolute or relative), or compare against the enum id rather than float equality to avoid spurious IllegalArgumentException throws.

Copilot uses AI. Check for mistakes.
INT_INFO("Creating FrameRate with value: %f, name: %s, id: %d", _value, _name.c_str(), _id);
return;
Comment thread
yuvaramachandran-gurusamy marked this conversation as resolved.
}
}
INT_ERROR("Frame rate value: %f is not valid", value);
throw IllegalArgumentException();
Comment thread
yuvaramachandran-gurusamy marked this conversation as resolved.
}

FrameRate::~FrameRate() {
}
}

/** @} */
/** @} */
Loading
Loading