| Message ID | 20250609151624.278754-1-barnabas.pocze@ideasonboard.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Barnabás, Thank you for the patch. On Mon, Jun 09, 2025 at 05:16:24PM +0200, Barnabás Pőcze wrote: > This is how uvcvideo and rkisp1 do it. See ee918b370a08b > ("ipa: rkisp1: agc: Initialize enum controls with a list of values") > for the motivation. In summary, having a list of values is used as a sign > that the control is an enum in multiple places (e.g. `cam`, `camshark`). > > Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> > --- > src/ipa/rpi/common/ipa_base.cpp | 16 +++++----------- > 1 file changed, 5 insertions(+), 11 deletions(-) > > diff --git a/src/ipa/rpi/common/ipa_base.cpp b/src/ipa/rpi/common/ipa_base.cpp > index e0f8b7e78..39e2db86e 100644 > --- a/src/ipa/rpi/common/ipa_base.cpp > +++ b/src/ipa/rpi/common/ipa_base.cpp > @@ -57,24 +57,18 @@ constexpr Duration controllerMinFrameDuration = 1.0s / 30.0; > const ControlInfoMap::Map ipaControls{ > /* \todo Move this to the Camera class */ > { &controls::AeEnable, ControlInfo(false, true, true) }, > - { &controls::ExposureTimeMode, > - ControlInfo(static_cast<int32_t>(controls::ExposureTimeModeAuto), > - static_cast<int32_t>(controls::ExposureTimeModeManual), > - static_cast<int32_t>(controls::ExposureTimeModeAuto)) }, > + { &controls::ExposureTimeMode, ControlInfo(controls::ExposureTimeModeValues) }, > { &controls::ExposureTime, > ControlInfo(1, 66666, static_cast<int32_t>(defaultExposureTime.get<std::micro>())) }, > - { &controls::AnalogueGainMode, > - ControlInfo(static_cast<int32_t>(controls::AnalogueGainModeAuto), > - static_cast<int32_t>(controls::AnalogueGainModeManual), > - static_cast<int32_t>(controls::AnalogueGainModeAuto)) }, > + { &controls::AnalogueGainMode, ControlInfo(controls::AnalogueGainModeValues) }, Could we list the values explicitly here, in case new values get added to the controls in the future ? > { &controls::AnalogueGain, ControlInfo(1.0f, 16.0f, 1.0f) }, > { &controls::AeMeteringMode, ControlInfo(controls::AeMeteringModeValues) }, > { &controls::AeConstraintMode, ControlInfo(controls::AeConstraintModeValues) }, > { &controls::AeExposureMode, ControlInfo(controls::AeExposureModeValues) }, > { &controls::ExposureValue, ControlInfo(-8.0f, 8.0f, 0.0f) }, > - { &controls::AeFlickerMode, ControlInfo(static_cast<int>(controls::FlickerOff), > - static_cast<int>(controls::FlickerManual), > - static_cast<int>(controls::FlickerOff)) }, > + { &controls::AeFlickerMode, > + ControlInfo({ { ControlValue(controls::FlickerOff), ControlValue(controls::FlickerManual) } }, > + controls::FlickerOff) }, > { &controls::AeFlickerPeriod, ControlInfo(100, 1000000) }, > { &controls::Brightness, ControlInfo(-1.0f, 1.0f, 0.0f) }, > { &controls::Contrast, ControlInfo(0.0f, 32.0f, 1.0f) },
2025. 06. 11. 16:08 keltezéssel, Laurent Pinchart írta: > Hi Barnabás, > > Thank you for the patch. > > On Mon, Jun 09, 2025 at 05:16:24PM +0200, Barnabás Pőcze wrote: >> This is how uvcvideo and rkisp1 do it. See ee918b370a08b >> ("ipa: rkisp1: agc: Initialize enum controls with a list of values") >> for the motivation. In summary, having a list of values is used as a sign >> that the control is an enum in multiple places (e.g. `cam`, `camshark`). >> >> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> >> --- >> src/ipa/rpi/common/ipa_base.cpp | 16 +++++----------- >> 1 file changed, 5 insertions(+), 11 deletions(-) >> >> diff --git a/src/ipa/rpi/common/ipa_base.cpp b/src/ipa/rpi/common/ipa_base.cpp >> index e0f8b7e78..39e2db86e 100644 >> --- a/src/ipa/rpi/common/ipa_base.cpp >> +++ b/src/ipa/rpi/common/ipa_base.cpp >> @@ -57,24 +57,18 @@ constexpr Duration controllerMinFrameDuration = 1.0s / 30.0; >> const ControlInfoMap::Map ipaControls{ >> /* \todo Move this to the Camera class */ >> { &controls::AeEnable, ControlInfo(false, true, true) }, >> - { &controls::ExposureTimeMode, >> - ControlInfo(static_cast<int32_t>(controls::ExposureTimeModeAuto), >> - static_cast<int32_t>(controls::ExposureTimeModeManual), >> - static_cast<int32_t>(controls::ExposureTimeModeAuto)) }, >> + { &controls::ExposureTimeMode, ControlInfo(controls::ExposureTimeModeValues) }, >> { &controls::ExposureTime, >> ControlInfo(1, 66666, static_cast<int32_t>(defaultExposureTime.get<std::micro>())) }, >> - { &controls::AnalogueGainMode, >> - ControlInfo(static_cast<int32_t>(controls::AnalogueGainModeAuto), >> - static_cast<int32_t>(controls::AnalogueGainModeManual), >> - static_cast<int32_t>(controls::AnalogueGainModeAuto)) }, >> + { &controls::AnalogueGainMode, ControlInfo(controls::AnalogueGainModeValues) }, > > Could we list the values explicitly here, in case new values get added > to the controls in the future ? Most other `ControlInfo`s also specify all values, that's why I did the same here. Do these two deserve special treatment? Or maybe all of the should be modified? Regards, Barnabás Pőcze > >> { &controls::AnalogueGain, ControlInfo(1.0f, 16.0f, 1.0f) }, >> { &controls::AeMeteringMode, ControlInfo(controls::AeMeteringModeValues) }, >> { &controls::AeConstraintMode, ControlInfo(controls::AeConstraintModeValues) }, >> { &controls::AeExposureMode, ControlInfo(controls::AeExposureModeValues) }, >> { &controls::ExposureValue, ControlInfo(-8.0f, 8.0f, 0.0f) }, >> - { &controls::AeFlickerMode, ControlInfo(static_cast<int>(controls::FlickerOff), >> - static_cast<int>(controls::FlickerManual), >> - static_cast<int>(controls::FlickerOff)) }, >> + { &controls::AeFlickerMode, >> + ControlInfo({ { ControlValue(controls::FlickerOff), ControlValue(controls::FlickerManual) } }, >> + controls::FlickerOff) }, >> { &controls::AeFlickerPeriod, ControlInfo(100, 1000000) }, >> { &controls::Brightness, ControlInfo(-1.0f, 1.0f, 0.0f) }, >> { &controls::Contrast, ControlInfo(0.0f, 32.0f, 1.0f) }, >
On Wed, Jun 11, 2025 at 04:11:36PM +0200, Barnabás Pőcze wrote: > 2025. 06. 11. 16:08 keltezéssel, Laurent Pinchart írta: > > On Mon, Jun 09, 2025 at 05:16:24PM +0200, Barnabás Pőcze wrote: > >> This is how uvcvideo and rkisp1 do it. See ee918b370a08b > >> ("ipa: rkisp1: agc: Initialize enum controls with a list of values") > >> for the motivation. In summary, having a list of values is used as a sign > >> that the control is an enum in multiple places (e.g. `cam`, `camshark`). > >> > >> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> > >> --- > >> src/ipa/rpi/common/ipa_base.cpp | 16 +++++----------- > >> 1 file changed, 5 insertions(+), 11 deletions(-) > >> > >> diff --git a/src/ipa/rpi/common/ipa_base.cpp b/src/ipa/rpi/common/ipa_base.cpp > >> index e0f8b7e78..39e2db86e 100644 > >> --- a/src/ipa/rpi/common/ipa_base.cpp > >> +++ b/src/ipa/rpi/common/ipa_base.cpp > >> @@ -57,24 +57,18 @@ constexpr Duration controllerMinFrameDuration = 1.0s / 30.0; > >> const ControlInfoMap::Map ipaControls{ > >> /* \todo Move this to the Camera class */ > >> { &controls::AeEnable, ControlInfo(false, true, true) }, > >> - { &controls::ExposureTimeMode, > >> - ControlInfo(static_cast<int32_t>(controls::ExposureTimeModeAuto), > >> - static_cast<int32_t>(controls::ExposureTimeModeManual), > >> - static_cast<int32_t>(controls::ExposureTimeModeAuto)) }, > >> + { &controls::ExposureTimeMode, ControlInfo(controls::ExposureTimeModeValues) }, > >> { &controls::ExposureTime, > >> ControlInfo(1, 66666, static_cast<int32_t>(defaultExposureTime.get<std::micro>())) }, > >> - { &controls::AnalogueGainMode, > >> - ControlInfo(static_cast<int32_t>(controls::AnalogueGainModeAuto), > >> - static_cast<int32_t>(controls::AnalogueGainModeManual), > >> - static_cast<int32_t>(controls::AnalogueGainModeAuto)) }, > >> + { &controls::AnalogueGainMode, ControlInfo(controls::AnalogueGainModeValues) }, > > > > Could we list the values explicitly here, in case new values get added > > to the controls in the future ? > > Most other `ControlInfo`s also specify all values, that's why I did the same here. > Do these two deserve special treatment? Or maybe all of the should be modified? I'm tempted to modify them all, or at least to use controls::*Values only in cases where we expect there will never be any new values, or where adding a new value would require implementing it in all IPA modules due to the nature of the control. Does that make sense ? > >> { &controls::AnalogueGain, ControlInfo(1.0f, 16.0f, 1.0f) }, > >> { &controls::AeMeteringMode, ControlInfo(controls::AeMeteringModeValues) }, > >> { &controls::AeConstraintMode, ControlInfo(controls::AeConstraintModeValues) }, > >> { &controls::AeExposureMode, ControlInfo(controls::AeExposureModeValues) }, > >> { &controls::ExposureValue, ControlInfo(-8.0f, 8.0f, 0.0f) }, > >> - { &controls::AeFlickerMode, ControlInfo(static_cast<int>(controls::FlickerOff), > >> - static_cast<int>(controls::FlickerManual), > >> - static_cast<int>(controls::FlickerOff)) }, > >> + { &controls::AeFlickerMode, > >> + ControlInfo({ { ControlValue(controls::FlickerOff), ControlValue(controls::FlickerManual) } }, > >> + controls::FlickerOff) }, > >> { &controls::AeFlickerPeriod, ControlInfo(100, 1000000) }, > >> { &controls::Brightness, ControlInfo(-1.0f, 1.0f, 0.0f) }, > >> { &controls::Contrast, ControlInfo(0.0f, 32.0f, 1.0f) },
diff --git a/src/ipa/rpi/common/ipa_base.cpp b/src/ipa/rpi/common/ipa_base.cpp index e0f8b7e78..39e2db86e 100644 --- a/src/ipa/rpi/common/ipa_base.cpp +++ b/src/ipa/rpi/common/ipa_base.cpp @@ -57,24 +57,18 @@ constexpr Duration controllerMinFrameDuration = 1.0s / 30.0; const ControlInfoMap::Map ipaControls{ /* \todo Move this to the Camera class */ { &controls::AeEnable, ControlInfo(false, true, true) }, - { &controls::ExposureTimeMode, - ControlInfo(static_cast<int32_t>(controls::ExposureTimeModeAuto), - static_cast<int32_t>(controls::ExposureTimeModeManual), - static_cast<int32_t>(controls::ExposureTimeModeAuto)) }, + { &controls::ExposureTimeMode, ControlInfo(controls::ExposureTimeModeValues) }, { &controls::ExposureTime, ControlInfo(1, 66666, static_cast<int32_t>(defaultExposureTime.get<std::micro>())) }, - { &controls::AnalogueGainMode, - ControlInfo(static_cast<int32_t>(controls::AnalogueGainModeAuto), - static_cast<int32_t>(controls::AnalogueGainModeManual), - static_cast<int32_t>(controls::AnalogueGainModeAuto)) }, + { &controls::AnalogueGainMode, ControlInfo(controls::AnalogueGainModeValues) }, { &controls::AnalogueGain, ControlInfo(1.0f, 16.0f, 1.0f) }, { &controls::AeMeteringMode, ControlInfo(controls::AeMeteringModeValues) }, { &controls::AeConstraintMode, ControlInfo(controls::AeConstraintModeValues) }, { &controls::AeExposureMode, ControlInfo(controls::AeExposureModeValues) }, { &controls::ExposureValue, ControlInfo(-8.0f, 8.0f, 0.0f) }, - { &controls::AeFlickerMode, ControlInfo(static_cast<int>(controls::FlickerOff), - static_cast<int>(controls::FlickerManual), - static_cast<int>(controls::FlickerOff)) }, + { &controls::AeFlickerMode, + ControlInfo({ { ControlValue(controls::FlickerOff), ControlValue(controls::FlickerManual) } }, + controls::FlickerOff) }, { &controls::AeFlickerPeriod, ControlInfo(100, 1000000) }, { &controls::Brightness, ControlInfo(-1.0f, 1.0f, 0.0f) }, { &controls::Contrast, ControlInfo(0.0f, 32.0f, 1.0f) },
This is how uvcvideo and rkisp1 do it. See ee918b370a08b ("ipa: rkisp1: agc: Initialize enum controls with a list of values") for the motivation. In summary, having a list of values is used as a sign that the control is an enum in multiple places (e.g. `cam`, `camshark`). Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> --- src/ipa/rpi/common/ipa_base.cpp | 16 +++++----------- 1 file changed, 5 insertions(+), 11 deletions(-)