[libcamera-devel] [PATCH] pipeline: ipa: raspberrypi: Correctly report available controls
Naushir Patuck
naush at raspberrypi.com
Fri Jun 10 12:02:26 CEST 2022
On Fri, 10 Jun 2022 at 10:27, Naushir Patuck <naush at raspberrypi.com> wrote:
> Hi David,
>
> Thank you for your feedback.
>
> On Fri, 10 Jun 2022 at 09:04, David Plowman <david.plowman at raspberrypi.com>
> wrote:
>
>> Hi Naush
>>
>> Thanks very much for this patch. I'm very happy with the functionality
>> added here!
>>
>> On Thu, 9 Jun 2022 at 17:08, Laurent Pinchart via libcamera-devel
>> <libcamera-devel at lists.libcamera.org> wrote:
>> >
>> > Hi Naush,
>> >
>> > Thank you for the patch.
>> >
>> > On Thu, Jun 09, 2022 at 01:58:30PM +0100, Naushir Patuck via
>> libcamera-devel wrote:
>> > > The pipeline handler currently advertises a static ControlInfoMap to
>> specify the
>> > > available controls and their limits. Fix this limitation by having
>> the IPA
>> > > populate a ControlInfoMap during Camera::Configure() that is then
>> returned from
>> > > the pipeline handle. This will allow the ExposureTime, AnalogueGain,
>> and
>> > > FrameDurationLimits controls to advertise the correct limits for the
>> programmed
>> > > mode.
>> > >
>> > > Note that with this change, the ControlInfoMap provided by
>> Camera::Controls()
>> > > is only valid after a successful Camera::Configure() call.
>> >
>> > That's an issue, we need to initialize it earlier. You can update it at
>> > configure() time, but it has to bet set at init() time.
>> >
>> > > Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
>>
>> The only minor thought I had looking through this was whether we could
>> supply some better min/max values for the ScalerCrop (for example, all
>> zeroes wouldn't be valid), but I think that's a separate
>> discussion/patch.
>>
>
> I can add a patch that does this since I will be moving the ScalerCrop
> control
> setup to the pipeline handler. About setting the limits, presumably the
> max
> wants to be set to the same value as properties::ScalerCropMaximum, which
> in turn is the value of the "analgCrop" from the sensor. Does this mean
> that
> properties::ScalerCropMaximum might become redundant?
>
Oops, I got that wrong. properties::ScalerCropMaximum is the sensor crop of
visible pixels, i.e. the full resolution readout. I need the current mode
readout,
which might be different!
>
> Regards,
> Naush
>
>
>
>>
>> Reviewed-by: David Plowman <david.plowman at raspberrypi.com>
>>
>> Thanks
>> David
>>
>> >
>> > > ---
>> > > include/libcamera/ipa/raspberrypi.h | 55
>> -------------------
>> > > include/libcamera/ipa/raspberrypi.mojom | 1 +
>> > > src/ipa/raspberrypi/raspberrypi.cpp | 42 +++++++++++++-
>> > > .../pipeline/raspberrypi/raspberrypi.cpp | 6 +-
>> > > .../pipeline/raspberrypi/rpi_stream.h | 1 -
>> > > 5 files changed, 45 insertions(+), 60 deletions(-)
>> > > delete mode 100644 include/libcamera/ipa/raspberrypi.h
>> > >
>> > > diff --git a/include/libcamera/ipa/raspberrypi.h
>> b/include/libcamera/ipa/raspberrypi.h
>> > > deleted file mode 100644
>> > > index 6a56b0083b85..000000000000
>> > > --- a/include/libcamera/ipa/raspberrypi.h
>> > > +++ /dev/null
>> > > @@ -1,55 +0,0 @@
>> > > -/* SPDX-License-Identifier: LGPL-2.1-or-later */
>> > > -/*
>> > > - * Copyright (C) 2019-2020, Raspberry Pi Ltd.
>> > > - *
>> > > - * raspberrypi.h - Image Processing Algorithm interface for
>> Raspberry Pi
>> > > - */
>> > > -
>> > > -#pragma once
>> > > -
>> > > -#include <stdint.h>
>> > > -
>> > > -#include <libcamera/control_ids.h>
>> > > -#include <libcamera/controls.h>
>> > > -
>> > > -#ifndef __DOXYGEN__
>> > > -
>> > > -namespace libcamera {
>> > > -
>> > > -namespace RPi {
>> > > -
>> > > -/*
>> > > - * List of controls handled by the Raspberry Pi IPA
>> > > - *
>> > > - * \todo This list will need to be built dynamically from the control
>> > > - * algorithms loaded by the json file, once this is supported. At
>> that
>> > > - * point applications should check first whether a control is
>> supported,
>> > > - * and the pipeline handler may be reverted so that it aborts when an
>> > > - * unsupported control is encountered.
>> > > - */
>> > > -static const ControlInfoMap Controls({
>> > > - { &controls::AeEnable, ControlInfo(false, true) },
>> > > - { &controls::ExposureTime, ControlInfo(0, 999999) },
>> > > - { &controls::AnalogueGain, ControlInfo(1.0f, 32.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::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(1000), INT64_C(1000000000)) },
>> > > - { &controls::draft::NoiseReductionMode,
>> ControlInfo(controls::draft::NoiseReductionModeValues) }
>> > > - }, controls::controls);
>> > > -
>> > > -} /* namespace RPi */
>> > > -
>> > > -} /* namespace libcamera */
>> > > -
>> > > -#endif /* __DOXYGEN__ */
>> > > diff --git a/include/libcamera/ipa/raspberrypi.mojom
>> b/include/libcamera/ipa/raspberrypi.mojom
>> > > index a60c3bb43d3c..ed7adebce1b8 100644
>> > > --- a/include/libcamera/ipa/raspberrypi.mojom
>> > > +++ b/include/libcamera/ipa/raspberrypi.mojom
>> > > @@ -40,6 +40,7 @@ struct IPAConfig {
>> > >
>> > > struct IPAConfigResult {
>> > > float modeSensitivity;
>> > > + libcamera.ControlInfoMap controlInfo;
>> > > };
>> > >
>> > > struct StartConfig {
>> > > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp
>> b/src/ipa/raspberrypi/raspberrypi.cpp
>> > > index 3b126bb5175e..7eb04a24c41e 100644
>> > > --- a/src/ipa/raspberrypi/raspberrypi.cpp
>> > > +++ b/src/ipa/raspberrypi/raspberrypi.cpp
>> > > @@ -24,7 +24,6 @@
>> > > #include <libcamera/framebuffer.h>
>> > > #include <libcamera/ipa/ipa_interface.h>
>> > > #include <libcamera/ipa/ipa_module_info.h>
>> > > -#include <libcamera/ipa/raspberrypi.h>
>> > > #include <libcamera/ipa/raspberrypi_ipa_interface.h>
>> > > #include <libcamera/request.h>
>> > >
>> > > @@ -72,6 +71,28 @@ constexpr Duration defaultMaxFrameDuration =
>> 250.0s;
>> > > */
>> > > constexpr Duration controllerMinFrameDuration = 1.0s / 30.0;
>> > >
>> > > +/* List of controls handled by the Raspberry Pi IPA */
>> > > +static const ControlInfoMap Controls({
>> >
>> > While at it, could you rename the variable to start with a lowercase
>> > letter ? "controls" is a bit too generic, so you could call it
>> > "rpiControls" or anything similar.
>> >
>> > > + { &controls::AeEnable, ControlInfo(false, true) },
>> > > + { &controls::ExposureTime, ControlInfo(0, 999999) },
>> > > + { &controls::AnalogueGain, ControlInfo(1.0f, 32.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::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{}) },
>> >
>> > As you've said yourself in
>> >
>> https://lists.libcamera.org/pipermail/libcamera-devel/2022-January/028735.html
>> ,
>> > this one doesn't belong to the IPA side :-) It can be fixed on top
>> > though.
>> >
>> > If it helps moving forward, I can resubmit the patch from
>> >
>> https://lists.libcamera.org/pipermail/libcamera-devel/2022-January/028731.html
>> > as a proper patch, and then you can update the frame duration limits and
>> > exposure time controls at configure time on top ?
>> >
>> > > + { &controls::FrameDurationLimits, ControlInfo(INT64_C(1000),
>> INT64_C(1000000000)) },
>> > > + { &controls::draft::NoiseReductionMode,
>> ControlInfo(controls::draft::NoiseReductionModeValues) }
>> > > +}, controls::controls);
>> > > +
>> > > LOG_DEFINE_CATEGORY(IPARPI)
>> > >
>> > > namespace ipa::RPi {
>> > > @@ -421,6 +442,25 @@ int IPARPi::configure(const IPACameraSensorInfo
>> &sensorInfo,
>> > > ASSERT(controls);
>> > > *controls = std::move(ctrls);
>> > >
>> > > + /*
>> > > + * Apply the correct limits to the exposure, gain and frame
>> duration controls
>> > > + * based on the current sensor mode.
>> > > + */
>> > > + result->controlInfo = Controls;
>> > > + const Duration minSensorFrameDuration = mode_.min_frame_length
>> * mode_.line_length;
>> > > + const Duration maxSensorFrameDuration = mode_.max_frame_length
>> * mode_.line_length;
>> > > + result->controlInfo.at(controls::FrameDurationLimits.id()) =
>> > > +
>> ControlInfo(static_cast<int64_t>(minSensorFrameDuration.get<std::micro>()),
>> > > +
>> static_cast<int64_t>(maxSensorFrameDuration.get<std::micro>()));
>> > > +
>> > > + result->controlInfo.at(controls::AnalogueGain.id()) =
>> > > + ControlInfo(1.0f,
>> static_cast<float>(helper_->Gain(maxSensorGainCode_)));
>> > > +
>> > > + const uint32_t exposureMin =
>> sensorCtrls_.at(V4L2_CID_EXPOSURE).min().get<int32_t>();
>> > > + const uint32_t exposureMax =
>> sensorCtrls_.at(V4L2_CID_EXPOSURE).max().get<int32_t>();
>> > > + result->controlInfo.at(controls::ExposureTime.id()) =
>> > > +
>> ControlInfo(static_cast<int32_t>(helper_->Exposure(exposureMin).get<std::micro>()),
>> > > +
>> static_cast<int32_t>(helper_->Exposure(exposureMax).get<std::micro>()));
>> > > return 0;
>> > > }
>> > >
>> > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
>> b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
>> > > index adc397e8aabd..abb29a8d24b9 100644
>> > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
>> > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
>> > > @@ -20,7 +20,6 @@
>> > > #include <libcamera/camera.h>
>> > > #include <libcamera/control_ids.h>
>> > > #include <libcamera/formats.h>
>> > > -#include <libcamera/ipa/raspberrypi.h>
>> > > #include <libcamera/ipa/raspberrypi_ipa_interface.h>
>> > > #include <libcamera/ipa/raspberrypi_ipa_proxy.h>
>> > > #include <libcamera/logging.h>
>> > > @@ -941,6 +940,9 @@ int PipelineHandlerRPi::configure(Camera *camera,
>> CameraConfiguration *config)
>> > > /* Store the mode sensitivity for the application. */
>> > > data->properties_.set(properties::SensorSensitivity,
>> result.modeSensitivity);
>> > >
>> > > + /* Register the controls that the Raspberry Pi IPA can handle.
>> */
>> > > + data->controlInfo_ = result.controlInfo;
>> > > +
>> > > /* Setup the Video Mux/Bridge entities. */
>> > > for (auto &[device, link] : data->bridgeDevices_) {
>> > > /*
>> > > @@ -1282,8 +1284,6 @@ int
>> PipelineHandlerRPi::registerCamera(MediaDevice *unicam, MediaDevice *isp, Me
>> > > data->delayedCtrls_ =
>> std::make_unique<DelayedControls>(data->sensor_->device(), params);
>> > > data->sensorMetadata_ = sensorConfig.sensorMetadata;
>> > >
>> > > - /* Register the controls that the Raspberry Pi IPA can handle.
>> */
>> > > - data->controlInfo_ = RPi::Controls;
>> > > /* Initialize the camera properties. */
>> > > data->properties_ = data->sensor_->properties();
>> > >
>> > > diff --git a/src/libcamera/pipeline/raspberrypi/rpi_stream.h
>> b/src/libcamera/pipeline/raspberrypi/rpi_stream.h
>> > > index c37f7e82eef6..fe01110019b7 100644
>> > > --- a/src/libcamera/pipeline/raspberrypi/rpi_stream.h
>> > > +++ b/src/libcamera/pipeline/raspberrypi/rpi_stream.h
>> > > @@ -12,7 +12,6 @@
>> > > #include <unordered_map>
>> > > #include <vector>
>> > >
>> > > -#include <libcamera/ipa/raspberrypi.h>
>> > > #include <libcamera/ipa/raspberrypi_ipa_interface.h>
>> > > #include <libcamera/stream.h>
>> > >
>> >
>> > --
>> > Regards,
>> >
>> > Laurent Pinchart
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20220610/baebfb20/attachment-0001.htm>
More information about the libcamera-devel
mailing list