[libcamera-devel] [PATCH 2/2] libcamera: control: explicitly define default control values
paul.elder at ideasonboard.com
paul.elder at ideasonboard.com
Fri Sep 16 05:12:31 CEST 2022
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 at 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
> >>>>>
More information about the libcamera-devel
mailing list