[{"id":19734,"web_url":"https://patchwork.libcamera.org/comment/19734/","msgid":"<20210921114618.3viuxka3hiy7xfq4@uno.localdomain>","date":"2021-09-21T11:46:18","subject":"Re: [libcamera-devel] [PATCH v3 09/10] android: camera_device: Send\n\tcapture results inspecting the descriptor","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Umang,\n\nOn Mon, Sep 20, 2021 at 11:07:51PM +0530, Umang Jain wrote:\n> A Camera3RequestDescriptors is constructed and queued to descriptors_\n> queue as soon as an (incoming) capture request is received on the\n> libcamera HAL. The capture request is picked up by HAL, in order to run\n> it to completion. At completion, CameraDevice::requestComplete() gets\n> invoked and capture results are populated and ready to be sent back\n> to the framework.\n>\n> All the data and framebuffers associated with the request are alive\n> and encapsulated inside this Camera3RequestDescriptor descriptor.\n> By inspecting the ProcessStatus on the descriptor, we can now send\n> capture results via the process_capture_result() callback.\n>\n> Hence, introduce a new private member function sendCaptureResults()\n> which will be responsible to send capture results back to the\n> framework by inspecting the descriptor on the queue. In subsequent\n> commit, when the post processsor shall run async, sendCaptureResults()\n> can be called from the post-processor's thread for e.g.\n> streamProcessComplete() hence, introduce the mutex lock to avoid the\n> races.\n>\n> Signed-off-by: Umang Jain \n> ---\n> src/android/camera_device.cpp | 47 +++++++++++++++++++++++++----------\n> src/android/camera_device.h | 4 +++\n> 2 files changed, 38 insertions(+), 13 deletions(-)\n>\n> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> index 4658e881..16ecdfc5 100644\n> --- a/src/android/camera_device.cpp\n> +++ b/src/android/camera_device.cpp\n> @@ -1078,7 +1078,7 @@ void CameraDevice::requestComplete(Request *request)\n> \t * The buffer status is set to OK and later changed to ERROR if\n> \t * post-processing/compression fails.\n> \t */\n> -\tcamera3_capture_result_t captureResult = {};\n> +\tcamera3_capture_result_t &captureResult = descriptor->captureResult_;\n> \tcaptureResult.frame_number = descriptor->frameNumber_;\n> \tcaptureResult.num_output_buffers = descriptor->buffers_.size();\n> \tfor (camera3_stream_buffer_t &buffer : descriptor->buffers_) {\n> @@ -1156,26 +1156,25 @@ void CameraDevice::requestComplete(Request *request)\n> \t\t\tcontinue;\n> \t\t}\n>\n> +\t\tif (cameraStream->type() == CameraStream::Type::Internal)\n> +\t\t\tdescriptor->internalBuffer_ = src;\n> +\n> \t\tdescriptor->status_ = Camera3RequestDescriptor::ProcessStatus::Processing;\n>\n> \t\tcameraStream->process(src, *buffer.buffer, descriptor);\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> +\t\treturn;\n> \t}\n>\n> -\tcaptureResult.result = descriptor->resultMetadata_->get();\n> -\tcallbacks_->process_capture_result(callbacks_, &captureResult);\n> -\n> -\tdescriptors_.pop_front();\n> +\t/*\n> +\t * Mark the status on the descriptor as success as no processing\n> +\t * is neeeded.\n> +\t */\n> +\tdescriptor->status_ = Camera3RequestDescriptor::ProcessStatus::Success;\n> +\tsendCaptureResults();\n> }\n>\n>\n> -void CameraDevice::streamProcessingComplete([[maybe_unused]] CameraStream *cameraStream,\n> +void CameraDevice::streamProcessingComplete(CameraStream *cameraStream,\n> \t\t\t\t\t Camera3RequestDescriptor *request)\n> {\n> \tif (request->status_ == Camera3RequestDescriptor::ProcessStatus::Error) {\n> @@ -1190,6 +1189,28 @@ void CameraDevice::streamProcessingComplete([[maybe_unused]] CameraStream *camer\n> \t\t\t\t CAMERA3_MSG_ERROR_BUFFER);\n> \t\t}\n> \t}\n> +\n> +\t/*\n> +\t * Return the FrameBuffer to the CameraStream now that we're\n> +\t * done processing it.\n> +\t */\n> +\tif (cameraStream->type() == CameraStream::Type::Internal)\n> +\t\tcameraStream->putBuffer(request->internalBuffer_);\n> +\n> +\tsendCaptureResults();\n> +}\n> +\n> +void CameraDevice::sendCaptureResults()\n> +{\n> +\tCamera3RequestDescriptor *d = descriptors_.front().get();\n\nIf I'm not mistaken, in case a request that doesn't need\npost-processing will complete while the post-processed one is still\nbeing worked on, this could lead to a completion order inversion\n\nprocessCaptureRequest(n)\n descriptors.push(n)\n\nprocessCaptureRequest(n+1)\n descriptors.push(n+1)\n\nequestComplete(n)\n postProcess(n) -----------> process(n)\n |\n .... ...\n |\nrequestComplete(n+1) |\n sendCaptureResult(n+1) |\n d = descriptors.front() |\n process_capture_res(n) |\n |\n sendCaptureResult(n) <----------------\n d = descritpros.front()\n process_capture_res(n + 1)\n\n> +\tif (d->status_ == Camera3RequestDescriptor::ProcessStatus::Processing ||\n> +\t d->status_ == Camera3RequestDescriptor::ProcessStatus::None)\n> +\t\treturn;\n> +\n> +\tMutexLocker lock(descriptorsMutex_);\n> +\td->captureResult_.result = d->resultMetadata_->get();\n> +\tcallbacks_->process_capture_result(callbacks_, &(d->captureResult_));\n> +\tdescriptors_.pop_front();\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 60c134dc..0bd570a1 100644\n> --- a/src/android/camera_device.h\n> +++ b/src/android/camera_device.h\n> @@ -56,6 +56,9 @@ struct Camera3RequestDescriptor {\n> \tstd::unique_ptr request_;\n> \tstd::unique_ptr resultMetadata_;\n>\n> +\tcamera3_capture_result_t captureResult_ = {};\n> +\tlibcamera::FrameBuffer *internalBuffer_;\n> +\n> \tProcessStatus status_;\n> };\n>\n> @@ -118,6 +121,7 @@ private:\n> \tint processControls(Camera3RequestDescriptor *descriptor);\n> \tstd::unique_ptr getResultMetadata(\n> \t\tconst Camera3RequestDescriptor *descriptor) const;\n> +\tvoid sendCaptureResults();\n>\n> \tunsigned int id_;\n> \tcamera3_device_t camera3Device_;\n> --\n> 2.31.1\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 4605ABDC71\n\tfor ;\n\tTue, 21 Sep 2021 11:45:34 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9D03E6918A;\n\tTue, 21 Sep 2021 13:45:33 +0200 (CEST)","from relay2-d.mail.gandi.net (relay2-d.mail.gandi.net\n\t[217.70.183.194])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 555BE60247\n\tfor ;\n\tTue, 21 Sep 2021 13:45:32 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby relay2-d.mail.gandi.net (Postfix) with ESMTPSA id D8E8A4000A;\n\tTue, 21 Sep 2021 11:45:31 +0000 (UTC)"],"Date":"Tue, 21 Sep 2021 13:46:18 +0200","From":"Jacopo Mondi ","To":"Umang Jain ","Message-ID":"<20210921114618.3viuxka3hiy7xfq4@uno.localdomain>","References":"<20210920173752.1346190-1-umang.jain@ideasonboard.com>\n\t<20210920173752.1346190-10-umang.jain@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210920173752.1346190-10-umang.jain@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v3 09/10] android: camera_device: Send\n\tcapture results inspecting the descriptor","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":19738,"web_url":"https://patchwork.libcamera.org/comment/19738/","msgid":"<20210921125021.n7ktso7rldtjrj4s@uno.localdomain>","date":"2021-09-21T12:50:21","subject":"Re: [libcamera-devel] [PATCH v3 09/10] android: camera_device: Send\n\tcapture results inspecting the descriptor","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Umang,\n\nOn Tue, Sep 21, 2021 at 01:46:18PM +0200, Jacopo Mondi wrote:\n> Hi Umang,\n>\n> On Mon, Sep 20, 2021 at 11:07:51PM +0530, Umang Jain wrote:\n> > A Camera3RequestDescriptors is constructed and queued to descriptors_\n> > queue as soon as an (incoming) capture request is received on the\n> > libcamera HAL. The capture request is picked up by HAL, in order to run\n> > it to completion. At completion, CameraDevice::requestComplete() gets\n> > invoked and capture results are populated and ready to be sent back\n> > to the framework.\n> >\n> > All the data and framebuffers associated with the request are alive\n> > and encapsulated inside this Camera3RequestDescriptor descriptor.\n> > By inspecting the ProcessStatus on the descriptor, we can now send\n> > capture results via the process_capture_result() callback.\n> >\n> > Hence, introduce a new private member function sendCaptureResults()\n> > which will be responsible to send capture results back to the\n> > framework by inspecting the descriptor on the queue. In subsequent\n> > commit, when the post processsor shall run async, sendCaptureResults()\n> > can be called from the post-processor's thread for e.g.\n> > streamProcessComplete() hence, introduce the mutex lock to avoid the\n> > races.\n> >\n> > Signed-off-by: Umang Jain \n> > ---\n> > src/android/camera_device.cpp | 47 +++++++++++++++++++++++++----------\n> > src/android/camera_device.h | 4 +++\n> > 2 files changed, 38 insertions(+), 13 deletions(-)\n> >\n> > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> > index 4658e881..16ecdfc5 100644\n> > --- a/src/android/camera_device.cpp\n> > +++ b/src/android/camera_device.cpp\n> > @@ -1078,7 +1078,7 @@ void CameraDevice::requestComplete(Request *request)\n> > \t * The buffer status is set to OK and later changed to ERROR if\n> > \t * post-processing/compression fails.\n> > \t */\n> > -\tcamera3_capture_result_t captureResult = {};\n> > +\tcamera3_capture_result_t &captureResult = descriptor->captureResult_;\n> > \tcaptureResult.frame_number = descriptor->frameNumber_;\n> > \tcaptureResult.num_output_buffers = descriptor->buffers_.size();\n> > \tfor (camera3_stream_buffer_t &buffer : descriptor->buffers_) {\n> > @@ -1156,26 +1156,25 @@ void CameraDevice::requestComplete(Request *request)\n> > \t\t\tcontinue;\n> > \t\t}\n> >\n> > +\t\tif (cameraStream->type() == CameraStream::Type::Internal)\n> > +\t\t\tdescriptor->internalBuffer_ = src;\n> > +\n> > \t\tdescriptor->status_ = Camera3RequestDescriptor::ProcessStatus::Processing;\n> >\n> > \t\tcameraStream->process(src, *buffer.buffer, descriptor);\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> > +\t\treturn;\n> > \t}\n> >\n> > -\tcaptureResult.result = descriptor->resultMetadata_->get();\n> > -\tcallbacks_->process_capture_result(callbacks_, &captureResult);\n> > -\n> > -\tdescriptors_.pop_front();\n> > +\t/*\n> > +\t * Mark the status on the descriptor as success as no processing\n> > +\t * is neeeded.\n> > +\t */\n> > +\tdescriptor->status_ = Camera3RequestDescriptor::ProcessStatus::Success;\n> > +\tsendCaptureResults();\n> > }\n> >\n> >\n> > -void CameraDevice::streamProcessingComplete([[maybe_unused]] CameraStream *cameraStream,\n> > +void CameraDevice::streamProcessingComplete(CameraStream *cameraStream,\n> > \t\t\t\t\t Camera3RequestDescriptor *request)\n> > {\n> > \tif (request->status_ == Camera3RequestDescriptor::ProcessStatus::Error) {\n> > @@ -1190,6 +1189,28 @@ void CameraDevice::streamProcessingComplete([[maybe_unused]] CameraStream *camer\n> > \t\t\t\t CAMERA3_MSG_ERROR_BUFFER);\n> > \t\t}\n> > \t}\n> > +\n> > +\t/*\n> > +\t * Return the FrameBuffer to the CameraStream now that we're\n> > +\t * done processing it.\n> > +\t */\n> > +\tif (cameraStream->type() == CameraStream::Type::Internal)\n> > +\t\tcameraStream->putBuffer(request->internalBuffer_);\n> > +\n> > +\tsendCaptureResults();\n> > +}\n> > +\n> > +void CameraDevice::sendCaptureResults()\n> > +{\n> > +\tCamera3RequestDescriptor *d = descriptors_.front().get();\n>\n> If I'm not mistaken, in case a request that doesn't need\n> post-processing will complete while the post-processed one is still\n> being worked on, this could lead to a completion order inversion\n>\n> processCaptureRequest(n)\n> descriptors.push(n)\n>\n> processCaptureRequest(n+1)\n> descriptors.push(n+1)\n>\n> equestComplete(n)\n> postProcess(n) -----------> process(n)\n> |\n> .... ...\n> |\n> requestComplete(n+1) |\n> sendCaptureResult(n+1) |\n> d = descriptors.front() |\n> process_capture_res(n) |\n> |\n> sendCaptureResult(n) <----------------\n> d = descritpros.front()\n> process_capture_res(n + 1)\n\nas you have clarified offline this can't happen as you skip\nnot-completed requests in sendCaptureResult().\n\nAt the same time it seems you need to iterate the queue when the\nfront() request has completed, to complete the next ones which are\nready but whose completion was post-poned.\n\nThanks\n j\n\n>\n> > +\tif (d->status_ == Camera3RequestDescriptor::ProcessStatus::Processing ||\n> > +\t d->status_ == Camera3RequestDescriptor::ProcessStatus::None)\n> > +\t\treturn;\n> > +\n> > +\tMutexLocker lock(descriptorsMutex_);\n> > +\td->captureResult_.result = d->resultMetadata_->get();\n> > +\tcallbacks_->process_capture_result(callbacks_, &(d->captureResult_));\n> > +\tdescriptors_.pop_front();\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 60c134dc..0bd570a1 100644\n> > --- a/src/android/camera_device.h\n> > +++ b/src/android/camera_device.h\n> > @@ -56,6 +56,9 @@ struct Camera3RequestDescriptor {\n> > \tstd::unique_ptr request_;\n> > \tstd::unique_ptr resultMetadata_;\n> >\n> > +\tcamera3_capture_result_t captureResult_ = {};\n> > +\tlibcamera::FrameBuffer *internalBuffer_;\n> > +\n> > \tProcessStatus status_;\n> > };\n> >\n> > @@ -118,6 +121,7 @@ private:\n> > \tint processControls(Camera3RequestDescriptor *descriptor);\n> > \tstd::unique_ptr getResultMetadata(\n> > \t\tconst Camera3RequestDescriptor *descriptor) const;\n> > +\tvoid sendCaptureResults();\n> >\n> > \tunsigned int id_;\n> > \tcamera3_device_t camera3Device_;\n> > --\n> > 2.31.1\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 3DF0BBF01C\n\tfor ;\n\tTue, 21 Sep 2021 12:49:37 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 715006918A;\n\tTue, 21 Sep 2021 14:49:36 +0200 (CEST)","from relay2-d.mail.gandi.net (relay2-d.mail.gandi.net\n\t[217.70.183.194])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 7562860247\n\tfor ;\n\tTue, 21 Sep 2021 14:49:35 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby relay2-d.mail.gandi.net (Postfix) with ESMTPSA id 01B5A40006;\n\tTue, 21 Sep 2021 12:49:34 +0000 (UTC)"],"Date":"Tue, 21 Sep 2021 14:50:21 +0200","From":"Jacopo Mondi ","To":"Umang Jain ","Message-ID":"<20210921125021.n7ktso7rldtjrj4s@uno.localdomain>","References":"<20210920173752.1346190-1-umang.jain@ideasonboard.com>\n\t<20210920173752.1346190-10-umang.jain@ideasonboard.com>\n\t<20210921114618.3viuxka3hiy7xfq4@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210921114618.3viuxka3hiy7xfq4@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH v3 09/10] android: camera_device: Send\n\tcapture results inspecting the descriptor","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":19739,"web_url":"https://patchwork.libcamera.org/comment/19739/","msgid":"<785084bd-785f-926d-2cfb-15d1d697dab8@ideasonboard.com>","date":"2021-09-21T12:55:03","subject":"Re: [libcamera-devel] [PATCH v3 09/10] android: camera_device: Send\n\tcapture results inspecting the descriptor","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hi Jacopo,\n\n(Post in call discussion summary on this if anyone here is interested)\n\nOn 9/21/21 5:16 PM, Jacopo Mondi wrote:\n> Hi Umang,\n>\n> On Mon, Sep 20, 2021 at 11:07:51PM +0530, Umang Jain wrote:\n>> A Camera3RequestDescriptors is constructed and queued to descriptors_\n>> queue as soon as an (incoming) capture request is received on the\n>> libcamera HAL. The capture request is picked up by HAL, in order to run\n>> it to completion. At completion, CameraDevice::requestComplete() gets\n>> invoked and capture results are populated and ready to be sent back\n>> to the framework.\n>>\n>> All the data and framebuffers associated with the request are alive\n>> and encapsulated inside this Camera3RequestDescriptor descriptor.\n>> By inspecting the ProcessStatus on the descriptor, we can now send\n>> capture results via the process_capture_result() callback.\n>>\n>> Hence, introduce a new private member function sendCaptureResults()\n>> which will be responsible to send capture results back to the\n>> framework by inspecting the descriptor on the queue. In subsequent\n>> commit, when the post processsor shall run async, sendCaptureResults()\n>> can be called from the post-processor's thread for e.g.\n>> streamProcessComplete() hence, introduce the mutex lock to avoid the\n>> races.\n>>\n>> Signed-off-by: Umang Jain \n>> ---\n>> src/android/camera_device.cpp | 47 +++++++++++++++++++++++++----------\n>> src/android/camera_device.h | 4 +++\n>> 2 files changed, 38 insertions(+), 13 deletions(-)\n>>\n>> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n>> index 4658e881..16ecdfc5 100644\n>> --- a/src/android/camera_device.cpp\n>> +++ b/src/android/camera_device.cpp\n>> @@ -1078,7 +1078,7 @@ void CameraDevice::requestComplete(Request *request)\n>> \t * The buffer status is set to OK and later changed to ERROR if\n>> \t * post-processing/compression fails.\n>> \t */\n>> -\tcamera3_capture_result_t captureResult = {};\n>> +\tcamera3_capture_result_t &captureResult = descriptor->captureResult_;\n>> \tcaptureResult.frame_number = descriptor->frameNumber_;\n>> \tcaptureResult.num_output_buffers = descriptor->buffers_.size();\n>> \tfor (camera3_stream_buffer_t &buffer : descriptor->buffers_) {\n>> @@ -1156,26 +1156,25 @@ void CameraDevice::requestComplete(Request *request)\n>> \t\t\tcontinue;\n>> \t\t}\n>>\n>> +\t\tif (cameraStream->type() == CameraStream::Type::Internal)\n>> +\t\t\tdescriptor->internalBuffer_ = src;\n>> +\n>> \t\tdescriptor->status_ = Camera3RequestDescriptor::ProcessStatus::Processing;\n>>\n>> \t\tcameraStream->process(src, *buffer.buffer, descriptor);\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>> +\t\treturn;\n>> \t}\n>>\n>> -\tcaptureResult.result = descriptor->resultMetadata_->get();\n>> -\tcallbacks_->process_capture_result(callbacks_, &captureResult);\n>> -\n>> -\tdescriptors_.pop_front();\n>> +\t/*\n>> +\t * Mark the status on the descriptor as success as no processing\n>> +\t * is neeeded.\n>> +\t */\n>> +\tdescriptor->status_ = Camera3RequestDescriptor::ProcessStatus::Success;\n>> +\tsendCaptureResults();\n>> }\n>>\n>>\n>> -void CameraDevice::streamProcessingComplete([[maybe_unused]] CameraStream *cameraStream,\n>> +void CameraDevice::streamProcessingComplete(CameraStream *cameraStream,\n>> \t\t\t\t\t Camera3RequestDescriptor *request)\n>> {\n>> \tif (request->status_ == Camera3RequestDescriptor::ProcessStatus::Error) {\n>> @@ -1190,6 +1189,28 @@ void CameraDevice::streamProcessingComplete([[maybe_unused]] CameraStream *camer\n>> \t\t\t\t CAMERA3_MSG_ERROR_BUFFER);\n>> \t\t}\n>> \t}\n>> +\n>> +\t/*\n>> +\t * Return the FrameBuffer to the CameraStream now that we're\n>> +\t * done processing it.\n>> +\t */\n>> +\tif (cameraStream->type() == CameraStream::Type::Internal)\n>> +\t\tcameraStream->putBuffer(request->internalBuffer_);\n>> +\n>> +\tsendCaptureResults();\n>> +}\n>> +\n>> +void CameraDevice::sendCaptureResults()\n>> +{\n>> +\tCamera3RequestDescriptor *d = descriptors_.front().get();\n> If I'm not mistaken, in case a request that doesn't need\n> post-processing will complete while the post-processed one is still\n> being worked on, this could lead to a completion order inversion\n>\n> processCaptureRequest(n)\n> descriptors.push(n)\n>\n> processCaptureRequest(n+1)\n> descriptors.push(n+1)\n>\n> equestComplete(n)\n> postProcess(n) -----------> process(n)\n> |\n> .... ...\n> |\n> requestComplete(n+1) |\n> sendCaptureResult(n+1) |\n> d = descriptors.front() |\n> process_capture_res(n) |\n> |\n> sendCaptureResult(n) <----------------\n> d = descritpros.front()\n> process_capture_res(n + 1)\n>\n\nNice flow diagram btw, I need to get better at that ;-)\n\nSo, first of all sendCaptureResults() doesn't take an argument, but I \nget your point that you are trying to match for which 'n' request, \nsendCapture result is called...\n\nsendCaptureResults() works on the queue's head. It doesn't care from \nwhere it's called; it can be called from requestComplete()[same thread] \nor streamProcessingComplete()[different thread]. All it does or suppose \nto do, is basically looks at the queue's head and see if the HEAD \ndescriptor  says - \"Send me back to the framework\".\n- If yes, it will send the results via process_capture_result  of the \nHEAD and drop the HEAD from the queue\n- If no, simply return;\n\nThat's it. Since, it looks at the head, it will only send results in \norder (since it's a queue) and drop the HEAD.\n\nHowever, the discussion has un-covered another point that \nsendCaptureResults() can lag behind w.r.t libcamera:Requests being \ncompleted, since there is no loop around to process other descriptors in \nthe queue which are ready to send (these descriptors can potentially be \nthe ones not requiring any post-processing). As discussed in the call, \nwe both agree that we should probably iterate over the queue and send \nthe results back to the framework as soon as possible, for ready \ndescriptor. So, sendCaptureResults() will be wrapped in a loop in \nsubsequent version.\n\n\n>> +\tif (d->status_ == Camera3RequestDescriptor::ProcessStatus::Processing ||\n>> +\t d->status_ == Camera3RequestDescriptor::ProcessStatus::None)\n>> +\t\treturn;\n>> +\n>> +\tMutexLocker lock(descriptorsMutex_);\n>> +\td->captureResult_.result = d->resultMetadata_->get();\n>> +\tcallbacks_->process_capture_result(callbacks_, &(d->captureResult_));\n>> +\tdescriptors_.pop_front();\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 60c134dc..0bd570a1 100644\n>> --- a/src/android/camera_device.h\n>> +++ b/src/android/camera_device.h\n>> @@ -56,6 +56,9 @@ struct Camera3RequestDescriptor {\n>> \tstd::unique_ptr request_;\n>> \tstd::unique_ptr resultMetadata_;\n>>\n>> +\tcamera3_capture_result_t captureResult_ = {};\n>> +\tlibcamera::FrameBuffer *internalBuffer_;\n>> +\n>> \tProcessStatus status_;\n>> };\n>>\n>> @@ -118,6 +121,7 @@ private:\n>> \tint processControls(Camera3RequestDescriptor *descriptor);\n>> \tstd::unique_ptr getResultMetadata(\n>> \t\tconst Camera3RequestDescriptor *descriptor) const;\n>> +\tvoid sendCaptureResults();\n>>\n>> \tunsigned int id_;\n>> \tcamera3_device_t camera3Device_;\n>> --\n>> 2.31.1\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 DDAB7BDC71\n\tfor ;\n\tTue, 21 Sep 2021 12:55:11 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 391AF6918A;\n\tTue, 21 Sep 2021 14:55: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 69E9960247\n\tfor ;\n\tTue, 21 Sep 2021 14:55:09 +0200 (CEST)","from [192.168.1.104] (unknown [103.251.226.144])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 743722BA;\n\tTue, 21 Sep 2021 14:55: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=\"SX8CIKmm\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1632228909;\n\tbh=i70GL4OouklDBt7kuW0SnwnVq8J3zCbA3DOiFaEKN1Q=;\n\th=Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=SX8CIKmmR6T15/8CE7l7KTv19SVW5XYzgVNNRRhLT9u779k0eCP66B4wu9Sl3EMU5\n\tbu7JJQYqFeMvcn4l48b/Cdbj6JV9H8p+gOccydKOs9ZJ9xR5vkiH84JLv9aPtd1Bs1\n\t1IFDaXj8rX2ewLcvDcjDgvn1puqbMXTcFvtcMA3c=","To":"Jacopo Mondi ","References":"<20210920173752.1346190-1-umang.jain@ideasonboard.com>\n\t<20210920173752.1346190-10-umang.jain@ideasonboard.com>\n\t<20210921114618.3viuxka3hiy7xfq4@uno.localdomain>","From":"Umang Jain ","Message-ID":"<785084bd-785f-926d-2cfb-15d1d697dab8@ideasonboard.com>","Date":"Tue, 21 Sep 2021 18:25:03 +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":"<20210921114618.3viuxka3hiy7xfq4@uno.localdomain>","Content-Type":"text/plain; charset=utf-8; format=flowed","Content-Transfer-Encoding":"8bit","Content-Language":"en-US","Subject":"Re: [libcamera-devel] [PATCH v3 09/10] android: camera_device: Send\n\tcapture results inspecting the descriptor","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":19760,"web_url":"https://patchwork.libcamera.org/comment/19760/","msgid":"","date":"2021-09-21T23:26:43","subject":"Re: [libcamera-devel] [PATCH v3 09/10] android: camera_device: Send\n\tcapture results inspecting the descriptor","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Umang,\n\nThank you for the patch.\n\nOn Tue, Sep 21, 2021 at 06:25:03PM +0530, Umang Jain wrote:\n> Hi Jacopo,\n> \n> (Post in call discussion summary on this if anyone here is interested)\n> \n> On 9/21/21 5:16 PM, Jacopo Mondi wrote:\n> > On Mon, Sep 20, 2021 at 11:07:51PM +0530, Umang Jain wrote:\n> >> A Camera3RequestDescriptors is constructed and queued to descriptors_\n\ns/Camera3RequestDescriptors/Camera3RequestDescriptor/\n\n> >> queue as soon as an (incoming) capture request is received on the\n> >> libcamera HAL. The capture request is picked up by HAL, in order to run\n> >> it to completion. At completion, CameraDevice::requestComplete() gets\n> >> invoked and capture results are populated and ready to be sent back\n> >> to the framework.\n> >>\n> >> All the data and framebuffers associated with the request are alive\n> >> and encapsulated inside this Camera3RequestDescriptor descriptor.\n> >> By inspecting the ProcessStatus on the descriptor, we can now send\n> >> capture results via the process_capture_result() callback.\n> >>\n> >> Hence, introduce a new private member function sendCaptureResults()\n> >> which will be responsible to send capture results back to the\n> >> framework by inspecting the descriptor on the queue. In subsequent\n> >> commit, when the post processsor shall run async, sendCaptureResults()\n> >> can be called from the post-processor's thread for e.g.\n> >> streamProcessComplete() hence, introduce the mutex lock to avoid the\n> >> races.\n> >>\n> >> Signed-off-by: Umang Jain \n> >> ---\n> >> src/android/camera_device.cpp | 47 +++++++++++++++++++++++++----------\n> >> src/android/camera_device.h | 4 +++\n> >> 2 files changed, 38 insertions(+), 13 deletions(-)\n> >>\n> >> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> >> index 4658e881..16ecdfc5 100644\n> >> --- a/src/android/camera_device.cpp\n> >> +++ b/src/android/camera_device.cpp\n> >> @@ -1078,7 +1078,7 @@ void CameraDevice::requestComplete(Request *request)\n> >> \t * The buffer status is set to OK and later changed to ERROR if\n> >> \t * post-processing/compression fails.\n> >> \t */\n> >> -\tcamera3_capture_result_t captureResult = {};\n> >> +\tcamera3_capture_result_t &captureResult = descriptor->captureResult_;\n> >> \tcaptureResult.frame_number = descriptor->frameNumber_;\n> >> \tcaptureResult.num_output_buffers = descriptor->buffers_.size();\n> >> \tfor (camera3_stream_buffer_t &buffer : descriptor->buffers_) {\n> >> @@ -1156,26 +1156,25 @@ void CameraDevice::requestComplete(Request *request)\n> >> \t\t\tcontinue;\n> >> \t\t}\n> >>\n> >> +\t\tif (cameraStream->type() == CameraStream::Type::Internal)\n> >> +\t\t\tdescriptor->internalBuffer_ = src;\n> >> +\n> >> \t\tdescriptor->status_ = Camera3RequestDescriptor::ProcessStatus::Processing;\n> >>\n> >> \t\tcameraStream->process(src, *buffer.buffer, descriptor);\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> >> +\t\treturn;\n> >> \t}\n> >>\n> >> -\tcaptureResult.result = descriptor->resultMetadata_->get();\n> >> -\tcallbacks_->process_capture_result(callbacks_, &captureResult);\n> >> -\n> >> -\tdescriptors_.pop_front();\n> >> +\t/*\n> >> +\t * Mark the status on the descriptor as success as no processing\n> >> +\t * is neeeded.\n> >> +\t */\n> >> +\tdescriptor->status_ = Camera3RequestDescriptor::ProcessStatus::Success;\n> >> +\tsendCaptureResults();\n> >> }\n> >>\n> >>\n> >> -void CameraDevice::streamProcessingComplete([[maybe_unused]] CameraStream *cameraStream,\n> >> +void CameraDevice::streamProcessingComplete(CameraStream *cameraStream,\n> >> \t\t\t\t\t Camera3RequestDescriptor *request)\n> >> {\n> >> \tif (request->status_ == Camera3RequestDescriptor::ProcessStatus::Error) {\n> >> @@ -1190,6 +1189,28 @@ void CameraDevice::streamProcessingComplete([[maybe_unused]] CameraStream *camer\n> >> \t\t\t\t CAMERA3_MSG_ERROR_BUFFER);\n> >> \t\t}\n> >> \t}\n> >> +\n> >> +\t/*\n> >> +\t * Return the FrameBuffer to the CameraStream now that we're\n> >> +\t * done processing it.\n> >> +\t */\n> >> +\tif (cameraStream->type() == CameraStream::Type::Internal)\n> >> +\t\tcameraStream->putBuffer(request->internalBuffer_);\n> >> +\n> >> +\tsendCaptureResults();\n> >> +}\n> >> +\n> >> +void CameraDevice::sendCaptureResults()\n> >> +{\n> >> +\tCamera3RequestDescriptor *d = descriptors_.front().get();\n\nThe variable is named descriptor everywhere else, let's keep a common\nnaming scheme (I'm tempted to rename Camera3RequestDescriptor to\nCamera3Request, and thus also rename all variables accordingly, but\nthat's for later, on top of this series).\n\n> >\n> > If I'm not mistaken, in case a request that doesn't need\n> > post-processing will complete while the post-processed one is still\n> > being worked on, this could lead to a completion order inversion\n> >\n> > processCaptureRequest(n)\n> > descriptors.push(n)\n> >\n> > processCaptureRequest(n+1)\n> > descriptors.push(n+1)\n> >\n> > equestComplete(n)\n> > postProcess(n) -----------> process(n)\n> > |\n> > .... ...\n> > |\n> > requestComplete(n+1) |\n> > sendCaptureResult(n+1) |\n> > d = descriptors.front() |\n> > process_capture_res(n) |\n> > |\n> > sendCaptureResult(n) <----------------\n> > d = descritpros.front()\n> > process_capture_res(n + 1)\n> >\n> \n> Nice flow diagram btw, I need to get better at that ;-)\n> \n> So, first of all sendCaptureResults() doesn't take an argument, but I \n> get your point that you are trying to match for which 'n' request, \n> sendCapture result is called...\n> \n> sendCaptureResults() works on the queue's head. It doesn't care from \n> where it's called; it can be called from requestComplete()[same thread] \n> or streamProcessingComplete()[different thread]. All it does or suppose \n> to do, is basically looks at the queue's head and see if the HEAD \n> descriptor  says - \"Send me back to the framework\".\n> - If yes, it will send the results via process_capture_result  of the \n> HEAD and drop the HEAD from the queue\n> - If no, simply return;\n> \n> That's it. Since, it looks at the head, it will only send results in \n> order (since it's a queue) and drop the HEAD.\n> \n> However, the discussion has un-covered another point that \n> sendCaptureResults() can lag behind w.r.t libcamera:Requests being \n> completed, since there is no loop around to process other descriptors in \n> the queue which are ready to send (these descriptors can potentially be \n> the ones not requiring any post-processing). As discussed in the call, \n> we both agree that we should probably iterate over the queue and send \n> the results back to the framework as soon as possible, for ready \n> descriptor. So, sendCaptureResults() will be wrapped in a loop in \n> subsequent version.\n\nI'd include the loop inside sendCaptureResults(). Something along the\nlines of\n\n\tMutexLocker lock(descriptorsMutex_);\n\n\twhile (!descriptors_.empty() &&\n\t descriptors_.front()->status_ != Camera3RequestDescriptor::Status::Pending) {\n\t\tstd::unique_ptr descriptor =\n\t\t\tstd::move(descriptors_.front());\n\t\tdescriptors_.pop();\n\n\t\tlock.unlock();\n\n\t\t/* Signal completion to the camera service. */\n\n\t\tlock.lock();\n\t}\n\n(see my review of 07/10 for the Pending status)\n\nNote that you'll need to handle both the Success and Error statuses\nhere, as requests that complete with an error need to be completed in\norder too.\n\nOne a side note, maybe adding the following helper to\nCamera3RequestDescriptor would be nice, to make the loop more readable:\n\n\tbool isPending() const { return status_ == Status::Pending; }\n\nYou would then get\n\n\twhile (!descriptors_.empty() && descriptors_.front()->isPending()) {\n\t\t...\n\t}\n\n> >> +\tif (d->status_ == Camera3RequestDescriptor::ProcessStatus::Processing ||\n> >> +\t d->status_ == Camera3RequestDescriptor::ProcessStatus::None)\n> >> +\t\treturn;\n> >> +\n> >> +\tMutexLocker lock(descriptorsMutex_);\n> >> +\td->captureResult_.result = d->resultMetadata_->get();\n> >> +\tcallbacks_->process_capture_result(callbacks_, &(d->captureResult_));\n> >> +\tdescriptors_.pop_front();\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 60c134dc..0bd570a1 100644\n> >> --- a/src/android/camera_device.h\n> >> +++ b/src/android/camera_device.h\n> >> @@ -56,6 +56,9 @@ struct Camera3RequestDescriptor {\n> >> \tstd::unique_ptr request_;\n> >> \tstd::unique_ptr resultMetadata_;\n> >>\n> >> +\tcamera3_capture_result_t captureResult_ = {};\n> >> +\tlibcamera::FrameBuffer *internalBuffer_;\n> >> +\n> >> \tProcessStatus status_;\n> >> };\n> >>\n> >> @@ -118,6 +121,7 @@ private:\n> >> \tint processControls(Camera3RequestDescriptor *descriptor);\n> >> \tstd::unique_ptr getResultMetadata(\n> >> \t\tconst Camera3RequestDescriptor *descriptor) const;\n> >> +\tvoid sendCaptureResults();\n> >>\n> >> \tunsigned int id_;\n> >> \tcamera3_device_t camera3Device_;","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 46340BF01C\n\tfor ;\n\tTue, 21 Sep 2021 23:27:18 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9F38669191;\n\tWed, 22 Sep 2021 01:27:16 +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 B6F2E69186\n\tfor ;\n\tWed, 22 Sep 2021 01:27:14 +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 49F342BA;\n\tWed, 22 Sep 2021 01:27:14 +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=\"dFqJaL5b\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1632266834;\n\tbh=CyazksLI2uZiRO7lIX7iewDCtP+EPkf5wU+WVRA01wo=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=dFqJaL5bQuovozfKIQz0O3y1J0wWvGs+v7SnXirG81j0nJmKS8rGVBcwHUtyyaXzR\n\tQxaPHZBXK3/ksXtXBFJOEQ3DJ52Pra2X0e0TBT1fk5THwNz5BHIRX+SutI7kyKRGMz\n\tivrXhmUe4pRv7KiJ20LW2bUjpvzIPpEaG/gskxPY=","Date":"Wed, 22 Sep 2021 02:26:43 +0300","From":"Laurent Pinchart ","To":"Umang Jain ","Message-ID":"","References":"<20210920173752.1346190-1-umang.jain@ideasonboard.com>\n\t<20210920173752.1346190-10-umang.jain@ideasonboard.com>\n\t<20210921114618.3viuxka3hiy7xfq4@uno.localdomain>\n\t<785084bd-785f-926d-2cfb-15d1d697dab8@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<785084bd-785f-926d-2cfb-15d1d697dab8@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v3 09/10] android: camera_device: Send\n\tcapture results inspecting the descriptor","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":19869,"web_url":"https://patchwork.libcamera.org/comment/19869/","msgid":"","date":"2021-09-27T07:02:14","subject":"Re: [libcamera-devel] [PATCH v3 09/10] android: camera_device: Send\n\tcapture results inspecting the descriptor","submitter":{"id":63,"url":"https://patchwork.libcamera.org/api/people/63/","name":"Hirokazu Honda","email":"hiroh@chromium.org"},"content":"Hi Umang, thank you for the patch.\n\nOn Wed, Sep 22, 2021 at 8:27 AM Laurent Pinchart\n wrote:\n>\n> Hi Umang,\n>\n> Thank you for the patch.\n>\n> On Tue, Sep 21, 2021 at 06:25:03PM +0530, Umang Jain wrote:\n> > Hi Jacopo,\n> >\n> > (Post in call discussion summary on this if anyone here is interested)\n> >\n> > On 9/21/21 5:16 PM, Jacopo Mondi wrote:\n> > > On Mon, Sep 20, 2021 at 11:07:51PM +0530, Umang Jain wrote:\n> > >> A Camera3RequestDescriptors is constructed and queued to descriptors_\n>\n> s/Camera3RequestDescriptors/Camera3RequestDescriptor/\n>\n> > >> queue as soon as an (incoming) capture request is received on the\n> > >> libcamera HAL. The capture request is picked up by HAL, in order to run\n> > >> it to completion. At completion, CameraDevice::requestComplete() gets\n> > >> invoked and capture results are populated and ready to be sent back\n> > >> to the framework.\n> > >>\n> > >> All the data and framebuffers associated with the request are alive\n> > >> and encapsulated inside this Camera3RequestDescriptor descriptor.\n> > >> By inspecting the ProcessStatus on the descriptor, we can now send\n> > >> capture results via the process_capture_result() callback.\n> > >>\n> > >> Hence, introduce a new private member function sendCaptureResults()\n> > >> which will be responsible to send capture results back to the\n> > >> framework by inspecting the descriptor on the queue. In subsequent\n> > >> commit, when the post processsor shall run async, sendCaptureResults()\n> > >> can be called from the post-processor's thread for e.g.\n> > >> streamProcessComplete() hence, introduce the mutex lock to avoid the\n> > >> races.\n> > >>\n> > >> Signed-off-by: Umang Jain \n> > >> ---\n> > >> src/android/camera_device.cpp | 47 +++++++++++++++++++++++++----------\n> > >> src/android/camera_device.h | 4 +++\n> > >> 2 files changed, 38 insertions(+), 13 deletions(-)\n> > >>\n> > >> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> > >> index 4658e881..16ecdfc5 100644\n> > >> --- a/src/android/camera_device.cpp\n> > >> +++ b/src/android/camera_device.cpp\n> > >> @@ -1078,7 +1078,7 @@ void CameraDevice::requestComplete(Request *request)\n> > >> * The buffer status is set to OK and later changed to ERROR if\n> > >> * post-processing/compression fails.\n> > >> */\n> > >> - camera3_capture_result_t captureResult = {};\n> > >> + camera3_capture_result_t &captureResult = descriptor->captureResult_;\n> > >> captureResult.frame_number = descriptor->frameNumber_;\n> > >> captureResult.num_output_buffers = descriptor->buffers_.size();\n> > >> for (camera3_stream_buffer_t &buffer : descriptor->buffers_) {\n> > >> @@ -1156,26 +1156,25 @@ void CameraDevice::requestComplete(Request *request)\n> > >> continue;\n> > >> }\n> > >>\n> > >> + if (cameraStream->type() == CameraStream::Type::Internal)\n> > >> + descriptor->internalBuffer_ = src;\n> > >> +\n> > >> descriptor->status_ = Camera3RequestDescriptor::ProcessStatus::Processing;\n> > >>\n> > >> cameraStream->process(src, *buffer.buffer, descriptor);\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> > >> + return;\n\nI read the patches 07-09/10.\n\nThe new code looks to assume the number of buffers that require post\nprocessing is only one.\nI consider so from\n1.) return added here\n2.) no protection to the Status\n3.) no check to wait for the number of pot-processing in\nstreamProcessingComplete(), so that a capture request complete is sent\nmultiple times.\n\nI also think descriptors_ pop logic is wrong.\nI think we ideally deny a request given in requestComplete if it is\nnot a top of descriptors_.\nThe top of descriptors is kept to the currently processing capture request.\nI think we should have two queues here like following.\ndescriptors_ - for request processed by camera\ndescriptors_on_processing - for request processed by post processors\nThen we pop descriptors_ in completeRequest and descriptors_on_processing.\n\nEven if a capture request doesn't require post processing, we should\nadd it to descriptors_on_processing to send it after processing for\nall the preceding request is complete\n\nThese may be resolved in the next patch. But this happens at least at\nthis patch if I understand correctly.\n\n-Hiro\n> > >> }\n> > >>\n> > >> - captureResult.result = descriptor->resultMetadata_->get();\n> > >> - callbacks_->process_capture_result(callbacks_, &captureResult);\n> > >> -\n> > >> - descriptors_.pop_front();\n> > >> + /*\n> > >> + * Mark the status on the descriptor as success as no processing\n> > >> + * is neeeded.\n> > >> + */\n> > >> + descriptor->status_ = Camera3RequestDescriptor::ProcessStatus::Success;\n> > >> + sendCaptureResults();\n> > >> }\n> > >>\n> > >>\n> > >> -void CameraDevice::streamProcessingComplete([[maybe_unused]] CameraStream *cameraStream,\n> > >> +void CameraDevice::streamProcessingComplete(CameraStream *cameraStream,\n> > >> Camera3RequestDescriptor *request)\n> > >> {\n> > >> if (request->status_ == Camera3RequestDescriptor::ProcessStatus::Error) {\n> > >> @@ -1190,6 +1189,28 @@ void CameraDevice::streamProcessingComplete([[maybe_unused]] CameraStream *camer\n> > >> CAMERA3_MSG_ERROR_BUFFER);\n> > >> }\n> > >> }\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(request->internalBuffer_);\n> > >> +\n> > >> + sendCaptureResults();\n> > >> +}\n> > >> +\n> > >> +void CameraDevice::sendCaptureResults()\n> > >> +{\n> > >> + Camera3RequestDescriptor *d = descriptors_.front().get();\n>\n> The variable is named descriptor everywhere else, let's keep a common\n> naming scheme (I'm tempted to rename Camera3RequestDescriptor to\n> Camera3Request, and thus also rename all variables accordingly, but\n> that's for later, on top of this series).\n>\n> > >\n> > > If I'm not mistaken, in case a request that doesn't need\n> > > post-processing will complete while the post-processed one is still\n> > > being worked on, this could lead to a completion order inversion\n> > >\n> > > processCaptureRequest(n)\n> > > descriptors.push(n)\n> > >\n> > > processCaptureRequest(n+1)\n> > > descriptors.push(n+1)\n> > >\n> > > equestComplete(n)\n> > > postProcess(n) -----------> process(n)\n> > > |\n> > > .... ...\n> > > |\n> > > requestComplete(n+1) |\n> > > sendCaptureResult(n+1) |\n> > > d = descriptors.front() |\n> > > process_capture_res(n) |\n> > > |\n> > > sendCaptureResult(n) <----------------\n> > > d = descritpros.front()\n> > > process_capture_res(n + 1)\n> > >\n> >\n> > Nice flow diagram btw, I need to get better at that ;-)\n> >\n> > So, first of all sendCaptureResults() doesn't take an argument, but I\n> > get your point that you are trying to match for which 'n' request,\n> > sendCapture result is called...\n> >\n> > sendCaptureResults() works on the queue's head. It doesn't care from\n> > where it's called; it can be called from requestComplete()[same thread]\n> > or streamProcessingComplete()[different thread]. All it does or suppose\n> > to do, is basically looks at the queue's head and see if the HEAD\n> > descriptor says - \"Send me back to the framework\".\n> > - If yes, it will send the results via process_capture_result of the\n> > HEAD and drop the HEAD from the queue\n> > - If no, simply return;\n> >\n> > That's it. Since, it looks at the head, it will only send results in\n> > order (since it's a queue) and drop the HEAD.\n> >\n> > However, the discussion has un-covered another point that\n> > sendCaptureResults() can lag behind w.r.t libcamera:Requests being\n> > completed, since there is no loop around to process other descriptors in\n> > the queue which are ready to send (these descriptors can potentially be\n> > the ones not requiring any post-processing). As discussed in the call,\n> > we both agree that we should probably iterate over the queue and send\n> > the results back to the framework as soon as possible, for ready\n> > descriptor. So, sendCaptureResults() will be wrapped in a loop in\n> > subsequent version.\n>\n> I'd include the loop inside sendCaptureResults(). Something along the\n> lines of\n>\n> MutexLocker lock(descriptorsMutex_);\n>\n> while (!descriptors_.empty() &&\n> descriptors_.front()->status_ != Camera3RequestDescriptor::Status::Pending) {\n> std::unique_ptr descriptor =\n> std::move(descriptors_.front());\n> descriptors_.pop();\n>\n> lock.unlock();\n>\n> /* Signal completion to the camera service. */\n>\n> lock.lock();\n> }\n>\n> (see my review of 07/10 for the Pending status)\n>\n> Note that you'll need to handle both the Success and Error statuses\n> here, as requests that complete with an error need to be completed in\n> order too.\n>\n> One a side note, maybe adding the following helper to\n> Camera3RequestDescriptor would be nice, to make the loop more readable:\n>\n> bool isPending() const { return status_ == Status::Pending; }\n>\n> You would then get\n>\n> while (!descriptors_.empty() && descriptors_.front()->isPending()) {\n> ...\n> }\n>\n> > >> + if (d->status_ == Camera3RequestDescriptor::ProcessStatus::Processing ||\n> > >> + d->status_ == Camera3RequestDescriptor::ProcessStatus::None)\n> > >> + return;\n> > >> +\n> > >> + MutexLocker lock(descriptorsMutex_);\n> > >> + d->captureResult_.result = d->resultMetadata_->get();\n> > >> + callbacks_->process_capture_result(callbacks_, &(d->captureResult_));\n> > >> + descriptors_.pop_front();\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 60c134dc..0bd570a1 100644\n> > >> --- a/src/android/camera_device.h\n> > >> +++ b/src/android/camera_device.h\n> > >> @@ -56,6 +56,9 @@ struct Camera3RequestDescriptor {\n> > >> std::unique_ptr request_;\n> > >> std::unique_ptr resultMetadata_;\n> > >>\n> > >> + camera3_capture_result_t captureResult_ = {};\n> > >> + libcamera::FrameBuffer *internalBuffer_;\n> > >> +\n> > >> ProcessStatus status_;\n> > >> };\n> > >>\n> > >> @@ -118,6 +121,7 @@ private:\n> > >> int processControls(Camera3RequestDescriptor *descriptor);\n> > >> std::unique_ptr getResultMetadata(\n> > >> const Camera3RequestDescriptor *descriptor) const;\n> > >> + void sendCaptureResults();\n> > >>\n> > >> unsigned int id_;\n> > >> camera3_device_t camera3Device_;\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 3F463BF01C\n\tfor ;\n\tMon, 27 Sep 2021 07:02:27 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 11F3F6918F;\n\tMon, 27 Sep 2021 09:02:27 +0200 (CEST)","from mail-ed1-x536.google.com (mail-ed1-x536.google.com\n\t[IPv6:2a00:1450:4864:20::536])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id AC64569188\n\tfor ;\n\tMon, 27 Sep 2021 09:02:25 +0200 (CEST)","by mail-ed1-x536.google.com with SMTP id b26so16654005edt.0\n\tfor ;\n\tMon, 27 Sep 2021 00:02:25 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"mtC4bz1+\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org;\n\ts=google; \n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=SIVnaK48ozfuK0rE0P0IIcXLrWK0/DzbaKhhC1EMHmM=;\n\tb=mtC4bz1+O46+B7ZE1JIy9og9qrEv3RQrAYL3uHDvAPZFGj7XraC/I9O3pKgog4nhCm\n\tBbGlo4lLQVICp3RFLoYTwn7E6zeX/WskkEqxs8kyiZ3AAW4HmjdOiLVKaaJ4jYJ47WpY\n\tAv3rIjjB5zNdzX9fNtKEfpjWPNNvyfAaDg5t0=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=SIVnaK48ozfuK0rE0P0IIcXLrWK0/DzbaKhhC1EMHmM=;\n\tb=4Ry3B3U5MbX48CPd9jEAjGbLEUqsXqLfxVMGP/fC0Xpg1f2zyyfUq7TA+uwlEmbv2W\n\tkKEDUVDNb29OHgE83jcjDUOJU4bSNMj/r8P9IwHzjCGkdYQwu2s/61iFXKcZ4yogZast\n\tNCVTkCkn+ZU3lGPRy8r2gFA8L4RHZUFVdg8OvDgd7ATJw3jtdz2cP3h40hKBjTDWci5f\n\tvN7aTo1SSFt1jH3RJLnfY5hKT89QVf5ewLfLkTaxKDrEAO7Vtdj9XKf+jxDXdAeQoD8i\n\t7si8f3HZxvbHJQGSJo5HZ73uIduyp24tRuG0GmvK8ycjU4zb/pfMGg+gY0pj3J1fGrGM\n\tqEQg==","X-Gm-Message-State":"AOAM533B+x5axR7Sy2DI5NYssBMur5ds7mro4eVIZ9vIfTHk9j9N+2kE\n\tEGJhFfKusjyXy6REoob0X6t0IdTX9Sm7pUPi1RKklykqqaAf4g==","X-Google-Smtp-Source":"ABdhPJyFRoBNJLa4grAi1FW+LD5wToT6Y0MJtw4id+SClVaTEMAQplzjRJ0XC5ktwLR7vaALGNjqsyyago6LrTq4oWE=","X-Received":"by 2002:aa7:c401:: with SMTP id j1mr3714176edq.354.1632726145280;\n\tMon, 27 Sep 2021 00:02:25 -0700 (PDT)","MIME-Version":"1.0","References":"<20210920173752.1346190-1-umang.jain@ideasonboard.com>\n\t<20210920173752.1346190-10-umang.jain@ideasonboard.com>\n\t<20210921114618.3viuxka3hiy7xfq4@uno.localdomain>\n\t<785084bd-785f-926d-2cfb-15d1d697dab8@ideasonboard.com>\n\t","In-Reply-To":"","From":"Hirokazu Honda ","Date":"Mon, 27 Sep 2021 16:02:14 +0900","Message-ID":"","To":"Laurent Pinchart ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH v3 09/10] android: camera_device: Send\n\tcapture results inspecting the descriptor","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 ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":19874,"web_url":"https://patchwork.libcamera.org/comment/19874/","msgid":"<25d0c5e0-aaa8-3abe-0ef5-1211ddf0b0e6@ideasonboard.com>","date":"2021-09-27T11:26:28","subject":"Re: [libcamera-devel] [PATCH v3 09/10] android: camera_device: Send\n\tcapture results inspecting the descriptor","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hi Hiro,\n\nOn 9/27/21 12:32 PM, Hirokazu Honda wrote:\n> Hi Umang, thank you for the patch.\n>\n> On Wed, Sep 22, 2021 at 8:27 AM Laurent Pinchart\n> wrote:\n>> Hi Umang,\n>>\n>> Thank you for the patch.\n>>\n>> On Tue, Sep 21, 2021 at 06:25:03PM +0530, Umang Jain wrote:\n>>> Hi Jacopo,\n>>>\n>>> (Post in call discussion summary on this if anyone here is interested)\n>>>\n>>> On 9/21/21 5:16 PM, Jacopo Mondi wrote:\n>>>> On Mon, Sep 20, 2021 at 11:07:51PM +0530, Umang Jain wrote:\n>>>>> A Camera3RequestDescriptors is constructed and queued to descriptors_\n>> s/Camera3RequestDescriptors/Camera3RequestDescriptor/\n>>\n>>>>> queue as soon as an (incoming) capture request is received on the\n>>>>> libcamera HAL. The capture request is picked up by HAL, in order to run\n>>>>> it to completion. At completion, CameraDevice::requestComplete() gets\n>>>>> invoked and capture results are populated and ready to be sent back\n>>>>> to the framework.\n>>>>>\n>>>>> All the data and framebuffers associated with the request are alive\n>>>>> and encapsulated inside this Camera3RequestDescriptor descriptor.\n>>>>> By inspecting the ProcessStatus on the descriptor, we can now send\n>>>>> capture results via the process_capture_result() callback.\n>>>>>\n>>>>> Hence, introduce a new private member function sendCaptureResults()\n>>>>> which will be responsible to send capture results back to the\n>>>>> framework by inspecting the descriptor on the queue. In subsequent\n>>>>> commit, when the post processsor shall run async, sendCaptureResults()\n>>>>> can be called from the post-processor's thread for e.g.\n>>>>> streamProcessComplete() hence, introduce the mutex lock to avoid the\n>>>>> races.\n>>>>>\n>>>>> Signed-off-by: Umang Jain \n>>>>> ---\n>>>>> src/android/camera_device.cpp | 47 +++++++++++++++++++++++++----------\n>>>>> src/android/camera_device.h | 4 +++\n>>>>> 2 files changed, 38 insertions(+), 13 deletions(-)\n>>>>>\n>>>>> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n>>>>> index 4658e881..16ecdfc5 100644\n>>>>> --- a/src/android/camera_device.cpp\n>>>>> +++ b/src/android/camera_device.cpp\n>>>>> @@ -1078,7 +1078,7 @@ void CameraDevice::requestComplete(Request *request)\n>>>>> * The buffer status is set to OK and later changed to ERROR if\n>>>>> * post-processing/compression fails.\n>>>>> */\n>>>>> - camera3_capture_result_t captureResult = {};\n>>>>> + camera3_capture_result_t &captureResult = descriptor->captureResult_;\n>>>>> captureResult.frame_number = descriptor->frameNumber_;\n>>>>> captureResult.num_output_buffers = descriptor->buffers_.size();\n>>>>> for (camera3_stream_buffer_t &buffer : descriptor->buffers_) {\n>>>>> @@ -1156,26 +1156,25 @@ void CameraDevice::requestComplete(Request *request)\n>>>>> continue;\n>>>>> }\n>>>>>\n>>>>> + if (cameraStream->type() == CameraStream::Type::Internal)\n>>>>> + descriptor->internalBuffer_ = src;\n>>>>> +\n>>>>> descriptor->status_ = Camera3RequestDescriptor::ProcessStatus::Processing;\n>>>>>\n>>>>> cameraStream->process(src, *buffer.buffer, descriptor);\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>>>>> + return;\n> I read the patches 07-09/10.\n>\n> The new code looks to assume the number of buffers that require post\n> processing is only one.\n> I consider so from\n> 1.) return added here\n> 2.) no protection to the Status\n> 3.) no check to wait for the number of pot-processing in\n> streamProcessingComplete(), so that a capture request complete is sent\n> multiple times.\n>\n> I also think descriptors_ pop logic is wrong.\n> I think we ideally deny a request given in requestComplete if it is\n> not a top of descriptors_.\n> The top of descriptors is kept to the currently processing capture request.\n> I think we should have two queues here like following.\n> descriptors_ - for request processed by camera\n> descriptors_on_processing - for request processed by post processors\n> Then we pop descriptors_ in completeRequest and descriptors_on_processing.\n>\n> Even if a capture request doesn't require post processing, we should\n> add it to descriptors_on_processing to send it after processing for\n> all the preceding request is complete\n>\n> These may be resolved in the next patch. But this happens at least at\n> this patch if I understand correctly.\n\n\nThe v3 *now* is a mere reference for future development on this front. \nWe have split up design of HAL into 3 parts and have been internally \ndiscussing to even clean up more the implementation that we have today. \nIt's still work-in-progress but I will posting the relevant modules of \nthe design that will eventually lead up to setting up infrastructure \nrequired for async post-processors.\n\nMore or less broadly:\n\n- We expect to have only queue only - to track each incoming request, \nirrespective of whether it requires post-processing or not\n\n- status variables will be set on Camera3RequestDescriptors-wide and \nthen Post-Processing-wide separately\n\n- Async Post-processing somehow needs to be \"cancellable\" on demand - to \nsatisfy flush use cases\n\n- Single point of sending back capture results ONLY a.k.a single \nprocess_capture_result\n\nThese are the hard requirements we are tracking to clean up the \nimplementation we have currently. It's substantial work plus keeping in \nmind not to regress CTS (which has happened recently, even hampering \ndevelopment on this front). I may / may not address reviews on this \nparticular v3, since I will be more inclined to develop in line to our \ndesign goals, while treating this series and it's reviews just as a \npoint of reference at this point.\n\nThanks!\n\n>\n> -Hiro\n>>>>> }\n>>>>>\n>>>>> - captureResult.result = descriptor->resultMetadata_->get();\n>>>>> - callbacks_->process_capture_result(callbacks_, &captureResult);\n>>>>> -\n>>>>> - descriptors_.pop_front();\n>>>>> + /*\n>>>>> + * Mark the status on the descriptor as success as no processing\n>>>>> + * is neeeded.\n>>>>> + */\n>>>>> + descriptor->status_ = Camera3RequestDescriptor::ProcessStatus::Success;\n>>>>> + sendCaptureResults();\n>>>>> }\n>>>>>\n>>>>>\n>>>>> -void CameraDevice::streamProcessingComplete([[maybe_unused]] CameraStream *cameraStream,\n>>>>> +void CameraDevice::streamProcessingComplete(CameraStream *cameraStream,\n>>>>> Camera3RequestDescriptor *request)\n>>>>> {\n>>>>> if (request->status_ == Camera3RequestDescriptor::ProcessStatus::Error) {\n>>>>> @@ -1190,6 +1189,28 @@ void CameraDevice::streamProcessingComplete([[maybe_unused]] CameraStream *camer\n>>>>> CAMERA3_MSG_ERROR_BUFFER);\n>>>>> }\n>>>>> }\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(request->internalBuffer_);\n>>>>> +\n>>>>> + sendCaptureResults();\n>>>>> +}\n>>>>> +\n>>>>> +void CameraDevice::sendCaptureResults()\n>>>>> +{\n>>>>> + Camera3RequestDescriptor *d = descriptors_.front().get();\n>> The variable is named descriptor everywhere else, let's keep a common\n>> naming scheme (I'm tempted to rename Camera3RequestDescriptor to\n>> Camera3Request, and thus also rename all variables accordingly, but\n>> that's for later, on top of this series).\n>>\n>>>> If I'm not mistaken, in case a request that doesn't need\n>>>> post-processing will complete while the post-processed one is still\n>>>> being worked on, this could lead to a completion order inversion\n>>>>\n>>>> processCaptureRequest(n)\n>>>> descriptors.push(n)\n>>>>\n>>>> processCaptureRequest(n+1)\n>>>> descriptors.push(n+1)\n>>>>\n>>>> equestComplete(n)\n>>>> postProcess(n) -----------> process(n)\n>>>> |\n>>>> .... ...\n>>>> |\n>>>> requestComplete(n+1) |\n>>>> sendCaptureResult(n+1) |\n>>>> d = descriptors.front() |\n>>>> process_capture_res(n) |\n>>>> |\n>>>> sendCaptureResult(n) <----------------\n>>>> d = descritpros.front()\n>>>> process_capture_res(n + 1)\n>>>>\n>>> Nice flow diagram btw, I need to get better at that ;-)\n>>>\n>>> So, first of all sendCaptureResults() doesn't take an argument, but I\n>>> get your point that you are trying to match for which 'n' request,\n>>> sendCapture result is called...\n>>>\n>>> sendCaptureResults() works on the queue's head. It doesn't care from\n>>> where it's called; it can be called from requestComplete()[same thread]\n>>> or streamProcessingComplete()[different thread]. All it does or suppose\n>>> to do, is basically looks at the queue's head and see if the HEAD\n>>> descriptor says - \"Send me back to the framework\".\n>>> - If yes, it will send the results via process_capture_result of the\n>>> HEAD and drop the HEAD from the queue\n>>> - If no, simply return;\n>>>\n>>> That's it. Since, it looks at the head, it will only send results in\n>>> order (since it's a queue) and drop the HEAD.\n>>>\n>>> However, the discussion has un-covered another point that\n>>> sendCaptureResults() can lag behind w.r.t libcamera:Requests being\n>>> completed, since there is no loop around to process other descriptors in\n>>> the queue which are ready to send (these descriptors can potentially be\n>>> the ones not requiring any post-processing). As discussed in the call,\n>>> we both agree that we should probably iterate over the queue and send\n>>> the results back to the framework as soon as possible, for ready\n>>> descriptor. So, sendCaptureResults() will be wrapped in a loop in\n>>> subsequent version.\n>> I'd include the loop inside sendCaptureResults(). Something along the\n>> lines of\n>>\n>> MutexLocker lock(descriptorsMutex_);\n>>\n>> while (!descriptors_.empty() &&\n>> descriptors_.front()->status_ != Camera3RequestDescriptor::Status::Pending) {\n>> std::unique_ptr descriptor =\n>> std::move(descriptors_.front());\n>> descriptors_.pop();\n>>\n>> lock.unlock();\n>>\n>> /* Signal completion to the camera service. */\n>>\n>> lock.lock();\n>> }\n>>\n>> (see my review of 07/10 for the Pending status)\n>>\n>> Note that you'll need to handle both the Success and Error statuses\n>> here, as requests that complete with an error need to be completed in\n>> order too.\n>>\n>> One a side note, maybe adding the following helper to\n>> Camera3RequestDescriptor would be nice, to make the loop more readable:\n>>\n>> bool isPending() const { return status_ == Status::Pending; }\n>>\n>> You would then get\n>>\n>> while (!descriptors_.empty() && descriptors_.front()->isPending()) {\n>> ...\n>> }\n>>\n>>>>> + if (d->status_ == Camera3RequestDescriptor::ProcessStatus::Processing ||\n>>>>> + d->status_ == Camera3RequestDescriptor::ProcessStatus::None)\n>>>>> + return;\n>>>>> +\n>>>>> + MutexLocker lock(descriptorsMutex_);\n>>>>> + d->captureResult_.result = d->resultMetadata_->get();\n>>>>> + callbacks_->process_capture_result(callbacks_, &(d->captureResult_));\n>>>>> + descriptors_.pop_front();\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 60c134dc..0bd570a1 100644\n>>>>> --- a/src/android/camera_device.h\n>>>>> +++ b/src/android/camera_device.h\n>>>>> @@ -56,6 +56,9 @@ struct Camera3RequestDescriptor {\n>>>>> std::unique_ptr request_;\n>>>>> std::unique_ptr resultMetadata_;\n>>>>>\n>>>>> + camera3_capture_result_t captureResult_ = {};\n>>>>> + libcamera::FrameBuffer *internalBuffer_;\n>>>>> +\n>>>>> ProcessStatus status_;\n>>>>> };\n>>>>>\n>>>>> @@ -118,6 +121,7 @@ private:\n>>>>> int processControls(Camera3RequestDescriptor *descriptor);\n>>>>> std::unique_ptr getResultMetadata(\n>>>>> const Camera3RequestDescriptor *descriptor) const;\n>>>>> + void sendCaptureResults();\n>>>>>\n>>>>> unsigned int id_;\n>>>>> camera3_device_t camera3Device_;\n>> --\n>> Regards,\n>>\n>> Laurent Pinchart","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id BC39FBDC71\n\tfor ;\n\tMon, 27 Sep 2021 11:26:35 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E732D6918E;\n\tMon, 27 Sep 2021 13:26:34 +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 CAD6B6012C\n\tfor ;\n\tMon, 27 Sep 2021 13:26:33 +0200 (CEST)","from [192.168.1.104] (unknown [103.251.226.181])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id AF10DB91;\n\tMon, 27 Sep 2021 13:26:32 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"Cd2OYjwS\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1632741993;\n\tbh=eZkEumnjw9MF6b1ZOHdDemqs0r9LP9smrEaNvUnZuZE=;\n\th=Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=Cd2OYjwS1VkjXwi1ipXRKUZ+TFK9boWOjGmlJcfbogwkDwkyZGcZnbTzQayaZCPPA\n\t2/DRhmgsekL5TDpQGxIJNy8xIj64xQELY23g0UlS/T/ouky7cZ/WAnO2TZlUEGhSgM\n\tkwr9Ze2pzrRIX2hb2iQEATLv4eaRU7RIU2ODlIMk=","To":"Hirokazu Honda ","References":"<20210920173752.1346190-1-umang.jain@ideasonboard.com>\n\t<20210920173752.1346190-10-umang.jain@ideasonboard.com>\n\t<20210921114618.3viuxka3hiy7xfq4@uno.localdomain>\n\t<785084bd-785f-926d-2cfb-15d1d697dab8@ideasonboard.com>\n\t\n\t","From":"Umang Jain ","Message-ID":"<25d0c5e0-aaa8-3abe-0ef5-1211ddf0b0e6@ideasonboard.com>","Date":"Mon, 27 Sep 2021 16:56:28 +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":"7bit","Content-Language":"en-US","Subject":"Re: [libcamera-devel] [PATCH v3 09/10] android: camera_device: Send\n\tcapture results inspecting the descriptor","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 ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]