[{"id":32918,"web_url":"https://patchwork.libcamera.org/comment/32918/","msgid":"<CAEB1ahuMRGbz_h73hbbSnTUW-qSnCp5BtvT89hmRZ+p2i=EvkQ@mail.gmail.com>","date":"2025-01-02T16:13:28","subject":"Re: [PATCH 1/8] libcamera: camera: Introduce metadataAvailable\n\tsignal","submitter":{"id":117,"url":"https://patchwork.libcamera.org/api/people/117/","name":"Cheng-Hao Yang","email":"chenghaoyang@chromium.org"},"content":"Hi Jacopo,\n\nThank you for the patch.\n\nOn Fri, Dec 6, 2024 at 5:07 PM Jacopo Mondi\n<jacopo.mondi@ideasonboard.com> wrote:\n>\n> Add a new signal to the Camera class that allows applications to\n> receive notifications for early completion of metadata results.\n>\n> To avoid expensive copies of the metadata results the signal\n> transports the ids of the metadata keys that are ready. Applications\n> can use these ids to access the metadata results from\n> Request::metadata().\n\nYes, it avoids expensive copies, while I think we should restrict\nusages of `Request->metadata()` both in the application and pipeline\nhandlers as well:\n1. They should both access the metadata ControlList in the\nCameraManager's thread, which is the pipeline handlers' owner thread,\nto avoid race conditions. (I think `std::unordered_map` of\n`ControlList::controls_` is not thread-safe, as a set of the map might\nlead to rehashing. Please correct me if I am wrong.)\n2. Pipeline handlers should not directly modify `Request->metadata()`,\nto avoid overwriting. Instead, they should utilize the helper\nfunctions `metadataAvailable` in the third patch in this series.\n\nI've seen (2.) has been implemented in the following patches, thanks.\nDo you think we can ensure (1.) though? Maybe at least documenting it?\n\nBR,\nHarvey\n\n>\n> The signal is an opt-in feature for applications and the sum of\n> all metadata results notified through this signal is available\n> in Request::metadata() at request completion time.\n>\n> Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>\n> ---\n>  include/libcamera/camera.h |  2 ++\n>  src/libcamera/camera.cpp   | 42 ++++++++++++++++++++++++++++++++++++++\n>  2 files changed, 44 insertions(+)\n>\n> diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h\n> index 94cee7bd86bb..20d51c191ecd 100644\n> --- a/include/libcamera/camera.h\n> +++ b/include/libcamera/camera.h\n> @@ -13,6 +13,7 @@\n>  #include <set>\n>  #include <stdint.h>\n>  #include <string>\n> +#include <unordered_set>\n>\n>  #include <libcamera/base/class.h>\n>  #include <libcamera/base/flags.h>\n> @@ -122,6 +123,7 @@ public:\n>\n>         const std::string &id() const;\n>\n> +       Signal<Request *, std::unordered_set<const ControlId *>> metadataAvailable;\n>         Signal<Request *, FrameBuffer *> bufferCompleted;\n>         Signal<Request *> requestCompleted;\n>         Signal<> disconnected;\n> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> index 4c865a46af53..63e78b24e271 100644\n> --- a/src/libcamera/camera.cpp\n> +++ b/src/libcamera/camera.cpp\n> @@ -886,6 +886,48 @@ const std::string &Camera::id() const\n>         return _d()->id_;\n>  }\n>\n> +/**\n> + * \\var Camera::metadataAvailable\n> + * \\brief Signal emitted when metadata for a request are available\n> + *\n> + * The metadataAvailable signal notifies applications about the availability\n> + * of metadata for a request before the request completes.\n> + *\n> + * As metadata results could be large in size, the signal transports the ids\n> + * of the metadata that have just been made available, but the actual control\n> + * values are stored in the Camera::metadata() list.\n> + *\n> + * Applications can access the value of the newly available metadata results\n> + * with:\n> + *\n> + * \\code\n> +\n> +       void metadataAvailableHandler(Request *request,\n> +                                     std::unordered_set<const ControlId *> ids)\n> +       {\n> +               const ControlList &metadata = request->metadata();\n> +\n> +               for (const auto id : ids) {\n> +                       ControlValue &value = metadata.get(id->id());\n> +\n> +                       ....\n> +               }\n> +       }\n> +   \\endcode\n> + *\n> + * This signal is emitted multiple times for the same request, it is in facts\n> + * emitted by the framework every time a new metadata list is made available\n> + * by the Camera to the application.\n> + *\n> + * The sum of all metadata lists reported through this signal is equal to\n> + * Request::metadata() list when the Request completes.\n> + *\n> + * Application can opt-in to handle this signal to receive fast notifications\n> + * of metadata availability or can equally access the full metadata list\n> + * at Request complete time through Request::metadata() if they have no interest\n> + * in early metadata notification.\n> + */\n> +\n>  /**\n>   * \\var Camera::bufferCompleted\n>   * \\brief Signal emitted when a buffer for a request queued to the camera has\n> --\n> 2.47.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 946B5BDB1C\n\tfor <parsemail@patchwork.libcamera.org>;\n\tThu,  2 Jan 2025 16:13:43 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A263D684CF;\n\tThu,  2 Jan 2025 17:13:42 +0100 (CET)","from mail-lj1-x22e.google.com (mail-lj1-x22e.google.com\n\t[IPv6:2a00:1450:4864:20::22e])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 4FA58684CC\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tThu,  2 Jan 2025 17:13:41 +0100 (CET)","by mail-lj1-x22e.google.com with SMTP id\n\t38308e7fff4ca-30229d5b229so123175161fa.0\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tThu, 02 Jan 2025 08:13:41 -0800 (PST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"gzSckh5t\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=chromium.org; s=google; t=1735834420; x=1736439220;\n\tdarn=lists.libcamera.org; \n\th=content-transfer-encoding:cc:to:subject:message-id:date:from\n\t:in-reply-to:references:mime-version:from:to:cc:subject:date\n\t:message-id:reply-to;\n\tbh=t0OS8MGiXaGVCngvrcwcxuVdDq1pXRtg1/WQDcij+EE=;\n\tb=gzSckh5tJdZXyBSHTYi7PHuE8xE/x1b+lhzVZDeFgli3ancZVlC3i3g09cIAJi6YMM\n\t+NOsVgt//k+sVfw4Xa+iqL7O8E2upF1cJLSKEZlHZycHH5eZsBeSbDcnjzsrPajQCxXl\n\t5cfy2jbCTeJnsqVrZJWcfYNTYP4GUYvPX2P8w=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1735834420; x=1736439220;\n\th=content-transfer-encoding:cc:to:subject:message-id:date:from\n\t:in-reply-to:references:mime-version:x-gm-message-state:from:to:cc\n\t:subject:date:message-id:reply-to;\n\tbh=t0OS8MGiXaGVCngvrcwcxuVdDq1pXRtg1/WQDcij+EE=;\n\tb=FYyBTrNfBavU9rGYwn4aLBtYlrAAE0CIyn+KzrTbxm/zQU5Mp0jSHBNCC5R2xhzJo9\n\tDxkhmsI8TmHKcnXLZgoQd8D86r7kktRE3ZjvsE2ie9krNkQIZBaBJ1d+k7Oc0Mv1+P6G\n\t5zn4c4604YKUg/62d8GhXYOaqIItitlzxN1+7n4o/jcWWDdvBbCSUIgqPW6qm5pQRzzC\n\tUY0KJGuILXrL05Gxu/7OScn09Gq6zVwNS8Qk9aDlQJsFV68/ABDQ4XSet099MaqeARO+\n\tDdiamFlqX3U4r0gqESuk30cTJ6RGiRCblUI+HhKe+Bx+H1yiOK8R99VJRvzGMZvUdJzG\n\tWH6w==","X-Gm-Message-State":"AOJu0YyCKUsnWbbBNbHgoS9Kj1oEMv9YqVfVY0fu6OnpXOq8Fe4Tj4Bn\n\tP8KPWoCknZ4STiIqRQwuRW8YCHQRt0e7aiNiv18qOEBZPGygprg0gVLHeAAlKa1bh+aAnM3p1RY\n\tAeg342nGV2Uk5j3OdOuawBgT0C568ZPINbeJ/tsCNpwHQVic=","X-Gm-Gg":"ASbGncukXgH6u40LvyumMHSE7Ygi9E+qwWSoT/sDNukHKy4pIYvAVywlS3Vbn8HTeSm\n\txUc+tYAL8I9jyU91Gjujgt+Oxhj8PxVVGf4JN","X-Google-Smtp-Source":"AGHT+IHHqpPYoMta2ZkWoYmkWSO7LE+QbE6egyAU2F/HjYMQtuE0pZeopo4hlPSF8ciKsMvOrD7yYq9PWPZjx7q4y8E=","X-Received":"by 2002:a2e:bc26:0:b0:2ff:cfbb:c893 with SMTP id\n\t38308e7fff4ca-304685032femr135156401fa.6.1735834420398;\n\tThu, 02 Jan 2025 08:13:40 -0800 (PST)","MIME-Version":"1.0","References":"<20241206160747.97176-1-jacopo.mondi@ideasonboard.com>\n\t<20241206160747.97176-2-jacopo.mondi@ideasonboard.com>","In-Reply-To":"<20241206160747.97176-2-jacopo.mondi@ideasonboard.com>","From":"Cheng-Hao Yang <chenghaoyang@chromium.org>","Date":"Thu, 2 Jan 2025 17:13:28 +0100","Message-ID":"<CAEB1ahuMRGbz_h73hbbSnTUW-qSnCp5BtvT89hmRZ+p2i=EvkQ@mail.gmail.com>","Subject":"Re: [PATCH 1/8] libcamera: camera: Introduce metadataAvailable\n\tsignal","To":"Jacopo Mondi <jacopo.mondi@ideasonboard.com>","Cc":"libcamera-devel@lists.libcamera.org, \n\tHan-Lin Chen <hanlinchen@chromium.org>","Content-Type":"text/plain; charset=\"UTF-8\"","Content-Transfer-Encoding":"quoted-printable","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":32927,"web_url":"https://patchwork.libcamera.org/comment/32927/","msgid":"<gjt7q4xa65uvlcsm4rhtcvwzatafwecgvqqd6alf3eqwacness@p3s7qzcmm3li>","date":"2025-01-03T17:01:04","subject":"Re: [PATCH 1/8] libcamera: camera: Introduce metadataAvailable\n\tsignal","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Harvey\n\nOn Thu, Jan 02, 2025 at 05:13:28PM +0100, Cheng-Hao Yang wrote:\n> Hi Jacopo,\n>\n> Thank you for the patch.\n>\n> On Fri, Dec 6, 2024 at 5:07 PM Jacopo Mondi\n> <jacopo.mondi@ideasonboard.com> wrote:\n> >\n> > Add a new signal to the Camera class that allows applications to\n> > receive notifications for early completion of metadata results.\n> >\n> > To avoid expensive copies of the metadata results the signal\n> > transports the ids of the metadata keys that are ready. Applications\n> > can use these ids to access the metadata results from\n> > Request::metadata().\n>\n> Yes, it avoids expensive copies, while I think we should restrict\n> usages of `Request->metadata()` both in the application and pipeline\n> handlers as well:\n> 1. They should both access the metadata ControlList in the\n> CameraManager's thread, which is the pipeline handlers' owner thread,\n> to avoid race conditions. (I think `std::unordered_map` of\n> `ControlList::controls_` is not thread-safe, as a set of the map might\n> lead to rehashing. Please correct me if I am wrong.)\n\nI think you're right, nice catch. Signals across threads are\nasynchronous and there's a window indeed where the map can be accessed\nby the app and the pipeline handler concurrently. Before this series\nthis was not an issue because apps only get the metadata list at\nrequest completion time when the pipeline handler is done (or should\nbe done).\n\nI think your concern is also supported by the following part of\nhttps://en.cppreference.com/w/cpp/container/unordered_map/operator_at\n\n-------------------------------------------------------------------------------\nIf after the operation the new number of elements is greater than old\nmax_load_factor() * bucket_count() a rehashing takes place.  If\nrehashing occurs (due to the insertion), all iterators are\ninvalidated. Otherwise (no rehashing), iterators are not invalidated.\n-------------------------------------------------------------------------------\n\n> 2. Pipeline handlers should not directly modify `Request->metadata()`,\n> to avoid overwriting. Instead, they should utilize the helper\n> functions `metadataAvailable` in the third patch in this series.\n>\n> I've seen (2.) has been implemented in the following patches, thanks.\n> Do you think we can ensure (1.) though? Maybe at least documenting it?\n\nI'm afraid documenting it is not enough, access to the metadata list\nshould be locked if the list is made available to the application before\nthe request has completed...\n\nI initially thought this wouldn't be hard. After all, in this series\npipeline handlers can access Request::metadata_ in rw mode only\nthrough PipelineHandler::metadataAvailable() we could simply lock\nthere and lock Request::metadata().\n\nHowever ControlList provides const iterators and an application might\ndecide to iterate the control list while the pipeline handler\npopulates it with more metadata.\n\nI wonder what are the consequences of increasing\nhttps://en.cppreference.com/w/cpp/container/unordered_map/max_load_factor\nto avoid rehashing...\n\nI'll ask around!\n\nThanks!\n  j\n\n>\n> BR,\n> Harvey\n>\n> >\n> > The signal is an opt-in feature for applications and the sum of\n> > all metadata results notified through this signal is available\n> > in Request::metadata() at request completion time.\n> >\n> > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>\n> > ---\n> >  include/libcamera/camera.h |  2 ++\n> >  src/libcamera/camera.cpp   | 42 ++++++++++++++++++++++++++++++++++++++\n> >  2 files changed, 44 insertions(+)\n> >\n> > diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h\n> > index 94cee7bd86bb..20d51c191ecd 100644\n> > --- a/include/libcamera/camera.h\n> > +++ b/include/libcamera/camera.h\n> > @@ -13,6 +13,7 @@\n> >  #include <set>\n> >  #include <stdint.h>\n> >  #include <string>\n> > +#include <unordered_set>\n> >\n> >  #include <libcamera/base/class.h>\n> >  #include <libcamera/base/flags.h>\n> > @@ -122,6 +123,7 @@ public:\n> >\n> >         const std::string &id() const;\n> >\n> > +       Signal<Request *, std::unordered_set<const ControlId *>> metadataAvailable;\n> >         Signal<Request *, FrameBuffer *> bufferCompleted;\n> >         Signal<Request *> requestCompleted;\n> >         Signal<> disconnected;\n> > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> > index 4c865a46af53..63e78b24e271 100644\n> > --- a/src/libcamera/camera.cpp\n> > +++ b/src/libcamera/camera.cpp\n> > @@ -886,6 +886,48 @@ const std::string &Camera::id() const\n> >         return _d()->id_;\n> >  }\n> >\n> > +/**\n> > + * \\var Camera::metadataAvailable\n> > + * \\brief Signal emitted when metadata for a request are available\n> > + *\n> > + * The metadataAvailable signal notifies applications about the availability\n> > + * of metadata for a request before the request completes.\n> > + *\n> > + * As metadata results could be large in size, the signal transports the ids\n> > + * of the metadata that have just been made available, but the actual control\n> > + * values are stored in the Camera::metadata() list.\n> > + *\n> > + * Applications can access the value of the newly available metadata results\n> > + * with:\n> > + *\n> > + * \\code\n> > +\n> > +       void metadataAvailableHandler(Request *request,\n> > +                                     std::unordered_set<const ControlId *> ids)\n> > +       {\n> > +               const ControlList &metadata = request->metadata();\n> > +\n> > +               for (const auto id : ids) {\n> > +                       ControlValue &value = metadata.get(id->id());\n> > +\n> > +                       ....\n> > +               }\n> > +       }\n> > +   \\endcode\n> > + *\n> > + * This signal is emitted multiple times for the same request, it is in facts\n> > + * emitted by the framework every time a new metadata list is made available\n> > + * by the Camera to the application.\n> > + *\n> > + * The sum of all metadata lists reported through this signal is equal to\n> > + * Request::metadata() list when the Request completes.\n> > + *\n> > + * Application can opt-in to handle this signal to receive fast notifications\n> > + * of metadata availability or can equally access the full metadata list\n> > + * at Request complete time through Request::metadata() if they have no interest\n> > + * in early metadata notification.\n> > + */\n> > +\n> >  /**\n> >   * \\var Camera::bufferCompleted\n> >   * \\brief Signal emitted when a buffer for a request queued to the camera has\n> > --\n> > 2.47.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 B126DC324C\n\tfor <parsemail@patchwork.libcamera.org>;\n\tFri,  3 Jan 2025 17:01:11 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D95D9684CC;\n\tFri,  3 Jan 2025 18:01:10 +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 54E7C61886\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tFri,  3 Jan 2025 18:01:08 +0100 (CET)","from ideasonboard.com (93-61-96-190.ip145.fastwebnet.it\n\t[93.61.96.190])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 94BD822E;\n\tFri,  3 Jan 2025 18:00:18 +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=\"OZrtGDSV\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1735923618;\n\tbh=SsDlwIZz3rMPJysLWADThOfotIp50bBaoIW0ykl/wF8=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=OZrtGDSVdzdNzsPfSapRgfOkJtYN36CRceexlSFGmTemcXpn1ZjqINI4ZYvrpyXHr\n\tw5lLNKOHhCVbPq3VqH3YCjdWgrm2vh2SOW5uNVRf8dptzqz2ccwbv75GglGuSUB7uc\n\t8DSxYru1Cy9cirj8Uiy6J0JF42GULBsGNT+KP63k=","Date":"Fri, 3 Jan 2025 18:01:04 +0100","From":"Jacopo Mondi <jacopo.mondi@ideasonboard.com>","To":"Cheng-Hao Yang <chenghaoyang@chromium.org>","Cc":"Jacopo Mondi <jacopo.mondi@ideasonboard.com>, \n\tlibcamera-devel@lists.libcamera.org,\n\tHan-Lin Chen <hanlinchen@chromium.org>","Subject":"Re: [PATCH 1/8] libcamera: camera: Introduce metadataAvailable\n\tsignal","Message-ID":"<gjt7q4xa65uvlcsm4rhtcvwzatafwecgvqqd6alf3eqwacness@p3s7qzcmm3li>","References":"<20241206160747.97176-1-jacopo.mondi@ideasonboard.com>\n\t<20241206160747.97176-2-jacopo.mondi@ideasonboard.com>\n\t<CAEB1ahuMRGbz_h73hbbSnTUW-qSnCp5BtvT89hmRZ+p2i=EvkQ@mail.gmail.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<CAEB1ahuMRGbz_h73hbbSnTUW-qSnCp5BtvT89hmRZ+p2i=EvkQ@mail.gmail.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":33108,"web_url":"https://patchwork.libcamera.org/comment/33108/","msgid":"<CAEB1ahvAT+ER0Rs8o+EQERO88+8_9Y021mJhRDik4c09SfLx=w@mail.gmail.com>","date":"2025-01-20T21:28:25","subject":"Re: [PATCH 1/8] libcamera: camera: Introduce metadataAvailable\n\tsignal","submitter":{"id":117,"url":"https://patchwork.libcamera.org/api/people/117/","name":"Cheng-Hao Yang","email":"chenghaoyang@chromium.org"},"content":"Hi Jacopo,\n\nFriendly ping: Did you get more opinions from others?\n\nOn Fri, Jan 3, 2025 at 6:01 PM Jacopo Mondi\n<jacopo.mondi@ideasonboard.com> wrote:\n>\n> Hi Harvey\n>\n> On Thu, Jan 02, 2025 at 05:13:28PM +0100, Cheng-Hao Yang wrote:\n> > Hi Jacopo,\n> >\n> > Thank you for the patch.\n> >\n> > On Fri, Dec 6, 2024 at 5:07 PM Jacopo Mondi\n> > <jacopo.mondi@ideasonboard.com> wrote:\n> > >\n> > > Add a new signal to the Camera class that allows applications to\n> > > receive notifications for early completion of metadata results.\n> > >\n> > > To avoid expensive copies of the metadata results the signal\n> > > transports the ids of the metadata keys that are ready. Applications\n> > > can use these ids to access the metadata results from\n> > > Request::metadata().\n> >\n> > Yes, it avoids expensive copies, while I think we should restrict\n> > usages of `Request->metadata()` both in the application and pipeline\n> > handlers as well:\n> > 1. They should both access the metadata ControlList in the\n> > CameraManager's thread, which is the pipeline handlers' owner thread,\n> > to avoid race conditions. (I think `std::unordered_map` of\n> > `ControlList::controls_` is not thread-safe, as a set of the map might\n> > lead to rehashing. Please correct me if I am wrong.)\n>\n> I think you're right, nice catch. Signals across threads are\n> asynchronous and there's a window indeed where the map can be accessed\n> by the app and the pipeline handler concurrently. Before this series\n> this was not an issue because apps only get the metadata list at\n> request completion time when the pipeline handler is done (or should\n> be done).\n>\n> I think your concern is also supported by the following part of\n> https://en.cppreference.com/w/cpp/container/unordered_map/operator_at\n>\n> -------------------------------------------------------------------------------\n> If after the operation the new number of elements is greater than old\n> max_load_factor() * bucket_count() a rehashing takes place.  If\n> rehashing occurs (due to the insertion), all iterators are\n> invalidated. Otherwise (no rehashing), iterators are not invalidated.\n> -------------------------------------------------------------------------------\n>\n> > 2. Pipeline handlers should not directly modify `Request->metadata()`,\n> > to avoid overwriting. Instead, they should utilize the helper\n> > functions `metadataAvailable` in the third patch in this series.\n> >\n> > I've seen (2.) has been implemented in the following patches, thanks.\n> > Do you think we can ensure (1.) though? Maybe at least documenting it?\n>\n> I'm afraid documenting it is not enough, access to the metadata list\n> should be locked if the list is made available to the application before\n> the request has completed...\n>\n> I initially thought this wouldn't be hard. After all, in this series\n> pipeline handlers can access Request::metadata_ in rw mode only\n> through PipelineHandler::metadataAvailable() we could simply lock\n> there and lock Request::metadata().\n>\n> However ControlList provides const iterators and an application might\n> decide to iterate the control list while the pipeline handler\n> populates it with more metadata.\n\nYes, this is one of the issues, if we allow other threads to access ControlList.\n\n>\n> I wonder what are the consequences of increasing\n> https://en.cppreference.com/w/cpp/container/unordered_map/max_load_factor\n> to avoid rehashing...\n\nHmm, it might be a bit hacky though, and the base class might need to\nknow how many control ids will be set in advance.\n\nBR,\nHarvey\n\n>\n> I'll ask around!\n>\n> Thanks!\n>   j\n>\n> >\n> > BR,\n> > Harvey\n> >\n> > >\n> > > The signal is an opt-in feature for applications and the sum of\n> > > all metadata results notified through this signal is available\n> > > in Request::metadata() at request completion time.\n> > >\n> > > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>\n> > > ---\n> > >  include/libcamera/camera.h |  2 ++\n> > >  src/libcamera/camera.cpp   | 42 ++++++++++++++++++++++++++++++++++++++\n> > >  2 files changed, 44 insertions(+)\n> > >\n> > > diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h\n> > > index 94cee7bd86bb..20d51c191ecd 100644\n> > > --- a/include/libcamera/camera.h\n> > > +++ b/include/libcamera/camera.h\n> > > @@ -13,6 +13,7 @@\n> > >  #include <set>\n> > >  #include <stdint.h>\n> > >  #include <string>\n> > > +#include <unordered_set>\n> > >\n> > >  #include <libcamera/base/class.h>\n> > >  #include <libcamera/base/flags.h>\n> > > @@ -122,6 +123,7 @@ public:\n> > >\n> > >         const std::string &id() const;\n> > >\n> > > +       Signal<Request *, std::unordered_set<const ControlId *>> metadataAvailable;\n> > >         Signal<Request *, FrameBuffer *> bufferCompleted;\n> > >         Signal<Request *> requestCompleted;\n> > >         Signal<> disconnected;\n> > > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> > > index 4c865a46af53..63e78b24e271 100644\n> > > --- a/src/libcamera/camera.cpp\n> > > +++ b/src/libcamera/camera.cpp\n> > > @@ -886,6 +886,48 @@ const std::string &Camera::id() const\n> > >         return _d()->id_;\n> > >  }\n> > >\n> > > +/**\n> > > + * \\var Camera::metadataAvailable\n> > > + * \\brief Signal emitted when metadata for a request are available\n> > > + *\n> > > + * The metadataAvailable signal notifies applications about the availability\n> > > + * of metadata for a request before the request completes.\n> > > + *\n> > > + * As metadata results could be large in size, the signal transports the ids\n> > > + * of the metadata that have just been made available, but the actual control\n> > > + * values are stored in the Camera::metadata() list.\n> > > + *\n> > > + * Applications can access the value of the newly available metadata results\n> > > + * with:\n> > > + *\n> > > + * \\code\n> > > +\n> > > +       void metadataAvailableHandler(Request *request,\n> > > +                                     std::unordered_set<const ControlId *> ids)\n> > > +       {\n> > > +               const ControlList &metadata = request->metadata();\n> > > +\n> > > +               for (const auto id : ids) {\n> > > +                       ControlValue &value = metadata.get(id->id());\n> > > +\n> > > +                       ....\n> > > +               }\n> > > +       }\n> > > +   \\endcode\n> > > + *\n> > > + * This signal is emitted multiple times for the same request, it is in facts\n> > > + * emitted by the framework every time a new metadata list is made available\n> > > + * by the Camera to the application.\n> > > + *\n> > > + * The sum of all metadata lists reported through this signal is equal to\n> > > + * Request::metadata() list when the Request completes.\n> > > + *\n> > > + * Application can opt-in to handle this signal to receive fast notifications\n> > > + * of metadata availability or can equally access the full metadata list\n> > > + * at Request complete time through Request::metadata() if they have no interest\n> > > + * in early metadata notification.\n> > > + */\n> > > +\n> > >  /**\n> > >   * \\var Camera::bufferCompleted\n> > >   * \\brief Signal emitted when a buffer for a request queued to the camera has\n> > > --\n> > > 2.47.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 9E1E7C327C\n\tfor <parsemail@patchwork.libcamera.org>;\n\tMon, 20 Jan 2025 21:28:39 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C009F684E7;\n\tMon, 20 Jan 2025 22:28:38 +0100 (CET)","from mail-lf1-x135.google.com (mail-lf1-x135.google.com\n\t[IPv6:2a00:1450:4864:20::135])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 2F77460354\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon, 20 Jan 2025 22:28:37 +0100 (CET)","by mail-lf1-x135.google.com with SMTP id\n\t2adb3069b0e04-5401e6efffcso5428008e87.3\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon, 20 Jan 2025 13:28:37 -0800 (PST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"EiygoD3k\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=chromium.org; s=google; t=1737408516; x=1738013316;\n\tdarn=lists.libcamera.org; \n\th=content-transfer-encoding:cc:to:subject:message-id:date:from\n\t:in-reply-to:references:mime-version:from:to:cc:subject:date\n\t:message-id:reply-to;\n\tbh=TsqS72toDpMUITEuZOfxVrXDIs/K+LCH6NtsYcT1ukw=;\n\tb=EiygoD3kMGaeLETrsXudLw5quos4tisYReX+eEYR3t4pcy9ZI6kjQ3WyANfeKNZwIY\n\tsjGcX24q9LkRogEX89eYuifcXJkKXGdvxfytTPiuwW/HZgw6131c5Rqqiu2oVdgnip4W\n\t4i3Tar0u91578corDVZIDKl5tvePJAhLKuknQ=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1737408516; x=1738013316;\n\th=content-transfer-encoding:cc:to:subject:message-id:date:from\n\t:in-reply-to:references:mime-version:x-gm-message-state:from:to:cc\n\t:subject:date:message-id:reply-to;\n\tbh=TsqS72toDpMUITEuZOfxVrXDIs/K+LCH6NtsYcT1ukw=;\n\tb=mSq2+4pNc2n8tJ2zO4lwdXfgzH/oLQA4aeQKL7rhom/Et6zxAonJpoLMsxLauulBVA\n\tj6r64lRKLB7zphWSAwAJ5HbdgPd4dycJI7HozIzlE4BydTCqRwHXYrZ4Rkrd55dAaTrY\n\tDcrhE+NdMnDvkC+XDBpjirLMvSm2VGirfbi1HPEllbpLP9a71CVoHumXMLpr206xsR0q\n\tVIXUkHvrsnk4Eq2fBzIjsTSGauygESjKKAuMWM8nF6bvAYl+QPGfodefjk6IIY+S30PQ\n\tPL8/lL2LaGgFmC74uBgC7U3f1+6n0Z8lJ3FG5ejzJY7/v1KBCyTmIXlbcKtLAn4RJZJl\n\th38Q==","X-Gm-Message-State":"AOJu0Yx+EnF8GWLYuP7PCTMCSZX2hk4vOsetJUf/ewVY0qerg/KqXXBR\n\tUal5O6KgM3ncWKFOg1pK2Vpgpt5aIGMUI14d5aSOI4y+Kq7qyu7CtjgDTOqLnYZ/sRJIHNxuHju\n\tTiJV+Ln96cvUcHbXeJne1m1FsvttFCcNeXtw1","X-Gm-Gg":"ASbGnct1CYoPpx9E3Co/1zh/drX5j4nWbXDjQILPsHgLEzIEE+hacyZ0TSgNmnwzCOK\n\tXBegK0cGtUwSJrZLURyUMwml0oWL/MFli05WWZnuO5v5SEYUgJ6MgL2wtAoU/","X-Google-Smtp-Source":"AGHT+IEHB8QOxigEMliTSK5Qr6V3ku8mCX6WSMfhC+nFUTEO30jwaMnaT7KUdT7Eh7ZhoLawFuL6/0AphPsbRpr0fxM=","X-Received":"by 2002:ac2:5e9d:0:b0:542:2166:44cb with SMTP id\n\t2adb3069b0e04-5439c282b25mr4591798e87.35.1737408516185;\n\tMon, 20 Jan 2025 13:28:36 -0800 (PST)","MIME-Version":"1.0","References":"<20241206160747.97176-1-jacopo.mondi@ideasonboard.com>\n\t<20241206160747.97176-2-jacopo.mondi@ideasonboard.com>\n\t<CAEB1ahuMRGbz_h73hbbSnTUW-qSnCp5BtvT89hmRZ+p2i=EvkQ@mail.gmail.com>\n\t<gjt7q4xa65uvlcsm4rhtcvwzatafwecgvqqd6alf3eqwacness@p3s7qzcmm3li>","In-Reply-To":"<gjt7q4xa65uvlcsm4rhtcvwzatafwecgvqqd6alf3eqwacness@p3s7qzcmm3li>","From":"Cheng-Hao Yang <chenghaoyang@chromium.org>","Date":"Mon, 20 Jan 2025 22:28:25 +0100","X-Gm-Features":"AbW1kvaaUyLGN2BSbojpmVCvQ3CYJXWRV59G-P0_n69yUxM2oL_7rbMuUSPpiFE","Message-ID":"<CAEB1ahvAT+ER0Rs8o+EQERO88+8_9Y021mJhRDik4c09SfLx=w@mail.gmail.com>","Subject":"Re: [PATCH 1/8] libcamera: camera: Introduce metadataAvailable\n\tsignal","To":"Jacopo Mondi <jacopo.mondi@ideasonboard.com>","Cc":"libcamera-devel@lists.libcamera.org, \n\tHan-Lin Chen <hanlinchen@chromium.org>","Content-Type":"text/plain; charset=\"UTF-8\"","Content-Transfer-Encoding":"quoted-printable","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":33112,"web_url":"https://patchwork.libcamera.org/comment/33112/","msgid":"<RqeiGJtrHv3e7oo6v5iQT1R-jH8BxuP6DffK25MzVBT0Lo6N6PK-isOJ1g77V7KIePv7ewtsN_Xqjc_uRSyCzbIsLES7YMkHX6uZFIuCgGk=@protonmail.com>","date":"2025-01-21T10:32:13","subject":"Re: [PATCH 1/8] libcamera: camera: Introduce metadataAvailable\n\tsignal","submitter":{"id":133,"url":"https://patchwork.libcamera.org/api/people/133/","name":"Pőcze Barnabás","email":"pobrn@protonmail.com"},"content":"Hi\n\n\n2025. január 3., péntek 18:01 keltezéssel, Jacopo Mondi <jacopo.mondi@ideasonboard.com> írta:\n\n> Hi Harvey\n> \n> On Thu, Jan 02, 2025 at 05:13:28PM +0100, Cheng-Hao Yang wrote:\n> > Hi Jacopo,\n> >\n> > Thank you for the patch.\n> >\n> > On Fri, Dec 6, 2024 at 5:07 PM Jacopo Mondi\n> > <jacopo.mondi@ideasonboard.com> wrote:\n> > >\n> > > Add a new signal to the Camera class that allows applications to\n> > > receive notifications for early completion of metadata results.\n> > >\n> > > To avoid expensive copies of the metadata results the signal\n> > > transports the ids of the metadata keys that are ready. Applications\n> > > can use these ids to access the metadata results from\n> > > Request::metadata().\n> >\n> > Yes, it avoids expensive copies, while I think we should restrict\n> > usages of `Request->metadata()` both in the application and pipeline\n> > handlers as well:\n> > 1. They should both access the metadata ControlList in the\n> > CameraManager's thread, which is the pipeline handlers' owner thread,\n> > to avoid race conditions. (I think `std::unordered_map` of\n> > `ControlList::controls_` is not thread-safe, as a set of the map might\n> > lead to rehashing. Please correct me if I am wrong.)\n> \n> I think you're right, nice catch. Signals across threads are\n> asynchronous and there's a window indeed where the map can be accessed\n> by the app and the pipeline handler concurrently. Before this series\n> this was not an issue because apps only get the metadata list at\n> request completion time when the pipeline handler is done (or should\n> be done).\n\nBased on my understanding, signals are delivered synchronously unless the target\nobject derives from `libcamera::Object`. So application callbacks will be invoked\ndirectly from the CameraManager's thread (or whichever internal thread is used).\nAs far as I am aware, inheriting `libcamera::Object` in application code is not\nsupported and it just would not work currently because `libcamera::Thread` is not\npublic, so the application would have no way of running the internal event loop.\n\n\n> \n> I think your concern is also supported by the following part of\n> https://en.cppreference.com/w/cpp/container/unordered_map/operator_at\n> \n> -------------------------------------------------------------------------------\n> If after the operation the new number of elements is greater than old\n> max_load_factor() * bucket_count() a rehashing takes place.  If\n> rehashing occurs (due to the insertion), all iterators are\n> invalidated. Otherwise (no rehashing), iterators are not invalidated.\n> -------------------------------------------------------------------------------\n> \n> > 2. Pipeline handlers should not directly modify `Request->metadata()`,\n> > to avoid overwriting. Instead, they should utilize the helper\n> > functions `metadataAvailable` in the third patch in this series.\n> >\n> > I've seen (2.) has been implemented in the following patches, thanks.\n> > Do you think we can ensure (1.) though? Maybe at least documenting it?\n> \n> I'm afraid documenting it is not enough, access to the metadata list\n> should be locked if the list is made available to the application before\n> the request has completed...\n> \n> I initially thought this wouldn't be hard. After all, in this series\n> pipeline handlers can access Request::metadata_ in rw mode only\n> through PipelineHandler::metadataAvailable() we could simply lock\n> there and lock Request::metadata().\n> \n> However ControlList provides const iterators and an application might\n> decide to iterate the control list while the pipeline handler\n> populates it with more metadata.\n\nSince the signal is dispatched synchronously, I think that is not possible*, so\nthe current setup might be acceptable. (Or am I missing something?) The burden of\nsynchronization falls fully on the user, but considering that the callback runs\nin an internal thread, the user code would probably have to do some kind of\nsynchronization in any case.\n\n[*]: Unless the user code saves iterators or references to the metadata control\nlist and reuses them after the signal handler returns in some other thread before\nthe request completes. But I am wondering if that is a significant concern?\n\nI suppose the parametrization of the signal could be changed to discourage the\ndirect use of `Request`. Although things like the request cookie would still\nneed to be available in some way.\n\nAnother thing, libdbus \"locks\" messages when they are submitted, maybe a\nsimilar idea could be implemented here as well with `Request`.\n\n\nRegards,\nBarnabás Pőcze\n\n> \n> I wonder what are the consequences of increasing\n> https://en.cppreference.com/w/cpp/container/unordered_map/max_load_factor\n> to avoid rehashing...\n> \n> I'll ask around!\n> \n> Thanks!\n>   j\n> \n> >\n> > BR,\n> > Harvey\n> >\n> > >\n> > > The signal is an opt-in feature for applications and the sum of\n> > > all metadata results notified through this signal is available\n> > > in Request::metadata() at request completion time.\n> > >\n> > > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>\n> > > ---\n> > >  include/libcamera/camera.h |  2 ++\n> > >  src/libcamera/camera.cpp   | 42 ++++++++++++++++++++++++++++++++++++++\n> > >  2 files changed, 44 insertions(+)\n> > >\n> > > diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h\n> > > index 94cee7bd86bb..20d51c191ecd 100644\n> > > --- a/include/libcamera/camera.h\n> > > +++ b/include/libcamera/camera.h\n> > > @@ -13,6 +13,7 @@\n> > >  #include <set>\n> > >  #include <stdint.h>\n> > >  #include <string>\n> > > +#include <unordered_set>\n> > >\n> > >  #include <libcamera/base/class.h>\n> > >  #include <libcamera/base/flags.h>\n> > > @@ -122,6 +123,7 @@ public:\n> > >\n> > >         const std::string &id() const;\n> > >\n> > > +       Signal<Request *, std::unordered_set<const ControlId *>> metadataAvailable;\n> > >         Signal<Request *, FrameBuffer *> bufferCompleted;\n> > >         Signal<Request *> requestCompleted;\n> > >         Signal<> disconnected;\n> > > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> > > index 4c865a46af53..63e78b24e271 100644\n> > > --- a/src/libcamera/camera.cpp\n> > > +++ b/src/libcamera/camera.cpp\n> > > @@ -886,6 +886,48 @@ const std::string &Camera::id() const\n> > >         return _d()->id_;\n> > >  }\n> > >\n> > > +/**\n> > > + * \\var Camera::metadataAvailable\n> > > + * \\brief Signal emitted when metadata for a request are available\n> > > + *\n> > > + * The metadataAvailable signal notifies applications about the availability\n> > > + * of metadata for a request before the request completes.\n> > > + *\n> > > + * As metadata results could be large in size, the signal transports the ids\n> > > + * of the metadata that have just been made available, but the actual control\n> > > + * values are stored in the Camera::metadata() list.\n> > > + *\n> > > + * Applications can access the value of the newly available metadata results\n> > > + * with:\n> > > + *\n> > > + * \\code\n> > > +\n> > > +       void metadataAvailableHandler(Request *request,\n> > > +                                     std::unordered_set<const ControlId *> ids)\n> > > +       {\n> > > +               const ControlList &metadata = request->metadata();\n> > > +\n> > > +               for (const auto id : ids) {\n> > > +                       ControlValue &value = metadata.get(id->id());\n> > > +\n> > > +                       ....\n> > > +               }\n> > > +       }\n> > > +   \\endcode\n> > > + *\n> > > + * This signal is emitted multiple times for the same request, it is in facts\n> > > + * emitted by the framework every time a new metadata list is made available\n> > > + * by the Camera to the application.\n> > > + *\n> > > + * The sum of all metadata lists reported through this signal is equal to\n> > > + * Request::metadata() list when the Request completes.\n> > > + *\n> > > + * Application can opt-in to handle this signal to receive fast notifications\n> > > + * of metadata availability or can equally access the full metadata list\n> > > + * at Request complete time through Request::metadata() if they have no interest\n> > > + * in early metadata notification.\n> > > + */\n> > > +\n> > >  /**\n> > >   * \\var Camera::bufferCompleted\n> > >   * \\brief Signal emitted when a buffer for a request queued to the camera has\n> > > --\n> > > 2.47.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 D4B98BD16B\n\tfor <parsemail@patchwork.libcamera.org>;\n\tTue, 21 Jan 2025 10:32:23 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id ACDFA68543;\n\tTue, 21 Jan 2025 11:32:22 +0100 (CET)","from mail-40134.protonmail.ch (mail-40134.protonmail.ch\n\t[185.70.40.134])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 649D668516\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tTue, 21 Jan 2025 11:32:20 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=protonmail.com header.i=@protonmail.com\n\theader.b=\"FBbJeNRq\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=protonmail.com;\n\ts=protonmail3; t=1737455539; x=1737714739;\n\tbh=eufXGZfQioPjZ2fUxmRZd5QBe5ucNPLkMW2kVsMOujQ=;\n\th=Date:To:From:Cc:Subject:Message-ID:In-Reply-To:References:\n\tFeedback-ID:From:To:Cc:Date:Subject:Reply-To:Feedback-ID:\n\tMessage-ID:BIMI-Selector:List-Unsubscribe:List-Unsubscribe-Post;\n\tb=FBbJeNRqk79aX2olnkEiWFxm89Pp5MTI22j82+MB55fz4GjuUZxQstM32VbkupMIy\n\tqAGWLKrevBM0R1kcLiduC+n34gVr5K5vp+mvSYE3Nn6RejBaoBZj0/KTkqpIA3/lCr\n\tZlUViCMiZMBL6ewkaSaoGv1hUQ+KFMNPkTybSpLVfl9W3vPQsMI9Iy8mE8MUyypEE+\n\t6JGlOIk7MtgAORmJuy/oMvr0iF1Y4WQSrSS1ZA/6WHfvjyHnt571q7LTXGECTNeSe2\n\tz2qB4LXufbrgKSO5KtlwxVO6+ucGZ6HNcQQ8YLyDQ3xQPleHY2ErpD11rxVsoBv4YG\n\tk6HASHVfvubxw==","Date":"Tue, 21 Jan 2025 10:32:13 +0000","To":"Jacopo Mondi <jacopo.mondi@ideasonboard.com>","From":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= <pobrn@protonmail.com>","Cc":"Cheng-Hao Yang <chenghaoyang@chromium.org>,\n\tlibcamera-devel@lists.libcamera.org,\n\tHan-Lin Chen <hanlinchen@chromium.org>","Subject":"Re: [PATCH 1/8] libcamera: camera: Introduce metadataAvailable\n\tsignal","Message-ID":"<RqeiGJtrHv3e7oo6v5iQT1R-jH8BxuP6DffK25MzVBT0Lo6N6PK-isOJ1g77V7KIePv7ewtsN_Xqjc_uRSyCzbIsLES7YMkHX6uZFIuCgGk=@protonmail.com>","In-Reply-To":"<gjt7q4xa65uvlcsm4rhtcvwzatafwecgvqqd6alf3eqwacness@p3s7qzcmm3li>","References":"<20241206160747.97176-1-jacopo.mondi@ideasonboard.com>\n\t<20241206160747.97176-2-jacopo.mondi@ideasonboard.com>\n\t<CAEB1ahuMRGbz_h73hbbSnTUW-qSnCp5BtvT89hmRZ+p2i=EvkQ@mail.gmail.com>\n\t<gjt7q4xa65uvlcsm4rhtcvwzatafwecgvqqd6alf3eqwacness@p3s7qzcmm3li>","Feedback-ID":"20568564:user:proton","X-Pm-Message-ID":"6ce0a83d76cabac438e5694d8be0dbfd26690d79","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Transfer-Encoding":"quoted-printable","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":33118,"web_url":"https://patchwork.libcamera.org/comment/33118/","msgid":"<CAEB1ahtQAe-x9jknDiD20fT6uKAZMgjLnNUFkkWYpvhXO_uOVQ@mail.gmail.com>","date":"2025-01-21T12:52:28","subject":"Re: [PATCH 1/8] libcamera: camera: Introduce metadataAvailable\n\tsignal","submitter":{"id":117,"url":"https://patchwork.libcamera.org/api/people/117/","name":"Cheng-Hao Yang","email":"chenghaoyang@chromium.org"},"content":"Hi Barnabás,\n\nOn Tue, Jan 21, 2025 at 11:32 AM Barnabás Pőcze <pobrn@protonmail.com> wrote:\n>\n> Hi\n>\n>\n> 2025. január 3., péntek 18:01 keltezéssel, Jacopo Mondi <jacopo.mondi@ideasonboard.com> írta:\n>\n> > Hi Harvey\n> >\n> > On Thu, Jan 02, 2025 at 05:13:28PM +0100, Cheng-Hao Yang wrote:\n> > > Hi Jacopo,\n> > >\n> > > Thank you for the patch.\n> > >\n> > > On Fri, Dec 6, 2024 at 5:07 PM Jacopo Mondi\n> > > <jacopo.mondi@ideasonboard.com> wrote:\n> > > >\n> > > > Add a new signal to the Camera class that allows applications to\n> > > > receive notifications for early completion of metadata results.\n> > > >\n> > > > To avoid expensive copies of the metadata results the signal\n> > > > transports the ids of the metadata keys that are ready. Applications\n> > > > can use these ids to access the metadata results from\n> > > > Request::metadata().\n> > >\n> > > Yes, it avoids expensive copies, while I think we should restrict\n> > > usages of `Request->metadata()` both in the application and pipeline\n> > > handlers as well:\n> > > 1. They should both access the metadata ControlList in the\n> > > CameraManager's thread, which is the pipeline handlers' owner thread,\n> > > to avoid race conditions. (I think `std::unordered_map` of\n> > > `ControlList::controls_` is not thread-safe, as a set of the map might\n> > > lead to rehashing. Please correct me if I am wrong.)\n> >\n> > I think you're right, nice catch. Signals across threads are\n> > asynchronous and there's a window indeed where the map can be accessed\n> > by the app and the pipeline handler concurrently. Before this series\n> > this was not an issue because apps only get the metadata list at\n> > request completion time when the pipeline handler is done (or should\n> > be done).\n>\n> Based on my understanding, signals are delivered synchronously unless the target\n> object derives from `libcamera::Object`. So application callbacks will be invoked\n> directly from the CameraManager's thread (or whichever internal thread is used).\n> As far as I am aware, inheriting `libcamera::Object` in application code is not\n> supported and it just would not work currently because `libcamera::Thread` is not\n> public, so the application would have no way of running the internal event loop.\n\nIn Android adapter though, it can directly use `libcamera::Thread` on\nCameraStream:\nhttps://git.libcamera.org/libcamera/libcamera.git/tree/src/android/camera_stream.h#n17\n\nAlso, other applications can use std::thread and cause similar issues,\nIIUC. It doesn't need to be `libcamera::Thread`, right?\n\n>\n>\n> >\n> > I think your concern is also supported by the following part of\n> > https://en.cppreference.com/w/cpp/container/unordered_map/operator_at\n> >\n> > -------------------------------------------------------------------------------\n> > If after the operation the new number of elements is greater than old\n> > max_load_factor() * bucket_count() a rehashing takes place.  If\n> > rehashing occurs (due to the insertion), all iterators are\n> > invalidated. Otherwise (no rehashing), iterators are not invalidated.\n> > -------------------------------------------------------------------------------\n> >\n> > > 2. Pipeline handlers should not directly modify `Request->metadata()`,\n> > > to avoid overwriting. Instead, they should utilize the helper\n> > > functions `metadataAvailable` in the third patch in this series.\n> > >\n> > > I've seen (2.) has been implemented in the following patches, thanks.\n> > > Do you think we can ensure (1.) though? Maybe at least documenting it?\n> >\n> > I'm afraid documenting it is not enough, access to the metadata list\n> > should be locked if the list is made available to the application before\n> > the request has completed...\n> >\n> > I initially thought this wouldn't be hard. After all, in this series\n> > pipeline handlers can access Request::metadata_ in rw mode only\n> > through PipelineHandler::metadataAvailable() we could simply lock\n> > there and lock Request::metadata().\n> >\n> > However ControlList provides const iterators and an application might\n> > decide to iterate the control list while the pipeline handler\n> > populates it with more metadata.\n>\n> Since the signal is dispatched synchronously, I think that is not possible*, so\n> the current setup might be acceptable. (Or am I missing something?) The burden of\n> synchronization falls fully on the user, but considering that the callback runs\n> in an internal thread, the user code would probably have to do some kind of\n> synchronization in any case.\n\nThe user does have the responsibility to keep the synchronization,\nwhile as I mentioned above, pipeline handler also needs to avoid\nmodifying metadata ControlList in other threads, right?\n\n>\n> [*]: Unless the user code saves iterators or references to the metadata control\n> list and reuses them after the signal handler returns in some other thread before\n> the request completes. But I am wondering if that is a significant concern?\n>\n> I suppose the parametrization of the signal could be changed to discourage the\n> direct use of `Request`. Although things like the request cookie would still\n> need to be available in some way.\n>\n> Another thing, libdbus \"locks\" messages when they are submitted, maybe a\n> similar idea could be implemented here as well with `Request`.\n\nIn this case though, metadata would be updated and sent multiple\ntimes. This series of patches aims to reduce duplicating data, so it\nuses the same map. Does that mean we need to lock the whole map\nwhenever accessing and modifying it?\n\nBR,\nHarvey\n\n>\n>\n> Regards,\n> Barnabás Pőcze\n>\n> >\n> > I wonder what are the consequences of increasing\n> > https://en.cppreference.com/w/cpp/container/unordered_map/max_load_factor\n> > to avoid rehashing...\n> >\n> > I'll ask around!\n> >\n> > Thanks!\n> >   j\n> >\n> > >\n> > > BR,\n> > > Harvey\n> > >\n> > > >\n> > > > The signal is an opt-in feature for applications and the sum of\n> > > > all metadata results notified through this signal is available\n> > > > in Request::metadata() at request completion time.\n> > > >\n> > > > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>\n> > > > ---\n> > > >  include/libcamera/camera.h |  2 ++\n> > > >  src/libcamera/camera.cpp   | 42 ++++++++++++++++++++++++++++++++++++++\n> > > >  2 files changed, 44 insertions(+)\n> > > >\n> > > > diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h\n> > > > index 94cee7bd86bb..20d51c191ecd 100644\n> > > > --- a/include/libcamera/camera.h\n> > > > +++ b/include/libcamera/camera.h\n> > > > @@ -13,6 +13,7 @@\n> > > >  #include <set>\n> > > >  #include <stdint.h>\n> > > >  #include <string>\n> > > > +#include <unordered_set>\n> > > >\n> > > >  #include <libcamera/base/class.h>\n> > > >  #include <libcamera/base/flags.h>\n> > > > @@ -122,6 +123,7 @@ public:\n> > > >\n> > > >         const std::string &id() const;\n> > > >\n> > > > +       Signal<Request *, std::unordered_set<const ControlId *>> metadataAvailable;\n> > > >         Signal<Request *, FrameBuffer *> bufferCompleted;\n> > > >         Signal<Request *> requestCompleted;\n> > > >         Signal<> disconnected;\n> > > > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> > > > index 4c865a46af53..63e78b24e271 100644\n> > > > --- a/src/libcamera/camera.cpp\n> > > > +++ b/src/libcamera/camera.cpp\n> > > > @@ -886,6 +886,48 @@ const std::string &Camera::id() const\n> > > >         return _d()->id_;\n> > > >  }\n> > > >\n> > > > +/**\n> > > > + * \\var Camera::metadataAvailable\n> > > > + * \\brief Signal emitted when metadata for a request are available\n> > > > + *\n> > > > + * The metadataAvailable signal notifies applications about the availability\n> > > > + * of metadata for a request before the request completes.\n> > > > + *\n> > > > + * As metadata results could be large in size, the signal transports the ids\n> > > > + * of the metadata that have just been made available, but the actual control\n> > > > + * values are stored in the Camera::metadata() list.\n> > > > + *\n> > > > + * Applications can access the value of the newly available metadata results\n> > > > + * with:\n> > > > + *\n> > > > + * \\code\n> > > > +\n> > > > +       void metadataAvailableHandler(Request *request,\n> > > > +                                     std::unordered_set<const ControlId *> ids)\n> > > > +       {\n> > > > +               const ControlList &metadata = request->metadata();\n> > > > +\n> > > > +               for (const auto id : ids) {\n> > > > +                       ControlValue &value = metadata.get(id->id());\n> > > > +\n> > > > +                       ....\n> > > > +               }\n> > > > +       }\n> > > > +   \\endcode\n> > > > + *\n> > > > + * This signal is emitted multiple times for the same request, it is in facts\n> > > > + * emitted by the framework every time a new metadata list is made available\n> > > > + * by the Camera to the application.\n> > > > + *\n> > > > + * The sum of all metadata lists reported through this signal is equal to\n> > > > + * Request::metadata() list when the Request completes.\n> > > > + *\n> > > > + * Application can opt-in to handle this signal to receive fast notifications\n> > > > + * of metadata availability or can equally access the full metadata list\n> > > > + * at Request complete time through Request::metadata() if they have no interest\n> > > > + * in early metadata notification.\n> > > > + */\n> > > > +\n> > > >  /**\n> > > >   * \\var Camera::bufferCompleted\n> > > >   * \\brief Signal emitted when a buffer for a request queued to the camera has\n> > > > --\n> > > > 2.47.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 A6152BD160\n\tfor <parsemail@patchwork.libcamera.org>;\n\tTue, 21 Jan 2025 12:52:43 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5C10A6851D;\n\tTue, 21 Jan 2025 13:52:42 +0100 (CET)","from mail-lj1-x230.google.com (mail-lj1-x230.google.com\n\t[IPv6:2a00:1450:4864:20::230])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id A836E60380\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tTue, 21 Jan 2025 13:52:40 +0100 (CET)","by mail-lj1-x230.google.com with SMTP id\n\t38308e7fff4ca-3061513d353so50538501fa.2\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tTue, 21 Jan 2025 04:52:40 -0800 (PST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"R36sAkyh\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=chromium.org; s=google; t=1737463960; x=1738068760;\n\tdarn=lists.libcamera.org; \n\th=content-transfer-encoding:cc:to:subject:message-id:date:from\n\t:in-reply-to:references:mime-version:from:to:cc:subject:date\n\t:message-id:reply-to;\n\tbh=15c0A2vJF+adVRYrRzH3+oCDTUi+iWJRamfoUIb1ofg=;\n\tb=R36sAkyh0CwzvqjgjlULKQ1Qe6fHFVSn8t7T0jb5d4JQ8FMbPUGQiTrOamTRkUs7A+\n\tlN27SxlUCOYcpw2x6swE8y0natlfuGIxGoQq1tuMk7Kq7bsZC4jcyYnqWHmy0QtVMTZf\n\tvudrYZlwaMV7OTqQD29UoGqztM4MtsHwGkwlI=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1737463960; x=1738068760;\n\th=content-transfer-encoding:cc:to:subject:message-id:date:from\n\t:in-reply-to:references:mime-version:x-gm-message-state:from:to:cc\n\t:subject:date:message-id:reply-to;\n\tbh=15c0A2vJF+adVRYrRzH3+oCDTUi+iWJRamfoUIb1ofg=;\n\tb=XmF5MFAlG9Cd2VYoBDpELVS0RHMHRVF59YMYcpYrhzwTyNCR0Wg4gCHagQmm0ePQQW\n\t59oUc4/L6LL33FT5yVfXOs6gbQHg37Rs4dew03mVIomU6lQHx7MWSOxqKamU4/MJXqxy\n\trsAVis0t3Q6zTNj8t5a0Aln94tYZPYDxiaS8WYXz1FYtLlGWRtVPLVtpUYBSH3QGYCQp\n\tvS4IujhEzsPYu5P5pmEXExQRQuq+1P+FE5lS6F2GEFXlO37Wwdh6YSZXcBgAz4I9ZJgW\n\trmSJHJlTFKzjRT2rrxUYxzbZa89z+mp0SBcSjROWf02gWcMe7/WT8NHmiZAHHSNjyTBp\n\tlGUA==","X-Forwarded-Encrypted":"i=1;\n\tAJvYcCUBVDsKJMMcSE9TNjuyqA2fQMfZceqI4jFH7uX0YYnKGlEBqp6beUe4JrkIiZpvID2lGgu/0OA+AEbmbobJDuE=@lists.libcamera.org","X-Gm-Message-State":"AOJu0YxEnZ0nd4YHxrih3wnz4prxgEI4jQ1lTP1WIxxVw9CRS6SL37/r\n\tVc/2ut35ydueN38di9G9gsOn5QeOtvQ5Ghque+YFoS2Bwp1XUvhnCj23QS6mZA3iT0d0tjeEzsM\n\tTJgbEjlKPcAB3O/2yYflwP4BNXD53z4Ew0naF","X-Gm-Gg":"ASbGnctJJeV03U1+vcSYE/lgUxftwqie8IRV6E6qo1ByV9FbUWHuKXoLofWeFJi5LUe\n\tS3vOGFuYX2Qc7pcSJrrw2N/8rIIn8ZHPO7lBqEn1AuZ/1DwzagA==","X-Google-Smtp-Source":"AGHT+IHGdiE57x20V87Q63fWtGIlCyx3bByFs6XYLQdnIyAKnHpnKQJBz3o1mg/d77hyfem3lLS8orHuh3xENGHt3Ic=","X-Received":"by 2002:a05:651c:198a:b0:302:3534:1d68 with SMTP id\n\t38308e7fff4ca-3072ca60ec8mr67680481fa.4.1737463959287;\n\tTue, 21 Jan 2025 04:52:39 -0800 (PST)","MIME-Version":"1.0","References":"<20241206160747.97176-1-jacopo.mondi@ideasonboard.com>\n\t<20241206160747.97176-2-jacopo.mondi@ideasonboard.com>\n\t<CAEB1ahuMRGbz_h73hbbSnTUW-qSnCp5BtvT89hmRZ+p2i=EvkQ@mail.gmail.com>\n\t<gjt7q4xa65uvlcsm4rhtcvwzatafwecgvqqd6alf3eqwacness@p3s7qzcmm3li>\n\t<RqeiGJtrHv3e7oo6v5iQT1R-jH8BxuP6DffK25MzVBT0Lo6N6PK-isOJ1g77V7KIePv7ewtsN_Xqjc_uRSyCzbIsLES7YMkHX6uZFIuCgGk=@protonmail.com>","In-Reply-To":"<RqeiGJtrHv3e7oo6v5iQT1R-jH8BxuP6DffK25MzVBT0Lo6N6PK-isOJ1g77V7KIePv7ewtsN_Xqjc_uRSyCzbIsLES7YMkHX6uZFIuCgGk=@protonmail.com>","From":"Cheng-Hao Yang <chenghaoyang@chromium.org>","Date":"Tue, 21 Jan 2025 13:52:28 +0100","X-Gm-Features":"AbW1kvaXzFzYlIFR3MJa-LLKdHGTQLppHF6E77WsRyhytDLUk6NQdaDBmVMrX3g","Message-ID":"<CAEB1ahtQAe-x9jknDiD20fT6uKAZMgjLnNUFkkWYpvhXO_uOVQ@mail.gmail.com>","Subject":"Re: [PATCH 1/8] libcamera: camera: Introduce metadataAvailable\n\tsignal","To":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= <pobrn@protonmail.com>","Cc":"Jacopo Mondi <jacopo.mondi@ideasonboard.com>,\n\tlibcamera-devel@lists.libcamera.org, \n\tHan-Lin Chen <hanlinchen@chromium.org>","Content-Type":"text/plain; charset=\"UTF-8\"","Content-Transfer-Encoding":"quoted-printable","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":33120,"web_url":"https://patchwork.libcamera.org/comment/33120/","msgid":"<tK5SevVVE46IAUjK9HC519boN8ZGWIbEKyF_Gud-1hevD1sLieH0C3nwftg_AeEGFHZMzTlSOQm6hNbkAKQVPQkZ_zu-C9C5HZBExHhIzYM=@protonmail.com>","date":"2025-01-21T13:25:56","subject":"Re: [PATCH 1/8] libcamera: camera: Introduce metadataAvailable\n\tsignal","submitter":{"id":133,"url":"https://patchwork.libcamera.org/api/people/133/","name":"Pőcze Barnabás","email":"pobrn@protonmail.com"},"content":"2025. január 21., kedd 13:52 keltezéssel, Cheng-Hao Yang <chenghaoyang@chromium.org> írta:\n\n> Hi Barnabás,\n> \n> On Tue, Jan 21, 2025 at 11:32 AM Barnabás Pőcze <pobrn@protonmail.com> wrote:\n> >\n> > Hi\n> >\n> >\n> > 2025. január 3., péntek 18:01 keltezéssel, Jacopo Mondi <jacopo.mondi@ideasonboard.com> írta:\n> >\n> > > Hi Harvey\n> > >\n> > > On Thu, Jan 02, 2025 at 05:13:28PM +0100, Cheng-Hao Yang wrote:\n> > > > Hi Jacopo,\n> > > >\n> > > > Thank you for the patch.\n> > > >\n> > > > On Fri, Dec 6, 2024 at 5:07 PM Jacopo Mondi\n> > > > <jacopo.mondi@ideasonboard.com> wrote:\n> > > > >\n> > > > > Add a new signal to the Camera class that allows applications to\n> > > > > receive notifications for early completion of metadata results.\n> > > > >\n> > > > > To avoid expensive copies of the metadata results the signal\n> > > > > transports the ids of the metadata keys that are ready. Applications\n> > > > > can use these ids to access the metadata results from\n> > > > > Request::metadata().\n> > > >\n> > > > Yes, it avoids expensive copies, while I think we should restrict\n> > > > usages of `Request->metadata()` both in the application and pipeline\n> > > > handlers as well:\n> > > > 1. They should both access the metadata ControlList in the\n> > > > CameraManager's thread, which is the pipeline handlers' owner thread,\n> > > > to avoid race conditions. (I think `std::unordered_map` of\n> > > > `ControlList::controls_` is not thread-safe, as a set of the map might\n> > > > lead to rehashing. Please correct me if I am wrong.)\n> > >\n> > > I think you're right, nice catch. Signals across threads are\n> > > asynchronous and there's a window indeed where the map can be accessed\n> > > by the app and the pipeline handler concurrently. Before this series\n> > > this was not an issue because apps only get the metadata list at\n> > > request completion time when the pipeline handler is done (or should\n> > > be done).\n> >\n> > Based on my understanding, signals are delivered synchronously unless the target\n> > object derives from `libcamera::Object`. So application callbacks will be invoked\n> > directly from the CameraManager's thread (or whichever internal thread is used).\n> > As far as I am aware, inheriting `libcamera::Object` in application code is not\n> > supported and it just would not work currently because `libcamera::Thread` is not\n> > public, so the application would have no way of running the internal event loop.\n> \n> In Android adapter though, it can directly use `libcamera::Thread` on\n> CameraStream:\n> https://git.libcamera.org/libcamera/libcamera.git/tree/src/android/camera_stream.h#n17\n> \n> Also, other applications can use std::thread and cause similar issues,\n> IIUC. It doesn't need to be `libcamera::Thread`, right?\n> \n> >\n> >\n> > >\n> > > I think your concern is also supported by the following part of\n> > > https://en.cppreference.com/w/cpp/container/unordered_map/operator_at\n> > >\n> > > -------------------------------------------------------------------------------\n> > > If after the operation the new number of elements is greater than old\n> > > max_load_factor() * bucket_count() a rehashing takes place.  If\n> > > rehashing occurs (due to the insertion), all iterators are\n> > > invalidated. Otherwise (no rehashing), iterators are not invalidated.\n> > > -------------------------------------------------------------------------------\n> > >\n> > > > 2. Pipeline handlers should not directly modify `Request->metadata()`,\n> > > > to avoid overwriting. Instead, they should utilize the helper\n> > > > functions `metadataAvailable` in the third patch in this series.\n> > > >\n> > > > I've seen (2.) has been implemented in the following patches, thanks.\n> > > > Do you think we can ensure (1.) though? Maybe at least documenting it?\n> > >\n> > > I'm afraid documenting it is not enough, access to the metadata list\n> > > should be locked if the list is made available to the application before\n> > > the request has completed...\n> > >\n> > > I initially thought this wouldn't be hard. After all, in this series\n> > > pipeline handlers can access Request::metadata_ in rw mode only\n> > > through PipelineHandler::metadataAvailable() we could simply lock\n> > > there and lock Request::metadata().\n> > >\n> > > However ControlList provides const iterators and an application might\n> > > decide to iterate the control list while the pipeline handler\n> > > populates it with more metadata.\n> >\n> > Since the signal is dispatched synchronously, I think that is not possible*, so\n> > the current setup might be acceptable. (Or am I missing something?) The burden of\n> > synchronization falls fully on the user, but considering that the callback runs\n> > in an internal thread, the user code would probably have to do some kind of\n> > synchronization in any case.\n> \n> The user does have the responsibility to keep the synchronization,\n> while as I mentioned above, pipeline handler also needs to avoid\n> modifying metadata ControlList in other threads, right?\n> \n> >\n> > [*]: Unless the user code saves iterators or references to the metadata control\n> > list and reuses them after the signal handler returns in some other thread before\n> > the request completes. But I am wondering if that is a significant concern?\n> >\n> > I suppose the parametrization of the signal could be changed to discourage the\n> > direct use of `Request`. Although things like the request cookie would still\n> > need to be available in some way.\n> >\n> > Another thing, libdbus \"locks\" messages when they are submitted, maybe a\n> > similar idea could be implemented here as well with `Request`.\n> \n> In this case though, metadata would be updated and sent multiple\n> times. This series of patches aims to reduce duplicating data, so it\n> uses the same map. Does that mean we need to lock the whole map\n> whenever accessing and modifying it?\n\nWell, the \"locking\" comment was just a tangential idea, as a potential way to\nminimize the misuse of a `Request` object that is in-flight.\n\nAs far as I understand, the way this patch series currently is, there is no need for\nlocking or extra synchronization when accessing the metadata inside the `metadataAvailable`\nsignal handler. However, as soon as one wants to access the metadata `ControlValue`s outside\nthe signal handler, then copies of the `ControlValue`s must be made (inside the signal handler).\nUnfortunately, as far as I am aware, at the moment there is no concrete plan yet for addressing\nthis shortcoming that could be implemented at once.\n\n\nRegards,\nBarnabás Pőcze\n\n> \n> BR,\n> Harvey\n> \n> >\n> >\n> > Regards,\n> > Barnabás Pőcze\n> >\n> > >\n> > > I wonder what are the consequences of increasing\n> > > https://en.cppreference.com/w/cpp/container/unordered_map/max_load_factor\n> > > to avoid rehashing...\n> > >\n> > > I'll ask around!\n> > >\n> > > Thanks!\n> > >   j\n> > >\n> > > >\n> > > > BR,\n> > > > Harvey\n> > > >\n> > > > >\n> > > > > The signal is an opt-in feature for applications and the sum of\n> > > > > all metadata results notified through this signal is available\n> > > > > in Request::metadata() at request completion time.\n> > > > >\n> > > > > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>\n> > > > > ---\n> > > > >  include/libcamera/camera.h |  2 ++\n> > > > >  src/libcamera/camera.cpp   | 42 ++++++++++++++++++++++++++++++++++++++\n> > > > >  2 files changed, 44 insertions(+)\n> > > > >\n> > > > > diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h\n> > > > > index 94cee7bd86bb..20d51c191ecd 100644\n> > > > > --- a/include/libcamera/camera.h\n> > > > > +++ b/include/libcamera/camera.h\n> > > > > @@ -13,6 +13,7 @@\n> > > > >  #include <set>\n> > > > >  #include <stdint.h>\n> > > > >  #include <string>\n> > > > > +#include <unordered_set>\n> > > > >\n> > > > >  #include <libcamera/base/class.h>\n> > > > >  #include <libcamera/base/flags.h>\n> > > > > @@ -122,6 +123,7 @@ public:\n> > > > >\n> > > > >         const std::string &id() const;\n> > > > >\n> > > > > +       Signal<Request *, std::unordered_set<const ControlId *>> metadataAvailable;\n> > > > >         Signal<Request *, FrameBuffer *> bufferCompleted;\n> > > > >         Signal<Request *> requestCompleted;\n> > > > >         Signal<> disconnected;\n> > > > > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> > > > > index 4c865a46af53..63e78b24e271 100644\n> > > > > --- a/src/libcamera/camera.cpp\n> > > > > +++ b/src/libcamera/camera.cpp\n> > > > > @@ -886,6 +886,48 @@ const std::string &Camera::id() const\n> > > > >         return _d()->id_;\n> > > > >  }\n> > > > >\n> > > > > +/**\n> > > > > + * \\var Camera::metadataAvailable\n> > > > > + * \\brief Signal emitted when metadata for a request are available\n> > > > > + *\n> > > > > + * The metadataAvailable signal notifies applications about the availability\n> > > > > + * of metadata for a request before the request completes.\n> > > > > + *\n> > > > > + * As metadata results could be large in size, the signal transports the ids\n> > > > > + * of the metadata that have just been made available, but the actual control\n> > > > > + * values are stored in the Camera::metadata() list.\n> > > > > + *\n> > > > > + * Applications can access the value of the newly available metadata results\n> > > > > + * with:\n> > > > > + *\n> > > > > + * \\code\n> > > > > +\n> > > > > +       void metadataAvailableHandler(Request *request,\n> > > > > +                                     std::unordered_set<const ControlId *> ids)\n> > > > > +       {\n> > > > > +               const ControlList &metadata = request->metadata();\n> > > > > +\n> > > > > +               for (const auto id : ids) {\n> > > > > +                       ControlValue &value = metadata.get(id->id());\n> > > > > +\n> > > > > +                       ....\n> > > > > +               }\n> > > > > +       }\n> > > > > +   \\endcode\n> > > > > + *\n> > > > > + * This signal is emitted multiple times for the same request, it is in facts\n> > > > > + * emitted by the framework every time a new metadata list is made available\n> > > > > + * by the Camera to the application.\n> > > > > + *\n> > > > > + * The sum of all metadata lists reported through this signal is equal to\n> > > > > + * Request::metadata() list when the Request completes.\n> > > > > + *\n> > > > > + * Application can opt-in to handle this signal to receive fast notifications\n> > > > > + * of metadata availability or can equally access the full metadata list\n> > > > > + * at Request complete time through Request::metadata() if they have no interest\n> > > > > + * in early metadata notification.\n> > > > > + */\n> > > > > +\n> > > > >  /**\n> > > > >   * \\var Camera::bufferCompleted\n> > > > >   * \\brief Signal emitted when a buffer for a request queued to the camera has\n> > > > > --\n> > > > > 2.47.1\n> > > > >\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 E42B8BD16B\n\tfor <parsemail@patchwork.libcamera.org>;\n\tTue, 21 Jan 2025 13:26:06 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D23AF68543;\n\tTue, 21 Jan 2025 14:26:05 +0100 (CET)","from mail-40131.protonmail.ch (mail-40131.protonmail.ch\n\t[185.70.40.131])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 2158068503\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tTue, 21 Jan 2025 14:26:04 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=protonmail.com header.i=@protonmail.com\n\theader.b=\"U5szcAXj\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=protonmail.com;\n\ts=protonmail3; t=1737465963; x=1737725163;\n\tbh=GOtpatHrzEPRIyaD66bPlnNJ87fqhtb7PA0CtSj9lIc=;\n\th=Date:To:From:Cc:Subject:Message-ID:In-Reply-To:References:\n\tFeedback-ID:From:To:Cc:Date:Subject:Reply-To:Feedback-ID:\n\tMessage-ID:BIMI-Selector:List-Unsubscribe:List-Unsubscribe-Post;\n\tb=U5szcAXj5lElJfAU36UIi7DbSeUaikKqAOz9B2fWHsg/widnw5xU7oMBlg/8icdso\n\tupfHAob8Kfdvz9ZPQPDv/hiaQ3BmVw4kA8qNlq18qQFe+md+KoQjwQ2PGzEl3eQRYj\n\tsbW/EBJG+5ZYYaco+uJxFbwMnHCk89yb+o/tdedwrBAhtlkAjRIeV1l3F6nz/PT2Og\n\tlsy3ZVPrij6ABO/muE0linOVjK/QYoFoct5ABwqswcUN5jWAVMK4lrdgXYBO7pLsYR\n\tbDX4YK7X6IKVMAmSCmQgGfMtQWnqvW7SFvd4qX53cArYeuNIRNNuNh8NMDOLDrTaiZ\n\tqJvKDlbuK3QrQ==","Date":"Tue, 21 Jan 2025 13:25:56 +0000","To":"Cheng-Hao Yang <chenghaoyang@chromium.org>","From":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= <pobrn@protonmail.com>","Cc":"Jacopo Mondi <jacopo.mondi@ideasonboard.com>,\n\tlibcamera-devel@lists.libcamera.org,\n\tHan-Lin Chen <hanlinchen@chromium.org>","Subject":"Re: [PATCH 1/8] libcamera: camera: Introduce metadataAvailable\n\tsignal","Message-ID":"<tK5SevVVE46IAUjK9HC519boN8ZGWIbEKyF_Gud-1hevD1sLieH0C3nwftg_AeEGFHZMzTlSOQm6hNbkAKQVPQkZ_zu-C9C5HZBExHhIzYM=@protonmail.com>","In-Reply-To":"<CAEB1ahtQAe-x9jknDiD20fT6uKAZMgjLnNUFkkWYpvhXO_uOVQ@mail.gmail.com>","References":"<20241206160747.97176-1-jacopo.mondi@ideasonboard.com>\n\t<20241206160747.97176-2-jacopo.mondi@ideasonboard.com>\n\t<CAEB1ahuMRGbz_h73hbbSnTUW-qSnCp5BtvT89hmRZ+p2i=EvkQ@mail.gmail.com>\n\t<gjt7q4xa65uvlcsm4rhtcvwzatafwecgvqqd6alf3eqwacness@p3s7qzcmm3li>\n\t<RqeiGJtrHv3e7oo6v5iQT1R-jH8BxuP6DffK25MzVBT0Lo6N6PK-isOJ1g77V7KIePv7ewtsN_Xqjc_uRSyCzbIsLES7YMkHX6uZFIuCgGk=@protonmail.com>\n\t<CAEB1ahtQAe-x9jknDiD20fT6uKAZMgjLnNUFkkWYpvhXO_uOVQ@mail.gmail.com>","Feedback-ID":"20568564:user:proton","X-Pm-Message-ID":"321168ad36406cae071127683c2d0d85500687f7","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Transfer-Encoding":"quoted-printable","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>"}}]