[libcamera-devel] [PATCH v2 8/9] android: camera_stream: Run post-processor in a separate thread

[Laurent Pinchart]

Hi Umang,

Thank you for the patch.

On Fri, Sep 10, 2021 at 12:36:37PM +0530, Umang Jain wrote:
> In CameraStream, introduce a new private PostProcessorWorker
> class derived from Object. A instance of PostProcessorWorker
> is moved to a separate thread instance which will be responsible
> to run the post-processor.
> 
> Running PostProcessor asynchronously should entail that all the
> data context needed by the PostProcessor should remain valid for
> the entire duration of its run. Most of the context preserving
> part has been addressed in the previous commits, we just need to
> ensure the source framebuffer data that comes via Camera::Request,
> should remain valid for the entire duration of post-processing
> running asynchronously. In order to so, we maintain a separate
> copy of the framebuffer data and add it to the Camera3RequestDescriptor
> structure in which we preserve rest of the context.
> 
> Signed-off-by: Umang Jain <umang.jain at ideasonboard.com>
> ---
>  src/android/camera_device.cpp | 19 ++++++++++++++++++-
>  src/android/camera_device.h   |  4 ++++
>  src/android/camera_stream.cpp | 16 ++++++++++++++--
>  src/android/camera_stream.h   | 28 ++++++++++++++++++++++++++++
>  4 files changed, 64 insertions(+), 3 deletions(-)
> 
> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> index 988d4232..73eb5758 100644
> --- a/src/android/camera_device.cpp
> +++ b/src/android/camera_device.cpp
> @@ -1174,13 +1174,29 @@ void CameraDevice::requestComplete(Request *request)
>  			return;
>  		}
>  
> +		FrameBuffer *source = src;
> +		if (cameraStream->type() != CameraStream::Type::Internal) {
> +			/*
> +			 * The source buffer is owned by Request object which
> +			 * can be reused by libcamera. Since post-processor will
> +			 * run asynchrnously, we need to copy the request's
> +			 * frame buffer and use that as the source buffer for
> +			 * post processing.

I don't think this is right. The request can be reused indeed, but with
new FrameBuffer instances every time. The frame buffers are owned by
Camera3RequestDescriptor, they're stored in a vector there.

> +			 */
> +			for (const auto &plane : src->planes())
> +				descriptor.srcPlanes_.push_back(plane);
> +			descriptor.srcFramebuffer_ =
> +				std::make_unique<FrameBuffer>(descriptor.srcPlanes_);
> +			source = descriptor.srcFramebuffer_.get();
> +		}
> +
>  		std::unique_ptr<Camera3RequestDescriptor> reqDescriptor =
>  			std::make_unique<Camera3RequestDescriptor>();
>  		*reqDescriptor = std::move(descriptor);
>  		queuedDescriptor_.push_back(std::move(reqDescriptor));
>  
>  		Camera3RequestDescriptor *currentDescriptor = queuedDescriptor_.back().get();
> -		int ret = cameraStream->process(src,
> +		int ret = cameraStream->process(source,
>  						currentDescriptor->destBuffer_.get(),
>  						currentDescriptor->settings_,
>  						currentDescriptor->resultMetadata_.get(),
> @@ -1255,6 +1271,7 @@ void CameraDevice::streamProcessingComplete(CameraStream *cameraStream,
>  
>  void CameraDevice::sendQueuedCaptureResults()
>  {
> +	MutexLocker lock(queuedDescriptorsMutex_);
>  	while (!queuedDescriptor_.empty()) {
>  		std::unique_ptr<Camera3RequestDescriptor> &d = queuedDescriptor_.front();
>  		if (d->status_ == Camera3RequestDescriptor::Pending)
> diff --git a/src/android/camera_device.h b/src/android/camera_device.h
> index b62d373c..ecdda06e 100644
> --- a/src/android/camera_device.h
> +++ b/src/android/camera_device.h
> @@ -62,6 +62,9 @@ struct Camera3RequestDescriptor {
>  	camera3_capture_result_t captureResult_;
>  	libcamera::FrameBuffer *internalBuffer_;
>  	completionStatus status_;
> +
> +	std::unique_ptr<libcamera::FrameBuffer> srcFramebuffer_;
> +	std::vector<libcamera::FrameBuffer::Plane> srcPlanes_;
>  };
>  
>  class CameraDevice : protected libcamera::Loggable
> @@ -147,6 +150,7 @@ private:
>  	libcamera::Mutex descriptorsMutex_; /* Protects descriptors_. */
>  	std::map<uint64_t, Camera3RequestDescriptor> descriptors_;
>  
> +	libcamera::Mutex queuedDescriptorsMutex_; /* Protects queuedDescriptor_. */

This belongs to the previous patch.

>  	std::deque<std::unique_ptr<Camera3RequestDescriptor>> queuedDescriptor_;
>  
>  	std::string maker_;
> diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp
> index 5fd04bbf..845e2462 100644
> --- a/src/android/camera_stream.cpp
> +++ b/src/android/camera_stream.cpp
> @@ -55,6 +55,15 @@ CameraStream::CameraStream(CameraDevice *const cameraDevice,
>  		 * is what we instantiate here.
>  		 */
>  		postProcessor_ = std::make_unique<PostProcessorJpeg>(cameraDevice_);
> +		ppWorker_ = std::make_unique<PostProcessorWorker>(postProcessor_.get());
> +
> +		thread_ = std::make_unique<libcamera::Thread>();
> +		ppWorker_->moveToThread(thread_.get());
> +		/*
> +		 * \todo: Class is MoveConstructible, so where to stop thread
> +		 * if we don't user-defined destructor? See RFC patch at the end.
> +		 */
> +		thread_->start();
>  	}
>  
>  	if (type == Type::Internal) {
> @@ -110,8 +119,11 @@ int CameraStream::process(const FrameBuffer *source,
>  	if (!postProcessor_)
>  		return 0;
>  
> -	return postProcessor_->process(source, destBuffer, requestMetadata,
> -				       resultMetadata, context);
> +	ppWorker_->invokeMethod(&PostProcessorWorker::process,
> +				ConnectionTypeQueued, source, destBuffer,
> +				requestMetadata, resultMetadata, context);
> +
> +	return 0;
>  }
>  
>  void CameraStream::postProcessingComplete(PostProcessor::Status status,
> diff --git a/src/android/camera_stream.h b/src/android/camera_stream.h
> index 8097ddbc..dbb7eee3 100644
> --- a/src/android/camera_stream.h
> +++ b/src/android/camera_stream.h
> @@ -13,7 +13,9 @@
>  
>  #include <hardware/camera3.h>
>  
> +#include <libcamera/base/object.h>
>  #include <libcamera/base/signal.h>
> +#include <libcamera/base/thread.h>
>  
>  #include <libcamera/camera.h>
>  #include <libcamera/framebuffer.h>
> @@ -23,6 +25,7 @@
>  
>  #include "post_processor.h"
>  
> +class CameraBuffer;
>  class CameraDevice;
>  class CameraMetadata;
>  
> @@ -137,6 +140,29 @@ public:
>  	};
>  
>  private:
> +	class PostProcessorWorker : public libcamera::Object
> +	{
> +	public:
> +		PostProcessorWorker(PostProcessor *postProcessor)
> +		{
> +			postProcessor_ = postProcessor;
> +		}
> +
> +		void process(const libcamera::FrameBuffer *source,
> +			     CameraBuffer *destination,
> +			     const CameraMetadata &requestMetadata,
> +			     CameraMetadata *resultMetadata,
> +			     const Camera3RequestDescriptor *context)
> +		{
> +			postProcessor_->process(source, destination,
> +						requestMetadata, resultMetadata,
> +						context);
> +		}
> +
> +	private:
> +		PostProcessor *postProcessor_;
> +	};

If it wasn't for the fact that I mentioned in the review of v1 that it
would be best to keep the PostProcessor class not thread-aware, I'd be
tempted to say we could make PostProcessor inherit from Object and drop
this class completely :-) Would you be tempted to do so too ? Compared
to v1 (the RFC) it would resurect patch 1/5, but we wouldn't need the
PostProcessorWorker class at all.

On the other hand, given that I've mentioned in the review of a 7/9 that
mapping the destination buffer could be done in the post-processor
thread, maybe PostProcessorWorker::process() is the right place to do
so. And now that I've written that, I now realize we've split the
construction of CameraBuffer from the actual mapping, which is performed
on the first call to CameraBuffer::plane(), so the mapping is already
done in the post-processor thread. It's thus likely fine to have the
construction of CameraBuffer in CameraStream or CameraDevice.

> +
>  	void postProcessingComplete(PostProcessor::Status status,
>  				    const Camera3RequestDescriptor *context);
>  
> @@ -154,6 +180,8 @@ private:
>  	 */
>  	std::unique_ptr<std::mutex> mutex_;
>  	std::unique_ptr<PostProcessor> postProcessor_;
> +	std::unique_ptr<PostProcessorWorker> ppWorker_;
> +	std::unique_ptr<libcamera::Thread> thread_;
>  };
>  
>  #endif /* __ANDROID_CAMERA_STREAM__ */

-- 
Regards,

Laurent Pinchart