[{"id":38548,"web_url":"https://patchwork.libcamera.org/comment/38548/","msgid":"<85h5pl9vs2.fsf@mzamazal-thinkpadp1gen7.tpbc.csb>","date":"2026-04-08T18:01:01","subject":"Re: [PATCH v2 02/42] libcamera: request: Move all private member\n\tvariables to Private class","submitter":{"id":177,"url":"https://patchwork.libcamera.org/api/people/177/","name":"Milan Zamazal","email":"mzamazal@redhat.com"},"content":"Laurent Pinchart writes:\n\n> The Request class has a set of private member variables, with some of\n> them stored in the Request class itself, and some in the\n> Request::Private class. Storing the variables in the Request class\n> itself has the advantage that accessors can be inline, at the cost of\n> ABI breakage if variables need to be added, removed or otherwise\n> modified.\n>\n> The controls_ and metadata_ variables have recently been turned from\n> pointers to instances. This broke the ABI. To avoid further breakages,\n> move all remaining private member variables to Request::Private. The\n> performance impact of not inlining accessors will be negligible.\n>\n> While at it, drop an unneeded class forward declaration.\n>\n> Signed-off-by: Laurent Pinchart \n\nReviewed-by: Milan Zamazal \n\n> ---\n> include/libcamera/internal/request.h | 12 +++-\n> include/libcamera/request.h | 15 ++--\n> src/libcamera/request.cpp | 104 +++++++++++++++------------\n> 3 files changed, 70 insertions(+), 61 deletions(-)\n>\n> diff --git a/include/libcamera/internal/request.h b/include/libcamera/internal/request.h\n> index 693097ee9a26..7715077b3f7c 100644\n> --- a/include/libcamera/internal/request.h\n> +++ b/include/libcamera/internal/request.h\n> @@ -30,7 +30,7 @@ class Request::Private : public Extensible::Private\n> \tLIBCAMERA_DECLARE_PUBLIC(Request)\n> \n> public:\n> -\tPrivate(Camera *camera);\n> +\tPrivate(Camera *camera, uint64_t cookie);\n> \t~Private();\n> \n> \tCamera *camera() const { return camera_; }\n> @@ -41,7 +41,7 @@ public:\n> \tbool completeBuffer(FrameBuffer *buffer);\n> \tvoid complete();\n> \tvoid cancel();\n> -\tvoid reset();\n> +\tvoid reset(Request::ReuseFlag flags);\n> \n> \tvoid prepare(std::chrono::milliseconds timeout = 0ms);\n> \tSignal<> prepared;\n> @@ -56,14 +56,20 @@ private:\n> \tvoid timeout();\n> \n> \tCamera *camera_;\n> +\tconst uint64_t cookie_;\n> +\n> +\tStatus status_;\n> \tbool cancelled_;\n> \tuint32_t sequence_ = 0;\n> \tbool prepared_ = false;\n> \n> +\tControlList controls_;\n> +\tControlList metadata_;\n> +\tBufferMap bufferMap_;\n> +\n> \tstd::unordered_set pending_;\n> \tstd::map notifiers_;\n> \tstd::unique_ptr timer_;\n> -\tControlList metadata_;\n> };\n> \n> } /* namespace libcamera */\n> diff --git a/include/libcamera/request.h b/include/libcamera/request.h\n> index 290983f61352..08ac6e8daba7 100644\n> --- a/include/libcamera/request.h\n> +++ b/include/libcamera/request.h\n> @@ -22,7 +22,6 @@\n> namespace libcamera {\n> \n> class Camera;\n> -class CameraControlValidator;\n> class FrameBuffer;\n> class Stream;\n> \n> @@ -49,16 +48,16 @@ public:\n> \n> \tvoid reuse(ReuseFlag flags = Default);\n> \n> -\tControlList &controls() { return controls_; }\n> +\tControlList &controls();\n> \tconst ControlList &metadata() const;\n> -\tconst BufferMap &buffers() const { return bufferMap_; }\n> +\tconst BufferMap &buffers() const;\n> \tint addBuffer(const Stream *stream, FrameBuffer *buffer,\n> \t\t std::unique_ptr &&fence = {});\n> \tFrameBuffer *findBuffer(const Stream *stream) const;\n> \n> \tuint32_t sequence() const;\n> -\tuint64_t cookie() const { return cookie_; }\n> -\tStatus status() const { return status_; }\n> +\tuint64_t cookie() const;\n> +\tStatus status() const;\n> \n> \tbool hasPendingBuffers() const;\n> \n> @@ -66,12 +65,6 @@ public:\n> \n> private:\n> \tLIBCAMERA_DISABLE_COPY(Request)\n> -\n> -\tControlList controls_;\n> -\tBufferMap bufferMap_;\n> -\n> -\tconst uint64_t cookie_;\n> -\tStatus status_;\n> };\n> \n> std::ostream &operator<<(std::ostream &out, const Request &r);\n> diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp\n> index b50a64205283..719e27aac763 100644\n> --- a/src/libcamera/request.cpp\n> +++ b/src/libcamera/request.cpp\n> @@ -52,12 +52,16 @@ LOG_DEFINE_CATEGORY(Request)\n> \n> /**\n> * \\brief Create a Request::Private\n> - * \\param camera The Camera that creates the request\n> + * \\param[in] camera The Camera that creates the request\n> + * \\param[in] cookie Opaque cookie for application use\n> *\n> * \\todo Add a validator for metadata controls.\n> */\n> -Request::Private::Private(Camera *camera)\n> -\t: camera_(camera), cancelled_(false), metadata_(controls::controls)\n> +Request::Private::Private(Camera *camera, uint64_t cookie)\n> +\t: camera_(camera), cookie_(cookie), status_(RequestPending),\n> +\t cancelled_(false),\n> +\t controls_(camera->controls(), camera->_d()->validator()),\n> +\t metadata_(controls::controls)\n> {\n> }\n> \n> @@ -132,7 +136,7 @@ void Request::Private::complete()\n> \tASSERT(request->status() == RequestPending);\n> \tASSERT(!hasPendingBuffers());\n> \n> -\trequest->status_ = cancelled_ ? RequestCancelled : RequestComplete;\n> +\tstatus_ = cancelled_ ? RequestCancelled : RequestComplete;\n> \n> \tLOG(Request, Debug) << request->toString();\n> \n> @@ -174,18 +178,37 @@ void Request::Private::cancel()\n> \n> /**\n> * \\brief Reset the request internal data to default values\n> + * \\param[in] flags Indicate whether or not to reuse the buffers\n> *\n> * After calling this function, all request internal data will have default\n> - * values as if the Request::Private instance had just been constructed.\n> + * values as if the Request::Private instance had just been constructed, with\n> + * the exception of bufferMap_ if the Request::ReuseFlag::ReuseBuffers flag is\n> + * set in \\a flags.\n> */\n> -void Request::Private::reset()\n> +void Request::Private::reset(Request::ReuseFlag flags)\n> {\n> -\tsequence_ = 0;\n> +\tstatus_ = RequestPending;\n> \tcancelled_ = false;\n> +\tsequence_ = 0;\n> \tprepared_ = false;\n> +\n> +\tcontrols_.clear();\n> +\tmetadata_.clear();\n> +\n> \tpending_.clear();\n> \tnotifiers_.clear();\n> \ttimer_.reset();\n> +\n> +\tif (flags & ReuseBuffers) {\n> +\t\tRequest *request = _o();\n> +\n> +\t\tfor (const auto &[stream, buffer] : bufferMap_) {\n> +\t\t\tbuffer->_d()->setRequest(request);\n> +\t\t\tpending_.insert(buffer);\n> +\t\t}\n> +\t} else {\n> +\t\tbufferMap_.clear();\n> +\t}\n> }\n> \n> /*\n> @@ -286,9 +309,8 @@ void Request::Private::notifierActivated(FrameBuffer *buffer)\n> \tASSERT(it != notifiers_.end());\n> \tnotifiers_.erase(it);\n> \n> -\tRequest *request = _o();\n> \tLOG(Request, Debug)\n> -\t\t<< \"Request \" << request->cookie() << \" buffer \" << buffer\n> +\t\t<< \"Request \" << cookie_ << \" buffer \" << buffer\n> \t\t<< \" fence signalled\";\n> \n> \tif (!notifiers_.empty())\n> @@ -305,8 +327,7 @@ void Request::Private::timeout()\n> \tASSERT(!notifiers_.empty());\n> \tnotifiers_.clear();\n> \n> -\tRequest *request = _o();\n> -\tLOG(Request, Debug) << \"Request prepare timeout: \" << request->cookie();\n> +\tLOG(Request, Debug) << \"Request prepare timeout: \" << cookie_;\n> \n> \tcancel();\n> \n> @@ -361,13 +382,11 @@ void Request::Private::timeout()\n> * completely opaque to libcamera.\n> */\n> Request::Request(Camera *camera, uint64_t cookie)\n> -\t: Extensible(std::make_unique(camera)),\n> -\t controls_(camera->controls(), camera->_d()->validator()),\n> -\t cookie_(cookie), status_(RequestPending)\n> +\t: Extensible(std::make_unique(camera, cookie))\n> {\n> \tLIBCAMERA_TRACEPOINT(request_construct, this);\n> \n> -\tLOG(Request, Debug) << \"Created request - cookie: \" << cookie_;\n> +\tLOG(Request, Debug) << \"Created request - cookie: \" << cookie;\n> }\n> \n> Request::~Request()\n> @@ -389,25 +408,10 @@ void Request::reuse(ReuseFlag flags)\n> {\n> \tLIBCAMERA_TRACEPOINT(request_reuse, this);\n> \n> -\t_d()->reset();\n> -\n> -\tif (flags & ReuseBuffers) {\n> -\t\tfor (const auto &[stream, buffer] : bufferMap_) {\n> -\t\t\tbuffer->_d()->setRequest(this);\n> -\t\t\t_d()->pending_.insert(buffer);\n> -\t\t}\n> -\t} else {\n> -\t\tbufferMap_.clear();\n> -\t}\n> -\n> -\tstatus_ = RequestPending;\n> -\n> -\tcontrols_.clear();\n> -\t_d()->metadata_.clear();\n> +\t_d()->reset(flags);\n> }\n> \n> /**\n> - * \\fn Request::controls()\n> * \\brief Retrieve the request's ControlList\n> *\n> * Requests store a list of controls to be applied to all frames captured for\n> @@ -421,6 +425,10 @@ void Request::reuse(ReuseFlag flags)\n> *\n> * \\return A reference to the ControlList in this request\n> */\n> +ControlList &Request::controls()\n> +{\n> +\treturn _d()->controls_;\n> +}\n> \n> /**\n> * \\brief Retrieve the request's metadata\n> @@ -432,14 +440,19 @@ const ControlList &Request::metadata() const\n> }\n> \n> /**\n> - * \\fn Request::buffers()\n> * \\brief Retrieve the request's streams to buffers map\n> *\n> * Return a reference to the map that associates each Stream part of the\n> - * request to the FrameBuffer the Stream output should be directed to.\n> + * request to the FrameBuffer the Stream output should be directed to. If a\n> + * stream is not utilised in this request there will be no buffer for that\n> + * stream in the map.\n> *\n> * \\return The map of Stream to FrameBuffer\n> */\n> +const Request::BufferMap &Request::buffers() const\n> +{\n> +\treturn _d()->bufferMap_;\n> +}\n> \n> /**\n> * \\brief Add a FrameBuffer with its associated Stream to the Request\n> @@ -492,7 +505,7 @@ int Request::addBuffer(const Stream *stream, FrameBuffer *buffer,\n> \t\treturn -EEXIST;\n> \t}\n> \n> -\tauto [it, inserted] = bufferMap_.try_emplace(stream, buffer);\n> +\tauto [it, inserted] = _d()->bufferMap_.try_emplace(stream, buffer);\n> \tif (!inserted) {\n> \t\tLOG(Request, Error) << \"FrameBuffer already set for stream\";\n> \t\treturn -EEXIST;\n> @@ -507,15 +520,6 @@ int Request::addBuffer(const Stream *stream, FrameBuffer *buffer,\n> \treturn 0;\n> }\n> \n> -/**\n> - * \\var Request::bufferMap_\n> - * \\brief Mapping of streams to buffers for this request\n> - *\n> - * The bufferMap_ tracks the buffers associated with each stream. If a stream is\n> - * not utilised in this request there will be no buffer for that stream in the\n> - * map.\n> - */\n> -\n> /**\n> * \\brief Return the buffer associated with a stream\n> * \\param[in] stream The stream the buffer is associated to\n> @@ -524,8 +528,8 @@ int Request::addBuffer(const Stream *stream, FrameBuffer *buffer,\n> */\n> FrameBuffer *Request::findBuffer(const Stream *stream) const\n> {\n> -\tconst auto it = bufferMap_.find(stream);\n> -\tif (it == bufferMap_.end())\n> +\tconst auto it = _d()->bufferMap_.find(stream);\n> +\tif (it == _d()->bufferMap_.end())\n> \t\treturn nullptr;\n> \n> \treturn it->second;\n> @@ -552,13 +556,15 @@ uint32_t Request::sequence() const\n> }\n> \n> /**\n> - * \\fn Request::cookie()\n> * \\brief Retrieve the cookie set when the request was created\n> * \\return The request cookie\n> */\n> +uint64_t Request::cookie() const\n> +{\n> +\treturn _d()->cookie_;\n> +}\n> \n> /**\n> - * \\fn Request::status()\n> * \\brief Retrieve the request completion status\n> *\n> * The request status indicates whether the request has completed successfully\n> @@ -569,6 +575,10 @@ uint32_t Request::sequence() const\n> *\n> * \\return The request completion status\n> */\n> +Request::Status Request::status() const\n> +{\n> +\treturn _d()->status_;\n> +}\n> \n> /**\n> * \\brief Check if a request has buffers yet to be completed","headers":{"Return-Path":"","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 C38A5BEFBE\n\tfor ;\n\tWed, 8 Apr 2026 18:01:11 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 81B3B62E08;\n\tWed, 8 Apr 2026 20:01:10 +0200 (CEST)","from us-smtp-delivery-124.mimecast.com\n\t(us-smtp-delivery-124.mimecast.com [170.10.129.124])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 49C9E62CE6\n\tfor ;\n\tWed, 8 Apr 2026 20:01:08 +0200 (CEST)","from mail-wr1-f69.google.com (mail-wr1-f69.google.com\n\t[209.85.221.69]) by relay.mimecast.com with ESMTP with STARTTLS\n\t(version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id\n\tus-mta-527-94hPSK9tOgqj5kvLNzR9Ng-1; Wed, 08 Apr 2026 14:01:05 -0400","by mail-wr1-f69.google.com with SMTP id\n\tffacd0b85a97d-43d15f2e0f4so2996f8f.0\n\tfor ;\n\tWed, 08 Apr 2026 11:01:05 -0700 (PDT)","from mzamazal-thinkpadp1gen7.tpbc.csb\n\t(ip-77-48-47-2.net.vodafone.cz. [77.48.47.2])\n\tby smtp.gmail.com with ESMTPSA id\n\t5b1f17b1804b1-488cd0cbea7sm2937535e9.6.2026.04.08.11.01.02\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tWed, 08 Apr 2026 11:01:02 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=redhat.com header.i=@redhat.com\n\theader.b=\"AO8sPqJM\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com;\n\ts=mimecast20190719; t=1775671267;\n\th=from:from:reply-to:subject:subject:date:date:message-id:message-id:\n\tto:to:cc:cc:mime-version:mime-version:content-type:content-type:\n\tin-reply-to:in-reply-to:references:references;\n\tbh=lB7sB6azEHMY+rJ+pxkD++bf4mtAZ3twWSQpN0FDxS4=;\n\tb=AO8sPqJM3vaEXyZmKJzgiMYF2Z17jUPR3DjwT9ta5AHByn0N4TfJZYVNPg+55CF4lTrne1\n\t24AeAVMsbDCRMueeSUjF+Mnk0EqHiw7ikeO/AkLPmab2OddN2DU8OaJzJIdIJUYahL/pU7\n\t1YZ2RTh4pVdoFLw7Jhptur6rj3sUPsI=","X-MC-Unique":"94hPSK9tOgqj5kvLNzR9Ng-1","X-Mimecast-MFC-AGG-ID":"94hPSK9tOgqj5kvLNzR9Ng_1775671265","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20251104; t=1775671264; x=1776276064;\n\th=mime-version:user-agent:message-id:date:references:in-reply-to\n\t:subject:cc:to:from:x-gm-gg:x-gm-message-state:from:to:cc:subject\n\t:date:message-id:reply-to;\n\tbh=lB7sB6azEHMY+rJ+pxkD++bf4mtAZ3twWSQpN0FDxS4=;\n\tb=dmnRVGgij74U6jLy1azprNcAL9y2B3xxp5mh5/Ufi9vbfeJ/0KiBdhXtZH3BnPgHO6\n\tc1HAVqb4N7h1EcHbWPMdDNcy5dwLqitWm74SzRStgMb4MpazP8ANUHXLENRfxHMQ43rF\n\tV1nSx6XCcp1PsPcv9oRE9ueVdhS2QuK9EFrHoX+in5A6CGMjAMiug7qOx9bjkamcLkAF\n\tB1uDmngf/pHUZ/PmkpYBYTTKv79sVsfywqMNtBvJX03Vrvao8OZ7k0QDl5Y6g/ssk+bX\n\tjUVvs3ZDkwP0So4jKl0tePA/KfEDOBMDR84w2BGs7tFBOVUiT90vbF1znl/ITpGqXrH3\n\t07Gw==","X-Gm-Message-State":"AOJu0Yz4/D97Vu719w/I5NUiGxTdPybtH0GbEAEGv92lD2dMP6pTCVIe\n\twYREJjL4aZeZu/i8xgtc4RD4hPeICVybFVCQYEZvAL4k2ZkBJJ1Hs5NGcnw6M+tZZCbCDSMeaW2\n\t0YVKwGbad9nopAdgT/PCvFTJN8NnPXviUO0zRbgat+ZeGbNSRiJ3FeIG/mGG48PDKxByOZL5LRX\n\tTZ/EX8TPD2ErZvllbTfBeLpv1Y+ok9MzMphDHWoOCsbWo8jCfPYeb+D8z7N2k=","X-Gm-Gg":"AeBDiesXqW1929Fy3X84neb04XWLscWrcqEcK37UmzP7Dxz82gdKU3owKjX6rJb0AsE\n\tTvzDNZWtx2eVJp22HsJYhubslarZStt/5urV35uyaxQ2en3S/7HMmYUdwpBeX6MYxmc8QcJQgug\n\tzhqosk6CBERz090FEcks0VK6gUcExMXZJgJ/Ii6iB6YtmuCF95tzc8MZN37WJWuQEANJ+r5xTou\n\tg2/Vp4rtHmJTOuqJsIM0IXIYA0vFw0areq0GVcqY3gT+zqKKhQKi9WoVaDoEGj2pu22ueslvLDX\n\tsvEPoYxY8IKnNF0fZgDUf/wjNGDo96EgcKm3JTnZm/GnBoQqowVkM6VE6JLp2erzP5psB6yW0Sm\n\thMniCS3W4XuZ1y9KJJeYbJNvbpCPF7pAA/WHA4vqKPzlp6GV/ob7g8iw0vNUoAkeM28Hf2R9EA4\n\tA=","X-Received":["by 2002:a05:600c:8b75:b0:486:f634:ef1 with SMTP id\n\t5b1f17b1804b1-4889978c516mr324927975e9.17.1775671264301; \n\tWed, 08 Apr 2026 11:01:04 -0700 (PDT)","by 2002:a05:600c:8b75:b0:486:f634:ef1 with SMTP id\n\t5b1f17b1804b1-4889978c516mr324927105e9.17.1775671263589; \n\tWed, 08 Apr 2026 11:01:03 -0700 (PDT)"],"From":"Milan Zamazal ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v2 02/42] libcamera: request: Move all private member\n\tvariables to Private class","In-Reply-To":"<20260407153427.1825999-3-laurent.pinchart@ideasonboard.com>\n\t(Laurent Pinchart's message of \"Tue, 7 Apr 2026 18:33:47 +0300\")","References":"<20260407153427.1825999-1-laurent.pinchart@ideasonboard.com>\n\t<20260407153427.1825999-3-laurent.pinchart@ideasonboard.com>","Date":"Wed, 08 Apr 2026 20:01:01 +0200","Message-ID":"<85h5pl9vs2.fsf@mzamazal-thinkpadp1gen7.tpbc.csb>","User-Agent":"Gnus/5.13 (Gnus v5.13)","MIME-Version":"1.0","X-Mimecast-Spam-Score":"0","X-Mimecast-MFC-PROC-ID":"7GlC5xfdoWMvwQO5eKYZXJyn5oYOXfxX898A8ZNjzuc_1775671265","X-Mimecast-Originator":"redhat.com","Content-Type":"text/plain","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]