| Message ID | 20251023144841.403689-32-stefan.klug@ideasonboard.com |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
Quoting Stefan Klug (2025-10-23 15:48:32) > Load the dewarp parameters from the tuning file. > > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com> > > --- > > Changes in v2: > - Dropped unused variable > --- > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 33 +++++++++++++++++++++++- > 1 file changed, 32 insertions(+), 1 deletion(-) > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > index eaf82d0f1097..2280a5554f5a 100644 > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > @@ -125,6 +125,11 @@ public: > */ > MediaPipeline pipe_; > > + struct DewarpParms { > + Matrix<double, 3, 3> cm; > + std::vector<double> coeffs; > + }; > + std::optional<DewarpParms> dewarpParams_; Can we push these out of the rkisp1.cpp and into the dw100 ? > bool canUseDewarper_; > bool usesDewarper_; > > @@ -458,8 +463,30 @@ int RkISP1CameraData::loadTuningFile(const std::string &path) > > const auto &algos = (*data)["algorithms"].asList(); > for (const auto &algo : algos) { > - if (algo.contains("Dewarp")) > + const auto ¶ms = algo["Dewarp"]; I don't remember if I've said this on the series yet, but I don't think Dewarp should be in the algorithms section of the tuning file. I think we should have a specific 'convertors'(or otherwise) key to put this under. I really don't want the IPA to have to have for algo in algorithms: if algo in [list of probably not algo] return; process(algo); > + if (params) { > canUseDewarper_ = true; > + DewarpParms dp; > + if (params["cm"]) { > + const auto &cm = params["cm"].get<Matrix<double, 3, 3>>(); > + if (!cm) { > + LOG(RkISP1, Error) << "Dewarp parameters are missing 'cm' value"; > + return -EINVAL; > + } > + dp.cm = *cm; > + } > + > + if (params["coefficients"]) { > + const auto &coeffs = params["coefficients"].getList<double>(); > + if (!coeffs) { > + LOG(RkISP1, Error) << "Dewarp parameters 'coefficients' value is not a list"; > + return -EINVAL; > + } > + dp.coeffs = *coeffs; > + } > + Same here - can we put this parsing into the dw100.cpp if it's specific to that. I'd like to think about how can we make it minimally possible to introduce the dw100 on the ISI pipeline handler as well as the RKISP1. What can we do to make the interface such that all the common handling is not duplicated. -- Kieran > + dewarpParams_ = dp; > + } > } > > return 0; > @@ -1050,6 +1077,10 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c) > auto &vertexMap = dewarper_->vertexMap(cfg.stream()); > vertexMap.setSensorCrop(sensorCrop); > vertexMap.setTransform(config->combinedTransform()); > + if (data->dewarpParams_) { > + vertexMap.setDewarpParams(data->dewarpParams_->cm, > + data->dewarpParams_->coeffs); > + } > data->properties_.set(properties::ScalerCropMaximum, sensorCrop); > > /* > -- > 2.48.1 >
Quoting Stefan Klug (2025-10-23 23:48:32) > Load the dewarp parameters from the tuning file. > > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com> Looks good to me. Reviewed-by: Paul Elder <paul.elder@ideasonboard.com> > > --- > > Changes in v2: > - Dropped unused variable > --- > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 33 +++++++++++++++++++++++- > 1 file changed, 32 insertions(+), 1 deletion(-) > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > index eaf82d0f1097..2280a5554f5a 100644 > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > @@ -125,6 +125,11 @@ public: > */ > MediaPipeline pipe_; > > + struct DewarpParms { > + Matrix<double, 3, 3> cm; > + std::vector<double> coeffs; > + }; > + std::optional<DewarpParms> dewarpParams_; > bool canUseDewarper_; > bool usesDewarper_; > > @@ -458,8 +463,30 @@ int RkISP1CameraData::loadTuningFile(const std::string &path) > > const auto &algos = (*data)["algorithms"].asList(); > for (const auto &algo : algos) { > - if (algo.contains("Dewarp")) > + const auto ¶ms = algo["Dewarp"]; > + if (params) { > canUseDewarper_ = true; > + DewarpParms dp; > + if (params["cm"]) { > + const auto &cm = params["cm"].get<Matrix<double, 3, 3>>(); > + if (!cm) { > + LOG(RkISP1, Error) << "Dewarp parameters are missing 'cm' value"; > + return -EINVAL; > + } > + dp.cm = *cm; > + } > + > + if (params["coefficients"]) { > + const auto &coeffs = params["coefficients"].getList<double>(); > + if (!coeffs) { > + LOG(RkISP1, Error) << "Dewarp parameters 'coefficients' value is not a list"; > + return -EINVAL; > + } > + dp.coeffs = *coeffs; > + } > + > + dewarpParams_ = dp; > + } > } > > return 0; > @@ -1050,6 +1077,10 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c) > auto &vertexMap = dewarper_->vertexMap(cfg.stream()); > vertexMap.setSensorCrop(sensorCrop); > vertexMap.setTransform(config->combinedTransform()); > + if (data->dewarpParams_) { > + vertexMap.setDewarpParams(data->dewarpParams_->cm, > + data->dewarpParams_->coeffs); > + } > data->properties_.set(properties::ScalerCropMaximum, sensorCrop); > > /* > -- > 2.48.1 >
Hi Kieran, Thank you for the review. Quoting Kieran Bingham (2025-11-06 13:17:54) > Quoting Stefan Klug (2025-10-23 15:48:32) > > Load the dewarp parameters from the tuning file. > > > > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com> > > > > --- > > > > Changes in v2: > > - Dropped unused variable > > --- > > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 33 +++++++++++++++++++++++- > > 1 file changed, 32 insertions(+), 1 deletion(-) > > > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > > index eaf82d0f1097..2280a5554f5a 100644 > > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > > @@ -125,6 +125,11 @@ public: > > */ > > MediaPipeline pipe_; > > > > + struct DewarpParms { > > + Matrix<double, 3, 3> cm; > > + std::vector<double> coeffs; > > + }; > > + std::optional<DewarpParms> dewarpParams_; > > Can we push these out of the rkisp1.cpp and into the dw100 ? You mean passing that struct to dewarper->setDewarpeParams as they are never set alone? Yes, I can do that. > > > > > bool canUseDewarper_; > > bool usesDewarper_; > > > > @@ -458,8 +463,30 @@ int RkISP1CameraData::loadTuningFile(const std::string &path) > > > > const auto &algos = (*data)["algorithms"].asList(); > > for (const auto &algo : algos) { > > - if (algo.contains("Dewarp")) > > + const auto ¶ms = algo["Dewarp"]; > > > I don't remember if I've said this on the series yet, but I don't think > Dewarp should be in the algorithms section of the tuning file. > > I think we should have a specific 'convertors'(or otherwise) key to put this under. Yes, you did in https://patchwork.libcamera.org/patch/24528/ . And for the same reasons I mentioned there I'm still unsure if we should do that. Maybe time for a Tuesday topic... > > > I really don't want the IPA to have to have > > for algo in algorithms: > if algo in [list of probably not algo] > return; > > process(algo); > > > > > + if (params) { > > canUseDewarper_ = true; > > + DewarpParms dp; > > + if (params["cm"]) { > > + const auto &cm = params["cm"].get<Matrix<double, 3, 3>>(); > > + if (!cm) { > > + LOG(RkISP1, Error) << "Dewarp parameters are missing 'cm' value"; > > + return -EINVAL; > > + } > > + dp.cm = *cm; > > + } > > + > > + if (params["coefficients"]) { > > + const auto &coeffs = params["coefficients"].getList<double>(); > > + if (!coeffs) { > > + LOG(RkISP1, Error) << "Dewarp parameters 'coefficients' value is not a list"; > > + return -EINVAL; > > + } > > + dp.coeffs = *coeffs; > > + } > > + > > Same here - can we put this parsing into the dw100.cpp if it's specific > to that. > > I'd like to think about how can we make it minimally possible to > introduce the dw100 on the ISI pipeline handler as well as the RKISP1. > > What can we do to make the interface such that all the common handling > is not duplicated. I understand the rationale. My understanding or maybe gut feeling was that the classes in libcamera/converter should only provide the technical base and the actual control handling lives inside libipa or the pipeline handlers. Do we really want to mix yaml/control handling into the dw100 class? I agree that it also pollutes the rkisp1 class and makes it difficult to reuse. Maybe I need to give it a try. Ahh I wish we had a concept for modular pipelines... Best regards, Stefan > > -- > Kieran > > > > + dewarpParams_ = dp; > > + } > > } > > > > return 0; > > @@ -1050,6 +1077,10 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c) > > auto &vertexMap = dewarper_->vertexMap(cfg.stream()); > > vertexMap.setSensorCrop(sensorCrop); > > vertexMap.setTransform(config->combinedTransform()); > > + if (data->dewarpParams_) { > > + vertexMap.setDewarpParams(data->dewarpParams_->cm, > > + data->dewarpParams_->coeffs); > > + } > > data->properties_.set(properties::ScalerCropMaximum, sensorCrop); > > > > /* > > -- > > 2.48.1 > >
Quoting Stefan Klug (2025-11-07 16:38:54) > Hi Kieran, > > Thank you for the review. > > Quoting Kieran Bingham (2025-11-06 13:17:54) > > Quoting Stefan Klug (2025-10-23 15:48:32) > > > Load the dewarp parameters from the tuning file. > > > > > > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com> > > > > > > --- > > > > > > Changes in v2: > > > - Dropped unused variable > > > --- > > > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 33 +++++++++++++++++++++++- > > > 1 file changed, 32 insertions(+), 1 deletion(-) > > > > > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > > > index eaf82d0f1097..2280a5554f5a 100644 > > > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp > > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > > > @@ -125,6 +125,11 @@ public: > > > */ > > > MediaPipeline pipe_; > > > > > > + struct DewarpParms { > > > + Matrix<double, 3, 3> cm; > > > + std::vector<double> coeffs; > > > + }; > > > + std::optional<DewarpParms> dewarpParams_; > > > > Can we push these out of the rkisp1.cpp and into the dw100 ? > > You mean passing that struct to dewarper->setDewarpeParams as they are > never set alone? Yes, I can do that. > > > > > > > > > > bool canUseDewarper_; > > > bool usesDewarper_; > > > > > > @@ -458,8 +463,30 @@ int RkISP1CameraData::loadTuningFile(const std::string &path) > > > > > > const auto &algos = (*data)["algorithms"].asList(); > > > for (const auto &algo : algos) { > > > - if (algo.contains("Dewarp")) > > > + const auto ¶ms = algo["Dewarp"]; > > > > > > I don't remember if I've said this on the series yet, but I don't think > > Dewarp should be in the algorithms section of the tuning file. > > > > I think we should have a specific 'convertors'(or otherwise) key to put this under. > > Yes, you did in https://patchwork.libcamera.org/patch/24528/ . And for > the same reasons I mentioned there I'm still unsure if we should do > that. Maybe time for a Tuesday topic... Aha, bringing that comment forward to here: > Kieran said: > > I think this means that really - we shouldn't put dewarp config under > the algorithms. > > It should be a separate base key. > > Perhaps under convertors ? ===== Stefan Reply ===== Yes, I didn't like that either. On the other hand it feels a bit superfluous to add another top level element for this single instance. For example on the soft-gpu-isp side, implementing this as part of the gpu pipeline would be quite easy and then it is suddenly an algorithm again? One could also argue that algorithms contains all the algorithms implemented in the pipeline. How that is done hardware wise is an implementation detail. There are similar issues with the current algorithm blocks on rkisp1. These are very centric to the hardware-blocks but could/should potentially be more centered around the algorithms supported by libipa. Ahh difficult :-) ===== Stefan Reply ===== I think the ability to have a GPU dewarp *proves* my point that we should not have code that does this in libipa: > > +++ b/src/ipa/libipa/module.h > > @@ -74,6 +74,10 @@ private: > > int createAlgorithm(Context &context, const YamlObject &data) > > { > > const auto &[name, algoData] = *data.asDict().begin(); > > + > > + if (name == "Dewarp") > > + return 0; because now - dewarp can't be managed by the IPA there if we did want it to. In the RKISP1 case, it is not the IPA/ISP handling the dewarp - it's a convertor/post-processor. If the (Soft)ISP were in control of the dewarp, I would agree there can be a dewarp key under algorithms at that point. But I think we have to distinguish the separation. Maybe 'algorithms' itself should actually be: ipa: algorithms or maybe we assume that is implicit already. > > I really don't want the IPA to have to have > > > > for algo in algorithms: > > if algo in [list of probably not algo] > > return; > > > > process(algo); > > > > > > > > > + if (params) { > > > canUseDewarper_ = true; > > > + DewarpParms dp; > > > + if (params["cm"]) { > > > + const auto &cm = params["cm"].get<Matrix<double, 3, 3>>(); > > > + if (!cm) { > > > + LOG(RkISP1, Error) << "Dewarp parameters are missing 'cm' value"; > > > + return -EINVAL; > > > + } > > > + dp.cm = *cm; > > > + } > > > + > > > + if (params["coefficients"]) { > > > + const auto &coeffs = params["coefficients"].getList<double>(); > > > + if (!coeffs) { > > > + LOG(RkISP1, Error) << "Dewarp parameters 'coefficients' value is not a list"; > > > + return -EINVAL; > > > + } > > > + dp.coeffs = *coeffs; > > > + } > > > + > > > > Same here - can we put this parsing into the dw100.cpp if it's specific > > to that. > > > > I'd like to think about how can we make it minimally possible to > > introduce the dw100 on the ISI pipeline handler as well as the RKISP1. > > > > What can we do to make the interface such that all the common handling > > is not duplicated. > > I understand the rationale. My understanding or maybe gut feeling was > that the classes in libcamera/converter should only provide the > technical base and the actual control handling lives inside libipa or > the pipeline handlers. Do we really want to mix yaml/control handling > into the dw100 class? I agree that it also pollutes the rkisp1 class and > makes it difficult to reuse. Yes, I really mean I think that yaml + control handling for the dw100 should be managed by the dw100 class! At most - all the rkisp1 pipeline handler should be doing is passing the yaml and requests in through an interface to let dw100 make it's decisionss. > > Maybe I need to give it a try. Ahh I wish we had a concept for modular > pipelines... Yes, that's what the above works towards in my view. Instead of having decisions for modules made by the rkisp1 - let the modules have control. DW100 is just 'one module'. -- Kieran > > Best regards, > Stefan > > > > > -- > > Kieran > > > > > > > + dewarpParams_ = dp; > > > + } > > > } > > > > > > return 0; > > > @@ -1050,6 +1077,10 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c) > > > auto &vertexMap = dewarper_->vertexMap(cfg.stream()); > > > vertexMap.setSensorCrop(sensorCrop); > > > vertexMap.setTransform(config->combinedTransform()); > > > + if (data->dewarpParams_) { > > > + vertexMap.setDewarpParams(data->dewarpParams_->cm, > > > + data->dewarpParams_->coeffs); > > > + } > > > data->properties_.set(properties::ScalerCropMaximum, sensorCrop); > > > > > > /* > > > -- > > > 2.48.1 > > >
diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp index eaf82d0f1097..2280a5554f5a 100644 --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp @@ -125,6 +125,11 @@ public: */ MediaPipeline pipe_; + struct DewarpParms { + Matrix<double, 3, 3> cm; + std::vector<double> coeffs; + }; + std::optional<DewarpParms> dewarpParams_; bool canUseDewarper_; bool usesDewarper_; @@ -458,8 +463,30 @@ int RkISP1CameraData::loadTuningFile(const std::string &path) const auto &algos = (*data)["algorithms"].asList(); for (const auto &algo : algos) { - if (algo.contains("Dewarp")) + const auto ¶ms = algo["Dewarp"]; + if (params) { canUseDewarper_ = true; + DewarpParms dp; + if (params["cm"]) { + const auto &cm = params["cm"].get<Matrix<double, 3, 3>>(); + if (!cm) { + LOG(RkISP1, Error) << "Dewarp parameters are missing 'cm' value"; + return -EINVAL; + } + dp.cm = *cm; + } + + if (params["coefficients"]) { + const auto &coeffs = params["coefficients"].getList<double>(); + if (!coeffs) { + LOG(RkISP1, Error) << "Dewarp parameters 'coefficients' value is not a list"; + return -EINVAL; + } + dp.coeffs = *coeffs; + } + + dewarpParams_ = dp; + } } return 0; @@ -1050,6 +1077,10 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c) auto &vertexMap = dewarper_->vertexMap(cfg.stream()); vertexMap.setSensorCrop(sensorCrop); vertexMap.setTransform(config->combinedTransform()); + if (data->dewarpParams_) { + vertexMap.setDewarpParams(data->dewarpParams_->cm, + data->dewarpParams_->coeffs); + } data->properties_.set(properties::ScalerCropMaximum, sensorCrop); /*
Load the dewarp parameters from the tuning file. Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com> --- Changes in v2: - Dropped unused variable --- src/libcamera/pipeline/rkisp1/rkisp1.cpp | 33 +++++++++++++++++++++++- 1 file changed, 32 insertions(+), 1 deletion(-)