[libcamera-devel] [PATCH v2 2/2] ipa: rpi: Handle controls for mono variant sensors
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Mon Jun 5 09:43:25 CEST 2023
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.
> > > /* 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