[libcamera-devel] [PATCH v3 1/8] ipa: raspberrypi: Code refactoring to match style guidelines
Naushir Patuck
naush at raspberrypi.com
Wed Jul 27 15:31:43 CEST 2022
On Wed, 27 Jul 2022 at 14:14, Laurent Pinchart <
laurent.pinchart at ideasonboard.com> wrote:
> Hi Naush,
>
> Thank you for the patch.
>
> On Wed, Jul 27, 2022 at 09:55:17AM +0100, Naushir Patuck wrote:
> > Refactor all the source files in src/ipa/raspberrypi/ to match the
> recommended
> > formatting guidelines for the libcamera project. The vast majority of
> changes
> > in this commit comprise of switching from snake_case to CamelCase, and
> starting
> > class member functions with a lower case character.
> >
> > Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
> > ---
> > src/ipa/raspberrypi/cam_helper.cpp | 88 +--
> > src/ipa/raspberrypi/cam_helper.hpp | 40 +-
> > src/ipa/raspberrypi/cam_helper_imx219.cpp | 34 +-
> > src/ipa/raspberrypi/cam_helper_imx290.cpp | 32 +-
> > src/ipa/raspberrypi/cam_helper_imx296.cpp | 24 +-
> > src/ipa/raspberrypi/cam_helper_imx477.cpp | 72 +-
> > src/ipa/raspberrypi/cam_helper_imx519.cpp | 70 +-
> > src/ipa/raspberrypi/cam_helper_ov5647.cpp | 44 +-
> > src/ipa/raspberrypi/cam_helper_ov9281.cpp | 28 +-
> > .../raspberrypi/controller/agc_algorithm.hpp | 19 +-
> > src/ipa/raspberrypi/controller/agc_status.h | 24 +-
> > src/ipa/raspberrypi/controller/algorithm.cpp | 20 +-
> > src/ipa/raspberrypi/controller/algorithm.hpp | 26 +-
> > .../raspberrypi/controller/awb_algorithm.hpp | 6 +-
> > src/ipa/raspberrypi/controller/awb_status.h | 8 +-
> > .../controller/black_level_status.h | 6 +-
> > src/ipa/raspberrypi/controller/camera_mode.h | 16 +-
> > .../raspberrypi/controller/ccm_algorithm.hpp | 2 +-
> > .../controller/contrast_algorithm.hpp | 4 +-
> > src/ipa/raspberrypi/controller/controller.cpp | 74 +-
> > src/ipa/raspberrypi/controller/controller.hpp | 22 +-
> > .../controller/denoise_algorithm.hpp | 2 +-
> > .../raspberrypi/controller/denoise_status.h | 4 +-
> > .../raspberrypi/controller/device_status.cpp | 18 +-
> > .../raspberrypi/controller/device_status.h | 16 +-
> > src/ipa/raspberrypi/controller/focus_status.h | 2 +-
> > src/ipa/raspberrypi/controller/histogram.cpp | 34 +-
> > src/ipa/raspberrypi/controller/histogram.hpp | 10 +-
> > src/ipa/raspberrypi/controller/metadata.hpp | 16 +-
> > src/ipa/raspberrypi/controller/noise_status.h | 4 +-
> > src/ipa/raspberrypi/controller/pwl.cpp | 130 ++--
> > src/ipa/raspberrypi/controller/pwl.hpp | 48 +-
> > src/ipa/raspberrypi/controller/rpi/agc.cpp | 732 +++++++++---------
> > src/ipa/raspberrypi/controller/rpi/agc.hpp | 130 ++--
> > src/ipa/raspberrypi/controller/rpi/alsc.cpp | 641 ++++++++-------
> > src/ipa/raspberrypi/controller/rpi/alsc.hpp | 86 +-
> > src/ipa/raspberrypi/controller/rpi/awb.cpp | 564 +++++++-------
> > src/ipa/raspberrypi/controller/rpi/awb.hpp | 110 +--
> > .../controller/rpi/black_level.cpp | 34 +-
> > .../controller/rpi/black_level.hpp | 12 +-
> > src/ipa/raspberrypi/controller/rpi/ccm.cpp | 84 +-
> > src/ipa/raspberrypi/controller/rpi/ccm.hpp | 12 +-
> > .../raspberrypi/controller/rpi/contrast.cpp | 118 ++-
> > .../raspberrypi/controller/rpi/contrast.hpp | 30 +-
> > src/ipa/raspberrypi/controller/rpi/dpc.cpp | 18 +-
> > src/ipa/raspberrypi/controller/rpi/dpc.hpp | 6 +-
> > src/ipa/raspberrypi/controller/rpi/focus.cpp | 14 +-
> > src/ipa/raspberrypi/controller/rpi/focus.hpp | 4 +-
> > src/ipa/raspberrypi/controller/rpi/geq.cpp | 48 +-
> > src/ipa/raspberrypi/controller/rpi/geq.hpp | 6 +-
> > src/ipa/raspberrypi/controller/rpi/lux.cpp | 70 +-
> > src/ipa/raspberrypi/controller/rpi/lux.hpp | 22 +-
> > src/ipa/raspberrypi/controller/rpi/noise.cpp | 38 +-
> > src/ipa/raspberrypi/controller/rpi/noise.hpp | 14 +-
> > src/ipa/raspberrypi/controller/rpi/sdn.cpp | 36 +-
> > src/ipa/raspberrypi/controller/rpi/sdn.hpp | 10 +-
> > .../raspberrypi/controller/rpi/sharpen.cpp | 42 +-
> > .../raspberrypi/controller/rpi/sharpen.hpp | 14 +-
> > .../controller/sharpen_algorithm.hpp | 2 +-
> > .../raspberrypi/controller/sharpen_status.h | 2 +-
> > src/ipa/raspberrypi/md_parser.hpp | 44 +-
> > src/ipa/raspberrypi/md_parser_smia.cpp | 108 +--
> > src/ipa/raspberrypi/raspberrypi.cpp | 272 +++----
> > 63 files changed, 2099 insertions(+), 2167 deletions(-)
>
> [snip]
>
> > diff --git a/src/ipa/raspberrypi/controller/rpi/agc.cpp
> b/src/ipa/raspberrypi/controller/rpi/agc.cpp
> > index f6a9cb0a2cd8..408ff9cf296d 100644
> > --- a/src/ipa/raspberrypi/controller/rpi/agc.cpp
> > +++ b/src/ipa/raspberrypi/controller/rpi/agc.cpp
>
> [snip]
>
> > -static double compute_initial_Y(bcm2835_isp_stats *stats, AwbStatus
> const &awb,
> > - double weights[], double gain)
> > +static double computeInitialY(bcm2835_isp_stats *stats, AwbStatus const
> &awb,
> > + double weights[], double gain)
> > {
> > bcm2835_isp_stats_region *regions = stats->agc_stats;
> > // Note how the calculation below means that equal weights give you
> > // "average" metering (i.e. all pixels equally important).
> > - double R_sum = 0, G_sum = 0, B_sum = 0, pixel_sum = 0;
> > + double rSum = 0, gSum = 0, bSum = 0, pixelSum = 0;
> > for (int i = 0; i < AGC_STATS_SIZE; i++) {
> > double counted = regions[i].counted;
> > - double r_sum = std::min(regions[i].r_sum * gain, ((1 <<
> PIPELINE_BITS) - 1) * counted);
> > - double g_sum = std::min(regions[i].g_sum * gain, ((1 <<
> PIPELINE_BITS) - 1) * counted);
> > - double b_sum = std::min(regions[i].b_sum * gain, ((1 <<
> PIPELINE_BITS) - 1) * counted);
> > - R_sum += r_sum * weights[i];
> > - G_sum += g_sum * weights[i];
> > - B_sum += b_sum * weights[i];
> > - pixel_sum += counted * weights[i];
> > + double rAcc = std::min(regions[i].r_sum * gain, ((1 <<
> PIPELINE_BITS) - 1) * counted);
> > + double gAcc = std::min(regions[i].g_sum * gain, ((1 <<
> PIPELINE_BITS) - 1) * counted);
> > + double bAcc = std::min(regions[i].b_sum * gain, ((1 <<
> PIPELINE_BITS) - 1) * counted);
> > + rSum += rAcc * weights[i];
> > + gSum += gAcc * weights[i];
> > + bSum += bAcc * weights[i];
> > + pixelSum += counted * weights[i];
> > }
> > - if (pixel_sum == 0.0) {
> > - LOG(RPiAgc, Warning) << "compute_initial_Y: pixel_sum is
> zero";
> > + if (pixelSum == 0.0) {
> > + LOG(RPiAgc, Warning) << "computeInitialY: pixel_sum is
> zero";
>
> s/pixel_sum/pixelSum/
>
> > return 0;
> > }
> > - double Y_sum = R_sum * awb.gain_r * .299 +
> > - G_sum * awb.gain_g * .587 +
> > - B_sum * awb.gain_b * .114;
> > - return Y_sum / pixel_sum / (1 << PIPELINE_BITS);
> > + double ySum = rSum * awb.gainR * .299 +
> > + gSum * awb.gainG * .587 +
> > + bSum * awb.gainB * .114;
> > + return ySum / pixelSum / (1 << PIPELINE_BITS);
> > }
>
> [snip]
>
> > diff --git a/src/ipa/raspberrypi/controller/rpi/awb.cpp
> b/src/ipa/raspberrypi/controller/rpi/awb.cpp
> > index d4c934473832..a305237f31fb 100644
> > --- a/src/ipa/raspberrypi/controller/rpi/awb.cpp
> > +++ b/src/ipa/raspberrypi/controller/rpi/awb.cpp
>
> [snip]
>
> > -double Awb::computeDelta2Sum(double gain_r, double gain_b)
> > +double Awb::computeDelta2Sum(double gainR, double gainB)
> > {
> > // Compute the sum of the squared colour error (non-greyness) as it
> > // appears in the log likelihood equation.
> > - double delta2_sum = 0;
> > + double delta2Sum = 0;
> > for (auto &z : zones_) {
> > - double delta_r = gain_r * z.R - 1 - config_.whitepoint_r;
> > - double delta_b = gain_b * z.B - 1 - config_.whitepoint_b;
> > - double delta2 = delta_r * delta_r + delta_b * delta_b;
> > + double deltaR = gainR * z.R - 1 - config_.whitepointR;
> > + double deltaB = gainB * z.B - 1 - config_.whitepointB;
> > + double delta2 = deltaR * deltaR + deltaB * deltaB;
> > //LOG(RPiAwb, Debug) << "delta_r " << delta_r << " delta_b
> " << delta_b << " delta2 " << delta2;
>
> The variables should be updated here too.
>
> > - delta2 = std::min(delta2, config_.delta_limit);
> > - delta2_sum += delta2;
> > + delta2 = std::min(delta2, config_.deltaLimit);
> > + delta2Sum += delta2;
> > }
> > - return delta2_sum;
> > + return delta2Sum;
> > }
>
> [snip]
>
> > diff --git a/src/ipa/raspberrypi/controller/rpi/awb.hpp
> b/src/ipa/raspberrypi/controller/rpi/awb.hpp
> > index ac3dca6f42fc..91251d6be2da 100644
> > --- a/src/ipa/raspberrypi/controller/rpi/awb.hpp
> > +++ b/src/ipa/raspberrypi/controller/rpi/awb.hpp
>
> [snip]
>
> > @@ -141,22 +141,22 @@ private:
> > StatisticsPtr statistics_;
> > AwbMode *mode_;
> > double lux_;
> > - AwbStatus async_results_;
> > + AwbStatus asyncResults_;
> > void doAwb();
> > void awbBayes();
> > void awbGrey();
> > void prepareStats();
> > - double computeDelta2Sum(double gain_r, double gain_b);
> > + double computeDelta2Sum(double gain_r, double gainB);
>
> s/gain_r/gainR/
>
> > Pwl interpolatePrior();
> > double coarseSearch(Pwl const &prior);
> > void fineSearch(double &t, double &r, double &b, Pwl const &prior);
> > std::vector<RGB> zones_;
> > std::vector<Pwl::Point> points_;
> > // manual r setting
> > - double manual_r_;
> > + double manualR_;
> > // manual b setting
> > - double manual_b_;
> > - bool first_switch_mode_; // is this the first call to SwitchMode?
> > + double manualB_;
> > + bool firstSwitchMode_; // is this the first call to SwitchMode?
> > };
>
> [snip]
>
> > diff --git a/src/ipa/raspberrypi/md_parser_smia.cpp
> b/src/ipa/raspberrypi/md_parser_smia.cpp
> > index ea5eac414b36..9fab6594baac 100644
> > --- a/src/ipa/raspberrypi/md_parser_smia.cpp
> > +++ b/src/ipa/raspberrypi/md_parser_smia.cpp
>
> [snip]
>
> > @@ -76,74 +76,74 @@ MdParserSmia::ParseStatus
> MdParserSmia::findRegs(libcamera::Span<const uint8_t>
> > {
> > ASSERT(offsets_.size());
> >
> > - if (buffer[0] != LINE_START)
> > - return NO_LINE_START;
> > + if (buffer[0] != LineStart)
> > + return NoLineStart;
> >
> > - unsigned int current_offset = 1; /* after the LINE_START */
> > - unsigned int current_line_start = 0, current_line = 0;
> > - unsigned int reg_num = 0, regs_done = 0;
> > + unsigned int currentOffset = 1; /* after the LineStart */
> > + unsigned int currentLineStart = 0, currentLine = 0;
> > + unsigned int regNum = 0, regsDone = 0;
> >
> > while (1) {
> > - int tag = buffer[current_offset++];
> > -
> > - if ((bits_per_pixel_ == 10 &&
> > - (current_offset + 1 - current_line_start) % 5 == 0) ||
> > - (bits_per_pixel_ == 12 &&
> > - (current_offset + 1 - current_line_start) % 3 == 0)) {
> > - if (buffer[current_offset++] != REG_SKIP)
> > - return BAD_DUMMY;
> > + int tag = buffer[currentOffset++];
> > +
> > + if ((bitsPerPixel_ == 10 &&
> > + (currentOffset + 1 - currentLineStart) % 5 == 0) ||
> > + (bitsPerPixel_ == 12 &&
> > + (currentOffset + 1 - currentLineStart) % 3 == 0)) {
> > + if (buffer[currentOffset++] != RegSkip)
> > + return BadDummy;
> > }
> >
> > - int data_byte = buffer[current_offset++];
> > + int dataByte = buffer[currentOffset++];
> >
> > - if (tag == LINE_END_TAG) {
> > - if (data_byte != LINE_END_TAG)
> > - return BAD_LINE_END;
> > + if (tag == LineEndTag) {
> > + if (dataByte != LineEndTag)
> > + return BadLineEnd;
> >
> > - if (num_lines_ && ++current_line == num_lines_)
> > - return MISSING_REGS;
> > + if (numLines_ && ++currentLine == numLines_)
> > + return MissingRegs;
> >
> > - if (line_length_bytes_) {
> > - current_offset = current_line_start +
> line_length_bytes_;
> > + if (lineLengthBytes_) {
> > + currentOffset = currentLineStart +
> lineLengthBytes_;
> >
> > /* Require whole line to be in the buffer
> (if buffer size set). */
> > if (buffer.size() &&
> > - current_offset + line_length_bytes_ >
> buffer.size())
> > - return MISSING_REGS;
> > + currentOffset + lineLengthBytes_ >
> buffer.size())
> > + return MissingRegs;
> >
> > - if (buffer[current_offset] != LINE_START)
> > - return NO_LINE_START;
> > + if (buffer[currentOffset] != LineStart)
> > + return NoLineStart;
> > } else {
> > /* allow a zero line length to mean "hunt
> for the next line" */
> > - while (current_offset < buffer.size() &&
> > - buffer[current_offset] !=
> LINE_START)
> > - current_offset++;
> > + while (currentOffset < buffer.size() &&
> > + buffer[currentOffset] != LineStart)
> > + currentOffset++;
> >
> > - if (current_offset == buffer.size())
> > - return NO_LINE_START;
> > + if (currentOffset == buffer.size())
> > + return NoLineStart;
> > }
> >
> > - /* inc current_offset to after LINE_START */
> > - current_line_start = current_offset++;
> > + /* inc current_offset to after LineStart */
>
> s/current_offset/currentOffset/
>
> > + currentLineStart = currentOffset++;
> > } else {
> > - if (tag == REG_HI_BITS)
> > - reg_num = (reg_num & 0xff) | (data_byte <<
> 8);
> > - else if (tag == REG_LOW_BITS)
> > - reg_num = (reg_num & 0xff00) | data_byte;
> > - else if (tag == REG_SKIP)
> > - reg_num++;
> > - else if (tag == REG_VALUE) {
> > - auto reg = offsets_.find(reg_num);
> > + if (tag == RegHiBits)
> > + regNum = (regNum & 0xff) | (dataByte << 8);
> > + else if (tag == RegLowBits)
> > + regNum = (regNum & 0xff00) | dataByte;
> > + else if (tag == RegSkip)
> > + regNum++;
> > + else if (tag == RegValue) {
> > + auto reg = offsets_.find(regNum);
> >
> > if (reg != offsets_.end()) {
> > - offsets_[reg_num] = current_offset
> - 1;
> > + offsets_[regNum] = currentOffset -
> 1;
> >
> > - if (++regs_done == offsets_.size())
> > - return PARSE_OK;
> > + if (++regsDone == offsets_.size())
> > + return ParseOk;
> > }
> > - reg_num++;
> > + regNum++;
> > } else
> > - return ILLEGAL_TAG;
> > + return IllegalTag;
> > }
> > }
> > }
>
> [snip]
>
> I'll handle all these smalll issues when applying.
>
Groan! I'm sure there are still more lurking around... :-(
Thanks for the fixups!
Naush
> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
>
> --
> Regards,
>
> Laurent Pinchart
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20220727/f6cf0e8f/attachment.htm>
More information about the libcamera-devel
mailing list