[libcamera-devel] [PATCH v2 2/2] ipa: rpi: Handle controls for mono variant sensors
Naushir Patuck
naush at raspberrypi.com
Mon Jun 5 09:52:15 CEST 2023
Hi Laurent,
On Mon, 5 Jun 2023 at 08:43, Laurent Pinchart
<laurent.pinchart at ideasonboard.com> wrote:
>
> Hi Naush,
>
> On Mon, Jun 05, 2023 at 08:20:28AM +0100, Naushir Patuck wrote:
> > On Fri, 2 Jun 2023 at 16:51, Laurent Pinchart wrote:
> > > On Fri, Jun 02, 2023 at 02:23:58PM +0100, Naushir Patuck wrote:
> > > > Do not advertise colour related controls (i.e. [A]WB, colour saturation)
> > > > in the ControlInfoMap of available controls returned out to the
> > > > application.
> > > >
> > > > Silently ignore these controls in the control handler in case applications
> > > > don't use the advertised ControlInfoMap to validate controls.
> > > >
> > > > As a drive-by, don't advertise controls::ColourCorrectionMatrix in the
> > > > ControlInfoMap as it is not handled by the IPA.
> > > >
> > > > Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
> > > > Reviewed-by: David Plowman <david.plowman at raspberrypi.com>
> > > > Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > > > ---
> > > > src/ipa/rpi/common/ipa_base.cpp | 35 ++++++++++++++++++++++++++++-----
> > > > src/ipa/rpi/common/ipa_base.h | 1 +
> > > > 2 files changed, 31 insertions(+), 5 deletions(-)
> > > >
> > > > diff --git a/src/ipa/rpi/common/ipa_base.cpp b/src/ipa/rpi/common/ipa_base.cpp
> > > > index db7a0eb3a1ca..813e01fa5cfd 100644
> > > > --- a/src/ipa/rpi/common/ipa_base.cpp
> > > > +++ b/src/ipa/rpi/common/ipa_base.cpp
> > > > @@ -12,6 +12,7 @@
> > > > #include <libcamera/base/log.h>
> > > > #include <libcamera/base/span.h>
> > > > #include <libcamera/control_ids.h>
> > > > +#include <libcamera/property_ids.h>
> > > >
> > > > #include "controller/af_algorithm.h"
> > > > #include "controller/af_status.h"
> > > > @@ -60,19 +61,22 @@ const ControlInfoMap::Map ipaControls{
> > > > { &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::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::ScalerCrop, ControlInfo(Rectangle{}, Rectangle(65535, 65535, 65535, 65535), Rectangle{}) },
> > > > { &controls::FrameDurationLimits, ControlInfo(INT64_C(33333), INT64_C(120000)) },
> > > > { &controls::draft::NoiseReductionMode, ControlInfo(controls::draft::NoiseReductionModeValues) }
> > > > };
> > > >
> > > > +/* IPA controls handled conditionally, if the sensor is not mono */
> > > > +const ControlInfoMap::Map ipaColourControls{
> > > > + { &controls::AwbEnable, ControlInfo(false, true) },
> > > > + { &controls::AwbMode, ControlInfo(controls::AwbModeValues) },
> > > > + { &controls::ColourGains, ControlInfo(0.0f, 32.0f) },
> > > > + { &controls::Saturation, ControlInfo(0.0f, 32.0f, 1.0f) },
> > > > +};
> > > > +
> > > > /* IPA controls handled conditionally, if the lens has a focus control */
> > > > const ControlInfoMap::Map ipaAfControls{
> > > > { &controls::AfMode, ControlInfo(controls::AfModeValues) },
> > > > @@ -220,6 +224,11 @@ int32_t IpaBase::configure(const IPACameraSensorInfo &sensorInfo, const ConfigPa
> > > > ControlInfo(static_cast<int32_t>(mode_.minShutter.get<std::micro>()),
> > > > static_cast<int32_t>(mode_.maxShutter.get<std::micro>()));
> > > >
> > > > + /* Declare colour processing related controls for non-mono sensors. */
> > > > + monoSensor_ = sensorInfo.cfaPattern == properties::draft::ColorFilterArrangementEnum::MONO;
> > > > + if (!monoSensor_)
> > > > + ctrlMap.merge(ControlInfoMap::Map(ipaColourControls));
> > > > +
> > >
> > > This means that the camera won't report the colour-related controls
> > > until it gets configured. Could we fix that by passing the IPASensorInfo
> > > to the init() function as well ?
> >
> > Yes this is true. I've always felt that the ControlInfoMap should not be
> > advertised at all during ipa->init(), since some controls limits (e.g.
> > AnalogueGain and ExposureTime) have invalid min/max/default values.
> > They only get correct values set after ipa->configure().
> >
> > Rather than passing IPASensorInfo to init(), I would probably prefer to entirely
> > remove setting up the ControlInfoMap there. What are your thoughts?
>
> This is an issue we'll need to tackle, and I don't know yet how to do
> so. Not reporting properties and controls until after configure() is an
> easy option, but it would make it more difficult for applications to
> figure out if a camera is suitable for their needs, or just adapt their
> behaviour based on the camera features. I don't want to drop reporting
> supported controls at init time completely until we have a proper,
> well-thought replacement.
Ok, I'll post an update with the IPASensorInfo passed into init() for now until
we have a better solution.
Regards,
Naush
>
> > > > /* Declare Autofocus controls, only if we have a controllable lens */
> > > > if (lensPresent_)
> > > > ctrlMap.merge(ControlInfoMap::Map(ipaAfControls));
> > > > @@ -780,6 +789,10 @@ void IpaBase::applyControls(const ControlList &controls)
> > > > }
> > > >
> > > > case controls::AWB_ENABLE: {
> > > > + /* Silently ignore this control for a mono sensor. */
> > > > + if (monoSensor_)
> > > > + break;
> > > > +
> > > > RPiController::AwbAlgorithm *awb = dynamic_cast<RPiController::AwbAlgorithm *>(
> > > > controller_.getAlgorithm("awb"));
> > > > if (!awb) {
> > > > @@ -799,6 +812,10 @@ void IpaBase::applyControls(const ControlList &controls)
> > > > }
> > > >
> > > > case controls::AWB_MODE: {
> > > > + /* Silently ignore this control for a mono sensor. */
> > > > + if (monoSensor_)
> > > > + break;
> > > > +
> > > > RPiController::AwbAlgorithm *awb = dynamic_cast<RPiController::AwbAlgorithm *>(
> > > > controller_.getAlgorithm("awb"));
> > > > if (!awb) {
> > > > @@ -819,6 +836,10 @@ void IpaBase::applyControls(const ControlList &controls)
> > > > }
> > > >
> > > > case controls::COLOUR_GAINS: {
> > > > + /* Silently ignore this control for a mono sensor. */
> > > > + if (monoSensor_)
> > > > + break;
> > > > +
> > > > auto gains = ctrl.second.get<Span<const float>>();
> > > > RPiController::AwbAlgorithm *awb = dynamic_cast<RPiController::AwbAlgorithm *>(
> > > > controller_.getAlgorithm("awb"));
> > > > @@ -867,6 +888,10 @@ void IpaBase::applyControls(const ControlList &controls)
> > > > }
> > > >
> > > > case controls::SATURATION: {
> > > > + /* Silently ignore this control for a mono sensor. */
> > > > + if (monoSensor_)
> > > > + break;
> > > > +
> > > > RPiController::CcmAlgorithm *ccm = dynamic_cast<RPiController::CcmAlgorithm *>(
> > > > controller_.getAlgorithm("ccm"));
> > > > if (!ccm) {
> > > > diff --git a/src/ipa/rpi/common/ipa_base.h b/src/ipa/rpi/common/ipa_base.h
> > > > index 6f9c46bb16b1..39d00760d012 100644
> > > > --- a/src/ipa/rpi/common/ipa_base.h
> > > > +++ b/src/ipa/rpi/common/ipa_base.h
> > > > @@ -87,6 +87,7 @@ private:
> > > > std::map<unsigned int, MappedFrameBuffer> buffers_;
> > > >
> > > > bool lensPresent_;
> > > > + bool monoSensor_;
> > > > ControlList libcameraMetadata_;
> > > >
> > > > std::array<RPiController::Metadata, numMetadataContexts> rpiMetadata_;
>
> --
> Regards,
>
> Laurent Pinchart
More information about the libcamera-devel
mailing list