[{"id":20413,"web_url":"https://patchwork.libcamera.org/comment/20413/","msgid":"","date":"2021-10-25T06:03:35","subject":"Re: [libcamera-devel] [PATCH v6 5/7] android: Track and notify post\n\tprocessing of streams","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 Sat, Oct 23, 2021 at 04:03:00PM +0530, Umang Jain wrote:\n> Notify that the post processing for a request has been completed,\n> via a signal. The signal emit with a context pointer along with status\n\ns/emit/is emitted/ ?\n\n> of the buffer. The function CameraDevice::streamProcessingComplete() will\n> finally set the status on the request descriptor and complete the\n> descriptor if all the streams requiring post processing are completed.\n> If buffer status obtained is in error state, notify the status to the\n> framework and set the overall error status on the descriptor via\n> setBufferStatus().\n> \n> We need to track the number of streams requiring post-processing\n> per Camera3RequestDescriptor (i.e. per capture request). Introduce\n> a std::map<> to track the post-processing of streams. The nodes\n> are dropped from the map when a particular stream post processing\n> is completed (or on error paths). A std::map is selected for tracking\n> post-processing requests, since we will move post-processing to be\n> asynchronous in subsequent commits. A vector or queue will not be\n> suitable then as the sequential order of post-processing completion\n> of various requests won't be guaranteed then.\n> \n> A streamProcessMutex_ has been introduced here as well, which will be\n\ns/streamProcessMutex_/streamsProcessMutex_/\n\n> applicable to guard access to descriptor's pendingStreamsToProcess_ when\n> post-processing is moved to be asynchronous in subsequent commits.\n> \n> Signed-off-by: Umang Jain \n> ---\n> src/android/camera_device.cpp | 95 ++++++++++++++++++------\n> src/android/camera_device.h | 4 +\n> src/android/camera_request.h | 6 ++\n> src/android/camera_stream.cpp | 15 ++++\n> src/android/jpeg/post_processor_jpeg.cpp | 2 +\n> src/android/post_processor.h | 9 +++\n> src/android/yuv/post_processor_yuv.cpp | 8 +-\n> 7 files changed, 114 insertions(+), 25 deletions(-)\n> \n> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> index 2a98a2e6..3114def0 100644\n> --- a/src/android/camera_device.cpp\n> +++ b/src/android/camera_device.cpp\n> @@ -926,6 +926,8 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n> \t\t\t * Request.\n> \t\t\t */\n> \t\t\tLOG(HAL, Debug) << ss.str() << \" (mapped)\";\n\nBlank line here, or no blank line below.\n\n> +\t\t\tdescriptor->pendingStreamsToProcess_.insert(\n> +\t\t\t\t{ cameraStream, &buffer });\n> \t\t\tcontinue;\n> \n> \t\tcase CameraStream::Type::Direct:\n> @@ -955,6 +957,9 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n> \t\t\tframeBuffer = cameraStream->getBuffer();\n> \t\t\tbuffer.internalBuffer = frameBuffer;\n> \t\t\tLOG(HAL, Debug) << ss.str() << \" (internal)\";\n> +\n> +\t\t\tdescriptor->pendingStreamsToProcess_.insert(\n> +\t\t\t\t{ cameraStream, &buffer });\n> \t\t\tbreak;\n> \t\t}\n> \n> @@ -1118,43 +1123,46 @@ void CameraDevice::requestComplete(Request *request)\n> \t}\n> \n> \t/* Handle post-processing. */\n> -\tbool hasPostProcessingErrors = false;\n> -\tfor (auto &buffer : descriptor->buffers_) {\n> -\t\tCameraStream *stream = buffer.stream;\n> -\n> -\t\tif (stream->type() == CameraStream::Type::Direct)\n> -\t\t\tcontinue;\n> +\tbool needPostProcessing = false;\n> +\t/*\n> +\t * \\todo Protect the loop below with streamProcessMutex_ when post\n> +\t * processor runs asynchronously.\n> +\t */\n> +\tauto iter = descriptor->pendingStreamsToProcess_.begin();\n> +\twhile (descriptor->pendingStreamsToProcess_.size() > 0) {\n\n\twhile (!descriptor->pendingStreamsToProcess_.empty()) {\n\nBut it's not correct. The fix is in 7/7:\n\n\twhile (iter != descriptor->pendingStreamsToProcess_.end()) {\n\n> +\t\tCameraStream *stream = iter->first;\n> +\t\tCamera3RequestDescriptor::StreamBuffer *buffer = iter->second;\n> +\t\tneedPostProcessing = true;\n> \n> \t\tFrameBuffer *src = request->findBuffer(stream->stream());\n> \t\tif (!src) {\n> \t\t\tLOG(HAL, Error) << \"Failed to find a source stream buffer\";\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\thasPostProcessingErrors = true;\n> +\t\t\tsetBufferStatus(*buffer, Camera3RequestDescriptor::Status::Error);\n> +\t\t\titer = descriptor->pendingStreamsToProcess_.erase(iter);\n> \t\t\tcontinue;\n> \t\t}\n> \n> -\t\tint ret = stream->process(*src, buffer);\n> +\t\t++iter;\n> +\t\tint ret = stream->process(*src, *buffer);\n> +\t\tif (ret) {\n> +\t\t\tsetBufferStatus(*buffer, Camera3RequestDescriptor::Status::Error);\n> +\t\t\tdescriptor->pendingStreamsToProcess_.erase(stream);\n> +\t\t}\n> +\t}\n> \n> +\tif (needPostProcessing) {\n> \t\t/*\n> -\t\t * If the framebuffer is internal to CameraStream return it back\n> -\t\t * now that we're done processing it.\n> +\t\t * \\todo We will require to check if we failed to queue\n> +\t\t * post-processing requests when we migrate to post-processor\n> +\t\t * running asynchronously.\n> +\t\t *\n> +\t\t * if (descriptor->pendingStreamsToProcess_.size() == 0)\n> +\t\t *\tcompleteDescriptor(descriptor);\n\nCan't we do this already here ? I think you can actually drop the\nneedPostProcessing variable and just write\n\n\tif (descriptor->pendingStreamsToProcess_.empty())\n\t\tcompleteDescriptor(descriptor);\n\nas needPostProcessing can only be false if pendingStreamsToProcess_ was\nempty before the while loop above, and it will thus be empty after the\nloop as well in that case.\n\n> \t\t */\n> -\t\tif (buffer.internalBuffer)\n> -\t\t\tstream->putBuffer(buffer.internalBuffer);\n> \n> -\t\tif (ret) {\n> -\t\t\tbuffer.status = Camera3RequestDescriptor::Status::Error;\n> -\t\t\thasPostProcessingErrors = true;\n> -\t\t\tnotifyError(descriptor->frameNumber_, stream->camera3Stream(),\n> -\t\t\t\t CAMERA3_MSG_ERROR_BUFFER);\n> -\t\t}\n> +\t\treturn;\n> \t}\n> \n> -\tif (hasPostProcessingErrors)\n> -\t\tdescriptor->status_ = Camera3RequestDescriptor::Status::Error;\n> -\n> \tcompleteDescriptor(descriptor);\n> }\n> \n> @@ -1210,6 +1218,45 @@ void CameraDevice::sendCaptureResults()\n> \t}\n> }\n> \n> +void CameraDevice::setBufferStatus(Camera3RequestDescriptor::StreamBuffer &streamBuffer,\n> +\t\t\t\t Camera3RequestDescriptor::Status status)\n> +{\n> +\t/*\n> +\t * If the framebuffer is internal to CameraStream return it back now\n> +\t * that we're done processing it.\n> +\t */\n> +\tif (streamBuffer.internalBuffer)\n> +\t\tstreamBuffer.stream->putBuffer(streamBuffer.internalBuffer);\n\nI'd move this to the caller, as it's not about the buffer status.\n\n> +\n> +\tstreamBuffer.status = status;\n> +\tif (status != Camera3RequestDescriptor::Status::Success) {\n> +\t\tnotifyError(streamBuffer.request->frameNumber_,\n> +\t\t\t streamBuffer.stream->camera3Stream(),\n> +\t\t\t CAMERA3_MSG_ERROR_BUFFER);\n> +\n> +\t\t/* Also set error status on entire request descriptor. */\n> +\t\tstreamBuffer.request->status_ =\n> +\t\t\tCamera3RequestDescriptor::Status::Error;\n> +\t}\n> +}\n> +\n> +void CameraDevice::streamProcessingComplete(Camera3RequestDescriptor::StreamBuffer *streamBuffer,\n> +\t\t\t\t\t Camera3RequestDescriptor::Status status)\n> +{\n> +\tCamera3RequestDescriptor *request = streamBuffer->request;\n> +\tMutexLocker locker(request->streamsProcessMutex_);\n> +\n> +\tsetBufferStatus(*streamBuffer, status);\n\nDo we need to protect the setBufferStatus() call with\nstreamsProcessMutex_ ? I thought it only protects\npendingStreamsToProcess_.\n\n> +\trequest->pendingStreamsToProcess_.erase(streamBuffer->stream);\n> +\n> +\tif (request->pendingStreamsToProcess_.size() > 0)\n\n\tif (!request->pendingStreamsToProcess_.empty())\n\n> +\t\treturn;\n> +\n> +\tlocker.unlock();\n> +\n> +\tcompleteDescriptor(streamBuffer->request);\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 e544f2bd..2a414020 100644\n> --- a/src/android/camera_device.h\n> +++ b/src/android/camera_device.h\n> @@ -66,6 +66,8 @@ 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(Camera3RequestDescriptor::StreamBuffer *bufferStream,\n> +\t\t\t\t Camera3RequestDescriptor::Status status);\n> \n> protected:\n> \tstd::string logPrefix() const override;\n> @@ -95,6 +97,8 @@ private:\n> \tint processControls(Camera3RequestDescriptor *descriptor);\n> \tvoid completeDescriptor(Camera3RequestDescriptor *descriptor);\n> \tvoid sendCaptureResults();\n> +\tvoid setBufferStatus(Camera3RequestDescriptor::StreamBuffer &buffer,\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 c4bc5d6e..cc2b7035 100644\n> --- a/src/android/camera_request.h\n> +++ b/src/android/camera_request.h\n> @@ -7,7 +7,9 @@\n> #ifndef __ANDROID_CAMERA_REQUEST_H__\n> #define __ANDROID_CAMERA_REQUEST_H__\n> \n> +#include \n> #include \n> +#include \n> #include \n> \n> #include \n> @@ -43,6 +45,10 @@ public:\n> \t\tCamera3RequestDescriptor *request;\n> \t};\n> \n> +\t/* Keeps track of streams requiring post-processing. */\n> +\tstd::map pendingStreamsToProcess_;\n> +\tstd::mutex streamsProcessMutex_;\n> +\n> \tCamera3RequestDescriptor(libcamera::Camera *camera,\n> \t\t\t\t const camera3_capture_request_t *camera3Request);\n> \t~Camera3RequestDescriptor();\n> diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp\n> index 0e268cdf..4e275cde 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,20 @@ 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::StreamBuffer *streamBuffer,\n> +\t\t\t\t PostProcessor::Status status) {\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> +\t\t\t\tcameraDevice_->streamProcessingComplete(streamBuffer,\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 da71f113..cbbe7128 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(Camera3RequestDescriptor::StreamBuffer *streamBuf\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(streamBuffer, PostProcessor::Status::Error);\n> \t\treturn jpeg_size;\n> \t}\n> \n> @@ -211,6 +212,7 @@ int PostProcessorJpeg::process(Camera3RequestDescriptor::StreamBuffer *streamBuf\n> \n> \t/* Update the JPEG result Metadata. */\n> \tresultMetadata->addEntry(ANDROID_JPEG_SIZE, jpeg_size);\n> +\tprocessComplete.emit(streamBuffer, 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 128161c8..4ac74fcf 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> @@ -16,11 +18,18 @@\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> \t\t\t const libcamera::StreamConfiguration &outCfg) = 0;\n> \tvirtual int process(Camera3RequestDescriptor::StreamBuffer *streamBuffer) = 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 eeb8f1f4..8e77bf57 100644\n> --- a/src/android/yuv/post_processor_yuv.cpp\n> +++ b/src/android/yuv/post_processor_yuv.cpp\n> @@ -54,12 +54,15 @@ int PostProcessorYuv::process(Camera3RequestDescriptor::StreamBuffer *streamBuff\n> \tconst FrameBuffer &source = *streamBuffer->srcBuffer;\n> \tCameraBuffer *destination = streamBuffer->destBuffer.get();\n> \n> -\tif (!isValidBuffers(source, *destination))\n> +\tif (!isValidBuffers(source, *destination)) {\n> +\t\tprocessComplete.emit(streamBuffer, 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(streamBuffer, PostProcessor::Status::Error);\n> \t\treturn -EINVAL;\n> \t}\n> \n> @@ -77,9 +80,12 @@ int PostProcessorYuv::process(Camera3RequestDescriptor::StreamBuffer *streamBuff\n> \t\t\t\t libyuv::FilterMode::kFilterBilinear);\n> \tif (ret) {\n> \t\tLOG(YUV, Error) << \"Failed NV12 scaling: \" << ret;\n> +\t\tprocessComplete.emit(streamBuffer, PostProcessor::Status::Error);\n> \t\treturn -EINVAL;\n> \t}\n> \n> +\tprocessComplete.emit(streamBuffer, 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 16766BF415\n\tfor ;\n\tMon, 25 Oct 2021 06:03:59 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 562E764870;\n\tMon, 25 Oct 2021 08:03:58 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 97D4E60237\n\tfor ;\n\tMon, 25 Oct 2021 08:03:57 +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 19B2BE0A;\n\tMon, 25 Oct 2021 08:03:57 +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=\"nSxDB4ZQ\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1635141837;\n\tbh=NrGXhf2pvoqqU+WjFdn55HStG75R5SgnhNPDDozGagM=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=nSxDB4ZQUFp3/mghreOu+BbEF8ifTyvkroMWmGvaNXeA/hZD4L6z870FeVKUhI6I6\n\tNuoQ6sFpF/QeuqVK9953CuVqrzL25zkpsssx6txYBJgQDrvdbD71vv9x7HbYfjhw+R\n\tZ90d495KIyDFz16/vlGnAurw7kq6TYk3JhrkN1gQ=","Date":"Mon, 25 Oct 2021 09:03:35 +0300","From":"Laurent Pinchart ","To":"Umang Jain ","Message-ID":"","References":"<20211023103302.152769-1-umang.jain@ideasonboard.com>\n\t<20211023103302.152769-6-umang.jain@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20211023103302.152769-6-umang.jain@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v6 5/7] android: Track and notify post\n\tprocessing of streams","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":20431,"web_url":"https://patchwork.libcamera.org/comment/20431/","msgid":"","date":"2021-10-25T11:49:11","subject":"Re: [libcamera-devel] [PATCH v6 5/7] android: Track and notify post\n\tprocessing of streams","submitter":{"id":63,"url":"https://patchwork.libcamera.org/api/people/63/","name":"Hirokazu Honda","email":"hiroh@chromium.org"},"content":"Hi Umang, thank you for the patch.\n\nOn Mon, Oct 25, 2021 at 3:03 PM Laurent Pinchart\n wrote:\n>\n> Hi Umang,\n>\n> Thank you for the patch.\n>\n> On Sat, Oct 23, 2021 at 04:03:00PM +0530, Umang Jain wrote:\n> > Notify that the post processing for a request has been completed,\n> > via a signal. The signal emit with a context pointer along with status\n>\n> s/emit/is emitted/ ?\n>\n> > of the buffer. The function CameraDevice::streamProcessingComplete() will\n> > finally set the status on the request descriptor and complete the\n> > descriptor if all the streams requiring post processing are completed.\n> > If buffer status obtained is in error state, notify the status to the\n> > framework and set the overall error status on the descriptor via\n> > setBufferStatus().\n> >\n> > We need to track the number of streams requiring post-processing\n> > per Camera3RequestDescriptor (i.e. per capture request). Introduce\n> > a std::map<> to track the post-processing of streams. The nodes\n> > are dropped from the map when a particular stream post processing\n> > is completed (or on error paths). A std::map is selected for tracking\n> > post-processing requests, since we will move post-processing to be\n> > asynchronous in subsequent commits. A vector or queue will not be\n> > suitable then as the sequential order of post-processing completion\n> > of various requests won't be guaranteed then.\n> >\n> > A streamProcessMutex_ has been introduced here as well, which will be\n>\n> s/streamProcessMutex_/streamsProcessMutex_/\n>\n> > applicable to guard access to descriptor's pendingStreamsToProcess_ when\n> > post-processing is moved to be asynchronous in subsequent commits.\n> >\n> > Signed-off-by: Umang Jain \n> > ---\n> > src/android/camera_device.cpp | 95 ++++++++++++++++++------\n> > src/android/camera_device.h | 4 +\n> > src/android/camera_request.h | 6 ++\n> > src/android/camera_stream.cpp | 15 ++++\n> > src/android/jpeg/post_processor_jpeg.cpp | 2 +\n> > src/android/post_processor.h | 9 +++\n> > src/android/yuv/post_processor_yuv.cpp | 8 +-\n> > 7 files changed, 114 insertions(+), 25 deletions(-)\n> >\n> > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> > index 2a98a2e6..3114def0 100644\n> > --- a/src/android/camera_device.cpp\n> > +++ b/src/android/camera_device.cpp\n> > @@ -926,6 +926,8 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n> > * Request.\n> > */\n> > LOG(HAL, Debug) << ss.str() << \" (mapped)\";\n>\n> Blank line here, or no blank line below.\n>\n> > + descriptor->pendingStreamsToProcess_.insert(\n> > + { cameraStream, &buffer });\n> > continue;\n> >\n> > case CameraStream::Type::Direct:\n> > @@ -955,6 +957,9 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n> > frameBuffer = cameraStream->getBuffer();\n> > buffer.internalBuffer = frameBuffer;\n> > LOG(HAL, Debug) << ss.str() << \" (internal)\";\n> > +\n> > + descriptor->pendingStreamsToProcess_.insert(\n> > + { cameraStream, &buffer });\n> > break;\n> > }\n> >\n> > @@ -1118,43 +1123,46 @@ void CameraDevice::requestComplete(Request *request)\n> > }\n> >\n> > /* Handle post-processing. */\n> > - bool hasPostProcessingErrors = false;\n> > - for (auto &buffer : descriptor->buffers_) {\n> > - CameraStream *stream = buffer.stream;\n> > -\n> > - if (stream->type() == CameraStream::Type::Direct)\n> > - continue;\n> > + bool needPostProcessing = false;\n\nnit: can just be\nconst bool needPostProcessing = !descriptor->pendingStreamToProcess.empty()?\n\n> > + /*\n> > + * \\todo Protect the loop below with streamProcessMutex_ when post\n> > + * processor runs asynchronously.\n> > + */\n> > + auto iter = descriptor->pendingStreamsToProcess_.begin();\n> > + while (descriptor->pendingStreamsToProcess_.size() > 0) {\n>\n> while (!descriptor->pendingStreamsToProcess_.empty()) {\n>\n> But it's not correct. The fix is in 7/7:\n>\n> while (iter != descriptor->pendingStreamsToProcess_.end()) {\n>\n> > + CameraStream *stream = iter->first;\n> > + Camera3RequestDescriptor::StreamBuffer *buffer = iter->second;\n> > + needPostProcessing = true;\n> >\n> > FrameBuffer *src = request->findBuffer(stream->stream());\n> > if (!src) {\n> > LOG(HAL, Error) << \"Failed to find a source stream buffer\";\n> > - buffer.status = Camera3RequestDescriptor::Status::Error;\n> > - notifyError(descriptor->frameNumber_, stream->camera3Stream(),\n> > - CAMERA3_MSG_ERROR_BUFFER);\n> > - hasPostProcessingErrors = true;\n> > + setBufferStatus(*buffer, Camera3RequestDescriptor::Status::Error);\n> > + iter = descriptor->pendingStreamsToProcess_.erase(iter);\n\nDo we need to proceed processing in this case?\n\n\n> > continue;\n> > }\n> >\n> > - int ret = stream->process(*src, buffer);\n> > + ++iter;\n> > + int ret = stream->process(*src, *buffer);\n> > + if (ret) {\n> > + setBufferStatus(*buffer, Camera3RequestDescriptor::Status::Error);\n> > + descriptor->pendingStreamsToProcess_.erase(stream);\n> > + }\n> > + }\n> >\n> > + if (needPostProcessing) {\n> > /*\n> > - * If the framebuffer is internal to CameraStream return it back\n> > - * now that we're done processing it.\n> > + * \\todo We will require to check if we failed to queue\n> > + * post-processing requests when we migrate to post-processor\n> > + * running asynchronously.\n> > + *\n> > + * if (descriptor->pendingStreamsToProcess_.size() == 0)\n> > + * completeDescriptor(descriptor);\n>\n> Can't we do this already here ? I think you can actually drop the\n> needPostProcessing variable and just write\n>\n> if (descriptor->pendingStreamsToProcess_.empty())\n> completeDescriptor(descriptor);\n>\n> as needPostProcessing can only be false if pendingStreamsToProcess_ was\n> empty before the while loop above, and it will thus be empty after the\n> loop as well in that case.\n>\n> > */\n> > - if (buffer.internalBuffer)\n> > - stream->putBuffer(buffer.internalBuffer);\n> >\n> > - if (ret) {\n> > - buffer.status = Camera3RequestDescriptor::Status::Error;\n> > - hasPostProcessingErrors = true;\n> > - notifyError(descriptor->frameNumber_, stream->camera3Stream(),\n> > - CAMERA3_MSG_ERROR_BUFFER);\n> > - }\n> > + return;\n> > }\n> >\n> > - if (hasPostProcessingErrors)\n> > - descriptor->status_ = Camera3RequestDescriptor::Status::Error;\n> > -\n> > completeDescriptor(descriptor);\n> > }\n> >\n> > @@ -1210,6 +1218,45 @@ void CameraDevice::sendCaptureResults()\n> > }\n> > }\n> >\n> > +void CameraDevice::setBufferStatus(Camera3RequestDescriptor::StreamBuffer &streamBuffer,\n> > + Camera3RequestDescriptor::Status status)\n> > +{\n> > + /*\n> > + * If the framebuffer is internal to CameraStream return it back now\n> > + * that we're done processing it.\n> > + */\n> > + if (streamBuffer.internalBuffer)\n> > + streamBuffer.stream->putBuffer(streamBuffer.internalBuffer);\n>\n> I'd move this to the caller, as it's not about the buffer status.\n>\n> > +\n> > + streamBuffer.status = status;\n> > + if (status != Camera3RequestDescriptor::Status::Success) {\n> > + notifyError(streamBuffer.request->frameNumber_,\n> > + streamBuffer.stream->camera3Stream(),\n> > + CAMERA3_MSG_ERROR_BUFFER);\n> > +\n> > + /* Also set error status on entire request descriptor. */\n> > + streamBuffer.request->status_ =\n> > + Camera3RequestDescriptor::Status::Error;\n> > + }\n> > +}\n> > +\n> > +void CameraDevice::streamProcessingComplete(Camera3RequestDescriptor::StreamBuffer *streamBuffer,\n> > + Camera3RequestDescriptor::Status status)\n> > +{\n> > + Camera3RequestDescriptor *request = streamBuffer->request;\n> > + MutexLocker locker(request->streamsProcessMutex_);\n> > +\n> > + setBufferStatus(*streamBuffer, status);\n>\n> Do we need to protect the setBufferStatus() call with\n> streamsProcessMutex_ ? I thought it only protects\n> pendingStreamsToProcess_.\n>\n\nI think if this function is called by any thread, we need to protect status too.\nIf it's true, I would rename the mutex name.\n\n> > + request->pendingStreamsToProcess_.erase(streamBuffer->stream);\n> > +\n> > + if (request->pendingStreamsToProcess_.size() > 0)\n>\n> if (!request->pendingStreamsToProcess_.empty())\n>\n> > + return;\n> > +\n> > + locker.unlock();\n> > +\n> > + completeDescriptor(streamBuffer->request);\n> > +}\n> > +\n> > std::string CameraDevice::logPrefix() const\n> > {\n> > return \"'\" + camera_->id() + \"'\";\n> > diff --git a/src/android/camera_device.h b/src/android/camera_device.h\n> > index e544f2bd..2a414020 100644\n> > --- a/src/android/camera_device.h\n> > +++ b/src/android/camera_device.h\n> > @@ -66,6 +66,8 @@ public:\n> > int configureStreams(camera3_stream_configuration_t *stream_list);\n> > int processCaptureRequest(camera3_capture_request_t *request);\n> > void requestComplete(libcamera::Request *request);\n> > + void streamProcessingComplete(Camera3RequestDescriptor::StreamBuffer *bufferStream,\n\nnit: streamBuffer may be better name?\n\n> > + Camera3RequestDescriptor::Status status);\n> >\n> > protected:\n> > std::string logPrefix() const override;\n> > @@ -95,6 +97,8 @@ private:\n> > int processControls(Camera3RequestDescriptor *descriptor);\n> > void completeDescriptor(Camera3RequestDescriptor *descriptor);\n> > void sendCaptureResults();\n> > + void setBufferStatus(Camera3RequestDescriptor::StreamBuffer &buffer,\n> > + Camera3RequestDescriptor::Status status);\n> > std::unique_ptr getResultMetadata(\n> > const Camera3RequestDescriptor &descriptor) const;\n> >\n> > diff --git a/src/android/camera_request.h b/src/android/camera_request.h\n> > index c4bc5d6e..cc2b7035 100644\n> > --- a/src/android/camera_request.h\n> > +++ b/src/android/camera_request.h\n> > @@ -7,7 +7,9 @@\n> > #ifndef __ANDROID_CAMERA_REQUEST_H__\n> > #define __ANDROID_CAMERA_REQUEST_H__\n> >\n> > +#include \n> > #include \n> > +#include \n> > #include \n> >\n> > #include \n> > @@ -43,6 +45,10 @@ public:\n> > Camera3RequestDescriptor *request;\n> > };\n> >\n> > + /* Keeps track of streams requiring post-processing. */\n> > + std::map pendingStreamsToProcess_;\n> > + std::mutex streamsProcessMutex_;\n> > +\n> > Camera3RequestDescriptor(libcamera::Camera *camera,\n> > const camera3_capture_request_t *camera3Request);\n> > ~Camera3RequestDescriptor();\n> > diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp\n> > index 0e268cdf..4e275cde 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,20 @@ int CameraStream::configure()\n> > int ret = postProcessor_->configure(configuration(), output);\n> > if (ret)\n> > return ret;\n> > +\n> > + postProcessor_->processComplete.connect(\n> > + this, [&](Camera3RequestDescriptor::StreamBuffer *streamBuffer,\n> > + PostProcessor::Status status) {\n\nI would not like capturing a reference of all visible variables.\ncameraDevice looks only needed.\nI would do [cameraDevice=cameraDevice_](...).\n\n> > + Camera3RequestDescriptor::Status bufferStatus;\n> > +\n> > + if (status == PostProcessor::Status::Success)\n> > + bufferStatus = Camera3RequestDescriptor::Status::Success;\n> > + else\n> > + bufferStatus = Camera3RequestDescriptor::Status::Error;\n> > +\n> > + cameraDevice_->streamProcessingComplete(streamBuffer,\n> > + bufferStatus);\n> > + });\n> > }\n> >\n> > if (type_ == Type::Internal) {\n> > diff --git a/src/android/jpeg/post_processor_jpeg.cpp b/src/android/jpeg/post_processor_jpeg.cpp\n> > index da71f113..cbbe7128 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(Camera3RequestDescriptor::StreamBuffer *streamBuf\n> > exif.data(), quality);\n> > if (jpeg_size < 0) {\n> > LOG(JPEG, Error) << \"Failed to encode stream image\";\n> > + processComplete.emit(streamBuffer, PostProcessor::Status::Error);\n> > return jpeg_size;\n> > }\n> >\n> > @@ -211,6 +212,7 @@ int PostProcessorJpeg::process(Camera3RequestDescriptor::StreamBuffer *streamBuf\n> >\n> > /* Update the JPEG result Metadata. */\n> > resultMetadata->addEntry(ANDROID_JPEG_SIZE, jpeg_size);\n> > + processComplete.emit(streamBuffer, PostProcessor::Status::Success);\n> >\n> > return 0;\n> > }\n> > diff --git a/src/android/post_processor.h b/src/android/post_processor.h\n> > index 128161c8..4ac74fcf 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> > @@ -16,11 +18,18 @@\n> > class PostProcessor\n> > {\n> > public:\n> > + enum class Status {\n> > + Error,\n> > + Success\n> > + };\n> > +\n> > virtual ~PostProcessor() = default;\n> >\n> > virtual int configure(const libcamera::StreamConfiguration &inCfg,\n> > const libcamera::StreamConfiguration &outCfg) = 0;\n> > virtual int process(Camera3RequestDescriptor::StreamBuffer *streamBuffer) = 0;\n> > +\n> > + libcamera::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 eeb8f1f4..8e77bf57 100644\n> > --- a/src/android/yuv/post_processor_yuv.cpp\n> > +++ b/src/android/yuv/post_processor_yuv.cpp\n> > @@ -54,12 +54,15 @@ int PostProcessorYuv::process(Camera3RequestDescriptor::StreamBuffer *streamBuff\n> > const FrameBuffer &source = *streamBuffer->srcBuffer;\n> > CameraBuffer *destination = streamBuffer->destBuffer.get();\n> >\n> > - if (!isValidBuffers(source, *destination))\n> > + if (!isValidBuffers(source, *destination)) {\n> > + processComplete.emit(streamBuffer, PostProcessor::Status::Error);\n> > return -EINVAL;\n> > + }\n> >\n> > const MappedFrameBuffer sourceMapped(&source, MappedFrameBuffer::MapFlag::Read);\n> > if (!sourceMapped.isValid()) {\n> > LOG(YUV, Error) << \"Failed to mmap camera frame buffer\";\n> > + processComplete.emit(streamBuffer, PostProcessor::Status::Error);\n> > return -EINVAL;\n> > }\n> >\n> > @@ -77,9 +80,12 @@ int PostProcessorYuv::process(Camera3RequestDescriptor::StreamBuffer *streamBuff\n> > libyuv::FilterMode::kFilterBilinear);\n> > if (ret) {\n> > LOG(YUV, Error) << \"Failed NV12 scaling: \" << ret;\n> > + processComplete.emit(streamBuffer, PostProcessor::Status::Error);\n> > return -EINVAL;\n> > }\n> >\n> > + processComplete.emit(streamBuffer, PostProcessor::Status::Success);\n> > +\n> > return 0;\n> > }\n> >\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id D10D3BF415\n\tfor ;\n\tMon, 25 Oct 2021 11:49:25 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 28D516486E;\n\tMon, 25 Oct 2021 13:49:25 +0200 (CEST)","from mail-ed1-x531.google.com (mail-ed1-x531.google.com\n\t[IPv6:2a00:1450:4864:20::531])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 65CB660124\n\tfor ;\n\tMon, 25 Oct 2021 13:49:23 +0200 (CEST)","by mail-ed1-x531.google.com with SMTP id u13so18070050edy.10\n\tfor ;\n\tMon, 25 Oct 2021 04:49:23 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"g8OmcvEW\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org;\n\ts=google; \n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=IW1Px/1xUv9OmFU8fXHcAA1Nt3nEH+dI8Wgdb4kQw3A=;\n\tb=g8OmcvEWGFuHr0/sqd+0OnT1xUT7u9aY64fXux77GOPrEce4lPeICNb3zO1qpwJd4X\n\tXP9wMlsGkZxntQq4evbhoZQ19aImBgPyJ3nAC9Ypqi9WAqcOqDqP7MOGKidYTSHsGzK7\n\tr+icV2tSjmkw+ZHp/uAyxC5KKNtuaPXfJqnvg=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=IW1Px/1xUv9OmFU8fXHcAA1Nt3nEH+dI8Wgdb4kQw3A=;\n\tb=niOgowDtpL55lH9HNMgtHZ9p+PKyM2MfDenVH4aE/FsiFVHSGjWY8sXmEgtQv1Br5A\n\t6b6AyOPsWE5T0CvOzRvWsRrUqiE2hN8fsR8SVuakpFi8c+NcRRFYlmi9iGvUCl5whSxM\n\tuLW0o9DnK/qmLdDaWIIauwFUbVWlx/kZLB2yaIxX0Vvd76PzfUOZke6vgb7A1bgwdA7t\n\tK08+3HA/TB5DjnaDBU92Gbe7X134ecTpgUkFDKXjzTY8Nn84iA+U41ZPsqDOS2YmGXOh\n\tUNfEdtm/Fhl9iuh8HRto0rgk6yG7LCMrX4Ak/rgpFY7RqC7KgxvYgiBMZtGvmxHp7gGB\n\tj/tA==","X-Gm-Message-State":"AOAM532kI/UYMSu9VGVBL/9QwLeTJ9XEGwOUtN3jZcoVYCayj3Eaxxuy\n\tfdAa9aRdSBFYJJjTV5IINw7HutMJs+oOlZIPFNWOS08qh+Rg6A==","X-Google-Smtp-Source":"ABdhPJyhAZjQU28nJUPKO4bucBUllF6xIsRecEO+p3SRCanNGQubU03gNFNUXoZVtX06/yXZ5jgk1Ccc6oITdd/8a1A=","X-Received":"by 2002:a17:906:5801:: with SMTP id\n\tm1mr22390942ejq.296.1635162562888; \n\tMon, 25 Oct 2021 04:49:22 -0700 (PDT)","MIME-Version":"1.0","References":"<20211023103302.152769-1-umang.jain@ideasonboard.com>\n\t<20211023103302.152769-6-umang.jain@ideasonboard.com>\n\t","In-Reply-To":"","From":"Hirokazu Honda ","Date":"Mon, 25 Oct 2021 20:49:11 +0900","Message-ID":"","To":"Laurent Pinchart ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH v6 5/7] android: Track and notify post\n\tprocessing of streams","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":20436,"web_url":"https://patchwork.libcamera.org/comment/20436/","msgid":"","date":"2021-10-25T13:16:39","subject":"Re: [libcamera-devel] [PATCH v6 5/7] android: Track and notify post\n\tprocessing of streams","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/25/21 11:33 AM, Laurent Pinchart wrote:\n> Hi Umang,\n>\n> Thank you for the patch.\n>\n> On Sat, Oct 23, 2021 at 04:03:00PM +0530, Umang Jain wrote:\n>> Notify that the post processing for a request has been completed,\n>> via a signal. The signal emit with a context pointer along with status\n> s/emit/is emitted/ ?\n>\n>> of the buffer. The function CameraDevice::streamProcessingComplete() will\n>> finally set the status on the request descriptor and complete the\n>> descriptor if all the streams requiring post processing are completed.\n>> If buffer status obtained is in error state, notify the status to the\n>> framework and set the overall error status on the descriptor via\n>> setBufferStatus().\n>>\n>> We need to track the number of streams requiring post-processing\n>> per Camera3RequestDescriptor (i.e. per capture request). Introduce\n>> a std::map<> to track the post-processing of streams. The nodes\n>> are dropped from the map when a particular stream post processing\n>> is completed (or on error paths). A std::map is selected for tracking\n>> post-processing requests, since we will move post-processing to be\n>> asynchronous in subsequent commits. A vector or queue will not be\n>> suitable then as the sequential order of post-processing completion\n>> of various requests won't be guaranteed then.\n>>\n>> A streamProcessMutex_ has been introduced here as well, which will be\n> s/streamProcessMutex_/streamsProcessMutex_/\n>\n>> applicable to guard access to descriptor's pendingStreamsToProcess_ when\n>> post-processing is moved to be asynchronous in subsequent commits.\n>>\n>> Signed-off-by: Umang Jain \n>> ---\n>> src/android/camera_device.cpp | 95 ++++++++++++++++++------\n>> src/android/camera_device.h | 4 +\n>> src/android/camera_request.h | 6 ++\n>> src/android/camera_stream.cpp | 15 ++++\n>> src/android/jpeg/post_processor_jpeg.cpp | 2 +\n>> src/android/post_processor.h | 9 +++\n>> src/android/yuv/post_processor_yuv.cpp | 8 +-\n>> 7 files changed, 114 insertions(+), 25 deletions(-)\n>>\n>> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n>> index 2a98a2e6..3114def0 100644\n>> --- a/src/android/camera_device.cpp\n>> +++ b/src/android/camera_device.cpp\n>> @@ -926,6 +926,8 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n>> \t\t\t * Request.\n>> \t\t\t */\n>> \t\t\tLOG(HAL, Debug) << ss.str() << \" (mapped)\";\n> Blank line here, or no blank line below.\n>\n>> +\t\t\tdescriptor->pendingStreamsToProcess_.insert(\n>> +\t\t\t\t{ cameraStream, &buffer });\n>> \t\t\tcontinue;\n>> \n>> \t\tcase CameraStream::Type::Direct:\n>> @@ -955,6 +957,9 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n>> \t\t\tframeBuffer = cameraStream->getBuffer();\n>> \t\t\tbuffer.internalBuffer = frameBuffer;\n>> \t\t\tLOG(HAL, Debug) << ss.str() << \" (internal)\";\n>> +\n>> +\t\t\tdescriptor->pendingStreamsToProcess_.insert(\n>> +\t\t\t\t{ cameraStream, &buffer });\n>> \t\t\tbreak;\n>> \t\t}\n>> \n>> @@ -1118,43 +1123,46 @@ void CameraDevice::requestComplete(Request *request)\n>> \t}\n>> \n>> \t/* Handle post-processing. */\n>> -\tbool hasPostProcessingErrors = false;\n>> -\tfor (auto &buffer : descriptor->buffers_) {\n>> -\t\tCameraStream *stream = buffer.stream;\n>> -\n>> -\t\tif (stream->type() == CameraStream::Type::Direct)\n>> -\t\t\tcontinue;\n>> +\tbool needPostProcessing = false;\n>> +\t/*\n>> +\t * \\todo Protect the loop below with streamProcessMutex_ when post\n>> +\t * processor runs asynchronously.\n>> +\t */\n>> +\tauto iter = descriptor->pendingStreamsToProcess_.begin();\n>> +\twhile (descriptor->pendingStreamsToProcess_.size() > 0) {\n> \twhile (!descriptor->pendingStreamsToProcess_.empty()) {\n>\n> But it's not correct. The fix is in 7/7:\n>\n> \twhile (iter != descriptor->pendingStreamsToProcess_.end()) {\n\n\nYes, it was a tricky to handle the loop for async and sync variations\n\nFor the async one, iter == end() was the only choice I  believe (Without \nintroducing a counter)\n\n>\n>> +\t\tCameraStream *stream = iter->first;\n>> +\t\tCamera3RequestDescriptor::StreamBuffer *buffer = iter->second;\n>> +\t\tneedPostProcessing = true;\n>> \n>> \t\tFrameBuffer *src = request->findBuffer(stream->stream());\n>> \t\tif (!src) {\n>> \t\t\tLOG(HAL, Error) << \"Failed to find a source stream buffer\";\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\thasPostProcessingErrors = true;\n>> +\t\t\tsetBufferStatus(*buffer, Camera3RequestDescriptor::Status::Error);\n>> +\t\t\titer = descriptor->pendingStreamsToProcess_.erase(iter);\n>> \t\t\tcontinue;\n>> \t\t}\n>> \n>> -\t\tint ret = stream->process(*src, buffer);\n>> +\t\t++iter;\n>> +\t\tint ret = stream->process(*src, *buffer);\n>> +\t\tif (ret) {\n>> +\t\t\tsetBufferStatus(*buffer, Camera3RequestDescriptor::Status::Error);\n>> +\t\t\tdescriptor->pendingStreamsToProcess_.erase(stream);\n>> +\t\t}\n>> +\t}\n>> \n>> +\tif (needPostProcessing) {\n>> \t\t/*\n>> -\t\t * If the framebuffer is internal to CameraStream return it back\n>> -\t\t * now that we're done processing it.\n>> +\t\t * \\todo We will require to check if we failed to queue\n>> +\t\t * post-processing requests when we migrate to post-processor\n>> +\t\t * running asynchronously.\n>> +\t\t *\n>> +\t\t * if (descriptor->pendingStreamsToProcess_.size() == 0)\n>> +\t\t *\tcompleteDescriptor(descriptor);\n> Can't we do this already here ? I think you can actually drop the\n> needPostProcessing variable and just write\n>\n> \tif (descriptor->pendingStreamsToProcess_.empty())\n> \t\tcompleteDescriptor(descriptor);\n>\n> as needPostProcessing can only be false if pendingStreamsToProcess_ was\n> empty before the while loop above, and it will thus be empty after the\n> loop as well in that case.\n\n\nTrue, but we need needPostProcessing variable for async. I pre-empted \nits introduction deliberately to set the design beforehand andthen,  I \ncan introduce async bits with minimal diff for \nCamreraDevice::requestComplete()\n\n>\n>> \t\t */\n>> -\t\tif (buffer.internalBuffer)\n>> -\t\t\tstream->putBuffer(buffer.internalBuffer);\n>> \n>> -\t\tif (ret) {\n>> -\t\t\tbuffer.status = Camera3RequestDescriptor::Status::Error;\n>> -\t\t\thasPostProcessingErrors = true;\n>> -\t\t\tnotifyError(descriptor->frameNumber_, stream->camera3Stream(),\n>> -\t\t\t\t CAMERA3_MSG_ERROR_BUFFER);\n>> -\t\t}\n>> +\t\treturn;\n>> \t}\n>> \n>> -\tif (hasPostProcessingErrors)\n>> -\t\tdescriptor->status_ = Camera3RequestDescriptor::Status::Error;\n>> -\n>> \tcompleteDescriptor(descriptor);\n>> }\n>> \n>> @@ -1210,6 +1218,45 @@ void CameraDevice::sendCaptureResults()\n>> \t}\n>> }\n>> \n>> +void CameraDevice::setBufferStatus(Camera3RequestDescriptor::StreamBuffer &streamBuffer,\n>> +\t\t\t\t Camera3RequestDescriptor::Status status)\n>> +{\n>> +\t/*\n>> +\t * If the framebuffer is internal to CameraStream return it back now\n>> +\t * that we're done processing it.\n>> +\t */\n>> +\tif (streamBuffer.internalBuffer)\n>> +\t\tstreamBuffer.stream->putBuffer(streamBuffer.internalBuffer);\n> I'd move this to the caller, as it's not about the buffer status.\n>\n>> +\n>> +\tstreamBuffer.status = status;\n>> +\tif (status != Camera3RequestDescriptor::Status::Success) {\n>> +\t\tnotifyError(streamBuffer.request->frameNumber_,\n>> +\t\t\t streamBuffer.stream->camera3Stream(),\n>> +\t\t\t CAMERA3_MSG_ERROR_BUFFER);\n>> +\n>> +\t\t/* Also set error status on entire request descriptor. */\n>> +\t\tstreamBuffer.request->status_ =\n>> +\t\t\tCamera3RequestDescriptor::Status::Error;\n>> +\t}\n>> +}\n>> +\n>> +void CameraDevice::streamProcessingComplete(Camera3RequestDescriptor::StreamBuffer *streamBuffer,\n>> +\t\t\t\t\t Camera3RequestDescriptor::Status status)\n>> +{\n>> +\tCamera3RequestDescriptor *request = streamBuffer->request;\n>> +\tMutexLocker locker(request->streamsProcessMutex_);\n>> +\n>> +\tsetBufferStatus(*streamBuffer, status);\n> Do we need to protect the setBufferStatus() call with\n> streamsProcessMutex_ ? I thought it only protects\n> pendingStreamsToProcess_.\n\n\nAh, I don't see why we need to lock it. This is a good catch.\n\nFor the async version, the main thread isn't expected to call \nsetBufferStatus() on the same descriptor (because we are already \nhandling synchronous errors beforehand), so I don't expect to race. So I \nshould remove setBufferStatus() from the lock section.\n\n>\n>> +\trequest->pendingStreamsToProcess_.erase(streamBuffer->stream);\n>> +\n>> +\tif (request->pendingStreamsToProcess_.size() > 0)\n> \tif (!request->pendingStreamsToProcess_.empty())\n>\n>> +\t\treturn;\n>> +\n>> +\tlocker.unlock();\n>> +\n>> +\tcompleteDescriptor(streamBuffer->request);\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 e544f2bd..2a414020 100644\n>> --- a/src/android/camera_device.h\n>> +++ b/src/android/camera_device.h\n>> @@ -66,6 +66,8 @@ 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(Camera3RequestDescriptor::StreamBuffer *bufferStream,\n>> +\t\t\t\t Camera3RequestDescriptor::Status status);\n>> \n>> protected:\n>> \tstd::string logPrefix() const override;\n>> @@ -95,6 +97,8 @@ private:\n>> \tint processControls(Camera3RequestDescriptor *descriptor);\n>> \tvoid completeDescriptor(Camera3RequestDescriptor *descriptor);\n>> \tvoid sendCaptureResults();\n>> +\tvoid setBufferStatus(Camera3RequestDescriptor::StreamBuffer &buffer,\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 c4bc5d6e..cc2b7035 100644\n>> --- a/src/android/camera_request.h\n>> +++ b/src/android/camera_request.h\n>> @@ -7,7 +7,9 @@\n>> #ifndef __ANDROID_CAMERA_REQUEST_H__\n>> #define __ANDROID_CAMERA_REQUEST_H__\n>> \n>> +#include \n>> #include \n>> +#include \n>> #include \n>> \n>> #include \n>> @@ -43,6 +45,10 @@ public:\n>> \t\tCamera3RequestDescriptor *request;\n>> \t};\n>> \n>> +\t/* Keeps track of streams requiring post-processing. */\n>> +\tstd::map pendingStreamsToProcess_;\n>> +\tstd::mutex streamsProcessMutex_;\n>> +\n>> \tCamera3RequestDescriptor(libcamera::Camera *camera,\n>> \t\t\t\t const camera3_capture_request_t *camera3Request);\n>> \t~Camera3RequestDescriptor();\n>> diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp\n>> index 0e268cdf..4e275cde 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,20 @@ 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::StreamBuffer *streamBuffer,\n>> +\t\t\t\t PostProcessor::Status status) {\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>> +\t\t\t\tcameraDevice_->streamProcessingComplete(streamBuffer,\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 da71f113..cbbe7128 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(Camera3RequestDescriptor::StreamBuffer *streamBuf\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(streamBuffer, PostProcessor::Status::Error);\n>> \t\treturn jpeg_size;\n>> \t}\n>> \n>> @@ -211,6 +212,7 @@ int PostProcessorJpeg::process(Camera3RequestDescriptor::StreamBuffer *streamBuf\n>> \n>> \t/* Update the JPEG result Metadata. */\n>> \tresultMetadata->addEntry(ANDROID_JPEG_SIZE, jpeg_size);\n>> +\tprocessComplete.emit(streamBuffer, 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 128161c8..4ac74fcf 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>> @@ -16,11 +18,18 @@\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>> \t\t\t const libcamera::StreamConfiguration &outCfg) = 0;\n>> \tvirtual int process(Camera3RequestDescriptor::StreamBuffer *streamBuffer) = 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 eeb8f1f4..8e77bf57 100644\n>> --- a/src/android/yuv/post_processor_yuv.cpp\n>> +++ b/src/android/yuv/post_processor_yuv.cpp\n>> @@ -54,12 +54,15 @@ int PostProcessorYuv::process(Camera3RequestDescriptor::StreamBuffer *streamBuff\n>> \tconst FrameBuffer &source = *streamBuffer->srcBuffer;\n>> \tCameraBuffer *destination = streamBuffer->destBuffer.get();\n>> \n>> -\tif (!isValidBuffers(source, *destination))\n>> +\tif (!isValidBuffers(source, *destination)) {\n>> +\t\tprocessComplete.emit(streamBuffer, 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(streamBuffer, PostProcessor::Status::Error);\n>> \t\treturn -EINVAL;\n>> \t}\n>> \n>> @@ -77,9 +80,12 @@ int PostProcessorYuv::process(Camera3RequestDescriptor::StreamBuffer *streamBuff\n>> \t\t\t\t libyuv::FilterMode::kFilterBilinear);\n>> \tif (ret) {\n>> \t\tLOG(YUV, Error) << \"Failed NV12 scaling: \" << ret;\n>> +\t\tprocessComplete.emit(streamBuffer, PostProcessor::Status::Error);\n>> \t\treturn -EINVAL;\n>> \t}\n>> \n>> +\tprocessComplete.emit(streamBuffer, 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 70A10BDB1C\n\tfor ;\n\tMon, 25 Oct 2021 13:16:47 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C6F9364872;\n\tMon, 25 Oct 2021 15:16:46 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 57ADF60124\n\tfor ;\n\tMon, 25 Oct 2021 15:16:45 +0200 (CEST)","from [192.168.1.106] (unknown [103.251.226.211])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 35CD3E0A;\n\tMon, 25 Oct 2021 15:16:44 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"LDPpRqSr\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1635167804;\n\tbh=kKEh/Rl/dnR5QjLFLLd7C/8FmHw01e7Djfa/saWYX3M=;\n\th=Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=LDPpRqSr00VtHFcrKgk6HR8rcfzv6nmwNfGixD8WRA9tJqd+CZWFdZS94pwY2Xeuv\n\tROSWe9mNEE49fYh9b5QlJJ7ian3zfZ/2N1Rru1OQWN2X7udmzpG6SHmvpv15jakJcq\n\t+84B0QQxPFZeNi8fItGmCvTf7+YgOhQrgp9433Rc=","To":"Laurent Pinchart ","References":"<20211023103302.152769-1-umang.jain@ideasonboard.com>\n\t<20211023103302.152769-6-umang.jain@ideasonboard.com>\n\t","From":"Umang Jain ","Message-ID":"","Date":"Mon, 25 Oct 2021 18:46:39 +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 v6 5/7] android: Track and notify post\n\tprocessing of streams","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":20437,"web_url":"https://patchwork.libcamera.org/comment/20437/","msgid":"","date":"2021-10-25T13:25:09","subject":"Re: [libcamera-devel] [PATCH v6 5/7] android: Track and notify post\n\tprocessing of streams","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Umang,\n\nOn Mon, Oct 25, 2021 at 06:46:39PM +0530, Umang Jain wrote:\n> On 10/25/21 11:33 AM, Laurent Pinchart wrote:\n> > On Sat, Oct 23, 2021 at 04:03:00PM +0530, Umang Jain wrote:\n> >> Notify that the post processing for a request has been completed,\n> >> via a signal. The signal emit with a context pointer along with status\n> > s/emit/is emitted/ ?\n> >\n> >> of the buffer. The function CameraDevice::streamProcessingComplete() will\n> >> finally set the status on the request descriptor and complete the\n> >> descriptor if all the streams requiring post processing are completed.\n> >> If buffer status obtained is in error state, notify the status to the\n> >> framework and set the overall error status on the descriptor via\n> >> setBufferStatus().\n> >>\n> >> We need to track the number of streams requiring post-processing\n> >> per Camera3RequestDescriptor (i.e. per capture request). Introduce\n> >> a std::map<> to track the post-processing of streams. The nodes\n> >> are dropped from the map when a particular stream post processing\n> >> is completed (or on error paths). A std::map is selected for tracking\n> >> post-processing requests, since we will move post-processing to be\n> >> asynchronous in subsequent commits. A vector or queue will not be\n> >> suitable then as the sequential order of post-processing completion\n> >> of various requests won't be guaranteed then.\n> >>\n> >> A streamProcessMutex_ has been introduced here as well, which will be\n> >\n> > s/streamProcessMutex_/streamsProcessMutex_/\n> >\n> >> applicable to guard access to descriptor's pendingStreamsToProcess_ when\n> >> post-processing is moved to be asynchronous in subsequent commits.\n> >>\n> >> Signed-off-by: Umang Jain \n> >> ---\n> >> src/android/camera_device.cpp | 95 ++++++++++++++++++------\n> >> src/android/camera_device.h | 4 +\n> >> src/android/camera_request.h | 6 ++\n> >> src/android/camera_stream.cpp | 15 ++++\n> >> src/android/jpeg/post_processor_jpeg.cpp | 2 +\n> >> src/android/post_processor.h | 9 +++\n> >> src/android/yuv/post_processor_yuv.cpp | 8 +-\n> >> 7 files changed, 114 insertions(+), 25 deletions(-)\n> >>\n> >> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> >> index 2a98a2e6..3114def0 100644\n> >> --- a/src/android/camera_device.cpp\n> >> +++ b/src/android/camera_device.cpp\n> >> @@ -926,6 +926,8 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n> >> \t\t\t * Request.\n> >> \t\t\t */\n> >> \t\t\tLOG(HAL, Debug) << ss.str() << \" (mapped)\";\n> > Blank line here, or no blank line below.\n> >\n> >> +\t\t\tdescriptor->pendingStreamsToProcess_.insert(\n> >> +\t\t\t\t{ cameraStream, &buffer });\n> >> \t\t\tcontinue;\n> >> \n> >> \t\tcase CameraStream::Type::Direct:\n> >> @@ -955,6 +957,9 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n> >> \t\t\tframeBuffer = cameraStream->getBuffer();\n> >> \t\t\tbuffer.internalBuffer = frameBuffer;\n> >> \t\t\tLOG(HAL, Debug) << ss.str() << \" (internal)\";\n> >> +\n> >> +\t\t\tdescriptor->pendingStreamsToProcess_.insert(\n> >> +\t\t\t\t{ cameraStream, &buffer });\n> >> \t\t\tbreak;\n> >> \t\t}\n> >> \n> >> @@ -1118,43 +1123,46 @@ void CameraDevice::requestComplete(Request *request)\n> >> \t}\n> >> \n> >> \t/* Handle post-processing. */\n> >> -\tbool hasPostProcessingErrors = false;\n> >> -\tfor (auto &buffer : descriptor->buffers_) {\n> >> -\t\tCameraStream *stream = buffer.stream;\n> >> -\n> >> -\t\tif (stream->type() == CameraStream::Type::Direct)\n> >> -\t\t\tcontinue;\n> >> +\tbool needPostProcessing = false;\n> >> +\t/*\n> >> +\t * \\todo Protect the loop below with streamProcessMutex_ when post\n> >> +\t * processor runs asynchronously.\n> >> +\t */\n> >> +\tauto iter = descriptor->pendingStreamsToProcess_.begin();\n> >> +\twhile (descriptor->pendingStreamsToProcess_.size() > 0) {\n> > \twhile (!descriptor->pendingStreamsToProcess_.empty()) {\n> >\n> > But it's not correct. The fix is in 7/7:\n> >\n> > \twhile (iter != descriptor->pendingStreamsToProcess_.end()) {\n> \n> Yes, it was a tricky to handle the loop for async and sync variations\n> \n> For the async one, iter == end() was the only choice I  believe (Without \n> introducing a counter)\n> \n> >> +\t\tCameraStream *stream = iter->first;\n> >> +\t\tCamera3RequestDescriptor::StreamBuffer *buffer = iter->second;\n> >> +\t\tneedPostProcessing = true;\n> >> \n> >> \t\tFrameBuffer *src = request->findBuffer(stream->stream());\n> >> \t\tif (!src) {\n> >> \t\t\tLOG(HAL, Error) << \"Failed to find a source stream buffer\";\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\thasPostProcessingErrors = true;\n> >> +\t\t\tsetBufferStatus(*buffer, Camera3RequestDescriptor::Status::Error);\n> >> +\t\t\titer = descriptor->pendingStreamsToProcess_.erase(iter);\n> >> \t\t\tcontinue;\n> >> \t\t}\n> >> \n> >> -\t\tint ret = stream->process(*src, buffer);\n> >> +\t\t++iter;\n> >> +\t\tint ret = stream->process(*src, *buffer);\n> >> +\t\tif (ret) {\n> >> +\t\t\tsetBufferStatus(*buffer, Camera3RequestDescriptor::Status::Error);\n> >> +\t\t\tdescriptor->pendingStreamsToProcess_.erase(stream);\n> >> +\t\t}\n> >> +\t}\n> >> \n> >> +\tif (needPostProcessing) {\n> >> \t\t/*\n> >> -\t\t * If the framebuffer is internal to CameraStream return it back\n> >> -\t\t * now that we're done processing it.\n> >> +\t\t * \\todo We will require to check if we failed to queue\n> >> +\t\t * post-processing requests when we migrate to post-processor\n> >> +\t\t * running asynchronously.\n> >> +\t\t *\n> >> +\t\t * if (descriptor->pendingStreamsToProcess_.size() == 0)\n> >> +\t\t *\tcompleteDescriptor(descriptor);\n> >\n> > Can't we do this already here ? I think you can actually drop the\n> > needPostProcessing variable and just write\n> >\n> > \tif (descriptor->pendingStreamsToProcess_.empty())\n> > \t\tcompleteDescriptor(descriptor);\n> >\n> > as needPostProcessing can only be false if pendingStreamsToProcess_ was\n> > empty before the while loop above, and it will thus be empty after the\n> > loop as well in that case.\n> \n> True, but we need needPostProcessing variable for async.\n\nWhy is that ? Won't\n\n \tif (descriptor->pendingStreamsToProcess_.empty())\n \t\tcompleteDescriptor(descriptor);\n\nwork in async mode too ?\n\n> I pre-empted \n> its introduction deliberately to set the design beforehand andthen,  I \n> can introduce async bits with minimal diff for \n> CamreraDevice::requestComplete()\n> \n> >> \t\t */\n> >> -\t\tif (buffer.internalBuffer)\n> >> -\t\t\tstream->putBuffer(buffer.internalBuffer);\n> >> \n> >> -\t\tif (ret) {\n> >> -\t\t\tbuffer.status = Camera3RequestDescriptor::Status::Error;\n> >> -\t\t\thasPostProcessingErrors = true;\n> >> -\t\t\tnotifyError(descriptor->frameNumber_, stream->camera3Stream(),\n> >> -\t\t\t\t CAMERA3_MSG_ERROR_BUFFER);\n> >> -\t\t}\n> >> +\t\treturn;\n> >> \t}\n> >> \n> >> -\tif (hasPostProcessingErrors)\n> >> -\t\tdescriptor->status_ = Camera3RequestDescriptor::Status::Error;\n> >> -\n> >> \tcompleteDescriptor(descriptor);\n> >> }\n> >> \n> >> @@ -1210,6 +1218,45 @@ void CameraDevice::sendCaptureResults()\n> >> \t}\n> >> }\n> >> \n> >> +void CameraDevice::setBufferStatus(Camera3RequestDescriptor::StreamBuffer &streamBuffer,\n> >> +\t\t\t\t Camera3RequestDescriptor::Status status)\n> >> +{\n> >> +\t/*\n> >> +\t * If the framebuffer is internal to CameraStream return it back now\n> >> +\t * that we're done processing it.\n> >> +\t */\n> >> +\tif (streamBuffer.internalBuffer)\n> >> +\t\tstreamBuffer.stream->putBuffer(streamBuffer.internalBuffer);\n> > I'd move this to the caller, as it's not about the buffer status.\n> >\n> >> +\n> >> +\tstreamBuffer.status = status;\n> >> +\tif (status != Camera3RequestDescriptor::Status::Success) {\n> >> +\t\tnotifyError(streamBuffer.request->frameNumber_,\n> >> +\t\t\t streamBuffer.stream->camera3Stream(),\n> >> +\t\t\t CAMERA3_MSG_ERROR_BUFFER);\n> >> +\n> >> +\t\t/* Also set error status on entire request descriptor. */\n> >> +\t\tstreamBuffer.request->status_ =\n> >> +\t\t\tCamera3RequestDescriptor::Status::Error;\n> >> +\t}\n> >> +}\n> >> +\n> >> +void CameraDevice::streamProcessingComplete(Camera3RequestDescriptor::StreamBuffer *streamBuffer,\n> >> +\t\t\t\t\t Camera3RequestDescriptor::Status status)\n> >> +{\n> >> +\tCamera3RequestDescriptor *request = streamBuffer->request;\n> >> +\tMutexLocker locker(request->streamsProcessMutex_);\n> >> +\n> >> +\tsetBufferStatus(*streamBuffer, status);\n> > Do we need to protect the setBufferStatus() call with\n> > streamsProcessMutex_ ? I thought it only protects\n> > pendingStreamsToProcess_.\n> \n> \n> Ah, I don't see why we need to lock it. This is a good catch.\n> \n> For the async version, the main thread isn't expected to call \n> setBufferStatus() on the same descriptor (because we are already \n> handling synchronous errors beforehand), so I don't expect to race. So I \n> should remove setBufferStatus() from the lock section.\n> \n> >\n> >> +\trequest->pendingStreamsToProcess_.erase(streamBuffer->stream);\n> >> +\n> >> +\tif (request->pendingStreamsToProcess_.size() > 0)\n> > \tif (!request->pendingStreamsToProcess_.empty())\n> >\n> >> +\t\treturn;\n> >> +\n> >> +\tlocker.unlock();\n> >> +\n> >> +\tcompleteDescriptor(streamBuffer->request);\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 e544f2bd..2a414020 100644\n> >> --- a/src/android/camera_device.h\n> >> +++ b/src/android/camera_device.h\n> >> @@ -66,6 +66,8 @@ 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(Camera3RequestDescriptor::StreamBuffer *bufferStream,\n> >> +\t\t\t\t Camera3RequestDescriptor::Status status);\n> >> \n> >> protected:\n> >> \tstd::string logPrefix() const override;\n> >> @@ -95,6 +97,8 @@ private:\n> >> \tint processControls(Camera3RequestDescriptor *descriptor);\n> >> \tvoid completeDescriptor(Camera3RequestDescriptor *descriptor);\n> >> \tvoid sendCaptureResults();\n> >> +\tvoid setBufferStatus(Camera3RequestDescriptor::StreamBuffer &buffer,\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 c4bc5d6e..cc2b7035 100644\n> >> --- a/src/android/camera_request.h\n> >> +++ b/src/android/camera_request.h\n> >> @@ -7,7 +7,9 @@\n> >> #ifndef __ANDROID_CAMERA_REQUEST_H__\n> >> #define __ANDROID_CAMERA_REQUEST_H__\n> >> \n> >> +#include \n> >> #include \n> >> +#include \n> >> #include \n> >> \n> >> #include \n> >> @@ -43,6 +45,10 @@ public:\n> >> \t\tCamera3RequestDescriptor *request;\n> >> \t};\n> >> \n> >> +\t/* Keeps track of streams requiring post-processing. */\n> >> +\tstd::map pendingStreamsToProcess_;\n> >> +\tstd::mutex streamsProcessMutex_;\n> >> +\n> >> \tCamera3RequestDescriptor(libcamera::Camera *camera,\n> >> \t\t\t\t const camera3_capture_request_t *camera3Request);\n> >> \t~Camera3RequestDescriptor();\n> >> diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp\n> >> index 0e268cdf..4e275cde 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,20 @@ 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::StreamBuffer *streamBuffer,\n> >> +\t\t\t\t PostProcessor::Status status) {\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> >> +\t\t\t\tcameraDevice_->streamProcessingComplete(streamBuffer,\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 da71f113..cbbe7128 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(Camera3RequestDescriptor::StreamBuffer *streamBuf\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(streamBuffer, PostProcessor::Status::Error);\n> >> \t\treturn jpeg_size;\n> >> \t}\n> >> \n> >> @@ -211,6 +212,7 @@ int PostProcessorJpeg::process(Camera3RequestDescriptor::StreamBuffer *streamBuf\n> >> \n> >> \t/* Update the JPEG result Metadata. */\n> >> \tresultMetadata->addEntry(ANDROID_JPEG_SIZE, jpeg_size);\n> >> +\tprocessComplete.emit(streamBuffer, 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 128161c8..4ac74fcf 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> >> @@ -16,11 +18,18 @@\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> >> \t\t\t const libcamera::StreamConfiguration &outCfg) = 0;\n> >> \tvirtual int process(Camera3RequestDescriptor::StreamBuffer *streamBuffer) = 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 eeb8f1f4..8e77bf57 100644\n> >> --- a/src/android/yuv/post_processor_yuv.cpp\n> >> +++ b/src/android/yuv/post_processor_yuv.cpp\n> >> @@ -54,12 +54,15 @@ int PostProcessorYuv::process(Camera3RequestDescriptor::StreamBuffer *streamBuff\n> >> \tconst FrameBuffer &source = *streamBuffer->srcBuffer;\n> >> \tCameraBuffer *destination = streamBuffer->destBuffer.get();\n> >> \n> >> -\tif (!isValidBuffers(source, *destination))\n> >> +\tif (!isValidBuffers(source, *destination)) {\n> >> +\t\tprocessComplete.emit(streamBuffer, 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(streamBuffer, PostProcessor::Status::Error);\n> >> \t\treturn -EINVAL;\n> >> \t}\n> >> \n> >> @@ -77,9 +80,12 @@ int PostProcessorYuv::process(Camera3RequestDescriptor::StreamBuffer *streamBuff\n> >> \t\t\t\t libyuv::FilterMode::kFilterBilinear);\n> >> \tif (ret) {\n> >> \t\tLOG(YUV, Error) << \"Failed NV12 scaling: \" << ret;\n> >> +\t\tprocessComplete.emit(streamBuffer, PostProcessor::Status::Error);\n> >> \t\treturn -EINVAL;\n> >> \t}\n> >> \n> >> +\tprocessComplete.emit(streamBuffer, 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 CD41CBF415\n\tfor ;\n\tMon, 25 Oct 2021 13:25:33 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 375776486E;\n\tMon, 25 Oct 2021 15:25:33 +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 3EA3F60124\n\tfor ;\n\tMon, 25 Oct 2021 15:25:32 +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 9DE2BE0A;\n\tMon, 25 Oct 2021 15:25:31 +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=\"lGYBxapP\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1635168331;\n\tbh=zCJ3gr+UfK7XAv72/HdRDrvUiuZKhOLPSAwAD8cGGJk=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=lGYBxapPIbgXYeABIV2Gj2OsFiheXd6D+O3TqQuXE7Buvp5lttXSkOlwYYymhxYPe\n\t2RGkUYWg5uwMaVMNLC7+6kRDe6WLOP8Rz5Q0UykRc6J9d155V2vckuni5H8YXz203M\n\tY9LAKmtrc1HAsJMf8c0vCvJMIoqy5LJWMAabHDYI=","Date":"Mon, 25 Oct 2021 16:25:09 +0300","From":"Laurent Pinchart ","To":"Umang Jain ","Message-ID":"","References":"<20211023103302.152769-1-umang.jain@ideasonboard.com>\n\t<20211023103302.152769-6-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 v6 5/7] android: Track and notify post\n\tprocessing of streams","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":20439,"web_url":"https://patchwork.libcamera.org/comment/20439/","msgid":"","date":"2021-10-25T13:46:56","subject":"Re: [libcamera-devel] [PATCH v6 5/7] android: Track and notify post\n\tprocessing of streams","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/25/21 6:55 PM, Laurent Pinchart wrote:\n> Hi Umang,\n>\n> On Mon, Oct 25, 2021 at 06:46:39PM +0530, Umang Jain wrote:\n>> On 10/25/21 11:33 AM, Laurent Pinchart wrote:\n>>> On Sat, Oct 23, 2021 at 04:03:00PM +0530, Umang Jain wrote:\n>>>> Notify that the post processing for a request has been completed,\n>>>> via a signal. The signal emit with a context pointer along with status\n>>> s/emit/is emitted/ ?\n>>>\n>>>> of the buffer. The function CameraDevice::streamProcessingComplete() will\n>>>> finally set the status on the request descriptor and complete the\n>>>> descriptor if all the streams requiring post processing are completed.\n>>>> If buffer status obtained is in error state, notify the status to the\n>>>> framework and set the overall error status on the descriptor via\n>>>> setBufferStatus().\n>>>>\n>>>> We need to track the number of streams requiring post-processing\n>>>> per Camera3RequestDescriptor (i.e. per capture request). Introduce\n>>>> a std::map<> to track the post-processing of streams. The nodes\n>>>> are dropped from the map when a particular stream post processing\n>>>> is completed (or on error paths). A std::map is selected for tracking\n>>>> post-processing requests, since we will move post-processing to be\n>>>> asynchronous in subsequent commits. A vector or queue will not be\n>>>> suitable then as the sequential order of post-processing completion\n>>>> of various requests won't be guaranteed then.\n>>>>\n>>>> A streamProcessMutex_ has been introduced here as well, which will be\n>>> s/streamProcessMutex_/streamsProcessMutex_/\n>>>\n>>>> applicable to guard access to descriptor's pendingStreamsToProcess_ when\n>>>> post-processing is moved to be asynchronous in subsequent commits.\n>>>>\n>>>> Signed-off-by: Umang Jain \n>>>> ---\n>>>> src/android/camera_device.cpp | 95 ++++++++++++++++++------\n>>>> src/android/camera_device.h | 4 +\n>>>> src/android/camera_request.h | 6 ++\n>>>> src/android/camera_stream.cpp | 15 ++++\n>>>> src/android/jpeg/post_processor_jpeg.cpp | 2 +\n>>>> src/android/post_processor.h | 9 +++\n>>>> src/android/yuv/post_processor_yuv.cpp | 8 +-\n>>>> 7 files changed, 114 insertions(+), 25 deletions(-)\n>>>>\n>>>> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n>>>> index 2a98a2e6..3114def0 100644\n>>>> --- a/src/android/camera_device.cpp\n>>>> +++ b/src/android/camera_device.cpp\n>>>> @@ -926,6 +926,8 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n>>>> \t\t\t * Request.\n>>>> \t\t\t */\n>>>> \t\t\tLOG(HAL, Debug) << ss.str() << \" (mapped)\";\n>>> Blank line here, or no blank line below.\n>>>\n>>>> +\t\t\tdescriptor->pendingStreamsToProcess_.insert(\n>>>> +\t\t\t\t{ cameraStream, &buffer });\n>>>> \t\t\tcontinue;\n>>>> \n>>>> \t\tcase CameraStream::Type::Direct:\n>>>> @@ -955,6 +957,9 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n>>>> \t\t\tframeBuffer = cameraStream->getBuffer();\n>>>> \t\t\tbuffer.internalBuffer = frameBuffer;\n>>>> \t\t\tLOG(HAL, Debug) << ss.str() << \" (internal)\";\n>>>> +\n>>>> +\t\t\tdescriptor->pendingStreamsToProcess_.insert(\n>>>> +\t\t\t\t{ cameraStream, &buffer });\n>>>> \t\t\tbreak;\n>>>> \t\t}\n>>>> \n>>>> @@ -1118,43 +1123,46 @@ void CameraDevice::requestComplete(Request *request)\n>>>> \t}\n>>>> \n>>>> \t/* Handle post-processing. */\n>>>> -\tbool hasPostProcessingErrors = false;\n>>>> -\tfor (auto &buffer : descriptor->buffers_) {\n>>>> -\t\tCameraStream *stream = buffer.stream;\n>>>> -\n>>>> -\t\tif (stream->type() == CameraStream::Type::Direct)\n>>>> -\t\t\tcontinue;\n>>>> +\tbool needPostProcessing = false;\n>>>> +\t/*\n>>>> +\t * \\todo Protect the loop below with streamProcessMutex_ when post\n>>>> +\t * processor runs asynchronously.\n>>>> +\t */\n>>>> +\tauto iter = descriptor->pendingStreamsToProcess_.begin();\n>>>> +\twhile (descriptor->pendingStreamsToProcess_.size() > 0) {\n>>> \twhile (!descriptor->pendingStreamsToProcess_.empty()) {\n>>>\n>>> But it's not correct. The fix is in 7/7:\n>>>\n>>> \twhile (iter != descriptor->pendingStreamsToProcess_.end()) {\n>> Yes, it was a tricky to handle the loop for async and sync variations\n>>\n>> For the async one, iter == end() was the only choice I  believe (Without\n>> introducing a counter)\n>>\n>>>> +\t\tCameraStream *stream = iter->first;\n>>>> +\t\tCamera3RequestDescriptor::StreamBuffer *buffer = iter->second;\n>>>> +\t\tneedPostProcessing = true;\n>>>> \n>>>> \t\tFrameBuffer *src = request->findBuffer(stream->stream());\n>>>> \t\tif (!src) {\n>>>> \t\t\tLOG(HAL, Error) << \"Failed to find a source stream buffer\";\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\thasPostProcessingErrors = true;\n>>>> +\t\t\tsetBufferStatus(*buffer, Camera3RequestDescriptor::Status::Error);\n>>>> +\t\t\titer = descriptor->pendingStreamsToProcess_.erase(iter);\n>>>> \t\t\tcontinue;\n>>>> \t\t}\n>>>> \n>>>> -\t\tint ret = stream->process(*src, buffer);\n>>>> +\t\t++iter;\n>>>> +\t\tint ret = stream->process(*src, *buffer);\n>>>> +\t\tif (ret) {\n>>>> +\t\t\tsetBufferStatus(*buffer, Camera3RequestDescriptor::Status::Error);\n>>>> +\t\t\tdescriptor->pendingStreamsToProcess_.erase(stream);\n>>>> +\t\t}\n>>>> +\t}\n>>>> \n>>>> +\tif (needPostProcessing) {\n>>>> \t\t/*\n>>>> -\t\t * If the framebuffer is internal to CameraStream return it back\n>>>> -\t\t * now that we're done processing it.\n>>>> +\t\t * \\todo We will require to check if we failed to queue\n>>>> +\t\t * post-processing requests when we migrate to post-processor\n>>>> +\t\t * running asynchronously.\n>>>> +\t\t *\n>>>> +\t\t * if (descriptor->pendingStreamsToProcess_.size() == 0)\n>>>> +\t\t *\tcompleteDescriptor(descriptor);\n>>> Can't we do this already here ? I think you can actually drop the\n>>> needPostProcessing variable and just write\n>>>\n>>> \tif (descriptor->pendingStreamsToProcess_.empty())\n>>> \t\tcompleteDescriptor(descriptor);\n>>>\n>>> as needPostProcessing can only be false if pendingStreamsToProcess_ was\n>>> empty before the while loop above, and it will thus be empty after the\n>>> loop as well in that case.\n>> True, but we need needPostProcessing variable for async.\n> Why is that ? Won't\n>\n> \tif (descriptor->pendingStreamsToProcess_.empty())\n> \t\tcompleteDescriptor(descriptor);\n>\n> work in async mode too ?\n\n\nOk yes, it should work (sorry I confused myself a bit, thinking it's \nwill be a double completeDescriptor(descriptor) for the same descriptor, \nin case post-processing completes early when main thread reaches this \nline. But I forgot that we are already holding a lock restricted slots \nto run)\n\nWe would end up requestComplete() like: https://paste.debian.net/1216820/\n\n>\n>> I pre-empted\n>> its introduction deliberately to set the design beforehand andthen,  I\n>> can introduce async bits with minimal diff for\n>> CamreraDevice::requestComplete()\n>>\n>>>> \t\t */\n>>>> -\t\tif (buffer.internalBuffer)\n>>>> -\t\t\tstream->putBuffer(buffer.internalBuffer);\n>>>> \n>>>> -\t\tif (ret) {\n>>>> -\t\t\tbuffer.status = Camera3RequestDescriptor::Status::Error;\n>>>> -\t\t\thasPostProcessingErrors = true;\n>>>> -\t\t\tnotifyError(descriptor->frameNumber_, stream->camera3Stream(),\n>>>> -\t\t\t\t CAMERA3_MSG_ERROR_BUFFER);\n>>>> -\t\t}\n>>>> +\t\treturn;\n>>>> \t}\n>>>> \n>>>> -\tif (hasPostProcessingErrors)\n>>>> -\t\tdescriptor->status_ = Camera3RequestDescriptor::Status::Error;\n>>>> -\n>>>> \tcompleteDescriptor(descriptor);\n>>>> }\n>>>> \n>>>> @@ -1210,6 +1218,45 @@ void CameraDevice::sendCaptureResults()\n>>>> \t}\n>>>> }\n>>>> \n>>>> +void CameraDevice::setBufferStatus(Camera3RequestDescriptor::StreamBuffer &streamBuffer,\n>>>> +\t\t\t\t Camera3RequestDescriptor::Status status)\n>>>> +{\n>>>> +\t/*\n>>>> +\t * If the framebuffer is internal to CameraStream return it back now\n>>>> +\t * that we're done processing it.\n>>>> +\t */\n>>>> +\tif (streamBuffer.internalBuffer)\n>>>> +\t\tstreamBuffer.stream->putBuffer(streamBuffer.internalBuffer);\n>>> I'd move this to the caller, as it's not about the buffer status.\n>>>\n>>>> +\n>>>> +\tstreamBuffer.status = status;\n>>>> +\tif (status != Camera3RequestDescriptor::Status::Success) {\n>>>> +\t\tnotifyError(streamBuffer.request->frameNumber_,\n>>>> +\t\t\t streamBuffer.stream->camera3Stream(),\n>>>> +\t\t\t CAMERA3_MSG_ERROR_BUFFER);\n>>>> +\n>>>> +\t\t/* Also set error status on entire request descriptor. */\n>>>> +\t\tstreamBuffer.request->status_ =\n>>>> +\t\t\tCamera3RequestDescriptor::Status::Error;\n>>>> +\t}\n>>>> +}\n>>>> +\n>>>> +void CameraDevice::streamProcessingComplete(Camera3RequestDescriptor::StreamBuffer *streamBuffer,\n>>>> +\t\t\t\t\t Camera3RequestDescriptor::Status status)\n>>>> +{\n>>>> +\tCamera3RequestDescriptor *request = streamBuffer->request;\n>>>> +\tMutexLocker locker(request->streamsProcessMutex_);\n>>>> +\n>>>> +\tsetBufferStatus(*streamBuffer, status);\n>>> Do we need to protect the setBufferStatus() call with\n>>> streamsProcessMutex_ ? I thought it only protects\n>>> pendingStreamsToProcess_.\n>>\n>> Ah, I don't see why we need to lock it. This is a good catch.\n>>\n>> For the async version, the main thread isn't expected to call\n>> setBufferStatus() on the same descriptor (because we are already\n>> handling synchronous errors beforehand), so I don't expect to race. So I\n>> should remove setBufferStatus() from the lock section.\n>>\n>>>> +\trequest->pendingStreamsToProcess_.erase(streamBuffer->stream);\n>>>> +\n>>>> +\tif (request->pendingStreamsToProcess_.size() > 0)\n>>> \tif (!request->pendingStreamsToProcess_.empty())\n>>>\n>>>> +\t\treturn;\n>>>> +\n>>>> +\tlocker.unlock();\n>>>> +\n>>>> +\tcompleteDescriptor(streamBuffer->request);\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 e544f2bd..2a414020 100644\n>>>> --- a/src/android/camera_device.h\n>>>> +++ b/src/android/camera_device.h\n>>>> @@ -66,6 +66,8 @@ 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(Camera3RequestDescriptor::StreamBuffer *bufferStream,\n>>>> +\t\t\t\t Camera3RequestDescriptor::Status status);\n>>>> \n>>>> protected:\n>>>> \tstd::string logPrefix() const override;\n>>>> @@ -95,6 +97,8 @@ private:\n>>>> \tint processControls(Camera3RequestDescriptor *descriptor);\n>>>> \tvoid completeDescriptor(Camera3RequestDescriptor *descriptor);\n>>>> \tvoid sendCaptureResults();\n>>>> +\tvoid setBufferStatus(Camera3RequestDescriptor::StreamBuffer &buffer,\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 c4bc5d6e..cc2b7035 100644\n>>>> --- a/src/android/camera_request.h\n>>>> +++ b/src/android/camera_request.h\n>>>> @@ -7,7 +7,9 @@\n>>>> #ifndef __ANDROID_CAMERA_REQUEST_H__\n>>>> #define __ANDROID_CAMERA_REQUEST_H__\n>>>> \n>>>> +#include \n>>>> #include \n>>>> +#include \n>>>> #include \n>>>> \n>>>> #include \n>>>> @@ -43,6 +45,10 @@ public:\n>>>> \t\tCamera3RequestDescriptor *request;\n>>>> \t};\n>>>> \n>>>> +\t/* Keeps track of streams requiring post-processing. */\n>>>> +\tstd::map pendingStreamsToProcess_;\n>>>> +\tstd::mutex streamsProcessMutex_;\n>>>> +\n>>>> \tCamera3RequestDescriptor(libcamera::Camera *camera,\n>>>> \t\t\t\t const camera3_capture_request_t *camera3Request);\n>>>> \t~Camera3RequestDescriptor();\n>>>> diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp\n>>>> index 0e268cdf..4e275cde 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,20 @@ 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::StreamBuffer *streamBuffer,\n>>>> +\t\t\t\t PostProcessor::Status status) {\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>>>> +\t\t\t\tcameraDevice_->streamProcessingComplete(streamBuffer,\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 da71f113..cbbe7128 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(Camera3RequestDescriptor::StreamBuffer *streamBuf\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(streamBuffer, PostProcessor::Status::Error);\n>>>> \t\treturn jpeg_size;\n>>>> \t}\n>>>> \n>>>> @@ -211,6 +212,7 @@ int PostProcessorJpeg::process(Camera3RequestDescriptor::StreamBuffer *streamBuf\n>>>> \n>>>> \t/* Update the JPEG result Metadata. */\n>>>> \tresultMetadata->addEntry(ANDROID_JPEG_SIZE, jpeg_size);\n>>>> +\tprocessComplete.emit(streamBuffer, 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 128161c8..4ac74fcf 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>>>> @@ -16,11 +18,18 @@\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>>>> \t\t\t const libcamera::StreamConfiguration &outCfg) = 0;\n>>>> \tvirtual int process(Camera3RequestDescriptor::StreamBuffer *streamBuffer) = 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 eeb8f1f4..8e77bf57 100644\n>>>> --- a/src/android/yuv/post_processor_yuv.cpp\n>>>> +++ b/src/android/yuv/post_processor_yuv.cpp\n>>>> @@ -54,12 +54,15 @@ int PostProcessorYuv::process(Camera3RequestDescriptor::StreamBuffer *streamBuff\n>>>> \tconst FrameBuffer &source = *streamBuffer->srcBuffer;\n>>>> \tCameraBuffer *destination = streamBuffer->destBuffer.get();\n>>>> \n>>>> -\tif (!isValidBuffers(source, *destination))\n>>>> +\tif (!isValidBuffers(source, *destination)) {\n>>>> +\t\tprocessComplete.emit(streamBuffer, 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(streamBuffer, PostProcessor::Status::Error);\n>>>> \t\treturn -EINVAL;\n>>>> \t}\n>>>> \n>>>> @@ -77,9 +80,12 @@ int PostProcessorYuv::process(Camera3RequestDescriptor::StreamBuffer *streamBuff\n>>>> \t\t\t\t libyuv::FilterMode::kFilterBilinear);\n>>>> \tif (ret) {\n>>>> \t\tLOG(YUV, Error) << \"Failed NV12 scaling: \" << ret;\n>>>> +\t\tprocessComplete.emit(streamBuffer, PostProcessor::Status::Error);\n>>>> \t\treturn -EINVAL;\n>>>> \t}\n>>>> \n>>>> +\tprocessComplete.emit(streamBuffer, 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 A1855BDB1C\n\tfor ;\n\tMon, 25 Oct 2021 13:47:04 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D70B56486E;\n\tMon, 25 Oct 2021 15:47:03 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 0D93160124\n\tfor ;\n\tMon, 25 Oct 2021 15:47:02 +0200 (CEST)","from [192.168.1.106] (unknown [103.251.226.211])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id B1BBFE0A;\n\tMon, 25 Oct 2021 15:47:00 +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=\"c85/Pt7F\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1635169621;\n\tbh=STG0NqZaWEeZs9o2FzCl8Z+VZrKCJgSO4GuG4GPPrqA=;\n\th=Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=c85/Pt7FiR4d00xKXxal/OJj/nVeqQG2UhpaOl/dGSC5uQ9xP9/qXTbOAoRzPHRyK\n\t8dTd6unh7Vs5sQq3Q+lu/OnimEhenkNiyIDJzMH1FxvKh14VRJmbHZ4j/A0VcpIM+S\n\tdzEbm/jaOMAaHtoc1V+SJrI0J5Lbk0Yj4/h6CjXY=","To":"Laurent Pinchart ","References":"<20211023103302.152769-1-umang.jain@ideasonboard.com>\n\t<20211023103302.152769-6-umang.jain@ideasonboard.com>\n\t\n\t\n\t","From":"Umang Jain ","Message-ID":"","Date":"Mon, 25 Oct 2021 19:16:56 +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 v6 5/7] android: Track and notify post\n\tprocessing of streams","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":20441,"web_url":"https://patchwork.libcamera.org/comment/20441/","msgid":"","date":"2021-10-25T13:53:45","subject":"Re: [libcamera-devel] [PATCH v6 5/7] android: Track and notify post\n\tprocessing of streams","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Umang,\n\nOn Mon, Oct 25, 2021 at 07:16:56PM +0530, Umang Jain wrote:\n> On 10/25/21 6:55 PM, Laurent Pinchart wrote:\n> > On Mon, Oct 25, 2021 at 06:46:39PM +0530, Umang Jain wrote:\n> >> On 10/25/21 11:33 AM, Laurent Pinchart wrote:\n> >>> On Sat, Oct 23, 2021 at 04:03:00PM +0530, Umang Jain wrote:\n> >>>> Notify that the post processing for a request has been completed,\n> >>>> via a signal. The signal emit with a context pointer along with status\n> >>> s/emit/is emitted/ ?\n> >>>\n> >>>> of the buffer. The function CameraDevice::streamProcessingComplete() will\n> >>>> finally set the status on the request descriptor and complete the\n> >>>> descriptor if all the streams requiring post processing are completed.\n> >>>> If buffer status obtained is in error state, notify the status to the\n> >>>> framework and set the overall error status on the descriptor via\n> >>>> setBufferStatus().\n> >>>>\n> >>>> We need to track the number of streams requiring post-processing\n> >>>> per Camera3RequestDescriptor (i.e. per capture request). Introduce\n> >>>> a std::map<> to track the post-processing of streams. The nodes\n> >>>> are dropped from the map when a particular stream post processing\n> >>>> is completed (or on error paths). A std::map is selected for tracking\n> >>>> post-processing requests, since we will move post-processing to be\n> >>>> asynchronous in subsequent commits. A vector or queue will not be\n> >>>> suitable then as the sequential order of post-processing completion\n> >>>> of various requests won't be guaranteed then.\n> >>>>\n> >>>> A streamProcessMutex_ has been introduced here as well, which will be\n> >>> s/streamProcessMutex_/streamsProcessMutex_/\n> >>>\n> >>>> applicable to guard access to descriptor's pendingStreamsToProcess_ when\n> >>>> post-processing is moved to be asynchronous in subsequent commits.\n> >>>>\n> >>>> Signed-off-by: Umang Jain \n> >>>> ---\n> >>>> src/android/camera_device.cpp | 95 ++++++++++++++++++------\n> >>>> src/android/camera_device.h | 4 +\n> >>>> src/android/camera_request.h | 6 ++\n> >>>> src/android/camera_stream.cpp | 15 ++++\n> >>>> src/android/jpeg/post_processor_jpeg.cpp | 2 +\n> >>>> src/android/post_processor.h | 9 +++\n> >>>> src/android/yuv/post_processor_yuv.cpp | 8 +-\n> >>>> 7 files changed, 114 insertions(+), 25 deletions(-)\n> >>>>\n> >>>> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> >>>> index 2a98a2e6..3114def0 100644\n> >>>> --- a/src/android/camera_device.cpp\n> >>>> +++ b/src/android/camera_device.cpp\n> >>>> @@ -926,6 +926,8 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n> >>>> \t\t\t * Request.\n> >>>> \t\t\t */\n> >>>> \t\t\tLOG(HAL, Debug) << ss.str() << \" (mapped)\";\n> >>> Blank line here, or no blank line below.\n> >>>\n> >>>> +\t\t\tdescriptor->pendingStreamsToProcess_.insert(\n> >>>> +\t\t\t\t{ cameraStream, &buffer });\n> >>>> \t\t\tcontinue;\n> >>>> \n> >>>> \t\tcase CameraStream::Type::Direct:\n> >>>> @@ -955,6 +957,9 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n> >>>> \t\t\tframeBuffer = cameraStream->getBuffer();\n> >>>> \t\t\tbuffer.internalBuffer = frameBuffer;\n> >>>> \t\t\tLOG(HAL, Debug) << ss.str() << \" (internal)\";\n> >>>> +\n> >>>> +\t\t\tdescriptor->pendingStreamsToProcess_.insert(\n> >>>> +\t\t\t\t{ cameraStream, &buffer });\n> >>>> \t\t\tbreak;\n> >>>> \t\t}\n> >>>> \n> >>>> @@ -1118,43 +1123,46 @@ void CameraDevice::requestComplete(Request *request)\n> >>>> \t}\n> >>>> \n> >>>> \t/* Handle post-processing. */\n> >>>> -\tbool hasPostProcessingErrors = false;\n> >>>> -\tfor (auto &buffer : descriptor->buffers_) {\n> >>>> -\t\tCameraStream *stream = buffer.stream;\n> >>>> -\n> >>>> -\t\tif (stream->type() == CameraStream::Type::Direct)\n> >>>> -\t\t\tcontinue;\n> >>>> +\tbool needPostProcessing = false;\n> >>>> +\t/*\n> >>>> +\t * \\todo Protect the loop below with streamProcessMutex_ when post\n> >>>> +\t * processor runs asynchronously.\n> >>>> +\t */\n> >>>> +\tauto iter = descriptor->pendingStreamsToProcess_.begin();\n> >>>> +\twhile (descriptor->pendingStreamsToProcess_.size() > 0) {\n> >>> \twhile (!descriptor->pendingStreamsToProcess_.empty()) {\n> >>>\n> >>> But it's not correct. The fix is in 7/7:\n> >>>\n> >>> \twhile (iter != descriptor->pendingStreamsToProcess_.end()) {\n> >> Yes, it was a tricky to handle the loop for async and sync variations\n> >>\n> >> For the async one, iter == end() was the only choice I  believe (Without\n> >> introducing a counter)\n> >>\n> >>>> +\t\tCameraStream *stream = iter->first;\n> >>>> +\t\tCamera3RequestDescriptor::StreamBuffer *buffer = iter->second;\n> >>>> +\t\tneedPostProcessing = true;\n> >>>> \n> >>>> \t\tFrameBuffer *src = request->findBuffer(stream->stream());\n> >>>> \t\tif (!src) {\n> >>>> \t\t\tLOG(HAL, Error) << \"Failed to find a source stream buffer\";\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\thasPostProcessingErrors = true;\n> >>>> +\t\t\tsetBufferStatus(*buffer, Camera3RequestDescriptor::Status::Error);\n> >>>> +\t\t\titer = descriptor->pendingStreamsToProcess_.erase(iter);\n> >>>> \t\t\tcontinue;\n> >>>> \t\t}\n> >>>> \n> >>>> -\t\tint ret = stream->process(*src, buffer);\n> >>>> +\t\t++iter;\n> >>>> +\t\tint ret = stream->process(*src, *buffer);\n> >>>> +\t\tif (ret) {\n> >>>> +\t\t\tsetBufferStatus(*buffer, Camera3RequestDescriptor::Status::Error);\n> >>>> +\t\t\tdescriptor->pendingStreamsToProcess_.erase(stream);\n> >>>> +\t\t}\n> >>>> +\t}\n> >>>> \n> >>>> +\tif (needPostProcessing) {\n> >>>> \t\t/*\n> >>>> -\t\t * If the framebuffer is internal to CameraStream return it back\n> >>>> -\t\t * now that we're done processing it.\n> >>>> +\t\t * \\todo We will require to check if we failed to queue\n> >>>> +\t\t * post-processing requests when we migrate to post-processor\n> >>>> +\t\t * running asynchronously.\n> >>>> +\t\t *\n> >>>> +\t\t * if (descriptor->pendingStreamsToProcess_.size() == 0)\n> >>>> +\t\t *\tcompleteDescriptor(descriptor);\n> >>> Can't we do this already here ? I think you can actually drop the\n> >>> needPostProcessing variable and just write\n> >>>\n> >>> \tif (descriptor->pendingStreamsToProcess_.empty())\n> >>> \t\tcompleteDescriptor(descriptor);\n> >>>\n> >>> as needPostProcessing can only be false if pendingStreamsToProcess_ was\n> >>> empty before the while loop above, and it will thus be empty after the\n> >>> loop as well in that case.\n> >> True, but we need needPostProcessing variable for async.\n> > Why is that ? Won't\n> >\n> > \tif (descriptor->pendingStreamsToProcess_.empty())\n> > \t\tcompleteDescriptor(descriptor);\n> >\n> > work in async mode too ?\n> \n> \n> Ok yes, it should work (sorry I confused myself a bit, thinking it's \n> will be a double completeDescriptor(descriptor) for the same descriptor, \n> in case post-processing completes early when main thread reaches this \n> line. But I forgot that we are already holding a lock restricted slots \n> to run)\n> \n> We would end up requestComplete() like: https://paste.debian.net/1216820/\n\nWith\n\n\tdescriptor->pendingStreamsToProcess_.size() == 0\n\nreplaced with\n\n\tdescriptor->pendingStreamsToProcess_.empty()\n\nit seems good to me.\n\n> >> I pre-empted\n> >> its introduction deliberately to set the design beforehand andthen,  I\n> >> can introduce async bits with minimal diff for\n> >> CamreraDevice::requestComplete()\n> >>\n> >>>> \t\t */\n> >>>> -\t\tif (buffer.internalBuffer)\n> >>>> -\t\t\tstream->putBuffer(buffer.internalBuffer);\n> >>>> \n> >>>> -\t\tif (ret) {\n> >>>> -\t\t\tbuffer.status = Camera3RequestDescriptor::Status::Error;\n> >>>> -\t\t\thasPostProcessingErrors = true;\n> >>>> -\t\t\tnotifyError(descriptor->frameNumber_, stream->camera3Stream(),\n> >>>> -\t\t\t\t CAMERA3_MSG_ERROR_BUFFER);\n> >>>> -\t\t}\n> >>>> +\t\treturn;\n> >>>> \t}\n> >>>> \n> >>>> -\tif (hasPostProcessingErrors)\n> >>>> -\t\tdescriptor->status_ = Camera3RequestDescriptor::Status::Error;\n> >>>> -\n> >>>> \tcompleteDescriptor(descriptor);\n> >>>> }\n> >>>> \n> >>>> @@ -1210,6 +1218,45 @@ void CameraDevice::sendCaptureResults()\n> >>>> \t}\n> >>>> }\n> >>>> \n> >>>> +void CameraDevice::setBufferStatus(Camera3RequestDescriptor::StreamBuffer &streamBuffer,\n> >>>> +\t\t\t\t Camera3RequestDescriptor::Status status)\n> >>>> +{\n> >>>> +\t/*\n> >>>> +\t * If the framebuffer is internal to CameraStream return it back now\n> >>>> +\t * that we're done processing it.\n> >>>> +\t */\n> >>>> +\tif (streamBuffer.internalBuffer)\n> >>>> +\t\tstreamBuffer.stream->putBuffer(streamBuffer.internalBuffer);\n> >>> I'd move this to the caller, as it's not about the buffer status.\n> >>>\n> >>>> +\n> >>>> +\tstreamBuffer.status = status;\n> >>>> +\tif (status != Camera3RequestDescriptor::Status::Success) {\n> >>>> +\t\tnotifyError(streamBuffer.request->frameNumber_,\n> >>>> +\t\t\t streamBuffer.stream->camera3Stream(),\n> >>>> +\t\t\t CAMERA3_MSG_ERROR_BUFFER);\n> >>>> +\n> >>>> +\t\t/* Also set error status on entire request descriptor. */\n> >>>> +\t\tstreamBuffer.request->status_ =\n> >>>> +\t\t\tCamera3RequestDescriptor::Status::Error;\n> >>>> +\t}\n> >>>> +}\n> >>>> +\n> >>>> +void CameraDevice::streamProcessingComplete(Camera3RequestDescriptor::StreamBuffer *streamBuffer,\n> >>>> +\t\t\t\t\t Camera3RequestDescriptor::Status status)\n> >>>> +{\n> >>>> +\tCamera3RequestDescriptor *request = streamBuffer->request;\n> >>>> +\tMutexLocker locker(request->streamsProcessMutex_);\n> >>>> +\n> >>>> +\tsetBufferStatus(*streamBuffer, status);\n> >>> Do we need to protect the setBufferStatus() call with\n> >>> streamsProcessMutex_ ? I thought it only protects\n> >>> pendingStreamsToProcess_.\n> >>\n> >> Ah, I don't see why we need to lock it. This is a good catch.\n> >>\n> >> For the async version, the main thread isn't expected to call\n> >> setBufferStatus() on the same descriptor (because we are already\n> >> handling synchronous errors beforehand), so I don't expect to race. So I\n> >> should remove setBufferStatus() from the lock section.\n> >>\n> >>>> +\trequest->pendingStreamsToProcess_.erase(streamBuffer->stream);\n> >>>> +\n> >>>> +\tif (request->pendingStreamsToProcess_.size() > 0)\n> >>> \tif (!request->pendingStreamsToProcess_.empty())\n> >>>\n> >>>> +\t\treturn;\n> >>>> +\n> >>>> +\tlocker.unlock();\n> >>>> +\n> >>>> +\tcompleteDescriptor(streamBuffer->request);\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 e544f2bd..2a414020 100644\n> >>>> --- a/src/android/camera_device.h\n> >>>> +++ b/src/android/camera_device.h\n> >>>> @@ -66,6 +66,8 @@ 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(Camera3RequestDescriptor::StreamBuffer *bufferStream,\n> >>>> +\t\t\t\t Camera3RequestDescriptor::Status status);\n> >>>> \n> >>>> protected:\n> >>>> \tstd::string logPrefix() const override;\n> >>>> @@ -95,6 +97,8 @@ private:\n> >>>> \tint processControls(Camera3RequestDescriptor *descriptor);\n> >>>> \tvoid completeDescriptor(Camera3RequestDescriptor *descriptor);\n> >>>> \tvoid sendCaptureResults();\n> >>>> +\tvoid setBufferStatus(Camera3RequestDescriptor::StreamBuffer &buffer,\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 c4bc5d6e..cc2b7035 100644\n> >>>> --- a/src/android/camera_request.h\n> >>>> +++ b/src/android/camera_request.h\n> >>>> @@ -7,7 +7,9 @@\n> >>>> #ifndef __ANDROID_CAMERA_REQUEST_H__\n> >>>> #define __ANDROID_CAMERA_REQUEST_H__\n> >>>> \n> >>>> +#include \n> >>>> #include \n> >>>> +#include \n> >>>> #include \n> >>>> \n> >>>> #include \n> >>>> @@ -43,6 +45,10 @@ public:\n> >>>> \t\tCamera3RequestDescriptor *request;\n> >>>> \t};\n> >>>> \n> >>>> +\t/* Keeps track of streams requiring post-processing. */\n> >>>> +\tstd::map pendingStreamsToProcess_;\n> >>>> +\tstd::mutex streamsProcessMutex_;\n> >>>> +\n> >>>> \tCamera3RequestDescriptor(libcamera::Camera *camera,\n> >>>> \t\t\t\t const camera3_capture_request_t *camera3Request);\n> >>>> \t~Camera3RequestDescriptor();\n> >>>> diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp\n> >>>> index 0e268cdf..4e275cde 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,20 @@ 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::StreamBuffer *streamBuffer,\n> >>>> +\t\t\t\t PostProcessor::Status status) {\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> >>>> +\t\t\t\tcameraDevice_->streamProcessingComplete(streamBuffer,\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 da71f113..cbbe7128 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(Camera3RequestDescriptor::StreamBuffer *streamBuf\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(streamBuffer, PostProcessor::Status::Error);\n> >>>> \t\treturn jpeg_size;\n> >>>> \t}\n> >>>> \n> >>>> @@ -211,6 +212,7 @@ int PostProcessorJpeg::process(Camera3RequestDescriptor::StreamBuffer *streamBuf\n> >>>> \n> >>>> \t/* Update the JPEG result Metadata. */\n> >>>> \tresultMetadata->addEntry(ANDROID_JPEG_SIZE, jpeg_size);\n> >>>> +\tprocessComplete.emit(streamBuffer, 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 128161c8..4ac74fcf 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> >>>> @@ -16,11 +18,18 @@\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> >>>> \t\t\t const libcamera::StreamConfiguration &outCfg) = 0;\n> >>>> \tvirtual int process(Camera3RequestDescriptor::StreamBuffer *streamBuffer) = 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 eeb8f1f4..8e77bf57 100644\n> >>>> --- a/src/android/yuv/post_processor_yuv.cpp\n> >>>> +++ b/src/android/yuv/post_processor_yuv.cpp\n> >>>> @@ -54,12 +54,15 @@ int PostProcessorYuv::process(Camera3RequestDescriptor::StreamBuffer *streamBuff\n> >>>> \tconst FrameBuffer &source = *streamBuffer->srcBuffer;\n> >>>> \tCameraBuffer *destination = streamBuffer->destBuffer.get();\n> >>>> \n> >>>> -\tif (!isValidBuffers(source, *destination))\n> >>>> +\tif (!isValidBuffers(source, *destination)) {\n> >>>> +\t\tprocessComplete.emit(streamBuffer, 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(streamBuffer, PostProcessor::Status::Error);\n> >>>> \t\treturn -EINVAL;\n> >>>> \t}\n> >>>> \n> >>>> @@ -77,9 +80,12 @@ int PostProcessorYuv::process(Camera3RequestDescriptor::StreamBuffer *streamBuff\n> >>>> \t\t\t\t libyuv::FilterMode::kFilterBilinear);\n> >>>> \tif (ret) {\n> >>>> \t\tLOG(YUV, Error) << \"Failed NV12 scaling: \" << ret;\n> >>>> +\t\tprocessComplete.emit(streamBuffer, PostProcessor::Status::Error);\n> >>>> \t\treturn -EINVAL;\n> >>>> \t}\n> >>>> \n> >>>> +\tprocessComplete.emit(streamBuffer, 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 99D41BF415\n\tfor ;\n\tMon, 25 Oct 2021 13:54:09 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0587C64870;\n\tMon, 25 Oct 2021 15:54:09 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 9151C60126\n\tfor ;\n\tMon, 25 Oct 2021 15:54:07 +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 15588E0A;\n\tMon, 25 Oct 2021 15:54:07 +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=\"V004G0pi\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1635170047;\n\tbh=SDVrTwi5c0MxLiDb+P2WLo6zOKjj+Ox/Fh1S+iieM8E=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=V004G0pinqjJqZeAZcIFeGJtZOzw55M1ijwX6kzCoHmiF9inM4+wsM1KEcjovCyrk\n\tqLd0MtCA05ys3zYhAdaq65MwevIWFlxcjW5KoaFaT/EmWvXIB1wNQTqOK/hW8H7pMb\n\tl+LP+H3WceuMgxN4g8/P4x5Bosj77w4s9s0ZlX/k=","Date":"Mon, 25 Oct 2021 16:53:45 +0300","From":"Laurent Pinchart ","To":"Umang Jain ","Message-ID":"","References":"<20211023103302.152769-1-umang.jain@ideasonboard.com>\n\t<20211023103302.152769-6-umang.jain@ideasonboard.com>\n\t\n\t\n\t\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v6 5/7] android: Track and notify post\n\tprocessing of streams","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\" "}}]