| Message ID | 20230314144834.85193-3-dse@thaumatec.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Daniel On Tue, Mar 14, 2023 at 03:48:26PM +0100, Daniel Semkowicz 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 | 12 ++++++++++++ > src/ipa/rkisp1/ipa_context.h | 5 +++++ > src/ipa/rkisp1/rkisp1.cpp | 13 ++++++++++++- > 3 files changed, 29 insertions(+), 1 deletion(-) > > diff --git a/src/ipa/rkisp1/ipa_context.cpp b/src/ipa/rkisp1/ipa_context.cpp > index 9bbf3684..401c098f 100644 > --- a/src/ipa/rkisp1/ipa_context.cpp > +++ b/src/ipa/rkisp1/ipa_context.cpp > @@ -89,6 +89,18 @@ namespace libcamera::ipa::rkisp1 { > * \brief Sensor output resolution > */ > > +/** > + * \var IPASessionConfiguration::lens > + * \brief Lens-specific information > + * > + * \var IPASessionConfiguration::lens.minFocusPosition > + * \brief Minimum position supported by the camera focus lens > + * > + * \var IPASessionConfiguration::lens.maxFocusPosition > + * \brief Maximum position supported by the camera focus lens This doesn't report units, but the ::sensor fields don't either > + * Additional blank line > + */ > + > /** > * \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..bfb6e1b7 100644 > --- a/src/ipa/rkisp1/ipa_context.h > +++ b/src/ipa/rkisp1/ipa_context.h > @@ -45,6 +45,11 @@ struct IPASessionConfiguration { > Size size; > } sensor; > > + struct { > + int32_t minFocusPosition; > + int32_t maxFocusPosition; > + } lens; > + > struct { > rkisp1_cif_isp_version revision; > } hw; > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp > index 248cf5e0..cd1fbae3 100644 > --- a/src/ipa/rkisp1/rkisp1.cpp > +++ b/src/ipa/rkisp1/rkisp1.cpp > @@ -267,9 +267,20 @@ int IPARkISP1::configure(const IPAConfigInfo &ipaConfig, > return format.colourEncoding == PixelFormatInfo::ColourEncodingRAW; > }); > > - if (!ipaConfig.lensControls.empty()) > + if (!ipaConfig.lensControls.empty()) { > lensControls_ = ipaConfig.lensControls; > > + const ControlInfo focusAbsolute = > + lensControls_->find(V4L2_CID_FOCUS_ABSOLUTE)->second; Can we assume V4L2_CID_FOCUS_ABSOLUTE is there ? I would fail, as it seems to me neither the CameraSensor nor the pipeline make sure the control is there.. > + > + LOG(IPARkISP1, Debug) > + << "Focus lens: " << focusAbsolute.toString(); > + > + auto &lensConfig = context_.configuration.lens; > + lensConfig.minFocusPosition = focusAbsolute.min().get<int32_t>(); > + lensConfig.maxFocusPosition = focusAbsolute.max().get<int32_t>(); > + } > + > for (auto const &a : algorithms()) { > Algorithm *algo = static_cast<Algorithm *>(a.get()); > > -- > 2.39.2 >
Hi Jacopo, Thank you for spending your time on the review :) On Mon, Mar 20, 2023 at 8:25 PM Jacopo Mondi <jacopo.mondi@ideasonboard.com> wrote: > > Hi Daniel > > On Tue, Mar 14, 2023 at 03:48:26PM +0100, Daniel Semkowicz 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 | 12 ++++++++++++ > > src/ipa/rkisp1/ipa_context.h | 5 +++++ > > src/ipa/rkisp1/rkisp1.cpp | 13 ++++++++++++- > > 3 files changed, 29 insertions(+), 1 deletion(-) > > > > diff --git a/src/ipa/rkisp1/ipa_context.cpp b/src/ipa/rkisp1/ipa_context.cpp > > index 9bbf3684..401c098f 100644 > > --- a/src/ipa/rkisp1/ipa_context.cpp > > +++ b/src/ipa/rkisp1/ipa_context.cpp > > @@ -89,6 +89,18 @@ namespace libcamera::ipa::rkisp1 { > > * \brief Sensor output resolution > > */ > > > > +/** > > + * \var IPASessionConfiguration::lens > > + * \brief Lens-specific information > > + * > > + * \var IPASessionConfiguration::lens.minFocusPosition > > + * \brief Minimum position supported by the camera focus lens > > + * > > + * \var IPASessionConfiguration::lens.maxFocusPosition > > + * \brief Maximum position supported by the camera focus lens > > This doesn't report units, but the ::sensor fields don't either I am just exposing raw values from the v4l subnode, so it is not even possible to know if any unit is assigned to the values. And until we have a standardized focus values in libcamera, it will be difficult to do it differently. > > > + * > > Additional blank line > > > + */ > > + > > /** > > * \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..bfb6e1b7 100644 > > --- a/src/ipa/rkisp1/ipa_context.h > > +++ b/src/ipa/rkisp1/ipa_context.h > > @@ -45,6 +45,11 @@ struct IPASessionConfiguration { > > Size size; > > } sensor; > > > > + struct { > > + int32_t minFocusPosition; > > + int32_t maxFocusPosition; > > + } lens; > > + > > struct { > > rkisp1_cif_isp_version revision; > > } hw; > > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp > > index 248cf5e0..cd1fbae3 100644 > > --- a/src/ipa/rkisp1/rkisp1.cpp > > +++ b/src/ipa/rkisp1/rkisp1.cpp > > @@ -267,9 +267,20 @@ int IPARkISP1::configure(const IPAConfigInfo &ipaConfig, > > return format.colourEncoding == PixelFormatInfo::ColourEncodingRAW; > > }); > > > > - if (!ipaConfig.lensControls.empty()) > > + if (!ipaConfig.lensControls.empty()) { > > lensControls_ = ipaConfig.lensControls; > > > > + const ControlInfo focusAbsolute = > > + lensControls_->find(V4L2_CID_FOCUS_ABSOLUTE)->second; > > Can we assume V4L2_CID_FOCUS_ABSOLUTE is there ? I would fail, as it > seems to me neither the CameraSensor nor the pipeline make sure the > control is there.. There is a CameraLens::validateLensDriver() function to assure there is a V4L2_CID_FOCUS_ABSOLUTE control, and it's called on CameraLens::init(). If CameraLens::init() called in the CameraSensor::discoverAncillaryDevices() returns error, the focusLens_ pointer is reset. This means the focusLens_ pointer in CameraLens is set only if lens have a V4L2_CID_FOCUS_ABSOLUTE control. I base on this behaviour to expose lens controls from pipeline to the IPA only if the pointer was set. It is a bit hidden under several layers, but I tried to avoid checking the same thing in different places. > > > + > > + LOG(IPARkISP1, Debug) > > + << "Focus lens: " << focusAbsolute.toString(); > > + > > + auto &lensConfig = context_.configuration.lens; > > + lensConfig.minFocusPosition = focusAbsolute.min().get<int32_t>(); > > + lensConfig.maxFocusPosition = focusAbsolute.max().get<int32_t>(); > > + } > > + > > for (auto const &a : algorithms()) { > > Algorithm *algo = static_cast<Algorithm *>(a.get()); > > > > -- > > 2.39.2 > > Best regards Daniel
Hi Daniel On Tue, Mar 21, 2023 at 07:42:53AM +0100, Daniel Semkowicz wrote: > Hi Jacopo, > > Thank you for spending your time on the review :) > > On Mon, Mar 20, 2023 at 8:25 PM Jacopo Mondi > <jacopo.mondi@ideasonboard.com> wrote: > > > > Hi Daniel > > > > On Tue, Mar 14, 2023 at 03:48:26PM +0100, Daniel Semkowicz 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 | 12 ++++++++++++ > > > src/ipa/rkisp1/ipa_context.h | 5 +++++ > > > src/ipa/rkisp1/rkisp1.cpp | 13 ++++++++++++- > > > 3 files changed, 29 insertions(+), 1 deletion(-) > > > > > > diff --git a/src/ipa/rkisp1/ipa_context.cpp b/src/ipa/rkisp1/ipa_context.cpp > > > index 9bbf3684..401c098f 100644 > > > --- a/src/ipa/rkisp1/ipa_context.cpp > > > +++ b/src/ipa/rkisp1/ipa_context.cpp > > > @@ -89,6 +89,18 @@ namespace libcamera::ipa::rkisp1 { > > > * \brief Sensor output resolution > > > */ > > > > > > +/** > > > + * \var IPASessionConfiguration::lens > > > + * \brief Lens-specific information > > > + * > > > + * \var IPASessionConfiguration::lens.minFocusPosition > > > + * \brief Minimum position supported by the camera focus lens > > > + * > > > + * \var IPASessionConfiguration::lens.maxFocusPosition > > > + * \brief Maximum position supported by the camera focus lens > > > > This doesn't report units, but the ::sensor fields don't either > > I am just exposing raw values from the v4l subnode, so it is not even > possible to know if any unit is assigned to the values. > And until we have a standardized focus values in libcamera, it will be > difficult to do it differently. > > > > > > + * > > > > Additional blank line > > > > > + */ > > > + > > > /** > > > * \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..bfb6e1b7 100644 > > > --- a/src/ipa/rkisp1/ipa_context.h > > > +++ b/src/ipa/rkisp1/ipa_context.h > > > @@ -45,6 +45,11 @@ struct IPASessionConfiguration { > > > Size size; > > > } sensor; > > > > > > + struct { > > > + int32_t minFocusPosition; > > > + int32_t maxFocusPosition; > > > + } lens; > > > + > > > struct { > > > rkisp1_cif_isp_version revision; > > > } hw; > > > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp > > > index 248cf5e0..cd1fbae3 100644 > > > --- a/src/ipa/rkisp1/rkisp1.cpp > > > +++ b/src/ipa/rkisp1/rkisp1.cpp > > > @@ -267,9 +267,20 @@ int IPARkISP1::configure(const IPAConfigInfo &ipaConfig, > > > return format.colourEncoding == PixelFormatInfo::ColourEncodingRAW; > > > }); > > > > > > - if (!ipaConfig.lensControls.empty()) > > > + if (!ipaConfig.lensControls.empty()) { > > > lensControls_ = ipaConfig.lensControls; > > > > > > + const ControlInfo focusAbsolute = > > > + lensControls_->find(V4L2_CID_FOCUS_ABSOLUTE)->second; > > > > Can we assume V4L2_CID_FOCUS_ABSOLUTE is there ? I would fail, as it > > seems to me neither the CameraSensor nor the pipeline make sure the > > control is there.. > > There is a CameraLens::validateLensDriver() function to assure there > is a V4L2_CID_FOCUS_ABSOLUTE control, and it's called on > CameraLens::init(). If CameraLens::init() called in the > CameraSensor::discoverAncillaryDevices() returns error, the focusLens_ > pointer is reset. This means the focusLens_ pointer in CameraLens is > set only if lens have a V4L2_CID_FOCUS_ABSOLUTE control. I base on this > behaviour to expose lens controls from pipeline to the IPA only if the > pointer was set. It is a bit hidden under several layers, but I tried > to avoid checking the same thing in different places. > Good point, you're right, please ignore the comment then ;) > > > > > + > > > + LOG(IPARkISP1, Debug) > > > + << "Focus lens: " << focusAbsolute.toString(); > > > + > > > + auto &lensConfig = context_.configuration.lens; > > > + lensConfig.minFocusPosition = focusAbsolute.min().get<int32_t>(); > > > + lensConfig.maxFocusPosition = focusAbsolute.max().get<int32_t>(); > > > + } > > > + > > > for (auto const &a : algorithms()) { > > > Algorithm *algo = static_cast<Algorithm *>(a.get()); > > > > > > -- > > > 2.39.2 > > > > > Best regards > Daniel
Hi Jacopo, On Tue, Mar 21, 2023 at 9:05 AM Jacopo Mondi <jacopo.mondi@ideasonboard.com> wrote: > > Hi Daniel > > On Tue, Mar 21, 2023 at 07:42:53AM +0100, Daniel Semkowicz wrote: > > Hi Jacopo, > > > > Thank you for spending your time on the review :) > > > > On Mon, Mar 20, 2023 at 8:25 PM Jacopo Mondi > > <jacopo.mondi@ideasonboard.com> wrote: > > > > > > Hi Daniel > > > > > > On Tue, Mar 14, 2023 at 03:48:26PM +0100, Daniel Semkowicz 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 | 12 ++++++++++++ > > > > src/ipa/rkisp1/ipa_context.h | 5 +++++ > > > > src/ipa/rkisp1/rkisp1.cpp | 13 ++++++++++++- > > > > 3 files changed, 29 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/src/ipa/rkisp1/ipa_context.cpp b/src/ipa/rkisp1/ipa_context.cpp > > > > index 9bbf3684..401c098f 100644 > > > > --- a/src/ipa/rkisp1/ipa_context.cpp > > > > +++ b/src/ipa/rkisp1/ipa_context.cpp > > > > @@ -89,6 +89,18 @@ namespace libcamera::ipa::rkisp1 { > > > > * \brief Sensor output resolution > > > > */ > > > > > > > > +/** > > > > + * \var IPASessionConfiguration::lens > > > > + * \brief Lens-specific information > > > > + * > > > > + * \var IPASessionConfiguration::lens.minFocusPosition > > > > + * \brief Minimum position supported by the camera focus lens > > > > + * > > > > + * \var IPASessionConfiguration::lens.maxFocusPosition > > > > + * \brief Maximum position supported by the camera focus lens > > > > > > This doesn't report units, but the ::sensor fields don't either > > > > I am just exposing raw values from the v4l subnode, so it is not even > > possible to know if any unit is assigned to the values. > > And until we have a standardized focus values in libcamera, it will be > > difficult to do it differently. > > > > > > > > > + * > > > > > > Additional blank line Regarding the additional blank line, it is intentional. For some reason doxygen does not include the last field "maxFocusPosition" in the documetnation without this blank line at the end of the block with multiples fields. This behaves the same for other already existing documentation blocks. Please see for example: IPASessionConfiguration::grid.stride from the ipu3/ipa_context.cpp. Other fields above like "bdsOutputSize" are correctly visible in the doxygen html docs, however stride brief description is not included. Best regards Daniel > > > > > > > + */ > > > > + > > > > /** > > > > * \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..bfb6e1b7 100644 > > > > --- a/src/ipa/rkisp1/ipa_context.h > > > > +++ b/src/ipa/rkisp1/ipa_context.h > > > > @@ -45,6 +45,11 @@ struct IPASessionConfiguration { > > > > Size size; > > > > } sensor; > > > > > > > > + struct { > > > > + int32_t minFocusPosition; > > > > + int32_t maxFocusPosition; > > > > + } lens; > > > > + > > > > struct { > > > > rkisp1_cif_isp_version revision; > > > > } hw; > > > > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp > > > > index 248cf5e0..cd1fbae3 100644 > > > > --- a/src/ipa/rkisp1/rkisp1.cpp > > > > +++ b/src/ipa/rkisp1/rkisp1.cpp > > > > @@ -267,9 +267,20 @@ int IPARkISP1::configure(const IPAConfigInfo &ipaConfig, > > > > return format.colourEncoding == PixelFormatInfo::ColourEncodingRAW; > > > > }); > > > > > > > > - if (!ipaConfig.lensControls.empty()) > > > > + if (!ipaConfig.lensControls.empty()) { > > > > lensControls_ = ipaConfig.lensControls; > > > > > > > > + const ControlInfo focusAbsolute = > > > > + lensControls_->find(V4L2_CID_FOCUS_ABSOLUTE)->second; > > > > > > Can we assume V4L2_CID_FOCUS_ABSOLUTE is there ? I would fail, as it > > > seems to me neither the CameraSensor nor the pipeline make sure the > > > control is there.. > > > > There is a CameraLens::validateLensDriver() function to assure there > > is a V4L2_CID_FOCUS_ABSOLUTE control, and it's called on > > CameraLens::init(). If CameraLens::init() called in the > > CameraSensor::discoverAncillaryDevices() returns error, the focusLens_ > > pointer is reset. This means the focusLens_ pointer in CameraLens is > > set only if lens have a V4L2_CID_FOCUS_ABSOLUTE control. I base on this > > behaviour to expose lens controls from pipeline to the IPA only if the > > pointer was set. It is a bit hidden under several layers, but I tried > > to avoid checking the same thing in different places. > > > > Good point, you're right, please ignore the comment then ;) > > > > > > > > + > > > > + LOG(IPARkISP1, Debug) > > > > + << "Focus lens: " << focusAbsolute.toString(); > > > > + > > > > + auto &lensConfig = context_.configuration.lens; > > > > + lensConfig.minFocusPosition = focusAbsolute.min().get<int32_t>(); > > > > + lensConfig.maxFocusPosition = focusAbsolute.max().get<int32_t>(); > > > > + } > > > > + > > > > for (auto const &a : algorithms()) { > > > > Algorithm *algo = static_cast<Algorithm *>(a.get()); > > > > > > > > -- > > > > 2.39.2 > > > > > > > > Best regards > > Daniel
diff --git a/src/ipa/rkisp1/ipa_context.cpp b/src/ipa/rkisp1/ipa_context.cpp index 9bbf3684..401c098f 100644 --- a/src/ipa/rkisp1/ipa_context.cpp +++ b/src/ipa/rkisp1/ipa_context.cpp @@ -89,6 +89,18 @@ namespace libcamera::ipa::rkisp1 { * \brief Sensor output resolution */ +/** + * \var IPASessionConfiguration::lens + * \brief Lens-specific information + * + * \var IPASessionConfiguration::lens.minFocusPosition + * \brief Minimum position supported by the camera focus lens + * + * \var IPASessionConfiguration::lens.maxFocusPosition + * \brief Maximum position supported by the camera focus lens + * + */ + /** * \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..bfb6e1b7 100644 --- a/src/ipa/rkisp1/ipa_context.h +++ b/src/ipa/rkisp1/ipa_context.h @@ -45,6 +45,11 @@ struct IPASessionConfiguration { Size size; } sensor; + struct { + int32_t minFocusPosition; + int32_t maxFocusPosition; + } lens; + struct { rkisp1_cif_isp_version revision; } hw; diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp index 248cf5e0..cd1fbae3 100644 --- a/src/ipa/rkisp1/rkisp1.cpp +++ b/src/ipa/rkisp1/rkisp1.cpp @@ -267,9 +267,20 @@ int IPARkISP1::configure(const IPAConfigInfo &ipaConfig, return format.colourEncoding == PixelFormatInfo::ColourEncodingRAW; }); - if (!ipaConfig.lensControls.empty()) + if (!ipaConfig.lensControls.empty()) { lensControls_ = ipaConfig.lensControls; + const ControlInfo focusAbsolute = + lensControls_->find(V4L2_CID_FOCUS_ABSOLUTE)->second; + + LOG(IPARkISP1, Debug) + << "Focus lens: " << focusAbsolute.toString(); + + auto &lensConfig = context_.configuration.lens; + lensConfig.minFocusPosition = focusAbsolute.min().get<int32_t>(); + lensConfig.maxFocusPosition = focusAbsolute.max().get<int32_t>(); + } + 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 | 12 ++++++++++++ src/ipa/rkisp1/ipa_context.h | 5 +++++ src/ipa/rkisp1/rkisp1.cpp | 13 ++++++++++++- 3 files changed, 29 insertions(+), 1 deletion(-)