[{"id":35054,"web_url":"https://patchwork.libcamera.org/comment/35054/","msgid":"<175329078831.50296.3286966613258024792@ping.linuxembedded.co.uk>","date":"2025-07-23T17:13:08","subject":"Re: [PATCH v2 6/8] libcamera: camera: Hook into the LayerManager","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Paul Elder (2025-07-03 12:42:21)\n> Add hooks into the CameraManager to call into the LayerManager on all\n> relevant public-facing interface of Camera.\n> \n> The entry point for each function into the LayerManager is has been\n> chosen based on the capabilities that we want to allow to Layer\n> implementations.\n> \n> Signed-off-by: Paul Elder \n> \n> ---\n> Changes in v2:\n> - move the LayerManager out of Camera into the CameraManager\n> - remove hooks for generateConfiguration() and streams()\n> - Camera now passes itself into the layer hooks, as it is required for\n> the LayerManager to fetch the closure based on the camera to pass to\n> each layer\n> ---\n> src/libcamera/camera.cpp | 40 ++++++++++++++++++++++++++++++++++++++--\n> 1 file changed, 38 insertions(+), 2 deletions(-)\n> \n> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> index c3e656cab671..665fc5157349 100644\n> --- a/src/libcamera/camera.cpp\n> +++ b/src/libcamera/camera.cpp\n> @@ -18,6 +18,7 @@\n> #include \n> #include \n> \n> +#include \n> #include \n> #include \n> #include \n> @@ -27,6 +28,8 @@\n> \n> #include \"libcamera/internal/camera.h\"\n> #include \"libcamera/internal/camera_controls.h\"\n> +#include \"libcamera/internal/camera_manager.h\"\n> +#include \"libcamera/internal/layer_manager.h\"\n> #include \"libcamera/internal/pipeline_handler.h\"\n> #include \"libcamera/internal/request.h\"\n> \n> @@ -741,18 +744,24 @@ void Camera::Private::setState(State state)\n> void Camera::Private::emitBufferCompleted(Request *request, FrameBuffer *buffer)\n> {\n> Camera *camera = _o();\n> + LayerManager *layerManager = pipe()->cameraManager()->_d()->layerManager();\n> + layerManager->bufferCompleted(camera, request, buffer);\n\nI think we need some sort of Layers controller in the Camera class\ndirectly. It should be responsible for the specific instances of the\nLayers and the ordering/enablement from the (future) camera specific\nconfiguration.\n\nWith that - these additions would be much 'cleaner' as well as we could\nexpect:\n\n\tlayerController->bufferCompleted(...)\n\nWhich would be a part of the Camera::Private or something rather than\nhaving to indirectly obtain the layerManager each time.\n\nOtherwise - if I've mis-understood and the layerManager is already doing\nthat functionality - perhaps we should have a reference/instance to it\nin the Camera::Private class so we don't have to grab it tyhrough a\npipe, cameraManager and private instance each time....\n\n> camera->bufferCompleted.emit(request, buffer);\n> }\n> \n> void Camera::Private::emitRequestCompleted(Request *request)\n> {\n> Camera *camera = _o();\n> + LayerManager *layerManager = pipe()->cameraManager()->_d()->layerManager();\n> + layerManager->requestCompleted(camera, request);\n> camera->requestCompleted.emit(request);\n> }\n> \n> void Camera::Private::emitDisconnected()\n> {\n> Camera *camera = _o();\n> + LayerManager *layerManager = pipe()->cameraManager()->_d()->layerManager();\n> + layerManager->disconnected(camera);\n> camera->disconnected.emit();\n> }\n> \n> @@ -943,10 +952,14 @@ Camera::Camera(std::unique_ptr d, const std::string &id,\n> _d()->id_ = id;\n> _d()->streams_ = streams;\n> _d()->validator_ = std::make_unique(this);\n> + _d()->pipe()->cameraManager()->_d()->layerManager()->init(this,\n> + _d()->properties_,\n> + _d()->controlInfo_);\n> }\n> \n> Camera::~Camera()\n> {\n> + _d()->pipe()->cameraManager()->_d()->layerManager()->terminate(this);\n\n\tSomehow I'd call the Camera::Private 'layers' though so we could\n\tdo a nice short:\n\n\tlayers->terminate(this);\n\n> }\n> \n> /**\n> @@ -1032,6 +1045,9 @@ int Camera::acquire()\n> return -EBUSY;\n> }\n> \n> + LayerManager *layerManager = d->pipe()->cameraManager()->_d()->layerManager();\n> + layerManager->acquire(this);\n> +\n> d->setState(Private::CameraAcquired);\n> \n> return 0;\n> @@ -1064,6 +1080,9 @@ int Camera::release()\n> d->pipe_->invokeMethod(&PipelineHandler::release,\n> ConnectionTypeBlocking, this);\n> \n> + LayerManager *layerManager = d->pipe()->cameraManager()->_d()->layerManager();\n> + layerManager->release(this);\n> +\n> d->setState(Private::CameraAvailable);\n> \n> return 0;\n> @@ -1081,7 +1100,8 @@ int Camera::release()\n> */\n> const ControlInfoMap &Camera::controls() const\n> {\n> - return _d()->controlInfo_;\n> + LayerManager *layerManager = _d()->pipe()->cameraManager()->_d()->layerManager();\n> + return layerManager->controls(this);\n\nSomehow I'm surprised we're not passing in _d()->controlInfo() here ...\n\treturn layerManager->controls(_d()->controlInfo_);\n\nDoes the layer manager access the existing camera control Info directly\nsomehow?\n\n> }\n> \n> /**\n> @@ -1094,7 +1114,8 @@ const ControlInfoMap &Camera::controls() const\n> */\n> const ControlList &Camera::properties() const\n> {\n> - return _d()->properties_;\n> + LayerManager *layerManager = _d()->pipe()->cameraManager()->_d()->layerManager();\n> + return layerManager->properties(this);\n\nSame thoughts as controls but perhaps it's just handled differently to\nhow I expect and the answer is elsewhere.\n\n\n> }\n> \n> /**\n> @@ -1242,6 +1263,9 @@ int Camera::configure(CameraConfiguration *config)\n> d->activeStreams_.insert(stream);\n> }\n> \n> + LayerManager *layerManager = d->pipe()->cameraManager()->_d()->layerManager();\n> + layerManager->configure(this, config, d->controlInfo_);\n> +\n> d->setState(Private::CameraConfigured);\n> \n> return 0;\n> @@ -1282,6 +1306,9 @@ std::unique_ptr Camera::createRequest(uint64_t cookie)\n> /* Associate the request with the pipeline handler. */\n> d->pipe_->registerRequest(request.get());\n> \n> + LayerManager *layerManager = d->pipe()->cameraManager()->_d()->layerManager();\n> + layerManager->createRequest(this, cookie, request.get());\n> +\n> return request;\n> }\n> \n> @@ -1366,6 +1393,9 @@ int Camera::queueRequest(Request *request)\n> }\n> }\n> \n> + LayerManager *layerManager = d->pipe()->cameraManager()->_d()->layerManager();\n> + layerManager->queueRequest(this, request);\n> +\n> d->pipe_->invokeMethod(&PipelineHandler::queueRequest,\n> ConnectionTypeQueued, request);\n> \n> @@ -1402,6 +1432,9 @@ int Camera::start(const ControlList *controls)\n> \n> ASSERT(d->requestSequence_ == 0);\n> \n> + LayerManager *layerManager = d->pipe()->cameraManager()->_d()->layerManager();\n> + layerManager->start(this, controls);\n> +\n> ret = d->pipe_->invokeMethod(&PipelineHandler::start,\n> ConnectionTypeBlocking, this, controls);\n> if (ret)\n> @@ -1446,6 +1479,9 @@ int Camera::stop()\n> \n> d->setState(Private::CameraStopping);\n> \n> + LayerManager *layerManager = d->pipe()->cameraManager()->_d()->layerManager();\n> + layerManager->stop(this);\n> +\n\nI wonder if symetrically - we should stop the layers after stopping the\ncamera, as part of the stop call on the camera will invoke completion(or\ncancellation?)?\n\nI'm not entirely certain on this one - but it's just that as a wrapper\nI think I'd expect:\n\n layers->start()\n camera->start()\n camera->stop()\n layers->stop\n?\n\n> d->pipe_->invokeMethod(&PipelineHandler::stop, ConnectionTypeBlocking,\n> this);\n> \n> -- \n> 2.47.2\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 E7C67BDCC1\n\tfor ;\n\tWed, 23 Jul 2025 17:13:12 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id DCEB169062;\n\tWed, 23 Jul 2025 19:13:11 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id A54426904D\n\tfor ;\n\tWed, 23 Jul 2025 19:13:10 +0200 (CEST)","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 35C41606;\n\tWed, 23 Jul 2025 19:12:32 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"i3iylzpG\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1753290752;\n\tbh=cEO2O/UwXPzxpAX/o2raARBIyIJOe5sI2ZO/40pfMA8=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=i3iylzpGw9fFybM5V3A7K4Yzu3/4aw2Z2mliar57nIH6UX8D+U21ELhapoiMZsLkA\n\t0hqOEDWjZgPbMM6xIZG78MK3XaYVxskeG4NHfENQw1HxX3okmiA3Oh+4IbnFPnBROb\n\tdoRr+kedS5MLZpIG66HEvznZZRV22nArknUjZdMc=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20250703114225.2074071-7-paul.elder@ideasonboard.com>","References":"<20250703114225.2074071-1-paul.elder@ideasonboard.com>\n\t<20250703114225.2074071-7-paul.elder@ideasonboard.com>","Subject":"Re: [PATCH v2 6/8] libcamera: camera: Hook into the LayerManager","From":"Kieran Bingham ","Cc":"Paul Elder , barnabas.pocze@ideasonboard.com","To":"Paul Elder ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Wed, 23 Jul 2025 18:13:08 +0100","Message-ID":"<175329078831.50296.3286966613258024792@ping.linuxembedded.co.uk>","User-Agent":"alot/0.9.1","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":35056,"web_url":"https://patchwork.libcamera.org/comment/35056/","msgid":"<175329183815.50296.10741750613773889845@ping.linuxembedded.co.uk>","date":"2025-07-23T17:30:38","subject":"Re: [PATCH v2 6/8] libcamera: camera: Hook into the LayerManager","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Kieran Bingham (2025-07-23 18:13:08)\n> Quoting Paul Elder (2025-07-03 12:42:21)\n> > Add hooks into the CameraManager to call into the LayerManager on all\n> > relevant public-facing interface of Camera.\n> > \n> > The entry point for each function into the LayerManager is has been\n> > chosen based on the capabilities that we want to allow to Layer\n> > implementations.\n> > \n> > Signed-off-by: Paul Elder \n> > \n> > ---\n> > Changes in v2:\n> > - move the LayerManager out of Camera into the CameraManager\n\nOh - interesting. I think we do need a top level loader - but probably\nper-camera instances of the Layers...\n\n> > - remove hooks for generateConfiguration() and streams()\n> > - Camera now passes itself into the layer hooks, as it is required for\n> > the LayerManager to fetch the closure based on the camera to pass to\n> > each layer\n> > ---\n> > src/libcamera/camera.cpp | 40 ++++++++++++++++++++++++++++++++++++++--\n> > 1 file changed, 38 insertions(+), 2 deletions(-)\n> > \n> > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> > index c3e656cab671..665fc5157349 100644\n> > --- a/src/libcamera/camera.cpp\n> > +++ b/src/libcamera/camera.cpp\n> > @@ -18,6 +18,7 @@\n> > #include \n> > #include \n> > \n> > +#include \n> > #include \n> > #include \n> > #include \n> > @@ -27,6 +28,8 @@\n> > \n> > #include \"libcamera/internal/camera.h\"\n> > #include \"libcamera/internal/camera_controls.h\"\n> > +#include \"libcamera/internal/camera_manager.h\"\n> > +#include \"libcamera/internal/layer_manager.h\"\n> > #include \"libcamera/internal/pipeline_handler.h\"\n> > #include \"libcamera/internal/request.h\"\n> > \n> > @@ -741,18 +744,24 @@ void Camera::Private::setState(State state)\n> > void Camera::Private::emitBufferCompleted(Request *request, FrameBuffer *buffer)\n> > {\n> > Camera *camera = _o();\n> > + LayerManager *layerManager = pipe()->cameraManager()->_d()->layerManager();\n> > + layerManager->bufferCompleted(camera, request, buffer);\n> \n> I think we need some sort of Layers controller in the Camera class\n> directly. It should be responsible for the specific instances of the\n> Layers and the ordering/enablement from the (future) camera specific\n> configuration.\n> \n> With that - these additions would be much 'cleaner' as well as we could\n> expect:\n> \n> layerController->bufferCompleted(...)\n> \n> Which would be a part of the Camera::Private or something rather than\n> having to indirectly obtain the layerManager each time.\n> \n> Otherwise - if I've mis-understood and the layerManager is already doing\n> that functionality - perhaps we should have a reference/instance to it\n> in the Camera::Private class so we don't have to grab it tyhrough a\n> pipe, cameraManager and private instance each time....\n> \n> > camera->bufferCompleted.emit(request, buffer);\n> > }\n> > \n> > void Camera::Private::emitRequestCompleted(Request *request)\n> > {\n> > Camera *camera = _o();\n> > + LayerManager *layerManager = pipe()->cameraManager()->_d()->layerManager();\n> > + layerManager->requestCompleted(camera, request);\n> > camera->requestCompleted.emit(request);\n> > }\n> > \n> > void Camera::Private::emitDisconnected()\n> > {\n> > Camera *camera = _o();\n> > + LayerManager *layerManager = pipe()->cameraManager()->_d()->layerManager();\n> > + layerManager->disconnected(camera);\n> > camera->disconnected.emit();\n> > }\n> > \n> > @@ -943,10 +952,14 @@ Camera::Camera(std::unique_ptr d, const std::string &id,\n> > _d()->id_ = id;\n> > _d()->streams_ = streams;\n> > _d()->validator_ = std::make_unique(this);\n> > + _d()->pipe()->cameraManager()->_d()->layerManager()->init(this,\n> > + _d()->properties_,\n> > + _d()->controlInfo_);\n> > }\n> > \n> > Camera::~Camera()\n> > {\n> > + _d()->pipe()->cameraManager()->_d()->layerManager()->terminate(this);\n> \n> Somehow I'd call the Camera::Private 'layers' though so we could\n> do a nice short:\n> \n> layers->terminate(this);\n> \n> > }\n> > \n> > /**\n> > @@ -1032,6 +1045,9 @@ int Camera::acquire()\n> > return -EBUSY;\n> > }\n> > \n> > + LayerManager *layerManager = d->pipe()->cameraManager()->_d()->layerManager();\n> > + layerManager->acquire(this);\n> > +\n> > d->setState(Private::CameraAcquired);\n> > \n> > return 0;\n> > @@ -1064,6 +1080,9 @@ int Camera::release()\n> > d->pipe_->invokeMethod(&PipelineHandler::release,\n> > ConnectionTypeBlocking, this);\n> > \n> > + LayerManager *layerManager = d->pipe()->cameraManager()->_d()->layerManager();\n> > + layerManager->release(this);\n> > +\n> > d->setState(Private::CameraAvailable);\n> > \n> > return 0;\n> > @@ -1081,7 +1100,8 @@ int Camera::release()\n> > */\n> > const ControlInfoMap &Camera::controls() const\n> > {\n> > - return _d()->controlInfo_;\n> > + LayerManager *layerManager = _d()->pipe()->cameraManager()->_d()->layerManager();\n> > + return layerManager->controls(this);\n> \n> Somehow I'm surprised we're not passing in _d()->controlInfo() here ...\n> return layerManager->controls(_d()->controlInfo_);\n> \n> Does the layer manager access the existing camera control Info directly\n> somehow?\n\nWait - now I read the previous review comments. It's because the layers\naren't going to modify the camera state - so they get indirected/merged\nof course...\n\nI'd forgotten about that.\n\n> \n> > }\n> > \n> > /**\n> > @@ -1094,7 +1114,8 @@ const ControlInfoMap &Camera::controls() const\n> > */\n> > const ControlList &Camera::properties() const\n> > {\n> > - return _d()->properties_;\n> > + LayerManager *layerManager = _d()->pipe()->cameraManager()->_d()->layerManager();\n> > + return layerManager->properties(this);\n> \n> Same thoughts as controls but perhaps it's just handled differently to\n> how I expect and the answer is elsewhere.\n\nAnd same here of course ;-)\n\n\n> \n> \n> > }\n> > \n> > /**\n> > @@ -1242,6 +1263,9 @@ int Camera::configure(CameraConfiguration *config)\n> > d->activeStreams_.insert(stream);\n> > }\n> > \n> > + LayerManager *layerManager = d->pipe()->cameraManager()->_d()->layerManager();\n> > + layerManager->configure(this, config, d->controlInfo_);\n> > +\n> > d->setState(Private::CameraConfigured);\n> > \n> > return 0;\n> > @@ -1282,6 +1306,9 @@ std::unique_ptr Camera::createRequest(uint64_t cookie)\n> > /* Associate the request with the pipeline handler. */\n> > d->pipe_->registerRequest(request.get());\n> > \n> > + LayerManager *layerManager = d->pipe()->cameraManager()->_d()->layerManager();\n> > + layerManager->createRequest(this, cookie, request.get());\n> > +\n> > return request;\n> > }\n> > \n> > @@ -1366,6 +1393,9 @@ int Camera::queueRequest(Request *request)\n> > }\n> > }\n> > \n> > + LayerManager *layerManager = d->pipe()->cameraManager()->_d()->layerManager();\n> > + layerManager->queueRequest(this, request);\n> > +\n> > d->pipe_->invokeMethod(&PipelineHandler::queueRequest,\n> > ConnectionTypeQueued, request);\n> > \n> > @@ -1402,6 +1432,9 @@ int Camera::start(const ControlList *controls)\n> > \n> > ASSERT(d->requestSequence_ == 0);\n> > \n> > + LayerManager *layerManager = d->pipe()->cameraManager()->_d()->layerManager();\n> > + layerManager->start(this, controls);\n> > +\n> > ret = d->pipe_->invokeMethod(&PipelineHandler::start,\n> > ConnectionTypeBlocking, this, controls);\n> > if (ret)\n> > @@ -1446,6 +1479,9 @@ int Camera::stop()\n> > \n> > d->setState(Private::CameraStopping);\n> > \n> > + LayerManager *layerManager = d->pipe()->cameraManager()->_d()->layerManager();\n> > + layerManager->stop(this);\n> > +\n> \n> I wonder if symetrically - we should stop the layers after stopping the\n> camera, as part of the stop call on the camera will invoke completion(or\n> cancellation?)?\n> \n> I'm not entirely certain on this one - but it's just that as a wrapper\n> I think I'd expect:\n> \n> layers->start()\n> camera->start()\n> camera->stop()\n> layers->stop\n> ?\n> \n> > d->pipe_->invokeMethod(&PipelineHandler::stop, ConnectionTypeBlocking,\n> > this);\n> > \n> > -- \n> > 2.47.2\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 207D3BDCC1\n\tfor ;\n\tWed, 23 Jul 2025 17:30:44 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 056C269062;\n\tWed, 23 Jul 2025 19:30:43 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id B0C9F614ED\n\tfor ;\n\tWed, 23 Jul 2025 19:30:41 +0200 (CEST)","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 EFBD31026;\n\tWed, 23 Jul 2025 19:30:02 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"V7QACYu1\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1753291803;\n\tbh=5/NV2GZvzhBDHkJ1hdcjpe3zpRUJWLw/Ml/oJAztL38=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=V7QACYu192G1qGvffgBkzputScniSnKBBCIa40EtDxjGK4mjzlf6zaHb17uoCx/kn\n\tTEqLO/ifGsSYksTYMeIVELuLPU1IG1jSFlh/c83dQJUEg6ibFnbzXibYEdHHEdSHeJ\n\t9HxHsMNmz6EjdLrYxA3CkYYsyL6AGh5oRO9l2ZYk=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<175329078831.50296.3286966613258024792@ping.linuxembedded.co.uk>","References":"<20250703114225.2074071-1-paul.elder@ideasonboard.com>\n\t<20250703114225.2074071-7-paul.elder@ideasonboard.com>\n\t<175329078831.50296.3286966613258024792@ping.linuxembedded.co.uk>","Subject":"Re: [PATCH v2 6/8] libcamera: camera: Hook into the LayerManager","From":"Kieran Bingham ","Cc":"Paul Elder , barnabas.pocze@ideasonboard.com","To":"Paul Elder ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Wed, 23 Jul 2025 18:30:38 +0100","Message-ID":"<175329183815.50296.10741750613773889845@ping.linuxembedded.co.uk>","User-Agent":"alot/0.9.1","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":35081,"web_url":"https://patchwork.libcamera.org/comment/35081/","msgid":"<175334568831.774292.15735847792710100495@neptunite.rasen.tech>","date":"2025-07-24T08:28:08","subject":"Re: [PATCH v2 6/8] libcamera: camera: Hook into the LayerManager","submitter":{"id":17,"url":"https://patchwork.libcamera.org/api/people/17/","name":"Paul Elder","email":"paul.elder@ideasonboard.com"},"content":"Quoting Kieran Bingham (2025-07-24 02:30:38)\n> Quoting Kieran Bingham (2025-07-23 18:13:08)\n> > Quoting Paul Elder (2025-07-03 12:42:21)\n> > > Add hooks into the CameraManager to call into the LayerManager on all\n> > > relevant public-facing interface of Camera.\n> > > \n> > > The entry point for each function into the LayerManager is has been\n> > > chosen based on the capabilities that we want to allow to Layer\n> > > implementations.\n> > > \n> > > Signed-off-by: Paul Elder \n> > > \n> > > ---\n> > > Changes in v2:\n> > > - move the LayerManager out of Camera into the CameraManager\n> \n> Oh - interesting. I think we do need a top level loader - but probably\n\nYeah.\n\n> per-camera instances of the Layers...\n\nHm yeah I guess we do need that in order to support per-camera layer queues.\nMight as well do it now since it's a pretty core architectural change.\n\n> \n> > > - remove hooks for generateConfiguration() and streams()\n> > > - Camera now passes itself into the layer hooks, as it is required for\n> > > the LayerManager to fetch the closure based on the camera to pass to\n> > > each layer\n> > > ---\n> > > src/libcamera/camera.cpp | 40 ++++++++++++++++++++++++++++++++++++++--\n> > > 1 file changed, 38 insertions(+), 2 deletions(-)\n> > > \n> > > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> > > index c3e656cab671..665fc5157349 100644\n> > > --- a/src/libcamera/camera.cpp\n> > > +++ b/src/libcamera/camera.cpp\n> > > @@ -18,6 +18,7 @@\n> > > #include \n> > > #include \n> > > \n> > > +#include \n> > > #include \n> > > #include \n> > > #include \n> > > @@ -27,6 +28,8 @@\n> > > \n> > > #include \"libcamera/internal/camera.h\"\n> > > #include \"libcamera/internal/camera_controls.h\"\n> > > +#include \"libcamera/internal/camera_manager.h\"\n> > > +#include \"libcamera/internal/layer_manager.h\"\n> > > #include \"libcamera/internal/pipeline_handler.h\"\n> > > #include \"libcamera/internal/request.h\"\n> > > \n> > > @@ -741,18 +744,24 @@ void Camera::Private::setState(State state)\n> > > void Camera::Private::emitBufferCompleted(Request *request, FrameBuffer *buffer)\n> > > {\n> > > Camera *camera = _o();\n> > > + LayerManager *layerManager = pipe()->cameraManager()->_d()->layerManager();\n> > > + layerManager->bufferCompleted(camera, request, buffer);\n> > \n> > I think we need some sort of Layers controller in the Camera class\n> > directly. It should be responsible for the specific instances of the\n> > Layers and the ordering/enablement from the (future) camera specific\n> > configuration.\n> > \n> > With that - these additions would be much 'cleaner' as well as we could\n> > expect:\n> > \n> > layerController->bufferCompleted(...)\n> > \n> > Which would be a part of the Camera::Private or something rather than\n> > having to indirectly obtain the layerManager each time.\n> > \n> > Otherwise - if I've mis-understood and the layerManager is already doing\n> > that functionality - perhaps we should have a reference/instance to it\n> > in the Camera::Private class so we don't have to grab it tyhrough a\n> > pipe, cameraManager and private instance each time....\n> > \n> > > camera->bufferCompleted.emit(request, buffer);\n> > > }\n> > > \n> > > void Camera::Private::emitRequestCompleted(Request *request)\n> > > {\n> > > Camera *camera = _o();\n> > > + LayerManager *layerManager = pipe()->cameraManager()->_d()->layerManager();\n> > > + layerManager->requestCompleted(camera, request);\n> > > camera->requestCompleted.emit(request);\n> > > }\n> > > \n> > > void Camera::Private::emitDisconnected()\n> > > {\n> > > Camera *camera = _o();\n> > > + LayerManager *layerManager = pipe()->cameraManager()->_d()->layerManager();\n> > > + layerManager->disconnected(camera);\n> > > camera->disconnected.emit();\n> > > }\n> > > \n> > > @@ -943,10 +952,14 @@ Camera::Camera(std::unique_ptr d, const std::string &id,\n> > > _d()->id_ = id;\n> > > _d()->streams_ = streams;\n> > > _d()->validator_ = std::make_unique(this);\n> > > + _d()->pipe()->cameraManager()->_d()->layerManager()->init(this,\n> > > + _d()->properties_,\n> > > + _d()->controlInfo_);\n> > > }\n> > > \n> > > Camera::~Camera()\n> > > {\n> > > + _d()->pipe()->cameraManager()->_d()->layerManager()->terminate(this);\n> > \n> > Somehow I'd call the Camera::Private 'layers' though so we could\n> > do a nice short:\n> > \n> > layers->terminate(this);\n> > \n> > > }\n> > > \n> > > /**\n> > > @@ -1032,6 +1045,9 @@ int Camera::acquire()\n> > > return -EBUSY;\n> > > }\n> > > \n> > > + LayerManager *layerManager = d->pipe()->cameraManager()->_d()->layerManager();\n> > > + layerManager->acquire(this);\n> > > +\n> > > d->setState(Private::CameraAcquired);\n> > > \n> > > return 0;\n> > > @@ -1064,6 +1080,9 @@ int Camera::release()\n> > > d->pipe_->invokeMethod(&PipelineHandler::release,\n> > > ConnectionTypeBlocking, this);\n> > > \n> > > + LayerManager *layerManager = d->pipe()->cameraManager()->_d()->layerManager();\n> > > + layerManager->release(this);\n> > > +\n> > > d->setState(Private::CameraAvailable);\n> > > \n> > > return 0;\n> > > @@ -1081,7 +1100,8 @@ int Camera::release()\n> > > */\n> > > const ControlInfoMap &Camera::controls() const\n> > > {\n> > > - return _d()->controlInfo_;\n> > > + LayerManager *layerManager = _d()->pipe()->cameraManager()->_d()->layerManager();\n> > > + return layerManager->controls(this);\n> > \n> > Somehow I'm surprised we're not passing in _d()->controlInfo() here ...\n> > return layerManager->controls(_d()->controlInfo_);\n> > \n> > Does the layer manager access the existing camera control Info directly\n> > somehow?\n> \n> Wait - now I read the previous review comments. It's because the layers\n> aren't going to modify the camera state - so they get indirected/merged\n> of course...\n> \n> I'd forgotten about that.\n> \n> > \n> > > }\n> > > \n> > > /**\n> > > @@ -1094,7 +1114,8 @@ const ControlInfoMap &Camera::controls() const\n> > > */\n> > > const ControlList &Camera::properties() const\n> > > {\n> > > - return _d()->properties_;\n> > > + LayerManager *layerManager = _d()->pipe()->cameraManager()->_d()->layerManager();\n> > > + return layerManager->properties(this);\n> > \n> > Same thoughts as controls but perhaps it's just handled differently to\n> > how I expect and the answer is elsewhere.\n> \n> And same here of course ;-)\n> \n> \n> > \n> > \n> > > }\n> > > \n> > > /**\n> > > @@ -1242,6 +1263,9 @@ int Camera::configure(CameraConfiguration *config)\n> > > d->activeStreams_.insert(stream);\n> > > }\n> > > \n> > > + LayerManager *layerManager = d->pipe()->cameraManager()->_d()->layerManager();\n> > > + layerManager->configure(this, config, d->controlInfo_);\n> > > +\n> > > d->setState(Private::CameraConfigured);\n> > > \n> > > return 0;\n> > > @@ -1282,6 +1306,9 @@ std::unique_ptr Camera::createRequest(uint64_t cookie)\n> > > /* Associate the request with the pipeline handler. */\n> > > d->pipe_->registerRequest(request.get());\n> > > \n> > > + LayerManager *layerManager = d->pipe()->cameraManager()->_d()->layerManager();\n> > > + layerManager->createRequest(this, cookie, request.get());\n> > > +\n> > > return request;\n> > > }\n> > > \n> > > @@ -1366,6 +1393,9 @@ int Camera::queueRequest(Request *request)\n> > > }\n> > > }\n> > > \n> > > + LayerManager *layerManager = d->pipe()->cameraManager()->_d()->layerManager();\n> > > + layerManager->queueRequest(this, request);\n> > > +\n> > > d->pipe_->invokeMethod(&PipelineHandler::queueRequest,\n> > > ConnectionTypeQueued, request);\n> > > \n> > > @@ -1402,6 +1432,9 @@ int Camera::start(const ControlList *controls)\n> > > \n> > > ASSERT(d->requestSequence_ == 0);\n> > > \n> > > + LayerManager *layerManager = d->pipe()->cameraManager()->_d()->layerManager();\n> > > + layerManager->start(this, controls);\n> > > +\n> > > ret = d->pipe_->invokeMethod(&PipelineHandler::start,\n> > > ConnectionTypeBlocking, this, controls);\n> > > if (ret)\n> > > @@ -1446,6 +1479,9 @@ int Camera::stop()\n> > > \n> > > d->setState(Private::CameraStopping);\n> > > \n> > > + LayerManager *layerManager = d->pipe()->cameraManager()->_d()->layerManager();\n> > > + layerManager->stop(this);\n> > > +\n> > \n> > I wonder if symetrically - we should stop the layers after stopping the\n> > camera, as part of the stop call on the camera will invoke completion(or\n> > cancellation?)?\n> > \n> > I'm not entirely certain on this one - but it's just that as a wrapper\n> > I think I'd expect:\n> > \n> > layers->start()\n> > camera->start()\n> > camera->stop()\n> > layers->stop\n> > ?\n\nGood point, yeah.\n\n\nThanks,\n\nPaul\n\n> > \n> > > d->pipe_->invokeMethod(&PipelineHandler::stop, ConnectionTypeBlocking,\n> > > this);\n> > > \n> > > -- \n> > > 2.47.2\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 554B2BDCC1\n\tfor ;\n\tThu, 24 Jul 2025 08:28:16 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5E07C690B8;\n\tThu, 24 Jul 2025 10:28:15 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C2BE269098\n\tfor ;\n\tThu, 24 Jul 2025 10:28:13 +0200 (CEST)","from neptunite.rasen.tech (unknown\n\t[IPv6:2404:7a81:160:2100:be7f:7fca:78b4:1343])\n\tby perceval.ideasonboard.com (Postfix) with UTF8SMTPSA id 5FD80C66;\n\tThu, 24 Jul 2025 10:27:34 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"hxjwfvvV\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1753345654;\n\tbh=zvsqGUMsisTFxXDgs3+5OeLv/cuvq6T3jUsPpTpZYa0=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=hxjwfvvVDUotzH6nov6fW/VzYXtg6okSrHbtesG47Nxp9+BEqmYoIcxNFw6l9BhiD\n\tfiuFoksi8895P+7vStT7dYyijzZTkFM0xHg5q9DQ4jgXEc837gEaFI1ORsy2bpvPjn\n\t+shZtdnI2S2iANMg5hgjcxIPUs+JFi5gMdDgx3RE=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<175329183815.50296.10741750613773889845@ping.linuxembedded.co.uk>","References":"<20250703114225.2074071-1-paul.elder@ideasonboard.com>\n\t<20250703114225.2074071-7-paul.elder@ideasonboard.com>\n\t<175329078831.50296.3286966613258024792@ping.linuxembedded.co.uk>\n\t<175329183815.50296.10741750613773889845@ping.linuxembedded.co.uk>","Subject":"Re: [PATCH v2 6/8] libcamera: camera: Hook into the LayerManager","From":"Paul Elder ","Cc":"barnabas.pocze@ideasonboard.com","To":"Kieran Bingham ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Thu, 24 Jul 2025 17:28:08 +0900","Message-ID":"<175334568831.774292.15735847792710100495@neptunite.rasen.tech>","User-Agent":"alot/0.0.0","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\" "}}]