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

19704 diff mbox series

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@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

Hi


2024. március 13., szerda 13:12 keltezéssel, Stefan Klug <stefan.klug@ideasonboard.com> írta:

> 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@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;
> +}
> +
> +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()) {
> +			auto data = in.planes()[0].data();
> +			auto streamConfig = stream->configuration();
> +			auto formatInfo = PixelFormatInfo::info(streamConfig.pixelFormat);

Use `const auto &` or `auto &&` with `streamConfig` and `formatInfo` to avoid copies.


> +
> +			std::function<double(const uint8_t *data)> calcPixelMean;

Just use `double (*calcPixelMean)(const uint8_t *)`, std::function is an overkill here.


> +			int pixelStride;
> +
> +			switch (streamConfig.pixelFormat) {
> +			case formats::NV12:
> +				calcPixelMean = calcPixelMeanNV12;

You could alternatively just say

  calcPixelMean = [](const uint8_t *data) -> double { return *data; };

and similarly below. Then you wouldn't need the named functions.


> +				pixelStride = 1;
> +				break;
> +			case formats::SRGGB10:
> +				calcPixelMean = calcPixelMeanRAW10;
> +				pixelStride = 2;
> +				break;
> +			default:
> +				std::stringstream s;
> +				s << "Unsupported Pixelformat " << formatInfo.name;
> +				throw std::invalid_argument(s.str());
> +			}
> +
> +			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;
> +				for (auto x = xs; x < xs + w; x++) {
> +					sum += calcPixelMean(line + x * pixelStride);
> +				}
> +			}
> +			sum = sum / (w * w);
> +			return sum;
> +		}
> +	}
> +	return 0;
> +}

The above functions should be in an anonymous namespace if they are not
used outside this translation unit.


> +
> +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->getSpotBrightness();
> +	}
> +	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;
> +		}
> +	}
> +
> +	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;
> +		}
> +	}
> +
> +	std::cout << "Calculated Brightness: " << spotBrightness_ << std::endl;
> +}
> +
> +TimeSheetEntry &TimeSheet::get(size_t pos)
> +{
> +	auto &entry = entries_[pos];
> +	if (!entry)
> +		entry = std::make_shared<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);
> +}
> +
> +void TimeSheet::printAllInfos()
> +{
> +	for (auto entry : entries_) {

`const auto &entry` to avoid the copy.


> +		if (entry)
> +			entry->printInfo();
> +	}
> +}
> 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>
> +#include <vector>
> +
> +#include <libcamera/libcamera.h>
> +
> +class TimeSheetEntry
> +{
> +public:
> +	TimeSheetEntry() = delete;
> +	TimeSheetEntry(const libcamera::ControlIdMap &idmap);
> +	TimeSheetEntry(TimeSheetEntry &&other) noexcept = default;
> +	TimeSheetEntry(const TimeSheetEntry &) = delete;
> +
> +	libcamera::ControlList &controls() { return controls_; };
> +	libcamera::ControlList &metadata() { return metadata_; };
> +	void handleCompleteRequest(libcamera::Request *request, const TimeSheetEntry *previous);
> +	void printInfo();
> +	double getSpotBrightness() const { return spotBrightness_; };
> +	double getBrightnessChange() const { return brightnessChange_; };
> +
> +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){};

No `;` is needed at the end. I'd probably put the `{ }` in a new line
to make it clear that that is the function body.

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

Why are get() and operator[] needed when they both do the same thing?


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

Does this need to be shared_ptr? At first glance it seems to me that
unique_ptr would be enough. 


> +};
> --
> 2.40.1


Regards,
Barnabás Pőcze

Hi Barnabás,

thank you for the review.

On Thu, Mar 14, 2024 at 07:07:32PM +0000, Barnabás Pőcze wrote:
> Hi
> 
> 
> 2024. március 13., szerda 13:12 keltezéssel, Stefan Klug <stefan.klug@ideasonboard.com> írta:
> 
> > 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@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;
> > +}
> > +
> > +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()) {
> > +			auto data = in.planes()[0].data();
> > +			auto streamConfig = stream->configuration();
> > +			auto formatInfo = PixelFormatInfo::info(streamConfig.pixelFormat);
> 
> Use `const auto &` or `auto &&` with `streamConfig` and `formatInfo` to avoid copies.
> 

Yes, right. I'll fix that.

> 
> > +
> > +			std::function<double(const uint8_t *data)> calcPixelMean;
> 
> Just use `double (*calcPixelMean)(const uint8_t *)`, std::function is an overkill here.
> 
> 
> > +			int pixelStride;
> > +
> > +			switch (streamConfig.pixelFormat) {
> > +			case formats::NV12:
> > +				calcPixelMean = calcPixelMeanNV12;
> 
> You could alternatively just say
> 
>   calcPixelMean = [](const uint8_t *data) -> double { return *data; };
> 
> and similarly below. Then you wouldn't need the named functions.
>

Oh, you are right. I tried that with a lambda capturing data, which is
not allowed. But with a non capturing lambda, it's fine.
 
> 
> > +				pixelStride = 1;
> > +				break;
> > +			case formats::SRGGB10:
> > +				calcPixelMean = calcPixelMeanRAW10;
> > +				pixelStride = 2;
> > +				break;
> > +			default:
> > +				std::stringstream s;
> > +				s << "Unsupported Pixelformat " << formatInfo.name;
> > +				throw std::invalid_argument(s.str());
> > +			}
> > +
> > +			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;
> > +				for (auto x = xs; x < xs + w; x++) {
> > +					sum += calcPixelMean(line + x * pixelStride);
> > +				}
> > +			}
> > +			sum = sum / (w * w);
> > +			return sum;
> > +		}
> > +	}
> > +	return 0;
> > +}
> 
> The above functions should be in an anonymous namespace if they are not
> used outside this translation unit.
> 

Ok.

> 
> > +
> > +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->getSpotBrightness();
> > +	}
> > +	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;
> > +		}
> > +	}
> > +
> > +	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;
> > +		}
> > +	}
> > +
> > +	std::cout << "Calculated Brightness: " << spotBrightness_ << std::endl;
> > +}
> > +
> > +TimeSheetEntry &TimeSheet::get(size_t pos)
> > +{
> > +	auto &entry = entries_[pos];
> > +	if (!entry)
> > +		entry = std::make_shared<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);
> > +}
> > +
> > +void TimeSheet::printAllInfos()
> > +{
> > +	for (auto entry : entries_) {
> 
> `const auto &entry` to avoid the copy.
> 

Yes

> 
> > +		if (entry)
> > +			entry->printInfo();
> > +	}
> > +}
> > 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>
> > +#include <vector>
> > +
> > +#include <libcamera/libcamera.h>
> > +
> > +class TimeSheetEntry
> > +{
> > +public:
> > +	TimeSheetEntry() = delete;
> > +	TimeSheetEntry(const libcamera::ControlIdMap &idmap);
> > +	TimeSheetEntry(TimeSheetEntry &&other) noexcept = default;
> > +	TimeSheetEntry(const TimeSheetEntry &) = delete;
> > +
> > +	libcamera::ControlList &controls() { return controls_; };
> > +	libcamera::ControlList &metadata() { return metadata_; };
> > +	void handleCompleteRequest(libcamera::Request *request, const TimeSheetEntry *previous);
> > +	void printInfo();
> > +	double getSpotBrightness() const { return spotBrightness_; };
> > +	double getBrightnessChange() const { return brightnessChange_; };
> > +
> > +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){};
> 
> No `;` is needed at the end. I'd probably put the `{ }` in a new line
> to make it clear that that is the function body.
> 

That was also found by the CI. Fixed.

> > +
> > +	void prepareForQueue(libcamera::Request *request, uint32_t sequence);
> > +	void handleCompleteRequest(libcamera::Request *request);
> > +	void printAllInfos();
> > +
> > +	TimeSheetEntry &operator[](size_t pos) { return get(pos); };
> > +	TimeSheetEntry &get(size_t pos);
> 
> Why are get() and operator[] needed when they both do the same thing?
> 

I wanted to have [] as syntactic shugar. If you have a pointer to a
timesheet, [] feels cumbersome and I left get() in there. It turns out I
only used it twice in the tests. I can remove get() if you want.

> 
> > +	size_t size() const { return entries_.size(); };
> > +
> > +private:
> > +	const libcamera::ControlIdMap &idmap_;
> > +	std::vector<std::shared_ptr<TimeSheetEntry>> entries_;
> 
> Does this need to be shared_ptr? At first glance it seems to me that
> unique_ptr would be enough. 
> 

Yes, I had a reason for that during development, but can't remember.
I'll switch to unique_ptr.

Thanks,
Stefan

> 
> > +};
> > --
> > 2.40.1
> 
> 
> Regards,
> Barnabás Pőcze

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@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
>

Hi Jacopo,

thanks for the review. I'll handle all the tiny style related stuff in
the next version. So I did not comment on these.
 
On Fri, Mar 15, 2024 at 02:12:13PM +0100, Jacopo Mondi wrote:
> 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@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 ?

That is handled by pixelStride in "line + x * pixelStride" so x is in
pexels. Or do I miss something here?

> 
> > +				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;

Oh I see. Sure.

> > +			}
> > +
> > +			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

Really? In my experience handling these things with unsigned ints often
leads to unexpected corner cases or ugly code (not beeing able to
subtract numbers without thinking twice, cast to signed for comparisons
etc.). The numbers are known to always be smaller than max int, so it
doesn't hurt. But I see that in this specific case having all as uint
would work.

> 
> > +
> > +			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 ?

No, as I'm always sampling the same amount of pixels.

> 
> 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 ?

Yes, I didn't see a need to handle multiple streams here. I couldn't
come up with a case where the first stream is invalid for the brightness
measurement. Do you have a usecase for that in mind?

> 
> > +		}
> > +	}
> 
> 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

I'm sorry, but I don't see the benefit here. In this case you
moved the knowledge of the underlying data types into the loops and can
loop over bytes (which fails in the "*pixel = ..." line). That gets more
complex when more formats shall be supported. My named functions are
definitely an overkill above. But looping over pixel positions and then
having a PixelFormat specific accessor function simplifies things IMHO.
I don't know if there is a runtime speed difference between the switch
inside the loop and the function calls...

The cleanest would be to also move pixelStride and lineStride into the
accessor, so that one could write the algorithm instead caring about the
data. Something like (pseudocode):

auto accessor = constructView(data, pixelFormat);
...
for (auto y = ys; y < ys + w; y++) {
    for (auto x = xs; x < xs + w; x++) {
	sum += accessor.calcPixelMean(x,y);
    }
}

> 
> > +
> > +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

Dang. Yes you are right. That is a bad one.

> 
> > +	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)

Well I tried to go for auto. But the compiler is unable to deduce the
type for previous, so I declared that explicitely. I remember a harsh
discussion where I tried to convince a teammate that auto shouldn't be
used. Now I'm advocating auto :-) Oh dear.

> 
> > +	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)

This was a leftover from my first try to use unique_ptr in the vector.
std::vector can only support move semantics on resize if constructor and
desctructor are noexcept. See https://stackoverflow.com/a/15418208 
So yes it will be used again :-)

Thanks again for your review.

Cheers,
Stefan

> 
> > +	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
> >