| Message ID | 20240517080802.3896531-2-paul.elder@ideasonboard.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Paul, thanks for the patch. On Fri, May 17, 2024 at 05:07:59PM +0900, Paul Elder wrote: > Add support to the rkisp1 AGC to read histogram weights from the tuning > file. As controls for selecting the metering mode are not yet supported, > for now hardcode the matrix metering mode, which is the same as what the > AGC previously hardcoded. > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> > > --- > Changes in v3: > - support the new tuning file layout for interpolated matrices > - support both v10 and v12 > > Changes in v2: > - add default metering mode if none are read successfully from the > tuning file > - compute the predivider instead of using a table > --- > src/ipa/rkisp1/algorithms/agc.cpp | 94 ++++++++++++++++++++++++++++++- > src/ipa/rkisp1/algorithms/agc.h | 6 ++ > 2 files changed, 97 insertions(+), 3 deletions(-) > > diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp > index 5aad5c7c2..4af397bdc 100644 > --- a/src/ipa/rkisp1/algorithms/agc.cpp > +++ b/src/ipa/rkisp1/algorithms/agc.cpp > @@ -17,6 +17,8 @@ > #include <libcamera/control_ids.h> > #include <libcamera/ipa/core_ipa_interface.h> > > +#include "libcamera/internal/yaml_parser.h" > + > #include "libipa/histogram.h" > > /** > @@ -36,6 +38,76 @@ namespace ipa::rkisp1::algorithms { > > LOG_DEFINE_CATEGORY(RkISP1Agc) > > +int Agc::parseMeteringModes(IPAContext &context, const YamlObject &tuningData, > + const char *prop) > +{ > + const YamlObject &yamlMeteringModes = tuningData[prop]; > + if (!yamlMeteringModes.isDictionary()) { > + LOG(RkISP1Agc, Error) > + << "'" << prop << "' parameter not found in tuning file"; > + return -EINVAL; > + } > + > + for (const auto &[key, value] : yamlMeteringModes.asDict()) { > + if (controls::AeMeteringModeNameValueMap.find(key) == > + controls::AeMeteringModeNameValueMap.end()) { > + LOG(RkISP1Agc, Warning) > + << "Skipping unknown metering mode '" << key << "'"; > + continue; > + } > + > + for (const auto &[version, matrix] : value.asDict()) { > + std::vector<uint8_t> weights = > + matrix.getList<uint8_t>().value_or(std::vector<uint8_t>{}); > + if (weights.size() != context.hw->numHistogramWeights) > + continue; > + > + LOG(RkISP1Agc, Debug) > + << "Matched metering matrix mode " > + << key << ", version " << version; > + > + meteringModes_[controls::AeMeteringModeNameValueMap.at(key)] = weights; > + } > + } > + > + if (meteringModes_.empty()) { > + int32_t meteringModeId = controls::AeMeteringModeNameValueMap.at("MeteringMatrix"); > + std::vector<uint8_t> weights(context.hw->numHistogramWeights, 1); > + > + meteringModes_[meteringModeId] = weights; > + } > + > + return 0; > +} > + > +uint8_t Agc::predivider(Size &size) > +{ > + /* > + * The maximum number of pixels that could potentially be in one bin is > + * if all the pixels of the image are in it, multiplied by 3 for the > + * three color channels. The counter for each bin is 16 bits wide, so > + * `factor` thus contains the number of times we'd wrap around. This is > + * obviously the number of pixels that we need to skip to make sure > + * that we don't wrap around, but we compute the square root of it > + * instead, as the skip that we need to program is for both the x and y > + * directions. > + * > + * There's a bit of extra rounding math to make sure the rounding goes > + * the correct direction so that the square of the step is big enough > + * to encompass the `factor` number of pixels that we need to skip. > + */ I like that... THe rest look also good to me. Reviewed-by: Stefan Klug <stefan.klug@ideasonboard.com> Cheers, Stefan > + double factor = size.width * size.height * 3 / 65535.0; > + double root = std::sqrt(factor); > + uint8_t ret; > + > + if (std::pow(std::floor(root), 2) + 0.01 < factor) > + ret = static_cast<uint8_t>(std::ceil(root)); > + else > + ret = static_cast<uint8_t>(std::floor(root)); > + > + return std::clamp<uint8_t>(ret, 3, 127); > +} > + > Agc::Agc() > { > supportsRaw_ = true; > @@ -59,6 +131,10 @@ int Agc::init(IPAContext &context, const YamlObject &tuningData) > if (ret) > return ret; > > + ret = parseMeteringModes(context, tuningData, "AeMeteringMode"); > + if (ret) > + return ret; > + > context.ctrlMap.merge(controls()); > > return 0; > @@ -160,6 +236,7 @@ void Agc::prepare(IPAContext &context, const uint32_t frame, > frameContext.agc.gain = context.activeState.agc.automatic.gain; > } > > + /* \todo Remove this when we can set the below with controls */ > if (frame > 0) > return; > > @@ -178,14 +255,25 @@ void Agc::prepare(IPAContext &context, const uint32_t frame, > params->meas.hst_config.meas_window = context.configuration.agc.measureWindow; > /* Produce the luminance histogram. */ > params->meas.hst_config.mode = RKISP1_CIF_ISP_HISTOGRAM_MODE_Y_HISTOGRAM; > + > /* Set an average weighted histogram. */ > Span<uint8_t> weights{ > params->meas.hst_config.hist_weight, > context.hw->numHistogramWeights > }; > - std::fill(weights.begin(), weights.end(), 1); > - /* Step size can't be less than 3. */ > - params->meas.hst_config.histogram_predivider = 4; > + /* \todo Get this from control */ > + std::vector<uint8_t> &modeWeights = meteringModes_.at(controls::MeteringMatrix); > + std::copy(modeWeights.begin(), modeWeights.end(), weights.begin()); > + > + std::stringstream str; > + str << "Histogram weights : "; > + for (size_t i = 0; i < context.hw->numHistogramWeights; i++) > + str << (int)params->meas.hst_config.hist_weight[i] << " "; > + LOG(RkISP1Agc, Debug) << str.str(); > + > + /* \todo Add a control for this? */ > + params->meas.hst_config.histogram_predivider = > + predivider(context.configuration.sensor.size); > > /* Update the configuration for histogram. */ > params->module_cfg_update |= RKISP1_CIF_ISP_MODULE_HST; > diff --git a/src/ipa/rkisp1/algorithms/agc.h b/src/ipa/rkisp1/algorithms/agc.h > index f2f5b59d0..77d944237 100644 > --- a/src/ipa/rkisp1/algorithms/agc.h > +++ b/src/ipa/rkisp1/algorithms/agc.h > @@ -44,11 +44,17 @@ public: > ControlList &metadata) override; > > private: > + int parseMeteringModes(IPAContext &context, const YamlObject &tuningData, > + const char *prop); > + uint8_t predivider(Size &size); > + > void fillMetadata(IPAContext &context, IPAFrameContext &frameContext, > ControlList &metadata); > double estimateLuminance(double gain) const override; > > Span<const uint8_t> expMeans_; > + > + std::map<int32_t, std::vector<uint8_t>> meteringModes_; > }; > > } /* namespace ipa::rkisp1::algorithms */ > -- > 2.39.2 >
Hi Paul On 17/05/2024 09:07, Paul Elder wrote: > Add support to the rkisp1 AGC to read histogram weights from the tuning > file. As controls for selecting the metering mode are not yet supported, > for now hardcode the matrix metering mode, which is the same as what the > AGC previously hardcoded. > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> > > --- > Changes in v3: > - support the new tuning file layout for interpolated matrices > - support both v10 and v12 > > Changes in v2: > - add default metering mode if none are read successfully from the > tuning file > - compute the predivider instead of using a table > --- > src/ipa/rkisp1/algorithms/agc.cpp | 94 ++++++++++++++++++++++++++++++- > src/ipa/rkisp1/algorithms/agc.h | 6 ++ > 2 files changed, 97 insertions(+), 3 deletions(-) > > diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp > index 5aad5c7c2..4af397bdc 100644 > --- a/src/ipa/rkisp1/algorithms/agc.cpp > +++ b/src/ipa/rkisp1/algorithms/agc.cpp > @@ -17,6 +17,8 @@ > #include <libcamera/control_ids.h> > #include <libcamera/ipa/core_ipa_interface.h> > > +#include "libcamera/internal/yaml_parser.h" > + > #include "libipa/histogram.h" > > /** > @@ -36,6 +38,76 @@ namespace ipa::rkisp1::algorithms { > > LOG_DEFINE_CATEGORY(RkISP1Agc) > > +int Agc::parseMeteringModes(IPAContext &context, const YamlObject &tuningData, > + const char *prop) I think a comment with an example of the format that it'll parse would be good. Also; is there a particular reason to have the property name as a parameter instead of just hardcoding it in here (rather than the caller)? Doesn't particularly matter, just curious. > +{ > + const YamlObject &yamlMeteringModes = tuningData[prop]; > + if (!yamlMeteringModes.isDictionary()) { > + LOG(RkISP1Agc, Error) > + << "'" << prop << "' parameter not found in tuning file"; > + return -EINVAL; > + } > + > + for (const auto &[key, value] : yamlMeteringModes.asDict()) { > + if (controls::AeMeteringModeNameValueMap.find(key) == > + controls::AeMeteringModeNameValueMap.end()) { > + LOG(RkISP1Agc, Warning) > + << "Skipping unknown metering mode '" << key << "'"; > + continue; > + } > + > + for (const auto &[version, matrix] : value.asDict()) { > + std::vector<uint8_t> weights = > + matrix.getList<uint8_t>().value_or(std::vector<uint8_t>{}); > + if (weights.size() != context.hw->numHistogramWeights) > + continue; Worth a debug printout imo - took me a little while to figure out that this was skipping arrays of weights for the other hardware revision. With those: Reviewed-by: Daniel Scally <dan.scally@ideasonboard.com> > + > + LOG(RkISP1Agc, Debug) > + << "Matched metering matrix mode " > + << key << ", version " << version; > + > + meteringModes_[controls::AeMeteringModeNameValueMap.at(key)] = weights; > + } > + } > + > + if (meteringModes_.empty()) { > + int32_t meteringModeId = controls::AeMeteringModeNameValueMap.at("MeteringMatrix"); > + std::vector<uint8_t> weights(context.hw->numHistogramWeights, 1); > + > + meteringModes_[meteringModeId] = weights; > + } > + > + return 0; > +} > + > +uint8_t Agc::predivider(Size &size) > +{ > + /* > + * The maximum number of pixels that could potentially be in one bin is > + * if all the pixels of the image are in it, multiplied by 3 for the > + * three color channels. The counter for each bin is 16 bits wide, so > + * `factor` thus contains the number of times we'd wrap around. This is > + * obviously the number of pixels that we need to skip to make sure > + * that we don't wrap around, but we compute the square root of it > + * instead, as the skip that we need to program is for both the x and y > + * directions. > + * > + * There's a bit of extra rounding math to make sure the rounding goes > + * the correct direction so that the square of the step is big enough > + * to encompass the `factor` number of pixels that we need to skip. > + */ > + double factor = size.width * size.height * 3 / 65535.0; > + double root = std::sqrt(factor); > + uint8_t ret; > + > + if (std::pow(std::floor(root), 2) + 0.01 < factor) > + ret = static_cast<uint8_t>(std::ceil(root)); > + else > + ret = static_cast<uint8_t>(std::floor(root)); > + > + return std::clamp<uint8_t>(ret, 3, 127); > +} > + > Agc::Agc() > { > supportsRaw_ = true; > @@ -59,6 +131,10 @@ int Agc::init(IPAContext &context, const YamlObject &tuningData) > if (ret) > return ret; > > + ret = parseMeteringModes(context, tuningData, "AeMeteringMode"); > + if (ret) > + return ret; > + > context.ctrlMap.merge(controls()); > > return 0; > @@ -160,6 +236,7 @@ void Agc::prepare(IPAContext &context, const uint32_t frame, > frameContext.agc.gain = context.activeState.agc.automatic.gain; > } > > + /* \todo Remove this when we can set the below with controls */ > if (frame > 0) > return; > > @@ -178,14 +255,25 @@ void Agc::prepare(IPAContext &context, const uint32_t frame, > params->meas.hst_config.meas_window = context.configuration.agc.measureWindow; > /* Produce the luminance histogram. */ > params->meas.hst_config.mode = RKISP1_CIF_ISP_HISTOGRAM_MODE_Y_HISTOGRAM; > + > /* Set an average weighted histogram. */ > Span<uint8_t> weights{ > params->meas.hst_config.hist_weight, > context.hw->numHistogramWeights > }; > - std::fill(weights.begin(), weights.end(), 1); > - /* Step size can't be less than 3. */ > - params->meas.hst_config.histogram_predivider = 4; > + /* \todo Get this from control */ > + std::vector<uint8_t> &modeWeights = meteringModes_.at(controls::MeteringMatrix); > + std::copy(modeWeights.begin(), modeWeights.end(), weights.begin()); > + > + std::stringstream str; > + str << "Histogram weights : "; > + for (size_t i = 0; i < context.hw->numHistogramWeights; i++) > + str << (int)params->meas.hst_config.hist_weight[i] << " "; > + LOG(RkISP1Agc, Debug) << str.str(); > + > + /* \todo Add a control for this? */ > + params->meas.hst_config.histogram_predivider = > + predivider(context.configuration.sensor.size); > > /* Update the configuration for histogram. */ > params->module_cfg_update |= RKISP1_CIF_ISP_MODULE_HST; > diff --git a/src/ipa/rkisp1/algorithms/agc.h b/src/ipa/rkisp1/algorithms/agc.h > index f2f5b59d0..77d944237 100644 > --- a/src/ipa/rkisp1/algorithms/agc.h > +++ b/src/ipa/rkisp1/algorithms/agc.h > @@ -44,11 +44,17 @@ public: > ControlList &metadata) override; > > private: > + int parseMeteringModes(IPAContext &context, const YamlObject &tuningData, > + const char *prop); > + uint8_t predivider(Size &size); > + > void fillMetadata(IPAContext &context, IPAFrameContext &frameContext, > ControlList &metadata); > double estimateLuminance(double gain) const override; > > Span<const uint8_t> expMeans_; > + > + std::map<int32_t, std::vector<uint8_t>> meteringModes_; > }; > > } /* namespace ipa::rkisp1::algorithms */
diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp index 5aad5c7c2..4af397bdc 100644 --- a/src/ipa/rkisp1/algorithms/agc.cpp +++ b/src/ipa/rkisp1/algorithms/agc.cpp @@ -17,6 +17,8 @@ #include <libcamera/control_ids.h> #include <libcamera/ipa/core_ipa_interface.h> +#include "libcamera/internal/yaml_parser.h" + #include "libipa/histogram.h" /** @@ -36,6 +38,76 @@ namespace ipa::rkisp1::algorithms { LOG_DEFINE_CATEGORY(RkISP1Agc) +int Agc::parseMeteringModes(IPAContext &context, const YamlObject &tuningData, + const char *prop) +{ + const YamlObject &yamlMeteringModes = tuningData[prop]; + if (!yamlMeteringModes.isDictionary()) { + LOG(RkISP1Agc, Error) + << "'" << prop << "' parameter not found in tuning file"; + return -EINVAL; + } + + for (const auto &[key, value] : yamlMeteringModes.asDict()) { + if (controls::AeMeteringModeNameValueMap.find(key) == + controls::AeMeteringModeNameValueMap.end()) { + LOG(RkISP1Agc, Warning) + << "Skipping unknown metering mode '" << key << "'"; + continue; + } + + for (const auto &[version, matrix] : value.asDict()) { + std::vector<uint8_t> weights = + matrix.getList<uint8_t>().value_or(std::vector<uint8_t>{}); + if (weights.size() != context.hw->numHistogramWeights) + continue; + + LOG(RkISP1Agc, Debug) + << "Matched metering matrix mode " + << key << ", version " << version; + + meteringModes_[controls::AeMeteringModeNameValueMap.at(key)] = weights; + } + } + + if (meteringModes_.empty()) { + int32_t meteringModeId = controls::AeMeteringModeNameValueMap.at("MeteringMatrix"); + std::vector<uint8_t> weights(context.hw->numHistogramWeights, 1); + + meteringModes_[meteringModeId] = weights; + } + + return 0; +} + +uint8_t Agc::predivider(Size &size) +{ + /* + * The maximum number of pixels that could potentially be in one bin is + * if all the pixels of the image are in it, multiplied by 3 for the + * three color channels. The counter for each bin is 16 bits wide, so + * `factor` thus contains the number of times we'd wrap around. This is + * obviously the number of pixels that we need to skip to make sure + * that we don't wrap around, but we compute the square root of it + * instead, as the skip that we need to program is for both the x and y + * directions. + * + * There's a bit of extra rounding math to make sure the rounding goes + * the correct direction so that the square of the step is big enough + * to encompass the `factor` number of pixels that we need to skip. + */ + double factor = size.width * size.height * 3 / 65535.0; + double root = std::sqrt(factor); + uint8_t ret; + + if (std::pow(std::floor(root), 2) + 0.01 < factor) + ret = static_cast<uint8_t>(std::ceil(root)); + else + ret = static_cast<uint8_t>(std::floor(root)); + + return std::clamp<uint8_t>(ret, 3, 127); +} + Agc::Agc() { supportsRaw_ = true; @@ -59,6 +131,10 @@ int Agc::init(IPAContext &context, const YamlObject &tuningData) if (ret) return ret; + ret = parseMeteringModes(context, tuningData, "AeMeteringMode"); + if (ret) + return ret; + context.ctrlMap.merge(controls()); return 0; @@ -160,6 +236,7 @@ void Agc::prepare(IPAContext &context, const uint32_t frame, frameContext.agc.gain = context.activeState.agc.automatic.gain; } + /* \todo Remove this when we can set the below with controls */ if (frame > 0) return; @@ -178,14 +255,25 @@ void Agc::prepare(IPAContext &context, const uint32_t frame, params->meas.hst_config.meas_window = context.configuration.agc.measureWindow; /* Produce the luminance histogram. */ params->meas.hst_config.mode = RKISP1_CIF_ISP_HISTOGRAM_MODE_Y_HISTOGRAM; + /* Set an average weighted histogram. */ Span<uint8_t> weights{ params->meas.hst_config.hist_weight, context.hw->numHistogramWeights }; - std::fill(weights.begin(), weights.end(), 1); - /* Step size can't be less than 3. */ - params->meas.hst_config.histogram_predivider = 4; + /* \todo Get this from control */ + std::vector<uint8_t> &modeWeights = meteringModes_.at(controls::MeteringMatrix); + std::copy(modeWeights.begin(), modeWeights.end(), weights.begin()); + + std::stringstream str; + str << "Histogram weights : "; + for (size_t i = 0; i < context.hw->numHistogramWeights; i++) + str << (int)params->meas.hst_config.hist_weight[i] << " "; + LOG(RkISP1Agc, Debug) << str.str(); + + /* \todo Add a control for this? */ + params->meas.hst_config.histogram_predivider = + predivider(context.configuration.sensor.size); /* Update the configuration for histogram. */ params->module_cfg_update |= RKISP1_CIF_ISP_MODULE_HST; diff --git a/src/ipa/rkisp1/algorithms/agc.h b/src/ipa/rkisp1/algorithms/agc.h index f2f5b59d0..77d944237 100644 --- a/src/ipa/rkisp1/algorithms/agc.h +++ b/src/ipa/rkisp1/algorithms/agc.h @@ -44,11 +44,17 @@ public: ControlList &metadata) override; private: + int parseMeteringModes(IPAContext &context, const YamlObject &tuningData, + const char *prop); + uint8_t predivider(Size &size); + void fillMetadata(IPAContext &context, IPAFrameContext &frameContext, ControlList &metadata); double estimateLuminance(double gain) const override; Span<const uint8_t> expMeans_; + + std::map<int32_t, std::vector<uint8_t>> meteringModes_; }; } /* namespace ipa::rkisp1::algorithms */
Add support to the rkisp1 AGC to read histogram weights from the tuning file. As controls for selecting the metering mode are not yet supported, for now hardcode the matrix metering mode, which is the same as what the AGC previously hardcoded. Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> --- Changes in v3: - support the new tuning file layout for interpolated matrices - support both v10 and v12 Changes in v2: - add default metering mode if none are read successfully from the tuning file - compute the predivider instead of using a table --- src/ipa/rkisp1/algorithms/agc.cpp | 94 ++++++++++++++++++++++++++++++- src/ipa/rkisp1/algorithms/agc.h | 6 ++ 2 files changed, 97 insertions(+), 3 deletions(-)