| Message ID | 20240319120517.362082-3-stefan.klug@ideasonboard.com |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
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@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 >
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@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). 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. > > + 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 > >
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@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 > > >
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()); + } + + 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; +} + +} /* 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); + 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_; +};
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 | 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