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