[{"id":18454,"web_url":"https://patchwork.libcamera.org/comment/18454/","msgid":"","date":"2021-08-01T20:54:48","subject":"Re: [libcamera-devel] [PATCH v7 04/11] lc-compliance: Factor common\n\tcapture code into SimpleCapture","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Nícolas,\n\nThank you for the patch.\n\nOn Thu, Jul 22, 2021 at 08:28:44PM -0300, Nícolas F. R. A. Prado wrote:\n> Factor common code from SimpleCapture* subclasses into the SimpleCapture\n> parent class in order to avoid duplicated code.\n> \n> Signed-off-by: Nícolas F. R. A. Prado \n> ---\n> \n> No changes in v7\n> \n> Changes in v6:\n> - Added missing blank line before return in captureCompleted()\n> - Switched queueRequests()'s 'buffers' and 'requests' parameters order, since\n> 'requests' is an output variable\n> - Added comment and blank line to runCaptureSession() \n> \n> src/lc-compliance/simple_capture.cpp | 101 ++++++++++++++++-----------\n> src/lc-compliance/simple_capture.h | 16 +++--\n> 2 files changed, 72 insertions(+), 45 deletions(-)\n> \n> diff --git a/src/lc-compliance/simple_capture.cpp b/src/lc-compliance/simple_capture.cpp\n> index 8683d9050806..06ef44ef7e42 100644\n> --- a/src/lc-compliance/simple_capture.cpp\n> +++ b/src/lc-compliance/simple_capture.cpp\n> @@ -78,6 +78,45 @@ void SimpleCapture::stop()\n> \tallocator_->free(stream);\n> }\n> \n> +bool SimpleCapture::captureCompleted()\n> +{\n> +\tcaptureCount_++;\n> +\tif (captureCount_ >= captureLimit_) {\n> +\t\tloop_->exit(0);\n> +\t\treturn true;\n> +\t}\n> +\n> +\treturn false;\n> +}\n> +\n> +void SimpleCapture::queueRequests(Stream *stream,\n> +\t\t\t\t const std::vector> &buffers,\n> +\t\t\t\t std::vector> &requests)\n\nThe fact that the caller is supposed to pass a vector of requests and\nkeep it valid for the duration of the capture session is a bit\nconfusing, especially without documentation. One option would be to\nstore the requests in a member variable that you could clear in stop().\n\n> +{\n> +\tfor (const std::unique_ptr &buffer : buffers) {\n> +\t\tstd::unique_ptr request = camera_->createRequest();\n> +\t\tASSERT_TRUE(request) << \"Can't create request\";\n> +\n> +\t\tASSERT_EQ(request->addBuffer(stream, buffer.get()), 0) << \"Can't set buffer for request\";\n> +\n> +\t\tASSERT_EQ(camera_->queueRequest(request.get()), 0) << \"Failed to queue request\";\n\nIf you added a\n\n\tvirtual int queueRequest(Request *request);\n\nimplemented as\n\nint SimpleCapture::queueRequest(Request *request)\n{\n\treturn camera_->queueRequest(request);\n}\n\nand called queueRequest() here instead of camera_->queueRequest(), you\nwouldn't need to implement a separate\nSimpleCaptureBalanced::queueRequests().\n\n> +\n> +\t\trequests.push_back(std::move(request));\n> +\t}\n> +}\n> +\n> +int SimpleCapture::runCaptureSession()\n> +{\n> +\tloop_ = new EventLoop();\n> +\tint status = loop_->exec();\n> +\n> +\t/* After session ended */\n> +\tstop();\n> +\tdelete loop_;\n> +\n> +\treturn status;\n> +}\n> +\n> /* SimpleCaptureBalanced */\n> \n> SimpleCaptureBalanced::SimpleCaptureBalanced(std::shared_ptr camera)\n> @@ -85,6 +124,22 @@ SimpleCaptureBalanced::SimpleCaptureBalanced(std::shared_ptr camera)\n> {\n> }\n> \n> +void SimpleCaptureBalanced::queueRequests(Stream *stream,\n> +\t\t\t\t\t const std::vector> &buffers,\n> +\t\t\t\t\t std::vector> &requests)\n> +{\n> +\tfor (const std::unique_ptr &buffer : buffers) {\n> +\t\tstd::unique_ptr request = camera_->createRequest();\n> +\t\tASSERT_TRUE(request) << \"Can't create request\";\n> +\n> +\t\tASSERT_EQ(request->addBuffer(stream, buffer.get()), 0) << \"Can't set buffer for request\";\n> +\n> +\t\tASSERT_EQ(queueRequest(request.get()), 0) << \"Failed to queue request\";\n> +\n> +\t\trequests.push_back(std::move(request));\n> +\t}\n> +}\n> +\n> void SimpleCaptureBalanced::capture(unsigned int numRequests)\n> {\n> \tstart();\n> @@ -104,24 +159,10 @@ void SimpleCaptureBalanced::capture(unsigned int numRequests)\n> \tcaptureCount_ = 0;\n> \tcaptureLimit_ = numRequests;\n> \n> -\t/* Queue the recommended number of reqeuests. */\n> \tstd::vector> requests;\n> -\tfor (const std::unique_ptr &buffer : buffers) {\n> -\t\tstd::unique_ptr request = camera_->createRequest();\n> -\t\tASSERT_TRUE(request) << \"Can't create request\";\n> -\n> -\t\tASSERT_EQ(request->addBuffer(stream, buffer.get()), 0) << \"Can't set buffer for request\";\n> -\n> -\t\tASSERT_EQ(queueRequest(request.get()), 0) << \"Failed to queue request\";\n> +\tqueueRequests(stream, buffers, requests);\n> \n> -\t\trequests.push_back(std::move(request));\n> -\t}\n> -\n> -\t/* Run capture session. */\n> -\tloop_ = new EventLoop();\n> -\tloop_->exec();\n> -\tstop();\n> -\tdelete loop_;\n> +\trunCaptureSession();\n> \n> \tASSERT_EQ(captureCount_, captureLimit_);\n> }\n> @@ -137,11 +178,8 @@ int SimpleCaptureBalanced::queueRequest(Request *request)\n> \n> void SimpleCaptureBalanced::requestComplete(Request *request)\n> {\n> -\tcaptureCount_++;\n> -\tif (captureCount_ >= captureLimit_) {\n> -\t\tloop_->exit(0);\n> +\tif (captureCompleted())\n> \t\treturn;\n> -\t}\n> \n> \trequest->reuse(Request::ReuseBuffers);\n> \tif (queueRequest(request))\n> @@ -165,35 +203,18 @@ void SimpleCaptureUnbalanced::capture(unsigned int numRequests)\n> \tcaptureCount_ = 0;\n> \tcaptureLimit_ = numRequests;\n> \n> -\t/* Queue the recommended number of reqeuests. */\n> \tstd::vector> requests;\n> -\tfor (const std::unique_ptr &buffer : buffers) {\n> -\t\tstd::unique_ptr request = camera_->createRequest();\n> -\t\tASSERT_TRUE(request) << \"Can't create request\";\n> -\n> -\t\tASSERT_EQ(request->addBuffer(stream, buffer.get()), 0) << \"Can't set buffer for request\";\n> -\n> -\t\tASSERT_EQ(camera_->queueRequest(request.get()), 0) << \"Failed to queue request\";\n> +\tqueueRequests(stream, buffers, requests);\n> \n> -\t\trequests.push_back(std::move(request));\n> -\t}\n> -\n> -\t/* Run capture session. */\n> -\tloop_ = new EventLoop();\n> -\tint status = loop_->exec();\n> -\tstop();\n> -\tdelete loop_;\n> +\tint status = runCaptureSession();\n> \n> \tASSERT_EQ(status, 0);\n> }\n> \n> void SimpleCaptureUnbalanced::requestComplete(Request *request)\n> {\n> -\tcaptureCount_++;\n> -\tif (captureCount_ >= captureLimit_) {\n> -\t\tloop_->exit(0);\n> +\tif (captureCompleted())\n> \t\treturn;\n> -\t}\n> \n> \trequest->reuse(Request::ReuseBuffers);\n> \tif (camera_->queueRequest(request))\n> diff --git a/src/lc-compliance/simple_capture.h b/src/lc-compliance/simple_capture.h\n> index 1a1e874a528c..0f9a060fece3 100644\n> --- a/src/lc-compliance/simple_capture.h\n> +++ b/src/lc-compliance/simple_capture.h\n> @@ -26,11 +26,19 @@ protected:\n> \n> \tvoid start();\n> \tvoid stop();\n> +\tvoid queueRequests(libcamera::Stream *stream,\n> +\t\t\t const std::vector> &buffers,\n> +\t\t\t std::vector> &requests);\n> +\tbool captureCompleted();\n> +\tint runCaptureSession();\n> \n> \tvirtual void requestComplete(libcamera::Request *request) = 0;\n> \n> \tEventLoop *loop_;\n> \n> +\tunsigned int captureCount_;\n> +\tunsigned int captureLimit_;\n> +\n> \tstd::shared_ptr camera_;\n> \tstd::unique_ptr allocator_;\n> \tstd::unique_ptr config_;\n> @@ -46,10 +54,11 @@ public:\n> private:\n> \tint queueRequest(libcamera::Request *request);\n> \tvoid requestComplete(libcamera::Request *request) override;\n> +\tvoid queueRequests(libcamera::Stream *stream,\n> +\t\t\t const std::vector> &buffers,\n> +\t\t\t std::vector> &requests);\n> \n> \tunsigned int queueCount_;\n> -\tunsigned int captureCount_;\n> -\tunsigned int captureLimit_;\n> };\n> \n> class SimpleCaptureUnbalanced : public SimpleCapture\n> @@ -61,9 +70,6 @@ public:\n> \n> private:\n> \tvoid requestComplete(libcamera::Request *request) override;\n> -\n> -\tunsigned int captureCount_;\n> -\tunsigned int captureLimit_;\n> };\n> \n> #endif /* __LC_COMPLIANCE_SIMPLE_CAPTURE_H__ */","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 5FCFABD878\n\tfor ;\n\tSun, 1 Aug 2021 20:55:01 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id CE37F687B6;\n\tSun, 1 Aug 2021 22:55:00 +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 2C83D687B1\n\tfor ;\n\tSun, 1 Aug 2021 22:54:59 +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 9216987C;\n\tSun, 1 Aug 2021 22:54: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=\"Cc1t1hjZ\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1627851298;\n\tbh=PMKgUPnSIYjWJfamNaQvH1GMahQ8fUe/PoRGr5JqATs=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=Cc1t1hjZKGIQe/wVI6kl40MD82e9kcll/WTk3mVCmdlB0qd9JuZllCs1gkCcZXmc+\n\tb51CcMxK+d5oBMtbAeFdEDHrSc62T68KMVoE+dH0BnUyoWRCz9PC3o1ow1P2SK6BeT\n\tbOOqdlX8UX5C8n64MtTIBN7pj9Hcc3Mrh9+vxBjg=","Date":"Sun, 1 Aug 2021 23:54:48 +0300","From":"Laurent Pinchart ","To":"=?utf-8?b?TsOtY29sYXMgRi4gUi4gQS4=?= Prado ","Message-ID":"","References":"<20210722232851.747614-1-nfraprado@collabora.com>\n\t<20210722232851.747614-5-nfraprado@collabora.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20210722232851.747614-5-nfraprado@collabora.com>","Subject":"Re: [libcamera-devel] [PATCH v7 04/11] lc-compliance: Factor common\n\tcapture code into SimpleCapture","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, kernel@collabora.com, =?utf-8?q?A?=\n\t=?utf-8?b?bmRyw6k=?= Almeida ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]