[v1] treewide: Use proper enum types for controls/properties

24954 diff mbox series

The enumerated controls/properties use `int32_t` as their backing type.
In multiple cases, when parsing such an enum value from a source, an
integer type is used. Replace the integer type with the proper enum
type where it is trivially doable.

This change also brings `CameraSensorLegacy::initProperties()` in line
with `CameraSensorRaw::initProperties()`, by defaulting the color
filter arrangement to `MONO`.

Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
---
 src/ipa/libipa/agc_mean_luminance.cpp         | 2 +-
 src/ipa/rpi/common/ipa_base.cpp               | 4 +++-
 src/libcamera/sensor/camera_sensor_legacy.cpp | 5 +++--
 src/libcamera/sensor/camera_sensor_raw.cpp    | 5 ++---
 4 files changed, 9 insertions(+), 7 deletions(-)

Hi Barnabás,

Thank you for the patch!

Quoting Barnabás Pőcze (2025-11-03 14:49:17)
> The enumerated controls/properties use `int32_t` as their backing type.
> In multiple cases, when parsing such an enum value from a source, an
> integer type is used. Replace the integer type with the proper enum
> type where it is trivially doable.
> 
> This change also brings `CameraSensorLegacy::initProperties()` in line
> with `CameraSensorRaw::initProperties()`, by defaulting the color
> filter arrangement to `MONO`.
> 
> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
> ---
>  src/ipa/libipa/agc_mean_luminance.cpp         | 2 +-
>  src/ipa/rpi/common/ipa_base.cpp               | 4 +++-
>  src/libcamera/sensor/camera_sensor_legacy.cpp | 5 +++--
>  src/libcamera/sensor/camera_sensor_raw.cpp    | 5 ++---
>  4 files changed, 9 insertions(+), 7 deletions(-)
> 
> diff --git a/src/ipa/libipa/agc_mean_luminance.cpp b/src/ipa/libipa/agc_mean_luminance.cpp
> index 64f36bd75d..0c46329c09 100644
> --- a/src/ipa/libipa/agc_mean_luminance.cpp
> +++ b/src/ipa/libipa/agc_mean_luminance.cpp
> @@ -297,7 +297,7 @@ int AgcMeanLuminance::parseExposureModes(const YamlObject &tuningData)
>          * possible before touching gain.
>          */
>         if (availableExposureModes.empty()) {
> -               int32_t exposureModeId = controls::ExposureNormal;
> +               auto exposureModeId = controls::ExposureNormal;

What is the type of 'auto' here? I'm not sure how I feel about using
auto, because I'm not sure its more readable (especially in the case of
the MONO below).

Assuming its still a int32_t, looks good to me!

Reviewed-by: Isaac Scott <isaac.scott@ideasonboard.com>

>                 std::vector<std::pair<utils::Duration, double>> stages = { };
>  
>                 std::shared_ptr<ExposureModeHelper> helper =
> diff --git a/src/ipa/rpi/common/ipa_base.cpp b/src/ipa/rpi/common/ipa_base.cpp
> index 8dfe35cc32..865035e578 100644
> --- a/src/ipa/rpi/common/ipa_base.cpp
> +++ b/src/ipa/rpi/common/ipa_base.cpp
> @@ -1584,7 +1584,9 @@ void IpaBase::reportMetadata(unsigned int ipaContext)
>  
>         const AfStatus *afStatus = rpiMetadata.getLocked<AfStatus>("af.status");
>         if (afStatus) {
> -               int32_t s, p;
> +               controls::AfStateEnum s;
> +               controls::AfPauseStateEnum p;
> +
>                 switch (afStatus->state) {
>                 case AfState::Scanning:
>                         s = controls::AfStateScanning;
> diff --git a/src/libcamera/sensor/camera_sensor_legacy.cpp b/src/libcamera/sensor/camera_sensor_legacy.cpp
> index f9e685a9ac..8b6df10ee6 100644
> --- a/src/libcamera/sensor/camera_sensor_legacy.cpp
> +++ b/src/libcamera/sensor/camera_sensor_legacy.cpp
> @@ -571,7 +571,7 @@ int CameraSensorLegacy::initProperties()
>         const auto &orientation = controls.find(V4L2_CID_CAMERA_ORIENTATION);
>         if (orientation != controls.end()) {
>                 int32_t v4l2Orientation = orientation->second.def().get<int32_t>();
> -               int32_t propertyValue;
> +               properties::LocationEnum propertyValue;
>  
>                 switch (v4l2Orientation) {
>                 default:
> @@ -624,7 +624,8 @@ int CameraSensorLegacy::initProperties()
>  
>         /* Color filter array pattern, register only for RAW sensors. */
>         if (bayerFormat_) {
> -               int32_t cfa;
> +               auto cfa = properties::draft::MONO;
> +
>                 switch (bayerFormat_->order) {
>                 case BayerFormat::BGGR:
>                         cfa = properties::draft::BGGR;
> diff --git a/src/libcamera/sensor/camera_sensor_raw.cpp b/src/libcamera/sensor/camera_sensor_raw.cpp
> index 8ea4423698..cabaa21635 100644
> --- a/src/libcamera/sensor/camera_sensor_raw.cpp
> +++ b/src/libcamera/sensor/camera_sensor_raw.cpp
> @@ -576,7 +576,7 @@ int CameraSensorRaw::initProperties()
>         const auto &orientation = controls.find(V4L2_CID_CAMERA_ORIENTATION);
>         if (orientation != controls.end()) {
>                 int32_t v4l2Orientation = orientation->second.def().get<int32_t>();
> -               int32_t propertyValue;
> +               properties::LocationEnum propertyValue;
>  
>                 switch (v4l2Orientation) {
>                 default:
> @@ -628,7 +628,7 @@ int CameraSensorRaw::initProperties()
>         properties_.set(properties::PixelArrayActiveAreas, { activeArea_ });
>  
>         /* Color filter array pattern. */
> -       uint32_t cfa;
> +       auto cfa = properties::draft::MONO;
>  
>         switch (cfaPattern_) {
>         case BayerFormat::BGGR:
> @@ -644,7 +644,6 @@ int CameraSensorRaw::initProperties()
>                 cfa = properties::draft::RGGB;
>                 break;
>         case BayerFormat::MONO:
> -       default:
>                 cfa = properties::draft::MONO;
>                 break;
>         }
> -- 
> 2.51.2
>

Hi

2025. 11. 04. 16:06 keltezéssel, Isaac Scott írta:
> Hi Barnabás,
> 
> Thank you for the patch!
> 
> Quoting Barnabás Pőcze (2025-11-03 14:49:17)
>> The enumerated controls/properties use `int32_t` as their backing type.
>> In multiple cases, when parsing such an enum value from a source, an
>> integer type is used. Replace the integer type with the proper enum
>> type where it is trivially doable.
>>
>> This change also brings `CameraSensorLegacy::initProperties()` in line
>> with `CameraSensorRaw::initProperties()`, by defaulting the color
>> filter arrangement to `MONO`.
>>
>> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
>> ---
>>   src/ipa/libipa/agc_mean_luminance.cpp         | 2 +-
>>   src/ipa/rpi/common/ipa_base.cpp               | 4 +++-
>>   src/libcamera/sensor/camera_sensor_legacy.cpp | 5 +++--
>>   src/libcamera/sensor/camera_sensor_raw.cpp    | 5 ++---
>>   4 files changed, 9 insertions(+), 7 deletions(-)
>>
>> diff --git a/src/ipa/libipa/agc_mean_luminance.cpp b/src/ipa/libipa/agc_mean_luminance.cpp
>> index 64f36bd75d..0c46329c09 100644
>> --- a/src/ipa/libipa/agc_mean_luminance.cpp
>> +++ b/src/ipa/libipa/agc_mean_luminance.cpp
>> @@ -297,7 +297,7 @@ int AgcMeanLuminance::parseExposureModes(const YamlObject &tuningData)
>>           * possible before touching gain.
>>           */
>>          if (availableExposureModes.empty()) {
>> -               int32_t exposureModeId = controls::ExposureNormal;
>> +               auto exposureModeId = controls::ExposureNormal;
> 
> What is the type of 'auto' here? I'm not sure how I feel about using
> auto, because I'm not sure its more readable (especially in the case of
> the MONO below).

The type would be `controls::AeExposureModeEnum` here and `controls::draft::ColorFilterArrangementEnum`
for "MONO"; but I thought they would be a bit long.


> 
> Assuming its still a int32_t, looks good to me!
> 
> Reviewed-by: Isaac Scott <isaac.scott@ideasonboard.com>
> 
>>                  std::vector<std::pair<utils::Duration, double>> stages = { };
>>   
>>                  std::shared_ptr<ExposureModeHelper> helper =
>> diff --git a/src/ipa/rpi/common/ipa_base.cpp b/src/ipa/rpi/common/ipa_base.cpp
>> index 8dfe35cc32..865035e578 100644
>> --- a/src/ipa/rpi/common/ipa_base.cpp
>> +++ b/src/ipa/rpi/common/ipa_base.cpp
>> @@ -1584,7 +1584,9 @@ void IpaBase::reportMetadata(unsigned int ipaContext)
>>   
>>          const AfStatus *afStatus = rpiMetadata.getLocked<AfStatus>("af.status");
>>          if (afStatus) {
>> -               int32_t s, p;
>> +               controls::AfStateEnum s;
>> +               controls::AfPauseStateEnum p;
>> +
>>                  switch (afStatus->state) {
>>                  case AfState::Scanning:
>>                          s = controls::AfStateScanning;
>> diff --git a/src/libcamera/sensor/camera_sensor_legacy.cpp b/src/libcamera/sensor/camera_sensor_legacy.cpp
>> index f9e685a9ac..8b6df10ee6 100644
>> --- a/src/libcamera/sensor/camera_sensor_legacy.cpp
>> +++ b/src/libcamera/sensor/camera_sensor_legacy.cpp
>> @@ -571,7 +571,7 @@ int CameraSensorLegacy::initProperties()
>>          const auto &orientation = controls.find(V4L2_CID_CAMERA_ORIENTATION);
>>          if (orientation != controls.end()) {
>>                  int32_t v4l2Orientation = orientation->second.def().get<int32_t>();
>> -               int32_t propertyValue;
>> +               properties::LocationEnum propertyValue;
>>   
>>                  switch (v4l2Orientation) {
>>                  default:
>> @@ -624,7 +624,8 @@ int CameraSensorLegacy::initProperties()
>>   
>>          /* Color filter array pattern, register only for RAW sensors. */
>>          if (bayerFormat_) {
>> -               int32_t cfa;
>> +               auto cfa = properties::draft::MONO;
>> +
>>                  switch (bayerFormat_->order) {
>>                  case BayerFormat::BGGR:
>>                          cfa = properties::draft::BGGR;
>> diff --git a/src/libcamera/sensor/camera_sensor_raw.cpp b/src/libcamera/sensor/camera_sensor_raw.cpp
>> index 8ea4423698..cabaa21635 100644
>> --- a/src/libcamera/sensor/camera_sensor_raw.cpp
>> +++ b/src/libcamera/sensor/camera_sensor_raw.cpp
>> @@ -576,7 +576,7 @@ int CameraSensorRaw::initProperties()
>>          const auto &orientation = controls.find(V4L2_CID_CAMERA_ORIENTATION);
>>          if (orientation != controls.end()) {
>>                  int32_t v4l2Orientation = orientation->second.def().get<int32_t>();
>> -               int32_t propertyValue;
>> +               properties::LocationEnum propertyValue;
>>   
>>                  switch (v4l2Orientation) {
>>                  default:
>> @@ -628,7 +628,7 @@ int CameraSensorRaw::initProperties()
>>          properties_.set(properties::PixelArrayActiveAreas, { activeArea_ });
>>   
>>          /* Color filter array pattern. */
>> -       uint32_t cfa;
>> +       auto cfa = properties::draft::MONO;
>>   
>>          switch (cfaPattern_) {
>>          case BayerFormat::BGGR:
>> @@ -644,7 +644,6 @@ int CameraSensorRaw::initProperties()
>>                  cfa = properties::draft::RGGB;
>>                  break;
>>          case BayerFormat::MONO:
>> -       default:
>>                  cfa = properties::draft::MONO;
>>                  break;
>>          }
>> -- 
>> 2.51.2
>>