[PATCH v2 02/12] libcamera: lc-compliance: Add TimeSheet class
Stefan Klug
stefan.klug at ideasonboard.com
Fri Mar 15 16:15:15 CET 2024
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 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 ?
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
> >
More information about the libcamera-devel
mailing list