[{"id":25610,"web_url":"https://patchwork.libcamera.org/comment/25610/","msgid":"<166680229104.2677993.15469998037998422292@Monstersaurus>","date":"2022-10-26T16:38:11","subject":"Re: [libcamera-devel] [PATCH v4 2/7] delayed_controls: Add user\n\tcookie to DelayedControls","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Naushir Patuck via libcamera-devel (2022-10-19 10:01:02)\n> Allow the caller to provide a cookie value to DelayedControls::reset and\n> DelayedControls::push. This cookie value is returned from DelayedControls::get\n> for the frame that has the control values applied to it.\n> \n> The cookie value is useful in tracking when a set of controls has been applied\n> to a frame. In a subsequent commit, it will be used by the Raspberry Pi IPA to\n> track the IPA context used when setting the control values.\n> \n> Signed-off-by: Naushir Patuck <naush@raspberrypi.com>\n> Reviewed-by: David Plowman <david.plowman@raspberrypi.com>\n> Tested-by: David Plowman <david.plowman@raspberrypi.com>\n> ---\n>  include/libcamera/internal/delayed_controls.h |  8 +++++---\n>  src/libcamera/delayed_controls.cpp            | 20 ++++++++++++-------\n>  src/libcamera/pipeline/ipu3/ipu3.cpp          |  3 ++-\n>  .../pipeline/raspberrypi/raspberrypi.cpp      |  2 +-\n>  src/libcamera/pipeline/rkisp1/rkisp1.cpp      |  3 ++-\n>  test/delayed_controls.cpp                     |  8 ++++----\n>  6 files changed, 27 insertions(+), 17 deletions(-)\n> \n> diff --git a/include/libcamera/internal/delayed_controls.h b/include/libcamera/internal/delayed_controls.h\n> index de8026e3e4f0..c7e79150afb8 100644\n> --- a/include/libcamera/internal/delayed_controls.h\n> +++ b/include/libcamera/internal/delayed_controls.h\n> @@ -9,6 +9,7 @@\n>  \n>  #include <stdint.h>\n>  #include <unordered_map>\n> +#include <utility>\n>  \n>  #include <libcamera/controls.h>\n>  \n> @@ -27,10 +28,10 @@ public:\n>         DelayedControls(V4L2Device *device,\n>                         const std::unordered_map<uint32_t, ControlParams> &controlParams);\n>  \n> -       void reset();\n> +       void reset(unsigned int cookie = 0);\n>  \n> -       bool push(const ControlList &controls);\n> -       ControlList get(uint32_t sequence);\n> +       bool push(const ControlList &controls, unsigned int cookie = 0);\n\nThe part that worries me here is that we /require/ always getting the\ncookie, but not setting it...\n\nI would feel safer if this wasn't defaulted to 0...\n\n> +       std::pair<ControlList, unsigned int> get(uint32_t sequence);\n>  \n>         void applyControls(uint32_t sequence);\n>  \n> @@ -77,6 +78,7 @@ private:\n>         uint32_t writeCount_;\n>         /* \\todo Evaluate if we should index on ControlId * or unsigned int */\n>         std::unordered_map<const ControlId *, RingBuffer<Info>> values_;\n> +       RingBuffer<unsigned int> cookies_;\n>  };\n>  \n>  } /* namespace libcamera */\n> diff --git a/src/libcamera/delayed_controls.cpp b/src/libcamera/delayed_controls.cpp\n> index 777441e8222a..3e7f60f9c421 100644\n> --- a/src/libcamera/delayed_controls.cpp\n> +++ b/src/libcamera/delayed_controls.cpp\n> @@ -109,14 +109,16 @@ DelayedControls::DelayedControls(V4L2Device *device,\n>  \n>  /**\n>   * \\brief Reset state machine\n> + * \\param[in] cookie User supplied reset cookie value\n>   *\n>   * Resets the state machine to a starting position based on control values\n>   * retrieved from the device.\n>   */\n> -void DelayedControls::reset()\n> +void DelayedControls::reset(unsigned int cookie)\n>  {\n>         queueCount_ = 1;\n>         writeCount_ = 0;\n> +       cookies_[0] = cookie;\n>  \n>         /* Retrieve control as reported by the device. */\n>         std::vector<uint32_t> ids;\n> @@ -140,13 +142,15 @@ void DelayedControls::reset()\n>  /**\n>   * \\brief Push a set of controls on the queue\n>   * \\param[in] controls List of controls to add to the device queue\n> + * \\param[in] cookie User supplied cookie value for \\a controls\n>   *\n>   * Push a set of controls to the control queue. This increases the control queue\n> - * depth by one.\n> + * depth by one. The \\a cookie value will be subsequently returned from\n> + * \\a get() for the frame with all controls applied.\n>   *\n>   * \\returns true if \\a controls are accepted, or false otherwise\n>   */\n> -bool DelayedControls::push(const ControlList &controls)\n> +bool DelayedControls::push(const ControlList &controls, const unsigned int cookie)\n>  {\n>         /* Copy state from previous frame. */\n>         for (auto &ctrl : values_) {\n> @@ -180,6 +184,7 @@ bool DelayedControls::push(const ControlList &controls)\n>                         << \" at index \" << queueCount_;\n>         }\n>  \n> +       cookies_[queueCount_] = cookie;\n>         queueCount_++;\n>  \n>         return true;\n> @@ -198,9 +203,10 @@ bool DelayedControls::push(const ControlList &controls)\n>   * push(). The max history from the current sequence number that yields valid\n>   * values are thus 16 minus number of controls pushed.\n>   *\n> - * \\return The controls at \\a sequence number\n> + * \\return The controls at \\a sequence number and associated user supplied\n> + * cookie value.\n>   */\n> -ControlList DelayedControls::get(uint32_t sequence)\n> +std::pair<ControlList, unsigned int> DelayedControls::get(uint32_t sequence)\n>  {\n>         unsigned int index = std::max<int>(0, sequence - maxDelay_);\n>  \n> @@ -217,7 +223,7 @@ ControlList DelayedControls::get(uint32_t sequence)\n>                         << \" at index \" << index;\n>         }\n>  \n> -       return out;\n> +       return { out, cookies_[index] };\n>  }\n>  \n>  /**\n> @@ -276,7 +282,7 @@ void DelayedControls::applyControls(uint32_t sequence)\n>         while (writeCount_ > queueCount_) {\n>                 LOG(DelayedControls, Debug)\n>                         << \"Queue is empty, auto queue no-op.\";\n> -               push({});\n> +               push({}, cookies_[queueCount_ - 1]);\n>         }\n>  \n>         device_->setControls(&out);\n> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> index 3b892d9671c5..bf612089fdcb 100644\n> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> @@ -1389,7 +1389,8 @@ void IPU3CameraData::cio2BufferReady(FrameBuffer *buffer)\n>         request->metadata().set(controls::SensorTimestamp,\n>                                 buffer->metadata().timestamp);\n>  \n> -       info->effectiveSensorControls = delayedCtrls_->get(buffer->metadata().sequence);\n> +       auto [controls, cookie] = delayedCtrls_->get(buffer->metadata().sequence);\n> +       info->effectiveSensorControls = std::move(controls);\n\nWhich means this cookie is always zero.\n\nCould you also update the push call to set the cookie?\n\nThat looks like it happens at\n\nvoid IPU3CameraData::setSensorControls([[maybe_unused]] unsigned int id,\n                                       const ControlList &sensorControls,\n                                       const ControlList &lensControls)\n{\n        delayedCtrls_->push(sensorControls);\n...\n\nWhere the 'id' is actually the request sequence number in the current\nimplementation.\n\nSo we can remove [[maybe_unused]] from unsigned int id, and pass it to push().\n\nUltimately, this then means that the cookie is the request sequence\nnumber of the request which is supposed to reflect these controls. Which\nis a nice way of tieing these together.\n\n\n>         if (request->findBuffer(&rawStream_))\n>                 pipe()->completeBuffer(request, buffer);\n> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> index 343f8cb2c7ed..23f2460190f4 100644\n> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> @@ -1867,7 +1867,7 @@ void RPiCameraData::unicamBufferDequeue(FrameBuffer *buffer)\n>                  * Lookup the sensor controls used for this frame sequence from\n>                  * DelayedControl and queue them along with the frame buffer.\n>                  */\n> -               ControlList ctrl = delayedCtrls_->get(buffer->metadata().sequence);\n> +               auto [ctrl, cookie] = delayedCtrls_->get(buffer->metadata().sequence);\n>                 /*\n>                  * Add the frame timestamp to the ControlList for the IPA to use\n>                  * as it does not receive the FrameBuffer object.\n> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> index 455ee2a0a711..20049d089472 100644\n> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> @@ -1191,8 +1191,9 @@ void PipelineHandlerRkISP1::statReady(FrameBuffer *buffer)\n>         if (data->frame_ <= buffer->metadata().sequence)\n>                 data->frame_ = buffer->metadata().sequence + 1;\n>  \n> +       auto [controls, cookie] = data->delayedCtrls_->get(buffer->metadata().sequence);\n\nSame for rkisp1:\n\nvoid RkISP1CameraData::setSensorControls([[maybe_unused]] unsigned int frame,\n                                         const ControlList &sensorControls)\n{\n        delayedCtrls_->push(sensorControls);\n}\n\n... 'frame' should be a suitable/expected cookie here.\n\n\n>         data->ipa_->processStatsBuffer(info->frame, info->statBuffer->cookie(),\n> -                                      data->delayedCtrls_->get(buffer->metadata().sequence));\n> +                                      controls);\n>  }\n>  \n>  REGISTER_PIPELINE_HANDLER(PipelineHandlerRkISP1)\n> diff --git a/test/delayed_controls.cpp b/test/delayed_controls.cpp\n> index a8ce9828d73d..322c545998b2 100644\n> --- a/test/delayed_controls.cpp\n> +++ b/test/delayed_controls.cpp\n\nAnd in here, testing locally - then the pushes here need a cookie - and\n'i' seems suitable.\n\nWith that,\n\n\nReviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>\n\n> @@ -96,7 +96,7 @@ protected:\n>  \n>                         delayed->applyControls(i);\n>  \n> -                       ControlList result = delayed->get(i);\n> +                       auto [result, cookie] = delayed->get(i);\n>                         int32_t brightness = result.get(V4L2_CID_BRIGHTNESS).get<int32_t>();\n>                         if (brightness != value) {\n>                                 cerr << \"Failed single control without delay\"\n> @@ -138,7 +138,7 @@ protected:\n>  \n>                         delayed->applyControls(i);\n>  \n> -                       ControlList result = delayed->get(i);\n> +                       auto [result, cookie] = delayed->get(i);\n>                         int32_t brightness = result.get(V4L2_CID_BRIGHTNESS).get<int32_t>();\n>                         if (brightness != expected) {\n>                                 cerr << \"Failed single control with delay\"\n> @@ -187,7 +187,7 @@ protected:\n>  \n>                         delayed->applyControls(i);\n>  \n> -                       ControlList result = delayed->get(i);\n> +                       auto [result, cookie] = delayed->get(i);\n>                         int32_t brightness = result.get(V4L2_CID_BRIGHTNESS).get<int32_t>();\n>                         int32_t contrast = result.get(V4L2_CID_CONTRAST).get<int32_t>();\n>                         if (brightness != expected || contrast != expected + 1) {\n> @@ -247,7 +247,7 @@ protected:\n>  \n>                         delayed->applyControls(i);\n>  \n> -                       ControlList result = delayed->get(i);\n> +                       auto [result, cookie] = delayed->get(i);\n>  \n>                         int32_t brightness = result.get(V4L2_CID_BRIGHTNESS).get<int32_t>();\n>                         int32_t contrast = result.get(V4L2_CID_CONTRAST).get<int32_t>();\n> -- \n> 2.25.1\n>","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id BF1A1BDB16\n\tfor <parsemail@patchwork.libcamera.org>;\n\tWed, 26 Oct 2022 16:38:16 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 06AB162F62;\n\tWed, 26 Oct 2022 18:38:16 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D5CE961F4B\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tWed, 26 Oct 2022 18:38:13 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 5A47C4F8;\n\tWed, 26 Oct 2022 18:38:13 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1666802296;\n\tbh=hHfXgHME2FbFAMUbgxMtSrmYcZb32c3fhbjGH65s4DI=;\n\th=In-Reply-To:References:To:Date:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:\n\tFrom;\n\tb=wCHVJfXOozpPq21KGlw8YKTtDCOnVIDCGLUBgp7jrAYNkGUamdx0j6ZSmbZQYe4YP\n\tky8oDNZPYTlT6XnvWtvqagmrTw4jU1P+AABps7O/ab6KCAkQ4Kwpn4MJV0vpGY7Agx\n\tdbNork2VTM0qxksl1hlxqbTStkK2cooFSTR8qRZkF/h2BMot2YtQ8aeJGLp87Dsl9V\n\tlRbUKQqMWLfCUX7QIQ6F14nFPAyrpJ4ygNzJJvVKQOpidfAq6dD/4/qR6J+Pk3eMXU\n\tDPOHZAIQGO149nanYkjSnauqQ8hsWoe55iNxH6rVbctdfA7rraNbwlD5IYxPJpcJY2\n\tyvvA081+8MVYg==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1666802293;\n\tbh=hHfXgHME2FbFAMUbgxMtSrmYcZb32c3fhbjGH65s4DI=;\n\th=In-Reply-To:References:Subject:From:To:Date:From;\n\tb=lDJ0yMKu9RJHs7DKYHWDKTuIsiz/x7DMXQAerCbwbBneZaVOOOY718gmPgEUk+Uxr\n\tzhsoqZpcwE+CFH3MvvgsIi6wngPvaa7/fIudwBi8bfeAWd1gjsIwaz6Eq9/LlOKAeC\n\tILNIEx0iMnC1KrMf/DI1A8P2fLucPp7SGLTcGiqQ="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"lDJ0yMKu\"; dkim-atps=neutral","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20221019090107.19975-3-naush@raspberrypi.com>","References":"<20221019090107.19975-1-naush@raspberrypi.com>\n\t<20221019090107.19975-3-naush@raspberrypi.com>","To":"Naushir Patuck <naush@raspberrypi.com>,\n\tlibcamera-devel@lists.libcamera.org","Date":"Wed, 26 Oct 2022 17:38:11 +0100","Message-ID":"<166680229104.2677993.15469998037998422292@Monstersaurus>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [PATCH v4 2/7] delayed_controls: Add user\n\tcookie to DelayedControls","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":"Kieran Bingham via libcamera-devel\n\t<libcamera-devel@lists.libcamera.org>","Reply-To":"Kieran Bingham <kieran.bingham@ideasonboard.com>","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":25620,"web_url":"https://patchwork.libcamera.org/comment/25620/","msgid":"<CAEmqJPrad5SbQ4v-L6K6pHXSAuReGkzd+RKQ48GXfm63qRA=5A@mail.gmail.com>","date":"2022-10-27T09:35:53","subject":"Re: [libcamera-devel] [PATCH v4 2/7] delayed_controls: Add user\n\tcookie to DelayedControls","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi Kieran,\n\nThank you for the review!\n\nOn Wed, 26 Oct 2022 at 17:38, Kieran Bingham <\nkieran.bingham@ideasonboard.com> wrote:\n\n> Quoting Naushir Patuck via libcamera-devel (2022-10-19 10:01:02)\n> > Allow the caller to provide a cookie value to DelayedControls::reset and\n> > DelayedControls::push. This cookie value is returned from\n> DelayedControls::get\n> > for the frame that has the control values applied to it.\n> >\n> > The cookie value is useful in tracking when a set of controls has been\n> applied\n> > to a frame. In a subsequent commit, it will be used by the Raspberry Pi\n> IPA to\n> > track the IPA context used when setting the control values.\n> >\n> > Signed-off-by: Naushir Patuck <naush@raspberrypi.com>\n> > Reviewed-by: David Plowman <david.plowman@raspberrypi.com>\n> > Tested-by: David Plowman <david.plowman@raspberrypi.com>\n> > ---\n> >  include/libcamera/internal/delayed_controls.h |  8 +++++---\n> >  src/libcamera/delayed_controls.cpp            | 20 ++++++++++++-------\n> >  src/libcamera/pipeline/ipu3/ipu3.cpp          |  3 ++-\n> >  .../pipeline/raspberrypi/raspberrypi.cpp      |  2 +-\n> >  src/libcamera/pipeline/rkisp1/rkisp1.cpp      |  3 ++-\n> >  test/delayed_controls.cpp                     |  8 ++++----\n> >  6 files changed, 27 insertions(+), 17 deletions(-)\n> >\n> > diff --git a/include/libcamera/internal/delayed_controls.h\n> b/include/libcamera/internal/delayed_controls.h\n> > index de8026e3e4f0..c7e79150afb8 100644\n> > --- a/include/libcamera/internal/delayed_controls.h\n> > +++ b/include/libcamera/internal/delayed_controls.h\n> > @@ -9,6 +9,7 @@\n> >\n> >  #include <stdint.h>\n> >  #include <unordered_map>\n> > +#include <utility>\n> >\n> >  #include <libcamera/controls.h>\n> >\n> > @@ -27,10 +28,10 @@ public:\n> >         DelayedControls(V4L2Device *device,\n> >                         const std::unordered_map<uint32_t,\n> ControlParams> &controlParams);\n> >\n> > -       void reset();\n> > +       void reset(unsigned int cookie = 0);\n> >\n> > -       bool push(const ControlList &controls);\n> > -       ControlList get(uint32_t sequence);\n> > +       bool push(const ControlList &controls, unsigned int cookie = 0);\n>\n> The part that worries me here is that we /require/ always getting the\n> cookie, but not setting it...\n>\n> I would feel safer if this wasn't defaulted to 0...\n>\n\nSure, I'll remove the default values from push() and reset(), and make the\nbelow\nsuggested changes to the ipu3 and rkisp pipeline handlers.\n\nRegards,\nNaush\n\n\n>\n> > +       std::pair<ControlList, unsigned int> get(uint32_t sequence);\n> >\n> >         void applyControls(uint32_t sequence);\n> >\n> > @@ -77,6 +78,7 @@ private:\n> >         uint32_t writeCount_;\n> >         /* \\todo Evaluate if we should index on ControlId * or unsigned\n> int */\n> >         std::unordered_map<const ControlId *, RingBuffer<Info>> values_;\n> > +       RingBuffer<unsigned int> cookies_;\n> >  };\n> >\n> >  } /* namespace libcamera */\n> > diff --git a/src/libcamera/delayed_controls.cpp\n> b/src/libcamera/delayed_controls.cpp\n> > index 777441e8222a..3e7f60f9c421 100644\n> > --- a/src/libcamera/delayed_controls.cpp\n> > +++ b/src/libcamera/delayed_controls.cpp\n> > @@ -109,14 +109,16 @@ DelayedControls::DelayedControls(V4L2Device\n> *device,\n> >\n> >  /**\n> >   * \\brief Reset state machine\n> > + * \\param[in] cookie User supplied reset cookie value\n> >   *\n> >   * Resets the state machine to a starting position based on control\n> values\n> >   * retrieved from the device.\n> >   */\n> > -void DelayedControls::reset()\n> > +void DelayedControls::reset(unsigned int cookie)\n> >  {\n> >         queueCount_ = 1;\n> >         writeCount_ = 0;\n> > +       cookies_[0] = cookie;\n> >\n> >         /* Retrieve control as reported by the device. */\n> >         std::vector<uint32_t> ids;\n> > @@ -140,13 +142,15 @@ void DelayedControls::reset()\n> >  /**\n> >   * \\brief Push a set of controls on the queue\n> >   * \\param[in] controls List of controls to add to the device queue\n> > + * \\param[in] cookie User supplied cookie value for \\a controls\n> >   *\n> >   * Push a set of controls to the control queue. This increases the\n> control queue\n> > - * depth by one.\n> > + * depth by one. The \\a cookie value will be subsequently returned from\n> > + * \\a get() for the frame with all controls applied.\n> >   *\n> >   * \\returns true if \\a controls are accepted, or false otherwise\n> >   */\n> > -bool DelayedControls::push(const ControlList &controls)\n> > +bool DelayedControls::push(const ControlList &controls, const unsigned\n> int cookie)\n> >  {\n> >         /* Copy state from previous frame. */\n> >         for (auto &ctrl : values_) {\n> > @@ -180,6 +184,7 @@ bool DelayedControls::push(const ControlList\n> &controls)\n> >                         << \" at index \" << queueCount_;\n> >         }\n> >\n> > +       cookies_[queueCount_] = cookie;\n> >         queueCount_++;\n> >\n> >         return true;\n> > @@ -198,9 +203,10 @@ bool DelayedControls::push(const ControlList\n> &controls)\n> >   * push(). The max history from the current sequence number that yields\n> valid\n> >   * values are thus 16 minus number of controls pushed.\n> >   *\n> > - * \\return The controls at \\a sequence number\n> > + * \\return The controls at \\a sequence number and associated user\n> supplied\n> > + * cookie value.\n> >   */\n> > -ControlList DelayedControls::get(uint32_t sequence)\n> > +std::pair<ControlList, unsigned int> DelayedControls::get(uint32_t\n> sequence)\n> >  {\n> >         unsigned int index = std::max<int>(0, sequence - maxDelay_);\n> >\n> > @@ -217,7 +223,7 @@ ControlList DelayedControls::get(uint32_t sequence)\n> >                         << \" at index \" << index;\n> >         }\n> >\n> > -       return out;\n> > +       return { out, cookies_[index] };\n> >  }\n> >\n> >  /**\n> > @@ -276,7 +282,7 @@ void DelayedControls::applyControls(uint32_t\n> sequence)\n> >         while (writeCount_ > queueCount_) {\n> >                 LOG(DelayedControls, Debug)\n> >                         << \"Queue is empty, auto queue no-op.\";\n> > -               push({});\n> > +               push({}, cookies_[queueCount_ - 1]);\n> >         }\n> >\n> >         device_->setControls(&out);\n> > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > index 3b892d9671c5..bf612089fdcb 100644\n> > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > @@ -1389,7 +1389,8 @@ void IPU3CameraData::cio2BufferReady(FrameBuffer\n> *buffer)\n> >         request->metadata().set(controls::SensorTimestamp,\n> >                                 buffer->metadata().timestamp);\n> >\n> > -       info->effectiveSensorControls =\n> delayedCtrls_->get(buffer->metadata().sequence);\n> > +       auto [controls, cookie] =\n> delayedCtrls_->get(buffer->metadata().sequence);\n> > +       info->effectiveSensorControls = std::move(controls);\n>\n> Which means this cookie is always zero.\n>\n> Could you also update the push call to set the cookie?\n>\n> That looks like it happens at\n>\n> void IPU3CameraData::setSensorControls([[maybe_unused]] unsigned int id,\n>                                        const ControlList &sensorControls,\n>                                        const ControlList &lensControls)\n> {\n>         delayedCtrls_->push(sensorControls);\n> ...\n>\n> Where the 'id' is actually the request sequence number in the current\n> implementation.\n>\n> So we can remove [[maybe_unused]] from unsigned int id, and pass it to\n> push().\n>\n> Ultimately, this then means that the cookie is the request sequence\n> number of the request which is supposed to reflect these controls. Which\n> is a nice way of tieing these together.\n>\n>\n> >         if (request->findBuffer(&rawStream_))\n> >                 pipe()->completeBuffer(request, buffer);\n> > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > index 343f8cb2c7ed..23f2460190f4 100644\n> > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > @@ -1867,7 +1867,7 @@ void\n> RPiCameraData::unicamBufferDequeue(FrameBuffer *buffer)\n> >                  * Lookup the sensor controls used for this frame\n> sequence from\n> >                  * DelayedControl and queue them along with the frame\n> buffer.\n> >                  */\n> > -               ControlList ctrl =\n> delayedCtrls_->get(buffer->metadata().sequence);\n> > +               auto [ctrl, cookie] =\n> delayedCtrls_->get(buffer->metadata().sequence);\n> >                 /*\n> >                  * Add the frame timestamp to the ControlList for the\n> IPA to use\n> >                  * as it does not receive the FrameBuffer object.\n> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > index 455ee2a0a711..20049d089472 100644\n> > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > @@ -1191,8 +1191,9 @@ void PipelineHandlerRkISP1::statReady(FrameBuffer\n> *buffer)\n> >         if (data->frame_ <= buffer->metadata().sequence)\n> >                 data->frame_ = buffer->metadata().sequence + 1;\n> >\n> > +       auto [controls, cookie] =\n> data->delayedCtrls_->get(buffer->metadata().sequence);\n>\n> Same for rkisp1:\n>\n> void RkISP1CameraData::setSensorControls([[maybe_unused]] unsigned int\n> frame,\n>                                          const ControlList &sensorControls)\n> {\n>         delayedCtrls_->push(sensorControls);\n> }\n>\n> ... 'frame' should be a suitable/expected cookie here.\n>\n>\n> >         data->ipa_->processStatsBuffer(info->frame,\n> info->statBuffer->cookie(),\n> > -\n> data->delayedCtrls_->get(buffer->metadata().sequence));\n> > +                                      controls);\n> >  }\n> >\n> >  REGISTER_PIPELINE_HANDLER(PipelineHandlerRkISP1)\n> > diff --git a/test/delayed_controls.cpp b/test/delayed_controls.cpp\n> > index a8ce9828d73d..322c545998b2 100644\n> > --- a/test/delayed_controls.cpp\n> > +++ b/test/delayed_controls.cpp\n>\n> And in here, testing locally - then the pushes here need a cookie - and\n> 'i' seems suitable.\n>\n> With that,\n>\n>\n> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>\n>\n> > @@ -96,7 +96,7 @@ protected:\n> >\n> >                         delayed->applyControls(i);\n> >\n> > -                       ControlList result = delayed->get(i);\n> > +                       auto [result, cookie] = delayed->get(i);\n> >                         int32_t brightness =\n> result.get(V4L2_CID_BRIGHTNESS).get<int32_t>();\n> >                         if (brightness != value) {\n> >                                 cerr << \"Failed single control without\n> delay\"\n> > @@ -138,7 +138,7 @@ protected:\n> >\n> >                         delayed->applyControls(i);\n> >\n> > -                       ControlList result = delayed->get(i);\n> > +                       auto [result, cookie] = delayed->get(i);\n> >                         int32_t brightness =\n> result.get(V4L2_CID_BRIGHTNESS).get<int32_t>();\n> >                         if (brightness != expected) {\n> >                                 cerr << \"Failed single control with\n> delay\"\n> > @@ -187,7 +187,7 @@ protected:\n> >\n> >                         delayed->applyControls(i);\n> >\n> > -                       ControlList result = delayed->get(i);\n> > +                       auto [result, cookie] = delayed->get(i);\n> >                         int32_t brightness =\n> result.get(V4L2_CID_BRIGHTNESS).get<int32_t>();\n> >                         int32_t contrast =\n> result.get(V4L2_CID_CONTRAST).get<int32_t>();\n> >                         if (brightness != expected || contrast !=\n> expected + 1) {\n> > @@ -247,7 +247,7 @@ protected:\n> >\n> >                         delayed->applyControls(i);\n> >\n> > -                       ControlList result = delayed->get(i);\n> > +                       auto [result, cookie] = delayed->get(i);\n> >\n> >                         int32_t brightness =\n> result.get(V4L2_CID_BRIGHTNESS).get<int32_t>();\n> >                         int32_t contrast =\n> result.get(V4L2_CID_CONTRAST).get<int32_t>();\n> > --\n> > 2.25.1\n> >\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 7099CBDB16\n\tfor <parsemail@patchwork.libcamera.org>;\n\tThu, 27 Oct 2022 09:36:13 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 8C86C62F6F;\n\tThu, 27 Oct 2022 11:36:12 +0200 (CEST)","from mail-qk1-x72f.google.com (mail-qk1-x72f.google.com\n\t[IPv6:2607:f8b0:4864:20::72f])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 6C91C62F69\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tThu, 27 Oct 2022 11:36:10 +0200 (CEST)","by mail-qk1-x72f.google.com with SMTP id j21so423617qkk.9\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tThu, 27 Oct 2022 02:36:10 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1666863372;\n\tbh=4M3C56XU+whFBVV7nRcBeSSlBOOBJNu7ChyXsZPQcf0=;\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=DC3f805vfJL8G8bEfOIKB97CSmWGUCAC3u4cv3h918Xf/JxMSAoCErPNTyl6PKS1q\n\trS5oUeWsQlbbceGle5XWy4Hm8qRBbgtJyTOo8kXU7qG0wZFhU2kJw1Ja90N6E7YqJI\n\thJSqS2fjc9119Z+76UI8elkU4edA+e6Bn/ppkOYI8Fbmd/bmJAZfmwivtbu8nUtSYV\n\t12D0j3f0ZXm1lWxrc0qe/HpHrkSbMqeZjGz5InVXF13w8Q5l8rkXpqNB7Ojqg2A6A2\n\tDRBvOOTuo1W4XSiqBsjFRs3PXSjZCwbcO0q9IaAAWoFNj4TviKeg0k6STE9TtN/R0l\n\tf1A0DNdTKu71A==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google;\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=s06yBXrbC+Qj4EnBuGMrWT2yqB7ILHFB+hd0XWqmMPc=;\n\tb=nlY1CywLE+XBzFAuKkJYMextZOR8M+ZItf+KoZjVtWlJ68lJk2SghFdwVIrVsVwRnF\n\tz9UGNil6x1gbIGQz20TWFYYf5sdWb0Buud7zi1uKDqjD/wxEeD/3AREZ4bRTXZHVwRPd\n\tmxjnPhj4yo0id6FAg4fyvuDmlMPEAPVnUF3+f8TXZ9NJfr73Dk4Vc5CzoHOZxRIJLQGs\n\tiPOhHXoOG6e8n/8OfS2hmKTVTcn2pCvnw66/dTMncZbd25ncWie/7tKsWfI2F2U+iGV+\n\tLGSeBp/h+1zURoiyn5K10gqlClx2GjiyMUTTqdAwx7qE9TIrLyQoUrSPDPnlPlCgn/+2\n\tN2AA=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=raspberrypi.com\n\theader.i=@raspberrypi.com\n\theader.b=\"nlY1CywL\"; dkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112;\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=s06yBXrbC+Qj4EnBuGMrWT2yqB7ILHFB+hd0XWqmMPc=;\n\tb=HLrtz3+de3oNZjk7ytkndkTSq+P1goiHruAfjULWzckTjNyhBbT8ye4hIo9OlnJi73\n\tssfJm0LqIUYonVHtiV8ypz7vdOTM8UigVo+PVc8Ntu3SL5qWDJ1jsN8CuMmOcyOaiPi+\n\tsNUkNEK+aYYs8ZpZ1p5ybzmFHyCISLImLr+XJvg7ByGxhktpHkjNKmYGHlzntFL6/AEE\n\tXhsmbWTA9PZ6ElInGU73L4cUkyt99oTPquPvgrFMlzwLBX3PMHtB9mX1x9W49xKnP2fQ\n\tVWytpAVSzaB70yckQaa3c271SYMjXx5iR/jfUS8wkwUlBakxyH5/ZGL2ls9cf4VLvG3w\n\tBkxw==","X-Gm-Message-State":"ACrzQf13txbNwifP+dLL01t/JMwcClikMQzmQemhWFZGvxeSiaKISd05\n\tRzUy9TlGqayF2L1Q02m7XVHLd6i1Rov/DMe5FAva4w==","X-Google-Smtp-Source":"AMsMyM4iDl99C3mBIDXAPaQQVVAisv99Nkdk2fnhf0t1Akg14IqdlCfRiWks9zBaOzqCsAnCA7yx3oGXo3rs0Y6c6+4=","X-Received":"by 2002:a37:f502:0:b0:6f8:c3b2:51b9 with SMTP id\n\tl2-20020a37f502000000b006f8c3b251b9mr6301280qkk.616.1666863368965;\n\tThu, 27 Oct 2022 02:36:08 -0700 (PDT)","MIME-Version":"1.0","References":"<20221019090107.19975-1-naush@raspberrypi.com>\n\t<20221019090107.19975-3-naush@raspberrypi.com>\n\t<166680229104.2677993.15469998037998422292@Monstersaurus>","In-Reply-To":"<166680229104.2677993.15469998037998422292@Monstersaurus>","Date":"Thu, 27 Oct 2022 10:35:53 +0100","Message-ID":"<CAEmqJPrad5SbQ4v-L6K6pHXSAuReGkzd+RKQ48GXfm63qRA=5A@mail.gmail.com>","To":"Kieran Bingham <kieran.bingham@ideasonboard.com>","Content-Type":"multipart/alternative; boundary=\"000000000000ff271805ec00de75\"","Subject":"Re: [libcamera-devel] [PATCH v4 2/7] delayed_controls: Add user\n\tcookie to DelayedControls","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":"Naushir Patuck via libcamera-devel\n\t<libcamera-devel@lists.libcamera.org>","Reply-To":"Naushir Patuck <naush@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>"}}]