[{"id":29023,"web_url":"https://patchwork.libcamera.org/comment/29023/","msgid":"<5x2uovooa2v3yrua3r2harjrp5ldswbn5xtwherc3xrt6janf2@n4vk7txou2xy>","date":"2024-03-21T09:42:43","subject":"Re: [PATCH v3 02/16] libcamera: lc-compliance: Add TimeSheet class","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Stefan\n\nOn Tue, Mar 19, 2024 at 01:05:03PM +0100, Stefan Klug wrote:\n> This class allows us to prepare a time sheet with controls for the frames\n> to capture (a bit like the capture script in cam).\n>\n> During capture the metadata is recorded. Additionally a mean brightness\n> value of a 20x20 pixel spot in the middle of the frame is calculated.\n>\n> This allows easy analysis after running the capture without complicated state\n> handling due to the asynchronous nature of the capturing process.\n>\n> Signed-off-by: Stefan Klug \n> ---\n> src/apps/lc-compliance/meson.build | 1 +\n> src/apps/lc-compliance/time_sheet.cpp | 148 ++++++++++++++++++++++++++\n> src/apps/lc-compliance/time_sheet.h | 62 +++++++++++\n> 3 files changed, 211 insertions(+)\n> create mode 100644 src/apps/lc-compliance/time_sheet.cpp\n> create mode 100644 src/apps/lc-compliance/time_sheet.h\n>\n> diff --git a/src/apps/lc-compliance/meson.build b/src/apps/lc-compliance/meson.build\n> index c792f072..eb7b2d71 100644\n> --- a/src/apps/lc-compliance/meson.build\n> +++ b/src/apps/lc-compliance/meson.build\n> @@ -16,6 +16,7 @@ lc_compliance_sources = files([\n> 'environment.cpp',\n> 'main.cpp',\n> 'simple_capture.cpp',\n> + 'time_sheet.cpp',\n> ])\n>\n> lc_compliance = executable('lc-compliance', lc_compliance_sources,\n> diff --git a/src/apps/lc-compliance/time_sheet.cpp b/src/apps/lc-compliance/time_sheet.cpp\n> new file mode 100644\n> index 00000000..8048cf30\n> --- /dev/null\n> +++ b/src/apps/lc-compliance/time_sheet.cpp\n> @@ -0,0 +1,148 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2024, Ideas on Board Oy\n> + *\n> + * time_sheet.cpp\n> + */\n> +\n> +#include \"time_sheet.h\"\n> +\n> +#include \n> +#include \n> +\n> +#include \"libcamera/internal/formats.h\"\n> +#include \"libcamera/internal/mapped_framebuffer.h\"\n> +\n> +using namespace libcamera;\n> +\n> +namespace {\n> +\n> +double calculateMeanBrightnessFromCenterSpot(libcamera::Request *request)\n> +{\n> +\tconst Request::BufferMap &buffers = request->buffers();\n> +\tfor (const auto &[stream, buffer] : buffers) {\n> +\t\tMappedFrameBuffer in(buffer, MappedFrameBuffer::MapFlag::Read);\n> +\t\tif (!in.isValid())\n> +\t\t\tcontinue;\n> +\n> +\t\tconst uint8_t *data = in.planes()[0].data();\n> +\t\tconst auto &streamConfig = stream->configuration();\n> +\t\tconst auto &formatInfo = PixelFormatInfo::info(streamConfig.pixelFormat);\n> +\t\tdouble (*calcPixelMean)(const uint8_t *);\n> +\t\tint pixelStride;\n> +\n> +\t\tswitch (streamConfig.pixelFormat) {\n> +\t\tcase formats::NV12:\n> +\t\t\tcalcPixelMean = [](const uint8_t *d) -> double {\n> +\t\t\t\treturn static_cast(*d);\n> +\t\t\t};\n> +\t\t\tpixelStride = 1;\n> +\t\t\tbreak;\n> +\t\tcase formats::SRGGB10:\n> +\t\t\tcalcPixelMean = [](const uint8_t *d) -> double {\n> +\t\t\t\treturn static_cast(*reinterpret_cast(d));\n> +\t\t\t};\n> +\t\t\tpixelStride = 2;\n> +\t\t\tbreak;\n> +\t\tdefault:\n> +\t\t\tstd::stringstream s;\n> +\t\t\ts << \"Unsupported Pixelformat \" << formatInfo.name;\n> +\t\t\tthrow std::invalid_argument(s.str());\n\nwe don't use exceptions (even if I see a throw in\nsrc/apps/lc-compliance/main.cpp ... )\n> +\t\t}\n> +\n> +\t\tdouble sum = 0;\n> +\t\tint w = 20;\n> +\t\tint xs = streamConfig.size.width / 2 - w / 2;\n> +\t\tint ys = streamConfig.size.height / 2 - w / 2;\n> +\n> +\t\tfor (auto y = ys; y < ys + w; y++) {\n> +\t\t\tauto line = data + y * streamConfig.stride;\n\nEmpy line after variable declaration is nice (and usually enforced in\nthe kernel coding style)\n> +\t\t\tfor (auto x = xs; x < xs + w; x++)\n> +\t\t\t\tsum += calcPixelMean(line + x * pixelStride);\n> +\t\t}\n\nnit: I would also empty line here to give the code some space to\nbreathe\n\n> +\t\tsum = sum / (w * w);\n> +\t\treturn sum;\n> +\t}\n> +\n> +\treturn 0;\n> +}\n> +\n> +} /* namespace */\n> +\n> +TimeSheetEntry::TimeSheetEntry(const ControlIdMap &idmap)\n> +\t: controls_(idmap)\n> +{\n> +}\n> +\n> +void TimeSheetEntry::handleCompleteRequest(libcamera::Request *request,\n> +\t\t\t\t\t const TimeSheetEntry *previous)\n> +{\n> +\tmetadata_ = request->metadata();\n> +\n> +\tspotBrightness_ = calculateMeanBrightnessFromCenterSpot(request);\n> +\tif (previous)\n> +\t\tbrightnessChange_ = spotBrightness_ / previous->spotBrightness();\n> +\n> +\tsequence_ = request->sequence();\n> +}\n> +\n> +std::ostream &operator<<(std::ostream &os, const TimeSheetEntry &te)\n> +{\n> +\tos << \"=== Frame \" << te.sequence_ << std::endl;\n> +\tif (!te.controls_.empty()) {\n> +\t\tos << \"Controls:\" << std::endl;\n> +\t\tconst auto &idMap = te.controls_.idMap();\n> +\t\tassert(idMap);\n> +\t\tfor (const auto &[id, value] : te.controls_)\n> +\t\t\tos << \" \" << idMap->at(id)->name() << \" : \"\n> +\t\t\t << value.toString() << std::endl;\n> +\t}\n> +\n> +\tif (!te.metadata_.empty()) {\n> +\t\tos << \"Metadata:\" << std::endl;\n> +\t\tconst auto &idMap = te.metadata_.idMap();\n> +\t\tassert(idMap);\n> +\t\tfor (const auto &[id, value] : te.metadata_)\n> +\t\t\tos << \" \" << idMap->at(id)->name() << \" : \"\n> +\t\t\t << value.toString() << std::endl;\n> +\t}\n> +\n> +\tos << \"Calculated Brightness: \" << te.spotBrightness() << std::endl;\n> +\treturn os;\n> +}\n> +\n> +TimeSheetEntry &TimeSheet::get(size_t pos)\n> +{\n> +\tentries_.reserve(pos + 1);\n\nNow, I've asked in v2 to check if pos < count as accessing a vector\nwith operator[] is unbounded. But my understanding was that pos\nshould never be >= count, and if this happen, it's an error. Do you\nexpect this to be a valid condition ? What if you get pos = UINT_MAX ?\n\n> +\tauto &entry = entries_[pos];\n> +\tif (!entry)\n> +\t\tentry = std::make_unique(idmap_);\n> +\n> +\treturn *entry;\n> +}\n> +\n> +void TimeSheet::prepareForQueue(libcamera::Request *request, uint32_t sequence)\n> +{\n> +\trequest->controls() = get(sequence).controls();\n> +}\n> +\n> +void TimeSheet::handleCompleteRequest(libcamera::Request *request)\n> +{\n> +\tuint32_t sequence = request->sequence();\n> +\tauto &entry = get(sequence);\n> +\tTimeSheetEntry *previous = nullptr;\n> +\tif (sequence >= 1)\n> +\t\tprevious = entries_[sequence - 1].get();\n> +\n> +\tentry.handleCompleteRequest(request, previous);\n> +}\n> +\n> +std::ostream &operator<<(std::ostream &os, const TimeSheet &ts)\n> +{\n> +\tfor (const auto &entry : ts.entries_) {\n> +\t\tif (entry)\n> +\t\t\tos << *entry;\n> +\t}\n> +\n> +\treturn os;\n> +}\n> diff --git a/src/apps/lc-compliance/time_sheet.h b/src/apps/lc-compliance/time_sheet.h\n> new file mode 100644\n> index 00000000..417277aa\n> --- /dev/null\n> +++ b/src/apps/lc-compliance/time_sheet.h\n> @@ -0,0 +1,62 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2024, Ideas on Board Oy\n> + *\n> + * time_sheet.h\n> + */\n> +\n> +#pragma once\n> +\n> +#include \n> +#include \n> +#include \n> +\n> +#include \n> +\n> +class TimeSheetEntry\n> +{\n> +public:\n> +\tTimeSheetEntry() = delete;\n> +\tTimeSheetEntry(const libcamera::ControlIdMap &idmap);\n> +\tTimeSheetEntry(TimeSheetEntry &&other) = default;\n> +\tTimeSheetEntry(const TimeSheetEntry &) = delete;\n> +\t~TimeSheetEntry() = default;\n> +\n> +\tlibcamera::ControlList &controls() { return controls_; }\n> +\tconst libcamera::ControlList &metadata() const { return metadata_; }\n> +\tvoid handleCompleteRequest(libcamera::Request *request,\n> +\t\t\t\t const TimeSheetEntry *previous);\n> +\tdouble spotBrightness() const { return spotBrightness_; }\n> +\tdouble brightnessChange() const { return brightnessChange_; }\n> +\n> +\tfriend std::ostream &operator<<(std::ostream &os, const TimeSheetEntry &te);\n> +\n> +private:\n> +\tdouble spotBrightness_ = 0.0;\n> +\tdouble brightnessChange_ = 0.0;\n> +\tlibcamera::ControlList controls_;\n> +\tlibcamera::ControlList metadata_;\n> +\tuint32_t sequence_ = 0;\n> +};\n> +\n> +class TimeSheet\n> +{\n> +public:\n> +\tTimeSheet(int count, const libcamera::ControlIdMap &idmap)\n> +\t\t: idmap_(idmap), entries_(count)\n> +\t{\n> +\t}\n> +\n> +\tvoid prepareForQueue(libcamera::Request *request, uint32_t sequence);\n> +\tvoid handleCompleteRequest(libcamera::Request *request);\n> +\n> +\tTimeSheetEntry &operator[](size_t pos) { return get(pos); }\n> +\tTimeSheetEntry &get(size_t pos);\n> +\tsize_t size() const { return entries_.size(); }\n> +\n> +\tfriend std::ostream &operator<<(std::ostream &os, const TimeSheet &ts);\n> +\n> +private:\n> +\tconst libcamera::ControlIdMap &idmap_;\n> +\tstd::vector> entries_;\n> +};\n> --\n> 2.40.1\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 756D2BD160\n\tfor ;\n\tThu, 21 Mar 2024 09:42:49 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 15F8B6305E;\n\tThu, 21 Mar 2024 10:42:49 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D562563036\n\tfor ;\n\tThu, 21 Mar 2024 10:42:46 +0100 (CET)","from ideasonboard.com (unknown\n\t[IPv6:2001:b07:5d2e:52c9:cc1e:e404:491f:e6ea])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id B3E5A7E9;\n\tThu, 21 Mar 2024 10:42:18 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"mbZMvkfn\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1711014138;\n\tbh=GvP4tlFhbsrm67KvWtUMEyzyLfZhW8/wfx0kp/l0DUY=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=mbZMvkfnLR580ZNgX6iTA4Yrbfq+ygPGb+e8ymWRTcQ2vndtq53lo/9WZEF/xovjb\n\tUEE94QI2FdWEC+/R6O+Ha0PXo/d8J5q99Ixfb3duPr0Cvfs9Y+KmWkENQjaiwBNbMz\n\tI32En/ak6Vo+U+YMTXOeUIRiDrax1DpkoJ4yqlD4=","Date":"Thu, 21 Mar 2024 10:42:43 +0100","From":"Jacopo Mondi ","To":"Stefan Klug ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v3 02/16] libcamera: lc-compliance: Add TimeSheet class","Message-ID":"<5x2uovooa2v3yrua3r2harjrp5ldswbn5xtwherc3xrt6janf2@n4vk7txou2xy>","References":"<20240319120517.362082-1-stefan.klug@ideasonboard.com>\n\t<20240319120517.362082-3-stefan.klug@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20240319120517.362082-3-stefan.klug@ideasonboard.com>","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":29025,"web_url":"https://patchwork.libcamera.org/comment/29025/","msgid":"<20240321110435.3xoaitlcrgmizpj5@jasper>","date":"2024-03-21T11:04:35","subject":"Re: [PATCH v3 02/16] libcamera: lc-compliance: Add TimeSheet class","submitter":{"id":184,"url":"https://patchwork.libcamera.org/api/people/184/","name":"Stefan Klug","email":"stefan.klug@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Thu, Mar 21, 2024 at 10:42:43AM +0100, Jacopo Mondi wrote:\n> Hi Stefan\n> \n> On Tue, Mar 19, 2024 at 01:05:03PM +0100, Stefan Klug wrote:\n> > This class allows us to prepare a time sheet with controls for the frames\n> > to capture (a bit like the capture script in cam).\n> >\n> > During capture the metadata is recorded. Additionally a mean brightness\n> > value of a 20x20 pixel spot in the middle of the frame is calculated.\n> >\n> > This allows easy analysis after running the capture without complicated state\n> > handling due to the asynchronous nature of the capturing process.\n> >\n> > Signed-off-by: Stefan Klug \n> > ---\n> > src/apps/lc-compliance/meson.build | 1 +\n> > src/apps/lc-compliance/time_sheet.cpp | 148 ++++++++++++++++++++++++++\n> > src/apps/lc-compliance/time_sheet.h | 62 +++++++++++\n> > 3 files changed, 211 insertions(+)\n> > create mode 100644 src/apps/lc-compliance/time_sheet.cpp\n> > create mode 100644 src/apps/lc-compliance/time_sheet.h\n> >\n> > diff --git a/src/apps/lc-compliance/meson.build b/src/apps/lc-compliance/meson.build\n> > index c792f072..eb7b2d71 100644\n> > --- a/src/apps/lc-compliance/meson.build\n> > +++ b/src/apps/lc-compliance/meson.build\n> > @@ -16,6 +16,7 @@ lc_compliance_sources = files([\n> > 'environment.cpp',\n> > 'main.cpp',\n> > 'simple_capture.cpp',\n> > + 'time_sheet.cpp',\n> > ])\n> >\n> > lc_compliance = executable('lc-compliance', lc_compliance_sources,\n> > diff --git a/src/apps/lc-compliance/time_sheet.cpp b/src/apps/lc-compliance/time_sheet.cpp\n> > new file mode 100644\n> > index 00000000..8048cf30\n> > --- /dev/null\n> > +++ b/src/apps/lc-compliance/time_sheet.cpp\n> > @@ -0,0 +1,148 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2024, Ideas on Board Oy\n> > + *\n> > + * time_sheet.cpp\n> > + */\n> > +\n> > +#include \"time_sheet.h\"\n> > +\n> > +#include \n> > +#include \n> > +\n> > +#include \"libcamera/internal/formats.h\"\n> > +#include \"libcamera/internal/mapped_framebuffer.h\"\n> > +\n> > +using namespace libcamera;\n> > +\n> > +namespace {\n> > +\n> > +double calculateMeanBrightnessFromCenterSpot(libcamera::Request *request)\n> > +{\n> > +\tconst Request::BufferMap &buffers = request->buffers();\n> > +\tfor (const auto &[stream, buffer] : buffers) {\n> > +\t\tMappedFrameBuffer in(buffer, MappedFrameBuffer::MapFlag::Read);\n> > +\t\tif (!in.isValid())\n> > +\t\t\tcontinue;\n> > +\n> > +\t\tconst uint8_t *data = in.planes()[0].data();\n> > +\t\tconst auto &streamConfig = stream->configuration();\n> > +\t\tconst auto &formatInfo = PixelFormatInfo::info(streamConfig.pixelFormat);\n> > +\t\tdouble (*calcPixelMean)(const uint8_t *);\n> > +\t\tint pixelStride;\n> > +\n> > +\t\tswitch (streamConfig.pixelFormat) {\n> > +\t\tcase formats::NV12:\n> > +\t\t\tcalcPixelMean = [](const uint8_t *d) -> double {\n> > +\t\t\t\treturn static_cast(*d);\n> > +\t\t\t};\n> > +\t\t\tpixelStride = 1;\n> > +\t\t\tbreak;\n> > +\t\tcase formats::SRGGB10:\n> > +\t\t\tcalcPixelMean = [](const uint8_t *d) -> double {\n> > +\t\t\t\treturn static_cast(*reinterpret_cast(d));\n> > +\t\t\t};\n> > +\t\t\tpixelStride = 2;\n> > +\t\t\tbreak;\n> > +\t\tdefault:\n> > +\t\t\tstd::stringstream s;\n> > +\t\t\ts << \"Unsupported Pixelformat \" << formatInfo.name;\n> > +\t\t\tthrow std::invalid_argument(s.str());\n> \n> we don't use exceptions (even if I see a throw in\n> src/apps/lc-compliance/main.cpp ... )\n\nOh, that got missed. Will be gone in v4.\n\n> > +\t\t}\n> > +\n> > +\t\tdouble sum = 0;\n> > +\t\tint w = 20;\n> > +\t\tint xs = streamConfig.size.width / 2 - w / 2;\n> > +\t\tint ys = streamConfig.size.height / 2 - w / 2;\n> > +\n> > +\t\tfor (auto y = ys; y < ys + w; y++) {\n> > +\t\t\tauto line = data + y * streamConfig.stride;\n> \n> Empy line after variable declaration is nice (and usually enforced in\n> the kernel coding style)\n> > +\t\t\tfor (auto x = xs; x < xs + w; x++)\n> > +\t\t\t\tsum += calcPixelMean(line + x * pixelStride);\n> > +\t\t}\n> \n> nit: I would also empty line here to give the code some space to\n> breathe\n> \n> > +\t\tsum = sum / (w * w);\n> > +\t\treturn sum;\n> > +\t}\n> > +\n> > +\treturn 0;\n> > +}\n> > +\n> > +} /* namespace */\n> > +\n> > +TimeSheetEntry::TimeSheetEntry(const ControlIdMap &idmap)\n> > +\t: controls_(idmap)\n> > +{\n> > +}\n> > +\n> > +void TimeSheetEntry::handleCompleteRequest(libcamera::Request *request,\n> > +\t\t\t\t\t const TimeSheetEntry *previous)\n> > +{\n> > +\tmetadata_ = request->metadata();\n> > +\n> > +\tspotBrightness_ = calculateMeanBrightnessFromCenterSpot(request);\n> > +\tif (previous)\n> > +\t\tbrightnessChange_ = spotBrightness_ / previous->spotBrightness();\n> > +\n> > +\tsequence_ = request->sequence();\n> > +}\n> > +\n> > +std::ostream &operator<<(std::ostream &os, const TimeSheetEntry &te)\n> > +{\n> > +\tos << \"=== Frame \" << te.sequence_ << std::endl;\n> > +\tif (!te.controls_.empty()) {\n> > +\t\tos << \"Controls:\" << std::endl;\n> > +\t\tconst auto &idMap = te.controls_.idMap();\n> > +\t\tassert(idMap);\n> > +\t\tfor (const auto &[id, value] : te.controls_)\n> > +\t\t\tos << \" \" << idMap->at(id)->name() << \" : \"\n> > +\t\t\t << value.toString() << std::endl;\n> > +\t}\n> > +\n> > +\tif (!te.metadata_.empty()) {\n> > +\t\tos << \"Metadata:\" << std::endl;\n> > +\t\tconst auto &idMap = te.metadata_.idMap();\n> > +\t\tassert(idMap);\n> > +\t\tfor (const auto &[id, value] : te.metadata_)\n> > +\t\t\tos << \" \" << idMap->at(id)->name() << \" : \"\n> > +\t\t\t << value.toString() << std::endl;\n> > +\t}\n> > +\n> > +\tos << \"Calculated Brightness: \" << te.spotBrightness() << std::endl;\n> > +\treturn os;\n> > +}\n> > +\n> > +TimeSheetEntry &TimeSheet::get(size_t pos)\n> > +{\n> > +\tentries_.reserve(pos + 1);\n> \n> Now, I've asked in v2 to check if pos < count as accessing a vector\n> with operator[] is unbounded. But my understanding was that pos\n> should never be >= count, and if this happen, it's an error. Do you\n> expect this to be a valid condition ? What if you get pos = UINT_MAX ?\n> \n\nWell, passing UINT_MAX won't be funny for the system (equal to calling\npush_back() UINT_MAX times) . For me it's totally fine to create the\nentries on the fly (Maybe I should remove the count from the\ncontructor).\n\nIf count should get honored. How would you handle that? The function\nreturns a reference, so it must return something. But I'm not allowed to\nthrow inside libcamera. Or would it be fine to use entries_->at() which\nwould then throw std::out_of_range.\n\n> > +\tauto &entry = entries_[pos];\n> > +\tif (!entry)\n> > +\t\tentry = std::make_unique(idmap_);\n> > +\n> > +\treturn *entry;\n> > +}\n> > +\n> > +void TimeSheet::prepareForQueue(libcamera::Request *request, uint32_t sequence)\n> > +{\n> > +\trequest->controls() = get(sequence).controls();\n> > +}\n> > +\n> > +void TimeSheet::handleCompleteRequest(libcamera::Request *request)\n> > +{\n> > +\tuint32_t sequence = request->sequence();\n> > +\tauto &entry = get(sequence);\n> > +\tTimeSheetEntry *previous = nullptr;\n> > +\tif (sequence >= 1)\n> > +\t\tprevious = entries_[sequence - 1].get();\n> > +\n> > +\tentry.handleCompleteRequest(request, previous);\n> > +}\n> > +\n> > +std::ostream &operator<<(std::ostream &os, const TimeSheet &ts)\n> > +{\n> > +\tfor (const auto &entry : ts.entries_) {\n> > +\t\tif (entry)\n> > +\t\t\tos << *entry;\n> > +\t}\n> > +\n> > +\treturn os;\n> > +}\n> > diff --git a/src/apps/lc-compliance/time_sheet.h b/src/apps/lc-compliance/time_sheet.h\n> > new file mode 100644\n> > index 00000000..417277aa\n> > --- /dev/null\n> > +++ b/src/apps/lc-compliance/time_sheet.h\n> > @@ -0,0 +1,62 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2024, Ideas on Board Oy\n> > + *\n> > + * time_sheet.h\n> > + */\n> > +\n> > +#pragma once\n> > +\n> > +#include \n> > +#include \n> > +#include \n> > +\n> > +#include \n> > +\n> > +class TimeSheetEntry\n> > +{\n> > +public:\n> > +\tTimeSheetEntry() = delete;\n> > +\tTimeSheetEntry(const libcamera::ControlIdMap &idmap);\n> > +\tTimeSheetEntry(TimeSheetEntry &&other) = default;\n> > +\tTimeSheetEntry(const TimeSheetEntry &) = delete;\n> > +\t~TimeSheetEntry() = default;\n> > +\n> > +\tlibcamera::ControlList &controls() { return controls_; }\n> > +\tconst libcamera::ControlList &metadata() const { return metadata_; }\n> > +\tvoid handleCompleteRequest(libcamera::Request *request,\n> > +\t\t\t\t const TimeSheetEntry *previous);\n> > +\tdouble spotBrightness() const { return spotBrightness_; }\n> > +\tdouble brightnessChange() const { return brightnessChange_; }\n> > +\n> > +\tfriend std::ostream &operator<<(std::ostream &os, const TimeSheetEntry &te);\n> > +\n> > +private:\n> > +\tdouble spotBrightness_ = 0.0;\n> > +\tdouble brightnessChange_ = 0.0;\n> > +\tlibcamera::ControlList controls_;\n> > +\tlibcamera::ControlList metadata_;\n> > +\tuint32_t sequence_ = 0;\n> > +};\n> > +\n> > +class TimeSheet\n> > +{\n> > +public:\n> > +\tTimeSheet(int count, const libcamera::ControlIdMap &idmap)\n> > +\t\t: idmap_(idmap), entries_(count)\n> > +\t{\n> > +\t}\n> > +\n> > +\tvoid prepareForQueue(libcamera::Request *request, uint32_t sequence);\n> > +\tvoid handleCompleteRequest(libcamera::Request *request);\n> > +\n> > +\tTimeSheetEntry &operator[](size_t pos) { return get(pos); }\n> > +\tTimeSheetEntry &get(size_t pos);\n> > +\tsize_t size() const { return entries_.size(); }\n> > +\n> > +\tfriend std::ostream &operator<<(std::ostream &os, const TimeSheet &ts);\n> > +\n> > +private:\n> > +\tconst libcamera::ControlIdMap &idmap_;\n> > +\tstd::vector> entries_;\n> > +};\n> > --\n> > 2.40.1\n> >","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id B5E1FBD160\n\tfor ;\n\tThu, 21 Mar 2024 11:04:41 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9E34C63055;\n\tThu, 21 Mar 2024 12:04:40 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E6CFF62827\n\tfor ;\n\tThu, 21 Mar 2024 12:04:38 +0100 (CET)","from ideasonboard.com (unknown\n\t[IPv6:2a00:6020:448c:6c00:1bc6:8c62:a44e:3d82])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 9D9857E9;\n\tThu, 21 Mar 2024 12:04:10 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"G6ohN6k6\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1711019050;\n\tbh=9GAO9iq7VPes3Iiv8UMRiSFDCWKvaqGIXT2piOaKmqo=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=G6ohN6k6MkuO1fgbK7bejJiDtVUoVF9CnQMfTzTv6KgGN/+m5rXKx0jbC2DLb27bQ\n\txFZSBCyvDPxDTzhOXR6dfitxsW7ffz7FgAInDY3+mTrHhCYOHBH/7V2QgWEwL/nxrS\n\tP4xs7J2s8YBdyQCqj0CLKAynKIiSH5S5piszxEkk=","Date":"Thu, 21 Mar 2024 12:04:35 +0100","From":"Stefan Klug ","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v3 02/16] libcamera: lc-compliance: Add TimeSheet class","Message-ID":"<20240321110435.3xoaitlcrgmizpj5@jasper>","References":"<20240319120517.362082-1-stefan.klug@ideasonboard.com>\n\t<20240319120517.362082-3-stefan.klug@ideasonboard.com>\n\t<5x2uovooa2v3yrua3r2harjrp5ldswbn5xtwherc3xrt6janf2@n4vk7txou2xy>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<5x2uovooa2v3yrua3r2harjrp5ldswbn5xtwherc3xrt6janf2@n4vk7txou2xy>","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":29035,"web_url":"https://patchwork.libcamera.org/comment/29035/","msgid":"","date":"2024-03-22T09:19:03","subject":"Re: [PATCH v3 02/16] libcamera: lc-compliance: Add TimeSheet class","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Stefan\n\nOn Thu, Mar 21, 2024 at 12:04:35PM +0100, Stefan Klug wrote:\n>\n> Hi Jacopo,\n>\n> On Thu, Mar 21, 2024 at 10:42:43AM +0100, Jacopo Mondi wrote:\n> > Hi Stefan\n> >\n> > On Tue, Mar 19, 2024 at 01:05:03PM +0100, Stefan Klug wrote:\n> > > This class allows us to prepare a time sheet with controls for the frames\n> > > to capture (a bit like the capture script in cam).\n> > >\n> > > During capture the metadata is recorded. Additionally a mean brightness\n> > > value of a 20x20 pixel spot in the middle of the frame is calculated.\n> > >\n> > > This allows easy analysis after running the capture without complicated state\n> > > handling due to the asynchronous nature of the capturing process.\n> > >\n> > > Signed-off-by: Stefan Klug \n> > > ---\n> > > src/apps/lc-compliance/meson.build | 1 +\n> > > src/apps/lc-compliance/time_sheet.cpp | 148 ++++++++++++++++++++++++++\n> > > src/apps/lc-compliance/time_sheet.h | 62 +++++++++++\n> > > 3 files changed, 211 insertions(+)\n> > > create mode 100644 src/apps/lc-compliance/time_sheet.cpp\n> > > create mode 100644 src/apps/lc-compliance/time_sheet.h\n> > >\n> > > diff --git a/src/apps/lc-compliance/meson.build b/src/apps/lc-compliance/meson.build\n> > > index c792f072..eb7b2d71 100644\n> > > --- a/src/apps/lc-compliance/meson.build\n> > > +++ b/src/apps/lc-compliance/meson.build\n> > > @@ -16,6 +16,7 @@ lc_compliance_sources = files([\n> > > 'environment.cpp',\n> > > 'main.cpp',\n> > > 'simple_capture.cpp',\n> > > + 'time_sheet.cpp',\n> > > ])\n> > >\n> > > lc_compliance = executable('lc-compliance', lc_compliance_sources,\n> > > diff --git a/src/apps/lc-compliance/time_sheet.cpp b/src/apps/lc-compliance/time_sheet.cpp\n> > > new file mode 100644\n> > > index 00000000..8048cf30\n> > > --- /dev/null\n> > > +++ b/src/apps/lc-compliance/time_sheet.cpp\n> > > @@ -0,0 +1,148 @@\n> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > +/*\n> > > + * Copyright (C) 2024, Ideas on Board Oy\n> > > + *\n> > > + * time_sheet.cpp\n> > > + */\n> > > +\n> > > +#include \"time_sheet.h\"\n> > > +\n> > > +#include \n> > > +#include \n> > > +\n> > > +#include \"libcamera/internal/formats.h\"\n> > > +#include \"libcamera/internal/mapped_framebuffer.h\"\n> > > +\n> > > +using namespace libcamera;\n> > > +\n> > > +namespace {\n> > > +\n> > > +double calculateMeanBrightnessFromCenterSpot(libcamera::Request *request)\n> > > +{\n> > > +\tconst Request::BufferMap &buffers = request->buffers();\n> > > +\tfor (const auto &[stream, buffer] : buffers) {\n> > > +\t\tMappedFrameBuffer in(buffer, MappedFrameBuffer::MapFlag::Read);\n> > > +\t\tif (!in.isValid())\n> > > +\t\t\tcontinue;\n> > > +\n> > > +\t\tconst uint8_t *data = in.planes()[0].data();\n> > > +\t\tconst auto &streamConfig = stream->configuration();\n> > > +\t\tconst auto &formatInfo = PixelFormatInfo::info(streamConfig.pixelFormat);\n> > > +\t\tdouble (*calcPixelMean)(const uint8_t *);\n> > > +\t\tint pixelStride;\n> > > +\n> > > +\t\tswitch (streamConfig.pixelFormat) {\n> > > +\t\tcase formats::NV12:\n> > > +\t\t\tcalcPixelMean = [](const uint8_t *d) -> double {\n> > > +\t\t\t\treturn static_cast(*d);\n> > > +\t\t\t};\n> > > +\t\t\tpixelStride = 1;\n> > > +\t\t\tbreak;\n> > > +\t\tcase formats::SRGGB10:\n> > > +\t\t\tcalcPixelMean = [](const uint8_t *d) -> double {\n> > > +\t\t\t\treturn static_cast(*reinterpret_cast(d));\n> > > +\t\t\t};\n> > > +\t\t\tpixelStride = 2;\n> > > +\t\t\tbreak;\n> > > +\t\tdefault:\n> > > +\t\t\tstd::stringstream s;\n> > > +\t\t\ts << \"Unsupported Pixelformat \" << formatInfo.name;\n> > > +\t\t\tthrow std::invalid_argument(s.str());\n> >\n> > we don't use exceptions (even if I see a throw in\n> > src/apps/lc-compliance/main.cpp ... )\n>\n> Oh, that got missed. Will be gone in v4.\n>\n> > > +\t\t}\n> > > +\n> > > +\t\tdouble sum = 0;\n> > > +\t\tint w = 20;\n> > > +\t\tint xs = streamConfig.size.width / 2 - w / 2;\n> > > +\t\tint ys = streamConfig.size.height / 2 - w / 2;\n> > > +\n> > > +\t\tfor (auto y = ys; y < ys + w; y++) {\n> > > +\t\t\tauto line = data + y * streamConfig.stride;\n> >\n> > Empy line after variable declaration is nice (and usually enforced in\n> > the kernel coding style)\n> > > +\t\t\tfor (auto x = xs; x < xs + w; x++)\n> > > +\t\t\t\tsum += calcPixelMean(line + x * pixelStride);\n> > > +\t\t}\n> >\n> > nit: I would also empty line here to give the code some space to\n> > breathe\n> >\n> > > +\t\tsum = sum / (w * w);\n> > > +\t\treturn sum;\n> > > +\t}\n> > > +\n> > > +\treturn 0;\n> > > +}\n> > > +\n> > > +} /* namespace */\n> > > +\n> > > +TimeSheetEntry::TimeSheetEntry(const ControlIdMap &idmap)\n> > > +\t: controls_(idmap)\n> > > +{\n> > > +}\n> > > +\n> > > +void TimeSheetEntry::handleCompleteRequest(libcamera::Request *request,\n> > > +\t\t\t\t\t const TimeSheetEntry *previous)\n> > > +{\n> > > +\tmetadata_ = request->metadata();\n> > > +\n> > > +\tspotBrightness_ = calculateMeanBrightnessFromCenterSpot(request);\n> > > +\tif (previous)\n> > > +\t\tbrightnessChange_ = spotBrightness_ / previous->spotBrightness();\n> > > +\n> > > +\tsequence_ = request->sequence();\n> > > +}\n> > > +\n> > > +std::ostream &operator<<(std::ostream &os, const TimeSheetEntry &te)\n> > > +{\n> > > +\tos << \"=== Frame \" << te.sequence_ << std::endl;\n> > > +\tif (!te.controls_.empty()) {\n> > > +\t\tos << \"Controls:\" << std::endl;\n> > > +\t\tconst auto &idMap = te.controls_.idMap();\n> > > +\t\tassert(idMap);\n> > > +\t\tfor (const auto &[id, value] : te.controls_)\n> > > +\t\t\tos << \" \" << idMap->at(id)->name() << \" : \"\n> > > +\t\t\t << value.toString() << std::endl;\n> > > +\t}\n> > > +\n> > > +\tif (!te.metadata_.empty()) {\n> > > +\t\tos << \"Metadata:\" << std::endl;\n> > > +\t\tconst auto &idMap = te.metadata_.idMap();\n> > > +\t\tassert(idMap);\n> > > +\t\tfor (const auto &[id, value] : te.metadata_)\n> > > +\t\t\tos << \" \" << idMap->at(id)->name() << \" : \"\n> > > +\t\t\t << value.toString() << std::endl;\n> > > +\t}\n> > > +\n> > > +\tos << \"Calculated Brightness: \" << te.spotBrightness() << std::endl;\n> > > +\treturn os;\n> > > +}\n> > > +\n> > > +TimeSheetEntry &TimeSheet::get(size_t pos)\n> > > +{\n> > > +\tentries_.reserve(pos + 1);\n> >\n> > Now, I've asked in v2 to check if pos < count as accessing a vector\n> > with operator[] is unbounded. But my understanding was that pos\n> > should never be >= count, and if this happen, it's an error. Do you\n> > expect this to be a valid condition ? What if you get pos = UINT_MAX ?\n> >\n>\n> Well, passing UINT_MAX won't be funny for the system (equal to calling\n> push_back() UINT_MAX times) . For me it's totally fine to create the\n> entries on the fly (Maybe I should remove the count from the\n> contructor).\n\nWell, pre-allocating instead of allocating memory for each frame is\nsurely more efficient\n\n>\n> If count should get honored. How would you handle that? The function\n> returns a reference, so it must return something. But I'm not allowed to\n> throw inside libcamera. Or would it be fine to use entries_->at() which\n> would then throw std::out_of_range.\n\nThe pattern we usually implement when having to return a reference in\ncase of error is something like\n\n\tstatic TimeSheetEntry empty(idmap_);\n\n\tif (pos > entries_.size())\n\t\treturn empty;\n\nIn this specific case passing in a pos larger than the expected frame count\nis a programming error, not something triggered by an external\ncomponent, so I would simply assert() here.\n\n>\n> > > +\tauto &entry = entries_[pos];\n> > > +\tif (!entry)\n> > > +\t\tentry = std::make_unique(idmap_);\n> > > +\n> > > +\treturn *entry;\n> > > +}\n> > > +\n> > > +void TimeSheet::prepareForQueue(libcamera::Request *request, uint32_t sequence)\n> > > +{\n> > > +\trequest->controls() = get(sequence).controls();\n> > > +}\n> > > +\n> > > +void TimeSheet::handleCompleteRequest(libcamera::Request *request)\n> > > +{\n> > > +\tuint32_t sequence = request->sequence();\n> > > +\tauto &entry = get(sequence);\n> > > +\tTimeSheetEntry *previous = nullptr;\n> > > +\tif (sequence >= 1)\n> > > +\t\tprevious = entries_[sequence - 1].get();\n> > > +\n> > > +\tentry.handleCompleteRequest(request, previous);\n> > > +}\n> > > +\n> > > +std::ostream &operator<<(std::ostream &os, const TimeSheet &ts)\n> > > +{\n> > > +\tfor (const auto &entry : ts.entries_) {\n> > > +\t\tif (entry)\n> > > +\t\t\tos << *entry;\n> > > +\t}\n> > > +\n> > > +\treturn os;\n> > > +}\n> > > diff --git a/src/apps/lc-compliance/time_sheet.h b/src/apps/lc-compliance/time_sheet.h\n> > > new file mode 100644\n> > > index 00000000..417277aa\n> > > --- /dev/null\n> > > +++ b/src/apps/lc-compliance/time_sheet.h\n> > > @@ -0,0 +1,62 @@\n> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > +/*\n> > > + * Copyright (C) 2024, Ideas on Board Oy\n> > > + *\n> > > + * time_sheet.h\n> > > + */\n> > > +\n> > > +#pragma once\n> > > +\n> > > +#include \n> > > +#include \n> > > +#include \n> > > +\n> > > +#include \n> > > +\n> > > +class TimeSheetEntry\n> > > +{\n> > > +public:\n> > > +\tTimeSheetEntry() = delete;\n> > > +\tTimeSheetEntry(const libcamera::ControlIdMap &idmap);\n> > > +\tTimeSheetEntry(TimeSheetEntry &&other) = default;\n> > > +\tTimeSheetEntry(const TimeSheetEntry &) = delete;\n> > > +\t~TimeSheetEntry() = default;\n> > > +\n> > > +\tlibcamera::ControlList &controls() { return controls_; }\n> > > +\tconst libcamera::ControlList &metadata() const { return metadata_; }\n> > > +\tvoid handleCompleteRequest(libcamera::Request *request,\n> > > +\t\t\t\t const TimeSheetEntry *previous);\n> > > +\tdouble spotBrightness() const { return spotBrightness_; }\n> > > +\tdouble brightnessChange() const { return brightnessChange_; }\n> > > +\n> > > +\tfriend std::ostream &operator<<(std::ostream &os, const TimeSheetEntry &te);\n> > > +\n> > > +private:\n> > > +\tdouble spotBrightness_ = 0.0;\n> > > +\tdouble brightnessChange_ = 0.0;\n> > > +\tlibcamera::ControlList controls_;\n> > > +\tlibcamera::ControlList metadata_;\n> > > +\tuint32_t sequence_ = 0;\n> > > +};\n> > > +\n> > > +class TimeSheet\n> > > +{\n> > > +public:\n> > > +\tTimeSheet(int count, const libcamera::ControlIdMap &idmap)\n> > > +\t\t: idmap_(idmap), entries_(count)\n> > > +\t{\n> > > +\t}\n> > > +\n> > > +\tvoid prepareForQueue(libcamera::Request *request, uint32_t sequence);\n> > > +\tvoid handleCompleteRequest(libcamera::Request *request);\n> > > +\n> > > +\tTimeSheetEntry &operator[](size_t pos) { return get(pos); }\n> > > +\tTimeSheetEntry &get(size_t pos);\n> > > +\tsize_t size() const { return entries_.size(); }\n> > > +\n> > > +\tfriend std::ostream &operator<<(std::ostream &os, const TimeSheet &ts);\n> > > +\n> > > +private:\n> > > +\tconst libcamera::ControlIdMap &idmap_;\n> > > +\tstd::vector> entries_;\n> > > +};\n> > > --\n> > > 2.40.1\n> > >","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id C8223C3272\n\tfor ;\n\tFri, 22 Mar 2024 09:19:09 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B0DCF63055;\n\tFri, 22 Mar 2024 10:19:08 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 2388862CA3\n\tfor ;\n\tFri, 22 Mar 2024 10:19:07 +0100 (CET)","from ideasonboard.com (unknown\n\t[IPv6:2001:b07:5d2e:52c9:cc1e:e404:491f:e6ea])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 40F9582A;\n\tFri, 22 Mar 2024 10:18:38 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"GBH/FmoI\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1711099118;\n\tbh=GNBnQVWOBB5sAeoITSjbuh4qJUNQDHnxz0jisXHF/eE=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=GBH/FmoIjzIiAYjhsSKZcvIddSC1HHvYGzD/Z+eoDuTDK6xujFxm1t4onddfdzvzv\n\tFI99pRNtOOHmYBfDR6MwRVuGevPB5BsCFD0py9lonojeactQFQMs3uNtS0/2wKzRh2\n\tkJq/kD3kT04d+r6S4/zDiKgqWGHm5O0Hl8w1zaXc=","Date":"Fri, 22 Mar 2024 10:19:03 +0100","From":"Jacopo Mondi ","To":"Stefan Klug ","Cc":"Jacopo Mondi , \n\tlibcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v3 02/16] libcamera: lc-compliance: Add TimeSheet class","Message-ID":"","References":"<20240319120517.362082-1-stefan.klug@ideasonboard.com>\n\t<20240319120517.362082-3-stefan.klug@ideasonboard.com>\n\t<5x2uovooa2v3yrua3r2harjrp5ldswbn5xtwherc3xrt6janf2@n4vk7txou2xy>\n\t<20240321110435.3xoaitlcrgmizpj5@jasper>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20240321110435.3xoaitlcrgmizpj5@jasper>","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]