[{"id":33594,"web_url":"https://patchwork.libcamera.org/comment/33594/","msgid":"<174212759783.1266550.6965728882415333950@ping.linuxembedded.co.uk>","date":"2025-03-16T12:19:57","subject":"Re: [PATCH v1] libcamera: request: Make `controls_`/`metadata_`\n\tmembers","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Barnabás Pőcze (2025-03-10 17:03:43)\n> The lifetimes of these two `ControlList`s are tied entirely to the request\n> object, so simplify the code by making them member variables instead of\n> manually managing their dynamic lifetime.\n> \n\nThis one looks reasonable, but I suspect it changes the ABI in some\nform?\n\nCould you run \n\n./utils/abi-compat.sh\n\nAnd check the report please? I'd like to ensure that any patches which\naffect ABI or API breakage include that statement in the commit message\nso we can identify them in the release notes. (Another candidate for the\nCI builds too of course)\n\n--\nKieran\n\n> Signed-off-by: Barnabás Pőcze \n> ---\n> include/libcamera/request.h | 8 ++++----\n> src/libcamera/request.cpp | 17 ++++-------------\n> 2 files changed, 8 insertions(+), 17 deletions(-)\n> \n> diff --git a/include/libcamera/request.h b/include/libcamera/request.h\n> index e214a9d13..3061e2fb0 100644\n> --- a/include/libcamera/request.h\n> +++ b/include/libcamera/request.h\n> @@ -49,8 +49,8 @@ public:\n> \n> void reuse(ReuseFlag flags = Default);\n> \n> - ControlList &controls() { return *controls_; }\n> - ControlList &metadata() { return *metadata_; }\n> + ControlList &controls() { return controls_; }\n> + ControlList &metadata() { return metadata_; }\n> const BufferMap &buffers() const { return bufferMap_; }\n> int addBuffer(const Stream *stream, FrameBuffer *buffer,\n> std::unique_ptr fence = nullptr);\n> @@ -67,8 +67,8 @@ public:\n> private:\n> LIBCAMERA_DISABLE_COPY(Request)\n> \n> - ControlList *controls_;\n> - ControlList *metadata_;\n> + ControlList controls_;\n> + ControlList metadata_;\n\nIf we're modifying Request public ABI - should they in fact go into the\nRequest::Private class though ?\n\n> BufferMap bufferMap_;\n> \n> const uint64_t cookie_;\n> diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp\n> index 6fa1801a0..59fc3fdf9 100644\n> --- a/src/libcamera/request.cpp\n> +++ b/src/libcamera/request.cpp\n> @@ -351,16 +351,10 @@ void Request::Private::timeout()\n> */\n> Request::Request(Camera *camera, uint64_t cookie)\n> : Extensible(std::make_unique(camera)),\n> + controls_(controls::controls, camera->_d()->validator()),\n> + metadata_(controls::controls), /* \\todo Add a validator for metadata controls. */\n> cookie_(cookie), status_(RequestPending)\n> {\n> - controls_ = new ControlList(controls::controls,\n> - camera->_d()->validator());\n> -\n> - /**\n> - * \\todo Add a validator for metadata controls.\n> - */\n> - metadata_ = new ControlList(controls::controls);\n> -\n> LIBCAMERA_TRACEPOINT(request_construct, this);\n> \n> LOG(Request, Debug) << \"Created request - cookie: \" << cookie_;\n> @@ -369,9 +363,6 @@ Request::Request(Camera *camera, uint64_t cookie)\n> Request::~Request()\n> {\n> LIBCAMERA_TRACEPOINT(request_destroy, this);\n> -\n> - delete metadata_;\n> - delete controls_;\n> }\n> \n> /**\n> @@ -402,8 +393,8 @@ void Request::reuse(ReuseFlag flags)\n> \n> status_ = RequestPending;\n> \n> - controls_->clear();\n> - metadata_->clear();\n> + controls_.clear();\n> + metadata_.clear();\n> }\n> \n> /**\n> -- \n> 2.48.1\n>","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 05279C32AF\n\tfor ;\n\tSun, 16 Mar 2025 12:20:04 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id BB66D68949;\n\tSun, 16 Mar 2025 13:20:02 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 846026893F\n\tfor ;\n\tSun, 16 Mar 2025 13:20:00 +0100 (CET)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust6594.18-1.cable.virginm.net [86.31.185.195])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id CE4CB289;\n\tSun, 16 Mar 2025 13:18:19 +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=\"pffJZCYy\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1742127499;\n\tbh=CbOt/qazbvhBaD6dm4NpJMr0OCwwyhS6OYsO4WF+Cng=;\n\th=In-Reply-To:References:Subject:From:To:Date:From;\n\tb=pffJZCYywIAb9wJyDaWmwuIoo39I1isI3f93VBln/tf6/BOKhUchG43QUfYt5A22P\n\tr9vqUxYoxKJr3FozJwdnIQeoMuJN7uam30+aigFSbX5dlwYVMP7XrFghIv6ZKS8lxX\n\tPEnEs/fhWX0IoWeb/6436w7MLHGjeRw0vldAkhBY=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20250310170343.185322-1-barnabas.pocze@ideasonboard.com>","References":"<20250310170343.185322-1-barnabas.pocze@ideasonboard.com>","Subject":"Re: [PATCH v1] libcamera: request: Make `controls_`/`metadata_`\n\tmembers","From":"Kieran Bingham ","To":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Sun, 16 Mar 2025 12:19:57 +0000","Message-ID":"<174212759783.1266550.6965728882415333950@ping.linuxembedded.co.uk>","User-Agent":"alot/0.10","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","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\" "}},{"id":33596,"web_url":"https://patchwork.libcamera.org/comment/33596/","msgid":"<20250316124209.GC29719@pendragon.ideasonboard.com>","date":"2025-03-16T12:42:09","subject":"Re: [PATCH v1] libcamera: request: Make `controls_`/`metadata_`\n\tmembers","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Sun, Mar 16, 2025 at 12:19:57PM +0000, Kieran Bingham wrote:\n> Quoting Barnabás Pőcze (2025-03-10 17:03:43)\n> > The lifetimes of these two `ControlList`s are tied entirely to the request\n> > object, so simplify the code by making them member variables instead of\n> > manually managing their dynamic lifetime.\n> > \n> \n> This one looks reasonable, but I suspect it changes the ABI in some\n> form?\n> \n> Could you run \n> \n> ./utils/abi-compat.sh\n> \n> And check the report please? I'd like to ensure that any patches which\n> affect ABI or API breakage include that statement in the commit message\n> so we can identify them in the release notes. (Another candidate for the\n> CI builds too of course)\n\nUnless there's something depending on this patch, we could also postpone\nthis change until we need to merge other ABI breakages.\n\n> > Signed-off-by: Barnabás Pőcze \n> > ---\n> > include/libcamera/request.h | 8 ++++----\n> > src/libcamera/request.cpp | 17 ++++-------------\n> > 2 files changed, 8 insertions(+), 17 deletions(-)\n> > \n> > diff --git a/include/libcamera/request.h b/include/libcamera/request.h\n> > index e214a9d13..3061e2fb0 100644\n> > --- a/include/libcamera/request.h\n> > +++ b/include/libcamera/request.h\n> > @@ -49,8 +49,8 @@ public:\n> > \n> > void reuse(ReuseFlag flags = Default);\n> > \n> > - ControlList &controls() { return *controls_; }\n> > - ControlList &metadata() { return *metadata_; }\n> > + ControlList &controls() { return controls_; }\n> > + ControlList &metadata() { return metadata_; }\n> > const BufferMap &buffers() const { return bufferMap_; }\n> > int addBuffer(const Stream *stream, FrameBuffer *buffer,\n> > std::unique_ptr fence = nullptr);\n> > @@ -67,8 +67,8 @@ public:\n> > private:\n> > LIBCAMERA_DISABLE_COPY(Request)\n> > \n> > - ControlList *controls_;\n> > - ControlList *metadata_;\n> > + ControlList controls_;\n> > + ControlList metadata_;\n> \n> If we're modifying Request public ABI - should they in fact go into the\n> Request::Private class though ?\n\nAnd while at it, move the rest of the members to the Request::Private\nclass ?\n\n> > BufferMap bufferMap_;\n> > \n> > const uint64_t cookie_;\n> > diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp\n> > index 6fa1801a0..59fc3fdf9 100644\n> > --- a/src/libcamera/request.cpp\n> > +++ b/src/libcamera/request.cpp\n> > @@ -351,16 +351,10 @@ void Request::Private::timeout()\n> > */\n> > Request::Request(Camera *camera, uint64_t cookie)\n> > : Extensible(std::make_unique(camera)),\n> > + controls_(controls::controls, camera->_d()->validator()),\n> > + metadata_(controls::controls), /* \\todo Add a validator for metadata controls. */\n> > cookie_(cookie), status_(RequestPending)\n> > {\n> > - controls_ = new ControlList(controls::controls,\n> > - camera->_d()->validator());\n> > -\n> > - /**\n> > - * \\todo Add a validator for metadata controls.\n> > - */\n> > - metadata_ = new ControlList(controls::controls);\n> > -\n> > LIBCAMERA_TRACEPOINT(request_construct, this);\n> > \n> > LOG(Request, Debug) << \"Created request - cookie: \" << cookie_;\n> > @@ -369,9 +363,6 @@ Request::Request(Camera *camera, uint64_t cookie)\n> > Request::~Request()\n> > {\n> > LIBCAMERA_TRACEPOINT(request_destroy, this);\n> > -\n> > - delete metadata_;\n> > - delete controls_;\n> > }\n> > \n> > /**\n> > @@ -402,8 +393,8 @@ void Request::reuse(ReuseFlag flags)\n> > \n> > status_ = RequestPending;\n> > \n> > - controls_->clear();\n> > - metadata_->clear();\n> > + controls_.clear();\n> > + metadata_.clear();\n> > }\n> > \n> > /**","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 E264CC32FA\n\tfor ;\n\tSun, 16 Mar 2025 12:42:33 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 315F168949;\n\tSun, 16 Mar 2025 13:42:33 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 4D40F6893F\n\tfor ;\n\tSun, 16 Mar 2025 13:42:32 +0100 (CET)","from pendragon.ideasonboard.com (unknown [185.201.63.252])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 3D1713E;\n\tSun, 16 Mar 2025 13:40:51 +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=\"CvuOuw4n\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1742128851;\n\tbh=Kaysw0b4FNCwRT8cl7TcMpBwj04JghWENnot9Cl6m6M=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=CvuOuw4nCUHntPLGm3uR66DDfEfRlt6VGuVFfqHdEUKfuel9mhdOBMQavFJkDHFB/\n\tXRQgUwh0iXNy3fQL0gIyvsYnmlgbVEL8rqo05+r2XBTtnRIKJmMLYj9VwEHgg/sR6Y\n\tSfjXLCQe6/o1KNPQnIFw/Ohhw3IM3ODaqWaZdin8=","Date":"Sun, 16 Mar 2025 14:42:09 +0200","From":"Laurent Pinchart ","To":"Kieran Bingham ","Cc":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ,\n\tlibcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v1] libcamera: request: Make `controls_`/`metadata_`\n\tmembers","Message-ID":"<20250316124209.GC29719@pendragon.ideasonboard.com>","References":"<20250310170343.185322-1-barnabas.pocze@ideasonboard.com>\n\t<174212759783.1266550.6965728882415333950@ping.linuxembedded.co.uk>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<174212759783.1266550.6965728882415333950@ping.linuxembedded.co.uk>","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\" "}},{"id":33597,"web_url":"https://patchwork.libcamera.org/comment/33597/","msgid":"<696e0e21-e9ef-41e2-ba8c-40905144c880@ideasonboard.com>","date":"2025-03-16T15:02:24","subject":"Re: [PATCH v1] libcamera: request: Make `controls_`/`metadata_`\n\tmembers","submitter":{"id":216,"url":"https://patchwork.libcamera.org/api/people/216/","name":"Barnabás Pőcze","email":"barnabas.pocze@ideasonboard.com"},"content":"Hi\n\n\n2025. 03. 16. 13:19 keltezéssel, Kieran Bingham írta:\n> Quoting Barnabás Pőcze (2025-03-10 17:03:43)\n>> The lifetimes of these two `ControlList`s are tied entirely to the request\n>> object, so simplify the code by making them member variables instead of\n>> manually managing their dynamic lifetime.\n>>\n> \n> This one looks reasonable, but I suspect it changes the ABI in some\n> form?\n> \n> Could you run\n> \n> ./utils/abi-compat.sh\n> \n> And check the report please? I'd like to ensure that any patches which\n> affect ABI or API breakage include that statement in the commit message\n> so we can identify them in the release notes. (Another candidate for the\n> CI builds too of course)\n\nIt definitely changes the ABI and API.\n\n\n> \n> --\n> Kieran\n> \n>> Signed-off-by: Barnabás Pőcze \n>> ---\n>> include/libcamera/request.h | 8 ++++----\n>> src/libcamera/request.cpp | 17 ++++-------------\n>> 2 files changed, 8 insertions(+), 17 deletions(-)\n>>\n>> diff --git a/include/libcamera/request.h b/include/libcamera/request.h\n>> index e214a9d13..3061e2fb0 100644\n>> --- a/include/libcamera/request.h\n>> +++ b/include/libcamera/request.h\n>> @@ -49,8 +49,8 @@ public:\n>> \n>> void reuse(ReuseFlag flags = Default);\n>> \n>> - ControlList &controls() { return *controls_; }\n>> - ControlList &metadata() { return *metadata_; }\n>> + ControlList &controls() { return controls_; }\n>> + ControlList &metadata() { return metadata_; }\n>> const BufferMap &buffers() const { return bufferMap_; }\n>> int addBuffer(const Stream *stream, FrameBuffer *buffer,\n>> std::unique_ptr fence = nullptr);\n>> @@ -67,8 +67,8 @@ public:\n>> private:\n>> LIBCAMERA_DISABLE_COPY(Request)\n>> \n>> - ControlList *controls_;\n>> - ControlList *metadata_;\n>> + ControlList controls_;\n>> + ControlList metadata_;\n> \n> If we're modifying Request public ABI - should they in fact go into the\n> Request::Private class though ?\n\nThat's also an option, I went with this because this was the \"simplest simplification\".\n\n\nRegards,\nBarnabás Pőcze\n\n\n> \n>> BufferMap bufferMap_;\n>> \n>> const uint64_t cookie_;\n>> diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp\n>> index 6fa1801a0..59fc3fdf9 100644\n>> --- a/src/libcamera/request.cpp\n>> +++ b/src/libcamera/request.cpp\n>> @@ -351,16 +351,10 @@ void Request::Private::timeout()\n>> */\n>> Request::Request(Camera *camera, uint64_t cookie)\n>> : Extensible(std::make_unique(camera)),\n>> + controls_(controls::controls, camera->_d()->validator()),\n>> + metadata_(controls::controls), /* \\todo Add a validator for metadata controls. */\n>> cookie_(cookie), status_(RequestPending)\n>> {\n>> - controls_ = new ControlList(controls::controls,\n>> - camera->_d()->validator());\n>> -\n>> - /**\n>> - * \\todo Add a validator for metadata controls.\n>> - */\n>> - metadata_ = new ControlList(controls::controls);\n>> -\n>> LIBCAMERA_TRACEPOINT(request_construct, this);\n>> \n>> LOG(Request, Debug) << \"Created request - cookie: \" << cookie_;\n>> @@ -369,9 +363,6 @@ Request::Request(Camera *camera, uint64_t cookie)\n>> Request::~Request()\n>> {\n>> LIBCAMERA_TRACEPOINT(request_destroy, this);\n>> -\n>> - delete metadata_;\n>> - delete controls_;\n>> }\n>> \n>> /**\n>> @@ -402,8 +393,8 @@ void Request::reuse(ReuseFlag flags)\n>> \n>> status_ = RequestPending;\n>> \n>> - controls_->clear();\n>> - metadata_->clear();\n>> + controls_.clear();\n>> + metadata_.clear();\n>> }\n>> \n>> /**\n>> -- \n>> 2.48.1\n>>","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 B6BA6C32AF\n\tfor ;\n\tSun, 16 Mar 2025 15:02:33 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D6AAC68940;\n\tSun, 16 Mar 2025 16:02:32 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D6B6B6893F\n\tfor ;\n\tSun, 16 Mar 2025 16:02:30 +0100 (CET)","from [192.168.33.20] (185.221.143.221.nat.pool.zt.hu\n\t[185.221.143.221])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 65EA11E6;\n\tSun, 16 Mar 2025 16:00:49 +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=\"DyNRUx2E\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1742137249;\n\tbh=ITH73ZzRN1mwzJ2vMON5ibG8PmjJXEm4yjjAhhXQrk0=;\n\th=Date:Subject:To:References:From:In-Reply-To:From;\n\tb=DyNRUx2EiJVm5sxDN33W8hMdHCWIs/u5I69ZMTI0xGDavlW//tOPT9z1hJi0LsXj7\n\t5sSFjtA3tgYMMfOTbE+5rYozPRed/faM9vHpIwKRRhc/kzERRmUaSoCFOSQW7pU2NY\n\t81gDFdcLgN3hvY92Uulm+fWSxIz4SLvOmZHBrW1U=","Message-ID":"<696e0e21-e9ef-41e2-ba8c-40905144c880@ideasonboard.com>","Date":"Sun, 16 Mar 2025 16:02:24 +0100","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH v1] libcamera: request: Make `controls_`/`metadata_`\n\tmembers","To":"Kieran Bingham ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20250310170343.185322-1-barnabas.pocze@ideasonboard.com>\n\t<174212759783.1266550.6965728882415333950@ping.linuxembedded.co.uk>","From":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Content-Language":"en-US, hu-HU","In-Reply-To":"<174212759783.1266550.6965728882415333950@ping.linuxembedded.co.uk>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"8bit","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\" "}}]