| Message ID | 20210910070638.467294-9-umang.jain@ideasonboard.com |
|---|---|
| State | Superseded |
| Delegated to: | Umang Jain |
| Headers | show |
| Series |
|
| Related | show |
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@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__ */
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@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__ */
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@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__ */
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. + */ + 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_. */ 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_; + }; + 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__ */
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@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(-)