[{"id":32576,"web_url":"https://patchwork.libcamera.org/comment/32576/","msgid":"<173349087522.3135963.11705101284708571797@ping.linuxembedded.co.uk>","date":"2024-12-06T13:14:35","subject":"Re: [PATCH resend 5/5] libcamera: swstats_cpu: Add processFrame()\n\tmethod","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Hans de Goede (2024-12-05 19:25:19)\n> Add a method to the SwstatsCpu class to process a whole Framebuffer in\n> one go, rather then line by line. This is useful for gathering stats\n> when debayering is not necessary or is not done on the CPU.\n\nThis is the bit I can see will be useful for any scenario where we don't\nhave stats. I think for instance on RKISP1 when running raw mode we\ndon't get stats, so something like this would be helpful to be able to\nsupport some AEGC control which would likely be beneficial for tuning or\ncapturing raw streams.\n\n> Reviewed-by: Milan Zamazal <mzamazal@redhat.com>\n> Signed-off-by: Hans de Goede <hdegoede@redhat.com>\n> ---\n> Changes since the RFC:\n> - Make processFrame() call startFrame() and finishFrame() rather then\n>   making the caller do this\n> - Add doxygen documentation for processFrame()\n> ---\n>  .../internal/software_isp/swstats_cpu.h       | 12 +++++\n>  src/libcamera/software_isp/swstats_cpu.cpp    | 51 +++++++++++++++++++\n>  2 files changed, 63 insertions(+)\n> \n> diff --git a/include/libcamera/internal/software_isp/swstats_cpu.h b/include/libcamera/internal/software_isp/swstats_cpu.h\n> index 26a2f462..fa47cec9 100644\n> --- a/include/libcamera/internal/software_isp/swstats_cpu.h\n> +++ b/include/libcamera/internal/software_isp/swstats_cpu.h\n> @@ -18,12 +18,16 @@\n>  #include <libcamera/geometry.h>\n>  \n>  #include \"libcamera/internal/bayer_format.h\"\n> +#include \"libcamera/internal/framebuffer.h\"\n>  #include \"libcamera/internal/shared_mem_object.h\"\n>  #include \"libcamera/internal/software_isp/swisp_stats.h\"\n>  \n> +#include \"benchmark.h\"\n> +\n>  namespace libcamera {\n>  \n>  class PixelFormat;\n> +class MappedFrameBuffer;\n>  struct StreamConfiguration;\n>  \n>  class SwStatsCpu\n> @@ -42,6 +46,7 @@ public:\n>         void setWindow(const Rectangle &window);\n>         void startFrame();\n>         void finishFrame(uint32_t frame, uint32_t bufferId);\n> +       void processFrame(uint32_t frame, uint32_t bufferId, FrameBuffer *input);\n>  \n>         void processLine0(unsigned int y, const uint8_t *src[])\n>         {\n> @@ -65,6 +70,7 @@ public:\n>  \n>  private:\n>         using statsProcessFn = void (SwStatsCpu::*)(const uint8_t *src[]);\n> +       using processFrameFn = void (SwStatsCpu::*)(MappedFrameBuffer &in);\n>  \n>         int setupStandardBayerOrder(BayerFormat::Order order);\n>         /* Bayer 8 bpp unpacked */\n> @@ -77,6 +83,10 @@ private:\n>         void statsBGGR10PLine0(const uint8_t *src[]);\n>         void statsGBRG10PLine0(const uint8_t *src[]);\n>  \n> +       void processBayerFrame2(MappedFrameBuffer &in);\n\nAyeee ... I shudder a little seeing 'Thing2'.\n\nWould this really just be processMappedBayerFrame() ?\n\n> +\n> +       processFrameFn processFrame_;\n> +\n>         /* Variables set by configure(), used every line */\n>         statsProcessFn stats0_;\n>         statsProcessFn stats2_;\n> @@ -89,9 +99,11 @@ private:\n>         Size patternSize_;\n>  \n>         unsigned int xShift_;\n> +       unsigned int stride_;\n>  \n>         SharedMemObject<SwIspStats> sharedStats_;\n>         SwIspStats stats_;\n> +       Benchmark bench_;\n>  };\n>  \n>  } /* namespace libcamera */\n> diff --git a/src/libcamera/software_isp/swstats_cpu.cpp b/src/libcamera/software_isp/swstats_cpu.cpp\n> index aa5654dc..1ff15f5b 100644\n> --- a/src/libcamera/software_isp/swstats_cpu.cpp\n> +++ b/src/libcamera/software_isp/swstats_cpu.cpp\n> @@ -16,6 +16,7 @@\n>  #include <libcamera/stream.h>\n>  \n>  #include \"libcamera/internal/bayer_format.h\"\n> +#include \"libcamera/internal/mapped_framebuffer.h\"\n>  \n>  namespace libcamera {\n>  \n> @@ -360,11 +361,14 @@ int SwStatsCpu::setupStandardBayerOrder(BayerFormat::Order order)\n>   */\n>  int SwStatsCpu::configure(const StreamConfiguration &inputCfg)\n>  {\n> +       stride_ = inputCfg.stride;\n> +\n>         BayerFormat bayerFormat =\n>                 BayerFormat::fromPixelFormat(inputCfg.pixelFormat);\n>  \n>         if (bayerFormat.packing == BayerFormat::Packing::None &&\n>             setupStandardBayerOrder(bayerFormat.order) == 0) {\n> +               processFrame_ = &SwStatsCpu::processBayerFrame2;\n>                 switch (bayerFormat.bitDepth) {\n>                 case 8:\n>                         stats0_ = &SwStatsCpu::statsBGGR8Line0;\n> @@ -385,6 +389,7 @@ int SwStatsCpu::configure(const StreamConfiguration &inputCfg)\n>                 /* Skip every 3th and 4th line, sample every other 2x2 block */\n>                 ySkipMask_ = 0x02;\n>                 xShift_ = 0;\n> +               processFrame_ = &SwStatsCpu::processBayerFrame2;\n>  \n>                 switch (bayerFormat.order) {\n>                 case BayerFormat::BGGR:\n> @@ -425,4 +430,50 @@ void SwStatsCpu::setWindow(const Rectangle &window)\n>         window_.height &= ~(patternSize_.height - 1);\n>  }\n>  \n> +void SwStatsCpu::processBayerFrame2(MappedFrameBuffer &in)\n> +{\n> +       const uint8_t *src = in.planes()[0].data();\n> +       const uint8_t *linePointers[3];\n> +\n> +       /* Adjust src for starting at window_.y */\n> +       src += window_.y * stride_;\n> +\n> +       for (unsigned int y = 0; y < window_.height; y += 2) {\n> +               if (y & ySkipMask_) {\n> +                       src += stride_ * 2;\n> +                       continue;\n> +               }\n> +\n> +               /* linePointers[0] is not used by any stats0_ functions */\n> +               linePointers[1] = src;\n> +               linePointers[2] = src + stride_;\n> +               (this->*stats0_)(linePointers);\n> +               src += stride_ * 2;\n> +       }\n> +}\n> +\n> +/**\n> + * \\brief Calculate statistics for a frame in one go\n> + * \\param[in] frame The frame number\n> + * \\param[in] bufferId ID of the statistics buffer\n> + * \\param[in] input The frame to process\n> + *\n> + * This may only be called after a successful setWindow() call.\n> + */\n> +void SwStatsCpu::processFrame(uint32_t frame, uint32_t bufferId, FrameBuffer *input)\n> +{\n> +       bench_.startFrame();\n> +       startFrame();\n> +\n> +       MappedFrameBuffer in(input, MappedFrameBuffer::MapFlag::Read);\n> +       if (!in.isValid()) {\n> +               LOG(SwStatsCpu, Error) << \"mmap-ing buffer(s) failed\";\n> +               return;\n> +       }\n> +\n> +       (this->*processFrame_)(in);\n\nI can't see why this goes through a function pointer at the moment. Is\nthere some later patches that will have 'different' processFrame_\nimplemenations?\n\nOr could SwStatsCpu::processBayerFrame2 just be inlined here ?\n\n(I'm fine keeping it separate if something else is about to use it, or\nchange it soon).\n\n--\n\nKieran\n\n> +       finishFrame(frame, bufferId);\n> +       bench_.finishFrame();\n> +}\n> +\n>  } /* namespace libcamera */\n> -- \n> 2.47.0\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 82B12BE173\n\tfor <parsemail@patchwork.libcamera.org>;\n\tFri,  6 Dec 2024 13:14:39 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 285FA6614B;\n\tFri,  6 Dec 2024 14:14:39 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 33D6A618B3\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tFri,  6 Dec 2024 14:14:38 +0100 (CET)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust6594.18-1.cable.virginm.net [86.31.185.195])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id C08D6641;\n\tFri,  6 Dec 2024 14:14:08 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"J0Bp4W7f\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1733490848;\n\tbh=35rvETZz+Fdxe2Es6VUBlF5yIFvmV7agGzv87teA6V4=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=J0Bp4W7fp07PJksayiePwCKwp+D9LkTZlLFqB1dO9hJWmEXz6vYHH/Ii3gSvSAk4p\n\t3kt0Iq/k42Vp517XAAzVyajFnxM7TVNtwJR6a59H0/8Uc5PYBp8KweRd6+RqdGhZGl\n\tdBQN8BtlPnat8Yh8OVLpmGlSeZTFOFZpDX7fIdwo=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20241205192519.49104-6-hdegoede@redhat.com>","References":"<20241205192519.49104-1-hdegoede@redhat.com>\n\t<20241205192519.49104-6-hdegoede@redhat.com>","Subject":"Re: [PATCH resend 5/5] libcamera: swstats_cpu: Add processFrame()\n\tmethod","From":"Kieran Bingham <kieran.bingham@ideasonboard.com>","Cc":"Milan Zamazal <mzamazal@redhat.com>, Hans de Goede <hdegoede@redhat.com>","To":"Hans de Goede <hdegoede@redhat.com>, libcamera-devel@lists.libcamera.org","Date":"Fri, 06 Dec 2024 13:14:35 +0000","Message-ID":"<173349087522.3135963.11705101284708571797@ping.linuxembedded.co.uk>","User-Agent":"alot/0.10","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>","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":32580,"web_url":"https://patchwork.libcamera.org/comment/32580/","msgid":"<20241206132803.GJ25902@pendragon.ideasonboard.com>","date":"2024-12-06T13:28:03","subject":"Re: [PATCH resend 5/5] libcamera: swstats_cpu: Add processFrame()\n\tmethod","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Fri, Dec 06, 2024 at 01:14:35PM +0000, Kieran Bingham wrote:\n> Quoting Hans de Goede (2024-12-05 19:25:19)\n> > Add a method to the SwstatsCpu class to process a whole Framebuffer in\n> > one go, rather then line by line. This is useful for gathering stats\n> > when debayering is not necessary or is not done on the CPU.\n> \n> This is the bit I can see will be useful for any scenario where we don't\n> have stats. I think for instance on RKISP1 when running raw mode we\n> don't get stats, so something like this would be helpful to be able to\n> support some AEGC control which would likely be beneficial for tuning or\n> capturing raw streams.\n\nI'm in two minds about that. If it's only about tuning with ISPs that\ncan't generate statistics when capturing raw frames, an application-side\nAE could be a better option. We should discuss it when the need arises\nbefore writing code.\n\n> > Reviewed-by: Milan Zamazal <mzamazal@redhat.com>\n> > Signed-off-by: Hans de Goede <hdegoede@redhat.com>\n> > ---\n> > Changes since the RFC:\n> > - Make processFrame() call startFrame() and finishFrame() rather then\n> >   making the caller do this\n> > - Add doxygen documentation for processFrame()\n> > ---\n> >  .../internal/software_isp/swstats_cpu.h       | 12 +++++\n> >  src/libcamera/software_isp/swstats_cpu.cpp    | 51 +++++++++++++++++++\n> >  2 files changed, 63 insertions(+)\n> > \n> > diff --git a/include/libcamera/internal/software_isp/swstats_cpu.h b/include/libcamera/internal/software_isp/swstats_cpu.h\n> > index 26a2f462..fa47cec9 100644\n> > --- a/include/libcamera/internal/software_isp/swstats_cpu.h\n> > +++ b/include/libcamera/internal/software_isp/swstats_cpu.h\n> > @@ -18,12 +18,16 @@\n> >  #include <libcamera/geometry.h>\n> >  \n> >  #include \"libcamera/internal/bayer_format.h\"\n> > +#include \"libcamera/internal/framebuffer.h\"\n> >  #include \"libcamera/internal/shared_mem_object.h\"\n> >  #include \"libcamera/internal/software_isp/swisp_stats.h\"\n> >  \n> > +#include \"benchmark.h\"\n> > +\n> >  namespace libcamera {\n> >  \n> >  class PixelFormat;\n> > +class MappedFrameBuffer;\n> >  struct StreamConfiguration;\n> >  \n> >  class SwStatsCpu\n> > @@ -42,6 +46,7 @@ public:\n> >         void setWindow(const Rectangle &window);\n> >         void startFrame();\n> >         void finishFrame(uint32_t frame, uint32_t bufferId);\n> > +       void processFrame(uint32_t frame, uint32_t bufferId, FrameBuffer *input);\n> >  \n> >         void processLine0(unsigned int y, const uint8_t *src[])\n> >         {\n> > @@ -65,6 +70,7 @@ public:\n> >  \n> >  private:\n> >         using statsProcessFn = void (SwStatsCpu::*)(const uint8_t *src[]);\n> > +       using processFrameFn = void (SwStatsCpu::*)(MappedFrameBuffer &in);\n> >  \n> >         int setupStandardBayerOrder(BayerFormat::Order order);\n> >         /* Bayer 8 bpp unpacked */\n> > @@ -77,6 +83,10 @@ private:\n> >         void statsBGGR10PLine0(const uint8_t *src[]);\n> >         void statsGBRG10PLine0(const uint8_t *src[]);\n> >  \n> > +       void processBayerFrame2(MappedFrameBuffer &in);\n> \n> Ayeee ... I shudder a little seeing 'Thing2'.\n> \n> Would this really just be processMappedBayerFrame() ?\n> \n> > +\n> > +       processFrameFn processFrame_;\n> > +\n> >         /* Variables set by configure(), used every line */\n> >         statsProcessFn stats0_;\n> >         statsProcessFn stats2_;\n> > @@ -89,9 +99,11 @@ private:\n> >         Size patternSize_;\n> >  \n> >         unsigned int xShift_;\n> > +       unsigned int stride_;\n> >  \n> >         SharedMemObject<SwIspStats> sharedStats_;\n> >         SwIspStats stats_;\n> > +       Benchmark bench_;\n> >  };\n> >  \n> >  } /* namespace libcamera */\n> > diff --git a/src/libcamera/software_isp/swstats_cpu.cpp b/src/libcamera/software_isp/swstats_cpu.cpp\n> > index aa5654dc..1ff15f5b 100644\n> > --- a/src/libcamera/software_isp/swstats_cpu.cpp\n> > +++ b/src/libcamera/software_isp/swstats_cpu.cpp\n> > @@ -16,6 +16,7 @@\n> >  #include <libcamera/stream.h>\n> >  \n> >  #include \"libcamera/internal/bayer_format.h\"\n> > +#include \"libcamera/internal/mapped_framebuffer.h\"\n> >  \n> >  namespace libcamera {\n> >  \n> > @@ -360,11 +361,14 @@ int SwStatsCpu::setupStandardBayerOrder(BayerFormat::Order order)\n> >   */\n> >  int SwStatsCpu::configure(const StreamConfiguration &inputCfg)\n> >  {\n> > +       stride_ = inputCfg.stride;\n> > +\n> >         BayerFormat bayerFormat =\n> >                 BayerFormat::fromPixelFormat(inputCfg.pixelFormat);\n> >  \n> >         if (bayerFormat.packing == BayerFormat::Packing::None &&\n> >             setupStandardBayerOrder(bayerFormat.order) == 0) {\n> > +               processFrame_ = &SwStatsCpu::processBayerFrame2;\n> >                 switch (bayerFormat.bitDepth) {\n> >                 case 8:\n> >                         stats0_ = &SwStatsCpu::statsBGGR8Line0;\n> > @@ -385,6 +389,7 @@ int SwStatsCpu::configure(const StreamConfiguration &inputCfg)\n> >                 /* Skip every 3th and 4th line, sample every other 2x2 block */\n> >                 ySkipMask_ = 0x02;\n> >                 xShift_ = 0;\n> > +               processFrame_ = &SwStatsCpu::processBayerFrame2;\n> >  \n> >                 switch (bayerFormat.order) {\n> >                 case BayerFormat::BGGR:\n> > @@ -425,4 +430,50 @@ void SwStatsCpu::setWindow(const Rectangle &window)\n> >         window_.height &= ~(patternSize_.height - 1);\n> >  }\n> >  \n> > +void SwStatsCpu::processBayerFrame2(MappedFrameBuffer &in)\n> > +{\n> > +       const uint8_t *src = in.planes()[0].data();\n> > +       const uint8_t *linePointers[3];\n> > +\n> > +       /* Adjust src for starting at window_.y */\n> > +       src += window_.y * stride_;\n> > +\n> > +       for (unsigned int y = 0; y < window_.height; y += 2) {\n> > +               if (y & ySkipMask_) {\n> > +                       src += stride_ * 2;\n> > +                       continue;\n> > +               }\n> > +\n> > +               /* linePointers[0] is not used by any stats0_ functions */\n> > +               linePointers[1] = src;\n> > +               linePointers[2] = src + stride_;\n> > +               (this->*stats0_)(linePointers);\n> > +               src += stride_ * 2;\n> > +       }\n> > +}\n> > +\n> > +/**\n> > + * \\brief Calculate statistics for a frame in one go\n> > + * \\param[in] frame The frame number\n> > + * \\param[in] bufferId ID of the statistics buffer\n> > + * \\param[in] input The frame to process\n> > + *\n> > + * This may only be called after a successful setWindow() call.\n> > + */\n> > +void SwStatsCpu::processFrame(uint32_t frame, uint32_t bufferId, FrameBuffer *input)\n> > +{\n> > +       bench_.startFrame();\n> > +       startFrame();\n> > +\n> > +       MappedFrameBuffer in(input, MappedFrameBuffer::MapFlag::Read);\n> > +       if (!in.isValid()) {\n> > +               LOG(SwStatsCpu, Error) << \"mmap-ing buffer(s) failed\";\n> > +               return;\n> > +       }\n> > +\n> > +       (this->*processFrame_)(in);\n> \n> I can't see why this goes through a function pointer at the moment. Is\n> there some later patches that will have 'different' processFrame_\n> implemenations?\n> \n> Or could SwStatsCpu::processBayerFrame2 just be inlined here ?\n> \n> (I'm fine keeping it separate if something else is about to use it, or\n> change it soon).\n> \n> > +       finishFrame(frame, bufferId);\n> > +       bench_.finishFrame();\n> > +}\n> > +\n> >  } /* namespace libcamera */","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 EF2FCBE173\n\tfor <parsemail@patchwork.libcamera.org>;\n\tFri,  6 Dec 2024 13:28:18 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 06FF66614C;\n\tFri,  6 Dec 2024 14:28:18 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id B4756618B3\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tFri,  6 Dec 2024 14:28:16 +0100 (CET)","from pendragon.ideasonboard.com (81-175-209-231.bb.dnainternet.fi\n\t[81.175.209.231])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 06C3A641;\n\tFri,  6 Dec 2024 14:27:46 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"nNB3U4Gr\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1733491667;\n\tbh=I1kmux1FBWWC2r2QkMTU79sfY+15LKc+JY39TSadQBk=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=nNB3U4GrcF+yjT04NSFx7xNhiww4HiS4Y5612+JpXyjwkJIBGfHtYMXMKoqtLtCOe\n\tcFpysOkkfknXDeAQ4ch+vUhGhovKeCNabLE/qFzDfDoe8zKxkq/QMcUIJ66W0aFbcV\n\twOl3EfQ1WP4NjcIFmkDCxqDI1f9NQSJPORHv3+8Y=","Date":"Fri, 6 Dec 2024 15:28:03 +0200","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"Kieran Bingham <kieran.bingham@ideasonboard.com>","Cc":"Hans de Goede <hdegoede@redhat.com>, libcamera-devel@lists.libcamera.org,\n\tMilan Zamazal <mzamazal@redhat.com>","Subject":"Re: [PATCH resend 5/5] libcamera: swstats_cpu: Add processFrame()\n\tmethod","Message-ID":"<20241206132803.GJ25902@pendragon.ideasonboard.com>","References":"<20241205192519.49104-1-hdegoede@redhat.com>\n\t<20241205192519.49104-6-hdegoede@redhat.com>\n\t<173349087522.3135963.11705101284708571797@ping.linuxembedded.co.uk>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<173349087522.3135963.11705101284708571797@ping.linuxembedded.co.uk>","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>","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":32583,"web_url":"https://patchwork.libcamera.org/comment/32583/","msgid":"<8734j0yky7.fsf@redhat.com>","date":"2024-12-06T13:56:00","subject":"Re: [PATCH resend 5/5] libcamera: swstats_cpu: Add processFrame()\n\tmethod","submitter":{"id":177,"url":"https://patchwork.libcamera.org/api/people/177/","name":"Milan Zamazal","email":"mzamazal@redhat.com"},"content":"Laurent Pinchart <laurent.pinchart@ideasonboard.com> writes:\n\n> On Fri, Dec 06, 2024 at 01:14:35PM +0000, Kieran Bingham wrote:\n>> Quoting Hans de Goede (2024-12-05 19:25:19)\n>> > Add a method to the SwstatsCpu class to process a whole Framebuffer in\n>\n>> > one go, rather then line by line. This is useful for gathering stats\n>> > when debayering is not necessary or is not done on the CPU.\n>> \n>> This is the bit I can see will be useful for any scenario where we don't\n>> have stats. I think for instance on RKISP1 when running raw mode we\n>> don't get stats, so something like this would be helpful to be able to\n>> support some AEGC control which would likely be beneficial for tuning or\n>> capturing raw streams.\n>\n> I'm in two minds about that. If it's only about tuning with ISPs that\n> can't generate statistics when capturing raw frames, an application-side\n> AE could be a better option. We should discuss it when the need arises\n> before writing code.\n\nAnother use, discussed in one of the recent software ISP syncs, would be\nto get statistics from CPU when doing debayering on a GPU.\n\nAnd when all I need is getting raw frames, let's say from `simple'\npipeline for debugging purposes or something similar, getting them\nreasonably exposed out of the box may be convenient.\n\n>> > Reviewed-by: Milan Zamazal <mzamazal@redhat.com>\n>> > Signed-off-by: Hans de Goede <hdegoede@redhat.com>\n>> > ---\n>> > Changes since the RFC:\n>> > - Make processFrame() call startFrame() and finishFrame() rather then\n>> >   making the caller do this\n>> > - Add doxygen documentation for processFrame()\n>> > ---\n>> >  .../internal/software_isp/swstats_cpu.h       | 12 +++++\n>> >  src/libcamera/software_isp/swstats_cpu.cpp    | 51 +++++++++++++++++++\n>> >  2 files changed, 63 insertions(+)\n>> > \n>> > diff --git a/include/libcamera/internal/software_isp/swstats_cpu.h b/include/libcamera/internal/software_isp/swstats_cpu.h\n>> > index 26a2f462..fa47cec9 100644\n>> > --- a/include/libcamera/internal/software_isp/swstats_cpu.h\n>> > +++ b/include/libcamera/internal/software_isp/swstats_cpu.h\n>> > @@ -18,12 +18,16 @@\n>> >  #include <libcamera/geometry.h>\n>> >  \n>> >  #include \"libcamera/internal/bayer_format.h\"\n>> > +#include \"libcamera/internal/framebuffer.h\"\n>> >  #include \"libcamera/internal/shared_mem_object.h\"\n>> >  #include \"libcamera/internal/software_isp/swisp_stats.h\"\n>> >  \n>> > +#include \"benchmark.h\"\n>> > +\n>> >  namespace libcamera {\n>> >  \n>> >  class PixelFormat;\n>> > +class MappedFrameBuffer;\n>> >  struct StreamConfiguration;\n>> >  \n>> >  class SwStatsCpu\n>> > @@ -42,6 +46,7 @@ public:\n>> >         void setWindow(const Rectangle &window);\n>> >         void startFrame();\n>> >         void finishFrame(uint32_t frame, uint32_t bufferId);\n>> > +       void processFrame(uint32_t frame, uint32_t bufferId, FrameBuffer *input);\n>> >  \n>> >         void processLine0(unsigned int y, const uint8_t *src[])\n>> >         {\n>> > @@ -65,6 +70,7 @@ public:\n>> >  \n>> >  private:\n>> >         using statsProcessFn = void (SwStatsCpu::*)(const uint8_t *src[]);\n>> > +       using processFrameFn = void (SwStatsCpu::*)(MappedFrameBuffer &in);\n>> >  \n>> >         int setupStandardBayerOrder(BayerFormat::Order order);\n>> >         /* Bayer 8 bpp unpacked */\n>> > @@ -77,6 +83,10 @@ private:\n>> >         void statsBGGR10PLine0(const uint8_t *src[]);\n>> >         void statsGBRG10PLine0(const uint8_t *src[]);\n>> >  \n>> > +       void processBayerFrame2(MappedFrameBuffer &in);\n>> \n>> Ayeee ... I shudder a little seeing 'Thing2'.\n>> \n>> Would this really just be processMappedBayerFrame() ?\n>> \n>> > +\n>> > +       processFrameFn processFrame_;\n>> > +\n>> >         /* Variables set by configure(), used every line */\n>> >         statsProcessFn stats0_;\n>> >         statsProcessFn stats2_;\n>> > @@ -89,9 +99,11 @@ private:\n>> >         Size patternSize_;\n>> >  \n>> >         unsigned int xShift_;\n>> > +       unsigned int stride_;\n>> >  \n>> >         SharedMemObject<SwIspStats> sharedStats_;\n>> >         SwIspStats stats_;\n>> > +       Benchmark bench_;\n>> >  };\n>> >  \n>> >  } /* namespace libcamera */\n>> > diff --git a/src/libcamera/software_isp/swstats_cpu.cpp b/src/libcamera/software_isp/swstats_cpu.cpp\n>> > index aa5654dc..1ff15f5b 100644\n>> > --- a/src/libcamera/software_isp/swstats_cpu.cpp\n>> > +++ b/src/libcamera/software_isp/swstats_cpu.cpp\n>> > @@ -16,6 +16,7 @@\n>> >  #include <libcamera/stream.h>\n>> >  \n>> >  #include \"libcamera/internal/bayer_format.h\"\n>> > +#include \"libcamera/internal/mapped_framebuffer.h\"\n>> >  \n>> >  namespace libcamera {\n>> >  \n>> > @@ -360,11 +361,14 @@ int SwStatsCpu::setupStandardBayerOrder(BayerFormat::Order order)\n>> >   */\n>> >  int SwStatsCpu::configure(const StreamConfiguration &inputCfg)\n>> >  {\n>> > +       stride_ = inputCfg.stride;\n>> > +\n>> >         BayerFormat bayerFormat =\n>> >                 BayerFormat::fromPixelFormat(inputCfg.pixelFormat);\n>> >  \n>> >         if (bayerFormat.packing == BayerFormat::Packing::None &&\n>> >             setupStandardBayerOrder(bayerFormat.order) == 0) {\n>> > +               processFrame_ = &SwStatsCpu::processBayerFrame2;\n>> >                 switch (bayerFormat.bitDepth) {\n>> >                 case 8:\n>> >                         stats0_ = &SwStatsCpu::statsBGGR8Line0;\n>> > @@ -385,6 +389,7 @@ int SwStatsCpu::configure(const StreamConfiguration &inputCfg)\n>> >                 /* Skip every 3th and 4th line, sample every other 2x2 block */\n>> >                 ySkipMask_ = 0x02;\n>> >                 xShift_ = 0;\n>> > +               processFrame_ = &SwStatsCpu::processBayerFrame2;\n>> >  \n>> >                 switch (bayerFormat.order) {\n>> >                 case BayerFormat::BGGR:\n>> > @@ -425,4 +430,50 @@ void SwStatsCpu::setWindow(const Rectangle &window)\n>> >         window_.height &= ~(patternSize_.height - 1);\n>> >  }\n>> >  \n>> > +void SwStatsCpu::processBayerFrame2(MappedFrameBuffer &in)\n>> > +{\n>> > +       const uint8_t *src = in.planes()[0].data();\n>> > +       const uint8_t *linePointers[3];\n>> > +\n>> > +       /* Adjust src for starting at window_.y */\n>> > +       src += window_.y * stride_;\n>> > +\n>> > +       for (unsigned int y = 0; y < window_.height; y += 2) {\n>> > +               if (y & ySkipMask_) {\n>> > +                       src += stride_ * 2;\n>> > +                       continue;\n>> > +               }\n>> > +\n>> > +               /* linePointers[0] is not used by any stats0_ functions */\n>> > +               linePointers[1] = src;\n>> > +               linePointers[2] = src + stride_;\n>> > +               (this->*stats0_)(linePointers);\n>> > +               src += stride_ * 2;\n>> > +       }\n>> > +}\n>> > +\n>> > +/**\n>> > + * \\brief Calculate statistics for a frame in one go\n>> > + * \\param[in] frame The frame number\n>> > + * \\param[in] bufferId ID of the statistics buffer\n>> > + * \\param[in] input The frame to process\n>> > + *\n>> > + * This may only be called after a successful setWindow() call.\n>> > + */\n>> > +void SwStatsCpu::processFrame(uint32_t frame, uint32_t bufferId, FrameBuffer *input)\n>> > +{\n>> > +       bench_.startFrame();\n>> > +       startFrame();\n>> > +\n>> > +       MappedFrameBuffer in(input, MappedFrameBuffer::MapFlag::Read);\n>> > +       if (!in.isValid()) {\n>> > +               LOG(SwStatsCpu, Error) << \"mmap-ing buffer(s) failed\";\n>> > +               return;\n>> > +       }\n>> > +\n>> > +       (this->*processFrame_)(in);\n>> \n>> I can't see why this goes through a function pointer at the moment. Is\n>> there some later patches that will have 'different' processFrame_\n>> implemenations?\n>> \n>> Or could SwStatsCpu::processBayerFrame2 just be inlined here ?\n>> \n>> (I'm fine keeping it separate if something else is about to use it, or\n>> change it soon).\n>> \n>> > +       finishFrame(frame, bufferId);\n>> > +       bench_.finishFrame();\n>> > +}\n>> > +\n>> >  } /* namespace libcamera */","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 4CDC4BE173\n\tfor <parsemail@patchwork.libcamera.org>;\n\tFri,  6 Dec 2024 13:56:09 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 31D7F66158;\n\tFri,  6 Dec 2024 14:56:08 +0100 (CET)","from us-smtp-delivery-124.mimecast.com\n\t(us-smtp-delivery-124.mimecast.com [170.10.133.124])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 8C7CE6614C\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tFri,  6 Dec 2024 14:56:06 +0100 (CET)","from mail-wr1-f72.google.com (mail-wr1-f72.google.com\n\t[209.85.221.72]) by relay.mimecast.com with ESMTP with STARTTLS\n\t(version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id\n\tus-mta-396-wYMyjr3yPZSPq30ULNU10Q-1; Fri, 06 Dec 2024 08:56:04 -0500","by mail-wr1-f72.google.com with SMTP id\n\tffacd0b85a97d-385dfa9b758so918473f8f.0\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tFri, 06 Dec 2024 05:56:04 -0800 (PST)","from nuthatch (ip-77-48-47-2.net.vodafone.cz. [77.48.47.2])\n\tby smtp.gmail.com with ESMTPSA id\n\tffacd0b85a97d-38621fbbd70sm4575428f8f.90.2024.12.06.05.56.01\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tFri, 06 Dec 2024 05:56:01 -0800 (PST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=redhat.com header.i=@redhat.com\n\theader.b=\"FSLlbd2z\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com;\n\ts=mimecast20190719; t=1733493365;\n\th=from:from:reply-to:subject:subject:date:date:message-id:message-id:\n\tto:to:cc:cc:mime-version:mime-version:content-type:content-type:\n\tin-reply-to:in-reply-to:references:references;\n\tbh=FTVV4z2c9k7vOSxZ4l1RBl0Alw7NfyxeeguCx0xMO9o=;\n\tb=FSLlbd2zr4/58Zkxhd88aXDrMztKVnFKWV7rcZqU/QyoXnxvau8djcaH2bcHN31VnL2V7E\n\tRsSfE8GSWyPf7uBK/nSuTcXWnUOhOBuOIexU949kprIGXwlgoetbrP4WbuiJUAw1Ym8MHL\n\tTIImA5jWzgHAYlL4KvW6/PhRB53fGkY=","X-MC-Unique":"wYMyjr3yPZSPq30ULNU10Q-1","X-Mimecast-MFC-AGG-ID":"wYMyjr3yPZSPq30ULNU10Q","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1733493362; x=1734098162;\n\th=mime-version:user-agent:message-id:date:references:in-reply-to\n\t:subject:cc:to:from:x-gm-message-state:from:to:cc:subject:date\n\t:message-id:reply-to;\n\tbh=FTVV4z2c9k7vOSxZ4l1RBl0Alw7NfyxeeguCx0xMO9o=;\n\tb=EH2POx4oDd0RFNM38b/FZkhOBGS4tEDKDtyVxyHLu5NoPKy6qDlkMl53DmsdTEPD96\n\tvwpXTX88MCe0UYEdQxmvcA9vTQ8iim3oweN6/BRibCz9+yuW6SmHTiWyOqCixSDM13za\n\tWqUglLyrm+tfpsLkPTjZ9wtoMdbppsC48/eXyvb/Z2MFzYimzTtHsVyPEQ7BYSnmqI0b\n\tln3+FxHIAl9+alE0UoiLBUDiYc0lbOslJBnA/sSCA0bGGqSf8cbuej8+h7aBFvR0/9QA\n\tpv7Mld9TMSB1HDzCIUjsfujBBqfR0M4/d1ADI07zYoim8cscWZQglTAb/vvvnNg9Drn2\n\tB5mg==","X-Forwarded-Encrypted":"i=1;\n\tAJvYcCXBUMjZ5+U+Fp69k4DJJzwhltnnDtSwahwlD79mgg8F+vcbsFMwxtU0bZG2qWQawRf8kqjJmgNRr9unVsRuay4=@lists.libcamera.org","X-Gm-Message-State":"AOJu0YycZN/qQu7lrTDHZ9dzD0FZsxtvMagi7dP62LQf0NS9xI0EJHuz\n\tTg+9FOJZv9TAtvXkRBKCrQ7cBqqmO+rVjM3AUWvfFLNXZS4h7FZ/au8hbkMA9g35T+5HsXn6zg/\n\t/StHgq6AGsf5d9OcQR3Wfsf1YoVgxPsYOLwK32qmFPUHAef8Y0phXx0HGsghTiwG79XNe1vn6yQ\n\tHc/6O75ffbLG7yIy3TI7HlS1cv1OuWjyWVS4DZphHEccfuR3fjbr2ZEg0=","X-Gm-Gg":"ASbGncvZRvmWmHIZhh0DYw6NiWgTtromOLsjeDKHdt4J3vFUArZou3J4+zCsQCgTC7C\n\tPODil5De/USqP97A+kR6cTH6Ph7W9pWt3JZhKt3gAjNRIrocIGk1K8S7x/sdIY+eq9W0dDUSagi\n\ton3W60tgekr+5NWB2WLQv0xvSDdy8cQG9xXhVa/Ie5/+zv1Zoom6pnxC5UuJJDWtVtef9ELQVXS\n\tk972QXLlaSGyv+Pl58cS1QfAXR3YMzC5v1zMODK/RbhlpXJo+LeL2/HWraBVxYNohscf7c=","X-Received":["by 2002:a05:6000:1a86:b0:385:df6b:7ef6 with SMTP id\n\tffacd0b85a97d-3862b3e5d4cmr2412284f8f.51.1733493362428; \n\tFri, 06 Dec 2024 05:56:02 -0800 (PST)","by 2002:a05:6000:1a86:b0:385:df6b:7ef6 with SMTP id\n\tffacd0b85a97d-3862b3e5d4cmr2412258f8f.51.1733493361971; \n\tFri, 06 Dec 2024 05:56:01 -0800 (PST)"],"X-Google-Smtp-Source":"AGHT+IHMlRnhEolhMmngzCKDHHZlSDc6/RtleQsEimRn26qlurhO3LmR8/ZyA4UZcFUCHOHCeoyqIA==","From":"Milan Zamazal <mzamazal@redhat.com>","To":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","Cc":"Kieran Bingham <kieran.bingham@ideasonboard.com>,  Hans de Goede\n\t<hdegoede@redhat.com>,  libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH resend 5/5] libcamera: swstats_cpu: Add processFrame()\n\tmethod","In-Reply-To":"<20241206132803.GJ25902@pendragon.ideasonboard.com> (Laurent\n\tPinchart's message of \"Fri, 6 Dec 2024 15:28:03 +0200\")","References":"<20241205192519.49104-1-hdegoede@redhat.com>\n\t<20241205192519.49104-6-hdegoede@redhat.com>\n\t<173349087522.3135963.11705101284708571797@ping.linuxembedded.co.uk>\n\t<20241206132803.GJ25902@pendragon.ideasonboard.com>","Date":"Fri, 06 Dec 2024 14:56:00 +0100","Message-ID":"<8734j0yky7.fsf@redhat.com>","User-Agent":"Gnus/5.13 (Gnus v5.13)","MIME-Version":"1.0","X-Mimecast-Spam-Score":"0","X-Mimecast-MFC-PROC-ID":"YKhzKtK167-jc82HUhd4oJNpIBAWVXjqaGHNfmav-YQ_1733493363","X-Mimecast-Originator":"redhat.com","Content-Type":"text/plain","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>","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":32607,"web_url":"https://patchwork.libcamera.org/comment/32607/","msgid":"<20241206181852.GE17570@pendragon.ideasonboard.com>","date":"2024-12-06T18:18:52","subject":"Re: [PATCH resend 5/5] libcamera: swstats_cpu: Add processFrame()\n\tmethod","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Fri, Dec 06, 2024 at 02:56:00PM +0100, Milan Zamazal wrote:\n> Laurent Pinchart writes:\n> > On Fri, Dec 06, 2024 at 01:14:35PM +0000, Kieran Bingham wrote:\n> >> Quoting Hans de Goede (2024-12-05 19:25:19)\n> >> > Add a method to the SwstatsCpu class to process a whole Framebuffer in\n> >\n> >> > one go, rather then line by line. This is useful for gathering stats\n> >> > when debayering is not necessary or is not done on the CPU.\n> >> \n> >> This is the bit I can see will be useful for any scenario where we don't\n> >> have stats. I think for instance on RKISP1 when running raw mode we\n> >> don't get stats, so something like this would be helpful to be able to\n> >> support some AEGC control which would likely be beneficial for tuning or\n> >> capturing raw streams.\n> >\n> > I'm in two minds about that. If it's only about tuning with ISPs that\n> > can't generate statistics when capturing raw frames, an application-side\n> > AE could be a better option. We should discuss it when the need arises\n> > before writing code.\n> \n> Another use, discussed in one of the recent software ISP syncs, would be\n> to get statistics from CPU when doing debayering on a GPU.\n\nThat use case is an important one. I hope we'll eventualy get the GPU to\ncalculates stats too, but in the interim period the CPU is useful.\n\n> And when all I need is getting raw frames, let's say from `simple'\n> pipeline for debugging purposes or something similar, getting them\n> reasonably exposed out of the box may be convenient.\n\nIn that case I'd expect the frames to go through the software ISP, and\nif the user only wants raw frames, we would \"configure\" the software ISP\nto only produce statistics, not processed frames.\n\n> >> > Reviewed-by: Milan Zamazal <mzamazal@redhat.com>\n> >> > Signed-off-by: Hans de Goede <hdegoede@redhat.com>\n> >> > ---\n> >> > Changes since the RFC:\n> >> > - Make processFrame() call startFrame() and finishFrame() rather then\n> >> >   making the caller do this\n> >> > - Add doxygen documentation for processFrame()\n> >> > ---\n> >> >  .../internal/software_isp/swstats_cpu.h       | 12 +++++\n> >> >  src/libcamera/software_isp/swstats_cpu.cpp    | 51 +++++++++++++++++++\n> >> >  2 files changed, 63 insertions(+)\n> >> > \n> >> > diff --git a/include/libcamera/internal/software_isp/swstats_cpu.h b/include/libcamera/internal/software_isp/swstats_cpu.h\n> >> > index 26a2f462..fa47cec9 100644\n> >> > --- a/include/libcamera/internal/software_isp/swstats_cpu.h\n> >> > +++ b/include/libcamera/internal/software_isp/swstats_cpu.h\n> >> > @@ -18,12 +18,16 @@\n> >> >  #include <libcamera/geometry.h>\n> >> >  \n> >> >  #include \"libcamera/internal/bayer_format.h\"\n> >> > +#include \"libcamera/internal/framebuffer.h\"\n> >> >  #include \"libcamera/internal/shared_mem_object.h\"\n> >> >  #include \"libcamera/internal/software_isp/swisp_stats.h\"\n> >> >  \n> >> > +#include \"benchmark.h\"\n> >> > +\n> >> >  namespace libcamera {\n> >> >  \n> >> >  class PixelFormat;\n> >> > +class MappedFrameBuffer;\n> >> >  struct StreamConfiguration;\n> >> >  \n> >> >  class SwStatsCpu\n> >> > @@ -42,6 +46,7 @@ public:\n> >> >         void setWindow(const Rectangle &window);\n> >> >         void startFrame();\n> >> >         void finishFrame(uint32_t frame, uint32_t bufferId);\n> >> > +       void processFrame(uint32_t frame, uint32_t bufferId, FrameBuffer *input);\n> >> >  \n> >> >         void processLine0(unsigned int y, const uint8_t *src[])\n> >> >         {\n> >> > @@ -65,6 +70,7 @@ public:\n> >> >  \n> >> >  private:\n> >> >         using statsProcessFn = void (SwStatsCpu::*)(const uint8_t *src[]);\n> >> > +       using processFrameFn = void (SwStatsCpu::*)(MappedFrameBuffer &in);\n> >> >  \n> >> >         int setupStandardBayerOrder(BayerFormat::Order order);\n> >> >         /* Bayer 8 bpp unpacked */\n> >> > @@ -77,6 +83,10 @@ private:\n> >> >         void statsBGGR10PLine0(const uint8_t *src[]);\n> >> >         void statsGBRG10PLine0(const uint8_t *src[]);\n> >> >  \n> >> > +       void processBayerFrame2(MappedFrameBuffer &in);\n> >> \n> >> Ayeee ... I shudder a little seeing 'Thing2'.\n> >> \n> >> Would this really just be processMappedBayerFrame() ?\n> >> \n> >> > +\n> >> > +       processFrameFn processFrame_;\n> >> > +\n> >> >         /* Variables set by configure(), used every line */\n> >> >         statsProcessFn stats0_;\n> >> >         statsProcessFn stats2_;\n> >> > @@ -89,9 +99,11 @@ private:\n> >> >         Size patternSize_;\n> >> >  \n> >> >         unsigned int xShift_;\n> >> > +       unsigned int stride_;\n> >> >  \n> >> >         SharedMemObject<SwIspStats> sharedStats_;\n> >> >         SwIspStats stats_;\n> >> > +       Benchmark bench_;\n> >> >  };\n> >> >  \n> >> >  } /* namespace libcamera */\n> >> > diff --git a/src/libcamera/software_isp/swstats_cpu.cpp b/src/libcamera/software_isp/swstats_cpu.cpp\n> >> > index aa5654dc..1ff15f5b 100644\n> >> > --- a/src/libcamera/software_isp/swstats_cpu.cpp\n> >> > +++ b/src/libcamera/software_isp/swstats_cpu.cpp\n> >> > @@ -16,6 +16,7 @@\n> >> >  #include <libcamera/stream.h>\n> >> >  \n> >> >  #include \"libcamera/internal/bayer_format.h\"\n> >> > +#include \"libcamera/internal/mapped_framebuffer.h\"\n> >> >  \n> >> >  namespace libcamera {\n> >> >  \n> >> > @@ -360,11 +361,14 @@ int SwStatsCpu::setupStandardBayerOrder(BayerFormat::Order order)\n> >> >   */\n> >> >  int SwStatsCpu::configure(const StreamConfiguration &inputCfg)\n> >> >  {\n> >> > +       stride_ = inputCfg.stride;\n> >> > +\n> >> >         BayerFormat bayerFormat =\n> >> >                 BayerFormat::fromPixelFormat(inputCfg.pixelFormat);\n> >> >  \n> >> >         if (bayerFormat.packing == BayerFormat::Packing::None &&\n> >> >             setupStandardBayerOrder(bayerFormat.order) == 0) {\n> >> > +               processFrame_ = &SwStatsCpu::processBayerFrame2;\n> >> >                 switch (bayerFormat.bitDepth) {\n> >> >                 case 8:\n> >> >                         stats0_ = &SwStatsCpu::statsBGGR8Line0;\n> >> > @@ -385,6 +389,7 @@ int SwStatsCpu::configure(const StreamConfiguration &inputCfg)\n> >> >                 /* Skip every 3th and 4th line, sample every other 2x2 block */\n> >> >                 ySkipMask_ = 0x02;\n> >> >                 xShift_ = 0;\n> >> > +               processFrame_ = &SwStatsCpu::processBayerFrame2;\n> >> >  \n> >> >                 switch (bayerFormat.order) {\n> >> >                 case BayerFormat::BGGR:\n> >> > @@ -425,4 +430,50 @@ void SwStatsCpu::setWindow(const Rectangle &window)\n> >> >         window_.height &= ~(patternSize_.height - 1);\n> >> >  }\n> >> >  \n> >> > +void SwStatsCpu::processBayerFrame2(MappedFrameBuffer &in)\n> >> > +{\n> >> > +       const uint8_t *src = in.planes()[0].data();\n> >> > +       const uint8_t *linePointers[3];\n> >> > +\n> >> > +       /* Adjust src for starting at window_.y */\n> >> > +       src += window_.y * stride_;\n> >> > +\n> >> > +       for (unsigned int y = 0; y < window_.height; y += 2) {\n> >> > +               if (y & ySkipMask_) {\n> >> > +                       src += stride_ * 2;\n> >> > +                       continue;\n> >> > +               }\n> >> > +\n> >> > +               /* linePointers[0] is not used by any stats0_ functions */\n> >> > +               linePointers[1] = src;\n> >> > +               linePointers[2] = src + stride_;\n> >> > +               (this->*stats0_)(linePointers);\n> >> > +               src += stride_ * 2;\n> >> > +       }\n> >> > +}\n> >> > +\n> >> > +/**\n> >> > + * \\brief Calculate statistics for a frame in one go\n> >> > + * \\param[in] frame The frame number\n> >> > + * \\param[in] bufferId ID of the statistics buffer\n> >> > + * \\param[in] input The frame to process\n> >> > + *\n> >> > + * This may only be called after a successful setWindow() call.\n> >> > + */\n> >> > +void SwStatsCpu::processFrame(uint32_t frame, uint32_t bufferId, FrameBuffer *input)\n> >> > +{\n> >> > +       bench_.startFrame();\n> >> > +       startFrame();\n> >> > +\n> >> > +       MappedFrameBuffer in(input, MappedFrameBuffer::MapFlag::Read);\n> >> > +       if (!in.isValid()) {\n> >> > +               LOG(SwStatsCpu, Error) << \"mmap-ing buffer(s) failed\";\n> >> > +               return;\n> >> > +       }\n> >> > +\n> >> > +       (this->*processFrame_)(in);\n> >> \n> >> I can't see why this goes through a function pointer at the moment. Is\n> >> there some later patches that will have 'different' processFrame_\n> >> implemenations?\n> >> \n> >> Or could SwStatsCpu::processBayerFrame2 just be inlined here ?\n> >> \n> >> (I'm fine keeping it separate if something else is about to use it, or\n> >> change it soon).\n> >> \n> >> > +       finishFrame(frame, bufferId);\n> >> > +       bench_.finishFrame();\n> >> > +}\n> >> > +\n> >> >  } /* namespace libcamera */","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 821FABE173\n\tfor <parsemail@patchwork.libcamera.org>;\n\tFri,  6 Dec 2024 18:19:08 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A4E2D67E3B;\n\tFri,  6 Dec 2024 19:19:07 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 49C9567E24\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tFri,  6 Dec 2024 19:19:06 +0100 (CET)","from pendragon.ideasonboard.com (81-175-209-231.bb.dnainternet.fi\n\t[81.175.209.231])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 5D9C4641;\n\tFri,  6 Dec 2024 19:18:36 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"KJnqIo8e\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1733509116;\n\tbh=jyCLUVvvGPn0v9uQwOjmjGI5VseP4P4zzWUczYNqaGE=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=KJnqIo8e834cLMlCbz6QsQlZPcD3rFvizOIzEIFYGcpYY/wWHLzz2KmKeQo2MBg8C\n\tLiN/fMXJrjTP8Kwtm9mmPwXafrdvEAsKhXaXqGpTbMyzWx05i1Kzx5eouOrEYqG1he\n\t82PGGadisMS9mvhqlrOIZ7v3JuPMejB7KFxBJ+Bw=","Date":"Fri, 6 Dec 2024 20:18:52 +0200","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"Milan Zamazal <mzamazal@redhat.com>","Cc":"Kieran Bingham <kieran.bingham@ideasonboard.com>,\n\tHans de Goede <hdegoede@redhat.com>, libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH resend 5/5] libcamera: swstats_cpu: Add processFrame()\n\tmethod","Message-ID":"<20241206181852.GE17570@pendragon.ideasonboard.com>","References":"<20241205192519.49104-1-hdegoede@redhat.com>\n\t<20241205192519.49104-6-hdegoede@redhat.com>\n\t<173349087522.3135963.11705101284708571797@ping.linuxembedded.co.uk>\n\t<20241206132803.GJ25902@pendragon.ideasonboard.com>\n\t<8734j0yky7.fsf@redhat.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<8734j0yky7.fsf@redhat.com>","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":34166,"web_url":"https://patchwork.libcamera.org/comment/34166/","msgid":"<264e452b-a4b3-42cf-a0e1-86d364fe7d92@redhat.com>","date":"2025-05-09T11:31:25","subject":"Re: [PATCH resend 5/5] libcamera: swstats_cpu: Add processFrame()\n\tmethod","submitter":{"id":102,"url":"https://patchwork.libcamera.org/api/people/102/","name":"Hans de Goede","email":"hdegoede@redhat.com"},"content":"Hi Kieran,\n\nI realize this series / your comments are quite old now,\nbut I finally have some time to rebase this and work on\nit more.\n\nOn 6-Dec-24 2:14 PM, Kieran Bingham wrote:\n> Quoting Hans de Goede (2024-12-05 19:25:19)\n>> Add a method to the SwstatsCpu class to process a whole Framebuffer in\n>> one go, rather then line by line. This is useful for gathering stats\n>> when debayering is not necessary or is not done on the CPU.\n> \n> This is the bit I can see will be useful for any scenario where we don't\n> have stats. I think for instance on RKISP1 when running raw mode we\n> don't get stats, so something like this would be helpful to be able to\n> support some AEGC control which would likely be beneficial for tuning or\n> capturing raw streams.\n> \n>> Reviewed-by: Milan Zamazal <mzamazal@redhat.com>\n>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>\n>> ---\n>> Changes since the RFC:\n>> - Make processFrame() call startFrame() and finishFrame() rather then\n>>   making the caller do this\n>> - Add doxygen documentation for processFrame()\n>> ---\n>>  .../internal/software_isp/swstats_cpu.h       | 12 +++++\n>>  src/libcamera/software_isp/swstats_cpu.cpp    | 51 +++++++++++++++++++\n>>  2 files changed, 63 insertions(+)\n>>\n>> diff --git a/include/libcamera/internal/software_isp/swstats_cpu.h b/include/libcamera/internal/software_isp/swstats_cpu.h\n>> index 26a2f462..fa47cec9 100644\n>> --- a/include/libcamera/internal/software_isp/swstats_cpu.h\n>> +++ b/include/libcamera/internal/software_isp/swstats_cpu.h\n>> @@ -18,12 +18,16 @@\n>>  #include <libcamera/geometry.h>\n>>  \n>>  #include \"libcamera/internal/bayer_format.h\"\n>> +#include \"libcamera/internal/framebuffer.h\"\n>>  #include \"libcamera/internal/shared_mem_object.h\"\n>>  #include \"libcamera/internal/software_isp/swisp_stats.h\"\n>>  \n>> +#include \"benchmark.h\"\n>> +\n>>  namespace libcamera {\n>>  \n>>  class PixelFormat;\n>> +class MappedFrameBuffer;\n>>  struct StreamConfiguration;\n>>  \n>>  class SwStatsCpu\n>> @@ -42,6 +46,7 @@ public:\n>>         void setWindow(const Rectangle &window);\n>>         void startFrame();\n>>         void finishFrame(uint32_t frame, uint32_t bufferId);\n>> +       void processFrame(uint32_t frame, uint32_t bufferId, FrameBuffer *input);\n>>  \n>>         void processLine0(unsigned int y, const uint8_t *src[])\n>>         {\n>> @@ -65,6 +70,7 @@ public:\n>>  \n>>  private:\n>>         using statsProcessFn = void (SwStatsCpu::*)(const uint8_t *src[]);\n>> +       using processFrameFn = void (SwStatsCpu::*)(MappedFrameBuffer &in);\n>>  \n>>         int setupStandardBayerOrder(BayerFormat::Order order);\n>>         /* Bayer 8 bpp unpacked */\n>> @@ -77,6 +83,10 @@ private:\n>>         void statsBGGR10PLine0(const uint8_t *src[]);\n>>         void statsGBRG10PLine0(const uint8_t *src[]);\n>>  \n>> +       void processBayerFrame2(MappedFrameBuffer &in);\n> \n> Ayeee ... I shudder a little seeing 'Thing2'.\n> \n> Would this really just be processMappedBayerFrame() ?\n\nThe \"2\" refers to the number of lines in the bayer pattern\nbefore it repeats itself. There are also some RGB+Ir combined\nsensors where the pattern repeats every 4 lines, these will\nget a processBayerFrame4() function.\n\nAnd likewise there also will be separate process frame functions\nfor planar YUV vs packed YUV/RGB.\n\n...\n\n>> diff --git a/src/libcamera/software_isp/swstats_cpu.cpp b/src/libcamera/software_isp/swstats_cpu.cpp\n>> index aa5654dc..1ff15f5b 100644\n>> --- a/src/libcamera/software_isp/swstats_cpu.cpp\n>> +++ b/src/libcamera/software_isp/swstats_cpu.cpp\n\n...\n\n>> +/**\n>> + * \\brief Calculate statistics for a frame in one go\n>> + * \\param[in] frame The frame number\n>> + * \\param[in] bufferId ID of the statistics buffer\n>> + * \\param[in] input The frame to process\n>> + *\n>> + * This may only be called after a successful setWindow() call.\n>> + */\n>> +void SwStatsCpu::processFrame(uint32_t frame, uint32_t bufferId, FrameBuffer *input)\n>> +{\n>> +       bench_.startFrame();\n>> +       startFrame();\n>> +\n>> +       MappedFrameBuffer in(input, MappedFrameBuffer::MapFlag::Read);\n>> +       if (!in.isValid()) {\n>> +               LOG(SwStatsCpu, Error) << \"mmap-ing buffer(s) failed\";\n>> +               return;\n>> +       }\n>> +\n>> +       (this->*processFrame_)(in);\n> \n> I can't see why this goes through a function pointer at the moment. Is\n> there some later patches that will have 'different' processFrame_\n> implemenations?\n\nYes, as mentioned above there will be separate processFrame_ functions\nfor planar YUV + packed YUV/RGB. The full atomisp patch-series this\nis part of adds a processYUV420Frame() function and configure()\nwill set this pointer to one or the other depending on the pixelfmt.\n\nSo I plan to keep this as is for the new atomisp pipelinehandler\nseries which I hope to post soon.\n\nRegards,\n\nHans","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 3D723C3200\n\tfor <parsemail@patchwork.libcamera.org>;\n\tFri,  9 May 2025 11:31:33 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 6DAFC68B45;\n\tFri,  9 May 2025 13:31:32 +0200 (CEST)","from us-smtp-delivery-124.mimecast.com\n\t(us-smtp-delivery-124.mimecast.com [170.10.129.124])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id EC1B268B25\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tFri,  9 May 2025 13:31:30 +0200 (CEST)","from mail-ej1-f69.google.com (mail-ej1-f69.google.com\n\t[209.85.218.69]) by relay.mimecast.com with ESMTP with STARTTLS\n\t(version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id\n\tus-mta-563-Hrb-BIyFNBmUrcp_Qjgy1Q-1; Fri, 09 May 2025 07:31:28 -0400","by mail-ej1-f69.google.com with SMTP id\n\ta640c23a62f3a-acbbb00099eso190501266b.2\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tFri, 09 May 2025 04:31:28 -0700 (PDT)","from ?IPV6:2001:1c00:c32:7800:5bfa:a036:83f0:f9ec?\n\t(2001-1c00-0c32-7800-5bfa-a036-83f0-f9ec.cable.dynamic.v6.ziggo.nl.\n\t[2001:1c00:c32:7800:5bfa:a036:83f0:f9ec])\n\tby smtp.gmail.com with ESMTPSA id\n\t4fb4d7f45d1cf-5fc9d700e6fsm1235600a12.62.2025.05.09.04.31.25\n\t(version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128);\n\tFri, 09 May 2025 04:31:25 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=redhat.com header.i=@redhat.com\n\theader.b=\"fr5dckHD\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com;\n\ts=mimecast20190719; t=1746790289;\n\th=from:from:reply-to:subject:subject:date:date:message-id:message-id:\n\tto:to:cc:cc:mime-version:mime-version:content-type:content-type:\n\tcontent-transfer-encoding:content-transfer-encoding:\n\tin-reply-to:in-reply-to:references:references;\n\tbh=RGt593lh3B90PBJBP5bJRDch3g5MXFSyMNos0o9dZ10=;\n\tb=fr5dckHDFBDOFb0kapkx3Q/otN17M/8KEMRJnHO64KL1+aSD5GjdFDDpIrZaPFhBwEWIe6\n\tIK/I3Pu+eaOTegx3g4jiMv31qocn1g/d/CN9j7c1TIha8EPLB9DSGLNhDcrntw0CjEWR3C\n\tBvDl6BgqsTB8JBXzRyHTyEJ6Ei8Ogm0=","X-MC-Unique":"Hrb-BIyFNBmUrcp_Qjgy1Q-1","X-Mimecast-MFC-AGG-ID":"Hrb-BIyFNBmUrcp_Qjgy1Q_1746790287","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1746790287; x=1747395087;\n\th=content-transfer-encoding:in-reply-to:from:content-language\n\t:references:cc:to:subject:user-agent:mime-version:date:message-id\n\t:x-gm-message-state:from:to:cc:subject:date:message-id:reply-to;\n\tbh=RGt593lh3B90PBJBP5bJRDch3g5MXFSyMNos0o9dZ10=;\n\tb=EpQgJLs30jqMQZnmN6qL69W9aPXTkEAOWGabiLWsb21uNOo6VSDw7M2orTTGy/aUrB\n\tfTG+kCVisvJXBXu9ZXWAzQB/Dtaz4sDLzhqCHZYX5F1vTGcE19km+CVF0pkTY2YkvHMM\n\tv658nilHInWJj5oEkcQ2RAYqmeJp7FHhDD3+r2Vgn/n4l76o1d/0nVp1XX1pZptkrfyP\n\t4qkrkWsr3dZaGliAvqwrtG/aIvDWdlowBtR7GZLZLhDhZ2UFmBPwSt7Xu93dlUmcAs5t\n\tMrCutiXJhAINQ4rwIE/jBCG+KzKWdt0j5VV6MXmGeOggCJsNkuM2eZPfc/2FTQhFm3NW\n\tVvSA==","X-Forwarded-Encrypted":"i=1;\n\tAJvYcCVb/AvyiLetRNzKzpE1BYmL5aRGtiFVV5bSvAv+lioSOC8HKecEv37YsFrTt96dIAHHV1OILs+tF1OTNAlECic=@lists.libcamera.org","X-Gm-Message-State":"AOJu0Yy94O/sYgSJk0MR0s29oSsgmyO3Cn7oPXwGgiHXLY6fHCf0SLGZ\n\twaNHfNBIk4dTmT7I8a62QzZ/OPCsi8yKuM1ZusIaebTAYpksm4N5USbv2QGAQnMA8Lwk3DBuVfG\n\tECizJS6LLk7eabFklVPfBt9A7SoqvA4U9OHah5efXmiV4/1xJCZ+qkvT6zy324Ebreg/C+7c=","X-Gm-Gg":"ASbGncvQzGlV7yxTcI7jeA+556BF/NLYu3ERKv7pOr6DPNzLr+LeEHYpT5ssUwuq9Q1\n\tlh0h/u/HyDZea2ccPNM2pJiYpXbYRMg+ba/bJgGkbXspbkbfCZtNQ7yURllpTbcEx1cosfpxQWj\n\tlAnwrEHYiQlsUXO4mV1DbDKjRy9O/kI1dL4JQo10aPaL+RiHKZ0su2ohsU5b3Kg2Z7BRIcYZW8S\n\te8O6Y1jw7NFlmsL/wFXbZ7bd4cSBWu/XET3YO+JGSrLPmC/OGf3Mzsd9DyUjO9xsVWdwMXPok/I\n\tjKUroLsg29/mhA16c/RsnZYOJ/MhOrsU0RPDP04oH5QoLdC/qtZpQ+xbsjxKPVMUjIWAz3N7ZRr\n\t3SDP8y6ecSj2nOcvbqY/CO6YWc2qAOszps5jLODyYJ/a5aL1osmmpFdprP29OFA==","X-Received":["by 2002:a17:907:a0ca:b0:acb:b267:436b with SMTP id\n\ta640c23a62f3a-ad21900181amr326033066b.25.1746790287188; \n\tFri, 09 May 2025 04:31:27 -0700 (PDT)","by 2002:a17:907:a0ca:b0:acb:b267:436b with SMTP id\n\ta640c23a62f3a-ad21900181amr326030266b.25.1746790286750; \n\tFri, 09 May 2025 04:31:26 -0700 (PDT)"],"X-Google-Smtp-Source":"AGHT+IFYsbg5gVIaraoElDRI4gPoydVplPO9BjosRhg2VnIhflg0TYYXmzpJZemaS3QGEy/brXqCwA==","Message-ID":"<264e452b-a4b3-42cf-a0e1-86d364fe7d92@redhat.com>","Date":"Fri, 9 May 2025 13:31:25 +0200","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH resend 5/5] libcamera: swstats_cpu: Add processFrame()\n\tmethod","To":"Kieran Bingham <kieran.bingham@ideasonboard.com>,\n\tlibcamera-devel@lists.libcamera.org","Cc":"Milan Zamazal <mzamazal@redhat.com>","References":"<20241205192519.49104-1-hdegoede@redhat.com>\n\t<20241205192519.49104-6-hdegoede@redhat.com>\n\t<173349087522.3135963.11705101284708571797@ping.linuxembedded.co.uk>","From":"Hans de Goede <hdegoede@redhat.com>","In-Reply-To":"<173349087522.3135963.11705101284708571797@ping.linuxembedded.co.uk>","X-Mimecast-Spam-Score":"0","X-Mimecast-MFC-PROC-ID":"aQVmJwdpMzvNgedp_ldFaV7tUwxpdY5d6myE39DdN0Q_1746790287","X-Mimecast-Originator":"redhat.com","Content-Language":"en-US, nl","Content-Type":"text/plain; charset=UTF-8","Content-Transfer-Encoding":"7bit","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>","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":34167,"web_url":"https://patchwork.libcamera.org/comment/34167/","msgid":"<f57a842e-a26d-451e-a8d9-a8c31b17ee78@redhat.com>","date":"2025-05-09T11:44:08","subject":"Re: [PATCH resend 5/5] libcamera: swstats_cpu: Add processFrame()\n\tmethod","submitter":{"id":102,"url":"https://patchwork.libcamera.org/api/people/102/","name":"Hans de Goede","email":"hdegoede@redhat.com"},"content":"Hi,\n\nOn 6-Dec-24 7:18 PM, Laurent Pinchart wrote:\n> On Fri, Dec 06, 2024 at 02:56:00PM +0100, Milan Zamazal wrote:\n>> Laurent Pinchart writes:\n>>> On Fri, Dec 06, 2024 at 01:14:35PM +0000, Kieran Bingham wrote:\n>>>> Quoting Hans de Goede (2024-12-05 19:25:19)\n>>>>> Add a method to the SwstatsCpu class to process a whole Framebuffer in\n>>>\n>>>>> one go, rather then line by line. This is useful for gathering stats\n>>>>> when debayering is not necessary or is not done on the CPU.\n>>>>\n>>>> This is the bit I can see will be useful for any scenario where we don't\n>>>> have stats. I think for instance on RKISP1 when running raw mode we\n>>>> don't get stats, so something like this would be helpful to be able to\n>>>> support some AEGC control which would likely be beneficial for tuning or\n>>>> capturing raw streams.\n>>>\n>>> I'm in two minds about that. If it's only about tuning with ISPs that\n>>> can't generate statistics when capturing raw frames, an application-side\n>>> AE could be a better option. We should discuss it when the need arises\n>>> before writing code.\n>>\n>> Another use, discussed in one of the recent software ISP syncs, would be\n>> to get statistics from CPU when doing debayering on a GPU.\n> \n> That use case is an important one. I hope we'll eventualy get the GPU to\n> calculates stats too, but in the interim period the CPU is useful.\n> \n>> And when all I need is getting raw frames, let's say from `simple'\n>> pipeline for debugging purposes or something similar, getting them\n>> reasonably exposed out of the box may be convenient.\n> \n> In that case I'd expect the frames to go through the software ISP, and\n> if the user only wants raw frames, we would \"configure\" the software ISP\n> to only produce statistics, not processed frames.\n\nRight, but even in that case we still need the processFrame() method\nfor doing stats on a full frame of raw bayer data. ATM the statistics\nare done 2 lines at a time driven by the debayer code, so that\nthe bayer code operates on src data which is still hot in the cache\nbecause of the stats code just having read it.\n\nWithout actually calling the main debayer loop since we don't want\nto debayer we will need another loop which just calls the existing\nline based statistics code on all lines (all lines in the statistics\nwindow). This other loop is what this patch provides.\n\nRegards,\n\nHans","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 C133BC3226\n\tfor <parsemail@patchwork.libcamera.org>;\n\tFri,  9 May 2025 11:44:17 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id CECE868B42;\n\tFri,  9 May 2025 13:44:16 +0200 (CEST)","from us-smtp-delivery-124.mimecast.com\n\t(us-smtp-delivery-124.mimecast.com [170.10.133.124])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 9B5D268B25\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tFri,  9 May 2025 13:44:14 +0200 (CEST)","from mail-ej1-f71.google.com (mail-ej1-f71.google.com\n\t[209.85.218.71]) by relay.mimecast.com with ESMTP with STARTTLS\n\t(version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id\n\tus-mta-622-VntuKM46Ph-1QFE0B2jgwA-1; Fri, 09 May 2025 07:44:11 -0400","by mail-ej1-f71.google.com with SMTP id\n\ta640c23a62f3a-ad22c5408e7so6264766b.3\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tFri, 09 May 2025 04:44:11 -0700 (PDT)","from ?IPV6:2001:1c00:c32:7800:5bfa:a036:83f0:f9ec?\n\t(2001-1c00-0c32-7800-5bfa-a036-83f0-f9ec.cable.dynamic.v6.ziggo.nl.\n\t[2001:1c00:c32:7800:5bfa:a036:83f0:f9ec])\n\tby smtp.gmail.com with ESMTPSA id\n\ta640c23a62f3a-ad219348781sm139338066b.61.2025.05.09.04.44.09\n\t(version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128);\n\tFri, 09 May 2025 04:44:09 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=redhat.com header.i=@redhat.com\n\theader.b=\"NK96WmQ8\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com;\n\ts=mimecast20190719; t=1746791053;\n\th=from:from:reply-to:subject:subject:date:date:message-id:message-id:\n\tto:to:cc:cc:mime-version:mime-version:content-type:content-type:\n\tcontent-transfer-encoding:content-transfer-encoding:\n\tin-reply-to:in-reply-to:references:references;\n\tbh=kV41CpgsPcYoryIGT/yrIB6e9/pFgSh21BEOJ7LK3Q8=;\n\tb=NK96WmQ8PuQeKWtsxw6UicmgOVeFyrA3e53IFm48rxzM84yTWf5RMXu4Y7Csts0BiLerJR\n\ti6wNr3VmFvQZUFwg2VvH2KLHEKmpEcc+Rmu1BF+I3OHusAUPdxgRlUFIf8WMniE6Mp38Zy\n\tZ21CNPphkeyLJ4jyZEX2RXjZ3CQDC0A=","X-MC-Unique":"VntuKM46Ph-1QFE0B2jgwA-1","X-Mimecast-MFC-AGG-ID":"VntuKM46Ph-1QFE0B2jgwA_1746791050","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1746791050; x=1747395850;\n\th=content-transfer-encoding:in-reply-to:from:content-language\n\t:references:cc:to:subject:user-agent:mime-version:date:message-id\n\t:x-gm-message-state:from:to:cc:subject:date:message-id:reply-to;\n\tbh=kV41CpgsPcYoryIGT/yrIB6e9/pFgSh21BEOJ7LK3Q8=;\n\tb=UNgi2wglk1hxZDbeVa6P8+GFn2PUqnlccf/huKoedVrqjduURwTx+576p/OpSj8T/I\n\t4D/hObDNUoTSpYYolF9Gk2XOB4ISH/CH6bXE3vuqbMLfvAiXDssjjO6mJKeYO1GfwPvF\n\tHb4HQUElYYmEJ7BlekQr8ir3dwygQ4m9l8FzYs+hlskvsUp3w9RDds1dC7JmyKfJT0FH\n\tKTSwIveFjvWLa/98G1KcVL6xj+P7IeIAV0RsUy6PvE+pJLel/rVxjByM5qhoIMEFxxM+\n\t+UC8tNnz0L2LKXNqCTeCOvPilXKKtMV7iKsE0KCSG1hbnc6C3K60FnMx2v4eDrFDWLcu\n\tAIUA==","X-Forwarded-Encrypted":"i=1;\n\tAJvYcCVpj97dIXADyQGgslX4HphDZmUGXsuhjZUjzLd+JU5Dj1oLNAIPpMmJgmt8fAFt+xfBh9SNyUzq4YwSUDJxa+0=@lists.libcamera.org","X-Gm-Message-State":"AOJu0Yy3zszKALJW+nqfJVmSVq47+Ros9dQDZrYn+K3XkyDKSlg3RGGP\n\tFecY2jf6UuiEfQOSkTOszAeLuKdXQQOHa+TpV/5wpl0985YhUkMoqG2Ghfa06ME6Wc+wMi9Ck7T\n\t1TuNMcHnGZNrDjMHnCgXRG+8cAU9wrePJ1uMaURX4Kg3j4Zp9rK4SYxlgTDQJI+ogDEeSKJI=","X-Gm-Gg":"ASbGncuDDh01jLZQgBMT434hzas8ODf+uGE/SSuKhpzBPznUmifqDl/1E/31TGw0091\n\tAj+LyppiR3jk4eI4WNp7Aee/E3c4lonWyDDwWk5sJGL9bFMlZJ4jpM2X1TtVpxSIQYfSTPi2OSO\n\tmMFuEFr9EtrmrSxFe1ioYqbUeKgX8LcqK+Fev4aHHBhlT9D8ddmkt1xDHlU1ML2POTcIvUMcNTJ\n\tf+EI/0LGLkySOH+Qr1h1zKIBnqrIaRk6MXm4k1IOPoRdhhpiFcjzGF+43Y4C05I0cgfzIUBos++\n\tRP7IW3c4ifU3D3CY8ZpP3KoiJ/M7f2wuAxPn3RcNnvuRGNFgqHAEtqwf7+4/2vF6P2XPSQEaiK/\n\t4rYojmDR0giz2AVsxJliu9s1A7W8Fl5e1Ekjc4pLNh8eLUlBZOY7vdK1g/XEckw==","X-Received":["by 2002:a17:906:f5a2:b0:ace:4fde:c28f with SMTP id\n\ta640c23a62f3a-ad219170641mr308256466b.41.1746791050394; \n\tFri, 09 May 2025 04:44:10 -0700 (PDT)","by 2002:a17:906:f5a2:b0:ace:4fde:c28f with SMTP id\n\ta640c23a62f3a-ad219170641mr308252666b.41.1746791049870; \n\tFri, 09 May 2025 04:44:09 -0700 (PDT)"],"X-Google-Smtp-Source":"AGHT+IGu0GHSG0Sh+2I5MPaJ+sJr80XCq8bCjPkEMGXjUR9kZzdJ/94H0SBjbhBA3HrSkPxjOYtckg==","Message-ID":"<f57a842e-a26d-451e-a8d9-a8c31b17ee78@redhat.com>","Date":"Fri, 9 May 2025 13:44:08 +0200","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH resend 5/5] libcamera: swstats_cpu: Add processFrame()\n\tmethod","To":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>,\n\tMilan Zamazal <mzamazal@redhat.com>","Cc":"Kieran Bingham <kieran.bingham@ideasonboard.com>,\n\tlibcamera-devel@lists.libcamera.org","References":"<20241205192519.49104-1-hdegoede@redhat.com>\n\t<20241205192519.49104-6-hdegoede@redhat.com>\n\t<173349087522.3135963.11705101284708571797@ping.linuxembedded.co.uk>\n\t<20241206132803.GJ25902@pendragon.ideasonboard.com>\n\t<8734j0yky7.fsf@redhat.com>\n\t<20241206181852.GE17570@pendragon.ideasonboard.com>","From":"Hans de Goede <hdegoede@redhat.com>","In-Reply-To":"<20241206181852.GE17570@pendragon.ideasonboard.com>","X-Mimecast-Spam-Score":"0","X-Mimecast-MFC-PROC-ID":"R7EQzyB_hG76U1tkJKOfQPy4XGFVeN7TnWdsdoaNeUo_1746791050","X-Mimecast-Originator":"redhat.com","Content-Language":"en-US, nl","Content-Type":"text/plain; charset=UTF-8","Content-Transfer-Encoding":"7bit","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>","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}}]