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