[libcamera-devel] [PATCH v1 2/3] android: camera_device: Transform descriptors_ map to queue
Umang Jain
umang.jain at ideasonboard.com
Tue Sep 28 15:15:44 CEST 2021
Hi Laurent,
On 9/28/21 3:53 AM, Laurent Pinchart wrote:
> Hello,
>
> On Mon, Sep 27, 2021 at 10:38:25PM +0200, Jacopo Mondi wrote:
>> On Mon, Sep 27, 2021 at 04:41:48PM +0530, Umang Jain wrote:
>>> The descriptors_ map hold Camera3RequestDescriptor(s) which are
> s/hold/holds/
>
>>> per-capture request placed by the framework to libcamera HAL.
> s/request/requests/
>
>>> CameraDevice::requestComplete() looks for the descriptor for which the
>>> camera request has been completed and removes it from the map.
>>> Since, the request is placed in form of FIFO and the framework expects
> s/Since, the request is/Since the requests are/
>
>>> the order of completion to be FIFO as well, this calls for a need of
>>> a queue rather than a std::map.
>>>
>>> This patch still keeps the same lifetime of Camera3RequestDescriptor as
>>> before i.e. in the requestComplete(). Previously, a descriptor is
> s/ is/ was/
>
>>> extracted from the map and its lifetime is bound to requestComplete().
> s/ is / was /
>
>>> The lifetime is kept same by manually calling .pop_front() on the
>>> queue. In the subsequent commit, this is likely to change with a
>>> centralized location of dropping descriptors from the queue for request
>>> completion.
>>>
>>> Signed-off-by: Umang Jain <umang.jain at ideasonboard.com>
>>> ---
>>> src/android/camera_device.cpp | 91 ++++++++++++++++++-----------------
>>> src/android/camera_device.h | 5 +-
>>> 2 files changed, 49 insertions(+), 47 deletions(-)
>>>
>>> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
>>> index 8b5dd7a1..b0b7f4fd 100644
>>> --- a/src/android/camera_device.cpp
>>> +++ b/src/android/camera_device.cpp
>>> @@ -955,7 +955,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
>>> @@ -966,12 +968,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";
>> Many times I touched this file I wanted to insert an empty line here
>> but didn't dare not to receive an "is this related?" comment, but if
>> you have more guts than me, please go ahead. I surely won't complain :D
> Neither will I :-)
>
>>> + for (unsigned int i = 0; i < descriptor->buffers_.size(); ++i) {
>> Should we also take the occasion to
>>
>> for (auto &[i, camera3Buffer] : utils::enumerate(descriptor.buffers_)) {
>>
>>> + const camera3_stream_buffer_t &camera3Buffer = descriptor->buffers_[i];
>> and drop this ?
> Fine with me.
>
>>> camera3_stream *camera3Stream = camera3Buffer.stream;
>>> CameraStream *cameraStream = static_cast<CameraStream *>(camera3Stream->priv);
>>>
>>> @@ -1003,12 +1005,12 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
>>> * associate it with the Camera3RequestDescriptor for
>>> * lifetime management only.
>>> */
>>> - descriptor.frameBuffers_.push_back(
>>> + descriptor->frameBuffers_.push_back(
>>> createFrameBuffer(*camera3Buffer.buffer,
>>> cameraStream->configuration().pixelFormat,
>>> cameraStream->configuration().size));
>>>
>>> - buffer = descriptor.frameBuffers_.back().get();
>>> + buffer = descriptor->frameBuffers_.back().get();
>>> LOG(HAL, Debug) << ss.str() << " (direct)";
>>> break;
>>>
>>> @@ -1030,7 +1032,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);
>>> }
>>>
>>> @@ -1038,7 +1040,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;
>>>
>>> @@ -1066,11 +1068,11 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
>>> state_ = State::Running;
>>> }
>>>
>>> - CaptureRequest *request = descriptor.request_.get();
>>> + CaptureRequest *request = descriptor->request_.get();
>>>
>>> {
>>> MutexLocker descriptorsLock(descriptorsMutex_);
>>> - descriptors_[descriptor.request_->cookie()] = std::move(descriptor);
>>> + descriptors_.push_back(std::move(descriptor));
>>> }
>>>
>>> worker_.queueRequest(request);
>>> @@ -1080,31 +1082,27 @@ 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;
>> Access to descriptor_ should be locked.
>>
>> Can't you
>>
>> std::unique_ptr<Camera3RequestDescriptor> descriptor;
>> {
>> MutexLocker descriptorsLock(descriptorsMutex_);
>>
>> if (descriptors_.empty())
>> return;
>>
>> descriptor = std::move(descriptors_.front());
>> descriptors_.pop_front();
>> }
>>
>> So that you have descrptor managed by a unique_ptr here and you can
>> save the pop_front() at the end ?
> It may conflict with patch 3/3. Another option is
>
> Camera3RequestDescriptor *descriptor;
>
> {
> MutexLocker descriptorsLock(descriptorsMutex_);
> ASSERT(!descriptors_.empty());
> descriptor = descriptors_.front().get();
> }
>
> with a pop at the end.
Yes, I am inclined towards popping the descriptor at the end
>
> I've used an ASSERT() here as it's really not supposed to happen, but
> maybe an error message would be better ? In that case, you should call
> notifyError(). I don't think we would be able to recover from this.
Indeed, there was another suggestion on this in async post-processing
series, which I am inclining towards.
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() ?
We are logging with Fatal currently, so it's similar to ASSERT with an
error log printed. I will keep the choice of Fatal over ASSERT(), should
be ok?
>
>>> - return;
>>> - }
>>> + Camera3RequestDescriptor *descriptor = descriptors_.front().get();
>>> + if (descriptor->request_->cookie() != request->cookie()) {
>>> + /*
>>> + * \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();
>>>
>>> - node = descriptors_.extract(it);
>>> + return;
>>> }
>>> - Camera3RequestDescriptor &descriptor = node.mapped();
>>>
>>> camera3_capture_result_t captureResult = {};
>>> - captureResult.frame_number = descriptor.frameNumber_;
>>> - captureResult.num_output_buffers = descriptor.buffers_.size();
>>> - captureResult.output_buffers = descriptor.buffers_.data();
>>> + captureResult.frame_number = descriptor->frameNumber_;
>>> + captureResult.num_output_buffers = descriptor->buffers_.size();
>>> + captureResult.output_buffers = descriptor->buffers_.data();
>>>
>>> /*
>>> * If the Request has failed, abort the request by notifying the error
>>> @@ -1115,11 +1113,11 @@ 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_) {
>>> CameraStream *cameraStream =
>>> static_cast<CameraStream *>(buffer.stream->priv);
>>>
>>> @@ -1142,6 +1140,7 @@ void CameraDevice::requestComplete(Request *request)
>>> }
>>> callbacks_->process_capture_result(callbacks_, &captureResult);
>>>
>>> + descriptors_.pop_front();
>>> return;
>>> }
>>>
>>> @@ -1153,10 +1152,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.
>>> @@ -1166,14 +1165,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 post-processing. */
>>> - for (camera3_stream_buffer_t &buffer : descriptor.buffers_) {
>>> + for (camera3_stream_buffer_t &buffer : descriptor->buffers_) {
>>> CameraStream *cameraStream =
>>> static_cast<CameraStream *>(buffer.stream->priv);
>>>
>>> @@ -1184,13 +1183,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,
>>> - descriptor.settings_,
>>> + descriptor->settings_,
>>> resultMetadata.get());
>>> /*
>>> * Return the FrameBuffer to the CameraStream now that we're
>>> @@ -1201,7 +1200,7 @@ 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);
>>> }
>>> }
>>> @@ -1210,7 +1209,7 @@ void CameraDevice::requestComplete(Request *request)
>>> * Finalize the capture result by setting fences and buffer status
>>> * before executing the callback.
>>> */
>>> - for (camera3_stream_buffer_t &buffer : descriptor.buffers_) {
>>> + for (camera3_stream_buffer_t &buffer : descriptor->buffers_) {
>>> buffer.acquire_fence = -1;
>>> buffer.release_fence = -1;
>>> buffer.status = CAMERA3_BUFFER_STATUS_OK;
>>> @@ -1219,6 +1218,8 @@ void CameraDevice::requestComplete(Request *request)
>>>
>>> captureResult.result = resultMetadata->get();
>>> callbacks_->process_capture_result(callbacks_, &captureResult);
>>> +
> You need
>
> MutexLocker descriptorsLock(descriptorsMutex_);
>
> here.
>
>>> + descriptors_.pop_front();
>>> }
>>>
>>> std::string CameraDevice::logPrefix() const
>>> @@ -1254,10 +1255,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
>> Not sure it's better to make a pointer here, or keep a reference and
>> dereference the argument in the caller... A minor anyway...
> Keeping a reference would be better I think, as the argument can't be
> null.
>
>>> {
>>> - 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 43eb5895..5889a0e7 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>
>>> @@ -105,7 +106,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_;
>>> @@ -125,7 +126,7 @@ private:
>>> std::vector<CameraStream> streams_;
>>>
>>> libcamera::Mutex descriptorsMutex_; /* Protects descriptors_. */
>>> - std::map<uint64_t, Camera3RequestDescriptor> descriptors_;
>>> + std::deque<std::unique_ptr<Camera3RequestDescriptor>> descriptors_;
> You can use std::queue as it's a FIFO.
Ack,
>
>>> std::string maker_;
>>> std::string model_;
More information about the libcamera-devel
mailing list