[{"id":28965,"web_url":"https://patchwork.libcamera.org/comment/28965/","msgid":"<7oOHNk5JnI2Zvo68fm9yNnm6z5m0J89FU8CiFiwKjaJROBryysT7VzXdl4bczb1wo3aASPuflHa62fKBcDK_xchFJIdB7nvvbr3As7TA56Y=@protonmail.com>","date":"2024-03-14T19:07:32","subject":"Re: [PATCH v2 02/12] libcamera: lc-compliance: Add TimeSheet class","submitter":{"id":133,"url":"https://patchwork.libcamera.org/api/people/133/","name":"Pőcze Barnabás","email":"pobrn@protonmail.com"},"content":"Hi\n\n\n2024. március 13., szerda 13:12 keltezéssel, Stefan Klug <stefan.klug@ideasonboard.com> írta:\n\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 <stefan.klug@ideasonboard.com>\n> ---\n>  src/apps/lc-compliance/meson.build    |   1 +\n>  src/apps/lc-compliance/time_sheet.cpp | 145 ++++++++++++++++++++++++++\n>  src/apps/lc-compliance/time_sheet.h   |  55 ++++++++++\n>  3 files changed, 201 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..0ac544d6\n> --- /dev/null\n> +++ b/src/apps/lc-compliance/time_sheet.cpp\n> @@ -0,0 +1,145 @@\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 <sstream>\n> +#include <libcamera/libcamera.h>\n> +\n> +#include \"libcamera/internal/formats.h\"\n> +#include \"libcamera/internal/mapped_framebuffer.h\"\n> +\n> +using namespace libcamera;\n> +\n> +double calcPixelMeanNV12(const uint8_t *data)\n> +{\n> +\treturn (double)*data;\n> +}\n> +\n> +double calcPixelMeanRAW10(const uint8_t *data)\n> +{\n> +\treturn (double)*((const uint16_t *)data);\n> +}\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\tauto data = in.planes()[0].data();\n> +\t\t\tauto streamConfig = stream->configuration();\n> +\t\t\tauto formatInfo = PixelFormatInfo::info(streamConfig.pixelFormat);\n\nUse `const auto &` or `auto &&` with `streamConfig` and `formatInfo` to avoid copies.\n\n\n> +\n> +\t\t\tstd::function<double(const uint8_t *data)> calcPixelMean;\n\nJust use `double (*calcPixelMean)(const uint8_t *)`, std::function is an overkill here.\n\n\n> +\t\t\tint pixelStride;\n> +\n> +\t\t\tswitch (streamConfig.pixelFormat) {\n> +\t\t\tcase formats::NV12:\n> +\t\t\t\tcalcPixelMean = calcPixelMeanNV12;\n\nYou could alternatively just say\n\n  calcPixelMean = [](const uint8_t *data) -> double { return *data; };\n\nand similarly below. Then you wouldn't need the named functions.\n\n\n> +\t\t\t\tpixelStride = 1;\n> +\t\t\t\tbreak;\n> +\t\t\tcase formats::SRGGB10:\n> +\t\t\t\tcalcPixelMean = calcPixelMeanRAW10;\n> +\t\t\t\tpixelStride = 2;\n> +\t\t\t\tbreak;\n> +\t\t\tdefault:\n> +\t\t\t\tstd::stringstream s;\n> +\t\t\t\ts << \"Unsupported Pixelformat \" << formatInfo.name;\n> +\t\t\t\tthrow std::invalid_argument(s.str());\n> +\t\t\t}\n> +\n> +\t\t\tdouble sum = 0;\n> +\t\t\tint w = 20;\n> +\t\t\tint xs = streamConfig.size.width / 2 - w / 2;\n> +\t\t\tint ys = streamConfig.size.height / 2 - w / 2;\n> +\n> +\t\t\tfor (auto y = ys; y < ys + w; y++) {\n> +\t\t\t\tauto line = data + y * streamConfig.stride;\n> +\t\t\t\tfor (auto x = xs; x < xs + w; x++) {\n> +\t\t\t\t\tsum += calcPixelMean(line + x * pixelStride);\n> +\t\t\t\t}\n> +\t\t\t}\n> +\t\t\tsum = sum / (w * w);\n> +\t\t\treturn sum;\n> +\t\t}\n> +\t}\n> +\treturn 0;\n> +}\n\nThe above functions should be in an anonymous namespace if they are not\nused outside this translation unit.\n\n\n> +\n> +TimeSheetEntry::TimeSheetEntry(const ControlIdMap &idmap)\n> +\t: controls_(idmap)\n> +{\n> +}\n> +\n> +void TimeSheetEntry::handleCompleteRequest(libcamera::Request *request, const TimeSheetEntry *previous)\n> +{\n> +\tmetadata_ = request->metadata();\n> +\n> +\tspotBrightness_ = calculateMeanBrightnessFromCenterSpot(request);\n> +\tif (previous) {\n> +\t\tbrightnessChange_ = spotBrightness_ / previous->getSpotBrightness();\n> +\t}\n> +\tsequence_ = request->sequence();\n> +}\n> +\n> +void TimeSheetEntry::printInfo()\n> +{\n> +\tstd::cout << \"=== Frame \" << sequence_ << std::endl;\n> +\tif (!controls_.empty()) {\n> +\t\tstd::cout << \"Controls:\" << std::endl;\n> +\t\tauto idMap = controls_.idMap();\n> +\t\tassert(idMap);\n> +\t\tfor (const auto &[id, value] : controls_) {\n> +\t\t\tstd::cout << \"  \" << idMap->at(id)->name() << \" : \" << value.toString() << std::endl;\n> +\t\t}\n> +\t}\n> +\n> +\tif (!metadata_.empty()) {\n> +\t\tstd::cout << \"Metadata:\" << std::endl;\n> +\t\tauto idMap = metadata_.idMap();\n> +\t\tassert(idMap);\n> +\t\tfor (const auto &[id, value] : metadata_) {\n> +\t\t\tstd::cout << \"  \" << idMap->at(id)->name() << \" : \" << value.toString() << std::endl;\n> +\t\t}\n> +\t}\n> +\n> +\tstd::cout << \"Calculated Brightness: \" << spotBrightness_ << std::endl;\n> +}\n> +\n> +TimeSheetEntry &TimeSheet::get(size_t pos)\n> +{\n> +\tauto &entry = entries_[pos];\n> +\tif (!entry)\n> +\t\tentry = std::make_shared<TimeSheetEntry>(idmap_);\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> +\t}\n> +\n> +\tentry.handleCompleteRequest(request, previous);\n> +}\n> +\n> +void TimeSheet::printAllInfos()\n> +{\n> +\tfor (auto entry : entries_) {\n\n`const auto &entry` to avoid the copy.\n\n\n> +\t\tif (entry)\n> +\t\t\tentry->printInfo();\n> +\t}\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..2bb89ab3\n> --- /dev/null\n> +++ b/src/apps/lc-compliance/time_sheet.h\n> @@ -0,0 +1,55 @@\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 <future>\n> +#include <vector>\n> +\n> +#include <libcamera/libcamera.h>\n> +\n> +class TimeSheetEntry\n> +{\n> +public:\n> +\tTimeSheetEntry() = delete;\n> +\tTimeSheetEntry(const libcamera::ControlIdMap &idmap);\n> +\tTimeSheetEntry(TimeSheetEntry &&other) noexcept = default;\n> +\tTimeSheetEntry(const TimeSheetEntry &) = delete;\n> +\n> +\tlibcamera::ControlList &controls() { return controls_; };\n> +\tlibcamera::ControlList &metadata() { return metadata_; };\n> +\tvoid handleCompleteRequest(libcamera::Request *request, const TimeSheetEntry *previous);\n> +\tvoid printInfo();\n> +\tdouble getSpotBrightness() const { return spotBrightness_; };\n> +\tdouble getBrightnessChange() const { return brightnessChange_; };\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\nNo `;` is needed at the end. I'd probably put the `{ }` in a new line\nto make it clear that that is the function body.\n\n> +\n> +\tvoid prepareForQueue(libcamera::Request *request, uint32_t sequence);\n> +\tvoid handleCompleteRequest(libcamera::Request *request);\n> +\tvoid printAllInfos();\n> +\n> +\tTimeSheetEntry &operator[](size_t pos) { return get(pos); };\n> +\tTimeSheetEntry &get(size_t pos);\n\nWhy are get() and operator[] needed when they both do the same thing?\n\n\n> +\tsize_t size() const { return entries_.size(); };\n> +\n> +private:\n> +\tconst libcamera::ControlIdMap &idmap_;\n> +\tstd::vector<std::shared_ptr<TimeSheetEntry>> entries_;\n\nDoes this need to be shared_ptr? At first glance it seems to me that\nunique_ptr would be enough. \n\n\n> +};\n> --\n> 2.40.1\n\n\nRegards,\nBarnabás Pőcze","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 A9C1CBD160\n\tfor <parsemail@patchwork.libcamera.org>;\n\tThu, 14 Mar 2024 19:07:42 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id CE58F62973;\n\tThu, 14 Mar 2024 20:07:41 +0100 (CET)","from mail-40134.protonmail.ch (mail-40134.protonmail.ch\n\t[185.70.40.134])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 2932D6286F\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tThu, 14 Mar 2024 20:07:40 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key;\n\tunprotected) header.d=protonmail.com header.i=@protonmail.com\n\theader.b=\"hWuSeing\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=protonmail.com;\n\ts=protonmail3; t=1710443258; x=1710702458;\n\tbh=LlY+z+d3YC+Q01sH8j0JmIxdCNuajAebYizFDvAcfmM=;\n\th=Date:To:From:Cc:Subject:Message-ID:In-Reply-To:References:\n\tFeedback-ID:From:To:Cc:Date:Subject:Reply-To:Feedback-ID:\n\tMessage-ID:BIMI-Selector;\n\tb=hWuSeingw5Kyf7mBo2VfJZz3J+Qdrolb/TuUk5HGvd+CPMKj1tzMbMgXNt6MmruYO\n\teQIlk6BseOrkyvXLqAFIx0uhquWzSMlQ6F6GzqPXUlGVOWIkwSDdaRX6b7gPb7SGYB\n\tzRq0r9ufy73rfR8a5CcdsR6xQgZ6Y6hTajhYmGt0PFGviyk0JnYTjayvY9LGYH6aPI\n\t6wzK8YbVa81sk7ssueFHngLY3cBjbzPtSqKvHWtQQEI7sTTmJxogtAkHlt5SyTGq9j\n\t0HlVHtVwwZ5Yr6NlTPrnzf0xt70lo2SdPq1DBKmAwzH89COZSKU6QyPXQvrsyTMaF4\n\twSclxCTvoByxA==","Date":"Thu, 14 Mar 2024 19:07:32 +0000","To":"Stefan Klug <stefan.klug@ideasonboard.com>","From":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= <pobrn@protonmail.com>","Subject":"Re: [PATCH v2 02/12] libcamera: lc-compliance: Add TimeSheet class","Message-ID":"<7oOHNk5JnI2Zvo68fm9yNnm6z5m0J89FU8CiFiwKjaJROBryysT7VzXdl4bczb1wo3aASPuflHa62fKBcDK_xchFJIdB7nvvbr3As7TA56Y=@protonmail.com>","In-Reply-To":"<20240313121223.138150-3-stefan.klug@ideasonboard.com>","References":"<20240313121223.138150-1-stefan.klug@ideasonboard.com>\n\t<20240313121223.138150-3-stefan.klug@ideasonboard.com>","Feedback-ID":"20568564:user:proton","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Transfer-Encoding":"quoted-printable","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":28978,"web_url":"https://patchwork.libcamera.org/comment/28978/","msgid":"<20240315104644.742gboqlczqdkvie@jasper>","date":"2024-03-15T10:46:44","subject":"Re: [PATCH v2 02/12] 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 Barnabás,\n\nthank you for the review.\n\nOn Thu, Mar 14, 2024 at 07:07:32PM +0000, Barnabás Pőcze wrote:\n> Hi\n> \n> \n> 2024. március 13., szerda 13:12 keltezéssel, Stefan Klug <stefan.klug@ideasonboard.com> írta:\n> \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 <stefan.klug@ideasonboard.com>\n> > ---\n> >  src/apps/lc-compliance/meson.build    |   1 +\n> >  src/apps/lc-compliance/time_sheet.cpp | 145 ++++++++++++++++++++++++++\n> >  src/apps/lc-compliance/time_sheet.h   |  55 ++++++++++\n> >  3 files changed, 201 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..0ac544d6\n> > --- /dev/null\n> > +++ b/src/apps/lc-compliance/time_sheet.cpp\n> > @@ -0,0 +1,145 @@\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 <sstream>\n> > +#include <libcamera/libcamera.h>\n> > +\n> > +#include \"libcamera/internal/formats.h\"\n> > +#include \"libcamera/internal/mapped_framebuffer.h\"\n> > +\n> > +using namespace libcamera;\n> > +\n> > +double calcPixelMeanNV12(const uint8_t *data)\n> > +{\n> > +\treturn (double)*data;\n> > +}\n> > +\n> > +double calcPixelMeanRAW10(const uint8_t *data)\n> > +{\n> > +\treturn (double)*((const uint16_t *)data);\n> > +}\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\tauto data = in.planes()[0].data();\n> > +\t\t\tauto streamConfig = stream->configuration();\n> > +\t\t\tauto formatInfo = PixelFormatInfo::info(streamConfig.pixelFormat);\n> \n> Use `const auto &` or `auto &&` with `streamConfig` and `formatInfo` to avoid copies.\n> \n\nYes, right. I'll fix that.\n\n> \n> > +\n> > +\t\t\tstd::function<double(const uint8_t *data)> calcPixelMean;\n> \n> Just use `double (*calcPixelMean)(const uint8_t *)`, std::function is an overkill here.\n> \n> \n> > +\t\t\tint pixelStride;\n> > +\n> > +\t\t\tswitch (streamConfig.pixelFormat) {\n> > +\t\t\tcase formats::NV12:\n> > +\t\t\t\tcalcPixelMean = calcPixelMeanNV12;\n> \n> You could alternatively just say\n> \n>   calcPixelMean = [](const uint8_t *data) -> double { return *data; };\n> \n> and similarly below. Then you wouldn't need the named functions.\n>\n\nOh, you are right. I tried that with a lambda capturing data, which is\nnot allowed. But with a non capturing lambda, it's fine.\n \n> \n> > +\t\t\t\tpixelStride = 1;\n> > +\t\t\t\tbreak;\n> > +\t\t\tcase formats::SRGGB10:\n> > +\t\t\t\tcalcPixelMean = calcPixelMeanRAW10;\n> > +\t\t\t\tpixelStride = 2;\n> > +\t\t\t\tbreak;\n> > +\t\t\tdefault:\n> > +\t\t\t\tstd::stringstream s;\n> > +\t\t\t\ts << \"Unsupported Pixelformat \" << formatInfo.name;\n> > +\t\t\t\tthrow std::invalid_argument(s.str());\n> > +\t\t\t}\n> > +\n> > +\t\t\tdouble sum = 0;\n> > +\t\t\tint w = 20;\n> > +\t\t\tint xs = streamConfig.size.width / 2 - w / 2;\n> > +\t\t\tint ys = streamConfig.size.height / 2 - w / 2;\n> > +\n> > +\t\t\tfor (auto y = ys; y < ys + w; y++) {\n> > +\t\t\t\tauto line = data + y * streamConfig.stride;\n> > +\t\t\t\tfor (auto x = xs; x < xs + w; x++) {\n> > +\t\t\t\t\tsum += calcPixelMean(line + x * pixelStride);\n> > +\t\t\t\t}\n> > +\t\t\t}\n> > +\t\t\tsum = sum / (w * w);\n> > +\t\t\treturn sum;\n> > +\t\t}\n> > +\t}\n> > +\treturn 0;\n> > +}\n> \n> The above functions should be in an anonymous namespace if they are not\n> used outside this translation unit.\n> \n\nOk.\n\n> \n> > +\n> > +TimeSheetEntry::TimeSheetEntry(const ControlIdMap &idmap)\n> > +\t: controls_(idmap)\n> > +{\n> > +}\n> > +\n> > +void TimeSheetEntry::handleCompleteRequest(libcamera::Request *request, const TimeSheetEntry *previous)\n> > +{\n> > +\tmetadata_ = request->metadata();\n> > +\n> > +\tspotBrightness_ = calculateMeanBrightnessFromCenterSpot(request);\n> > +\tif (previous) {\n> > +\t\tbrightnessChange_ = spotBrightness_ / previous->getSpotBrightness();\n> > +\t}\n> > +\tsequence_ = request->sequence();\n> > +}\n> > +\n> > +void TimeSheetEntry::printInfo()\n> > +{\n> > +\tstd::cout << \"=== Frame \" << sequence_ << std::endl;\n> > +\tif (!controls_.empty()) {\n> > +\t\tstd::cout << \"Controls:\" << std::endl;\n> > +\t\tauto idMap = controls_.idMap();\n> > +\t\tassert(idMap);\n> > +\t\tfor (const auto &[id, value] : controls_) {\n> > +\t\t\tstd::cout << \"  \" << idMap->at(id)->name() << \" : \" << value.toString() << std::endl;\n> > +\t\t}\n> > +\t}\n> > +\n> > +\tif (!metadata_.empty()) {\n> > +\t\tstd::cout << \"Metadata:\" << std::endl;\n> > +\t\tauto idMap = metadata_.idMap();\n> > +\t\tassert(idMap);\n> > +\t\tfor (const auto &[id, value] : metadata_) {\n> > +\t\t\tstd::cout << \"  \" << idMap->at(id)->name() << \" : \" << value.toString() << std::endl;\n> > +\t\t}\n> > +\t}\n> > +\n> > +\tstd::cout << \"Calculated Brightness: \" << spotBrightness_ << std::endl;\n> > +}\n> > +\n> > +TimeSheetEntry &TimeSheet::get(size_t pos)\n> > +{\n> > +\tauto &entry = entries_[pos];\n> > +\tif (!entry)\n> > +\t\tentry = std::make_shared<TimeSheetEntry>(idmap_);\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> > +\t}\n> > +\n> > +\tentry.handleCompleteRequest(request, previous);\n> > +}\n> > +\n> > +void TimeSheet::printAllInfos()\n> > +{\n> > +\tfor (auto entry : entries_) {\n> \n> `const auto &entry` to avoid the copy.\n> \n\nYes\n\n> \n> > +\t\tif (entry)\n> > +\t\t\tentry->printInfo();\n> > +\t}\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..2bb89ab3\n> > --- /dev/null\n> > +++ b/src/apps/lc-compliance/time_sheet.h\n> > @@ -0,0 +1,55 @@\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 <future>\n> > +#include <vector>\n> > +\n> > +#include <libcamera/libcamera.h>\n> > +\n> > +class TimeSheetEntry\n> > +{\n> > +public:\n> > +\tTimeSheetEntry() = delete;\n> > +\tTimeSheetEntry(const libcamera::ControlIdMap &idmap);\n> > +\tTimeSheetEntry(TimeSheetEntry &&other) noexcept = default;\n> > +\tTimeSheetEntry(const TimeSheetEntry &) = delete;\n> > +\n> > +\tlibcamera::ControlList &controls() { return controls_; };\n> > +\tlibcamera::ControlList &metadata() { return metadata_; };\n> > +\tvoid handleCompleteRequest(libcamera::Request *request, const TimeSheetEntry *previous);\n> > +\tvoid printInfo();\n> > +\tdouble getSpotBrightness() const { return spotBrightness_; };\n> > +\tdouble getBrightnessChange() const { return brightnessChange_; };\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> \n> No `;` is needed at the end. I'd probably put the `{ }` in a new line\n> to make it clear that that is the function body.\n> \n\nThat was also found by the CI. Fixed.\n\n> > +\n> > +\tvoid prepareForQueue(libcamera::Request *request, uint32_t sequence);\n> > +\tvoid handleCompleteRequest(libcamera::Request *request);\n> > +\tvoid printAllInfos();\n> > +\n> > +\tTimeSheetEntry &operator[](size_t pos) { return get(pos); };\n> > +\tTimeSheetEntry &get(size_t pos);\n> \n> Why are get() and operator[] needed when they both do the same thing?\n> \n\nI wanted to have [] as syntactic shugar. If you have a pointer to a\ntimesheet, [] feels cumbersome and I left get() in there. It turns out I\nonly used it twice in the tests. I can remove get() if you want.\n\n> \n> > +\tsize_t size() const { return entries_.size(); };\n> > +\n> > +private:\n> > +\tconst libcamera::ControlIdMap &idmap_;\n> > +\tstd::vector<std::shared_ptr<TimeSheetEntry>> entries_;\n> \n> Does this need to be shared_ptr? At first glance it seems to me that\n> unique_ptr would be enough. \n> \n\nYes, I had a reason for that during development, but can't remember.\nI'll switch to unique_ptr.\n\nThanks,\nStefan\n\n> \n> > +};\n> > --\n> > 2.40.1\n> \n> \n> Regards,\n> Barnabás Pőcze","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 727C9BD160\n\tfor <parsemail@patchwork.libcamera.org>;\n\tFri, 15 Mar 2024 10:46:49 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id AEE4E62C90;\n\tFri, 15 Mar 2024 11:46:48 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id EB8E962C80\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tFri, 15 Mar 2024 11:46:46 +0100 (CET)","from ideasonboard.com (unknown\n\t[IPv6:2a00:6020:448c:6c00:c477:4921:2179:342c])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 09568B3;\n\tFri, 15 Mar 2024 11:46:22 +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=\"A6isMHmm\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1710499583;\n\tbh=QbsI0F3Pm9f1Iw6qm/R5g5JZLKpgTyxejnBzlUGW60o=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=A6isMHmmnuN0vRVCPP2Fv/3sm4vCENR3yy2eXPH0FWBAh09JSKxv7mWLKD9ne7SHq\n\tohhQwyAtOQRD8s/PH1Xn5GtOK0Y5fqtnQG74QZQx0mA+NtmYjPw/72SZ4amhnUOf+1\n\tgWsRgNfEchgnzTipc54F3Vp7FL0CMrWM5u5UHQjU=","Date":"Fri, 15 Mar 2024 11:46:44 +0100","From":"Stefan Klug <stefan.klug@ideasonboard.com>","To":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= <pobrn@protonmail.com>","Subject":"Re: [PATCH v2 02/12] libcamera: lc-compliance: Add TimeSheet class","Message-ID":"<20240315104644.742gboqlczqdkvie@jasper>","References":"<20240313121223.138150-1-stefan.klug@ideasonboard.com>\n\t<20240313121223.138150-3-stefan.klug@ideasonboard.com>\n\t<7oOHNk5JnI2Zvo68fm9yNnm6z5m0J89FU8CiFiwKjaJROBryysT7VzXdl4bczb1wo3aASPuflHa62fKBcDK_xchFJIdB7nvvbr3As7TA56Y=@protonmail.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<7oOHNk5JnI2Zvo68fm9yNnm6z5m0J89FU8CiFiwKjaJROBryysT7VzXdl4bczb1wo3aASPuflHa62fKBcDK_xchFJIdB7nvvbr3As7TA56Y=@protonmail.com>","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":28986,"web_url":"https://patchwork.libcamera.org/comment/28986/","msgid":"<2h46tcz4ifvtcm2rwndsyoclk7zduwiclw3u6dxm6y4hmmb7oo@gc2b4sfoxd6q>","date":"2024-03-15T13:12:13","subject":"Re: [PATCH v2 02/12] 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 Wed, Mar 13, 2024 at 01:12:13PM +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 <stefan.klug@ideasonboard.com>\n> ---\n>  src/apps/lc-compliance/meson.build    |   1 +\n>  src/apps/lc-compliance/time_sheet.cpp | 145 ++++++++++++++++++++++++++\n>  src/apps/lc-compliance/time_sheet.h   |  55 ++++++++++\n>  3 files changed, 201 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..0ac544d6\n> --- /dev/null\n> +++ b/src/apps/lc-compliance/time_sheet.cpp\n> @@ -0,0 +1,145 @@\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 <sstream>\n> +#include <libcamera/libcamera.h>\n> +\n> +#include \"libcamera/internal/formats.h\"\n> +#include \"libcamera/internal/mapped_framebuffer.h\"\n> +\n> +using namespace libcamera;\n> +\n> +double calcPixelMeanNV12(const uint8_t *data)\n> +{\n> +\treturn (double)*data;\n\nWe try to avoid plain C casts\n\nThese can be\n\ndouble calcPixelMeanNV12(const uint8_t *data)\n{\n\treturn *data;\n}\n\ndouble calcPixelMeanRAW10(const uint8_t *data)\n{\n\treturn *reinterpret_cast<const uint16_t *>(data);\n}\n\n(more on this below)\n\n> +}\n> +\n> +double calcPixelMeanRAW10(const uint8_t *data)\n> +{\n> +\treturn (double)*((const uint16_t *)data);\n> +}\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\n                if (!in.isValid())\n                        continue;\n\nAnd you can save one indentation level\n\n> +\t\t\tauto data = in.planes()[0].data();\n\nauto is convenient but hinders readability when the type is not\ntrivial. Also 'uint8_t *' is almost the same number of characters to\ntype in\n\n> +\t\t\tauto streamConfig = stream->configuration();\n> +\t\t\tauto formatInfo = PixelFormatInfo::info(streamConfig.pixelFormat);\n\nHere it's more ok to use 'auto' as the variables are not used for any\ncomputation like 'data' is (however I still don't like it but it's\nmaybe more a personal taste thing)\n> +\n> +\t\t\tstd::function<double(const uint8_t *data)> calcPixelMean;\n> +\t\t\tint pixelStride;\n> +\n> +\t\t\tswitch (streamConfig.pixelFormat) {\n> +\t\t\tcase formats::NV12:\n> +\t\t\t\tcalcPixelMean = calcPixelMeanNV12;\n> +\t\t\t\tpixelStride = 1;\n> +\t\t\t\tbreak;\n> +\t\t\tcase formats::SRGGB10:\n> +\t\t\t\tcalcPixelMean = calcPixelMeanRAW10;\n> +\t\t\t\tpixelStride = 2;\n\nShouldn't you then advance 'x' by 2 in the below loop if you consider 2\nbytes at a time ?\n\n> +\t\t\t\tbreak;\n> +\t\t\tdefault:\n> +\t\t\t\tstd::stringstream s;\n> +\t\t\t\ts << \"Unsupported Pixelformat \" << formatInfo.name;\n> +\t\t\t\tthrow std::invalid_argument(s.str());\n\nlibcamera doesn't use exceptions, or are tests different ?\n\nThis can just be\n                                std::cerr << ... ;\n\n                                return 0;\n> +\t\t\t}\n> +\n> +\t\t\tdouble sum = 0;\n\n> +\t\t\tint w = 20;\n\nThis is a constant\n                        constexpr unsigned int kSampleSize = 20;\n\n> +\t\t\tint xs = streamConfig.size.width / 2 - w / 2;\n> +\t\t\tint ys = streamConfig.size.height / 2 - w / 2;\n\nThese are unsigned\n\n> +\n> +\t\t\tfor (auto y = ys; y < ys + w; y++) {\n> +\t\t\t\tauto line = data + y * streamConfig.stride;\n> +\t\t\t\tfor (auto x = xs; x < xs + w; x++) {\n> +\t\t\t\t\tsum += calcPixelMean(line + x * pixelStride);\n> +\t\t\t\t}\n\nNo {} for single lines\n\n> +\t\t\t}\n> +\t\t\tsum = sum / (w * w);\n\nShould the number of sampled pixels be divided by 2 for RAW10 (or should the\nsample size be doubled) because you're sampling 2 bytes at a time ?\n\nTo follow the kernel coding style, empty line before return when\npossibile/appropriate\n\n> +\t\t\treturn sum;\n\nYou stop at the first valid stream, is this intended ?\n\n> +\t\t}\n> +\t}\n\nditto, empty line\n\n> +\treturn 0;\n> +}\n\nCan this be then\n\ndouble 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\tauto streamConfig = stream->configuration();\n\t\tauto formatInfo = PixelFormatInfo::info(streamConfig.pixelFormat);\n\n\t\tconstexpr unsigned int kNumSamples = 20;\n\t\tunsigned int pixelStride;\n\n\t\tswitch (streamConfig.pixelFormat) {\n\t\tcase formats::NV12:\n\t\t\tpixelStride = 1;\n\t\t\tbreak;\n\t\tcase formats::SRGGB10:\n\t\t\tpixelStride = 2;\n\t\t\tbreak;\n\t\tdefault:\n\t\t\tstd::cerr << \"Unsupported Pixelformat \"\n\t\t\t\t  << formatInfo.name << std::endl;\n\t\t\treturn 0;\n\t\t}\n\n\t\tunsigned int sampleSize = kNumSamples * pixelStride;\n\t\tunsigned int xs = streamConfig.size.width / 2 - sampleSize / 2;\n\t\tunsigned int ys = streamConfig.size.height / 2 - sampleSize / 2;\n\t\tconst uint8_t *data = in.planes()[0].data();\n\t\tdouble sum = 0;\n\n\t\tfor (unsigned int y = ys; y < ys + sampleSize ; y++) {\n\t\t\tconst uint8_t *line = data + y * streamConfig.stride;\n\n\t\t\tfor (unsigned int x = xs; x < xs + sampleSize; x += pixelStride ) {\n\t\t\t\tconst uint8_t *pixel = line + x * pixelStride;\n\n\t\t\t\tswitch (streamConfig.pixelFormat) {\n\t\t\t\tcase formats::NV12:\n\t\t\t\t\tsum += static_cast<double>(*pixel);\n\t\t\t\t\tbreak;\n\t\t\t\tcase formats::SRGGB10:\n\t\t\t\t\tauto *p = reinterpret_cast<const uint16_t *>(pixel);\n\t\t\t\t\tsum += static_cast<double>(*p);\n\t\t\t\t\tbreak;\n\t\t\t\t}\n\t\t\t}\n\t\t}\n\n\t\treturn sum / kNumSamples * kNumSamples;\n\t}\n\n\treturn 0;\n}\n\nI admit I haven't tested ia, butt only compiled it\n\n> +\n> +TimeSheetEntry::TimeSheetEntry(const ControlIdMap &idmap)\n> +\t: controls_(idmap)\n> +{\n> +}\n> +\n> +void TimeSheetEntry::handleCompleteRequest(libcamera::Request *request, const TimeSheetEntry *previous)\n\nIf it's easy to do, try to stay in 80 cols (libcamera allows up to 120\nactually, but when it's trivial such as in this case...\n\nvoid TimeSheetEntry::handleCompleteRequest(libcamera::Request *request,\n\t\t\t\t\t   const TimeSheetEntry *previous)\n\n> +{\n> +\tmetadata_ = request->metadata();\n> +\n> +\tspotBrightness_ = calculateMeanBrightnessFromCenterSpot(request);\n> +\tif (previous) {\n> +\t\tbrightnessChange_ = spotBrightness_ / previous->getSpotBrightness();\n> +\t}\n\nNo {}\n\n> +\tsequence_ = request->sequence();\n> +}\n> +\n> +void TimeSheetEntry::printInfo()\n> +{\n> +\tstd::cout << \"=== Frame \" << sequence_ << std::endl;\n> +\tif (!controls_.empty()) {\n> +\t\tstd::cout << \"Controls:\" << std::endl;\n> +\t\tauto idMap = controls_.idMap();\n> +\t\tassert(idMap);\n> +\t\tfor (const auto &[id, value] : controls_) {\n> +\t\t\tstd::cout << \"  \" << idMap->at(id)->name() << \" : \" << value.toString() << std::endl;\n> +\t\t}\n\nditto, no {}\n\n> +\t}\n> +\n> +\tif (!metadata_.empty()) {\n> +\t\tstd::cout << \"Metadata:\" << std::endl;\n> +\t\tauto idMap = metadata_.idMap();\n> +\t\tassert(idMap);\n> +\t\tfor (const auto &[id, value] : metadata_) {\n> +\t\t\tstd::cout << \"  \" << idMap->at(id)->name() << \" : \" << value.toString() << std::endl;\n> +\t\t}\n\nsame here\n\n> +\t}\n> +\n> +\tstd::cout << \"Calculated Brightness: \" << spotBrightness_ << std::endl;\n> +}\n> +\n> +TimeSheetEntry &TimeSheet::get(size_t pos)\n> +{\n> +\tauto &entry = entries_[pos];\n\nShould you make sure pos is smaller than the allocated vector lenght ?\nAfaik operator[] is not bound checked\n\n> +\tif (!entry)\n> +\t\tentry = std::make_shared<TimeSheetEntry>(idmap_);\n\nempty line please\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\nWhy one 'auto' and one explicit type ? (You know my preference\nalready, but up yo you which one to chose as long as you're\nconsistent)\n\n> +\tif (sequence >= 1) {\n> +\t\tprevious = entries_[sequence - 1].get();\n\nCould this be: get(sequence - 1).get() or you don't like get().get() ?\n\n> +\t}\n\nno braces\n\n> +\n> +\tentry.handleCompleteRequest(request, previous);\n> +}\n> +\n> +void TimeSheet::printAllInfos()\n> +{\n> +\tfor (auto entry : entries_) {\n> +\t\tif (entry)\n> +\t\t\tentry->printInfo();\n> +\t}\n\nFor multiple lines statements like this, even if not required by the\nlanguage, {} are fine\n\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..2bb89ab3\n> --- /dev/null\n> +++ b/src/apps/lc-compliance/time_sheet.h\n> @@ -0,0 +1,55 @@\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 <future>\n\nWhat for ?\n\n> +#include <vector>\n\nAlso include <memory> for smart pointers\n\n> +\n> +#include <libcamera/libcamera.h>\n> +\n> +class TimeSheetEntry\n> +{\n> +public:\n> +\tTimeSheetEntry() = delete;\n> +\tTimeSheetEntry(const libcamera::ControlIdMap &idmap);\n> +\tTimeSheetEntry(TimeSheetEntry &&other) noexcept = default;\n\nIs this used ? (and I was not expecting noexcept as we don't use\nexceptions, however we don't compile with exception disabled)\n\n> +\tTimeSheetEntry(const TimeSheetEntry &) = delete;\n> +\n> +\tlibcamera::ControlList &controls() { return controls_; };\n> +\tlibcamera::ControlList &metadata() { return metadata_; };\n\nconst ..... const {};\n\nfor both\n\n> +\tvoid handleCompleteRequest(libcamera::Request *request, const TimeSheetEntry *previous);\n\nwe usually pass const parameters in by reference and not by pointer\n(if possible and convenient ofc)\n\n> +\tvoid printInfo();\n\noperator<< ?\n\n> +\tdouble getSpotBrightness() const { return spotBrightness_; };\n> +\tdouble getBrightnessChange() const { return brightnessChange_; };\n\nNot used\n\nlibcamera doesn't usually prepend 'get' to getter function names\n\n        type fieldName const { return fieldName_; }\n\nand no need for ; at the of functions implementation (here and above)\n\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\nbraces in two lines below\n\n        {\n        }\n\nNo need for ;\n\n> +\n> +\tvoid prepareForQueue(libcamera::Request *request, uint32_t sequence);\n> +\tvoid handleCompleteRequest(libcamera::Request *request);\n> +\tvoid printAllInfos();\n> +\n> +\tTimeSheetEntry &operator[](size_t pos) { return get(pos); };\n\nditto\n\n> +\tTimeSheetEntry &get(size_t pos);\n> +\tsize_t size() const { return entries_.size(); };\n> +\n> +private:\n> +\tconst libcamera::ControlIdMap &idmap_;\n> +\tstd::vector<std::shared_ptr<TimeSheetEntry>> entries_;\n\nAs noted already, you can use a unique_ptr<>\n\n> +};\n> --\n> 2.40.1\n>","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 2AD3ABD808\n\tfor <parsemail@patchwork.libcamera.org>;\n\tFri, 15 Mar 2024 13:12:19 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id DEC5062C8E;\n\tFri, 15 Mar 2024 14:12:17 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id AC0BF62C8B\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tFri, 15 Mar 2024 14:12:16 +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 A0960667;\n\tFri, 15 Mar 2024 14:11:52 +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=\"CihmuGAF\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1710508312;\n\tbh=f7oNsRuhYqSp+M79uNJcH2gglkd0AmqOnQigsIHPChI=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=CihmuGAF0IfC0VQ0RUKz5nfteG9zGA9EntdIyVpwaMKu7EXMFuSjmIgXE12rszmB6\n\tUuvaEYJdhHnzqpIM04E3J7yNDpxi9IokuC7TVcu2CEVQApqB0Ta+SAnqep58nPTtix\n\t2EgoPIau0/4rXKDrOc7aUvLkbtq++PQHaW1zgFsc=","Date":"Fri, 15 Mar 2024 14:12:13 +0100","From":"Jacopo Mondi <jacopo.mondi@ideasonboard.com>","To":"Stefan Klug <stefan.klug@ideasonboard.com>","Subject":"Re: [PATCH v2 02/12] libcamera: lc-compliance: Add TimeSheet class","Message-ID":"<2h46tcz4ifvtcm2rwndsyoclk7zduwiclw3u6dxm6y4hmmb7oo@gc2b4sfoxd6q>","References":"<20240313121223.138150-1-stefan.klug@ideasonboard.com>\n\t<20240313121223.138150-3-stefan.klug@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20240313121223.138150-3-stefan.klug@ideasonboard.com>","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":28988,"web_url":"https://patchwork.libcamera.org/comment/28988/","msgid":"<20240315151515.xgvfnxgxumjcbikp@jasper>","date":"2024-03-15T15:15:15","subject":"Re: [PATCH v2 02/12] 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\nthanks for the review. I'll handle all the tiny style related stuff in\nthe next version. So I did not comment on these.\n \nOn Fri, Mar 15, 2024 at 02:12:13PM +0100, Jacopo Mondi wrote:\n> Hi Stefan\n> \n> On Wed, Mar 13, 2024 at 01:12:13PM +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 <stefan.klug@ideasonboard.com>\n> > ---\n> >  src/apps/lc-compliance/meson.build    |   1 +\n> >  src/apps/lc-compliance/time_sheet.cpp | 145 ++++++++++++++++++++++++++\n> >  src/apps/lc-compliance/time_sheet.h   |  55 ++++++++++\n> >  3 files changed, 201 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..0ac544d6\n> > --- /dev/null\n> > +++ b/src/apps/lc-compliance/time_sheet.cpp\n> > @@ -0,0 +1,145 @@\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 <sstream>\n> > +#include <libcamera/libcamera.h>\n> > +\n> > +#include \"libcamera/internal/formats.h\"\n> > +#include \"libcamera/internal/mapped_framebuffer.h\"\n> > +\n> > +using namespace libcamera;\n> > +\n> > +double calcPixelMeanNV12(const uint8_t *data)\n> > +{\n> > +\treturn (double)*data;\n> \n> We try to avoid plain C casts\n> \n> These can be\n> \n> double calcPixelMeanNV12(const uint8_t *data)\n> {\n> \treturn *data;\n> }\n> \n> double calcPixelMeanRAW10(const uint8_t *data)\n> {\n> \treturn *reinterpret_cast<const uint16_t *>(data);\n> }\n> \n> (more on this below)\n> \n> > +}\n> > +\n> > +double calcPixelMeanRAW10(const uint8_t *data)\n> > +{\n> > +\treturn (double)*((const uint16_t *)data);\n> > +}\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> \n>                 if (!in.isValid())\n>                         continue;\n> \n> And you can save one indentation level\n> \n> > +\t\t\tauto data = in.planes()[0].data();\n> \n> auto is convenient but hinders readability when the type is not\n> trivial. Also 'uint8_t *' is almost the same number of characters to\n> type in\n> \n> > +\t\t\tauto streamConfig = stream->configuration();\n> > +\t\t\tauto formatInfo = PixelFormatInfo::info(streamConfig.pixelFormat);\n> \n> Here it's more ok to use 'auto' as the variables are not used for any\n> computation like 'data' is (however I still don't like it but it's\n> maybe more a personal taste thing)\n> > +\n> > +\t\t\tstd::function<double(const uint8_t *data)> calcPixelMean;\n> > +\t\t\tint pixelStride;\n> > +\n> > +\t\t\tswitch (streamConfig.pixelFormat) {\n> > +\t\t\tcase formats::NV12:\n> > +\t\t\t\tcalcPixelMean = calcPixelMeanNV12;\n> > +\t\t\t\tpixelStride = 1;\n> > +\t\t\t\tbreak;\n> > +\t\t\tcase formats::SRGGB10:\n> > +\t\t\t\tcalcPixelMean = calcPixelMeanRAW10;\n> > +\t\t\t\tpixelStride = 2;\n> \n> Shouldn't you then advance 'x' by 2 in the below loop if you consider 2\n> bytes at a time ?\n\nThat is handled by pixelStride in \"line + x * pixelStride\" so x is in\npexels. Or do I miss something here?\n\n> \n> > +\t\t\t\tbreak;\n> > +\t\t\tdefault:\n> > +\t\t\t\tstd::stringstream s;\n> > +\t\t\t\ts << \"Unsupported Pixelformat \" << formatInfo.name;\n> > +\t\t\t\tthrow std::invalid_argument(s.str());\n> \n> libcamera doesn't use exceptions, or are tests different ?\n> \n> This can just be\n>                                 std::cerr << ... ;\n> \n>                                 return 0;\n\nOh I see. Sure.\n\n> > +\t\t\t}\n> > +\n> > +\t\t\tdouble sum = 0;\n> \n> > +\t\t\tint w = 20;\n> \n> This is a constant\n>                         constexpr unsigned int kSampleSize = 20;\n> \n> > +\t\t\tint xs = streamConfig.size.width / 2 - w / 2;\n> > +\t\t\tint ys = streamConfig.size.height / 2 - w / 2;\n> \n> These are unsigned\n\nReally? In my experience handling these things with unsigned ints often\nleads to unexpected corner cases or ugly code (not beeing able to\nsubtract numbers without thinking twice, cast to signed for comparisons\netc.). The numbers are known to always be smaller than max int, so it\ndoesn't hurt. But I see that in this specific case having all as uint\nwould work.\n\n> \n> > +\n> > +\t\t\tfor (auto y = ys; y < ys + w; y++) {\n> > +\t\t\t\tauto line = data + y * streamConfig.stride;\n> > +\t\t\t\tfor (auto x = xs; x < xs + w; x++) {\n> > +\t\t\t\t\tsum += calcPixelMean(line + x * pixelStride);\n> > +\t\t\t\t}\n> \n> No {} for single lines\n> \n> > +\t\t\t}\n> > +\t\t\tsum = sum / (w * w);\n> \n> Should the number of sampled pixels be divided by 2 for RAW10 (or should the\n> sample size be doubled) because you're sampling 2 bytes at a time ?\n\nNo, as I'm always sampling the same amount of pixels.\n\n> \n> To follow the kernel coding style, empty line before return when\n> possibile/appropriate\n> \n> > +\t\t\treturn sum;\n> \n> You stop at the first valid stream, is this intended ?\n\nYes, I didn't see a need to handle multiple streams here. I couldn't\ncome up with a case where the first stream is invalid for the brightness\nmeasurement. Do you have a usecase for that in mind?\n\n> \n> > +\t\t}\n> > +\t}\n> \n> ditto, empty line\n> \n> > +\treturn 0;\n> > +}\n> \n> Can this be then\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\tauto streamConfig = stream->configuration();\n> \t\tauto formatInfo = PixelFormatInfo::info(streamConfig.pixelFormat);\n> \n> \t\tconstexpr unsigned int kNumSamples = 20;\n> \t\tunsigned int pixelStride;\n> \n> \t\tswitch (streamConfig.pixelFormat) {\n> \t\tcase formats::NV12:\n> \t\t\tpixelStride = 1;\n> \t\t\tbreak;\n> \t\tcase formats::SRGGB10:\n> \t\t\tpixelStride = 2;\n> \t\t\tbreak;\n> \t\tdefault:\n> \t\t\tstd::cerr << \"Unsupported Pixelformat \"\n> \t\t\t\t  << formatInfo.name << std::endl;\n> \t\t\treturn 0;\n> \t\t}\n> \n> \t\tunsigned int sampleSize = kNumSamples * pixelStride;\n> \t\tunsigned int xs = streamConfig.size.width / 2 - sampleSize / 2;\n> \t\tunsigned int ys = streamConfig.size.height / 2 - sampleSize / 2;\n> \t\tconst uint8_t *data = in.planes()[0].data();\n> \t\tdouble sum = 0;\n> \n> \t\tfor (unsigned int y = ys; y < ys + sampleSize ; y++) {\n> \t\t\tconst uint8_t *line = data + y * streamConfig.stride;\n> \n> \t\t\tfor (unsigned int x = xs; x < xs + sampleSize; x += pixelStride ) {\n> \t\t\t\tconst uint8_t *pixel = line + x * pixelStride;\n> \n> \t\t\t\tswitch (streamConfig.pixelFormat) {\n> \t\t\t\tcase formats::NV12:\n> \t\t\t\t\tsum += static_cast<double>(*pixel);\n> \t\t\t\t\tbreak;\n> \t\t\t\tcase formats::SRGGB10:\n> \t\t\t\t\tauto *p = reinterpret_cast<const uint16_t *>(pixel);\n> \t\t\t\t\tsum += static_cast<double>(*p);\n> \t\t\t\t\tbreak;\n> \t\t\t\t}\n> \t\t\t}\n> \t\t}\n> \n> \t\treturn sum / kNumSamples * kNumSamples;\n> \t}\n> \n> \treturn 0;\n> }\n> \n> I admit I haven't tested ia, butt only compiled it\n\nI'm sorry, but I don't see the benefit here. In this case you\nmoved the knowledge of the underlying data types into the loops and can\nloop over bytes (which fails in the \"*pixel = ...\" line). That gets more\ncomplex when more formats shall be supported. My named functions are\ndefinitely an overkill above. But looping over pixel positions and then\nhaving a PixelFormat specific accessor function simplifies things IMHO.\nI don't know if there is a runtime speed difference between the switch\ninside the loop and the function calls...\n\nThe cleanest would be to also move pixelStride and lineStride into the\naccessor, so that one could write the algorithm instead caring about the\ndata. Something like (pseudocode):\n\nauto accessor = constructView(data, pixelFormat);\n...\nfor (auto y = ys; y < ys + w; y++) {\n    for (auto x = xs; x < xs + w; x++) {\n\tsum += accessor.calcPixelMean(x,y);\n    }\n}\n\n> \n> > +\n> > +TimeSheetEntry::TimeSheetEntry(const ControlIdMap &idmap)\n> > +\t: controls_(idmap)\n> > +{\n> > +}\n> > +\n> > +void TimeSheetEntry::handleCompleteRequest(libcamera::Request *request, const TimeSheetEntry *previous)\n> \n> If it's easy to do, try to stay in 80 cols (libcamera allows up to 120\n> actually, but when it's trivial such as in this case...\n> \n> void TimeSheetEntry::handleCompleteRequest(libcamera::Request *request,\n> \t\t\t\t\t   const TimeSheetEntry *previous)\n> \n> > +{\n> > +\tmetadata_ = request->metadata();\n> > +\n> > +\tspotBrightness_ = calculateMeanBrightnessFromCenterSpot(request);\n> > +\tif (previous) {\n> > +\t\tbrightnessChange_ = spotBrightness_ / previous->getSpotBrightness();\n> > +\t}\n> \n> No {}\n> \n> > +\tsequence_ = request->sequence();\n> > +}\n> > +\n> > +void TimeSheetEntry::printInfo()\n> > +{\n> > +\tstd::cout << \"=== Frame \" << sequence_ << std::endl;\n> > +\tif (!controls_.empty()) {\n> > +\t\tstd::cout << \"Controls:\" << std::endl;\n> > +\t\tauto idMap = controls_.idMap();\n> > +\t\tassert(idMap);\n> > +\t\tfor (const auto &[id, value] : controls_) {\n> > +\t\t\tstd::cout << \"  \" << idMap->at(id)->name() << \" : \" << value.toString() << std::endl;\n> > +\t\t}\n> \n> ditto, no {}\n> \n> > +\t}\n> > +\n> > +\tif (!metadata_.empty()) {\n> > +\t\tstd::cout << \"Metadata:\" << std::endl;\n> > +\t\tauto idMap = metadata_.idMap();\n> > +\t\tassert(idMap);\n> > +\t\tfor (const auto &[id, value] : metadata_) {\n> > +\t\t\tstd::cout << \"  \" << idMap->at(id)->name() << \" : \" << value.toString() << std::endl;\n> > +\t\t}\n> \n> same here\n> \n> > +\t}\n> > +\n> > +\tstd::cout << \"Calculated Brightness: \" << spotBrightness_ << std::endl;\n> > +}\n> > +\n> > +TimeSheetEntry &TimeSheet::get(size_t pos)\n> > +{\n> > +\tauto &entry = entries_[pos];\n> \n> Should you make sure pos is smaller than the allocated vector lenght ?\n> Afaik operator[] is not bound checked\n\nDang. Yes you are right. That is a bad one.\n\n> \n> > +\tif (!entry)\n> > +\t\tentry = std::make_shared<TimeSheetEntry>(idmap_);\n> \n> empty line please\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> \n> Why one 'auto' and one explicit type ? (You know my preference\n> already, but up yo you which one to chose as long as you're\n> consistent)\n\nWell I tried to go for auto. But the compiler is unable to deduce the\ntype for previous, so I declared that explicitely. I remember a harsh\ndiscussion where I tried to convince a teammate that auto shouldn't be\nused. Now I'm advocating auto :-) Oh dear.\n\n> \n> > +\tif (sequence >= 1) {\n> > +\t\tprevious = entries_[sequence - 1].get();\n> \n> Could this be: get(sequence - 1).get() or you don't like get().get() ?\n> \n> > +\t}\n> \n> no braces\n> \n> > +\n> > +\tentry.handleCompleteRequest(request, previous);\n> > +}\n> > +\n> > +void TimeSheet::printAllInfos()\n> > +{\n> > +\tfor (auto entry : entries_) {\n> > +\t\tif (entry)\n> > +\t\t\tentry->printInfo();\n> > +\t}\n> \n> For multiple lines statements like this, even if not required by the\n> language, {} are fine\n> \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..2bb89ab3\n> > --- /dev/null\n> > +++ b/src/apps/lc-compliance/time_sheet.h\n> > @@ -0,0 +1,55 @@\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 <future>\n> \n> What for ?\n> \n> > +#include <vector>\n> \n> Also include <memory> for smart pointers\n> \n> > +\n> > +#include <libcamera/libcamera.h>\n> > +\n> > +class TimeSheetEntry\n> > +{\n> > +public:\n> > +\tTimeSheetEntry() = delete;\n> > +\tTimeSheetEntry(const libcamera::ControlIdMap &idmap);\n> > +\tTimeSheetEntry(TimeSheetEntry &&other) noexcept = default;\n> \n> Is this used ? (and I was not expecting noexcept as we don't use\n> exceptions, however we don't compile with exception disabled)\n\nThis was a leftover from my first try to use unique_ptr in the vector.\nstd::vector can only support move semantics on resize if constructor and\ndesctructor are noexcept. See https://stackoverflow.com/a/15418208 \nSo yes it will be used again :-)\n\nThanks again for your review.\n\nCheers,\nStefan\n\n> \n> > +\tTimeSheetEntry(const TimeSheetEntry &) = delete;\n> > +\n> > +\tlibcamera::ControlList &controls() { return controls_; };\n> > +\tlibcamera::ControlList &metadata() { return metadata_; };\n> \n> const ..... const {};\n> \n> for both\n> \n> > +\tvoid handleCompleteRequest(libcamera::Request *request, const TimeSheetEntry *previous);\n> \n> we usually pass const parameters in by reference and not by pointer\n> (if possible and convenient ofc)\n> \n> > +\tvoid printInfo();\n> \n> operator<< ?\n> \n> > +\tdouble getSpotBrightness() const { return spotBrightness_; };\n> > +\tdouble getBrightnessChange() const { return brightnessChange_; };\n> \n> Not used\n> \n> libcamera doesn't usually prepend 'get' to getter function names\n> \n>         type fieldName const { return fieldName_; }\n> \n> and no need for ; at the of functions implementation (here and above)\n> \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> \n> braces in two lines below\n> \n>         {\n>         }\n> \n> No need for ;\n> \n> > +\n> > +\tvoid prepareForQueue(libcamera::Request *request, uint32_t sequence);\n> > +\tvoid handleCompleteRequest(libcamera::Request *request);\n> > +\tvoid printAllInfos();\n> > +\n> > +\tTimeSheetEntry &operator[](size_t pos) { return get(pos); };\n> \n> ditto\n> \n> > +\tTimeSheetEntry &get(size_t pos);\n> > +\tsize_t size() const { return entries_.size(); };\n> > +\n> > +private:\n> > +\tconst libcamera::ControlIdMap &idmap_;\n> > +\tstd::vector<std::shared_ptr<TimeSheetEntry>> entries_;\n> \n> As noted already, you can use a unique_ptr<>\n> \n> > +};\n> > --\n> > 2.40.1\n> >","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 4D50CBD160\n\tfor <parsemail@patchwork.libcamera.org>;\n\tFri, 15 Mar 2024 15:15:20 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C01B16294A;\n\tFri, 15 Mar 2024 16:15:19 +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 3EA1761C65\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tFri, 15 Mar 2024 16:15:18 +0100 (CET)","from ideasonboard.com (unknown\n\t[IPv6:2a00:6020:448c:6c00:c477:4921:2179:342c])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 21E1D667;\n\tFri, 15 Mar 2024 16:14:54 +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=\"rw4pf4+z\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1710515694;\n\tbh=8G/K2O6/KhJ/1zrHwVP7Vmuhr4ODU/NVZ4cgbHl5wNs=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=rw4pf4+zQ0+BRR8HymrI+6xgYI5pppgQH5VOJaS/sT9fnNBKGDTJNd21htU1FSB7y\n\tTz3aO4Y0ij1gIdCSvcWNVkoTLmKeQw7LFKNhm0UpVoN40SSdE+aeRgs9o05vjVUqdO\n\trwUXjdtJiSqiibhzcnFbnZcXGq3Hl2UF0WtwYAfM=","Date":"Fri, 15 Mar 2024 16:15:15 +0100","From":"Stefan Klug <stefan.klug@ideasonboard.com>","To":"Jacopo Mondi <jacopo.mondi@ideasonboard.com>","Subject":"Re: [PATCH v2 02/12] libcamera: lc-compliance: Add TimeSheet class","Message-ID":"<20240315151515.xgvfnxgxumjcbikp@jasper>","References":"<20240313121223.138150-1-stefan.klug@ideasonboard.com>\n\t<20240313121223.138150-3-stefan.klug@ideasonboard.com>\n\t<2h46tcz4ifvtcm2rwndsyoclk7zduwiclw3u6dxm6y4hmmb7oo@gc2b4sfoxd6q>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<2h46tcz4ifvtcm2rwndsyoclk7zduwiclw3u6dxm6y4hmmb7oo@gc2b4sfoxd6q>","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}}]