[{"id":16813,"web_url":"https://patchwork.libcamera.org/comment/16813/","msgid":"<CAO-P9RriwXCv-pxOtvAzWZKSUQmjTBw9rV8AW7oSPUQ7Vc82=g@mail.gmail.com>","date":"2021-05-06T17:17:56","subject":"Re: [libcamera-devel] [PATCH v3 1/1] ipa: raspberrypi: Use\n\tCamHelpers to generalise sensor embedded data parsing","submitter":{"id":51,"url":"https://patchwork.libcamera.org/api/people/51/","name":"Marvin Schmidt","email":"marvin.schmidt1987@gmail.com"},"content":"Hi David,\n\nThanks for your work on this\n\nAm Mi., 5. Mai 2021 um 16:04 Uhr schrieb David Plowman\n<david.plowman@raspberrypi.com>:\n>\n> CamHelpers get virtual Prepare() and Process() methods, running just\n> before and just after the ISP, just like Raspberry Pi Algorithms.\n>\n> The Prepare() method is able to parse the register dumps in embedded\n> data buffers, and can be specialised to perform custom processing when\n> necessary.\n>\n> The IPA itself only needs to call the new Prepare() and Process()\n> methods. It must fill in the DeviceStatus from the controls first, in\n> case the Prepare() method does not supply its own values.\n>\n> Signed-off-by: David Plowman <david.plowman@raspberrypi.com>\n> Reviewed-by: Naushir Patuck <naush@raspberrypi.com>\n> ---\n>  src/ipa/raspberrypi/cam_helper.cpp  | 51 ++++++++++++++++++\n>  src/ipa/raspberrypi/cam_helper.hpp  | 11 +++-\n>  src/ipa/raspberrypi/raspberrypi.cpp | 80 ++++++++++-------------------\n>  3 files changed, 87 insertions(+), 55 deletions(-)\n>\n> diff --git a/src/ipa/raspberrypi/cam_helper.cpp b/src/ipa/raspberrypi/cam_helper.cpp\n> index 0ae0baa0..5af73c9c 100644\n> --- a/src/ipa/raspberrypi/cam_helper.cpp\n> +++ b/src/ipa/raspberrypi/cam_helper.cpp\n> @@ -17,6 +17,11 @@\n>  #include \"md_parser.hpp\"\n>\n>  using namespace RPiController;\n> +using namespace libcamera;\n> +\n> +namespace libcamera {\n> +LOG_DECLARE_CATEGORY(IPARPI)\n> +}\n>\n>  static std::map<std::string, CamHelperCreateFunc> cam_helpers;\n>\n> @@ -45,6 +50,17 @@ CamHelper::~CamHelper()\n>         delete parser_;\n>  }\n>\n> +void CamHelper::Prepare(const Span<uint8_t> &buffer,\n> +                       Metadata &metadata)\n> +{\n> +       parseEmbeddedData(buffer, metadata);\n> +}\n> +\n> +void CamHelper::Process([[maybe_unused]] StatisticsPtr &stats,\n> +                       [[maybe_unused]] Metadata &metadata)\n> +{\n> +}\n> +\n>  uint32_t CamHelper::ExposureLines(double exposure_us) const\n>  {\n>         assert(initialized_);\n> @@ -139,6 +155,41 @@ unsigned int CamHelper::MistrustFramesModeSwitch() const\n>         return 0;\n>  }\n>\n> +void CamHelper::parseEmbeddedData(const Span<uint8_t> &buffer,\n> +                                 Metadata &metadata)\n> +{\n> +       if (buffer.size()) {\n\nYou can use `Span::empty()` to check if the span is... empty ;-)\n\nSince the function can't do much without some data, I would return\nimmediately in that case\nand save a level of indentation:\n\n    if (buffer.empty())\n        return;\n\n    // do real work\n\n> +               bool success = false;\n> +               uint32_t exposureLines, gainCode;\n> +\n> +               parser_->SetBufferSize(buffer.size());\n> +               success = parser_->Parse(buffer.data()) == MdParser::Status::OK &&\n> +                         parser_->GetExposureLines(exposureLines) == MdParser::Status::OK &&\n> +                         parser_->GetGainCode(gainCode) == MdParser::Status::OK;\n> +\n\nI would probably check `success` right here and return early if it\nwasn't successful\n\n> +               /*\n> +                * Overwrite the exposure/gain values in the DeviceStatus, as\n> +                * we know better. Fetch it first in case any other fields were\n> +                * set meaningfully.\n> +                */\n> +               struct DeviceStatus deviceStatus;\n\nThe `struct` isn't needed. And just for good measure, I would\nzero-initialize it with `= {}`\n\n> +\n> +               if (success &&\n> +                   metadata.Get(\"device.status\", deviceStatus) == 0) {\n> +                       deviceStatus.shutter_speed = Exposure(exposureLines);\n> +                       deviceStatus.analogue_gain = Gain(gainCode);\n> +\n> +                       LOG(IPARPI, Debug) << \"Metadata updated - Exposure : \"\n> +                                          << deviceStatus.shutter_speed\n> +                                          << \" Gain : \"\n> +                                          << deviceStatus.analogue_gain;\n> +\n> +                       metadata.Set(\"device.status\", deviceStatus);\n> +               } else\n> +                       LOG(IPARPI, Error) << \"Embedded buffer parsing failed\";\n\nIf we check `success` right after the parsing step and bail out if it\nwasn't successful, this could be written\nlike:\n\n    if (metadata.Get(\"device.status\", deviceStatus) != 0) {\n        LOG(IPARPI, Error) << \"Could not get device status from metadata\";\n        return;\n    }\n\n    deviceStatus.shutter_speed = Exposure(exposureLines);\n    deviceStatus.analogue_gain = Gain(gainCode);\n    [...]\n\nI guess it's good to have a different error message for the\n`metadata.Get(\"device.status\", deviceStatus) != 0` case\nanyway, since it's not really related to the parsing of the embedded data?\n\n> +       }\n> +}\n> +\n>  RegisterCamHelper::RegisterCamHelper(char const *cam_name,\n>                                      CamHelperCreateFunc create_func)\n>  {\n> diff --git a/src/ipa/raspberrypi/cam_helper.hpp b/src/ipa/raspberrypi/cam_helper.hpp\n> index c3ed5362..6ee5051c 100644\n> --- a/src/ipa/raspberrypi/cam_helper.hpp\n> +++ b/src/ipa/raspberrypi/cam_helper.hpp\n> @@ -8,7 +8,11 @@\n>\n>  #include <string>\n>\n> +#include <libcamera/span.h>\n> +\n>  #include \"camera_mode.h\"\n> +#include \"controller/controller.hpp\"\n> +#include \"controller/metadata.hpp\"\n>  #include \"md_parser.hpp\"\n>\n>  #include \"libcamera/internal/v4l2_videodevice.h\"\n> @@ -65,7 +69,9 @@ public:\n>         CamHelper(MdParser *parser, unsigned int frameIntegrationDiff);\n>         virtual ~CamHelper();\n>         void SetCameraMode(const CameraMode &mode);\n> -       MdParser &Parser() const { return *parser_; }\n> +       virtual void Prepare(const libcamera::Span<uint8_t> &buffer,\n> +                            Metadata &metadata);\n\nThe CppCoreGuidelines suggest passing spans by value:\n\n    Note: A span<T> object does not own its elements and is so small\nthat it can be passed by value.\n    Passing a span object as an argument is exactly as efficient as\npassing a pair of pointer arguments\n    or passing a pointer and an integer count.\n\nhttps://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#f24-use-a-spant-or-a-span_pt-to-designate-a-half-open-sequence\n\nSo this could be `libcamera::Span<const uint8_t> buffer` since the\nfunction just needs read-only access to the\nbuffer data. This would require changing the signature of\n`MdParser::Parse()` though. Either to `const void *` or\nbetter yet `libcamera::Span<const uint8_t>` as well. Which would allow\nremoving the `static_cast<uint8_t *>(data)`\ninside the `MdParser::Parse()` implementations.\n\n> +       virtual void Process(StatisticsPtr &stats, Metadata &metadata);\n>         uint32_t ExposureLines(double exposure_us) const;\n>         double Exposure(uint32_t exposure_lines) const; // in us\n>         virtual uint32_t GetVBlanking(double &exposure_us, double minFrameDuration,\n> @@ -81,6 +87,9 @@ public:\n>         virtual unsigned int MistrustFramesModeSwitch() const;\n>\n>  protected:\n> +       void parseEmbeddedData(const libcamera::Span<uint8_t> &buffer,\n> +                              Metadata &metadata);\n> +\n>         MdParser *parser_;\n>         CameraMode mode_;\n>\n> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp\n> index dad6395f..b0f61d35 100644\n> --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> @@ -101,9 +101,7 @@ private:\n>         void returnEmbeddedBuffer(unsigned int bufferId);\n>         void prepareISP(const ipa::RPi::ISPConfig &data);\n>         void reportMetadata();\n> -       bool parseEmbeddedData(unsigned int bufferId, struct DeviceStatus &deviceStatus);\n> -       void fillDeviceStatus(uint32_t exposureLines, uint32_t gainCode,\n> -                             struct DeviceStatus &deviceStatus);\n> +       void fillDeviceStatus(const ControlList &sensorControls);\n>         void processStats(unsigned int bufferId);\n>         void applyFrameDurations(double minFrameDuration, double maxFrameDuration);\n>         void applyAGC(const struct AgcStatus *agcStatus, ControlList &ctrls);\n> @@ -894,35 +892,34 @@ void IPARPi::returnEmbeddedBuffer(unsigned int bufferId)\n>\n>  void IPARPi::prepareISP(const ipa::RPi::ISPConfig &data)\n>  {\n> -       struct DeviceStatus deviceStatus = {};\n> -       bool success = false;\n> +       Span<uint8_t> embeddedBuffer;\n\nThis can be `Span<const uint8_t>`\n\n> +\n> +       rpiMetadata_.Clear();\n> +\n> +       fillDeviceStatus(data.controls);\n>\n>         if (data.embeddedBufferPresent) {\n>                 /*\n>                  * Pipeline handler has supplied us with an embedded data buffer,\n> -                * so parse it and extract the exposure and gain.\n> +                * we must pass it to the CamHelper for parsing.\n>                  */\n> -               success = parseEmbeddedData(data.embeddedBufferId, deviceStatus);\n> -\n> -               /* Done with embedded data now, return to pipeline handler asap. */\n> -               returnEmbeddedBuffer(data.embeddedBufferId);\n> +               auto it = buffers_.find(data.embeddedBufferId);\n> +               ASSERT(it != buffers_.end());\n> +               embeddedBuffer = it->second.maps()[0];\n>         }\n>\n> -       if (!success) {\n> -               /*\n> -                * Pipeline handler has not supplied an embedded data buffer,\n> -                * or embedded data buffer parsing has failed for some reason,\n> -                * so pull the exposure and gain values from the control list.\n> -                */\n> -               int32_t exposureLines = data.controls.get(V4L2_CID_EXPOSURE).get<int32_t>();\n> -               int32_t gainCode = data.controls.get(V4L2_CID_ANALOGUE_GAIN).get<int32_t>();\n> -               fillDeviceStatus(exposureLines, gainCode, deviceStatus);\n> -       }\n> +       /*\n> +        * This may overwrite the DeviceStatus using values from the sensor\n> +        * metadata, and may also do additional custom processing.\n> +        */\n> +       helper_->Prepare(embeddedBuffer, rpiMetadata_);\n> +\n> +       /* Done with embedded data now, return to pipeline handler asap. */\n> +       if (data.embeddedBufferPresent)\n> +               returnEmbeddedBuffer(data.embeddedBufferId);\n>\n>         ControlList ctrls(ispCtrls_);\n>\n> -       rpiMetadata_.Clear();\n> -       rpiMetadata_.Set(\"device.status\", deviceStatus);\n>         controller_.Prepare(&rpiMetadata_);\n>\n>         /* Lock the metadata buffer to avoid constant locks/unlocks. */\n> @@ -972,41 +969,13 @@ void IPARPi::prepareISP(const ipa::RPi::ISPConfig &data)\n>                 setIspControls.emit(ctrls);\n>  }\n>\n> -bool IPARPi::parseEmbeddedData(unsigned int bufferId, struct DeviceStatus &deviceStatus)\n> +void IPARPi::fillDeviceStatus(const ControlList &sensorControls)\n>  {\n> -       auto it = buffers_.find(bufferId);\n> -       if (it == buffers_.end()) {\n> -               LOG(IPARPI, Error) << \"Could not find embedded buffer!\";\n> -               return false;\n> -       }\n> -\n> -       Span<uint8_t> mem = it->second.maps()[0];\n> -       helper_->Parser().SetBufferSize(mem.size());\n> -       RPiController::MdParser::Status status = helper_->Parser().Parse(mem.data());\n> -       if (status != RPiController::MdParser::Status::OK) {\n> -               LOG(IPARPI, Error) << \"Embedded Buffer parsing failed, error \" << status;\n> -               return false;\n> -       } else {\n> -               uint32_t exposureLines, gainCode;\n> -               if (helper_->Parser().GetExposureLines(exposureLines) != RPiController::MdParser::Status::OK) {\n> -                       LOG(IPARPI, Error) << \"Exposure time failed\";\n> -                       return false;\n> -               }\n> -\n> -               if (helper_->Parser().GetGainCode(gainCode) != RPiController::MdParser::Status::OK) {\n> -                       LOG(IPARPI, Error) << \"Gain failed\";\n> -                       return false;\n> -               }\n> -\n> -               fillDeviceStatus(exposureLines, gainCode, deviceStatus);\n> -       }\n> +       struct DeviceStatus deviceStatus = {};\n\n`struct` can be removed here as well\n\n>\n> -       return true;\n> -}\n> +       int32_t exposureLines = sensorControls.get(V4L2_CID_EXPOSURE).get<int32_t>();\n> +       int32_t gainCode = sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get<int32_t>();\n>\n> -void IPARPi::fillDeviceStatus(uint32_t exposureLines, uint32_t gainCode,\n> -                             struct DeviceStatus &deviceStatus)\n> -{\n>         deviceStatus.shutter_speed = helper_->Exposure(exposureLines);\n>         deviceStatus.analogue_gain = helper_->Gain(gainCode);\n>\n> @@ -1014,6 +983,8 @@ void IPARPi::fillDeviceStatus(uint32_t exposureLines, uint32_t gainCode,\n>                            << deviceStatus.shutter_speed\n>                            << \" Gain : \"\n>                            << deviceStatus.analogue_gain;\n> +\n> +       rpiMetadata_.Set(\"device.status\", deviceStatus);\n>  }\n>\n>  void IPARPi::processStats(unsigned int bufferId)\n> @@ -1027,6 +998,7 @@ void IPARPi::processStats(unsigned int bufferId)\n>         Span<uint8_t> mem = it->second.maps()[0];\n>         bcm2835_isp_stats *stats = reinterpret_cast<bcm2835_isp_stats *>(mem.data());\n>         RPiController::StatisticsPtr statistics = std::make_shared<bcm2835_isp_stats>(*stats);\n> +       helper_->Process(statistics, rpiMetadata_);\n>         controller_.Process(statistics, &rpiMetadata_);\n>\n>         struct AgcStatus agcStatus;\n> --\n> 2.20.1\n>\n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","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 5284FBF825\n\tfor <parsemail@patchwork.libcamera.org>;\n\tThu,  6 May 2021 17:18:04 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A9ADB6890C;\n\tThu,  6 May 2021 19:18:03 +0200 (CEST)","from mail-wr1-x42f.google.com (mail-wr1-x42f.google.com\n\t[IPv6:2a00:1450:4864:20::42f])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D9E2268901\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tThu,  6 May 2021 19:18:01 +0200 (CEST)","by mail-wr1-x42f.google.com with SMTP id l14so6441991wrx.5\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tThu, 06 May 2021 10:18:01 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=gmail.com header.i=@gmail.com\n\theader.b=\"bh6B4ctO\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025;\n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=ty6+dfDm1KnT2wqK5DzsMp8fZE0n0kTiW9G8jM4w0M0=;\n\tb=bh6B4ctOHJa4EZYHd5jSYo8qX3VGqYo1SgE9mzPR7XYBhOCzwfktzlyrXniCrXzNTI\n\tVOlr3cO1VDo6bH/jL9M4mCEMcEvZ8DYP55mKzufvyCtKTjUaQMUM4rPWrIPA784y2S5G\n\tpsJu52owK/XK1TVAxXEHDytoG4/9mo8UZwI7VZ/qbt+Xkw+RXuQGF5IHmWWoYcbW0ngl\n\tyQVbO9kfAkhh0Sn/w7wt/y3Ll2DYF92OgOLic9m8h9cr06SW+uL5xo86YlgzkHtXIXgb\n\t98T8Bxg6KYeFK6NqBt/xH0Ac/J6O/ROb1eGf5QAlD7awhoPP5ooL+TBsZ83goZUv4CEY\n\tgDwg==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=ty6+dfDm1KnT2wqK5DzsMp8fZE0n0kTiW9G8jM4w0M0=;\n\tb=Tl6yPVAS91h9yn1OErf8998jnxi96NfRfuY/2LwMpGPqkkX0qftHMnxFe7XjcZ9Vlj\n\tOUZ4VkT6npBnXUsRlu+nGSJGebxWh3l7YTFvo6q+4aD+qBogsDf+ilrhhDYC0v3bT3ZR\n\tJLReTdNrudXiuEl6naHsyTjvB0TjybuUOKYBDy9E2RtjkpoYEEJF6YU08dG3k5KBgb6E\n\ts5yskt6rvZRBeAyputZdmTUWiH9jWbvl1WaEGcxmHQFfDHHQq3lWl5XdnxMiWEDYpbNx\n\t2zongfgIlmc3jWwDueO31nB8G2YUu8lCnSHisamD4qpwo0vU+0uEYXXDHBxHI9iiE1HO\n\tQlcQ==","X-Gm-Message-State":"AOAM531aYxz95vt7cmbmV1mvluogffsUhPMfYGPkk9BwVvAAjZxDGWLi\n\t/Ly/wp+R6dwr5gWWdya2KDHF48xW2t5UoOj8fLfCGNNPzeo=","X-Google-Smtp-Source":"ABdhPJwAx7jpcas+0LrF+hL1MKP4L5eHrrxdln0UBxnOaupds53UUqpBYS7YvQ09ORdIUekPlYYvGMNCsi71+oNkCCw=","X-Received":"by 2002:a05:6000:1ac7:: with SMTP id\n\ti7mr6763088wry.380.1620321481335; \n\tThu, 06 May 2021 10:18:01 -0700 (PDT)","MIME-Version":"1.0","References":"<20210505140438.4042-1-david.plowman@raspberrypi.com>\n\t<20210505140438.4042-2-david.plowman@raspberrypi.com>","In-Reply-To":"<20210505140438.4042-2-david.plowman@raspberrypi.com>","From":"Marvin Schmidt <marvin.schmidt1987@gmail.com>","Date":"Thu, 6 May 2021 19:17:56 +0200","Message-ID":"<CAO-P9RriwXCv-pxOtvAzWZKSUQmjTBw9rV8AW7oSPUQ7Vc82=g@mail.gmail.com>","To":"David Plowman <david.plowman@raspberrypi.com>","Subject":"Re: [libcamera-devel] [PATCH v3 1/1] ipa: raspberrypi: Use\n\tCamHelpers to generalise sensor embedded data parsing","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","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":16825,"web_url":"https://patchwork.libcamera.org/comment/16825/","msgid":"<CAHW6GYJNP8-hwXJ5+cwq9GHKg5qvLW7mdFxRrmrXTVj1xMgevQ@mail.gmail.com>","date":"2021-05-07T09:21:32","subject":"Re: [libcamera-devel] [PATCH v3 1/1] ipa: raspberrypi: Use\n\tCamHelpers to generalise sensor embedded data parsing","submitter":{"id":42,"url":"https://patchwork.libcamera.org/api/people/42/","name":"David Plowman","email":"david.plowman@raspberrypi.com"},"content":"Hi Marvin\n\nThanks for the feedback!\n\nOn Thu, 6 May 2021 at 18:18, Marvin Schmidt\n<marvin.schmidt1987@gmail.com> wrote:\n>\n> Hi David,\n>\n> Thanks for your work on this\n>\n> Am Mi., 5. Mai 2021 um 16:04 Uhr schrieb David Plowman\n> <david.plowman@raspberrypi.com>:\n> >\n> > CamHelpers get virtual Prepare() and Process() methods, running just\n> > before and just after the ISP, just like Raspberry Pi Algorithms.\n> >\n> > The Prepare() method is able to parse the register dumps in embedded\n> > data buffers, and can be specialised to perform custom processing when\n> > necessary.\n> >\n> > The IPA itself only needs to call the new Prepare() and Process()\n> > methods. It must fill in the DeviceStatus from the controls first, in\n> > case the Prepare() method does not supply its own values.\n> >\n> > Signed-off-by: David Plowman <david.plowman@raspberrypi.com>\n> > Reviewed-by: Naushir Patuck <naush@raspberrypi.com>\n> > ---\n> >  src/ipa/raspberrypi/cam_helper.cpp  | 51 ++++++++++++++++++\n> >  src/ipa/raspberrypi/cam_helper.hpp  | 11 +++-\n> >  src/ipa/raspberrypi/raspberrypi.cpp | 80 ++++++++++-------------------\n> >  3 files changed, 87 insertions(+), 55 deletions(-)\n> >\n> > diff --git a/src/ipa/raspberrypi/cam_helper.cpp b/src/ipa/raspberrypi/cam_helper.cpp\n> > index 0ae0baa0..5af73c9c 100644\n> > --- a/src/ipa/raspberrypi/cam_helper.cpp\n> > +++ b/src/ipa/raspberrypi/cam_helper.cpp\n> > @@ -17,6 +17,11 @@\n> >  #include \"md_parser.hpp\"\n> >\n> >  using namespace RPiController;\n> > +using namespace libcamera;\n> > +\n> > +namespace libcamera {\n> > +LOG_DECLARE_CATEGORY(IPARPI)\n> > +}\n> >\n> >  static std::map<std::string, CamHelperCreateFunc> cam_helpers;\n> >\n> > @@ -45,6 +50,17 @@ CamHelper::~CamHelper()\n> >         delete parser_;\n> >  }\n> >\n> > +void CamHelper::Prepare(const Span<uint8_t> &buffer,\n> > +                       Metadata &metadata)\n> > +{\n> > +       parseEmbeddedData(buffer, metadata);\n> > +}\n> > +\n> > +void CamHelper::Process([[maybe_unused]] StatisticsPtr &stats,\n> > +                       [[maybe_unused]] Metadata &metadata)\n> > +{\n> > +}\n> > +\n> >  uint32_t CamHelper::ExposureLines(double exposure_us) const\n> >  {\n> >         assert(initialized_);\n> > @@ -139,6 +155,41 @@ unsigned int CamHelper::MistrustFramesModeSwitch() const\n> >         return 0;\n> >  }\n> >\n> > +void CamHelper::parseEmbeddedData(const Span<uint8_t> &buffer,\n> > +                                 Metadata &metadata)\n> > +{\n> > +       if (buffer.size()) {\n>\n> You can use `Span::empty()` to check if the span is... empty ;-)\n\nYes, thanks, that's nicer!\n\n>\n> Since the function can't do much without some data, I would return\n> immediately in that case\n> and save a level of indentation:\n>\n>     if (buffer.empty())\n>         return;\n>\n>     // do real work\n\nSure, I think that makes sense too.\n\n>\n> > +               bool success = false;\n> > +               uint32_t exposureLines, gainCode;\n> > +\n> > +               parser_->SetBufferSize(buffer.size());\n> > +               success = parser_->Parse(buffer.data()) == MdParser::Status::OK &&\n> > +                         parser_->GetExposureLines(exposureLines) == MdParser::Status::OK &&\n> > +                         parser_->GetGainCode(gainCode) == MdParser::Status::OK;\n> > +\n>\n> I would probably check `success` right here and return early if it\n> wasn't successful\n>\n> > +               /*\n> > +                * Overwrite the exposure/gain values in the DeviceStatus, as\n> > +                * we know better. Fetch it first in case any other fields were\n> > +                * set meaningfully.\n> > +                */\n> > +               struct DeviceStatus deviceStatus;\n>\n> The `struct` isn't needed. And just for good measure, I would\n> zero-initialize it with `= {}`\n\nI tend to put \"struct\" in to remind myself that something is a vanilla\nC data type. But I agree, I can just as easily take it out. I'm\nslightly disinclined to zero-initialise things when it's not necessary\n- if you do it all the time then it can cost actual CPU cycles. But I\ndon't particularly mind if there's a general preference for\nzero-initialising stuff...\n\n>\n> > +\n> > +               if (success &&\n> > +                   metadata.Get(\"device.status\", deviceStatus) == 0) {\n> > +                       deviceStatus.shutter_speed = Exposure(exposureLines);\n> > +                       deviceStatus.analogue_gain = Gain(gainCode);\n> > +\n> > +                       LOG(IPARPI, Debug) << \"Metadata updated - Exposure : \"\n> > +                                          << deviceStatus.shutter_speed\n> > +                                          << \" Gain : \"\n> > +                                          << deviceStatus.analogue_gain;\n> > +\n> > +                       metadata.Set(\"device.status\", deviceStatus);\n> > +               } else\n> > +                       LOG(IPARPI, Error) << \"Embedded buffer parsing failed\";\n>\n> If we check `success` right after the parsing step and bail out if it\n> wasn't successful, this could be written\n> like:\n>\n>     if (metadata.Get(\"device.status\", deviceStatus) != 0) {\n>         LOG(IPARPI, Error) << \"Could not get device status from metadata\";\n>         return;\n>     }\n>\n>     deviceStatus.shutter_speed = Exposure(exposureLines);\n>     deviceStatus.analogue_gain = Gain(gainCode);\n>     [...]\n>\n> I guess it's good to have a different error message for the\n> `metadata.Get(\"device.status\", deviceStatus) != 0` case\n> anyway, since it's not really related to the parsing of the embedded data?\n>\n> > +       }\n> > +}\n> > +\n> >  RegisterCamHelper::RegisterCamHelper(char const *cam_name,\n> >                                      CamHelperCreateFunc create_func)\n> >  {\n> > diff --git a/src/ipa/raspberrypi/cam_helper.hpp b/src/ipa/raspberrypi/cam_helper.hpp\n> > index c3ed5362..6ee5051c 100644\n> > --- a/src/ipa/raspberrypi/cam_helper.hpp\n> > +++ b/src/ipa/raspberrypi/cam_helper.hpp\n> > @@ -8,7 +8,11 @@\n> >\n> >  #include <string>\n> >\n> > +#include <libcamera/span.h>\n> > +\n> >  #include \"camera_mode.h\"\n> > +#include \"controller/controller.hpp\"\n> > +#include \"controller/metadata.hpp\"\n> >  #include \"md_parser.hpp\"\n> >\n> >  #include \"libcamera/internal/v4l2_videodevice.h\"\n> > @@ -65,7 +69,9 @@ public:\n> >         CamHelper(MdParser *parser, unsigned int frameIntegrationDiff);\n> >         virtual ~CamHelper();\n> >         void SetCameraMode(const CameraMode &mode);\n> > -       MdParser &Parser() const { return *parser_; }\n> > +       virtual void Prepare(const libcamera::Span<uint8_t> &buffer,\n> > +                            Metadata &metadata);\n>\n> The CppCoreGuidelines suggest passing spans by value:\n>\n>     Note: A span<T> object does not own its elements and is so small\n> that it can be passed by value.\n>     Passing a span object as an argument is exactly as efficient as\n> passing a pair of pointer arguments\n>     or passing a pointer and an integer count.\n>\n> https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#f24-use-a-spant-or-a-span_pt-to-designate-a-half-open-sequence\n\nThanks, I hadn't read that. I guess it doesn't formally tell us about\nthe merits of passing a Span by reference vs. passing it by value, but\nit's tiny so hardly matters. Looking through libcamera, I would say\nthey're mostly passed by value so I can copy that pattern too.\n\nAnyway, thanks for all those suggestions. I'll make those various\nchanges and submit another version.\n\nBest regards\nDavid\n\n>\n> So this could be `libcamera::Span<const uint8_t> buffer` since the\n> function just needs read-only access to the\n> buffer data. This would require changing the signature of\n> `MdParser::Parse()` though. Either to `const void *` or\n> better yet `libcamera::Span<const uint8_t>` as well. Which would allow\n> removing the `static_cast<uint8_t *>(data)`\n> inside the `MdParser::Parse()` implementations.\n>\n> > +       virtual void Process(StatisticsPtr &stats, Metadata &metadata);\n> >         uint32_t ExposureLines(double exposure_us) const;\n> >         double Exposure(uint32_t exposure_lines) const; // in us\n> >         virtual uint32_t GetVBlanking(double &exposure_us, double minFrameDuration,\n> > @@ -81,6 +87,9 @@ public:\n> >         virtual unsigned int MistrustFramesModeSwitch() const;\n> >\n> >  protected:\n> > +       void parseEmbeddedData(const libcamera::Span<uint8_t> &buffer,\n> > +                              Metadata &metadata);\n> > +\n> >         MdParser *parser_;\n> >         CameraMode mode_;\n> >\n> > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp\n> > index dad6395f..b0f61d35 100644\n> > --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> > +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> > @@ -101,9 +101,7 @@ private:\n> >         void returnEmbeddedBuffer(unsigned int bufferId);\n> >         void prepareISP(const ipa::RPi::ISPConfig &data);\n> >         void reportMetadata();\n> > -       bool parseEmbeddedData(unsigned int bufferId, struct DeviceStatus &deviceStatus);\n> > -       void fillDeviceStatus(uint32_t exposureLines, uint32_t gainCode,\n> > -                             struct DeviceStatus &deviceStatus);\n> > +       void fillDeviceStatus(const ControlList &sensorControls);\n> >         void processStats(unsigned int bufferId);\n> >         void applyFrameDurations(double minFrameDuration, double maxFrameDuration);\n> >         void applyAGC(const struct AgcStatus *agcStatus, ControlList &ctrls);\n> > @@ -894,35 +892,34 @@ void IPARPi::returnEmbeddedBuffer(unsigned int bufferId)\n> >\n> >  void IPARPi::prepareISP(const ipa::RPi::ISPConfig &data)\n> >  {\n> > -       struct DeviceStatus deviceStatus = {};\n> > -       bool success = false;\n> > +       Span<uint8_t> embeddedBuffer;\n>\n> This can be `Span<const uint8_t>`\n>\n> > +\n> > +       rpiMetadata_.Clear();\n> > +\n> > +       fillDeviceStatus(data.controls);\n> >\n> >         if (data.embeddedBufferPresent) {\n> >                 /*\n> >                  * Pipeline handler has supplied us with an embedded data buffer,\n> > -                * so parse it and extract the exposure and gain.\n> > +                * we must pass it to the CamHelper for parsing.\n> >                  */\n> > -               success = parseEmbeddedData(data.embeddedBufferId, deviceStatus);\n> > -\n> > -               /* Done with embedded data now, return to pipeline handler asap. */\n> > -               returnEmbeddedBuffer(data.embeddedBufferId);\n> > +               auto it = buffers_.find(data.embeddedBufferId);\n> > +               ASSERT(it != buffers_.end());\n> > +               embeddedBuffer = it->second.maps()[0];\n> >         }\n> >\n> > -       if (!success) {\n> > -               /*\n> > -                * Pipeline handler has not supplied an embedded data buffer,\n> > -                * or embedded data buffer parsing has failed for some reason,\n> > -                * so pull the exposure and gain values from the control list.\n> > -                */\n> > -               int32_t exposureLines = data.controls.get(V4L2_CID_EXPOSURE).get<int32_t>();\n> > -               int32_t gainCode = data.controls.get(V4L2_CID_ANALOGUE_GAIN).get<int32_t>();\n> > -               fillDeviceStatus(exposureLines, gainCode, deviceStatus);\n> > -       }\n> > +       /*\n> > +        * This may overwrite the DeviceStatus using values from the sensor\n> > +        * metadata, and may also do additional custom processing.\n> > +        */\n> > +       helper_->Prepare(embeddedBuffer, rpiMetadata_);\n> > +\n> > +       /* Done with embedded data now, return to pipeline handler asap. */\n> > +       if (data.embeddedBufferPresent)\n> > +               returnEmbeddedBuffer(data.embeddedBufferId);\n> >\n> >         ControlList ctrls(ispCtrls_);\n> >\n> > -       rpiMetadata_.Clear();\n> > -       rpiMetadata_.Set(\"device.status\", deviceStatus);\n> >         controller_.Prepare(&rpiMetadata_);\n> >\n> >         /* Lock the metadata buffer to avoid constant locks/unlocks. */\n> > @@ -972,41 +969,13 @@ void IPARPi::prepareISP(const ipa::RPi::ISPConfig &data)\n> >                 setIspControls.emit(ctrls);\n> >  }\n> >\n> > -bool IPARPi::parseEmbeddedData(unsigned int bufferId, struct DeviceStatus &deviceStatus)\n> > +void IPARPi::fillDeviceStatus(const ControlList &sensorControls)\n> >  {\n> > -       auto it = buffers_.find(bufferId);\n> > -       if (it == buffers_.end()) {\n> > -               LOG(IPARPI, Error) << \"Could not find embedded buffer!\";\n> > -               return false;\n> > -       }\n> > -\n> > -       Span<uint8_t> mem = it->second.maps()[0];\n> > -       helper_->Parser().SetBufferSize(mem.size());\n> > -       RPiController::MdParser::Status status = helper_->Parser().Parse(mem.data());\n> > -       if (status != RPiController::MdParser::Status::OK) {\n> > -               LOG(IPARPI, Error) << \"Embedded Buffer parsing failed, error \" << status;\n> > -               return false;\n> > -       } else {\n> > -               uint32_t exposureLines, gainCode;\n> > -               if (helper_->Parser().GetExposureLines(exposureLines) != RPiController::MdParser::Status::OK) {\n> > -                       LOG(IPARPI, Error) << \"Exposure time failed\";\n> > -                       return false;\n> > -               }\n> > -\n> > -               if (helper_->Parser().GetGainCode(gainCode) != RPiController::MdParser::Status::OK) {\n> > -                       LOG(IPARPI, Error) << \"Gain failed\";\n> > -                       return false;\n> > -               }\n> > -\n> > -               fillDeviceStatus(exposureLines, gainCode, deviceStatus);\n> > -       }\n> > +       struct DeviceStatus deviceStatus = {};\n>\n> `struct` can be removed here as well\n>\n> >\n> > -       return true;\n> > -}\n> > +       int32_t exposureLines = sensorControls.get(V4L2_CID_EXPOSURE).get<int32_t>();\n> > +       int32_t gainCode = sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get<int32_t>();\n> >\n> > -void IPARPi::fillDeviceStatus(uint32_t exposureLines, uint32_t gainCode,\n> > -                             struct DeviceStatus &deviceStatus)\n> > -{\n> >         deviceStatus.shutter_speed = helper_->Exposure(exposureLines);\n> >         deviceStatus.analogue_gain = helper_->Gain(gainCode);\n> >\n> > @@ -1014,6 +983,8 @@ void IPARPi::fillDeviceStatus(uint32_t exposureLines, uint32_t gainCode,\n> >                            << deviceStatus.shutter_speed\n> >                            << \" Gain : \"\n> >                            << deviceStatus.analogue_gain;\n> > +\n> > +       rpiMetadata_.Set(\"device.status\", deviceStatus);\n> >  }\n> >\n> >  void IPARPi::processStats(unsigned int bufferId)\n> > @@ -1027,6 +998,7 @@ void IPARPi::processStats(unsigned int bufferId)\n> >         Span<uint8_t> mem = it->second.maps()[0];\n> >         bcm2835_isp_stats *stats = reinterpret_cast<bcm2835_isp_stats *>(mem.data());\n> >         RPiController::StatisticsPtr statistics = std::make_shared<bcm2835_isp_stats>(*stats);\n> > +       helper_->Process(statistics, rpiMetadata_);\n> >         controller_.Process(statistics, &rpiMetadata_);\n> >\n> >         struct AgcStatus agcStatus;\n> > --\n> > 2.20.1\n> >\n> > _______________________________________________\n> > libcamera-devel mailing list\n> > libcamera-devel@lists.libcamera.org\n> > https://lists.libcamera.org/listinfo/libcamera-devel","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 B38BABF829\n\tfor <parsemail@patchwork.libcamera.org>;\n\tFri,  7 May 2021 09:21:45 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 28D446890E;\n\tFri,  7 May 2021 11:21:45 +0200 (CEST)","from mail-wr1-x429.google.com (mail-wr1-x429.google.com\n\t[IPv6:2a00:1450:4864:20::429])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 32DCA688E4\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tFri,  7 May 2021 11:21:44 +0200 (CEST)","by mail-wr1-x429.google.com with SMTP id s8so8438990wrw.10\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tFri, 07 May 2021 02:21:44 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=raspberrypi.com header.i=@raspberrypi.com\n\theader.b=\"TwgEEE26\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google;\n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=oma9hIu+6NvxpHm1Vxan9GuGahffvnDbgZLGnm7SRLA=;\n\tb=TwgEEE2656iQCusZm/JCKUwROT8ASCcLeydb0cAb0mykg/f7jJio1pKFJXjZ7sa/Di\n\taFPi2N/x5XR8PcZ+1/3E/Xm3EDOVPNhWjbnoFuH6hYbAFvps4n1hpoZjHplzN7yj2lq4\n\tmG8h5a9HYo6LnrXPYHMmyn7WIGO7wnLmsi03UE8YyOw+ehyE4yANu3GrbVyWB2cJnbAA\n\tWKfUwe+GfpSwhkcgIyyhD+sHkSK1qpIHCir6KNVPAgIdgecp+uhW/RbX7c7Af3awR+AW\n\teGWEoFrQ9jZltbrBdzQIot4Y/w1NBb38FmmyQCutCW1CExrvWOPIahuHs5zDQZLPh/dE\n\tRSKA==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=oma9hIu+6NvxpHm1Vxan9GuGahffvnDbgZLGnm7SRLA=;\n\tb=gbZzirrmpotfr0ugMlbkZT5+pEjT7gYbgX/De2A0PfQ3HScqjI6HYPCuUnb3A2zxko\n\tDKaxG/gSYn/79VSHULsnslzC7X7Nk0HLnnbyKNrB4J/MuZMia6DW/RZeIpDIWFjFnKwP\n\tjYKHi7rkkEXTGJONvrNqdbW26K9uwWj9YTkCixJ9qu1dqnGhcJtY8/OOhn2gLwMzwES6\n\trop9Kfwro7mNDD2UzZ3hbvvuTz3NxafIhqmaDMBaQNPa8VE5mchaktjwITTHvuobbOg2\n\t5LGH910ouqGXawEqUosrDQ9KtfxvBVPq5IQURjxG0kHn29r0tI0lrdfcgQ7EuDsDhfJz\n\tSKwA==","X-Gm-Message-State":"AOAM531ioCmcAe/EBuKCphL3zl809V6LugYKj4q6H/XDCxNOWuIfhuQE\n\t3pW1w5pqgmCagFmaLV1pyjaxXOigGdcMa44vLj9lkg==","X-Google-Smtp-Source":"ABdhPJzhJ9CMkjXD6ze1fQ1OWzdRLRSrjI0wgjc0p2Q6/zYeqvZLWjCCWgKaQqwrYkqAAPPIWPNo7Y7rIoFGZ8HOBnI=","X-Received":"by 2002:adf:e3c6:: with SMTP id\n\tk6mr11000794wrm.236.1620379303710; \n\tFri, 07 May 2021 02:21:43 -0700 (PDT)","MIME-Version":"1.0","References":"<20210505140438.4042-1-david.plowman@raspberrypi.com>\n\t<20210505140438.4042-2-david.plowman@raspberrypi.com>\n\t<CAO-P9RriwXCv-pxOtvAzWZKSUQmjTBw9rV8AW7oSPUQ7Vc82=g@mail.gmail.com>","In-Reply-To":"<CAO-P9RriwXCv-pxOtvAzWZKSUQmjTBw9rV8AW7oSPUQ7Vc82=g@mail.gmail.com>","From":"David Plowman <david.plowman@raspberrypi.com>","Date":"Fri, 7 May 2021 10:21:32 +0100","Message-ID":"<CAHW6GYJNP8-hwXJ5+cwq9GHKg5qvLW7mdFxRrmrXTVj1xMgevQ@mail.gmail.com>","To":"Marvin Schmidt <marvin.schmidt1987@gmail.com>","Subject":"Re: [libcamera-devel] [PATCH v3 1/1] ipa: raspberrypi: Use\n\tCamHelpers to generalise sensor embedded data parsing","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 <libcamera-devel@lists.libcamera.org>","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}}]