[{"id":13017,"web_url":"https://patchwork.libcamera.org/comment/13017/","msgid":"<20201006161217.2zqlu45ddqxvncxs@uno.localdomain>","date":"2020-10-06T16:12:17","subject":"Re: [libcamera-devel] [RFC 2/2] android: camera_device: Queue\n\trequest to Worker","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Jacopo,\n\nOn Tue, Oct 06, 2020 at 06:06:37PM +0200, Jacopo Mondi wrote:\n> Add a CameraWorker class member to the CameraDevice class and\n> queue capture requests to it to delegate fence handling and capture\n> requests queueing to the camera.\n>\n> Signed-off-by: Jacopo Mondi \n> ---\n> src/android/camera_device.cpp | 21 ++++++++-------------\n> src/android/camera_device.h | 3 +++\n> 2 files changed, 11 insertions(+), 13 deletions(-)\n>\n> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> index 8da70e817b46..edac9f28ab67 100644\n> --- a/src/android/camera_device.cpp\n> +++ b/src/android/camera_device.cpp\n> @@ -194,8 +194,8 @@ CameraDevice::Camera3RequestDescriptor::~Camera3RequestDescriptor()\n> */\n>\n> CameraDevice::CameraDevice(unsigned int id, const std::shared_ptr &camera)\n> -\t: id_(id), running_(false), camera_(camera), staticMetadata_(nullptr),\n> -\t facing_(CAMERA_FACING_FRONT), orientation_(0)\n> +\t: id_(id), worker_(camera), running_(false), camera_(camera),\n> +\t staticMetadata_(nullptr), facing_(CAMERA_FACING_FRONT), orientation_(0)\n> {\n> \tcamera_->requestCompleted.connect(this, &CameraDevice::requestComplete);\n>\n> @@ -1375,8 +1375,9 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n> \t\tnew Camera3RequestDescriptor(camera3Request->frame_number,\n> \t\t\t\t\t camera3Request->num_output_buffers);\n>\n> -\tRequest *request =\n> -\t\tcamera_->createRequest(reinterpret_cast(descriptor));\n> +\tstd::unique_ptr captureRequest =\n> +\t\tstd::make_unique(\n> +\t\t\tcamera_->createRequest(reinterpret_cast(descriptor)));\n>\n> \tLOG(HAL, Debug) << \"Queueing Request to libcamera with \"\n> \t\t\t<< descriptor->numBuffers << \" HAL streams\";\n> @@ -1440,21 +1441,15 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n>\n> \t\tif (!buffer) {\n> \t\t\tLOG(HAL, Error) << \"Failed to create buffer\";\n> -\t\t\tdelete request;\n\nYou're leaking request :p\n\n> \t\t\tdelete descriptor;\n> \t\t\treturn -ENOMEM;\n> \t\t}\n>\n> -\t\trequest->addBuffer(cameraStream->stream(), buffer);\n> +\t\tcaptureRequest->addBuffer(cameraStream->stream(), buffer,\n> +\t\t\t\t\t camera3Buffers[i].acquire_fence);\n> \t}\n>\n> -\tint ret = camera_->queueRequest(request);\n> -\tif (ret) {\n> -\t\tLOG(HAL, Error) << \"Failed to queue request\";\n> -\t\tdelete request;\n> -\t\tdelete descriptor;\n> -\t\treturn ret;\n> -\t}\n> +\tworker_.queueRequest(std::move(captureRequest));\n>\n> \treturn 0;\n> }\n> diff --git a/src/android/camera_device.h b/src/android/camera_device.h\n> index 777d1a35e801..b4b32f77a29a 100644\n> --- a/src/android/camera_device.h\n> +++ b/src/android/camera_device.h\n> @@ -25,6 +25,7 @@\n> #include \"libcamera/internal/message.h\"\n>\n> #include \"camera_stream.h\"\n> +#include \"camera_worker.h\"\n> #include \"jpeg/encoder.h\"\n>\n> class CameraMetadata;\n> @@ -108,6 +109,8 @@ private:\n> \tunsigned int id_;\n> \tcamera3_device_t camera3Device_;\n>\n> +\tCameraWorker worker_;\n> +\n> \tbool running_;\n> \tstd::shared_ptr camera_;\n> \tstd::unique_ptr config_;\n> --\n> 2.28.0\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 6DAB6BEEDF\n\tfor ;\n\tTue, 6 Oct 2020 16:08:20 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 1686263C1B;\n\tTue, 6 Oct 2020 18:08:20 +0200 (CEST)","from relay11.mail.gandi.net (relay11.mail.gandi.net\n\t[217.70.178.231])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E18DC60363\n\tfor ;\n\tTue, 6 Oct 2020 18:08:18 +0200 (CEST)","from uno.localdomain (93-34-118-233.ip49.fastwebnet.it\n\t[93.34.118.233]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay11.mail.gandi.net (Postfix) with ESMTPSA id A1176100004\n\tfor ;\n\tTue, 6 Oct 2020 16:08:18 +0000 (UTC)"],"Date":"Tue, 6 Oct 2020 18:12:17 +0200","From":"Jacopo Mondi ","To":"libcamera-devel@lists.libcamera.org","Message-ID":"<20201006161217.2zqlu45ddqxvncxs@uno.localdomain>","References":"<20201006160637.29841-1-jacopo@jmondi.org>\n\t<20201006160637.29841-3-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20201006160637.29841-3-jacopo@jmondi.org>","Subject":"Re: [libcamera-devel] [RFC 2/2] android: camera_device: Queue\n\trequest to Worker","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","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":13097,"web_url":"https://patchwork.libcamera.org/comment/13097/","msgid":"<20201008042438.GW3937@pendragon.ideasonboard.com>","date":"2020-10-08T04:24:38","subject":"Re: [libcamera-devel] [RFC 2/2] android: camera_device: Queue\n\trequest to Worker","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nThank you for the patch.\n\nOn Tue, Oct 06, 2020 at 06:12:17PM +0200, Jacopo Mondi wrote:\n> On Tue, Oct 06, 2020 at 06:06:37PM +0200, Jacopo Mondi wrote:\n> > Add a CameraWorker class member to the CameraDevice class and\n> > queue capture requests to it to delegate fence handling and capture\n> > requests queueing to the camera.\n> >\n> > Signed-off-by: Jacopo Mondi \n> > ---\n> > src/android/camera_device.cpp | 21 ++++++++-------------\n> > src/android/camera_device.h | 3 +++\n> > 2 files changed, 11 insertions(+), 13 deletions(-)\n> >\n> > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> > index 8da70e817b46..edac9f28ab67 100644\n> > --- a/src/android/camera_device.cpp\n> > +++ b/src/android/camera_device.cpp\n> > @@ -194,8 +194,8 @@ CameraDevice::Camera3RequestDescriptor::~Camera3RequestDescriptor()\n> > */\n> >\n> > CameraDevice::CameraDevice(unsigned int id, const std::shared_ptr &camera)\n> > -\t: id_(id), running_(false), camera_(camera), staticMetadata_(nullptr),\n> > -\t facing_(CAMERA_FACING_FRONT), orientation_(0)\n> > +\t: id_(id), worker_(camera), running_(false), camera_(camera),\n> > +\t staticMetadata_(nullptr), facing_(CAMERA_FACING_FRONT), orientation_(0)\n> > {\n> > \tcamera_->requestCompleted.connect(this, &CameraDevice::requestComplete);\n> >\n> > @@ -1375,8 +1375,9 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n> > \t\tnew Camera3RequestDescriptor(camera3Request->frame_number,\n> > \t\t\t\t\t camera3Request->num_output_buffers);\n> >\n> > -\tRequest *request =\n> > -\t\tcamera_->createRequest(reinterpret_cast(descriptor));\n> > +\tstd::unique_ptr captureRequest =\n> > +\t\tstd::make_unique(\n> > +\t\t\tcamera_->createRequest(reinterpret_cast(descriptor)));\n> >\n> > \tLOG(HAL, Debug) << \"Queueing Request to libcamera with \"\n> > \t\t\t<< descriptor->numBuffers << \" HAL streams\";\n> > @@ -1440,21 +1441,15 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n> >\n> > \t\tif (!buffer) {\n> > \t\t\tLOG(HAL, Error) << \"Failed to create buffer\";\n> > -\t\t\tdelete request;\n> \n> You're leaking request :p\n\nApart from that, and the need to start the worker on open and stop it on\nclose (with proper synchronization to make sure nothing gets lost on the\nmessage queue), it looks good to me.\n\n> > \t\t\tdelete descriptor;\n> > \t\t\treturn -ENOMEM;\n> > \t\t}\n> >\n> > -\t\trequest->addBuffer(cameraStream->stream(), buffer);\n> > +\t\tcaptureRequest->addBuffer(cameraStream->stream(), buffer,\n> > +\t\t\t\t\t camera3Buffers[i].acquire_fence);\n> > \t}\n> >\n> > -\tint ret = camera_->queueRequest(request);\n> > -\tif (ret) {\n> > -\t\tLOG(HAL, Error) << \"Failed to queue request\";\n> > -\t\tdelete request;\n> > -\t\tdelete descriptor;\n> > -\t\treturn ret;\n> > -\t}\n> > +\tworker_.queueRequest(std::move(captureRequest));\n> >\n> > \treturn 0;\n> > }\n> > diff --git a/src/android/camera_device.h b/src/android/camera_device.h\n> > index 777d1a35e801..b4b32f77a29a 100644\n> > --- a/src/android/camera_device.h\n> > +++ b/src/android/camera_device.h\n> > @@ -25,6 +25,7 @@\n> > #include \"libcamera/internal/message.h\"\n> >\n> > #include \"camera_stream.h\"\n> > +#include \"camera_worker.h\"\n> > #include \"jpeg/encoder.h\"\n> >\n> > class CameraMetadata;\n> > @@ -108,6 +109,8 @@ private:\n> > \tunsigned int id_;\n> > \tcamera3_device_t camera3Device_;\n> >\n> > +\tCameraWorker worker_;\n> > +\n> > \tbool running_;\n> > \tstd::shared_ptr camera_;\n> > \tstd::unique_ptr config_;","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 B007EBEEE0\n\tfor ;\n\tThu, 8 Oct 2020 04:25:22 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4E317605B7;\n\tThu, 8 Oct 2020 06:25:22 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id ED3D260356\n\tfor ;\n\tThu, 8 Oct 2020 06:25:20 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 4EFDA59E;\n\tThu, 8 Oct 2020 06:25:20 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"ulgKYdon\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1602131120;\n\tbh=78MIhXSG+1ltHpZYoqqaaJY3ZER5QvEBfC93jK1SF5M=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=ulgKYdontuzBwshGIZEwSVPGpC8jlBcWGEWkfZOxBd5H0Jd7dKMomIZ1DD7d6rR5e\n\tv3Qxt0ItwFlwuIYzk+WHLwdh0IWcewm30CcI0umGpXGxS5GQjgPhptn2tF/2GM1lmS\n\t4+5mRx7uAixOuapq7L/0QCoHgu0+84Pfr4SQCIxw=","Date":"Thu, 8 Oct 2020 07:24:38 +0300","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Message-ID":"<20201008042438.GW3937@pendragon.ideasonboard.com>","References":"<20201006160637.29841-1-jacopo@jmondi.org>\n\t<20201006160637.29841-3-jacopo@jmondi.org>\n\t<20201006161217.2zqlu45ddqxvncxs@uno.localdomain>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20201006161217.2zqlu45ddqxvncxs@uno.localdomain>","Subject":"Re: [libcamera-devel] [RFC 2/2] android: camera_device: Queue\n\trequest to Worker","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","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":13101,"web_url":"https://patchwork.libcamera.org/comment/13101/","msgid":"<20201008083807.ge65d6qxduhwv5ti@uno.localdomain>","date":"2020-10-08T08:38:07","subject":"Re: [libcamera-devel] [RFC 2/2] android: camera_device: Queue\n\trequest to Worker","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent,\n\nOn Thu, Oct 08, 2020 at 07:24:38AM +0300, Laurent Pinchart wrote:\n> Hi Jacopo,\n>\n> Thank you for the patch.\n>\n> On Tue, Oct 06, 2020 at 06:12:17PM +0200, Jacopo Mondi wrote:\n> > On Tue, Oct 06, 2020 at 06:06:37PM +0200, Jacopo Mondi wrote:\n> > > Add a CameraWorker class member to the CameraDevice class and\n> > > queue capture requests to it to delegate fence handling and capture\n> > > requests queueing to the camera.\n> > >\n> > > Signed-off-by: Jacopo Mondi \n> > > ---\n> > > src/android/camera_device.cpp | 21 ++++++++-------------\n> > > src/android/camera_device.h | 3 +++\n> > > 2 files changed, 11 insertions(+), 13 deletions(-)\n> > >\n> > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> > > index 8da70e817b46..edac9f28ab67 100644\n> > > --- a/src/android/camera_device.cpp\n> > > +++ b/src/android/camera_device.cpp\n> > > @@ -194,8 +194,8 @@ CameraDevice::Camera3RequestDescriptor::~Camera3RequestDescriptor()\n> > > */\n> > >\n> > > CameraDevice::CameraDevice(unsigned int id, const std::shared_ptr &camera)\n> > > -\t: id_(id), running_(false), camera_(camera), staticMetadata_(nullptr),\n> > > -\t facing_(CAMERA_FACING_FRONT), orientation_(0)\n> > > +\t: id_(id), worker_(camera), running_(false), camera_(camera),\n> > > +\t staticMetadata_(nullptr), facing_(CAMERA_FACING_FRONT), orientation_(0)\n> > > {\n> > > \tcamera_->requestCompleted.connect(this, &CameraDevice::requestComplete);\n> > >\n> > > @@ -1375,8 +1375,9 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n> > > \t\tnew Camera3RequestDescriptor(camera3Request->frame_number,\n> > > \t\t\t\t\t camera3Request->num_output_buffers);\n> > >\n> > > -\tRequest *request =\n> > > -\t\tcamera_->createRequest(reinterpret_cast(descriptor));\n> > > +\tstd::unique_ptr captureRequest =\n> > > +\t\tstd::make_unique(\n> > > +\t\t\tcamera_->createRequest(reinterpret_cast(descriptor)));\n> > >\n> > > \tLOG(HAL, Debug) << \"Queueing Request to libcamera with \"\n> > > \t\t\t<< descriptor->numBuffers << \" HAL streams\";\n> > > @@ -1440,21 +1441,15 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n> > >\n> > > \t\tif (!buffer) {\n> > > \t\t\tLOG(HAL, Error) << \"Failed to create buffer\";\n> > > -\t\t\tdelete request;\n> >\n> > You're leaking request :p\n>\n> Apart from that, and the need to start the worker on open and stop it on\n> close (with proper synchronization to make sure nothing gets lost on the\n> message queue), it looks good to me.\n>\n\ndepending if Paul's reusable request gets in first, this might become\nless awful.\n\n\n> > > \t\t\tdelete descriptor;\n> > > \t\t\treturn -ENOMEM;\n> > > \t\t}\n> > >\n> > > -\t\trequest->addBuffer(cameraStream->stream(), buffer);\n> > > +\t\tcaptureRequest->addBuffer(cameraStream->stream(), buffer,\n> > > +\t\t\t\t\t camera3Buffers[i].acquire_fence);\n> > > \t}\n> > >\n> > > -\tint ret = camera_->queueRequest(request);\n> > > -\tif (ret) {\n> > > -\t\tLOG(HAL, Error) << \"Failed to queue request\";\n> > > -\t\tdelete request;\n> > > -\t\tdelete descriptor;\n> > > -\t\treturn ret;\n> > > -\t}\n> > > +\tworker_.queueRequest(std::move(captureRequest));\n> > >\n> > > \treturn 0;\n> > > }\n> > > diff --git a/src/android/camera_device.h b/src/android/camera_device.h\n> > > index 777d1a35e801..b4b32f77a29a 100644\n> > > --- a/src/android/camera_device.h\n> > > +++ b/src/android/camera_device.h\n> > > @@ -25,6 +25,7 @@\n> > > #include \"libcamera/internal/message.h\"\n> > >\n> > > #include \"camera_stream.h\"\n> > > +#include \"camera_worker.h\"\n> > > #include \"jpeg/encoder.h\"\n> > >\n> > > class CameraMetadata;\n> > > @@ -108,6 +109,8 @@ private:\n> > > \tunsigned int id_;\n> > > \tcamera3_device_t camera3Device_;\n> > >\n> > > +\tCameraWorker worker_;\n> > > +\n> > > \tbool running_;\n> > > \tstd::shared_ptr camera_;\n> > > \tstd::unique_ptr config_;\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","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 37DBABEEDF\n\tfor ;\n\tThu, 8 Oct 2020 08:34:10 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C376C605BD;\n\tThu, 8 Oct 2020 10:34:09 +0200 (CEST)","from relay1-d.mail.gandi.net (relay1-d.mail.gandi.net\n\t[217.70.183.193])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 8186D60358\n\tfor ;\n\tThu, 8 Oct 2020 10:34:08 +0200 (CEST)","from uno.localdomain (93-34-118-233.ip49.fastwebnet.it\n\t[93.34.118.233]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay1-d.mail.gandi.net (Postfix) with ESMTPSA id 0CC1D240009;\n\tThu, 8 Oct 2020 08:34:07 +0000 (UTC)"],"X-Originating-IP":"93.34.118.233","Date":"Thu, 8 Oct 2020 10:38:07 +0200","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Message-ID":"<20201008083807.ge65d6qxduhwv5ti@uno.localdomain>","References":"<20201006160637.29841-1-jacopo@jmondi.org>\n\t<20201006160637.29841-3-jacopo@jmondi.org>\n\t<20201006161217.2zqlu45ddqxvncxs@uno.localdomain>\n\t<20201008042438.GW3937@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20201008042438.GW3937@pendragon.ideasonboard.com>","Subject":"Re: [libcamera-devel] [RFC 2/2] android: camera_device: Queue\n\trequest to Worker","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","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]