[libcamera-devel] [PATCH] pipeline: ipa: raspberrypi: Correctly report available controls
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Thu Jun 9 18:08:24 CEST 2022
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>
> ---
> 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
More information about the libcamera-devel
mailing list