[{"id":25527,"web_url":"https://patchwork.libcamera.org/comment/25527/","msgid":"<20221024020220.GU3874866@pyrite.rasen.tech>","date":"2022-10-24T02:02:20","subject":"Re: [libcamera-devel] [PATCH v3 03/13] ipa: rkisp1: Support raw\n\tcapture in IPA operations","submitter":{"id":97,"url":"https://patchwork.libcamera.org/api/people/97/","name":"Nicolas Dufresne via libcamera-devel","email":"libcamera-devel@lists.libcamera.org"},"content":"On Mon, Oct 24, 2022 at 03:03:46AM +0300, Laurent Pinchart wrote:\n> The RkISP1 can capture raw frames by bypassing the ISP. In that mode,\n> the ISP will not produce statistics nor consume parameters. Most\n> algorithms will thus be unable to run, with one exception: the AGC will\n> still be able to configure the sensor exposure time and analog gain in\n> manual mode.\n> \n> To prepare for this, add the ability to disable algorithms for the\n> duration of the capture session based on the camera configuration.\n> Individual algorithms report whether they support raw formats at\n> construction time, and the IPA module disables algorithms in configure()\n> based on the stream configurations.\n> \n> Disabled algorithms are skipped during the capture session in the\n> processStatsBuffer() operation. As the ISP doesn't produce statistics,\n> don't try to access the stats buffer. There is no need for similar logic\n> in fillParamsBuffer() as that operation won't be called for raw capture.\n> \n> All algorithms report not supporting raw capture by default. Raw support\n> in AGC will be added separately.\n> \n> The feature is implemented in the RkISP1 module without any support from\n> libipa at this point to avoid designing a generic API based on a single\n> user. This may be changed in the future.\n> \n> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\n> ---\n>  src/ipa/rkisp1/algorithms/algorithm.h | 12 +++++++-\n>  src/ipa/rkisp1/ipa_context.cpp        |  5 ++++\n>  src/ipa/rkisp1/ipa_context.h          |  2 ++\n>  src/ipa/rkisp1/rkisp1.cpp             | 42 +++++++++++++++++++++++----\n>  4 files changed, 54 insertions(+), 7 deletions(-)\n> \n> diff --git a/src/ipa/rkisp1/algorithms/algorithm.h b/src/ipa/rkisp1/algorithms/algorithm.h\n> index c3212cff76fe..9454c9a1fc06 100644\n> --- a/src/ipa/rkisp1/algorithms/algorithm.h\n> +++ b/src/ipa/rkisp1/algorithms/algorithm.h\n> @@ -15,7 +15,17 @@ namespace libcamera {\n>  \n>  namespace ipa::rkisp1 {\n>  \n> -using Algorithm = libcamera::ipa::Algorithm<Module>;\n> +class Algorithm : public libcamera::ipa::Algorithm<Module>\n> +{\n> +public:\n> +\tAlgorithm()\n> +\t\t: disabled_(false), supportsRaw_(false)\n> +\t{\n> +\t}\n> +\n> +\tbool disabled_;\n> +\tbool supportsRaw_;\n> +};\n>  \n>  } /* namespace ipa::rkisp1 */\n>  \n> diff --git a/src/ipa/rkisp1/ipa_context.cpp b/src/ipa/rkisp1/ipa_context.cpp\n> index 7a987497bd03..9bbf368432fa 100644\n> --- a/src/ipa/rkisp1/ipa_context.cpp\n> +++ b/src/ipa/rkisp1/ipa_context.cpp\n> @@ -89,6 +89,11 @@ namespace libcamera::ipa::rkisp1 {\n>   * \\brief Sensor output resolution\n>   */\n>  \n> +/**\n> + * \\var IPASessionConfiguration::raw\n> + * \\brief Indicates if the camera is configured to capture raw frames\n> + */\n> +\n>  /**\n>   * \\struct IPAActiveState\n>   * \\brief Active state for algorithms\n> diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h\n> index bb60ab9eab72..3e47ac663c58 100644\n> --- a/src/ipa/rkisp1/ipa_context.h\n> +++ b/src/ipa/rkisp1/ipa_context.h\n> @@ -48,6 +48,8 @@ struct IPASessionConfiguration {\n>  \tstruct {\n>  \t\trkisp1_cif_isp_version revision;\n>  \t} hw;\n> +\n> +\tbool raw;\n>  };\n>  \n>  struct IPAActiveState {\n> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp\n> index fcb9dacccc3c..538e42cb6f7f 100644\n> --- a/src/ipa/rkisp1/rkisp1.cpp\n> +++ b/src/ipa/rkisp1/rkisp1.cpp\n> @@ -24,6 +24,7 @@\n>  #include <libcamera/ipa/rkisp1_ipa_interface.h>\n>  #include <libcamera/request.h>\n>  \n> +#include \"libcamera/internal/formats.h\"\n>  #include \"libcamera/internal/mapped_framebuffer.h\"\n>  #include \"libcamera/internal/yaml_parser.h\"\n>  \n> @@ -207,7 +208,7 @@ void IPARkISP1::stop()\n>   * before accessing them.\n>   */\n>  int IPARkISP1::configure([[maybe_unused]] const IPACameraSensorInfo &info,\n> -\t\t\t [[maybe_unused]] const std::map<uint32_t, IPAStream> &streamConfig,\n> +\t\t\t const std::map<uint32_t, IPAStream> &streamConfig,\n>  \t\t\t const std::map<uint32_t, ControlInfoMap> &entityControls)\n>  {\n>  \tif (entityControls.empty())\n> @@ -264,7 +265,21 @@ int IPARkISP1::configure([[maybe_unused]] const IPACameraSensorInfo &info,\n>  \tcontext_.configuration.sensor.minAnalogueGain = camHelper_->gain(minGain);\n>  \tcontext_.configuration.sensor.maxAnalogueGain = camHelper_->gain(maxGain);\n>  \n> -\tfor (auto const &algo : algorithms()) {\n> +\tcontext_.configuration.raw = std::any_of(streamConfig.begin(), streamConfig.end(),\n> +\t\t[](auto &cfg) -> bool {\n> +\t\t\tPixelFormat pixelFormat{ cfg.second.pixelFormat };\n> +\t\t\tconst PixelFormatInfo &format = PixelFormatInfo::info(pixelFormat);\n> +\t\t\treturn format.colourEncoding == PixelFormatInfo::ColourEncodingRAW;\n> +\t\t});\n> +\n> +\tfor (auto const &a : algorithms()) {\n> +\t\tAlgorithm *algo = static_cast<Algorithm *>(a.get());\n> +\n> +\t\t/* Disable algorithms that don't support raw formats. */\n> +\t\talgo->disabled_ = context_.configuration.raw && !algo->supportsRaw_;\n> +\t\tif (algo->disabled_)\n> +\t\t\tcontinue;\n> +\n\nOn one hand I was thinking if the algorithms should internally nop if\nthey're in raw mode, but I think that indeed it's the job of the IPA to\ncontrol its algorithms based on its own config, so this is fine.\n\n>  \t\tint ret = algo->configure(context_, info);\n>  \t\tif (ret)\n>  \t\t\treturn ret;\n> @@ -307,8 +322,12 @@ void IPARkISP1::queueRequest(const uint32_t frame, const ControlList &controls)\n>  {\n>  \tIPAFrameContext &frameContext = context_.frameContexts.alloc(frame);\n>  \n> -\tfor (auto const &algo : algorithms())\n> +\tfor (auto const &a : algorithms()) {\n> +\t\tAlgorithm *algo = static_cast<Algorithm *>(a.get());\n> +\t\tif (algo->disabled_)\n> +\t\t\tcontinue;\n>  \t\talgo->queueRequest(context_, frame, frameContext, controls);\n> +\t}\n>  }\n>  \n>  void IPARkISP1::fillParamsBuffer(const uint32_t frame, const uint32_t bufferId)\n> @@ -333,9 +352,16 @@ void IPARkISP1::processStatsBuffer(const uint32_t frame, const uint32_t bufferId\n>  {\n>  \tIPAFrameContext &frameContext = context_.frameContexts.get(frame);\n>  \n> -\tconst rkisp1_stat_buffer *stats =\n> -\t\treinterpret_cast<rkisp1_stat_buffer *>(\n> +\t/*\n> +\t * In raw capture mode, the ISP is bypassed and no statistics buffer is\n> +\t * provided.\n> +\t */\n> +\tconst rkisp1_stat_buffer *stats;\n> +\tif (!context_.configuration.raw)\n> +\t\tstats = reinterpret_cast<rkisp1_stat_buffer *>(\n>  \t\t\tmappedBuffers_.at(bufferId).planes()[0].data());\n> +\telse\n> +\t\tstats = nullptr;\n\nprocessStatsBuffer() won't ever be called in raw mode *except* for\n(before the pipeline hander is adjusted (I expect it to be later in the\nseries)) the first buffer that got queued at start time, and which would\nbe bufferReady-dequeued at streamoff. At least this protects against\ndereferencing the null stats buffer, plus we don't report supporting raw\nformats yet anyway, so I guess it's fine.\n\nJust thinking aloud :)\n\n\nReviewed-by: Paul Elder <paul.elder@ideasonboard.com>\n\n>  \n>  \tframeContext.sensor.exposure =\n>  \t\tsensorControls.get(V4L2_CID_EXPOSURE).get<int32_t>();\n> @@ -344,8 +370,12 @@ void IPARkISP1::processStatsBuffer(const uint32_t frame, const uint32_t bufferId\n>  \n>  \tControlList metadata(controls::controls);\n>  \n> -\tfor (auto const &algo : algorithms())\n> +\tfor (auto const &a : algorithms()) {\n> +\t\tAlgorithm *algo = static_cast<Algorithm *>(a.get());\n> +\t\tif (algo->disabled_)\n> +\t\t\tcontinue;\n>  \t\talgo->process(context_, frame, frameContext, stats, metadata);\n> +\t}\n>  \n>  \tsetControls(frame);\n>  \n> -- \n> Regards,\n> \n> Laurent Pinchart\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 5821DBDB16\n\tfor <parsemail@patchwork.libcamera.org>;\n\tMon, 24 Oct 2022 02:02:31 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B09D762EDE;\n\tMon, 24 Oct 2022 04:02:30 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 28CC662EA6\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon, 24 Oct 2022 04:02:29 +0200 (CEST)","from pyrite.rasen.tech (h175-177-042-159.catv02.itscom.jp\n\t[175.177.42.159])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 0C8BE471;\n\tMon, 24 Oct 2022 04:02:26 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1666576950;\n\tbh=V3rJkulmO8QGmGHKGMtVaMNUpXcVocRqoBiHZyy77wI=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=BqNSlE2f1Ug+bOHv4Y1Ig0sA77gJZUtAWkG8x0b6b8PmCEirAuqDejEDxO8EzSloz\n\tcQHgBhN7k2hb770R+din1AdQI2eVnXE8wNwTTOYH/jgrrHiA8cGZt3DNEIx4MFWExz\n\twfEbhwvCD4kE9ef7ispnuCGl9R++9zZiVYKLdRxf+UBKwD9i8/2FHKyTOEN5JWr8EF\n\td6sVGzdHMCyB+8oUhRUWQmazduJtQQxkrJhIhVCQWivQRzKnyhS4KObrC3EG0OCovX\n\tOEB9w+B/VzZGirOdOQy6Qyw/XOtLKHjJT5T/NyK4JqeFf/kBl3tSALj4w6GhOFF2NW\n\tq99vBZf0uj2nQ==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1666576948;\n\tbh=V3rJkulmO8QGmGHKGMtVaMNUpXcVocRqoBiHZyy77wI=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=UMdoHLfo/BKzJLEkI/0A9AJ8KnUXMQUz2PTNGMsTlXE18n0/JDKjSiqS58yNd3XoX\n\tSnjRyhZSIjPb23UT+xDu/GgyTrOm/hJKQytHPut8BxLcbucn9PHZY1UnmRuyC3YrdZ\n\tnGhxbDOk20UgZVUzdFMjx1s0sa3SEtsb2U74jeP0="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"UMdoHLfo\"; dkim-atps=neutral","Date":"Mon, 24 Oct 2022 11:02:20 +0900","To":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","Message-ID":"<20221024020220.GU3874866@pyrite.rasen.tech>","References":"<20221024000356.29521-1-laurent.pinchart@ideasonboard.com>\n\t<20221024000356.29521-4-laurent.pinchart@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<20221024000356.29521-4-laurent.pinchart@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v3 03/13] ipa: rkisp1: Support raw\n\tcapture in IPA operations","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>","From":"Paul Elder via libcamera-devel <libcamera-devel@lists.libcamera.org>","Reply-To":"paul.elder@ideasonboard.com","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":25531,"web_url":"https://patchwork.libcamera.org/comment/25531/","msgid":"<Y1ZDA9PjJExBKgxa@pendragon.ideasonboard.com>","date":"2022-10-24T07:47:15","subject":"Re: [libcamera-devel] [PATCH v3 03/13] ipa: rkisp1: Support raw\n\tcapture in IPA operations","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Paul,\n\nOn Mon, Oct 24, 2022 at 11:02:20AM +0900, paul.elder@ideasonboard.com wrote:\n> On Mon, Oct 24, 2022 at 03:03:46AM +0300, Laurent Pinchart wrote:\n> > The RkISP1 can capture raw frames by bypassing the ISP. In that mode,\n> > the ISP will not produce statistics nor consume parameters. Most\n> > algorithms will thus be unable to run, with one exception: the AGC will\n> > still be able to configure the sensor exposure time and analog gain in\n> > manual mode.\n> > \n> > To prepare for this, add the ability to disable algorithms for the\n> > duration of the capture session based on the camera configuration.\n> > Individual algorithms report whether they support raw formats at\n> > construction time, and the IPA module disables algorithms in configure()\n> > based on the stream configurations.\n> > \n> > Disabled algorithms are skipped during the capture session in the\n> > processStatsBuffer() operation. As the ISP doesn't produce statistics,\n> > don't try to access the stats buffer. There is no need for similar logic\n> > in fillParamsBuffer() as that operation won't be called for raw capture.\n> > \n> > All algorithms report not supporting raw capture by default. Raw support\n> > in AGC will be added separately.\n> > \n> > The feature is implemented in the RkISP1 module without any support from\n> > libipa at this point to avoid designing a generic API based on a single\n> > user. This may be changed in the future.\n> > \n> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\n> > ---\n> >  src/ipa/rkisp1/algorithms/algorithm.h | 12 +++++++-\n> >  src/ipa/rkisp1/ipa_context.cpp        |  5 ++++\n> >  src/ipa/rkisp1/ipa_context.h          |  2 ++\n> >  src/ipa/rkisp1/rkisp1.cpp             | 42 +++++++++++++++++++++++----\n> >  4 files changed, 54 insertions(+), 7 deletions(-)\n> > \n> > diff --git a/src/ipa/rkisp1/algorithms/algorithm.h b/src/ipa/rkisp1/algorithms/algorithm.h\n> > index c3212cff76fe..9454c9a1fc06 100644\n> > --- a/src/ipa/rkisp1/algorithms/algorithm.h\n> > +++ b/src/ipa/rkisp1/algorithms/algorithm.h\n> > @@ -15,7 +15,17 @@ namespace libcamera {\n> >  \n> >  namespace ipa::rkisp1 {\n> >  \n> > -using Algorithm = libcamera::ipa::Algorithm<Module>;\n> > +class Algorithm : public libcamera::ipa::Algorithm<Module>\n> > +{\n> > +public:\n> > +\tAlgorithm()\n> > +\t\t: disabled_(false), supportsRaw_(false)\n> > +\t{\n> > +\t}\n> > +\n> > +\tbool disabled_;\n> > +\tbool supportsRaw_;\n> > +};\n> >  \n> >  } /* namespace ipa::rkisp1 */\n> >  \n> > diff --git a/src/ipa/rkisp1/ipa_context.cpp b/src/ipa/rkisp1/ipa_context.cpp\n> > index 7a987497bd03..9bbf368432fa 100644\n> > --- a/src/ipa/rkisp1/ipa_context.cpp\n> > +++ b/src/ipa/rkisp1/ipa_context.cpp\n> > @@ -89,6 +89,11 @@ namespace libcamera::ipa::rkisp1 {\n> >   * \\brief Sensor output resolution\n> >   */\n> >  \n> > +/**\n> > + * \\var IPASessionConfiguration::raw\n> > + * \\brief Indicates if the camera is configured to capture raw frames\n> > + */\n> > +\n> >  /**\n> >   * \\struct IPAActiveState\n> >   * \\brief Active state for algorithms\n> > diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h\n> > index bb60ab9eab72..3e47ac663c58 100644\n> > --- a/src/ipa/rkisp1/ipa_context.h\n> > +++ b/src/ipa/rkisp1/ipa_context.h\n> > @@ -48,6 +48,8 @@ struct IPASessionConfiguration {\n> >  \tstruct {\n> >  \t\trkisp1_cif_isp_version revision;\n> >  \t} hw;\n> > +\n> > +\tbool raw;\n> >  };\n> >  \n> >  struct IPAActiveState {\n> > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp\n> > index fcb9dacccc3c..538e42cb6f7f 100644\n> > --- a/src/ipa/rkisp1/rkisp1.cpp\n> > +++ b/src/ipa/rkisp1/rkisp1.cpp\n> > @@ -24,6 +24,7 @@\n> >  #include <libcamera/ipa/rkisp1_ipa_interface.h>\n> >  #include <libcamera/request.h>\n> >  \n> > +#include \"libcamera/internal/formats.h\"\n> >  #include \"libcamera/internal/mapped_framebuffer.h\"\n> >  #include \"libcamera/internal/yaml_parser.h\"\n> >  \n> > @@ -207,7 +208,7 @@ void IPARkISP1::stop()\n> >   * before accessing them.\n> >   */\n> >  int IPARkISP1::configure([[maybe_unused]] const IPACameraSensorInfo &info,\n> > -\t\t\t [[maybe_unused]] const std::map<uint32_t, IPAStream> &streamConfig,\n> > +\t\t\t const std::map<uint32_t, IPAStream> &streamConfig,\n> >  \t\t\t const std::map<uint32_t, ControlInfoMap> &entityControls)\n> >  {\n> >  \tif (entityControls.empty())\n> > @@ -264,7 +265,21 @@ int IPARkISP1::configure([[maybe_unused]] const IPACameraSensorInfo &info,\n> >  \tcontext_.configuration.sensor.minAnalogueGain = camHelper_->gain(minGain);\n> >  \tcontext_.configuration.sensor.maxAnalogueGain = camHelper_->gain(maxGain);\n> >  \n> > -\tfor (auto const &algo : algorithms()) {\n> > +\tcontext_.configuration.raw = std::any_of(streamConfig.begin(), streamConfig.end(),\n> > +\t\t[](auto &cfg) -> bool {\n> > +\t\t\tPixelFormat pixelFormat{ cfg.second.pixelFormat };\n> > +\t\t\tconst PixelFormatInfo &format = PixelFormatInfo::info(pixelFormat);\n> > +\t\t\treturn format.colourEncoding == PixelFormatInfo::ColourEncodingRAW;\n> > +\t\t});\n> > +\n> > +\tfor (auto const &a : algorithms()) {\n> > +\t\tAlgorithm *algo = static_cast<Algorithm *>(a.get());\n> > +\n> > +\t\t/* Disable algorithms that don't support raw formats. */\n> > +\t\talgo->disabled_ = context_.configuration.raw && !algo->supportsRaw_;\n> > +\t\tif (algo->disabled_)\n> > +\t\t\tcontinue;\n> > +\n> \n> On one hand I was thinking if the algorithms should internally nop if\n> they're in raw mode, but I think that indeed it's the job of the IPA to\n> control its algorithms based on its own config, so this is fine.\n> \n> >  \t\tint ret = algo->configure(context_, info);\n> >  \t\tif (ret)\n> >  \t\t\treturn ret;\n> > @@ -307,8 +322,12 @@ void IPARkISP1::queueRequest(const uint32_t frame, const ControlList &controls)\n> >  {\n> >  \tIPAFrameContext &frameContext = context_.frameContexts.alloc(frame);\n> >  \n> > -\tfor (auto const &algo : algorithms())\n> > +\tfor (auto const &a : algorithms()) {\n> > +\t\tAlgorithm *algo = static_cast<Algorithm *>(a.get());\n> > +\t\tif (algo->disabled_)\n> > +\t\t\tcontinue;\n> >  \t\talgo->queueRequest(context_, frame, frameContext, controls);\n> > +\t}\n> >  }\n> >  \n> >  void IPARkISP1::fillParamsBuffer(const uint32_t frame, const uint32_t bufferId)\n> > @@ -333,9 +352,16 @@ void IPARkISP1::processStatsBuffer(const uint32_t frame, const uint32_t bufferId\n> >  {\n> >  \tIPAFrameContext &frameContext = context_.frameContexts.get(frame);\n> >  \n> > -\tconst rkisp1_stat_buffer *stats =\n> > -\t\treinterpret_cast<rkisp1_stat_buffer *>(\n> > +\t/*\n> > +\t * In raw capture mode, the ISP is bypassed and no statistics buffer is\n> > +\t * provided.\n> > +\t */\n> > +\tconst rkisp1_stat_buffer *stats;\n> > +\tif (!context_.configuration.raw)\n> > +\t\tstats = reinterpret_cast<rkisp1_stat_buffer *>(\n> >  \t\t\tmappedBuffers_.at(bufferId).planes()[0].data());\n> > +\telse\n> > +\t\tstats = nullptr;\n> \n> processStatsBuffer() won't ever be called in raw mode *except* for\n> (before the pipeline hander is adjusted (I expect it to be later in the\n> series)) the first buffer that got queued at start time, and which would\n> be bufferReady-dequeued at streamoff. At least this protects against\n> dereferencing the null stats buffer, plus we don't report supporting raw\n> formats yet anyway, so I guess it's fine.\n\nThe function will actually be called to fill metadata for the request.\nIt will not receive statistics, but it will receive the effective values\nof the camera sensor controls for that frame.\n\n> Just thinking aloud :)\n> \n> \n> Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>\n> \n> >  \n> >  \tframeContext.sensor.exposure =\n> >  \t\tsensorControls.get(V4L2_CID_EXPOSURE).get<int32_t>();\n> > @@ -344,8 +370,12 @@ void IPARkISP1::processStatsBuffer(const uint32_t frame, const uint32_t bufferId\n> >  \n> >  \tControlList metadata(controls::controls);\n> >  \n> > -\tfor (auto const &algo : algorithms())\n> > +\tfor (auto const &a : algorithms()) {\n> > +\t\tAlgorithm *algo = static_cast<Algorithm *>(a.get());\n> > +\t\tif (algo->disabled_)\n> > +\t\t\tcontinue;\n> >  \t\talgo->process(context_, frame, frameContext, stats, metadata);\n> > +\t}\n> >  \n> >  \tsetControls(frame);\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 0DE37BD16B\n\tfor <parsemail@patchwork.libcamera.org>;\n\tMon, 24 Oct 2022 07:47:43 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 6959462EED;\n\tMon, 24 Oct 2022 09:47:42 +0200 (CEST)","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 6AB7E62EBC\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon, 24 Oct 2022 09:47:41 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id B39EB471;\n\tMon, 24 Oct 2022 09:47:40 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1666597662;\n\tbh=1MMIxX+I5iVrxs7MbPD0+Aqkv7uTOaIMwcywFIw/K00=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=1m9/XT2tqunWmcEGQBRNiLDKJ9rPFGq03m8VKhYzpfFt5tDlOSGZ4AZTHTVBpiaGL\n\t3FBKF0SBwSv8QFkRWRwXlKLH7ZNLz1+jN0K50yPLmBT1ucK+pSA3NnuD4uu9QKikVd\n\tyQrfQlZQciaU9fmfT5vQF5OHWPdQEleF83zmUS1z4mHvtsgwDBJ5iRrs0aT8J52p9k\n\tQ0yvqjC2SK6OE9ifjSXYDQ9nALNd0EbpmV9aYl4IJ8EJt62hezFpeVnGXH6xxLeubN\n\teJC1V8g7dBBs9xXdM/AjoXecJ9k1S/fBb0k1YJnwDYrCCYCIjj26cSgOnsUNvYbrrO\n\tynqYeas6eEc6Q==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1666597661;\n\tbh=1MMIxX+I5iVrxs7MbPD0+Aqkv7uTOaIMwcywFIw/K00=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=Uys7HqpPxwomxF3hWDJngiwr2CPLtCQkE5pVbftf3qlad7Rxbq6q8P1nQwpuTz7On\n\tOlhkaGxg35kIG+nVq+OFJoYwhiw8zMBKQG4kMCPtUZ3N4ECT3Kj8iHKOOMRd0aZszE\n\tCSYG7m448rCMAJMe8eqJ9wpXhfMLdPfwRBgPYedM="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"Uys7HqpP\"; dkim-atps=neutral","Date":"Mon, 24 Oct 2022 10:47:15 +0300","To":"paul.elder@ideasonboard.com","Message-ID":"<Y1ZDA9PjJExBKgxa@pendragon.ideasonboard.com>","References":"<20221024000356.29521-1-laurent.pinchart@ideasonboard.com>\n\t<20221024000356.29521-4-laurent.pinchart@ideasonboard.com>\n\t<20221024020220.GU3874866@pyrite.rasen.tech>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20221024020220.GU3874866@pyrite.rasen.tech>","Subject":"Re: [libcamera-devel] [PATCH v3 03/13] ipa: rkisp1: Support raw\n\tcapture in IPA operations","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>","From":"Laurent Pinchart via libcamera-devel\n\t<libcamera-devel@lists.libcamera.org>","Reply-To":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":25537,"web_url":"https://patchwork.libcamera.org/comment/25537/","msgid":"<20221024092510.GG3874866@pyrite.rasen.tech>","date":"2022-10-24T09:25:10","subject":"Re: [libcamera-devel] [PATCH v3 03/13] ipa: rkisp1: Support raw\n\tcapture in IPA operations","submitter":{"id":97,"url":"https://patchwork.libcamera.org/api/people/97/","name":"Nicolas Dufresne via libcamera-devel","email":"libcamera-devel@lists.libcamera.org"},"content":"On Mon, Oct 24, 2022 at 10:47:15AM +0300, Laurent Pinchart wrote:\n> Hi Paul,\n> \n> On Mon, Oct 24, 2022 at 11:02:20AM +0900, paul.elder@ideasonboard.com wrote:\n> > On Mon, Oct 24, 2022 at 03:03:46AM +0300, Laurent Pinchart wrote:\n> > > The RkISP1 can capture raw frames by bypassing the ISP. In that mode,\n> > > the ISP will not produce statistics nor consume parameters. Most\n> > > algorithms will thus be unable to run, with one exception: the AGC will\n> > > still be able to configure the sensor exposure time and analog gain in\n> > > manual mode.\n> > > \n> > > To prepare for this, add the ability to disable algorithms for the\n> > > duration of the capture session based on the camera configuration.\n> > > Individual algorithms report whether they support raw formats at\n> > > construction time, and the IPA module disables algorithms in configure()\n> > > based on the stream configurations.\n> > > \n> > > Disabled algorithms are skipped during the capture session in the\n> > > processStatsBuffer() operation. As the ISP doesn't produce statistics,\n> > > don't try to access the stats buffer. There is no need for similar logic\n> > > in fillParamsBuffer() as that operation won't be called for raw capture.\n> > > \n> > > All algorithms report not supporting raw capture by default. Raw support\n> > > in AGC will be added separately.\n> > > \n> > > The feature is implemented in the RkISP1 module without any support from\n> > > libipa at this point to avoid designing a generic API based on a single\n> > > user. This may be changed in the future.\n> > > \n> > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\n> > > ---\n> > >  src/ipa/rkisp1/algorithms/algorithm.h | 12 +++++++-\n> > >  src/ipa/rkisp1/ipa_context.cpp        |  5 ++++\n> > >  src/ipa/rkisp1/ipa_context.h          |  2 ++\n> > >  src/ipa/rkisp1/rkisp1.cpp             | 42 +++++++++++++++++++++++----\n> > >  4 files changed, 54 insertions(+), 7 deletions(-)\n> > > \n> > > diff --git a/src/ipa/rkisp1/algorithms/algorithm.h b/src/ipa/rkisp1/algorithms/algorithm.h\n> > > index c3212cff76fe..9454c9a1fc06 100644\n> > > --- a/src/ipa/rkisp1/algorithms/algorithm.h\n> > > +++ b/src/ipa/rkisp1/algorithms/algorithm.h\n> > > @@ -15,7 +15,17 @@ namespace libcamera {\n> > >  \n> > >  namespace ipa::rkisp1 {\n> > >  \n> > > -using Algorithm = libcamera::ipa::Algorithm<Module>;\n> > > +class Algorithm : public libcamera::ipa::Algorithm<Module>\n> > > +{\n> > > +public:\n> > > +\tAlgorithm()\n> > > +\t\t: disabled_(false), supportsRaw_(false)\n> > > +\t{\n> > > +\t}\n> > > +\n> > > +\tbool disabled_;\n> > > +\tbool supportsRaw_;\n> > > +};\n> > >  \n> > >  } /* namespace ipa::rkisp1 */\n> > >  \n> > > diff --git a/src/ipa/rkisp1/ipa_context.cpp b/src/ipa/rkisp1/ipa_context.cpp\n> > > index 7a987497bd03..9bbf368432fa 100644\n> > > --- a/src/ipa/rkisp1/ipa_context.cpp\n> > > +++ b/src/ipa/rkisp1/ipa_context.cpp\n> > > @@ -89,6 +89,11 @@ namespace libcamera::ipa::rkisp1 {\n> > >   * \\brief Sensor output resolution\n> > >   */\n> > >  \n> > > +/**\n> > > + * \\var IPASessionConfiguration::raw\n> > > + * \\brief Indicates if the camera is configured to capture raw frames\n> > > + */\n> > > +\n> > >  /**\n> > >   * \\struct IPAActiveState\n> > >   * \\brief Active state for algorithms\n> > > diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h\n> > > index bb60ab9eab72..3e47ac663c58 100644\n> > > --- a/src/ipa/rkisp1/ipa_context.h\n> > > +++ b/src/ipa/rkisp1/ipa_context.h\n> > > @@ -48,6 +48,8 @@ struct IPASessionConfiguration {\n> > >  \tstruct {\n> > >  \t\trkisp1_cif_isp_version revision;\n> > >  \t} hw;\n> > > +\n> > > +\tbool raw;\n> > >  };\n> > >  \n> > >  struct IPAActiveState {\n> > > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp\n> > > index fcb9dacccc3c..538e42cb6f7f 100644\n> > > --- a/src/ipa/rkisp1/rkisp1.cpp\n> > > +++ b/src/ipa/rkisp1/rkisp1.cpp\n> > > @@ -24,6 +24,7 @@\n> > >  #include <libcamera/ipa/rkisp1_ipa_interface.h>\n> > >  #include <libcamera/request.h>\n> > >  \n> > > +#include \"libcamera/internal/formats.h\"\n> > >  #include \"libcamera/internal/mapped_framebuffer.h\"\n> > >  #include \"libcamera/internal/yaml_parser.h\"\n> > >  \n> > > @@ -207,7 +208,7 @@ void IPARkISP1::stop()\n> > >   * before accessing them.\n> > >   */\n> > >  int IPARkISP1::configure([[maybe_unused]] const IPACameraSensorInfo &info,\n> > > -\t\t\t [[maybe_unused]] const std::map<uint32_t, IPAStream> &streamConfig,\n> > > +\t\t\t const std::map<uint32_t, IPAStream> &streamConfig,\n> > >  \t\t\t const std::map<uint32_t, ControlInfoMap> &entityControls)\n> > >  {\n> > >  \tif (entityControls.empty())\n> > > @@ -264,7 +265,21 @@ int IPARkISP1::configure([[maybe_unused]] const IPACameraSensorInfo &info,\n> > >  \tcontext_.configuration.sensor.minAnalogueGain = camHelper_->gain(minGain);\n> > >  \tcontext_.configuration.sensor.maxAnalogueGain = camHelper_->gain(maxGain);\n> > >  \n> > > -\tfor (auto const &algo : algorithms()) {\n> > > +\tcontext_.configuration.raw = std::any_of(streamConfig.begin(), streamConfig.end(),\n> > > +\t\t[](auto &cfg) -> bool {\n> > > +\t\t\tPixelFormat pixelFormat{ cfg.second.pixelFormat };\n> > > +\t\t\tconst PixelFormatInfo &format = PixelFormatInfo::info(pixelFormat);\n> > > +\t\t\treturn format.colourEncoding == PixelFormatInfo::ColourEncodingRAW;\n> > > +\t\t});\n> > > +\n> > > +\tfor (auto const &a : algorithms()) {\n> > > +\t\tAlgorithm *algo = static_cast<Algorithm *>(a.get());\n> > > +\n> > > +\t\t/* Disable algorithms that don't support raw formats. */\n> > > +\t\talgo->disabled_ = context_.configuration.raw && !algo->supportsRaw_;\n> > > +\t\tif (algo->disabled_)\n> > > +\t\t\tcontinue;\n> > > +\n> > \n> > On one hand I was thinking if the algorithms should internally nop if\n> > they're in raw mode, but I think that indeed it's the job of the IPA to\n> > control its algorithms based on its own config, so this is fine.\n> > \n> > >  \t\tint ret = algo->configure(context_, info);\n> > >  \t\tif (ret)\n> > >  \t\t\treturn ret;\n> > > @@ -307,8 +322,12 @@ void IPARkISP1::queueRequest(const uint32_t frame, const ControlList &controls)\n> > >  {\n> > >  \tIPAFrameContext &frameContext = context_.frameContexts.alloc(frame);\n> > >  \n> > > -\tfor (auto const &algo : algorithms())\n> > > +\tfor (auto const &a : algorithms()) {\n> > > +\t\tAlgorithm *algo = static_cast<Algorithm *>(a.get());\n> > > +\t\tif (algo->disabled_)\n> > > +\t\t\tcontinue;\n> > >  \t\talgo->queueRequest(context_, frame, frameContext, controls);\n> > > +\t}\n> > >  }\n> > >  \n> > >  void IPARkISP1::fillParamsBuffer(const uint32_t frame, const uint32_t bufferId)\n> > > @@ -333,9 +352,16 @@ void IPARkISP1::processStatsBuffer(const uint32_t frame, const uint32_t bufferId\n> > >  {\n> > >  \tIPAFrameContext &frameContext = context_.frameContexts.get(frame);\n> > >  \n> > > -\tconst rkisp1_stat_buffer *stats =\n> > > -\t\treinterpret_cast<rkisp1_stat_buffer *>(\n> > > +\t/*\n> > > +\t * In raw capture mode, the ISP is bypassed and no statistics buffer is\n> > > +\t * provided.\n> > > +\t */\n> > > +\tconst rkisp1_stat_buffer *stats;\n> > > +\tif (!context_.configuration.raw)\n> > > +\t\tstats = reinterpret_cast<rkisp1_stat_buffer *>(\n> > >  \t\t\tmappedBuffers_.at(bufferId).planes()[0].data());\n> > > +\telse\n> > > +\t\tstats = nullptr;\n> > \n> > processStatsBuffer() won't ever be called in raw mode *except* for\n> > (before the pipeline hander is adjusted (I expect it to be later in the\n> > series)) the first buffer that got queued at start time, and which would\n> > be bufferReady-dequeued at streamoff. At least this protects against\n> > dereferencing the null stats buffer, plus we don't report supporting raw\n> > formats yet anyway, so I guess it's fine.\n> \n> The function will actually be called to fill metadata for the request.\n> It will not receive statistics, but it will receive the effective values\n> of the camera sensor controls for that frame.\n\nYeah... I hadn't to that patch yet :p\n\n\nPaul\n\n> \n> > Just thinking aloud :)\n> > \n> > \n> > Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>\n> > \n> > >  \n> > >  \tframeContext.sensor.exposure =\n> > >  \t\tsensorControls.get(V4L2_CID_EXPOSURE).get<int32_t>();\n> > > @@ -344,8 +370,12 @@ void IPARkISP1::processStatsBuffer(const uint32_t frame, const uint32_t bufferId\n> > >  \n> > >  \tControlList metadata(controls::controls);\n> > >  \n> > > -\tfor (auto const &algo : algorithms())\n> > > +\tfor (auto const &a : algorithms()) {\n> > > +\t\tAlgorithm *algo = static_cast<Algorithm *>(a.get());\n> > > +\t\tif (algo->disabled_)\n> > > +\t\t\tcontinue;\n> > >  \t\talgo->process(context_, frame, frameContext, stats, metadata);\n> > > +\t}\n> > >  \n> > >  \tsetControls(frame);\n> > >  \n> \n> -- \n> Regards,\n> \n> Laurent Pinchart","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 5BD83BD16B\n\tfor <parsemail@patchwork.libcamera.org>;\n\tMon, 24 Oct 2022 09:25:20 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 88C8D62F04;\n\tMon, 24 Oct 2022 11:25:19 +0200 (CEST)","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 C531061F4D\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon, 24 Oct 2022 11:25:18 +0200 (CEST)","from pyrite.rasen.tech (h175-177-042-159.catv02.itscom.jp\n\t[175.177.42.159])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id D3702471;\n\tMon, 24 Oct 2022 11:25:16 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1666603519;\n\tbh=KZcKK3rcYLuyDgzEO2rXPe/l4dLgR4tY6BlO3Jvxof8=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=VpelUwarKwBJGauECRhhdqDG1Xpanu9+s/Mj1zomjPqQcqQURDOtJETRb5P95Xus2\n\twNhKA+11F0AAd6FvVmpt74zdIKA9TIVeB4dIsMZTJ8S4MjQoroCx/8NMbWczDD+OO9\n\tUpNJmkOLqEberPDqEcrW9aOaIkTRXrYonOeC0yMAhnPfIuHg6XMJaM+yDghJGWdD/w\n\tMsbkOkix0OGaP3+oOnVozUEw/lgEUSJuzJ+i5MuBFQtau8yy00cz5owBv81oLyV+Q4\n\tYSIyk8tcKkWvEC9U8kL+4YFzhMzQHlwK6tV+ZyeqFpOjACu1A1pZpvIInF8xoCzWnL\n\tq3jm88ddXr77Q==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1666603518;\n\tbh=KZcKK3rcYLuyDgzEO2rXPe/l4dLgR4tY6BlO3Jvxof8=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=tyZ2VUKmOKDeNQVJFvBzuWSXlGB+zVjbNH7pabhxlOWIU6eHNuCYxJrIL93AzmJqu\n\tnMdZWj5HBdP+ozN91fRrfoqgwLhqO4kS+4qlR9oZOoyqMYwns0D/mENO2w3+6HotmW\n\tSfRfzdbnElF7FVtxpkAHgH49mso1aDNcf1//sN/w="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"tyZ2VUKm\"; dkim-atps=neutral","Date":"Mon, 24 Oct 2022 18:25:10 +0900","To":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","Message-ID":"<20221024092510.GG3874866@pyrite.rasen.tech>","References":"<20221024000356.29521-1-laurent.pinchart@ideasonboard.com>\n\t<20221024000356.29521-4-laurent.pinchart@ideasonboard.com>\n\t<20221024020220.GU3874866@pyrite.rasen.tech>\n\t<Y1ZDA9PjJExBKgxa@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<Y1ZDA9PjJExBKgxa@pendragon.ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v3 03/13] ipa: rkisp1: Support raw\n\tcapture in IPA operations","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>","From":"Paul Elder via libcamera-devel <libcamera-devel@lists.libcamera.org>","Reply-To":"paul.elder@ideasonboard.com","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":25598,"web_url":"https://patchwork.libcamera.org/comment/25598/","msgid":"<20221026141801.ykn3qidb3z63zho3@uno.localdomain>","date":"2022-10-26T14:18:01","subject":"Re: [libcamera-devel] [PATCH v3 03/13] ipa: rkisp1: Support raw\n\tcapture in IPA operations","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent\n\nOn Mon, Oct 24, 2022 at 03:03:46AM +0300, Laurent Pinchart via libcamera-devel wrote:\n> The RkISP1 can capture raw frames by bypassing the ISP. In that mode,\n> the ISP will not produce statistics nor consume parameters. Most\n> algorithms will thus be unable to run, with one exception: the AGC will\n> still be able to configure the sensor exposure time and analog gain in\n> manual mode.\n>\n> To prepare for this, add the ability to disable algorithms for the\n> duration of the capture session based on the camera configuration.\n> Individual algorithms report whether they support raw formats at\n> construction time, and the IPA module disables algorithms in configure()\n> based on the stream configurations.\n>\n> Disabled algorithms are skipped during the capture session in the\n> processStatsBuffer() operation. As the ISP doesn't produce statistics,\n> don't try to access the stats buffer. There is no need for similar logic\n> in fillParamsBuffer() as that operation won't be called for raw capture.\n>\n> All algorithms report not supporting raw capture by default. Raw support\n> in AGC will be added separately.\n>\n> The feature is implemented in the RkISP1 module without any support from\n> libipa at this point to avoid designing a generic API based on a single\n> user. This may be changed in the future.\n>\n> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\n\nWhile reading the patch I had the same thought I later found out Paul\nhas already expressed.\n\nOn making the algorithms self-disabling, I guess it would actually be\nnicer if they would handle it in configure() by returning immediately,\njust after having set their 'disabled' flag. However I understand it's\na more invasive change, and all that encapsulation is not really\nnecessary. Alternatively, the base class could do the check, but again\nit's not worth the complication most probably..\n\nA few minor nits below\n\n> ---\n>  src/ipa/rkisp1/algorithms/algorithm.h | 12 +++++++-\n>  src/ipa/rkisp1/ipa_context.cpp        |  5 ++++\n>  src/ipa/rkisp1/ipa_context.h          |  2 ++\n>  src/ipa/rkisp1/rkisp1.cpp             | 42 +++++++++++++++++++++++----\n>  4 files changed, 54 insertions(+), 7 deletions(-)\n>\n> diff --git a/src/ipa/rkisp1/algorithms/algorithm.h b/src/ipa/rkisp1/algorithms/algorithm.h\n> index c3212cff76fe..9454c9a1fc06 100644\n> --- a/src/ipa/rkisp1/algorithms/algorithm.h\n> +++ b/src/ipa/rkisp1/algorithms/algorithm.h\n> @@ -15,7 +15,17 @@ namespace libcamera {\n>\n>  namespace ipa::rkisp1 {\n>\n> -using Algorithm = libcamera::ipa::Algorithm<Module>;\n> +class Algorithm : public libcamera::ipa::Algorithm<Module>\n> +{\n> +public:\n> +\tAlgorithm()\n> +\t\t: disabled_(false), supportsRaw_(false)\n> +\t{\n> +\t}\n> +\n> +\tbool disabled_;\n> +\tbool supportsRaw_;\n> +};\n>\n>  } /* namespace ipa::rkisp1 */\n>\n> diff --git a/src/ipa/rkisp1/ipa_context.cpp b/src/ipa/rkisp1/ipa_context.cpp\n> index 7a987497bd03..9bbf368432fa 100644\n> --- a/src/ipa/rkisp1/ipa_context.cpp\n> +++ b/src/ipa/rkisp1/ipa_context.cpp\n> @@ -89,6 +89,11 @@ namespace libcamera::ipa::rkisp1 {\n>   * \\brief Sensor output resolution\n>   */\n>\n> +/**\n> + * \\var IPASessionConfiguration::raw\n> + * \\brief Indicates if the camera is configured to capture raw frames\n> + */\n> +\n>  /**\n>   * \\struct IPAActiveState\n>   * \\brief Active state for algorithms\n> diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h\n> index bb60ab9eab72..3e47ac663c58 100644\n> --- a/src/ipa/rkisp1/ipa_context.h\n> +++ b/src/ipa/rkisp1/ipa_context.h\n> @@ -48,6 +48,8 @@ struct IPASessionConfiguration {\n>  \tstruct {\n>  \t\trkisp1_cif_isp_version revision;\n>  \t} hw;\n> +\n> +\tbool raw;\n>  };\n>\n>  struct IPAActiveState {\n> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp\n> index fcb9dacccc3c..538e42cb6f7f 100644\n> --- a/src/ipa/rkisp1/rkisp1.cpp\n> +++ b/src/ipa/rkisp1/rkisp1.cpp\n> @@ -24,6 +24,7 @@\n>  #include <libcamera/ipa/rkisp1_ipa_interface.h>\n>  #include <libcamera/request.h>\n>\n> +#include \"libcamera/internal/formats.h\"\n>  #include \"libcamera/internal/mapped_framebuffer.h\"\n>  #include \"libcamera/internal/yaml_parser.h\"\n>\n> @@ -207,7 +208,7 @@ void IPARkISP1::stop()\n>   * before accessing them.\n>   */\n>  int IPARkISP1::configure([[maybe_unused]] const IPACameraSensorInfo &info,\n> -\t\t\t [[maybe_unused]] const std::map<uint32_t, IPAStream> &streamConfig,\n> +\t\t\t const std::map<uint32_t, IPAStream> &streamConfig,\n>  \t\t\t const std::map<uint32_t, ControlInfoMap> &entityControls)\n>  {\n>  \tif (entityControls.empty())\n> @@ -264,7 +265,21 @@ int IPARkISP1::configure([[maybe_unused]] const IPACameraSensorInfo &info,\n>  \tcontext_.configuration.sensor.minAnalogueGain = camHelper_->gain(minGain);\n>  \tcontext_.configuration.sensor.maxAnalogueGain = camHelper_->gain(maxGain);\n>\n> -\tfor (auto const &algo : algorithms()) {\n> +\tcontext_.configuration.raw = std::any_of(streamConfig.begin(), streamConfig.end(),\n> +\t\t[](auto &cfg) -> bool {\n> +\t\t\tPixelFormat pixelFormat{ cfg.second.pixelFormat };\n\nI guess there are no differences between using value contruction or\naggregate construction when the argument is an int, I found () more\nnatural..\n\n> +\t\t\tconst PixelFormatInfo &format = PixelFormatInfo::info(pixelFormat);\n> +\t\t\treturn format.colourEncoding == PixelFormatInfo::ColourEncodingRAW;\n> +\t\t});\n> +\n> +\tfor (auto const &a : algorithms()) {\n> +\t\tAlgorithm *algo = static_cast<Algorithm *>(a.get());\n> +\n> +\t\t/* Disable algorithms that don't support raw formats. */\n> +\t\talgo->disabled_ = context_.configuration.raw && !algo->supportsRaw_;\n> +\t\tif (algo->disabled_)\n> +\t\t\tcontinue;\n> +\n>  \t\tint ret = algo->configure(context_, info);\n>  \t\tif (ret)\n>  \t\t\treturn ret;\n> @@ -307,8 +322,12 @@ void IPARkISP1::queueRequest(const uint32_t frame, const ControlList &controls)\n>  {\n>  \tIPAFrameContext &frameContext = context_.frameContexts.alloc(frame);\n>\n> -\tfor (auto const &algo : algorithms())\n> +\tfor (auto const &a : algorithms()) {\n> +\t\tAlgorithm *algo = static_cast<Algorithm *>(a.get());\n> +\t\tif (algo->disabled_)\n> +\t\t\tcontinue;\n>  \t\talgo->queueRequest(context_, frame, frameContext, controls);\n> +\t}\n>  }\n>\n>  void IPARkISP1::fillParamsBuffer(const uint32_t frame, const uint32_t bufferId)\n> @@ -333,9 +352,16 @@ void IPARkISP1::processStatsBuffer(const uint32_t frame, const uint32_t bufferId\n>  {\n>  \tIPAFrameContext &frameContext = context_.frameContexts.get(frame);\n>\n> -\tconst rkisp1_stat_buffer *stats =\n> -\t\treinterpret_cast<rkisp1_stat_buffer *>(\n> +\t/*\n> +\t * In raw capture mode, the ISP is bypassed and no statistics buffer is\n> +\t * provided.\n> +\t */\n> +\tconst rkisp1_stat_buffer *stats;\n> +\tif (!context_.configuration.raw)\n> +\t\tstats = reinterpret_cast<rkisp1_stat_buffer *>(\n>  \t\t\tmappedBuffers_.at(bufferId).planes()[0].data());\n> +\telse\n> +\t\tstats = nullptr;\n>\n\nOr\n\tconst rkisp1_stat_buffer *stats = nullptr;\n\tif (!context_.configuration.raw)\n\t\tstats = reinterpret_cast<rkisp1_stat_buffer *>(\n  \t\t\tmappedBuffers_.at(bufferId).planes()[0].data());\n\nAll minors and mostly personal preferences\n\nReviewed-by: Jacopo Mondi <jacopo@jmondi.org>\n\n\n>  \tframeContext.sensor.exposure =\n>  \t\tsensorControls.get(V4L2_CID_EXPOSURE).get<int32_t>();\n> @@ -344,8 +370,12 @@ void IPARkISP1::processStatsBuffer(const uint32_t frame, const uint32_t bufferId\n>\n>  \tControlList metadata(controls::controls);\n>\n> -\tfor (auto const &algo : algorithms())\n> +\tfor (auto const &a : algorithms()) {\n> +\t\tAlgorithm *algo = static_cast<Algorithm *>(a.get());\n> +\t\tif (algo->disabled_)\n> +\t\t\tcontinue;\n>  \t\talgo->process(context_, frame, frameContext, stats, metadata);\n> +\t}\n>\n>  \tsetControls(frame);\n>\n> --\n> Regards,\n>\n> Laurent Pinchart\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 74960BD16B\n\tfor <parsemail@patchwork.libcamera.org>;\n\tWed, 26 Oct 2022 14:18:06 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C330262F54;\n\tWed, 26 Oct 2022 16:18:05 +0200 (CEST)","from relay5-d.mail.gandi.net (relay5-d.mail.gandi.net\n\t[IPv6:2001:4b98:dc4:8::225])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 337F861F4B\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tWed, 26 Oct 2022 16:18:04 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby mail.gandi.net (Postfix) with ESMTPSA id A00F51C000D;\n\tWed, 26 Oct 2022 14:18:03 +0000 (UTC)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1666793885;\n\tbh=lufkNCNYCM404gRRSz4fFl/MzrnMjJxTybl6h1z/0rI=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=rL/PiyssiJAczNOMQ7fd7FwOewxrIcXGn4+4uhYAg2o4YpMSGyGVbqOLDNOhPnYXR\n\t1WvoXFCU5VT5KVe9WNb9a6kagoSI6blRFKQ4M7mDO5/ZgVDMiY+J1/ci8z9F3YlU8r\n\tUiVVQFa1TiNhHZevvXAAHY5p6BQx4l5fANv971Ixu4AMs8zfdlrSsqeF/0m7wlwF8i\n\tXH+MQu87E8j5kZ8vD6ud4pqUURx7Vo5WyG45V5rUYeKhGYfOZgIqC11cTjIKT9Tl60\n\tT9+AfinMBmDoBOpXnonLlKmjORO0ARCBIkXz5LBdveACW05dWrer1RyxODStK2Zajj\n\ty31GSQcTg0X0w==","Date":"Wed, 26 Oct 2022 16:18:01 +0200","To":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","Message-ID":"<20221026141801.ykn3qidb3z63zho3@uno.localdomain>","References":"<20221024000356.29521-1-laurent.pinchart@ideasonboard.com>\n\t<20221024000356.29521-4-laurent.pinchart@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20221024000356.29521-4-laurent.pinchart@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v3 03/13] ipa: rkisp1: Support raw\n\tcapture in IPA operations","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>","From":"Jacopo Mondi via libcamera-devel <libcamera-devel@lists.libcamera.org>","Reply-To":"Jacopo Mondi <jacopo@jmondi.org>","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":25683,"web_url":"https://patchwork.libcamera.org/comment/25683/","msgid":"<Y16w+X1bGzbRJHMg@pendragon.ideasonboard.com>","date":"2022-10-30T17:14:33","subject":"Re: [libcamera-devel] [PATCH v3 03/13] ipa: rkisp1: Support raw\n\tcapture in IPA operations","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Wed, Oct 26, 2022 at 04:18:01PM +0200, Jacopo Mondi wrote:\n> On Mon, Oct 24, 2022 at 03:03:46AM +0300, Laurent Pinchart wrote:\n> > The RkISP1 can capture raw frames by bypassing the ISP. In that mode,\n> > the ISP will not produce statistics nor consume parameters. Most\n> > algorithms will thus be unable to run, with one exception: the AGC will\n> > still be able to configure the sensor exposure time and analog gain in\n> > manual mode.\n> >\n> > To prepare for this, add the ability to disable algorithms for the\n> > duration of the capture session based on the camera configuration.\n> > Individual algorithms report whether they support raw formats at\n> > construction time, and the IPA module disables algorithms in configure()\n> > based on the stream configurations.\n> >\n> > Disabled algorithms are skipped during the capture session in the\n> > processStatsBuffer() operation. As the ISP doesn't produce statistics,\n> > don't try to access the stats buffer. There is no need for similar logic\n> > in fillParamsBuffer() as that operation won't be called for raw capture.\n> >\n> > All algorithms report not supporting raw capture by default. Raw support\n> > in AGC will be added separately.\n> >\n> > The feature is implemented in the RkISP1 module without any support from\n> > libipa at this point to avoid designing a generic API based on a single\n> > user. This may be changed in the future.\n> >\n> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\n> \n> While reading the patch I had the same thought I later found out Paul\n> has already expressed.\n> \n> On making the algorithms self-disabling, I guess it would actually be\n> nicer if they would handle it in configure() by returning immediately,\n> just after having set their 'disabled' flag. However I understand it's\n> a more invasive change, and all that encapsulation is not really\n> necessary. Alternatively, the base class could do the check, but again\n> it's not worth the complication most probably..\n\nI've considered multiple options, and given that we have, for the\nrkisp1, a single algorithm that can work in raw mode, I picked a\nsolution that didn't require all algorithms to be modified with raw\nchecks in configure(). I've also considered moving the infrastructure to\nthe base Algorithm class, but decided I didn't have enough IPA modules\nsupporting raw capture do design this properly. We may move it later,\nwhen we'll have a second data point.\n\n> A few minor nits below\n> \n> > ---\n> >  src/ipa/rkisp1/algorithms/algorithm.h | 12 +++++++-\n> >  src/ipa/rkisp1/ipa_context.cpp        |  5 ++++\n> >  src/ipa/rkisp1/ipa_context.h          |  2 ++\n> >  src/ipa/rkisp1/rkisp1.cpp             | 42 +++++++++++++++++++++++----\n> >  4 files changed, 54 insertions(+), 7 deletions(-)\n> >\n> > diff --git a/src/ipa/rkisp1/algorithms/algorithm.h b/src/ipa/rkisp1/algorithms/algorithm.h\n> > index c3212cff76fe..9454c9a1fc06 100644\n> > --- a/src/ipa/rkisp1/algorithms/algorithm.h\n> > +++ b/src/ipa/rkisp1/algorithms/algorithm.h\n> > @@ -15,7 +15,17 @@ namespace libcamera {\n> >\n> >  namespace ipa::rkisp1 {\n> >\n> > -using Algorithm = libcamera::ipa::Algorithm<Module>;\n> > +class Algorithm : public libcamera::ipa::Algorithm<Module>\n> > +{\n> > +public:\n> > +\tAlgorithm()\n> > +\t\t: disabled_(false), supportsRaw_(false)\n> > +\t{\n> > +\t}\n> > +\n> > +\tbool disabled_;\n> > +\tbool supportsRaw_;\n> > +};\n> >\n> >  } /* namespace ipa::rkisp1 */\n> >\n> > diff --git a/src/ipa/rkisp1/ipa_context.cpp b/src/ipa/rkisp1/ipa_context.cpp\n> > index 7a987497bd03..9bbf368432fa 100644\n> > --- a/src/ipa/rkisp1/ipa_context.cpp\n> > +++ b/src/ipa/rkisp1/ipa_context.cpp\n> > @@ -89,6 +89,11 @@ namespace libcamera::ipa::rkisp1 {\n> >   * \\brief Sensor output resolution\n> >   */\n> >\n> > +/**\n> > + * \\var IPASessionConfiguration::raw\n> > + * \\brief Indicates if the camera is configured to capture raw frames\n> > + */\n> > +\n> >  /**\n> >   * \\struct IPAActiveState\n> >   * \\brief Active state for algorithms\n> > diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h\n> > index bb60ab9eab72..3e47ac663c58 100644\n> > --- a/src/ipa/rkisp1/ipa_context.h\n> > +++ b/src/ipa/rkisp1/ipa_context.h\n> > @@ -48,6 +48,8 @@ struct IPASessionConfiguration {\n> >  \tstruct {\n> >  \t\trkisp1_cif_isp_version revision;\n> >  \t} hw;\n> > +\n> > +\tbool raw;\n> >  };\n> >\n> >  struct IPAActiveState {\n> > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp\n> > index fcb9dacccc3c..538e42cb6f7f 100644\n> > --- a/src/ipa/rkisp1/rkisp1.cpp\n> > +++ b/src/ipa/rkisp1/rkisp1.cpp\n> > @@ -24,6 +24,7 @@\n> >  #include <libcamera/ipa/rkisp1_ipa_interface.h>\n> >  #include <libcamera/request.h>\n> >\n> > +#include \"libcamera/internal/formats.h\"\n> >  #include \"libcamera/internal/mapped_framebuffer.h\"\n> >  #include \"libcamera/internal/yaml_parser.h\"\n> >\n> > @@ -207,7 +208,7 @@ void IPARkISP1::stop()\n> >   * before accessing them.\n> >   */\n> >  int IPARkISP1::configure([[maybe_unused]] const IPACameraSensorInfo &info,\n> > -\t\t\t [[maybe_unused]] const std::map<uint32_t, IPAStream> &streamConfig,\n> > +\t\t\t const std::map<uint32_t, IPAStream> &streamConfig,\n> >  \t\t\t const std::map<uint32_t, ControlInfoMap> &entityControls)\n> >  {\n> >  \tif (entityControls.empty())\n> > @@ -264,7 +265,21 @@ int IPARkISP1::configure([[maybe_unused]] const IPACameraSensorInfo &info,\n> >  \tcontext_.configuration.sensor.minAnalogueGain = camHelper_->gain(minGain);\n> >  \tcontext_.configuration.sensor.maxAnalogueGain = camHelper_->gain(maxGain);\n> >\n> > -\tfor (auto const &algo : algorithms()) {\n> > +\tcontext_.configuration.raw = std::any_of(streamConfig.begin(), streamConfig.end(),\n> > +\t\t[](auto &cfg) -> bool {\n> > +\t\t\tPixelFormat pixelFormat{ cfg.second.pixelFormat };\n> \n> I guess there are no differences between using value contruction or\n> aggregate construction when the argument is an int, I found () more\n> natural..\n\nDirect list initialization (T object { arg1, arg2, ... }) and direct\ninitialization (T object ( arg1, arg2, ... )) differ mostly in two ways\n(for class types). First of all, list initialization will never perform\nnarrowing:\n\n- An integer cannot be converted to another integer that cannot hold its\n  value. For example, char to int is allowed, but not int to char.\n- A floating-point value cannot be converted to another floating-point\n  type that cannot hold its value. For example, float to double is\n  allowed, but not double to float.\n- A floating-point value cannot be converted to an integer type.\n- An integer value cannot be converted to a floating-point type.\n\nThen, list initialization will first try constructors that take a\nstd::initializer_list as a single argument, so\n\nstd::vector<int> v1(10, 20);\n\nand\n\nstd::vector<int> v1{10, 20};\n\nlead to different results. The former creates a vector of 10 elements\ninitialized to value 20, while the latter creates a vector of 2\nelements.\n\nThis often lead to recommending list initialization by default unless\nthere is a reason to do otherwise. We don't have a clear default rule in\nlibcamera, maybe we should ?\n\n> > +\t\t\tconst PixelFormatInfo &format = PixelFormatInfo::info(pixelFormat);\n> > +\t\t\treturn format.colourEncoding == PixelFormatInfo::ColourEncodingRAW;\n> > +\t\t});\n> > +\n> > +\tfor (auto const &a : algorithms()) {\n> > +\t\tAlgorithm *algo = static_cast<Algorithm *>(a.get());\n> > +\n> > +\t\t/* Disable algorithms that don't support raw formats. */\n> > +\t\talgo->disabled_ = context_.configuration.raw && !algo->supportsRaw_;\n> > +\t\tif (algo->disabled_)\n> > +\t\t\tcontinue;\n> > +\n> >  \t\tint ret = algo->configure(context_, info);\n> >  \t\tif (ret)\n> >  \t\t\treturn ret;\n> > @@ -307,8 +322,12 @@ void IPARkISP1::queueRequest(const uint32_t frame, const ControlList &controls)\n> >  {\n> >  \tIPAFrameContext &frameContext = context_.frameContexts.alloc(frame);\n> >\n> > -\tfor (auto const &algo : algorithms())\n> > +\tfor (auto const &a : algorithms()) {\n> > +\t\tAlgorithm *algo = static_cast<Algorithm *>(a.get());\n> > +\t\tif (algo->disabled_)\n> > +\t\t\tcontinue;\n> >  \t\talgo->queueRequest(context_, frame, frameContext, controls);\n> > +\t}\n> >  }\n> >\n> >  void IPARkISP1::fillParamsBuffer(const uint32_t frame, const uint32_t bufferId)\n> > @@ -333,9 +352,16 @@ void IPARkISP1::processStatsBuffer(const uint32_t frame, const uint32_t bufferId\n> >  {\n> >  \tIPAFrameContext &frameContext = context_.frameContexts.get(frame);\n> >\n> > -\tconst rkisp1_stat_buffer *stats =\n> > -\t\treinterpret_cast<rkisp1_stat_buffer *>(\n> > +\t/*\n> > +\t * In raw capture mode, the ISP is bypassed and no statistics buffer is\n> > +\t * provided.\n> > +\t */\n> > +\tconst rkisp1_stat_buffer *stats;\n> > +\tif (!context_.configuration.raw)\n> > +\t\tstats = reinterpret_cast<rkisp1_stat_buffer *>(\n> >  \t\t\tmappedBuffers_.at(bufferId).planes()[0].data());\n> > +\telse\n> > +\t\tstats = nullptr;\n> >\n> \n> Or\n> \tconst rkisp1_stat_buffer *stats = nullptr;\n> \tif (!context_.configuration.raw)\n> \t\tstats = reinterpret_cast<rkisp1_stat_buffer *>(\n>   \t\t\tmappedBuffers_.at(bufferId).planes()[0].data());\n\nI don't mind much either way.\n\n> All minors and mostly personal preferences\n> \n> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>\n> \n> >  \tframeContext.sensor.exposure =\n> >  \t\tsensorControls.get(V4L2_CID_EXPOSURE).get<int32_t>();\n> > @@ -344,8 +370,12 @@ void IPARkISP1::processStatsBuffer(const uint32_t frame, const uint32_t bufferId\n> >\n> >  \tControlList metadata(controls::controls);\n> >\n> > -\tfor (auto const &algo : algorithms())\n> > +\tfor (auto const &a : algorithms()) {\n> > +\t\tAlgorithm *algo = static_cast<Algorithm *>(a.get());\n> > +\t\tif (algo->disabled_)\n> > +\t\t\tcontinue;\n> >  \t\talgo->process(context_, frame, frameContext, stats, metadata);\n> > +\t}\n> >\n> >  \tsetControls(frame);\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 4B73ABDB16\n\tfor <parsemail@patchwork.libcamera.org>;\n\tSun, 30 Oct 2022 17:15:02 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id AB3E06301F;\n\tSun, 30 Oct 2022 18:15:01 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 4AAEB61F47\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tSun, 30 Oct 2022 18:15:00 +0100 (CET)","from pendragon.ideasonboard.com (85-76-1-64-nat.elisa-mobile.fi\n\t[85.76.1.64])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 8E2147C5;\n\tSun, 30 Oct 2022 18:14:58 +0100 (CET)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1667150101;\n\tbh=xpimSi/HrD9QTPPWQL8QHCI1GkPWKbEn5W95zDXLUBo=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=UZwR7C5wMs+Mi8gFa+UjozENtz+0jckjYNHhuMZ2xoOncCCOQ1Ghu2u6Scqe0LYu3\n\tQWiQBbCAAIEfe2dYrxcaYiT2shlqLiU8eoe6itmqWj9XY8FcPMuBhfseiMkb2eUi1z\n\tpR4tYlB4aLh2xTIxg6YcFgKIRNghGpdIDTwonyxxA8cD7SCjXX+oIBabeP+LNvCbtu\n\tJaphVdbjYWedngXSo44NHTEG1WbFH8cyDP7h+bQn5FL1twgATZG71E7F8E+L/+85y/\n\tbS26HHbtKB2WC8DSKuGcTdP+J7XbuyYA7MvVETrCSuLp7SLjTHc8BhEU2K4miWzw0c\n\tNl4J+yQiUBmDA==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1667150099;\n\tbh=xpimSi/HrD9QTPPWQL8QHCI1GkPWKbEn5W95zDXLUBo=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=iG3qBHuwqSOMOQxKXxwuST6axl6rLb+O/605vSNy9PlOlsycmDNcCyggd+H39PmfT\n\t/8nmJtaLvgCnwqcvmp0IKAZtEPQk4Y3uq5qvOH3mhQtxpel6clZTevYpS0M5ZOMAi1\n\t5gEwvvBvyqk+l0FBckC07tQJwxv4SzP1Vz+8yUbI="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"iG3qBHuw\"; dkim-atps=neutral","Date":"Sun, 30 Oct 2022 19:14:33 +0200","To":"Jacopo Mondi <jacopo@jmondi.org>","Message-ID":"<Y16w+X1bGzbRJHMg@pendragon.ideasonboard.com>","References":"<20221024000356.29521-1-laurent.pinchart@ideasonboard.com>\n\t<20221024000356.29521-4-laurent.pinchart@ideasonboard.com>\n\t<20221026141801.ykn3qidb3z63zho3@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20221026141801.ykn3qidb3z63zho3@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH v3 03/13] ipa: rkisp1: Support raw\n\tcapture in IPA operations","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>","From":"Laurent Pinchart via libcamera-devel\n\t<libcamera-devel@lists.libcamera.org>","Reply-To":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}}]