[libcamera-devel] [PATCH v3 07/10] android: post_processor: Notify post processing completion status

[Laurent Pinchart]

Hello,

On Tue, Sep 21, 2021 at 01:29:28PM +0200, Jacopo Mondi wrote:
> On Mon, Sep 20, 2021 at 11:07:49PM +0530, Umang Jain wrote:
> > We should be able to know if post-processing has been completed
> > successfully or encountered some errors. This commit introduces a
> > Signal<> that will notify that the post-processing has been completed.
> > If the post processing was successful, status on the request descriptor
> > will be set to Camera3RequestDescriptor::ProcessStatus::Success.
> > The signal will be required when the post-processor is meant to
> > run asynchronously (in subsequent commits) and capture results need
> > to be sent back to the framework from the signal's slot instead.
> >
> > Signed-off-by: Umang Jain <umang.jain at ideasonboard.com>
> > ---
> >  src/android/camera_device.cpp            | 18 ++++++++++++++++++
> >  src/android/camera_device.h              | 11 +++++++++++
> >  src/android/camera_stream.cpp            | 15 ++++++++++++++-
> >  src/android/camera_stream.h              |  2 ++
> >  src/android/jpeg/post_processor_jpeg.cpp | 10 +++++++++-
> >  src/android/post_processor.h             |  4 ++++
> >  src/android/yuv/post_processor_yuv.cpp   | 16 ++++++++++++++--
> >  7 files changed, 72 insertions(+), 4 deletions(-)
> >
> > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> > index fa462368..e2de4012 100644
> > --- a/src/android/camera_device.cpp
> > +++ b/src/android/camera_device.cpp
> > @@ -246,6 +246,12 @@ Camera3RequestDescriptor::Camera3RequestDescriptor(
> >  	 */
> >  	request_ = std::make_unique<CaptureRequest>(camera,
> >  						    reinterpret_cast<uint64_t>(this));
> > +
> > +	/*
> > +	 * Denotes the post-processing status required by any stream. It is set
> > +	 * to ProcessStatus::Success if processing is successful.
> > +	 */
> > +	status_ = ProcessStatus::None;
> 
> Looking at how the status is used in CameraDevice, I would rather make
> it a paramter of CameraDevice::streamProcessingComplete() and in the
> following patch of sendCaptureResults().
> 
> This way you could avoid there
> 	if (d->status_ == Camera3RequestDescriptor::ProcessStatus::Processing ||
> 	    d->status_ == Camera3RequestDescriptor::ProcessStatus::None)
> 		return;

I'm in two minds about this. For the PostProcessor::processComplete()
signal, I agree, we could pass a status back as a signal parameter. It
would have the advantage of not having to set the request status in
quite a few locations in the post-processors. In that case, the status
should be a

	enum class Status {
		Success,
		Error,
	};

in the PostProcessor class, as we don't need more than that.

However, once we reach CameraStream::handleProcessComplete(), I think it
would make sense to store the status in Camera3RequestDescriptor, to
keep all request-related data together. This will be needed when
iterating over all descriptors in the queue to find out which ones are
complete. We would need a different Camera3RequestDescriptor::Status
there, and I think it should indicate the status of the request, not the
status of the post-processing. We would thus have

	enum class Status {
		Pending,
		Success,
		Error,
	};

in Camera3RequestDescriptor.

> >  }
> >
> >  /*
> > @@ -1150,6 +1156,8 @@ void CameraDevice::requestComplete(Request *request)
> >  			continue;
> >  		}
> >
> > +		descriptor->status_ = Camera3RequestDescriptor::ProcessStatus::Processing;
> > +
> >  		int ret = cameraStream->process(src, *buffer.buffer, descriptor);
> >
> >  		/*
> > @@ -1172,6 +1180,16 @@ void CameraDevice::requestComplete(Request *request)
> >  	descriptors_.pop_front();
> >  }
> >
> > +
> > +void CameraDevice::streamProcessingComplete([[maybe_unused]] CameraStream *cameraStream,
> > +					    [[maybe_unused]] Camera3RequestDescriptor *request)
> > +{
> > +	/*
> > +	 * \todo Stream processing is completed hence, check for errors and
> > +	 * if any, mark the corresponding buffer with CAMERA3_BUFFER_STATUS_ERROR.
> > +	 */
> > +}
> > +
> >  std::string CameraDevice::logPrefix() const
> >  {
> >  	return "'" + camera_->id() + "'";
> > diff --git a/src/android/camera_device.h b/src/android/camera_device.h
> > index b2871e52..60c134dc 100644
> > --- a/src/android/camera_device.h
> > +++ b/src/android/camera_device.h
> > @@ -36,6 +36,13 @@
> >  struct CameraConfigData;
> >
> >  struct Camera3RequestDescriptor {
> > +	enum class ProcessStatus {
> > +		None,
> > +		Processing,
> > +		Success,
> > +		Error
> > +	};
> > +
> >  	Camera3RequestDescriptor() = default;
> >  	~Camera3RequestDescriptor() = default;
> >  	Camera3RequestDescriptor(libcamera::Camera *camera,
> > @@ -48,6 +55,8 @@ struct Camera3RequestDescriptor {
> >  	CameraMetadata settings_;
> >  	std::unique_ptr<CaptureRequest> request_;
> >  	std::unique_ptr<CameraMetadata> resultMetadata_;
> > +
> > +	ProcessStatus status_;
> >  };
> >
> >  class CameraDevice : protected libcamera::Loggable
> > @@ -79,6 +88,8 @@ public:
> >  	int configureStreams(camera3_stream_configuration_t *stream_list);
> >  	int processCaptureRequest(camera3_capture_request_t *request);
> >  	void requestComplete(libcamera::Request *request);
> > +	void streamProcessingComplete(CameraStream *cameraStream,
> > +				      Camera3RequestDescriptor *request);
> >
> >  protected:
> >  	std::string logPrefix() const override;
> > diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp
> > index d494f5d5..70471959 100644
> > --- a/src/android/camera_stream.cpp
> > +++ b/src/android/camera_stream.cpp
> > @@ -81,6 +81,9 @@ int CameraStream::configure()
> >  		int ret = postProcessor_->configure(configuration(), output);
> >  		if (ret)
> >  			return ret;
> > +
> > +		postProcessor_->processComplete.connect(
> > +			this, &CameraStream::handleProcessComplete);
> >  	}
> >
> >  	if (allocator_) {
> > @@ -102,8 +105,11 @@ int CameraStream::process(const FrameBuffer *source,
> >  			  buffer_handle_t camera3Dest,
> >  			  Camera3RequestDescriptor *request)
> >  {
> > -	if (!postProcessor_)
> > +	if (!postProcessor_) {
> > +		request->status_ = Camera3RequestDescriptor::ProcessStatus::Error;
> > +		handleProcessComplete(request);
> >  		return 0;
> > +	}
> 
> That was like this before, but I feel like this is a development
> mistaken and should be better caught with Fatal
> 
> same for the below !encoder and the YUV post-processor

Or

	ASSERT(postProcessor_);

?

> >
> >  	/*
> >  	 * \todo Buffer mapping and processing should be moved to a
> > @@ -114,12 +120,19 @@ int CameraStream::process(const FrameBuffer *source,
> >  			  PROT_READ | PROT_WRITE);
> >  	if (!dest.isValid()) {
> >  		LOG(HAL, Error) << "Failed to map android blob buffer";
> > +		request->status_ = Camera3RequestDescriptor::ProcessStatus::Error;
> > +		handleProcessComplete(request);
> >  		return -EINVAL;
> >  	}
> >
> >  	return postProcessor_->process(source, &dest, request);
> >  }
> >
> > +void CameraStream::handleProcessComplete(Camera3RequestDescriptor *request)
> > +{
> > +	cameraDevice_->streamProcessingComplete(this, request);
> > +}
> 
> Can't CameraStream connect CameraDevice::streamProcessingComplete with
> the post-processor's processComplete signal ?

We can do so with a lambda function, now supported by the signal class:

		postProcessor_->processComplete.connect(
			[&](Camera3RequestDescriptor *request) {
				cameraDevice_->streamProcessingComplete(this, request);
			}
		);

> > +
> >  FrameBuffer *CameraStream::getBuffer()
> >  {
> >  	if (!allocator_)
> > diff --git a/src/android/camera_stream.h b/src/android/camera_stream.h
> > index 68789700..f8ad6245 100644
> > --- a/src/android/camera_stream.h
> > +++ b/src/android/camera_stream.h
> > @@ -127,6 +127,8 @@ public:
> >  	void putBuffer(libcamera::FrameBuffer *buffer);
> >
> >  private:
> > +	void handleProcessComplete(Camera3RequestDescriptor *request);
> > +
> >  	CameraDevice *const cameraDevice_;
> >  	const libcamera::CameraConfiguration *config_;
> >  	const Type type_;
> > diff --git a/src/android/jpeg/post_processor_jpeg.cpp b/src/android/jpeg/post_processor_jpeg.cpp
> > index 31f68330..87252acd 100644
> > --- a/src/android/jpeg/post_processor_jpeg.cpp
> > +++ b/src/android/jpeg/post_processor_jpeg.cpp
> > @@ -101,8 +101,11 @@ int PostProcessorJpeg::process(const FrameBuffer *source,
> >  			       CameraBuffer *destination,
> >  			       Camera3RequestDescriptor *request)
> >  {
> > -	if (!encoder_)
> > +	if (!encoder_) {
> > +		request->status_ = Camera3RequestDescriptor::ProcessStatus::Error;
> > +		processComplete.emit(request);
> >  		return 0;
> > +	}
> >
> >  	ASSERT(destination->numPlanes() == 1);
> >
> > @@ -197,6 +200,8 @@ int PostProcessorJpeg::process(const FrameBuffer *source,
> >  					 exif.data(), quality);
> >  	if (jpeg_size < 0) {
> >  		LOG(JPEG, Error) << "Failed to encode stream image";
> > +		request->status_ = Camera3RequestDescriptor::ProcessStatus::Error;
> > +		processComplete.emit(request);
> >  		return jpeg_size;
> >  	}
> >
> > @@ -211,5 +216,8 @@ int PostProcessorJpeg::process(const FrameBuffer *source,
> >  	/* Update the JPEG result Metadata. */
> >  	resultMetadata->addEntry(ANDROID_JPEG_SIZE, jpeg_size);
> >
> > +	request->status_ = Camera3RequestDescriptor::ProcessStatus::Success;
> > +	processComplete.emit(request);
> > +
> >  	return 0;
> >  }
> > diff --git a/src/android/post_processor.h b/src/android/post_processor.h
> > index bdd6afe7..f639b237 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>
> >
> > @@ -26,6 +28,8 @@ public:
> >  	virtual int process(const libcamera::FrameBuffer *source,
> >  			    CameraBuffer *destination,
> >  			    Camera3RequestDescriptor *request) = 0;
> > +
> > +	libcamera::Signal<Camera3RequestDescriptor *> 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 5e18caee..b144649a 100644
> > --- a/src/android/yuv/post_processor_yuv.cpp
> > +++ b/src/android/yuv/post_processor_yuv.cpp
> > @@ -18,6 +18,8 @@
> >  #include "libcamera/internal/formats.h"
> >  #include "libcamera/internal/mapped_framebuffer.h"
> >
> > +#include "../camera_device.h"
> > +
> >  using namespace libcamera;
> >
> >  LOG_DEFINE_CATEGORY(YUV)
> > @@ -51,14 +53,19 @@ int PostProcessorYuv::configure(const StreamConfiguration &inCfg,
> >
> >  int PostProcessorYuv::process(const FrameBuffer *source,
> >  			      CameraBuffer *destination,
> > -			      [[maybe_unused]] Camera3RequestDescriptor *request)
> > +			      Camera3RequestDescriptor *request)
> >  {
> > -	if (!isValidBuffers(source, *destination))
> > +	if (!isValidBuffers(source, *destination)) {
> > +		request->status_ = Camera3RequestDescriptor::ProcessStatus::Error;
> > +		processComplete.emit(request);
> 
> If my first suggestion will be considered, the post processor will
> send the status in the signal, and you can reduce the number of status
> to Success or Error maybe ?
> 
> >  		return -EINVAL;
> > +	}
> >
> >  	const MappedFrameBuffer sourceMapped(source, MappedFrameBuffer::MapFlag::Read);
> >  	if (!sourceMapped.isValid()) {
> >  		LOG(YUV, Error) << "Failed to mmap camera frame buffer";
> > +		request->status_ = Camera3RequestDescriptor::ProcessStatus::Error;
> > +		processComplete.emit(request);
> >  		return -EINVAL;
> >  	}
> >
> > @@ -76,9 +83,14 @@ int PostProcessorYuv::process(const FrameBuffer *source,
> >  				    libyuv::FilterMode::kFilterBilinear);
> >  	if (ret) {
> >  		LOG(YUV, Error) << "Failed NV12 scaling: " << ret;
> > +		request->status_ = Camera3RequestDescriptor::ProcessStatus::Error;
> > +		processComplete.emit(request);
> >  		return -EINVAL;
> >  	}
> >
> > +	request->status_ = Camera3RequestDescriptor::ProcessStatus::Success;
> > +	processComplete.emit(request);
> > +
> >  	return 0;
> >  }
> >

-- 
Regards,

Laurent Pinchart