[libcamera-devel] [PATCH v2 6/9] android: camera_device: Add a queue for sending capture results
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Fri Sep 17 13:55:13 CEST 2021
Hi Umang,
On Fri, Sep 17, 2021 at 02:23:28PM +0530, Umang Jain wrote:
> On 9/15/21 6:58 AM, Laurent Pinchart wrote:
> > On Mon, Sep 13, 2021 at 12:31:46PM +0530, Umang Jain wrote:
> >> On 9/13/21 6:29 AM, Laurent Pinchart wrote:
> >>> On Fri, Sep 10, 2021 at 12:36:35PM +0530, Umang Jain wrote:
> >>>> When a camera capture request completes, the next step is to send the
> >>>> capture results to the framework via process_capture_results(). All
> >>>> the capture associated result metadata is prepared and populated. If
> >>>> any post-processing is required, it will also take place in the same
> >>>> thread (which can be blocking for a subtaintial amount of time) before
> >>>> the results can be sent back to the framework.
> >>>>
> >>>> Subsequent patches will move the post-processing to run in a separate
> >>>> thread. In order to do so, there is few bits of groundwork which this
> >>>> patch entails. Mainly, we need to preserve the order of sending
> >>>> the capture results back in which they were queued by the framework
> >>>> to the HAL (i.e. the order is sequential). Hence, we need to add a queue
> >>>> in which capture results can be queued with context.
> >>>>
> >>>> As per this patch, the post-processor still runs synchronously as
> >>>> before, but it will queue up the current capture results with context.
> >>>> Later down the line, the capture results are dequeud in order and
> >>>> sent back to the framework via sendQueuedCaptureResults(). If the queue
> >>>> is empty or unused, the current behaviour is to skip the queue and
> >>>> send capture results directly.
> >>>>
> >>>> The context is preserved using Camera3RequestDescriptor utility
> >>>> structure. It has been expanded accordingly to address the needs of
> >>>> the functionality.
> >>>> ---
> >>>> Check if we can drop unique_ptr to camera3descriptor
> >>>> ---
> >>>>
> >>>> Signed-off-by: Umang Jain <umang.jain at ideasonboard.com>
> >>>> ---
> >>>> src/android/camera_device.cpp | 106 ++++++++++++++++++++++++++++++----
> >>>> src/android/camera_device.h | 22 +++++++
> >>>> src/android/camera_stream.cpp | 2 +
> >>>> 3 files changed, 119 insertions(+), 11 deletions(-)
> >>>>
> >>>> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> >>>> index 84549d04..7f04d225 100644
> >>>> --- a/src/android/camera_device.cpp
> >>>> +++ b/src/android/camera_device.cpp
> >>>> @@ -240,6 +240,8 @@ Camera3RequestDescriptor::Camera3RequestDescriptor(
> >>>> /* Clone the controls associated with the camera3 request. */
> >>>> settings_ = CameraMetadata(camera3Request->settings);
> >>>>
> >>>> + internalBuffer_ = nullptr;
> >>>> +
> >>>> /*
> >>>> * Create the CaptureRequest, stored as a unique_ptr<> to tie its
> >>>> * lifetime to the descriptor.
> >>>> @@ -989,6 +991,7 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
> >>>> * once it has been processed.
> >>>> */
> >>>> buffer = cameraStream->getBuffer();
> >>>> + descriptor.internalBuffer_ = buffer;
> >>>> LOG(HAL, Debug) << ss.str() << " (internal)";
> >>>> break;
> >>>> }
> >>>> @@ -1148,26 +1151,107 @@ void CameraDevice::requestComplete(Request *request)
> >>>> continue;
> >>>> }
> >>>>
> >>>> +
> >>>> + /*
> >>>> + * Save the current context of capture result and queue the
> >>>> + * descriptor before processing the camera stream.
> >>>> + *
> >>>> + * When the processing completes, the descriptor will be
> >>>> + * dequeued and capture results will be sent to the framework.
> >>>> + */
> >>>> + descriptor.status_ = Camera3RequestDescriptor::Pending;
> >>>
> >>> The status should also be initialized to Pending in the
> >>> Camera3RequestDescriptor constructor.
> >>>
> >>>> + descriptor.resultMetadata_ = std::move(resultMetadata);
> >>>
> >>> Let's move the resultMetadata to the descriptor right when creating it,
> >>> it will be cleaner.
> >>>
> >>>> + descriptor.captureResult_ = captureResult;
> >>>
> >>> This causes a copy of a fairly large structure. You can prevent this by
> >>> replacing
> >>>
> >>> camera3_capture_result_t captureResult = {};
> >>>
> >>> above with
> >>>
> >>> camera3_capture_result_t &captureResult = descriptor.captureResult_;
> >>> captureResult = {};
> >>>
> >>> or better, in the definition of Camera3RequestDescriptor, replace
> >>>
> >>> camera3_capture_result_t captureResult_;
> >>>
> >>> with
> >>>
> >>> camera3_capture_result_t captureResult_ = {};
> >>>
> >>> and drop the initialization in this function. This will ensure that the
> >>> capture results are correctly initialized as soon as
> >>> Camera3RequestDescriptor is constructed, avoiding possibly
> >>> use-before-init issues.
> >>>
> >>>> +
> >>>> + std::unique_ptr<Camera3RequestDescriptor> reqDescriptor =
> >>>> + std::make_unique<Camera3RequestDescriptor>();
> >>>> + *reqDescriptor = std::move(descriptor);
> >>>
> >>> This will copy the whole class as there's no move constructor for
> >>> Camera3RequestDescriptor. I don't think that's what you want. If the
> >>> descriptor needs to be moved out of the map to the queue, without a
> >>> copy, then they should be stored in the map as unique_ptr already. This
> >>> should be done as a separate patch before this one.
> >>
> >> So, you mean descriptors in the map should be unique_ptr beforehand?
> >> That will avoid the copy on std::move?
> >
> > The only thing that std::move() does is to return an rvalue reference to
> > the parameter passed to it. That's all. What it implies is that the
> > assignment will then pick the version of operator=() that takes an
> > rvalue reference (called the move assignment operator). It's really just
> > a way to pick one implementation of operator=() over the other.
> >
> > For classes such as std::unique_ptr<>, the move assignment operator will
> > transfer ownership of the pointer. For classes such as std::vector<>, it
> > will transfer ownership of the internal data, so it's cheap. For
> > Camera3RequestDescriptor, we don't define any move assignement operator,
> > so all members will be moved one by one, and for most members, that will
> > just be a copy.
> >
> > Bottom line, moving a std::unique_ptr<> makes sense, moving a
> > Camera3RequestDescriptor less so.
>
> I agree with this particular pointer, that storing
> Camera3RequestDescriptor can be made via std::unique<> to essentially
> leverage std::move.
>
> But I am not too sure of your strategy below.
>
> > After reviewing the whole series, and thinking some more about it, I
> > think we should introduce a
> > std::queue<std::unique_ptr<Camera3RequestDescriptor>> that will own the
> > Camera3RequestDescriptor instances, and a std::map<uint64_t,
> > Camera3RequestDescriptor *> to lookup the Camera3RequestDescriptor in
> > CameraDevice::requestComplete(). The queue will have ownership of the
> > request descriptor, the map will store pointers to the instance stored
> > in the queue. You'll need to
> >
> > - add an entry to the queue in CameraDevice::processCaptureRequest(),
> > and add it to the map there too, right before queuing the request to
> > the worker
> >
> > - look up the entry from the map in CameraDevice::requestComplete(), and
> > remove it from the map there
> >
> > - handle completion in order using the queue, with a function like
> > CameraDevice::sendQueuedCaptureResults()
> >
> > It would be best, I think, if this rework could be done early in the
> > series to make it easier to review, and then add the async
> > post-processor. I haven't looked at it in details though, so maybe
> > that's not practical.
>
> This particular bit is getting complicated here. I understand you want
> that the ownership is held by a queue but all look-ups happen via a map
> from the start, but there are error paths that will clean up the
> descriptors be removing it from the map and free the buffers held by it
> right away. If the ownership is held with the queue, we need to clean it
> up from the queue, to actually free the buffers right? .... And queue
> might need to get re-adjusted with remaining elements which might not be
> that great. It's adding up complexity (at least from error-ed requests
> point of view), that when iterating over the queue, a descriptor is
> present in the queue, but has been removed from the map, which the queue
> is supposed to do with it?
>
> I am still pondering over it (and also looking at "android: Fix
> descriptors_ clean up" series), trying to answer my questions but it's
> making me a bit scared.
My rationale is that requests are queued by Android and are expected by
Android to complete in the same order. Camera3RequestDescriptor is the
counterpart of camera3_capture_request_t, I think it should have the
same lifetime as camera3_capture_request_t, and follow the same rules. A
queue thus seems to be the best storage.
The map is there only to look up the Camera3RequestDescriptor when the
libcamera Request completes. This would normally be done using
Request::cookie(), but the request cookie is handled by the
CameraWorker. Looking at it, CameraWorker sets the cookie to a pointer
to CameraRequest, but nobody actually depends on that. I think you could
pass a uint64_t cookie to the CameraRequest constructor, and pass
reinterpret_cast<uint64_t>(this) to it from the Camera3RequestDescriptor
constructor. We should then be able to get rid of the map.
> >>> I wonder if we shouldn't keep the descriptor in the map until we finish
> >>> post-processing, as that would help implementing partial completion
> >>> support in the future (where we would connect the bufferComplete signal
> >>> and start post-processing of buffers before libcamera completes the
> >>> whole request). Maybe that will be best handled in the future, not now,
> >>> so I think I'm fine moving descriptors from the map to the queue for
> >>> now.
> >>
> >> Ack.
> >>
> >>>> + queuedDescriptor_.push_back(std::move(reqDescriptor));
> >>>> +
> >>>> + Camera3RequestDescriptor *currentDescriptor = queuedDescriptor_.back().get();
> >>>> int ret = cameraStream->process(src, *buffer.buffer,
> >>>
> >>> ret is unused, this causes a compilation failure. Could you please
> >>> compile-test individual patches in the series ?
> >>>
> >>> Now that camera stream processing completion is signaled, the process()
> >>> function should become void. This can go in a separate patch.
> >>>
> >>>> - descriptor.settings_,
> >>>> - resultMetadata.get(),
> >>>> - &descriptor);
> >>>> + currentDescriptor->settings_,
> >>>> + currentDescriptor->resultMetadata_.get(),
> >>>
> >>> You can drop the resultMetadata parameter from the process() function
> >>> and access it through the descriptor instead.
> >>
> >> As I mentioned in the previous replies, I think fields that are modified
> >> as part of process(), should be passed explitcity. The descriptor I pass
> >> down the line, is a const.
> >>
> >>>> + currentDescriptor);
> >>>> + return;
> >>>> + }
> >>>> +
> >>>> + if (queuedDescriptor_.empty()) {
> >>>> + captureResult.result = resultMetadata->get();
> >>>> + callbacks_->process_capture_result(callbacks_, &captureResult);
> >>>> + } else {
> >>>> + /*
> >>>> + * Previous capture results waiting to be sent to framework
> >>>> + * hence, queue the current capture results as well. After that,
> >>>> + * check if any results are ready to be sent back to the
> >>>> + * framework.
> >>>> + */
> >>>> + descriptor.status_ = Camera3RequestDescriptor::Succeeded;
> >>>> + descriptor.resultMetadata_ = std::move(resultMetadata);
> >>>> + descriptor.captureResult_ = captureResult;
> >>>> + std::unique_ptr<Camera3RequestDescriptor> reqDescriptor =
> >>>> + std::make_unique<Camera3RequestDescriptor>();
> >>>> + *reqDescriptor = std::move(descriptor);
> >>>> + queuedDescriptor_.push_back(std::move(reqDescriptor));
> >>>> +
> >>>> + sendQueuedCaptureResults();
> >>>> + }
> >>>> +}
> >>>> +
> >>>> +void CameraDevice::streamProcessingComplete(CameraStream *cameraStream,
> >>>> + CameraStream::ProcessStatus status,
> >>>> + const Camera3RequestDescriptor *context)
> >>>> +{
> >>>> + for (auto &d : queuedDescriptor_) {
> >>>
> >>> This function is called from the post-processing completion thread, so
> >>> it will race with CameraDevice::requestComplete(). Both access
> >>> queuedDescriptor_ with a lock, it won't end well.
> >>
> >> There is no thread here in this patch. It's all synchronous as mentioned
> >> in the commit message. The threading only appears 8/9 patch and there we
> >> need to start looking at racing issues.
> >
> > You're right, but could we introduce the required locking in this patch
> > already ? It's easier to review if the new data structures and the
> > locking come together.
> >
> >>>> + if (d.get() != context)
> >>>> + continue;
> >>>
> >>> Iterating over a queue to find the element we need sounds like we're not
> >>> using the right type of container. I think you should keep the
> >>> Camera3RequestDescriptor in the map, and look it up from the map here.
> >>
> >> No, The descriptor is already getting cleaned up in requestComplete().
> >>
> >> Let me explain:
> >>
> >> As per master right now, the descriptor is extracted from the map at the
> >> start of requestComplete. The lifetime of the descriptor is now only in
> >> the requestComplete() function. Once it returns, the descriptor is
> >> destroyed.
> >>
> >> What am I doing through this series is, adding a descriptor to a queue.
> >> This happens because we don't want to destroy it, rather preserve it
> >> until post-processing completes(well, it's going to happen async, so it
> >> makes more sense). But still it should be removed from the map, just
> >> that, not let it get destroyed.
> >>
> >> Now, why a queue? Because we should be able to send capture results as
> >> per ordering. The descriptors are queued up in order.
> >
> > That's a very clear explanation, I agree with all of this.
> >
> > My comment about looking it up from the map indeed doesn't make too much
> > sense. Reading this function again, can't you just access the
> > Camera3RequestDescriptor from the context parameter ? You'll need to
> > const_cast<> to remove the const, but that's fine, we know here that we
> > can do so because we own the Camera3RequestDescriptor.
> >
> >> Not removing the descriptor from the map might have it's own
> >> implications for e.g. you will need to Request around as well, as the
> >> descriptors_ map is looked with with request->cookie(). I think it's
> >> slightly invasive changes, no?
> >
> > Yes, not a good idea.
> >
> >> Plus I think the "android: Fix
> >> descriptors_ clean up" will also conflict to some extend.
> >
> > There could be some conflicts, yes :-S
> >
> > Does my above proposal with a queue owning the request and a map to look
> > it up make sense ? We should ideally even be able to get rid of the map
> > by storing the pointer to the Camera3RequestDescriptor in the Request
> > cookie, but it's currently used to store the CaptureRequest pointer.
> > CaptureRequest should really be dropped as fence handling should go to
> > the libcamera core, but I don't want to include that in this series,
> > it's big enough already. A map is fine as an interim solution.
> >
> >>>> +
> >>>> /*
> >>>> * Return the FrameBuffer to the CameraStream now that we're
> >>>> * done processing it.
> >>>> */
> >>>> if (cameraStream->type() == CameraStream::Type::Internal)
> >>>> - cameraStream->putBuffer(src);
> >>>> -
> >>>> - if (ret) {
> >>>> - buffer.status = CAMERA3_BUFFER_STATUS_ERROR;
> >>>> - notifyError(descriptor.frameNumber_, buffer.stream,
> >>>> - CAMERA3_MSG_ERROR_BUFFER);
> >>>> + cameraStream->putBuffer(d->internalBuffer_);
> >>>> +
> >>>> + if (status == CameraStream::ProcessStatus::Succeeded) {
> >>>> + d->status_ = Camera3RequestDescriptor::Succeeded;
> >>>> + } else {
> >>>> + d->status_ = Camera3RequestDescriptor::Failed;
> >>>> +
> >>>> + d->captureResult_.partial_result = 0;
> >>>> + for (camera3_stream_buffer_t &buffer : d->buffers_) {
> >>>> + CameraStream *cs = static_cast<CameraStream *>(buffer.stream->priv);
> >>>> +
> >>>> + if (cs->camera3Stream().format != HAL_PIXEL_FORMAT_BLOB)
> >>>> + continue;
> >>>> + buffer.status = CAMERA3_BUFFER_STATUS_ERROR;
> >>>> + notifyError(d->frameNumber_, buffer.stream,
> >>>> + CAMERA3_MSG_ERROR_BUFFER);
> >>>> + }
> >>>> }
> >>>> + break;
> >>>> }
> >>>>
> >>>> - captureResult.result = resultMetadata->get();
> >>>> - callbacks_->process_capture_result(callbacks_, &captureResult);
> >>>> + /*
> >>>> + * Send back capture results to the framework by inspecting the queue.
> >>>> + * The framework can defer queueing further requests to the HAL (via
> >>>> + * process_capture_request) unless until it receives the capture results
> >>>> + * for already queued requests.
> >>>> + */
> >>>> + sendQueuedCaptureResults();
> >>>> +}
> >>>> +
> >>>> +void CameraDevice::sendQueuedCaptureResults()
> >>>> +{
> >>>> + while (!queuedDescriptor_.empty()) {
> >>>> + std::unique_ptr<Camera3RequestDescriptor> &d = queuedDescriptor_.front();
> >>>> + if (d->status_ == Camera3RequestDescriptor::Pending)
> >>>> + return;
> >>>> +
> >>>> + d->captureResult_.result = d->resultMetadata_->get();
> >>>> + callbacks_->process_capture_result(callbacks_, &(d->captureResult_));
> >>>> + queuedDescriptor_.pop_front();
> >>>> + }
> >>>> }
> >>>>
> >>>> std::string CameraDevice::logPrefix() const
> >>>> diff --git a/src/android/camera_device.h b/src/android/camera_device.h
> >>>> index b59ac3e7..e7318358 100644
> >>>> --- a/src/android/camera_device.h
> >>>> +++ b/src/android/camera_device.h
> >>>> @@ -7,6 +7,7 @@
> >>>> #ifndef __ANDROID_CAMERA_DEVICE_H__
> >>>> #define __ANDROID_CAMERA_DEVICE_H__
> >>>>
> >>>> +#include <deque>
> >>>> #include <map>
> >>>> #include <memory>
> >>>> #include <mutex>
> >>>> @@ -46,6 +47,20 @@ struct Camera3RequestDescriptor {
> >>>> std::vector<std::unique_ptr<libcamera::FrameBuffer>> frameBuffers_;
> >>>> CameraMetadata settings_;
> >>>> std::unique_ptr<CaptureRequest> request_;
> >>>> +
> >>>> + /*
> >>>> + * Below are utility placeholders used when a capture result
> >>>> + * needs to be queued before completion via process_capture_result()
> >>>> + */
> >>>> + enum completionStatus {
> >>>> + Pending,
> >>>> + Succeeded,
> >>>> + Failed,
> >>>> + };
> >>>> + std::unique_ptr<CameraMetadata> resultMetadata_;
> >>>> + camera3_capture_result_t captureResult_;
> >>>
> >>> This patch is a bit hard to review. Could you split the addition of
> >>> resultMetadata_ and captureResult_ to Camera3RequestDescriptor to a
> >>> separate patch (including addressing the comments above) ?
> >>
> >> Yes, it's hard since it setups with the ground work for post-processor.
> >> It has many pieces linked together giving very little room for splitting up.
> >>
> >>>> + libcamera::FrameBuffer *internalBuffer_;
> >>>> + completionStatus status_;
> >>>> };
> >>>>
> >>>> class CameraDevice : protected libcamera::Loggable
> >>>> @@ -78,6 +93,9 @@ public:
> >>>> int processCaptureRequest(camera3_capture_request_t *request);
> >>>> void requestComplete(libcamera::Request *request);
> >>>>
> >>>> + void streamProcessingComplete(CameraStream *stream,
> >>>> + CameraStream::ProcessStatus status,
> >>>> + const Camera3RequestDescriptor *context);
> >>>> protected:
> >>>> std::string logPrefix() const override;
> >>>>
> >>>> @@ -106,6 +124,8 @@ private:
> >>>> std::unique_ptr<CameraMetadata> getResultMetadata(
> >>>> const Camera3RequestDescriptor &descriptor) const;
> >>>>
> >>>> + void sendQueuedCaptureResults();
> >>>> +
> >>>> unsigned int id_;
> >>>> camera3_device_t camera3Device_;
> >>>>
> >>>> @@ -126,6 +146,8 @@ private:
> >>>> libcamera::Mutex descriptorsMutex_; /* Protects descriptors_. */
> >>>> std::map<uint64_t, Camera3RequestDescriptor> descriptors_;
> >>>>
> >>>> + std::deque<std::unique_ptr<Camera3RequestDescriptor>> queuedDescriptor_;
> >>>> +
> >>>> std::string maker_;
> >>>> std::string model_;
> >>>>
> >>>> diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp
> >>>> index 996779c4..c7d874b2 100644
> >>>> --- a/src/android/camera_stream.cpp
> >>>> +++ b/src/android/camera_stream.cpp
> >>>> @@ -134,6 +134,8 @@ void CameraStream::postProcessingComplete(PostProcessor::Status status,
> >>>> processStatus = ProcessStatus::Succeeded;
> >>>> else
> >>>> processStatus = ProcessStatus::Failed;
> >>>> +
> >>>> + cameraDevice_->streamProcessingComplete(this, processStatus, context);
> >>>> }
> >>>>
> >>>> FrameBuffer *CameraStream::getBuffer()
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list