| Message ID | 20230331081930.19289-3-dse@thaumatec.com |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
Hi Daniel 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@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 > + * > + */ > + > /** > * \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. 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()); > > -- > 2.39.2 >
Hi Jacopo, On Mon, Apr 3, 2023 at 10:58 AM Jacopo Mondi <jacopo.mondi@ideasonboard.com> wrote: > > Hi Daniel > > 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@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 > > + * > > + */ > > + > > /** > > * \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. 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. 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(). > > 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()); > > > > -- > > 2.39.2 > > Best regards Daniel
Hi Daniel On Tue, Apr 04, 2023 at 08:06:33AM +0200, Daniel Semkowicz wrote: > Hi Jacopo, > > On Mon, Apr 3, 2023 at 10:58 AM Jacopo Mondi > <jacopo.mondi@ideasonboard.com> wrote: > > > > Hi Daniel > > > > 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@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 > > > + * > > > + */ > > > + > > > /** > > > * \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 > 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. > > > > 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()); > > > > > > -- > > > 2.39.2 > > > > > Best regards > Daniel
On Tue, Apr 4, 2023 at 8:25 AM Jacopo Mondi <jacopo.mondi@ideasonboard.com> wrote: > > Hi Daniel > > On Tue, Apr 04, 2023 at 08:06:33AM +0200, Daniel Semkowicz wrote: > > Hi Jacopo, > > > > On Mon, Apr 3, 2023 at 10:58 AM Jacopo Mondi > > <jacopo.mondi@ideasonboard.com> wrote: > > > > > > Hi Daniel > > > > > > 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@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 > > > > + * > > > > + */ > > > > + > > > > /** > > > > * \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. > > > > > > > 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()); > > > > > > > > -- > > > > 2.39.2 > > > > > > > > Best regards > > Daniel Thanks! Daniel
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 + * + */ + /** * \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>() + }; + } + /* 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; + /* 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());
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@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(-)