[PATCH v3 02/16] libcamera: lc-compliance: Add TimeSheet class
Jacopo Mondi
jacopo.mondi at ideasonboard.com
Fri Mar 22 10:19:03 CET 2024
Hi Stefan
On Thu, Mar 21, 2024 at 12:04:35PM +0100, Stefan Klug wrote:
>
> Hi Jacopo,
>
> On Thu, Mar 21, 2024 at 10:42:43AM +0100, Jacopo Mondi wrote:
> > 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 ... )
>
> Oh, that got missed. Will be gone in v4.
>
> > > + }
> > > +
> > > + 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 ?
> >
>
> Well, passing UINT_MAX won't be funny for the system (equal to calling
> push_back() UINT_MAX times) . For me it's totally fine to create the
> entries on the fly (Maybe I should remove the count from the
> contructor).
Well, pre-allocating instead of allocating memory for each frame is
surely more efficient
>
> If count should get honored. How would you handle that? The function
> returns a reference, so it must return something. But I'm not allowed to
> throw inside libcamera. Or would it be fine to use entries_->at() which
> would then throw std::out_of_range.
The pattern we usually implement when having to return a reference in
case of error is something like
static TimeSheetEntry empty(idmap_);
if (pos > entries_.size())
return empty;
In this specific case passing in a pos larger than the expected frame count
is a programming error, not something triggered by an external
component, so I would simply assert() here.
>
> > > + 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
> > >
More information about the libcamera-devel
mailing list