[{"id":20335,"web_url":"https://patchwork.libcamera.org/comment/20335/","msgid":"","date":"2021-10-21T00:31:26","subject":"Re: [libcamera-devel] [PATCH v5 1/4] android: Notify post\n\tprocessing completion via a signal","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 Wed, Oct 20, 2021 at 04:12:09PM +0530, Umang Jain wrote:\n> Notify that the post processing for a request has been completed,\n> via a signal. A pointer to the descriptor which is tracking the\n> capture request is emitted along with the status of post processed\n> buffer. The function CameraDevice::streamProcessingComplete() will\n> finally set the status on the request descriptor and send capture\n> results back to the framework accordingly.\n> \n> We also need to save a pointer to any internal buffers that might have\n> been allocated by CameraStream. The buffer should be returned back to\n> CameraStream just before capture results are sent.\n> \n> A streamProcessMutex_ has been introduced here itself, which will be\n\nDid you mean s/itself/as well/ ?\n\n> applicable to guard access to descriptor->buffers_ when post-processing\n> is moved to be asynchronous in subsequent commits.\n> \n> Signed-off-by: Umang Jain \n> ---\n> src/android/camera_device.cpp | 92 +++++++++++++++++++++---\n> src/android/camera_device.h | 7 ++\n> src/android/camera_request.h | 4 ++\n> src/android/camera_stream.cpp | 13 ++++\n> src/android/jpeg/post_processor_jpeg.cpp | 2 +\n> src/android/post_processor.h | 9 +++\n> src/android/yuv/post_processor_yuv.cpp | 10 ++-\n> 7 files changed, 125 insertions(+), 12 deletions(-)\n> \n> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> index 806b4090..541c2c81 100644\n> --- a/src/android/camera_device.cpp\n> +++ b/src/android/camera_device.cpp\n> @@ -1117,6 +1117,15 @@ void CameraDevice::requestComplete(Request *request)\n> \t}\n> \n> \t/* Handle post-processing. */\n> +\tbool needsPostProcessing = false;\n> +\tCamera3RequestDescriptor::Status processingStatus =\n> +\t\tCamera3RequestDescriptor::Status::Pending;\n> +\t/*\n> +\t * \\todo Apply streamsProcessMutex_ when post-processing is adapted to run\n> +\t * asynchronously. If we introduce the streamsProcessMutex_ right away, the\n> +\t * lock will be held forever since it is synchronous at this point\n> +\t * (see streamProcessingComplete).\n> +\t */\n> \tfor (auto &buffer : descriptor->buffers_) {\n> \t\tCameraStream *stream = buffer.stream;\n> \n> @@ -1132,22 +1141,27 @@ void CameraDevice::requestComplete(Request *request)\n> \t\t\tcontinue;\n> \t\t}\n> \n> -\t\tint ret = stream->process(*src, 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 (stream->type() == CameraStream::Type::Internal)\n> -\t\t\tstream->putBuffer(src);\n> +\t\t\tbuffer.internalBuffer = src;\n\nLet's make the field name more self-explanatory. A possible candidate is\nsourceBuffer instead of internalBuffer.\n\nCould we do this in CameraDevice::processCaptureRequest(), just after\ncalling cameraStream->getBuffer() ? I'll feel less worried about a leak\nif we store the buffer for later release right after acquiring it.\n\n> \n> +\t\tneedsPostProcessing = true;\n> +\t\tint ret = stream->process(*src, buffer, descriptor);\n> \t\tif (ret) {\n> -\t\t\tbuffer.status = Camera3RequestDescriptor::Status::Error;\n> -\t\t\tnotifyError(descriptor->frameNumber_, stream->camera3Stream(),\n> -\t\t\t\t CAMERA3_MSG_ERROR_BUFFER);\n> +\t\t\tsetBufferStatus(stream, buffer, descriptor,\n> +\t\t\t\t\tCamera3RequestDescriptor::Status::Error);\n> +\t\t\tprocessingStatus = Camera3RequestDescriptor::Status::Error;\n> \t\t}\n\nI think we need to improve error handling a little bit, in order to\nsupport multiple streams being post-processed. We need to complete the\nrequest in CameraDevice::streamProcessingComplete() when the last\nprocessing stream completes, so the number of post-processed streams\nneed to be somehow recorded in the Camera3RequestDescriptor. It could be\ntempted to have a counter, but a\n\n\tstd::map pendingPostProcess_;\n\ncould be better, as it will also remove the need for loops in\nstreamProcessingComplete() to lookup the buffer from the stream and to\ncheck if the request has been fully processed. Accessing the map has to\nbe done with the mutex held to avoid race conditions, but it's a bit\nmore tricky than that. If we just add elements to the map here, we could\nrace with streamProcessingComplete() as it could be called for the first\npost-processed stream before we have a chance to add the second one\nhere. streamProcessingComplete() would then incorrectly think it has\ncompleted the last stream, and proceed to complete the request.\n\nThis race could be solved by holding the lock for the whole duration of\nthe loop here, covering all calls to stream->process(), but it may be\npossible to do better. We could add entries to the map in\nprocessCaptureRequest() instead. We would need, here, to remove entries\nfrom the map if stream->process() fails, and complete the request if we\nremove the last entry (as otherwise, streamProcessingComplete() could\ncomplete all streams, then the last stream->process() call could fail,\nand nobody would complete the request).\n\nOther designs may be possible.\n\n> \t}\n> \n> +\tif (needsPostProcessing) {\n> +\t\tif (processingStatus == Camera3RequestDescriptor::Status::Error) {\n> +\t\t\tdescriptor->status_ = processingStatus;\n> +\t\t\tsendCaptureResults();\n> +\t\t}\n> +\n> +\t\treturn;\n> +\t}\n> +\n> \tdescriptor->status_ = Camera3RequestDescriptor::Status::Success;\n> \tsendCaptureResults();\n> }\n> @@ -1206,6 +1220,64 @@ void CameraDevice::sendCaptureResults()\n> \t}\n> }\n> \n> +void CameraDevice::setBufferStatus(CameraStream *cameraStream,\n> +\t\t\t\t Camera3RequestDescriptor::StreamBuffer &buffer,\n> +\t\t\t\t Camera3RequestDescriptor *request,\n> +\t\t\t\t Camera3RequestDescriptor::Status status)\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(buffer.internalBuffer);\n> +\n> +\tbuffer.status = status;\n> +\tif (status != Camera3RequestDescriptor::Status::Success)\n> +\t\tnotifyError(request->frameNumber_, buffer.stream->camera3Stream(),\n> +\t\t\t CAMERA3_MSG_ERROR_BUFFER);\n> +}\n> +\n> +void CameraDevice::streamProcessingComplete(CameraStream *cameraStream,\n> +\t\t\t\t\t Camera3RequestDescriptor *request,\n> +\t\t\t\t\t Camera3RequestDescriptor::Status status)\n> +{\n> +\tMutexLocker locker(request->streamsProcessMutex_);\n> +\tfor (auto &buffer : request->buffers_) {\n> +\t\tif (buffer.stream != cameraStream)\n> +\t\t\tcontinue;\n> +\n> +\t\tsetBufferStatus(cameraStream, buffer, request, status);\n> +\t}\n> +\n> +\tbool hasPostProcessingErrors = false;\n> +\tfor (auto &buffer : request->buffers_) {\n> +\t\tif (cameraStream->type() == CameraStream::Type::Direct)\n> +\t\t\tcontinue;\n> +\n> +\t\t/*\n> +\t\t * Other eligible buffers might be waiting to get post-processed.\n> +\t\t * So wait for their turn before sendCaptureResults() for the\n> +\t\t * descriptor.\n> +\t\t */\n> +\t\tif (buffer.status == Camera3RequestDescriptor::Status::Pending)\n> +\t\t\treturn;\n> +\n> +\t\tif (!hasPostProcessingErrors &&\n> +\t\t buffer.status == Camera3RequestDescriptor::Status::Error)\n> +\t\t\thasPostProcessingErrors = true;\n> +\t}\n> +\n> +\tif (hasPostProcessingErrors)\n> +\t\trequest->status_ = Camera3RequestDescriptor::Status::Error;\n> +\telse\n> +\t\trequest->status_ = Camera3RequestDescriptor::Status::Success;\n> +\n> +\tlocker.unlock();\n> +\n> +\tsendCaptureResults();\n> +}\n> +\n> std::string CameraDevice::logPrefix() const\n> {\n> \treturn \"'\" + camera_->id() + \"'\";\n> diff --git a/src/android/camera_device.h b/src/android/camera_device.h\n> index 863cf414..1ef933da 100644\n> --- a/src/android/camera_device.h\n> +++ b/src/android/camera_device.h\n> @@ -66,6 +66,9 @@ public:\n> \tint configureStreams(camera3_stream_configuration_t *stream_list);\n> \tint processCaptureRequest(camera3_capture_request_t *request);\n> \tvoid requestComplete(libcamera::Request *request);\n> +\tvoid streamProcessingComplete(CameraStream *cameraStream,\n> +\t\t\t\t Camera3RequestDescriptor *request,\n> +\t\t\t\t Camera3RequestDescriptor::Status status);\n> \n> protected:\n> \tstd::string logPrefix() const override;\n> @@ -94,6 +97,10 @@ private:\n> \t\t\t camera3_error_msg_code code) const;\n> \tint processControls(Camera3RequestDescriptor *descriptor);\n> \tvoid sendCaptureResults();\n> +\tvoid setBufferStatus(CameraStream *cameraStream,\n> +\t\t\t Camera3RequestDescriptor::StreamBuffer &buffer,\n> +\t\t\t Camera3RequestDescriptor *request,\n> +\t\t\t Camera3RequestDescriptor::Status status);\n> \tstd::unique_ptr getResultMetadata(\n> \t\tconst Camera3RequestDescriptor &descriptor) const;\n> \n> diff --git a/src/android/camera_request.h b/src/android/camera_request.h\n> index 05dabf89..3a2774e0 100644\n> --- a/src/android/camera_request.h\n> +++ b/src/android/camera_request.h\n> @@ -8,6 +8,7 @@\n> #define __ANDROID_CAMERA_REQUEST_H__\n> \n> #include \n> +#include \n> #include \n> \n> #include \n> @@ -37,6 +38,7 @@ public:\n> \t\tstd::unique_ptr frameBuffer;\n> \t\tint fence;\n> \t\tStatus status;\n> +\t\tlibcamera::FrameBuffer *internalBuffer = nullptr;\n> \t};\n> \n> \tCamera3RequestDescriptor(libcamera::Camera *camera,\n> @@ -47,6 +49,8 @@ public:\n> \n> \tuint32_t frameNumber_ = 0;\n> \n> +\t/* Protects buffers_ for post-processing streams. */\n> +\tstd::mutex streamsProcessMutex_;\n> \tstd::vector buffers_;\n> \n> \tCameraMetadata settings_;\n> diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp\n> index f44a2717..04cbef8c 100644\n> --- a/src/android/camera_stream.cpp\n> +++ b/src/android/camera_stream.cpp\n> @@ -22,6 +22,7 @@\n> #include \"camera_capabilities.h\"\n> #include \"camera_device.h\"\n> #include \"camera_metadata.h\"\n> +#include \"post_processor.h\"\n> \n> using namespace libcamera;\n> \n> @@ -97,6 +98,18 @@ int CameraStream::configure()\n> \t\tint ret = postProcessor_->configure(configuration(), output);\n> \t\tif (ret)\n> \t\t\treturn ret;\n> +\n> +\t\tpostProcessor_->processComplete.connect(\n> +\t\t\tthis, [&](Camera3RequestDescriptor *request, PostProcessor::Status status) {\n> +\t\t\t\tCamera3RequestDescriptor::Status bufferStatus =\n> +\t\t\t\t\tCamera3RequestDescriptor::Status::Error;\n> +\n> +\t\t\t\tif (status == PostProcessor::Status::Success)\n> +\t\t\t\t\tbufferStatus = Camera3RequestDescriptor::Status::Success;\n\nIt looks weird to make Error a default (and thus special) case. Would\nany of the following be better ?\n\n\t\t\t\tCamera3RequestDescriptor::Status bufferStatus;\n\n\t\t\t\tif (status == PostProcessor::Status::Success)\n\t\t\t\t\tbufferStatus = Camera3RequestDescriptor::Status::Success;\n\t\t\t\telse\n\t\t\t\t\tbufferStatus = Camera3RequestDescriptor::Status::Error;\n\n\n\t\t\t\tCamera3RequestDescriptor::Status bufferStatus =\n\t\t\t\t\tstatus == PostProcessor::Status::Success ?\n\t\t\t\t\tCamera3RequestDescriptor::Status::Success :\n\t\t\t\t\tCamera3RequestDescriptor::Status::Error;\n\nI think I like the second option best.\n\n> +\n> +\t\t\t\tcameraDevice_->streamProcessingComplete(this, request,\n> +\t\t\t\t\t\t\t\t\tbufferStatus);\n> +\t\t\t});\n> \t}\n> \n> \tif (type_ == Type::Internal) {\n> diff --git a/src/android/jpeg/post_processor_jpeg.cpp b/src/android/jpeg/post_processor_jpeg.cpp\n> index 699576ef..a001fede 100644\n> --- a/src/android/jpeg/post_processor_jpeg.cpp\n> +++ b/src/android/jpeg/post_processor_jpeg.cpp\n> @@ -198,6 +198,7 @@ int PostProcessorJpeg::process(const FrameBuffer &source,\n> \t\t\t\t\t exif.data(), quality);\n> \tif (jpeg_size < 0) {\n> \t\tLOG(JPEG, Error) << \"Failed to encode stream image\";\n> +\t\tprocessComplete.emit(request, PostProcessor::Status::Error);\n\nI think you can write Status::Error here and below, including in\npost_processor_yuv.cpp (not above in camera_stream.cpp though).\n\n> \t\treturn jpeg_size;\n> \t}\n> \n> @@ -211,6 +212,7 @@ int PostProcessorJpeg::process(const FrameBuffer &source,\n> \n> \t/* Update the JPEG result Metadata. */\n> \tresultMetadata->addEntry(ANDROID_JPEG_SIZE, jpeg_size);\n> +\tprocessComplete.emit(request, PostProcessor::Status::Success);\n> \n> \treturn 0;\n> }\n> diff --git a/src/android/post_processor.h b/src/android/post_processor.h\n> index 27eaef88..14f5e8c7 100644\n> --- a/src/android/post_processor.h\n> +++ b/src/android/post_processor.h\n> @@ -7,6 +7,8 @@\n> #ifndef __ANDROID_POST_PROCESSOR_H__\n> #define __ANDROID_POST_PROCESSOR_H__\n> \n> +#include \n> +\n> #include \n> #include \n> \n> @@ -17,6 +19,11 @@ class Camera3RequestDescriptor;\n> class PostProcessor\n> {\n> public:\n> +\tenum class Status {\n> +\t\tError,\n> +\t\tSuccess\n> +\t};\n> +\n> \tvirtual ~PostProcessor() = default;\n> \n> \tvirtual int configure(const libcamera::StreamConfiguration &inCfg,\n> @@ -24,6 +31,8 @@ public:\n> \tvirtual int process(const libcamera::FrameBuffer &source,\n> \t\t\t CameraBuffer *destination,\n> \t\t\t Camera3RequestDescriptor *request) = 0;\n> +\n> +\tlibcamera::Signal processComplete;\n> };\n> \n> #endif /* __ANDROID_POST_PROCESSOR_H__ */\n> diff --git a/src/android/yuv/post_processor_yuv.cpp b/src/android/yuv/post_processor_yuv.cpp\n> index 8110a1f1..fd364741 100644\n> --- a/src/android/yuv/post_processor_yuv.cpp\n> +++ b/src/android/yuv/post_processor_yuv.cpp\n> @@ -51,14 +51,17 @@ int PostProcessorYuv::configure(const StreamConfiguration &inCfg,\n> \n> int PostProcessorYuv::process(const FrameBuffer &source,\n> \t\t\t CameraBuffer *destination,\n> -\t\t\t [[maybe_unused]] Camera3RequestDescriptor *request)\n> +\t\t\t Camera3RequestDescriptor *request)\n> {\n> -\tif (!isValidBuffers(source, *destination))\n> +\tif (!isValidBuffers(source, *destination)) {\n> +\t\tprocessComplete.emit(request, PostProcessor::Status::Error);\n> \t\treturn -EINVAL;\n> +\t}\n> \n> \tconst MappedFrameBuffer sourceMapped(&source, MappedFrameBuffer::MapFlag::Read);\n> \tif (!sourceMapped.isValid()) {\n> \t\tLOG(YUV, Error) << \"Failed to mmap camera frame buffer\";\n> +\t\tprocessComplete.emit(request, PostProcessor::Status::Error);\n> \t\treturn -EINVAL;\n> \t}\n> \n> @@ -76,9 +79,12 @@ int PostProcessorYuv::process(const FrameBuffer &source,\n> \t\t\t\t libyuv::FilterMode::kFilterBilinear);\n> \tif (ret) {\n> \t\tLOG(YUV, Error) << \"Failed NV12 scaling: \" << ret;\n> +\t\tprocessComplete.emit(request, PostProcessor::Status::Error);\n> \t\treturn -EINVAL;\n> \t}\n> \n> +\tprocessComplete.emit(request, PostProcessor::Status::Success);\n> +\n> \treturn 0;\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 AC4C2BF415\n\tfor ;\n\tThu, 21 Oct 2021 00:31:48 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 170EE68F59;\n\tThu, 21 Oct 2021 02:31:48 +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 DC9F6604FE\n\tfor ;\n\tThu, 21 Oct 2021 02:31:45 +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 4F23119FD;\n\tThu, 21 Oct 2021 02:31:45 +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=\"H4xdcbcU\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1634776305;\n\tbh=xl4GFTrCiCFrdH8xoXbU9yc7RZ28vbNnpwns4i3WxUI=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=H4xdcbcU6SWmwA74+m072AwyjC4dukj1Xa/ce8aRhVOQA/GUYZ/66bwPGuEPzScoG\n\t1wouwq5NIVe+daPfAlAEFWaTHuwS8qiTdf4jjSLH6TBCdXlwiviPEaMBAHWPrqHB2x\n\tGbmVPQxnv9UBX2nQk9qFwdCYszu0VWh4qN5earcE=","Date":"Thu, 21 Oct 2021 03:31:26 +0300","From":"Laurent Pinchart ","To":"Umang Jain ","Message-ID":"","References":"<20211020104212.121743-1-umang.jain@ideasonboard.com>\n\t<20211020104212.121743-2-umang.jain@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20211020104212.121743-2-umang.jain@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v5 1/4] android: Notify post\n\tprocessing completion via a signal","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":20337,"web_url":"https://patchwork.libcamera.org/comment/20337/","msgid":"","date":"2021-10-21T00:57:54","subject":"Re: [libcamera-devel] [PATCH v5 1/4] android: Notify post\n\tprocessing completion via a signal","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Another comment.\nOn Thu, Oct 21, 2021 at 03:31:27AM +0300, Laurent Pinchart wrote:\n> Hi Umang,\n> \n> Thank you for the patch.\n> \n> On Wed, Oct 20, 2021 at 04:12:09PM +0530, Umang Jain wrote:\n> > Notify that the post processing for a request has been completed,\n> > via a signal. A pointer to the descriptor which is tracking the\n> > capture request is emitted along with the status of post processed\n> > buffer. The function CameraDevice::streamProcessingComplete() will\n> > finally set the status on the request descriptor and send capture\n> > results back to the framework accordingly.\n> > \n> > We also need to save a pointer to any internal buffers that might have\n> > been allocated by CameraStream. The buffer should be returned back to\n> > CameraStream just before capture results are sent.\n> > \n> > A streamProcessMutex_ has been introduced here itself, which will be\n> \n> Did you mean s/itself/as well/ ?\n> \n> > applicable to guard access to descriptor->buffers_ when post-processing\n> > is moved to be asynchronous in subsequent commits.\n> > \n> > Signed-off-by: Umang Jain \n> > ---\n> > src/android/camera_device.cpp | 92 +++++++++++++++++++++---\n> > src/android/camera_device.h | 7 ++\n> > src/android/camera_request.h | 4 ++\n> > src/android/camera_stream.cpp | 13 ++++\n> > src/android/jpeg/post_processor_jpeg.cpp | 2 +\n> > src/android/post_processor.h | 9 +++\n> > src/android/yuv/post_processor_yuv.cpp | 10 ++-\n> > 7 files changed, 125 insertions(+), 12 deletions(-)\n> > \n> > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> > index 806b4090..541c2c81 100644\n> > --- a/src/android/camera_device.cpp\n> > +++ b/src/android/camera_device.cpp\n> > @@ -1117,6 +1117,15 @@ void CameraDevice::requestComplete(Request *request)\n> > \t}\n> > \n> > \t/* Handle post-processing. */\n> > +\tbool needsPostProcessing = false;\n> > +\tCamera3RequestDescriptor::Status processingStatus =\n> > +\t\tCamera3RequestDescriptor::Status::Pending;\n> > +\t/*\n> > +\t * \\todo Apply streamsProcessMutex_ when post-processing is adapted to run\n> > +\t * asynchronously. If we introduce the streamsProcessMutex_ right away, the\n> > +\t * lock will be held forever since it is synchronous at this point\n> > +\t * (see streamProcessingComplete).\n> > +\t */\n> > \tfor (auto &buffer : descriptor->buffers_) {\n> > \t\tCameraStream *stream = buffer.stream;\n> > \n> > @@ -1132,22 +1141,27 @@ void CameraDevice::requestComplete(Request *request)\n> > \t\t\tcontinue;\n> > \t\t}\n> > \n> > -\t\tint ret = stream->process(*src, 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 (stream->type() == CameraStream::Type::Internal)\n> > -\t\t\tstream->putBuffer(src);\n> > +\t\t\tbuffer.internalBuffer = src;\n> \n> Let's make the field name more self-explanatory. A possible candidate is\n> sourceBuffer instead of internalBuffer.\n\nI saw you've added an srcBuffer member in 3/4 to store the source buffer\nfor mapped streams as well. A single pointer should be enough, as you\ncheck the stream type in setBufferStatus() before calling putBuffer().\nHowever, we can't easily set this pointer in processCaptureRequest() for\nmapped streams, and setting it in different locations depending on the\nstream type isn't nice.\n\nI'm tempted to have two pointers, internalBuffer and sourceBuffer, the\nformer set in processCaptureRequest() for internal streams only, and the\nlatter set here for both internal and mapped streams. You could then\nremove the stream type check in setBufferStatus()() and replace it with\na 'if (buffer.internalBuffer)'.\n\n> Could we do this in CameraDevice::processCaptureRequest(), just after\n> calling cameraStream->getBuffer() ? I'll feel less worried about a leak\n> if we store the buffer for later release right after acquiring it.\n> \n> > \n> > +\t\tneedsPostProcessing = true;\n> > +\t\tint ret = stream->process(*src, buffer, descriptor);\n> > \t\tif (ret) {\n> > -\t\t\tbuffer.status = Camera3RequestDescriptor::Status::Error;\n> > -\t\t\tnotifyError(descriptor->frameNumber_, stream->camera3Stream(),\n> > -\t\t\t\t CAMERA3_MSG_ERROR_BUFFER);\n> > +\t\t\tsetBufferStatus(stream, buffer, descriptor,\n> > +\t\t\t\t\tCamera3RequestDescriptor::Status::Error);\n> > +\t\t\tprocessingStatus = Camera3RequestDescriptor::Status::Error;\n> > \t\t}\n> \n> I think we need to improve error handling a little bit, in order to\n> support multiple streams being post-processed. We need to complete the\n> request in CameraDevice::streamProcessingComplete() when the last\n> processing stream completes, so the number of post-processed streams\n> need to be somehow recorded in the Camera3RequestDescriptor. It could be\n> tempted to have a counter, but a\n> \n> \tstd::map pendingPostProcess_;\n> \n> could be better, as it will also remove the need for loops in\n> streamProcessingComplete() to lookup the buffer from the stream and to\n> check if the request has been fully processed. Accessing the map has to\n> be done with the mutex held to avoid race conditions, but it's a bit\n> more tricky than that. If we just add elements to the map here, we could\n> race with streamProcessingComplete() as it could be called for the first\n> post-processed stream before we have a chance to add the second one\n> here. streamProcessingComplete() would then incorrectly think it has\n> completed the last stream, and proceed to complete the request.\n> \n> This race could be solved by holding the lock for the whole duration of\n> the loop here, covering all calls to stream->process(), but it may be\n> possible to do better. We could add entries to the map in\n> processCaptureRequest() instead. We would need, here, to remove entries\n> from the map if stream->process() fails, and complete the request if we\n> remove the last entry (as otherwise, streamProcessingComplete() could\n> complete all streams, then the last stream->process() call could fail,\n> and nobody would complete the request).\n> \n> Other designs may be possible.\n> \n> > \t}\n> > \n> > +\tif (needsPostProcessing) {\n> > +\t\tif (processingStatus == Camera3RequestDescriptor::Status::Error) {\n> > +\t\t\tdescriptor->status_ = processingStatus;\n> > +\t\t\tsendCaptureResults();\n> > +\t\t}\n> > +\n> > +\t\treturn;\n> > +\t}\n> > +\n> > \tdescriptor->status_ = Camera3RequestDescriptor::Status::Success;\n> > \tsendCaptureResults();\n> > }\n> > @@ -1206,6 +1220,64 @@ void CameraDevice::sendCaptureResults()\n> > \t}\n> > }\n> > \n> > +void CameraDevice::setBufferStatus(CameraStream *cameraStream,\n> > +\t\t\t\t Camera3RequestDescriptor::StreamBuffer &buffer,\n> > +\t\t\t\t Camera3RequestDescriptor *request,\n> > +\t\t\t\t Camera3RequestDescriptor::Status status)\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(buffer.internalBuffer);\n> > +\n> > +\tbuffer.status = status;\n> > +\tif (status != Camera3RequestDescriptor::Status::Success)\n> > +\t\tnotifyError(request->frameNumber_, buffer.stream->camera3Stream(),\n> > +\t\t\t CAMERA3_MSG_ERROR_BUFFER);\n> > +}\n> > +\n> > +void CameraDevice::streamProcessingComplete(CameraStream *cameraStream,\n> > +\t\t\t\t\t Camera3RequestDescriptor *request,\n> > +\t\t\t\t\t Camera3RequestDescriptor::Status status)\n> > +{\n> > +\tMutexLocker locker(request->streamsProcessMutex_);\n> > +\tfor (auto &buffer : request->buffers_) {\n> > +\t\tif (buffer.stream != cameraStream)\n> > +\t\t\tcontinue;\n> > +\n> > +\t\tsetBufferStatus(cameraStream, buffer, request, status);\n> > +\t}\n> > +\n> > +\tbool hasPostProcessingErrors = false;\n> > +\tfor (auto &buffer : request->buffers_) {\n> > +\t\tif (cameraStream->type() == CameraStream::Type::Direct)\n> > +\t\t\tcontinue;\n> > +\n> > +\t\t/*\n> > +\t\t * Other eligible buffers might be waiting to get post-processed.\n> > +\t\t * So wait for their turn before sendCaptureResults() for the\n> > +\t\t * descriptor.\n> > +\t\t */\n> > +\t\tif (buffer.status == Camera3RequestDescriptor::Status::Pending)\n> > +\t\t\treturn;\n> > +\n> > +\t\tif (!hasPostProcessingErrors &&\n> > +\t\t buffer.status == Camera3RequestDescriptor::Status::Error)\n> > +\t\t\thasPostProcessingErrors = true;\n> > +\t}\n> > +\n> > +\tif (hasPostProcessingErrors)\n> > +\t\trequest->status_ = Camera3RequestDescriptor::Status::Error;\n> > +\telse\n> > +\t\trequest->status_ = Camera3RequestDescriptor::Status::Success;\n> > +\n> > +\tlocker.unlock();\n> > +\n> > +\tsendCaptureResults();\n> > +}\n> > +\n> > std::string CameraDevice::logPrefix() const\n> > {\n> > \treturn \"'\" + camera_->id() + \"'\";\n> > diff --git a/src/android/camera_device.h b/src/android/camera_device.h\n> > index 863cf414..1ef933da 100644\n> > --- a/src/android/camera_device.h\n> > +++ b/src/android/camera_device.h\n> > @@ -66,6 +66,9 @@ public:\n> > \tint configureStreams(camera3_stream_configuration_t *stream_list);\n> > \tint processCaptureRequest(camera3_capture_request_t *request);\n> > \tvoid requestComplete(libcamera::Request *request);\n> > +\tvoid streamProcessingComplete(CameraStream *cameraStream,\n> > +\t\t\t\t Camera3RequestDescriptor *request,\n> > +\t\t\t\t Camera3RequestDescriptor::Status status);\n> > \n> > protected:\n> > \tstd::string logPrefix() const override;\n> > @@ -94,6 +97,10 @@ private:\n> > \t\t\t camera3_error_msg_code code) const;\n> > \tint processControls(Camera3RequestDescriptor *descriptor);\n> > \tvoid sendCaptureResults();\n> > +\tvoid setBufferStatus(CameraStream *cameraStream,\n> > +\t\t\t Camera3RequestDescriptor::StreamBuffer &buffer,\n> > +\t\t\t Camera3RequestDescriptor *request,\n> > +\t\t\t Camera3RequestDescriptor::Status status);\n> > \tstd::unique_ptr getResultMetadata(\n> > \t\tconst Camera3RequestDescriptor &descriptor) const;\n> > \n> > diff --git a/src/android/camera_request.h b/src/android/camera_request.h\n> > index 05dabf89..3a2774e0 100644\n> > --- a/src/android/camera_request.h\n> > +++ b/src/android/camera_request.h\n> > @@ -8,6 +8,7 @@\n> > #define __ANDROID_CAMERA_REQUEST_H__\n> > \n> > #include \n> > +#include \n> > #include \n> > \n> > #include \n> > @@ -37,6 +38,7 @@ public:\n> > \t\tstd::unique_ptr frameBuffer;\n> > \t\tint fence;\n> > \t\tStatus status;\n> > +\t\tlibcamera::FrameBuffer *internalBuffer = nullptr;\n> > \t};\n> > \n> > \tCamera3RequestDescriptor(libcamera::Camera *camera,\n> > @@ -47,6 +49,8 @@ public:\n> > \n> > \tuint32_t frameNumber_ = 0;\n> > \n> > +\t/* Protects buffers_ for post-processing streams. */\n> > +\tstd::mutex streamsProcessMutex_;\n> > \tstd::vector buffers_;\n> > \n> > \tCameraMetadata settings_;\n> > diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp\n> > index f44a2717..04cbef8c 100644\n> > --- a/src/android/camera_stream.cpp\n> > +++ b/src/android/camera_stream.cpp\n> > @@ -22,6 +22,7 @@\n> > #include \"camera_capabilities.h\"\n> > #include \"camera_device.h\"\n> > #include \"camera_metadata.h\"\n> > +#include \"post_processor.h\"\n> > \n> > using namespace libcamera;\n> > \n> > @@ -97,6 +98,18 @@ int CameraStream::configure()\n> > \t\tint ret = postProcessor_->configure(configuration(), output);\n> > \t\tif (ret)\n> > \t\t\treturn ret;\n> > +\n> > +\t\tpostProcessor_->processComplete.connect(\n> > +\t\t\tthis, [&](Camera3RequestDescriptor *request, PostProcessor::Status status) {\n> > +\t\t\t\tCamera3RequestDescriptor::Status bufferStatus =\n> > +\t\t\t\t\tCamera3RequestDescriptor::Status::Error;\n> > +\n> > +\t\t\t\tif (status == PostProcessor::Status::Success)\n> > +\t\t\t\t\tbufferStatus = Camera3RequestDescriptor::Status::Success;\n> \n> It looks weird to make Error a default (and thus special) case. Would\n> any of the following be better ?\n> \n> \t\t\t\tCamera3RequestDescriptor::Status bufferStatus;\n> \n> \t\t\t\tif (status == PostProcessor::Status::Success)\n> \t\t\t\t\tbufferStatus = Camera3RequestDescriptor::Status::Success;\n> \t\t\t\telse\n> \t\t\t\t\tbufferStatus = Camera3RequestDescriptor::Status::Error;\n> \n> \n> \t\t\t\tCamera3RequestDescriptor::Status bufferStatus =\n> \t\t\t\t\tstatus == PostProcessor::Status::Success ?\n> \t\t\t\t\tCamera3RequestDescriptor::Status::Success :\n> \t\t\t\t\tCamera3RequestDescriptor::Status::Error;\n> \n> I think I like the second option best.\n> \n> > +\n> > +\t\t\t\tcameraDevice_->streamProcessingComplete(this, request,\n> > +\t\t\t\t\t\t\t\t\tbufferStatus);\n> > +\t\t\t});\n> > \t}\n> > \n> > \tif (type_ == Type::Internal) {\n> > diff --git a/src/android/jpeg/post_processor_jpeg.cpp b/src/android/jpeg/post_processor_jpeg.cpp\n> > index 699576ef..a001fede 100644\n> > --- a/src/android/jpeg/post_processor_jpeg.cpp\n> > +++ b/src/android/jpeg/post_processor_jpeg.cpp\n> > @@ -198,6 +198,7 @@ int PostProcessorJpeg::process(const FrameBuffer &source,\n> > \t\t\t\t\t exif.data(), quality);\n> > \tif (jpeg_size < 0) {\n> > \t\tLOG(JPEG, Error) << \"Failed to encode stream image\";\n> > +\t\tprocessComplete.emit(request, PostProcessor::Status::Error);\n> \n> I think you can write Status::Error here and below, including in\n> post_processor_yuv.cpp (not above in camera_stream.cpp though).\n> \n> > \t\treturn jpeg_size;\n> > \t}\n> > \n> > @@ -211,6 +212,7 @@ int PostProcessorJpeg::process(const FrameBuffer &source,\n> > \n> > \t/* Update the JPEG result Metadata. */\n> > \tresultMetadata->addEntry(ANDROID_JPEG_SIZE, jpeg_size);\n> > +\tprocessComplete.emit(request, PostProcessor::Status::Success);\n> > \n> > \treturn 0;\n> > }\n> > diff --git a/src/android/post_processor.h b/src/android/post_processor.h\n> > index 27eaef88..14f5e8c7 100644\n> > --- a/src/android/post_processor.h\n> > +++ b/src/android/post_processor.h\n> > @@ -7,6 +7,8 @@\n> > #ifndef __ANDROID_POST_PROCESSOR_H__\n> > #define __ANDROID_POST_PROCESSOR_H__\n> > \n> > +#include \n> > +\n> > #include \n> > #include \n> > \n> > @@ -17,6 +19,11 @@ class Camera3RequestDescriptor;\n> > class PostProcessor\n> > {\n> > public:\n> > +\tenum class Status {\n> > +\t\tError,\n> > +\t\tSuccess\n> > +\t};\n> > +\n> > \tvirtual ~PostProcessor() = default;\n> > \n> > \tvirtual int configure(const libcamera::StreamConfiguration &inCfg,\n> > @@ -24,6 +31,8 @@ public:\n> > \tvirtual int process(const libcamera::FrameBuffer &source,\n> > \t\t\t CameraBuffer *destination,\n> > \t\t\t Camera3RequestDescriptor *request) = 0;\n> > +\n> > +\tlibcamera::Signal processComplete;\n> > };\n> > \n> > #endif /* __ANDROID_POST_PROCESSOR_H__ */\n> > diff --git a/src/android/yuv/post_processor_yuv.cpp b/src/android/yuv/post_processor_yuv.cpp\n> > index 8110a1f1..fd364741 100644\n> > --- a/src/android/yuv/post_processor_yuv.cpp\n> > +++ b/src/android/yuv/post_processor_yuv.cpp\n> > @@ -51,14 +51,17 @@ int PostProcessorYuv::configure(const StreamConfiguration &inCfg,\n> > \n> > int PostProcessorYuv::process(const FrameBuffer &source,\n> > \t\t\t CameraBuffer *destination,\n> > -\t\t\t [[maybe_unused]] Camera3RequestDescriptor *request)\n> > +\t\t\t Camera3RequestDescriptor *request)\n> > {\n> > -\tif (!isValidBuffers(source, *destination))\n> > +\tif (!isValidBuffers(source, *destination)) {\n> > +\t\tprocessComplete.emit(request, PostProcessor::Status::Error);\n> > \t\treturn -EINVAL;\n> > +\t}\n> > \n> > \tconst MappedFrameBuffer sourceMapped(&source, MappedFrameBuffer::MapFlag::Read);\n> > \tif (!sourceMapped.isValid()) {\n> > \t\tLOG(YUV, Error) << \"Failed to mmap camera frame buffer\";\n> > +\t\tprocessComplete.emit(request, PostProcessor::Status::Error);\n> > \t\treturn -EINVAL;\n> > \t}\n> > \n> > @@ -76,9 +79,12 @@ int PostProcessorYuv::process(const FrameBuffer &source,\n> > \t\t\t\t libyuv::FilterMode::kFilterBilinear);\n> > \tif (ret) {\n> > \t\tLOG(YUV, Error) << \"Failed NV12 scaling: \" << ret;\n> > +\t\tprocessComplete.emit(request, PostProcessor::Status::Error);\n> > \t\treturn -EINVAL;\n> > \t}\n> > \n> > +\tprocessComplete.emit(request, PostProcessor::Status::Success);\n> > +\n> > \treturn 0;\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 5A973BF415\n\tfor ;\n\tThu, 21 Oct 2021 00:58:16 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9FE7768F5D;\n\tThu, 21 Oct 2021 02:58:15 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 9F11468F57\n\tfor ;\n\tThu, 21 Oct 2021 02:58: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 0E3572BA;\n\tThu, 21 Oct 2021 02:58:12 +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=\"Oj/M2iHx\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1634777893;\n\tbh=yaDL+MgqYFLWjrHOubPHxFqRwo7K5EdVtLhQ+4XMlTk=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=Oj/M2iHxSrh33F1arNdRQ6yNqfqoF3rdiPo49RGfD4V98KlHoC9TN7BMxRjIY9Pv0\n\twZA1FVmX0ITN4iwQywaYyMlFz2lUv2EIOOhK8zC+B4FPFKqRia7dXvsC34YcVI3vjZ\n\t5Bu+1M1Lnu+/l92IILlMjLfl65wljJZUb4grXNMk=","Date":"Thu, 21 Oct 2021 03:57:54 +0300","From":"Laurent Pinchart ","To":"Umang Jain ","Message-ID":"","References":"<20211020104212.121743-1-umang.jain@ideasonboard.com>\n\t<20211020104212.121743-2-umang.jain@ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v5 1/4] android: Notify post\n\tprocessing completion via a signal","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":20351,"web_url":"https://patchwork.libcamera.org/comment/20351/","msgid":"","date":"2021-10-21T05:35:44","subject":"Re: [libcamera-devel] [PATCH v5 1/4] android: Notify post\n\tprocessing completion via a signal","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hi Laurent,\n\nOn 10/21/21 6:01 AM, Laurent Pinchart wrote:\n> Hi Umang,\n>\n> Thank you for the patch.\n>\n> On Wed, Oct 20, 2021 at 04:12:09PM +0530, Umang Jain wrote:\n>> Notify that the post processing for a request has been completed,\n>> via a signal. A pointer to the descriptor which is tracking the\n>> capture request is emitted along with the status of post processed\n>> buffer. The function CameraDevice::streamProcessingComplete() will\n>> finally set the status on the request descriptor and send capture\n>> results back to the framework accordingly.\n>>\n>> We also need to save a pointer to any internal buffers that might have\n>> been allocated by CameraStream. The buffer should be returned back to\n>> CameraStream just before capture results are sent.\n>>\n>> A streamProcessMutex_ has been introduced here itself, which will be\n> Did you mean s/itself/as well/ ?\n>\n>> applicable to guard access to descriptor->buffers_ when post-processing\n>> is moved to be asynchronous in subsequent commits.\n>>\n>> Signed-off-by: Umang Jain \n>> ---\n>> src/android/camera_device.cpp | 92 +++++++++++++++++++++---\n>> src/android/camera_device.h | 7 ++\n>> src/android/camera_request.h | 4 ++\n>> src/android/camera_stream.cpp | 13 ++++\n>> src/android/jpeg/post_processor_jpeg.cpp | 2 +\n>> src/android/post_processor.h | 9 +++\n>> src/android/yuv/post_processor_yuv.cpp | 10 ++-\n>> 7 files changed, 125 insertions(+), 12 deletions(-)\n>>\n>> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n>> index 806b4090..541c2c81 100644\n>> --- a/src/android/camera_device.cpp\n>> +++ b/src/android/camera_device.cpp\n>> @@ -1117,6 +1117,15 @@ void CameraDevice::requestComplete(Request *request)\n>> \t}\n>> \n>> \t/* Handle post-processing. */\n>> +\tbool needsPostProcessing = false;\n>> +\tCamera3RequestDescriptor::Status processingStatus =\n>> +\t\tCamera3RequestDescriptor::Status::Pending;\n>> +\t/*\n>> +\t * \\todo Apply streamsProcessMutex_ when post-processing is adapted to run\n>> +\t * asynchronously. If we introduce the streamsProcessMutex_ right away, the\n>> +\t * lock will be held forever since it is synchronous at this point\n>> +\t * (see streamProcessingComplete).\n>> +\t */\n>> \tfor (auto &buffer : descriptor->buffers_) {\n>> \t\tCameraStream *stream = buffer.stream;\n>> \n>> @@ -1132,22 +1141,27 @@ void CameraDevice::requestComplete(Request *request)\n>> \t\t\tcontinue;\n>> \t\t}\n>> \n>> -\t\tint ret = stream->process(*src, 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 (stream->type() == CameraStream::Type::Internal)\n>> -\t\t\tstream->putBuffer(src);\n>> +\t\t\tbuffer.internalBuffer = src;\n> Let's make the field name more self-explanatory. A possible candidate is\n> sourceBuffer instead of internalBuffer.\n>\n> Could we do this in CameraDevice::processCaptureRequest(), just after\n> calling cameraStream->getBuffer() ? I'll feel less worried about a leak\n> if we store the buffer for later release right after acquiring it.\n\n\nAck.\n\n>\n>> \n>> +\t\tneedsPostProcessing = true;\n>> +\t\tint ret = stream->process(*src, buffer, descriptor);\n>> \t\tif (ret) {\n>> -\t\t\tbuffer.status = Camera3RequestDescriptor::Status::Error;\n>> -\t\t\tnotifyError(descriptor->frameNumber_, stream->camera3Stream(),\n>> -\t\t\t\t CAMERA3_MSG_ERROR_BUFFER);\n>> +\t\t\tsetBufferStatus(stream, buffer, descriptor,\n>> +\t\t\t\t\tCamera3RequestDescriptor::Status::Error);\n>> +\t\t\tprocessingStatus = Camera3RequestDescriptor::Status::Error;\n>> \t\t}\n> I think we need to improve error handling a little bit, in order to\n> support multiple streams being post-processed. We need to complete the\n> request in CameraDevice::streamProcessingComplete() when the last\n> processing stream completes, so the number of post-processed streams\n> need to be somehow recorded in the Camera3RequestDescriptor. It could be\n> tempted to have a counter, but a\n\n\nI have implemented a counter based solution, it still didn't address all \nthe corner cases. Since we have two  path of error handling (synchronous \nerror and asynchronous error), a counter based implementation was \nprobably too verbose with much of code duplication to handle various \nscenarios.\n\n\n>\n> \tstd::map pendingPostProcess_;\n>\n> could be better, as it will also remove the need for loops in\n> streamProcessingComplete() to lookup the buffer from the stream and to\n> check if the request has been fully processed. Accessing the map has to\n> be done with the mutex held to avoid race conditions, but it's a bit\n> more tricky than that. If we just add elements to the map here, we could\n\n\nGood, so you do understand so many looping lookups in \nstreamProcessingComplete.\n\nI broadly feel okay with std::map \npendingPostProcess_; as long as it resides inside the descriptor. For \nme, it avoids lookups, and can address the \\todo in 3/4.\n\nHaving said that, I feel a bit wary of introducing more data structures \nfor management. We already have a queue of post-processing (convert to \nstd:deque if we want to access the queued requests for post-processing) \nand now we will have another map to tracking the requests. I wonder if \nwe can get away with a PostProcessorWorker's std::deque, with the \nbenefits of the map. But again, we have to take care of exposure / mutex \nwith main thread, which can lead to subtle bugs and unwanted waiting on \neach thread\n\nNow I think about it, the pendingPostProcess_ map is about Stream's \npending processing _per descriptor_, whereas the PostProcessorWorker's \nqueue is replacement for Thread's message queue. Hmm.\n\n> race with streamProcessingComplete() as it could be called for the first\n> post-processed stream before we have a chance to add the second one\n> here. streamProcessingComplete() would then incorrectly think it has\n> completed the last stream, and proceed to complete the request.\n>\n> This race could be solved by holding the lock for the whole duration of\n> the loop here, covering all calls to stream->process(), but it may be\n\n\nYes,  I am considering this in current implementation too. The way it \nwill look like, is first queue up all the post-processing requests to \nPostProcessorWoe, only then, allow any of the \nslot(streamProcessingComplete) to get executed. A simple mutex hold can \ndo that\n\n> possible to do better. We could add entries to the map in\n> processCaptureRequest() instead. We would need, here, to remove entries\n\n\nI am not so sure about this, We could add entries in \nprocessCaptureRequest() but it feels too early. Let me try to address \nother comments, and I can iterate on this on top.\n\n> from the map if stream->process() fails, and complete the request if we\n> remove the last entry (as otherwise, streamProcessingComplete() could\n> complete all streams, then the last stream->process() call could fail,\n> and nobody would complete the request).\n\n\nThis is still a bit tricky to handle. What's the de-facto criteria to \nremove entries from the map (not just the error path) ?\n\nAssuming we hold the lock for entire duration of\n\n     Locker.lock();\n\n     for {\n         ...\n         int ret = stream->process(...);\n         if (ret) {\n             descriptor.pendingPostProcess_.erase(stream);\n             setBufferStatus(stream, error);\n\n             if (descriptor.pendingPostProcess_.size() == 0)\n                 completeDescriptor (error);\n\n         }\n\n     }\n     locker.unlock();\n\n....\n\nstreamProcessingComplete(..., bufferStatus)\n\n{\n\n     Locker.lock();\n\ndescriptor.pendingPostProcess_.erase(stream);\n     setBufferStatus(stream, bufferStatus);\n\n     if (descriptor.pendingPostProcess_.size() == 0)\n         completeDescriptor (?);\n\n}\n\nthe ? indicates with what status I should complete the descriptor? I \nwould probably need to iterate over all the descriptor's buffer status \nto see if any of the (previous)buffer has failed to set error status on \nthe descriptor overall? I think so, we would still need a look up loop \nhere, I can't see deciphering this from the map.\n\n\nAre we getting too complicated with this? I have started to feel so ...\n\n>\n> Other designs may be possible.\n>\n>> \t}\n>> \n>> +\tif (needsPostProcessing) {\n>> +\t\tif (processingStatus == Camera3RequestDescriptor::Status::Error) {\n>> +\t\t\tdescriptor->status_ = processingStatus;\n>> +\t\t\tsendCaptureResults();\n>> +\t\t}\n>> +\n>> +\t\treturn;\n>> +\t}\n>> +\n>> \tdescriptor->status_ = Camera3RequestDescriptor::Status::Success;\n>> \tsendCaptureResults();\n>> }\n>> @@ -1206,6 +1220,64 @@ void CameraDevice::sendCaptureResults()\n>> \t}\n>> }\n>> \n>> +void CameraDevice::setBufferStatus(CameraStream *cameraStream,\n>> +\t\t\t\t Camera3RequestDescriptor::StreamBuffer &buffer,\n>> +\t\t\t\t Camera3RequestDescriptor *request,\n>> +\t\t\t\t Camera3RequestDescriptor::Status status)\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(buffer.internalBuffer);\n>> +\n>> +\tbuffer.status = status;\n>> +\tif (status != Camera3RequestDescriptor::Status::Success)\n>> +\t\tnotifyError(request->frameNumber_, buffer.stream->camera3Stream(),\n>> +\t\t\t CAMERA3_MSG_ERROR_BUFFER);\n>> +}\n>> +\n>> +void CameraDevice::streamProcessingComplete(CameraStream *cameraStream,\n>> +\t\t\t\t\t Camera3RequestDescriptor *request,\n>> +\t\t\t\t\t Camera3RequestDescriptor::Status status)\n>> +{\n>> +\tMutexLocker locker(request->streamsProcessMutex_);\n>> +\tfor (auto &buffer : request->buffers_) {\n>> +\t\tif (buffer.stream != cameraStream)\n>> +\t\t\tcontinue;\n>> +\n>> +\t\tsetBufferStatus(cameraStream, buffer, request, status);\n>> +\t}\n>> +\n>> +\tbool hasPostProcessingErrors = false;\n>> +\tfor (auto &buffer : request->buffers_) {\n>> +\t\tif (cameraStream->type() == CameraStream::Type::Direct)\n>> +\t\t\tcontinue;\n>> +\n>> +\t\t/*\n>> +\t\t * Other eligible buffers might be waiting to get post-processed.\n>> +\t\t * So wait for their turn before sendCaptureResults() for the\n>> +\t\t * descriptor.\n>> +\t\t */\n>> +\t\tif (buffer.status == Camera3RequestDescriptor::Status::Pending)\n>> +\t\t\treturn;\n>> +\n>> +\t\tif (!hasPostProcessingErrors &&\n>> +\t\t buffer.status == Camera3RequestDescriptor::Status::Error)\n>> +\t\t\thasPostProcessingErrors = true;\n>> +\t}\n>> +\n>> +\tif (hasPostProcessingErrors)\n>> +\t\trequest->status_ = Camera3RequestDescriptor::Status::Error;\n>> +\telse\n>> +\t\trequest->status_ = Camera3RequestDescriptor::Status::Success;\n>> +\n>> +\tlocker.unlock();\n>> +\n>> +\tsendCaptureResults();\n>> +}\n>> +\n>> std::string CameraDevice::logPrefix() const\n>> {\n>> \treturn \"'\" + camera_->id() + \"'\";\n>> diff --git a/src/android/camera_device.h b/src/android/camera_device.h\n>> index 863cf414..1ef933da 100644\n>> --- a/src/android/camera_device.h\n>> +++ b/src/android/camera_device.h\n>> @@ -66,6 +66,9 @@ public:\n>> \tint configureStreams(camera3_stream_configuration_t *stream_list);\n>> \tint processCaptureRequest(camera3_capture_request_t *request);\n>> \tvoid requestComplete(libcamera::Request *request);\n>> +\tvoid streamProcessingComplete(CameraStream *cameraStream,\n>> +\t\t\t\t Camera3RequestDescriptor *request,\n>> +\t\t\t\t Camera3RequestDescriptor::Status status);\n>> \n>> protected:\n>> \tstd::string logPrefix() const override;\n>> @@ -94,6 +97,10 @@ private:\n>> \t\t\t camera3_error_msg_code code) const;\n>> \tint processControls(Camera3RequestDescriptor *descriptor);\n>> \tvoid sendCaptureResults();\n>> +\tvoid setBufferStatus(CameraStream *cameraStream,\n>> +\t\t\t Camera3RequestDescriptor::StreamBuffer &buffer,\n>> +\t\t\t Camera3RequestDescriptor *request,\n>> +\t\t\t Camera3RequestDescriptor::Status status);\n>> \tstd::unique_ptr getResultMetadata(\n>> \t\tconst Camera3RequestDescriptor &descriptor) const;\n>> \n>> diff --git a/src/android/camera_request.h b/src/android/camera_request.h\n>> index 05dabf89..3a2774e0 100644\n>> --- a/src/android/camera_request.h\n>> +++ b/src/android/camera_request.h\n>> @@ -8,6 +8,7 @@\n>> #define __ANDROID_CAMERA_REQUEST_H__\n>> \n>> #include \n>> +#include \n>> #include \n>> \n>> #include \n>> @@ -37,6 +38,7 @@ public:\n>> \t\tstd::unique_ptr frameBuffer;\n>> \t\tint fence;\n>> \t\tStatus status;\n>> +\t\tlibcamera::FrameBuffer *internalBuffer = nullptr;\n>> \t};\n>> \n>> \tCamera3RequestDescriptor(libcamera::Camera *camera,\n>> @@ -47,6 +49,8 @@ public:\n>> \n>> \tuint32_t frameNumber_ = 0;\n>> \n>> +\t/* Protects buffers_ for post-processing streams. */\n>> +\tstd::mutex streamsProcessMutex_;\n>> \tstd::vector buffers_;\n>> \n>> \tCameraMetadata settings_;\n>> diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp\n>> index f44a2717..04cbef8c 100644\n>> --- a/src/android/camera_stream.cpp\n>> +++ b/src/android/camera_stream.cpp\n>> @@ -22,6 +22,7 @@\n>> #include \"camera_capabilities.h\"\n>> #include \"camera_device.h\"\n>> #include \"camera_metadata.h\"\n>> +#include \"post_processor.h\"\n>> \n>> using namespace libcamera;\n>> \n>> @@ -97,6 +98,18 @@ int CameraStream::configure()\n>> \t\tint ret = postProcessor_->configure(configuration(), output);\n>> \t\tif (ret)\n>> \t\t\treturn ret;\n>> +\n>> +\t\tpostProcessor_->processComplete.connect(\n>> +\t\t\tthis, [&](Camera3RequestDescriptor *request, PostProcessor::Status status) {\n>> +\t\t\t\tCamera3RequestDescriptor::Status bufferStatus =\n>> +\t\t\t\t\tCamera3RequestDescriptor::Status::Error;\n>> +\n>> +\t\t\t\tif (status == PostProcessor::Status::Success)\n>> +\t\t\t\t\tbufferStatus = Camera3RequestDescriptor::Status::Success;\n> It looks weird to make Error a default (and thus special) case. Would\n> any of the following be better ?\n>\n> \t\t\t\tCamera3RequestDescriptor::Status bufferStatus;\n>\n> \t\t\t\tif (status == PostProcessor::Status::Success)\n> \t\t\t\t\tbufferStatus = Camera3RequestDescriptor::Status::Success;\n> \t\t\t\telse\n> \t\t\t\t\tbufferStatus = Camera3RequestDescriptor::Status::Error;\n>\n>\n> \t\t\t\tCamera3RequestDescriptor::Status bufferStatus =\n> \t\t\t\t\tstatus == PostProcessor::Status::Success ?\n> \t\t\t\t\tCamera3RequestDescriptor::Status::Success :\n> \t\t\t\t\tCamera3RequestDescriptor::Status::Error;\n>\n> I think I like the second option best.\n>\n>> +\n>> +\t\t\t\tcameraDevice_->streamProcessingComplete(this, request,\n>> +\t\t\t\t\t\t\t\t\tbufferStatus);\n>> +\t\t\t});\n>> \t}\n>> \n>> \tif (type_ == Type::Internal) {\n>> diff --git a/src/android/jpeg/post_processor_jpeg.cpp b/src/android/jpeg/post_processor_jpeg.cpp\n>> index 699576ef..a001fede 100644\n>> --- a/src/android/jpeg/post_processor_jpeg.cpp\n>> +++ b/src/android/jpeg/post_processor_jpeg.cpp\n>> @@ -198,6 +198,7 @@ int PostProcessorJpeg::process(const FrameBuffer &source,\n>> \t\t\t\t\t exif.data(), quality);\n>> \tif (jpeg_size < 0) {\n>> \t\tLOG(JPEG, Error) << \"Failed to encode stream image\";\n>> +\t\tprocessComplete.emit(request, PostProcessor::Status::Error);\n> I think you can write Status::Error here and below, including in\n> post_processor_yuv.cpp (not above in camera_stream.cpp though).\n>\n>> \t\treturn jpeg_size;\n>> \t}\n>> \n>> @@ -211,6 +212,7 @@ int PostProcessorJpeg::process(const FrameBuffer &source,\n>> \n>> \t/* Update the JPEG result Metadata. */\n>> \tresultMetadata->addEntry(ANDROID_JPEG_SIZE, jpeg_size);\n>> +\tprocessComplete.emit(request, PostProcessor::Status::Success);\n>> \n>> \treturn 0;\n>> }\n>> diff --git a/src/android/post_processor.h b/src/android/post_processor.h\n>> index 27eaef88..14f5e8c7 100644\n>> --- a/src/android/post_processor.h\n>> +++ b/src/android/post_processor.h\n>> @@ -7,6 +7,8 @@\n>> #ifndef __ANDROID_POST_PROCESSOR_H__\n>> #define __ANDROID_POST_PROCESSOR_H__\n>> \n>> +#include \n>> +\n>> #include \n>> #include \n>> \n>> @@ -17,6 +19,11 @@ class Camera3RequestDescriptor;\n>> class PostProcessor\n>> {\n>> public:\n>> +\tenum class Status {\n>> +\t\tError,\n>> +\t\tSuccess\n>> +\t};\n>> +\n>> \tvirtual ~PostProcessor() = default;\n>> \n>> \tvirtual int configure(const libcamera::StreamConfiguration &inCfg,\n>> @@ -24,6 +31,8 @@ public:\n>> \tvirtual int process(const libcamera::FrameBuffer &source,\n>> \t\t\t CameraBuffer *destination,\n>> \t\t\t Camera3RequestDescriptor *request) = 0;\n>> +\n>> +\tlibcamera::Signal processComplete;\n>> };\n>> \n>> #endif /* __ANDROID_POST_PROCESSOR_H__ */\n>> diff --git a/src/android/yuv/post_processor_yuv.cpp b/src/android/yuv/post_processor_yuv.cpp\n>> index 8110a1f1..fd364741 100644\n>> --- a/src/android/yuv/post_processor_yuv.cpp\n>> +++ b/src/android/yuv/post_processor_yuv.cpp\n>> @@ -51,14 +51,17 @@ int PostProcessorYuv::configure(const StreamConfiguration &inCfg,\n>> \n>> int PostProcessorYuv::process(const FrameBuffer &source,\n>> \t\t\t CameraBuffer *destination,\n>> -\t\t\t [[maybe_unused]] Camera3RequestDescriptor *request)\n>> +\t\t\t Camera3RequestDescriptor *request)\n>> {\n>> -\tif (!isValidBuffers(source, *destination))\n>> +\tif (!isValidBuffers(source, *destination)) {\n>> +\t\tprocessComplete.emit(request, PostProcessor::Status::Error);\n>> \t\treturn -EINVAL;\n>> +\t}\n>> \n>> \tconst MappedFrameBuffer sourceMapped(&source, MappedFrameBuffer::MapFlag::Read);\n>> \tif (!sourceMapped.isValid()) {\n>> \t\tLOG(YUV, Error) << \"Failed to mmap camera frame buffer\";\n>> +\t\tprocessComplete.emit(request, PostProcessor::Status::Error);\n>> \t\treturn -EINVAL;\n>> \t}\n>> \n>> @@ -76,9 +79,12 @@ int PostProcessorYuv::process(const FrameBuffer &source,\n>> \t\t\t\t libyuv::FilterMode::kFilterBilinear);\n>> \tif (ret) {\n>> \t\tLOG(YUV, Error) << \"Failed NV12 scaling: \" << ret;\n>> +\t\tprocessComplete.emit(request, PostProcessor::Status::Error);\n>> \t\treturn -EINVAL;\n>> \t}\n>> \n>> +\tprocessComplete.emit(request, PostProcessor::Status::Success);\n>> +\n>> \treturn 0;\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 3DA08BDB1C\n\tfor ;\n\tThu, 21 Oct 2021 05:35:51 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7D0C768F57;\n\tThu, 21 Oct 2021 07:35:50 +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 AB02B60124\n\tfor ;\n\tThu, 21 Oct 2021 07:35:49 +0200 (CEST)","from [192.168.1.106] (unknown [103.251.226.185])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 93B4E2BA;\n\tThu, 21 Oct 2021 07:35:48 +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=\"J39SKdj4\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1634794549;\n\tbh=wtEk4gf+AUKrPdsV82eBxdzBX0d1j59imw7D+izwRf8=;\n\th=Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=J39SKdj46MEjYBUpcyHX1Mz6goDRcbZSSwk4rjuqoiqVcG3Ismmvcb+Jw4g1Iv8ie\n\t5Yyt0Oxtk6ZZ2TYa19qrV3DMzQV1QNc9O/KoKzm4O0zy0qtaNcU3HMwnAcHi4rZVvV\n\t8jI3PhOuPRE3yol6i3Vb3omVEtdMdzH7sErWBTf8=","To":"Laurent Pinchart ","References":"<20211020104212.121743-1-umang.jain@ideasonboard.com>\n\t<20211020104212.121743-2-umang.jain@ideasonboard.com>\n\t","From":"Umang Jain ","Message-ID":"","Date":"Thu, 21 Oct 2021 11:05:44 +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 v5 1/4] android: Notify post\n\tprocessing completion via a signal","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":20360,"web_url":"https://patchwork.libcamera.org/comment/20360/","msgid":"","date":"2021-10-21T13:52:10","subject":"Re: [libcamera-devel] [PATCH v5 1/4] android: Notify post\n\tprocessing completion via a signal","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, Oct 21, 2021 at 11:05:44AM +0530, Umang Jain wrote:\n> On 10/21/21 6:01 AM, Laurent Pinchart wrote:\n> > On Wed, Oct 20, 2021 at 04:12:09PM +0530, Umang Jain wrote:\n> >> Notify that the post processing for a request has been completed,\n> >> via a signal. A pointer to the descriptor which is tracking the\n> >> capture request is emitted along with the status of post processed\n> >> buffer. The function CameraDevice::streamProcessingComplete() will\n> >> finally set the status on the request descriptor and send capture\n> >> results back to the framework accordingly.\n> >>\n> >> We also need to save a pointer to any internal buffers that might have\n> >> been allocated by CameraStream. The buffer should be returned back to\n> >> CameraStream just before capture results are sent.\n> >>\n> >> A streamProcessMutex_ has been introduced here itself, which will be\n> > Did you mean s/itself/as well/ ?\n> >\n> >> applicable to guard access to descriptor->buffers_ when post-processing\n> >> is moved to be asynchronous in subsequent commits.\n> >>\n> >> Signed-off-by: Umang Jain \n> >> ---\n> >> src/android/camera_device.cpp | 92 +++++++++++++++++++++---\n> >> src/android/camera_device.h | 7 ++\n> >> src/android/camera_request.h | 4 ++\n> >> src/android/camera_stream.cpp | 13 ++++\n> >> src/android/jpeg/post_processor_jpeg.cpp | 2 +\n> >> src/android/post_processor.h | 9 +++\n> >> src/android/yuv/post_processor_yuv.cpp | 10 ++-\n> >> 7 files changed, 125 insertions(+), 12 deletions(-)\n> >>\n> >> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> >> index 806b4090..541c2c81 100644\n> >> --- a/src/android/camera_device.cpp\n> >> +++ b/src/android/camera_device.cpp\n> >> @@ -1117,6 +1117,15 @@ void CameraDevice::requestComplete(Request *request)\n> >> \t}\n> >> \n> >> \t/* Handle post-processing. */\n> >> +\tbool needsPostProcessing = false;\n> >> +\tCamera3RequestDescriptor::Status processingStatus =\n> >> +\t\tCamera3RequestDescriptor::Status::Pending;\n> >> +\t/*\n> >> +\t * \\todo Apply streamsProcessMutex_ when post-processing is adapted to run\n> >> +\t * asynchronously. If we introduce the streamsProcessMutex_ right away, the\n> >> +\t * lock will be held forever since it is synchronous at this point\n> >> +\t * (see streamProcessingComplete).\n> >> +\t */\n> >> \tfor (auto &buffer : descriptor->buffers_) {\n> >> \t\tCameraStream *stream = buffer.stream;\n> >> \n> >> @@ -1132,22 +1141,27 @@ void CameraDevice::requestComplete(Request *request)\n> >> \t\t\tcontinue;\n> >> \t\t}\n> >> \n> >> -\t\tint ret = stream->process(*src, 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 (stream->type() == CameraStream::Type::Internal)\n> >> -\t\t\tstream->putBuffer(src);\n> >> +\t\t\tbuffer.internalBuffer = src;\n> >\n> > Let's make the field name more self-explanatory. A possible candidate is\n> > sourceBuffer instead of internalBuffer.\n> >\n> > Could we do this in CameraDevice::processCaptureRequest(), just after\n> > calling cameraStream->getBuffer() ? I'll feel less worried about a leak\n> > if we store the buffer for later release right after acquiring it.\n> \n> Ack.\n> \n> >> \n> >> +\t\tneedsPostProcessing = true;\n> >> +\t\tint ret = stream->process(*src, buffer, descriptor);\n> >> \t\tif (ret) {\n> >> -\t\t\tbuffer.status = Camera3RequestDescriptor::Status::Error;\n> >> -\t\t\tnotifyError(descriptor->frameNumber_, stream->camera3Stream(),\n> >> -\t\t\t\t CAMERA3_MSG_ERROR_BUFFER);\n> >> +\t\t\tsetBufferStatus(stream, buffer, descriptor,\n> >> +\t\t\t\t\tCamera3RequestDescriptor::Status::Error);\n> >> +\t\t\tprocessingStatus = Camera3RequestDescriptor::Status::Error;\n> >> \t\t}\n> >\n> > I think we need to improve error handling a little bit, in order to\n> > support multiple streams being post-processed. We need to complete the\n> > request in CameraDevice::streamProcessingComplete() when the last\n> > processing stream completes, so the number of post-processed streams\n> > need to be somehow recorded in the Camera3RequestDescriptor. It could be\n> > tempted to have a counter, but a\n> \n> I have implemented a counter based solution, it still didn't address all \n> the corner cases. Since we have two  path of error handling (synchronous \n> error and asynchronous error), a counter based implementation was \n> probably too verbose with much of code duplication to handle various \n> scenarios.\n> \n> >\n> > \tstd::map pendingPostProcess_;\n> >\n> > could be better, as it will also remove the need for loops in\n> > streamProcessingComplete() to lookup the buffer from the stream and to\n> > check if the request has been fully processed. Accessing the map has to\n> > be done with the mutex held to avoid race conditions, but it's a bit\n> > more tricky than that. If we just add elements to the map here, we could\n> \n> Good, so you do understand so many looping lookups in \n> streamProcessingComplete.\n> \n> I broadly feel okay with std::map \n> pendingPostProcess_; as long as it resides inside the descriptor. For \n> me, it avoids lookups, and can address the \\todo in 3/4.\n> \n> Having said that, I feel a bit wary of introducing more data structures \n> for management. We already have a queue of post-processing (convert to \n> std:deque if we want to access the queued requests for post-processing) \n> and now we will have another map to tracking the requests. I wonder if \n> we can get away with a PostProcessorWorker's std::deque, with the \n> benefits of the map. But again, we have to take care of exposure / mutex \n> with main thread, which can lead to subtle bugs and unwanted waiting on \n> each thread\n> \n> Now I think about it, the pendingPostProcess_ map is about Stream's \n> pending processing _per descriptor_, whereas the PostProcessorWorker's \n> queue is replacement for Thread's message queue. Hmm.\n\nThat's right, those two data structures are independent from each other.\n\n> > race with streamProcessingComplete() as it could be called for the first\n> > post-processed stream before we have a chance to add the second one\n> > here. streamProcessingComplete() would then incorrectly think it has\n> > completed the last stream, and proceed to complete the request.\n> >\n> > This race could be solved by holding the lock for the whole duration of\n> > the loop here, covering all calls to stream->process(), but it may be\n> \n> Yes,  I am considering this in current implementation too. The way it \n> will look like, is first queue up all the post-processing requests to \n> PostProcessorWoe, only then, allow any of the \n> slot(streamProcessingComplete) to get executed. A simple mutex hold can \n> do that\n\nI usually try to minimize the amount of code covered by locks, and focus\non data access only if possible instead of keeping the lock held over\nthe callbacks. Let's see if that's possible. There's a tricky part\nthough, as sendCaptureResults() is called from two different threads\n(synchronously with request queuing in error paths and asynchronously in\nthe completion path), we currently have a race condition.\n\nThread A\t\t\t\t\t\t\tThread B\n-----------------------------------------------------------------------------------------------------------\n\nMutexLocker lock(descriptorsMutex_);\nwhile (!descriptors_.empty() &&\n !descriptors_.front()->isPending()) {\n auto descriptor = std::move(descriptors_.front());\n descriptors_.pop();\n\n lock.unlock();\n\t\t\t\t\t\t\t\tMutexLocker lock(descriptorsMutex_);\n\t\t\t\t\t\t\t\twhile (!descriptors_.empty() &&\n\t\t\t\t\t\t\t\t !descriptors_.front()->isPending()) {\n\t\t\t\t\t\t\t\t\tauto descriptor = std::move(descriptors_.front());\n\t\t\t\t\t\t\t\t\tdescriptors_.pop();\n\n\t\t\t\t\t\t\t\t\tlock.unlock();\n\n\t\t\t\t\t\t\t\t\t...\n\n\t\t\t\t\t\t\t\t\tcallbacks_->process_capture_result(callbacks_,\n\t\t\t\t\t\t\t\t\t\t\t\t\t &captureResult);\n\n\t\t\t\t\t\t\t\t\tlock.lock();\n\t\t\t\t\t\t\t\t}\n\t...\n\n callbacks_->process_capture_result(callbacks_,\n\t\t\t\t\t &captureResult);\n\n lock.lock();\n}\n\nThread A gets the request at the top of the queue, thread B gets the\nnext one, and signals its completion before thread A.\n\nIt's likely possible to implement something clever here, to minimize the\nlocked sections and still be correct, but that may be unnecessary\noptimization. We could start by holding the lock for the whole duration\nof the function. In that case, I would remove the lock from\nsendCaptureResults() completely, and call the function with the lock\nheld in completeDescriptor().\n\n> > possible to do better. We could add entries to the map in\n> > processCaptureRequest() instead. We would need, here, to remove entries\n> \n> I am not so sure about this, We could add entries in \n> processCaptureRequest() but it feels too early. Let me try to address \n> other comments, and I can iterate on this on top.\n> \n> > from the map if stream->process() fails, and complete the request if we\n> > remove the last entry (as otherwise, streamProcessingComplete() could\n> > complete all streams, then the last stream->process() call could fail,\n> > and nobody would complete the request).\n> \n> This is still a bit tricky to handle. What's the de-facto criteria to \n> remove entries from the map (not just the error path) ?\n> \n> Assuming we hold the lock for entire duration of\n> \n>     Locker.lock();\n> \n>     for {\n>         ...\n>         int ret = stream->process(...);\n>         if (ret) {\n>             descriptor.pendingPostProcess_.erase(stream);\n>             setBufferStatus(stream, error);\n> \n>             if (descriptor.pendingPostProcess_.size() == 0)\n>                 completeDescriptor (error);\n> \n>         }\n> \n>     }\n>     locker.unlock();\n> \n> ....\n> \n> streamProcessingComplete(..., bufferStatus)\n> \n> {\n> \n>     Locker.lock();\n> \n> descriptor.pendingPostProcess_.erase(stream);\n>     setBufferStatus(stream, bufferStatus);\n> \n>     if (descriptor.pendingPostProcess_.size() == 0)\n>         completeDescriptor (?);\n> \n> }\n> \n> the ? indicates with what status I should complete the descriptor? I \n> would probably need to iterate over all the descriptor's buffer status \n> to see if any of the (previous)buffer has failed to set error status on \n> the descriptor overall? I think so, we would still need a look up loop \n> here, I can't see deciphering this from the map.\n\nCan we record a post-processing status in the descriptor, setting it to\nsuccess when constructor, and setting it to error when a post-processing\nfails ? Then you can inspect that state only when completing the\nrequest. A boolean flag (hasPostProcessingErrors for instance) could be\nenough.\n\nOr we could reuse the status_ member variable for that. It's currently\nused both to tell if a descriptor is complete (when !pending) and what\nits status is. We could split that into a status flag that would only\nhave success and error values, and a pending (or complete) boolean. This\nmay be a better design, as it would allow us to update the request\nstatus as soon as an error is detected, and set the completion flag\nseparately only when all processing has completed.\n\n> Are we getting too complicated with this? I have started to feel so ...\n> \n> > Other designs may be possible.\n> >\n> >> \t}\n> >> \n> >> +\tif (needsPostProcessing) {\n> >> +\t\tif (processingStatus == Camera3RequestDescriptor::Status::Error) {\n> >> +\t\t\tdescriptor->status_ = processingStatus;\n> >> +\t\t\tsendCaptureResults();\n> >> +\t\t}\n> >> +\n> >> +\t\treturn;\n> >> +\t}\n> >> +\n> >> \tdescriptor->status_ = Camera3RequestDescriptor::Status::Success;\n> >> \tsendCaptureResults();\n> >> }\n> >> @@ -1206,6 +1220,64 @@ void CameraDevice::sendCaptureResults()\n> >> \t}\n> >> }\n> >> \n> >> +void CameraDevice::setBufferStatus(CameraStream *cameraStream,\n> >> +\t\t\t\t Camera3RequestDescriptor::StreamBuffer &buffer,\n> >> +\t\t\t\t Camera3RequestDescriptor *request,\n> >> +\t\t\t\t Camera3RequestDescriptor::Status status)\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(buffer.internalBuffer);\n> >> +\n> >> +\tbuffer.status = status;\n> >> +\tif (status != Camera3RequestDescriptor::Status::Success)\n> >> +\t\tnotifyError(request->frameNumber_, buffer.stream->camera3Stream(),\n> >> +\t\t\t CAMERA3_MSG_ERROR_BUFFER);\n> >> +}\n> >> +\n> >> +void CameraDevice::streamProcessingComplete(CameraStream *cameraStream,\n> >> +\t\t\t\t\t Camera3RequestDescriptor *request,\n> >> +\t\t\t\t\t Camera3RequestDescriptor::Status status)\n> >> +{\n> >> +\tMutexLocker locker(request->streamsProcessMutex_);\n> >> +\tfor (auto &buffer : request->buffers_) {\n> >> +\t\tif (buffer.stream != cameraStream)\n> >> +\t\t\tcontinue;\n> >> +\n> >> +\t\tsetBufferStatus(cameraStream, buffer, request, status);\n> >> +\t}\n> >> +\n> >> +\tbool hasPostProcessingErrors = false;\n> >> +\tfor (auto &buffer : request->buffers_) {\n> >> +\t\tif (cameraStream->type() == CameraStream::Type::Direct)\n> >> +\t\t\tcontinue;\n> >> +\n> >> +\t\t/*\n> >> +\t\t * Other eligible buffers might be waiting to get post-processed.\n> >> +\t\t * So wait for their turn before sendCaptureResults() for the\n> >> +\t\t * descriptor.\n> >> +\t\t */\n> >> +\t\tif (buffer.status == Camera3RequestDescriptor::Status::Pending)\n> >> +\t\t\treturn;\n> >> +\n> >> +\t\tif (!hasPostProcessingErrors &&\n> >> +\t\t buffer.status == Camera3RequestDescriptor::Status::Error)\n> >> +\t\t\thasPostProcessingErrors = true;\n> >> +\t}\n> >> +\n> >> +\tif (hasPostProcessingErrors)\n> >> +\t\trequest->status_ = Camera3RequestDescriptor::Status::Error;\n> >> +\telse\n> >> +\t\trequest->status_ = Camera3RequestDescriptor::Status::Success;\n> >> +\n> >> +\tlocker.unlock();\n> >> +\n> >> +\tsendCaptureResults();\n> >> +}\n> >> +\n> >> std::string CameraDevice::logPrefix() const\n> >> {\n> >> \treturn \"'\" + camera_->id() + \"'\";\n> >> diff --git a/src/android/camera_device.h b/src/android/camera_device.h\n> >> index 863cf414..1ef933da 100644\n> >> --- a/src/android/camera_device.h\n> >> +++ b/src/android/camera_device.h\n> >> @@ -66,6 +66,9 @@ public:\n> >> \tint configureStreams(camera3_stream_configuration_t *stream_list);\n> >> \tint processCaptureRequest(camera3_capture_request_t *request);\n> >> \tvoid requestComplete(libcamera::Request *request);\n> >> +\tvoid streamProcessingComplete(CameraStream *cameraStream,\n> >> +\t\t\t\t Camera3RequestDescriptor *request,\n> >> +\t\t\t\t Camera3RequestDescriptor::Status status);\n> >> \n> >> protected:\n> >> \tstd::string logPrefix() const override;\n> >> @@ -94,6 +97,10 @@ private:\n> >> \t\t\t camera3_error_msg_code code) const;\n> >> \tint processControls(Camera3RequestDescriptor *descriptor);\n> >> \tvoid sendCaptureResults();\n> >> +\tvoid setBufferStatus(CameraStream *cameraStream,\n> >> +\t\t\t Camera3RequestDescriptor::StreamBuffer &buffer,\n> >> +\t\t\t Camera3RequestDescriptor *request,\n> >> +\t\t\t Camera3RequestDescriptor::Status status);\n> >> \tstd::unique_ptr getResultMetadata(\n> >> \t\tconst Camera3RequestDescriptor &descriptor) const;\n> >> \n> >> diff --git a/src/android/camera_request.h b/src/android/camera_request.h\n> >> index 05dabf89..3a2774e0 100644\n> >> --- a/src/android/camera_request.h\n> >> +++ b/src/android/camera_request.h\n> >> @@ -8,6 +8,7 @@\n> >> #define __ANDROID_CAMERA_REQUEST_H__\n> >> \n> >> #include \n> >> +#include \n> >> #include \n> >> \n> >> #include \n> >> @@ -37,6 +38,7 @@ public:\n> >> \t\tstd::unique_ptr frameBuffer;\n> >> \t\tint fence;\n> >> \t\tStatus status;\n> >> +\t\tlibcamera::FrameBuffer *internalBuffer = nullptr;\n> >> \t};\n> >> \n> >> \tCamera3RequestDescriptor(libcamera::Camera *camera,\n> >> @@ -47,6 +49,8 @@ public:\n> >> \n> >> \tuint32_t frameNumber_ = 0;\n> >> \n> >> +\t/* Protects buffers_ for post-processing streams. */\n> >> +\tstd::mutex streamsProcessMutex_;\n> >> \tstd::vector buffers_;\n> >> \n> >> \tCameraMetadata settings_;\n> >> diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp\n> >> index f44a2717..04cbef8c 100644\n> >> --- a/src/android/camera_stream.cpp\n> >> +++ b/src/android/camera_stream.cpp\n> >> @@ -22,6 +22,7 @@\n> >> #include \"camera_capabilities.h\"\n> >> #include \"camera_device.h\"\n> >> #include \"camera_metadata.h\"\n> >> +#include \"post_processor.h\"\n> >> \n> >> using namespace libcamera;\n> >> \n> >> @@ -97,6 +98,18 @@ int CameraStream::configure()\n> >> \t\tint ret = postProcessor_->configure(configuration(), output);\n> >> \t\tif (ret)\n> >> \t\t\treturn ret;\n> >> +\n> >> +\t\tpostProcessor_->processComplete.connect(\n> >> +\t\t\tthis, [&](Camera3RequestDescriptor *request, PostProcessor::Status status) {\n> >> +\t\t\t\tCamera3RequestDescriptor::Status bufferStatus =\n> >> +\t\t\t\t\tCamera3RequestDescriptor::Status::Error;\n> >> +\n> >> +\t\t\t\tif (status == PostProcessor::Status::Success)\n> >> +\t\t\t\t\tbufferStatus = Camera3RequestDescriptor::Status::Success;\n> >\n> > It looks weird to make Error a default (and thus special) case. Would\n> > any of the following be better ?\n> >\n> > \t\t\t\tCamera3RequestDescriptor::Status bufferStatus;\n> >\n> > \t\t\t\tif (status == PostProcessor::Status::Success)\n> > \t\t\t\t\tbufferStatus = Camera3RequestDescriptor::Status::Success;\n> > \t\t\t\telse\n> > \t\t\t\t\tbufferStatus = Camera3RequestDescriptor::Status::Error;\n> >\n> >\n> > \t\t\t\tCamera3RequestDescriptor::Status bufferStatus =\n> > \t\t\t\t\tstatus == PostProcessor::Status::Success ?\n> > \t\t\t\t\tCamera3RequestDescriptor::Status::Success :\n> > \t\t\t\t\tCamera3RequestDescriptor::Status::Error;\n> >\n> > I think I like the second option best.\n> >\n> >> +\n> >> +\t\t\t\tcameraDevice_->streamProcessingComplete(this, request,\n> >> +\t\t\t\t\t\t\t\t\tbufferStatus);\n> >> +\t\t\t});\n> >> \t}\n> >> \n> >> \tif (type_ == Type::Internal) {\n> >> diff --git a/src/android/jpeg/post_processor_jpeg.cpp b/src/android/jpeg/post_processor_jpeg.cpp\n> >> index 699576ef..a001fede 100644\n> >> --- a/src/android/jpeg/post_processor_jpeg.cpp\n> >> +++ b/src/android/jpeg/post_processor_jpeg.cpp\n> >> @@ -198,6 +198,7 @@ int PostProcessorJpeg::process(const FrameBuffer &source,\n> >> \t\t\t\t\t exif.data(), quality);\n> >> \tif (jpeg_size < 0) {\n> >> \t\tLOG(JPEG, Error) << \"Failed to encode stream image\";\n> >> +\t\tprocessComplete.emit(request, PostProcessor::Status::Error);\n> >\n> > I think you can write Status::Error here and below, including in\n> > post_processor_yuv.cpp (not above in camera_stream.cpp though).\n> >\n> >> \t\treturn jpeg_size;\n> >> \t}\n> >> \n> >> @@ -211,6 +212,7 @@ int PostProcessorJpeg::process(const FrameBuffer &source,\n> >> \n> >> \t/* Update the JPEG result Metadata. */\n> >> \tresultMetadata->addEntry(ANDROID_JPEG_SIZE, jpeg_size);\n> >> +\tprocessComplete.emit(request, PostProcessor::Status::Success);\n> >> \n> >> \treturn 0;\n> >> }\n> >> diff --git a/src/android/post_processor.h b/src/android/post_processor.h\n> >> index 27eaef88..14f5e8c7 100644\n> >> --- a/src/android/post_processor.h\n> >> +++ b/src/android/post_processor.h\n> >> @@ -7,6 +7,8 @@\n> >> #ifndef __ANDROID_POST_PROCESSOR_H__\n> >> #define __ANDROID_POST_PROCESSOR_H__\n> >> \n> >> +#include \n> >> +\n> >> #include \n> >> #include \n> >> \n> >> @@ -17,6 +19,11 @@ class Camera3RequestDescriptor;\n> >> class PostProcessor\n> >> {\n> >> public:\n> >> +\tenum class Status {\n> >> +\t\tError,\n> >> +\t\tSuccess\n> >> +\t};\n> >> +\n> >> \tvirtual ~PostProcessor() = default;\n> >> \n> >> \tvirtual int configure(const libcamera::StreamConfiguration &inCfg,\n> >> @@ -24,6 +31,8 @@ public:\n> >> \tvirtual int process(const libcamera::FrameBuffer &source,\n> >> \t\t\t CameraBuffer *destination,\n> >> \t\t\t Camera3RequestDescriptor *request) = 0;\n> >> +\n> >> +\tlibcamera::Signal processComplete;\n> >> };\n> >> \n> >> #endif /* __ANDROID_POST_PROCESSOR_H__ */\n> >> diff --git a/src/android/yuv/post_processor_yuv.cpp b/src/android/yuv/post_processor_yuv.cpp\n> >> index 8110a1f1..fd364741 100644\n> >> --- a/src/android/yuv/post_processor_yuv.cpp\n> >> +++ b/src/android/yuv/post_processor_yuv.cpp\n> >> @@ -51,14 +51,17 @@ int PostProcessorYuv::configure(const StreamConfiguration &inCfg,\n> >> \n> >> int PostProcessorYuv::process(const FrameBuffer &source,\n> >> \t\t\t CameraBuffer *destination,\n> >> -\t\t\t [[maybe_unused]] Camera3RequestDescriptor *request)\n> >> +\t\t\t Camera3RequestDescriptor *request)\n> >> {\n> >> -\tif (!isValidBuffers(source, *destination))\n> >> +\tif (!isValidBuffers(source, *destination)) {\n> >> +\t\tprocessComplete.emit(request, PostProcessor::Status::Error);\n> >> \t\treturn -EINVAL;\n> >> +\t}\n> >> \n> >> \tconst MappedFrameBuffer sourceMapped(&source, MappedFrameBuffer::MapFlag::Read);\n> >> \tif (!sourceMapped.isValid()) {\n> >> \t\tLOG(YUV, Error) << \"Failed to mmap camera frame buffer\";\n> >> +\t\tprocessComplete.emit(request, PostProcessor::Status::Error);\n> >> \t\treturn -EINVAL;\n> >> \t}\n> >> \n> >> @@ -76,9 +79,12 @@ int PostProcessorYuv::process(const FrameBuffer &source,\n> >> \t\t\t\t libyuv::FilterMode::kFilterBilinear);\n> >> \tif (ret) {\n> >> \t\tLOG(YUV, Error) << \"Failed NV12 scaling: \" << ret;\n> >> +\t\tprocessComplete.emit(request, PostProcessor::Status::Error);\n> >> \t\treturn -EINVAL;\n> >> \t}\n> >> \n> >> +\tprocessComplete.emit(request, PostProcessor::Status::Success);\n> >> +\n> >> \treturn 0;\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 59101BDB1C\n\tfor ;\n\tThu, 21 Oct 2021 13:52:32 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id CCB9268F56;\n\tThu, 21 Oct 2021 15:52:31 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id EAC84604FE\n\tfor ;\n\tThu, 21 Oct 2021 15:52:29 +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 5C0F78B6;\n\tThu, 21 Oct 2021 15:52:29 +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=\"hxahvgnn\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1634824349;\n\tbh=mw4XGDUHsxyjzHnUOFpTrrsGxd9F+AD+K7RpwEUGiis=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=hxahvgnnVqB/YGxFxTfTm/Gu0iHQVwZCY/XZBQ0LZrrMXjMpJ6Rpmdz0G7apkeT8K\n\tgKbviC+umm40EFAZMdgPVQacmCUXpDLlvZNfK8oESNDZTpPG5HCBRcqnYlmiR1H8Nb\n\tZ1IpXmYDW6lUvhshoawaTfHE/JhX051FBTPzlmFo=","Date":"Thu, 21 Oct 2021 16:52:10 +0300","From":"Laurent Pinchart ","To":"Umang Jain ","Message-ID":"","References":"<20211020104212.121743-1-umang.jain@ideasonboard.com>\n\t<20211020104212.121743-2-umang.jain@ideasonboard.com>\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 v5 1/4] android: Notify post\n\tprocessing completion via a signal","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":20361,"web_url":"https://patchwork.libcamera.org/comment/20361/","msgid":"<27967c1d-8ff8-b735-cdb3-154feeaefd57@ideasonboard.com>","date":"2021-10-21T14:34:18","subject":"Re: [libcamera-devel] [PATCH v5 1/4] android: Notify post\n\tprocessing completion via a signal","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hi Laurent,\n\nOn 10/21/21 7:22 PM, Laurent Pinchart wrote:\n> Hi Umang,\n>\n> On Thu, Oct 21, 2021 at 11:05:44AM +0530, Umang Jain wrote:\n>> On 10/21/21 6:01 AM, Laurent Pinchart wrote:\n>>> On Wed, Oct 20, 2021 at 04:12:09PM +0530, Umang Jain wrote:\n>>>> Notify that the post processing for a request has been completed,\n>>>> via a signal. A pointer to the descriptor which is tracking the\n>>>> capture request is emitted along with the status of post processed\n>>>> buffer. The function CameraDevice::streamProcessingComplete() will\n>>>> finally set the status on the request descriptor and send capture\n>>>> results back to the framework accordingly.\n>>>>\n>>>> We also need to save a pointer to any internal buffers that might have\n>>>> been allocated by CameraStream. The buffer should be returned back to\n>>>> CameraStream just before capture results are sent.\n>>>>\n>>>> A streamProcessMutex_ has been introduced here itself, which will be\n>>> Did you mean s/itself/as well/ ?\n>>>\n>>>> applicable to guard access to descriptor->buffers_ when post-processing\n>>>> is moved to be asynchronous in subsequent commits.\n>>>>\n>>>> Signed-off-by: Umang Jain \n>>>> ---\n>>>> src/android/camera_device.cpp | 92 +++++++++++++++++++++---\n>>>> src/android/camera_device.h | 7 ++\n>>>> src/android/camera_request.h | 4 ++\n>>>> src/android/camera_stream.cpp | 13 ++++\n>>>> src/android/jpeg/post_processor_jpeg.cpp | 2 +\n>>>> src/android/post_processor.h | 9 +++\n>>>> src/android/yuv/post_processor_yuv.cpp | 10 ++-\n>>>> 7 files changed, 125 insertions(+), 12 deletions(-)\n>>>>\n>>>> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n>>>> index 806b4090..541c2c81 100644\n>>>> --- a/src/android/camera_device.cpp\n>>>> +++ b/src/android/camera_device.cpp\n>>>> @@ -1117,6 +1117,15 @@ void CameraDevice::requestComplete(Request *request)\n>>>> \t}\n>>>> \n>>>> \t/* Handle post-processing. */\n>>>> +\tbool needsPostProcessing = false;\n>>>> +\tCamera3RequestDescriptor::Status processingStatus =\n>>>> +\t\tCamera3RequestDescriptor::Status::Pending;\n>>>> +\t/*\n>>>> +\t * \\todo Apply streamsProcessMutex_ when post-processing is adapted to run\n>>>> +\t * asynchronously. If we introduce the streamsProcessMutex_ right away, the\n>>>> +\t * lock will be held forever since it is synchronous at this point\n>>>> +\t * (see streamProcessingComplete).\n>>>> +\t */\n>>>> \tfor (auto &buffer : descriptor->buffers_) {\n>>>> \t\tCameraStream *stream = buffer.stream;\n>>>> \n>>>> @@ -1132,22 +1141,27 @@ void CameraDevice::requestComplete(Request *request)\n>>>> \t\t\tcontinue;\n>>>> \t\t}\n>>>> \n>>>> -\t\tint ret = stream->process(*src, 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 (stream->type() == CameraStream::Type::Internal)\n>>>> -\t\t\tstream->putBuffer(src);\n>>>> +\t\t\tbuffer.internalBuffer = src;\n>>> Let's make the field name more self-explanatory. A possible candidate is\n>>> sourceBuffer instead of internalBuffer.\n>>>\n>>> Could we do this in CameraDevice::processCaptureRequest(), just after\n>>> calling cameraStream->getBuffer() ? I'll feel less worried about a leak\n>>> if we store the buffer for later release right after acquiring it.\n>> Ack.\n>>\n>>>> \n>>>> +\t\tneedsPostProcessing = true;\n>>>> +\t\tint ret = stream->process(*src, buffer, descriptor);\n>>>> \t\tif (ret) {\n>>>> -\t\t\tbuffer.status = Camera3RequestDescriptor::Status::Error;\n>>>> -\t\t\tnotifyError(descriptor->frameNumber_, stream->camera3Stream(),\n>>>> -\t\t\t\t CAMERA3_MSG_ERROR_BUFFER);\n>>>> +\t\t\tsetBufferStatus(stream, buffer, descriptor,\n>>>> +\t\t\t\t\tCamera3RequestDescriptor::Status::Error);\n>>>> +\t\t\tprocessingStatus = Camera3RequestDescriptor::Status::Error;\n>>>> \t\t}\n>>> I think we need to improve error handling a little bit, in order to\n>>> support multiple streams being post-processed. We need to complete the\n>>> request in CameraDevice::streamProcessingComplete() when the last\n>>> processing stream completes, so the number of post-processed streams\n>>> need to be somehow recorded in the Camera3RequestDescriptor. It could be\n>>> tempted to have a counter, but a\n>> I have implemented a counter based solution, it still didn't address all\n>> the corner cases. Since we have two  path of error handling (synchronous\n>> error and asynchronous error), a counter based implementation was\n>> probably too verbose with much of code duplication to handle various\n>> scenarios.\n>>\n>>> \tstd::map pendingPostProcess_;\n>>>\n>>> could be better, as it will also remove the need for loops in\n>>> streamProcessingComplete() to lookup the buffer from the stream and to\n>>> check if the request has been fully processed. Accessing the map has to\n>>> be done with the mutex held to avoid race conditions, but it's a bit\n>>> more tricky than that. If we just add elements to the map here, we could\n>> Good, so you do understand so many looping lookups in\n>> streamProcessingComplete.\n>>\n>> I broadly feel okay with std::map\n>> pendingPostProcess_; as long as it resides inside the descriptor. For\n>> me, it avoids lookups, and can address the \\todo in 3/4.\n>>\n>> Having said that, I feel a bit wary of introducing more data structures\n>> for management. We already have a queue of post-processing (convert to\n>> std:deque if we want to access the queued requests for post-processing)\n>> and now we will have another map to tracking the requests. I wonder if\n>> we can get away with a PostProcessorWorker's std::deque, with the\n>> benefits of the map. But again, we have to take care of exposure / mutex\n>> with main thread, which can lead to subtle bugs and unwanted waiting on\n>> each thread\n>>\n>> Now I think about it, the pendingPostProcess_ map is about Stream's\n>> pending processing _per descriptor_, whereas the PostProcessorWorker's\n>> queue is replacement for Thread's message queue. Hmm.\n> That's right, those two data structures are independent from each other.\n>\n>>> race with streamProcessingComplete() as it could be called for the first\n>>> post-processed stream before we have a chance to add the second one\n>>> here. streamProcessingComplete() would then incorrectly think it has\n>>> completed the last stream, and proceed to complete the request.\n>>>\n>>> This race could be solved by holding the lock for the whole duration of\n>>> the loop here, covering all calls to stream->process(), but it may be\n>> Yes,  I am considering this in current implementation too. The way it\n>> will look like, is first queue up all the post-processing requests to\n>> PostProcessorWoe, only then, allow any of the\n>> slot(streamProcessingComplete) to get executed. A simple mutex hold can\n>> do that\n> I usually try to minimize the amount of code covered by locks, and focus\n> on data access only if possible instead of keeping the lock held over\n> the callbacks. Let's see if that's possible. There's a tricky part\n> though, as sendCaptureResults() is called from two different threads\n> (synchronously with request queuing in error paths and asynchronously in\n> the completion path), we currently have a race condition.\n>\n> Thread A\t\t\t\t\t\t\tThread B\n> -----------------------------------------------------------------------------------------------------------\n>\n> MutexLocker lock(descriptorsMutex_);\n> while (!descriptors_.empty() &&\n> !descriptors_.front()->isPending()) {\n> auto descriptor = std::move(descriptors_.front());\n> descriptors_.pop();\n>\n> lock.unlock();\n> \t\t\t\t\t\t\t\tMutexLocker lock(descriptorsMutex_);\n> \t\t\t\t\t\t\t\twhile (!descriptors_.empty() &&\n> \t\t\t\t\t\t\t\t !descriptors_.front()->isPending()) {\n> \t\t\t\t\t\t\t\t\tauto descriptor = std::move(descriptors_.front());\n> \t\t\t\t\t\t\t\t\tdescriptors_.pop();\n>\n> \t\t\t\t\t\t\t\t\tlock.unlock();\n>\n> \t\t\t\t\t\t\t\t\t...\n>\n> \t\t\t\t\t\t\t\t\tcallbacks_->process_capture_result(callbacks_,\n> \t\t\t\t\t\t\t\t\t\t\t\t\t &captureResult);\n>\n> \t\t\t\t\t\t\t\t\tlock.lock();\n> \t\t\t\t\t\t\t\t}\n> \t...\n>\n> callbacks_->process_capture_result(callbacks_,\n> \t\t\t\t\t &captureResult);\n>\n> lock.lock();\n> }\n>\n> Thread A gets the request at the top of the queue, thread B gets the\n> next one, and signals its completion before thread A.\n>\n> It's likely possible to implement something clever here, to minimize the\n> locked sections and still be correct, but that may be unnecessary\n> optimization. We could start by holding the lock for the whole duration\n> of the function. In that case, I would remove the lock from\n> sendCaptureResults() completely, and call the function with the lock\n> held in completeDescriptor().\n\n\nYes, provided sendCaptureResults() is only called completeDescriptor(). \nI don't want to get a implementation which uses completeDescriptor() and \nsendCaptureResults() both for process_capture_result() callback.\n\n>\n>>> possible to do better. We could add entries to the map in\n>>> processCaptureRequest() instead. We would need, here, to remove entries\n>> I am not so sure about this, We could add entries in\n>> processCaptureRequest() but it feels too early. Let me try to address\n>> other comments, and I can iterate on this on top.\n>>\n>>> from the map if stream->process() fails, and complete the request if we\n>>> remove the last entry (as otherwise, streamProcessingComplete() could\n>>> complete all streams, then the last stream->process() call could fail,\n>>> and nobody would complete the request).\n>> This is still a bit tricky to handle. What's the de-facto criteria to\n>> remove entries from the map (not just the error path) ?\n>>\n>> Assuming we hold the lock for entire duration of\n>>\n>>     Locker.lock();\n>>\n>>     for {\n>>         ...\n>>         int ret = stream->process(...);\n>>         if (ret) {\n>>             descriptor.pendingPostProcess_.erase(stream);\n>>             setBufferStatus(stream, error);\n>>\n>>             if (descriptor.pendingPostProcess_.size() == 0)\n>>                 completeDescriptor (error);\n>>\n>>         }\n>>\n>>     }\n>>     locker.unlock();\n>>\n>> ....\n>>\n>> streamProcessingComplete(..., bufferStatus)\n>>\n>> {\n>>\n>>     Locker.lock();\n>>\n>> descriptor.pendingPostProcess_.erase(stream);\n>>     setBufferStatus(stream, bufferStatus);\n>>\n>>     if (descriptor.pendingPostProcess_.size() == 0)\n>>         completeDescriptor (?);\n>>\n>> }\n>>\n>> the ? indicates with what status I should complete the descriptor? I\n>> would probably need to iterate over all the descriptor's buffer status\n>> to see if any of the (previous)buffer has failed to set error status on\n>> the descriptor overall? I think so, we would still need a look up loop\n>> here, I can't see deciphering this from the map.\n> Can we record a post-processing status in the descriptor, setting it to\n> success when constructor, and setting it to error when a post-processing\n> fails ? Then you can inspect that state only when completing the\n> request. A boolean flag (hasPostProcessingErrors for instance) could be\n> enough.\n\n\nI would take the boolean flag approach for now.\n\n>\n> Or we could reuse the status_ member variable for that. It's currently\n> used both to tell if a descriptor is complete (when !pending) and what\n> its status is. We could split that into a status flag that would only\n> have success and error values, and a pending (or complete) boolean. This\n> may be a better design, as it would allow us to update the request\n> status as soon as an error is detected, and set the completion flag\n> separately only when all processing has completed.\n\n\nThis is sane, which in interest of not increasing further scope of the \nseries, I am tempted to address this on top. I can record it with a \\todo\n\n>\n>> Are we getting too complicated with this? I have started to feel so ...\n>>\n>>> Other designs may be possible.\n>>>\n>>>> \t}\n>>>> \n>>>> +\tif (needsPostProcessing) {\n>>>> +\t\tif (processingStatus == Camera3RequestDescriptor::Status::Error) {\n>>>> +\t\t\tdescriptor->status_ = processingStatus;\n>>>> +\t\t\tsendCaptureResults();\n>>>> +\t\t}\n>>>> +\n>>>> +\t\treturn;\n>>>> +\t}\n>>>> +\n>>>> \tdescriptor->status_ = Camera3RequestDescriptor::Status::Success;\n>>>> \tsendCaptureResults();\n>>>> }\n>>>> @@ -1206,6 +1220,64 @@ void CameraDevice::sendCaptureResults()\n>>>> \t}\n>>>> }\n>>>> \n>>>> +void CameraDevice::setBufferStatus(CameraStream *cameraStream,\n>>>> +\t\t\t\t Camera3RequestDescriptor::StreamBuffer &buffer,\n>>>> +\t\t\t\t Camera3RequestDescriptor *request,\n>>>> +\t\t\t\t Camera3RequestDescriptor::Status status)\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(buffer.internalBuffer);\n>>>> +\n>>>> +\tbuffer.status = status;\n>>>> +\tif (status != Camera3RequestDescriptor::Status::Success)\n>>>> +\t\tnotifyError(request->frameNumber_, buffer.stream->camera3Stream(),\n>>>> +\t\t\t CAMERA3_MSG_ERROR_BUFFER);\n>>>> +}\n>>>> +\n>>>> +void CameraDevice::streamProcessingComplete(CameraStream *cameraStream,\n>>>> +\t\t\t\t\t Camera3RequestDescriptor *request,\n>>>> +\t\t\t\t\t Camera3RequestDescriptor::Status status)\n>>>> +{\n>>>> +\tMutexLocker locker(request->streamsProcessMutex_);\n>>>> +\tfor (auto &buffer : request->buffers_) {\n>>>> +\t\tif (buffer.stream != cameraStream)\n>>>> +\t\t\tcontinue;\n>>>> +\n>>>> +\t\tsetBufferStatus(cameraStream, buffer, request, status);\n>>>> +\t}\n>>>> +\n>>>> +\tbool hasPostProcessingErrors = false;\n>>>> +\tfor (auto &buffer : request->buffers_) {\n>>>> +\t\tif (cameraStream->type() == CameraStream::Type::Direct)\n>>>> +\t\t\tcontinue;\n>>>> +\n>>>> +\t\t/*\n>>>> +\t\t * Other eligible buffers might be waiting to get post-processed.\n>>>> +\t\t * So wait for their turn before sendCaptureResults() for the\n>>>> +\t\t * descriptor.\n>>>> +\t\t */\n>>>> +\t\tif (buffer.status == Camera3RequestDescriptor::Status::Pending)\n>>>> +\t\t\treturn;\n>>>> +\n>>>> +\t\tif (!hasPostProcessingErrors &&\n>>>> +\t\t buffer.status == Camera3RequestDescriptor::Status::Error)\n>>>> +\t\t\thasPostProcessingErrors = true;\n>>>> +\t}\n>>>> +\n>>>> +\tif (hasPostProcessingErrors)\n>>>> +\t\trequest->status_ = Camera3RequestDescriptor::Status::Error;\n>>>> +\telse\n>>>> +\t\trequest->status_ = Camera3RequestDescriptor::Status::Success;\n>>>> +\n>>>> +\tlocker.unlock();\n>>>> +\n>>>> +\tsendCaptureResults();\n>>>> +}\n>>>> +\n>>>> std::string CameraDevice::logPrefix() const\n>>>> {\n>>>> \treturn \"'\" + camera_->id() + \"'\";\n>>>> diff --git a/src/android/camera_device.h b/src/android/camera_device.h\n>>>> index 863cf414..1ef933da 100644\n>>>> --- a/src/android/camera_device.h\n>>>> +++ b/src/android/camera_device.h\n>>>> @@ -66,6 +66,9 @@ public:\n>>>> \tint configureStreams(camera3_stream_configuration_t *stream_list);\n>>>> \tint processCaptureRequest(camera3_capture_request_t *request);\n>>>> \tvoid requestComplete(libcamera::Request *request);\n>>>> +\tvoid streamProcessingComplete(CameraStream *cameraStream,\n>>>> +\t\t\t\t Camera3RequestDescriptor *request,\n>>>> +\t\t\t\t Camera3RequestDescriptor::Status status);\n>>>> \n>>>> protected:\n>>>> \tstd::string logPrefix() const override;\n>>>> @@ -94,6 +97,10 @@ private:\n>>>> \t\t\t camera3_error_msg_code code) const;\n>>>> \tint processControls(Camera3RequestDescriptor *descriptor);\n>>>> \tvoid sendCaptureResults();\n>>>> +\tvoid setBufferStatus(CameraStream *cameraStream,\n>>>> +\t\t\t Camera3RequestDescriptor::StreamBuffer &buffer,\n>>>> +\t\t\t Camera3RequestDescriptor *request,\n>>>> +\t\t\t Camera3RequestDescriptor::Status status);\n>>>> \tstd::unique_ptr getResultMetadata(\n>>>> \t\tconst Camera3RequestDescriptor &descriptor) const;\n>>>> \n>>>> diff --git a/src/android/camera_request.h b/src/android/camera_request.h\n>>>> index 05dabf89..3a2774e0 100644\n>>>> --- a/src/android/camera_request.h\n>>>> +++ b/src/android/camera_request.h\n>>>> @@ -8,6 +8,7 @@\n>>>> #define __ANDROID_CAMERA_REQUEST_H__\n>>>> \n>>>> #include \n>>>> +#include \n>>>> #include \n>>>> \n>>>> #include \n>>>> @@ -37,6 +38,7 @@ public:\n>>>> \t\tstd::unique_ptr frameBuffer;\n>>>> \t\tint fence;\n>>>> \t\tStatus status;\n>>>> +\t\tlibcamera::FrameBuffer *internalBuffer = nullptr;\n>>>> \t};\n>>>> \n>>>> \tCamera3RequestDescriptor(libcamera::Camera *camera,\n>>>> @@ -47,6 +49,8 @@ public:\n>>>> \n>>>> \tuint32_t frameNumber_ = 0;\n>>>> \n>>>> +\t/* Protects buffers_ for post-processing streams. */\n>>>> +\tstd::mutex streamsProcessMutex_;\n>>>> \tstd::vector buffers_;\n>>>> \n>>>> \tCameraMetadata settings_;\n>>>> diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp\n>>>> index f44a2717..04cbef8c 100644\n>>>> --- a/src/android/camera_stream.cpp\n>>>> +++ b/src/android/camera_stream.cpp\n>>>> @@ -22,6 +22,7 @@\n>>>> #include \"camera_capabilities.h\"\n>>>> #include \"camera_device.h\"\n>>>> #include \"camera_metadata.h\"\n>>>> +#include \"post_processor.h\"\n>>>> \n>>>> using namespace libcamera;\n>>>> \n>>>> @@ -97,6 +98,18 @@ int CameraStream::configure()\n>>>> \t\tint ret = postProcessor_->configure(configuration(), output);\n>>>> \t\tif (ret)\n>>>> \t\t\treturn ret;\n>>>> +\n>>>> +\t\tpostProcessor_->processComplete.connect(\n>>>> +\t\t\tthis, [&](Camera3RequestDescriptor *request, PostProcessor::Status status) {\n>>>> +\t\t\t\tCamera3RequestDescriptor::Status bufferStatus =\n>>>> +\t\t\t\t\tCamera3RequestDescriptor::Status::Error;\n>>>> +\n>>>> +\t\t\t\tif (status == PostProcessor::Status::Success)\n>>>> +\t\t\t\t\tbufferStatus = Camera3RequestDescriptor::Status::Success;\n>>> It looks weird to make Error a default (and thus special) case. Would\n>>> any of the following be better ?\n>>>\n>>> \t\t\t\tCamera3RequestDescriptor::Status bufferStatus;\n>>>\n>>> \t\t\t\tif (status == PostProcessor::Status::Success)\n>>> \t\t\t\t\tbufferStatus = Camera3RequestDescriptor::Status::Success;\n>>> \t\t\t\telse\n>>> \t\t\t\t\tbufferStatus = Camera3RequestDescriptor::Status::Error;\n>>>\n>>>\n>>> \t\t\t\tCamera3RequestDescriptor::Status bufferStatus =\n>>> \t\t\t\t\tstatus == PostProcessor::Status::Success ?\n>>> \t\t\t\t\tCamera3RequestDescriptor::Status::Success :\n>>> \t\t\t\t\tCamera3RequestDescriptor::Status::Error;\n>>>\n>>> I think I like the second option best.\n>>>\n>>>> +\n>>>> +\t\t\t\tcameraDevice_->streamProcessingComplete(this, request,\n>>>> +\t\t\t\t\t\t\t\t\tbufferStatus);\n>>>> +\t\t\t});\n>>>> \t}\n>>>> \n>>>> \tif (type_ == Type::Internal) {\n>>>> diff --git a/src/android/jpeg/post_processor_jpeg.cpp b/src/android/jpeg/post_processor_jpeg.cpp\n>>>> index 699576ef..a001fede 100644\n>>>> --- a/src/android/jpeg/post_processor_jpeg.cpp\n>>>> +++ b/src/android/jpeg/post_processor_jpeg.cpp\n>>>> @@ -198,6 +198,7 @@ int PostProcessorJpeg::process(const FrameBuffer &source,\n>>>> \t\t\t\t\t exif.data(), quality);\n>>>> \tif (jpeg_size < 0) {\n>>>> \t\tLOG(JPEG, Error) << \"Failed to encode stream image\";\n>>>> +\t\tprocessComplete.emit(request, PostProcessor::Status::Error);\n>>> I think you can write Status::Error here and below, including in\n>>> post_processor_yuv.cpp (not above in camera_stream.cpp though).\n>>>\n>>>> \t\treturn jpeg_size;\n>>>> \t}\n>>>> \n>>>> @@ -211,6 +212,7 @@ int PostProcessorJpeg::process(const FrameBuffer &source,\n>>>> \n>>>> \t/* Update the JPEG result Metadata. */\n>>>> \tresultMetadata->addEntry(ANDROID_JPEG_SIZE, jpeg_size);\n>>>> +\tprocessComplete.emit(request, PostProcessor::Status::Success);\n>>>> \n>>>> \treturn 0;\n>>>> }\n>>>> diff --git a/src/android/post_processor.h b/src/android/post_processor.h\n>>>> index 27eaef88..14f5e8c7 100644\n>>>> --- a/src/android/post_processor.h\n>>>> +++ b/src/android/post_processor.h\n>>>> @@ -7,6 +7,8 @@\n>>>> #ifndef __ANDROID_POST_PROCESSOR_H__\n>>>> #define __ANDROID_POST_PROCESSOR_H__\n>>>> \n>>>> +#include \n>>>> +\n>>>> #include \n>>>> #include \n>>>> \n>>>> @@ -17,6 +19,11 @@ class Camera3RequestDescriptor;\n>>>> class PostProcessor\n>>>> {\n>>>> public:\n>>>> +\tenum class Status {\n>>>> +\t\tError,\n>>>> +\t\tSuccess\n>>>> +\t};\n>>>> +\n>>>> \tvirtual ~PostProcessor() = default;\n>>>> \n>>>> \tvirtual int configure(const libcamera::StreamConfiguration &inCfg,\n>>>> @@ -24,6 +31,8 @@ public:\n>>>> \tvirtual int process(const libcamera::FrameBuffer &source,\n>>>> \t\t\t CameraBuffer *destination,\n>>>> \t\t\t Camera3RequestDescriptor *request) = 0;\n>>>> +\n>>>> +\tlibcamera::Signal processComplete;\n>>>> };\n>>>> \n>>>> #endif /* __ANDROID_POST_PROCESSOR_H__ */\n>>>> diff --git a/src/android/yuv/post_processor_yuv.cpp b/src/android/yuv/post_processor_yuv.cpp\n>>>> index 8110a1f1..fd364741 100644\n>>>> --- a/src/android/yuv/post_processor_yuv.cpp\n>>>> +++ b/src/android/yuv/post_processor_yuv.cpp\n>>>> @@ -51,14 +51,17 @@ int PostProcessorYuv::configure(const StreamConfiguration &inCfg,\n>>>> \n>>>> int PostProcessorYuv::process(const FrameBuffer &source,\n>>>> \t\t\t CameraBuffer *destination,\n>>>> -\t\t\t [[maybe_unused]] Camera3RequestDescriptor *request)\n>>>> +\t\t\t Camera3RequestDescriptor *request)\n>>>> {\n>>>> -\tif (!isValidBuffers(source, *destination))\n>>>> +\tif (!isValidBuffers(source, *destination)) {\n>>>> +\t\tprocessComplete.emit(request, PostProcessor::Status::Error);\n>>>> \t\treturn -EINVAL;\n>>>> +\t}\n>>>> \n>>>> \tconst MappedFrameBuffer sourceMapped(&source, MappedFrameBuffer::MapFlag::Read);\n>>>> \tif (!sourceMapped.isValid()) {\n>>>> \t\tLOG(YUV, Error) << \"Failed to mmap camera frame buffer\";\n>>>> +\t\tprocessComplete.emit(request, PostProcessor::Status::Error);\n>>>> \t\treturn -EINVAL;\n>>>> \t}\n>>>> \n>>>> @@ -76,9 +79,12 @@ int PostProcessorYuv::process(const FrameBuffer &source,\n>>>> \t\t\t\t libyuv::FilterMode::kFilterBilinear);\n>>>> \tif (ret) {\n>>>> \t\tLOG(YUV, Error) << \"Failed NV12 scaling: \" << ret;\n>>>> +\t\tprocessComplete.emit(request, PostProcessor::Status::Error);\n>>>> \t\treturn -EINVAL;\n>>>> \t}\n>>>> \n>>>> +\tprocessComplete.emit(request, PostProcessor::Status::Success);\n>>>> +\n>>>> \treturn 0;\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 EEDB6BF415\n\tfor ;\n\tThu, 21 Oct 2021 14:34:26 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 34EE968F57;\n\tThu, 21 Oct 2021 16:34:26 +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 F31E7604FE\n\tfor ;\n\tThu, 21 Oct 2021 16:34:24 +0200 (CEST)","from [192.168.1.106] (unknown [103.238.109.25])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id C96B88B6;\n\tThu, 21 Oct 2021 16:34: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=\"sKEkDL7C\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1634826864;\n\tbh=9UqS5JGMQ+Hdojjf4DE5tTm7wS2g8N8eu34qc7/lDS0=;\n\th=Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=sKEkDL7Co95HADWOoud9dxsV2POfrpP7ZcBXYaTkKIvrMOSPoMU7BzhZ81pnGRvI+\n\tDwTwL4VgTivN8roT1fllWBtqb58kJgtfqNfXzngbikauO9FXB8Zu7PQs+UKaeFkjZJ\n\te3cWILrI2aeOzKhjdj0Ad6kAlulFre1jnlGi4QsI=","To":"Laurent Pinchart ","References":"<20211020104212.121743-1-umang.jain@ideasonboard.com>\n\t<20211020104212.121743-2-umang.jain@ideasonboard.com>\n\t\n\t\n\t","From":"Umang Jain ","Message-ID":"<27967c1d-8ff8-b735-cdb3-154feeaefd57@ideasonboard.com>","Date":"Thu, 21 Oct 2021 20:04:18 +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 v5 1/4] android: Notify post\n\tprocessing completion via a signal","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":20362,"web_url":"https://patchwork.libcamera.org/comment/20362/","msgid":"","date":"2021-10-21T16:01:27","subject":"Re: [libcamera-devel] [PATCH v5 1/4] android: Notify post\n\tprocessing completion via a signal","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, Oct 21, 2021 at 08:04:18PM +0530, Umang Jain wrote:\n> On 10/21/21 7:22 PM, Laurent Pinchart wrote:\n> > On Thu, Oct 21, 2021 at 11:05:44AM +0530, Umang Jain wrote:\n> >> On 10/21/21 6:01 AM, Laurent Pinchart wrote:\n> >>> On Wed, Oct 20, 2021 at 04:12:09PM +0530, Umang Jain wrote:\n> >>>> Notify that the post processing for a request has been completed,\n> >>>> via a signal. A pointer to the descriptor which is tracking the\n> >>>> capture request is emitted along with the status of post processed\n> >>>> buffer. The function CameraDevice::streamProcessingComplete() will\n> >>>> finally set the status on the request descriptor and send capture\n> >>>> results back to the framework accordingly.\n> >>>>\n> >>>> We also need to save a pointer to any internal buffers that might have\n> >>>> been allocated by CameraStream. The buffer should be returned back to\n> >>>> CameraStream just before capture results are sent.\n> >>>>\n> >>>> A streamProcessMutex_ has been introduced here itself, which will be\n> >>> Did you mean s/itself/as well/ ?\n> >>>\n> >>>> applicable to guard access to descriptor->buffers_ when post-processing\n> >>>> is moved to be asynchronous in subsequent commits.\n> >>>>\n> >>>> Signed-off-by: Umang Jain \n> >>>> ---\n> >>>> src/android/camera_device.cpp | 92 +++++++++++++++++++++---\n> >>>> src/android/camera_device.h | 7 ++\n> >>>> src/android/camera_request.h | 4 ++\n> >>>> src/android/camera_stream.cpp | 13 ++++\n> >>>> src/android/jpeg/post_processor_jpeg.cpp | 2 +\n> >>>> src/android/post_processor.h | 9 +++\n> >>>> src/android/yuv/post_processor_yuv.cpp | 10 ++-\n> >>>> 7 files changed, 125 insertions(+), 12 deletions(-)\n> >>>>\n> >>>> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> >>>> index 806b4090..541c2c81 100644\n> >>>> --- a/src/android/camera_device.cpp\n> >>>> +++ b/src/android/camera_device.cpp\n> >>>> @@ -1117,6 +1117,15 @@ void CameraDevice::requestComplete(Request *request)\n> >>>> \t}\n> >>>> \n> >>>> \t/* Handle post-processing. */\n> >>>> +\tbool needsPostProcessing = false;\n> >>>> +\tCamera3RequestDescriptor::Status processingStatus =\n> >>>> +\t\tCamera3RequestDescriptor::Status::Pending;\n> >>>> +\t/*\n> >>>> +\t * \\todo Apply streamsProcessMutex_ when post-processing is adapted to run\n> >>>> +\t * asynchronously. If we introduce the streamsProcessMutex_ right away, the\n> >>>> +\t * lock will be held forever since it is synchronous at this point\n> >>>> +\t * (see streamProcessingComplete).\n> >>>> +\t */\n> >>>> \tfor (auto &buffer : descriptor->buffers_) {\n> >>>> \t\tCameraStream *stream = buffer.stream;\n> >>>> \n> >>>> @@ -1132,22 +1141,27 @@ void CameraDevice::requestComplete(Request *request)\n> >>>> \t\t\tcontinue;\n> >>>> \t\t}\n> >>>> \n> >>>> -\t\tint ret = stream->process(*src, 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 (stream->type() == CameraStream::Type::Internal)\n> >>>> -\t\t\tstream->putBuffer(src);\n> >>>> +\t\t\tbuffer.internalBuffer = src;\n> >>>\n> >>> Let's make the field name more self-explanatory. A possible candidate is\n> >>> sourceBuffer instead of internalBuffer.\n> >>>\n> >>> Could we do this in CameraDevice::processCaptureRequest(), just after\n> >>> calling cameraStream->getBuffer() ? I'll feel less worried about a leak\n> >>> if we store the buffer for later release right after acquiring it.\n> >>\n> >> Ack.\n> >>\n> >>>> \n> >>>> +\t\tneedsPostProcessing = true;\n> >>>> +\t\tint ret = stream->process(*src, buffer, descriptor);\n> >>>> \t\tif (ret) {\n> >>>> -\t\t\tbuffer.status = Camera3RequestDescriptor::Status::Error;\n> >>>> -\t\t\tnotifyError(descriptor->frameNumber_, stream->camera3Stream(),\n> >>>> -\t\t\t\t CAMERA3_MSG_ERROR_BUFFER);\n> >>>> +\t\t\tsetBufferStatus(stream, buffer, descriptor,\n> >>>> +\t\t\t\t\tCamera3RequestDescriptor::Status::Error);\n> >>>> +\t\t\tprocessingStatus = Camera3RequestDescriptor::Status::Error;\n> >>>> \t\t}\n> >>>\n> >>> I think we need to improve error handling a little bit, in order to\n> >>> support multiple streams being post-processed. We need to complete the\n> >>> request in CameraDevice::streamProcessingComplete() when the last\n> >>> processing stream completes, so the number of post-processed streams\n> >>> need to be somehow recorded in the Camera3RequestDescriptor. It could be\n> >>> tempted to have a counter, but a\n> >>\n> >> I have implemented a counter based solution, it still didn't address all\n> >> the corner cases. Since we have two  path of error handling (synchronous\n> >> error and asynchronous error), a counter based implementation was\n> >> probably too verbose with much of code duplication to handle various\n> >> scenarios.\n> >>\n> >>> \tstd::map pendingPostProcess_;\n> >>>\n> >>> could be better, as it will also remove the need for loops in\n> >>> streamProcessingComplete() to lookup the buffer from the stream and to\n> >>> check if the request has been fully processed. Accessing the map has to\n> >>> be done with the mutex held to avoid race conditions, but it's a bit\n> >>> more tricky than that. If we just add elements to the map here, we could\n> >>\n> >> Good, so you do understand so many looping lookups in\n> >> streamProcessingComplete.\n> >>\n> >> I broadly feel okay with std::map\n> >> pendingPostProcess_; as long as it resides inside the descriptor. For\n> >> me, it avoids lookups, and can address the \\todo in 3/4.\n> >>\n> >> Having said that, I feel a bit wary of introducing more data structures\n> >> for management. We already have a queue of post-processing (convert to\n> >> std:deque if we want to access the queued requests for post-processing)\n> >> and now we will have another map to tracking the requests. I wonder if\n> >> we can get away with a PostProcessorWorker's std::deque, with the\n> >> benefits of the map. But again, we have to take care of exposure / mutex\n> >> with main thread, which can lead to subtle bugs and unwanted waiting on\n> >> each thread\n> >>\n> >> Now I think about it, the pendingPostProcess_ map is about Stream's\n> >> pending processing _per descriptor_, whereas the PostProcessorWorker's\n> >> queue is replacement for Thread's message queue. Hmm.\n> >\n> > That's right, those two data structures are independent from each other.\n> >\n> >>> race with streamProcessingComplete() as it could be called for the first\n> >>> post-processed stream before we have a chance to add the second one\n> >>> here. streamProcessingComplete() would then incorrectly think it has\n> >>> completed the last stream, and proceed to complete the request.\n> >>>\n> >>> This race could be solved by holding the lock for the whole duration of\n> >>> the loop here, covering all calls to stream->process(), but it may be\n> >>\n> >> Yes,  I am considering this in current implementation too. The way it\n> >> will look like, is first queue up all the post-processing requests to\n> >> PostProcessorWoe, only then, allow any of the\n> >> slot(streamProcessingComplete) to get executed. A simple mutex hold can\n> >> do that\n> >\n> > I usually try to minimize the amount of code covered by locks, and focus\n> > on data access only if possible instead of keeping the lock held over\n> > the callbacks. Let's see if that's possible. There's a tricky part\n> > though, as sendCaptureResults() is called from two different threads\n> > (synchronously with request queuing in error paths and asynchronously in\n> > the completion path), we currently have a race condition.\n> >\n> > Thread A\t\t\t\t\t\t\tThread B\n> > -----------------------------------------------------------------------------------------------------------\n> >\n> > MutexLocker lock(descriptorsMutex_);\n> > while (!descriptors_.empty() &&\n> > !descriptors_.front()->isPending()) {\n> > auto descriptor = std::move(descriptors_.front());\n> > descriptors_.pop();\n> >\n> > lock.unlock();\n> > \t\t\t\t\t\t\t\tMutexLocker lock(descriptorsMutex_);\n> > \t\t\t\t\t\t\t\twhile (!descriptors_.empty() &&\n> > \t\t\t\t\t\t\t\t !descriptors_.front()->isPending()) {\n> > \t\t\t\t\t\t\t\t\tauto descriptor = std::move(descriptors_.front());\n> > \t\t\t\t\t\t\t\t\tdescriptors_.pop();\n> >\n> > \t\t\t\t\t\t\t\t\tlock.unlock();\n> >\n> > \t\t\t\t\t\t\t\t\t...\n> >\n> > \t\t\t\t\t\t\t\t\tcallbacks_->process_capture_result(callbacks_,\n> > \t\t\t\t\t\t\t\t\t\t\t\t\t &captureResult);\n> >\n> > \t\t\t\t\t\t\t\t\tlock.lock();\n> > \t\t\t\t\t\t\t\t}\n> > \t...\n> >\n> > callbacks_->process_capture_result(callbacks_,\n> > \t\t\t\t\t &captureResult);\n> >\n> > lock.lock();\n> > }\n> >\n> > Thread A gets the request at the top of the queue, thread B gets the\n> > next one, and signals its completion before thread A.\n> >\n> > It's likely possible to implement something clever here, to minimize the\n> > locked sections and still be correct, but that may be unnecessary\n> > optimization. We could start by holding the lock for the whole duration\n> > of the function. In that case, I would remove the lock from\n> > sendCaptureResults() completely, and call the function with the lock\n> > held in completeDescriptor().\n> \n> Yes, provided sendCaptureResults() is only called completeDescriptor().\n\nI think that's the direction we're taking (we could even consider\nmerging the two functions together, but I'm not sure that would be a\ngood idea).\n\n> I don't want to get a implementation which uses completeDescriptor() and \n> sendCaptureResults() both for process_capture_result() callback.\n> \n> >>> possible to do better. We could add entries to the map in\n> >>> processCaptureRequest() instead. We would need, here, to remove entries\n> >>\n> >> I am not so sure about this, We could add entries in\n> >> processCaptureRequest() but it feels too early. Let me try to address\n> >> other comments, and I can iterate on this on top.\n> >>\n> >>> from the map if stream->process() fails, and complete the request if we\n> >>> remove the last entry (as otherwise, streamProcessingComplete() could\n> >>> complete all streams, then the last stream->process() call could fail,\n> >>> and nobody would complete the request).\n> >>\n> >> This is still a bit tricky to handle. What's the de-facto criteria to\n> >> remove entries from the map (not just the error path) ?\n> >>\n> >> Assuming we hold the lock for entire duration of\n> >>\n> >>     Locker.lock();\n> >>\n> >>     for {\n> >>         ...\n> >>         int ret = stream->process(...);\n> >>         if (ret) {\n> >>             descriptor.pendingPostProcess_.erase(stream);\n> >>             setBufferStatus(stream, error);\n> >>\n> >>             if (descriptor.pendingPostProcess_.size() == 0)\n> >>                 completeDescriptor (error);\n> >>\n> >>         }\n> >>\n> >>     }\n> >>     locker.unlock();\n> >>\n> >> ....\n> >>\n> >> streamProcessingComplete(..., bufferStatus)\n> >>\n> >> {\n> >>\n> >>     Locker.lock();\n> >>\n> >> descriptor.pendingPostProcess_.erase(stream);\n> >>     setBufferStatus(stream, bufferStatus);\n> >>\n> >>     if (descriptor.pendingPostProcess_.size() == 0)\n> >>         completeDescriptor (?);\n> >>\n> >> }\n> >>\n> >> the ? indicates with what status I should complete the descriptor? I\n> >> would probably need to iterate over all the descriptor's buffer status\n> >> to see if any of the (previous)buffer has failed to set error status on\n> >> the descriptor overall? I think so, we would still need a look up loop\n> >> here, I can't see deciphering this from the map.\n> >\n> > Can we record a post-processing status in the descriptor, setting it to\n> > success when constructor, and setting it to error when a post-processing\n> > fails ? Then you can inspect that state only when completing the\n> > request. A boolean flag (hasPostProcessingErrors for instance) could be\n> > enough.\n> \n> I would take the boolean flag approach for now.\n> \n> > Or we could reuse the status_ member variable for that. It's currently\n> > used both to tell if a descriptor is complete (when !pending) and what\n> > its status is. We could split that into a status flag that would only\n> > have success and error values, and a pending (or complete) boolean. This\n> > may be a better design, as it would allow us to update the request\n> > status as soon as an error is detected, and set the completion flag\n> > separately only when all processing has completed.\n> \n> This is sane, which in interest of not increasing further scope of the \n> series, I am tempted to address this on top. I can record it with a \\todo\n\nI'm OK with that if you think it would be too disruptive to make this\nchange at the bottom of the series, but I wonder if the rest wouldn't be\ncleaner and clearer if implemented on top of this (hopefully) simple\nchange.\n\n> >> Are we getting too complicated with this? I have started to feel so ...\n> >>\n> >>> Other designs may be possible.\n> >>>\n> >>>> \t}\n> >>>> \n> >>>> +\tif (needsPostProcessing) {\n> >>>> +\t\tif (processingStatus == Camera3RequestDescriptor::Status::Error) {\n> >>>> +\t\t\tdescriptor->status_ = processingStatus;\n> >>>> +\t\t\tsendCaptureResults();\n> >>>> +\t\t}\n> >>>> +\n> >>>> +\t\treturn;\n> >>>> +\t}\n> >>>> +\n> >>>> \tdescriptor->status_ = Camera3RequestDescriptor::Status::Success;\n> >>>> \tsendCaptureResults();\n> >>>> }\n> >>>> @@ -1206,6 +1220,64 @@ void CameraDevice::sendCaptureResults()\n> >>>> \t}\n> >>>> }\n> >>>> \n> >>>> +void CameraDevice::setBufferStatus(CameraStream *cameraStream,\n> >>>> +\t\t\t\t Camera3RequestDescriptor::StreamBuffer &buffer,\n> >>>> +\t\t\t\t Camera3RequestDescriptor *request,\n> >>>> +\t\t\t\t Camera3RequestDescriptor::Status status)\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(buffer.internalBuffer);\n> >>>> +\n> >>>> +\tbuffer.status = status;\n> >>>> +\tif (status != Camera3RequestDescriptor::Status::Success)\n> >>>> +\t\tnotifyError(request->frameNumber_, buffer.stream->camera3Stream(),\n> >>>> +\t\t\t CAMERA3_MSG_ERROR_BUFFER);\n> >>>> +}\n> >>>> +\n> >>>> +void CameraDevice::streamProcessingComplete(CameraStream *cameraStream,\n> >>>> +\t\t\t\t\t Camera3RequestDescriptor *request,\n> >>>> +\t\t\t\t\t Camera3RequestDescriptor::Status status)\n> >>>> +{\n> >>>> +\tMutexLocker locker(request->streamsProcessMutex_);\n> >>>> +\tfor (auto &buffer : request->buffers_) {\n> >>>> +\t\tif (buffer.stream != cameraStream)\n> >>>> +\t\t\tcontinue;\n> >>>> +\n> >>>> +\t\tsetBufferStatus(cameraStream, buffer, request, status);\n> >>>> +\t}\n> >>>> +\n> >>>> +\tbool hasPostProcessingErrors = false;\n> >>>> +\tfor (auto &buffer : request->buffers_) {\n> >>>> +\t\tif (cameraStream->type() == CameraStream::Type::Direct)\n> >>>> +\t\t\tcontinue;\n> >>>> +\n> >>>> +\t\t/*\n> >>>> +\t\t * Other eligible buffers might be waiting to get post-processed.\n> >>>> +\t\t * So wait for their turn before sendCaptureResults() for the\n> >>>> +\t\t * descriptor.\n> >>>> +\t\t */\n> >>>> +\t\tif (buffer.status == Camera3RequestDescriptor::Status::Pending)\n> >>>> +\t\t\treturn;\n> >>>> +\n> >>>> +\t\tif (!hasPostProcessingErrors &&\n> >>>> +\t\t buffer.status == Camera3RequestDescriptor::Status::Error)\n> >>>> +\t\t\thasPostProcessingErrors = true;\n> >>>> +\t}\n> >>>> +\n> >>>> +\tif (hasPostProcessingErrors)\n> >>>> +\t\trequest->status_ = Camera3RequestDescriptor::Status::Error;\n> >>>> +\telse\n> >>>> +\t\trequest->status_ = Camera3RequestDescriptor::Status::Success;\n> >>>> +\n> >>>> +\tlocker.unlock();\n> >>>> +\n> >>>> +\tsendCaptureResults();\n> >>>> +}\n> >>>> +\n> >>>> std::string CameraDevice::logPrefix() const\n> >>>> {\n> >>>> \treturn \"'\" + camera_->id() + \"'\";\n> >>>> diff --git a/src/android/camera_device.h b/src/android/camera_device.h\n> >>>> index 863cf414..1ef933da 100644\n> >>>> --- a/src/android/camera_device.h\n> >>>> +++ b/src/android/camera_device.h\n> >>>> @@ -66,6 +66,9 @@ public:\n> >>>> \tint configureStreams(camera3_stream_configuration_t *stream_list);\n> >>>> \tint processCaptureRequest(camera3_capture_request_t *request);\n> >>>> \tvoid requestComplete(libcamera::Request *request);\n> >>>> +\tvoid streamProcessingComplete(CameraStream *cameraStream,\n> >>>> +\t\t\t\t Camera3RequestDescriptor *request,\n> >>>> +\t\t\t\t Camera3RequestDescriptor::Status status);\n> >>>> \n> >>>> protected:\n> >>>> \tstd::string logPrefix() const override;\n> >>>> @@ -94,6 +97,10 @@ private:\n> >>>> \t\t\t camera3_error_msg_code code) const;\n> >>>> \tint processControls(Camera3RequestDescriptor *descriptor);\n> >>>> \tvoid sendCaptureResults();\n> >>>> +\tvoid setBufferStatus(CameraStream *cameraStream,\n> >>>> +\t\t\t Camera3RequestDescriptor::StreamBuffer &buffer,\n> >>>> +\t\t\t Camera3RequestDescriptor *request,\n> >>>> +\t\t\t Camera3RequestDescriptor::Status status);\n> >>>> \tstd::unique_ptr getResultMetadata(\n> >>>> \t\tconst Camera3RequestDescriptor &descriptor) const;\n> >>>> \n> >>>> diff --git a/src/android/camera_request.h b/src/android/camera_request.h\n> >>>> index 05dabf89..3a2774e0 100644\n> >>>> --- a/src/android/camera_request.h\n> >>>> +++ b/src/android/camera_request.h\n> >>>> @@ -8,6 +8,7 @@\n> >>>> #define __ANDROID_CAMERA_REQUEST_H__\n> >>>> \n> >>>> #include \n> >>>> +#include \n> >>>> #include \n> >>>> \n> >>>> #include \n> >>>> @@ -37,6 +38,7 @@ public:\n> >>>> \t\tstd::unique_ptr frameBuffer;\n> >>>> \t\tint fence;\n> >>>> \t\tStatus status;\n> >>>> +\t\tlibcamera::FrameBuffer *internalBuffer = nullptr;\n> >>>> \t};\n> >>>> \n> >>>> \tCamera3RequestDescriptor(libcamera::Camera *camera,\n> >>>> @@ -47,6 +49,8 @@ public:\n> >>>> \n> >>>> \tuint32_t frameNumber_ = 0;\n> >>>> \n> >>>> +\t/* Protects buffers_ for post-processing streams. */\n> >>>> +\tstd::mutex streamsProcessMutex_;\n> >>>> \tstd::vector buffers_;\n> >>>> \n> >>>> \tCameraMetadata settings_;\n> >>>> diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp\n> >>>> index f44a2717..04cbef8c 100644\n> >>>> --- a/src/android/camera_stream.cpp\n> >>>> +++ b/src/android/camera_stream.cpp\n> >>>> @@ -22,6 +22,7 @@\n> >>>> #include \"camera_capabilities.h\"\n> >>>> #include \"camera_device.h\"\n> >>>> #include \"camera_metadata.h\"\n> >>>> +#include \"post_processor.h\"\n> >>>> \n> >>>> using namespace libcamera;\n> >>>> \n> >>>> @@ -97,6 +98,18 @@ int CameraStream::configure()\n> >>>> \t\tint ret = postProcessor_->configure(configuration(), output);\n> >>>> \t\tif (ret)\n> >>>> \t\t\treturn ret;\n> >>>> +\n> >>>> +\t\tpostProcessor_->processComplete.connect(\n> >>>> +\t\t\tthis, [&](Camera3RequestDescriptor *request, PostProcessor::Status status) {\n> >>>> +\t\t\t\tCamera3RequestDescriptor::Status bufferStatus =\n> >>>> +\t\t\t\t\tCamera3RequestDescriptor::Status::Error;\n> >>>> +\n> >>>> +\t\t\t\tif (status == PostProcessor::Status::Success)\n> >>>> +\t\t\t\t\tbufferStatus = Camera3RequestDescriptor::Status::Success;\n> >>>\n> >>> It looks weird to make Error a default (and thus special) case. Would\n> >>> any of the following be better ?\n> >>>\n> >>> \t\t\t\tCamera3RequestDescriptor::Status bufferStatus;\n> >>>\n> >>> \t\t\t\tif (status == PostProcessor::Status::Success)\n> >>> \t\t\t\t\tbufferStatus = Camera3RequestDescriptor::Status::Success;\n> >>> \t\t\t\telse\n> >>> \t\t\t\t\tbufferStatus = Camera3RequestDescriptor::Status::Error;\n> >>>\n> >>>\n> >>> \t\t\t\tCamera3RequestDescriptor::Status bufferStatus =\n> >>> \t\t\t\t\tstatus == PostProcessor::Status::Success ?\n> >>> \t\t\t\t\tCamera3RequestDescriptor::Status::Success :\n> >>> \t\t\t\t\tCamera3RequestDescriptor::Status::Error;\n> >>>\n> >>> I think I like the second option best.\n> >>>\n> >>>> +\n> >>>> +\t\t\t\tcameraDevice_->streamProcessingComplete(this, request,\n> >>>> +\t\t\t\t\t\t\t\t\tbufferStatus);\n> >>>> +\t\t\t});\n> >>>> \t}\n> >>>> \n> >>>> \tif (type_ == Type::Internal) {\n> >>>> diff --git a/src/android/jpeg/post_processor_jpeg.cpp b/src/android/jpeg/post_processor_jpeg.cpp\n> >>>> index 699576ef..a001fede 100644\n> >>>> --- a/src/android/jpeg/post_processor_jpeg.cpp\n> >>>> +++ b/src/android/jpeg/post_processor_jpeg.cpp\n> >>>> @@ -198,6 +198,7 @@ int PostProcessorJpeg::process(const FrameBuffer &source,\n> >>>> \t\t\t\t\t exif.data(), quality);\n> >>>> \tif (jpeg_size < 0) {\n> >>>> \t\tLOG(JPEG, Error) << \"Failed to encode stream image\";\n> >>>> +\t\tprocessComplete.emit(request, PostProcessor::Status::Error);\n> >>>\n> >>> I think you can write Status::Error here and below, including in\n> >>> post_processor_yuv.cpp (not above in camera_stream.cpp though).\n> >>>\n> >>>> \t\treturn jpeg_size;\n> >>>> \t}\n> >>>> \n> >>>> @@ -211,6 +212,7 @@ int PostProcessorJpeg::process(const FrameBuffer &source,\n> >>>> \n> >>>> \t/* Update the JPEG result Metadata. */\n> >>>> \tresultMetadata->addEntry(ANDROID_JPEG_SIZE, jpeg_size);\n> >>>> +\tprocessComplete.emit(request, PostProcessor::Status::Success);\n> >>>> \n> >>>> \treturn 0;\n> >>>> }\n> >>>> diff --git a/src/android/post_processor.h b/src/android/post_processor.h\n> >>>> index 27eaef88..14f5e8c7 100644\n> >>>> --- a/src/android/post_processor.h\n> >>>> +++ b/src/android/post_processor.h\n> >>>> @@ -7,6 +7,8 @@\n> >>>> #ifndef __ANDROID_POST_PROCESSOR_H__\n> >>>> #define __ANDROID_POST_PROCESSOR_H__\n> >>>> \n> >>>> +#include \n> >>>> +\n> >>>> #include \n> >>>> #include \n> >>>> \n> >>>> @@ -17,6 +19,11 @@ class Camera3RequestDescriptor;\n> >>>> class PostProcessor\n> >>>> {\n> >>>> public:\n> >>>> +\tenum class Status {\n> >>>> +\t\tError,\n> >>>> +\t\tSuccess\n> >>>> +\t};\n> >>>> +\n> >>>> \tvirtual ~PostProcessor() = default;\n> >>>> \n> >>>> \tvirtual int configure(const libcamera::StreamConfiguration &inCfg,\n> >>>> @@ -24,6 +31,8 @@ public:\n> >>>> \tvirtual int process(const libcamera::FrameBuffer &source,\n> >>>> \t\t\t CameraBuffer *destination,\n> >>>> \t\t\t Camera3RequestDescriptor *request) = 0;\n> >>>> +\n> >>>> +\tlibcamera::Signal processComplete;\n> >>>> };\n> >>>> \n> >>>> #endif /* __ANDROID_POST_PROCESSOR_H__ */\n> >>>> diff --git a/src/android/yuv/post_processor_yuv.cpp b/src/android/yuv/post_processor_yuv.cpp\n> >>>> index 8110a1f1..fd364741 100644\n> >>>> --- a/src/android/yuv/post_processor_yuv.cpp\n> >>>> +++ b/src/android/yuv/post_processor_yuv.cpp\n> >>>> @@ -51,14 +51,17 @@ int PostProcessorYuv::configure(const StreamConfiguration &inCfg,\n> >>>> \n> >>>> int PostProcessorYuv::process(const FrameBuffer &source,\n> >>>> \t\t\t CameraBuffer *destination,\n> >>>> -\t\t\t [[maybe_unused]] Camera3RequestDescriptor *request)\n> >>>> +\t\t\t Camera3RequestDescriptor *request)\n> >>>> {\n> >>>> -\tif (!isValidBuffers(source, *destination))\n> >>>> +\tif (!isValidBuffers(source, *destination)) {\n> >>>> +\t\tprocessComplete.emit(request, PostProcessor::Status::Error);\n> >>>> \t\treturn -EINVAL;\n> >>>> +\t}\n> >>>> \n> >>>> \tconst MappedFrameBuffer sourceMapped(&source, MappedFrameBuffer::MapFlag::Read);\n> >>>> \tif (!sourceMapped.isValid()) {\n> >>>> \t\tLOG(YUV, Error) << \"Failed to mmap camera frame buffer\";\n> >>>> +\t\tprocessComplete.emit(request, PostProcessor::Status::Error);\n> >>>> \t\treturn -EINVAL;\n> >>>> \t}\n> >>>> \n> >>>> @@ -76,9 +79,12 @@ int PostProcessorYuv::process(const FrameBuffer &source,\n> >>>> \t\t\t\t libyuv::FilterMode::kFilterBilinear);\n> >>>> \tif (ret) {\n> >>>> \t\tLOG(YUV, Error) << \"Failed NV12 scaling: \" << ret;\n> >>>> +\t\tprocessComplete.emit(request, PostProcessor::Status::Error);\n> >>>> \t\treturn -EINVAL;\n> >>>> \t}\n> >>>> \n> >>>> +\tprocessComplete.emit(request, PostProcessor::Status::Success);\n> >>>> +\n> >>>> \treturn 0;\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 0BB7CBDB1C\n\tfor ;\n\tThu, 21 Oct 2021 16:01:49 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4D9E168F56;\n\tThu, 21 Oct 2021 18:01:48 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 70E93604FE\n\tfor ;\n\tThu, 21 Oct 2021 18:01: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 CFEF48B6;\n\tThu, 21 Oct 2021 18:01:46 +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=\"wiVop8kB\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1634832107;\n\tbh=QiPa3UdqR2kNaTyjyTl3v9oTEOBtImO9kOvvbm0mzyk=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=wiVop8kBymHzVauxDvwJzmpcDzEvSvzBQYUHKpn5nQ7tP3awZ8LmAYKbDrsb08jBZ\n\t3DiRnDA7pXN7lRomPndvSHdwrlC0rDA5GKixjba59GHPWlrqhNcqk+I3uRq0Y6Kz//\n\ta9qVLp/5kIncJpyanIQSYkN40nUUOymsfS5Bt3Zk=","Date":"Thu, 21 Oct 2021 19:01:27 +0300","From":"Laurent Pinchart ","To":"Umang Jain ","Message-ID":"","References":"<20211020104212.121743-1-umang.jain@ideasonboard.com>\n\t<20211020104212.121743-2-umang.jain@ideasonboard.com>\n\t\n\t\n\t\n\t<27967c1d-8ff8-b735-cdb3-154feeaefd57@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<27967c1d-8ff8-b735-cdb3-154feeaefd57@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v5 1/4] android: Notify post\n\tprocessing completion via a signal","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\" "}}]