[PATCH v4 3/5] libcamera: software_isp: Move color mappings out of debayering
Milan Zamazal
mzamazal at redhat.com
Fri May 31 14:46:07 CEST 2024
Hi Laurent,
Laurent Pinchart <laurent.pinchart at ideasonboard.com> writes:
> 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.
Yes, done.
> 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).
Yes, I'd definitely like to have this improved in the future refactoring
series. The current state is confusing.
>> > 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;
More information about the libcamera-devel
mailing list