[PATCH v6 3/5] libcamera: software_isp: Move color mappings out of debayering
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Sun Jun 2 00:54:29 CEST 2024
Hi Milan,
Thank you for the patch.
On Fri, May 31, 2024 at 02:38:38PM +0200, Milan Zamazal wrote:
> Constructing the color mapping tables is related to stats rather than
> debayering, where they are applied. Let's move the corresponding code
> to stats processing.
>
> The same applies to the auxiliary gamma table. As the gamma value is
> currently fixed and used in a single place, with the temporary exception
> mentioned below, there is no need to share it anywhere anymore.
>
> It's necessary to initialize SoftwareIsp::debayerParams_ to default
> values. These initial values are used for the first two frames, before
> they are changed based on determined stats. To avoid sharing the gamma
> value constant in artificial ways, we use 0.5 directly in the
> initialization. This all is not a particularly elegant thing to do,
> such a code belongs conceptually to the similar code in stats
> processing, but doing better is left for larger refactoring.
>
> This is a preliminary step towards building this functionality on top of
> libipa/algorithm.h, which should follow.
>
> Signed-off-by: Milan Zamazal <mzamazal at redhat.com>
> Reviewed-by: Andrei Konovalov <andrey.konovalov.ynk at gmail.com>
> ---
> .../internal/software_isp/debayer_params.h | 18 +++----
> src/ipa/simple/soft_simple.cpp | 51 +++++++++++++++----
> src/libcamera/software_isp/debayer.cpp | 29 +++++------
> src/libcamera/software_isp/debayer_cpu.cpp | 43 +++-------------
> src/libcamera/software_isp/debayer_cpu.h | 11 ++--
> src/libcamera/software_isp/software_isp.cpp | 23 +++++++--
> 6 files changed, 95 insertions(+), 80 deletions(-)
>
> diff --git a/include/libcamera/internal/software_isp/debayer_params.h b/include/libcamera/internal/software_isp/debayer_params.h
> index ce1b5945..463d24b3 100644
> --- a/include/libcamera/internal/software_isp/debayer_params.h
> +++ b/include/libcamera/internal/software_isp/debayer_params.h
> @@ -1,6 +1,6 @@
> /* SPDX-License-Identifier: LGPL-2.1-or-later */
> /*
> - * Copyright (C) 2023, Red Hat Inc.
> + * Copyright (C) 2023, 2024 Red Hat Inc.
> *
> * Authors:
> * Hans de Goede <hdegoede at redhat.com>
> @@ -10,20 +10,20 @@
>
> #pragma once
>
> +#include <array>
> +#include <stdint.h>
> +
> namespace libcamera {
>
> struct DebayerParams {
> static constexpr unsigned int kGain10 = 256;
> + static constexpr unsigned int kRGBLookupSize = 256;
>
> - unsigned int gainR;
> - unsigned int gainG;
> - unsigned int gainB;
> + using ColorLookupTable = std::array<uint8_t, kRGBLookupSize>;
>
> - float gamma;
> - /**
> - * \brief Level of the black point, 0..255, 0 is no correction.
> - */
> - unsigned int blackLevel;
> + ColorLookupTable red;
> + ColorLookupTable green;
> + ColorLookupTable blue;
> };
>
> } /* namespace libcamera */
> diff --git a/src/ipa/simple/soft_simple.cpp b/src/ipa/simple/soft_simple.cpp
> index f383f994..3388686f 100644
> --- a/src/ipa/simple/soft_simple.cpp
> +++ b/src/ipa/simple/soft_simple.cpp
> @@ -5,6 +5,7 @@
> * Simple Software Image Processing Algorithm module
> */
>
> +#include <cmath>
> #include <numeric>
> #include <stdint.h>
> #include <sys/mman.h>
> @@ -84,6 +85,10 @@ private:
> ControlInfoMap sensorInfoMap_;
> BlackLevel blackLevel_;
>
> + static constexpr unsigned int kGammaLookupSize = 1024;
> + std::array<uint8_t, kGammaLookupSize> gammaTable_;
> + int lastBlackLevel_ = -1;
> +
> int32_t exposureMin_, exposureMax_;
> int32_t exposure_;
> double againMin_, againMax_, againMinStep_;
> @@ -246,7 +251,6 @@ void IPASoftSimple::processStats(const ControlList &sensorControls)
> if (ignoreUpdates_ > 0)
> blackLevel_.update(histogram);
> const uint8_t blackLevel = blackLevel_.get();
> - params_->blackLevel = blackLevel;
>
> /*
> * Black level must be subtracted to get the correct AWB ratios,
> @@ -263,13 +267,42 @@ void IPASoftSimple::processStats(const ControlList &sensorControls)
> /*
> * Calculate red and blue gains for AWB.
> * Clamp max gain at 4.0, this also avoids 0 division.
> + * Gain: 128 = 0.5, 256 = 1.0, 512 = 2.0, etc.
> */
> - params_->gainR = sumR <= sumG / 4 ? 1024 : 256 * sumG / sumR;
> - params_->gainB = sumB <= sumG / 4 ? 1024 : 256 * sumG / sumB;
> -
> + const unsigned int gainR = sumR <= sumG / 4 ? 1024 : 256 * sumG / sumR;
> + const unsigned int gainB = sumB <= sumG / 4 ? 1024 : 256 * sumG / sumB;
> /* Green gain and gamma values are fixed */
> - params_->gainG = 256;
> - params_->gamma = 0.5;
> + constexpr unsigned int gainG = 256;
> +
> + /* Update the gamma table if needed */
> + if (blackLevel != lastBlackLevel_) {
> + constexpr float gamma = 0.5;
> + const unsigned int blackIndex = blackLevel * kGammaLookupSize / 256;
> + std::fill(gammaTable_.begin(), gammaTable_.begin() + blackIndex, 0);
> + const float divisor = kGammaLookupSize - blackIndex - 1.0;
> + for (unsigned int i = blackIndex; i < kGammaLookupSize; i++)
> + gammaTable_[i] = UINT8_MAX *
> + std::pow((i - blackIndex) / divisor, gamma);
> +
> + lastBlackLevel_ = blackLevel;
> + }
> +
> + for (unsigned int i = 0; i < DebayerParams::kRGBLookupSize; i++) {
> + constexpr unsigned int div =
> + DebayerParams::kRGBLookupSize * DebayerParams::kGain10 /
> + kGammaLookupSize;
> + unsigned int idx;
> +
> + /* Apply gamma after gain! */
> + idx = std::min({ i * gainR / div, (kGammaLookupSize - 1) });
> + params_->red[i] = gammaTable_[idx];
> +
> + idx = std::min({ i * gainG / div, (kGammaLookupSize - 1) });
> + params_->green[i] = gammaTable_[idx];
> +
> + idx = std::min({ i * gainB / div, (kGammaLookupSize - 1) });
> + params_->blue[i] = gammaTable_[idx];
> + }
>
> setIspParams.emit();
>
> @@ -290,7 +323,7 @@ void IPASoftSimple::processStats(const ControlList &sensorControls)
> * https://www.araa.asn.au/acra/acra2007/papers/paper84final.pdf
> */
> const unsigned int blackLevelHistIdx =
> - params_->blackLevel / (256 / SwIspStats::kYHistogramSize);
> + blackLevel / (256 / SwIspStats::kYHistogramSize);
> const unsigned int histogramSize =
> SwIspStats::kYHistogramSize - blackLevelHistIdx;
> const unsigned int yHistValsPerBin = histogramSize / kExposureBinsCount;
> @@ -338,8 +371,8 @@ void IPASoftSimple::processStats(const ControlList &sensorControls)
>
> LOG(IPASoft, Debug) << "exposureMSV " << exposureMSV
> << " exp " << exposure_ << " again " << again_
> - << " gain R/B " << params_->gainR << "/" << params_->gainB
> - << " black level " << params_->blackLevel;
> + << " gain R/B " << gainR << "/" << gainB
> + << " black level " << static_cast<unsigned int>(blackLevel);
> }
>
> void IPASoftSimple::updateExposure(double exposureMSV)
> diff --git a/src/libcamera/software_isp/debayer.cpp b/src/libcamera/software_isp/debayer.cpp
> index efe75ea8..3f3969f7 100644
> --- a/src/libcamera/software_isp/debayer.cpp
> +++ b/src/libcamera/software_isp/debayer.cpp
> @@ -1,7 +1,7 @@
> /* SPDX-License-Identifier: LGPL-2.1-or-later */
> /*
> * Copyright (C) 2023, Linaro Ltd
> - * Copyright (C) 2023, Red Hat Inc.
> + * Copyright (C) 2023, 2024 Red Hat Inc.
> *
> * Authors:
> * Hans de Goede <hdegoede at redhat.com>
> @@ -24,29 +24,28 @@ namespace libcamera {
> */
>
> /**
> - * \var DebayerParams::gainR
> - * \brief Red gain
> - *
> - * 128 = 0.5, 256 = 1.0, 512 = 2.0, etc.
> + * \var DebayerParams::kRGBLookupSize
> + * \brief Size of a color lookup table
> */
>
> /**
> - * \var DebayerParams::gainG
> - * \brief Green gain
> - *
> - * 128 = 0.5, 256 = 1.0, 512 = 2.0, etc.
> + * \typedef DebayerParams::ColorLookupTable
> + * \brief Type of the lookup tables for red, green, blue values
> */
>
> /**
> - * \var DebayerParams::gainB
> - * \brief Blue gain
> - *
> - * 128 = 0.5, 256 = 1.0, 512 = 2.0, etc.
> + * \var DebayerParams::red
> + * \brief Lookup table for red color, mapping input values to output values
> + */
> +
> +/**
> + * \var DebayerParams::green
> + * \brief Lookup table for green color, mapping input values to output values
> */
>
> /**
> - * \var DebayerParams::gamma
> - * \brief Gamma correction, 1.0 is no correction
> + * \var DebayerParams::blue
> + * \brief Lookup table for blue color, mapping input values to output values
> */
>
> /**
> diff --git a/src/libcamera/software_isp/debayer_cpu.cpp b/src/libcamera/software_isp/debayer_cpu.cpp
> index 8254bbe9..c038eed4 100644
> --- a/src/libcamera/software_isp/debayer_cpu.cpp
> +++ b/src/libcamera/software_isp/debayer_cpu.cpp
> @@ -11,7 +11,6 @@
>
> #include "debayer_cpu.h"
>
> -#include <math.h>
> #include <stdlib.h>
> #include <time.h>
>
> @@ -35,7 +34,7 @@ namespace libcamera {
> * \param[in] stats Pointer to the stats object to use
> */
> DebayerCpu::DebayerCpu(std::unique_ptr<SwStatsCpu> stats)
> - : stats_(std::move(stats)), gammaCorrection_(1.0), blackLevel_(0)
> + : stats_(std::move(stats))
> {
> /*
> * Reading from uncached buffers may be very slow.
> @@ -47,9 +46,9 @@ DebayerCpu::DebayerCpu(std::unique_ptr<SwStatsCpu> stats)
> */
> enableInputMemcpy_ = true;
>
> - /* Initialize gamma to 1.0 curve */
> - for (unsigned int i = 0; i < kGammaLookupSize; i++)
> - gamma_[i] = i / (kGammaLookupSize / kRGBLookupSize);
> + /* Initialize color lookup tables */
> + for (unsigned int i = 0; i < DebayerParams::kRGBLookupSize; i++)
> + red_[i] = green_[i] = blue_[i] = i;
>
> for (unsigned int i = 0; i < kMaxLineBuffers; i++)
> lineBuffers_[i] = nullptr;
> @@ -698,37 +697,9 @@ void DebayerCpu::process(FrameBuffer *input, FrameBuffer *output, DebayerParams
> clock_gettime(CLOCK_MONOTONIC_RAW, &frameStartTime);
> }
>
> - /* Apply DebayerParams */
> - if (params.gamma != gammaCorrection_ || params.blackLevel != blackLevel_) {
> - const unsigned int blackIndex =
> - params.blackLevel * kGammaLookupSize / 256;
> - std::fill(gamma_.begin(), gamma_.begin() + blackIndex, 0);
> - const float divisor = kGammaLookupSize - blackIndex - 1.0;
> - for (unsigned int i = blackIndex; i < kGammaLookupSize; i++)
> - gamma_[i] = UINT8_MAX * powf((i - blackIndex) / divisor, params.gamma);
> -
> - gammaCorrection_ = params.gamma;
> - blackLevel_ = params.blackLevel;
> - }
> -
> - if (swapRedBlueGains_)
> - std::swap(params.gainR, params.gainB);
> -
> - for (unsigned int i = 0; i < kRGBLookupSize; i++) {
> - constexpr unsigned int div =
> - kRGBLookupSize * DebayerParams::kGain10 / kGammaLookupSize;
> - unsigned int idx;
> -
> - /* Apply gamma after gain! */
> - idx = std::min({ i * params.gainR / div, (kGammaLookupSize - 1) });
> - red_[i] = gamma_[idx];
> -
> - idx = std::min({ i * params.gainG / div, (kGammaLookupSize - 1) });
> - green_[i] = gamma_[idx];
> -
> - idx = std::min({ i * params.gainB / div, (kGammaLookupSize - 1) });
> - blue_[i] = gamma_[idx];
> - }
> + green_ = params.green;
> + red_ = swapRedBlueGains_ ? params.blue : params.red;
> + blue_ = swapRedBlueGains_ ? params.red : params.blue;
>
> /* Copy metadata from the input buffer */
> FrameMetadata &metadata = output->_d()->metadata();
> diff --git a/src/libcamera/software_isp/debayer_cpu.h b/src/libcamera/software_isp/debayer_cpu.h
> index de216fe3..be7dcdca 100644
> --- a/src/libcamera/software_isp/debayer_cpu.h
> +++ b/src/libcamera/software_isp/debayer_cpu.h
> @@ -122,15 +122,12 @@ private:
> void process2(const uint8_t *src, uint8_t *dst);
> void process4(const uint8_t *src, uint8_t *dst);
>
> - static constexpr unsigned int kGammaLookupSize = 1024;
> - static constexpr unsigned int kRGBLookupSize = 256;
> /* Max. supported Bayer pattern height is 4, debayering this requires 5 lines */
> static constexpr unsigned int kMaxLineBuffers = 5;
>
> - std::array<uint8_t, kGammaLookupSize> gamma_;
> - std::array<uint8_t, kRGBLookupSize> red_;
> - std::array<uint8_t, kRGBLookupSize> green_;
> - std::array<uint8_t, kRGBLookupSize> blue_;
> + DebayerParams::ColorLookupTable red_;
> + DebayerParams::ColorLookupTable green_;
> + DebayerParams::ColorLookupTable blue_;
> debayerFn debayer0_;
> debayerFn debayer1_;
> debayerFn debayer2_;
> @@ -146,8 +143,6 @@ private:
> unsigned int xShift_; /* Offset of 0/1 applied to window_.x */
> bool enableInputMemcpy_;
> bool swapRedBlueGains_;
> - float gammaCorrection_;
> - unsigned int blackLevel_;
> unsigned int measuredFrames_;
> int64_t frameProcessTime_;
> /* Skip 30 frames for things to stabilize then measure 30 frames */
> diff --git a/src/libcamera/software_isp/software_isp.cpp b/src/libcamera/software_isp/software_isp.cpp
> index c9b6be56..ccd703bb 100644
> --- a/src/libcamera/software_isp/software_isp.cpp
> +++ b/src/libcamera/software_isp/software_isp.cpp
> @@ -7,6 +7,8 @@
>
> #include "libcamera/internal/software_isp/software_isp.h"
>
> +#include <cmath>
> +#include <stdint.h>
> #include <sys/mman.h>
> #include <sys/types.h>
> #include <unistd.h>
> @@ -18,6 +20,7 @@
> #include "libcamera/internal/framebuffer.h"
> #include "libcamera/internal/ipa_manager.h"
> #include "libcamera/internal/mapped_framebuffer.h"
> +#include "libcamera/internal/software_isp/debayer_params.h"
>
> #include "debayer_cpu.h"
>
> @@ -63,10 +66,24 @@ LOG_DEFINE_CATEGORY(SoftwareIsp)
> * handler
> */
> SoftwareIsp::SoftwareIsp(PipelineHandler *pipe, const CameraSensor *sensor)
> - : debayerParams_{ DebayerParams::kGain10, DebayerParams::kGain10,
> - DebayerParams::kGain10, 0.5f, 0 },
> - dmaHeap_(DmaHeap::DmaHeapFlag::Cma | DmaHeap::DmaHeapFlag::System)
> + : dmaHeap_(DmaHeap::DmaHeapFlag::Cma | DmaHeap::DmaHeapFlag::System)
> {
> + /*
> + * debayerParams_ must be initialized because the initial value is used for
> + * the first two frames, i.e. until stats processing starts providing its
> + * own parameters.
> + *
> + * \todo This should be handled in the same place as the related operations.
I would write
* \todo This should be handled in the same place as the related
* operations, in the IPA module.
No need to resubmit just for this, I can update the comment when
applying.
Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> + */
> + std::array<uint8_t, 256> gammaTable;
> + for (unsigned int i = 0; i < 256; i++)
> + gammaTable[i] = UINT8_MAX * std::pow(i / 256.0, 0.5);
> + for (unsigned int i = 0; i < DebayerParams::kRGBLookupSize; i++) {
> + debayerParams_.red[i] = gammaTable[i];
> + debayerParams_.green[i] = gammaTable[i];
> + debayerParams_.blue[i] = gammaTable[i];
> + }
> +
> if (!dmaHeap_.isValid()) {
> LOG(SoftwareIsp, Error) << "Failed to create DmaHeap object";
> return;
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list