[libcamera-devel] [PATCH v9 2/3] libcamera: raspberrypi: Add control of sensor vblanking
Jacopo Mondi
jacopo at jmondi.org
Thu Dec 17 17:18:57 CET 2020
Hi Naush
On Thu, Dec 17, 2020 at 03:49:22PM +0000, Naushir Patuck wrote:
> Hi Jacopo,
>
> Thank you for your review comments.
>
> On Thu, 17 Dec 2020 at 15:17, Jacopo Mondi <jacopo at jmondi.org> wrote:
>
> > Hi Naush,
> >
> > a few minors here and there
> >
> > On Wed, Dec 16, 2020 at 11:22:01AM +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>
> > > ---
> > > 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 | 45 ++++++++++++++++---
> > > .../pipeline/raspberrypi/raspberrypi.cpp | 3 +-
> > > 8 files changed, 120 insertions(+), 14 deletions(-)
> > >
> > > diff --git a/include/libcamera/ipa/raspberrypi.h
> > b/include/libcamera/ipa/raspberrypi.h
> > > index 01fe5abc..1de36039 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) },
> > > };
> > >
> > > } /* namespace RPi */
> > > diff --git a/src/ipa/raspberrypi/cam_helper.cpp
> > b/src/ipa/raspberrypi/cam_helper.cpp
> > > index 6efa0d7f..018c17b9 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,
> >
> > Output parameters should be really passed as pointers, it's very hard
> > to follow what the caller does otherwise. I see the same usage of
> > reference for ouput params is widly spread in the IPA, so I won't
> > bother you asking for a change.
> >
>
> Ack. There is a tidy-up task for our Controller that we need to get to at
> some point.
>
>
> >
> > > + double maxFrameDuration) const
> > > +{
> > > + uint32_t frameLengthMin, frameLengthMax, vblank;
> > > + uint32_t exposureLines = ExposureLines(exposure);
> > > +
> > > + assert(initialized_);
> > > +
> > > + /*
> > > + * Clip 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_);
> > > + /*
> > > + * Limit the exposure to the maximum frame duration requested, and
> > > + * re-calculate if it has been clipped.
> > > + */
> > > + exposureLines = std::min(frameLengthMax - frameIntegrationDiff_,
> > exposureLines);
> > > + exposure = Exposure(exposureLines);
> > > +
> > > + /* Limit the vblank to the range allowed by the frame length
> > limits. */
> > > + vblank = std::clamp(exposureLines + frameIntegrationDiff_,
> > > + frameLengthMin, frameLengthMax) - mode_.height;
> >
> > This looks good and matches the description of controls priorities
> >
> > > + 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 044c2866..b1739a57 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,
> > > + double maxFrameDuration) const;
> >
> > Is this really virtual ?
> >
>
> Yes, I intentionally marked it as virtual. There will be some sensors that
> use a different formula/method to calculate exposure time. In particular,
> I am thinking about imx477 when we eventually get to enable very long
> exposure modes (i.e. in order of minutes) where some extra multipliers are
> used in the calculation. Additionally, global shutter sensors are also
> ones that don't use a simple VTS * HTS type calculation. Overriding this
> in the specific camera helper class will allow us to account for these.
>
>
> >
> > > 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_;
> > > };
> > >
> > > // 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 db8ab879..8688ec09 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 0e896ac7..53961310 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 0b841cd1..2bd8a754 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 d087b07e..e1fe35f5 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;
> >
> > Do I count the number of 0s wrong, or the control maximum value is set
> > to 10^9 and this is 10^8 ? Not much difference though both 100 or 1000
> > seconds of max exposure should suit all needs :)
> >
>
> Yes, they are different. This default value for the IPA is just a value I
> picked, not strictly related to the Control max value. One could argue
> that a more sensible number might be closer to around 500ms, but really
> this gets clipped by the exposure mode profile anyway.
>
>
> >
> > These can be made uint_64 already to avoid casts. Up to you.
> >
>
> I intentionally left them as doubles as our internal representation in the
> Controller and IPA state uses doubles. As above, there is a tidy up task
> to do in our controllers to rationalise this at some point.
>
>
> >
> > >
> > > LOG_DEFINE_CATEGORY(IPARPI)
> > >
> > > @@ -150,6 +152,9 @@ private:
> > >
> > > /* Distinguish the first camera start from others. */
> > > bool firstStart_;
> > > +
> > > + /* Frame duration (1/fps) given in microseconds. */
> > > + double minFrameDuration_, maxFrameDuration_;
> > > };
> > >
> > > int IPARPi::init(const IPASettings &settings)
> > > @@ -332,7 +337,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 */
> >
> > s/FOR/for/
> >
>
> Ack.
>
>
> >
> > > + result->data.push_back(exposureDelay); /* For VBLANK ctrl
> > */
> >
> > So, VBLANK and EXPOSURE ctrl have the same delay. How do we guarantee
> > VBLANK is updated first ?
> >
> > > result->data.push_back(sensorMetadata);
> > >
> > > result->operation |= RPi::IPA_CONFIG_STAGGERED_WRITE;
> > > @@ -382,6 +388,8 @@ void IPARPi::configure(const CameraSensorInfo
> > &sensorInfo,
> > > AgcStatus agcStatus;
> > > agcStatus.shutter_time = DefaultExposureTime;
> > > agcStatus.analogue_gain = DefaultAnalogueGain;
> > > + minFrameDuration_ = defaultMinFrameDuration;
> > > + maxFrameDuration_ = defaultMaxFrameDuration;
> >
> > I would initialize these before the ctrls declaration. These are not
> > sent to the sensor, and seeing them in the middle here is confusing.
> > Or are they needed by the applyAGC() function?
> >
>
> They are needed by the applyAGC() function. However, I can move them above
> the ctrl declaration to improve readability.
>
>
> >
> > > applyAGC(&agcStatus, ctrls);
> > >
> > > result->controls.emplace_back(ctrls);
> > > @@ -524,7 +532,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) {
> > > @@ -804,6 +813,18 @@ void IPARPi::queueRequest(const ControlList
> > &controls)
> > > break;
> > > }
> >
> > Doesn't the SetFixedShutter() function need to be updated to clip the
> > shutter time in the frameDuration limits too ? Can we address it or at
> > least record it with a \todo ?
> >
>
> Yes, SetFixedShutter does need to account for frame duration limits. I do
> have a set of patches that are waiting to be sent out for review that does
> exactly this (along with passing the limits to the AGC algorithms).
> Probably a \todo is not worth it?
>
>
> >
> > >
> > > + 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;
> >
> > I'm surprised how cast from int64_t to double is automatically
> > handled..
> >
>
> Seems to compile without any warnings :-)
>
>
> >
> > > + libcameraMetadata_.set(controls::FrameDurations,
> > > + {
> > static_cast<int64_t>(minFrameDuration_),
> > > +
> > static_cast<int64_t>(maxFrameDuration_) });
> >
> > I don't think this matches the control description though:
> >
> > + When reported by pipelines, the control expresses the duration
> > of the
> > + sensor frame used to produce streams part of the completed
> > Request.
> > + The minimum and maximum values shall then be the same, as the
> > sensor
> > + frame duration is a fixed parameter.
> >
> > Should you record the exposure time calculated by GetVBlanking() and
> > report it here ?
> >
>
> Yes, this should be the "adjusted" frame durations. I do have that fix in
> my next set of patches, but perhaps I apply them here instead? Note that I
> do not use exposure directly to calculate this, rather I use the vblank
> limits provided by the sensor. There is a sensor dependent offset between
> exposure lines and VTS, so the numbers may be close, but not fully
> accurate, when using exposure time to calculate frame durations.
>
>
> >
> > > + break;
> > > + }
> > > +
> > > default:
> > > LOG(IPARPI, Warning)
> > > << "Ctrl " << controls::controls.at
> > (ctrl.first)->name()
> > > @@ -962,15 +983,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.
> >
> > Replying to myself: we skip one frame in case VBLANK clips exposure.
> > I guess setting VBLANK one frame in advance compared to exposure won't
> > help, as it could clip the previous frame. The only solution is to
> > prioritize VBLANK when setting controls to the device I guess.
> >
>
> In my next patch set, I address this by allowing the staggered write
> controls to write VBLANK before any other controls. Depending on the
> timing of DelayedControls being merged, I can easily port it if needed, or
> just submit my change to staggered write.
>
>
> >
> > > + */
> > > + 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 7a5f5881..252cab64 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++] } });
> >
> > A few issues apart (metadata handling and manual exposure clipping
> > apart) the rest is all minor stuff. With these fixed/clarified
> >
>
> I will update with a v10 to address the metadata and typo. The manual
> exposure clipping ought to be part of the next patch set as it is part of
> the bigger change where the AGC applies the limits as well. Is that ok
> with you?
Indeed, I trust that you want that addressed as much as I do :)
Cheers
>
> Regards,
> Naush
>
>
>
> >
> > Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
> >
> > Thanks
> > j
> >
> > > sensorMetadata_ = result.data[resultIdx++];
> > > }
> > > }
> > > --
> > > 2.25.1
> > >
> > > _______________________________________________
> > > libcamera-devel mailing list
> > > libcamera-devel at lists.libcamera.org
> > > https://lists.libcamera.org/listinfo/libcamera-devel
> >
More information about the libcamera-devel
mailing list