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

[Umang Jain]

Hi Laurent,

On 9/13/21 6:51 AM, Laurent Pinchart wrote:
> 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.


Yes, I realized by discussion with Jacopo. I think we don't need a copy 
here, working on it as we speak.

>
>> +			 */
>> +			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.


No, I think as this patch is the one which introduces the thread, it 
should introduce the necessary locking too. Correct me if I am wrong.

>
>>   	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


I was tempted that from the start which was RFC v1. I should start to 
trust my gut more....

> to v1 (the RFC) it would resurect patch 1/5, but we wouldn't need the
> PostProcessorWorker class at all.


hmmm....

>
> 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.


Aha, very interesting. So I think this brings one more \todo down in the 
code base

>
>> +
>>   	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__ */