[PATCH v3 02/16] libcamera: lc-compliance: Add TimeSheet class

[Jacopo Mondi]

Hi Stefan

On Tue, Mar 19, 2024 at 01:05:03PM +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 | 148 ++++++++++++++++++++++++++
>  src/apps/lc-compliance/time_sheet.h   |  62 +++++++++++
>  3 files changed, 211 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..8048cf30
> --- /dev/null
> +++ b/src/apps/lc-compliance/time_sheet.cpp
> @@ -0,0 +1,148 @@
> +/* 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;
> +
> +namespace {
> +
> +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;
> +
> +		const uint8_t *data = in.planes()[0].data();
> +		const auto &streamConfig = stream->configuration();
> +		const auto &formatInfo = PixelFormatInfo::info(streamConfig.pixelFormat);
> +		double (*calcPixelMean)(const uint8_t *);
> +		int pixelStride;
> +
> +		switch (streamConfig.pixelFormat) {
> +		case formats::NV12:
> +			calcPixelMean = [](const uint8_t *d) -> double {
> +				return static_cast<double>(*d);
> +			};
> +			pixelStride = 1;
> +			break;
> +		case formats::SRGGB10:
> +			calcPixelMean = [](const uint8_t *d) -> double {
> +				return static_cast<double>(*reinterpret_cast<const uint16_t *>(d));
> +			};
> +			pixelStride = 2;
> +			break;
> +		default:
> +			std::stringstream s;
> +			s << "Unsupported Pixelformat " << formatInfo.name;
> +			throw std::invalid_argument(s.str());

we don't use exceptions (even if I see a throw in
src/apps/lc-compliance/main.cpp ... )
> +		}
> +
> +		double sum = 0;
> +		int w = 20;
> +		int xs = streamConfig.size.width / 2 - w / 2;
> +		int ys = streamConfig.size.height / 2 - w / 2;
> +
> +		for (auto y = ys; y < ys + w; y++) {
> +			auto line = data + y * streamConfig.stride;

Empy line after variable declaration is nice (and usually enforced in
the kernel coding style)
> +			for (auto x = xs; x < xs + w; x++)
> +				sum += calcPixelMean(line + x * pixelStride);
> +		}

nit: I would also empty line here to give the code some space to
breathe

> +		sum = sum / (w * w);
> +		return sum;
> +	}
> +
> +	return 0;
> +}
> +
> +} /* namespace */
> +
> +TimeSheetEntry::TimeSheetEntry(const ControlIdMap &idmap)
> +	: controls_(idmap)
> +{
> +}
> +
> +void TimeSheetEntry::handleCompleteRequest(libcamera::Request *request,
> +					   const TimeSheetEntry *previous)
> +{
> +	metadata_ = request->metadata();
> +
> +	spotBrightness_ = calculateMeanBrightnessFromCenterSpot(request);
> +	if (previous)
> +		brightnessChange_ = spotBrightness_ / previous->spotBrightness();
> +
> +	sequence_ = request->sequence();
> +}
> +
> +std::ostream &operator<<(std::ostream &os, const TimeSheetEntry &te)
> +{
> +	os << "=== Frame " << te.sequence_ << std::endl;
> +	if (!te.controls_.empty()) {
> +		os << "Controls:" << std::endl;
> +		const auto &idMap = te.controls_.idMap();
> +		assert(idMap);
> +		for (const auto &[id, value] : te.controls_)
> +			os << "  " << idMap->at(id)->name() << " : "
> +			   << value.toString() << std::endl;
> +	}
> +
> +	if (!te.metadata_.empty()) {
> +		os << "Metadata:" << std::endl;
> +		const auto &idMap = te.metadata_.idMap();
> +		assert(idMap);
> +		for (const auto &[id, value] : te.metadata_)
> +			os << "  " << idMap->at(id)->name() << " : "
> +			   << value.toString() << std::endl;
> +	}
> +
> +	os << "Calculated Brightness: " << te.spotBrightness() << std::endl;
> +	return os;
> +}
> +
> +TimeSheetEntry &TimeSheet::get(size_t pos)
> +{
> +	entries_.reserve(pos + 1);

Now, I've asked in v2 to check if pos < count as accessing a vector
with operator[] is unbounded. But my understanding was that pos
should never be >= count, and if this happen, it's an error. Do you
expect this to be a valid condition ? What if you get pos = UINT_MAX ?

> +	auto &entry = entries_[pos];
> +	if (!entry)
> +		entry = std::make_unique<TimeSheetEntry>(idmap_);
> +
> +	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;
> +	if (sequence >= 1)
> +		previous = entries_[sequence - 1].get();
> +
> +	entry.handleCompleteRequest(request, previous);
> +}
> +
> +std::ostream &operator<<(std::ostream &os, const TimeSheet &ts)
> +{
> +	for (const auto &entry : ts.entries_) {
> +		if (entry)
> +			os << *entry;
> +	}
> +
> +	return os;
> +}
> diff --git a/src/apps/lc-compliance/time_sheet.h b/src/apps/lc-compliance/time_sheet.h
> new file mode 100644
> index 00000000..417277aa
> --- /dev/null
> +++ b/src/apps/lc-compliance/time_sheet.h
> @@ -0,0 +1,62 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2024, Ideas on Board Oy
> + *
> + * time_sheet.h
> + */
> +
> +#pragma once
> +
> +#include <memory>
> +#include <ostream>
> +#include <vector>
> +
> +#include <libcamera/libcamera.h>
> +
> +class TimeSheetEntry
> +{
> +public:
> +	TimeSheetEntry() = delete;
> +	TimeSheetEntry(const libcamera::ControlIdMap &idmap);
> +	TimeSheetEntry(TimeSheetEntry &&other) = default;
> +	TimeSheetEntry(const TimeSheetEntry &) = delete;
> +	~TimeSheetEntry() = default;
> +
> +	libcamera::ControlList &controls() { return controls_; }
> +	const libcamera::ControlList &metadata() const { return metadata_; }
> +	void handleCompleteRequest(libcamera::Request *request,
> +				   const TimeSheetEntry *previous);
> +	double spotBrightness() const { return spotBrightness_; }
> +	double brightnessChange() const { return brightnessChange_; }
> +
> +	friend std::ostream &operator<<(std::ostream &os, const TimeSheetEntry &te);
> +
> +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)
> +	{
> +	}
> +
> +	void prepareForQueue(libcamera::Request *request, uint32_t sequence);
> +	void handleCompleteRequest(libcamera::Request *request);
> +
> +	TimeSheetEntry &operator[](size_t pos) { return get(pos); }
> +	TimeSheetEntry &get(size_t pos);
> +	size_t size() const { return entries_.size(); }
> +
> +	friend std::ostream &operator<<(std::ostream &os, const TimeSheet &ts);
> +
> +private:
> +	const libcamera::ControlIdMap &idmap_;
> +	std::vector<std::unique_ptr<TimeSheetEntry>> entries_;
> +};
> --
> 2.40.1
>