[libcamera-devel] [PATCH v6 02/10] ipa: rkisp1: Add lens limits to the session config
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Wed Apr 26 03:11:23 CEST 2023
Hi Daniel,
Thank you for the patch.
On Tue, Apr 04, 2023 at 08:47:38AM +0200, Daniel Semkowicz via libcamera-devel wrote:
> On Tue, Apr 4, 2023 at 8:25 AM Jacopo Mondi wrote:
> > On Tue, Apr 04, 2023 at 08:06:33AM +0200, Daniel Semkowicz wrote:
> > > On Mon, Apr 3, 2023 at 10:58 AM Jacopo Mondi wrote:
> > > > On Fri, Mar 31, 2023 at 10:19:22AM +0200, Daniel Semkowicz via libcamera-devel wrote:
> > > > > Add information about focus lens position limits to the IPA session
> > > > > configuration. These information can then be used by IPA algorithms
> > > > > to know which focus positions are valid.
> > > > >
> > > > > Signed-off-by: Daniel Semkowicz <dse at thaumatec.com>
> > > > > ---
> > > > > src/ipa/rkisp1/ipa_context.cpp | 17 +++++++++++++++++
> > > > > src/ipa/rkisp1/ipa_context.h | 9 +++++++++
> > > > > src/ipa/rkisp1/rkisp1.cpp | 23 ++++++++++++++++++++++-
> > > > > 3 files changed, 48 insertions(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/src/ipa/rkisp1/ipa_context.cpp b/src/ipa/rkisp1/ipa_context.cpp
> > > > > index 9bbf3684..aea99299 100644
> > > > > --- a/src/ipa/rkisp1/ipa_context.cpp
> > > > > +++ b/src/ipa/rkisp1/ipa_context.cpp
> > > > > @@ -14,6 +14,18 @@
> > > > >
> > > > > namespace libcamera::ipa::rkisp1 {
> > > > >
> > > > > +/**
> > > > > + * \struct LensConfiguration
> > > > > + * \brief Lens-specific parameters
> > > > > + *
> > > > > + * \var LensConfiguration::minFocusPosition
> > > > > + * \brief Minimum position supported by the camera focus lens
> > > > > + *
> > > > > + * \var LensConfiguration::maxFocusPosition
> > > > > + * \brief Maximum position supported by the camera focus lens
> > > > > + *
Extra blank line.
> > > > > + */
> > > > > +
> > > > > /**
> > > > > * \struct IPASessionConfiguration
> > > > > * \brief Session configuration for the IPA module
> > > > > @@ -89,6 +101,11 @@ namespace libcamera::ipa::rkisp1 {
> > > > > * \brief Sensor output resolution
> > > > > */
> > > > >
> > > > > +/**
> > > > > + * \var IPASessionConfiguration::lens
> > > > > + * \brief Contains lens-specific parameters if lens was detected
> > > > > + */
> > > > > +
> > > > > /**
> > > > > * \var IPASessionConfiguration::raw
> > > > > * \brief Indicates if the camera is configured to capture raw frames
> > > > > diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h
> > > > > index b9b20653..65b3fbfe 100644
> > > > > --- a/src/ipa/rkisp1/ipa_context.h
> > > > > +++ b/src/ipa/rkisp1/ipa_context.h
> > > > > @@ -8,6 +8,8 @@
> > > > >
> > > > > #pragma once
> > > > >
> > > > > +#include <optional>
> > > > > +
> > > > > #include <linux/rkisp1-config.h>
> > > > >
> > > > > #include <libcamera/base/utils.h>
> > > > > @@ -20,6 +22,11 @@ namespace libcamera {
> > > > >
> > > > > namespace ipa::rkisp1 {
> > > > >
> > > > > +struct LensConfiguration {
> > > > > + int32_t minFocusPosition = 0;
> > > > > + int32_t maxFocusPosition = 0;
> > > > > +};
> > > > > +
> > > > > struct IPASessionConfiguration {
> > > > > struct {
> > > > > struct rkisp1_cif_isp_window measureWindow;
> > > > > @@ -45,6 +52,8 @@ struct IPASessionConfiguration {
> > > > > Size size;
> > > > > } sensor;
> > > > >
> > > > > + std::optional<LensConfiguration> lens;
> > > > > +
> > > > > struct {
> > > > > rkisp1_cif_isp_version revision;
> > > > > } hw;
> > > > > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
> > > > > index d338d374..292768cf 100644
> > > > > --- a/src/ipa/rkisp1/rkisp1.cpp
> > > > > +++ b/src/ipa/rkisp1/rkisp1.cpp
> > > > > @@ -164,9 +164,21 @@ int IPARkISP1::init(const IPASettings &settings, unsigned int hwRevision,
> > > > > context_.configuration.sensor.lineDuration = sensorInfo.minLineLength
> > > > > * 1.0s / sensorInfo.pixelRate;
> > > > >
> > > > > - if (!lensControls.empty())
> > > > > + if (!lensControls.empty()) {
> > > > > lensControls_ = lensControls;
> > > > >
> > > > > + const ControlInfo &focusAbsolute =
> > > > > + lensControls_->at(V4L2_CID_FOCUS_ABSOLUTE);
> > > > > +
> > > > > + LOG(IPARkISP1, Debug)
> > > > > + << "Focus lens: " << focusAbsolute.toString();
> > > > > +
> > > > > + context_.configuration.lens = {
> > > > > + .minFocusPosition = focusAbsolute.min().get<int32_t>(),
> > > > > + .maxFocusPosition = focusAbsolute.max().get<int32_t>()
> > > > > + };
> > > >
> > > > Do you need to initialize context_.configuration.lens at init() time ?
> > > >
> > > > It doesn't seem so to me as in updateControls() below you anyway
> > > > re-fetch V4L2_CID_FOCUS_ABSOLUTE from lensControls_.
> > > >
> > > > Can't it be done in configure() to avoid ...
> > > >
> > > > > + }
> > > > > +
> > > > > /* Load the tuning data file. */
> > > > > File file(settings.configurationFile);
> > > > > if (!file.open(File::OpenModeFlag::ReadOnly)) {
> > > > > @@ -234,6 +246,13 @@ int IPARkISP1::configure(const IPAConfigInfo &ipaConfig,
> > > > > << "Exposure: [" << minExposure << ", " << maxExposure
> > > > > << "], gain: [" << minGain << ", " << maxGain << "]";
> > > > >
> > > > > + /*
> > > > > + * Save the existing lens configuration and restore it after context
> > > > > + * reset. It does not change since init().
> > > > > + */
> > > > > + const std::optional<LensConfiguration> lensConfig =
> > > > > + context_.configuration.lens;
> > > > > +
> > > >
> > > > ... this ?
> > > >
> > > > The computation seems very cheap and camera configuration happens once
> > > > per streaming session, so I wonder if the it wouldn't be better to
> > > > maintain a cleaner coding patch at the expense of this little cost.
> > >
> > > As you already noticed in the next commits, the
> > > context_.configuration.lens is used to validate early in the Af::init()
> > > if lens is available and fail if it is missing. I would like to have
> > > the lens configuration available as early as possible to be able to
> > > check it early and it should not change during runtime.
> > >
> >
> > This makes sense, what doesn't sound right to me is that
> > context_.configuration is meant to be transient between configurations
> >
> > > I can move the context_.configuration.lens initialization to a separate
> > > function and call it twice (in the init() and configure()) to avoid
> > > copying and code duplication.
> >
> > Not worth it imho.
> >
> > What I actually wonder is if we don't need a context_.initdata or
> > something where to store parameters that are fixed for the whole
> > lifetime of the IPA
>
> This sounds like something that would simplify the configuration.
>
> > > However, I do not understand why the context_.configuration is cleared
> > > in the configure(). Problems with setting the same config multiple
> > > times could be easily avoided, if configuration persist between
> > > function calls. Parameters that change can just overwrite the existing
> > > value, without clearing the whole structure. And right now, only the
> > > lineDuration is set in the init().
> > >
> >
> > I think instead it is desirable to clear context_.configuration as it
> > should only contain data that are valid for a single capture session.
> > It's for extra safty if you like, to make sure the IPA doesn't operate
> > on stale data.
> >
> > Let's put it in this way, there's no need to re-design this if you
> > don't want to, you can keep it this way and I can explore on top if a
> > context_.initdata might help.
>
> Yes, I would like to avoid intermediate steps in this case, as there
> is only a neglible profit here.
I've submitted a small patch to add context init data and CC'ed you.
> > > > The rest looks good!
> > > >
> > > > > /* Clear the IPA context before the streaming session. */
> > > > > context_.configuration = {};
> > > > > context_.activeState = {};
> > > > > @@ -272,6 +291,8 @@ int IPARkISP1::configure(const IPAConfigInfo &ipaConfig,
> > > > > return format.colourEncoding == PixelFormatInfo::ColourEncodingRAW;
> > > > > });
> > > > >
> > > > > + context_.configuration.lens = lensConfig;
> > > > > +
> > > > > for (auto const &a : algorithms()) {
> > > > > Algorithm *algo = static_cast<Algorithm *>(a.get());
> > > > >
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list