[libcamera-devel] [PATCH v3 02/10] android: camera_device: Transform descriptors_ map to queue
Hirokazu Honda
hiroh at chromium.org
Mon Sep 27 07:59:25 CEST 2021
Hi Umang, thank you for the patch.
On Wed, Sep 22, 2021 at 5:28 PM Umang Jain <umang.jain at ideasonboard.com> wrote:
>
> Hi Laurent,
>
> Thanks for the review.
>
> On 9/22/21 3:46 AM, Laurent Pinchart wrote:
> > Hi Umang,
> >
> > On Tue, Sep 21, 2021 at 09:56:22AM +0530, Umang Jain wrote:
> >> On 9/21/21 12:36 AM, Laurent Pinchart wrote:
> >>> On Mon, Sep 20, 2021 at 11:07:44PM +0530, Umang Jain wrote:
> >>>> The descriptors_ map hold Camera3RequestDescriptor(s) which are
> >>>> per-capture request placed by the framework to libcamera HAL.
> >>>> CameraDevice::requestComplete() looks for the descriptor for which the
> >>>> camera request has been completed and removes it from the map.
> >>>> However, this approach has its limitations going forwards.
> >>>>
> >>>> In subsequent commits, the post-processing operation which happens
> >>>> in requestComplete() synchronously, is going to be run asynchronously.
> >>>> Therefore, instead of a map for descriptors, a queue makes more sense
> >>>> going forwards and the framework expects capture results to be received
> >>>> in the same order as they were queued. When the async processing is
> >>>> completed, the descriptor queue is inspected to send back the capture
> >>>> results and then de-queued. This helps to maintain the order of sending
> >>>> back the capture results whilst preventing unnecessary complexity of
> >>>> using a map.
> >>>>
> >>>> Signed-off-by: Umang Jain <umang.jain at ideasonboard.com>
> >>>> ---
> >>>> src/android/camera_device.cpp | 89 ++++++++++++++++++-----------------
> >>>> src/android/camera_device.h | 5 +-
> >>>> 2 files changed, 48 insertions(+), 46 deletions(-)
> >>>>
> >>>> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> >>>> index f461e14c..0562c225 100644
> >>>> --- a/src/android/camera_device.cpp
> >>>> +++ b/src/android/camera_device.cpp
> >>>> @@ -926,7 +926,9 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
> >>>> * The descriptor and the associated memory reserved here are freed
> >>>> * at request complete time.
> >>>> */
> >>>> - Camera3RequestDescriptor descriptor(camera_.get(), camera3Request);
> >>>> + std::unique_ptr<Camera3RequestDescriptor> descriptor =
> >>>> + std::make_unique<Camera3RequestDescriptor>(camera_.get(),
> >>>> + camera3Request);
nit: I would use auto here.
> >>>>
> >>>> /*
> >>>> * \todo The Android request model is incremental, settings passed in
> >>>> @@ -937,12 +939,12 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
> >>>> if (camera3Request->settings)
> >>>> lastSettings_ = camera3Request->settings;
> >>>> else
> >>>> - descriptor.settings_ = lastSettings_;
> >>>> + descriptor->settings_ = lastSettings_;
> >>>>
> >>>> - LOG(HAL, Debug) << "Queueing request " << descriptor.request_->cookie()
> >>>> - << " with " << descriptor.buffers_.size() << " streams";
> >>>> - for (unsigned int i = 0; i < descriptor.buffers_.size(); ++i) {
> >>>> - const camera3_stream_buffer_t &camera3Buffer = descriptor.buffers_[i];
> >>>> + LOG(HAL, Debug) << "Queueing request " << descriptor->request_->cookie()
> >>>> + << " with " << descriptor->buffers_.size() << " streams";
> >>>> + for (unsigned int i = 0; i < descriptor->buffers_.size(); ++i) {
> >>>> + const camera3_stream_buffer_t &camera3Buffer = descriptor->buffers_[i];
> >>>> camera3_stream *camera3Stream = camera3Buffer.stream;
> >>>> CameraStream *cameraStream = static_cast<CameraStream *>(camera3Stream->priv);
> >>>>
> >>>> @@ -977,7 +979,7 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
> >>>> buffer = createFrameBuffer(*camera3Buffer.buffer,
> >>>> cameraStream->configuration().pixelFormat,
> >>>> cameraStream->configuration().size);
> >>>> - descriptor.frameBuffers_.emplace_back(buffer);
> >>>> + descriptor->frameBuffers_.emplace_back(buffer);
> >>>> LOG(HAL, Debug) << ss.str() << " (direct)";
> >>>> break;
> >>>>
> >>>> @@ -999,7 +1001,7 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
> >>>> return -ENOMEM;
> >>>> }
> >>>>
> >>>> - descriptor.request_->addBuffer(cameraStream->stream(), buffer,
> >>>> + descriptor->request_->addBuffer(cameraStream->stream(), buffer,
> >>>> camera3Buffer.acquire_fence);
> >>>> }
> >>>>
> >>>> @@ -1007,7 +1009,7 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
> >>>> * Translate controls from Android to libcamera and queue the request
> >>>> * to the CameraWorker thread.
> >>>> */
> >>>> - int ret = processControls(&descriptor);
> >>>> + int ret = processControls(descriptor.get());
> >>>> if (ret)
> >>>> return ret;
> >>>>
> >>>> @@ -1035,11 +1037,11 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
> >>>> state_ = State::Running;
> >>>> }
> >>>>
> >>>> - worker_.queueRequest(descriptor.request_.get());
> >>>> + worker_.queueRequest(descriptor->request_.get());
> >>>>
> >>>> {
> >>>> MutexLocker descriptorsLock(descriptorsMutex_);
> >>>> - descriptors_[descriptor.request_->cookie()] = std::move(descriptor);
> >>>> + descriptors_.push_back(std::move(descriptor));
> >>>> }
> >>> We have a race condition here, worker_.queueRequest() should go after
> >>> adding the request to the queue. Could you fix it in a patch on top ?
> >> Do you mean the race condition is existing already, with the
> >> descriptors_ map (that has been removed from this patch)?
> > Correct, it's already here.
> >
> >> Yes, I can introduce a patch before this one, that fixes the race first
> >> in the map itself. Is my understanding correct?
> > Sounds good to me. It should be a small patch.
> >
> >>>>
> >>>> return 0;
> >>>> @@ -1047,26 +1049,22 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
> >>>>
> >>>> void CameraDevice::requestComplete(Request *request)
> >>>> {
> >>>> - decltype(descriptors_)::node_type node;
> >>>> - {
> >>>> - MutexLocker descriptorsLock(descriptorsMutex_);
> >>>> - auto it = descriptors_.find(request->cookie());
> >>>> - if (it == descriptors_.end()) {
> >>>> - /*
> >>>> - * \todo Clarify if the Camera has to be closed on
> >>>> - * ERROR_DEVICE and possibly demote the Fatal to simple
> >>>> - * Error.
> >>>> - */
> >>>> - notifyError(0, nullptr, CAMERA3_MSG_ERROR_DEVICE);
> >>>> - LOG(HAL, Fatal)
> >>>> - << "Unknown request: " << request->cookie();
> >>>> + if (descriptors_.empty())
> >>>> + return;
> >>>>
> >>>> - return;
> >>>> - }
> >>>> + Camera3RequestDescriptor *descriptor = descriptors_.front().get();
> >>> This needs to be protected by descriptorsMutex_.
> >>>
> >>> Camera3RequestDescriptor *descriptor;
> >>>
> >>> {
> >>> MutexLocker descriptorsLock(descriptorsMutex_);
> >>> descriptor = descriptors_.front().get();
> >>> }
> >>>
> >>>> + if (descriptor->request_->cookie() != request->cookie()) {
> >>> This is correct as long as we handle post-processing synchronously.
> >>> Let's see how it evolves in subsequent patches.
> >> Why not valid for async post-processing?
> >>
> >> So this is requestComplete() function, invoked whenever a
> >> libcamera::Request is completed by libcamera::Camera. The completion is
> >> guaranteed to be done in order, right ? Later in this function, the
> >> post-processing shall happen (sync or async).
> > When we'll move to async post-processing, the request at the front of
> > the queue will be a request undergoing post-processing. libcamera may
> > signal completion of the next request before the post-processing is
> > complete, so the check will fail.
>
>
> Yes, you are right, my bad :S
>
> >
> >>>> + /*
> >>>> + * \todo Clarify if the Camera has to be closed on
> >>>> + * ERROR_DEVICE and possibly demote the Fatal to simple
> >>>> + * Error.
> >>>> + */
> >>>> + notifyError(0, nullptr, CAMERA3_MSG_ERROR_DEVICE);
> >>>> + LOG(HAL, Fatal)
> >>>> + << "Unknown request: " << request->cookie();
> >>> I'd change the message to
> >>>
> >>> << "Out-of-order completion for request "
> >>> << request->cookie();
> >>>
> >>> By the way, with the cookie containing a pointer, I think it would be
> >>> more readable in hex. Maybe a patch on top to use utils::hex() ?
> >> Makes sense, I'll double check
> >>
> >>>>
> >>>> - node = descriptors_.extract(it);
> >>>> + return;
> >>>> }
> >>>> - Camera3RequestDescriptor &descriptor = node.mapped();
> >>>>
> >>>> /*
> >>>> * Prepare the capture result for the Android camera stack.
> >>>> @@ -1075,14 +1073,14 @@ void CameraDevice::requestComplete(Request *request)
> >>>> * post-processing/compression fails.
> >>>> */
> >>>> camera3_capture_result_t captureResult = {};
> >>>> - captureResult.frame_number = descriptor.frameNumber_;
> >>>> - captureResult.num_output_buffers = descriptor.buffers_.size();
> >>>> - for (camera3_stream_buffer_t &buffer : descriptor.buffers_) {
> >>>> + captureResult.frame_number = descriptor->frameNumber_;
> >>>> + captureResult.num_output_buffers = descriptor->buffers_.size();
> >>>> + for (camera3_stream_buffer_t &buffer : descriptor->buffers_) {
> >>>> buffer.acquire_fence = -1;
> >>>> buffer.release_fence = -1;
> >>>> buffer.status = CAMERA3_BUFFER_STATUS_OK;
> >>>> }
> >>>> - captureResult.output_buffers = descriptor.buffers_.data();
> >>>> + captureResult.output_buffers = descriptor->buffers_.data();
> >>>> captureResult.partial_result = 1;
> >>>>
> >>>> /*
> >>>> @@ -1094,14 +1092,15 @@ void CameraDevice::requestComplete(Request *request)
> >>>> << " not successfully completed: "
> >>>> << request->status();
> >>>>
> >>>> - notifyError(descriptor.frameNumber_, nullptr,
> >>>> + notifyError(descriptor->frameNumber_, nullptr,
> >>>> CAMERA3_MSG_ERROR_REQUEST);
> >>>>
> >>>> captureResult.partial_result = 0;
> >>>> - for (camera3_stream_buffer_t &buffer : descriptor.buffers_)
> >>>> + for (camera3_stream_buffer_t &buffer : descriptor->buffers_)
> >>>> buffer.status = CAMERA3_BUFFER_STATUS_ERROR;
> >>>> callbacks_->process_capture_result(callbacks_, &captureResult);
> >>>>
> >>>> + descriptors_.pop_front();
> >>> I'm slightly concerned that in some paths we could complete the request
> >>> but forget to remove it from the queue. Maybe wrapping
> >>> callbacks_->process_capture_result() and descriptors_.pop_front() in a
> >>> function would be good. Let's see how it looks like with the whole
> >>> series applied.
> >> This is a good point and has been address already via
> >> sendCaptureResults() in subsequent patches.
> >>
> >> However, this particular error path you have pointed out here, is the
> >> /only/ part where queue.front() is dropped, other than ofcoourse in
> >> sendCaptureResults(). hmm, I'll take a look if I can figure it out too
> >> and have a singular place of processsing the queue.
> >>
> >>>> return;
> >>>> }
> >>>>
> >>>> @@ -1113,10 +1112,10 @@ void CameraDevice::requestComplete(Request *request)
> >>>> */
> >>>> uint64_t sensorTimestamp = static_cast<uint64_t>(request->metadata()
> >>>> .get(controls::SensorTimestamp));
> >>>> - notifyShutter(descriptor.frameNumber_, sensorTimestamp);
> >>>> + notifyShutter(descriptor->frameNumber_, sensorTimestamp);
> >>>>
> >>>> LOG(HAL, Debug) << "Request " << request->cookie() << " completed with "
> >>>> - << descriptor.buffers_.size() << " streams";
> >>>> + << descriptor->buffers_.size() << " streams";
> >>>>
> >>>> /*
> >>>> * Generate the metadata associated with the captured buffers.
> >>>> @@ -1126,14 +1125,14 @@ void CameraDevice::requestComplete(Request *request)
> >>>> */
> >>>> std::unique_ptr<CameraMetadata> resultMetadata = getResultMetadata(descriptor);
> >>>> if (!resultMetadata) {
> >>>> - notifyError(descriptor.frameNumber_, nullptr, CAMERA3_MSG_ERROR_RESULT);
> >>>> + notifyError(descriptor->frameNumber_, nullptr, CAMERA3_MSG_ERROR_RESULT);
> >>>>
> >>>> /* The camera framework expects an empty metadata pack on error. */
> >>>> resultMetadata = std::make_unique<CameraMetadata>(0, 0);
> >>>> }
> >>>>
> >>>> /* Handle any JPEG compression. */
> >>>> - for (camera3_stream_buffer_t &buffer : descriptor.buffers_) {
> >>>> + for (camera3_stream_buffer_t &buffer : descriptor->buffers_) {
> >>>> CameraStream *cameraStream =
> >>>> static_cast<CameraStream *>(buffer.stream->priv);
> >>>>
> >>>> @@ -1144,13 +1143,13 @@ void CameraDevice::requestComplete(Request *request)
> >>>> if (!src) {
> >>>> LOG(HAL, Error) << "Failed to find a source stream buffer";
> >>>> buffer.status = CAMERA3_BUFFER_STATUS_ERROR;
> >>>> - notifyError(descriptor.frameNumber_, buffer.stream,
> >>>> + notifyError(descriptor->frameNumber_, buffer.stream,
> >>>> CAMERA3_MSG_ERROR_BUFFER);
> >>>> continue;
> >>>> }
> >>>>
> >>>> int ret = cameraStream->process(*src, *buffer.buffer,
> >>>> - descriptor.settings_,
> >>>> + descriptor->settings_,
> >>>> resultMetadata.get());
> >>>> /*
> >>>> * Return the FrameBuffer to the CameraStream now that we're
> >>>> @@ -1161,13 +1160,15 @@ void CameraDevice::requestComplete(Request *request)
> >>>>
> >>>> if (ret) {
> >>>> buffer.status = CAMERA3_BUFFER_STATUS_ERROR;
> >>>> - notifyError(descriptor.frameNumber_, buffer.stream,
> >>>> + notifyError(descriptor->frameNumber_, buffer.stream,
> >>>> CAMERA3_MSG_ERROR_BUFFER);
> >>>> }
> >>>> }
> >>>>
> >>>> captureResult.result = resultMetadata->get();
> >>>> callbacks_->process_capture_result(callbacks_, &captureResult);
> >>>> +
> >>>> + descriptors_.pop_front();
> >>>> }
> >>>>
> >>>> std::string CameraDevice::logPrefix() const
> >>>> @@ -1203,10 +1204,10 @@ void CameraDevice::notifyError(uint32_t frameNumber, camera3_stream_t *stream,
> >>>> * Produce a set of fixed result metadata.
> >>>> */
> >>>> std::unique_ptr<CameraMetadata>
> >>>> -CameraDevice::getResultMetadata(const Camera3RequestDescriptor &descriptor) const
> >>>> +CameraDevice::getResultMetadata(const Camera3RequestDescriptor *descriptor) const
> >>>> {
> >>>> - const ControlList &metadata = descriptor.request_->metadata();
> >>>> - const CameraMetadata &settings = descriptor.settings_;
> >>>> + const ControlList &metadata = descriptor->request_->metadata();
> >>>> + const CameraMetadata &settings = descriptor->settings_;
> >>>> camera_metadata_ro_entry_t entry;
> >>>> bool found;
> >>>>
> >>>> diff --git a/src/android/camera_device.h b/src/android/camera_device.h
> >>>> index a5576927..9c895b25 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>
> >>>> @@ -103,7 +104,7 @@ private:
> >>>> camera3_error_msg_code code);
> >>>> int processControls(Camera3RequestDescriptor *descriptor);
> >>>> std::unique_ptr<CameraMetadata> getResultMetadata(
> >>>> - const Camera3RequestDescriptor &descriptor) const;
> >>>> + const Camera3RequestDescriptor *descriptor) const;
> >>>>
> >>>> unsigned int id_;
> >>>> camera3_device_t camera3Device_;
> >>>> @@ -123,7 +124,7 @@ private:
> >>>> std::vector<CameraStream> streams_;
> >>>>
> >>>> libcamera::Mutex descriptorsMutex_; /* Protects descriptors_. */
> >>>> - std::map<uint64_t, Camera3RequestDescriptor> descriptors_;
> >>>> + std::deque<std::unique_ptr<Camera3RequestDescriptor>> descriptors_;
> >>> Could we use a std::queue instead ? It will be implemented on top of a
> >>> std::deque, so there will be no change in performances, but it gives us
> >>> the semantics we need (essentially, a FIFO).
> >> Yes, great! The deque is coming from earlier version where we need to
> >> iterate over the queue.
> >>
> >> I see, no place as such in v3, where the queue is iterated upon, so we
> >> can surely use std::queue.
> >>
> >>> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
All the problems I noticed have been pointed out by Laurent and Jacopo.
I look forward to the next version.
-Hiro
> >>>
> >>>> std::string maker_;
> >>>> std::string model_;
More information about the libcamera-devel
mailing list