[libcamera-devel] [PATCH 2/2] libcamera: control: explicitly define default control values
Kieran Bingham
kieran.bingham at ideasonboard.com
Tue Aug 23 01:01:12 CEST 2022
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