[libcamera-devel] [PATCH v3 02/10] android: camera_device: Transform descriptors_ map to queue
Umang Jain
umang.jain at ideasonboard.com
Wed Sep 22 10:28:19 CEST 2021
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);
>>>>
>>>> /*
>>>> * \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>
>>>
>>>> std::string maker_;
>>>> std::string model_;
More information about the libcamera-devel
mailing list