[PATCH v4 3/5] libcamera: software_isp: Move color mappings out of debayering
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Thu May 30 23:01:55 CEST 2024
Hi Milan,
On Thu, May 30, 2024 at 10:47:11PM +0200, Milan Zamazal wrote:
> Laurent Pinchart writes:
>
> > Hi Milan,
> >
> > Thank you for the patch. This is a nice change, I think the resulting
> > code is easier to understand.
> >
> > On Tue, May 28, 2024 at 06:11:24PM +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.
> >
> > You're also changing how gamma is handled, and that's not explained
> > here.
>
> I'll add the explanation.
>
> >> 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 | 19 +++----
> >> src/ipa/simple/soft_simple.cpp | 50 +++++++++++++++----
> >> src/libcamera/software_isp/debayer.cpp | 34 +++++++------
> >> src/libcamera/software_isp/debayer_cpu.cpp | 43 +++-------------
> >> src/libcamera/software_isp/debayer_cpu.h | 11 ++--
> >> src/libcamera/software_isp/software_isp.cpp | 15 ++++--
> >> 6 files changed, 92 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..69fed1e7 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,21 @@
> >>
> >> #pragma once
> >>
> >> +#include <array>
> >> +#include <stdint.h>
> >> +
> >> namespace libcamera {
> >>
> >> struct DebayerParams {
> >> static constexpr unsigned int kGain10 = 256;
> >> + static constexpr unsigned int kRGBLookupSize = 256;
> >> + static constexpr float kGamma = 0.5;
> >
> > With this patch the gamma value isn't an ISP parameter anything, so I
> > don't think this field belongs here. As far as I understand, the only
> > reason why you need it is to initialize debayerParams_ in
> > SoftwareIsp::SoftwareIsp().
>
> Yes.
>
> > Is that needed, or does the IPA module provide ISP parameters for the
> > first frame ?
>
> It's needed for the first two frames, i.e. until stats processing starts
> providing its own parameters.
Could you record that in a comment ? I think it's important information
for the readers. It would also be worth trying to fix that and get the
IPA to produce initial parameters for the first frames at some point.
There's no urgency, recording a todo item is fine for now (if you think
it's a good idea).
> > If you need to initialize the parameters for the first frame on the
> > pipeline handler side for the time being, I would hardcode the 0.5 value
> > there and move the kGamma member from this structure to the IPA module.
>
> OK.
>
> >> - 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 722aac83..2c9fe98e 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 <math.h>
> >
> > #include <cmath>
> >
> > See Documentation/coding-style.rst
>
> I see, tricky :-). Could checkstyle.py catch this or is it more
> complicated (I can see math.h is used in multiple places, including
> documentation)?
I would have sworn checkstyle.py would catch this already. I think it
should be added. We already have a IncludeChecker class, it should be
easy to extend it to cover this.
> >> #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;
> >>
> >> /*
> >> * Calculate red and blue gains for AWB.
> >> @@ -265,12 +269,40 @@ void IPASoftSimple::processStats(const ControlList &sensorControls)
> >> const uint64_t sumG = subtractBlackLevel(stats_->sumG_, 2);
> >> const uint64_t sumB = subtractBlackLevel(stats_->sumB_, 4);
> >>
> >> - params_->gainR = sumR <= sumG / 4 ? 1024 : 256 * sumG / sumR;
> >> - params_->gainB = sumB <= sumG / 4 ? 1024 : 256 * sumG / sumB;
> >> -
> >> + /* Gain: 128 = 0.5, 256 = 1.0, 512 = 2.0, etc. */
> >> + 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_) {
> >> + 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 *
> >> + powf((i - blackIndex) / divisor, DebayerParams::kGamma);
> >
> > s/powf/std::pow/
> >
> >> +
> >> + 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();
> >>
> >> @@ -291,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;
> >> @@ -339,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..028bf27e 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,33 @@ 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.
> >> + * \var DebayerParams::kGamma
> >> + * \brief Gamma correction, 1.0 is no correction
> >> */
> >>
> >> /**
> >> - * \var DebayerParams::gainB
> >> - * \brief Blue 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::gamma
> >> - * \brief Gamma correction, 1.0 is no correction
> >> + * \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::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..84558c4e 100644
> >> --- a/src/libcamera/software_isp/software_isp.cpp
> >> +++ b/src/libcamera/software_isp/software_isp.cpp
> >> @@ -7,6 +7,7 @@
> >>
> >> #include "libcamera/internal/software_isp/software_isp.h"
> >>
> >> +#include <math.h>
> >
> > #include <cmath>
> >
> >> #include <sys/mman.h>
> >> #include <sys/types.h>
> >> #include <unistd.h>
> >> @@ -18,6 +19,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 +65,17 @@ 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)
> >> {
> >> + std::array<float, 256> gammaTable;
> >> + for (unsigned int i = 0; i < 256; i++)
> >> + gammaTable[i] = powf(i / 256.0, DebayerParams::kGamma);
> >
> > s/powf/std::pow/
>
> And it should be std::array<uint8_t, 256> here, I'll fix both.
>
> >> + 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