[libcamera-devel,v6,5/7] android: Track and notify post processing of streams

14291 diff mbox series

Notify that the post processing for a request has been completed,
via a signal. The signal emit with a context pointer along with status
of the buffer. The function CameraDevice::streamProcessingComplete() will
finally set the status on the request descriptor and complete the
descriptor if all the streams requiring post processing are completed.
If buffer status obtained is in error state, notify the status to the
framework and set the overall error status on the descriptor via
setBufferStatus().

We need to track the number of streams requiring post-processing
per Camera3RequestDescriptor (i.e. per capture request). Introduce
a std::map<> to track the post-processing of streams. The nodes
are dropped from the map when a particular stream post processing
is completed (or on error paths). A std::map is selected for tracking
post-processing requests, since we will move post-processing to be
asynchronous in subsequent commits. A vector or queue will not be
suitable then as the sequential order of post-processing completion
of various requests won't be guaranteed then.

A streamProcessMutex_ has been introduced here as well, which will be
applicable to guard access to descriptor's pendingStreamsToProcess_ when
post-processing is moved to be asynchronous in subsequent commits.

Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
---
 src/android/camera_device.cpp            | 95 ++++++++++++++++++------
 src/android/camera_device.h              |  4 +
 src/android/camera_request.h             |  6 ++
 src/android/camera_stream.cpp            | 15 ++++
 src/android/jpeg/post_processor_jpeg.cpp |  2 +
 src/android/post_processor.h             |  9 +++
 src/android/yuv/post_processor_yuv.cpp   |  8 +-
 7 files changed, 114 insertions(+), 25 deletions(-)

Hi Umang,

Thank you for the patch.

On Sat, Oct 23, 2021 at 04:03:00PM +0530, Umang Jain wrote:
> Notify that the post processing for a request has been completed,
> via a signal. The signal emit with a context pointer along with status

s/emit/is emitted/ ?

> of the buffer. The function CameraDevice::streamProcessingComplete() will
> finally set the status on the request descriptor and complete the
> descriptor if all the streams requiring post processing are completed.
> If buffer status obtained is in error state, notify the status to the
> framework and set the overall error status on the descriptor via
> setBufferStatus().
> 
> We need to track the number of streams requiring post-processing
> per Camera3RequestDescriptor (i.e. per capture request). Introduce
> a std::map<> to track the post-processing of streams. The nodes
> are dropped from the map when a particular stream post processing
> is completed (or on error paths). A std::map is selected for tracking
> post-processing requests, since we will move post-processing to be
> asynchronous in subsequent commits. A vector or queue will not be
> suitable then as the sequential order of post-processing completion
> of various requests won't be guaranteed then.
> 
> A streamProcessMutex_ has been introduced here as well, which will be

s/streamProcessMutex_/streamsProcessMutex_/

> applicable to guard access to descriptor's pendingStreamsToProcess_ when
> post-processing is moved to be asynchronous in subsequent commits.
> 
> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
> ---
>  src/android/camera_device.cpp            | 95 ++++++++++++++++++------
>  src/android/camera_device.h              |  4 +
>  src/android/camera_request.h             |  6 ++
>  src/android/camera_stream.cpp            | 15 ++++
>  src/android/jpeg/post_processor_jpeg.cpp |  2 +
>  src/android/post_processor.h             |  9 +++
>  src/android/yuv/post_processor_yuv.cpp   |  8 +-
>  7 files changed, 114 insertions(+), 25 deletions(-)
> 
> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> index 2a98a2e6..3114def0 100644
> --- a/src/android/camera_device.cpp
> +++ b/src/android/camera_device.cpp
> @@ -926,6 +926,8 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
>  			 * Request.
>  			 */
>  			LOG(HAL, Debug) << ss.str() << " (mapped)";

Blank line here, or no blank line below.

> +			descriptor->pendingStreamsToProcess_.insert(
> +				{ cameraStream, &buffer });
>  			continue;
>  
>  		case CameraStream::Type::Direct:
> @@ -955,6 +957,9 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
>  			frameBuffer = cameraStream->getBuffer();
>  			buffer.internalBuffer = frameBuffer;
>  			LOG(HAL, Debug) << ss.str() << " (internal)";
> +
> +			descriptor->pendingStreamsToProcess_.insert(
> +				{ cameraStream, &buffer });
>  			break;
>  		}
>  
> @@ -1118,43 +1123,46 @@ void CameraDevice::requestComplete(Request *request)
>  	}
>  
>  	/* Handle post-processing. */
> -	bool hasPostProcessingErrors = false;
> -	for (auto &buffer : descriptor->buffers_) {
> -		CameraStream *stream = buffer.stream;
> -
> -		if (stream->type() == CameraStream::Type::Direct)
> -			continue;
> +	bool needPostProcessing = false;
> +	/*
> +	 * \todo Protect the loop below with streamProcessMutex_ when post
> +	 * processor runs asynchronously.
> +	 */
> +	auto iter = descriptor->pendingStreamsToProcess_.begin();
> +	while (descriptor->pendingStreamsToProcess_.size() > 0) {

	while (!descriptor->pendingStreamsToProcess_.empty()) {

But it's not correct. The fix is in 7/7:

	while (iter != descriptor->pendingStreamsToProcess_.end()) {

> +		CameraStream *stream = iter->first;
> +		Camera3RequestDescriptor::StreamBuffer *buffer = iter->second;
> +		needPostProcessing = true;
>  
>  		FrameBuffer *src = request->findBuffer(stream->stream());
>  		if (!src) {
>  			LOG(HAL, Error) << "Failed to find a source stream buffer";
> -			buffer.status = Camera3RequestDescriptor::Status::Error;
> -			notifyError(descriptor->frameNumber_, stream->camera3Stream(),
> -				    CAMERA3_MSG_ERROR_BUFFER);
> -			hasPostProcessingErrors = true;
> +			setBufferStatus(*buffer, Camera3RequestDescriptor::Status::Error);
> +			iter = descriptor->pendingStreamsToProcess_.erase(iter);
>  			continue;
>  		}
>  
> -		int ret = stream->process(*src, buffer);
> +		++iter;
> +		int ret = stream->process(*src, *buffer);
> +		if (ret) {
> +			setBufferStatus(*buffer, Camera3RequestDescriptor::Status::Error);
> +			descriptor->pendingStreamsToProcess_.erase(stream);
> +		}
> +	}
>  
> +	if (needPostProcessing) {
>  		/*
> -		 * If the framebuffer is internal to CameraStream return it back
> -		 * now that we're done processing it.
> +		 * \todo We will require to check if we failed to queue
> +		 * post-processing requests when we migrate to post-processor
> +		 * running asynchronously.
> +		 *
> +		 * if (descriptor->pendingStreamsToProcess_.size() == 0)
> +		 *	completeDescriptor(descriptor);

Can't we do this already here ? I think you can actually drop the
needPostProcessing variable and just write

	if (descriptor->pendingStreamsToProcess_.empty())
		completeDescriptor(descriptor);

as needPostProcessing can only be false if pendingStreamsToProcess_ was
empty before the while loop above, and it will thus be empty after the
loop as well in that case.

>  		 */
> -		if (buffer.internalBuffer)
> -			stream->putBuffer(buffer.internalBuffer);
>  
> -		if (ret) {
> -			buffer.status = Camera3RequestDescriptor::Status::Error;
> -			hasPostProcessingErrors = true;
> -			notifyError(descriptor->frameNumber_, stream->camera3Stream(),
> -				    CAMERA3_MSG_ERROR_BUFFER);
> -		}
> +		return;
>  	}
>  
> -	if (hasPostProcessingErrors)
> -		descriptor->status_ = Camera3RequestDescriptor::Status::Error;
> -
>  	completeDescriptor(descriptor);
>  }
>  
> @@ -1210,6 +1218,45 @@ void CameraDevice::sendCaptureResults()
>  	}
>  }
>  
> +void CameraDevice::setBufferStatus(Camera3RequestDescriptor::StreamBuffer &streamBuffer,
> +				   Camera3RequestDescriptor::Status status)
> +{
> +	/*
> +	 * If the framebuffer is internal to CameraStream return it back now
> +	 * that we're done processing it.
> +	 */
> +	if (streamBuffer.internalBuffer)
> +		streamBuffer.stream->putBuffer(streamBuffer.internalBuffer);

I'd move this to the caller, as it's not about the buffer status.

> +
> +	streamBuffer.status = status;
> +	if (status != Camera3RequestDescriptor::Status::Success) {
> +		notifyError(streamBuffer.request->frameNumber_,
> +			    streamBuffer.stream->camera3Stream(),
> +			    CAMERA3_MSG_ERROR_BUFFER);
> +
> +		/* Also set error status on entire request descriptor. */
> +		streamBuffer.request->status_ =
> +			Camera3RequestDescriptor::Status::Error;
> +	}
> +}
> +
> +void CameraDevice::streamProcessingComplete(Camera3RequestDescriptor::StreamBuffer *streamBuffer,
> +					    Camera3RequestDescriptor::Status status)
> +{
> +	Camera3RequestDescriptor *request = streamBuffer->request;
> +	MutexLocker locker(request->streamsProcessMutex_);
> +
> +	setBufferStatus(*streamBuffer, status);

Do we need to protect the setBufferStatus() call with
streamsProcessMutex_ ? I thought it only protects
pendingStreamsToProcess_.

> +	request->pendingStreamsToProcess_.erase(streamBuffer->stream);
> +
> +	if (request->pendingStreamsToProcess_.size() > 0)

	if (!request->pendingStreamsToProcess_.empty())

> +		return;
> +
> +	locker.unlock();
> +
> +	completeDescriptor(streamBuffer->request);
> +}
> +
>  std::string CameraDevice::logPrefix() const
>  {
>  	return "'" + camera_->id() + "'";
> diff --git a/src/android/camera_device.h b/src/android/camera_device.h
> index e544f2bd..2a414020 100644
> --- a/src/android/camera_device.h
> +++ b/src/android/camera_device.h
> @@ -66,6 +66,8 @@ public:
>  	int configureStreams(camera3_stream_configuration_t *stream_list);
>  	int processCaptureRequest(camera3_capture_request_t *request);
>  	void requestComplete(libcamera::Request *request);
> +	void streamProcessingComplete(Camera3RequestDescriptor::StreamBuffer *bufferStream,
> +				      Camera3RequestDescriptor::Status status);
>  
>  protected:
>  	std::string logPrefix() const override;
> @@ -95,6 +97,8 @@ private:
>  	int processControls(Camera3RequestDescriptor *descriptor);
>  	void completeDescriptor(Camera3RequestDescriptor *descriptor);
>  	void sendCaptureResults();
> +	void setBufferStatus(Camera3RequestDescriptor::StreamBuffer &buffer,
> +			     Camera3RequestDescriptor::Status status);
>  	std::unique_ptr<CameraMetadata> getResultMetadata(
>  		const Camera3RequestDescriptor &descriptor) const;
>  
> diff --git a/src/android/camera_request.h b/src/android/camera_request.h
> index c4bc5d6e..cc2b7035 100644
> --- a/src/android/camera_request.h
> +++ b/src/android/camera_request.h
> @@ -7,7 +7,9 @@
>  #ifndef __ANDROID_CAMERA_REQUEST_H__
>  #define __ANDROID_CAMERA_REQUEST_H__
>  
> +#include <map>
>  #include <memory>
> +#include <mutex>
>  #include <vector>
>  
>  #include <libcamera/base/class.h>
> @@ -43,6 +45,10 @@ public:
>  		Camera3RequestDescriptor *request;
>  	};
>  
> +	/* Keeps track of streams requiring post-processing. */
> +	std::map<CameraStream *, StreamBuffer *> pendingStreamsToProcess_;
> +	std::mutex streamsProcessMutex_;
> +
>  	Camera3RequestDescriptor(libcamera::Camera *camera,
>  				 const camera3_capture_request_t *camera3Request);
>  	~Camera3RequestDescriptor();
> diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp
> index 0e268cdf..4e275cde 100644
> --- a/src/android/camera_stream.cpp
> +++ b/src/android/camera_stream.cpp
> @@ -22,6 +22,7 @@
>  #include "camera_capabilities.h"
>  #include "camera_device.h"
>  #include "camera_metadata.h"
> +#include "post_processor.h"
>  
>  using namespace libcamera;
>  
> @@ -97,6 +98,20 @@ int CameraStream::configure()
>  		int ret = postProcessor_->configure(configuration(), output);
>  		if (ret)
>  			return ret;
> +
> +		postProcessor_->processComplete.connect(
> +			this, [&](Camera3RequestDescriptor::StreamBuffer *streamBuffer,
> +				  PostProcessor::Status status) {
> +				Camera3RequestDescriptor::Status bufferStatus;
> +
> +				if (status == PostProcessor::Status::Success)
> +					bufferStatus = Camera3RequestDescriptor::Status::Success;
> +				else
> +					bufferStatus = Camera3RequestDescriptor::Status::Error;
> +
> +				cameraDevice_->streamProcessingComplete(streamBuffer,
> +									bufferStatus);
> +			});
>  	}
>  
>  	if (type_ == Type::Internal) {
> diff --git a/src/android/jpeg/post_processor_jpeg.cpp b/src/android/jpeg/post_processor_jpeg.cpp
> index da71f113..cbbe7128 100644
> --- a/src/android/jpeg/post_processor_jpeg.cpp
> +++ b/src/android/jpeg/post_processor_jpeg.cpp
> @@ -198,6 +198,7 @@ int PostProcessorJpeg::process(Camera3RequestDescriptor::StreamBuffer *streamBuf
>  					 exif.data(), quality);
>  	if (jpeg_size < 0) {
>  		LOG(JPEG, Error) << "Failed to encode stream image";
> +		processComplete.emit(streamBuffer, PostProcessor::Status::Error);
>  		return jpeg_size;
>  	}
>  
> @@ -211,6 +212,7 @@ int PostProcessorJpeg::process(Camera3RequestDescriptor::StreamBuffer *streamBuf
>  
>  	/* Update the JPEG result Metadata. */
>  	resultMetadata->addEntry(ANDROID_JPEG_SIZE, jpeg_size);
> +	processComplete.emit(streamBuffer, PostProcessor::Status::Success);
>  
>  	return 0;
>  }
> diff --git a/src/android/post_processor.h b/src/android/post_processor.h
> index 128161c8..4ac74fcf 100644
> --- a/src/android/post_processor.h
> +++ b/src/android/post_processor.h
> @@ -7,6 +7,8 @@
>  #ifndef __ANDROID_POST_PROCESSOR_H__
>  #define __ANDROID_POST_PROCESSOR_H__
>  
> +#include <libcamera/base/signal.h>
> +
>  #include <libcamera/framebuffer.h>
>  #include <libcamera/stream.h>
>  
> @@ -16,11 +18,18 @@
>  class PostProcessor
>  {
>  public:
> +	enum class Status {
> +		Error,
> +		Success
> +	};
> +
>  	virtual ~PostProcessor() = default;
>  
>  	virtual int configure(const libcamera::StreamConfiguration &inCfg,
>  			      const libcamera::StreamConfiguration &outCfg) = 0;
>  	virtual int process(Camera3RequestDescriptor::StreamBuffer *streamBuffer) = 0;
> +
> +	libcamera::Signal<Camera3RequestDescriptor::StreamBuffer *, Status> processComplete;
>  };
>  
>  #endif /* __ANDROID_POST_PROCESSOR_H__ */
> diff --git a/src/android/yuv/post_processor_yuv.cpp b/src/android/yuv/post_processor_yuv.cpp
> index eeb8f1f4..8e77bf57 100644
> --- a/src/android/yuv/post_processor_yuv.cpp
> +++ b/src/android/yuv/post_processor_yuv.cpp
> @@ -54,12 +54,15 @@ int PostProcessorYuv::process(Camera3RequestDescriptor::StreamBuffer *streamBuff
>  	const FrameBuffer &source = *streamBuffer->srcBuffer;
>  	CameraBuffer *destination = streamBuffer->destBuffer.get();
>  
> -	if (!isValidBuffers(source, *destination))
> +	if (!isValidBuffers(source, *destination)) {
> +		processComplete.emit(streamBuffer, PostProcessor::Status::Error);
>  		return -EINVAL;
> +	}
>  
>  	const MappedFrameBuffer sourceMapped(&source, MappedFrameBuffer::MapFlag::Read);
>  	if (!sourceMapped.isValid()) {
>  		LOG(YUV, Error) << "Failed to mmap camera frame buffer";
> +		processComplete.emit(streamBuffer, PostProcessor::Status::Error);
>  		return -EINVAL;
>  	}
>  
> @@ -77,9 +80,12 @@ int PostProcessorYuv::process(Camera3RequestDescriptor::StreamBuffer *streamBuff
>  				    libyuv::FilterMode::kFilterBilinear);
>  	if (ret) {
>  		LOG(YUV, Error) << "Failed NV12 scaling: " << ret;
> +		processComplete.emit(streamBuffer, PostProcessor::Status::Error);
>  		return -EINVAL;
>  	}
>  
> +	processComplete.emit(streamBuffer, PostProcessor::Status::Success);
> +
>  	return 0;
>  }
>

Hi Umang, thank you for the patch.

On Mon, Oct 25, 2021 at 3:03 PM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Umang,
>
> Thank you for the patch.
>
> On Sat, Oct 23, 2021 at 04:03:00PM +0530, Umang Jain wrote:
> > Notify that the post processing for a request has been completed,
> > via a signal. The signal emit with a context pointer along with status
>
> s/emit/is emitted/ ?
>
> > of the buffer. The function CameraDevice::streamProcessingComplete() will
> > finally set the status on the request descriptor and complete the
> > descriptor if all the streams requiring post processing are completed.
> > If buffer status obtained is in error state, notify the status to the
> > framework and set the overall error status on the descriptor via
> > setBufferStatus().
> >
> > We need to track the number of streams requiring post-processing
> > per Camera3RequestDescriptor (i.e. per capture request). Introduce
> > a std::map<> to track the post-processing of streams. The nodes
> > are dropped from the map when a particular stream post processing
> > is completed (or on error paths). A std::map is selected for tracking
> > post-processing requests, since we will move post-processing to be
> > asynchronous in subsequent commits. A vector or queue will not be
> > suitable then as the sequential order of post-processing completion
> > of various requests won't be guaranteed then.
> >
> > A streamProcessMutex_ has been introduced here as well, which will be
>
> s/streamProcessMutex_/streamsProcessMutex_/
>
> > applicable to guard access to descriptor's pendingStreamsToProcess_ when
> > post-processing is moved to be asynchronous in subsequent commits.
> >
> > Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
> > ---
> >  src/android/camera_device.cpp            | 95 ++++++++++++++++++------
> >  src/android/camera_device.h              |  4 +
> >  src/android/camera_request.h             |  6 ++
> >  src/android/camera_stream.cpp            | 15 ++++
> >  src/android/jpeg/post_processor_jpeg.cpp |  2 +
> >  src/android/post_processor.h             |  9 +++
> >  src/android/yuv/post_processor_yuv.cpp   |  8 +-
> >  7 files changed, 114 insertions(+), 25 deletions(-)
> >
> > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> > index 2a98a2e6..3114def0 100644
> > --- a/src/android/camera_device.cpp
> > +++ b/src/android/camera_device.cpp
> > @@ -926,6 +926,8 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
> >                        * Request.
> >                        */
> >                       LOG(HAL, Debug) << ss.str() << " (mapped)";
>
> Blank line here, or no blank line below.
>
> > +                     descriptor->pendingStreamsToProcess_.insert(
> > +                             { cameraStream, &buffer });
> >                       continue;
> >
> >               case CameraStream::Type::Direct:
> > @@ -955,6 +957,9 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
> >                       frameBuffer = cameraStream->getBuffer();
> >                       buffer.internalBuffer = frameBuffer;
> >                       LOG(HAL, Debug) << ss.str() << " (internal)";
> > +
> > +                     descriptor->pendingStreamsToProcess_.insert(
> > +                             { cameraStream, &buffer });
> >                       break;
> >               }
> >
> > @@ -1118,43 +1123,46 @@ void CameraDevice::requestComplete(Request *request)
> >       }
> >
> >       /* Handle post-processing. */
> > -     bool hasPostProcessingErrors = false;
> > -     for (auto &buffer : descriptor->buffers_) {
> > -             CameraStream *stream = buffer.stream;
> > -
> > -             if (stream->type() == CameraStream::Type::Direct)
> > -                     continue;
> > +     bool needPostProcessing = false;

nit: can just be
const bool needPostProcessing = !descriptor->pendingStreamToProcess.empty()?

> > +     /*
> > +      * \todo Protect the loop below with streamProcessMutex_ when post
> > +      * processor runs asynchronously.
> > +      */
> > +     auto iter = descriptor->pendingStreamsToProcess_.begin();
> > +     while (descriptor->pendingStreamsToProcess_.size() > 0) {
>
>         while (!descriptor->pendingStreamsToProcess_.empty()) {
>
> But it's not correct. The fix is in 7/7:
>
>         while (iter != descriptor->pendingStreamsToProcess_.end()) {
>
> > +             CameraStream *stream = iter->first;
> > +             Camera3RequestDescriptor::StreamBuffer *buffer = iter->second;
> > +             needPostProcessing = true;
> >
> >               FrameBuffer *src = request->findBuffer(stream->stream());
> >               if (!src) {
> >                       LOG(HAL, Error) << "Failed to find a source stream buffer";
> > -                     buffer.status = Camera3RequestDescriptor::Status::Error;
> > -                     notifyError(descriptor->frameNumber_, stream->camera3Stream(),
> > -                                 CAMERA3_MSG_ERROR_BUFFER);
> > -                     hasPostProcessingErrors = true;
> > +                     setBufferStatus(*buffer, Camera3RequestDescriptor::Status::Error);
> > +                     iter = descriptor->pendingStreamsToProcess_.erase(iter);

Do we need to proceed processing in this case?


> >                       continue;
> >               }
> >
> > -             int ret = stream->process(*src, buffer);
> > +             ++iter;
> > +             int ret = stream->process(*src, *buffer);
> > +             if (ret) {
> > +                     setBufferStatus(*buffer, Camera3RequestDescriptor::Status::Error);
> > +                     descriptor->pendingStreamsToProcess_.erase(stream);
> > +             }
> > +     }
> >
> > +     if (needPostProcessing) {
> >               /*
> > -              * If the framebuffer is internal to CameraStream return it back
> > -              * now that we're done processing it.
> > +              * \todo We will require to check if we failed to queue
> > +              * post-processing requests when we migrate to post-processor
> > +              * running asynchronously.
> > +              *
> > +              * if (descriptor->pendingStreamsToProcess_.size() == 0)
> > +              *      completeDescriptor(descriptor);
>
> Can't we do this already here ? I think you can actually drop the
> needPostProcessing variable and just write
>
>         if (descriptor->pendingStreamsToProcess_.empty())
>                 completeDescriptor(descriptor);
>
> as needPostProcessing can only be false if pendingStreamsToProcess_ was
> empty before the while loop above, and it will thus be empty after the
> loop as well in that case.
>
> >                */
> > -             if (buffer.internalBuffer)
> > -                     stream->putBuffer(buffer.internalBuffer);
> >
> > -             if (ret) {
> > -                     buffer.status = Camera3RequestDescriptor::Status::Error;
> > -                     hasPostProcessingErrors = true;
> > -                     notifyError(descriptor->frameNumber_, stream->camera3Stream(),
> > -                                 CAMERA3_MSG_ERROR_BUFFER);
> > -             }
> > +             return;
> >       }
> >
> > -     if (hasPostProcessingErrors)
> > -             descriptor->status_ = Camera3RequestDescriptor::Status::Error;
> > -
> >       completeDescriptor(descriptor);
> >  }
> >
> > @@ -1210,6 +1218,45 @@ void CameraDevice::sendCaptureResults()
> >       }
> >  }
> >
> > +void CameraDevice::setBufferStatus(Camera3RequestDescriptor::StreamBuffer &streamBuffer,
> > +                                Camera3RequestDescriptor::Status status)
> > +{
> > +     /*
> > +      * If the framebuffer is internal to CameraStream return it back now
> > +      * that we're done processing it.
> > +      */
> > +     if (streamBuffer.internalBuffer)
> > +             streamBuffer.stream->putBuffer(streamBuffer.internalBuffer);
>
> I'd move this to the caller, as it's not about the buffer status.
>
> > +
> > +     streamBuffer.status = status;
> > +     if (status != Camera3RequestDescriptor::Status::Success) {
> > +             notifyError(streamBuffer.request->frameNumber_,
> > +                         streamBuffer.stream->camera3Stream(),
> > +                         CAMERA3_MSG_ERROR_BUFFER);
> > +
> > +             /* Also set error status on entire request descriptor. */
> > +             streamBuffer.request->status_ =
> > +                     Camera3RequestDescriptor::Status::Error;
> > +     }
> > +}
> > +
> > +void CameraDevice::streamProcessingComplete(Camera3RequestDescriptor::StreamBuffer *streamBuffer,
> > +                                         Camera3RequestDescriptor::Status status)
> > +{
> > +     Camera3RequestDescriptor *request = streamBuffer->request;
> > +     MutexLocker locker(request->streamsProcessMutex_);
> > +
> > +     setBufferStatus(*streamBuffer, status);
>
> Do we need to protect the setBufferStatus() call with
> streamsProcessMutex_ ? I thought it only protects
> pendingStreamsToProcess_.
>

I think if this function is called by any thread, we need to protect status too.
If it's true, I would rename the mutex name.

> > +     request->pendingStreamsToProcess_.erase(streamBuffer->stream);
> > +
> > +     if (request->pendingStreamsToProcess_.size() > 0)
>
>         if (!request->pendingStreamsToProcess_.empty())
>
> > +             return;
> > +
> > +     locker.unlock();
> > +
> > +     completeDescriptor(streamBuffer->request);
> > +}
> > +
> >  std::string CameraDevice::logPrefix() const
> >  {
> >       return "'" + camera_->id() + "'";
> > diff --git a/src/android/camera_device.h b/src/android/camera_device.h
> > index e544f2bd..2a414020 100644
> > --- a/src/android/camera_device.h
> > +++ b/src/android/camera_device.h
> > @@ -66,6 +66,8 @@ public:
> >       int configureStreams(camera3_stream_configuration_t *stream_list);
> >       int processCaptureRequest(camera3_capture_request_t *request);
> >       void requestComplete(libcamera::Request *request);
> > +     void streamProcessingComplete(Camera3RequestDescriptor::StreamBuffer *bufferStream,

nit: streamBuffer may be better name?

> > +                                   Camera3RequestDescriptor::Status status);
> >
> >  protected:
> >       std::string logPrefix() const override;
> > @@ -95,6 +97,8 @@ private:
> >       int processControls(Camera3RequestDescriptor *descriptor);
> >       void completeDescriptor(Camera3RequestDescriptor *descriptor);
> >       void sendCaptureResults();
> > +     void setBufferStatus(Camera3RequestDescriptor::StreamBuffer &buffer,
> > +                          Camera3RequestDescriptor::Status status);
> >       std::unique_ptr<CameraMetadata> getResultMetadata(
> >               const Camera3RequestDescriptor &descriptor) const;
> >
> > diff --git a/src/android/camera_request.h b/src/android/camera_request.h
> > index c4bc5d6e..cc2b7035 100644
> > --- a/src/android/camera_request.h
> > +++ b/src/android/camera_request.h
> > @@ -7,7 +7,9 @@
> >  #ifndef __ANDROID_CAMERA_REQUEST_H__
> >  #define __ANDROID_CAMERA_REQUEST_H__
> >
> > +#include <map>
> >  #include <memory>
> > +#include <mutex>
> >  #include <vector>
> >
> >  #include <libcamera/base/class.h>
> > @@ -43,6 +45,10 @@ public:
> >               Camera3RequestDescriptor *request;
> >       };
> >
> > +     /* Keeps track of streams requiring post-processing. */
> > +     std::map<CameraStream *, StreamBuffer *> pendingStreamsToProcess_;
> > +     std::mutex streamsProcessMutex_;
> > +
> >       Camera3RequestDescriptor(libcamera::Camera *camera,
> >                                const camera3_capture_request_t *camera3Request);
> >       ~Camera3RequestDescriptor();
> > diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp
> > index 0e268cdf..4e275cde 100644
> > --- a/src/android/camera_stream.cpp
> > +++ b/src/android/camera_stream.cpp
> > @@ -22,6 +22,7 @@
> >  #include "camera_capabilities.h"
> >  #include "camera_device.h"
> >  #include "camera_metadata.h"
> > +#include "post_processor.h"
> >
> >  using namespace libcamera;
> >
> > @@ -97,6 +98,20 @@ int CameraStream::configure()
> >               int ret = postProcessor_->configure(configuration(), output);
> >               if (ret)
> >                       return ret;
> > +
> > +             postProcessor_->processComplete.connect(
> > +                     this, [&](Camera3RequestDescriptor::StreamBuffer *streamBuffer,
> > +                               PostProcessor::Status status) {

I would not like capturing a reference of all visible variables.
cameraDevice looks only needed.
I would do [cameraDevice=cameraDevice_](...).

> > +                             Camera3RequestDescriptor::Status bufferStatus;
> > +
> > +                             if (status == PostProcessor::Status::Success)
> > +                                     bufferStatus = Camera3RequestDescriptor::Status::Success;
> > +                             else
> > +                                     bufferStatus = Camera3RequestDescriptor::Status::Error;
> > +
> > +                             cameraDevice_->streamProcessingComplete(streamBuffer,
> > +                                                                     bufferStatus);
> > +                     });
> >       }
> >
> >       if (type_ == Type::Internal) {
> > diff --git a/src/android/jpeg/post_processor_jpeg.cpp b/src/android/jpeg/post_processor_jpeg.cpp
> > index da71f113..cbbe7128 100644
> > --- a/src/android/jpeg/post_processor_jpeg.cpp
> > +++ b/src/android/jpeg/post_processor_jpeg.cpp
> > @@ -198,6 +198,7 @@ int PostProcessorJpeg::process(Camera3RequestDescriptor::StreamBuffer *streamBuf
> >                                        exif.data(), quality);
> >       if (jpeg_size < 0) {
> >               LOG(JPEG, Error) << "Failed to encode stream image";
> > +             processComplete.emit(streamBuffer, PostProcessor::Status::Error);
> >               return jpeg_size;
> >       }
> >
> > @@ -211,6 +212,7 @@ int PostProcessorJpeg::process(Camera3RequestDescriptor::StreamBuffer *streamBuf
> >
> >       /* Update the JPEG result Metadata. */
> >       resultMetadata->addEntry(ANDROID_JPEG_SIZE, jpeg_size);
> > +     processComplete.emit(streamBuffer, PostProcessor::Status::Success);
> >
> >       return 0;
> >  }
> > diff --git a/src/android/post_processor.h b/src/android/post_processor.h
> > index 128161c8..4ac74fcf 100644
> > --- a/src/android/post_processor.h
> > +++ b/src/android/post_processor.h
> > @@ -7,6 +7,8 @@
> >  #ifndef __ANDROID_POST_PROCESSOR_H__
> >  #define __ANDROID_POST_PROCESSOR_H__
> >
> > +#include <libcamera/base/signal.h>
> > +
> >  #include <libcamera/framebuffer.h>
> >  #include <libcamera/stream.h>
> >
> > @@ -16,11 +18,18 @@
> >  class PostProcessor
> >  {
> >  public:
> > +     enum class Status {
> > +             Error,
> > +             Success
> > +     };
> > +
> >       virtual ~PostProcessor() = default;
> >
> >       virtual int configure(const libcamera::StreamConfiguration &inCfg,
> >                             const libcamera::StreamConfiguration &outCfg) = 0;
> >       virtual int process(Camera3RequestDescriptor::StreamBuffer *streamBuffer) = 0;
> > +
> > +     libcamera::Signal<Camera3RequestDescriptor::StreamBuffer *, Status> processComplete;
> >  };
> >
> >  #endif /* __ANDROID_POST_PROCESSOR_H__ */
> > diff --git a/src/android/yuv/post_processor_yuv.cpp b/src/android/yuv/post_processor_yuv.cpp
> > index eeb8f1f4..8e77bf57 100644
> > --- a/src/android/yuv/post_processor_yuv.cpp
> > +++ b/src/android/yuv/post_processor_yuv.cpp
> > @@ -54,12 +54,15 @@ int PostProcessorYuv::process(Camera3RequestDescriptor::StreamBuffer *streamBuff
> >       const FrameBuffer &source = *streamBuffer->srcBuffer;
> >       CameraBuffer *destination = streamBuffer->destBuffer.get();
> >
> > -     if (!isValidBuffers(source, *destination))
> > +     if (!isValidBuffers(source, *destination)) {
> > +             processComplete.emit(streamBuffer, PostProcessor::Status::Error);
> >               return -EINVAL;
> > +     }
> >
> >       const MappedFrameBuffer sourceMapped(&source, MappedFrameBuffer::MapFlag::Read);
> >       if (!sourceMapped.isValid()) {
> >               LOG(YUV, Error) << "Failed to mmap camera frame buffer";
> > +             processComplete.emit(streamBuffer, PostProcessor::Status::Error);
> >               return -EINVAL;
> >       }
> >
> > @@ -77,9 +80,12 @@ int PostProcessorYuv::process(Camera3RequestDescriptor::StreamBuffer *streamBuff
> >                                   libyuv::FilterMode::kFilterBilinear);
> >       if (ret) {
> >               LOG(YUV, Error) << "Failed NV12 scaling: " << ret;
> > +             processComplete.emit(streamBuffer, PostProcessor::Status::Error);
> >               return -EINVAL;
> >       }
> >
> > +     processComplete.emit(streamBuffer, PostProcessor::Status::Success);
> > +
> >       return 0;
> >  }
> >
>
> --
> Regards,
>
> Laurent Pinchart

Hi Laurent,

On 10/25/21 11:33 AM, Laurent Pinchart wrote:
> Hi Umang,
>
> Thank you for the patch.
>
> On Sat, Oct 23, 2021 at 04:03:00PM +0530, Umang Jain wrote:
>> Notify that the post processing for a request has been completed,
>> via a signal. The signal emit with a context pointer along with status
> s/emit/is emitted/ ?
>
>> of the buffer. The function CameraDevice::streamProcessingComplete() will
>> finally set the status on the request descriptor and complete the
>> descriptor if all the streams requiring post processing are completed.
>> If buffer status obtained is in error state, notify the status to the
>> framework and set the overall error status on the descriptor via
>> setBufferStatus().
>>
>> We need to track the number of streams requiring post-processing
>> per Camera3RequestDescriptor (i.e. per capture request). Introduce
>> a std::map<> to track the post-processing of streams. The nodes
>> are dropped from the map when a particular stream post processing
>> is completed (or on error paths). A std::map is selected for tracking
>> post-processing requests, since we will move post-processing to be
>> asynchronous in subsequent commits. A vector or queue will not be
>> suitable then as the sequential order of post-processing completion
>> of various requests won't be guaranteed then.
>>
>> A streamProcessMutex_ has been introduced here as well, which will be
> s/streamProcessMutex_/streamsProcessMutex_/
>
>> applicable to guard access to descriptor's pendingStreamsToProcess_ when
>> post-processing is moved to be asynchronous in subsequent commits.
>>
>> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
>> ---
>>   src/android/camera_device.cpp            | 95 ++++++++++++++++++------
>>   src/android/camera_device.h              |  4 +
>>   src/android/camera_request.h             |  6 ++
>>   src/android/camera_stream.cpp            | 15 ++++
>>   src/android/jpeg/post_processor_jpeg.cpp |  2 +
>>   src/android/post_processor.h             |  9 +++
>>   src/android/yuv/post_processor_yuv.cpp   |  8 +-
>>   7 files changed, 114 insertions(+), 25 deletions(-)
>>
>> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
>> index 2a98a2e6..3114def0 100644
>> --- a/src/android/camera_device.cpp
>> +++ b/src/android/camera_device.cpp
>> @@ -926,6 +926,8 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
>>   			 * Request.
>>   			 */
>>   			LOG(HAL, Debug) << ss.str() << " (mapped)";
> Blank line here, or no blank line below.
>
>> +			descriptor->pendingStreamsToProcess_.insert(
>> +				{ cameraStream, &buffer });
>>   			continue;
>>   
>>   		case CameraStream::Type::Direct:
>> @@ -955,6 +957,9 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
>>   			frameBuffer = cameraStream->getBuffer();
>>   			buffer.internalBuffer = frameBuffer;
>>   			LOG(HAL, Debug) << ss.str() << " (internal)";
>> +
>> +			descriptor->pendingStreamsToProcess_.insert(
>> +				{ cameraStream, &buffer });
>>   			break;
>>   		}
>>   
>> @@ -1118,43 +1123,46 @@ void CameraDevice::requestComplete(Request *request)
>>   	}
>>   
>>   	/* Handle post-processing. */
>> -	bool hasPostProcessingErrors = false;
>> -	for (auto &buffer : descriptor->buffers_) {
>> -		CameraStream *stream = buffer.stream;
>> -
>> -		if (stream->type() == CameraStream::Type::Direct)
>> -			continue;
>> +	bool needPostProcessing = false;
>> +	/*
>> +	 * \todo Protect the loop below with streamProcessMutex_ when post
>> +	 * processor runs asynchronously.
>> +	 */
>> +	auto iter = descriptor->pendingStreamsToProcess_.begin();
>> +	while (descriptor->pendingStreamsToProcess_.size() > 0) {
> 	while (!descriptor->pendingStreamsToProcess_.empty()) {
>
> But it's not correct. The fix is in 7/7:
>
> 	while (iter != descriptor->pendingStreamsToProcess_.end()) {


Yes, it was a tricky to handle the loop for async and sync variations

For the async one, iter == end() was the only choice I  believe (Without 
introducing a counter)

>
>> +		CameraStream *stream = iter->first;
>> +		Camera3RequestDescriptor::StreamBuffer *buffer = iter->second;
>> +		needPostProcessing = true;
>>   
>>   		FrameBuffer *src = request->findBuffer(stream->stream());
>>   		if (!src) {
>>   			LOG(HAL, Error) << "Failed to find a source stream buffer";
>> -			buffer.status = Camera3RequestDescriptor::Status::Error;
>> -			notifyError(descriptor->frameNumber_, stream->camera3Stream(),
>> -				    CAMERA3_MSG_ERROR_BUFFER);
>> -			hasPostProcessingErrors = true;
>> +			setBufferStatus(*buffer, Camera3RequestDescriptor::Status::Error);
>> +			iter = descriptor->pendingStreamsToProcess_.erase(iter);
>>   			continue;
>>   		}
>>   
>> -		int ret = stream->process(*src, buffer);
>> +		++iter;
>> +		int ret = stream->process(*src, *buffer);
>> +		if (ret) {
>> +			setBufferStatus(*buffer, Camera3RequestDescriptor::Status::Error);
>> +			descriptor->pendingStreamsToProcess_.erase(stream);
>> +		}
>> +	}
>>   
>> +	if (needPostProcessing) {
>>   		/*
>> -		 * If the framebuffer is internal to CameraStream return it back
>> -		 * now that we're done processing it.
>> +		 * \todo We will require to check if we failed to queue
>> +		 * post-processing requests when we migrate to post-processor
>> +		 * running asynchronously.
>> +		 *
>> +		 * if (descriptor->pendingStreamsToProcess_.size() == 0)
>> +		 *	completeDescriptor(descriptor);
> Can't we do this already here ? I think you can actually drop the
> needPostProcessing variable and just write
>
> 	if (descriptor->pendingStreamsToProcess_.empty())
> 		completeDescriptor(descriptor);
>
> as needPostProcessing can only be false if pendingStreamsToProcess_ was
> empty before the while loop above, and it will thus be empty after the
> loop as well in that case.


True, but we need needPostProcessing variable for async. I pre-empted 
its introduction deliberately to set the design beforehand andthen,  I 
can introduce async bits with minimal diff for 
CamreraDevice::requestComplete()

>
>>   		 */
>> -		if (buffer.internalBuffer)
>> -			stream->putBuffer(buffer.internalBuffer);
>>   
>> -		if (ret) {
>> -			buffer.status = Camera3RequestDescriptor::Status::Error;
>> -			hasPostProcessingErrors = true;
>> -			notifyError(descriptor->frameNumber_, stream->camera3Stream(),
>> -				    CAMERA3_MSG_ERROR_BUFFER);
>> -		}
>> +		return;
>>   	}
>>   
>> -	if (hasPostProcessingErrors)
>> -		descriptor->status_ = Camera3RequestDescriptor::Status::Error;
>> -
>>   	completeDescriptor(descriptor);
>>   }
>>   
>> @@ -1210,6 +1218,45 @@ void CameraDevice::sendCaptureResults()
>>   	}
>>   }
>>   
>> +void CameraDevice::setBufferStatus(Camera3RequestDescriptor::StreamBuffer &streamBuffer,
>> +				   Camera3RequestDescriptor::Status status)
>> +{
>> +	/*
>> +	 * If the framebuffer is internal to CameraStream return it back now
>> +	 * that we're done processing it.
>> +	 */
>> +	if (streamBuffer.internalBuffer)
>> +		streamBuffer.stream->putBuffer(streamBuffer.internalBuffer);
> I'd move this to the caller, as it's not about the buffer status.
>
>> +
>> +	streamBuffer.status = status;
>> +	if (status != Camera3RequestDescriptor::Status::Success) {
>> +		notifyError(streamBuffer.request->frameNumber_,
>> +			    streamBuffer.stream->camera3Stream(),
>> +			    CAMERA3_MSG_ERROR_BUFFER);
>> +
>> +		/* Also set error status on entire request descriptor. */
>> +		streamBuffer.request->status_ =
>> +			Camera3RequestDescriptor::Status::Error;
>> +	}
>> +}
>> +
>> +void CameraDevice::streamProcessingComplete(Camera3RequestDescriptor::StreamBuffer *streamBuffer,
>> +					    Camera3RequestDescriptor::Status status)
>> +{
>> +	Camera3RequestDescriptor *request = streamBuffer->request;
>> +	MutexLocker locker(request->streamsProcessMutex_);
>> +
>> +	setBufferStatus(*streamBuffer, status);
> Do we need to protect the setBufferStatus() call with
> streamsProcessMutex_ ? I thought it only protects
> pendingStreamsToProcess_.


Ah, I don't see why we need to lock it. This is a good catch.

For the async version, the main thread isn't expected to call 
setBufferStatus() on the same descriptor (because we are already 
handling synchronous errors beforehand), so I don't expect to race. So I 
should remove setBufferStatus() from the lock section.

>
>> +	request->pendingStreamsToProcess_.erase(streamBuffer->stream);
>> +
>> +	if (request->pendingStreamsToProcess_.size() > 0)
> 	if (!request->pendingStreamsToProcess_.empty())
>
>> +		return;
>> +
>> +	locker.unlock();
>> +
>> +	completeDescriptor(streamBuffer->request);
>> +}
>> +
>>   std::string CameraDevice::logPrefix() const
>>   {
>>   	return "'" + camera_->id() + "'";
>> diff --git a/src/android/camera_device.h b/src/android/camera_device.h
>> index e544f2bd..2a414020 100644
>> --- a/src/android/camera_device.h
>> +++ b/src/android/camera_device.h
>> @@ -66,6 +66,8 @@ public:
>>   	int configureStreams(camera3_stream_configuration_t *stream_list);
>>   	int processCaptureRequest(camera3_capture_request_t *request);
>>   	void requestComplete(libcamera::Request *request);
>> +	void streamProcessingComplete(Camera3RequestDescriptor::StreamBuffer *bufferStream,
>> +				      Camera3RequestDescriptor::Status status);
>>   
>>   protected:
>>   	std::string logPrefix() const override;
>> @@ -95,6 +97,8 @@ private:
>>   	int processControls(Camera3RequestDescriptor *descriptor);
>>   	void completeDescriptor(Camera3RequestDescriptor *descriptor);
>>   	void sendCaptureResults();
>> +	void setBufferStatus(Camera3RequestDescriptor::StreamBuffer &buffer,
>> +			     Camera3RequestDescriptor::Status status);
>>   	std::unique_ptr<CameraMetadata> getResultMetadata(
>>   		const Camera3RequestDescriptor &descriptor) const;
>>   
>> diff --git a/src/android/camera_request.h b/src/android/camera_request.h
>> index c4bc5d6e..cc2b7035 100644
>> --- a/src/android/camera_request.h
>> +++ b/src/android/camera_request.h
>> @@ -7,7 +7,9 @@
>>   #ifndef __ANDROID_CAMERA_REQUEST_H__
>>   #define __ANDROID_CAMERA_REQUEST_H__
>>   
>> +#include <map>
>>   #include <memory>
>> +#include <mutex>
>>   #include <vector>
>>   
>>   #include <libcamera/base/class.h>
>> @@ -43,6 +45,10 @@ public:
>>   		Camera3RequestDescriptor *request;
>>   	};
>>   
>> +	/* Keeps track of streams requiring post-processing. */
>> +	std::map<CameraStream *, StreamBuffer *> pendingStreamsToProcess_;
>> +	std::mutex streamsProcessMutex_;
>> +
>>   	Camera3RequestDescriptor(libcamera::Camera *camera,
>>   				 const camera3_capture_request_t *camera3Request);
>>   	~Camera3RequestDescriptor();
>> diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp
>> index 0e268cdf..4e275cde 100644
>> --- a/src/android/camera_stream.cpp
>> +++ b/src/android/camera_stream.cpp
>> @@ -22,6 +22,7 @@
>>   #include "camera_capabilities.h"
>>   #include "camera_device.h"
>>   #include "camera_metadata.h"
>> +#include "post_processor.h"
>>   
>>   using namespace libcamera;
>>   
>> @@ -97,6 +98,20 @@ int CameraStream::configure()
>>   		int ret = postProcessor_->configure(configuration(), output);
>>   		if (ret)
>>   			return ret;
>> +
>> +		postProcessor_->processComplete.connect(
>> +			this, [&](Camera3RequestDescriptor::StreamBuffer *streamBuffer,
>> +				  PostProcessor::Status status) {
>> +				Camera3RequestDescriptor::Status bufferStatus;
>> +
>> +				if (status == PostProcessor::Status::Success)
>> +					bufferStatus = Camera3RequestDescriptor::Status::Success;
>> +				else
>> +					bufferStatus = Camera3RequestDescriptor::Status::Error;
>> +
>> +				cameraDevice_->streamProcessingComplete(streamBuffer,
>> +									bufferStatus);
>> +			});
>>   	}
>>   
>>   	if (type_ == Type::Internal) {
>> diff --git a/src/android/jpeg/post_processor_jpeg.cpp b/src/android/jpeg/post_processor_jpeg.cpp
>> index da71f113..cbbe7128 100644
>> --- a/src/android/jpeg/post_processor_jpeg.cpp
>> +++ b/src/android/jpeg/post_processor_jpeg.cpp
>> @@ -198,6 +198,7 @@ int PostProcessorJpeg::process(Camera3RequestDescriptor::StreamBuffer *streamBuf
>>   					 exif.data(), quality);
>>   	if (jpeg_size < 0) {
>>   		LOG(JPEG, Error) << "Failed to encode stream image";
>> +		processComplete.emit(streamBuffer, PostProcessor::Status::Error);
>>   		return jpeg_size;
>>   	}
>>   
>> @@ -211,6 +212,7 @@ int PostProcessorJpeg::process(Camera3RequestDescriptor::StreamBuffer *streamBuf
>>   
>>   	/* Update the JPEG result Metadata. */
>>   	resultMetadata->addEntry(ANDROID_JPEG_SIZE, jpeg_size);
>> +	processComplete.emit(streamBuffer, PostProcessor::Status::Success);
>>   
>>   	return 0;
>>   }
>> diff --git a/src/android/post_processor.h b/src/android/post_processor.h
>> index 128161c8..4ac74fcf 100644
>> --- a/src/android/post_processor.h
>> +++ b/src/android/post_processor.h
>> @@ -7,6 +7,8 @@
>>   #ifndef __ANDROID_POST_PROCESSOR_H__
>>   #define __ANDROID_POST_PROCESSOR_H__
>>   
>> +#include <libcamera/base/signal.h>
>> +
>>   #include <libcamera/framebuffer.h>
>>   #include <libcamera/stream.h>
>>   
>> @@ -16,11 +18,18 @@
>>   class PostProcessor
>>   {
>>   public:
>> +	enum class Status {
>> +		Error,
>> +		Success
>> +	};
>> +
>>   	virtual ~PostProcessor() = default;
>>   
>>   	virtual int configure(const libcamera::StreamConfiguration &inCfg,
>>   			      const libcamera::StreamConfiguration &outCfg) = 0;
>>   	virtual int process(Camera3RequestDescriptor::StreamBuffer *streamBuffer) = 0;
>> +
>> +	libcamera::Signal<Camera3RequestDescriptor::StreamBuffer *, Status> processComplete;
>>   };
>>   
>>   #endif /* __ANDROID_POST_PROCESSOR_H__ */
>> diff --git a/src/android/yuv/post_processor_yuv.cpp b/src/android/yuv/post_processor_yuv.cpp
>> index eeb8f1f4..8e77bf57 100644
>> --- a/src/android/yuv/post_processor_yuv.cpp
>> +++ b/src/android/yuv/post_processor_yuv.cpp
>> @@ -54,12 +54,15 @@ int PostProcessorYuv::process(Camera3RequestDescriptor::StreamBuffer *streamBuff
>>   	const FrameBuffer &source = *streamBuffer->srcBuffer;
>>   	CameraBuffer *destination = streamBuffer->destBuffer.get();
>>   
>> -	if (!isValidBuffers(source, *destination))
>> +	if (!isValidBuffers(source, *destination)) {
>> +		processComplete.emit(streamBuffer, PostProcessor::Status::Error);
>>   		return -EINVAL;
>> +	}
>>   
>>   	const MappedFrameBuffer sourceMapped(&source, MappedFrameBuffer::MapFlag::Read);
>>   	if (!sourceMapped.isValid()) {
>>   		LOG(YUV, Error) << "Failed to mmap camera frame buffer";
>> +		processComplete.emit(streamBuffer, PostProcessor::Status::Error);
>>   		return -EINVAL;
>>   	}
>>   
>> @@ -77,9 +80,12 @@ int PostProcessorYuv::process(Camera3RequestDescriptor::StreamBuffer *streamBuff
>>   				    libyuv::FilterMode::kFilterBilinear);
>>   	if (ret) {
>>   		LOG(YUV, Error) << "Failed NV12 scaling: " << ret;
>> +		processComplete.emit(streamBuffer, PostProcessor::Status::Error);
>>   		return -EINVAL;
>>   	}
>>   
>> +	processComplete.emit(streamBuffer, PostProcessor::Status::Success);
>> +
>>   	return 0;
>>   }
>>

Hi Umang,

On Mon, Oct 25, 2021 at 06:46:39PM +0530, Umang Jain wrote:
> On 10/25/21 11:33 AM, Laurent Pinchart wrote:
> > On Sat, Oct 23, 2021 at 04:03:00PM +0530, Umang Jain wrote:
> >> Notify that the post processing for a request has been completed,
> >> via a signal. The signal emit with a context pointer along with status
> > s/emit/is emitted/ ?
> >
> >> of the buffer. The function CameraDevice::streamProcessingComplete() will
> >> finally set the status on the request descriptor and complete the
> >> descriptor if all the streams requiring post processing are completed.
> >> If buffer status obtained is in error state, notify the status to the
> >> framework and set the overall error status on the descriptor via
> >> setBufferStatus().
> >>
> >> We need to track the number of streams requiring post-processing
> >> per Camera3RequestDescriptor (i.e. per capture request). Introduce
> >> a std::map<> to track the post-processing of streams. The nodes
> >> are dropped from the map when a particular stream post processing
> >> is completed (or on error paths). A std::map is selected for tracking
> >> post-processing requests, since we will move post-processing to be
> >> asynchronous in subsequent commits. A vector or queue will not be
> >> suitable then as the sequential order of post-processing completion
> >> of various requests won't be guaranteed then.
> >>
> >> A streamProcessMutex_ has been introduced here as well, which will be
> >
> > s/streamProcessMutex_/streamsProcessMutex_/
> >
> >> applicable to guard access to descriptor's pendingStreamsToProcess_ when
> >> post-processing is moved to be asynchronous in subsequent commits.
> >>
> >> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
> >> ---
> >>   src/android/camera_device.cpp            | 95 ++++++++++++++++++------
> >>   src/android/camera_device.h              |  4 +
> >>   src/android/camera_request.h             |  6 ++
> >>   src/android/camera_stream.cpp            | 15 ++++
> >>   src/android/jpeg/post_processor_jpeg.cpp |  2 +
> >>   src/android/post_processor.h             |  9 +++
> >>   src/android/yuv/post_processor_yuv.cpp   |  8 +-
> >>   7 files changed, 114 insertions(+), 25 deletions(-)
> >>
> >> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> >> index 2a98a2e6..3114def0 100644
> >> --- a/src/android/camera_device.cpp
> >> +++ b/src/android/camera_device.cpp
> >> @@ -926,6 +926,8 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
> >>   			 * Request.
> >>   			 */
> >>   			LOG(HAL, Debug) << ss.str() << " (mapped)";
> > Blank line here, or no blank line below.
> >
> >> +			descriptor->pendingStreamsToProcess_.insert(
> >> +				{ cameraStream, &buffer });
> >>   			continue;
> >>   
> >>   		case CameraStream::Type::Direct:
> >> @@ -955,6 +957,9 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
> >>   			frameBuffer = cameraStream->getBuffer();
> >>   			buffer.internalBuffer = frameBuffer;
> >>   			LOG(HAL, Debug) << ss.str() << " (internal)";
> >> +
> >> +			descriptor->pendingStreamsToProcess_.insert(
> >> +				{ cameraStream, &buffer });
> >>   			break;
> >>   		}
> >>   
> >> @@ -1118,43 +1123,46 @@ void CameraDevice::requestComplete(Request *request)
> >>   	}
> >>   
> >>   	/* Handle post-processing. */
> >> -	bool hasPostProcessingErrors = false;
> >> -	for (auto &buffer : descriptor->buffers_) {
> >> -		CameraStream *stream = buffer.stream;
> >> -
> >> -		if (stream->type() == CameraStream::Type::Direct)
> >> -			continue;
> >> +	bool needPostProcessing = false;
> >> +	/*
> >> +	 * \todo Protect the loop below with streamProcessMutex_ when post
> >> +	 * processor runs asynchronously.
> >> +	 */
> >> +	auto iter = descriptor->pendingStreamsToProcess_.begin();
> >> +	while (descriptor->pendingStreamsToProcess_.size() > 0) {
> > 	while (!descriptor->pendingStreamsToProcess_.empty()) {
> >
> > But it's not correct. The fix is in 7/7:
> >
> > 	while (iter != descriptor->pendingStreamsToProcess_.end()) {
> 
> Yes, it was a tricky to handle the loop for async and sync variations
> 
> For the async one, iter == end() was the only choice I  believe (Without 
> introducing a counter)
> 
> >> +		CameraStream *stream = iter->first;
> >> +		Camera3RequestDescriptor::StreamBuffer *buffer = iter->second;
> >> +		needPostProcessing = true;
> >>   
> >>   		FrameBuffer *src = request->findBuffer(stream->stream());
> >>   		if (!src) {
> >>   			LOG(HAL, Error) << "Failed to find a source stream buffer";
> >> -			buffer.status = Camera3RequestDescriptor::Status::Error;
> >> -			notifyError(descriptor->frameNumber_, stream->camera3Stream(),
> >> -				    CAMERA3_MSG_ERROR_BUFFER);
> >> -			hasPostProcessingErrors = true;
> >> +			setBufferStatus(*buffer, Camera3RequestDescriptor::Status::Error);
> >> +			iter = descriptor->pendingStreamsToProcess_.erase(iter);
> >>   			continue;
> >>   		}
> >>   
> >> -		int ret = stream->process(*src, buffer);
> >> +		++iter;
> >> +		int ret = stream->process(*src, *buffer);
> >> +		if (ret) {
> >> +			setBufferStatus(*buffer, Camera3RequestDescriptor::Status::Error);
> >> +			descriptor->pendingStreamsToProcess_.erase(stream);
> >> +		}
> >> +	}
> >>   
> >> +	if (needPostProcessing) {
> >>   		/*
> >> -		 * If the framebuffer is internal to CameraStream return it back
> >> -		 * now that we're done processing it.
> >> +		 * \todo We will require to check if we failed to queue
> >> +		 * post-processing requests when we migrate to post-processor
> >> +		 * running asynchronously.
> >> +		 *
> >> +		 * if (descriptor->pendingStreamsToProcess_.size() == 0)
> >> +		 *	completeDescriptor(descriptor);
> >
> > Can't we do this already here ? I think you can actually drop the
> > needPostProcessing variable and just write
> >
> > 	if (descriptor->pendingStreamsToProcess_.empty())
> > 		completeDescriptor(descriptor);
> >
> > as needPostProcessing can only be false if pendingStreamsToProcess_ was
> > empty before the while loop above, and it will thus be empty after the
> > loop as well in that case.
> 
> True, but we need needPostProcessing variable for async.

Why is that ? Won't

 	if (descriptor->pendingStreamsToProcess_.empty())
 		completeDescriptor(descriptor);

work in async mode too ?

> I pre-empted 
> its introduction deliberately to set the design beforehand andthen,  I 
> can introduce async bits with minimal diff for 
> CamreraDevice::requestComplete()
> 
> >>   		 */
> >> -		if (buffer.internalBuffer)
> >> -			stream->putBuffer(buffer.internalBuffer);
> >>   
> >> -		if (ret) {
> >> -			buffer.status = Camera3RequestDescriptor::Status::Error;
> >> -			hasPostProcessingErrors = true;
> >> -			notifyError(descriptor->frameNumber_, stream->camera3Stream(),
> >> -				    CAMERA3_MSG_ERROR_BUFFER);
> >> -		}
> >> +		return;
> >>   	}
> >>   
> >> -	if (hasPostProcessingErrors)
> >> -		descriptor->status_ = Camera3RequestDescriptor::Status::Error;
> >> -
> >>   	completeDescriptor(descriptor);
> >>   }
> >>   
> >> @@ -1210,6 +1218,45 @@ void CameraDevice::sendCaptureResults()
> >>   	}
> >>   }
> >>   
> >> +void CameraDevice::setBufferStatus(Camera3RequestDescriptor::StreamBuffer &streamBuffer,
> >> +				   Camera3RequestDescriptor::Status status)
> >> +{
> >> +	/*
> >> +	 * If the framebuffer is internal to CameraStream return it back now
> >> +	 * that we're done processing it.
> >> +	 */
> >> +	if (streamBuffer.internalBuffer)
> >> +		streamBuffer.stream->putBuffer(streamBuffer.internalBuffer);
> > I'd move this to the caller, as it's not about the buffer status.
> >
> >> +
> >> +	streamBuffer.status = status;
> >> +	if (status != Camera3RequestDescriptor::Status::Success) {
> >> +		notifyError(streamBuffer.request->frameNumber_,
> >> +			    streamBuffer.stream->camera3Stream(),
> >> +			    CAMERA3_MSG_ERROR_BUFFER);
> >> +
> >> +		/* Also set error status on entire request descriptor. */
> >> +		streamBuffer.request->status_ =
> >> +			Camera3RequestDescriptor::Status::Error;
> >> +	}
> >> +}
> >> +
> >> +void CameraDevice::streamProcessingComplete(Camera3RequestDescriptor::StreamBuffer *streamBuffer,
> >> +					    Camera3RequestDescriptor::Status status)
> >> +{
> >> +	Camera3RequestDescriptor *request = streamBuffer->request;
> >> +	MutexLocker locker(request->streamsProcessMutex_);
> >> +
> >> +	setBufferStatus(*streamBuffer, status);
> > Do we need to protect the setBufferStatus() call with
> > streamsProcessMutex_ ? I thought it only protects
> > pendingStreamsToProcess_.
> 
> 
> Ah, I don't see why we need to lock it. This is a good catch.
> 
> For the async version, the main thread isn't expected to call 
> setBufferStatus() on the same descriptor (because we are already 
> handling synchronous errors beforehand), so I don't expect to race. So I 
> should remove setBufferStatus() from the lock section.
> 
> >
> >> +	request->pendingStreamsToProcess_.erase(streamBuffer->stream);
> >> +
> >> +	if (request->pendingStreamsToProcess_.size() > 0)
> > 	if (!request->pendingStreamsToProcess_.empty())
> >
> >> +		return;
> >> +
> >> +	locker.unlock();
> >> +
> >> +	completeDescriptor(streamBuffer->request);
> >> +}
> >> +
> >>   std::string CameraDevice::logPrefix() const
> >>   {
> >>   	return "'" + camera_->id() + "'";
> >> diff --git a/src/android/camera_device.h b/src/android/camera_device.h
> >> index e544f2bd..2a414020 100644
> >> --- a/src/android/camera_device.h
> >> +++ b/src/android/camera_device.h
> >> @@ -66,6 +66,8 @@ public:
> >>   	int configureStreams(camera3_stream_configuration_t *stream_list);
> >>   	int processCaptureRequest(camera3_capture_request_t *request);
> >>   	void requestComplete(libcamera::Request *request);
> >> +	void streamProcessingComplete(Camera3RequestDescriptor::StreamBuffer *bufferStream,
> >> +				      Camera3RequestDescriptor::Status status);
> >>   
> >>   protected:
> >>   	std::string logPrefix() const override;
> >> @@ -95,6 +97,8 @@ private:
> >>   	int processControls(Camera3RequestDescriptor *descriptor);
> >>   	void completeDescriptor(Camera3RequestDescriptor *descriptor);
> >>   	void sendCaptureResults();
> >> +	void setBufferStatus(Camera3RequestDescriptor::StreamBuffer &buffer,
> >> +			     Camera3RequestDescriptor::Status status);
> >>   	std::unique_ptr<CameraMetadata> getResultMetadata(
> >>   		const Camera3RequestDescriptor &descriptor) const;
> >>   
> >> diff --git a/src/android/camera_request.h b/src/android/camera_request.h
> >> index c4bc5d6e..cc2b7035 100644
> >> --- a/src/android/camera_request.h
> >> +++ b/src/android/camera_request.h
> >> @@ -7,7 +7,9 @@
> >>   #ifndef __ANDROID_CAMERA_REQUEST_H__
> >>   #define __ANDROID_CAMERA_REQUEST_H__
> >>   
> >> +#include <map>
> >>   #include <memory>
> >> +#include <mutex>
> >>   #include <vector>
> >>   
> >>   #include <libcamera/base/class.h>
> >> @@ -43,6 +45,10 @@ public:
> >>   		Camera3RequestDescriptor *request;
> >>   	};
> >>   
> >> +	/* Keeps track of streams requiring post-processing. */
> >> +	std::map<CameraStream *, StreamBuffer *> pendingStreamsToProcess_;
> >> +	std::mutex streamsProcessMutex_;
> >> +
> >>   	Camera3RequestDescriptor(libcamera::Camera *camera,
> >>   				 const camera3_capture_request_t *camera3Request);
> >>   	~Camera3RequestDescriptor();
> >> diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp
> >> index 0e268cdf..4e275cde 100644
> >> --- a/src/android/camera_stream.cpp
> >> +++ b/src/android/camera_stream.cpp
> >> @@ -22,6 +22,7 @@
> >>   #include "camera_capabilities.h"
> >>   #include "camera_device.h"
> >>   #include "camera_metadata.h"
> >> +#include "post_processor.h"
> >>   
> >>   using namespace libcamera;
> >>   
> >> @@ -97,6 +98,20 @@ int CameraStream::configure()
> >>   		int ret = postProcessor_->configure(configuration(), output);
> >>   		if (ret)
> >>   			return ret;
> >> +
> >> +		postProcessor_->processComplete.connect(
> >> +			this, [&](Camera3RequestDescriptor::StreamBuffer *streamBuffer,
> >> +				  PostProcessor::Status status) {
> >> +				Camera3RequestDescriptor::Status bufferStatus;
> >> +
> >> +				if (status == PostProcessor::Status::Success)
> >> +					bufferStatus = Camera3RequestDescriptor::Status::Success;
> >> +				else
> >> +					bufferStatus = Camera3RequestDescriptor::Status::Error;
> >> +
> >> +				cameraDevice_->streamProcessingComplete(streamBuffer,
> >> +									bufferStatus);
> >> +			});
> >>   	}
> >>   
> >>   	if (type_ == Type::Internal) {
> >> diff --git a/src/android/jpeg/post_processor_jpeg.cpp b/src/android/jpeg/post_processor_jpeg.cpp
> >> index da71f113..cbbe7128 100644
> >> --- a/src/android/jpeg/post_processor_jpeg.cpp
> >> +++ b/src/android/jpeg/post_processor_jpeg.cpp
> >> @@ -198,6 +198,7 @@ int PostProcessorJpeg::process(Camera3RequestDescriptor::StreamBuffer *streamBuf
> >>   					 exif.data(), quality);
> >>   	if (jpeg_size < 0) {
> >>   		LOG(JPEG, Error) << "Failed to encode stream image";
> >> +		processComplete.emit(streamBuffer, PostProcessor::Status::Error);
> >>   		return jpeg_size;
> >>   	}
> >>   
> >> @@ -211,6 +212,7 @@ int PostProcessorJpeg::process(Camera3RequestDescriptor::StreamBuffer *streamBuf
> >>   
> >>   	/* Update the JPEG result Metadata. */
> >>   	resultMetadata->addEntry(ANDROID_JPEG_SIZE, jpeg_size);
> >> +	processComplete.emit(streamBuffer, PostProcessor::Status::Success);
> >>   
> >>   	return 0;
> >>   }
> >> diff --git a/src/android/post_processor.h b/src/android/post_processor.h
> >> index 128161c8..4ac74fcf 100644
> >> --- a/src/android/post_processor.h
> >> +++ b/src/android/post_processor.h
> >> @@ -7,6 +7,8 @@
> >>   #ifndef __ANDROID_POST_PROCESSOR_H__
> >>   #define __ANDROID_POST_PROCESSOR_H__
> >>   
> >> +#include <libcamera/base/signal.h>
> >> +
> >>   #include <libcamera/framebuffer.h>
> >>   #include <libcamera/stream.h>
> >>   
> >> @@ -16,11 +18,18 @@
> >>   class PostProcessor
> >>   {
> >>   public:
> >> +	enum class Status {
> >> +		Error,
> >> +		Success
> >> +	};
> >> +
> >>   	virtual ~PostProcessor() = default;
> >>   
> >>   	virtual int configure(const libcamera::StreamConfiguration &inCfg,
> >>   			      const libcamera::StreamConfiguration &outCfg) = 0;
> >>   	virtual int process(Camera3RequestDescriptor::StreamBuffer *streamBuffer) = 0;
> >> +
> >> +	libcamera::Signal<Camera3RequestDescriptor::StreamBuffer *, Status> processComplete;
> >>   };
> >>   
> >>   #endif /* __ANDROID_POST_PROCESSOR_H__ */
> >> diff --git a/src/android/yuv/post_processor_yuv.cpp b/src/android/yuv/post_processor_yuv.cpp
> >> index eeb8f1f4..8e77bf57 100644
> >> --- a/src/android/yuv/post_processor_yuv.cpp
> >> +++ b/src/android/yuv/post_processor_yuv.cpp
> >> @@ -54,12 +54,15 @@ int PostProcessorYuv::process(Camera3RequestDescriptor::StreamBuffer *streamBuff
> >>   	const FrameBuffer &source = *streamBuffer->srcBuffer;
> >>   	CameraBuffer *destination = streamBuffer->destBuffer.get();
> >>   
> >> -	if (!isValidBuffers(source, *destination))
> >> +	if (!isValidBuffers(source, *destination)) {
> >> +		processComplete.emit(streamBuffer, PostProcessor::Status::Error);
> >>   		return -EINVAL;
> >> +	}
> >>   
> >>   	const MappedFrameBuffer sourceMapped(&source, MappedFrameBuffer::MapFlag::Read);
> >>   	if (!sourceMapped.isValid()) {
> >>   		LOG(YUV, Error) << "Failed to mmap camera frame buffer";
> >> +		processComplete.emit(streamBuffer, PostProcessor::Status::Error);
> >>   		return -EINVAL;
> >>   	}
> >>   
> >> @@ -77,9 +80,12 @@ int PostProcessorYuv::process(Camera3RequestDescriptor::StreamBuffer *streamBuff
> >>   				    libyuv::FilterMode::kFilterBilinear);
> >>   	if (ret) {
> >>   		LOG(YUV, Error) << "Failed NV12 scaling: " << ret;
> >> +		processComplete.emit(streamBuffer, PostProcessor::Status::Error);
> >>   		return -EINVAL;
> >>   	}
> >>   
> >> +	processComplete.emit(streamBuffer, PostProcessor::Status::Success);
> >> +
> >>   	return 0;
> >>   }
> >>

Hi Laurent,

On 10/25/21 6:55 PM, Laurent Pinchart wrote:
> Hi Umang,
>
> On Mon, Oct 25, 2021 at 06:46:39PM +0530, Umang Jain wrote:
>> On 10/25/21 11:33 AM, Laurent Pinchart wrote:
>>> On Sat, Oct 23, 2021 at 04:03:00PM +0530, Umang Jain wrote:
>>>> Notify that the post processing for a request has been completed,
>>>> via a signal. The signal emit with a context pointer along with status
>>> s/emit/is emitted/ ?
>>>
>>>> of the buffer. The function CameraDevice::streamProcessingComplete() will
>>>> finally set the status on the request descriptor and complete the
>>>> descriptor if all the streams requiring post processing are completed.
>>>> If buffer status obtained is in error state, notify the status to the
>>>> framework and set the overall error status on the descriptor via
>>>> setBufferStatus().
>>>>
>>>> We need to track the number of streams requiring post-processing
>>>> per Camera3RequestDescriptor (i.e. per capture request). Introduce
>>>> a std::map<> to track the post-processing of streams. The nodes
>>>> are dropped from the map when a particular stream post processing
>>>> is completed (or on error paths). A std::map is selected for tracking
>>>> post-processing requests, since we will move post-processing to be
>>>> asynchronous in subsequent commits. A vector or queue will not be
>>>> suitable then as the sequential order of post-processing completion
>>>> of various requests won't be guaranteed then.
>>>>
>>>> A streamProcessMutex_ has been introduced here as well, which will be
>>> s/streamProcessMutex_/streamsProcessMutex_/
>>>
>>>> applicable to guard access to descriptor's pendingStreamsToProcess_ when
>>>> post-processing is moved to be asynchronous in subsequent commits.
>>>>
>>>> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
>>>> ---
>>>>    src/android/camera_device.cpp            | 95 ++++++++++++++++++------
>>>>    src/android/camera_device.h              |  4 +
>>>>    src/android/camera_request.h             |  6 ++
>>>>    src/android/camera_stream.cpp            | 15 ++++
>>>>    src/android/jpeg/post_processor_jpeg.cpp |  2 +
>>>>    src/android/post_processor.h             |  9 +++
>>>>    src/android/yuv/post_processor_yuv.cpp   |  8 +-
>>>>    7 files changed, 114 insertions(+), 25 deletions(-)
>>>>
>>>> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
>>>> index 2a98a2e6..3114def0 100644
>>>> --- a/src/android/camera_device.cpp
>>>> +++ b/src/android/camera_device.cpp
>>>> @@ -926,6 +926,8 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
>>>>    			 * Request.
>>>>    			 */
>>>>    			LOG(HAL, Debug) << ss.str() << " (mapped)";
>>> Blank line here, or no blank line below.
>>>
>>>> +			descriptor->pendingStreamsToProcess_.insert(
>>>> +				{ cameraStream, &buffer });
>>>>    			continue;
>>>>    
>>>>    		case CameraStream::Type::Direct:
>>>> @@ -955,6 +957,9 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
>>>>    			frameBuffer = cameraStream->getBuffer();
>>>>    			buffer.internalBuffer = frameBuffer;
>>>>    			LOG(HAL, Debug) << ss.str() << " (internal)";
>>>> +
>>>> +			descriptor->pendingStreamsToProcess_.insert(
>>>> +				{ cameraStream, &buffer });
>>>>    			break;
>>>>    		}
>>>>    
>>>> @@ -1118,43 +1123,46 @@ void CameraDevice::requestComplete(Request *request)
>>>>    	}
>>>>    
>>>>    	/* Handle post-processing. */
>>>> -	bool hasPostProcessingErrors = false;
>>>> -	for (auto &buffer : descriptor->buffers_) {
>>>> -		CameraStream *stream = buffer.stream;
>>>> -
>>>> -		if (stream->type() == CameraStream::Type::Direct)
>>>> -			continue;
>>>> +	bool needPostProcessing = false;
>>>> +	/*
>>>> +	 * \todo Protect the loop below with streamProcessMutex_ when post
>>>> +	 * processor runs asynchronously.
>>>> +	 */
>>>> +	auto iter = descriptor->pendingStreamsToProcess_.begin();
>>>> +	while (descriptor->pendingStreamsToProcess_.size() > 0) {
>>> 	while (!descriptor->pendingStreamsToProcess_.empty()) {
>>>
>>> But it's not correct. The fix is in 7/7:
>>>
>>> 	while (iter != descriptor->pendingStreamsToProcess_.end()) {
>> Yes, it was a tricky to handle the loop for async and sync variations
>>
>> For the async one, iter == end() was the only choice I  believe (Without
>> introducing a counter)
>>
>>>> +		CameraStream *stream = iter->first;
>>>> +		Camera3RequestDescriptor::StreamBuffer *buffer = iter->second;
>>>> +		needPostProcessing = true;
>>>>    
>>>>    		FrameBuffer *src = request->findBuffer(stream->stream());
>>>>    		if (!src) {
>>>>    			LOG(HAL, Error) << "Failed to find a source stream buffer";
>>>> -			buffer.status = Camera3RequestDescriptor::Status::Error;
>>>> -			notifyError(descriptor->frameNumber_, stream->camera3Stream(),
>>>> -				    CAMERA3_MSG_ERROR_BUFFER);
>>>> -			hasPostProcessingErrors = true;
>>>> +			setBufferStatus(*buffer, Camera3RequestDescriptor::Status::Error);
>>>> +			iter = descriptor->pendingStreamsToProcess_.erase(iter);
>>>>    			continue;
>>>>    		}
>>>>    
>>>> -		int ret = stream->process(*src, buffer);
>>>> +		++iter;
>>>> +		int ret = stream->process(*src, *buffer);
>>>> +		if (ret) {
>>>> +			setBufferStatus(*buffer, Camera3RequestDescriptor::Status::Error);
>>>> +			descriptor->pendingStreamsToProcess_.erase(stream);
>>>> +		}
>>>> +	}
>>>>    
>>>> +	if (needPostProcessing) {
>>>>    		/*
>>>> -		 * If the framebuffer is internal to CameraStream return it back
>>>> -		 * now that we're done processing it.
>>>> +		 * \todo We will require to check if we failed to queue
>>>> +		 * post-processing requests when we migrate to post-processor
>>>> +		 * running asynchronously.
>>>> +		 *
>>>> +		 * if (descriptor->pendingStreamsToProcess_.size() == 0)
>>>> +		 *	completeDescriptor(descriptor);
>>> Can't we do this already here ? I think you can actually drop the
>>> needPostProcessing variable and just write
>>>
>>> 	if (descriptor->pendingStreamsToProcess_.empty())
>>> 		completeDescriptor(descriptor);
>>>
>>> as needPostProcessing can only be false if pendingStreamsToProcess_ was
>>> empty before the while loop above, and it will thus be empty after the
>>> loop as well in that case.
>> True, but we need needPostProcessing variable for async.
> Why is that ? Won't
>
>   	if (descriptor->pendingStreamsToProcess_.empty())
>   		completeDescriptor(descriptor);
>
> work in async mode too ?


Ok yes, it should work (sorry I confused myself a bit, thinking it's 
will be a double completeDescriptor(descriptor) for the same descriptor, 
in case post-processing completes early when main thread reaches this 
line. But I forgot that we are already holding a lock restricted slots 
to run)

We would end up requestComplete() like: https://paste.debian.net/1216820/

>
>> I pre-empted
>> its introduction deliberately to set the design beforehand andthen,  I
>> can introduce async bits with minimal diff for
>> CamreraDevice::requestComplete()
>>
>>>>    		 */
>>>> -		if (buffer.internalBuffer)
>>>> -			stream->putBuffer(buffer.internalBuffer);
>>>>    
>>>> -		if (ret) {
>>>> -			buffer.status = Camera3RequestDescriptor::Status::Error;
>>>> -			hasPostProcessingErrors = true;
>>>> -			notifyError(descriptor->frameNumber_, stream->camera3Stream(),
>>>> -				    CAMERA3_MSG_ERROR_BUFFER);
>>>> -		}
>>>> +		return;
>>>>    	}
>>>>    
>>>> -	if (hasPostProcessingErrors)
>>>> -		descriptor->status_ = Camera3RequestDescriptor::Status::Error;
>>>> -
>>>>    	completeDescriptor(descriptor);
>>>>    }
>>>>    
>>>> @@ -1210,6 +1218,45 @@ void CameraDevice::sendCaptureResults()
>>>>    	}
>>>>    }
>>>>    
>>>> +void CameraDevice::setBufferStatus(Camera3RequestDescriptor::StreamBuffer &streamBuffer,
>>>> +				   Camera3RequestDescriptor::Status status)
>>>> +{
>>>> +	/*
>>>> +	 * If the framebuffer is internal to CameraStream return it back now
>>>> +	 * that we're done processing it.
>>>> +	 */
>>>> +	if (streamBuffer.internalBuffer)
>>>> +		streamBuffer.stream->putBuffer(streamBuffer.internalBuffer);
>>> I'd move this to the caller, as it's not about the buffer status.
>>>
>>>> +
>>>> +	streamBuffer.status = status;
>>>> +	if (status != Camera3RequestDescriptor::Status::Success) {
>>>> +		notifyError(streamBuffer.request->frameNumber_,
>>>> +			    streamBuffer.stream->camera3Stream(),
>>>> +			    CAMERA3_MSG_ERROR_BUFFER);
>>>> +
>>>> +		/* Also set error status on entire request descriptor. */
>>>> +		streamBuffer.request->status_ =
>>>> +			Camera3RequestDescriptor::Status::Error;
>>>> +	}
>>>> +}
>>>> +
>>>> +void CameraDevice::streamProcessingComplete(Camera3RequestDescriptor::StreamBuffer *streamBuffer,
>>>> +					    Camera3RequestDescriptor::Status status)
>>>> +{
>>>> +	Camera3RequestDescriptor *request = streamBuffer->request;
>>>> +	MutexLocker locker(request->streamsProcessMutex_);
>>>> +
>>>> +	setBufferStatus(*streamBuffer, status);
>>> Do we need to protect the setBufferStatus() call with
>>> streamsProcessMutex_ ? I thought it only protects
>>> pendingStreamsToProcess_.
>>
>> Ah, I don't see why we need to lock it. This is a good catch.
>>
>> For the async version, the main thread isn't expected to call
>> setBufferStatus() on the same descriptor (because we are already
>> handling synchronous errors beforehand), so I don't expect to race. So I
>> should remove setBufferStatus() from the lock section.
>>
>>>> +	request->pendingStreamsToProcess_.erase(streamBuffer->stream);
>>>> +
>>>> +	if (request->pendingStreamsToProcess_.size() > 0)
>>> 	if (!request->pendingStreamsToProcess_.empty())
>>>
>>>> +		return;
>>>> +
>>>> +	locker.unlock();
>>>> +
>>>> +	completeDescriptor(streamBuffer->request);
>>>> +}
>>>> +
>>>>    std::string CameraDevice::logPrefix() const
>>>>    {
>>>>    	return "'" + camera_->id() + "'";
>>>> diff --git a/src/android/camera_device.h b/src/android/camera_device.h
>>>> index e544f2bd..2a414020 100644
>>>> --- a/src/android/camera_device.h
>>>> +++ b/src/android/camera_device.h
>>>> @@ -66,6 +66,8 @@ public:
>>>>    	int configureStreams(camera3_stream_configuration_t *stream_list);
>>>>    	int processCaptureRequest(camera3_capture_request_t *request);
>>>>    	void requestComplete(libcamera::Request *request);
>>>> +	void streamProcessingComplete(Camera3RequestDescriptor::StreamBuffer *bufferStream,
>>>> +				      Camera3RequestDescriptor::Status status);
>>>>    
>>>>    protected:
>>>>    	std::string logPrefix() const override;
>>>> @@ -95,6 +97,8 @@ private:
>>>>    	int processControls(Camera3RequestDescriptor *descriptor);
>>>>    	void completeDescriptor(Camera3RequestDescriptor *descriptor);
>>>>    	void sendCaptureResults();
>>>> +	void setBufferStatus(Camera3RequestDescriptor::StreamBuffer &buffer,
>>>> +			     Camera3RequestDescriptor::Status status);
>>>>    	std::unique_ptr<CameraMetadata> getResultMetadata(
>>>>    		const Camera3RequestDescriptor &descriptor) const;
>>>>    
>>>> diff --git a/src/android/camera_request.h b/src/android/camera_request.h
>>>> index c4bc5d6e..cc2b7035 100644
>>>> --- a/src/android/camera_request.h
>>>> +++ b/src/android/camera_request.h
>>>> @@ -7,7 +7,9 @@
>>>>    #ifndef __ANDROID_CAMERA_REQUEST_H__
>>>>    #define __ANDROID_CAMERA_REQUEST_H__
>>>>    
>>>> +#include <map>
>>>>    #include <memory>
>>>> +#include <mutex>
>>>>    #include <vector>
>>>>    
>>>>    #include <libcamera/base/class.h>
>>>> @@ -43,6 +45,10 @@ public:
>>>>    		Camera3RequestDescriptor *request;
>>>>    	};
>>>>    
>>>> +	/* Keeps track of streams requiring post-processing. */
>>>> +	std::map<CameraStream *, StreamBuffer *> pendingStreamsToProcess_;
>>>> +	std::mutex streamsProcessMutex_;
>>>> +
>>>>    	Camera3RequestDescriptor(libcamera::Camera *camera,
>>>>    				 const camera3_capture_request_t *camera3Request);
>>>>    	~Camera3RequestDescriptor();
>>>> diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp
>>>> index 0e268cdf..4e275cde 100644
>>>> --- a/src/android/camera_stream.cpp
>>>> +++ b/src/android/camera_stream.cpp
>>>> @@ -22,6 +22,7 @@
>>>>    #include "camera_capabilities.h"
>>>>    #include "camera_device.h"
>>>>    #include "camera_metadata.h"
>>>> +#include "post_processor.h"
>>>>    
>>>>    using namespace libcamera;
>>>>    
>>>> @@ -97,6 +98,20 @@ int CameraStream::configure()
>>>>    		int ret = postProcessor_->configure(configuration(), output);
>>>>    		if (ret)
>>>>    			return ret;
>>>> +
>>>> +		postProcessor_->processComplete.connect(
>>>> +			this, [&](Camera3RequestDescriptor::StreamBuffer *streamBuffer,
>>>> +				  PostProcessor::Status status) {
>>>> +				Camera3RequestDescriptor::Status bufferStatus;
>>>> +
>>>> +				if (status == PostProcessor::Status::Success)
>>>> +					bufferStatus = Camera3RequestDescriptor::Status::Success;
>>>> +				else
>>>> +					bufferStatus = Camera3RequestDescriptor::Status::Error;
>>>> +
>>>> +				cameraDevice_->streamProcessingComplete(streamBuffer,
>>>> +									bufferStatus);
>>>> +			});
>>>>    	}
>>>>    
>>>>    	if (type_ == Type::Internal) {
>>>> diff --git a/src/android/jpeg/post_processor_jpeg.cpp b/src/android/jpeg/post_processor_jpeg.cpp
>>>> index da71f113..cbbe7128 100644
>>>> --- a/src/android/jpeg/post_processor_jpeg.cpp
>>>> +++ b/src/android/jpeg/post_processor_jpeg.cpp
>>>> @@ -198,6 +198,7 @@ int PostProcessorJpeg::process(Camera3RequestDescriptor::StreamBuffer *streamBuf
>>>>    					 exif.data(), quality);
>>>>    	if (jpeg_size < 0) {
>>>>    		LOG(JPEG, Error) << "Failed to encode stream image";
>>>> +		processComplete.emit(streamBuffer, PostProcessor::Status::Error);
>>>>    		return jpeg_size;
>>>>    	}
>>>>    
>>>> @@ -211,6 +212,7 @@ int PostProcessorJpeg::process(Camera3RequestDescriptor::StreamBuffer *streamBuf
>>>>    
>>>>    	/* Update the JPEG result Metadata. */
>>>>    	resultMetadata->addEntry(ANDROID_JPEG_SIZE, jpeg_size);
>>>> +	processComplete.emit(streamBuffer, PostProcessor::Status::Success);
>>>>    
>>>>    	return 0;
>>>>    }
>>>> diff --git a/src/android/post_processor.h b/src/android/post_processor.h
>>>> index 128161c8..4ac74fcf 100644
>>>> --- a/src/android/post_processor.h
>>>> +++ b/src/android/post_processor.h
>>>> @@ -7,6 +7,8 @@
>>>>    #ifndef __ANDROID_POST_PROCESSOR_H__
>>>>    #define __ANDROID_POST_PROCESSOR_H__
>>>>    
>>>> +#include <libcamera/base/signal.h>
>>>> +
>>>>    #include <libcamera/framebuffer.h>
>>>>    #include <libcamera/stream.h>
>>>>    
>>>> @@ -16,11 +18,18 @@
>>>>    class PostProcessor
>>>>    {
>>>>    public:
>>>> +	enum class Status {
>>>> +		Error,
>>>> +		Success
>>>> +	};
>>>> +
>>>>    	virtual ~PostProcessor() = default;
>>>>    
>>>>    	virtual int configure(const libcamera::StreamConfiguration &inCfg,
>>>>    			      const libcamera::StreamConfiguration &outCfg) = 0;
>>>>    	virtual int process(Camera3RequestDescriptor::StreamBuffer *streamBuffer) = 0;
>>>> +
>>>> +	libcamera::Signal<Camera3RequestDescriptor::StreamBuffer *, Status> processComplete;
>>>>    };
>>>>    
>>>>    #endif /* __ANDROID_POST_PROCESSOR_H__ */
>>>> diff --git a/src/android/yuv/post_processor_yuv.cpp b/src/android/yuv/post_processor_yuv.cpp
>>>> index eeb8f1f4..8e77bf57 100644
>>>> --- a/src/android/yuv/post_processor_yuv.cpp
>>>> +++ b/src/android/yuv/post_processor_yuv.cpp
>>>> @@ -54,12 +54,15 @@ int PostProcessorYuv::process(Camera3RequestDescriptor::StreamBuffer *streamBuff
>>>>    	const FrameBuffer &source = *streamBuffer->srcBuffer;
>>>>    	CameraBuffer *destination = streamBuffer->destBuffer.get();
>>>>    
>>>> -	if (!isValidBuffers(source, *destination))
>>>> +	if (!isValidBuffers(source, *destination)) {
>>>> +		processComplete.emit(streamBuffer, PostProcessor::Status::Error);
>>>>    		return -EINVAL;
>>>> +	}
>>>>    
>>>>    	const MappedFrameBuffer sourceMapped(&source, MappedFrameBuffer::MapFlag::Read);
>>>>    	if (!sourceMapped.isValid()) {
>>>>    		LOG(YUV, Error) << "Failed to mmap camera frame buffer";
>>>> +		processComplete.emit(streamBuffer, PostProcessor::Status::Error);
>>>>    		return -EINVAL;
>>>>    	}
>>>>    
>>>> @@ -77,9 +80,12 @@ int PostProcessorYuv::process(Camera3RequestDescriptor::StreamBuffer *streamBuff
>>>>    				    libyuv::FilterMode::kFilterBilinear);
>>>>    	if (ret) {
>>>>    		LOG(YUV, Error) << "Failed NV12 scaling: " << ret;
>>>> +		processComplete.emit(streamBuffer, PostProcessor::Status::Error);
>>>>    		return -EINVAL;
>>>>    	}
>>>>    
>>>> +	processComplete.emit(streamBuffer, PostProcessor::Status::Success);
>>>> +
>>>>    	return 0;
>>>>    }
>>>>

Hi Umang,

On Mon, Oct 25, 2021 at 07:16:56PM +0530, Umang Jain wrote:
> On 10/25/21 6:55 PM, Laurent Pinchart wrote:
> > On Mon, Oct 25, 2021 at 06:46:39PM +0530, Umang Jain wrote:
> >> On 10/25/21 11:33 AM, Laurent Pinchart wrote:
> >>> On Sat, Oct 23, 2021 at 04:03:00PM +0530, Umang Jain wrote:
> >>>> Notify that the post processing for a request has been completed,
> >>>> via a signal. The signal emit with a context pointer along with status
> >>> s/emit/is emitted/ ?
> >>>
> >>>> of the buffer. The function CameraDevice::streamProcessingComplete() will
> >>>> finally set the status on the request descriptor and complete the
> >>>> descriptor if all the streams requiring post processing are completed.
> >>>> If buffer status obtained is in error state, notify the status to the
> >>>> framework and set the overall error status on the descriptor via
> >>>> setBufferStatus().
> >>>>
> >>>> We need to track the number of streams requiring post-processing
> >>>> per Camera3RequestDescriptor (i.e. per capture request). Introduce
> >>>> a std::map<> to track the post-processing of streams. The nodes
> >>>> are dropped from the map when a particular stream post processing
> >>>> is completed (or on error paths). A std::map is selected for tracking
> >>>> post-processing requests, since we will move post-processing to be
> >>>> asynchronous in subsequent commits. A vector or queue will not be
> >>>> suitable then as the sequential order of post-processing completion
> >>>> of various requests won't be guaranteed then.
> >>>>
> >>>> A streamProcessMutex_ has been introduced here as well, which will be
> >>> s/streamProcessMutex_/streamsProcessMutex_/
> >>>
> >>>> applicable to guard access to descriptor's pendingStreamsToProcess_ when
> >>>> post-processing is moved to be asynchronous in subsequent commits.
> >>>>
> >>>> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
> >>>> ---
> >>>>    src/android/camera_device.cpp            | 95 ++++++++++++++++++------
> >>>>    src/android/camera_device.h              |  4 +
> >>>>    src/android/camera_request.h             |  6 ++
> >>>>    src/android/camera_stream.cpp            | 15 ++++
> >>>>    src/android/jpeg/post_processor_jpeg.cpp |  2 +
> >>>>    src/android/post_processor.h             |  9 +++
> >>>>    src/android/yuv/post_processor_yuv.cpp   |  8 +-
> >>>>    7 files changed, 114 insertions(+), 25 deletions(-)
> >>>>
> >>>> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> >>>> index 2a98a2e6..3114def0 100644
> >>>> --- a/src/android/camera_device.cpp
> >>>> +++ b/src/android/camera_device.cpp
> >>>> @@ -926,6 +926,8 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
> >>>>    			 * Request.
> >>>>    			 */
> >>>>    			LOG(HAL, Debug) << ss.str() << " (mapped)";
> >>> Blank line here, or no blank line below.
> >>>
> >>>> +			descriptor->pendingStreamsToProcess_.insert(
> >>>> +				{ cameraStream, &buffer });
> >>>>    			continue;
> >>>>    
> >>>>    		case CameraStream::Type::Direct:
> >>>> @@ -955,6 +957,9 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
> >>>>    			frameBuffer = cameraStream->getBuffer();
> >>>>    			buffer.internalBuffer = frameBuffer;
> >>>>    			LOG(HAL, Debug) << ss.str() << " (internal)";
> >>>> +
> >>>> +			descriptor->pendingStreamsToProcess_.insert(
> >>>> +				{ cameraStream, &buffer });
> >>>>    			break;
> >>>>    		}
> >>>>    
> >>>> @@ -1118,43 +1123,46 @@ void CameraDevice::requestComplete(Request *request)
> >>>>    	}
> >>>>    
> >>>>    	/* Handle post-processing. */
> >>>> -	bool hasPostProcessingErrors = false;
> >>>> -	for (auto &buffer : descriptor->buffers_) {
> >>>> -		CameraStream *stream = buffer.stream;
> >>>> -
> >>>> -		if (stream->type() == CameraStream::Type::Direct)
> >>>> -			continue;
> >>>> +	bool needPostProcessing = false;
> >>>> +	/*
> >>>> +	 * \todo Protect the loop below with streamProcessMutex_ when post
> >>>> +	 * processor runs asynchronously.
> >>>> +	 */
> >>>> +	auto iter = descriptor->pendingStreamsToProcess_.begin();
> >>>> +	while (descriptor->pendingStreamsToProcess_.size() > 0) {
> >>> 	while (!descriptor->pendingStreamsToProcess_.empty()) {
> >>>
> >>> But it's not correct. The fix is in 7/7:
> >>>
> >>> 	while (iter != descriptor->pendingStreamsToProcess_.end()) {
> >> Yes, it was a tricky to handle the loop for async and sync variations
> >>
> >> For the async one, iter == end() was the only choice I  believe (Without
> >> introducing a counter)
> >>
> >>>> +		CameraStream *stream = iter->first;
> >>>> +		Camera3RequestDescriptor::StreamBuffer *buffer = iter->second;
> >>>> +		needPostProcessing = true;
> >>>>    
> >>>>    		FrameBuffer *src = request->findBuffer(stream->stream());
> >>>>    		if (!src) {
> >>>>    			LOG(HAL, Error) << "Failed to find a source stream buffer";
> >>>> -			buffer.status = Camera3RequestDescriptor::Status::Error;
> >>>> -			notifyError(descriptor->frameNumber_, stream->camera3Stream(),
> >>>> -				    CAMERA3_MSG_ERROR_BUFFER);
> >>>> -			hasPostProcessingErrors = true;
> >>>> +			setBufferStatus(*buffer, Camera3RequestDescriptor::Status::Error);
> >>>> +			iter = descriptor->pendingStreamsToProcess_.erase(iter);
> >>>>    			continue;
> >>>>    		}
> >>>>    
> >>>> -		int ret = stream->process(*src, buffer);
> >>>> +		++iter;
> >>>> +		int ret = stream->process(*src, *buffer);
> >>>> +		if (ret) {
> >>>> +			setBufferStatus(*buffer, Camera3RequestDescriptor::Status::Error);
> >>>> +			descriptor->pendingStreamsToProcess_.erase(stream);
> >>>> +		}
> >>>> +	}
> >>>>    
> >>>> +	if (needPostProcessing) {
> >>>>    		/*
> >>>> -		 * If the framebuffer is internal to CameraStream return it back
> >>>> -		 * now that we're done processing it.
> >>>> +		 * \todo We will require to check if we failed to queue
> >>>> +		 * post-processing requests when we migrate to post-processor
> >>>> +		 * running asynchronously.
> >>>> +		 *
> >>>> +		 * if (descriptor->pendingStreamsToProcess_.size() == 0)
> >>>> +		 *	completeDescriptor(descriptor);
> >>> Can't we do this already here ? I think you can actually drop the
> >>> needPostProcessing variable and just write
> >>>
> >>> 	if (descriptor->pendingStreamsToProcess_.empty())
> >>> 		completeDescriptor(descriptor);
> >>>
> >>> as needPostProcessing can only be false if pendingStreamsToProcess_ was
> >>> empty before the while loop above, and it will thus be empty after the
> >>> loop as well in that case.
> >> True, but we need needPostProcessing variable for async.
> > Why is that ? Won't
> >
> >   	if (descriptor->pendingStreamsToProcess_.empty())
> >   		completeDescriptor(descriptor);
> >
> > work in async mode too ?
> 
> 
> Ok yes, it should work (sorry I confused myself a bit, thinking it's 
> will be a double completeDescriptor(descriptor) for the same descriptor, 
> in case post-processing completes early when main thread reaches this 
> line. But I forgot that we are already holding a lock restricted slots 
> to run)
> 
> We would end up requestComplete() like: https://paste.debian.net/1216820/

With

	descriptor->pendingStreamsToProcess_.size() == 0

replaced with

	descriptor->pendingStreamsToProcess_.empty()

it seems good to me.

> >> I pre-empted
> >> its introduction deliberately to set the design beforehand andthen,  I
> >> can introduce async bits with minimal diff for
> >> CamreraDevice::requestComplete()
> >>
> >>>>    		 */
> >>>> -		if (buffer.internalBuffer)
> >>>> -			stream->putBuffer(buffer.internalBuffer);
> >>>>    
> >>>> -		if (ret) {
> >>>> -			buffer.status = Camera3RequestDescriptor::Status::Error;
> >>>> -			hasPostProcessingErrors = true;
> >>>> -			notifyError(descriptor->frameNumber_, stream->camera3Stream(),
> >>>> -				    CAMERA3_MSG_ERROR_BUFFER);
> >>>> -		}
> >>>> +		return;
> >>>>    	}
> >>>>    
> >>>> -	if (hasPostProcessingErrors)
> >>>> -		descriptor->status_ = Camera3RequestDescriptor::Status::Error;
> >>>> -
> >>>>    	completeDescriptor(descriptor);
> >>>>    }
> >>>>    
> >>>> @@ -1210,6 +1218,45 @@ void CameraDevice::sendCaptureResults()
> >>>>    	}
> >>>>    }
> >>>>    
> >>>> +void CameraDevice::setBufferStatus(Camera3RequestDescriptor::StreamBuffer &streamBuffer,
> >>>> +				   Camera3RequestDescriptor::Status status)
> >>>> +{
> >>>> +	/*
> >>>> +	 * If the framebuffer is internal to CameraStream return it back now
> >>>> +	 * that we're done processing it.
> >>>> +	 */
> >>>> +	if (streamBuffer.internalBuffer)
> >>>> +		streamBuffer.stream->putBuffer(streamBuffer.internalBuffer);
> >>> I'd move this to the caller, as it's not about the buffer status.
> >>>
> >>>> +
> >>>> +	streamBuffer.status = status;
> >>>> +	if (status != Camera3RequestDescriptor::Status::Success) {
> >>>> +		notifyError(streamBuffer.request->frameNumber_,
> >>>> +			    streamBuffer.stream->camera3Stream(),
> >>>> +			    CAMERA3_MSG_ERROR_BUFFER);
> >>>> +
> >>>> +		/* Also set error status on entire request descriptor. */
> >>>> +		streamBuffer.request->status_ =
> >>>> +			Camera3RequestDescriptor::Status::Error;
> >>>> +	}
> >>>> +}
> >>>> +
> >>>> +void CameraDevice::streamProcessingComplete(Camera3RequestDescriptor::StreamBuffer *streamBuffer,
> >>>> +					    Camera3RequestDescriptor::Status status)
> >>>> +{
> >>>> +	Camera3RequestDescriptor *request = streamBuffer->request;
> >>>> +	MutexLocker locker(request->streamsProcessMutex_);
> >>>> +
> >>>> +	setBufferStatus(*streamBuffer, status);
> >>> Do we need to protect the setBufferStatus() call with
> >>> streamsProcessMutex_ ? I thought it only protects
> >>> pendingStreamsToProcess_.
> >>
> >> Ah, I don't see why we need to lock it. This is a good catch.
> >>
> >> For the async version, the main thread isn't expected to call
> >> setBufferStatus() on the same descriptor (because we are already
> >> handling synchronous errors beforehand), so I don't expect to race. So I
> >> should remove setBufferStatus() from the lock section.
> >>
> >>>> +	request->pendingStreamsToProcess_.erase(streamBuffer->stream);
> >>>> +
> >>>> +	if (request->pendingStreamsToProcess_.size() > 0)
> >>> 	if (!request->pendingStreamsToProcess_.empty())
> >>>
> >>>> +		return;
> >>>> +
> >>>> +	locker.unlock();
> >>>> +
> >>>> +	completeDescriptor(streamBuffer->request);
> >>>> +}
> >>>> +
> >>>>    std::string CameraDevice::logPrefix() const
> >>>>    {
> >>>>    	return "'" + camera_->id() + "'";
> >>>> diff --git a/src/android/camera_device.h b/src/android/camera_device.h
> >>>> index e544f2bd..2a414020 100644
> >>>> --- a/src/android/camera_device.h
> >>>> +++ b/src/android/camera_device.h
> >>>> @@ -66,6 +66,8 @@ public:
> >>>>    	int configureStreams(camera3_stream_configuration_t *stream_list);
> >>>>    	int processCaptureRequest(camera3_capture_request_t *request);
> >>>>    	void requestComplete(libcamera::Request *request);
> >>>> +	void streamProcessingComplete(Camera3RequestDescriptor::StreamBuffer *bufferStream,
> >>>> +				      Camera3RequestDescriptor::Status status);
> >>>>    
> >>>>    protected:
> >>>>    	std::string logPrefix() const override;
> >>>> @@ -95,6 +97,8 @@ private:
> >>>>    	int processControls(Camera3RequestDescriptor *descriptor);
> >>>>    	void completeDescriptor(Camera3RequestDescriptor *descriptor);
> >>>>    	void sendCaptureResults();
> >>>> +	void setBufferStatus(Camera3RequestDescriptor::StreamBuffer &buffer,
> >>>> +			     Camera3RequestDescriptor::Status status);
> >>>>    	std::unique_ptr<CameraMetadata> getResultMetadata(
> >>>>    		const Camera3RequestDescriptor &descriptor) const;
> >>>>    
> >>>> diff --git a/src/android/camera_request.h b/src/android/camera_request.h
> >>>> index c4bc5d6e..cc2b7035 100644
> >>>> --- a/src/android/camera_request.h
> >>>> +++ b/src/android/camera_request.h
> >>>> @@ -7,7 +7,9 @@
> >>>>    #ifndef __ANDROID_CAMERA_REQUEST_H__
> >>>>    #define __ANDROID_CAMERA_REQUEST_H__
> >>>>    
> >>>> +#include <map>
> >>>>    #include <memory>
> >>>> +#include <mutex>
> >>>>    #include <vector>
> >>>>    
> >>>>    #include <libcamera/base/class.h>
> >>>> @@ -43,6 +45,10 @@ public:
> >>>>    		Camera3RequestDescriptor *request;
> >>>>    	};
> >>>>    
> >>>> +	/* Keeps track of streams requiring post-processing. */
> >>>> +	std::map<CameraStream *, StreamBuffer *> pendingStreamsToProcess_;
> >>>> +	std::mutex streamsProcessMutex_;
> >>>> +
> >>>>    	Camera3RequestDescriptor(libcamera::Camera *camera,
> >>>>    				 const camera3_capture_request_t *camera3Request);
> >>>>    	~Camera3RequestDescriptor();
> >>>> diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp
> >>>> index 0e268cdf..4e275cde 100644
> >>>> --- a/src/android/camera_stream.cpp
> >>>> +++ b/src/android/camera_stream.cpp
> >>>> @@ -22,6 +22,7 @@
> >>>>    #include "camera_capabilities.h"
> >>>>    #include "camera_device.h"
> >>>>    #include "camera_metadata.h"
> >>>> +#include "post_processor.h"
> >>>>    
> >>>>    using namespace libcamera;
> >>>>    
> >>>> @@ -97,6 +98,20 @@ int CameraStream::configure()
> >>>>    		int ret = postProcessor_->configure(configuration(), output);
> >>>>    		if (ret)
> >>>>    			return ret;
> >>>> +
> >>>> +		postProcessor_->processComplete.connect(
> >>>> +			this, [&](Camera3RequestDescriptor::StreamBuffer *streamBuffer,
> >>>> +				  PostProcessor::Status status) {
> >>>> +				Camera3RequestDescriptor::Status bufferStatus;
> >>>> +
> >>>> +				if (status == PostProcessor::Status::Success)
> >>>> +					bufferStatus = Camera3RequestDescriptor::Status::Success;
> >>>> +				else
> >>>> +					bufferStatus = Camera3RequestDescriptor::Status::Error;
> >>>> +
> >>>> +				cameraDevice_->streamProcessingComplete(streamBuffer,
> >>>> +									bufferStatus);
> >>>> +			});
> >>>>    	}
> >>>>    
> >>>>    	if (type_ == Type::Internal) {
> >>>> diff --git a/src/android/jpeg/post_processor_jpeg.cpp b/src/android/jpeg/post_processor_jpeg.cpp
> >>>> index da71f113..cbbe7128 100644
> >>>> --- a/src/android/jpeg/post_processor_jpeg.cpp
> >>>> +++ b/src/android/jpeg/post_processor_jpeg.cpp
> >>>> @@ -198,6 +198,7 @@ int PostProcessorJpeg::process(Camera3RequestDescriptor::StreamBuffer *streamBuf
> >>>>    					 exif.data(), quality);
> >>>>    	if (jpeg_size < 0) {
> >>>>    		LOG(JPEG, Error) << "Failed to encode stream image";
> >>>> +		processComplete.emit(streamBuffer, PostProcessor::Status::Error);
> >>>>    		return jpeg_size;
> >>>>    	}
> >>>>    
> >>>> @@ -211,6 +212,7 @@ int PostProcessorJpeg::process(Camera3RequestDescriptor::StreamBuffer *streamBuf
> >>>>    
> >>>>    	/* Update the JPEG result Metadata. */
> >>>>    	resultMetadata->addEntry(ANDROID_JPEG_SIZE, jpeg_size);
> >>>> +	processComplete.emit(streamBuffer, PostProcessor::Status::Success);
> >>>>    
> >>>>    	return 0;
> >>>>    }
> >>>> diff --git a/src/android/post_processor.h b/src/android/post_processor.h
> >>>> index 128161c8..4ac74fcf 100644
> >>>> --- a/src/android/post_processor.h
> >>>> +++ b/src/android/post_processor.h
> >>>> @@ -7,6 +7,8 @@
> >>>>    #ifndef __ANDROID_POST_PROCESSOR_H__
> >>>>    #define __ANDROID_POST_PROCESSOR_H__
> >>>>    
> >>>> +#include <libcamera/base/signal.h>
> >>>> +
> >>>>    #include <libcamera/framebuffer.h>
> >>>>    #include <libcamera/stream.h>
> >>>>    
> >>>> @@ -16,11 +18,18 @@
> >>>>    class PostProcessor
> >>>>    {
> >>>>    public:
> >>>> +	enum class Status {
> >>>> +		Error,
> >>>> +		Success
> >>>> +	};
> >>>> +
> >>>>    	virtual ~PostProcessor() = default;
> >>>>    
> >>>>    	virtual int configure(const libcamera::StreamConfiguration &inCfg,
> >>>>    			      const libcamera::StreamConfiguration &outCfg) = 0;
> >>>>    	virtual int process(Camera3RequestDescriptor::StreamBuffer *streamBuffer) = 0;
> >>>> +
> >>>> +	libcamera::Signal<Camera3RequestDescriptor::StreamBuffer *, Status> processComplete;
> >>>>    };
> >>>>    
> >>>>    #endif /* __ANDROID_POST_PROCESSOR_H__ */
> >>>> diff --git a/src/android/yuv/post_processor_yuv.cpp b/src/android/yuv/post_processor_yuv.cpp
> >>>> index eeb8f1f4..8e77bf57 100644
> >>>> --- a/src/android/yuv/post_processor_yuv.cpp
> >>>> +++ b/src/android/yuv/post_processor_yuv.cpp
> >>>> @@ -54,12 +54,15 @@ int PostProcessorYuv::process(Camera3RequestDescriptor::StreamBuffer *streamBuff
> >>>>    	const FrameBuffer &source = *streamBuffer->srcBuffer;
> >>>>    	CameraBuffer *destination = streamBuffer->destBuffer.get();
> >>>>    
> >>>> -	if (!isValidBuffers(source, *destination))
> >>>> +	if (!isValidBuffers(source, *destination)) {
> >>>> +		processComplete.emit(streamBuffer, PostProcessor::Status::Error);
> >>>>    		return -EINVAL;
> >>>> +	}
> >>>>    
> >>>>    	const MappedFrameBuffer sourceMapped(&source, MappedFrameBuffer::MapFlag::Read);
> >>>>    	if (!sourceMapped.isValid()) {
> >>>>    		LOG(YUV, Error) << "Failed to mmap camera frame buffer";
> >>>> +		processComplete.emit(streamBuffer, PostProcessor::Status::Error);
> >>>>    		return -EINVAL;
> >>>>    	}
> >>>>    
> >>>> @@ -77,9 +80,12 @@ int PostProcessorYuv::process(Camera3RequestDescriptor::StreamBuffer *streamBuff
> >>>>    				    libyuv::FilterMode::kFilterBilinear);
> >>>>    	if (ret) {
> >>>>    		LOG(YUV, Error) << "Failed NV12 scaling: " << ret;
> >>>> +		processComplete.emit(streamBuffer, PostProcessor::Status::Error);
> >>>>    		return -EINVAL;
> >>>>    	}
> >>>>    
> >>>> +	processComplete.emit(streamBuffer, PostProcessor::Status::Success);
> >>>> +
> >>>>    	return 0;
> >>>>    }
> >>>>