Hi Paul,
thanks for the patch.
On Fri, Apr 05, 2024 at 11:47:25PM +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 at ideasonboard.com>
> ---
> src/ipa/rkisp1/algorithms/agc.cpp | 80 +++++++++++++++++++++++++++++--
> src/ipa/rkisp1/algorithms/agc.h | 6 +++
> 2 files changed, 83 insertions(+), 3 deletions(-)
>
> diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp
> index 1cd977a0..fd47ba4e 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"
>
> /**
> @@ -42,6 +44,62 @@ static constexpr double kMinAnalogueGain = 1.0;
> /* \todo Honour the FrameDurationLimits control instead of hardcoding a limit */
> static constexpr utils::Duration kMaxShutterSpeed = 60ms;
>
> +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;
> + }
> +
> + std::vector<uint8_t> weights =
> + value.getList<uint8_t>().value_or(std::vector<uint8_t>{});
> + if (weights.size() != context.hw->numHistogramWeights) {
> + LOG(RkISP1Agc, Error)
> + << "Invalid '" << key << "' values: expected "
> + << context.hw->numHistogramWeights
> + << " elements, got " << weights.size();
> + return -ENODATA;
> + }
> +
> + meteringModes_[controls::AeMeteringModeNameValueMap.at(key)] = weights;
> + }
Kieran already mentioned that. What about adding the matrix metering
mode (+ warning) in case meteringModes_ is still empty.
> +
> + return 0;
> +}
> +
> +uint8_t Agc::predivider(Size &size)
> +{
> + static const std::vector<std::pair<unsigned int, unsigned int>>
> + pixelCountThresholds = {
> + { 640 * 480, 3 },
> + { 800 * 600, 4 },
> + { 1024 * 768, 5 },
> + { 1280 * 960, 6 },
> + { 1600 * 1200, 8 },
> + { 2048 * 1536, 10 },
> + { 2600 * 1950, 12 },
> + { 4096 * 3072, 16 },
> + };
Where do these values come from? What would happen if we keep the predivider
of 4?
> +
> + unsigned long pixels = size.width * size.height;
> + for (auto &pair : pixelCountThresholds)
> + if (pixels < pair.first)
> + return pair.second;
> +
> + return 24;
> +}
> +
> Agc::Agc()
> {
> supportsRaw_ = true;
> @@ -72,6 +130,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;
> @@ -177,6 +239,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;
>
> @@ -195,14 +258,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 311d4e94..43e3d5b2 100644
> --- a/src/ipa/rkisp1/algorithms/agc.h
> +++ b/src/ipa/rkisp1/algorithms/agc.h
> @@ -44,6 +44,10 @@ 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);
> void parseStatistics(const rkisp1_stat_buffer *stats,
> @@ -52,6 +56,8 @@ private:
>
> Histogram hist_;
> Span<const uint8_t> expMeans_;
> +
> + std::map<int32_t, std::vector<uint8_t>> meteringModes_;
> };
>
> } /* namespace ipa::rkisp1::algorithms */
> --
> 2.39.2
>