[libcamera-devel,2/2] libcamera: control: explicitly define default control values
diff mbox series

Message ID 20220821124343.487963-2-Rauch.Christian@gmx.de
State Superseded
Headers show
Series
  • [libcamera-devel,1/2] libcamera: control: define an explicit default ControlInfo constructor
Related show

Commit Message

Christian Rauch Aug. 21, 2022, 12:43 p.m. UTC 
Explicitly set the default value for all ControlInfo to 0, false or minimum.

Signed-off-by: Christian Rauch <Rauch.Christian@gmx.de>
---
 src/ipa/raspberrypi/raspberrypi.cpp           | 22 ++++++++++---------
 src/ipa/rkisp1/rkisp1.cpp                     | 10 ++++-----
 src/libcamera/pipeline/ipu3/ipu3.cpp          |  2 +-
 .../pipeline/raspberrypi/raspberrypi.cpp      |  2 +-
 4 files changed, 19 insertions(+), 17 deletions(-)

--
2.34.1

Comments

Kieran Bingham Aug. 22, 2022, 9:52 p.m. UTC | #1 
Quoting Christian Rauch via libcamera-devel (2022-08-21 13:43:43)
> Explicitly set the default value for all ControlInfo to 0, false or minimum.
> 
> Signed-off-by: Christian Rauch <Rauch.Christian@gmx.de>
> ---
>  src/ipa/raspberrypi/raspberrypi.cpp           | 22 ++++++++++---------
>  src/ipa/rkisp1/rkisp1.cpp                     | 10 ++++-----
>  src/libcamera/pipeline/ipu3/ipu3.cpp          |  2 +-
>  .../pipeline/raspberrypi/raspberrypi.cpp      |  2 +-
>  4 files changed, 19 insertions(+), 17 deletions(-)
> 
> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
> index 69c73f8c..c6360a51 100644
> --- a/src/ipa/raspberrypi/raspberrypi.cpp
> +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> @@ -74,23 +74,23 @@ constexpr Duration controllerMinFrameDuration = 1.0s / 30.0;
> 
>  /* List of controls handled by the Raspberry Pi IPA */
>  static const ControlInfoMap::Map ipaControls{
> -       { &controls::AeEnable, ControlInfo(false, true) },
> -       { &controls::ExposureTime, ControlInfo(0, 66666) },
> -       { &controls::AnalogueGain, ControlInfo(1.0f, 16.0f) },
> +       { &controls::AeEnable, ControlInfo(false, true, false) },

I would expect most of the time AutoExposure would be enabled ... so a
default of false seems misleading... Yet - perhaps that's what the 0 was
already implying? So - does that mean a default is unreasonable here?


> +       { &controls::ExposureTime, ControlInfo(0, 66666, 0) },
> +       { &controls::AnalogueGain, ControlInfo(1.0f, 16.0f, 0.0f) },

This default is less than the minimum.


>         { &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::AwbEnable, ControlInfo(false, true) },
> -       { &controls::ColourGains, ControlInfo(0.0f, 32.0f) },
> +       { &controls::AwbEnable, ControlInfo(false, true, false) },

Same as AeEnable.... it doesn't seem reasonble to say the default is
disabled? I think applications probably expect the default IPA behaviour
to have AE/AWB enabled, unless it is disabled explicitly ?


> +       { &controls::ColourGains, ControlInfo(0.0f, 32.0f, 0.0f) },
>         { &controls::AwbMode, ControlInfo(controls::AwbModeValues) },
>         { &controls::Brightness, ControlInfo(-1.0f, 1.0f, 0.0f) },
>         { &controls::Contrast, ControlInfo(0.0f, 32.0f, 1.0f) },
>         { &controls::Saturation, ControlInfo(0.0f, 32.0f, 1.0f) },
>         { &controls::Sharpness, ControlInfo(0.0f, 16.0f, 1.0f) },
> -       { &controls::ColourCorrectionMatrix, ControlInfo(-16.0f, 16.0f) },
> +       { &controls::ColourCorrectionMatrix, ControlInfo(-16.0f, 16.0f, 0.0f) },
>         { &controls::ScalerCrop, ControlInfo(Rectangle{}, Rectangle(65535, 65535, 65535, 65535), Rectangle{}) },
> -       { &controls::FrameDurationLimits, ControlInfo(INT64_C(33333), INT64_C(120000)) },
> +       { &controls::FrameDurationLimits, ControlInfo(INT64_C(33333), INT64_C(120000), INT64_C(0)) },

Again, this is less than the minimum. A default should probably be
specific to the mode of the sensor too ?


>         { &controls::draft::NoiseReductionMode, ControlInfo(controls::draft::NoiseReductionModeValues) }
>  };
> 
> @@ -463,10 +463,11 @@ int IPARPi::configure(const IPACameraSensorInfo &sensorInfo,
>         const Duration maxSensorFrameDuration = mode_.maxFrameLength * mode_.lineLength;
>         ctrlMap[&controls::FrameDurationLimits] =
>                 ControlInfo(static_cast<int64_t>(minSensorFrameDuration.get<std::micro>()),
> -                           static_cast<int64_t>(maxSensorFrameDuration.get<std::micro>()));
> +                           static_cast<int64_t>(maxSensorFrameDuration.get<std::micro>()),
> +                           int64_t(0));

This is smaller than the minimum. Perhaps at least here during configure
it could be set to something more specific about the camera mode
capability - or something that is more 'default' like 30 FPS ? (33333 ?)

> 
>         ctrlMap[&controls::AnalogueGain] =
> -               ControlInfo(1.0f, static_cast<float>(helper_->gain(maxSensorGainCode_)));
> +               ControlInfo(1.0f, static_cast<float>(helper_->gain(maxSensorGainCode_)), 0.0f);

Less than minimum. I think 1.0f makes sense here too as that's unity
gain.

> 
>         /*
>          * Calculate the max exposure limit from the frame duration limit as V4L2
> @@ -478,7 +479,8 @@ int IPARPi::configure(const IPACameraSensorInfo &sensorInfo,
> 
>         ctrlMap[&controls::ExposureTime] =
>                 ControlInfo(static_cast<int32_t>(helper_->exposure(exposureMin).get<std::micro>()),
> -                           static_cast<int32_t>(maxShutter.get<std::micro>()));
> +                           static_cast<int32_t>(maxShutter.get<std::micro>()),
> +                           int32_t(0));

I'm not sure an exposure time of 0 makes sense ?, but I'm not sure what
a reasonable default is here either. Maybe something like the maximum of the frame
duration, and maxShutter ?

> 
>         result->controlInfo = ControlInfoMap(std::move(ctrlMap), controls::controls);
>         return 0;
> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
> index 27b4212b..2121bfd2 100644
> --- a/src/ipa/rkisp1/rkisp1.cpp
> +++ b/src/ipa/rkisp1/rkisp1.cpp
> @@ -91,12 +91,12 @@ namespace {
> 
>  /* List of controls handled by the RkISP1 IPA */
>  const ControlInfoMap::Map rkisp1Controls{
> -       { &controls::AeEnable, ControlInfo(false, true) },
> -       { &controls::AwbEnable, ControlInfo(false, true) },
> +       { &controls::AeEnable, ControlInfo(false, true, false) },
> +       { &controls::AwbEnable, ControlInfo(false, true, false) },

Same issue as in RPi.


>         { &controls::ColourGains, ControlInfo(0.0f, 3.996f, 1.0f) },
> -       { &controls::Brightness, ControlInfo(-1.0f, 0.993f) },
> -       { &controls::Contrast, ControlInfo(0.0f, 1.993f) },
> -       { &controls::Saturation, ControlInfo(0.0f, 1.993f) },
> +       { &controls::Brightness, ControlInfo(-1.0f, 0.993f, 0.0f) },
> +       { &controls::Contrast, ControlInfo(0.0f, 1.993f, 0.0f) },
> +       { &controls::Saturation, ControlInfo(0.0f, 1.993f, 0.0f) },

I think 0.0 makes sense here though

>         { &controls::Sharpness, ControlInfo(0.0f, 10.0f, 1.0f) },
>         { &controls::draft::NoiseReductionMode, ControlInfo(controls::draft::NoiseReductionModeValues) },
>  };
> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> index 9df24603..a1bcfe49 100644
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> @@ -42,7 +42,7 @@ namespace libcamera {
>  LOG_DEFINE_CATEGORY(IPU3)
> 
>  static const ControlInfoMap::Map IPU3Controls = {
> -       { &controls::draft::PipelineDepth, ControlInfo(2, 3) },
> +       { &controls::draft::PipelineDepth, ControlInfo(2, 3, 0) },


This needs looking at. The PipelineDepth isn't something the application
can set. So a default doesn't makes sense, and I'm not sure a min/max
does either.



>  };
> 
>  class IPU3CameraData : public Camera::Private
> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> index e895584d..8fd7634d 100644
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> @@ -954,7 +954,7 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)
>         /* Add the ScalerCrop control limits based on the current mode. */
>         Rectangle ispMinCrop(data->ispMinCropSize_);
>         ispMinCrop.scaleBy(data->sensorInfo_.analogCrop.size(), data->sensorInfo_.outputSize);
> -       ctrlMap[&controls::ScalerCrop] = ControlInfo(ispMinCrop, Rectangle(data->sensorInfo_.analogCrop.size()));
> +       ctrlMap[&controls::ScalerCrop] = ControlInfo(ispMinCrop, Rectangle(data->sensorInfo_.analogCrop.size()), ispMinCrop);

I think a ScalerCrop would probably expect a default of the largest size
possible, not the smallest ? (At least if I were to think about
'resetting' the ScalerCrop that is).

I suspect forcing a default value might be better than implicitly
leaving it zero - but it means we need to give more thought to what the
defaults mean or represent - or provide a way for the default to be
'unset' if it's not appropriate to have one.

--
Kieran


> 
>         data->controlInfo_ = ControlInfoMap(std::move(ctrlMap), result.controlInfo.idmap());
> 
> --
> 2.34.1
>
Christian Rauch Aug. 22, 2022, 10:23 p.m. UTC | #2 
Hi Kieran,

Regarding all your comments, I simply replaced the "default" 0 for "def"
from the old constructor with explicitly setting it 0 (or its binary
counterpart 'false') to match the types. If 0 is the wrong "def" value
for those controls, then they were already wrong before :-) They were
just implicitly wrong. Now you have to explicitly specify a "def" value,
and I kept 0 for backward compatibility.

Feel free to change the default values. You should also be able to merge
this patch (2/2) without the first (1/2).

Best,
Christian


Am 22.08.22 um 23:52 schrieb Kieran Bingham:
> Quoting Christian Rauch via libcamera-devel (2022-08-21 13:43:43)
>> Explicitly set the default value for all ControlInfo to 0, false or minimum.
>>
>> Signed-off-by: Christian Rauch <Rauch.Christian@gmx.de>
>> ---
>>  src/ipa/raspberrypi/raspberrypi.cpp           | 22 ++++++++++---------
>>  src/ipa/rkisp1/rkisp1.cpp                     | 10 ++++-----
>>  src/libcamera/pipeline/ipu3/ipu3.cpp          |  2 +-
>>  .../pipeline/raspberrypi/raspberrypi.cpp      |  2 +-
>>  4 files changed, 19 insertions(+), 17 deletions(-)
>>
>> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
>> index 69c73f8c..c6360a51 100644
>> --- a/src/ipa/raspberrypi/raspberrypi.cpp
>> +++ b/src/ipa/raspberrypi/raspberrypi.cpp
>> @@ -74,23 +74,23 @@ constexpr Duration controllerMinFrameDuration = 1.0s / 30.0;
>>
>>  /* List of controls handled by the Raspberry Pi IPA */
>>  static const ControlInfoMap::Map ipaControls{
>> -       { &controls::AeEnable, ControlInfo(false, true) },
>> -       { &controls::ExposureTime, ControlInfo(0, 66666) },
>> -       { &controls::AnalogueGain, ControlInfo(1.0f, 16.0f) },
>> +       { &controls::AeEnable, ControlInfo(false, true, false) },
>
> I would expect most of the time AutoExposure would be enabled ... so a
> default of false seems misleading... Yet - perhaps that's what the 0 was
> already implying? So - does that mean a default is unreasonable here?
>
>
>> +       { &controls::ExposureTime, ControlInfo(0, 66666, 0) },
>> +       { &controls::AnalogueGain, ControlInfo(1.0f, 16.0f, 0.0f) },
>
> This default is less than the minimum.
>
>
>>         { &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::AwbEnable, ControlInfo(false, true) },
>> -       { &controls::ColourGains, ControlInfo(0.0f, 32.0f) },
>> +       { &controls::AwbEnable, ControlInfo(false, true, false) },
>
> Same as AeEnable.... it doesn't seem reasonble to say the default is
> disabled? I think applications probably expect the default IPA behaviour
> to have AE/AWB enabled, unless it is disabled explicitly ?
>
>
>> +       { &controls::ColourGains, ControlInfo(0.0f, 32.0f, 0.0f) },
>>         { &controls::AwbMode, ControlInfo(controls::AwbModeValues) },
>>         { &controls::Brightness, ControlInfo(-1.0f, 1.0f, 0.0f) },
>>         { &controls::Contrast, ControlInfo(0.0f, 32.0f, 1.0f) },
>>         { &controls::Saturation, ControlInfo(0.0f, 32.0f, 1.0f) },
>>         { &controls::Sharpness, ControlInfo(0.0f, 16.0f, 1.0f) },
>> -       { &controls::ColourCorrectionMatrix, ControlInfo(-16.0f, 16.0f) },
>> +       { &controls::ColourCorrectionMatrix, ControlInfo(-16.0f, 16.0f, 0.0f) },
>>         { &controls::ScalerCrop, ControlInfo(Rectangle{}, Rectangle(65535, 65535, 65535, 65535), Rectangle{}) },
>> -       { &controls::FrameDurationLimits, ControlInfo(INT64_C(33333), INT64_C(120000)) },
>> +       { &controls::FrameDurationLimits, ControlInfo(INT64_C(33333), INT64_C(120000), INT64_C(0)) },
>
> Again, this is less than the minimum. A default should probably be
> specific to the mode of the sensor too ?
>
>
>>         { &controls::draft::NoiseReductionMode, ControlInfo(controls::draft::NoiseReductionModeValues) }
>>  };
>>
>> @@ -463,10 +463,11 @@ int IPARPi::configure(const IPACameraSensorInfo &sensorInfo,
>>         const Duration maxSensorFrameDuration = mode_.maxFrameLength * mode_.lineLength;
>>         ctrlMap[&controls::FrameDurationLimits] =
>>                 ControlInfo(static_cast<int64_t>(minSensorFrameDuration.get<std::micro>()),
>> -                           static_cast<int64_t>(maxSensorFrameDuration.get<std::micro>()));
>> +                           static_cast<int64_t>(maxSensorFrameDuration.get<std::micro>()),
>> +                           int64_t(0));
>
> This is smaller than the minimum. Perhaps at least here during configure
> it could be set to something more specific about the camera mode
> capability - or something that is more 'default' like 30 FPS ? (33333 ?)
>
>>
>>         ctrlMap[&controls::AnalogueGain] =
>> -               ControlInfo(1.0f, static_cast<float>(helper_->gain(maxSensorGainCode_)));
>> +               ControlInfo(1.0f, static_cast<float>(helper_->gain(maxSensorGainCode_)), 0.0f);
>
> Less than minimum. I think 1.0f makes sense here too as that's unity
> gain.
>
>>
>>         /*
>>          * Calculate the max exposure limit from the frame duration limit as V4L2
>> @@ -478,7 +479,8 @@ int IPARPi::configure(const IPACameraSensorInfo &sensorInfo,
>>
>>         ctrlMap[&controls::ExposureTime] =
>>                 ControlInfo(static_cast<int32_t>(helper_->exposure(exposureMin).get<std::micro>()),
>> -                           static_cast<int32_t>(maxShutter.get<std::micro>()));
>> +                           static_cast<int32_t>(maxShutter.get<std::micro>()),
>> +                           int32_t(0));
>
> I'm not sure an exposure time of 0 makes sense ?, but I'm not sure what
> a reasonable default is here either. Maybe something like the maximum of the frame
> duration, and maxShutter ?
>
>>
>>         result->controlInfo = ControlInfoMap(std::move(ctrlMap), controls::controls);
>>         return 0;
>> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
>> index 27b4212b..2121bfd2 100644
>> --- a/src/ipa/rkisp1/rkisp1.cpp
>> +++ b/src/ipa/rkisp1/rkisp1.cpp
>> @@ -91,12 +91,12 @@ namespace {
>>
>>  /* List of controls handled by the RkISP1 IPA */
>>  const ControlInfoMap::Map rkisp1Controls{
>> -       { &controls::AeEnable, ControlInfo(false, true) },
>> -       { &controls::AwbEnable, ControlInfo(false, true) },
>> +       { &controls::AeEnable, ControlInfo(false, true, false) },
>> +       { &controls::AwbEnable, ControlInfo(false, true, false) },
>
> Same issue as in RPi.
>
>
>>         { &controls::ColourGains, ControlInfo(0.0f, 3.996f, 1.0f) },
>> -       { &controls::Brightness, ControlInfo(-1.0f, 0.993f) },
>> -       { &controls::Contrast, ControlInfo(0.0f, 1.993f) },
>> -       { &controls::Saturation, ControlInfo(0.0f, 1.993f) },
>> +       { &controls::Brightness, ControlInfo(-1.0f, 0.993f, 0.0f) },
>> +       { &controls::Contrast, ControlInfo(0.0f, 1.993f, 0.0f) },
>> +       { &controls::Saturation, ControlInfo(0.0f, 1.993f, 0.0f) },
>
> I think 0.0 makes sense here though
>
>>         { &controls::Sharpness, ControlInfo(0.0f, 10.0f, 1.0f) },
>>         { &controls::draft::NoiseReductionMode, ControlInfo(controls::draft::NoiseReductionModeValues) },
>>  };
>> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
>> index 9df24603..a1bcfe49 100644
>> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
>> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
>> @@ -42,7 +42,7 @@ namespace libcamera {
>>  LOG_DEFINE_CATEGORY(IPU3)
>>
>>  static const ControlInfoMap::Map IPU3Controls = {
>> -       { &controls::draft::PipelineDepth, ControlInfo(2, 3) },
>> +       { &controls::draft::PipelineDepth, ControlInfo(2, 3, 0) },
>
>
> This needs looking at. The PipelineDepth isn't something the application
> can set. So a default doesn't makes sense, and I'm not sure a min/max
> does either.
>
>
>
>>  };
>>
>>  class IPU3CameraData : public Camera::Private
>> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
>> index e895584d..8fd7634d 100644
>> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
>> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
>> @@ -954,7 +954,7 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)
>>         /* Add the ScalerCrop control limits based on the current mode. */
>>         Rectangle ispMinCrop(data->ispMinCropSize_);
>>         ispMinCrop.scaleBy(data->sensorInfo_.analogCrop.size(), data->sensorInfo_.outputSize);
>> -       ctrlMap[&controls::ScalerCrop] = ControlInfo(ispMinCrop, Rectangle(data->sensorInfo_.analogCrop.size()));
>> +       ctrlMap[&controls::ScalerCrop] = ControlInfo(ispMinCrop, Rectangle(data->sensorInfo_.analogCrop.size()), ispMinCrop);
>
> I think a ScalerCrop would probably expect a default of the largest size
> possible, not the smallest ? (At least if I were to think about
> 'resetting' the ScalerCrop that is).
>
> I suspect forcing a default value might be better than implicitly
> leaving it zero - but it means we need to give more thought to what the
> defaults mean or represent - or provide a way for the default to be
> 'unset' if it's not appropriate to have one.
>
> --
> Kieran
>
>
>>
>>         data->controlInfo_ = ControlInfoMap(std::move(ctrlMap), result.controlInfo.idmap());
>>
>> --
>> 2.34.1
>>
Kieran Bingham Aug. 22, 2022, 11:01 p.m. UTC | #3 
Quoting Christian Rauch via libcamera-devel (2022-08-22 23:23:45)
> Hi Kieran,
> 
> Regarding all your comments, I simply replaced the "default" 0 for "def"
> from the old constructor with explicitly setting it 0 (or its binary
> counterpart 'false') to match the types. If 0 is the wrong "def" value
> for those controls, then they were already wrong before :-) They were
> just implicitly wrong. Now you have to explicitly specify a "def" value,
> and I kept 0 for backward compatibility.

Yes, they may have been implicitly incorrect before, but we shouldn't
set them to be explictly incorrect. So this proposed series means we
need to consider each one carefully.

As mentioned though, I think we need to work out if all controls make
sense to have a default value, and if not - how to express that it isn't
available. I suspect that previously the omision of a default value was
the expression that the default was 'redundant' (or unnecessary), thus
by making it required, you need to explicitly set each one.

--
Kieran



> Feel free to change the default values. You should also be able to merge
> this patch (2/2) without the first (1/2).
> 
> Best,
> Christian
> 
> 
> Am 22.08.22 um 23:52 schrieb Kieran Bingham:
> > Quoting Christian Rauch via libcamera-devel (2022-08-21 13:43:43)
> >> Explicitly set the default value for all ControlInfo to 0, false or minimum.
> >>
> >> Signed-off-by: Christian Rauch <Rauch.Christian@gmx.de>
> >> ---
> >>  src/ipa/raspberrypi/raspberrypi.cpp           | 22 ++++++++++---------
> >>  src/ipa/rkisp1/rkisp1.cpp                     | 10 ++++-----
> >>  src/libcamera/pipeline/ipu3/ipu3.cpp          |  2 +-
> >>  .../pipeline/raspberrypi/raspberrypi.cpp      |  2 +-
> >>  4 files changed, 19 insertions(+), 17 deletions(-)
> >>
> >> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
> >> index 69c73f8c..c6360a51 100644
> >> --- a/src/ipa/raspberrypi/raspberrypi.cpp
> >> +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> >> @@ -74,23 +74,23 @@ constexpr Duration controllerMinFrameDuration = 1.0s / 30.0;
> >>
> >>  /* List of controls handled by the Raspberry Pi IPA */
> >>  static const ControlInfoMap::Map ipaControls{
> >> -       { &controls::AeEnable, ControlInfo(false, true) },
> >> -       { &controls::ExposureTime, ControlInfo(0, 66666) },
> >> -       { &controls::AnalogueGain, ControlInfo(1.0f, 16.0f) },
> >> +       { &controls::AeEnable, ControlInfo(false, true, false) },
> >
> > I would expect most of the time AutoExposure would be enabled ... so a
> > default of false seems misleading... Yet - perhaps that's what the 0 was
> > already implying? So - does that mean a default is unreasonable here?
> >
> >
> >> +       { &controls::ExposureTime, ControlInfo(0, 66666, 0) },
> >> +       { &controls::AnalogueGain, ControlInfo(1.0f, 16.0f, 0.0f) },
> >
> > This default is less than the minimum.
> >
> >
> >>         { &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::AwbEnable, ControlInfo(false, true) },
> >> -       { &controls::ColourGains, ControlInfo(0.0f, 32.0f) },
> >> +       { &controls::AwbEnable, ControlInfo(false, true, false) },
> >
> > Same as AeEnable.... it doesn't seem reasonble to say the default is
> > disabled? I think applications probably expect the default IPA behaviour
> > to have AE/AWB enabled, unless it is disabled explicitly ?
> >
> >
> >> +       { &controls::ColourGains, ControlInfo(0.0f, 32.0f, 0.0f) },
> >>         { &controls::AwbMode, ControlInfo(controls::AwbModeValues) },
> >>         { &controls::Brightness, ControlInfo(-1.0f, 1.0f, 0.0f) },
> >>         { &controls::Contrast, ControlInfo(0.0f, 32.0f, 1.0f) },
> >>         { &controls::Saturation, ControlInfo(0.0f, 32.0f, 1.0f) },
> >>         { &controls::Sharpness, ControlInfo(0.0f, 16.0f, 1.0f) },
> >> -       { &controls::ColourCorrectionMatrix, ControlInfo(-16.0f, 16.0f) },
> >> +       { &controls::ColourCorrectionMatrix, ControlInfo(-16.0f, 16.0f, 0.0f) },
> >>         { &controls::ScalerCrop, ControlInfo(Rectangle{}, Rectangle(65535, 65535, 65535, 65535), Rectangle{}) },
> >> -       { &controls::FrameDurationLimits, ControlInfo(INT64_C(33333), INT64_C(120000)) },
> >> +       { &controls::FrameDurationLimits, ControlInfo(INT64_C(33333), INT64_C(120000), INT64_C(0)) },
> >
> > Again, this is less than the minimum. A default should probably be
> > specific to the mode of the sensor too ?
> >
> >
> >>         { &controls::draft::NoiseReductionMode, ControlInfo(controls::draft::NoiseReductionModeValues) }
> >>  };
> >>
> >> @@ -463,10 +463,11 @@ int IPARPi::configure(const IPACameraSensorInfo &sensorInfo,
> >>         const Duration maxSensorFrameDuration = mode_.maxFrameLength * mode_.lineLength;
> >>         ctrlMap[&controls::FrameDurationLimits] =
> >>                 ControlInfo(static_cast<int64_t>(minSensorFrameDuration.get<std::micro>()),
> >> -                           static_cast<int64_t>(maxSensorFrameDuration.get<std::micro>()));
> >> +                           static_cast<int64_t>(maxSensorFrameDuration.get<std::micro>()),
> >> +                           int64_t(0));
> >
> > This is smaller than the minimum. Perhaps at least here during configure
> > it could be set to something more specific about the camera mode
> > capability - or something that is more 'default' like 30 FPS ? (33333 ?)
> >
> >>
> >>         ctrlMap[&controls::AnalogueGain] =
> >> -               ControlInfo(1.0f, static_cast<float>(helper_->gain(maxSensorGainCode_)));
> >> +               ControlInfo(1.0f, static_cast<float>(helper_->gain(maxSensorGainCode_)), 0.0f);
> >
> > Less than minimum. I think 1.0f makes sense here too as that's unity
> > gain.
> >
> >>
> >>         /*
> >>          * Calculate the max exposure limit from the frame duration limit as V4L2
> >> @@ -478,7 +479,8 @@ int IPARPi::configure(const IPACameraSensorInfo &sensorInfo,
> >>
> >>         ctrlMap[&controls::ExposureTime] =
> >>                 ControlInfo(static_cast<int32_t>(helper_->exposure(exposureMin).get<std::micro>()),
> >> -                           static_cast<int32_t>(maxShutter.get<std::micro>()));
> >> +                           static_cast<int32_t>(maxShutter.get<std::micro>()),
> >> +                           int32_t(0));
> >
> > I'm not sure an exposure time of 0 makes sense ?, but I'm not sure what
> > a reasonable default is here either. Maybe something like the maximum of the frame
> > duration, and maxShutter ?
> >
> >>
> >>         result->controlInfo = ControlInfoMap(std::move(ctrlMap), controls::controls);
> >>         return 0;
> >> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
> >> index 27b4212b..2121bfd2 100644
> >> --- a/src/ipa/rkisp1/rkisp1.cpp
> >> +++ b/src/ipa/rkisp1/rkisp1.cpp
> >> @@ -91,12 +91,12 @@ namespace {
> >>
> >>  /* List of controls handled by the RkISP1 IPA */
> >>  const ControlInfoMap::Map rkisp1Controls{
> >> -       { &controls::AeEnable, ControlInfo(false, true) },
> >> -       { &controls::AwbEnable, ControlInfo(false, true) },
> >> +       { &controls::AeEnable, ControlInfo(false, true, false) },
> >> +       { &controls::AwbEnable, ControlInfo(false, true, false) },
> >
> > Same issue as in RPi.
> >
> >
> >>         { &controls::ColourGains, ControlInfo(0.0f, 3.996f, 1.0f) },
> >> -       { &controls::Brightness, ControlInfo(-1.0f, 0.993f) },
> >> -       { &controls::Contrast, ControlInfo(0.0f, 1.993f) },
> >> -       { &controls::Saturation, ControlInfo(0.0f, 1.993f) },
> >> +       { &controls::Brightness, ControlInfo(-1.0f, 0.993f, 0.0f) },
> >> +       { &controls::Contrast, ControlInfo(0.0f, 1.993f, 0.0f) },
> >> +       { &controls::Saturation, ControlInfo(0.0f, 1.993f, 0.0f) },
> >
> > I think 0.0 makes sense here though
> >
> >>         { &controls::Sharpness, ControlInfo(0.0f, 10.0f, 1.0f) },
> >>         { &controls::draft::NoiseReductionMode, ControlInfo(controls::draft::NoiseReductionModeValues) },
> >>  };
> >> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> >> index 9df24603..a1bcfe49 100644
> >> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> >> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> >> @@ -42,7 +42,7 @@ namespace libcamera {
> >>  LOG_DEFINE_CATEGORY(IPU3)
> >>
> >>  static const ControlInfoMap::Map IPU3Controls = {
> >> -       { &controls::draft::PipelineDepth, ControlInfo(2, 3) },
> >> +       { &controls::draft::PipelineDepth, ControlInfo(2, 3, 0) },
> >
> >
> > This needs looking at. The PipelineDepth isn't something the application
> > can set. So a default doesn't makes sense, and I'm not sure a min/max
> > does either.
> >
> >
> >
> >>  };
> >>
> >>  class IPU3CameraData : public Camera::Private
> >> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> >> index e895584d..8fd7634d 100644
> >> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> >> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> >> @@ -954,7 +954,7 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)
> >>         /* Add the ScalerCrop control limits based on the current mode. */
> >>         Rectangle ispMinCrop(data->ispMinCropSize_);
> >>         ispMinCrop.scaleBy(data->sensorInfo_.analogCrop.size(), data->sensorInfo_.outputSize);
> >> -       ctrlMap[&controls::ScalerCrop] = ControlInfo(ispMinCrop, Rectangle(data->sensorInfo_.analogCrop.size()));
> >> +       ctrlMap[&controls::ScalerCrop] = ControlInfo(ispMinCrop, Rectangle(data->sensorInfo_.analogCrop.size()), ispMinCrop);
> >
> > I think a ScalerCrop would probably expect a default of the largest size
> > possible, not the smallest ? (At least if I were to think about
> > 'resetting' the ScalerCrop that is).
> >
> > I suspect forcing a default value might be better than implicitly
> > leaving it zero - but it means we need to give more thought to what the
> > defaults mean or represent - or provide a way for the default to be
> > 'unset' if it's not appropriate to have one.
> >
> > --
> > Kieran
> >
> >
> >>
> >>         data->controlInfo_ = ControlInfoMap(std::move(ctrlMap), result.controlInfo.idmap());
> >>
> >> --
> >> 2.34.1
> >>
Christian Rauch Aug. 23, 2022, 5:50 p.m. UTC | #4 
Hi Kieran,

Am 23.08.22 um 01:01 schrieb Kieran Bingham:
> Quoting Christian Rauch via libcamera-devel (2022-08-22 23:23:45)
>> Hi Kieran,
>>
>> Regarding all your comments, I simply replaced the "default" 0 for "def"
>> from the old constructor with explicitly setting it 0 (or its binary
>> counterpart 'false') to match the types. If 0 is the wrong "def" value
>> for those controls, then they were already wrong before :-) They were
>> just implicitly wrong. Now you have to explicitly specify a "def" value,
>> and I kept 0 for backward compatibility.
>
> Yes, they may have been implicitly incorrect before, but we shouldn't
> set them to be explictly incorrect. So this proposed series means we
> need to consider each one carefully.
>
> As mentioned though, I think we need to work out if all controls make
> sense to have a default value, and if not - how to express that it isn't
> available. I suspect that previously the omision of a default value was
> the expression that the default was 'redundant' (or unnecessary), thus
> by making it required, you need to explicitly set each one.

Deliberating the right choice of default values will take a lot of time
and I am probably also not the right person to make these choices. My
intention with this patch series was to not change the current
behaviour, but just to make it a requirement that the min/max/def values
are either explicitly set fully, or not set at all.

If the current (and proposed) behaviour of initialising by 0 is not
desired, then I would suggest that this should be handled in a separate
patch series that involves more stakeholders that know better than me
which default values are sensible.

Best,
Christian

>
> --
> Kieran
>
>
>
>> Feel free to change the default values. You should also be able to merge
>> this patch (2/2) without the first (1/2).
>>
>> Best,
>> Christian
>>
>>
>> Am 22.08.22 um 23:52 schrieb Kieran Bingham:
>>> Quoting Christian Rauch via libcamera-devel (2022-08-21 13:43:43)
>>>> Explicitly set the default value for all ControlInfo to 0, false or minimum.
>>>>
>>>> Signed-off-by: Christian Rauch <Rauch.Christian@gmx.de>
>>>> ---
>>>>  src/ipa/raspberrypi/raspberrypi.cpp           | 22 ++++++++++---------
>>>>  src/ipa/rkisp1/rkisp1.cpp                     | 10 ++++-----
>>>>  src/libcamera/pipeline/ipu3/ipu3.cpp          |  2 +-
>>>>  .../pipeline/raspberrypi/raspberrypi.cpp      |  2 +-
>>>>  4 files changed, 19 insertions(+), 17 deletions(-)
>>>>
>>>> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
>>>> index 69c73f8c..c6360a51 100644
>>>> --- a/src/ipa/raspberrypi/raspberrypi.cpp
>>>> +++ b/src/ipa/raspberrypi/raspberrypi.cpp
>>>> @@ -74,23 +74,23 @@ constexpr Duration controllerMinFrameDuration = 1.0s / 30.0;
>>>>
>>>>  /* List of controls handled by the Raspberry Pi IPA */
>>>>  static const ControlInfoMap::Map ipaControls{
>>>> -       { &controls::AeEnable, ControlInfo(false, true) },
>>>> -       { &controls::ExposureTime, ControlInfo(0, 66666) },
>>>> -       { &controls::AnalogueGain, ControlInfo(1.0f, 16.0f) },
>>>> +       { &controls::AeEnable, ControlInfo(false, true, false) },
>>>
>>> I would expect most of the time AutoExposure would be enabled ... so a
>>> default of false seems misleading... Yet - perhaps that's what the 0 was
>>> already implying? So - does that mean a default is unreasonable here?
>>>
>>>
>>>> +       { &controls::ExposureTime, ControlInfo(0, 66666, 0) },
>>>> +       { &controls::AnalogueGain, ControlInfo(1.0f, 16.0f, 0.0f) },
>>>
>>> This default is less than the minimum.
>>>
>>>
>>>>         { &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::AwbEnable, ControlInfo(false, true) },
>>>> -       { &controls::ColourGains, ControlInfo(0.0f, 32.0f) },
>>>> +       { &controls::AwbEnable, ControlInfo(false, true, false) },
>>>
>>> Same as AeEnable.... it doesn't seem reasonble to say the default is
>>> disabled? I think applications probably expect the default IPA behaviour
>>> to have AE/AWB enabled, unless it is disabled explicitly ?
>>>
>>>
>>>> +       { &controls::ColourGains, ControlInfo(0.0f, 32.0f, 0.0f) },
>>>>         { &controls::AwbMode, ControlInfo(controls::AwbModeValues) },
>>>>         { &controls::Brightness, ControlInfo(-1.0f, 1.0f, 0.0f) },
>>>>         { &controls::Contrast, ControlInfo(0.0f, 32.0f, 1.0f) },
>>>>         { &controls::Saturation, ControlInfo(0.0f, 32.0f, 1.0f) },
>>>>         { &controls::Sharpness, ControlInfo(0.0f, 16.0f, 1.0f) },
>>>> -       { &controls::ColourCorrectionMatrix, ControlInfo(-16.0f, 16.0f) },
>>>> +       { &controls::ColourCorrectionMatrix, ControlInfo(-16.0f, 16.0f, 0.0f) },
>>>>         { &controls::ScalerCrop, ControlInfo(Rectangle{}, Rectangle(65535, 65535, 65535, 65535), Rectangle{}) },
>>>> -       { &controls::FrameDurationLimits, ControlInfo(INT64_C(33333), INT64_C(120000)) },
>>>> +       { &controls::FrameDurationLimits, ControlInfo(INT64_C(33333), INT64_C(120000), INT64_C(0)) },
>>>
>>> Again, this is less than the minimum. A default should probably be
>>> specific to the mode of the sensor too ?
>>>
>>>
>>>>         { &controls::draft::NoiseReductionMode, ControlInfo(controls::draft::NoiseReductionModeValues) }
>>>>  };
>>>>
>>>> @@ -463,10 +463,11 @@ int IPARPi::configure(const IPACameraSensorInfo &sensorInfo,
>>>>         const Duration maxSensorFrameDuration = mode_.maxFrameLength * mode_.lineLength;
>>>>         ctrlMap[&controls::FrameDurationLimits] =
>>>>                 ControlInfo(static_cast<int64_t>(minSensorFrameDuration.get<std::micro>()),
>>>> -                           static_cast<int64_t>(maxSensorFrameDuration.get<std::micro>()));
>>>> +                           static_cast<int64_t>(maxSensorFrameDuration.get<std::micro>()),
>>>> +                           int64_t(0));
>>>
>>> This is smaller than the minimum. Perhaps at least here during configure
>>> it could be set to something more specific about the camera mode
>>> capability - or something that is more 'default' like 30 FPS ? (33333 ?)
>>>
>>>>
>>>>         ctrlMap[&controls::AnalogueGain] =
>>>> -               ControlInfo(1.0f, static_cast<float>(helper_->gain(maxSensorGainCode_)));
>>>> +               ControlInfo(1.0f, static_cast<float>(helper_->gain(maxSensorGainCode_)), 0.0f);
>>>
>>> Less than minimum. I think 1.0f makes sense here too as that's unity
>>> gain.
>>>
>>>>
>>>>         /*
>>>>          * Calculate the max exposure limit from the frame duration limit as V4L2
>>>> @@ -478,7 +479,8 @@ int IPARPi::configure(const IPACameraSensorInfo &sensorInfo,
>>>>
>>>>         ctrlMap[&controls::ExposureTime] =
>>>>                 ControlInfo(static_cast<int32_t>(helper_->exposure(exposureMin).get<std::micro>()),
>>>> -                           static_cast<int32_t>(maxShutter.get<std::micro>()));
>>>> +                           static_cast<int32_t>(maxShutter.get<std::micro>()),
>>>> +                           int32_t(0));
>>>
>>> I'm not sure an exposure time of 0 makes sense ?, but I'm not sure what
>>> a reasonable default is here either. Maybe something like the maximum of the frame
>>> duration, and maxShutter ?
>>>
>>>>
>>>>         result->controlInfo = ControlInfoMap(std::move(ctrlMap), controls::controls);
>>>>         return 0;
>>>> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
>>>> index 27b4212b..2121bfd2 100644
>>>> --- a/src/ipa/rkisp1/rkisp1.cpp
>>>> +++ b/src/ipa/rkisp1/rkisp1.cpp
>>>> @@ -91,12 +91,12 @@ namespace {
>>>>
>>>>  /* List of controls handled by the RkISP1 IPA */
>>>>  const ControlInfoMap::Map rkisp1Controls{
>>>> -       { &controls::AeEnable, ControlInfo(false, true) },
>>>> -       { &controls::AwbEnable, ControlInfo(false, true) },
>>>> +       { &controls::AeEnable, ControlInfo(false, true, false) },
>>>> +       { &controls::AwbEnable, ControlInfo(false, true, false) },
>>>
>>> Same issue as in RPi.
>>>
>>>
>>>>         { &controls::ColourGains, ControlInfo(0.0f, 3.996f, 1.0f) },
>>>> -       { &controls::Brightness, ControlInfo(-1.0f, 0.993f) },
>>>> -       { &controls::Contrast, ControlInfo(0.0f, 1.993f) },
>>>> -       { &controls::Saturation, ControlInfo(0.0f, 1.993f) },
>>>> +       { &controls::Brightness, ControlInfo(-1.0f, 0.993f, 0.0f) },
>>>> +       { &controls::Contrast, ControlInfo(0.0f, 1.993f, 0.0f) },
>>>> +       { &controls::Saturation, ControlInfo(0.0f, 1.993f, 0.0f) },
>>>
>>> I think 0.0 makes sense here though
>>>
>>>>         { &controls::Sharpness, ControlInfo(0.0f, 10.0f, 1.0f) },
>>>>         { &controls::draft::NoiseReductionMode, ControlInfo(controls::draft::NoiseReductionModeValues) },
>>>>  };
>>>> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
>>>> index 9df24603..a1bcfe49 100644
>>>> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
>>>> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
>>>> @@ -42,7 +42,7 @@ namespace libcamera {
>>>>  LOG_DEFINE_CATEGORY(IPU3)
>>>>
>>>>  static const ControlInfoMap::Map IPU3Controls = {
>>>> -       { &controls::draft::PipelineDepth, ControlInfo(2, 3) },
>>>> +       { &controls::draft::PipelineDepth, ControlInfo(2, 3, 0) },
>>>
>>>
>>> This needs looking at. The PipelineDepth isn't something the application
>>> can set. So a default doesn't makes sense, and I'm not sure a min/max
>>> does either.
>>>
>>>
>>>
>>>>  };
>>>>
>>>>  class IPU3CameraData : public Camera::Private
>>>> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
>>>> index e895584d..8fd7634d 100644
>>>> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
>>>> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
>>>> @@ -954,7 +954,7 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)
>>>>         /* Add the ScalerCrop control limits based on the current mode. */
>>>>         Rectangle ispMinCrop(data->ispMinCropSize_);
>>>>         ispMinCrop.scaleBy(data->sensorInfo_.analogCrop.size(), data->sensorInfo_.outputSize);
>>>> -       ctrlMap[&controls::ScalerCrop] = ControlInfo(ispMinCrop, Rectangle(data->sensorInfo_.analogCrop.size()));
>>>> +       ctrlMap[&controls::ScalerCrop] = ControlInfo(ispMinCrop, Rectangle(data->sensorInfo_.analogCrop.size()), ispMinCrop);
>>>
>>> I think a ScalerCrop would probably expect a default of the largest size
>>> possible, not the smallest ? (At least if I were to think about
>>> 'resetting' the ScalerCrop that is).
>>>
>>> I suspect forcing a default value might be better than implicitly
>>> leaving it zero - but it means we need to give more thought to what the
>>> defaults mean or represent - or provide a way for the default to be
>>> 'unset' if it's not appropriate to have one.
>>>
>>> --
>>> Kieran
>>>
>>>
>>>>
>>>>         data->controlInfo_ = ControlInfoMap(std::move(ctrlMap), result.controlInfo.idmap());
>>>>
>>>> --
>>>> 2.34.1
>>>>
Christian Rauch Aug. 29, 2022, 8:32 p.m. UTC | #5 
Hi Kieran,

I replaced the [1/2] patch in this set with a new one ("libcamera:
control: initialise control info to ControlTypeNone by default") that
default constructs the min/max/def ControlValue instead of setting it to
0. This allows explicitly checking for 'ControlTypeNone' instead of
relying on 0 as an implicit representation.

Best,
Christian


Am 23.08.22 um 01:01 schrieb Kieran Bingham:
> Quoting Christian Rauch via libcamera-devel (2022-08-22 23:23:45)
>> Hi Kieran,
>>
>> Regarding all your comments, I simply replaced the "default" 0 for "def"
>> from the old constructor with explicitly setting it 0 (or its binary
>> counterpart 'false') to match the types. If 0 is the wrong "def" value
>> for those controls, then they were already wrong before :-) They were
>> just implicitly wrong. Now you have to explicitly specify a "def" value,
>> and I kept 0 for backward compatibility.
>
> Yes, they may have been implicitly incorrect before, but we shouldn't
> set them to be explictly incorrect. So this proposed series means we
> need to consider each one carefully.
>
> As mentioned though, I think we need to work out if all controls make
> sense to have a default value, and if not - how to express that it isn't
> available. I suspect that previously the omision of a default value was
> the expression that the default was 'redundant' (or unnecessary), thus
> by making it required, you need to explicitly set each one.
>
> --
> Kieran
>
>
>
>> Feel free to change the default values. You should also be able to merge
>> this patch (2/2) without the first (1/2).
>>
>> Best,
>> Christian
>>
>>
>> Am 22.08.22 um 23:52 schrieb Kieran Bingham:
>>> Quoting Christian Rauch via libcamera-devel (2022-08-21 13:43:43)
>>>> Explicitly set the default value for all ControlInfo to 0, false or minimum.
>>>>
>>>> Signed-off-by: Christian Rauch <Rauch.Christian@gmx.de>
>>>> ---
>>>>  src/ipa/raspberrypi/raspberrypi.cpp           | 22 ++++++++++---------
>>>>  src/ipa/rkisp1/rkisp1.cpp                     | 10 ++++-----
>>>>  src/libcamera/pipeline/ipu3/ipu3.cpp          |  2 +-
>>>>  .../pipeline/raspberrypi/raspberrypi.cpp      |  2 +-
>>>>  4 files changed, 19 insertions(+), 17 deletions(-)
>>>>
>>>> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
>>>> index 69c73f8c..c6360a51 100644
>>>> --- a/src/ipa/raspberrypi/raspberrypi.cpp
>>>> +++ b/src/ipa/raspberrypi/raspberrypi.cpp
>>>> @@ -74,23 +74,23 @@ constexpr Duration controllerMinFrameDuration = 1.0s / 30.0;
>>>>
>>>>  /* List of controls handled by the Raspberry Pi IPA */
>>>>  static const ControlInfoMap::Map ipaControls{
>>>> -       { &controls::AeEnable, ControlInfo(false, true) },
>>>> -       { &controls::ExposureTime, ControlInfo(0, 66666) },
>>>> -       { &controls::AnalogueGain, ControlInfo(1.0f, 16.0f) },
>>>> +       { &controls::AeEnable, ControlInfo(false, true, false) },
>>>
>>> I would expect most of the time AutoExposure would be enabled ... so a
>>> default of false seems misleading... Yet - perhaps that's what the 0 was
>>> already implying? So - does that mean a default is unreasonable here?
>>>
>>>
>>>> +       { &controls::ExposureTime, ControlInfo(0, 66666, 0) },
>>>> +       { &controls::AnalogueGain, ControlInfo(1.0f, 16.0f, 0.0f) },
>>>
>>> This default is less than the minimum.
>>>
>>>
>>>>         { &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::AwbEnable, ControlInfo(false, true) },
>>>> -       { &controls::ColourGains, ControlInfo(0.0f, 32.0f) },
>>>> +       { &controls::AwbEnable, ControlInfo(false, true, false) },
>>>
>>> Same as AeEnable.... it doesn't seem reasonble to say the default is
>>> disabled? I think applications probably expect the default IPA behaviour
>>> to have AE/AWB enabled, unless it is disabled explicitly ?
>>>
>>>
>>>> +       { &controls::ColourGains, ControlInfo(0.0f, 32.0f, 0.0f) },
>>>>         { &controls::AwbMode, ControlInfo(controls::AwbModeValues) },
>>>>         { &controls::Brightness, ControlInfo(-1.0f, 1.0f, 0.0f) },
>>>>         { &controls::Contrast, ControlInfo(0.0f, 32.0f, 1.0f) },
>>>>         { &controls::Saturation, ControlInfo(0.0f, 32.0f, 1.0f) },
>>>>         { &controls::Sharpness, ControlInfo(0.0f, 16.0f, 1.0f) },
>>>> -       { &controls::ColourCorrectionMatrix, ControlInfo(-16.0f, 16.0f) },
>>>> +       { &controls::ColourCorrectionMatrix, ControlInfo(-16.0f, 16.0f, 0.0f) },
>>>>         { &controls::ScalerCrop, ControlInfo(Rectangle{}, Rectangle(65535, 65535, 65535, 65535), Rectangle{}) },
>>>> -       { &controls::FrameDurationLimits, ControlInfo(INT64_C(33333), INT64_C(120000)) },
>>>> +       { &controls::FrameDurationLimits, ControlInfo(INT64_C(33333), INT64_C(120000), INT64_C(0)) },
>>>
>>> Again, this is less than the minimum. A default should probably be
>>> specific to the mode of the sensor too ?
>>>
>>>
>>>>         { &controls::draft::NoiseReductionMode, ControlInfo(controls::draft::NoiseReductionModeValues) }
>>>>  };
>>>>
>>>> @@ -463,10 +463,11 @@ int IPARPi::configure(const IPACameraSensorInfo &sensorInfo,
>>>>         const Duration maxSensorFrameDuration = mode_.maxFrameLength * mode_.lineLength;
>>>>         ctrlMap[&controls::FrameDurationLimits] =
>>>>                 ControlInfo(static_cast<int64_t>(minSensorFrameDuration.get<std::micro>()),
>>>> -                           static_cast<int64_t>(maxSensorFrameDuration.get<std::micro>()));
>>>> +                           static_cast<int64_t>(maxSensorFrameDuration.get<std::micro>()),
>>>> +                           int64_t(0));
>>>
>>> This is smaller than the minimum. Perhaps at least here during configure
>>> it could be set to something more specific about the camera mode
>>> capability - or something that is more 'default' like 30 FPS ? (33333 ?)
>>>
>>>>
>>>>         ctrlMap[&controls::AnalogueGain] =
>>>> -               ControlInfo(1.0f, static_cast<float>(helper_->gain(maxSensorGainCode_)));
>>>> +               ControlInfo(1.0f, static_cast<float>(helper_->gain(maxSensorGainCode_)), 0.0f);
>>>
>>> Less than minimum. I think 1.0f makes sense here too as that's unity
>>> gain.
>>>
>>>>
>>>>         /*
>>>>          * Calculate the max exposure limit from the frame duration limit as V4L2
>>>> @@ -478,7 +479,8 @@ int IPARPi::configure(const IPACameraSensorInfo &sensorInfo,
>>>>
>>>>         ctrlMap[&controls::ExposureTime] =
>>>>                 ControlInfo(static_cast<int32_t>(helper_->exposure(exposureMin).get<std::micro>()),
>>>> -                           static_cast<int32_t>(maxShutter.get<std::micro>()));
>>>> +                           static_cast<int32_t>(maxShutter.get<std::micro>()),
>>>> +                           int32_t(0));
>>>
>>> I'm not sure an exposure time of 0 makes sense ?, but I'm not sure what
>>> a reasonable default is here either. Maybe something like the maximum of the frame
>>> duration, and maxShutter ?
>>>
>>>>
>>>>         result->controlInfo = ControlInfoMap(std::move(ctrlMap), controls::controls);
>>>>         return 0;
>>>> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
>>>> index 27b4212b..2121bfd2 100644
>>>> --- a/src/ipa/rkisp1/rkisp1.cpp
>>>> +++ b/src/ipa/rkisp1/rkisp1.cpp
>>>> @@ -91,12 +91,12 @@ namespace {
>>>>
>>>>  /* List of controls handled by the RkISP1 IPA */
>>>>  const ControlInfoMap::Map rkisp1Controls{
>>>> -       { &controls::AeEnable, ControlInfo(false, true) },
>>>> -       { &controls::AwbEnable, ControlInfo(false, true) },
>>>> +       { &controls::AeEnable, ControlInfo(false, true, false) },
>>>> +       { &controls::AwbEnable, ControlInfo(false, true, false) },
>>>
>>> Same issue as in RPi.
>>>
>>>
>>>>         { &controls::ColourGains, ControlInfo(0.0f, 3.996f, 1.0f) },
>>>> -       { &controls::Brightness, ControlInfo(-1.0f, 0.993f) },
>>>> -       { &controls::Contrast, ControlInfo(0.0f, 1.993f) },
>>>> -       { &controls::Saturation, ControlInfo(0.0f, 1.993f) },
>>>> +       { &controls::Brightness, ControlInfo(-1.0f, 0.993f, 0.0f) },
>>>> +       { &controls::Contrast, ControlInfo(0.0f, 1.993f, 0.0f) },
>>>> +       { &controls::Saturation, ControlInfo(0.0f, 1.993f, 0.0f) },
>>>
>>> I think 0.0 makes sense here though
>>>
>>>>         { &controls::Sharpness, ControlInfo(0.0f, 10.0f, 1.0f) },
>>>>         { &controls::draft::NoiseReductionMode, ControlInfo(controls::draft::NoiseReductionModeValues) },
>>>>  };
>>>> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
>>>> index 9df24603..a1bcfe49 100644
>>>> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
>>>> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
>>>> @@ -42,7 +42,7 @@ namespace libcamera {
>>>>  LOG_DEFINE_CATEGORY(IPU3)
>>>>
>>>>  static const ControlInfoMap::Map IPU3Controls = {
>>>> -       { &controls::draft::PipelineDepth, ControlInfo(2, 3) },
>>>> +       { &controls::draft::PipelineDepth, ControlInfo(2, 3, 0) },
>>>
>>>
>>> This needs looking at. The PipelineDepth isn't something the application
>>> can set. So a default doesn't makes sense, and I'm not sure a min/max
>>> does either.
>>>
>>>
>>>
>>>>  };
>>>>
>>>>  class IPU3CameraData : public Camera::Private
>>>> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
>>>> index e895584d..8fd7634d 100644
>>>> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
>>>> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
>>>> @@ -954,7 +954,7 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)
>>>>         /* Add the ScalerCrop control limits based on the current mode. */
>>>>         Rectangle ispMinCrop(data->ispMinCropSize_);
>>>>         ispMinCrop.scaleBy(data->sensorInfo_.analogCrop.size(), data->sensorInfo_.outputSize);
>>>> -       ctrlMap[&controls::ScalerCrop] = ControlInfo(ispMinCrop, Rectangle(data->sensorInfo_.analogCrop.size()));
>>>> +       ctrlMap[&controls::ScalerCrop] = ControlInfo(ispMinCrop, Rectangle(data->sensorInfo_.analogCrop.size()), ispMinCrop);
>>>
>>> I think a ScalerCrop would probably expect a default of the largest size
>>> possible, not the smallest ? (At least if I were to think about
>>> 'resetting' the ScalerCrop that is).
>>>
>>> I suspect forcing a default value might be better than implicitly
>>> leaving it zero - but it means we need to give more thought to what the
>>> defaults mean or represent - or provide a way for the default to be
>>> 'unset' if it's not appropriate to have one.
>>>
>>> --
>>> Kieran
>>>
>>>
>>>>
>>>>         data->controlInfo_ = ControlInfoMap(std::move(ctrlMap), result.controlInfo.idmap());
>>>>
>>>> --
>>>> 2.34.1
>>>>
Christian Rauch Sept. 12, 2022, 7:37 p.m. UTC | #6 
Hi,

Not sure what the "proper" way is to close or retract proposed patches
on mailing lists. I want to mark this patch series, and a couple of
previous patches touching the default values, as "obsolete", such that
they do not appear on patchwork anymore and are not considered for
review/merging.

Best,
Christian


Am 29.08.22 um 22:32 schrieb Christian Rauch via libcamera-devel:
> Hi Kieran,
>
> I replaced the [1/2] patch in this set with a new one ("libcamera:
> control: initialise control info to ControlTypeNone by default") that
> default constructs the min/max/def ControlValue instead of setting it to
> 0. This allows explicitly checking for 'ControlTypeNone' instead of
> relying on 0 as an implicit representation.
>
> Best,
> Christian
>
>
> Am 23.08.22 um 01:01 schrieb Kieran Bingham:
>> Quoting Christian Rauch via libcamera-devel (2022-08-22 23:23:45)
>>> Hi Kieran,
>>>
>>> Regarding all your comments, I simply replaced the "default" 0 for "def"
>>> from the old constructor with explicitly setting it 0 (or its binary
>>> counterpart 'false') to match the types. If 0 is the wrong "def" value
>>> for those controls, then they were already wrong before :-) They were
>>> just implicitly wrong. Now you have to explicitly specify a "def" value,
>>> and I kept 0 for backward compatibility.
>>
>> Yes, they may have been implicitly incorrect before, but we shouldn't
>> set them to be explictly incorrect. So this proposed series means we
>> need to consider each one carefully.
>>
>> As mentioned though, I think we need to work out if all controls make
>> sense to have a default value, and if not - how to express that it isn't
>> available. I suspect that previously the omision of a default value was
>> the expression that the default was 'redundant' (or unnecessary), thus
>> by making it required, you need to explicitly set each one.
>>
>> --
>> Kieran
>>
>>
>>
>>> Feel free to change the default values. You should also be able to merge
>>> this patch (2/2) without the first (1/2).
>>>
>>> Best,
>>> Christian
>>>
>>>
>>> Am 22.08.22 um 23:52 schrieb Kieran Bingham:
>>>> Quoting Christian Rauch via libcamera-devel (2022-08-21 13:43:43)
>>>>> Explicitly set the default value for all ControlInfo to 0, false or minimum.
>>>>>
>>>>> Signed-off-by: Christian Rauch <Rauch.Christian@gmx.de>
>>>>> ---
>>>>>  src/ipa/raspberrypi/raspberrypi.cpp           | 22 ++++++++++---------
>>>>>  src/ipa/rkisp1/rkisp1.cpp                     | 10 ++++-----
>>>>>  src/libcamera/pipeline/ipu3/ipu3.cpp          |  2 +-
>>>>>  .../pipeline/raspberrypi/raspberrypi.cpp      |  2 +-
>>>>>  4 files changed, 19 insertions(+), 17 deletions(-)
>>>>>
>>>>> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
>>>>> index 69c73f8c..c6360a51 100644
>>>>> --- a/src/ipa/raspberrypi/raspberrypi.cpp
>>>>> +++ b/src/ipa/raspberrypi/raspberrypi.cpp
>>>>> @@ -74,23 +74,23 @@ constexpr Duration controllerMinFrameDuration = 1.0s / 30.0;
>>>>>
>>>>>  /* List of controls handled by the Raspberry Pi IPA */
>>>>>  static const ControlInfoMap::Map ipaControls{
>>>>> -       { &controls::AeEnable, ControlInfo(false, true) },
>>>>> -       { &controls::ExposureTime, ControlInfo(0, 66666) },
>>>>> -       { &controls::AnalogueGain, ControlInfo(1.0f, 16.0f) },
>>>>> +       { &controls::AeEnable, ControlInfo(false, true, false) },
>>>>
>>>> I would expect most of the time AutoExposure would be enabled ... so a
>>>> default of false seems misleading... Yet - perhaps that's what the 0 was
>>>> already implying? So - does that mean a default is unreasonable here?
>>>>
>>>>
>>>>> +       { &controls::ExposureTime, ControlInfo(0, 66666, 0) },
>>>>> +       { &controls::AnalogueGain, ControlInfo(1.0f, 16.0f, 0.0f) },
>>>>
>>>> This default is less than the minimum.
>>>>
>>>>
>>>>>         { &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::AwbEnable, ControlInfo(false, true) },
>>>>> -       { &controls::ColourGains, ControlInfo(0.0f, 32.0f) },
>>>>> +       { &controls::AwbEnable, ControlInfo(false, true, false) },
>>>>
>>>> Same as AeEnable.... it doesn't seem reasonble to say the default is
>>>> disabled? I think applications probably expect the default IPA behaviour
>>>> to have AE/AWB enabled, unless it is disabled explicitly ?
>>>>
>>>>
>>>>> +       { &controls::ColourGains, ControlInfo(0.0f, 32.0f, 0.0f) },
>>>>>         { &controls::AwbMode, ControlInfo(controls::AwbModeValues) },
>>>>>         { &controls::Brightness, ControlInfo(-1.0f, 1.0f, 0.0f) },
>>>>>         { &controls::Contrast, ControlInfo(0.0f, 32.0f, 1.0f) },
>>>>>         { &controls::Saturation, ControlInfo(0.0f, 32.0f, 1.0f) },
>>>>>         { &controls::Sharpness, ControlInfo(0.0f, 16.0f, 1.0f) },
>>>>> -       { &controls::ColourCorrectionMatrix, ControlInfo(-16.0f, 16.0f) },
>>>>> +       { &controls::ColourCorrectionMatrix, ControlInfo(-16.0f, 16.0f, 0.0f) },
>>>>>         { &controls::ScalerCrop, ControlInfo(Rectangle{}, Rectangle(65535, 65535, 65535, 65535), Rectangle{}) },
>>>>> -       { &controls::FrameDurationLimits, ControlInfo(INT64_C(33333), INT64_C(120000)) },
>>>>> +       { &controls::FrameDurationLimits, ControlInfo(INT64_C(33333), INT64_C(120000), INT64_C(0)) },
>>>>
>>>> Again, this is less than the minimum. A default should probably be
>>>> specific to the mode of the sensor too ?
>>>>
>>>>
>>>>>         { &controls::draft::NoiseReductionMode, ControlInfo(controls::draft::NoiseReductionModeValues) }
>>>>>  };
>>>>>
>>>>> @@ -463,10 +463,11 @@ int IPARPi::configure(const IPACameraSensorInfo &sensorInfo,
>>>>>         const Duration maxSensorFrameDuration = mode_.maxFrameLength * mode_.lineLength;
>>>>>         ctrlMap[&controls::FrameDurationLimits] =
>>>>>                 ControlInfo(static_cast<int64_t>(minSensorFrameDuration.get<std::micro>()),
>>>>> -                           static_cast<int64_t>(maxSensorFrameDuration.get<std::micro>()));
>>>>> +                           static_cast<int64_t>(maxSensorFrameDuration.get<std::micro>()),
>>>>> +                           int64_t(0));
>>>>
>>>> This is smaller than the minimum. Perhaps at least here during configure
>>>> it could be set to something more specific about the camera mode
>>>> capability - or something that is more 'default' like 30 FPS ? (33333 ?)
>>>>
>>>>>
>>>>>         ctrlMap[&controls::AnalogueGain] =
>>>>> -               ControlInfo(1.0f, static_cast<float>(helper_->gain(maxSensorGainCode_)));
>>>>> +               ControlInfo(1.0f, static_cast<float>(helper_->gain(maxSensorGainCode_)), 0.0f);
>>>>
>>>> Less than minimum. I think 1.0f makes sense here too as that's unity
>>>> gain.
>>>>
>>>>>
>>>>>         /*
>>>>>          * Calculate the max exposure limit from the frame duration limit as V4L2
>>>>> @@ -478,7 +479,8 @@ int IPARPi::configure(const IPACameraSensorInfo &sensorInfo,
>>>>>
>>>>>         ctrlMap[&controls::ExposureTime] =
>>>>>                 ControlInfo(static_cast<int32_t>(helper_->exposure(exposureMin).get<std::micro>()),
>>>>> -                           static_cast<int32_t>(maxShutter.get<std::micro>()));
>>>>> +                           static_cast<int32_t>(maxShutter.get<std::micro>()),
>>>>> +                           int32_t(0));
>>>>
>>>> I'm not sure an exposure time of 0 makes sense ?, but I'm not sure what
>>>> a reasonable default is here either. Maybe something like the maximum of the frame
>>>> duration, and maxShutter ?
>>>>
>>>>>
>>>>>         result->controlInfo = ControlInfoMap(std::move(ctrlMap), controls::controls);
>>>>>         return 0;
>>>>> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
>>>>> index 27b4212b..2121bfd2 100644
>>>>> --- a/src/ipa/rkisp1/rkisp1.cpp
>>>>> +++ b/src/ipa/rkisp1/rkisp1.cpp
>>>>> @@ -91,12 +91,12 @@ namespace {
>>>>>
>>>>>  /* List of controls handled by the RkISP1 IPA */
>>>>>  const ControlInfoMap::Map rkisp1Controls{
>>>>> -       { &controls::AeEnable, ControlInfo(false, true) },
>>>>> -       { &controls::AwbEnable, ControlInfo(false, true) },
>>>>> +       { &controls::AeEnable, ControlInfo(false, true, false) },
>>>>> +       { &controls::AwbEnable, ControlInfo(false, true, false) },
>>>>
>>>> Same issue as in RPi.
>>>>
>>>>
>>>>>         { &controls::ColourGains, ControlInfo(0.0f, 3.996f, 1.0f) },
>>>>> -       { &controls::Brightness, ControlInfo(-1.0f, 0.993f) },
>>>>> -       { &controls::Contrast, ControlInfo(0.0f, 1.993f) },
>>>>> -       { &controls::Saturation, ControlInfo(0.0f, 1.993f) },
>>>>> +       { &controls::Brightness, ControlInfo(-1.0f, 0.993f, 0.0f) },
>>>>> +       { &controls::Contrast, ControlInfo(0.0f, 1.993f, 0.0f) },
>>>>> +       { &controls::Saturation, ControlInfo(0.0f, 1.993f, 0.0f) },
>>>>
>>>> I think 0.0 makes sense here though
>>>>
>>>>>         { &controls::Sharpness, ControlInfo(0.0f, 10.0f, 1.0f) },
>>>>>         { &controls::draft::NoiseReductionMode, ControlInfo(controls::draft::NoiseReductionModeValues) },
>>>>>  };
>>>>> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
>>>>> index 9df24603..a1bcfe49 100644
>>>>> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
>>>>> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
>>>>> @@ -42,7 +42,7 @@ namespace libcamera {
>>>>>  LOG_DEFINE_CATEGORY(IPU3)
>>>>>
>>>>>  static const ControlInfoMap::Map IPU3Controls = {
>>>>> -       { &controls::draft::PipelineDepth, ControlInfo(2, 3) },
>>>>> +       { &controls::draft::PipelineDepth, ControlInfo(2, 3, 0) },
>>>>
>>>>
>>>> This needs looking at. The PipelineDepth isn't something the application
>>>> can set. So a default doesn't makes sense, and I'm not sure a min/max
>>>> does either.
>>>>
>>>>
>>>>
>>>>>  };
>>>>>
>>>>>  class IPU3CameraData : public Camera::Private
>>>>> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
>>>>> index e895584d..8fd7634d 100644
>>>>> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
>>>>> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
>>>>> @@ -954,7 +954,7 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)
>>>>>         /* Add the ScalerCrop control limits based on the current mode. */
>>>>>         Rectangle ispMinCrop(data->ispMinCropSize_);
>>>>>         ispMinCrop.scaleBy(data->sensorInfo_.analogCrop.size(), data->sensorInfo_.outputSize);
>>>>> -       ctrlMap[&controls::ScalerCrop] = ControlInfo(ispMinCrop, Rectangle(data->sensorInfo_.analogCrop.size()));
>>>>> +       ctrlMap[&controls::ScalerCrop] = ControlInfo(ispMinCrop, Rectangle(data->sensorInfo_.analogCrop.size()), ispMinCrop);
>>>>
>>>> I think a ScalerCrop would probably expect a default of the largest size
>>>> possible, not the smallest ? (At least if I were to think about
>>>> 'resetting' the ScalerCrop that is).
>>>>
>>>> I suspect forcing a default value might be better than implicitly
>>>> leaving it zero - but it means we need to give more thought to what the
>>>> defaults mean or represent - or provide a way for the default to be
>>>> 'unset' if it's not appropriate to have one.
>>>>
>>>> --
>>>> Kieran
>>>>
>>>>
>>>>>
>>>>>         data->controlInfo_ = ControlInfoMap(std::move(ctrlMap), result.controlInfo.idmap());
>>>>>
>>>>> --
>>>>> 2.34.1
>>>>>
Nicolas Dufresne via libcamera-devel Sept. 16, 2022, 3:12 a.m. UTC | #7 
On Mon, Sep 12, 2022 at 09:37:21PM +0200, Christian Rauch via libcamera-devel wrote:
> Hi,
> 
> Not sure what the "proper" way is to close or retract proposed patches
> on mailing lists. I want to mark this patch series, and a couple of
> previous patches touching the default values, as "obsolete", such that
> they do not appear on patchwork anymore and are not considered for
> review/merging.

I've marked these as "superseded" on patchwork.


Paul

> 
> 
> Am 29.08.22 um 22:32 schrieb Christian Rauch via libcamera-devel:
> > Hi Kieran,
> >
> > I replaced the [1/2] patch in this set with a new one ("libcamera:
> > control: initialise control info to ControlTypeNone by default") that
> > default constructs the min/max/def ControlValue instead of setting it to
> > 0. This allows explicitly checking for 'ControlTypeNone' instead of
> > relying on 0 as an implicit representation.
> >
> > Best,
> > Christian
> >
> >
> > Am 23.08.22 um 01:01 schrieb Kieran Bingham:
> >> Quoting Christian Rauch via libcamera-devel (2022-08-22 23:23:45)
> >>> Hi Kieran,
> >>>
> >>> Regarding all your comments, I simply replaced the "default" 0 for "def"
> >>> from the old constructor with explicitly setting it 0 (or its binary
> >>> counterpart 'false') to match the types. If 0 is the wrong "def" value
> >>> for those controls, then they were already wrong before :-) They were
> >>> just implicitly wrong. Now you have to explicitly specify a "def" value,
> >>> and I kept 0 for backward compatibility.
> >>
> >> Yes, they may have been implicitly incorrect before, but we shouldn't
> >> set them to be explictly incorrect. So this proposed series means we
> >> need to consider each one carefully.
> >>
> >> As mentioned though, I think we need to work out if all controls make
> >> sense to have a default value, and if not - how to express that it isn't
> >> available. I suspect that previously the omision of a default value was
> >> the expression that the default was 'redundant' (or unnecessary), thus
> >> by making it required, you need to explicitly set each one.
> >>
> >> --
> >> Kieran
> >>
> >>
> >>
> >>> Feel free to change the default values. You should also be able to merge
> >>> this patch (2/2) without the first (1/2).
> >>>
> >>> Best,
> >>> Christian
> >>>
> >>>
> >>> Am 22.08.22 um 23:52 schrieb Kieran Bingham:
> >>>> Quoting Christian Rauch via libcamera-devel (2022-08-21 13:43:43)
> >>>>> Explicitly set the default value for all ControlInfo to 0, false or minimum.
> >>>>>
> >>>>> Signed-off-by: Christian Rauch <Rauch.Christian@gmx.de>
> >>>>> ---
> >>>>>  src/ipa/raspberrypi/raspberrypi.cpp           | 22 ++++++++++---------
> >>>>>  src/ipa/rkisp1/rkisp1.cpp                     | 10 ++++-----
> >>>>>  src/libcamera/pipeline/ipu3/ipu3.cpp          |  2 +-
> >>>>>  .../pipeline/raspberrypi/raspberrypi.cpp      |  2 +-
> >>>>>  4 files changed, 19 insertions(+), 17 deletions(-)
> >>>>>
> >>>>> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
> >>>>> index 69c73f8c..c6360a51 100644
> >>>>> --- a/src/ipa/raspberrypi/raspberrypi.cpp
> >>>>> +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> >>>>> @@ -74,23 +74,23 @@ constexpr Duration controllerMinFrameDuration = 1.0s / 30.0;
> >>>>>
> >>>>>  /* List of controls handled by the Raspberry Pi IPA */
> >>>>>  static const ControlInfoMap::Map ipaControls{
> >>>>> -       { &controls::AeEnable, ControlInfo(false, true) },
> >>>>> -       { &controls::ExposureTime, ControlInfo(0, 66666) },
> >>>>> -       { &controls::AnalogueGain, ControlInfo(1.0f, 16.0f) },
> >>>>> +       { &controls::AeEnable, ControlInfo(false, true, false) },
> >>>>
> >>>> I would expect most of the time AutoExposure would be enabled ... so a
> >>>> default of false seems misleading... Yet - perhaps that's what the 0 was
> >>>> already implying? So - does that mean a default is unreasonable here?
> >>>>
> >>>>
> >>>>> +       { &controls::ExposureTime, ControlInfo(0, 66666, 0) },
> >>>>> +       { &controls::AnalogueGain, ControlInfo(1.0f, 16.0f, 0.0f) },
> >>>>
> >>>> This default is less than the minimum.
> >>>>
> >>>>
> >>>>>         { &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::AwbEnable, ControlInfo(false, true) },
> >>>>> -       { &controls::ColourGains, ControlInfo(0.0f, 32.0f) },
> >>>>> +       { &controls::AwbEnable, ControlInfo(false, true, false) },
> >>>>
> >>>> Same as AeEnable.... it doesn't seem reasonble to say the default is
> >>>> disabled? I think applications probably expect the default IPA behaviour
> >>>> to have AE/AWB enabled, unless it is disabled explicitly ?
> >>>>
> >>>>
> >>>>> +       { &controls::ColourGains, ControlInfo(0.0f, 32.0f, 0.0f) },
> >>>>>         { &controls::AwbMode, ControlInfo(controls::AwbModeValues) },
> >>>>>         { &controls::Brightness, ControlInfo(-1.0f, 1.0f, 0.0f) },
> >>>>>         { &controls::Contrast, ControlInfo(0.0f, 32.0f, 1.0f) },
> >>>>>         { &controls::Saturation, ControlInfo(0.0f, 32.0f, 1.0f) },
> >>>>>         { &controls::Sharpness, ControlInfo(0.0f, 16.0f, 1.0f) },
> >>>>> -       { &controls::ColourCorrectionMatrix, ControlInfo(-16.0f, 16.0f) },
> >>>>> +       { &controls::ColourCorrectionMatrix, ControlInfo(-16.0f, 16.0f, 0.0f) },
> >>>>>         { &controls::ScalerCrop, ControlInfo(Rectangle{}, Rectangle(65535, 65535, 65535, 65535), Rectangle{}) },
> >>>>> -       { &controls::FrameDurationLimits, ControlInfo(INT64_C(33333), INT64_C(120000)) },
> >>>>> +       { &controls::FrameDurationLimits, ControlInfo(INT64_C(33333), INT64_C(120000), INT64_C(0)) },
> >>>>
> >>>> Again, this is less than the minimum. A default should probably be
> >>>> specific to the mode of the sensor too ?
> >>>>
> >>>>
> >>>>>         { &controls::draft::NoiseReductionMode, ControlInfo(controls::draft::NoiseReductionModeValues) }
> >>>>>  };
> >>>>>
> >>>>> @@ -463,10 +463,11 @@ int IPARPi::configure(const IPACameraSensorInfo &sensorInfo,
> >>>>>         const Duration maxSensorFrameDuration = mode_.maxFrameLength * mode_.lineLength;
> >>>>>         ctrlMap[&controls::FrameDurationLimits] =
> >>>>>                 ControlInfo(static_cast<int64_t>(minSensorFrameDuration.get<std::micro>()),
> >>>>> -                           static_cast<int64_t>(maxSensorFrameDuration.get<std::micro>()));
> >>>>> +                           static_cast<int64_t>(maxSensorFrameDuration.get<std::micro>()),
> >>>>> +                           int64_t(0));
> >>>>
> >>>> This is smaller than the minimum. Perhaps at least here during configure
> >>>> it could be set to something more specific about the camera mode
> >>>> capability - or something that is more 'default' like 30 FPS ? (33333 ?)
> >>>>
> >>>>>
> >>>>>         ctrlMap[&controls::AnalogueGain] =
> >>>>> -               ControlInfo(1.0f, static_cast<float>(helper_->gain(maxSensorGainCode_)));
> >>>>> +               ControlInfo(1.0f, static_cast<float>(helper_->gain(maxSensorGainCode_)), 0.0f);
> >>>>
> >>>> Less than minimum. I think 1.0f makes sense here too as that's unity
> >>>> gain.
> >>>>
> >>>>>
> >>>>>         /*
> >>>>>          * Calculate the max exposure limit from the frame duration limit as V4L2
> >>>>> @@ -478,7 +479,8 @@ int IPARPi::configure(const IPACameraSensorInfo &sensorInfo,
> >>>>>
> >>>>>         ctrlMap[&controls::ExposureTime] =
> >>>>>                 ControlInfo(static_cast<int32_t>(helper_->exposure(exposureMin).get<std::micro>()),
> >>>>> -                           static_cast<int32_t>(maxShutter.get<std::micro>()));
> >>>>> +                           static_cast<int32_t>(maxShutter.get<std::micro>()),
> >>>>> +                           int32_t(0));
> >>>>
> >>>> I'm not sure an exposure time of 0 makes sense ?, but I'm not sure what
> >>>> a reasonable default is here either. Maybe something like the maximum of the frame
> >>>> duration, and maxShutter ?
> >>>>
> >>>>>
> >>>>>         result->controlInfo = ControlInfoMap(std::move(ctrlMap), controls::controls);
> >>>>>         return 0;
> >>>>> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
> >>>>> index 27b4212b..2121bfd2 100644
> >>>>> --- a/src/ipa/rkisp1/rkisp1.cpp
> >>>>> +++ b/src/ipa/rkisp1/rkisp1.cpp
> >>>>> @@ -91,12 +91,12 @@ namespace {
> >>>>>
> >>>>>  /* List of controls handled by the RkISP1 IPA */
> >>>>>  const ControlInfoMap::Map rkisp1Controls{
> >>>>> -       { &controls::AeEnable, ControlInfo(false, true) },
> >>>>> -       { &controls::AwbEnable, ControlInfo(false, true) },
> >>>>> +       { &controls::AeEnable, ControlInfo(false, true, false) },
> >>>>> +       { &controls::AwbEnable, ControlInfo(false, true, false) },
> >>>>
> >>>> Same issue as in RPi.
> >>>>
> >>>>
> >>>>>         { &controls::ColourGains, ControlInfo(0.0f, 3.996f, 1.0f) },
> >>>>> -       { &controls::Brightness, ControlInfo(-1.0f, 0.993f) },
> >>>>> -       { &controls::Contrast, ControlInfo(0.0f, 1.993f) },
> >>>>> -       { &controls::Saturation, ControlInfo(0.0f, 1.993f) },
> >>>>> +       { &controls::Brightness, ControlInfo(-1.0f, 0.993f, 0.0f) },
> >>>>> +       { &controls::Contrast, ControlInfo(0.0f, 1.993f, 0.0f) },
> >>>>> +       { &controls::Saturation, ControlInfo(0.0f, 1.993f, 0.0f) },
> >>>>
> >>>> I think 0.0 makes sense here though
> >>>>
> >>>>>         { &controls::Sharpness, ControlInfo(0.0f, 10.0f, 1.0f) },
> >>>>>         { &controls::draft::NoiseReductionMode, ControlInfo(controls::draft::NoiseReductionModeValues) },
> >>>>>  };
> >>>>> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> >>>>> index 9df24603..a1bcfe49 100644
> >>>>> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> >>>>> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> >>>>> @@ -42,7 +42,7 @@ namespace libcamera {
> >>>>>  LOG_DEFINE_CATEGORY(IPU3)
> >>>>>
> >>>>>  static const ControlInfoMap::Map IPU3Controls = {
> >>>>> -       { &controls::draft::PipelineDepth, ControlInfo(2, 3) },
> >>>>> +       { &controls::draft::PipelineDepth, ControlInfo(2, 3, 0) },
> >>>>
> >>>>
> >>>> This needs looking at. The PipelineDepth isn't something the application
> >>>> can set. So a default doesn't makes sense, and I'm not sure a min/max
> >>>> does either.
> >>>>
> >>>>
> >>>>
> >>>>>  };
> >>>>>
> >>>>>  class IPU3CameraData : public Camera::Private
> >>>>> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> >>>>> index e895584d..8fd7634d 100644
> >>>>> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> >>>>> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> >>>>> @@ -954,7 +954,7 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)
> >>>>>         /* Add the ScalerCrop control limits based on the current mode. */
> >>>>>         Rectangle ispMinCrop(data->ispMinCropSize_);
> >>>>>         ispMinCrop.scaleBy(data->sensorInfo_.analogCrop.size(), data->sensorInfo_.outputSize);
> >>>>> -       ctrlMap[&controls::ScalerCrop] = ControlInfo(ispMinCrop, Rectangle(data->sensorInfo_.analogCrop.size()));
> >>>>> +       ctrlMap[&controls::ScalerCrop] = ControlInfo(ispMinCrop, Rectangle(data->sensorInfo_.analogCrop.size()), ispMinCrop);
> >>>>
> >>>> I think a ScalerCrop would probably expect a default of the largest size
> >>>> possible, not the smallest ? (At least if I were to think about
> >>>> 'resetting' the ScalerCrop that is).
> >>>>
> >>>> I suspect forcing a default value might be better than implicitly
> >>>> leaving it zero - but it means we need to give more thought to what the
> >>>> defaults mean or represent - or provide a way for the default to be
> >>>> 'unset' if it's not appropriate to have one.
> >>>>
> >>>> --
> >>>> Kieran
> >>>>
> >>>>
> >>>>>
> >>>>>         data->controlInfo_ = ControlInfoMap(std::move(ctrlMap), result.controlInfo.idmap());
> >>>>>
> >>>>> --
> >>>>> 2.34.1
> >>>>>

Patch
diff mbox series 

diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
index 69c73f8c..c6360a51 100644
--- a/src/ipa/raspberrypi/raspberrypi.cpp
+++ b/src/ipa/raspberrypi/raspberrypi.cpp
@@ -74,23 +74,23 @@  constexpr Duration controllerMinFrameDuration = 1.0s / 30.0;

 /* List of controls handled by the Raspberry Pi IPA */
 static const ControlInfoMap::Map ipaControls{
-	{ &controls::AeEnable, ControlInfo(false, true) },
-	{ &controls::ExposureTime, ControlInfo(0, 66666) },
-	{ &controls::AnalogueGain, ControlInfo(1.0f, 16.0f) },
+	{ &controls::AeEnable, ControlInfo(false, true, false) },
+	{ &controls::ExposureTime, ControlInfo(0, 66666, 0) },
+	{ &controls::AnalogueGain, ControlInfo(1.0f, 16.0f, 0.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::AwbEnable, ControlInfo(false, true) },
-	{ &controls::ColourGains, ControlInfo(0.0f, 32.0f) },
+	{ &controls::AwbEnable, ControlInfo(false, true, false) },
+	{ &controls::ColourGains, ControlInfo(0.0f, 32.0f, 0.0f) },
 	{ &controls::AwbMode, ControlInfo(controls::AwbModeValues) },
 	{ &controls::Brightness, ControlInfo(-1.0f, 1.0f, 0.0f) },
 	{ &controls::Contrast, ControlInfo(0.0f, 32.0f, 1.0f) },
 	{ &controls::Saturation, ControlInfo(0.0f, 32.0f, 1.0f) },
 	{ &controls::Sharpness, ControlInfo(0.0f, 16.0f, 1.0f) },
-	{ &controls::ColourCorrectionMatrix, ControlInfo(-16.0f, 16.0f) },
+	{ &controls::ColourCorrectionMatrix, ControlInfo(-16.0f, 16.0f, 0.0f) },
 	{ &controls::ScalerCrop, ControlInfo(Rectangle{}, Rectangle(65535, 65535, 65535, 65535), Rectangle{}) },
-	{ &controls::FrameDurationLimits, ControlInfo(INT64_C(33333), INT64_C(120000)) },
+	{ &controls::FrameDurationLimits, ControlInfo(INT64_C(33333), INT64_C(120000), INT64_C(0)) },
 	{ &controls::draft::NoiseReductionMode, ControlInfo(controls::draft::NoiseReductionModeValues) }
 };

@@ -463,10 +463,11 @@  int IPARPi::configure(const IPACameraSensorInfo &sensorInfo,
 	const Duration maxSensorFrameDuration = mode_.maxFrameLength * mode_.lineLength;
 	ctrlMap[&controls::FrameDurationLimits] =
 		ControlInfo(static_cast<int64_t>(minSensorFrameDuration.get<std::micro>()),
-			    static_cast<int64_t>(maxSensorFrameDuration.get<std::micro>()));
+			    static_cast<int64_t>(maxSensorFrameDuration.get<std::micro>()),
+			    int64_t(0));

 	ctrlMap[&controls::AnalogueGain] =
-		ControlInfo(1.0f, static_cast<float>(helper_->gain(maxSensorGainCode_)));
+		ControlInfo(1.0f, static_cast<float>(helper_->gain(maxSensorGainCode_)), 0.0f);

 	/*
 	 * Calculate the max exposure limit from the frame duration limit as V4L2
@@ -478,7 +479,8 @@  int IPARPi::configure(const IPACameraSensorInfo &sensorInfo,

 	ctrlMap[&controls::ExposureTime] =
 		ControlInfo(static_cast<int32_t>(helper_->exposure(exposureMin).get<std::micro>()),
-			    static_cast<int32_t>(maxShutter.get<std::micro>()));
+			    static_cast<int32_t>(maxShutter.get<std::micro>()),
+			    int32_t(0));

 	result->controlInfo = ControlInfoMap(std::move(ctrlMap), controls::controls);
 	return 0;
diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
index 27b4212b..2121bfd2 100644
--- a/src/ipa/rkisp1/rkisp1.cpp
+++ b/src/ipa/rkisp1/rkisp1.cpp
@@ -91,12 +91,12 @@  namespace {

 /* List of controls handled by the RkISP1 IPA */
 const ControlInfoMap::Map rkisp1Controls{
-	{ &controls::AeEnable, ControlInfo(false, true) },
-	{ &controls::AwbEnable, ControlInfo(false, true) },
+	{ &controls::AeEnable, ControlInfo(false, true, false) },
+	{ &controls::AwbEnable, ControlInfo(false, true, false) },
 	{ &controls::ColourGains, ControlInfo(0.0f, 3.996f, 1.0f) },
-	{ &controls::Brightness, ControlInfo(-1.0f, 0.993f) },
-	{ &controls::Contrast, ControlInfo(0.0f, 1.993f) },
-	{ &controls::Saturation, ControlInfo(0.0f, 1.993f) },
+	{ &controls::Brightness, ControlInfo(-1.0f, 0.993f, 0.0f) },
+	{ &controls::Contrast, ControlInfo(0.0f, 1.993f, 0.0f) },
+	{ &controls::Saturation, ControlInfo(0.0f, 1.993f, 0.0f) },
 	{ &controls::Sharpness, ControlInfo(0.0f, 10.0f, 1.0f) },
 	{ &controls::draft::NoiseReductionMode, ControlInfo(controls::draft::NoiseReductionModeValues) },
 };
diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
index 9df24603..a1bcfe49 100644
--- a/src/libcamera/pipeline/ipu3/ipu3.cpp
+++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
@@ -42,7 +42,7 @@  namespace libcamera {
 LOG_DEFINE_CATEGORY(IPU3)

 static const ControlInfoMap::Map IPU3Controls = {
-	{ &controls::draft::PipelineDepth, ControlInfo(2, 3) },
+	{ &controls::draft::PipelineDepth, ControlInfo(2, 3, 0) },
 };

 class IPU3CameraData : public Camera::Private
diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
index e895584d..8fd7634d 100644
--- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
+++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
@@ -954,7 +954,7 @@  int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)
 	/* Add the ScalerCrop control limits based on the current mode. */
 	Rectangle ispMinCrop(data->ispMinCropSize_);
 	ispMinCrop.scaleBy(data->sensorInfo_.analogCrop.size(), data->sensorInfo_.outputSize);
-	ctrlMap[&controls::ScalerCrop] = ControlInfo(ispMinCrop, Rectangle(data->sensorInfo_.analogCrop.size()));
+	ctrlMap[&controls::ScalerCrop] = ControlInfo(ispMinCrop, Rectangle(data->sensorInfo_.analogCrop.size()), ispMinCrop);

 	data->controlInfo_ = ControlInfoMap(std::move(ctrlMap), result.controlInfo.idmap());