| Message ID | 20251014075252.2876485-9-stefan.klug@ideasonboard.com |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
Hi 2025. 10. 14. 9:52 keltezéssel, Stefan Klug írta: > There is no need to recalculate the sampling positions over and over. > Pass them as parameter into the sampling function. The vectors are still > kept inside the loop as this is also a preparatory change for the > upcoming refactoring. > > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com> > --- > src/ipa/rkisp1/algorithms/lsc.cpp | 37 +++++++++++++++---------------- > 1 file changed, 18 insertions(+), 19 deletions(-) > > diff --git a/src/ipa/rkisp1/algorithms/lsc.cpp b/src/ipa/rkisp1/algorithms/lsc.cpp > index 0c1bb470c346..2fbc8bd1b242 100644 > --- a/src/ipa/rkisp1/algorithms/lsc.cpp > +++ b/src/ipa/rkisp1/algorithms/lsc.cpp > @@ -89,7 +89,9 @@ public: > > private: > std::vector<double> sizesListToPositions(const std::vector<double> &sizes); > - std::vector<uint16_t> samplePolynomial(const LscPolynomial &poly); > + std::vector<uint16_t> samplePolynomial(const LscPolynomial &poly, > + const std::vector<double> &xPositions, > + const std::vector<double> &yPositions); > > Size sensorSize_; > Rectangle cropRectangle_; > @@ -129,10 +131,13 @@ int LscPolynomialLoader::parseLscData(const YamlObject &yamlSets, > pgr->setReferenceImageSize(sensorSize_); > pgb->setReferenceImageSize(sensorSize_); > pb->setReferenceImageSize(sensorSize_); > - set.r = samplePolynomial(*pr); > - set.gr = samplePolynomial(*pgr); > - set.gb = samplePolynomial(*pgb); > - set.b = samplePolynomial(*pb); > + > + std::vector<double> xPos(sizesListToPositions(xSizes_)); > + std::vector<double> yPos(sizesListToPositions(ySizes_)); I suppose it might be just me, but I vastly prefer `=` in such cases. > + set.r = samplePolynomial(*pr, xPos, yPos); > + set.gr = samplePolynomial(*pgr, xPos, yPos); > + set.gb = samplePolynomial(*pgb, xPos, yPos); > + set.b = samplePolynomial(*pb, xPos, yPos); > } > > if (lscData.empty()) { > @@ -169,10 +174,10 @@ std::vector<double> LscPolynomialLoader::sizesListToPositions(const std::vector< > return positions; > } > > -std::vector<uint16_t> LscPolynomialLoader::samplePolynomial(const LscPolynomial &poly) > +std::vector<uint16_t> LscPolynomialLoader::samplePolynomial(const LscPolynomial &poly, > + const std::vector<double> &xPositions, > + const std::vector<double> &yPositions) Span<const double> xPositions, etc. ? > { > - constexpr int k = RKISP1_CIF_ISP_LSC_SAMPLES_MAX; > - > double m = poly.getM(); > double x0 = cropRectangle_.x / m; > double y0 = cropRectangle_.y / m; > @@ -180,18 +185,12 @@ std::vector<uint16_t> LscPolynomialLoader::samplePolynomial(const LscPolynomial > double h = cropRectangle_.height / m; > std::vector<uint16_t> samples; > > - ASSERT(xSizes_.size() * 2 + 1 == k); > - ASSERT(ySizes_.size() * 2 + 1 == k); Is it not worth to move these assertions instead of removing them? Reviewed-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> > - > - samples.reserve(k * k); > - > - std::vector<double> xPos(sizesListToPositions(xSizes_)); > - std::vector<double> yPos(sizesListToPositions(ySizes_)); > + samples.reserve(xPositions.size() * yPositions.size()); > > - for (int y = 0; y < k; y++) { > - for (int x = 0; x < k; x++) { > - double xp = x0 + xPos[x] * w; > - double yp = y0 + yPos[y] * h; > + for (double y : yPositions) { > + for (double x : xPositions) { > + double xp = x0 + x * w; > + double yp = y0 + y * h; > /* > * The hardware uses 2.10 fixed point format and limits > * the legal values to [1..3.999]. Scale and clamp the
Hi, Thank you for the review. Quoting Barnabás Pőcze (2025-10-14 10:25:56) > Hi > > 2025. 10. 14. 9:52 keltezéssel, Stefan Klug írta: > > There is no need to recalculate the sampling positions over and over. > > Pass them as parameter into the sampling function. The vectors are still > > kept inside the loop as this is also a preparatory change for the > > upcoming refactoring. > > > > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com> > > --- > > src/ipa/rkisp1/algorithms/lsc.cpp | 37 +++++++++++++++---------------- > > 1 file changed, 18 insertions(+), 19 deletions(-) > > > > diff --git a/src/ipa/rkisp1/algorithms/lsc.cpp b/src/ipa/rkisp1/algorithms/lsc.cpp > > index 0c1bb470c346..2fbc8bd1b242 100644 > > --- a/src/ipa/rkisp1/algorithms/lsc.cpp > > +++ b/src/ipa/rkisp1/algorithms/lsc.cpp > > @@ -89,7 +89,9 @@ public: > > > > private: > > std::vector<double> sizesListToPositions(const std::vector<double> &sizes); > > - std::vector<uint16_t> samplePolynomial(const LscPolynomial &poly); > > + std::vector<uint16_t> samplePolynomial(const LscPolynomial &poly, > > + const std::vector<double> &xPositions, > > + const std::vector<double> &yPositions); > > > > Size sensorSize_; > > Rectangle cropRectangle_; > > @@ -129,10 +131,13 @@ int LscPolynomialLoader::parseLscData(const YamlObject &yamlSets, > > pgr->setReferenceImageSize(sensorSize_); > > pgb->setReferenceImageSize(sensorSize_); > > pb->setReferenceImageSize(sensorSize_); > > - set.r = samplePolynomial(*pr); > > - set.gr = samplePolynomial(*pgr); > > - set.gb = samplePolynomial(*pgb); > > - set.b = samplePolynomial(*pb); > > + > > + std::vector<double> xPos(sizesListToPositions(xSizes_)); > > + std::vector<double> yPos(sizesListToPositions(ySizes_)); > > I suppose it might be just me, but I vastly prefer `=` in such cases. > > > > + set.r = samplePolynomial(*pr, xPos, yPos); > > + set.gr = samplePolynomial(*pgr, xPos, yPos); > > + set.gb = samplePolynomial(*pgb, xPos, yPos); > > + set.b = samplePolynomial(*pb, xPos, yPos); > > } > > > > if (lscData.empty()) { > > @@ -169,10 +174,10 @@ std::vector<double> LscPolynomialLoader::sizesListToPositions(const std::vector< > > return positions; > > } > > > > -std::vector<uint16_t> LscPolynomialLoader::samplePolynomial(const LscPolynomial &poly) > > +std::vector<uint16_t> LscPolynomialLoader::samplePolynomial(const LscPolynomial &poly, > > + const std::vector<double> &xPositions, > > + const std::vector<double> &yPositions) > > Span<const double> xPositions, etc. As it is local to that compile unit only, I guess it doesn't make a big difference. But making it a habit might be worthwhile. > > ? > > > > { > > - constexpr int k = RKISP1_CIF_ISP_LSC_SAMPLES_MAX; > > - > > double m = poly.getM(); > > double x0 = cropRectangle_.x / m; > > double y0 = cropRectangle_.y / m; > > @@ -180,18 +185,12 @@ std::vector<uint16_t> LscPolynomialLoader::samplePolynomial(const LscPolynomial > > double h = cropRectangle_.height / m; > > std::vector<uint16_t> samples; > > > > - ASSERT(xSizes_.size() * 2 + 1 == k); > > - ASSERT(ySizes_.size() * 2 + 1 == k); > > Is it not worth to move these assertions instead of removing them? The sizes vectors are checked in parseSizes(). The sampling is independent of the number of sampling points, so I believ it is fine to just drop these. > > > Reviewed-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> Thanks, Stefan > > > > - > > - samples.reserve(k * k); > > - > > - std::vector<double> xPos(sizesListToPositions(xSizes_)); > > - std::vector<double> yPos(sizesListToPositions(ySizes_)); > > + samples.reserve(xPositions.size() * yPositions.size()); > > > > - for (int y = 0; y < k; y++) { > > - for (int x = 0; x < k; x++) { > > - double xp = x0 + xPos[x] * w; > > - double yp = y0 + yPos[y] * h; > > + for (double y : yPositions) { > > + for (double x : xPositions) { > > + double xp = x0 + x * w; > > + double yp = y0 + y * h; > > /* > > * The hardware uses 2.10 fixed point format and limits > > * the legal values to [1..3.999]. Scale and clamp the >
Quoting Stefan Klug (2025-10-14 12:22:58) > Hi, > > Thank you for the review. > > Quoting Barnabás Pőcze (2025-10-14 10:25:56) > > Hi > > > > 2025. 10. 14. 9:52 keltezéssel, Stefan Klug írta: > > > There is no need to recalculate the sampling positions over and over. > > > Pass them as parameter into the sampling function. The vectors are still > > > kept inside the loop as this is also a preparatory change for the > > > upcoming refactoring. > > > > > > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com> > > > --- > > > src/ipa/rkisp1/algorithms/lsc.cpp | 37 +++++++++++++++---------------- > > > 1 file changed, 18 insertions(+), 19 deletions(-) > > > > > > diff --git a/src/ipa/rkisp1/algorithms/lsc.cpp b/src/ipa/rkisp1/algorithms/lsc.cpp > > > index 0c1bb470c346..2fbc8bd1b242 100644 > > > --- a/src/ipa/rkisp1/algorithms/lsc.cpp > > > +++ b/src/ipa/rkisp1/algorithms/lsc.cpp > > > @@ -89,7 +89,9 @@ public: > > > > > > private: > > > std::vector<double> sizesListToPositions(const std::vector<double> &sizes); > > > - std::vector<uint16_t> samplePolynomial(const LscPolynomial &poly); > > > + std::vector<uint16_t> samplePolynomial(const LscPolynomial &poly, > > > + const std::vector<double> &xPositions, > > > + const std::vector<double> &yPositions); > > > > > > Size sensorSize_; > > > Rectangle cropRectangle_; > > > @@ -129,10 +131,13 @@ int LscPolynomialLoader::parseLscData(const YamlObject &yamlSets, > > > pgr->setReferenceImageSize(sensorSize_); > > > pgb->setReferenceImageSize(sensorSize_); > > > pb->setReferenceImageSize(sensorSize_); > > > - set.r = samplePolynomial(*pr); > > > - set.gr = samplePolynomial(*pgr); > > > - set.gb = samplePolynomial(*pgb); > > > - set.b = samplePolynomial(*pb); > > > + > > > + std::vector<double> xPos(sizesListToPositions(xSizes_)); > > > + std::vector<double> yPos(sizesListToPositions(ySizes_)); > > > > I suppose it might be just me, but I vastly prefer `=` in such cases. I missed to comment on this one. Yes, I'll replace it. > > > > > > > + set.r = samplePolynomial(*pr, xPos, yPos); > > > + set.gr = samplePolynomial(*pgr, xPos, yPos); > > > + set.gb = samplePolynomial(*pgb, xPos, yPos); > > > + set.b = samplePolynomial(*pb, xPos, yPos); > > > } > > > > > > if (lscData.empty()) { > > > @@ -169,10 +174,10 @@ std::vector<double> LscPolynomialLoader::sizesListToPositions(const std::vector< > > > return positions; > > > } > > > > > > -std::vector<uint16_t> LscPolynomialLoader::samplePolynomial(const LscPolynomial &poly) > > > +std::vector<uint16_t> LscPolynomialLoader::samplePolynomial(const LscPolynomial &poly, > > > + const std::vector<double> &xPositions, > > > + const std::vector<double> &yPositions) > > > > Span<const double> xPositions, etc. > > As it is local to that compile unit only, I guess it doesn't make a big > difference. But making it a habit might be worthwhile. > > > > > ? > > > > > > > { > > > - constexpr int k = RKISP1_CIF_ISP_LSC_SAMPLES_MAX; > > > - > > > double m = poly.getM(); > > > double x0 = cropRectangle_.x / m; > > > double y0 = cropRectangle_.y / m; > > > @@ -180,18 +185,12 @@ std::vector<uint16_t> LscPolynomialLoader::samplePolynomial(const LscPolynomial > > > double h = cropRectangle_.height / m; > > > std::vector<uint16_t> samples; > > > > > > - ASSERT(xSizes_.size() * 2 + 1 == k); > > > - ASSERT(ySizes_.size() * 2 + 1 == k); > > > > Is it not worth to move these assertions instead of removing them? > > The sizes vectors are checked in parseSizes(). The sampling is > independent of the number of sampling points, so I believ it is fine to > just drop these. > > > > > > > Reviewed-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> > > Thanks, > Stefan > > > > > > > > - > > > - samples.reserve(k * k); > > > - > > > - std::vector<double> xPos(sizesListToPositions(xSizes_)); > > > - std::vector<double> yPos(sizesListToPositions(ySizes_)); > > > + samples.reserve(xPositions.size() * yPositions.size()); > > > > > > - for (int y = 0; y < k; y++) { > > > - for (int x = 0; x < k; x++) { > > > - double xp = x0 + xPos[x] * w; > > > - double yp = y0 + yPos[y] * h; > > > + for (double y : yPositions) { > > > + for (double x : xPositions) { > > > + double xp = x0 + x * w; > > > + double yp = y0 + y * h; > > > /* > > > * The hardware uses 2.10 fixed point format and limits > > > * the legal values to [1..3.999]. Scale and clamp the > >
diff --git a/src/ipa/rkisp1/algorithms/lsc.cpp b/src/ipa/rkisp1/algorithms/lsc.cpp index 0c1bb470c346..2fbc8bd1b242 100644 --- a/src/ipa/rkisp1/algorithms/lsc.cpp +++ b/src/ipa/rkisp1/algorithms/lsc.cpp @@ -89,7 +89,9 @@ public: private: std::vector<double> sizesListToPositions(const std::vector<double> &sizes); - std::vector<uint16_t> samplePolynomial(const LscPolynomial &poly); + std::vector<uint16_t> samplePolynomial(const LscPolynomial &poly, + const std::vector<double> &xPositions, + const std::vector<double> &yPositions); Size sensorSize_; Rectangle cropRectangle_; @@ -129,10 +131,13 @@ int LscPolynomialLoader::parseLscData(const YamlObject &yamlSets, pgr->setReferenceImageSize(sensorSize_); pgb->setReferenceImageSize(sensorSize_); pb->setReferenceImageSize(sensorSize_); - set.r = samplePolynomial(*pr); - set.gr = samplePolynomial(*pgr); - set.gb = samplePolynomial(*pgb); - set.b = samplePolynomial(*pb); + + std::vector<double> xPos(sizesListToPositions(xSizes_)); + std::vector<double> yPos(sizesListToPositions(ySizes_)); + set.r = samplePolynomial(*pr, xPos, yPos); + set.gr = samplePolynomial(*pgr, xPos, yPos); + set.gb = samplePolynomial(*pgb, xPos, yPos); + set.b = samplePolynomial(*pb, xPos, yPos); } if (lscData.empty()) { @@ -169,10 +174,10 @@ std::vector<double> LscPolynomialLoader::sizesListToPositions(const std::vector< return positions; } -std::vector<uint16_t> LscPolynomialLoader::samplePolynomial(const LscPolynomial &poly) +std::vector<uint16_t> LscPolynomialLoader::samplePolynomial(const LscPolynomial &poly, + const std::vector<double> &xPositions, + const std::vector<double> &yPositions) { - constexpr int k = RKISP1_CIF_ISP_LSC_SAMPLES_MAX; - double m = poly.getM(); double x0 = cropRectangle_.x / m; double y0 = cropRectangle_.y / m; @@ -180,18 +185,12 @@ std::vector<uint16_t> LscPolynomialLoader::samplePolynomial(const LscPolynomial double h = cropRectangle_.height / m; std::vector<uint16_t> samples; - ASSERT(xSizes_.size() * 2 + 1 == k); - ASSERT(ySizes_.size() * 2 + 1 == k); - - samples.reserve(k * k); - - std::vector<double> xPos(sizesListToPositions(xSizes_)); - std::vector<double> yPos(sizesListToPositions(ySizes_)); + samples.reserve(xPositions.size() * yPositions.size()); - for (int y = 0; y < k; y++) { - for (int x = 0; x < k; x++) { - double xp = x0 + xPos[x] * w; - double yp = y0 + yPos[y] * h; + for (double y : yPositions) { + for (double x : xPositions) { + double xp = x0 + x * w; + double yp = y0 + y * h; /* * The hardware uses 2.10 fixed point format and limits * the legal values to [1..3.999]. Scale and clamp the
There is no need to recalculate the sampling positions over and over. Pass them as parameter into the sampling function. The vectors are still kept inside the loop as this is also a preparatory change for the upcoming refactoring. Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com> --- src/ipa/rkisp1/algorithms/lsc.cpp | 37 +++++++++++++++---------------- 1 file changed, 18 insertions(+), 19 deletions(-)