[libcamera-devel] [PATCH v3 10/10] android: camera_stream: Run post processor in a thread
Umang Jain
umang.jain at ideasonboard.com
Mon Oct 11 11:45:52 CEST 2021
Hi Laurent,
On 9/22/21 5:28 AM, Laurent Pinchart wrote:
> Hi Umang,
>
> Thank you for the patch.
>
> On Mon, Sep 20, 2021 at 11:07:52PM +0530, Umang Jain wrote:
>> This commit makes the post processor run in a separate thread.
>> To enable the move to a separate thread, he post processor
> s/he post/the post/
>
>> needs to be inherited from libcamera::Object and use the
> s/to be inherited/to inherit/
>
>> Object::moveToThread() API to execute the move.
>>
>> A user defined destructor is introduced to stop the thread for cleanup.
> s/user defined/user-defined/
>
>> Since CameraStream is move-constructible and compiler implicitly
>> generates its constructor, defining a destructor will prevent the
>> compiler from adding an implicitly-declared move constructor. Therefore,
>> we need to explicitly add a move constructor as well.
>>
>> Also, the destination buffer for post-processing needs to be alive and
>> stored as a part of overall context, hence a CameraBuffer unique-pointer
>> member is introduced in the Camera3RequestDescriptor struct.
>>
>> ---
>> /* \todo Use ConnectionTypeQueued instead of ConnectionTypeBlocking */
> Indeed, that defeates the point of a thread a little bit :-) What's
> blocking the switch to the queued connection type ?
>
>> ---
>>
>> Signed-off-by: Umang Jain <umang.jain at ideasonboard.com>
>> ---
>> src/android/camera_device.h | 2 ++
>> src/android/camera_stream.cpp | 29 +++++++++++++++++++++--------
>> src/android/camera_stream.h | 5 +++++
>> src/android/post_processor.h | 3 ++-
>> 4 files changed, 30 insertions(+), 9 deletions(-)
>>
>> diff --git a/src/android/camera_device.h b/src/android/camera_device.h
>> index 0bd570a1..e3f3fe7c 100644
>> --- a/src/android/camera_device.h
>> +++ b/src/android/camera_device.h
>> @@ -27,6 +27,7 @@
>> #include <libcamera/request.h>
>> #include <libcamera/stream.h>
>>
>> +#include "camera_buffer.h"
>> #include "camera_capabilities.h"
>> #include "camera_metadata.h"
>> #include "camera_stream.h"
>> @@ -55,6 +56,7 @@ struct Camera3RequestDescriptor {
>> CameraMetadata settings_;
>> std::unique_ptr<CaptureRequest> request_;
>> std::unique_ptr<CameraMetadata> resultMetadata_;
>> + std::unique_ptr<CameraBuffer> destBuffer_;
>>
>> camera3_capture_result_t captureResult_ = {};
>> libcamera::FrameBuffer *internalBuffer_;
>> diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp
>> index c18c7041..6578ce09 100644
>> --- a/src/android/camera_stream.cpp
>> +++ b/src/android/camera_stream.cpp
>> @@ -55,6 +55,9 @@ CameraStream::CameraStream(CameraDevice *const cameraDevice,
>> * is what we instantiate here.
>> */
>> postProcessor_ = std::make_unique<PostProcessorJpeg>(cameraDevice_);
>> + thread_ = std::make_unique<libcamera::Thread>();
> I don't think you need to allocate the thread dynamically, you can have
> a
>
> libcamera::Thread thread_;
>
> in CameraStream.
>
>> + postProcessor_->moveToThread(thread_.get());
>> + thread_->start();
>> }
>>
>> if (type == Type::Internal) {
>> @@ -63,6 +66,14 @@ CameraStream::CameraStream(CameraDevice *const cameraDevice,
>> }
>> }
> Please add
>
> CameraStream::CameraStream(CameraStream &&other) = default;
>
> here and drop the ' = default' in the header file, we don't need to
> inline the move-constructor.
>
>> +CameraStream::~CameraStream()
>> +{
>> + if (thread_) {
>> + thread_->exit();
>> + thread_->wait();
>> + }
> This makes me realize that we're missing support for being able to flush
> the requests queued to the post-processor. That will require some extra
> work, and I'm tempted to say we should get this series merged with
> ConnectionTypeBlocking first as an intermediate step. I'm fairly
> confident this is going in the right direction and that implementing
> flush support won't require a complete redesign, but there's still this
> little voice that tells me the risk is not zero. Feel free to agree or
> disagree :-)
>
> One thing that will likely need to be reworked more extensively is this
> patch though. The most naive implementation would be to add start() and
> stop() functions to CameraStream. start() will start the thread, stop()
> will stop it (calling exit and wait). You'll have to call create a
> custom class inheriting from Thread (really sorry for going back and
> forth on this topic :-S) to reimplement run() and call
>
> dispatchMessages(Message::Type::InvokeMessage);
>
> after calling exec(). Otherwise you'll have queued process() calls that
> will be left dangling. Performance won't be great, as *all* queued
> requests will be processed, even the ones whose processing hasn't
> started yet when stop() is called.
>
> To make it better, we should complete the ongoing processing, and then
> complete the other requests that have been queued but haven't been
> processed yet. This will require keeping a queue of pending requests in
> the camera stream. Requests will be added to the queue before they get
> passed to the post-processor, and removed when the post-processor
> signals completion. You'll be able to remove the dispatchMessages() call
> (and won't have to reimplement Thread::run(), so no need to subclass it
> after all \o/), and after the thread stops (after wait() returns),
> you'll have to go through the queue and signal completion of all
> requests still stored there. This shouldn't be too complex, I'd aim for
> this option directly as you won't need to subclass Thread.
>
> Ideally, to avoid delays when starting the camera and when flushing it,
> we should keep the thread running at all times (when no work will be
> queued to it, it will not consume any CPU time). There's no way to
> instruct a running thread to finish the ongoing processing and drop the
> pending invokeMethod() calls, so you'll need to subclass Thread (did I
> mention going back and forth ? :-)) and reimplement run(). This time you
> won't be able to rely on exec() and invokeMethod(), so you'll have to
> implement a queue of request in the thread subclass, a custom loop in
> run() that will process the requests from the queue, and add a condition
> variable to signal to the run() function that a new request has been
> added to the queue, or that a flush() has been requested. That way
> you'll be able to get rid of start() and stop(), and only add flush().
I went ahead with using the condition variable along with maintaining a
queue in v4 as you stated out the design here (in the latter parts). All
the Object inheritence and ::invokeMethod() call have be dropped.
A queue in the worker class helps to purge the requests on-demand. As
per my understand around flush(), I have designed a solution in Patch
7/7 v4. To test it, I would have used CTS, but it is not playing well as
of now (I am looking into it), but a assert:
@@ -479,6 +479,7 @@ void CameraDevice::stop()
stopCamera();
+ ASSERT (descriptors_.empty());
descriptors_ = {};
tells me all the descriptors are getting cleared as expected on camera
stop. Complying with CTS will be a double confirmation on this, hopefully.
>
>> +}
>> +
>> const StreamConfiguration &CameraStream::configuration() const
>> {
>> return config_->at(index_);
>> @@ -111,21 +122,23 @@ void CameraStream::process(const FrameBuffer *source,
>> return;
>> }
>>
>> - /*
>> - * \todo Buffer mapping and processing should be moved to a
>> - * separate thread.
>> - */
>> const StreamConfiguration &output = configuration();
>> - CameraBuffer dest(camera3Dest, formats::MJPEG, output.size,
>> - PROT_READ | PROT_WRITE);
>> - if (!dest.isValid()) {
>> + std::unique_ptr<CameraBuffer> dest =
>> + std::make_unique<CameraBuffer>(camera3Dest, formats::MJPEG,
>> + output.size, PROT_READ | PROT_WRITE);
>> +
>> + if (!dest->isValid()) {
>> LOG(HAL, Error) << "Failed to map android blob buffer";
>> request->status_ = Camera3RequestDescriptor::ProcessStatus::Error;
>> handleProcessComplete(request);
>> return;
>> }
> Blank line.
>
>> + request->destBuffer_ = std::move(dest);
>>
>> - postProcessor_->process(source, &dest, request);
>> + /* \todo Use ConnectionTypeQueued instead of ConnectionTypeBlocking */
>> + postProcessor_->invokeMethod(&PostProcessor::process,
>> + ConnectionTypeBlocking, source,
>> + request->destBuffer_.get(), request);
>> }
>>
>> void CameraStream::handleProcessComplete(Camera3RequestDescriptor *request)
>> diff --git a/src/android/camera_stream.h b/src/android/camera_stream.h
>> index d4ec5c25..42db72d8 100644
>> --- a/src/android/camera_stream.h
>> +++ b/src/android/camera_stream.h
>> @@ -13,6 +13,8 @@
>>
>> #include <hardware/camera3.h>
>>
>> +#include <libcamera/base/thread.h>
>> +
>> #include <libcamera/camera.h>
>> #include <libcamera/framebuffer.h>
>> #include <libcamera/framebuffer_allocator.h>
>> @@ -113,6 +115,8 @@ public:
>> CameraStream(CameraDevice *const cameraDevice,
>> libcamera::CameraConfiguration *config, Type type,
>> camera3_stream_t *camera3Stream, unsigned int index);
>> + CameraStream(CameraStream &&other) = default;
>> + ~CameraStream();
>>
>> Type type() const { return type_; }
>> const camera3_stream_t &camera3Stream() const { return *camera3Stream_; }
>> @@ -143,6 +147,7 @@ private:
>> */
>> std::unique_ptr<std::mutex> mutex_;
>> std::unique_ptr<PostProcessor> postProcessor_;
>> + std::unique_ptr<libcamera::Thread> thread_;
>> };
>>
>> #endif /* __ANDROID_CAMERA_STREAM__ */
>> diff --git a/src/android/post_processor.h b/src/android/post_processor.h
>> index 48ddd8ac..fdfd52d3 100644
>> --- a/src/android/post_processor.h
>> +++ b/src/android/post_processor.h
>> @@ -7,6 +7,7 @@
>> #ifndef __ANDROID_POST_PROCESSOR_H__
>> #define __ANDROID_POST_PROCESSOR_H__
>>
>> +#include <libcamera/base/object.h>
>> #include <libcamera/base/signal.h>
>>
>> #include <libcamera/framebuffer.h>
>> @@ -18,7 +19,7 @@ class CameraMetadata;
>>
>> struct Camera3RequestDescriptor;
>>
>> -class PostProcessor
>> +class PostProcessor : public libcamera::Object
>> {
>> public:
>> virtual ~PostProcessor() = default;
More information about the libcamera-devel
mailing list