<div dir="ltr"><div dir="ltr">Hi Jacopo,</div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, 19 Jan 2021 at 18:47, Jacopo Mondi <<a href="mailto:jacopo@jmondi.org">jacopo@jmondi.org</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>
On Tue, Jan 19, 2021 at 06:13:33PM +0000, Naushir Patuck wrote:<br>
> Hi Jacopo,<br>
><br>
> Thank you for your review comments.<br>
><br>
> On Tue, 19 Jan 2021 at 16:59, Jacopo Mondi <<a href="mailto:jacopo@jmondi.org" target="_blank">jacopo@jmondi.org</a>> wrote:<br>
><br>
> > Hi Naush,<br>
> ><br>
> > I know I have my tag here, and I'm possibly going to repeat some<br>
> > comments. I don't think any of them should block your series though<br>
> ><br>
> > On Tue, Jan 19, 2021 at 03:30:46PM +0000, Naushir Patuck wrote:<br>
> > > Add support for setting V4L2_CID_VBLANK appropriately when setting<br>
> > > V4L2_CID_EXPOSURE. This will allow adaptive framerates during<br>
> > > viewfinder use cases (e.g. when the exposure time goes above 33ms, we<br>
> > > can reduce the framerate to lower than 30fps).<br>
> > ><br>
> > > The minimum and maximum frame durations are provided via libcamera<br>
> > > controls, and will prioritise exposure time limits over any AGC request.<br>
> > ><br>
> > > V4L2_CID_VBLANK is controlled through the staggered writer, just like<br>
> > > the exposure and gain controls.<br>
> > ><br>
> > > Signed-off-by: Naushir Patuck <<a href="mailto:naush@raspberrypi.com" target="_blank">naush@raspberrypi.com</a>><br>
> > > Reviewed-by: David Plowman <<a href="mailto:david.plowman@raspberrypi.com" target="_blank">david.plowman@raspberrypi.com</a>><br>
> > > Tested-by: David Plowman <<a href="mailto:david.plowman@raspberrypi.com" target="_blank">david.plowman@raspberrypi.com</a>><br>
> > > Reviewed-by: Jacopo Mondi <<a href="mailto:jacopo@jmondi.org" target="_blank">jacopo@jmondi.org</a>><br>
> > > ---<br>
> > > include/libcamera/ipa/raspberrypi.h | 1 +<br>
> > > src/ipa/raspberrypi/cam_helper.cpp | 35 +++++++++++-<br>
> > > src/ipa/raspberrypi/cam_helper.hpp | 15 ++++-<br>
> > > src/ipa/raspberrypi/cam_helper_imx219.cpp | 13 ++++-<br>
> > > src/ipa/raspberrypi/cam_helper_imx477.cpp | 11 +++-<br>
> > > src/ipa/raspberrypi/cam_helper_ov5647.cpp | 11 +++-<br>
> > > src/ipa/raspberrypi/raspberrypi.cpp | 56 ++++++++++++++++---<br>
> > > .../pipeline/raspberrypi/raspberrypi.cpp | 3 +-<br>
> > > 8 files changed, 130 insertions(+), 15 deletions(-)<br>
> > ><br>
> > > diff --git a/include/libcamera/ipa/raspberrypi.h<br>
> > b/include/libcamera/ipa/raspberrypi.h<br>
> > > index 01fe5abce9a6..1de36039cee0 100644<br>
> > > --- a/include/libcamera/ipa/raspberrypi.h<br>
> > > +++ b/include/libcamera/ipa/raspberrypi.h<br>
> > > @@ -65,6 +65,7 @@ static const ControlInfoMap Controls = {<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,<br>
> > 65535, 65535, 65535), Rectangle{}) },<br>
> > > + { &controls::FrameDurations, ControlInfo(1000, 1000000000) },<br>
> ><br>
> > The static initialization of the control limits worries me a bit.<br>
> > This should match the sensor limits. I can't ask you to go to fully<br>
> > implement this, but I fear the arbitrary initialization of the<br>
> > controls limits the RPi implementation does will be problematic for<br>
> > application that might what to know in which range they can safely<br>
> > operate.<br>
> ><br>
> > I presume your forthcoming applications will probably know those limits<br>
> > from a per-sensor configuration file ?<br>
> ><br>
><br>
> You are correct here, the static initialization should match the current<br>
> sensor mode limits. There currently exists no mechanism for this, so these<br>
> values above are indeed somewhat arbitrary for now.<br>
><br>
><br>
> ><br>
> > > };<br>
> > ><br>
> > > } /* namespace RPi */<br>
> > > diff --git a/src/ipa/raspberrypi/cam_helper.cpp<br>
> > b/src/ipa/raspberrypi/cam_helper.cpp<br>
> > > index 6efa0d7fd030..b7b8faf09c69 100644<br>
> > > --- a/src/ipa/raspberrypi/cam_helper.cpp<br>
> > > +++ b/src/ipa/raspberrypi/cam_helper.cpp<br>
> > > @@ -34,8 +34,10 @@ CamHelper *CamHelper::Create(std::string const<br>
> > &cam_name)<br>
> > > return nullptr;<br>
> > > }<br>
> > ><br>
> > > -CamHelper::CamHelper(MdParser *parser)<br>
> > > - : parser_(parser), initialized_(false)<br>
> > > +CamHelper::CamHelper(MdParser *parser, unsigned int maxFrameLength,<br>
> > > + unsigned int frameIntegrationDiff)<br>
> > > + : parser_(parser), initialized_(false),<br>
> > maxFrameLength_(maxFrameLength),<br>
> > > + frameIntegrationDiff_(frameIntegrationDiff)<br>
> > > {<br>
> > > }<br>
> > ><br>
> > > @@ -56,6 +58,35 @@ double CamHelper::Exposure(uint32_t exposure_lines)<br>
> > const<br>
> > > return exposure_lines * mode_.line_length / 1000.0;<br>
> > > }<br>
> > ><br>
> > > +uint32_t CamHelper::GetVBlanking(double &exposure, double<br>
> > minFrameDuration,<br>
> > > + double maxFrameDuration) const<br>
> > > +{<br>
> > > + uint32_t frameLengthMin, frameLengthMax, vblank;<br>
> > > + uint32_t exposureLines = ExposureLines(exposure);<br>
> > > +<br>
> > > + assert(initialized_);<br>
> > > +<br>
> > > + /*<br>
> > > + * Clamp frame length by the frame duration range and the maximum<br>
> > allowable<br>
> > > + * value in the sensor, given by maxFrameLength_.<br>
> > > + */<br>
> > > + frameLengthMin = std::clamp<uint32_t>(1e3 * minFrameDuration /<br>
> > mode_.line_length,<br>
> > > + mode_.height,<br>
> > maxFrameLength_);<br>
> > > + frameLengthMax = std::clamp<uint32_t>(1e3 * maxFrameDuration /<br>
> > mode_.line_length,<br>
> > > + mode_.height,<br>
> > maxFrameLength_);<br>
> ><br>
> > min and max frame durations are here adjusted to respect the sensor's<br>
> > configuration. Shouldn't this be reflected in the metadata ?<br>
> ><br>
><br>
> Correct, they should - but currently don't. We discussed this in an<br>
> earlier thread, but I have another series related to fps for plumbing in<br>
> frame duration limits into our AGC - and with this change we return back<br>
> the fully constrained durations in the metadata. There is a \todo in the<br>
> handling of the FrameDurations control saying this is a pending change to<br>
> be made.<br>
><br>
><br>
> ><br>
> > > + /*<br>
> > > + * Limit the exposure to the maximum frame duration requested, and<br>
> > > + * re-calculate if it has been clipped.<br>
> > > + */<br>
> > > + exposureLines = std::min(frameLengthMax - frameIntegrationDiff_,<br>
> > exposureLines);<br>
> ><br>
> > Can't you here<br>
> > exposureLines = std::max(frameLengthMin, exposureLines +<br>
> > frameIntegrationDiff_);<br>
> ><br>
><br>
> Is this correct? For rolling shutter sensors, it is typically the case that<br>
><br>
> frame length - exposure lines must be >= frame integration difference<br>
><br>
> The line in question is ensuring the maximum exposure cannot exceed the<br>
> maximum frame length - frame integration difference. With the suggested<br>
> change, we are instead saying that it might be possible to have<br>
> exposureLine = frameLengthMin, so not accounting for<br>
> frameIntegrationDiff_. But also, if there is no clipping, we are<br>
> increasing the requested exposure time by frameIntegrationDiff_ lines. My<br>
> head is spinning a bit with all of this, so please do correct me if what I<br>
> have understood is rubbish :-)<br>
><br>
><br>
> > To avoid the below clamp, which is partially useless as we're already<br>
> > sure exposureLines < (frameLengthMax - frameIntegrationDiff_)<br>
> ><br>
> > (and I'm not sure if frameIntegrationDiff_ has only to be considered<br>
> > when inspecting the upper limit or also when comparing to the bottom<br>
> > one)<br>
> ><br>
><br>
> Ah, I *think* maybe I understand where you are going with this. In my<br>
> calculations, I modify frame length to match the exposure requested (within<br>
> allowable limits). Hence I only consider frameIntegrationDiff_ on the<br>
> upper limit, and explicitly subtract it for the vblank calculation below.<br>
> I think your change is doing things the other way round, i.e. adjusting<br>
> exposure to match the given frame length? If that is the case, I would<br>
> prefer to keep it as is, so we get the exposure as close as possible to the<br>
> request. If that is not the case, I'm sorry, but I have misunderstood :-)<br>
><br>
<br>
Ok sorry for the mess. My suggestion was to:<br>
exposureLines = min(frameLengthMax - frameIntegrationDiff, exposureLine);<br>
exposureLines = max(frameLenghtMin, exposureLines + frameIntegrationDiff);<br>
<br>
But as you said this might add (+ frameIntegrationDiff) to the<br>
exposure. I should have probably said:<br>
<br>
exposureLines = min(frameLengthMax - frameIntegrationDiff, exposureLines);<br>
exposureLines = max(frameLenghtMin - frameIntegrationDiff, exposureLines);<br>
<br>
to clamp exposureLines in [min - diff, max - diff] interval<br>
But I now get this is not required, as exposure time might be<br>
shorter than the min frame size, and VBLANK gets adjusted to respect<br>
the min frame size regardless of the exposure time.<br>
<br>
IOW the exposure time is clipped by the max frame length ( -<br>
integration diff) but can be shorter than min frame length.<br>
<br>
Sorry to have you re-think this twice to disprove my suggestion.<br>
<br>
><br>
> ><br>
> > it will also make sure the below recalculation of 'exposure' is in the<br>
> > limits<br>
> ><br>
> ><br>
> ><br>
> > > + exposure = Exposure(exposureLines);<br>
> > > +<br>
> > > + /* Limit the vblank to the range allowed by the frame length<br>
> > limits. */<br>
> > > + vblank = std::clamp(exposureLines + frameIntegrationDiff_,<br>
> > > + frameLengthMin, frameLengthMax) - mode_.height;<br>
> ><br>
> > Then you can simply<br>
> > return exposureLines - mode_.height;<br>
> ><br>
> > > + return vblank;<br>
> > > +}<br>
> > > +<br>
> > > void CamHelper::SetCameraMode(const CameraMode &mode)<br>
> > > {<br>
> > > mode_ = mode;<br>
> > > diff --git a/src/ipa/raspberrypi/cam_helper.hpp<br>
> > b/src/ipa/raspberrypi/cam_helper.hpp<br>
> > > index 044c28667f75..b1739a57e342 100644<br>
> > > --- a/src/ipa/raspberrypi/cam_helper.hpp<br>
> > > +++ b/src/ipa/raspberrypi/cam_helper.hpp<br>
> > > @@ -62,12 +62,15 @@ class CamHelper<br>
> > > {<br>
> > > public:<br>
> > > static CamHelper *Create(std::string const &cam_name);<br>
> > > - CamHelper(MdParser *parser);<br>
> > > + CamHelper(MdParser *parser, unsigned int maxFrameLength,<br>
> > > + unsigned int frameIntegrationDiff);<br>
> > > virtual ~CamHelper();<br>
> > > void SetCameraMode(const CameraMode &mode);<br>
> > > MdParser &Parser() const { return *parser_; }<br>
> > > uint32_t ExposureLines(double exposure_us) const;<br>
> > > double Exposure(uint32_t exposure_lines) const; // in us<br>
> > > + virtual uint32_t GetVBlanking(double &exposure_us, double<br>
> > minFrameDuration,<br>
> ><br>
> > The first parameter is called just 'exposure' in the function<br>
> > implementation. Mixing of snake_case and camelCase, but it's in many<br>
> > places already in the IPA, so...<br>
> ><br>
><br>
> Indeed, this was also brought up in earlier comments. Our "controller"<br>
> uses snake case throughout (mostly) and we do have a task of converting to<br>
> camel case as some point to match our IPA (which is camel case throughout).<br>
><br>
><br>
> ><br>
> > > + double maxFrameDuration) const;<br>
> > > virtual uint32_t GainCode(double gain) const = 0;<br>
> > > virtual double Gain(uint32_t gain_code) const = 0;<br>
> > > virtual void GetDelays(int &exposure_delay, int &gain_delay) const;<br>
> > > @@ -76,10 +79,20 @@ public:<br>
> > > virtual unsigned int HideFramesModeSwitch() const;<br>
> > > virtual unsigned int MistrustFramesStartup() const;<br>
> > > virtual unsigned int MistrustFramesModeSwitch() const;<br>
> > > +<br>
> > > protected:<br>
> > > MdParser *parser_;<br>
> > > CameraMode mode_;<br>
> > > +<br>
> > > +private:<br>
> > > bool initialized_;<br>
> > > + /* Largest possible frame length, in units of lines. */<br>
> > > + unsigned int maxFrameLength_;<br>
> > > + /*<br>
> > > + * Smallest difference between the frame length and integration<br>
> > time,<br>
> > > + * in units of lines.<br>
> > > + */<br>
> > > + unsigned int frameIntegrationDiff_;<br>
> ><br>
> > nit: different declaration order than in the derived classes, but<br>
> > well, minor..<br>
> ><br>
><br>
> Ack.<br>
><br>
><br>
> ><br>
> > > };<br>
> > ><br>
> > > // This is for registering camera helpers with the system, so that the<br>
> > > diff --git a/src/ipa/raspberrypi/cam_helper_imx219.cpp<br>
> > b/src/ipa/raspberrypi/cam_helper_imx219.cpp<br>
> > > index db8ab879deb7..8688ec091c44 100644<br>
> > > --- a/src/ipa/raspberrypi/cam_helper_imx219.cpp<br>
> > > +++ b/src/ipa/raspberrypi/cam_helper_imx219.cpp<br>
> > > @@ -49,13 +49,22 @@ public:<br>
> > > double Gain(uint32_t gain_code) const override;<br>
> > > unsigned int MistrustFramesModeSwitch() const override;<br>
> > > bool SensorEmbeddedDataPresent() const override;<br>
> > > +<br>
> > > +private:<br>
> > > + /*<br>
> > > + * Smallest difference between the frame length and integration<br>
> > time,<br>
> > > + * in units of lines.<br>
> > > + */<br>
> > > + static constexpr int frameIntegrationDiff = 4;<br>
> > > + /* Largest possible frame length, in units of lines. */<br>
> > > + static constexpr int maxFrameLength = 0xffff;<br>
> > > };<br>
> > ><br>
> > > CamHelperImx219::CamHelperImx219()<br>
> > > #if ENABLE_EMBEDDED_DATA<br>
> > > - : CamHelper(new MdParserImx219())<br>
> > > + : CamHelper(new MdParserImx219(), maxFrameLength,<br>
> > frameIntegrationDiff)<br>
> > > #else<br>
> > > - : CamHelper(new MdParserRPi())<br>
> > > + : CamHelper(new MdParserRPi(), maxFrameLength,<br>
> > frameIntegrationDiff)<br>
> > > #endif<br>
> > > {<br>
> > > }<br>
> > > diff --git a/src/ipa/raspberrypi/cam_helper_imx477.cpp<br>
> > b/src/ipa/raspberrypi/cam_helper_imx477.cpp<br>
> > > index 0e896ac74f88..5396131003f1 100644<br>
> > > --- a/src/ipa/raspberrypi/cam_helper_imx477.cpp<br>
> > > +++ b/src/ipa/raspberrypi/cam_helper_imx477.cpp<br>
> > > @@ -38,10 +38,19 @@ public:<br>
> > > uint32_t GainCode(double gain) const override;<br>
> > > double Gain(uint32_t gain_code) const override;<br>
> > > bool SensorEmbeddedDataPresent() const override;<br>
> > > +<br>
> > > +private:<br>
> > > + /*<br>
> > > + * Smallest difference between the frame length and integration<br>
> > time,<br>
> > > + * in units of lines.<br>
> > > + */<br>
> > > + static constexpr int frameIntegrationDiff = 22;<br>
> > > + /* Largest possible frame length, in units of lines. */<br>
> > > + static constexpr int maxFrameLength = 0xffdc;<br>
> > > };<br>
> > ><br>
> > > CamHelperImx477::CamHelperImx477()<br>
> > > - : CamHelper(new MdParserImx477())<br>
> > > + : CamHelper(new MdParserImx477(), maxFrameLength,<br>
> > frameIntegrationDiff)<br>
> > > {<br>
> > > }<br>
> > ><br>
> > > diff --git a/src/ipa/raspberrypi/cam_helper_ov5647.cpp<br>
> > b/src/ipa/raspberrypi/cam_helper_ov5647.cpp<br>
> > > index 0b841cd175fa..2bd8a754f133 100644<br>
> > > --- a/src/ipa/raspberrypi/cam_helper_ov5647.cpp<br>
> > > +++ b/src/ipa/raspberrypi/cam_helper_ov5647.cpp<br>
> > > @@ -23,6 +23,15 @@ public:<br>
> > > unsigned int HideFramesModeSwitch() const override;<br>
> > > unsigned int MistrustFramesStartup() const override;<br>
> > > unsigned int MistrustFramesModeSwitch() const override;<br>
> > > +<br>
> > > +private:<br>
> > > + /*<br>
> > > + * Smallest difference between the frame length and integration<br>
> > time,<br>
> > > + * in units of lines.<br>
> > > + */<br>
> > > + static constexpr int frameIntegrationDiff = 4;<br>
> > > + /* Largest possible frame length, in units of lines. */<br>
> > > + static constexpr int maxFrameLength = 0xffff;<br>
> > > };<br>
> > ><br>
> > > /*<br>
> > > @@ -31,7 +40,7 @@ public:<br>
> > > */<br>
> > ><br>
> > > CamHelperOv5647::CamHelperOv5647()<br>
> > > - : CamHelper(new MdParserRPi())<br>
> > > + : CamHelper(new MdParserRPi(), maxFrameLength,<br>
> > frameIntegrationDiff)<br>
> > > {<br>
> > > }<br>
> > ><br>
> > > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp<br>
> > b/src/ipa/raspberrypi/raspberrypi.cpp<br>
> > > index d087b07e186b..ba9bc398ef87 100644<br>
> > > --- a/src/ipa/raspberrypi/raspberrypi.cpp<br>
> > > +++ b/src/ipa/raspberrypi/raspberrypi.cpp<br>
> > > @@ -58,6 +58,8 @@ namespace libcamera {<br>
> > > /* Configure the sensor with these values initially. */<br>
> > > constexpr double DefaultAnalogueGain = 1.0;<br>
> > > constexpr unsigned int DefaultExposureTime = 20000;<br>
> > > +constexpr double defaultMinFrameDuration = 1e6 / 30.0;<br>
> > > +constexpr double defaultMaxFrameDuration = 1e6 / 0.01;<br>
> ><br>
> > Quite arbitrary, isn't it ?<br>
> ><br>
><br>
> Yes it is somewhat. I wanted to establish a default frameduration interval<br>
> to start with in the absence of a request from the user. Now this could be<br>
> variable framerate (as I have done here), or a fixed framerate. We have a<br>
> preference to run at variable framerate by default, hence the values<br>
> above. Note that these defaults are not necessarily what the sensor min<br>
> and max are. For example, a sensor may be able to do 200fps, and we do not<br>
> want to go to such extreme exposure times for a simple viewfinder as it<br>
> would probably degrade image quality. But if an application really wants<br>
> to do that, it is free to set the frame duration limits appropriately. In<br>
> that respect, having the default values as a const in the IPA seemed to<br>
> make more sense.<br>
><br>
><br>
> ><br>
> > I would have expected this to be sent by the pipeline to the IPA by<br>
> > populating the FrameDuration ControlInfoMap at configure() time. The<br>
> > 'sensorControls' provided at configure might very well contain the<br>
> > VBLANK control limits, which once combined with the sensor mode<br>
> > information should give you precise limits<br>
> ><br>
> > minFrameDuration = (mode.height + min(VBLANK)) * mode.line_lenght<br>
> > / (mode.pixel_rate / 1e6)<br>
> ><br>
> > Or is this not required ?<br>
> ><br>
><br>
> Correct, this is a calculation that is required, but not yet in place. My<br>
> next patch series for fps does add this calculation in to control sensor<br>
> limits. We discussed this previously, and decided not to introduce that<br>
> series just yet to get this one through with basic functionality.<br>
><br>
><br>
> ><br>
> > ><br>
> > > LOG_DEFINE_CATEGORY(IPARPI)<br>
> > ><br>
> > > @@ -150,6 +152,10 @@ private:<br>
> > ><br>
> > > /* Distinguish the first camera start from others. */<br>
> > > bool firstStart_;<br>
> > > +<br>
> > > + /* Frame duration (1/fps) limits, given in microseconds. */<br>
> > > + double minFrameDuration_;<br>
> > > + double maxFrameDuration_;<br>
> > > };<br>
> > ><br>
> > > int IPARPi::init(const IPASettings &settings)<br>
> > > @@ -332,7 +338,8 @@ void IPARPi::configure(const CameraSensorInfo<br>
> > &sensorInfo,<br>
> > > sensorMetadata = helper_->SensorEmbeddedDataPresent();<br>
> > ><br>
> > > result->data.push_back(gainDelay);<br>
> > > - result->data.push_back(exposureDelay);<br>
> > > + result->data.push_back(exposureDelay); /* For EXPOSURE<br>
> > ctrl */<br>
> > > + result->data.push_back(exposureDelay); /* For VBLANK ctrl<br>
> > */<br>
> > > result->data.push_back(sensorMetadata);<br>
> > ><br>
> > > result->operation |= RPi::IPA_CONFIG_STAGGERED_WRITE;<br>
> > > @@ -377,6 +384,9 @@ void IPARPi::configure(const CameraSensorInfo<br>
> > &sensorInfo,<br>
> > > controller_.Initialise();<br>
> > > controllerInit_ = true;<br>
> > ><br>
> > > + minFrameDuration_ = defaultMinFrameDuration;<br>
> > > + maxFrameDuration_ = defaultMaxFrameDuration;<br>
> > > +<br>
> > > /* Supply initial values for gain and exposure. */<br>
> > > ControlList ctrls(sensorCtrls_);<br>
> > > AgcStatus agcStatus;<br>
> > > @@ -524,7 +534,8 @@ bool IPARPi::validateSensorControls()<br>
> > > {<br>
> > > static const uint32_t ctrls[] = {<br>
> > > V4L2_CID_ANALOGUE_GAIN,<br>
> > > - V4L2_CID_EXPOSURE<br>
> > > + V4L2_CID_EXPOSURE,<br>
> > > + V4L2_CID_VBLANK,<br>
> > > };<br>
> > ><br>
> > > for (auto c : ctrls) {<br>
> > > @@ -551,7 +562,7 @@ bool IPARPi::validateIspControls()<br>
> > > V4L2_CID_USER_BCM2835_ISP_DENOISE,<br>
> > > V4L2_CID_USER_BCM2835_ISP_SHARPEN,<br>
> > > V4L2_CID_USER_BCM2835_ISP_DPC,<br>
> > > - V4L2_CID_USER_BCM2835_ISP_LENS_SHADING<br>
> > > + V4L2_CID_USER_BCM2835_ISP_LENS_SHADING,<br>
> ><br>
> > Unrelated it seems<br>
> ><br>
><br>
> Ack. Thank Kieran for pointing it out :-)<br>
><br>
><br>
> ><br>
> > > };<br>
> > ><br>
> > > for (auto c : ctrls) {<br>
> > > @@ -804,6 +815,25 @@ void IPARPi::queueRequest(const ControlList<br>
> > &controls)<br>
> > > break;<br>
> > > }<br>
> > ><br>
> > > + case controls::FRAME_DURATIONS: {<br>
> > > + auto frameDurations = ctrl.second.get<Span<const<br>
> > int64_t>>();<br>
> > > +<br>
> > > + /* This will be applied once AGC recalculations<br>
> > occur. */<br>
> > > + minFrameDuration_ = frameDurations[0] ?<br>
> > frameDurations[0] : defaultMinFrameDuration;<br>
> > > + maxFrameDuration_ = frameDurations[1] ?<br>
> > frameDurations[1] : defaultMaxFrameDuration;<br>
> > > + maxFrameDuration_ = std::max(maxFrameDuration_,<br>
> > minFrameDuration_);<br>
> ><br>
> > Why this last line ?<br>
> ><br>
><br>
> That last line was a recent addition. Laurent correctly pointed out that<br>
> bad things would happen if maxFrameDuration_ < minFrameDuration_.<br>
><br>
><br>
> > Also, it really seems seems arbitrary defaults clips the exposure to<br>
> > pretty abitrary values. If you don't want to do the calculation of the<br>
> > defaults, what about skipping the clipping completely ? Or is this<br>
> > your intetion ? "Pick default values large enough that no clipping<br>
> > occours" ?<br>
> ><br>
><br>
> The idea was that setting a 0 for either min or max would revert to the ipa<br>
> defaults. With default values for either/both min and max values, we still<br>
> need to ensure maxFrameDuration_ >= minFrameDuration_. Perhaps the<br>
> disconnect here is the notion of using IPA provided default values?<br>
><br>
><br>
> ><br>
> > > +<br>
> > > + /*<br>
> > > + * \todo The values returned in the metadata below<br>
> > must be<br>
> > > + * correctly clipped by what the sensor mode<br>
> > suppots and<br>
> > > + * what the AGC exposure mode or manual shutter<br>
> > speed limits<br>
> > > + */<br>
> ><br>
> > Why this metadata can't be set in reportMetadata() which is called<br>
> > after processStat() which calls GetVBlanking() and updates the limits<br>
> > ?<br>
> ><br>
><br>
> reportMetadata() is used for per-frame dynamic metadata.<br>
> FrameDurationLimits metadata is only set once when a request comes<br>
> through. This is to inform the app if any clipping has occurred over the<br>
> user requested values. I know we had quite a long discussion about what<br>
> exactly the return metadata value should be, and I think we agreed on<br>
> this? The next patch series for fps does call GetVBlanking() from here to<br>
> correctly clip the return values.<br>
<br>
I'm sorry, it seems we already have gone through this in the past, I'm<br>
repeating the same questions it seems.<br>
<br>
Could you add my tag twice ? :)<br>
<br>
><br>
><br>
> ><br>
> ><br>
> > Sorry for the many questions. The only thing that worries me is the<br>
> > ControlInfo intialization, but that's not something you should address<br>
> > at the moment. Other than that with the metadata and default value<br>
> > assignement clarified, you can retain my tag.<br>
> ><br>
><br>
> Not a problem. Good to ensure things are right by everyone. Please let me<br>
> know your thoughts on my reply, there may be more to discuss before<br>
> submitting is.<br>
<br>
Yeah, good to be on the same page but sorry for having asked the same<br>
questions again.<br>
<br>
Let's get this merged soon so we can close all points highlighted here<br>
in follow-up patches..<br>
<br></blockquote><div><br></div><div>Thanks! So would you be ok if I leave this v12 series as-is and pending feedback comments from Laurent, merge this version if he raises no other issues? I note that there is a minor nit on standardising the ordering of member variables in the base and derived classes, but I will fix that up in my next series. Hope that is ok.<br></div><div><br></div><div>Regards,</div><div>Naush</div><div><br></div><div><br></div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
Thanks<br>
j<br>
<br>
><br>
> Regards,<br>
> Naush<br>
><br>
><br>
> ><br>
> > Thanks<br>
> > j<br>
> ><br>
> > > + libcameraMetadata_.set(controls::FrameDurations,<br>
> > > + {<br>
> > static_cast<int64_t>(minFrameDuration_),<br>
> > > +<br>
> > static_cast<int64_t>(maxFrameDuration_) });<br>
> > > + break;<br>
> > > + }<br>
> > > +<br>
> > > default:<br>
> > > LOG(IPARPI, Warning)<br>
> > > << "Ctrl " << controls::<a href="http://controls.at" rel="noreferrer" target="_blank">controls.at</a><br>
> > (ctrl.first)->name()<br>
> > > @@ -962,15 +992,27 @@ void IPARPi::applyAWB(const struct AwbStatus<br>
> > *awbStatus, ControlList &ctrls)<br>
> > > void IPARPi::applyAGC(const struct AgcStatus *agcStatus, ControlList<br>
> > &ctrls)<br>
> > > {<br>
> > > int32_t gainCode = helper_->GainCode(agcStatus->analogue_gain);<br>
> > > - int32_t exposureLines =<br>
> > helper_->ExposureLines(agcStatus->shutter_time);<br>
> > ><br>
> > > - LOG(IPARPI, Debug) << "Applying AGC Exposure: " <<<br>
> > agcStatus->shutter_time<br>
> > > - << " (Shutter lines: " << exposureLines << ")<br>
> > Gain: "<br>
> > > + /* GetVBlanking might clip exposure time to the fps limits. */<br>
> > > + double exposure = agcStatus->shutter_time;<br>
> > > + int32_t vblanking = helper_->GetVBlanking(exposure,<br>
> > minFrameDuration_,<br>
> > > + maxFrameDuration_);<br>
> > > + int32_t exposureLines = helper_->ExposureLines(exposure);<br>
> > > +<br>
> > > + LOG(IPARPI, Debug) << "Applying AGC Exposure: " << exposure<br>
> > > + << " (Shutter lines: " << exposureLines << ",<br>
> > AGC requested "<br>
> > > + << agcStatus->shutter_time << ") Gain: "<br>
> > > << agcStatus->analogue_gain << " (Gain Code: "<br>
> > > << gainCode << ")";<br>
> > ><br>
> > > - ctrls.set(V4L2_CID_ANALOGUE_GAIN, gainCode);<br>
> > > + /*<br>
> > > + * Due to the behavior of V4L2, the current value of VBLANK could<br>
> > clip the<br>
> > > + * exposure time without us knowing. The next time though this<br>
> > function should<br>
> > > + * clip exposure correctly.<br>
> > > + */<br>
> > > + ctrls.set(V4L2_CID_VBLANK, vblanking);<br>
> > > ctrls.set(V4L2_CID_EXPOSURE, exposureLines);<br>
> > > + ctrls.set(V4L2_CID_ANALOGUE_GAIN, gainCode);<br>
> > > }<br>
> > ><br>
> > > void IPARPi::applyDG(const struct AgcStatus *dgStatus, ControlList<br>
> > &ctrls)<br>
> > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> > b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> > > index 7a5f5881b9b3..252cab64023e 100644<br>
> > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> > > @@ -1233,7 +1233,8 @@ int RPiCameraData::configureIPA(const<br>
> > CameraConfiguration *config)<br>
> > > if (!staggeredCtrl_) {<br>
> > > staggeredCtrl_.init(unicam_[Unicam::Image].dev(),<br>
> > > { { V4L2_CID_ANALOGUE_GAIN,<br>
> > result.data[resultIdx++] },<br>
> > > - { V4L2_CID_EXPOSURE,<br>
> > result.data[resultIdx++] } });<br>
> > > + { V4L2_CID_EXPOSURE,<br>
> > result.data[resultIdx++] },<br>
> > > + { V4L2_CID_VBLANK,<br>
> > result.data[resultIdx++] } });<br>
> > > sensorMetadata_ = result.data[resultIdx++];<br>
> > > }<br>
> > > }<br>
> > > --<br>
> > > 2.25.1<br>
> > ><br>
> ><br>
</blockquote></div></div>