Message ID | 20220609125830.4325-1-naush@raspberrypi.com |
---|---|
State | Superseded |
Headers | show |
Series |
|
Related | show |
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@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> >
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@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@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. Reviewed-by: David Plowman <david.plowman@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
Hi Laurent, Thank you for your feedback. On Thu, 9 Jun 2022 at 17:08, Laurent Pinchart < laurent.pinchart@ideasonboard.com> 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. > Sure I can do that. What do you suggest we do about the AnalogueGain, ExposureTime and FrameDurationLimits controls during the init() phase? Should I exclude them from the list, or include time, with min/max values set to 0 or something else? I'm leaning towards simply excluding them from the list until the limits are actually known. > > > Signed-off-by: Naushir Patuck <naush@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. > Ack. > > > + { &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. > I can add a patch on top to move ScalerCrop to the pipeline handler. It's a bit awkward to code because I cannot add elements to a ControlInfoMap after initialisation since it derives from a private unordered_map. Instead, I will have to construct a new ControlInfoMap::Map with the contents of the ControlInfoMap provided by the IPA, add ScalerCrop to the ControlInfoMap::Map, and then convert back to a ControlInfoMap. Can you suggest a more elegant way to do this? Regards, Naush > > 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 >
Hi David, Thank you for your feedback. On Fri, 10 Jun 2022 at 09:04, David Plowman <david.plowman@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@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@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? Regards, Naush > > Reviewed-by: David Plowman <david.plowman@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 >
On Fri, 10 Jun 2022 at 10:27, Naushir Patuck <naush@raspberrypi.com> wrote: > Hi David, > > Thank you for your feedback. > > On Fri, 10 Jun 2022 at 09:04, David Plowman <david.plowman@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@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@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@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 >> >
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({ + { &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); + 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>
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. Signed-off-by: Naushir Patuck <naush@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