[libcamera-devel] [PATCH v2 8/9] android: camera_stream: Run post-processor in a separate thread
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Wed Sep 15 03:35:04 CEST 2021
Hi Umang,
On Mon, Sep 13, 2021 at 12:39:11PM +0530, Umang Jain wrote:
> On 9/13/21 6:51 AM, Laurent Pinchart wrote:
> > 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.
I prefer reviewing the locking scheme with the data classes as it's
easier to ensure the locking is right. If the lock was moved to the
previous patch I could check the introduced new usages of the data along
with the lock, while with locking in this patch I need to look at both
patches together.
This being said, if moving the lock to the previous patch causes issues,
it's not a very strong rule.
> >> 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__ */
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list