| Message ID | 20261008165113.67511-3-stefan.klug@ideasonboard.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Quoting Stefan Klug (2026-10-08 17:50:47) > The dewarper is only instantiated once per pipeline. One pipeline can > support multiple cameras with different sets of dewarp parmeters. > > In the current implementation the dewarp parameters are stored in the > dewarper when the tuning file is loaded at init() time. Therefore the > tuning file loaded last will overwrite the dewarp params for all > cameras. > > Fix that by storing the dewarp parameters in the camera data and passing > them to the dewarper at configure() time. > > While at it, mention the dwNew parameter in the yaml code sample. Do you mean the new cmNew ? And well if that's all I can find then: Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com> > --- > .../internal/converter/converter_dw100.h | 8 ++-- > src/libcamera/converter/converter_dw100.cpp | 39 +++++++++++++------ > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 5 ++- > 3 files changed, 36 insertions(+), 16 deletions(-) > > diff --git a/include/libcamera/internal/converter/converter_dw100.h b/include/libcamera/internal/converter/converter_dw100.h > index 003f5eb954e9..029810bb4fa6 100644 > --- a/include/libcamera/internal/converter/converter_dw100.h > +++ b/include/libcamera/internal/converter/converter_dw100.h > @@ -8,6 +8,7 @@ > #pragma once > > #include <memory> > +#include <optional> > #include <queue> > > #include <libcamera/control_ids.h> > @@ -31,11 +32,12 @@ public: > > static std::unique_ptr<ConverterDW100Module> createModule(DeviceEnumerator *enumerator); > > - int init(const ValueNode ¶ms); > + int loadDewarpParams(const ValueNode ¶ms, std::optional<Dw100VertexMap::DewarpParams> &dewarpParams); > > int configure(const StreamConfiguration &inputCfg, > const std::vector<std::reference_wrapper<const StreamConfiguration>> > - &outputCfg); > + &outputCfg, > + const std::optional<const Dw100VertexMap::DewarpParams> dewarpParams); > bool isConfigured(const Stream *stream) const; > > Size adjustInputSize(const PixelFormat &pixFmt, const Size &size, > @@ -75,7 +77,7 @@ private: > }; > > std::map<const Stream *, VertexMapInfo> vertexMaps_; > - std::optional<Dw100VertexMap::DewarpParams> dewarpParams_; > + bool hasDewarpParams_; > unsigned int inputBufferCount_; > V4L2M2MConverter converter_; > Rectangle sensorCrop_; > diff --git a/src/libcamera/converter/converter_dw100.cpp b/src/libcamera/converter/converter_dw100.cpp > index 300ea808ceff..ba7337b4abdd 100644 > --- a/src/libcamera/converter/converter_dw100.cpp > +++ b/src/libcamera/converter/converter_dw100.cpp > @@ -72,11 +72,14 @@ ConverterDW100Module::createModule(DeviceEnumerator *enumerator) > } > > /** > - * \brief Initialize the module with configuration data > + * \brief Load dewarp parameters from configuration file > * \param[in] params The config parameters > + * \param[out] dewarpParams The dewarp parameters > * > - * This function shall be called from the pipeline handler to initialize the > - * module with the provided parameters. > + * This function shall be called from the pipeline handler to load dewarp > + * parameters from a tuning file. The dewarpParams can then be passed to > + * configure() to configure the dewarper. If the tuning file does not contain > + * any property, 0 is returned and dewarpParams is set to nullopt_t. > * > * A typical tuning file entry for the dewarper looks like this: > * \code{.unparsed} > @@ -87,6 +90,7 @@ ConverterDW100Module::createModule(DeviceEnumerator *enumerator) > * 0.0, 1.0, 0.0, > * 0.0, 0.0, 1.0, > * ] > + * cmNew: <optional new camera matrix> > * coefficients: [ > * 0,0,0,0,0, > * ] > @@ -98,9 +102,11 @@ ConverterDW100Module::createModule(DeviceEnumerator *enumerator) > * \sa Dw100VertexMap::setDewarpParams() > * \return 0 if successful, an error code otherwise > */ > -int ConverterDW100Module::init(const ValueNode ¶ms) > +int ConverterDW100Module::loadDewarpParams(const ValueNode ¶ms, > + std::optional<Dw100VertexMap::DewarpParams> &dewarpParams) > { > Dw100VertexMap::DewarpParams dp; > + dewarpParams.reset(); > > auto &cm = params["cm"]; > auto &coefficients = params["coefficients"]; > @@ -153,17 +159,26 @@ int ConverterDW100Module::init(const ValueNode ¶ms) > dp.cmNew = dp.cm; > } > > - dewarpParams_ = dp; > + dewarpParams = dp; > > return 0; > } > > /** > - * \copydoc libcamera::V4L2M2MConverter::configure > + * \brief Configure a the dw100 converter module > + * \param[in] inputCfg Input stream configuration > + * \param[in] outputCfgs A list of output stream configurations > + * \param[in] dewarpParams The lens dewarp parameters to apply > + * > + * Configures the converter for the given input and output stream configurations > + * and an optional set of dewarp parameters. > + * > + * \return 0 on success or a negative error code otherwise > */ > int ConverterDW100Module::configure(const StreamConfiguration &inputCfg, > const std::vector<std::reference_wrapper<const StreamConfiguration>> > - &outputCfgs) > + &outputCfgs, > + const std::optional<const Dw100VertexMap::DewarpParams> dewarpParams) > { > int ret; > > @@ -174,6 +189,8 @@ int ConverterDW100Module::configure(const StreamConfiguration &inputCfg, > > inputBufferCount_ = inputCfg.bufferCount; > > + hasDewarpParams_ = dewarpParams.has_value(); > + > for (auto &ref : outputCfgs) { > const auto &outputCfg = ref.get(); > auto &info = vertexMaps_[outputCfg.stream()]; > @@ -182,8 +199,8 @@ int ConverterDW100Module::configure(const StreamConfiguration &inputCfg, > vertexMap.setOutputSize(outputCfg.size); > vertexMap.setSensorCrop(sensorCrop_); > > - if (dewarpParams_) > - vertexMap.setDewarpParams(*dewarpParams_); > + if (dewarpParams) > + vertexMap.setDewarpParams(*dewarpParams); > info.update = true; > } > > @@ -363,7 +380,7 @@ void ConverterDW100Module::updateControlInfos(const Stream *stream, ControlInfoM > controls[&controls::ScalerCrop] = ControlInfo(Rectangle(sensorCrop_.x, sensorCrop_.y, 1, 1), > sensorCrop_, sensorCrop_); > > - if (dewarpParams_.has_value()) > + if (hasDewarpParams_) > controls[&controls::LensDewarpEnable] = ControlInfo(false, true, true); > > if (!converter_.supportsRequests()) > @@ -424,7 +441,7 @@ void ConverterDW100Module::populateMetadata(const Stream *stream, ControlList &m > > meta.set(controls::ScalerCrop, vertexMap.effectiveScalerCrop()); > > - if (dewarpParams_.has_value()) > + if (hasDewarpParams_) > meta.set(controls::LensDewarpEnable, vertexMap.lensDewarpEnable()); > } > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > index 96382c93a427..39fe0f0964d7 100644 > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > @@ -128,6 +128,7 @@ public: > > bool canUseDewarper_; > bool usesDewarper_; > + std::optional<Dw100VertexMap::DewarpParams> dewarpParams_; > > private: > void paramsComputed(unsigned int frame, unsigned int bytesused); > @@ -462,7 +463,7 @@ int RkISP1CameraData::loadTuningFile(const std::string &path) > if (!params) > continue; > > - ret = pipe()->dewarper_->init(params); > + ret = pipe()->dewarper_->loadDewarpParams(params, dewarpParams_); > if (ret) > return ret; > > @@ -1058,7 +1059,7 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c) > PixelFormatInfo::info(ispCfg.pixelFormat) > .stride(ispCfg.size.width, 0); > > - ret = dewarper_->configure(ispCfg, { cfg }); > + ret = dewarper_->configure(ispCfg, { cfg }, data->dewarpParams_); > if (ret) > return ret; > > -- > 2.53.0 >
Morning Stefan On 08/10/2026 17:50, Stefan Klug wrote: > The dewarper is only instantiated once per pipeline. One pipeline can > support multiple cameras with different sets of dewarp parmeters. > > In the current implementation the dewarp parameters are stored in the > dewarper when the tuning file is loaded at init() time. Therefore the > tuning file loaded last will overwrite the dewarp params for all > cameras. > > Fix that by storing the dewarp parameters in the camera data and passing > them to the dewarper at configure() time. > > While at it, mention the dwNew parameter in the yaml code sample. > > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com> > --- Looks good, and I have tried it out and confirmed it works as expected: Reviewed-by: Daniel Scally <dan.scally@ideasonboard.com> Tested-by: Daniel Scally <dan.scally@ideasonboard.com> > .../internal/converter/converter_dw100.h | 8 ++-- > src/libcamera/converter/converter_dw100.cpp | 39 +++++++++++++------ > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 5 ++- > 3 files changed, 36 insertions(+), 16 deletions(-) > > diff --git a/include/libcamera/internal/converter/converter_dw100.h b/include/libcamera/internal/converter/converter_dw100.h > index 003f5eb954e9..029810bb4fa6 100644 > --- a/include/libcamera/internal/converter/converter_dw100.h > +++ b/include/libcamera/internal/converter/converter_dw100.h > @@ -8,6 +8,7 @@ > #pragma once > > #include <memory> > +#include <optional> > #include <queue> > > #include <libcamera/control_ids.h> > @@ -31,11 +32,12 @@ public: > > static std::unique_ptr<ConverterDW100Module> createModule(DeviceEnumerator *enumerator); > > - int init(const ValueNode ¶ms); > + int loadDewarpParams(const ValueNode ¶ms, std::optional<Dw100VertexMap::DewarpParams> &dewarpParams); > > int configure(const StreamConfiguration &inputCfg, > const std::vector<std::reference_wrapper<const StreamConfiguration>> > - &outputCfg); > + &outputCfg, > + const std::optional<const Dw100VertexMap::DewarpParams> dewarpParams); > bool isConfigured(const Stream *stream) const; > > Size adjustInputSize(const PixelFormat &pixFmt, const Size &size, > @@ -75,7 +77,7 @@ private: > }; > > std::map<const Stream *, VertexMapInfo> vertexMaps_; > - std::optional<Dw100VertexMap::DewarpParams> dewarpParams_; > + bool hasDewarpParams_; > unsigned int inputBufferCount_; > V4L2M2MConverter converter_; > Rectangle sensorCrop_; > diff --git a/src/libcamera/converter/converter_dw100.cpp b/src/libcamera/converter/converter_dw100.cpp > index 300ea808ceff..ba7337b4abdd 100644 > --- a/src/libcamera/converter/converter_dw100.cpp > +++ b/src/libcamera/converter/converter_dw100.cpp > @@ -72,11 +72,14 @@ ConverterDW100Module::createModule(DeviceEnumerator *enumerator) > } > > /** > - * \brief Initialize the module with configuration data > + * \brief Load dewarp parameters from configuration file > * \param[in] params The config parameters > + * \param[out] dewarpParams The dewarp parameters > * > - * This function shall be called from the pipeline handler to initialize the > - * module with the provided parameters. > + * This function shall be called from the pipeline handler to load dewarp > + * parameters from a tuning file. The dewarpParams can then be passed to > + * configure() to configure the dewarper. If the tuning file does not contain > + * any property, 0 is returned and dewarpParams is set to nullopt_t. > * > * A typical tuning file entry for the dewarper looks like this: > * \code{.unparsed} > @@ -87,6 +90,7 @@ ConverterDW100Module::createModule(DeviceEnumerator *enumerator) > * 0.0, 1.0, 0.0, > * 0.0, 0.0, 1.0, > * ] > + * cmNew: <optional new camera matrix> > * coefficients: [ > * 0,0,0,0,0, > * ] > @@ -98,9 +102,11 @@ ConverterDW100Module::createModule(DeviceEnumerator *enumerator) > * \sa Dw100VertexMap::setDewarpParams() > * \return 0 if successful, an error code otherwise > */ > -int ConverterDW100Module::init(const ValueNode ¶ms) > +int ConverterDW100Module::loadDewarpParams(const ValueNode ¶ms, > + std::optional<Dw100VertexMap::DewarpParams> &dewarpParams) > { > Dw100VertexMap::DewarpParams dp; > + dewarpParams.reset(); > > auto &cm = params["cm"]; > auto &coefficients = params["coefficients"]; > @@ -153,17 +159,26 @@ int ConverterDW100Module::init(const ValueNode ¶ms) > dp.cmNew = dp.cm; > } > > - dewarpParams_ = dp; > + dewarpParams = dp; > > return 0; > } > > /** > - * \copydoc libcamera::V4L2M2MConverter::configure > + * \brief Configure a the dw100 converter module > + * \param[in] inputCfg Input stream configuration > + * \param[in] outputCfgs A list of output stream configurations > + * \param[in] dewarpParams The lens dewarp parameters to apply > + * > + * Configures the converter for the given input and output stream configurations > + * and an optional set of dewarp parameters. > + * > + * \return 0 on success or a negative error code otherwise > */ > int ConverterDW100Module::configure(const StreamConfiguration &inputCfg, > const std::vector<std::reference_wrapper<const StreamConfiguration>> > - &outputCfgs) > + &outputCfgs, > + const std::optional<const Dw100VertexMap::DewarpParams> dewarpParams) > { > int ret; > > @@ -174,6 +189,8 @@ int ConverterDW100Module::configure(const StreamConfiguration &inputCfg, > > inputBufferCount_ = inputCfg.bufferCount; > > + hasDewarpParams_ = dewarpParams.has_value(); > + > for (auto &ref : outputCfgs) { > const auto &outputCfg = ref.get(); > auto &info = vertexMaps_[outputCfg.stream()]; > @@ -182,8 +199,8 @@ int ConverterDW100Module::configure(const StreamConfiguration &inputCfg, > vertexMap.setOutputSize(outputCfg.size); > vertexMap.setSensorCrop(sensorCrop_); > > - if (dewarpParams_) > - vertexMap.setDewarpParams(*dewarpParams_); > + if (dewarpParams) > + vertexMap.setDewarpParams(*dewarpParams); > info.update = true; > } > > @@ -363,7 +380,7 @@ void ConverterDW100Module::updateControlInfos(const Stream *stream, ControlInfoM > controls[&controls::ScalerCrop] = ControlInfo(Rectangle(sensorCrop_.x, sensorCrop_.y, 1, 1), > sensorCrop_, sensorCrop_); > > - if (dewarpParams_.has_value()) > + if (hasDewarpParams_) > controls[&controls::LensDewarpEnable] = ControlInfo(false, true, true); > > if (!converter_.supportsRequests()) > @@ -424,7 +441,7 @@ void ConverterDW100Module::populateMetadata(const Stream *stream, ControlList &m > > meta.set(controls::ScalerCrop, vertexMap.effectiveScalerCrop()); > > - if (dewarpParams_.has_value()) > + if (hasDewarpParams_) > meta.set(controls::LensDewarpEnable, vertexMap.lensDewarpEnable()); > } > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > index 96382c93a427..39fe0f0964d7 100644 > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > @@ -128,6 +128,7 @@ public: > > bool canUseDewarper_; > bool usesDewarper_; > + std::optional<Dw100VertexMap::DewarpParams> dewarpParams_; > > private: > void paramsComputed(unsigned int frame, unsigned int bytesused); > @@ -462,7 +463,7 @@ int RkISP1CameraData::loadTuningFile(const std::string &path) > if (!params) > continue; > > - ret = pipe()->dewarper_->init(params); > + ret = pipe()->dewarper_->loadDewarpParams(params, dewarpParams_); > if (ret) > return ret; > > @@ -1058,7 +1059,7 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c) > PixelFormatInfo::info(ispCfg.pixelFormat) > .stride(ispCfg.size.width, 0); > > - ret = dewarper_->configure(ispCfg, { cfg }); > + ret = dewarper_->configure(ispCfg, { cfg }, data->dewarpParams_); > if (ret) > return ret; >
Hi 2026. 10. 08. 18:50 keltezéssel, Stefan Klug írta: > The dewarper is only instantiated once per pipeline. One pipeline can > support multiple cameras with different sets of dewarp parmeters. > > In the current implementation the dewarp parameters are stored in the > dewarper when the tuning file is loaded at init() time. Therefore the > tuning file loaded last will overwrite the dewarp params for all > cameras. > > Fix that by storing the dewarp parameters in the camera data and passing > them to the dewarper at configure() time. > > While at it, mention the dwNew parameter in the yaml code sample. > > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com> > --- > .../internal/converter/converter_dw100.h | 8 ++-- > src/libcamera/converter/converter_dw100.cpp | 39 +++++++++++++------ > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 5 ++- > 3 files changed, 36 insertions(+), 16 deletions(-) > > diff --git a/include/libcamera/internal/converter/converter_dw100.h b/include/libcamera/internal/converter/converter_dw100.h > index 003f5eb954e9..029810bb4fa6 100644 > --- a/include/libcamera/internal/converter/converter_dw100.h > +++ b/include/libcamera/internal/converter/converter_dw100.h > @@ -8,6 +8,7 @@ > #pragma once > > #include <memory> > +#include <optional> > #include <queue> > > #include <libcamera/control_ids.h> > @@ -31,11 +32,12 @@ public: > > static std::unique_ptr<ConverterDW100Module> createModule(DeviceEnumerator *enumerator); > > - int init(const ValueNode ¶ms); > + int loadDewarpParams(const ValueNode ¶ms, std::optional<Dw100VertexMap::DewarpParams> &dewarpParams); > > int configure(const StreamConfiguration &inputCfg, > const std::vector<std::reference_wrapper<const StreamConfiguration>> > - &outputCfg); > + &outputCfg, > + const std::optional<const Dw100VertexMap::DewarpParams> dewarpParams); Was this meant to be const std::optional<Dw100VertexMap::DewarpParams> &dewarpParams ? > bool isConfigured(const Stream *stream) const; > > Size adjustInputSize(const PixelFormat &pixFmt, const Size &size, > @@ -75,7 +77,7 @@ private: > }; > > std::map<const Stream *, VertexMapInfo> vertexMaps_; > - std::optional<Dw100VertexMap::DewarpParams> dewarpParams_; > + bool hasDewarpParams_; I think I'd add ` = false` to not have it ever be uninitialized. Or in the constructor maybe. > unsigned int inputBufferCount_; > V4L2M2MConverter converter_; > Rectangle sensorCrop_; > diff --git a/src/libcamera/converter/converter_dw100.cpp b/src/libcamera/converter/converter_dw100.cpp > index 300ea808ceff..ba7337b4abdd 100644 > --- a/src/libcamera/converter/converter_dw100.cpp > +++ b/src/libcamera/converter/converter_dw100.cpp > @@ -72,11 +72,14 @@ ConverterDW100Module::createModule(DeviceEnumerator *enumerator) > } > > /** > - * \brief Initialize the module with configuration data > + * \brief Load dewarp parameters from configuration file > * \param[in] params The config parameters > + * \param[out] dewarpParams The dewarp parameters > * > - * This function shall be called from the pipeline handler to initialize the > - * module with the provided parameters. > + * This function shall be called from the pipeline handler to load dewarp > + * parameters from a tuning file. The dewarpParams can then be passed to > + * configure() to configure the dewarper. If the tuning file does not contain > + * any property, 0 is returned and dewarpParams is set to nullopt_t. This is a very minor thing, but I'd say std::nullopt since `nullopt_t` is the type, not the value. > * > * A typical tuning file entry for the dewarper looks like this: > * \code{.unparsed} > @@ -87,6 +90,7 @@ ConverterDW100Module::createModule(DeviceEnumerator *enumerator) > * 0.0, 1.0, 0.0, > * 0.0, 0.0, 1.0, > * ] > + * cmNew: <optional new camera matrix> > * coefficients: [ > * 0,0,0,0,0, > * ] > @@ -98,9 +102,11 @@ ConverterDW100Module::createModule(DeviceEnumerator *enumerator) > * \sa Dw100VertexMap::setDewarpParams() > * \return 0 if successful, an error code otherwise > */ > -int ConverterDW100Module::init(const ValueNode ¶ms) > +int ConverterDW100Module::loadDewarpParams(const ValueNode ¶ms, > + std::optional<Dw100VertexMap::DewarpParams> &dewarpParams) Wouldn't it make sense to have this be a static member function of `Dw100VertexMap::DewarpParams` ? As far as I can see there is nothing device dependent in here. > { > Dw100VertexMap::DewarpParams dp; > + dewarpParams.reset(); > > auto &cm = params["cm"]; > auto &coefficients = params["coefficients"]; > @@ -153,17 +159,26 @@ int ConverterDW100Module::init(const ValueNode ¶ms) > dp.cmNew = dp.cm; > } > > - dewarpParams_ = dp; > + dewarpParams = dp; > > return 0; > } > > [...]
Quoting Kieran Bingham (2026-10-08 23:36:36) > Quoting Stefan Klug (2026-10-08 17:50:47) > > The dewarper is only instantiated once per pipeline. One pipeline can > > support multiple cameras with different sets of dewarp parmeters. > > > > In the current implementation the dewarp parameters are stored in the > > dewarper when the tuning file is loaded at init() time. Therefore the > > tuning file loaded last will overwrite the dewarp params for all > > cameras. > > > > Fix that by storing the dewarp parameters in the camera data and passing > > them to the dewarper at configure() time. > > > > While at it, mention the dwNew parameter in the yaml code sample. > > Do you mean the new cmNew ? Oh right, will fix on applying. > > And well if that's all I can find then: > > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> Thanks, Stefan > > > > > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com> > > --- > > .../internal/converter/converter_dw100.h | 8 ++-- > > src/libcamera/converter/converter_dw100.cpp | 39 +++++++++++++------ > > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 5 ++- > > 3 files changed, 36 insertions(+), 16 deletions(-) > > > > diff --git a/include/libcamera/internal/converter/converter_dw100.h b/include/libcamera/internal/converter/converter_dw100.h > > index 003f5eb954e9..029810bb4fa6 100644 > > --- a/include/libcamera/internal/converter/converter_dw100.h > > +++ b/include/libcamera/internal/converter/converter_dw100.h > > @@ -8,6 +8,7 @@ > > #pragma once > > > > #include <memory> > > +#include <optional> > > #include <queue> > > > > #include <libcamera/control_ids.h> > > @@ -31,11 +32,12 @@ public: > > > > static std::unique_ptr<ConverterDW100Module> createModule(DeviceEnumerator *enumerator); > > > > - int init(const ValueNode ¶ms); > > + int loadDewarpParams(const ValueNode ¶ms, std::optional<Dw100VertexMap::DewarpParams> &dewarpParams); > > > > int configure(const StreamConfiguration &inputCfg, > > const std::vector<std::reference_wrapper<const StreamConfiguration>> > > - &outputCfg); > > + &outputCfg, > > + const std::optional<const Dw100VertexMap::DewarpParams> dewarpParams); > > bool isConfigured(const Stream *stream) const; > > > > Size adjustInputSize(const PixelFormat &pixFmt, const Size &size, > > @@ -75,7 +77,7 @@ private: > > }; > > > > std::map<const Stream *, VertexMapInfo> vertexMaps_; > > - std::optional<Dw100VertexMap::DewarpParams> dewarpParams_; > > + bool hasDewarpParams_; > > unsigned int inputBufferCount_; > > V4L2M2MConverter converter_; > > Rectangle sensorCrop_; > > diff --git a/src/libcamera/converter/converter_dw100.cpp b/src/libcamera/converter/converter_dw100.cpp > > index 300ea808ceff..ba7337b4abdd 100644 > > --- a/src/libcamera/converter/converter_dw100.cpp > > +++ b/src/libcamera/converter/converter_dw100.cpp > > @@ -72,11 +72,14 @@ ConverterDW100Module::createModule(DeviceEnumerator *enumerator) > > } > > > > /** > > - * \brief Initialize the module with configuration data > > + * \brief Load dewarp parameters from configuration file > > * \param[in] params The config parameters > > + * \param[out] dewarpParams The dewarp parameters > > * > > - * This function shall be called from the pipeline handler to initialize the > > - * module with the provided parameters. > > + * This function shall be called from the pipeline handler to load dewarp > > + * parameters from a tuning file. The dewarpParams can then be passed to > > + * configure() to configure the dewarper. If the tuning file does not contain > > + * any property, 0 is returned and dewarpParams is set to nullopt_t. > > * > > * A typical tuning file entry for the dewarper looks like this: > > * \code{.unparsed} > > @@ -87,6 +90,7 @@ ConverterDW100Module::createModule(DeviceEnumerator *enumerator) > > * 0.0, 1.0, 0.0, > > * 0.0, 0.0, 1.0, > > * ] > > + * cmNew: <optional new camera matrix> > > * coefficients: [ > > * 0,0,0,0,0, > > * ] > > @@ -98,9 +102,11 @@ ConverterDW100Module::createModule(DeviceEnumerator *enumerator) > > * \sa Dw100VertexMap::setDewarpParams() > > * \return 0 if successful, an error code otherwise > > */ > > -int ConverterDW100Module::init(const ValueNode ¶ms) > > +int ConverterDW100Module::loadDewarpParams(const ValueNode ¶ms, > > + std::optional<Dw100VertexMap::DewarpParams> &dewarpParams) > > { > > Dw100VertexMap::DewarpParams dp; > > + dewarpParams.reset(); > > > > auto &cm = params["cm"]; > > auto &coefficients = params["coefficients"]; > > @@ -153,17 +159,26 @@ int ConverterDW100Module::init(const ValueNode ¶ms) > > dp.cmNew = dp.cm; > > } > > > > - dewarpParams_ = dp; > > + dewarpParams = dp; > > > > return 0; > > } > > > > /** > > - * \copydoc libcamera::V4L2M2MConverter::configure > > + * \brief Configure a the dw100 converter module > > + * \param[in] inputCfg Input stream configuration > > + * \param[in] outputCfgs A list of output stream configurations > > + * \param[in] dewarpParams The lens dewarp parameters to apply > > + * > > + * Configures the converter for the given input and output stream configurations > > + * and an optional set of dewarp parameters. > > + * > > + * \return 0 on success or a negative error code otherwise > > */ > > int ConverterDW100Module::configure(const StreamConfiguration &inputCfg, > > const std::vector<std::reference_wrapper<const StreamConfiguration>> > > - &outputCfgs) > > + &outputCfgs, > > + const std::optional<const Dw100VertexMap::DewarpParams> dewarpParams) > > { > > int ret; > > > > @@ -174,6 +189,8 @@ int ConverterDW100Module::configure(const StreamConfiguration &inputCfg, > > > > inputBufferCount_ = inputCfg.bufferCount; > > > > + hasDewarpParams_ = dewarpParams.has_value(); > > + > > for (auto &ref : outputCfgs) { > > const auto &outputCfg = ref.get(); > > auto &info = vertexMaps_[outputCfg.stream()]; > > @@ -182,8 +199,8 @@ int ConverterDW100Module::configure(const StreamConfiguration &inputCfg, > > vertexMap.setOutputSize(outputCfg.size); > > vertexMap.setSensorCrop(sensorCrop_); > > > > - if (dewarpParams_) > > - vertexMap.setDewarpParams(*dewarpParams_); > > + if (dewarpParams) > > + vertexMap.setDewarpParams(*dewarpParams); > > info.update = true; > > } > > > > @@ -363,7 +380,7 @@ void ConverterDW100Module::updateControlInfos(const Stream *stream, ControlInfoM > > controls[&controls::ScalerCrop] = ControlInfo(Rectangle(sensorCrop_.x, sensorCrop_.y, 1, 1), > > sensorCrop_, sensorCrop_); > > > > - if (dewarpParams_.has_value()) > > + if (hasDewarpParams_) > > controls[&controls::LensDewarpEnable] = ControlInfo(false, true, true); > > > > if (!converter_.supportsRequests()) > > @@ -424,7 +441,7 @@ void ConverterDW100Module::populateMetadata(const Stream *stream, ControlList &m > > > > meta.set(controls::ScalerCrop, vertexMap.effectiveScalerCrop()); > > > > - if (dewarpParams_.has_value()) > > + if (hasDewarpParams_) > > meta.set(controls::LensDewarpEnable, vertexMap.lensDewarpEnable()); > > } > > > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > > index 96382c93a427..39fe0f0964d7 100644 > > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > > @@ -128,6 +128,7 @@ public: > > > > bool canUseDewarper_; > > bool usesDewarper_; > > + std::optional<Dw100VertexMap::DewarpParams> dewarpParams_; > > > > private: > > void paramsComputed(unsigned int frame, unsigned int bytesused); > > @@ -462,7 +463,7 @@ int RkISP1CameraData::loadTuningFile(const std::string &path) > > if (!params) > > continue; > > > > - ret = pipe()->dewarper_->init(params); > > + ret = pipe()->dewarper_->loadDewarpParams(params, dewarpParams_); > > if (ret) > > return ret; > > > > @@ -1058,7 +1059,7 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c) > > PixelFormatInfo::info(ispCfg.pixelFormat) > > .stride(ispCfg.size.width, 0); > > > > - ret = dewarper_->configure(ispCfg, { cfg }); > > + ret = dewarper_->configure(ispCfg, { cfg }, data->dewarpParams_); > > if (ret) > > return ret; > > > > -- > > 2.53.0 > >
Hi Barnabás, Thank you for the review. Quoting Barnabás Pőcze (2026-10-09 10:11:06) > Hi > > 2026. 10. 08. 18:50 keltezéssel, Stefan Klug írta: > > The dewarper is only instantiated once per pipeline. One pipeline can > > support multiple cameras with different sets of dewarp parmeters. > > > > In the current implementation the dewarp parameters are stored in the > > dewarper when the tuning file is loaded at init() time. Therefore the > > tuning file loaded last will overwrite the dewarp params for all > > cameras. > > > > Fix that by storing the dewarp parameters in the camera data and passing > > them to the dewarper at configure() time. > > > > While at it, mention the dwNew parameter in the yaml code sample. > > > > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com> > > --- > > .../internal/converter/converter_dw100.h | 8 ++-- > > src/libcamera/converter/converter_dw100.cpp | 39 +++++++++++++------ > > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 5 ++- > > 3 files changed, 36 insertions(+), 16 deletions(-) > > > > diff --git a/include/libcamera/internal/converter/converter_dw100.h b/include/libcamera/internal/converter/converter_dw100.h > > index 003f5eb954e9..029810bb4fa6 100644 > > --- a/include/libcamera/internal/converter/converter_dw100.h > > +++ b/include/libcamera/internal/converter/converter_dw100.h > > @@ -8,6 +8,7 @@ > > #pragma once > > > > #include <memory> > > +#include <optional> > > #include <queue> > > > > #include <libcamera/control_ids.h> > > @@ -31,11 +32,12 @@ public: > > > > static std::unique_ptr<ConverterDW100Module> createModule(DeviceEnumerator *enumerator); > > > > - int init(const ValueNode ¶ms); > > + int loadDewarpParams(const ValueNode ¶ms, std::optional<Dw100VertexMap::DewarpParams> &dewarpParams); > > > > int configure(const StreamConfiguration &inputCfg, > > const std::vector<std::reference_wrapper<const StreamConfiguration>> > > - &outputCfg); > > + &outputCfg, > > + const std::optional<const Dw100VertexMap::DewarpParams> dewarpParams); > > Was this meant to be > > const std::optional<Dw100VertexMap::DewarpParams> &dewarpParams Yes that would be better, fixed it. > > ? > > > > bool isConfigured(const Stream *stream) const; > > > > Size adjustInputSize(const PixelFormat &pixFmt, const Size &size, > > @@ -75,7 +77,7 @@ private: > > }; > > > > std::map<const Stream *, VertexMapInfo> vertexMaps_; > > - std::optional<Dw100VertexMap::DewarpParams> dewarpParams_; > > + bool hasDewarpParams_; > > I think I'd add ` = false` to not have it ever be uninitialized. > Or in the constructor maybe. Thanks, I added the initialization to the constructor. This is actually a bug as updateControlInfos() might get called before configure(). > > > > unsigned int inputBufferCount_; > > V4L2M2MConverter converter_; > > Rectangle sensorCrop_; > > diff --git a/src/libcamera/converter/converter_dw100.cpp b/src/libcamera/converter/converter_dw100.cpp > > index 300ea808ceff..ba7337b4abdd 100644 > > --- a/src/libcamera/converter/converter_dw100.cpp > > +++ b/src/libcamera/converter/converter_dw100.cpp > > @@ -72,11 +72,14 @@ ConverterDW100Module::createModule(DeviceEnumerator *enumerator) > > } > > > > /** > > - * \brief Initialize the module with configuration data > > + * \brief Load dewarp parameters from configuration file > > * \param[in] params The config parameters > > + * \param[out] dewarpParams The dewarp parameters > > * > > - * This function shall be called from the pipeline handler to initialize the > > - * module with the provided parameters. > > + * This function shall be called from the pipeline handler to load dewarp > > + * parameters from a tuning file. The dewarpParams can then be passed to > > + * configure() to configure the dewarper. If the tuning file does not contain > > + * any property, 0 is returned and dewarpParams is set to nullopt_t. > > This is a very minor thing, but I'd say > > std::nullopt > > since `nullopt_t` is the type, not the value. Fair enough. Fixed. > > > > * > > * A typical tuning file entry for the dewarper looks like this: > > * \code{.unparsed} > > @@ -87,6 +90,7 @@ ConverterDW100Module::createModule(DeviceEnumerator *enumerator) > > * 0.0, 1.0, 0.0, > > * 0.0, 0.0, 1.0, > > * ] > > + * cmNew: <optional new camera matrix> > > * coefficients: [ > > * 0,0,0,0,0, > > * ] > > @@ -98,9 +102,11 @@ ConverterDW100Module::createModule(DeviceEnumerator *enumerator) > > * \sa Dw100VertexMap::setDewarpParams() > > * \return 0 if successful, an error code otherwise > > */ > > -int ConverterDW100Module::init(const ValueNode ¶ms) > > +int ConverterDW100Module::loadDewarpParams(const ValueNode ¶ms, > > + std::optional<Dw100VertexMap::DewarpParams> &dewarpParams) > > Wouldn't it make sense to have this be a static member function of `Dw100VertexMap::DewarpParams` ? > As far as I can see there is nothing device dependent in here. Technically it it doesn't matter and indeed the function is fully self contained. I looked at it, but couldn't convince myself that that would actually be better. To me the DewarpParams is an internal type, whilst the tuning file format and therefore the parsing of the ValueNode is specific to the DewarperModule. So I prefer having that logic in the DewarperModule. Having it as normal member instead of a static member imho makes it easier for the calling code: dewarper->loadDewarpParams(params); dewarper->configure(params); instead of TheDewarperType::loadParams(params); dewarper->configure(params); As everything is routed through a single point which makes it less likely to fail if we ever replace the dewarper by a different type. Best regards, Stefan > > > > > { > > Dw100VertexMap::DewarpParams dp; > > + dewarpParams.reset(); > > > > auto &cm = params["cm"]; > > auto &coefficients = params["coefficients"]; > > @@ -153,17 +159,26 @@ int ConverterDW100Module::init(const ValueNode ¶ms) > > dp.cmNew = dp.cm; > > } > > > > - dewarpParams_ = dp; > > + dewarpParams = dp; > > > > return 0; > > } > > > > [...]
diff --git a/include/libcamera/internal/converter/converter_dw100.h b/include/libcamera/internal/converter/converter_dw100.h index 003f5eb954e9..029810bb4fa6 100644 --- a/include/libcamera/internal/converter/converter_dw100.h +++ b/include/libcamera/internal/converter/converter_dw100.h @@ -8,6 +8,7 @@ #pragma once #include <memory> +#include <optional> #include <queue> #include <libcamera/control_ids.h> @@ -31,11 +32,12 @@ public: static std::unique_ptr<ConverterDW100Module> createModule(DeviceEnumerator *enumerator); - int init(const ValueNode ¶ms); + int loadDewarpParams(const ValueNode ¶ms, std::optional<Dw100VertexMap::DewarpParams> &dewarpParams); int configure(const StreamConfiguration &inputCfg, const std::vector<std::reference_wrapper<const StreamConfiguration>> - &outputCfg); + &outputCfg, + const std::optional<const Dw100VertexMap::DewarpParams> dewarpParams); bool isConfigured(const Stream *stream) const; Size adjustInputSize(const PixelFormat &pixFmt, const Size &size, @@ -75,7 +77,7 @@ private: }; std::map<const Stream *, VertexMapInfo> vertexMaps_; - std::optional<Dw100VertexMap::DewarpParams> dewarpParams_; + bool hasDewarpParams_; unsigned int inputBufferCount_; V4L2M2MConverter converter_; Rectangle sensorCrop_; diff --git a/src/libcamera/converter/converter_dw100.cpp b/src/libcamera/converter/converter_dw100.cpp index 300ea808ceff..ba7337b4abdd 100644 --- a/src/libcamera/converter/converter_dw100.cpp +++ b/src/libcamera/converter/converter_dw100.cpp @@ -72,11 +72,14 @@ ConverterDW100Module::createModule(DeviceEnumerator *enumerator) } /** - * \brief Initialize the module with configuration data + * \brief Load dewarp parameters from configuration file * \param[in] params The config parameters + * \param[out] dewarpParams The dewarp parameters * - * This function shall be called from the pipeline handler to initialize the - * module with the provided parameters. + * This function shall be called from the pipeline handler to load dewarp + * parameters from a tuning file. The dewarpParams can then be passed to + * configure() to configure the dewarper. If the tuning file does not contain + * any property, 0 is returned and dewarpParams is set to nullopt_t. * * A typical tuning file entry for the dewarper looks like this: * \code{.unparsed} @@ -87,6 +90,7 @@ ConverterDW100Module::createModule(DeviceEnumerator *enumerator) * 0.0, 1.0, 0.0, * 0.0, 0.0, 1.0, * ] + * cmNew: <optional new camera matrix> * coefficients: [ * 0,0,0,0,0, * ] @@ -98,9 +102,11 @@ ConverterDW100Module::createModule(DeviceEnumerator *enumerator) * \sa Dw100VertexMap::setDewarpParams() * \return 0 if successful, an error code otherwise */ -int ConverterDW100Module::init(const ValueNode ¶ms) +int ConverterDW100Module::loadDewarpParams(const ValueNode ¶ms, + std::optional<Dw100VertexMap::DewarpParams> &dewarpParams) { Dw100VertexMap::DewarpParams dp; + dewarpParams.reset(); auto &cm = params["cm"]; auto &coefficients = params["coefficients"]; @@ -153,17 +159,26 @@ int ConverterDW100Module::init(const ValueNode ¶ms) dp.cmNew = dp.cm; } - dewarpParams_ = dp; + dewarpParams = dp; return 0; } /** - * \copydoc libcamera::V4L2M2MConverter::configure + * \brief Configure a the dw100 converter module + * \param[in] inputCfg Input stream configuration + * \param[in] outputCfgs A list of output stream configurations + * \param[in] dewarpParams The lens dewarp parameters to apply + * + * Configures the converter for the given input and output stream configurations + * and an optional set of dewarp parameters. + * + * \return 0 on success or a negative error code otherwise */ int ConverterDW100Module::configure(const StreamConfiguration &inputCfg, const std::vector<std::reference_wrapper<const StreamConfiguration>> - &outputCfgs) + &outputCfgs, + const std::optional<const Dw100VertexMap::DewarpParams> dewarpParams) { int ret; @@ -174,6 +189,8 @@ int ConverterDW100Module::configure(const StreamConfiguration &inputCfg, inputBufferCount_ = inputCfg.bufferCount; + hasDewarpParams_ = dewarpParams.has_value(); + for (auto &ref : outputCfgs) { const auto &outputCfg = ref.get(); auto &info = vertexMaps_[outputCfg.stream()]; @@ -182,8 +199,8 @@ int ConverterDW100Module::configure(const StreamConfiguration &inputCfg, vertexMap.setOutputSize(outputCfg.size); vertexMap.setSensorCrop(sensorCrop_); - if (dewarpParams_) - vertexMap.setDewarpParams(*dewarpParams_); + if (dewarpParams) + vertexMap.setDewarpParams(*dewarpParams); info.update = true; } @@ -363,7 +380,7 @@ void ConverterDW100Module::updateControlInfos(const Stream *stream, ControlInfoM controls[&controls::ScalerCrop] = ControlInfo(Rectangle(sensorCrop_.x, sensorCrop_.y, 1, 1), sensorCrop_, sensorCrop_); - if (dewarpParams_.has_value()) + if (hasDewarpParams_) controls[&controls::LensDewarpEnable] = ControlInfo(false, true, true); if (!converter_.supportsRequests()) @@ -424,7 +441,7 @@ void ConverterDW100Module::populateMetadata(const Stream *stream, ControlList &m meta.set(controls::ScalerCrop, vertexMap.effectiveScalerCrop()); - if (dewarpParams_.has_value()) + if (hasDewarpParams_) meta.set(controls::LensDewarpEnable, vertexMap.lensDewarpEnable()); } diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp index 96382c93a427..39fe0f0964d7 100644 --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp @@ -128,6 +128,7 @@ public: bool canUseDewarper_; bool usesDewarper_; + std::optional<Dw100VertexMap::DewarpParams> dewarpParams_; private: void paramsComputed(unsigned int frame, unsigned int bytesused); @@ -462,7 +463,7 @@ int RkISP1CameraData::loadTuningFile(const std::string &path) if (!params) continue; - ret = pipe()->dewarper_->init(params); + ret = pipe()->dewarper_->loadDewarpParams(params, dewarpParams_); if (ret) return ret; @@ -1058,7 +1059,7 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c) PixelFormatInfo::info(ispCfg.pixelFormat) .stride(ispCfg.size.width, 0); - ret = dewarper_->configure(ispCfg, { cfg }); + ret = dewarper_->configure(ispCfg, { cfg }, data->dewarpParams_); if (ret) return ret;
The dewarper is only instantiated once per pipeline. One pipeline can support multiple cameras with different sets of dewarp parmeters. In the current implementation the dewarp parameters are stored in the dewarper when the tuning file is loaded at init() time. Therefore the tuning file loaded last will overwrite the dewarp params for all cameras. Fix that by storing the dewarp parameters in the camera data and passing them to the dewarper at configure() time. While at it, mention the dwNew parameter in the yaml code sample. Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com> --- .../internal/converter/converter_dw100.h | 8 ++-- src/libcamera/converter/converter_dw100.cpp | 39 +++++++++++++------ src/libcamera/pipeline/rkisp1/rkisp1.cpp | 5 ++- 3 files changed, 36 insertions(+), 16 deletions(-)