[PATCH v2 04/20] libcamera: ipu3: Formatting improvements
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Sat Aug 31 01:36:31 CEST 2024
Hi Milan,
Thank you for the patch.
On Fri, Aug 30, 2024 at 05:27:01PM +0200, Milan Zamazal wrote:
> The LSP autoformatter doesn't like some of the current formatting, let's
> make it happy.
>
> Signed-off-by: Milan Zamazal <mzamazal at redhat.com>
> ---
> src/ipa/ipu3/algorithms/agc.cpp | 31 ++++++++++-----------
> src/ipa/ipu3/algorithms/blc.cpp | 4 +--
> src/ipa/ipu3/ipu3.cpp | 11 ++++----
> src/libcamera/pipeline/ipu3/ipu3.cpp | 40 +++++++++++++++-------------
> 4 files changed, 45 insertions(+), 41 deletions(-)
>
> diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp
> index 3378c4fd..548b64a4 100644
> --- a/src/ipa/ipu3/algorithms/agc.cpp
> +++ b/src/ipa/ipu3/algorithms/agc.cpp
> @@ -14,6 +14,7 @@
> #include <libcamera/base/utils.h>
>
> #include <libcamera/control_ids.h>
> +
Ack.
> #include <libcamera/ipa/core_ipa_interface.h>
>
> #include "libipa/histogram.h"
> @@ -136,11 +137,9 @@ Histogram Agc::parseStatistics(const ipu3_uapi_stats_3a *stats,
> reinterpret_cast<const ipu3_uapi_awb_set_item *>(
> &stats->awb_raw_buffer.meta_data[cellPosition]);
>
> - rgbTriples_.push_back({
> - cell->R_avg,
> - (cell->Gr_avg + cell->Gb_avg) / 2,
> - cell->B_avg
> - });
> + rgbTriples_.push_back({ cell->R_avg,
> + (cell->Gr_avg + cell->Gb_avg) / 2,
> + cell->B_avg });
I think this hinders readability.
>
> /*
> * Store the average green value to estimate the
> @@ -184,9 +183,10 @@ double Agc::estimateLuminance(double gain) const
> blueSum += std::min(std::get<2>(rgbTriples_[i]) * gain, 255.0);
> }
>
> - double ySum = redSum * rGain_ * 0.299
> - + greenSum * gGain_ * 0.587
> - + blueSum * bGain_ * 0.114;
> + double ySum =
> + redSum * rGain_ * 0.299 +
> + greenSum * gGain_ * 0.587 +
> + blueSum * bGain_ * 0.114;
Ditto.
>
> return ySum / (bdsGrid_.height * bdsGrid_.width) / 255;
> }
> @@ -216,8 +216,8 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,
> * The Agc algorithm needs to know the effective exposure value that was
> * applied to the sensor when the statistics were collected.
> */
> - utils::Duration exposureTime = context.configuration.sensor.lineDuration
> - * frameContext.sensor.exposure;
> + utils::Duration exposureTime =
> + context.configuration.sensor.lineDuration * frameContext.sensor.exposure;
Ditto.
> double analogueGain = frameContext.sensor.gain;
> utils::Duration effectiveExposureValue = exposureTime * analogueGain;
>
> @@ -241,12 +241,13 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,
> metadata.set(controls::ExposureTime, exposureTime.get<std::micro>());
>
> /* \todo Use VBlank value calculated from each frame exposure. */
> - uint32_t vTotal = context.configuration.sensor.size.height
> - + context.configuration.sensor.defVBlank;
> - utils::Duration frameDuration = context.configuration.sensor.lineDuration
> - * vTotal;
> + uint32_t vTotal =
> + context.configuration.sensor.size.height +
> + context.configuration.sensor.defVBlank;
> + utils::Duration frameDuration =
> + context.configuration.sensor.lineDuration *
> + vTotal;
Ditto.
> metadata.set(controls::FrameDuration, frameDuration.get<std::micro>());
> -
Ack.
> }
>
> REGISTER_IPA_ALGORITHM(Agc, "Agc")
> diff --git a/src/ipa/ipu3/algorithms/blc.cpp b/src/ipa/ipu3/algorithms/blc.cpp
> index fa4b9272..35748fb2 100644
> --- a/src/ipa/ipu3/algorithms/blc.cpp
> +++ b/src/ipa/ipu3/algorithms/blc.cpp
> @@ -55,8 +55,8 @@ void BlackLevelCorrection::prepare([[maybe_unused]] IPAContext &context,
> * tuning processes. This is a first rough approximation.
> */
> params->obgrid_param.gr = 64;
> - params->obgrid_param.r = 64;
> - params->obgrid_param.b = 64;
> + params->obgrid_param.r = 64;
> + params->obgrid_param.b = 64;
Not strong opinion.
> params->obgrid_param.gb = 64;
>
> /* Enable the custom black level correction processing */
> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
> index 656c51fc..e6b2b5bb 100644
> --- a/src/ipa/ipu3/ipu3.cpp
> +++ b/src/ipa/ipu3/ipu3.cpp
> @@ -24,10 +24,11 @@
>
> #include <libcamera/control_ids.h>
> #include <libcamera/framebuffer.h>
> +#include <libcamera/request.h>
> +
> #include <libcamera/ipa/ipa_interface.h>
> #include <libcamera/ipa/ipa_module_info.h>
> #include <libcamera/ipa/ipu3_ipa_interface.h>
> -#include <libcamera/request.h>
Ack.
>
> #include "libcamera/internal/mapped_framebuffer.h"
> #include "libcamera/internal/yaml_parser.h"
> @@ -308,8 +309,8 @@ int IPAIPU3::init(const IPASettings &settings,
>
> /* Clean context */
> context_.configuration = {};
> - context_.configuration.sensor.lineDuration = sensorInfo.minLineLength
> - * 1.0s / sensorInfo.pixelRate;
> + context_.configuration.sensor.lineDuration =
> + sensorInfo.minLineLength * 1.0s / sensorInfo.pixelRate;
I'd keep the existing formatting but I'm OK either way.
>
> /* Load the tuning data file. */
> File file(settings.configurationFile);
> @@ -472,8 +473,8 @@ int IPAIPU3::configure(const IPAConfigInfo &configInfo,
> context_.frameContexts.clear();
>
> /* Initialise the sensor configuration. */
> - context_.configuration.sensor.lineDuration = sensorInfo_.minLineLength
> - * 1.0s / sensorInfo_.pixelRate;
> + context_.configuration.sensor.lineDuration =
> + sensorInfo_.minLineLength * 1.0s / sensorInfo_.pixelRate;
Ditto.
> context_.configuration.sensor.size = sensorInfo_.outputSize;
>
> /*
> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> index 29172f34..6b4fe486 100644
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> @@ -18,12 +18,13 @@
> #include <libcamera/camera.h>
> #include <libcamera/control_ids.h>
> #include <libcamera/formats.h>
> -#include <libcamera/ipa/ipu3_ipa_interface.h>
> -#include <libcamera/ipa/ipu3_ipa_proxy.h>
> #include <libcamera/property_ids.h>
> #include <libcamera/request.h>
> #include <libcamera/stream.h>
>
> +#include <libcamera/ipa/ipu3_ipa_interface.h>
> +#include <libcamera/ipa/ipu3_ipa_proxy.h>
> +
Ack.
> #include "libcamera/internal/camera.h"
> #include "libcamera/internal/camera_lens.h"
> #include "libcamera/internal/camera_sensor.h"
> @@ -417,9 +418,9 @@ PipelineHandlerIPU3::generateConfiguration(Camera *camera, Span<const StreamRole
> * in validate()
> */
> size = sensorResolution.boundedTo(ImgUDevice::kOutputMaxSize)
> - .shrunkBy({ 1, 1 })
> - .alignedDownTo(ImgUDevice::kOutputMarginWidth,
> - ImgUDevice::kOutputMarginHeight);
> + .shrunkBy({ 1, 1 })
> + .alignedDownTo(ImgUDevice::kOutputMarginWidth,
> + ImgUDevice::kOutputMarginHeight);
Nack.
> pixelFormat = formats::NV12;
> bufferCount = IPU3CameraConfiguration::kBufferCount;
> streamFormats[pixelFormat] = { { ImgUDevice::kOutputMinSize, size } };
> @@ -447,8 +448,8 @@ PipelineHandlerIPU3::generateConfiguration(Camera *camera, Span<const StreamRole
> * to the ImgU output constraints.
> */
> size = sensorResolution.boundedTo(kViewfinderSize)
> - .alignedDownTo(ImgUDevice::kOutputAlignWidth,
> - ImgUDevice::kOutputAlignHeight);
> + .alignedDownTo(ImgUDevice::kOutputAlignWidth,
> + ImgUDevice::kOutputAlignHeight);
Nack.
> pixelFormat = formats::NV12;
> bufferCount = IPU3CameraConfiguration::kBufferCount;
> streamFormats[pixelFormat] = { { ImgUDevice::kOutputMinSize, size } };
> @@ -991,18 +992,19 @@ int PipelineHandlerIPU3::updateControls(IPU3CameraData *data)
> */
>
> /* The strictly smaller size than the sensor resolution, aligned to margins. */
> - Size minSize = sensor->resolution().shrunkBy({ 1, 1 })
> - .alignedDownTo(ImgUDevice::kOutputMarginWidth,
> - ImgUDevice::kOutputMarginHeight);
> + Size minSize = sensor->resolution()
> + .shrunkBy({ 1, 1 })
> + .alignedDownTo(ImgUDevice::kOutputMarginWidth,
> + ImgUDevice::kOutputMarginHeight);
I don't like this one much.
>
> /*
> * Either the smallest margin-aligned size larger than the viewfinder
> * size or the adjusted sensor resolution.
> */
> minSize = kViewfinderSize.grownBy({ 1, 1 })
> - .alignedUpTo(ImgUDevice::kOutputMarginWidth,
> - ImgUDevice::kOutputMarginHeight)
> - .boundedTo(minSize);
> + .alignedUpTo(ImgUDevice::kOutputMarginWidth,
> + ImgUDevice::kOutputMarginHeight)
> + .boundedTo(minSize);
Nack.
>
> /*
> * Re-scale in the sensor's native coordinates. Report (0,0) as
> @@ -1116,19 +1118,19 @@ int PipelineHandlerIPU3::registerCameras()
> * returned through the ImgU main and secondary outputs.
> */
> data->cio2_.bufferReady().connect(data.get(),
> - &IPU3CameraData::cio2BufferReady);
> + &IPU3CameraData::cio2BufferReady);
Ack for the rest.
> data->cio2_.bufferAvailable.connect(
> data.get(), &IPU3CameraData::queuePendingRequests);
> data->imgu_->input_->bufferReady.connect(&data->cio2_,
> - &CIO2Device::tryReturnBuffer);
> + &CIO2Device::tryReturnBuffer);
> data->imgu_->output_->bufferReady.connect(data.get(),
> - &IPU3CameraData::imguOutputBufferReady);
> + &IPU3CameraData::imguOutputBufferReady);
> data->imgu_->viewfinder_->bufferReady.connect(data.get(),
> - &IPU3CameraData::imguOutputBufferReady);
> + &IPU3CameraData::imguOutputBufferReady);
> data->imgu_->param_->bufferReady.connect(data.get(),
> - &IPU3CameraData::paramBufferReady);
> + &IPU3CameraData::paramBufferReady);
> data->imgu_->stat_->bufferReady.connect(data.get(),
> - &IPU3CameraData::statBufferReady);
> + &IPU3CameraData::statBufferReady);
>
> /* Create and register the Camera instance. */
> const std::string &cameraId = cio2->sensor()->id();
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list