[{"id":18127,"web_url":"https://patchwork.libcamera.org/comment/18127/","msgid":"","date":"2021-07-12T15:25:07","subject":"Re: [libcamera-devel] [PATCH 15/30] cam: Move event loop execution\n\tfrom CameraSession to CamApp","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Laurent,\n\nOn 07/07/2021 03:19, Laurent Pinchart wrote:\n> To prepare for multiple concurrent camera sessions, move the event loop\n> exec() call from the CameraSession class to the CamApp class.\n\nThere we go - now that global loop access is becoming clearer.\n\n> Signed-off-by: Laurent Pinchart \n> ---\n> src/cam/camera_session.cpp | 43 +++++++++++++++++---------------------\n> src/cam/camera_session.h | 6 ++++--\n> src/cam/main.cpp | 12 ++++++++++-\n> 3 files changed, 34 insertions(+), 27 deletions(-)\n> \n> diff --git a/src/cam/camera_session.cpp b/src/cam/camera_session.cpp\n> index 16c1c66a285a..b1200e60329c 100644\n> --- a/src/cam/camera_session.cpp\n> +++ b/src/cam/camera_session.cpp\n> @@ -22,11 +22,11 @@ CameraSession::CameraSession(std::shared_ptr camera,\n> \t\t\t CameraConfiguration *config)\n> \t: camera_(camera), config_(config), writer_(nullptr), last_(0),\n> \t queueCount_(0), captureCount_(0), captureLimit_(0),\n> -\t printMetadata_(false)\n> +\t printMetadata_(false), allocator_(nullptr)\n> {\n> }\n> \n> -int CameraSession::run(const OptionsParser::Options &options)\n> +int CameraSession::start(const OptionsParser::Options &options)\n> {\n> \tint ret;\n> \n> @@ -61,36 +61,39 @@ int CameraSession::run(const OptionsParser::Options &options)\n> \t\t\twriter_ = new BufferWriter();\n> \t}\n> \n> -\tFrameBufferAllocator *allocator = new FrameBufferAllocator(camera_);\n> +\tallocator_ = new FrameBufferAllocator(camera_);\n> \n> -\tret = capture(allocator);\n> +\treturn startCapture();\n> +}\n> \n> -\tif (options.isSet(OptFile)) {\n> -\t\tdelete writer_;\n> -\t\twriter_ = nullptr;\n> -\t}\n> +void CameraSession::stop()\n> +{\n> +\tint ret = camera_->stop();\n> +\tif (ret)\n> +\t\tstd::cout << \"Failed to stop capture\" << std::endl;\n> +\n> +\tdelete writer_;\n> +\twriter_ = nullptr;\n> \n> \trequests_.clear();\n> \n> -\tdelete allocator;\n> -\n> -\treturn ret;\n> +\tdelete allocator_;\n> }\n> \n> -int CameraSession::capture(FrameBufferAllocator *allocator)\n> +int CameraSession::startCapture()\n> {\n> \tint ret;\n> \n> \t/* Identify the stream with the least number of buffers. */\n> \tunsigned int nbuffers = UINT_MAX;\n> \tfor (StreamConfiguration &cfg : *config_) {\n> -\t\tret = allocator->allocate(cfg.stream());\n> +\t\tret = allocator_->allocate(cfg.stream());\n> \t\tif (ret < 0) {\n> \t\t\tstd::cerr << \"Can't allocate buffers\" << std::endl;\n> \t\t\treturn -ENOMEM;\n> \t\t}\n> \n> -\t\tunsigned int allocated = allocator->buffers(cfg.stream()).size();\n> +\t\tunsigned int allocated = allocator_->buffers(cfg.stream()).size();\n> \t\tnbuffers = std::min(nbuffers, allocated);\n> \t}\n> \n> @@ -109,7 +112,7 @@ int CameraSession::capture(FrameBufferAllocator *allocator)\n> \t\tfor (StreamConfiguration &cfg : *config_) {\n> \t\t\tStream *stream = cfg.stream();\n> \t\t\tconst std::vector> &buffers =\n> -\t\t\t\tallocator->buffers(stream);\n> +\t\t\t\tallocator_->buffers(stream);\n> \t\t\tconst std::unique_ptr &buffer = buffers[i];\n> \n> \t\t\tret = request->addBuffer(stream, buffer.get());\n> @@ -146,15 +149,7 @@ int CameraSession::capture(FrameBufferAllocator *allocator)\n> \telse\n> \t\tstd::cout << \"Capture until user interrupts by SIGINT\" << std::endl;\n> \n> -\tret = EventLoop::instance()->exec();\n\nWithout the context of the rest of the file at this point, is this the\nlast EventLoop access?\n\nI guess we're still waiting on the removal of\n\n> +\tEventLoop::instance()->callLater([=]() { processRequest(request); });\n\nfrom 13/30 before we can remove event_loop.h from the headers.\n\nReviewed-by: Kieran Bingham \n\n\n> -\tif (ret)\n> -\t\tstd::cout << \"Failed to run capture loop\" << std::endl;\n> -\n> -\tret = camera_->stop();\n> -\tif (ret)\n> -\t\tstd::cout << \"Failed to stop capture\" << std::endl;\n> -\n> -\treturn ret;\n> +\treturn 0;\n> }\n> \n> int CameraSession::queueRequest(Request *request)\n> diff --git a/src/cam/camera_session.h b/src/cam/camera_session.h\n> index 2728d7607db2..5131cfd48b4e 100644\n> --- a/src/cam/camera_session.h\n> +++ b/src/cam/camera_session.h\n> @@ -28,12 +28,13 @@ public:\n> \tCameraSession(std::shared_ptr camera,\n> \t\t libcamera::CameraConfiguration *config);\n> \n> -\tint run(const OptionsParser::Options &options);\n> +\tint start(const OptionsParser::Options &options);\n> +\tvoid stop();\n> \n> \tlibcamera::Signal<> captureDone;\n> \n> private:\n> -\tint capture(libcamera::FrameBufferAllocator *allocator);\n> +\tint startCapture();\n> \n> \tint queueRequest(libcamera::Request *request);\n> \tvoid requestComplete(libcamera::Request *request);\n> @@ -51,6 +52,7 @@ private:\n> \tunsigned int captureLimit_;\n> \tbool printMetadata_;\n> \n> +\tlibcamera::FrameBufferAllocator *allocator_;\n> \tstd::vector> requests_;\n> };\n> \n> diff --git a/src/cam/main.cpp b/src/cam/main.cpp\n> index a567a7cc7653..b13230c374ad 100644\n> --- a/src/cam/main.cpp\n> +++ b/src/cam/main.cpp\n> @@ -371,7 +371,17 @@ int CamApp::run()\n> \tif (options_.isSet(OptCapture)) {\n> \t\tCameraSession session(camera_, config_.get());\n> \t\tsession.captureDone.connect(this, &CamApp::captureDone);\n> -\t\treturn session.run(options_);\n> +\n> +\t\tret = session.start(options_);\n> +\t\tif (ret) {\n> +\t\t\tstd::cout << \"Failed to start camera session\" << std::endl;\n> +\t\t\treturn ret;\n> +\t\t}\n> +\n> +\t\tloop_.exec();\n> +\n> +\t\tsession.stop();\n> +\t\treturn 0;\n> \t}\n> \n> \tif (options_.isSet(OptMonitor)) {\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 28D6FC3226\n\tfor ;\n\tMon, 12 Jul 2021 15:25:13 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 8C0FD68526;\n\tMon, 12 Jul 2021 17:25:12 +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 D187168513\n\tfor ;\n\tMon, 12 Jul 2021 17:25:10 +0200 (CEST)","from [192.168.0.20]\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 54FDBCC;\n\tMon, 12 Jul 2021 17:25:10 +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=\"AthCx2z7\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1626103510;\n\tbh=Ee7LjbR4mPVQbqPB862CwRCJZBsRH4mawRGq4BKOyeA=;\n\th=To:References:From:Subject:Date:In-Reply-To:From;\n\tb=AthCx2z7/nYNNYMbEp6+1VCv/0oKkujifzZ9VHNcYsFKTF65qJJzMb7V4F2h5Rsbt\n\tZwAma/VfTAu9VTrQ09fVOa7YJ8/dMxJN9wna220+XNB5T1ZkjHtQUw70TSounQGATo\n\t4B6bVRrao6KcFxWgNw0Njy9MdQAVUYf9mcSvnVsU=","To":"Laurent Pinchart ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20210707021941.20804-1-laurent.pinchart@ideasonboard.com>\n\t<20210707021941.20804-16-laurent.pinchart@ideasonboard.com>","From":"Kieran Bingham ","Message-ID":"","Date":"Mon, 12 Jul 2021 16:25:07 +0100","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.11.0","MIME-Version":"1.0","In-Reply-To":"<20210707021941.20804-16-laurent.pinchart@ideasonboard.com>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-GB","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH 15/30] cam: Move event loop execution\n\tfrom CameraSession to CamApp","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":18147,"web_url":"https://patchwork.libcamera.org/comment/18147/","msgid":"","date":"2021-07-12T18:37:11","subject":"Re: [libcamera-devel] [PATCH 15/30] cam: Move event loop execution\n\tfrom CameraSession to CamApp","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Kieran,\n\nOn Mon, Jul 12, 2021 at 04:25:07PM +0100, Kieran Bingham wrote:\n> On 07/07/2021 03:19, Laurent Pinchart wrote:\n> > To prepare for multiple concurrent camera sessions, move the event loop\n> > exec() call from the CameraSession class to the CamApp class.\n> \n> There we go - now that global loop access is becoming clearer.\n> \n> > Signed-off-by: Laurent Pinchart \n> > ---\n> > src/cam/camera_session.cpp | 43 +++++++++++++++++---------------------\n> > src/cam/camera_session.h | 6 ++++--\n> > src/cam/main.cpp | 12 ++++++++++-\n> > 3 files changed, 34 insertions(+), 27 deletions(-)\n> > \n> > diff --git a/src/cam/camera_session.cpp b/src/cam/camera_session.cpp\n> > index 16c1c66a285a..b1200e60329c 100644\n> > --- a/src/cam/camera_session.cpp\n> > +++ b/src/cam/camera_session.cpp\n> > @@ -22,11 +22,11 @@ CameraSession::CameraSession(std::shared_ptr camera,\n> > \t\t\t CameraConfiguration *config)\n> > \t: camera_(camera), config_(config), writer_(nullptr), last_(0),\n> > \t queueCount_(0), captureCount_(0), captureLimit_(0),\n> > -\t printMetadata_(false)\n> > +\t printMetadata_(false), allocator_(nullptr)\n> > {\n> > }\n> > \n> > -int CameraSession::run(const OptionsParser::Options &options)\n> > +int CameraSession::start(const OptionsParser::Options &options)\n> > {\n> > \tint ret;\n> > \n> > @@ -61,36 +61,39 @@ int CameraSession::run(const OptionsParser::Options &options)\n> > \t\t\twriter_ = new BufferWriter();\n> > \t}\n> > \n> > -\tFrameBufferAllocator *allocator = new FrameBufferAllocator(camera_);\n> > +\tallocator_ = new FrameBufferAllocator(camera_);\n> > \n> > -\tret = capture(allocator);\n> > +\treturn startCapture();\n> > +}\n> > \n> > -\tif (options.isSet(OptFile)) {\n> > -\t\tdelete writer_;\n> > -\t\twriter_ = nullptr;\n> > -\t}\n> > +void CameraSession::stop()\n> > +{\n> > +\tint ret = camera_->stop();\n> > +\tif (ret)\n> > +\t\tstd::cout << \"Failed to stop capture\" << std::endl;\n> > +\n> > +\tdelete writer_;\n> > +\twriter_ = nullptr;\n> > \n> > \trequests_.clear();\n> > \n> > -\tdelete allocator;\n> > -\n> > -\treturn ret;\n> > +\tdelete allocator_;\n> > }\n> > \n> > -int CameraSession::capture(FrameBufferAllocator *allocator)\n> > +int CameraSession::startCapture()\n> > {\n> > \tint ret;\n> > \n> > \t/* Identify the stream with the least number of buffers. */\n> > \tunsigned int nbuffers = UINT_MAX;\n> > \tfor (StreamConfiguration &cfg : *config_) {\n> > -\t\tret = allocator->allocate(cfg.stream());\n> > +\t\tret = allocator_->allocate(cfg.stream());\n> > \t\tif (ret < 0) {\n> > \t\t\tstd::cerr << \"Can't allocate buffers\" << std::endl;\n> > \t\t\treturn -ENOMEM;\n> > \t\t}\n> > \n> > -\t\tunsigned int allocated = allocator->buffers(cfg.stream()).size();\n> > +\t\tunsigned int allocated = allocator_->buffers(cfg.stream()).size();\n> > \t\tnbuffers = std::min(nbuffers, allocated);\n> > \t}\n> > \n> > @@ -109,7 +112,7 @@ int CameraSession::capture(FrameBufferAllocator *allocator)\n> > \t\tfor (StreamConfiguration &cfg : *config_) {\n> > \t\t\tStream *stream = cfg.stream();\n> > \t\t\tconst std::vector> &buffers =\n> > -\t\t\t\tallocator->buffers(stream);\n> > +\t\t\t\tallocator_->buffers(stream);\n> > \t\t\tconst std::unique_ptr &buffer = buffers[i];\n> > \n> > \t\t\tret = request->addBuffer(stream, buffer.get());\n> > @@ -146,15 +149,7 @@ int CameraSession::capture(FrameBufferAllocator *allocator)\n> > \telse\n> > \t\tstd::cout << \"Capture until user interrupts by SIGINT\" << std::endl;\n> > \n> > -\tret = EventLoop::instance()->exec();\n> \n> Without the context of the rest of the file at this point, is this the\n> last EventLoop access?\n> \n> I guess we're still waiting on the removal of\n> \n> > +\tEventLoop::instance()->callLater([=]() { processRequest(request); });\n> \n> from 13/30 before we can remove event_loop.h from the headers.\n\nThis one stays, at least for now. We could probably change that, but\nit's a common pattern to access the global event loop through a\nsingleton accessor. As this is specific to the cam application (qcam\nprobably has similar patterns, based on the Qt API that allows the same)\nin any case, I think it's acceptable.\n\n> Reviewed-by: Kieran Bingham \n> \n> > -\tif (ret)\n> > -\t\tstd::cout << \"Failed to run capture loop\" << std::endl;\n> > -\n> > -\tret = camera_->stop();\n> > -\tif (ret)\n> > -\t\tstd::cout << \"Failed to stop capture\" << std::endl;\n> > -\n> > -\treturn ret;\n> > +\treturn 0;\n> > }\n> > \n> > int CameraSession::queueRequest(Request *request)\n> > diff --git a/src/cam/camera_session.h b/src/cam/camera_session.h\n> > index 2728d7607db2..5131cfd48b4e 100644\n> > --- a/src/cam/camera_session.h\n> > +++ b/src/cam/camera_session.h\n> > @@ -28,12 +28,13 @@ public:\n> > \tCameraSession(std::shared_ptr camera,\n> > \t\t libcamera::CameraConfiguration *config);\n> > \n> > -\tint run(const OptionsParser::Options &options);\n> > +\tint start(const OptionsParser::Options &options);\n> > +\tvoid stop();\n> > \n> > \tlibcamera::Signal<> captureDone;\n> > \n> > private:\n> > -\tint capture(libcamera::FrameBufferAllocator *allocator);\n> > +\tint startCapture();\n> > \n> > \tint queueRequest(libcamera::Request *request);\n> > \tvoid requestComplete(libcamera::Request *request);\n> > @@ -51,6 +52,7 @@ private:\n> > \tunsigned int captureLimit_;\n> > \tbool printMetadata_;\n> > \n> > +\tlibcamera::FrameBufferAllocator *allocator_;\n> > \tstd::vector> requests_;\n> > };\n> > \n> > diff --git a/src/cam/main.cpp b/src/cam/main.cpp\n> > index a567a7cc7653..b13230c374ad 100644\n> > --- a/src/cam/main.cpp\n> > +++ b/src/cam/main.cpp\n> > @@ -371,7 +371,17 @@ int CamApp::run()\n> > \tif (options_.isSet(OptCapture)) {\n> > \t\tCameraSession session(camera_, config_.get());\n> > \t\tsession.captureDone.connect(this, &CamApp::captureDone);\n> > -\t\treturn session.run(options_);\n> > +\n> > +\t\tret = session.start(options_);\n> > +\t\tif (ret) {\n> > +\t\t\tstd::cout << \"Failed to start camera session\" << std::endl;\n> > +\t\t\treturn ret;\n> > +\t\t}\n> > +\n> > +\t\tloop_.exec();\n> > +\n> > +\t\tsession.stop();\n> > +\t\treturn 0;\n> > \t}\n> > \n> > \tif (options_.isSet(OptMonitor)) {","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 95216C3225\n\tfor ;\n\tMon, 12 Jul 2021 18:38:00 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E724568524;\n\tMon, 12 Jul 2021 20:37:59 +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 BE63D68513\n\tfor ;\n\tMon, 12 Jul 2021 20:37:58 +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 27F5BCC;\n\tMon, 12 Jul 2021 20:37:58 +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=\"r0GgAIsO\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1626115078;\n\tbh=G+YwMA9JkOT/YcRrzeW8JwQ25oKmmWwYyyDsrXtGuYo=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=r0GgAIsOF8qRazexYVaEjncwSAjbzG3HTJR8OiolJSgzgmGddwaY8yMxH58qlUoOR\n\tCaDb/VaXgp0e780q9ykwR6H58xobUFtpax39VvgdRt9Zbj2yt1/fbb5qOJlfGEgAo9\n\tpt3Ut7k6m4H66UTLtJT3cIXPoJIvIUSO6biko7is=","Date":"Mon, 12 Jul 2021 21:37:11 +0300","From":"Laurent Pinchart ","To":"Kieran Bingham ","Message-ID":"","References":"<20210707021941.20804-1-laurent.pinchart@ideasonboard.com>\n\t<20210707021941.20804-16-laurent.pinchart@ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH 15/30] cam: Move event loop execution\n\tfrom CameraSession to CamApp","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]