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