[PATCH v2 02/12] libcamera: lc-compliance: Add TimeSheet class

Jacopo Mondi jacopo.mondi at ideasonboard.com
Fri Mar 15 14:12:13 CET 2024


Hi Stefan

On Wed, Mar 13, 2024 at 01:12:13PM +0100, Stefan Klug wrote:
> This class allows us to prepare a time sheet with controls for the frames
> to capture (a bit like the capture script in cam).
>
> During capture the metadata is recorded. Additionally a mean brightness
> value of a 20x20 pixel spot in the middle of the frame is calculated.
>
> This allows easy analysis after running the capture without complicated state
> handling due to the asynchronous nature of the capturing process.
>
> Signed-off-by: Stefan Klug <stefan.klug at ideasonboard.com>
> ---
>  src/apps/lc-compliance/meson.build    |   1 +
>  src/apps/lc-compliance/time_sheet.cpp | 145 ++++++++++++++++++++++++++
>  src/apps/lc-compliance/time_sheet.h   |  55 ++++++++++
>  3 files changed, 201 insertions(+)
>  create mode 100644 src/apps/lc-compliance/time_sheet.cpp
>  create mode 100644 src/apps/lc-compliance/time_sheet.h
>
> diff --git a/src/apps/lc-compliance/meson.build b/src/apps/lc-compliance/meson.build
> index c792f072..eb7b2d71 100644
> --- a/src/apps/lc-compliance/meson.build
> +++ b/src/apps/lc-compliance/meson.build
> @@ -16,6 +16,7 @@ lc_compliance_sources = files([
>      'environment.cpp',
>      'main.cpp',
>      'simple_capture.cpp',
> +    'time_sheet.cpp',
>  ])
>
>  lc_compliance  = executable('lc-compliance', lc_compliance_sources,
> diff --git a/src/apps/lc-compliance/time_sheet.cpp b/src/apps/lc-compliance/time_sheet.cpp
> new file mode 100644
> index 00000000..0ac544d6
> --- /dev/null
> +++ b/src/apps/lc-compliance/time_sheet.cpp
> @@ -0,0 +1,145 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2024, Ideas on Board Oy
> + *
> + * time_sheet.cpp
> + */
> +
> +#include "time_sheet.h"
> +
> +#include <sstream>
> +#include <libcamera/libcamera.h>
> +
> +#include "libcamera/internal/formats.h"
> +#include "libcamera/internal/mapped_framebuffer.h"
> +
> +using namespace libcamera;
> +
> +double calcPixelMeanNV12(const uint8_t *data)
> +{
> +	return (double)*data;

We try to avoid plain C casts

These can be

double calcPixelMeanNV12(const uint8_t *data)
{
	return *data;
}

double calcPixelMeanRAW10(const uint8_t *data)
{
	return *reinterpret_cast<const uint16_t *>(data);
}

(more on this below)

> +}
> +
> +double calcPixelMeanRAW10(const uint8_t *data)
> +{
> +	return (double)*((const uint16_t *)data);
> +}
> +
> +double calculateMeanBrightnessFromCenterSpot(libcamera::Request *request)
> +{
> +	const Request::BufferMap &buffers = request->buffers();
> +	for (const auto &[stream, buffer] : buffers) {
> +		MappedFrameBuffer in(buffer, MappedFrameBuffer::MapFlag::Read);
> +		if (in.isValid()) {

                if (!in.isValid())
                        continue;

And you can save one indentation level

> +			auto data = in.planes()[0].data();

auto is convenient but hinders readability when the type is not
trivial. Also 'uint8_t *' is almost the same number of characters to
type in

> +			auto streamConfig = stream->configuration();
> +			auto formatInfo = PixelFormatInfo::info(streamConfig.pixelFormat);

Here it's more ok to use 'auto' as the variables are not used for any
computation like 'data' is (however I still don't like it but it's
maybe more a personal taste thing)
> +
> +			std::function<double(const uint8_t *data)> calcPixelMean;
> +			int pixelStride;
> +
> +			switch (streamConfig.pixelFormat) {
> +			case formats::NV12:
> +				calcPixelMean = calcPixelMeanNV12;
> +				pixelStride = 1;
> +				break;
> +			case formats::SRGGB10:
> +				calcPixelMean = calcPixelMeanRAW10;
> +				pixelStride = 2;

Shouldn't you then advance 'x' by 2 in the below loop if you consider 2
bytes at a time ?

> +				break;
> +			default:
> +				std::stringstream s;
> +				s << "Unsupported Pixelformat " << formatInfo.name;
> +				throw std::invalid_argument(s.str());

libcamera doesn't use exceptions, or are tests different ?

This can just be
                                std::cerr << ... ;

                                return 0;
> +			}
> +
> +			double sum = 0;

> +			int w = 20;

This is a constant
                        constexpr unsigned int kSampleSize = 20;

> +			int xs = streamConfig.size.width / 2 - w / 2;
> +			int ys = streamConfig.size.height / 2 - w / 2;

These are unsigned

> +
> +			for (auto y = ys; y < ys + w; y++) {
> +				auto line = data + y * streamConfig.stride;
> +				for (auto x = xs; x < xs + w; x++) {
> +					sum += calcPixelMean(line + x * pixelStride);
> +				}

No {} for single lines

> +			}
> +			sum = sum / (w * w);

Should the number of sampled pixels be divided by 2 for RAW10 (or should the
sample size be doubled) because you're sampling 2 bytes at a time ?

To follow the kernel coding style, empty line before return when
possibile/appropriate

> +			return sum;

You stop at the first valid stream, is this intended ?

> +		}
> +	}

ditto, empty line

> +	return 0;
> +}

Can this be then

double calculateMeanBrightnessFromCenterSpot(libcamera::Request *request)
{
	const Request::BufferMap &buffers = request->buffers();
	for (const auto &[stream, buffer] : buffers) {
		MappedFrameBuffer in(buffer, MappedFrameBuffer::MapFlag::Read);
		if (!in.isValid())
			continue;

		auto streamConfig = stream->configuration();
		auto formatInfo = PixelFormatInfo::info(streamConfig.pixelFormat);

		constexpr unsigned int kNumSamples = 20;
		unsigned int pixelStride;

		switch (streamConfig.pixelFormat) {
		case formats::NV12:
			pixelStride = 1;
			break;
		case formats::SRGGB10:
			pixelStride = 2;
			break;
		default:
			std::cerr << "Unsupported Pixelformat "
				  << formatInfo.name << std::endl;
			return 0;
		}

		unsigned int sampleSize = kNumSamples * pixelStride;
		unsigned int xs = streamConfig.size.width / 2 - sampleSize / 2;
		unsigned int ys = streamConfig.size.height / 2 - sampleSize / 2;
		const uint8_t *data = in.planes()[0].data();
		double sum = 0;

		for (unsigned int y = ys; y < ys + sampleSize ; y++) {
			const uint8_t *line = data + y * streamConfig.stride;

			for (unsigned int x = xs; x < xs + sampleSize; x += pixelStride ) {
				const uint8_t *pixel = line + x * pixelStride;

				switch (streamConfig.pixelFormat) {
				case formats::NV12:
					sum += static_cast<double>(*pixel);
					break;
				case formats::SRGGB10:
					auto *p = reinterpret_cast<const uint16_t *>(pixel);
					sum += static_cast<double>(*p);
					break;
				}
			}
		}

		return sum / kNumSamples * kNumSamples;
	}

	return 0;
}

I admit I haven't tested ia, butt only compiled it

> +
> +TimeSheetEntry::TimeSheetEntry(const ControlIdMap &idmap)
> +	: controls_(idmap)
> +{
> +}
> +
> +void TimeSheetEntry::handleCompleteRequest(libcamera::Request *request, const TimeSheetEntry *previous)

If it's easy to do, try to stay in 80 cols (libcamera allows up to 120
actually, but when it's trivial such as in this case...

void TimeSheetEntry::handleCompleteRequest(libcamera::Request *request,
					   const TimeSheetEntry *previous)

> +{
> +	metadata_ = request->metadata();
> +
> +	spotBrightness_ = calculateMeanBrightnessFromCenterSpot(request);
> +	if (previous) {
> +		brightnessChange_ = spotBrightness_ / previous->getSpotBrightness();
> +	}

No {}

> +	sequence_ = request->sequence();
> +}
> +
> +void TimeSheetEntry::printInfo()
> +{
> +	std::cout << "=== Frame " << sequence_ << std::endl;
> +	if (!controls_.empty()) {
> +		std::cout << "Controls:" << std::endl;
> +		auto idMap = controls_.idMap();
> +		assert(idMap);
> +		for (const auto &[id, value] : controls_) {
> +			std::cout << "  " << idMap->at(id)->name() << " : " << value.toString() << std::endl;
> +		}

ditto, no {}

> +	}
> +
> +	if (!metadata_.empty()) {
> +		std::cout << "Metadata:" << std::endl;
> +		auto idMap = metadata_.idMap();
> +		assert(idMap);
> +		for (const auto &[id, value] : metadata_) {
> +			std::cout << "  " << idMap->at(id)->name() << " : " << value.toString() << std::endl;
> +		}

same here

> +	}
> +
> +	std::cout << "Calculated Brightness: " << spotBrightness_ << std::endl;
> +}
> +
> +TimeSheetEntry &TimeSheet::get(size_t pos)
> +{
> +	auto &entry = entries_[pos];

Should you make sure pos is smaller than the allocated vector lenght ?
Afaik operator[] is not bound checked

> +	if (!entry)
> +		entry = std::make_shared<TimeSheetEntry>(idmap_);

empty line please

> +	return *entry;
> +}
> +
> +void TimeSheet::prepareForQueue(libcamera::Request *request, uint32_t sequence)
> +{
> +	request->controls() = get(sequence).controls();
> +}
> +
> +void TimeSheet::handleCompleteRequest(libcamera::Request *request)
> +{
> +	uint32_t sequence = request->sequence();
> +	auto &entry = get(sequence);
> +	TimeSheetEntry *previous = nullptr;

Why one 'auto' and one explicit type ? (You know my preference
already, but up yo you which one to chose as long as you're
consistent)

> +	if (sequence >= 1) {
> +		previous = entries_[sequence - 1].get();

Could this be: get(sequence - 1).get() or you don't like get().get() ?

> +	}

no braces

> +
> +	entry.handleCompleteRequest(request, previous);
> +}
> +
> +void TimeSheet::printAllInfos()
> +{
> +	for (auto entry : entries_) {
> +		if (entry)
> +			entry->printInfo();
> +	}

For multiple lines statements like this, even if not required by the
language, {} are fine

> +}
> diff --git a/src/apps/lc-compliance/time_sheet.h b/src/apps/lc-compliance/time_sheet.h
> new file mode 100644
> index 00000000..2bb89ab3
> --- /dev/null
> +++ b/src/apps/lc-compliance/time_sheet.h
> @@ -0,0 +1,55 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2024, Ideas on Board Oy
> + *
> + * time_sheet.h
> + */
> +
> +#pragma once
> +
> +#include <future>

What for ?

> +#include <vector>

Also include <memory> for smart pointers

> +
> +#include <libcamera/libcamera.h>
> +
> +class TimeSheetEntry
> +{
> +public:
> +	TimeSheetEntry() = delete;
> +	TimeSheetEntry(const libcamera::ControlIdMap &idmap);
> +	TimeSheetEntry(TimeSheetEntry &&other) noexcept = default;

Is this used ? (and I was not expecting noexcept as we don't use
exceptions, however we don't compile with exception disabled)

> +	TimeSheetEntry(const TimeSheetEntry &) = delete;
> +
> +	libcamera::ControlList &controls() { return controls_; };
> +	libcamera::ControlList &metadata() { return metadata_; };

const ..... const {};

for both

> +	void handleCompleteRequest(libcamera::Request *request, const TimeSheetEntry *previous);

we usually pass const parameters in by reference and not by pointer
(if possible and convenient ofc)

> +	void printInfo();

operator<< ?

> +	double getSpotBrightness() const { return spotBrightness_; };
> +	double getBrightnessChange() const { return brightnessChange_; };

Not used

libcamera doesn't usually prepend 'get' to getter function names

        type fieldName const { return fieldName_; }

and no need for ; at the of functions implementation (here and above)

> +
> +private:
> +	double spotBrightness_ = 0.0;
> +	double brightnessChange_ = 0.0;
> +	libcamera::ControlList controls_;
> +	libcamera::ControlList metadata_;
> +	uint32_t sequence_ = 0;
> +};
> +
> +class TimeSheet
> +{
> +public:
> +	TimeSheet(int count, const libcamera::ControlIdMap &idmap)
> +		: idmap_(idmap), entries_(count){};

braces in two lines below

        {
        }

No need for ;

> +
> +	void prepareForQueue(libcamera::Request *request, uint32_t sequence);
> +	void handleCompleteRequest(libcamera::Request *request);
> +	void printAllInfos();
> +
> +	TimeSheetEntry &operator[](size_t pos) { return get(pos); };

ditto

> +	TimeSheetEntry &get(size_t pos);
> +	size_t size() const { return entries_.size(); };
> +
> +private:
> +	const libcamera::ControlIdMap &idmap_;
> +	std::vector<std::shared_ptr<TimeSheetEntry>> entries_;

As noted already, you can use a unique_ptr<>

> +};
> --
> 2.40.1
>


More information about the libcamera-devel mailing list