[{"id":27762,"web_url":"https://patchwork.libcamera.org/comment/27762/","msgid":"<q243mf4ewetm6ph4q6nywvrbp5y47dq2qvxzu7n7zyrg5fhagl@umwelikkm3uo>","date":"2023-09-12T15:06:06","subject":"Re: [libcamera-devel] [PATCH v3 3/5] ipa: rpi: agc: Implementation\n\tof multi-channel AGC","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi David\n\nOn Tue, Sep 12, 2023 at 11:24:40AM +0100, David Plowman via libcamera-devel wrote:\n> The switchMode, prepare and process methods are updated to implement\n> multi-channel AGC correctly:\n>\n> * switchMode now invokes switchMode on all the channels (whether\n>   active or not).\n>\n> * prepare must find what channel the current frame is, and run on\n>   behalf of that channel.\n>\n> * process updates the most recent DeviceStatus and statistics for the\n>   channel of the frame that has just arrived, but generates updated\n>   values working through the active channels in round-robin fashion.\n>\n> One minor detail in process is that we don't want to change the\n> DeviceStatus metadata of the current frame, so we now pass this to the\n> AgcChannel's process method, rather than letting it find the\n> DeviceStatus in the metadata.\n>\n> Signed-off-by: David Plowman <david.plowman@raspberrypi.com>\n> Reviewed-by: Naushir Patuck <naush@raspberrypi.com>\n> ---\n>  src/ipa/rpi/controller/agc_status.h        |   1 +\n>  src/ipa/rpi/controller/rpi/agc.cpp         | 108 +++++++++++++++++++--\n>  src/ipa/rpi/controller/rpi/agc.h           |   3 +\n>  src/ipa/rpi/controller/rpi/agc_channel.cpp |  13 +--\n>  src/ipa/rpi/controller/rpi/agc_channel.h   |   4 +-\n>  5 files changed, 111 insertions(+), 18 deletions(-)\n>\n> diff --git a/src/ipa/rpi/controller/agc_status.h b/src/ipa/rpi/controller/agc_status.h\n> index 597eddd7..e5c4ee22 100644\n> --- a/src/ipa/rpi/controller/agc_status.h\n> +++ b/src/ipa/rpi/controller/agc_status.h\n> @@ -36,6 +36,7 @@ struct AgcStatus {\n>  \tint floatingRegionEnable;\n>  \tlibcamera::utils::Duration fixedShutter;\n>  \tdouble fixedAnalogueGain;\n> +\tunsigned int channel;\n>  };\n>\n>  struct AgcPrepareStatus {\n> diff --git a/src/ipa/rpi/controller/rpi/agc.cpp b/src/ipa/rpi/controller/rpi/agc.cpp\n> index 598fc890..58ba8839 100644\n> --- a/src/ipa/rpi/controller/rpi/agc.cpp\n> +++ b/src/ipa/rpi/controller/rpi/agc.cpp\n> @@ -22,7 +22,8 @@ LOG_DEFINE_CATEGORY(RPiAgc)\n>\n>  Agc::Agc(Controller *controller)\n>  \t: AgcAlgorithm(controller),\n> -\t  activeChannels_({ 0 })\n> +\t  activeChannels_({ 0 }),\n> +\t  index_(0)\n\nBoth initializers fits on a single line\n\n>  {\n>  }\n>\n> @@ -205,20 +206,113 @@ void Agc::setActiveChannels(const std::vector<unsigned int> &activeChannels)\n>  void Agc::switchMode(CameraMode const &cameraMode,\n>  \t\t     Metadata *metadata)\n>  {\n> -\tLOG(RPiAgc, Debug) << \"switchMode for channel 0\";\n> -\tchannelData_[0].channel.switchMode(cameraMode, metadata);\n> +\t/*\n> +\t * We run switchMode on every channel, and then we're going to start over\n> +\t * with the first active channel again which means that this channel's\n> +\t * status needs to be the one we leave in the metadata.\n> +\t */\n> +\tAgcStatus status;\n> +\n> +\tfor (unsigned int channelIndex = 0; channelIndex < channelData_.size(); channelIndex++) {\n> +\t\tLOG(RPiAgc, Debug) << \"switchMode for channel \" << channelIndex;\n> +\t\tchannelData_[channelIndex].channel.switchMode(cameraMode, metadata);\n> +\t\tif (channelIndex == activeChannels_[0])\n> +\t\t\tmetadata->get(\"agc.status\", status);\n> +\t}\n> +\n> +\tstatus.channel = activeChannels_[0];\n> +\tmetadata->set(\"agc.status\", status);\n> +\tindex_ = 0;\n> +}\n> +\n> +static void getChannelIndex(Metadata *metadata, const char *message, unsigned int &channelIndex)\n> +{\n> +\tstd::unique_lock<RPiController::Metadata> lock(*metadata);\n> +\tAgcStatus *status = metadata->getLocked<AgcStatus>(\"agc.delayed_status\");\n> +\tif (status)\n> +\t\tchannelIndex = status->channel;\n> +\telse\n> +\t\t/* This does happen at startup, otherwise it would be a Warning or Error. */\n> +\t\tLOG(RPiAgc, Debug) << message;\n> +}\n> +\n> +static void setChannelIndex(Metadata *metadata, const char *message, unsigned int channelIndex)\n> +{\n> +\tstd::unique_lock<RPiController::Metadata> lock(*metadata);\n> +\tAgcStatus *status = metadata->getLocked<AgcStatus>(\"agc.status\");\n> +\tif (status)\n> +\t\tstatus->channel = channelIndex;\n> +\telse\n> +\t\t/* This does happen at startup, otherwise it would be a Warning or Error. */\n> +\t\tLOG(RPiAgc, Debug) << message;\n>  }\n\nAs a nit, get and set channel index operates on the \"previous\" and\n\"current\" channel respectively. The function name could convey that\n\n>\n>  void Agc::prepare(Metadata *imageMetadata)\n>  {\n> -\tLOG(RPiAgc, Debug) << \"prepare for channel 0\";\n> -\tchannelData_[0].channel.prepare(imageMetadata);\n> +\t/*\n> +\t * The DeviceStatus in the metadata should be correct for the image we\n> +\t * are processing. THe delayed status should tell us what channel this frame\n> +\t * was from, so we will use that channel's prepare method.\n\ns/THe/The/\n\n> +\t *\n> +\t * \\todo To be honest, there's not much that's stateful in the prepare methods\n> +\t * so we should perhaps re-evaluate whether prepare even needs to be done\n> +\t * \"per channel\".\n> +\t */\n> +\tunsigned int channelIndex = activeChannels_[0];\n> +\tgetChannelIndex(imageMetadata, \"prepare: no delayed status\", channelIndex);\n> +\n> +\tLOG(RPiAgc, Debug) << \"prepare for channel \" << channelIndex;\n> +\tchannelData_[channelIndex].channel.prepare(imageMetadata);\n>  }\n>\n>  void Agc::process(StatisticsPtr &stats, Metadata *imageMetadata)\n>  {\n> -\tLOG(RPiAgc, Debug) << \"process for channel 0\";\n> -\tchannelData_[0].channel.process(stats, imageMetadata);\n> +\t/*\n> +\t * We want to generate values for the next channel in round robin fashion\n> +\t * (i.e. the channel at location index_ in the activeChannel list), even though\n> +\t * the statistics we have will be for a different channel (which we find\n> +\t * again from the delayed status).\n> +\t */\n> +\n> +\t/* Generate updated AGC values for this channel: */\n\ns/:/.\n\nYou use \"this channel\" in all comments, but it's not that helpful to\nunderstand which channel you're dealing with\n\n\t/* Generate updated AGC values for the currently active channel. */\n\n> +\tunsigned int channelIndex = activeChannels_[index_];\n> +\tAgcChannelData &channelData = channelData_[channelIndex];\n> +\t/* Stats are from this channel: */\n\n\t/* Stats are from the previously active channel: */\n\n> +\tunsigned int statsIndex = 0;\n> +\tgetChannelIndex(imageMetadata, \"process: no delayed status for stats\", statsIndex);\n> +\tLOG(RPiAgc, Debug) << \"process for channel \" << channelIndex;\n> +\n> +\t/*\n> +\t * We keep a cache of the most recent DeviceStatus and stats for each channel,\n> +\t * so that we can invoke the next channel's process method with the most up to date\n> +\t * values.\n> +\t */\n> +\tLOG(RPiAgc, Debug) << \"Save DeviceStatus and stats for channel \" << statsIndex;\n> +\tDeviceStatus deviceStatus;\n> +\tif (imageMetadata->get<DeviceStatus>(\"device.status\", deviceStatus) == 0)\n> +\t\tchannelData_[statsIndex].deviceStatus = deviceStatus;\n\nWhere is the deviceStatus of the \"previous\" channel read ?\n\nIt seems to me this function only reads the \"current\" one below\n\n\t\tdeviceStatus = *channelData.deviceStatus;\n\n> +\telse\n> +\t\t/* Every frame should have a DeviceStatus. */\n> +\t\tLOG(RPiAgc, Error) << \"process: no device status found\";\n> +\tchannelData_[statsIndex].statistics = stats;\n> +\n> +\t/*\n> +\t * Finally fetch the most recent DeviceStatus and stats for this channel, if both\n\n         * Finally fetch the most recent DeviceStatus and stats for\n         * the current channel channel, if both\n\n> +\t * exist, and call process(). We must make the agc.status metadata record correctly\n> +\t * which channel this is.\n> +\t */\n> +\tif (channelData.statistics && channelData.deviceStatus) {\n> +\t\tdeviceStatus = *channelData.deviceStatus;\n> +\t\tstats = channelData.statistics;\n> +\t} else\n> +\t\t/* Can also happen when new channels start. */\n> +\t\tLOG(RPiAgc, Debug) << \"process: channel \" << channelIndex << \" not seen yet\";\n\ncode style would require {} here, but this is IPA code..\n\n> +\n> +\tchannelData.channel.process(stats, &deviceStatus, imageMetadata);\n\nconst variable passed by pointer\nmodifiable variable passed by reference\n\nIt's really hard by reading the function signature understanding what\nis going to be modified and what won't\n\nUp to you\n\nAll minors/questions\n\nReviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>\n\nThanks\n  j\n\n> +\tsetChannelIndex(imageMetadata, \"process: no AGC status found\", channelIndex);\n> +\n> +\t/* And onto the next channel for the next call. */\n> +\tindex_ = (index_ + 1) % activeChannels_.size();\n>  }\n>\n>  /* Register algorithm with the system. */\n> diff --git a/src/ipa/rpi/controller/rpi/agc.h b/src/ipa/rpi/controller/rpi/agc.h\n> index 24f0a271..ee85c693 100644\n> --- a/src/ipa/rpi/controller/rpi/agc.h\n> +++ b/src/ipa/rpi/controller/rpi/agc.h\n> @@ -18,6 +18,8 @@ namespace RPiController {\n>\n>  struct AgcChannelData {\n>  \tAgcChannel channel;\n> +\tstd::optional<DeviceStatus> deviceStatus;\n> +\tStatisticsPtr statistics;\n>  };\n>\n>  class Agc : public AgcAlgorithm\n> @@ -52,6 +54,7 @@ private:\n>  \tint checkChannel(unsigned int channel) const;\n>  \tstd::vector<AgcChannelData> channelData_;\n>  \tstd::vector<unsigned int> activeChannels_;\n> +\tunsigned int index_; /* index into the activeChannels_ */\n>  };\n>\n>  } /* namespace RPiController */\n> diff --git a/src/ipa/rpi/controller/rpi/agc_channel.cpp b/src/ipa/rpi/controller/rpi/agc_channel.cpp\n> index 7c1aba81..f1f19598 100644\n> --- a/src/ipa/rpi/controller/rpi/agc_channel.cpp\n> +++ b/src/ipa/rpi/controller/rpi/agc_channel.cpp\n> @@ -444,7 +444,7 @@ void AgcChannel::prepare(Metadata *imageMetadata)\n>  \t}\n>  }\n>\n> -void AgcChannel::process(StatisticsPtr &stats, Metadata *imageMetadata)\n> +void AgcChannel::process(StatisticsPtr &stats, const DeviceStatus *deviceStatus, Metadata *imageMetadata)\n>  {\n>  \tframeCount_++;\n>  \t/*\n> @@ -455,7 +455,7 @@ void AgcChannel::process(StatisticsPtr &stats, Metadata *imageMetadata)\n>  \t/* Fetch the AWB status immediately, so that we can assume it's there. */\n>  \tfetchAwbStatus(imageMetadata);\n>  \t/* Get the current exposure values for the frame that's just arrived. */\n> -\tfetchCurrentExposure(imageMetadata);\n> +\tfetchCurrentExposure(deviceStatus);\n>  \t/* Compute the total gain we require relative to the current exposure. */\n>  \tdouble gain, targetY;\n>  \tcomputeGain(stats, imageMetadata, gain, targetY);\n> @@ -567,18 +567,13 @@ void AgcChannel::housekeepConfig()\n>  \t\t\t   << meteringModeName_;\n>  }\n>\n> -void AgcChannel::fetchCurrentExposure(Metadata *imageMetadata)\n> +void AgcChannel::fetchCurrentExposure(const DeviceStatus *deviceStatus)\n>  {\n> -\tstd::unique_lock<Metadata> lock(*imageMetadata);\n> -\tDeviceStatus *deviceStatus =\n> -\t\timageMetadata->getLocked<DeviceStatus>(\"device.status\");\n>  \tif (!deviceStatus)\n>  \t\tLOG(RPiAgc, Fatal) << \"No device metadata\";\n>  \tcurrent_.shutter = deviceStatus->shutterSpeed;\n>  \tcurrent_.analogueGain = deviceStatus->analogueGain;\n> -\tAgcStatus *agcStatus =\n> -\t\timageMetadata->getLocked<AgcStatus>(\"agc.status\");\n> -\tcurrent_.totalExposure = agcStatus ? agcStatus->totalExposureValue : 0s;\n> +\tcurrent_.totalExposure = 0s; /* this value is unused */\n>  \tcurrent_.totalExposureNoDG = current_.shutter * current_.analogueGain;\n>  }\n>\n> diff --git a/src/ipa/rpi/controller/rpi/agc_channel.h b/src/ipa/rpi/controller/rpi/agc_channel.h\n> index d5a5cf3a..24ee3491 100644\n> --- a/src/ipa/rpi/controller/rpi/agc_channel.h\n> +++ b/src/ipa/rpi/controller/rpi/agc_channel.h\n> @@ -85,13 +85,13 @@ public:\n>  \tvoid disableAuto();\n>  \tvoid switchMode(CameraMode const &cameraMode, Metadata *metadata);\n>  \tvoid prepare(Metadata *imageMetadata);\n> -\tvoid process(StatisticsPtr &stats, Metadata *imageMetadata);\n> +\tvoid process(StatisticsPtr &stats, const DeviceStatus *deviceStatus, Metadata *imageMetadata);\n>\n>  private:\n>  \tbool updateLockStatus(DeviceStatus const &deviceStatus);\n>  \tAgcConfig config_;\n>  \tvoid housekeepConfig();\n> -\tvoid fetchCurrentExposure(Metadata *imageMetadata);\n> +\tvoid fetchCurrentExposure(const DeviceStatus *deviceStatus);\n>  \tvoid fetchAwbStatus(Metadata *imageMetadata);\n>  \tvoid computeGain(StatisticsPtr &statistics, Metadata *imageMetadata,\n>  \t\t\t double &gain, double &targetY);\n> --\n> 2.30.2\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 26767BD160\n\tfor <parsemail@patchwork.libcamera.org>;\n\tTue, 12 Sep 2023 15:06:13 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 73085628EC;\n\tTue, 12 Sep 2023 17:06:12 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id DDB0461DEF\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tTue, 12 Sep 2023 17:06:10 +0200 (CEST)","from ideasonboard.com (mob-5-90-67-213.net.vodafone.it\n\t[5.90.67.213])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 46F97512;\n\tTue, 12 Sep 2023 17:04:39 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1694531172;\n\tbh=03bfqNZyIKv/IewYwhN5V/mcz+dzu123paHQABDsy9U=;\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=NJcZh5Q+8KNqByI7aP+3sroKGNeXDFU1/TIF1X9xWCNCoF4X4sXe3v1GjuE1tMRKM\n\t70Gzo3Ka4DTPjB+havcjueJuRjxITxxRvAiQuEWpc9xUwyVjRzBHY44uKthIqok77i\n\tZXibpJ/mGmmCRZS1DLx2H/9FNkaQCDqKhSzk2aacxNBE8PB41g9qdp4RC5Lh6IeENU\n\tAnpQCP+0sWupa98d2PSKEzssSmyZ6JI8SnGTTT3YAu5e5ft6foYVkJ3DHJNWoEyRGY\n\tE3FBh2zS7waAzz64yxIy+Vt8LLStc+Cwr7Bk/RFAFJWsrZFvvOb0BmbfdpSSH9OXGV\n\tyGy/Ql29roqjg==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1694531079;\n\tbh=03bfqNZyIKv/IewYwhN5V/mcz+dzu123paHQABDsy9U=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=qbvjWl40RMOCYaAp6WsbbRMFc2XdBwxlbG0ZetB5aYRv/mDhYVi4JGWLnM7A4Jte4\n\tbK4wsA1LWyUsEN+NUkMqpdFKgdx8UYH98XLIvjCPWCFuw9XX55Z014marDsSzxw3fq\n\tWm4CWn37/2DNiBzhrYiNFFwUJQFCCqS/vskL42as="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"qbvjWl40\"; dkim-atps=neutral","Date":"Tue, 12 Sep 2023 17:06:06 +0200","To":"David Plowman <david.plowman@raspberrypi.com>","Message-ID":"<q243mf4ewetm6ph4q6nywvrbp5y47dq2qvxzu7n7zyrg5fhagl@umwelikkm3uo>","References":"<20230912102442.169001-1-david.plowman@raspberrypi.com>\n\t<20230912102442.169001-4-david.plowman@raspberrypi.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20230912102442.169001-4-david.plowman@raspberrypi.com>","Subject":"Re: [libcamera-devel] [PATCH v3 3/5] ipa: rpi: agc: Implementation\n\tof multi-channel AGC","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.mondi@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":27785,"web_url":"https://patchwork.libcamera.org/comment/27785/","msgid":"<CAHW6GYJ_wu91=bCeYHrDkxVnEbRPwsa_RToUPYeahE3CkZ0MmA@mail.gmail.com>","date":"2023-09-15T09:00:02","subject":"Re: [libcamera-devel] [PATCH v3 3/5] ipa: rpi: agc: Implementation\n\tof multi-channel AGC","submitter":{"id":42,"url":"https://patchwork.libcamera.org/api/people/42/","name":"David Plowman","email":"david.plowman@raspberrypi.com"},"content":"Hi Jacopo\n\nThanks for the comments.\n\nOn Tue, 12 Sept 2023 at 16:06, Jacopo Mondi\n<jacopo.mondi@ideasonboard.com> wrote:\n>\n> Hi David\n>\n> On Tue, Sep 12, 2023 at 11:24:40AM +0100, David Plowman via libcamera-devel wrote:\n> > The switchMode, prepare and process methods are updated to implement\n> > multi-channel AGC correctly:\n> >\n> > * switchMode now invokes switchMode on all the channels (whether\n> >   active or not).\n> >\n> > * prepare must find what channel the current frame is, and run on\n> >   behalf of that channel.\n> >\n> > * process updates the most recent DeviceStatus and statistics for the\n> >   channel of the frame that has just arrived, but generates updated\n> >   values working through the active channels in round-robin fashion.\n> >\n> > One minor detail in process is that we don't want to change the\n> > DeviceStatus metadata of the current frame, so we now pass this to the\n> > AgcChannel's process method, rather than letting it find the\n> > DeviceStatus in the metadata.\n> >\n> > Signed-off-by: David Plowman <david.plowman@raspberrypi.com>\n> > Reviewed-by: Naushir Patuck <naush@raspberrypi.com>\n> > ---\n> >  src/ipa/rpi/controller/agc_status.h        |   1 +\n> >  src/ipa/rpi/controller/rpi/agc.cpp         | 108 +++++++++++++++++++--\n> >  src/ipa/rpi/controller/rpi/agc.h           |   3 +\n> >  src/ipa/rpi/controller/rpi/agc_channel.cpp |  13 +--\n> >  src/ipa/rpi/controller/rpi/agc_channel.h   |   4 +-\n> >  5 files changed, 111 insertions(+), 18 deletions(-)\n> >\n> > diff --git a/src/ipa/rpi/controller/agc_status.h b/src/ipa/rpi/controller/agc_status.h\n> > index 597eddd7..e5c4ee22 100644\n> > --- a/src/ipa/rpi/controller/agc_status.h\n> > +++ b/src/ipa/rpi/controller/agc_status.h\n> > @@ -36,6 +36,7 @@ struct AgcStatus {\n> >       int floatingRegionEnable;\n> >       libcamera::utils::Duration fixedShutter;\n> >       double fixedAnalogueGain;\n> > +     unsigned int channel;\n> >  };\n> >\n> >  struct AgcPrepareStatus {\n> > diff --git a/src/ipa/rpi/controller/rpi/agc.cpp b/src/ipa/rpi/controller/rpi/agc.cpp\n> > index 598fc890..58ba8839 100644\n> > --- a/src/ipa/rpi/controller/rpi/agc.cpp\n> > +++ b/src/ipa/rpi/controller/rpi/agc.cpp\n> > @@ -22,7 +22,8 @@ LOG_DEFINE_CATEGORY(RPiAgc)\n> >\n> >  Agc::Agc(Controller *controller)\n> >       : AgcAlgorithm(controller),\n> > -       activeChannels_({ 0 })\n> > +       activeChannels_({ 0 }),\n> > +       index_(0)\n>\n> Both initializers fits on a single line\n\nYes, I can do that.\n\n>\n> >  {\n> >  }\n> >\n> > @@ -205,20 +206,113 @@ void Agc::setActiveChannels(const std::vector<unsigned int> &activeChannels)\n> >  void Agc::switchMode(CameraMode const &cameraMode,\n> >                    Metadata *metadata)\n> >  {\n> > -     LOG(RPiAgc, Debug) << \"switchMode for channel 0\";\n> > -     channelData_[0].channel.switchMode(cameraMode, metadata);\n> > +     /*\n> > +      * We run switchMode on every channel, and then we're going to start over\n> > +      * with the first active channel again which means that this channel's\n> > +      * status needs to be the one we leave in the metadata.\n> > +      */\n> > +     AgcStatus status;\n> > +\n> > +     for (unsigned int channelIndex = 0; channelIndex < channelData_.size(); channelIndex++) {\n> > +             LOG(RPiAgc, Debug) << \"switchMode for channel \" << channelIndex;\n> > +             channelData_[channelIndex].channel.switchMode(cameraMode, metadata);\n> > +             if (channelIndex == activeChannels_[0])\n> > +                     metadata->get(\"agc.status\", status);\n> > +     }\n> > +\n> > +     status.channel = activeChannels_[0];\n> > +     metadata->set(\"agc.status\", status);\n> > +     index_ = 0;\n> > +}\n> > +\n> > +static void getChannelIndex(Metadata *metadata, const char *message, unsigned int &channelIndex)\n> > +{\n> > +     std::unique_lock<RPiController::Metadata> lock(*metadata);\n> > +     AgcStatus *status = metadata->getLocked<AgcStatus>(\"agc.delayed_status\");\n> > +     if (status)\n> > +             channelIndex = status->channel;\n> > +     else\n> > +             /* This does happen at startup, otherwise it would be a Warning or Error. */\n> > +             LOG(RPiAgc, Debug) << message;\n> > +}\n> > +\n> > +static void setChannelIndex(Metadata *metadata, const char *message, unsigned int channelIndex)\n> > +{\n> > +     std::unique_lock<RPiController::Metadata> lock(*metadata);\n> > +     AgcStatus *status = metadata->getLocked<AgcStatus>(\"agc.status\");\n> > +     if (status)\n> > +             status->channel = channelIndex;\n> > +     else\n> > +             /* This does happen at startup, otherwise it would be a Warning or Error. */\n> > +             LOG(RPiAgc, Debug) << message;\n> >  }\n>\n> As a nit, get and set channel index operates on the \"previous\" and\n> \"current\" channel respectively. The function name could convey that\n\nYes, I'll improve the names.\n\n>\n> >\n> >  void Agc::prepare(Metadata *imageMetadata)\n> >  {\n> > -     LOG(RPiAgc, Debug) << \"prepare for channel 0\";\n> > -     channelData_[0].channel.prepare(imageMetadata);\n> > +     /*\n> > +      * The DeviceStatus in the metadata should be correct for the image we\n> > +      * are processing. THe delayed status should tell us what channel this frame\n> > +      * was from, so we will use that channel's prepare method.\n>\n> s/THe/The/\n\nYep!\n\n>\n> > +      *\n> > +      * \\todo To be honest, there's not much that's stateful in the prepare methods\n> > +      * so we should perhaps re-evaluate whether prepare even needs to be done\n> > +      * \"per channel\".\n> > +      */\n> > +     unsigned int channelIndex = activeChannels_[0];\n> > +     getChannelIndex(imageMetadata, \"prepare: no delayed status\", channelIndex);\n> > +\n> > +     LOG(RPiAgc, Debug) << \"prepare for channel \" << channelIndex;\n> > +     channelData_[channelIndex].channel.prepare(imageMetadata);\n> >  }\n> >\n> >  void Agc::process(StatisticsPtr &stats, Metadata *imageMetadata)\n> >  {\n> > -     LOG(RPiAgc, Debug) << \"process for channel 0\";\n> > -     channelData_[0].channel.process(stats, imageMetadata);\n> > +     /*\n> > +      * We want to generate values for the next channel in round robin fashion\n> > +      * (i.e. the channel at location index_ in the activeChannel list), even though\n> > +      * the statistics we have will be for a different channel (which we find\n> > +      * again from the delayed status).\n> > +      */\n> > +\n> > +     /* Generate updated AGC values for this channel: */\n>\n> s/:/.\n\nWill do.\n\n>\n> You use \"this channel\" in all comments, but it's not that helpful to\n> understand which channel you're dealing with\n>\n>         /* Generate updated AGC values for the currently active channel. */\n\nI'll try to improve this, thanks.\n\n>\n> > +     unsigned int channelIndex = activeChannels_[index_];\n> > +     AgcChannelData &channelData = channelData_[channelIndex];\n> > +     /* Stats are from this channel: */\n>\n>         /* Stats are from the previously active channel: */\n\nHere too.\n\n>\n> > +     unsigned int statsIndex = 0;\n> > +     getChannelIndex(imageMetadata, \"process: no delayed status for stats\", statsIndex);\n> > +     LOG(RPiAgc, Debug) << \"process for channel \" << channelIndex;\n> > +\n> > +     /*\n> > +      * We keep a cache of the most recent DeviceStatus and stats for each channel,\n> > +      * so that we can invoke the next channel's process method with the most up to date\n> > +      * values.\n> > +      */\n> > +     LOG(RPiAgc, Debug) << \"Save DeviceStatus and stats for channel \" << statsIndex;\n> > +     DeviceStatus deviceStatus;\n> > +     if (imageMetadata->get<DeviceStatus>(\"device.status\", deviceStatus) == 0)\n> > +             channelData_[statsIndex].deviceStatus = deviceStatus;\n>\n> Where is the deviceStatus of the \"previous\" channel read ?\n>\n> It seems to me this function only reads the \"current\" one below\n>\n>                 deviceStatus = *channelData.deviceStatus;\n\nSo that happens just below where it says \"deviceStatus =\n*channelData.deviceStatus;\"\n\nWe have to save the statistics that have just arrived into the\n\"channel data\" that was used to produce that corresponding frame\n(which has also just arrived). But then we must use the most recent\nstatistics and device status from the last time we saw a frame from\nthe channel for which we're now calculating values.\n\nIf that makes sense. Yes, my head hurts a bit!\n\n>\n> > +     else\n> > +             /* Every frame should have a DeviceStatus. */\n> > +             LOG(RPiAgc, Error) << \"process: no device status found\";\n> > +     channelData_[statsIndex].statistics = stats;\n> > +\n> > +     /*\n> > +      * Finally fetch the most recent DeviceStatus and stats for this channel, if both\n>\n>          * Finally fetch the most recent DeviceStatus and stats for\n>          * the current channel channel, if both\n>\n> > +      * exist, and call process(). We must make the agc.status metadata record correctly\n> > +      * which channel this is.\n> > +      */\n> > +     if (channelData.statistics && channelData.deviceStatus) {\n> > +             deviceStatus = *channelData.deviceStatus;\n> > +             stats = channelData.statistics;\n> > +     } else\n> > +             /* Can also happen when new channels start. */\n> > +             LOG(RPiAgc, Debug) << \"process: channel \" << channelIndex << \" not seen yet\";\n>\n> code style would require {} here, but this is IPA code..\n\nIndeed. In fact I noticed a couple of other places, so I'll fix those too.\n\n>\n> > +\n> > +     channelData.channel.process(stats, &deviceStatus, imageMetadata);\n>\n> const variable passed by pointer\n> modifiable variable passed by reference\n>\n> It's really hard by reading the function signature understanding what\n> is going to be modified and what won't\n>\n> Up to you\n\nYes, true. Both deviceStatus and imageMetadata are pointers, but maybe\nthe deviceStatus could be a reference as I think folks prefer const\nreferences and pointers to mutable things?? agc_channel.cpp does\nactually check for a non-NULL deviceStatus pointer, but seeing as\nagc.cpp can never pass a NULL pointer it seems safe to remove that. So\nI'll change it.\n\n>\n> All minors/questions\n>\n> Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>\n\nThanks again for all the reviewing!\n\nDavid\n\n>\n> Thanks\n>   j\n>\n> > +     setChannelIndex(imageMetadata, \"process: no AGC status found\", channelIndex);\n> > +\n> > +     /* And onto the next channel for the next call. */\n> > +     index_ = (index_ + 1) % activeChannels_.size();\n> >  }\n> >\n> >  /* Register algorithm with the system. */\n> > diff --git a/src/ipa/rpi/controller/rpi/agc.h b/src/ipa/rpi/controller/rpi/agc.h\n> > index 24f0a271..ee85c693 100644\n> > --- a/src/ipa/rpi/controller/rpi/agc.h\n> > +++ b/src/ipa/rpi/controller/rpi/agc.h\n> > @@ -18,6 +18,8 @@ namespace RPiController {\n> >\n> >  struct AgcChannelData {\n> >       AgcChannel channel;\n> > +     std::optional<DeviceStatus> deviceStatus;\n> > +     StatisticsPtr statistics;\n> >  };\n> >\n> >  class Agc : public AgcAlgorithm\n> > @@ -52,6 +54,7 @@ private:\n> >       int checkChannel(unsigned int channel) const;\n> >       std::vector<AgcChannelData> channelData_;\n> >       std::vector<unsigned int> activeChannels_;\n> > +     unsigned int index_; /* index into the activeChannels_ */\n> >  };\n> >\n> >  } /* namespace RPiController */\n> > diff --git a/src/ipa/rpi/controller/rpi/agc_channel.cpp b/src/ipa/rpi/controller/rpi/agc_channel.cpp\n> > index 7c1aba81..f1f19598 100644\n> > --- a/src/ipa/rpi/controller/rpi/agc_channel.cpp\n> > +++ b/src/ipa/rpi/controller/rpi/agc_channel.cpp\n> > @@ -444,7 +444,7 @@ void AgcChannel::prepare(Metadata *imageMetadata)\n> >       }\n> >  }\n> >\n> > -void AgcChannel::process(StatisticsPtr &stats, Metadata *imageMetadata)\n> > +void AgcChannel::process(StatisticsPtr &stats, const DeviceStatus *deviceStatus, Metadata *imageMetadata)\n> >  {\n> >       frameCount_++;\n> >       /*\n> > @@ -455,7 +455,7 @@ void AgcChannel::process(StatisticsPtr &stats, Metadata *imageMetadata)\n> >       /* Fetch the AWB status immediately, so that we can assume it's there. */\n> >       fetchAwbStatus(imageMetadata);\n> >       /* Get the current exposure values for the frame that's just arrived. */\n> > -     fetchCurrentExposure(imageMetadata);\n> > +     fetchCurrentExposure(deviceStatus);\n> >       /* Compute the total gain we require relative to the current exposure. */\n> >       double gain, targetY;\n> >       computeGain(stats, imageMetadata, gain, targetY);\n> > @@ -567,18 +567,13 @@ void AgcChannel::housekeepConfig()\n> >                          << meteringModeName_;\n> >  }\n> >\n> > -void AgcChannel::fetchCurrentExposure(Metadata *imageMetadata)\n> > +void AgcChannel::fetchCurrentExposure(const DeviceStatus *deviceStatus)\n> >  {\n> > -     std::unique_lock<Metadata> lock(*imageMetadata);\n> > -     DeviceStatus *deviceStatus =\n> > -             imageMetadata->getLocked<DeviceStatus>(\"device.status\");\n> >       if (!deviceStatus)\n> >               LOG(RPiAgc, Fatal) << \"No device metadata\";\n> >       current_.shutter = deviceStatus->shutterSpeed;\n> >       current_.analogueGain = deviceStatus->analogueGain;\n> > -     AgcStatus *agcStatus =\n> > -             imageMetadata->getLocked<AgcStatus>(\"agc.status\");\n> > -     current_.totalExposure = agcStatus ? agcStatus->totalExposureValue : 0s;\n> > +     current_.totalExposure = 0s; /* this value is unused */\n> >       current_.totalExposureNoDG = current_.shutter * current_.analogueGain;\n> >  }\n> >\n> > diff --git a/src/ipa/rpi/controller/rpi/agc_channel.h b/src/ipa/rpi/controller/rpi/agc_channel.h\n> > index d5a5cf3a..24ee3491 100644\n> > --- a/src/ipa/rpi/controller/rpi/agc_channel.h\n> > +++ b/src/ipa/rpi/controller/rpi/agc_channel.h\n> > @@ -85,13 +85,13 @@ public:\n> >       void disableAuto();\n> >       void switchMode(CameraMode const &cameraMode, Metadata *metadata);\n> >       void prepare(Metadata *imageMetadata);\n> > -     void process(StatisticsPtr &stats, Metadata *imageMetadata);\n> > +     void process(StatisticsPtr &stats, const DeviceStatus *deviceStatus, Metadata *imageMetadata);\n> >\n> >  private:\n> >       bool updateLockStatus(DeviceStatus const &deviceStatus);\n> >       AgcConfig config_;\n> >       void housekeepConfig();\n> > -     void fetchCurrentExposure(Metadata *imageMetadata);\n> > +     void fetchCurrentExposure(const DeviceStatus *deviceStatus);\n> >       void fetchAwbStatus(Metadata *imageMetadata);\n> >       void computeGain(StatisticsPtr &statistics, Metadata *imageMetadata,\n> >                        double &gain, double &targetY);\n> > --\n> > 2.30.2\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 8F8FCBD160\n\tfor <parsemail@patchwork.libcamera.org>;\n\tFri, 15 Sep 2023 09:00:17 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B141D62917;\n\tFri, 15 Sep 2023 11:00:16 +0200 (CEST)","from mail-qv1-xf29.google.com (mail-qv1-xf29.google.com\n\t[IPv6:2607:f8b0:4864:20::f29])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 65A0E628D8\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tFri, 15 Sep 2023 11:00:14 +0200 (CEST)","by mail-qv1-xf29.google.com with SMTP id\n\t6a1803df08f44-655de2a5121so10973616d6.1\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tFri, 15 Sep 2023 02:00:14 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1694768416;\n\tbh=7YZsB1p6oBOPJdjGhvp5EDOJt0RTumXUZi3+05bsjUc=;\n\th=References:In-Reply-To:Date:To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=EEoYiZEoAKociTy2FSp9Di0Wvw+noMYsNWHZB0E+rw2tfkew2Kng6N4x9fiU93cNq\n\tTmu/F0v7mNNlp++i5yYPF3rzvU7glY3epHsPIk2P62pkLxogxH6MZVSNtX6nZJLYny\n\tIWXxLSF8spIT48Nh6FkGatFZnQZ5E9wGg3dwlgc0cIAJQOQEC1k2COlqgIPlnYNxr0\n\tRvb3g1raljoT3Uj+qbvgmYWrqkIMfnOQv58uQKib3dIFedK3ro3dxRYUPg4Om0yvrk\n\tk5VMlgT18wtL9pXTx0QGiK3kPHfX1dsunrK4qmS8RvEAMmSJgOo0vfhWyPRLPAPkIW\n\t2XPU3rzpTr/cQ==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google; t=1694768413; x=1695373213;\n\tdarn=lists.libcamera.org; \n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:from:to:cc:subject:date:message-id:reply-to;\n\tbh=9BeYKtui92vwBCSQBRybC2Qhf8wrUPcFWBW1hmn7tUs=;\n\tb=n541t3DFcFoGrbfgfqT4gBHZFdZ/V7FjpemBxqeBOhZaMmHwOS0uHu0n1SLLgzrDcB\n\tmNakD+QLw0JohaRVVyt+c3YweDvcMAvOoKh4DvqTFe+hlytLNyq4zyQ8YxhDrRIFTFct\n\t6nxltXNYLpuMoWE8B4Jt38fZLug6S+wYrwSAcC8PKJzOQVuZznRSnXdUlM18pow1zqAw\n\tEUdwfGfub5mUMZhAX7msxd4WKrlmr2IE6cTn5IZpwDnzK5ER7+08XX1IocmSTR52dqtG\n\tE5UolefTGEOwHoAH6B8Mk2moRthrhP9Y4OI4Q3JhgF8OciHPg0hboFMGhBOVjBIPzJvD\n\t5IdA=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=raspberrypi.com\n\theader.i=@raspberrypi.com\n\theader.b=\"n541t3DF\"; dkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1694768413; x=1695373213;\n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:x-gm-message-state:from:to:cc:subject:date:message-id\n\t:reply-to;\n\tbh=9BeYKtui92vwBCSQBRybC2Qhf8wrUPcFWBW1hmn7tUs=;\n\tb=sTIxcm3vmx6/PpfawBuxBEtVcrQgFY1DLxFND5JHbhh69XxCof8nBa/fHHTI9Huw6c\n\tw7ZMJk9914OE4M3TPnuq6Oa3O1NdPgbpP+0/IVRoLx7SRntL+vDTYW+b99GUVMGMylTt\n\t+Yns46tpMebYZDfmRKYIufgzmtq8p/qFdjI+d4fMqNKRvEMIWDFrU5yMvlFsqPk3Ray0\n\t3C9po2pxJHm9a3tmEMiJI/ybO0FQduhhiKvrag6wRdzkFHjUZVkZ9Adw481ZyjDMtDp4\n\t23c58uoMeKT/gTxvTJZ8Ug5W0WY9vgcAYv1nD33uQns86+93m+2nHIaTR05LB6NGrYBI\n\tbFsw==","X-Gm-Message-State":"AOJu0YxUskkBB3Mn1KyKyxZxFj0Dap7E4qsp08SKr5/CET/zo9N6DcoX\n\tW6DQk3jEcVci6zqT9fL0DAPnj29ULoEZEl2lvpkbuAoK/cBz14aUMfA=","X-Google-Smtp-Source":"AGHT+IGqGuH0duOfkd+zztYrL8mM90TfRs62gy3kgNskhzHESgVYp5EevjBKXdog+KD2U6TS+jDGqMKuv4PjbHDH+1w=","X-Received":"by 2002:a0c:e252:0:b0:653:583d:d400 with SMTP id\n\tx18-20020a0ce252000000b00653583dd400mr1047542qvl.52.1694768413036;\n\tFri, 15 Sep 2023 02:00:13 -0700 (PDT)","MIME-Version":"1.0","References":"<20230912102442.169001-1-david.plowman@raspberrypi.com>\n\t<20230912102442.169001-4-david.plowman@raspberrypi.com>\n\t<q243mf4ewetm6ph4q6nywvrbp5y47dq2qvxzu7n7zyrg5fhagl@umwelikkm3uo>","In-Reply-To":"<q243mf4ewetm6ph4q6nywvrbp5y47dq2qvxzu7n7zyrg5fhagl@umwelikkm3uo>","Date":"Fri, 15 Sep 2023 10:00:02 +0100","Message-ID":"<CAHW6GYJ_wu91=bCeYHrDkxVnEbRPwsa_RToUPYeahE3CkZ0MmA@mail.gmail.com>","To":"Jacopo Mondi <jacopo.mondi@ideasonboard.com>","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH v3 3/5] ipa: rpi: agc: Implementation\n\tof multi-channel AGC","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":"David Plowman via libcamera-devel <libcamera-devel@lists.libcamera.org>","Reply-To":"David Plowman <david.plowman@raspberrypi.com>","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}}]