[{"id":19549,"web_url":"https://patchwork.libcamera.org/comment/19549/","msgid":"","date":"2021-09-08T13:00:41","subject":"Re: [libcamera-devel] [PATCH v1 4/6] android: camera_device: Add a\n\tqueue for sending capture results","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"On 07/09/2021 20:57, Umang Jain wrote:\n> When a camera capture request completes, the next step is to send the\n> capture results to the framework via process_capture_results(). All\n> the capture associated result metadata is prepared and populated. If\n> any post-processing is required, it will also take place in the same\n> thread (which can be blocking for a subtaintial amount of time) before\n\ns/subtaintial/substantial/\n\n\n> the results can be sent back to the framework.\n> \n> A follow up commit will move the post-processing to run in a separate\n> thread. In order to do so, there is few bits of groundwork which this\n> patch entails. Mainly, we need to preserve the order of sending\n> the capture results back in which they were queued by the framework\n> to the HAL (i.e. the order is sequential). Hence, we need to add a queue\n> in which capture results can be queued with context.\n> \n> As per this patch, the post-processor still runs synchronously as\n> before, but it will queue up the current capture results with context.\n> Later down the line, the capture results are dequeud in order and\n\ns/dequeud/dequeued/\n\n\n> sent back to the framework.\n> \n> The context is preserved using Camera3RequestDescriptor utility\n> structure. It has been expanded accordingly to address the needs of\n> the functionality.\n> \n> Signed-off-by: Umang Jain \n> ---\n> src/android/camera_device.cpp | 113 +++++++++++++++++++++++++++++++---\n> src/android/camera_device.h | 23 +++++++\n> 2 files changed, 126 insertions(+), 10 deletions(-)\n> \n> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> index f2f36f32..9d4ec02e 100644\n> --- a/src/android/camera_device.cpp\n> +++ b/src/android/camera_device.cpp\n> @@ -240,6 +240,8 @@ CameraDevice::Camera3RequestDescriptor::Camera3RequestDescriptor(\n> \t/* Clone the controls associated with the camera3 request. */\n> \tsettings_ = CameraMetadata(camera3Request->settings);\n> \n> +\tinternalBuffer_ = nullptr;\n> +\n> \t/*\n> \t * Create the CaptureRequest, stored as a unique_ptr<> to tie its\n> \t * lifetime to the descriptor.\n> @@ -989,6 +991,7 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n> \t\t\t * once it has been processed.\n> \t\t\t */\n> \t\t\tbuffer = cameraStream->getBuffer();\n> +\t\t\tdescriptor.internalBuffer_ = buffer;\n> \t\t\tLOG(HAL, Debug) << ss.str() << \" (internal)\";\n> \t\t\tbreak;\n> \t\t}\n> @@ -1148,25 +1151,115 @@ void CameraDevice::requestComplete(Request *request)\n> \t\t\tcontinue;\n> \t\t}\n> \n> +\t\t/*\n> +\t\t * Save the current context of capture result and queue the\n> +\t\t * descriptor before processing the camera stream.\n> +\t\t *\n> +\t\t * When the processing completes, the descriptor will be\n> +\t\t * dequeued and capture results will be sent to the framework.\n> +\t\t */\n> +\t\tdescriptor.status_ = Camera3RequestDescriptor::NOT_FINISHED;\n> +\t\tdescriptor.resultMetadata_ = std::move(resultMetadata);\n> +\t\tdescriptor.captureResult_ = captureResult;\n> +\n> +\t\tstd::unique_ptr reqDescriptor =\n> +\t\t\tstd::make_unique();\n> +\t\t*reqDescriptor = std::move(descriptor);\n> +\t\tqueuedDescriptor_.push_back(std::move(reqDescriptor));\n> +\t\tCamera3RequestDescriptor *currentDescriptor = queuedDescriptor_.back().get();\n> +\n> +\t\tCameraMetadata *metadata = currentDescriptor->resultMetadata_.get();\n> +\t\tcameraStream->processComplete.connect(\n> +\t\t\tthis, [=](CameraStream::ProcessStatus status) {\n> +\t\t\t\tstreamProcessingComplete(cameraStream, status,\n> +\t\t\t\t\t\t\t metadata);\n\nI believe this to be unsafe to manage multiple operations.\n\ncameraStream->processComplete() should not be connected with per-run\nparameters. Those should be passed as the context of the signal emit(),\nnot by the connection.\n\nOtherwise, the lamba will be overwritten by the next requestComplete()\nand incorrect parameters will be processed when the signal actually fires.\n\n\n\n> +\t\t\t});\n> +\n> \t\tint ret = cameraStream->process(src, *buffer.buffer,\n> -\t\t\t\t\t\tdescriptor.settings_,\n> -\t\t\t\t\t\tresultMetadata.get());\n> +\t\t\t\t\t\tcurrentDescriptor->settings_,\n> +\t\t\t\t\t\tmetadata);\n> +\t\treturn;\n> +\t}\n> +\n> +\tif (queuedDescriptor_.empty()) {\n> +\t\tcaptureResult.result = resultMetadata->get();\n> +\t\tcallbacks_->process_capture_result(callbacks_, &captureResult);\n\n*1 duplication of completion callbacks (see below)\n\n\n> +\t} else {\n> +\t\t/*\n> +\t\t * Previous capture results waiting to be sent to framework\n> +\t\t * hence, queue the current capture results as well. After that,\n> +\t\t * check if any results are ready to be sent back to the\n> +\t\t * framework.\n> +\t\t */\n> +\t\tdescriptor.status_ = Camera3RequestDescriptor::FINISHED_SUCCESS;\n> +\t\tdescriptor.resultMetadata_ = std::move(resultMetadata);\n> +\t\tdescriptor.captureResult_ = captureResult;\n> +\t\tstd::unique_ptr reqDescriptor =\n> +\t\t\tstd::make_unique();\n> +\t\t*reqDescriptor = std::move(descriptor);\n> +\t\tqueuedDescriptor_.push_back(std::move(reqDescriptor));\n> +\n> +\t\tsendQueuedCaptureResults();\n> +\t}\n> +}\n> +\n> +void CameraDevice::streamProcessingComplete(CameraStream *cameraStream,\n> +\t\t\t\t\t CameraStream::ProcessStatus status,\n> +\t\t\t\t\t CameraMetadata *resultMetadata)\n> +{\n> +\tfor (auto &d : queuedDescriptor_) {\n> +\t\tif (d->resultMetadata_.get() != resultMetadata)\n> +\t\t\tcontinue;\n> +\n> \t\t/*\n> \t\t * Return the FrameBuffer to the CameraStream now that we're\n> \t\t * done processing it.\n> \t\t */\n> \t\tif (cameraStream->type() == CameraStream::Type::Internal)\n> -\t\t\tcameraStream->putBuffer(src);\n> -\n> -\t\tif (ret) {\n> -\t\t\tbuffer.status = CAMERA3_BUFFER_STATUS_ERROR;\n> -\t\t\tnotifyError(descriptor.frameNumber_, buffer.stream,\n> -\t\t\t\t CAMERA3_MSG_ERROR_BUFFER);\n> +\t\t\tcameraStream->putBuffer(d->internalBuffer_);\n> +\n> +\t\tif (status == CameraStream::ProcessStatus::Success) {\n> +\t\t\td->status_ = Camera3RequestDescriptor::FINISHED_SUCCESS;\n> +\t\t} else {\n> +\t\t\td->status_ = Camera3RequestDescriptor::FINISHED_FAILED;\n> +\t\t\td->captureResult_.partial_result = 0;\n> +\t\t\tfor (camera3_stream_buffer_t &buffer : d->buffers_) {\n> +\t\t\t\tCameraStream *cs = static_cast(buffer.stream->priv);\n> +\n> +\t\t\t\tif (cs->camera3Stream().format != HAL_PIXEL_FORMAT_BLOB)\n> +\t\t\t\t\tcontinue;\n> +\n> +\t\t\t\tbuffer.status = CAMERA3_BUFFER_STATUS_ERROR;\n> +\t\t\t\tnotifyError(d->frameNumber_, buffer.stream,\n> +\t\t\t\t\t CAMERA3_MSG_ERROR_BUFFER);\n> +\t\t\t}\n> \t\t}\n> +\n> +\t\tbreak;\n> \t}\n> \n> -\tcaptureResult.result = resultMetadata->get();\n> -\tcallbacks_->process_capture_result(callbacks_, &captureResult);\n> +\t/*\n> +\t * Send back capture results to the framework by inspecting the queue.\n> +\t * The framework can defer queueing further requests to the HAL (via\n> +\t * process_capture_request) unless until it receives the capture results\n> +\t * for already queued requests.\n> +\t */\n> +\tsendQueuedCaptureResults();\n> +}\n> +\n> +void CameraDevice::sendQueuedCaptureResults()\n> +{\n> +\twhile (!queuedDescriptor_.empty()) {\n> +\t\tstd::unique_ptr &d = queuedDescriptor_.front();\n\nI'd have this exit early if the front descriptor is not finished.\n\n\n> +\t\tif (d->status_ != Camera3RequestDescriptor::NOT_FINISHED) {\n> +\t\t\td->captureResult_.result = d->resultMetadata_->get();\n> +\t\t\tcallbacks_->process_capture_result(callbacks_,\n> +\t\t\t\t\t\t\t &(d->captureResult_));\n\nNot as part of this patch, but I feel like Camera3RequestDescriptor\ncould have a method called complete() to wrap the management of the\ncompletion against a request descriptor.\n\nThere's lots of occasions where the metadata is obtained or processed\ninto a result structure and called back with a flag.\n\n(such as *1 above, but I think there are more)\n\nIt seems to me like that could be made into\n\n\t\td->complete(CameraRequest::Success);\n\nor something.... But that's an overall design of breaking out the\nCamera3RequestDescriptor to a full class.\n\nAs this series is extending Camera3RequestDescriptor to maintain extra\nstate required for the asynchronous post-processing, that makes me think\nit would be better to have a full class to manage that state. (and thus\nmethods to operate on it)·\n\n\n\n> +\t\t\tqueuedDescriptor_.pop_front();\n> +\t\t} else {\n> +\t\t\tbreak;\n> +\t\t}\n> +\t}\n> }\n> \n> std::string CameraDevice::logPrefix() const\n> diff --git a/src/android/camera_device.h b/src/android/camera_device.h\n> index a5576927..36425773 100644\n> --- a/src/android/camera_device.h\n> +++ b/src/android/camera_device.h\n> @@ -7,6 +7,7 @@\n> #ifndef __ANDROID_CAMERA_DEVICE_H__\n> #define __ANDROID_CAMERA_DEVICE_H__\n> \n> +#include \n> #include \n> #include \n> #include \n> @@ -83,6 +84,21 @@ private:\n> \t\tstd::vector> frameBuffers_;\n> \t\tCameraMetadata settings_;\n> \t\tstd::unique_ptr request_;\n> +\n> +\t\t/*\n> +\t\t * Below are utility placeholders used when a capture result\n> +\t\t * needs to be queued before completion via\n> +\t\t * process_capture_result() callback.\n> +\t\t */\n> +\t\tenum completionStatus {\n> +\t\t\tNOT_FINISHED,\n\nI'd call this \"Active\" or \"Pending\" to reduce the negation...\n\n\t\"If (state == Active)\"\nrather than\n\t\"if (state != NOT_FINISHED)\"\n\n\n> +\t\t\tFINISHED_SUCCESS,\n\nI'd call this Success or Succeeded, or Successful ..\n\n> +\t\t\tFINISHED_FAILED,\n\nAnd Failed\n\n\n\n> +\t\t};\n> +\t\tstd::unique_ptr resultMetadata_;\n> +\t\tcamera3_capture_result_t captureResult_;\n> +\t\tlibcamera::FrameBuffer *internalBuffer_;\n> +\t\tcompletionStatus status_;\n\nThese are all used to track the post-processor context.\n\n\n\n\n> \t};\n> \n> \tenum class State {\n> @@ -105,6 +121,11 @@ private:\n> \tstd::unique_ptr getResultMetadata(\n> \t\tconst Camera3RequestDescriptor &descriptor) const;\n> \n> +\tvoid sendQueuedCaptureResults();\n> +\tvoid streamProcessingComplete(CameraStream *stream,\n> +\t\t\t\t CameraStream::ProcessStatus status,\n> +\t\t\t\t CameraMetadata *resultMetadata);\n> +\n> \tunsigned int id_;\n> \tcamera3_device_t camera3Device_;\n> \n> @@ -125,6 +146,8 @@ private:\n> \tlibcamera::Mutex descriptorsMutex_; /* Protects descriptors_. */\n> \tstd::map descriptors_;\n> \n> +\tstd::deque> queuedDescriptor_;\n\nI'm not yet sure if this should be a unique_ptr ..\n\n\n> +\n> \tstd::string maker_;\n> \tstd::string model_;\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 30C6FBDB1D\n\tfor ;\n\tWed, 8 Sep 2021 13:00:47 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9FD316916E;\n\tWed, 8 Sep 2021 15:00:46 +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 D611060503\n\tfor ;\n\tWed, 8 Sep 2021 15:00:44 +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 1F462993;\n\tWed, 8 Sep 2021 15:00:44 +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=\"lI3s5pkL\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1631106044;\n\tbh=kO1ajmaR27BMMS2a2R76vgSjD5gCI6VOqgyMXOm8MSo=;\n\th=To:References:From:Subject:Date:In-Reply-To:From;\n\tb=lI3s5pkLVKghZnDyh4K53E3hZgp7bYfzhehCSg23kuhsswUq2k/EvrKpeU6kvgtiy\n\ttFPdGeD+fnHHTjO2Ee/1VCeanKqX6FAReqC3Cxg7Jf9sVYx/5W/gy8P7MK/yQyupnJ\n\ty5PyLJ+gZpEwkFTkHdRLEtb6mYGsifCCRxgxfb2Y=","To":"Umang Jain ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20210907195704.338079-1-umang.jain@ideasonboard.com>\n\t<20210907195704.338079-5-umang.jain@ideasonboard.com>","From":"Kieran Bingham ","Message-ID":"","Date":"Wed, 8 Sep 2021 14:00:41 +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":"<20210907195704.338079-5-umang.jain@ideasonboard.com>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-GB","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH v1 4/6] android: camera_device: Add a\n\tqueue for sending capture results","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":19555,"web_url":"https://patchwork.libcamera.org/comment/19555/","msgid":"","date":"2021-09-08T13:33:26","subject":"Re: [libcamera-devel] [PATCH v1 4/6] android: camera_device: Add a\n\tqueue for sending capture results","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Wed, Sep 08, 2021 at 02:00:41PM +0100, Kieran Bingham wrote:\n> On 07/09/2021 20:57, Umang Jain wrote:\n> > When a camera capture request completes, the next step is to send the\n> > capture results to the framework via process_capture_results(). All\n> > the capture associated result metadata is prepared and populated. If\n> > any post-processing is required, it will also take place in the same\n> > thread (which can be blocking for a subtaintial amount of time) before\n> \n> s/subtaintial/substantial/\n> \n> > the results can be sent back to the framework.\n> > \n> > A follow up commit will move the post-processing to run in a separate\n> > thread. In order to do so, there is few bits of groundwork which this\n> > patch entails. Mainly, we need to preserve the order of sending\n> > the capture results back in which they were queued by the framework\n> > to the HAL (i.e. the order is sequential). Hence, we need to add a queue\n> > in which capture results can be queued with context.\n> > \n> > As per this patch, the post-processor still runs synchronously as\n> > before, but it will queue up the current capture results with context.\n> > Later down the line, the capture results are dequeud in order and\n> \n> s/dequeud/dequeued/\n> \n> > sent back to the framework.\n> > \n> > The context is preserved using Camera3RequestDescriptor utility\n> > structure. It has been expanded accordingly to address the needs of\n> > the functionality.\n> > \n> > Signed-off-by: Umang Jain \n> > ---\n> > src/android/camera_device.cpp | 113 +++++++++++++++++++++++++++++++---\n> > src/android/camera_device.h | 23 +++++++\n> > 2 files changed, 126 insertions(+), 10 deletions(-)\n> > \n> > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> > index f2f36f32..9d4ec02e 100644\n> > --- a/src/android/camera_device.cpp\n> > +++ b/src/android/camera_device.cpp\n> > @@ -240,6 +240,8 @@ CameraDevice::Camera3RequestDescriptor::Camera3RequestDescriptor(\n> > \t/* Clone the controls associated with the camera3 request. */\n> > \tsettings_ = CameraMetadata(camera3Request->settings);\n> > \n> > +\tinternalBuffer_ = nullptr;\n> > +\n> > \t/*\n> > \t * Create the CaptureRequest, stored as a unique_ptr<> to tie its\n> > \t * lifetime to the descriptor.\n> > @@ -989,6 +991,7 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n> > \t\t\t * once it has been processed.\n> > \t\t\t */\n> > \t\t\tbuffer = cameraStream->getBuffer();\n> > +\t\t\tdescriptor.internalBuffer_ = buffer;\n> > \t\t\tLOG(HAL, Debug) << ss.str() << \" (internal)\";\n> > \t\t\tbreak;\n> > \t\t}\n> > @@ -1148,25 +1151,115 @@ void CameraDevice::requestComplete(Request *request)\n> > \t\t\tcontinue;\n> > \t\t}\n> > \n> > +\t\t/*\n> > +\t\t * Save the current context of capture result and queue the\n> > +\t\t * descriptor before processing the camera stream.\n> > +\t\t *\n> > +\t\t * When the processing completes, the descriptor will be\n> > +\t\t * dequeued and capture results will be sent to the framework.\n> > +\t\t */\n> > +\t\tdescriptor.status_ = Camera3RequestDescriptor::NOT_FINISHED;\n> > +\t\tdescriptor.resultMetadata_ = std::move(resultMetadata);\n> > +\t\tdescriptor.captureResult_ = captureResult;\n> > +\n> > +\t\tstd::unique_ptr reqDescriptor =\n> > +\t\t\tstd::make_unique();\n> > +\t\t*reqDescriptor = std::move(descriptor);\n> > +\t\tqueuedDescriptor_.push_back(std::move(reqDescriptor));\n> > +\t\tCamera3RequestDescriptor *currentDescriptor = queuedDescriptor_.back().get();\n> > +\n> > +\t\tCameraMetadata *metadata = currentDescriptor->resultMetadata_.get();\n> > +\t\tcameraStream->processComplete.connect(\n> > +\t\t\tthis, [=](CameraStream::ProcessStatus status) {\n> > +\t\t\t\tstreamProcessingComplete(cameraStream, status,\n> > +\t\t\t\t\t\t\t metadata);\n> \n> I believe this to be unsafe to manage multiple operations.\n> \n> cameraStream->processComplete() should not be connected with per-run\n> parameters. Those should be passed as the context of the signal emit(),\n> not by the connection.\n> \n> Otherwise, the lamba will be overwritten by the next requestComplete()\n> and incorrect parameters will be processed when the signal actually fires.\n\nIt's worse than that, ever connect() will create a new connection, so\nthe slot will be called once for the first request, twice for the\nsecond, ...\n\n> > +\t\t\t});\n> > +\n> > \t\tint ret = cameraStream->process(src, *buffer.buffer,\n> > -\t\t\t\t\t\tdescriptor.settings_,\n> > -\t\t\t\t\t\tresultMetadata.get());\n> > +\t\t\t\t\t\tcurrentDescriptor->settings_,\n> > +\t\t\t\t\t\tmetadata);\n> > +\t\treturn;\n> > +\t}\n> > +\n> > +\tif (queuedDescriptor_.empty()) {\n> > +\t\tcaptureResult.result = resultMetadata->get();\n> > +\t\tcallbacks_->process_capture_result(callbacks_, &captureResult);\n> \n> *1 duplication of completion callbacks (see below)\n> \n> > +\t} else {\n> > +\t\t/*\n> > +\t\t * Previous capture results waiting to be sent to framework\n> > +\t\t * hence, queue the current capture results as well. After that,\n> > +\t\t * check if any results are ready to be sent back to the\n> > +\t\t * framework.\n> > +\t\t */\n> > +\t\tdescriptor.status_ = Camera3RequestDescriptor::FINISHED_SUCCESS;\n> > +\t\tdescriptor.resultMetadata_ = std::move(resultMetadata);\n> > +\t\tdescriptor.captureResult_ = captureResult;\n> > +\t\tstd::unique_ptr reqDescriptor =\n> > +\t\t\tstd::make_unique();\n> > +\t\t*reqDescriptor = std::move(descriptor);\n> > +\t\tqueuedDescriptor_.push_back(std::move(reqDescriptor));\n> > +\n> > +\t\tsendQueuedCaptureResults();\n> > +\t}\n> > +}\n> > +\n> > +void CameraDevice::streamProcessingComplete(CameraStream *cameraStream,\n> > +\t\t\t\t\t CameraStream::ProcessStatus status,\n> > +\t\t\t\t\t CameraMetadata *resultMetadata)\n> > +{\n> > +\tfor (auto &d : queuedDescriptor_) {\n> > +\t\tif (d->resultMetadata_.get() != resultMetadata)\n> > +\t\t\tcontinue;\n> > +\n> > \t\t/*\n> > \t\t * Return the FrameBuffer to the CameraStream now that we're\n> > \t\t * done processing it.\n> > \t\t */\n> > \t\tif (cameraStream->type() == CameraStream::Type::Internal)\n> > -\t\t\tcameraStream->putBuffer(src);\n> > -\n> > -\t\tif (ret) {\n> > -\t\t\tbuffer.status = CAMERA3_BUFFER_STATUS_ERROR;\n> > -\t\t\tnotifyError(descriptor.frameNumber_, buffer.stream,\n> > -\t\t\t\t CAMERA3_MSG_ERROR_BUFFER);\n> > +\t\t\tcameraStream->putBuffer(d->internalBuffer_);\n> > +\n> > +\t\tif (status == CameraStream::ProcessStatus::Success) {\n> > +\t\t\td->status_ = Camera3RequestDescriptor::FINISHED_SUCCESS;\n> > +\t\t} else {\n> > +\t\t\td->status_ = Camera3RequestDescriptor::FINISHED_FAILED;\n> > +\t\t\td->captureResult_.partial_result = 0;\n> > +\t\t\tfor (camera3_stream_buffer_t &buffer : d->buffers_) {\n> > +\t\t\t\tCameraStream *cs = static_cast(buffer.stream->priv);\n> > +\n> > +\t\t\t\tif (cs->camera3Stream().format != HAL_PIXEL_FORMAT_BLOB)\n> > +\t\t\t\t\tcontinue;\n> > +\n> > +\t\t\t\tbuffer.status = CAMERA3_BUFFER_STATUS_ERROR;\n> > +\t\t\t\tnotifyError(d->frameNumber_, buffer.stream,\n> > +\t\t\t\t\t CAMERA3_MSG_ERROR_BUFFER);\n> > +\t\t\t}\n> > \t\t}\n> > +\n> > +\t\tbreak;\n> > \t}\n> > \n> > -\tcaptureResult.result = resultMetadata->get();\n> > -\tcallbacks_->process_capture_result(callbacks_, &captureResult);\n> > +\t/*\n> > +\t * Send back capture results to the framework by inspecting the queue.\n> > +\t * The framework can defer queueing further requests to the HAL (via\n> > +\t * process_capture_request) unless until it receives the capture results\n> > +\t * for already queued requests.\n> > +\t */\n> > +\tsendQueuedCaptureResults();\n> > +}\n> > +\n> > +void CameraDevice::sendQueuedCaptureResults()\n> > +{\n> > +\twhile (!queuedDescriptor_.empty()) {\n> > +\t\tstd::unique_ptr &d = queuedDescriptor_.front();\n> \n> I'd have this exit early if the front descriptor is not finished.\n> \n> \n> > +\t\tif (d->status_ != Camera3RequestDescriptor::NOT_FINISHED) {\n> > +\t\t\td->captureResult_.result = d->resultMetadata_->get();\n> > +\t\t\tcallbacks_->process_capture_result(callbacks_,\n> > +\t\t\t\t\t\t\t &(d->captureResult_));\n> \n> Not as part of this patch, but I feel like Camera3RequestDescriptor\n> could have a method called complete() to wrap the management of the\n> completion against a request descriptor.\n> \n> There's lots of occasions where the metadata is obtained or processed\n> into a result structure and called back with a flag.\n> \n> (such as *1 above, but I think there are more)\n> \n> It seems to me like that could be made into\n> \n> \t\td->complete(CameraRequest::Success);\n> \n> or something.... But that's an overall design of breaking out the\n> Camera3RequestDescriptor to a full class.\n> \n> As this series is extending Camera3RequestDescriptor to maintain extra\n> state required for the asynchronous post-processing, that makes me think\n> it would be better to have a full class to manage that state. (and thus\n> methods to operate on it)·\n> \n> \n> \n> > +\t\t\tqueuedDescriptor_.pop_front();\n> > +\t\t} else {\n> > +\t\t\tbreak;\n> > +\t\t}\n> > +\t}\n> > }\n> > \n> > std::string CameraDevice::logPrefix() const\n> > diff --git a/src/android/camera_device.h b/src/android/camera_device.h\n> > index a5576927..36425773 100644\n> > --- a/src/android/camera_device.h\n> > +++ b/src/android/camera_device.h\n> > @@ -7,6 +7,7 @@\n> > #ifndef __ANDROID_CAMERA_DEVICE_H__\n> > #define __ANDROID_CAMERA_DEVICE_H__\n> > \n> > +#include \n> > #include \n> > #include \n> > #include \n> > @@ -83,6 +84,21 @@ private:\n> > \t\tstd::vector> frameBuffers_;\n> > \t\tCameraMetadata settings_;\n> > \t\tstd::unique_ptr request_;\n> > +\n> > +\t\t/*\n> > +\t\t * Below are utility placeholders used when a capture result\n> > +\t\t * needs to be queued before completion via\n> > +\t\t * process_capture_result() callback.\n> > +\t\t */\n> > +\t\tenum completionStatus {\n> > +\t\t\tNOT_FINISHED,\n> \n> I'd call this \"Active\" or \"Pending\" to reduce the negation...\n> \n> \t\"If (state == Active)\"\n> rather than\n> \t\"if (state != NOT_FINISHED)\"\n> \n> \n> > +\t\t\tFINISHED_SUCCESS,\n> \n> I'd call this Success or Succeeded, or Successful ..\n> \n> > +\t\t\tFINISHED_FAILED,\n> \n> And Failed\n> \n> \n> \n> > +\t\t};\n> > +\t\tstd::unique_ptr resultMetadata_;\n> > +\t\tcamera3_capture_result_t captureResult_;\n> > +\t\tlibcamera::FrameBuffer *internalBuffer_;\n> > +\t\tcompletionStatus status_;\n> \n> These are all used to track the post-processor context.\n> \n> \n> \n> \n> > \t};\n> > \n> > \tenum class State {\n> > @@ -105,6 +121,11 @@ private:\n> > \tstd::unique_ptr getResultMetadata(\n> > \t\tconst Camera3RequestDescriptor &descriptor) const;\n> > \n> > +\tvoid sendQueuedCaptureResults();\n> > +\tvoid streamProcessingComplete(CameraStream *stream,\n> > +\t\t\t\t CameraStream::ProcessStatus status,\n> > +\t\t\t\t CameraMetadata *resultMetadata);\n> > +\n> > \tunsigned int id_;\n> > \tcamera3_device_t camera3Device_;\n> > \n> > @@ -125,6 +146,8 @@ private:\n> > \tlibcamera::Mutex descriptorsMutex_; /* Protects descriptors_. */\n> > \tstd::map descriptors_;\n> > \n> > +\tstd::deque> queuedDescriptor_;\n> \n> I'm not yet sure if this should be a unique_ptr ..\n> \n> \n> > +\n> > \tstd::string maker_;\n> > \tstd::string model_;\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 3E1A8BDC71\n\tfor ;\n\tWed, 8 Sep 2021 13:33:50 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id AF4CD69169;\n\tWed, 8 Sep 2021 15:33:49 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 917D060503\n\tfor ;\n\tWed, 8 Sep 2021 15:33:47 +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 0FE4024F;\n\tWed, 8 Sep 2021 15:33:47 +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=\"Bz6QtAlq\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1631108027;\n\tbh=DtZ1wlPlnR1GjydChGesuWy/plNO5cjX2wVi9cM9O+8=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=Bz6QtAlq6XRw6kcfym2knVVYJcgSA1oGSqgU4z/97o5Cxh+NdvpdOLMH60nPVt6o3\n\tRzO4KycbzfvuyDLLNWhbkM6CIiqoVfqFrvUzIozYFnuJ9xu8nYVzwWZQT80wZspnLp\n\tMz6snInWkFDZEZRtVsFL4g+fIHVPWyWIfJ1xFF5s=","Date":"Wed, 8 Sep 2021 16:33:26 +0300","From":"Laurent Pinchart ","To":"Kieran Bingham ","Message-ID":"","References":"<20210907195704.338079-1-umang.jain@ideasonboard.com>\n\t<20210907195704.338079-5-umang.jain@ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v1 4/6] android: camera_device: Add a\n\tqueue for sending capture results","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\" "}},{"id":19559,"web_url":"https://patchwork.libcamera.org/comment/19559/","msgid":"","date":"2021-09-08T14:23:04","subject":"Re: [libcamera-devel] [PATCH v1 4/6] android: camera_device: Add a\n\tqueue for sending capture results","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hi Laurent,\n\nOn 9/8/21 7:03 PM, Laurent Pinchart wrote:\n> On Wed, Sep 08, 2021 at 02:00:41PM +0100, Kieran Bingham wrote:\n>> On 07/09/2021 20:57, Umang Jain wrote:\n>>> When a camera capture request completes, the next step is to send the\n>>> capture results to the framework via process_capture_results(). All\n>>> the capture associated result metadata is prepared and populated. If\n>>> any post-processing is required, it will also take place in the same\n>>> thread (which can be blocking for a subtaintial amount of time) before\n>> s/subtaintial/substantial/\n>>\n>>> the results can be sent back to the framework.\n>>>\n>>> A follow up commit will move the post-processing to run in a separate\n>>> thread. In order to do so, there is few bits of groundwork which this\n>>> patch entails. Mainly, we need to preserve the order of sending\n>>> the capture results back in which they were queued by the framework\n>>> to the HAL (i.e. the order is sequential). Hence, we need to add a queue\n>>> in which capture results can be queued with context.\n>>>\n>>> As per this patch, the post-processor still runs synchronously as\n>>> before, but it will queue up the current capture results with context.\n>>> Later down the line, the capture results are dequeud in order and\n>> s/dequeud/dequeued/\n>>\n>>> sent back to the framework.\n>>>\n>>> The context is preserved using Camera3RequestDescriptor utility\n>>> structure. It has been expanded accordingly to address the needs of\n>>> the functionality.\n>>>\n>>> Signed-off-by: Umang Jain \n>>> ---\n>>> src/android/camera_device.cpp | 113 +++++++++++++++++++++++++++++++---\n>>> src/android/camera_device.h | 23 +++++++\n>>> 2 files changed, 126 insertions(+), 10 deletions(-)\n>>>\n>>> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n>>> index f2f36f32..9d4ec02e 100644\n>>> --- a/src/android/camera_device.cpp\n>>> +++ b/src/android/camera_device.cpp\n>>> @@ -240,6 +240,8 @@ CameraDevice::Camera3RequestDescriptor::Camera3RequestDescriptor(\n>>> \t/* Clone the controls associated with the camera3 request. */\n>>> \tsettings_ = CameraMetadata(camera3Request->settings);\n>>> \n>>> +\tinternalBuffer_ = nullptr;\n>>> +\n>>> \t/*\n>>> \t * Create the CaptureRequest, stored as a unique_ptr<> to tie its\n>>> \t * lifetime to the descriptor.\n>>> @@ -989,6 +991,7 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n>>> \t\t\t * once it has been processed.\n>>> \t\t\t */\n>>> \t\t\tbuffer = cameraStream->getBuffer();\n>>> +\t\t\tdescriptor.internalBuffer_ = buffer;\n>>> \t\t\tLOG(HAL, Debug) << ss.str() << \" (internal)\";\n>>> \t\t\tbreak;\n>>> \t\t}\n>>> @@ -1148,25 +1151,115 @@ void CameraDevice::requestComplete(Request *request)\n>>> \t\t\tcontinue;\n>>> \t\t}\n>>> \n>>> +\t\t/*\n>>> +\t\t * Save the current context of capture result and queue the\n>>> +\t\t * descriptor before processing the camera stream.\n>>> +\t\t *\n>>> +\t\t * When the processing completes, the descriptor will be\n>>> +\t\t * dequeued and capture results will be sent to the framework.\n>>> +\t\t */\n>>> +\t\tdescriptor.status_ = Camera3RequestDescriptor::NOT_FINISHED;\n>>> +\t\tdescriptor.resultMetadata_ = std::move(resultMetadata);\n>>> +\t\tdescriptor.captureResult_ = captureResult;\n>>> +\n>>> +\t\tstd::unique_ptr reqDescriptor =\n>>> +\t\t\tstd::make_unique();\n>>> +\t\t*reqDescriptor = std::move(descriptor);\n>>> +\t\tqueuedDescriptor_.push_back(std::move(reqDescriptor));\n>>> +\t\tCamera3RequestDescriptor *currentDescriptor = queuedDescriptor_.back().get();\n>>> +\n>>> +\t\tCameraMetadata *metadata = currentDescriptor->resultMetadata_.get();\n>>> +\t\tcameraStream->processComplete.connect(\n>>> +\t\t\tthis, [=](CameraStream::ProcessStatus status) {\n>>> +\t\t\t\tstreamProcessingComplete(cameraStream, status,\n>>> +\t\t\t\t\t\t\t metadata);\n>> I believe this to be unsafe to manage multiple operations.\n>>\n>> cameraStream->processComplete() should not be connected with per-run\n>> parameters. Those should be passed as the context of the signal emit(),\n>> not by the connection.\n>>\n>> Otherwise, the lamba will be overwritten by the next requestComplete()\n>> and incorrect parameters will be processed when the signal actually fires.\nWhile this is true.. and a bug in my  code\n> It's worse than that, ever connect() will create a new connection, so\n> the slot will be called once for the first request, twice for the\n> second, ...\n\nOh, I gotta see this in action by sprinkling some printfs...\n\nBut What is a alternative to this? While I tried to disconnect signal \nhandlers in the slot the last time, it came back with:\n\n'''\n\nWhile signals can be connected and disconnected on demand, it often\nindicates a design issue. Is there a reason why you can't connect the\nsignal when the cameraStream instance is created, and keep it connected\nfor the whole lifetime of the stream ?\n'''\n\nin \nhttps://lists.libcamera.org/pipermail/libcamera-devel/2021-August/023936.html\n\nNot sure if it's possible to check on a object's signal to check for a \nslot's existence. And doing so, feels weird as well.\n\nSo we need to connect to slot /only/ once, for all post-processing needs \nto be done in future. This sounds like an operation that can sit well in \nCameraStream::configure(). But how can we pass a slot (which is probably \nprivate to class) to another class's configure()? A function object ?\n\n>\n>>> +\t\t\t});\n>>> +\n>>> \t\tint ret = cameraStream->process(src, *buffer.buffer,\n>>> -\t\t\t\t\t\tdescriptor.settings_,\n>>> -\t\t\t\t\t\tresultMetadata.get());\n>>> +\t\t\t\t\t\tcurrentDescriptor->settings_,\n>>> +\t\t\t\t\t\tmetadata);\n>>> +\t\treturn;\n>>> +\t}\n>>> +\n>>> +\tif (queuedDescriptor_.empty()) {\n>>> +\t\tcaptureResult.result = resultMetadata->get();\n>>> +\t\tcallbacks_->process_capture_result(callbacks_, &captureResult);\n>> *1 duplication of completion callbacks (see below)\n>>\n>>> +\t} else {\n>>> +\t\t/*\n>>> +\t\t * Previous capture results waiting to be sent to framework\n>>> +\t\t * hence, queue the current capture results as well. After that,\n>>> +\t\t * check if any results are ready to be sent back to the\n>>> +\t\t * framework.\n>>> +\t\t */\n>>> +\t\tdescriptor.status_ = Camera3RequestDescriptor::FINISHED_SUCCESS;\n>>> +\t\tdescriptor.resultMetadata_ = std::move(resultMetadata);\n>>> +\t\tdescriptor.captureResult_ = captureResult;\n>>> +\t\tstd::unique_ptr reqDescriptor =\n>>> +\t\t\tstd::make_unique();\n>>> +\t\t*reqDescriptor = std::move(descriptor);\n>>> +\t\tqueuedDescriptor_.push_back(std::move(reqDescriptor));\n>>> +\n>>> +\t\tsendQueuedCaptureResults();\n>>> +\t}\n>>> +}\n>>> +\n>>> +void CameraDevice::streamProcessingComplete(CameraStream *cameraStream,\n>>> +\t\t\t\t\t CameraStream::ProcessStatus status,\n>>> +\t\t\t\t\t CameraMetadata *resultMetadata)\n>>> +{\n>>> +\tfor (auto &d : queuedDescriptor_) {\n>>> +\t\tif (d->resultMetadata_.get() != resultMetadata)\n>>> +\t\t\tcontinue;\n>>> +\n>>> \t\t/*\n>>> \t\t * Return the FrameBuffer to the CameraStream now that we're\n>>> \t\t * done processing it.\n>>> \t\t */\n>>> \t\tif (cameraStream->type() == CameraStream::Type::Internal)\n>>> -\t\t\tcameraStream->putBuffer(src);\n>>> -\n>>> -\t\tif (ret) {\n>>> -\t\t\tbuffer.status = CAMERA3_BUFFER_STATUS_ERROR;\n>>> -\t\t\tnotifyError(descriptor.frameNumber_, buffer.stream,\n>>> -\t\t\t\t CAMERA3_MSG_ERROR_BUFFER);\n>>> +\t\t\tcameraStream->putBuffer(d->internalBuffer_);\n>>> +\n>>> +\t\tif (status == CameraStream::ProcessStatus::Success) {\n>>> +\t\t\td->status_ = Camera3RequestDescriptor::FINISHED_SUCCESS;\n>>> +\t\t} else {\n>>> +\t\t\td->status_ = Camera3RequestDescriptor::FINISHED_FAILED;\n>>> +\t\t\td->captureResult_.partial_result = 0;\n>>> +\t\t\tfor (camera3_stream_buffer_t &buffer : d->buffers_) {\n>>> +\t\t\t\tCameraStream *cs = static_cast(buffer.stream->priv);\n>>> +\n>>> +\t\t\t\tif (cs->camera3Stream().format != HAL_PIXEL_FORMAT_BLOB)\n>>> +\t\t\t\t\tcontinue;\n>>> +\n>>> +\t\t\t\tbuffer.status = CAMERA3_BUFFER_STATUS_ERROR;\n>>> +\t\t\t\tnotifyError(d->frameNumber_, buffer.stream,\n>>> +\t\t\t\t\t CAMERA3_MSG_ERROR_BUFFER);\n>>> +\t\t\t}\n>>> \t\t}\n>>> +\n>>> +\t\tbreak;\n>>> \t}\n>>> \n>>> -\tcaptureResult.result = resultMetadata->get();\n>>> -\tcallbacks_->process_capture_result(callbacks_, &captureResult);\n>>> +\t/*\n>>> +\t * Send back capture results to the framework by inspecting the queue.\n>>> +\t * The framework can defer queueing further requests to the HAL (via\n>>> +\t * process_capture_request) unless until it receives the capture results\n>>> +\t * for already queued requests.\n>>> +\t */\n>>> +\tsendQueuedCaptureResults();\n>>> +}\n>>> +\n>>> +void CameraDevice::sendQueuedCaptureResults()\n>>> +{\n>>> +\twhile (!queuedDescriptor_.empty()) {\n>>> +\t\tstd::unique_ptr &d = queuedDescriptor_.front();\n>> I'd have this exit early if the front descriptor is not finished.\n>>\n>>\n>>> +\t\tif (d->status_ != Camera3RequestDescriptor::NOT_FINISHED) {\n>>> +\t\t\td->captureResult_.result = d->resultMetadata_->get();\n>>> +\t\t\tcallbacks_->process_capture_result(callbacks_,\n>>> +\t\t\t\t\t\t\t &(d->captureResult_));\n>> Not as part of this patch, but I feel like Camera3RequestDescriptor\n>> could have a method called complete() to wrap the management of the\n>> completion against a request descriptor.\n>>\n>> There's lots of occasions where the metadata is obtained or processed\n>> into a result structure and called back with a flag.\n>>\n>> (such as *1 above, but I think there are more)\n>>\n>> It seems to me like that could be made into\n>>\n>> \t\td->complete(CameraRequest::Success);\n>>\n>> or something.... But that's an overall design of breaking out the\n>> Camera3RequestDescriptor to a full class.\n>>\n>> As this series is extending Camera3RequestDescriptor to maintain extra\n>> state required for the asynchronous post-processing, that makes me think\n>> it would be better to have a full class to manage that state. (and thus\n>> methods to operate on it)·\n>>\n>>\n>>\n>>> +\t\t\tqueuedDescriptor_.pop_front();\n>>> +\t\t} else {\n>>> +\t\t\tbreak;\n>>> +\t\t}\n>>> +\t}\n>>> }\n>>> \n>>> std::string CameraDevice::logPrefix() const\n>>> diff --git a/src/android/camera_device.h b/src/android/camera_device.h\n>>> index a5576927..36425773 100644\n>>> --- a/src/android/camera_device.h\n>>> +++ b/src/android/camera_device.h\n>>> @@ -7,6 +7,7 @@\n>>> #ifndef __ANDROID_CAMERA_DEVICE_H__\n>>> #define __ANDROID_CAMERA_DEVICE_H__\n>>> \n>>> +#include \n>>> #include \n>>> #include \n>>> #include \n>>> @@ -83,6 +84,21 @@ private:\n>>> \t\tstd::vector> frameBuffers_;\n>>> \t\tCameraMetadata settings_;\n>>> \t\tstd::unique_ptr request_;\n>>> +\n>>> +\t\t/*\n>>> +\t\t * Below are utility placeholders used when a capture result\n>>> +\t\t * needs to be queued before completion via\n>>> +\t\t * process_capture_result() callback.\n>>> +\t\t */\n>>> +\t\tenum completionStatus {\n>>> +\t\t\tNOT_FINISHED,\n>> I'd call this \"Active\" or \"Pending\" to reduce the negation...\n>>\n>> \t\"If (state == Active)\"\n>> rather than\n>> \t\"if (state != NOT_FINISHED)\"\n>>\n>>\n>>> +\t\t\tFINISHED_SUCCESS,\n>> I'd call this Success or Succeeded, or Successful ..\n>>\n>>> +\t\t\tFINISHED_FAILED,\n>> And Failed\n>>\n>>\n>>\n>>> +\t\t};\n>>> +\t\tstd::unique_ptr resultMetadata_;\n>>> +\t\tcamera3_capture_result_t captureResult_;\n>>> +\t\tlibcamera::FrameBuffer *internalBuffer_;\n>>> +\t\tcompletionStatus status_;\n>> These are all used to track the post-processor context.\n>>\n>>\n>>\n>>\n>>> \t};\n>>> \n>>> \tenum class State {\n>>> @@ -105,6 +121,11 @@ private:\n>>> \tstd::unique_ptr getResultMetadata(\n>>> \t\tconst Camera3RequestDescriptor &descriptor) const;\n>>> \n>>> +\tvoid sendQueuedCaptureResults();\n>>> +\tvoid streamProcessingComplete(CameraStream *stream,\n>>> +\t\t\t\t CameraStream::ProcessStatus status,\n>>> +\t\t\t\t CameraMetadata *resultMetadata);\n>>> +\n>>> \tunsigned int id_;\n>>> \tcamera3_device_t camera3Device_;\n>>> \n>>> @@ -125,6 +146,8 @@ private:\n>>> \tlibcamera::Mutex descriptorsMutex_; /* Protects descriptors_. */\n>>> \tstd::map descriptors_;\n>>> \n>>> +\tstd::deque> queuedDescriptor_;\n>> I'm not yet sure if this should be a unique_ptr ..\n>>\n>>\n>>> +\n>>> \tstd::string maker_;\n>>> \tstd::string model_;\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 764E2BDC71\n\tfor ;\n\tWed, 8 Sep 2021 14:23:12 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C0CF469171;\n\tWed, 8 Sep 2021 16:23: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 5C52160503\n\tfor ;\n\tWed, 8 Sep 2021 16:23:10 +0200 (CEST)","from [192.168.1.104] (unknown [103.251.226.149])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id BDFDD993;\n\tWed, 8 Sep 2021 16:23:08 +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=\"FDbF6l1i\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1631110990;\n\tbh=Q0w8TpnlunrZ5ueAljx8CEYGteh/adK3uR/xuzYNya4=;\n\th=Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=FDbF6l1iz+4Uivu/GqOkfyjnrpbfhvE6gCW1RVi8hhgjyAPUcH28IUefZKaiw0VbR\n\tjF+JgPOp9G5CvDaUrFM38TTV1VHAoVFafGFXLXL0hXmnkRBB4WxMRq/uYTIyLTsZXW\n\t7XYwi8XSyNPeO0SNUhDeX44Eja7VYUrm5zixMGEQ=","To":"Laurent Pinchart ,\n\tKieran Bingham ","References":"<20210907195704.338079-1-umang.jain@ideasonboard.com>\n\t<20210907195704.338079-5-umang.jain@ideasonboard.com>\n\t\n\t","From":"Umang Jain ","Message-ID":"","Date":"Wed, 8 Sep 2021 19:53:04 +0530","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.10.2","MIME-Version":"1.0","In-Reply-To":"","Content-Type":"multipart/alternative;\n\tboundary=\"------------51F925514F3BA3665FD49E95\"","Content-Language":"en-US","Subject":"Re: [libcamera-devel] [PATCH v1 4/6] android: camera_device: Add a\n\tqueue for sending capture results","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\" "}},{"id":19560,"web_url":"https://patchwork.libcamera.org/comment/19560/","msgid":"<658bdbec-42e6-59fc-4dd1-f094d2c00427@ideasonboard.com>","date":"2021-09-08T14:31:20","subject":"Re: [libcamera-devel] [PATCH v1 4/6] android: camera_device: Add a\n\tqueue for sending capture results","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hi again,\n\nOn 9/8/21 7:53 PM, Umang Jain wrote:\n> Hi Laurent,\n>\n> On 9/8/21 7:03 PM, Laurent Pinchart wrote:\n>> On Wed, Sep 08, 2021 at 02:00:41PM +0100, Kieran Bingham wrote:\n>>> On 07/09/2021 20:57, Umang Jain wrote:\n>>>> When a camera capture request completes, the next step is to send the\n>>>> capture results to the framework via process_capture_results(). All\n>>>> the capture associated result metadata is prepared and populated. If\n>>>> any post-processing is required, it will also take place in the same\n>>>> thread (which can be blocking for a subtaintial amount of time) before\n>>> s/subtaintial/substantial/\n>>>\n>>>> the results can be sent back to the framework.\n>>>>\n>>>> A follow up commit will move the post-processing to run in a separate\n>>>> thread. In order to do so, there is few bits of groundwork which this\n>>>> patch entails. Mainly, we need to preserve the order of sending\n>>>> the capture results back in which they were queued by the framework\n>>>> to the HAL (i.e. the order is sequential). Hence, we need to add a \n>>>> queue\n>>>> in which capture results can be queued with context.\n>>>>\n>>>> As per this patch, the post-processor still runs synchronously as\n>>>> before, but it will queue up the current capture results with context.\n>>>> Later down the line, the capture results are dequeud in order and\n>>> s/dequeud/dequeued/\n>>>\n>>>> sent back to the framework.\n>>>>\n>>>> The context is preserved using Camera3RequestDescriptor utility\n>>>> structure. It has been expanded accordingly to address the needs of\n>>>> the functionality.\n>>>>\n>>>> Signed-off-by: Umang Jain \n>>>> ---\n>>>>   src/android/camera_device.cpp | 113 \n>>>> +++++++++++++++++++++++++++++++---\n>>>>   src/android/camera_device.h   |  23 +++++++\n>>>>   2 files changed, 126 insertions(+), 10 deletions(-)\n>>>>\n>>>> diff --git a/src/android/camera_device.cpp \n>>>> b/src/android/camera_device.cpp\n>>>> index f2f36f32..9d4ec02e 100644\n>>>> --- a/src/android/camera_device.cpp\n>>>> +++ b/src/android/camera_device.cpp\n>>>> @@ -240,6 +240,8 @@ \n>>>> CameraDevice::Camera3RequestDescriptor::Camera3RequestDescriptor(\n>>>>       /* Clone the controls associated with the camera3 request. */\n>>>>       settings_ = CameraMetadata(camera3Request->settings);\n>>>>   +    internalBuffer_ = nullptr;\n>>>> +\n>>>>       /*\n>>>>        * Create the CaptureRequest, stored as a unique_ptr<> to tie \n>>>> its\n>>>>        * lifetime to the descriptor.\n>>>> @@ -989,6 +991,7 @@ int \n>>>> CameraDevice::processCaptureRequest(camera3_capture_request_t \n>>>> *camera3Reques\n>>>>                * once it has been processed.\n>>>>                */\n>>>>               buffer = cameraStream->getBuffer();\n>>>> +            descriptor.internalBuffer_ = buffer;\n>>>>               LOG(HAL, Debug) << ss.str() << \" (internal)\";\n>>>>               break;\n>>>>           }\n>>>> @@ -1148,25 +1151,115 @@ void CameraDevice::requestComplete(Request \n>>>> *request)\n>>>>               continue;\n>>>>           }\n>>>>   +        /*\n>>>> +         * Save the current context of capture result and queue the\n>>>> +         * descriptor before processing the camera stream.\n>>>> +         *\n>>>> +         * When the processing completes, the descriptor will be\n>>>> +         * dequeued and capture results will be sent to the \n>>>> framework.\n>>>> +         */\n>>>> +        descriptor.status_ = Camera3RequestDescriptor::NOT_FINISHED;\n>>>> +        descriptor.resultMetadata_ = std::move(resultMetadata);\n>>>> +        descriptor.captureResult_ = captureResult;\n>>>> +\n>>>> +        std::unique_ptr reqDescriptor =\n>>>> + std::make_unique();\n>>>> +        *reqDescriptor = std::move(descriptor);\n>>>> + queuedDescriptor_.push_back(std::move(reqDescriptor));\n>>>> +        Camera3RequestDescriptor *currentDescriptor = \n>>>> queuedDescriptor_.back().get();\n>>>> +\n>>>> +        CameraMetadata *metadata = \n>>>> currentDescriptor->resultMetadata_.get();\n>>>> +        cameraStream->processComplete.connect(\n>>>> +            this, [=](CameraStream::ProcessStatus status) {\n>>>> +                streamProcessingComplete(cameraStream, status,\n>>>> +                             metadata);\n>>> I believe this to be unsafe to manage multiple operations.\n>>>\n>>> cameraStream->processComplete() should not be connected with per-run\n>>> parameters. Those should be passed as the context of the signal emit(),\n>>> not by the connection.\n>>>\n>>> Otherwise, the lamba will be overwritten by the next requestComplete()\n>>> and incorrect parameters will be processed when the signal actually \n>>> fires.\n> While this is true.. and a bug in my  code\n>> It's worse than that, ever connect() will create a new connection, so\n>> the slot will be called once for the first request, twice for the\n>> second, ...\n>\n> Oh, I gotta see this in action by sprinkling some printfs...\n>\n> But What is a alternative to this? While I tried to disconnect signal \n> handlers in the slot the last time, it came back with:\n>\n> '''\n>\n> While signals can be connected and disconnected on demand, it often\n> indicates a design issue. Is there a reason why you can't connect the\n> signal when the cameraStream instance is created, and keep it connected\n> for the whole lifetime of the stream ?\n> '''\n>\n> in \n> https://lists.libcamera.org/pipermail/libcamera-devel/2021-August/023936.html\n>\n> Not sure if it's possible to check on a object's signal to check for a \n> slot's existence. And doing so, feels weird as well.\n>\n> So we need to connect to slot /only/ once, for all post-processing \n> needs to be done in future. This sounds like an operation that can sit \n> well in CameraStream::configure(). But how can we pass a slot (which \n> is probably private to class) to another class's configure()? A \n> function object ?\n\n\nAh no, that would be bad as well. Probably when streams are constructed \nin CameraDevice::configureStreams(). We can connect the \npost-processing-complete-signal right there and not on every \npost-process's run.\n\n\n>\n>>\n>>>> +            });\n>>>> +\n>>>>           int ret = cameraStream->process(src, *buffer.buffer,\n>>>> -                        descriptor.settings_,\n>>>> -                        resultMetadata.get());\n>>>> +                        currentDescriptor->settings_,\n>>>> +                        metadata);\n>>>> +        return;\n>>>> +    }\n>>>> +\n>>>> +    if (queuedDescriptor_.empty()) {\n>>>> +        captureResult.result = resultMetadata->get();\n>>>> +        callbacks_->process_capture_result(callbacks_, \n>>>> &captureResult);\n>>> *1 duplication of completion callbacks (see below)\n>>>\n>>>> +    } else {\n>>>> +        /*\n>>>> +         * Previous capture results waiting to be sent to framework\n>>>> +         * hence, queue the current capture results as well. After \n>>>> that,\n>>>> +         * check if any results are ready to be sent back to the\n>>>> +         * framework.\n>>>> +         */\n>>>> +        descriptor.status_ = \n>>>> Camera3RequestDescriptor::FINISHED_SUCCESS;\n>>>> +        descriptor.resultMetadata_ = std::move(resultMetadata);\n>>>> +        descriptor.captureResult_ = captureResult;\n>>>> +        std::unique_ptr reqDescriptor =\n>>>> + std::make_unique();\n>>>> +        *reqDescriptor = std::move(descriptor);\n>>>> + queuedDescriptor_.push_back(std::move(reqDescriptor));\n>>>> +\n>>>> +        sendQueuedCaptureResults();\n>>>> +    }\n>>>> +}\n>>>> +\n>>>> +void CameraDevice::streamProcessingComplete(CameraStream \n>>>> *cameraStream,\n>>>> +                        CameraStream::ProcessStatus status,\n>>>> +                        CameraMetadata *resultMetadata)\n>>>> +{\n>>>> +    for (auto &d : queuedDescriptor_) {\n>>>> +        if (d->resultMetadata_.get() != resultMetadata)\n>>>> +            continue;\n>>>> +\n>>>>           /*\n>>>>            * Return the FrameBuffer to the CameraStream now that we're\n>>>>            * done processing it.\n>>>>            */\n>>>>           if (cameraStream->type() == CameraStream::Type::Internal)\n>>>> -            cameraStream->putBuffer(src);\n>>>> -\n>>>> -        if (ret) {\n>>>> -            buffer.status = CAMERA3_BUFFER_STATUS_ERROR;\n>>>> -            notifyError(descriptor.frameNumber_, buffer.stream,\n>>>> -                    CAMERA3_MSG_ERROR_BUFFER);\n>>>> + cameraStream->putBuffer(d->internalBuffer_);\n>>>> +\n>>>> +        if (status == CameraStream::ProcessStatus::Success) {\n>>>> +            d->status_ = Camera3RequestDescriptor::FINISHED_SUCCESS;\n>>>> +        } else {\n>>>> +            d->status_ = Camera3RequestDescriptor::FINISHED_FAILED;\n>>>> +            d->captureResult_.partial_result = 0;\n>>>> +            for (camera3_stream_buffer_t &buffer : d->buffers_) {\n>>>> +                CameraStream *cs = static_cast>>> *>(buffer.stream->priv);\n>>>> +\n>>>> +                if (cs->camera3Stream().format != \n>>>> HAL_PIXEL_FORMAT_BLOB)\n>>>> +                    continue;\n>>>> +\n>>>> +                buffer.status = CAMERA3_BUFFER_STATUS_ERROR;\n>>>> +                notifyError(d->frameNumber_, buffer.stream,\n>>>> +                        CAMERA3_MSG_ERROR_BUFFER);\n>>>> +            }\n>>>>           }\n>>>> +\n>>>> +        break;\n>>>>       }\n>>>>   -    captureResult.result = resultMetadata->get();\n>>>> -    callbacks_->process_capture_result(callbacks_, &captureResult);\n>>>> +    /*\n>>>> +     * Send back capture results to the framework by inspecting \n>>>> the queue.\n>>>> +     * The framework can defer queueing further requests to the \n>>>> HAL (via\n>>>> +     * process_capture_request) unless until it receives the \n>>>> capture results\n>>>> +     * for already queued requests.\n>>>> +     */\n>>>> +    sendQueuedCaptureResults();\n>>>> +}\n>>>> +\n>>>> +void CameraDevice::sendQueuedCaptureResults()\n>>>> +{\n>>>> +    while (!queuedDescriptor_.empty()) {\n>>>> +        std::unique_ptr &d = \n>>>> queuedDescriptor_.front();\n>>> I'd have this exit early if the front descriptor is not finished.\n>>>\n>>>\n>>>> +        if (d->status_ != Camera3RequestDescriptor::NOT_FINISHED) {\n>>>> +            d->captureResult_.result = d->resultMetadata_->get();\n>>>> + callbacks_->process_capture_result(callbacks_,\n>>>> + &(d->captureResult_));\n>>> Not as part of this patch, but I feel like Camera3RequestDescriptor\n>>> could have a method called complete() to wrap the management of the\n>>> completion against a request descriptor.\n>>>\n>>> There's lots of occasions where the metadata is obtained or processed\n>>> into a result structure and called back with a flag.\n>>>\n>>> (such as *1 above, but I think there are more)\n>>>\n>>> It seems to me like that could be made into\n>>>\n>>>         d->complete(CameraRequest::Success);\n>>>\n>>> or something.... But that's an overall design of breaking out the\n>>> Camera3RequestDescriptor to a full class.\n>>>\n>>> As this series is extending Camera3RequestDescriptor to maintain extra\n>>> state required for the asynchronous post-processing, that makes me \n>>> think\n>>> it would be better to have a full class to manage that state. (and thus\n>>> methods to operate on it)·\n>>>\n>>>\n>>>\n>>>> + queuedDescriptor_.pop_front();\n>>>> +        } else {\n>>>> +            break;\n>>>> +        }\n>>>> +    }\n>>>>   }\n>>>>     std::string CameraDevice::logPrefix() const\n>>>> diff --git a/src/android/camera_device.h b/src/android/camera_device.h\n>>>> index a5576927..36425773 100644\n>>>> --- a/src/android/camera_device.h\n>>>> +++ b/src/android/camera_device.h\n>>>> @@ -7,6 +7,7 @@\n>>>>   #ifndef __ANDROID_CAMERA_DEVICE_H__\n>>>>   #define __ANDROID_CAMERA_DEVICE_H__\n>>>>   +#include \n>>>>   #include \n>>>>   #include \n>>>>   #include \n>>>> @@ -83,6 +84,21 @@ private:\n>>>> std::vector> frameBuffers_;\n>>>>           CameraMetadata settings_;\n>>>>           std::unique_ptr request_;\n>>>> +\n>>>> +        /*\n>>>> +         * Below are utility placeholders used when a capture result\n>>>> +         * needs to be queued before completion via\n>>>> +         * process_capture_result() callback.\n>>>> +         */\n>>>> +        enum completionStatus {\n>>>> +            NOT_FINISHED,\n>>> I'd call this \"Active\" or \"Pending\" to reduce the negation...\n>>>\n>>>     \"If (state == Active)\"\n>>> rather than\n>>>     \"if (state != NOT_FINISHED)\"\n>>>\n>>>\n>>>> +            FINISHED_SUCCESS,\n>>> I'd call this Success or Succeeded, or Successful ..\n>>>\n>>>> +            FINISHED_FAILED,\n>>> And Failed\n>>>\n>>>\n>>>\n>>>> +        };\n>>>> +        std::unique_ptr resultMetadata_;\n>>>> +        camera3_capture_result_t captureResult_;\n>>>> +        libcamera::FrameBuffer *internalBuffer_;\n>>>> +        completionStatus status_;\n>>> These are all used to track the post-processor context.\n>>>\n>>>\n>>>\n>>>\n>>>>       };\n>>>>         enum class State {\n>>>> @@ -105,6 +121,11 @@ private:\n>>>>       std::unique_ptr getResultMetadata(\n>>>>           const Camera3RequestDescriptor &descriptor) const;\n>>>>   +    void sendQueuedCaptureResults();\n>>>> +    void streamProcessingComplete(CameraStream *stream,\n>>>> +                      CameraStream::ProcessStatus status,\n>>>> +                      CameraMetadata *resultMetadata);\n>>>> +\n>>>>       unsigned int id_;\n>>>>       camera3_device_t camera3Device_;\n>>>>   @@ -125,6 +146,8 @@ private:\n>>>>       libcamera::Mutex descriptorsMutex_; /* Protects descriptors_. */\n>>>>       std::map descriptors_;\n>>>>   + std::deque> \n>>>> queuedDescriptor_;\n>>> I'm not yet sure if this should be a unique_ptr ..\n>>>\n>>>\n>>>> +\n>>>>       std::string maker_;\n>>>>       std::string model_;\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 D32C6BDB1D\n\tfor ;\n\tWed, 8 Sep 2021 14:31:27 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 554F569167;\n\tWed, 8 Sep 2021 16:31:27 +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 5273A60503\n\tfor ;\n\tWed, 8 Sep 2021 16:31:25 +0200 (CEST)","from [192.168.1.104] (unknown [103.251.226.149])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id DE1DA993;\n\tWed, 8 Sep 2021 16:31:23 +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=\"rLROz+jD\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1631111484;\n\tbh=P/PGbq8qBSGnLSjpyQgMqKLOHIOG2NnokOvJYzx4Su8=;\n\th=Subject:From:To:Cc:References:Date:In-Reply-To:From;\n\tb=rLROz+jDa08Zag99sc281jqmgV8WY975V+h//YnaCNyeE1AhxUiEupfcPtPPXV8jm\n\tct8QHrcTqKFGohrEKlobFiglClHuL0lFY9I7ENZBm7rEG0MUU09nIMdv2BTK6B6ofA\n\ti3p+oTOBtAFMUauu7aVjoHu/LwfIxEG/PnT/xWyI=","From":"Umang Jain ","To":"Laurent Pinchart ,\n\tKieran Bingham ","References":"<20210907195704.338079-1-umang.jain@ideasonboard.com>\n\t<20210907195704.338079-5-umang.jain@ideasonboard.com>\n\t\n\t\n\t","Message-ID":"<658bdbec-42e6-59fc-4dd1-f094d2c00427@ideasonboard.com>","Date":"Wed, 8 Sep 2021 20:01:20 +0530","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.10.2","MIME-Version":"1.0","In-Reply-To":"","Content-Type":"text/plain; charset=utf-8; format=flowed","Content-Transfer-Encoding":"8bit","Content-Language":"en-US","Subject":"Re: [libcamera-devel] [PATCH v1 4/6] android: camera_device: Add a\n\tqueue for sending capture results","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\" "}},{"id":19561,"web_url":"https://patchwork.libcamera.org/comment/19561/","msgid":"","date":"2021-09-08T21:52:33","subject":"Re: [libcamera-devel] [PATCH v1 4/6] android: camera_device: Add a\n\tqueue for sending capture results","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Umang,\n\nOn Wed, Sep 08, 2021 at 07:53:04PM +0530, Umang Jain wrote:\n> On 9/8/21 7:03 PM, Laurent Pinchart wrote:\n> > On Wed, Sep 08, 2021 at 02:00:41PM +0100, Kieran Bingham wrote:\n> >> On 07/09/2021 20:57, Umang Jain wrote:\n> >>> When a camera capture request completes, the next step is to send the\n> >>> capture results to the framework via process_capture_results(). All\n> >>> the capture associated result metadata is prepared and populated. If\n> >>> any post-processing is required, it will also take place in the same\n> >>> thread (which can be blocking for a subtaintial amount of time) before\n> >>\n> >> s/subtaintial/substantial/\n> >>\n> >>> the results can be sent back to the framework.\n> >>>\n> >>> A follow up commit will move the post-processing to run in a separate\n> >>> thread. In order to do so, there is few bits of groundwork which this\n> >>> patch entails. Mainly, we need to preserve the order of sending\n> >>> the capture results back in which they were queued by the framework\n> >>> to the HAL (i.e. the order is sequential). Hence, we need to add a queue\n> >>> in which capture results can be queued with context.\n> >>>\n> >>> As per this patch, the post-processor still runs synchronously as\n> >>> before, but it will queue up the current capture results with context.\n> >>> Later down the line, the capture results are dequeud in order and\n> >>\n> >> s/dequeud/dequeued/\n> >>\n> >>> sent back to the framework.\n> >>>\n> >>> The context is preserved using Camera3RequestDescriptor utility\n> >>> structure. It has been expanded accordingly to address the needs of\n> >>> the functionality.\n> >>>\n> >>> Signed-off-by: Umang Jain \n> >>> ---\n> >>> src/android/camera_device.cpp | 113 +++++++++++++++++++++++++++++++---\n> >>> src/android/camera_device.h | 23 +++++++\n> >>> 2 files changed, 126 insertions(+), 10 deletions(-)\n> >>>\n> >>> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> >>> index f2f36f32..9d4ec02e 100644\n> >>> --- a/src/android/camera_device.cpp\n> >>> +++ b/src/android/camera_device.cpp\n> >>> @@ -240,6 +240,8 @@ CameraDevice::Camera3RequestDescriptor::Camera3RequestDescriptor(\n> >>> \t/* Clone the controls associated with the camera3 request. */\n> >>> \tsettings_ = CameraMetadata(camera3Request->settings);\n> >>> \n> >>> +\tinternalBuffer_ = nullptr;\n> >>> +\n> >>> \t/*\n> >>> \t * Create the CaptureRequest, stored as a unique_ptr<> to tie its\n> >>> \t * lifetime to the descriptor.\n> >>> @@ -989,6 +991,7 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n> >>> \t\t\t * once it has been processed.\n> >>> \t\t\t */\n> >>> \t\t\tbuffer = cameraStream->getBuffer();\n> >>> +\t\t\tdescriptor.internalBuffer_ = buffer;\n> >>> \t\t\tLOG(HAL, Debug) << ss.str() << \" (internal)\";\n> >>> \t\t\tbreak;\n> >>> \t\t}\n> >>> @@ -1148,25 +1151,115 @@ void CameraDevice::requestComplete(Request *request)\n> >>> \t\t\tcontinue;\n> >>> \t\t}\n> >>> \n> >>> +\t\t/*\n> >>> +\t\t * Save the current context of capture result and queue the\n> >>> +\t\t * descriptor before processing the camera stream.\n> >>> +\t\t *\n> >>> +\t\t * When the processing completes, the descriptor will be\n> >>> +\t\t * dequeued and capture results will be sent to the framework.\n> >>> +\t\t */\n> >>> +\t\tdescriptor.status_ = Camera3RequestDescriptor::NOT_FINISHED;\n> >>> +\t\tdescriptor.resultMetadata_ = std::move(resultMetadata);\n> >>> +\t\tdescriptor.captureResult_ = captureResult;\n> >>> +\n> >>> +\t\tstd::unique_ptr reqDescriptor =\n> >>> +\t\t\tstd::make_unique();\n> >>> +\t\t*reqDescriptor = std::move(descriptor);\n> >>> +\t\tqueuedDescriptor_.push_back(std::move(reqDescriptor));\n> >>> +\t\tCamera3RequestDescriptor *currentDescriptor = queuedDescriptor_.back().get();\n> >>> +\n> >>> +\t\tCameraMetadata *metadata = currentDescriptor->resultMetadata_.get();\n> >>> +\t\tcameraStream->processComplete.connect(\n> >>> +\t\t\tthis, [=](CameraStream::ProcessStatus status) {\n> >>> +\t\t\t\tstreamProcessingComplete(cameraStream, status,\n> >>> +\t\t\t\t\t\t\t metadata);\n> >>\n> >> I believe this to be unsafe to manage multiple operations.\n> >>\n> >> cameraStream->processComplete() should not be connected with per-run\n> >> parameters. Those should be passed as the context of the signal emit(),\n> >> not by the connection.\n> >>\n> >> Otherwise, the lamba will be overwritten by the next requestComplete()\n> >> and incorrect parameters will be processed when the signal actually fires.\n>\n> While this is true.. and a bug in my  code\n>\n> > It's worse than that, ever connect() will create a new connection, so\n> > the slot will be called once for the first request, twice for the\n> > second, ...\n> \n> Oh, I gotta see this in action by sprinkling some printfs...\n> \n> But What is a alternative to this? While I tried to disconnect signal \n> handlers in the slot the last time, it came back with:\n> \n> '''\n> While signals can be connected and disconnected on demand, it often\n> indicates a design issue. Is there a reason why you can't connect the\n> signal when the cameraStream instance is created, and keep it connected\n> for the whole lifetime of the stream ?\n> '''\n> \n> in \n> https://lists.libcamera.org/pipermail/libcamera-devel/2021-August/023936.html\n> \n> Not sure if it's possible to check on a object's signal to check for a \n> slot's existence. And doing so, feels weird as well.\n> \n> So we need to connect to slot /only/ once, for all post-processing needs \n> to be done in future. This sounds like an operation that can sit well in \n> CameraStream::configure().\n\nCorrect. You've removed the disconnection based on my comments for the\nprevious version, but the connection also needs to be addressed :-)\nConnecting in \n\n> But how can we pass a slot (which is probably \n> private to class) to another class's configure()? A function object ?\n\nYou can create a slot in CameraStream, and from there call\n\n\tcameraDevice_->streamProcessingComplete(this, request, status);\n\nrequest should be a pointer to Camera3RequestDescriptor (which we could\nrename to Camera3Request), it's the object that models the context of\nthe processing. You need to pass it to cameraStream->process() in the\nfirst place of course.\n\nIt's tempting to merge Camera3RequestDescriptor with CaptureRequest, but\nlet's not do that, as fence handling needs to move to the libcamera\ncore, so the CameraWorker will disappear then, and so will\nCaptureRequest.\n\n> >>> +\t\t\t});\n> >>> +\n> >>> \t\tint ret = cameraStream->process(src, *buffer.buffer,\n> >>> -\t\t\t\t\t\tdescriptor.settings_,\n> >>> -\t\t\t\t\t\tresultMetadata.get());\n> >>> +\t\t\t\t\t\tcurrentDescriptor->settings_,\n> >>> +\t\t\t\t\t\tmetadata);\n> >>> +\t\treturn;\n> >>> +\t}\n> >>> +\n> >>> +\tif (queuedDescriptor_.empty()) {\n> >>> +\t\tcaptureResult.result = resultMetadata->get();\n> >>> +\t\tcallbacks_->process_capture_result(callbacks_, &captureResult);\n> >>\n> >> *1 duplication of completion callbacks (see below)\n> >>\n> >>> +\t} else {\n> >>> +\t\t/*\n> >>> +\t\t * Previous capture results waiting to be sent to framework\n> >>> +\t\t * hence, queue the current capture results as well. After that,\n> >>> +\t\t * check if any results are ready to be sent back to the\n> >>> +\t\t * framework.\n> >>> +\t\t */\n> >>> +\t\tdescriptor.status_ = Camera3RequestDescriptor::FINISHED_SUCCESS;\n> >>> +\t\tdescriptor.resultMetadata_ = std::move(resultMetadata);\n> >>> +\t\tdescriptor.captureResult_ = captureResult;\n> >>> +\t\tstd::unique_ptr reqDescriptor =\n> >>> +\t\t\tstd::make_unique();\n> >>> +\t\t*reqDescriptor = std::move(descriptor);\n> >>> +\t\tqueuedDescriptor_.push_back(std::move(reqDescriptor));\n> >>> +\n> >>> +\t\tsendQueuedCaptureResults();\n> >>> +\t}\n> >>> +}\n> >>> +\n> >>> +void CameraDevice::streamProcessingComplete(CameraStream *cameraStream,\n> >>> +\t\t\t\t\t CameraStream::ProcessStatus status,\n> >>> +\t\t\t\t\t CameraMetadata *resultMetadata)\n> >>> +{\n> >>> +\tfor (auto &d : queuedDescriptor_) {\n> >>> +\t\tif (d->resultMetadata_.get() != resultMetadata)\n> >>> +\t\t\tcontinue;\n\nThis is a hack that you'll be able to drop once you get the pointer to\nthe Camera3RequestDescriptor passed as a function argument.\n\n> >>> +\n> >>> \t\t/*\n> >>> \t\t * Return the FrameBuffer to the CameraStream now that we're\n> >>> \t\t * done processing it.\n> >>> \t\t */\n> >>> \t\tif (cameraStream->type() == CameraStream::Type::Internal)\n> >>> -\t\t\tcameraStream->putBuffer(src);\n> >>> -\n> >>> -\t\tif (ret) {\n> >>> -\t\t\tbuffer.status = CAMERA3_BUFFER_STATUS_ERROR;\n> >>> -\t\t\tnotifyError(descriptor.frameNumber_, buffer.stream,\n> >>> -\t\t\t\t CAMERA3_MSG_ERROR_BUFFER);\n> >>> +\t\t\tcameraStream->putBuffer(d->internalBuffer_);\n> >>> +\n> >>> +\t\tif (status == CameraStream::ProcessStatus::Success) {\n> >>> +\t\t\td->status_ = Camera3RequestDescriptor::FINISHED_SUCCESS;\n> >>> +\t\t} else {\n> >>> +\t\t\td->status_ = Camera3RequestDescriptor::FINISHED_FAILED;\n> >>> +\t\t\td->captureResult_.partial_result = 0;\n> >>> +\t\t\tfor (camera3_stream_buffer_t &buffer : d->buffers_) {\n> >>> +\t\t\t\tCameraStream *cs = static_cast(buffer.stream->priv);\n> >>> +\n> >>> +\t\t\t\tif (cs->camera3Stream().format != HAL_PIXEL_FORMAT_BLOB)\n> >>> +\t\t\t\t\tcontinue;\n> >>> +\n> >>> +\t\t\t\tbuffer.status = CAMERA3_BUFFER_STATUS_ERROR;\n> >>> +\t\t\t\tnotifyError(d->frameNumber_, buffer.stream,\n> >>> +\t\t\t\t\t CAMERA3_MSG_ERROR_BUFFER);\n> >>> +\t\t\t}\n> >>> \t\t}\n> >>> +\n> >>> +\t\tbreak;\n> >>> \t}\n> >>> \n> >>> -\tcaptureResult.result = resultMetadata->get();\n> >>> -\tcallbacks_->process_capture_result(callbacks_, &captureResult);\n> >>> +\t/*\n> >>> +\t * Send back capture results to the framework by inspecting the queue.\n> >>> +\t * The framework can defer queueing further requests to the HAL (via\n> >>> +\t * process_capture_request) unless until it receives the capture results\n> >>> +\t * for already queued requests.\n> >>> +\t */\n> >>> +\tsendQueuedCaptureResults();\n> >>> +}\n> >>> +\n> >>> +void CameraDevice::sendQueuedCaptureResults()\n> >>> +{\n> >>> +\twhile (!queuedDescriptor_.empty()) {\n> >>> +\t\tstd::unique_ptr &d = queuedDescriptor_.front();\n> >>\n> >> I'd have this exit early if the front descriptor is not finished.\n> >>\n> >>> +\t\tif (d->status_ != Camera3RequestDescriptor::NOT_FINISHED) {\n> >>> +\t\t\td->captureResult_.result = d->resultMetadata_->get();\n> >>> +\t\t\tcallbacks_->process_capture_result(callbacks_,\n> >>> +\t\t\t\t\t\t\t &(d->captureResult_));\n> >>\n> >> Not as part of this patch, but I feel like Camera3RequestDescriptor\n> >> could have a method called complete() to wrap the management of the\n> >> completion against a request descriptor.\n> >>\n> >> There's lots of occasions where the metadata is obtained or processed\n> >> into a result structure and called back with a flag.\n> >>\n> >> (such as *1 above, but I think there are more)\n> >>\n> >> It seems to me like that could be made into\n> >>\n> >> \t\td->complete(CameraRequest::Success);\n> >>\n> >> or something.... But that's an overall design of breaking out the\n> >> Camera3RequestDescriptor to a full class.\n> >>\n> >> As this series is extending Camera3RequestDescriptor to maintain extra\n> >> state required for the asynchronous post-processing, that makes me think\n> >> it would be better to have a full class to manage that state. (and thus\n> >> methods to operate on it)·\n> >>\n> >>> +\t\t\tqueuedDescriptor_.pop_front();\n> >>> +\t\t} else {\n> >>> +\t\t\tbreak;\n> >>> +\t\t}\n> >>> +\t}\n> >>> }\n> >>> \n> >>> std::string CameraDevice::logPrefix() const\n> >>> diff --git a/src/android/camera_device.h b/src/android/camera_device.h\n> >>> index a5576927..36425773 100644\n> >>> --- a/src/android/camera_device.h\n> >>> +++ b/src/android/camera_device.h\n> >>> @@ -7,6 +7,7 @@\n> >>> #ifndef __ANDROID_CAMERA_DEVICE_H__\n> >>> #define __ANDROID_CAMERA_DEVICE_H__\n> >>> \n> >>> +#include \n> >>> #include \n> >>> #include \n> >>> #include \n> >>> @@ -83,6 +84,21 @@ private:\n> >>> \t\tstd::vector> frameBuffers_;\n> >>> \t\tCameraMetadata settings_;\n> >>> \t\tstd::unique_ptr request_;\n> >>> +\n> >>> +\t\t/*\n> >>> +\t\t * Below are utility placeholders used when a capture result\n> >>> +\t\t * needs to be queued before completion via\n> >>> +\t\t * process_capture_result() callback.\n> >>> +\t\t */\n> >>> +\t\tenum completionStatus {\n> >>> +\t\t\tNOT_FINISHED,\n> >>\n> >> I'd call this \"Active\" or \"Pending\" to reduce the negation...\n> >>\n> >> \t\"If (state == Active)\"\n> >> rather than\n> >> \t\"if (state != NOT_FINISHED)\"\n> >>\n> >>> +\t\t\tFINISHED_SUCCESS,\n> >>\n> >> I'd call this Success or Succeeded, or Successful ..\n> >>\n> >>> +\t\t\tFINISHED_FAILED,\n> >>\n> >> And Failed\n> >>\n> >>> +\t\t};\n> >>> +\t\tstd::unique_ptr resultMetadata_;\n> >>> +\t\tcamera3_capture_result_t captureResult_;\n> >>> +\t\tlibcamera::FrameBuffer *internalBuffer_;\n> >>> +\t\tcompletionStatus status_;\n> >>\n> >> These are all used to track the post-processor context.\n> >>\n> >>> \t};\n> >>> \n> >>> \tenum class State {\n> >>> @@ -105,6 +121,11 @@ private:\n> >>> \tstd::unique_ptr getResultMetadata(\n> >>> \t\tconst Camera3RequestDescriptor &descriptor) const;\n> >>> \n> >>> +\tvoid sendQueuedCaptureResults();\n> >>> +\tvoid streamProcessingComplete(CameraStream *stream,\n> >>> +\t\t\t\t CameraStream::ProcessStatus status,\n> >>> +\t\t\t\t CameraMetadata *resultMetadata);\n> >>> +\n> >>> \tunsigned int id_;\n> >>> \tcamera3_device_t camera3Device_;\n> >>> \n> >>> @@ -125,6 +146,8 @@ private:\n> >>> \tlibcamera::Mutex descriptorsMutex_; /* Protects descriptors_. */\n> >>> \tstd::map descriptors_;\n> >>> \n> >>> +\tstd::deque> queuedDescriptor_;\n> >>\n> >> I'm not yet sure if this should be a unique_ptr ..\n> >>\n> >>> +\n> >>> \tstd::string maker_;\n> >>> \tstd::string model_;\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 74E90BDC71\n\tfor ;\n\tWed, 8 Sep 2021 21:52:56 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C6FAD69170;\n\tWed, 8 Sep 2021 23:52:55 +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 EA93A60503\n\tfor ;\n\tWed, 8 Sep 2021 23:52:53 +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 51ABB993;\n\tWed, 8 Sep 2021 23:52:53 +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=\"sp3WBUsl\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1631137973;\n\tbh=O7LzQ2wb4HhfRdPlYymxB2ysMY/iFMG8V8aVH5ZAMUI=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=sp3WBUslpesff9n8DxaRlZv5qzYTQhqEPlWStwqsVxCmxdF5q67UUlkE1ELQE93Pb\n\tuC5O9OxU2iQaG1KfuKwOZ+XDGUbE80HKi/7fmFsCt0DLwKAqQpIVp1m4btKAfbd0Py\n\tQxo9ujGF5xzGDFVjOig+znRLUCTTdxpxsP1RNGd0=","Date":"Thu, 9 Sep 2021 00:52:33 +0300","From":"Laurent Pinchart ","To":"Umang Jain ","Message-ID":"","References":"<20210907195704.338079-1-umang.jain@ideasonboard.com>\n\t<20210907195704.338079-5-umang.jain@ideasonboard.com>\n\t\n\t\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v1 4/6] android: camera_device: Add a\n\tqueue for sending capture results","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\" "}},{"id":19562,"web_url":"https://patchwork.libcamera.org/comment/19562/","msgid":"<4fc71131-3e84-6142-d926-a5aee23afa7a@ideasonboard.com>","date":"2021-09-09T05:05:05","subject":"Re: [libcamera-devel] [PATCH v1 4/6] android: camera_device: Add a\n\tqueue for sending capture results","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hi Laurent,\n\nThanks for the inputs.\n\nOn 9/9/21 3:22 AM, Laurent Pinchart wrote:\n> Hi Umang,\n>\n> On Wed, Sep 08, 2021 at 07:53:04PM +0530, Umang Jain wrote:\n>> On 9/8/21 7:03 PM, Laurent Pinchart wrote:\n>>> On Wed, Sep 08, 2021 at 02:00:41PM +0100, Kieran Bingham wrote:\n>>>> On 07/09/2021 20:57, Umang Jain wrote:\n>>>>> When a camera capture request completes, the next step is to send the\n>>>>> capture results to the framework via process_capture_results(). All\n>>>>> the capture associated result metadata is prepared and populated. If\n>>>>> any post-processing is required, it will also take place in the same\n>>>>> thread (which can be blocking for a subtaintial amount of time) before\n>>>> s/subtaintial/substantial/\n>>>>\n>>>>> the results can be sent back to the framework.\n>>>>>\n>>>>> A follow up commit will move the post-processing to run in a separate\n>>>>> thread. In order to do so, there is few bits of groundwork which this\n>>>>> patch entails. Mainly, we need to preserve the order of sending\n>>>>> the capture results back in which they were queued by the framework\n>>>>> to the HAL (i.e. the order is sequential). Hence, we need to add a queue\n>>>>> in which capture results can be queued with context.\n>>>>>\n>>>>> As per this patch, the post-processor still runs synchronously as\n>>>>> before, but it will queue up the current capture results with context.\n>>>>> Later down the line, the capture results are dequeud in order and\n>>>> s/dequeud/dequeued/\n>>>>\n>>>>> sent back to the framework.\n>>>>>\n>>>>> The context is preserved using Camera3RequestDescriptor utility\n>>>>> structure. It has been expanded accordingly to address the needs of\n>>>>> the functionality.\n>>>>>\n>>>>> Signed-off-by: Umang Jain \n>>>>> ---\n>>>>> src/android/camera_device.cpp | 113 +++++++++++++++++++++++++++++++---\n>>>>> src/android/camera_device.h | 23 +++++++\n>>>>> 2 files changed, 126 insertions(+), 10 deletions(-)\n>>>>>\n>>>>> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n>>>>> index f2f36f32..9d4ec02e 100644\n>>>>> --- a/src/android/camera_device.cpp\n>>>>> +++ b/src/android/camera_device.cpp\n>>>>> @@ -240,6 +240,8 @@ CameraDevice::Camera3RequestDescriptor::Camera3RequestDescriptor(\n>>>>> \t/* Clone the controls associated with the camera3 request. */\n>>>>> \tsettings_ = CameraMetadata(camera3Request->settings);\n>>>>> \n>>>>> +\tinternalBuffer_ = nullptr;\n>>>>> +\n>>>>> \t/*\n>>>>> \t * Create the CaptureRequest, stored as a unique_ptr<> to tie its\n>>>>> \t * lifetime to the descriptor.\n>>>>> @@ -989,6 +991,7 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n>>>>> \t\t\t * once it has been processed.\n>>>>> \t\t\t */\n>>>>> \t\t\tbuffer = cameraStream->getBuffer();\n>>>>> +\t\t\tdescriptor.internalBuffer_ = buffer;\n>>>>> \t\t\tLOG(HAL, Debug) << ss.str() << \" (internal)\";\n>>>>> \t\t\tbreak;\n>>>>> \t\t}\n>>>>> @@ -1148,25 +1151,115 @@ void CameraDevice::requestComplete(Request *request)\n>>>>> \t\t\tcontinue;\n>>>>> \t\t}\n>>>>> \n>>>>> +\t\t/*\n>>>>> +\t\t * Save the current context of capture result and queue the\n>>>>> +\t\t * descriptor before processing the camera stream.\n>>>>> +\t\t *\n>>>>> +\t\t * When the processing completes, the descriptor will be\n>>>>> +\t\t * dequeued and capture results will be sent to the framework.\n>>>>> +\t\t */\n>>>>> +\t\tdescriptor.status_ = Camera3RequestDescriptor::NOT_FINISHED;\n>>>>> +\t\tdescriptor.resultMetadata_ = std::move(resultMetadata);\n>>>>> +\t\tdescriptor.captureResult_ = captureResult;\n>>>>> +\n>>>>> +\t\tstd::unique_ptr reqDescriptor =\n>>>>> +\t\t\tstd::make_unique();\n>>>>> +\t\t*reqDescriptor = std::move(descriptor);\n>>>>> +\t\tqueuedDescriptor_.push_back(std::move(reqDescriptor));\n>>>>> +\t\tCamera3RequestDescriptor *currentDescriptor = queuedDescriptor_.back().get();\n>>>>> +\n>>>>> +\t\tCameraMetadata *metadata = currentDescriptor->resultMetadata_.get();\n>>>>> +\t\tcameraStream->processComplete.connect(\n>>>>> +\t\t\tthis, [=](CameraStream::ProcessStatus status) {\n>>>>> +\t\t\t\tstreamProcessingComplete(cameraStream, status,\n>>>>> +\t\t\t\t\t\t\t metadata);\n>>>> I believe this to be unsafe to manage multiple operations.\n>>>>\n>>>> cameraStream->processComplete() should not be connected with per-run\n>>>> parameters. Those should be passed as the context of the signal emit(),\n>>>> not by the connection.\n>>>>\n>>>> Otherwise, the lamba will be overwritten by the next requestComplete()\n>>>> and incorrect parameters will be processed when the signal actually fires.\n>> While this is true.. and a bug in my  code\n>>\n>>> It's worse than that, ever connect() will create a new connection, so\n>>> the slot will be called once for the first request, twice for the\n>>> second, ...\n>> Oh, I gotta see this in action by sprinkling some printfs...\n>>\n>> But What is a alternative to this? While I tried to disconnect signal\n>> handlers in the slot the last time, it came back with:\n>>\n>> '''\n>> While signals can be connected and disconnected on demand, it often\n>> indicates a design issue. Is there a reason why you can't connect the\n>> signal when the cameraStream instance is created, and keep it connected\n>> for the whole lifetime of the stream ?\n>> '''\n>>\n>> in\n>> https://lists.libcamera.org/pipermail/libcamera-devel/2021-August/023936.html\n>>\n>> Not sure if it's possible to check on a object's signal to check for a\n>> slot's existence. And doing so, feels weird as well.\n>>\n>> So we need to connect to slot /only/ once, for all post-processing needs\n>> to be done in future. This sounds like an operation that can sit well in\n>> CameraStream::configure().\n> Correct. You've removed the disconnection based on my comments for the\n> previous version, but the connection also needs to be addressed :-)\n> Connecting in\n>\n>> But how can we pass a slot (which is probably\n>> private to class) to another class's configure()? A function object ?\n> You can create a slot in CameraStream, and from there call\n>\n> \tcameraDevice_->streamProcessingComplete(this, request, status);\n>\n> request should be a pointer to Camera3RequestDescriptor (which we could\n> rename to Camera3Request), it's the object that models the context of\n\n\nI would leave re-naming stuff for now. Camera3RequestDescriptor seems to \nbe getting bigger and we (Kieran and I) have discussed re-working it to \nreduce code duplication and efficient use of the structure. I am, in the \nfavour of, not doing that as part of this series, but another series \nwhich is solely dedicated to the refactor of this.\n\n> the processing. You need to pass it to cameraStream->process() in the\n> first place of course.\n\n\nThis would require exposing Camera3RequestDescriptor struct To \nCameraStream class and PostProcessor. Probably it worth to break it out \nfrom CameraDevice private: and set the definition sit outside the scope \nof CameraDevice\n\nI quickly compile-tested this to check: https://paste.debian.net/1211030/\n\n>\n> It's tempting to merge Camera3RequestDescriptor with CaptureRequest, but\n> let's not do that, as fence handling needs to move to the libcamera\n> core, so the CameraWorker will disappear then, and so will\n> CaptureRequest.\n>\n>>>>> +\t\t\t});\n>>>>> +\n>>>>> \t\tint ret = cameraStream->process(src, *buffer.buffer,\n>>>>> -\t\t\t\t\t\tdescriptor.settings_,\n>>>>> -\t\t\t\t\t\tresultMetadata.get());\n>>>>> +\t\t\t\t\t\tcurrentDescriptor->settings_,\n>>>>> +\t\t\t\t\t\tmetadata);\n>>>>> +\t\treturn;\n>>>>> +\t}\n>>>>> +\n>>>>> +\tif (queuedDescriptor_.empty()) {\n>>>>> +\t\tcaptureResult.result = resultMetadata->get();\n>>>>> +\t\tcallbacks_->process_capture_result(callbacks_, &captureResult);\n>>>> *1 duplication of completion callbacks (see below)\n>>>>\n>>>>> +\t} else {\n>>>>> +\t\t/*\n>>>>> +\t\t * Previous capture results waiting to be sent to framework\n>>>>> +\t\t * hence, queue the current capture results as well. After that,\n>>>>> +\t\t * check if any results are ready to be sent back to the\n>>>>> +\t\t * framework.\n>>>>> +\t\t */\n>>>>> +\t\tdescriptor.status_ = Camera3RequestDescriptor::FINISHED_SUCCESS;\n>>>>> +\t\tdescriptor.resultMetadata_ = std::move(resultMetadata);\n>>>>> +\t\tdescriptor.captureResult_ = captureResult;\n>>>>> +\t\tstd::unique_ptr reqDescriptor =\n>>>>> +\t\t\tstd::make_unique();\n>>>>> +\t\t*reqDescriptor = std::move(descriptor);\n>>>>> +\t\tqueuedDescriptor_.push_back(std::move(reqDescriptor));\n>>>>> +\n>>>>> +\t\tsendQueuedCaptureResults();\n>>>>> +\t}\n>>>>> +}\n>>>>> +\n>>>>> +void CameraDevice::streamProcessingComplete(CameraStream *cameraStream,\n>>>>> +\t\t\t\t\t CameraStream::ProcessStatus status,\n>>>>> +\t\t\t\t\t CameraMetadata *resultMetadata)\n>>>>> +{\n>>>>> +\tfor (auto &d : queuedDescriptor_) {\n>>>>> +\t\tif (d->resultMetadata_.get() != resultMetadata)\n>>>>> +\t\t\tcontinue;\n> This is a hack that you'll be able to drop once you get the pointer to\n> the Camera3RequestDescriptor passed as a function argument.\n\n\nYep, I was thinking to pass the frameNumber alternatively ... and \ncompare it here to find the descriptor..\n\n>\n>>>>> +\n>>>>> \t\t/*\n>>>>> \t\t * Return the FrameBuffer to the CameraStream now that we're\n>>>>> \t\t * done processing it.\n>>>>> \t\t */\n>>>>> \t\tif (cameraStream->type() == CameraStream::Type::Internal)\n>>>>> -\t\t\tcameraStream->putBuffer(src);\n>>>>> -\n>>>>> -\t\tif (ret) {\n>>>>> -\t\t\tbuffer.status = CAMERA3_BUFFER_STATUS_ERROR;\n>>>>> -\t\t\tnotifyError(descriptor.frameNumber_, buffer.stream,\n>>>>> -\t\t\t\t CAMERA3_MSG_ERROR_BUFFER);\n>>>>> +\t\t\tcameraStream->putBuffer(d->internalBuffer_);\n>>>>> +\n>>>>> +\t\tif (status == CameraStream::ProcessStatus::Success) {\n>>>>> +\t\t\td->status_ = Camera3RequestDescriptor::FINISHED_SUCCESS;\n>>>>> +\t\t} else {\n>>>>> +\t\t\td->status_ = Camera3RequestDescriptor::FINISHED_FAILED;\n>>>>> +\t\t\td->captureResult_.partial_result = 0;\n>>>>> +\t\t\tfor (camera3_stream_buffer_t &buffer : d->buffers_) {\n>>>>> +\t\t\t\tCameraStream *cs = static_cast(buffer.stream->priv);\n>>>>> +\n>>>>> +\t\t\t\tif (cs->camera3Stream().format != HAL_PIXEL_FORMAT_BLOB)\n>>>>> +\t\t\t\t\tcontinue;\n>>>>> +\n>>>>> +\t\t\t\tbuffer.status = CAMERA3_BUFFER_STATUS_ERROR;\n>>>>> +\t\t\t\tnotifyError(d->frameNumber_, buffer.stream,\n>>>>> +\t\t\t\t\t CAMERA3_MSG_ERROR_BUFFER);\n>>>>> +\t\t\t}\n>>>>> \t\t}\n>>>>> +\n>>>>> +\t\tbreak;\n>>>>> \t}\n>>>>> \n>>>>> -\tcaptureResult.result = resultMetadata->get();\n>>>>> -\tcallbacks_->process_capture_result(callbacks_, &captureResult);\n>>>>> +\t/*\n>>>>> +\t * Send back capture results to the framework by inspecting the queue.\n>>>>> +\t * The framework can defer queueing further requests to the HAL (via\n>>>>> +\t * process_capture_request) unless until it receives the capture results\n>>>>> +\t * for already queued requests.\n>>>>> +\t */\n>>>>> +\tsendQueuedCaptureResults();\n>>>>> +}\n>>>>> +\n>>>>> +void CameraDevice::sendQueuedCaptureResults()\n>>>>> +{\n>>>>> +\twhile (!queuedDescriptor_.empty()) {\n>>>>> +\t\tstd::unique_ptr &d = queuedDescriptor_.front();\n>>>> I'd have this exit early if the front descriptor is not finished.\n>>>>\n>>>>> +\t\tif (d->status_ != Camera3RequestDescriptor::NOT_FINISHED) {\n>>>>> +\t\t\td->captureResult_.result = d->resultMetadata_->get();\n>>>>> +\t\t\tcallbacks_->process_capture_result(callbacks_,\n>>>>> +\t\t\t\t\t\t\t &(d->captureResult_));\n>>>> Not as part of this patch, but I feel like Camera3RequestDescriptor\n>>>> could have a method called complete() to wrap the management of the\n>>>> completion against a request descriptor.\n>>>>\n>>>> There's lots of occasions where the metadata is obtained or processed\n>>>> into a result structure and called back with a flag.\n>>>>\n>>>> (such as *1 above, but I think there are more)\n>>>>\n>>>> It seems to me like that could be made into\n>>>>\n>>>> \t\td->complete(CameraRequest::Success);\n>>>>\n>>>> or something.... But that's an overall design of breaking out the\n>>>> Camera3RequestDescriptor to a full class.\n>>>>\n>>>> As this series is extending Camera3RequestDescriptor to maintain extra\n>>>> state required for the asynchronous post-processing, that makes me think\n>>>> it would be better to have a full class to manage that state. (and thus\n>>>> methods to operate on it)·\n>>>>\n>>>>> +\t\t\tqueuedDescriptor_.pop_front();\n>>>>> +\t\t} else {\n>>>>> +\t\t\tbreak;\n>>>>> +\t\t}\n>>>>> +\t}\n>>>>> }\n>>>>> \n>>>>> std::string CameraDevice::logPrefix() const\n>>>>> diff --git a/src/android/camera_device.h b/src/android/camera_device.h\n>>>>> index a5576927..36425773 100644\n>>>>> --- a/src/android/camera_device.h\n>>>>> +++ b/src/android/camera_device.h\n>>>>> @@ -7,6 +7,7 @@\n>>>>> #ifndef __ANDROID_CAMERA_DEVICE_H__\n>>>>> #define __ANDROID_CAMERA_DEVICE_H__\n>>>>> \n>>>>> +#include \n>>>>> #include \n>>>>> #include \n>>>>> #include \n>>>>> @@ -83,6 +84,21 @@ private:\n>>>>> \t\tstd::vector> frameBuffers_;\n>>>>> \t\tCameraMetadata settings_;\n>>>>> \t\tstd::unique_ptr request_;\n>>>>> +\n>>>>> +\t\t/*\n>>>>> +\t\t * Below are utility placeholders used when a capture result\n>>>>> +\t\t * needs to be queued before completion via\n>>>>> +\t\t * process_capture_result() callback.\n>>>>> +\t\t */\n>>>>> +\t\tenum completionStatus {\n>>>>> +\t\t\tNOT_FINISHED,\n>>>> I'd call this \"Active\" or \"Pending\" to reduce the negation...\n>>>>\n>>>> \t\"If (state == Active)\"\n>>>> rather than\n>>>> \t\"if (state != NOT_FINISHED)\"\n>>>>\n>>>>> +\t\t\tFINISHED_SUCCESS,\n>>>> I'd call this Success or Succeeded, or Successful ..\n>>>>\n>>>>> +\t\t\tFINISHED_FAILED,\n>>>> And Failed\n>>>>\n>>>>> +\t\t};\n>>>>> +\t\tstd::unique_ptr resultMetadata_;\n>>>>> +\t\tcamera3_capture_result_t captureResult_;\n>>>>> +\t\tlibcamera::FrameBuffer *internalBuffer_;\n>>>>> +\t\tcompletionStatus status_;\n>>>> These are all used to track the post-processor context.\n>>>>\n>>>>> \t};\n>>>>> \n>>>>> \tenum class State {\n>>>>> @@ -105,6 +121,11 @@ private:\n>>>>> \tstd::unique_ptr getResultMetadata(\n>>>>> \t\tconst Camera3RequestDescriptor &descriptor) const;\n>>>>> \n>>>>> +\tvoid sendQueuedCaptureResults();\n>>>>> +\tvoid streamProcessingComplete(CameraStream *stream,\n>>>>> +\t\t\t\t CameraStream::ProcessStatus status,\n>>>>> +\t\t\t\t CameraMetadata *resultMetadata);\n>>>>> +\n>>>>> \tunsigned int id_;\n>>>>> \tcamera3_device_t camera3Device_;\n>>>>> \n>>>>> @@ -125,6 +146,8 @@ private:\n>>>>> \tlibcamera::Mutex descriptorsMutex_; /* Protects descriptors_. */\n>>>>> \tstd::map descriptors_;\n>>>>> \n>>>>> +\tstd::deque> queuedDescriptor_;\n>>>> I'm not yet sure if this should be a unique_ptr ..\n>>>>\n>>>>> +\n>>>>> \tstd::string maker_;\n>>>>> \tstd::string model_;\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 53898BDB1D\n\tfor ;\n\tThu, 9 Sep 2021 05:05:13 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A526D69169;\n\tThu, 9 Sep 2021 07:05:12 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 5B07A6024A\n\tfor ;\n\tThu, 9 Sep 2021 07:05:11 +0200 (CEST)","from [192.168.1.104] (unknown [103.251.226.149])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id E5CF2466;\n\tThu, 9 Sep 2021 07:05:09 +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=\"v6q5n79w\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1631163910;\n\tbh=pDRLUUlS1Aits18q1nFlXNOYGdT0K2YBwwmF72npdjU=;\n\th=Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=v6q5n79w0x+Ks2cRYHFqNAYcWSHSkMTK0Y6/70iz1Na7pgdtpRNoh+u4ocrlpjiCu\n\tMK7/6TF9G4c88k9bAY6uJCVVFE41TH42LzAlyHfJpknqBlWCDJZVhRIiA+sPMrTJ3L\n\t+hVL9nxAW8Rw3nEEKd0F4ojoJV1aGaGq58hg2MgY=","To":"Laurent Pinchart ","References":"<20210907195704.338079-1-umang.jain@ideasonboard.com>\n\t<20210907195704.338079-5-umang.jain@ideasonboard.com>\n\t\n\t\n\t\n\t","From":"Umang Jain ","Message-ID":"<4fc71131-3e84-6142-d926-a5aee23afa7a@ideasonboard.com>","Date":"Thu, 9 Sep 2021 10:35:05 +0530","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.10.2","MIME-Version":"1.0","In-Reply-To":"","Content-Type":"text/plain; charset=utf-8; format=flowed","Content-Transfer-Encoding":"8bit","Content-Language":"en-US","Subject":"Re: [libcamera-devel] [PATCH v1 4/6] android: camera_device: Add a\n\tqueue for sending capture results","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\" "}},{"id":19594,"web_url":"https://patchwork.libcamera.org/comment/19594/","msgid":"","date":"2021-09-10T04:25:50","subject":"Re: [libcamera-devel] [PATCH v1 4/6] android: camera_device: Add a\n\tqueue for sending capture results","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Umang,\n\nOn Thu, Sep 09, 2021 at 10:35:05AM +0530, Umang Jain wrote:\n> On 9/9/21 3:22 AM, Laurent Pinchart wrote:\n> > On Wed, Sep 08, 2021 at 07:53:04PM +0530, Umang Jain wrote:\n> >> On 9/8/21 7:03 PM, Laurent Pinchart wrote:\n> >>> On Wed, Sep 08, 2021 at 02:00:41PM +0100, Kieran Bingham wrote:\n> >>>> On 07/09/2021 20:57, Umang Jain wrote:\n> >>>>> When a camera capture request completes, the next step is to send the\n> >>>>> capture results to the framework via process_capture_results(). All\n> >>>>> the capture associated result metadata is prepared and populated. If\n> >>>>> any post-processing is required, it will also take place in the same\n> >>>>> thread (which can be blocking for a subtaintial amount of time) before\n> >>>> s/subtaintial/substantial/\n> >>>>\n> >>>>> the results can be sent back to the framework.\n> >>>>>\n> >>>>> A follow up commit will move the post-processing to run in a separate\n> >>>>> thread. In order to do so, there is few bits of groundwork which this\n> >>>>> patch entails. Mainly, we need to preserve the order of sending\n> >>>>> the capture results back in which they were queued by the framework\n> >>>>> to the HAL (i.e. the order is sequential). Hence, we need to add a queue\n> >>>>> in which capture results can be queued with context.\n> >>>>>\n> >>>>> As per this patch, the post-processor still runs synchronously as\n> >>>>> before, but it will queue up the current capture results with context.\n> >>>>> Later down the line, the capture results are dequeud in order and\n> >>>> s/dequeud/dequeued/\n> >>>>\n> >>>>> sent back to the framework.\n> >>>>>\n> >>>>> The context is preserved using Camera3RequestDescriptor utility\n> >>>>> structure. It has been expanded accordingly to address the needs of\n> >>>>> the functionality.\n> >>>>>\n> >>>>> Signed-off-by: Umang Jain \n> >>>>> ---\n> >>>>> src/android/camera_device.cpp | 113 +++++++++++++++++++++++++++++++---\n> >>>>> src/android/camera_device.h | 23 +++++++\n> >>>>> 2 files changed, 126 insertions(+), 10 deletions(-)\n> >>>>>\n> >>>>> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> >>>>> index f2f36f32..9d4ec02e 100644\n> >>>>> --- a/src/android/camera_device.cpp\n> >>>>> +++ b/src/android/camera_device.cpp\n> >>>>> @@ -240,6 +240,8 @@ CameraDevice::Camera3RequestDescriptor::Camera3RequestDescriptor(\n> >>>>> \t/* Clone the controls associated with the camera3 request. */\n> >>>>> \tsettings_ = CameraMetadata(camera3Request->settings);\n> >>>>> \n> >>>>> +\tinternalBuffer_ = nullptr;\n> >>>>> +\n> >>>>> \t/*\n> >>>>> \t * Create the CaptureRequest, stored as a unique_ptr<> to tie its\n> >>>>> \t * lifetime to the descriptor.\n> >>>>> @@ -989,6 +991,7 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n> >>>>> \t\t\t * once it has been processed.\n> >>>>> \t\t\t */\n> >>>>> \t\t\tbuffer = cameraStream->getBuffer();\n> >>>>> +\t\t\tdescriptor.internalBuffer_ = buffer;\n> >>>>> \t\t\tLOG(HAL, Debug) << ss.str() << \" (internal)\";\n> >>>>> \t\t\tbreak;\n> >>>>> \t\t}\n> >>>>> @@ -1148,25 +1151,115 @@ void CameraDevice::requestComplete(Request *request)\n> >>>>> \t\t\tcontinue;\n> >>>>> \t\t}\n> >>>>> \n> >>>>> +\t\t/*\n> >>>>> +\t\t * Save the current context of capture result and queue the\n> >>>>> +\t\t * descriptor before processing the camera stream.\n> >>>>> +\t\t *\n> >>>>> +\t\t * When the processing completes, the descriptor will be\n> >>>>> +\t\t * dequeued and capture results will be sent to the framework.\n> >>>>> +\t\t */\n> >>>>> +\t\tdescriptor.status_ = Camera3RequestDescriptor::NOT_FINISHED;\n> >>>>> +\t\tdescriptor.resultMetadata_ = std::move(resultMetadata);\n> >>>>> +\t\tdescriptor.captureResult_ = captureResult;\n> >>>>> +\n> >>>>> +\t\tstd::unique_ptr reqDescriptor =\n> >>>>> +\t\t\tstd::make_unique();\n> >>>>> +\t\t*reqDescriptor = std::move(descriptor);\n> >>>>> +\t\tqueuedDescriptor_.push_back(std::move(reqDescriptor));\n> >>>>> +\t\tCamera3RequestDescriptor *currentDescriptor = queuedDescriptor_.back().get();\n> >>>>> +\n> >>>>> +\t\tCameraMetadata *metadata = currentDescriptor->resultMetadata_.get();\n> >>>>> +\t\tcameraStream->processComplete.connect(\n> >>>>> +\t\t\tthis, [=](CameraStream::ProcessStatus status) {\n> >>>>> +\t\t\t\tstreamProcessingComplete(cameraStream, status,\n> >>>>> +\t\t\t\t\t\t\t metadata);\n> >>>> I believe this to be unsafe to manage multiple operations.\n> >>>>\n> >>>> cameraStream->processComplete() should not be connected with per-run\n> >>>> parameters. Those should be passed as the context of the signal emit(),\n> >>>> not by the connection.\n> >>>>\n> >>>> Otherwise, the lamba will be overwritten by the next requestComplete()\n> >>>> and incorrect parameters will be processed when the signal actually fires.\n> >> While this is true.. and a bug in my  code\n> >>\n> >>> It's worse than that, ever connect() will create a new connection, so\n> >>> the slot will be called once for the first request, twice for the\n> >>> second, ...\n> >> Oh, I gotta see this in action by sprinkling some printfs...\n> >>\n> >> But What is a alternative to this? While I tried to disconnect signal\n> >> handlers in the slot the last time, it came back with:\n> >>\n> >> '''\n> >> While signals can be connected and disconnected on demand, it often\n> >> indicates a design issue. Is there a reason why you can't connect the\n> >> signal when the cameraStream instance is created, and keep it connected\n> >> for the whole lifetime of the stream ?\n> >> '''\n> >>\n> >> in\n> >> https://lists.libcamera.org/pipermail/libcamera-devel/2021-August/023936.html\n> >>\n> >> Not sure if it's possible to check on a object's signal to check for a\n> >> slot's existence. And doing so, feels weird as well.\n> >>\n> >> So we need to connect to slot /only/ once, for all post-processing needs\n> >> to be done in future. This sounds like an operation that can sit well in\n> >> CameraStream::configure().\n> > Correct. You've removed the disconnection based on my comments for the\n> > previous version, but the connection also needs to be addressed :-)\n> > Connecting in\n> >\n> >> But how can we pass a slot (which is probably\n> >> private to class) to another class's configure()? A function object ?\n> > You can create a slot in CameraStream, and from there call\n> >\n> > \tcameraDevice_->streamProcessingComplete(this, request, status);\n> >\n> > request should be a pointer to Camera3RequestDescriptor (which we could\n> > rename to Camera3Request), it's the object that models the context of\n> \n> I would leave re-naming stuff for now.\n\nOK.\n\n> Camera3RequestDescriptor seems to \n> be getting bigger and we (Kieran and I) have discussed re-working it to \n> reduce code duplication and efficient use of the structure. I am, in the \n> favour of, not doing that as part of this series, but another series \n> which is solely dedicated to the refactor of this.\n\nThat's fine with me, but I think that refactoring series should then\ncome before this one.\n\n> > the processing. You need to pass it to cameraStream->process() in the\n> > first place of course.\n> \n> This would require exposing Camera3RequestDescriptor struct To \n> CameraStream class and PostProcessor. Probably it worth to break it out \n> from CameraDevice private: and set the definition sit outside the scope \n> of CameraDevice\n> \n> I quickly compile-tested this to check: https://paste.debian.net/1211030/\n\nSoudns good to me.\n\n> > It's tempting to merge Camera3RequestDescriptor with CaptureRequest, but\n> > let's not do that, as fence handling needs to move to the libcamera\n> > core, so the CameraWorker will disappear then, and so will\n> > CaptureRequest.\n> >\n> >>>>> +\t\t\t});\n> >>>>> +\n> >>>>> \t\tint ret = cameraStream->process(src, *buffer.buffer,\n> >>>>> -\t\t\t\t\t\tdescriptor.settings_,\n> >>>>> -\t\t\t\t\t\tresultMetadata.get());\n> >>>>> +\t\t\t\t\t\tcurrentDescriptor->settings_,\n> >>>>> +\t\t\t\t\t\tmetadata);\n> >>>>> +\t\treturn;\n> >>>>> +\t}\n> >>>>> +\n> >>>>> +\tif (queuedDescriptor_.empty()) {\n> >>>>> +\t\tcaptureResult.result = resultMetadata->get();\n> >>>>> +\t\tcallbacks_->process_capture_result(callbacks_, &captureResult);\n> >>>> *1 duplication of completion callbacks (see below)\n> >>>>\n> >>>>> +\t} else {\n> >>>>> +\t\t/*\n> >>>>> +\t\t * Previous capture results waiting to be sent to framework\n> >>>>> +\t\t * hence, queue the current capture results as well. After that,\n> >>>>> +\t\t * check if any results are ready to be sent back to the\n> >>>>> +\t\t * framework.\n> >>>>> +\t\t */\n> >>>>> +\t\tdescriptor.status_ = Camera3RequestDescriptor::FINISHED_SUCCESS;\n> >>>>> +\t\tdescriptor.resultMetadata_ = std::move(resultMetadata);\n> >>>>> +\t\tdescriptor.captureResult_ = captureResult;\n> >>>>> +\t\tstd::unique_ptr reqDescriptor =\n> >>>>> +\t\t\tstd::make_unique();\n> >>>>> +\t\t*reqDescriptor = std::move(descriptor);\n> >>>>> +\t\tqueuedDescriptor_.push_back(std::move(reqDescriptor));\n> >>>>> +\n> >>>>> +\t\tsendQueuedCaptureResults();\n> >>>>> +\t}\n> >>>>> +}\n> >>>>> +\n> >>>>> +void CameraDevice::streamProcessingComplete(CameraStream *cameraStream,\n> >>>>> +\t\t\t\t\t CameraStream::ProcessStatus status,\n> >>>>> +\t\t\t\t\t CameraMetadata *resultMetadata)\n> >>>>> +{\n> >>>>> +\tfor (auto &d : queuedDescriptor_) {\n> >>>>> +\t\tif (d->resultMetadata_.get() != resultMetadata)\n> >>>>> +\t\t\tcontinue;\n> >\n> > This is a hack that you'll be able to drop once you get the pointer to\n> > the Camera3RequestDescriptor passed as a function argument.\n> \n> Yep, I was thinking to pass the frameNumber alternatively ... and \n> compare it here to find the descriptor..\n> \n> >>>>> +\n> >>>>> \t\t/*\n> >>>>> \t\t * Return the FrameBuffer to the CameraStream now that we're\n> >>>>> \t\t * done processing it.\n> >>>>> \t\t */\n> >>>>> \t\tif (cameraStream->type() == CameraStream::Type::Internal)\n> >>>>> -\t\t\tcameraStream->putBuffer(src);\n> >>>>> -\n> >>>>> -\t\tif (ret) {\n> >>>>> -\t\t\tbuffer.status = CAMERA3_BUFFER_STATUS_ERROR;\n> >>>>> -\t\t\tnotifyError(descriptor.frameNumber_, buffer.stream,\n> >>>>> -\t\t\t\t CAMERA3_MSG_ERROR_BUFFER);\n> >>>>> +\t\t\tcameraStream->putBuffer(d->internalBuffer_);\n> >>>>> +\n> >>>>> +\t\tif (status == CameraStream::ProcessStatus::Success) {\n> >>>>> +\t\t\td->status_ = Camera3RequestDescriptor::FINISHED_SUCCESS;\n> >>>>> +\t\t} else {\n> >>>>> +\t\t\td->status_ = Camera3RequestDescriptor::FINISHED_FAILED;\n> >>>>> +\t\t\td->captureResult_.partial_result = 0;\n> >>>>> +\t\t\tfor (camera3_stream_buffer_t &buffer : d->buffers_) {\n> >>>>> +\t\t\t\tCameraStream *cs = static_cast(buffer.stream->priv);\n> >>>>> +\n> >>>>> +\t\t\t\tif (cs->camera3Stream().format != HAL_PIXEL_FORMAT_BLOB)\n> >>>>> +\t\t\t\t\tcontinue;\n> >>>>> +\n> >>>>> +\t\t\t\tbuffer.status = CAMERA3_BUFFER_STATUS_ERROR;\n> >>>>> +\t\t\t\tnotifyError(d->frameNumber_, buffer.stream,\n> >>>>> +\t\t\t\t\t CAMERA3_MSG_ERROR_BUFFER);\n> >>>>> +\t\t\t}\n> >>>>> \t\t}\n> >>>>> +\n> >>>>> +\t\tbreak;\n> >>>>> \t}\n> >>>>> \n> >>>>> -\tcaptureResult.result = resultMetadata->get();\n> >>>>> -\tcallbacks_->process_capture_result(callbacks_, &captureResult);\n> >>>>> +\t/*\n> >>>>> +\t * Send back capture results to the framework by inspecting the queue.\n> >>>>> +\t * The framework can defer queueing further requests to the HAL (via\n> >>>>> +\t * process_capture_request) unless until it receives the capture results\n> >>>>> +\t * for already queued requests.\n> >>>>> +\t */\n> >>>>> +\tsendQueuedCaptureResults();\n> >>>>> +}\n> >>>>> +\n> >>>>> +void CameraDevice::sendQueuedCaptureResults()\n> >>>>> +{\n> >>>>> +\twhile (!queuedDescriptor_.empty()) {\n> >>>>> +\t\tstd::unique_ptr &d = queuedDescriptor_.front();\n> >>>> I'd have this exit early if the front descriptor is not finished.\n> >>>>\n> >>>>> +\t\tif (d->status_ != Camera3RequestDescriptor::NOT_FINISHED) {\n> >>>>> +\t\t\td->captureResult_.result = d->resultMetadata_->get();\n> >>>>> +\t\t\tcallbacks_->process_capture_result(callbacks_,\n> >>>>> +\t\t\t\t\t\t\t &(d->captureResult_));\n> >>>> Not as part of this patch, but I feel like Camera3RequestDescriptor\n> >>>> could have a method called complete() to wrap the management of the\n> >>>> completion against a request descriptor.\n> >>>>\n> >>>> There's lots of occasions where the metadata is obtained or processed\n> >>>> into a result structure and called back with a flag.\n> >>>>\n> >>>> (such as *1 above, but I think there are more)\n> >>>>\n> >>>> It seems to me like that could be made into\n> >>>>\n> >>>> \t\td->complete(CameraRequest::Success);\n> >>>>\n> >>>> or something.... But that's an overall design of breaking out the\n> >>>> Camera3RequestDescriptor to a full class.\n> >>>>\n> >>>> As this series is extending Camera3RequestDescriptor to maintain extra\n> >>>> state required for the asynchronous post-processing, that makes me think\n> >>>> it would be better to have a full class to manage that state. (and thus\n> >>>> methods to operate on it)·\n> >>>>\n> >>>>> +\t\t\tqueuedDescriptor_.pop_front();\n> >>>>> +\t\t} else {\n> >>>>> +\t\t\tbreak;\n> >>>>> +\t\t}\n> >>>>> +\t}\n> >>>>> }\n> >>>>> \n> >>>>> std::string CameraDevice::logPrefix() const\n> >>>>> diff --git a/src/android/camera_device.h b/src/android/camera_device.h\n> >>>>> index a5576927..36425773 100644\n> >>>>> --- a/src/android/camera_device.h\n> >>>>> +++ b/src/android/camera_device.h\n> >>>>> @@ -7,6 +7,7 @@\n> >>>>> #ifndef __ANDROID_CAMERA_DEVICE_H__\n> >>>>> #define __ANDROID_CAMERA_DEVICE_H__\n> >>>>> \n> >>>>> +#include \n> >>>>> #include \n> >>>>> #include \n> >>>>> #include \n> >>>>> @@ -83,6 +84,21 @@ private:\n> >>>>> \t\tstd::vector> frameBuffers_;\n> >>>>> \t\tCameraMetadata settings_;\n> >>>>> \t\tstd::unique_ptr request_;\n> >>>>> +\n> >>>>> +\t\t/*\n> >>>>> +\t\t * Below are utility placeholders used when a capture result\n> >>>>> +\t\t * needs to be queued before completion via\n> >>>>> +\t\t * process_capture_result() callback.\n> >>>>> +\t\t */\n> >>>>> +\t\tenum completionStatus {\n> >>>>> +\t\t\tNOT_FINISHED,\n> >>>> I'd call this \"Active\" or \"Pending\" to reduce the negation...\n> >>>>\n> >>>> \t\"If (state == Active)\"\n> >>>> rather than\n> >>>> \t\"if (state != NOT_FINISHED)\"\n> >>>>\n> >>>>> +\t\t\tFINISHED_SUCCESS,\n> >>>> I'd call this Success or Succeeded, or Successful ..\n> >>>>\n> >>>>> +\t\t\tFINISHED_FAILED,\n> >>>> And Failed\n> >>>>\n> >>>>> +\t\t};\n> >>>>> +\t\tstd::unique_ptr resultMetadata_;\n> >>>>> +\t\tcamera3_capture_result_t captureResult_;\n> >>>>> +\t\tlibcamera::FrameBuffer *internalBuffer_;\n> >>>>> +\t\tcompletionStatus status_;\n> >>>> These are all used to track the post-processor context.\n> >>>>\n> >>>>> \t};\n> >>>>> \n> >>>>> \tenum class State {\n> >>>>> @@ -105,6 +121,11 @@ private:\n> >>>>> \tstd::unique_ptr getResultMetadata(\n> >>>>> \t\tconst Camera3RequestDescriptor &descriptor) const;\n> >>>>> \n> >>>>> +\tvoid sendQueuedCaptureResults();\n> >>>>> +\tvoid streamProcessingComplete(CameraStream *stream,\n> >>>>> +\t\t\t\t CameraStream::ProcessStatus status,\n> >>>>> +\t\t\t\t CameraMetadata *resultMetadata);\n> >>>>> +\n> >>>>> \tunsigned int id_;\n> >>>>> \tcamera3_device_t camera3Device_;\n> >>>>> \n> >>>>> @@ -125,6 +146,8 @@ private:\n> >>>>> \tlibcamera::Mutex descriptorsMutex_; /* Protects descriptors_. */\n> >>>>> \tstd::map descriptors_;\n> >>>>> \n> >>>>> +\tstd::deque> queuedDescriptor_;\n> >>>> I'm not yet sure if this should be a unique_ptr ..\n> >>>>\n> >>>>> +\n> >>>>> \tstd::string maker_;\n> >>>>> \tstd::string model_;\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 D3E7CBDC71\n\tfor ;\n\tFri, 10 Sep 2021 04:26:14 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 6065269170;\n\tFri, 10 Sep 2021 06:26:14 +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 4643F60249\n\tfor ;\n\tFri, 10 Sep 2021 06:26:13 +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 AAF51883;\n\tFri, 10 Sep 2021 06:26:12 +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=\"khIWSwxM\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1631247972;\n\tbh=xkWIFLSQXbGwDh4Vr6UdSEGnwVbP4M7W2vpH6F07uw8=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=khIWSwxMEKliuivMcvnx2zK2CWpKv9SS5CICk/Hojyx3gAwfvbzkwLsNwImBAYqXi\n\tWunzZ4yS2IAxMwtN583IGjL0dSxcd2zgkWoKRvdci/Y06Bzy3h4rBRxsXwhlf5eRRw\n\tGvA1L4UkjXiD3kOJYNCO+bpeLWuWXONYhUJD7Esc=","Date":"Fri, 10 Sep 2021 07:25:50 +0300","From":"Laurent Pinchart ","To":"Umang Jain ","Message-ID":"","References":"<20210907195704.338079-1-umang.jain@ideasonboard.com>\n\t<20210907195704.338079-5-umang.jain@ideasonboard.com>\n\t\n\t\n\t\n\t\n\t<4fc71131-3e84-6142-d926-a5aee23afa7a@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<4fc71131-3e84-6142-d926-a5aee23afa7a@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v1 4/6] android: camera_device: Add a\n\tqueue for sending capture results","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\" "}},{"id":19595,"web_url":"https://patchwork.libcamera.org/comment/19595/","msgid":"<2d35ffe6-9e83-9725-cfaa-40cc4520de4a@ideasonboard.com>","date":"2021-09-10T04:48:50","subject":"Re: [libcamera-devel] [PATCH v1 4/6] android: camera_device: Add a\n\tqueue for sending capture results","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hi Laurent,\n\nOn 9/10/21 9:55 AM, Laurent Pinchart wrote:\n> Hi Umang,\n>\n> On Thu, Sep 09, 2021 at 10:35:05AM +0530, Umang Jain wrote:\n>> On 9/9/21 3:22 AM, Laurent Pinchart wrote:\n>>> On Wed, Sep 08, 2021 at 07:53:04PM +0530, Umang Jain wrote:\n>>>> On 9/8/21 7:03 PM, Laurent Pinchart wrote:\n>>>>> On Wed, Sep 08, 2021 at 02:00:41PM +0100, Kieran Bingham wrote:\n>>>>>> On 07/09/2021 20:57, Umang Jain wrote:\n>>>>>>> When a camera capture request completes, the next step is to send the\n>>>>>>> capture results to the framework via process_capture_results(). All\n>>>>>>> the capture associated result metadata is prepared and populated. If\n>>>>>>> any post-processing is required, it will also take place in the same\n>>>>>>> thread (which can be blocking for a subtaintial amount of time) before\n>>>>>> s/subtaintial/substantial/\n>>>>>>\n>>>>>>> the results can be sent back to the framework.\n>>>>>>>\n>>>>>>> A follow up commit will move the post-processing to run in a separate\n>>>>>>> thread. In order to do so, there is few bits of groundwork which this\n>>>>>>> patch entails. Mainly, we need to preserve the order of sending\n>>>>>>> the capture results back in which they were queued by the framework\n>>>>>>> to the HAL (i.e. the order is sequential). Hence, we need to add a queue\n>>>>>>> in which capture results can be queued with context.\n>>>>>>>\n>>>>>>> As per this patch, the post-processor still runs synchronously as\n>>>>>>> before, but it will queue up the current capture results with context.\n>>>>>>> Later down the line, the capture results are dequeud in order and\n>>>>>> s/dequeud/dequeued/\n>>>>>>\n>>>>>>> sent back to the framework.\n>>>>>>>\n>>>>>>> The context is preserved using Camera3RequestDescriptor utility\n>>>>>>> structure. It has been expanded accordingly to address the needs of\n>>>>>>> the functionality.\n>>>>>>>\n>>>>>>> Signed-off-by: Umang Jain \n>>>>>>> ---\n>>>>>>> src/android/camera_device.cpp | 113 +++++++++++++++++++++++++++++++---\n>>>>>>> src/android/camera_device.h | 23 +++++++\n>>>>>>> 2 files changed, 126 insertions(+), 10 deletions(-)\n>>>>>>>\n>>>>>>> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n>>>>>>> index f2f36f32..9d4ec02e 100644\n>>>>>>> --- a/src/android/camera_device.cpp\n>>>>>>> +++ b/src/android/camera_device.cpp\n>>>>>>> @@ -240,6 +240,8 @@ CameraDevice::Camera3RequestDescriptor::Camera3RequestDescriptor(\n>>>>>>> \t/* Clone the controls associated with the camera3 request. */\n>>>>>>> \tsettings_ = CameraMetadata(camera3Request->settings);\n>>>>>>> \n>>>>>>> +\tinternalBuffer_ = nullptr;\n>>>>>>> +\n>>>>>>> \t/*\n>>>>>>> \t * Create the CaptureRequest, stored as a unique_ptr<> to tie its\n>>>>>>> \t * lifetime to the descriptor.\n>>>>>>> @@ -989,6 +991,7 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n>>>>>>> \t\t\t * once it has been processed.\n>>>>>>> \t\t\t */\n>>>>>>> \t\t\tbuffer = cameraStream->getBuffer();\n>>>>>>> +\t\t\tdescriptor.internalBuffer_ = buffer;\n>>>>>>> \t\t\tLOG(HAL, Debug) << ss.str() << \" (internal)\";\n>>>>>>> \t\t\tbreak;\n>>>>>>> \t\t}\n>>>>>>> @@ -1148,25 +1151,115 @@ void CameraDevice::requestComplete(Request *request)\n>>>>>>> \t\t\tcontinue;\n>>>>>>> \t\t}\n>>>>>>> \n>>>>>>> +\t\t/*\n>>>>>>> +\t\t * Save the current context of capture result and queue the\n>>>>>>> +\t\t * descriptor before processing the camera stream.\n>>>>>>> +\t\t *\n>>>>>>> +\t\t * When the processing completes, the descriptor will be\n>>>>>>> +\t\t * dequeued and capture results will be sent to the framework.\n>>>>>>> +\t\t */\n>>>>>>> +\t\tdescriptor.status_ = Camera3RequestDescriptor::NOT_FINISHED;\n>>>>>>> +\t\tdescriptor.resultMetadata_ = std::move(resultMetadata);\n>>>>>>> +\t\tdescriptor.captureResult_ = captureResult;\n>>>>>>> +\n>>>>>>> +\t\tstd::unique_ptr reqDescriptor =\n>>>>>>> +\t\t\tstd::make_unique();\n>>>>>>> +\t\t*reqDescriptor = std::move(descriptor);\n>>>>>>> +\t\tqueuedDescriptor_.push_back(std::move(reqDescriptor));\n>>>>>>> +\t\tCamera3RequestDescriptor *currentDescriptor = queuedDescriptor_.back().get();\n>>>>>>> +\n>>>>>>> +\t\tCameraMetadata *metadata = currentDescriptor->resultMetadata_.get();\n>>>>>>> +\t\tcameraStream->processComplete.connect(\n>>>>>>> +\t\t\tthis, [=](CameraStream::ProcessStatus status) {\n>>>>>>> +\t\t\t\tstreamProcessingComplete(cameraStream, status,\n>>>>>>> +\t\t\t\t\t\t\t metadata);\n>>>>>> I believe this to be unsafe to manage multiple operations.\n>>>>>>\n>>>>>> cameraStream->processComplete() should not be connected with per-run\n>>>>>> parameters. Those should be passed as the context of the signal emit(),\n>>>>>> not by the connection.\n>>>>>>\n>>>>>> Otherwise, the lamba will be overwritten by the next requestComplete()\n>>>>>> and incorrect parameters will be processed when the signal actually fires.\n>>>> While this is true.. and a bug in my  code\n>>>>\n>>>>> It's worse than that, ever connect() will create a new connection, so\n>>>>> the slot will be called once for the first request, twice for the\n>>>>> second, ...\n>>>> Oh, I gotta see this in action by sprinkling some printfs...\n>>>>\n>>>> But What is a alternative to this? While I tried to disconnect signal\n>>>> handlers in the slot the last time, it came back with:\n>>>>\n>>>> '''\n>>>> While signals can be connected and disconnected on demand, it often\n>>>> indicates a design issue. Is there a reason why you can't connect the\n>>>> signal when the cameraStream instance is created, and keep it connected\n>>>> for the whole lifetime of the stream ?\n>>>> '''\n>>>>\n>>>> in\n>>>> https://lists.libcamera.org/pipermail/libcamera-devel/2021-August/023936.html\n>>>>\n>>>> Not sure if it's possible to check on a object's signal to check for a\n>>>> slot's existence. And doing so, feels weird as well.\n>>>>\n>>>> So we need to connect to slot /only/ once, for all post-processing needs\n>>>> to be done in future. This sounds like an operation that can sit well in\n>>>> CameraStream::configure().\n>>> Correct. You've removed the disconnection based on my comments for the\n>>> previous version, but the connection also needs to be addressed :-)\n>>> Connecting in\n>>>\n>>>> But how can we pass a slot (which is probably\n>>>> private to class) to another class's configure()? A function object ?\n>>> You can create a slot in CameraStream, and from there call\n>>>\n>>> \tcameraDevice_->streamProcessingComplete(this, request, status);\n>>>\n>>> request should be a pointer to Camera3RequestDescriptor (which we could\n>>> rename to Camera3Request), it's the object that models the context of\n>> I would leave re-naming stuff for now.\n> OK.\n>\n>> Camera3RequestDescriptor seems to\n>> be getting bigger and we (Kieran and I) have discussed re-working it to\n>> reduce code duplication and efficient use of the structure. I am, in the\n>> favour of, not doing that as part of this series, but another series\n>> which is solely dedicated to the refactor of this.\n> That's fine with me, but I think that refactoring series should then\n> come before this one.\n\n\nOh hmm, I don't have any major development bits on refactoring \nCamera3RequestDescriptor right now. And I am about to finalize v2 of \nthis series.\n\nI'll check with Kieran in our today sync and discuss the refactoring. \nI'll submit the v2 anyway as Jacopo could also give hints about what he \nwould like to see in the refactoring as well.\n\n>>> the processing. You need to pass it to cameraStream->process() in the\n>>> first place of course.\n>> This would require exposing Camera3RequestDescriptor struct To\n>> CameraStream class and PostProcessor. Probably it worth to break it out\n>> from CameraDevice private: and set the definition sit outside the scope\n>> of CameraDevice\n>>\n>> I quickly compile-tested this to check: https://paste.debian.net/1211030/\n> Soudns good to me.\n>\n>>> It's tempting to merge Camera3RequestDescriptor with CaptureRequest, but\n>>> let's not do that, as fence handling needs to move to the libcamera\n>>> core, so the CameraWorker will disappear then, and so will\n>>> CaptureRequest.\n>>>\n>>>>>>> +\t\t\t});\n>>>>>>> +\n>>>>>>> \t\tint ret = cameraStream->process(src, *buffer.buffer,\n>>>>>>> -\t\t\t\t\t\tdescriptor.settings_,\n>>>>>>> -\t\t\t\t\t\tresultMetadata.get());\n>>>>>>> +\t\t\t\t\t\tcurrentDescriptor->settings_,\n>>>>>>> +\t\t\t\t\t\tmetadata);\n>>>>>>> +\t\treturn;\n>>>>>>> +\t}\n>>>>>>> +\n>>>>>>> +\tif (queuedDescriptor_.empty()) {\n>>>>>>> +\t\tcaptureResult.result = resultMetadata->get();\n>>>>>>> +\t\tcallbacks_->process_capture_result(callbacks_, &captureResult);\n>>>>>> *1 duplication of completion callbacks (see below)\n>>>>>>\n>>>>>>> +\t} else {\n>>>>>>> +\t\t/*\n>>>>>>> +\t\t * Previous capture results waiting to be sent to framework\n>>>>>>> +\t\t * hence, queue the current capture results as well. After that,\n>>>>>>> +\t\t * check if any results are ready to be sent back to the\n>>>>>>> +\t\t * framework.\n>>>>>>> +\t\t */\n>>>>>>> +\t\tdescriptor.status_ = Camera3RequestDescriptor::FINISHED_SUCCESS;\n>>>>>>> +\t\tdescriptor.resultMetadata_ = std::move(resultMetadata);\n>>>>>>> +\t\tdescriptor.captureResult_ = captureResult;\n>>>>>>> +\t\tstd::unique_ptr reqDescriptor =\n>>>>>>> +\t\t\tstd::make_unique();\n>>>>>>> +\t\t*reqDescriptor = std::move(descriptor);\n>>>>>>> +\t\tqueuedDescriptor_.push_back(std::move(reqDescriptor));\n>>>>>>> +\n>>>>>>> +\t\tsendQueuedCaptureResults();\n>>>>>>> +\t}\n>>>>>>> +}\n>>>>>>> +\n>>>>>>> +void CameraDevice::streamProcessingComplete(CameraStream *cameraStream,\n>>>>>>> +\t\t\t\t\t CameraStream::ProcessStatus status,\n>>>>>>> +\t\t\t\t\t CameraMetadata *resultMetadata)\n>>>>>>> +{\n>>>>>>> +\tfor (auto &d : queuedDescriptor_) {\n>>>>>>> +\t\tif (d->resultMetadata_.get() != resultMetadata)\n>>>>>>> +\t\t\tcontinue;\n>>> This is a hack that you'll be able to drop once you get the pointer to\n>>> the Camera3RequestDescriptor passed as a function argument.\n>> Yep, I was thinking to pass the frameNumber alternatively ... and\n>> compare it here to find the descriptor..\n>>\n>>>>>>> +\n>>>>>>> \t\t/*\n>>>>>>> \t\t * Return the FrameBuffer to the CameraStream now that we're\n>>>>>>> \t\t * done processing it.\n>>>>>>> \t\t */\n>>>>>>> \t\tif (cameraStream->type() == CameraStream::Type::Internal)\n>>>>>>> -\t\t\tcameraStream->putBuffer(src);\n>>>>>>> -\n>>>>>>> -\t\tif (ret) {\n>>>>>>> -\t\t\tbuffer.status = CAMERA3_BUFFER_STATUS_ERROR;\n>>>>>>> -\t\t\tnotifyError(descriptor.frameNumber_, buffer.stream,\n>>>>>>> -\t\t\t\t CAMERA3_MSG_ERROR_BUFFER);\n>>>>>>> +\t\t\tcameraStream->putBuffer(d->internalBuffer_);\n>>>>>>> +\n>>>>>>> +\t\tif (status == CameraStream::ProcessStatus::Success) {\n>>>>>>> +\t\t\td->status_ = Camera3RequestDescriptor::FINISHED_SUCCESS;\n>>>>>>> +\t\t} else {\n>>>>>>> +\t\t\td->status_ = Camera3RequestDescriptor::FINISHED_FAILED;\n>>>>>>> +\t\t\td->captureResult_.partial_result = 0;\n>>>>>>> +\t\t\tfor (camera3_stream_buffer_t &buffer : d->buffers_) {\n>>>>>>> +\t\t\t\tCameraStream *cs = static_cast(buffer.stream->priv);\n>>>>>>> +\n>>>>>>> +\t\t\t\tif (cs->camera3Stream().format != HAL_PIXEL_FORMAT_BLOB)\n>>>>>>> +\t\t\t\t\tcontinue;\n>>>>>>> +\n>>>>>>> +\t\t\t\tbuffer.status = CAMERA3_BUFFER_STATUS_ERROR;\n>>>>>>> +\t\t\t\tnotifyError(d->frameNumber_, buffer.stream,\n>>>>>>> +\t\t\t\t\t CAMERA3_MSG_ERROR_BUFFER);\n>>>>>>> +\t\t\t}\n>>>>>>> \t\t}\n>>>>>>> +\n>>>>>>> +\t\tbreak;\n>>>>>>> \t}\n>>>>>>> \n>>>>>>> -\tcaptureResult.result = resultMetadata->get();\n>>>>>>> -\tcallbacks_->process_capture_result(callbacks_, &captureResult);\n>>>>>>> +\t/*\n>>>>>>> +\t * Send back capture results to the framework by inspecting the queue.\n>>>>>>> +\t * The framework can defer queueing further requests to the HAL (via\n>>>>>>> +\t * process_capture_request) unless until it receives the capture results\n>>>>>>> +\t * for already queued requests.\n>>>>>>> +\t */\n>>>>>>> +\tsendQueuedCaptureResults();\n>>>>>>> +}\n>>>>>>> +\n>>>>>>> +void CameraDevice::sendQueuedCaptureResults()\n>>>>>>> +{\n>>>>>>> +\twhile (!queuedDescriptor_.empty()) {\n>>>>>>> +\t\tstd::unique_ptr &d = queuedDescriptor_.front();\n>>>>>> I'd have this exit early if the front descriptor is not finished.\n>>>>>>\n>>>>>>> +\t\tif (d->status_ != Camera3RequestDescriptor::NOT_FINISHED) {\n>>>>>>> +\t\t\td->captureResult_.result = d->resultMetadata_->get();\n>>>>>>> +\t\t\tcallbacks_->process_capture_result(callbacks_,\n>>>>>>> +\t\t\t\t\t\t\t &(d->captureResult_));\n>>>>>> Not as part of this patch, but I feel like Camera3RequestDescriptor\n>>>>>> could have a method called complete() to wrap the management of the\n>>>>>> completion against a request descriptor.\n>>>>>>\n>>>>>> There's lots of occasions where the metadata is obtained or processed\n>>>>>> into a result structure and called back with a flag.\n>>>>>>\n>>>>>> (such as *1 above, but I think there are more)\n>>>>>>\n>>>>>> It seems to me like that could be made into\n>>>>>>\n>>>>>> \t\td->complete(CameraRequest::Success);\n>>>>>>\n>>>>>> or something.... But that's an overall design of breaking out the\n>>>>>> Camera3RequestDescriptor to a full class.\n>>>>>>\n>>>>>> As this series is extending Camera3RequestDescriptor to maintain extra\n>>>>>> state required for the asynchronous post-processing, that makes me think\n>>>>>> it would be better to have a full class to manage that state. (and thus\n>>>>>> methods to operate on it)·\n>>>>>>\n>>>>>>> +\t\t\tqueuedDescriptor_.pop_front();\n>>>>>>> +\t\t} else {\n>>>>>>> +\t\t\tbreak;\n>>>>>>> +\t\t}\n>>>>>>> +\t}\n>>>>>>> }\n>>>>>>> \n>>>>>>> std::string CameraDevice::logPrefix() const\n>>>>>>> diff --git a/src/android/camera_device.h b/src/android/camera_device.h\n>>>>>>> index a5576927..36425773 100644\n>>>>>>> --- a/src/android/camera_device.h\n>>>>>>> +++ b/src/android/camera_device.h\n>>>>>>> @@ -7,6 +7,7 @@\n>>>>>>> #ifndef __ANDROID_CAMERA_DEVICE_H__\n>>>>>>> #define __ANDROID_CAMERA_DEVICE_H__\n>>>>>>> \n>>>>>>> +#include \n>>>>>>> #include \n>>>>>>> #include \n>>>>>>> #include \n>>>>>>> @@ -83,6 +84,21 @@ private:\n>>>>>>> \t\tstd::vector> frameBuffers_;\n>>>>>>> \t\tCameraMetadata settings_;\n>>>>>>> \t\tstd::unique_ptr request_;\n>>>>>>> +\n>>>>>>> +\t\t/*\n>>>>>>> +\t\t * Below are utility placeholders used when a capture result\n>>>>>>> +\t\t * needs to be queued before completion via\n>>>>>>> +\t\t * process_capture_result() callback.\n>>>>>>> +\t\t */\n>>>>>>> +\t\tenum completionStatus {\n>>>>>>> +\t\t\tNOT_FINISHED,\n>>>>>> I'd call this \"Active\" or \"Pending\" to reduce the negation...\n>>>>>>\n>>>>>> \t\"If (state == Active)\"\n>>>>>> rather than\n>>>>>> \t\"if (state != NOT_FINISHED)\"\n>>>>>>\n>>>>>>> +\t\t\tFINISHED_SUCCESS,\n>>>>>> I'd call this Success or Succeeded, or Successful ..\n>>>>>>\n>>>>>>> +\t\t\tFINISHED_FAILED,\n>>>>>> And Failed\n>>>>>>\n>>>>>>> +\t\t};\n>>>>>>> +\t\tstd::unique_ptr resultMetadata_;\n>>>>>>> +\t\tcamera3_capture_result_t captureResult_;\n>>>>>>> +\t\tlibcamera::FrameBuffer *internalBuffer_;\n>>>>>>> +\t\tcompletionStatus status_;\n>>>>>> These are all used to track the post-processor context.\n>>>>>>\n>>>>>>> \t};\n>>>>>>> \n>>>>>>> \tenum class State {\n>>>>>>> @@ -105,6 +121,11 @@ private:\n>>>>>>> \tstd::unique_ptr getResultMetadata(\n>>>>>>> \t\tconst Camera3RequestDescriptor &descriptor) const;\n>>>>>>> \n>>>>>>> +\tvoid sendQueuedCaptureResults();\n>>>>>>> +\tvoid streamProcessingComplete(CameraStream *stream,\n>>>>>>> +\t\t\t\t CameraStream::ProcessStatus status,\n>>>>>>> +\t\t\t\t CameraMetadata *resultMetadata);\n>>>>>>> +\n>>>>>>> \tunsigned int id_;\n>>>>>>> \tcamera3_device_t camera3Device_;\n>>>>>>> \n>>>>>>> @@ -125,6 +146,8 @@ private:\n>>>>>>> \tlibcamera::Mutex descriptorsMutex_; /* Protects descriptors_. */\n>>>>>>> \tstd::map descriptors_;\n>>>>>>> \n>>>>>>> +\tstd::deque> queuedDescriptor_;\n>>>>>> I'm not yet sure if this should be a unique_ptr ..\n>>>>>>\n>>>>>>> +\n>>>>>>> \tstd::string maker_;\n>>>>>>> \tstd::string model_;\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 76D64BDB1D\n\tfor ;\n\tFri, 10 Sep 2021 04:48:58 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id ADB9B6916E;\n\tFri, 10 Sep 2021 06:48:57 +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 217D860249\n\tfor ;\n\tFri, 10 Sep 2021 06:48:56 +0200 (CEST)","from [192.168.1.104] (unknown [103.251.226.149])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id CFC7024F;\n\tFri, 10 Sep 2021 06:48:54 +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=\"v99N0wPK\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1631249335;\n\tbh=gxQt8zT/Ev5UEovCXHxCTfs7KZILd0rvPC3vBXng2Bo=;\n\th=Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=v99N0wPKFUgXbkfnW3Oj41HacbuF+iRdyfgN5c7BZRqdh7OgYRYXOcb/jn1cIHK7c\n\tkddXGJVYgP6oAQCAlmdrMnHYCdKV8Eqq/GLU5I8BUIIa8eZ7znzzriuyGojiMwJbuk\n\tmr+YHVTWipgmFzfWr9Y7uHU1mIMinuJL7+Ioc2eI=","To":"Laurent Pinchart ","References":"<20210907195704.338079-1-umang.jain@ideasonboard.com>\n\t<20210907195704.338079-5-umang.jain@ideasonboard.com>\n\t\n\t\n\t\n\t\n\t<4fc71131-3e84-6142-d926-a5aee23afa7a@ideasonboard.com>\n\t","From":"Umang Jain ","Message-ID":"<2d35ffe6-9e83-9725-cfaa-40cc4520de4a@ideasonboard.com>","Date":"Fri, 10 Sep 2021 10:18:50 +0530","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.10.2","MIME-Version":"1.0","In-Reply-To":"","Content-Type":"text/plain; charset=utf-8; format=flowed","Content-Transfer-Encoding":"8bit","Content-Language":"en-US","Subject":"Re: [libcamera-devel] [PATCH v1 4/6] android: camera_device: Add a\n\tqueue for sending capture results","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\" "}}]