[libcamera-devel] [PATCH v2 2/3] android: camera_device: Transform descriptors_ map to queue
Umang Jain
umang.jain at ideasonboard.com
Wed Sep 29 07:45:46 CEST 2021
Hi,
On 9/29/21 11:10 AM, Umang Jain wrote:
> Hi Hiro,
>
> On 9/29/21 4:59 AM, Hirokazu Honda wrote:
>> Hi Umang, thank you for the patch.
>>
>> On Wed, Sep 29, 2021 at 6:07 AM Laurent Pinchart
>> <laurent.pinchart at ideasonboard.com> wrote:
>>> Hi Umang,
>>>
>>> Thank you for the patch.
>>>
>>> On Tue, Sep 28, 2021 at 09:55:35PM +0530, Umang Jain wrote:
>>>> The descriptors_ map holds Camera3RequestDescriptor(s) which are
>>>> per-capture requests 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.
>>>> Since the requests are placed in form of FIFO and the framework
>>>> expects
>>>> 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 was
>>>> extracted from the map and its lifetime was bound to
>>>> requestComplete().
>>>> 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 | 92
>>>> +++++++++++++++++++----------------
>>>> src/android/camera_device.h | 3 +-
>>>> 2 files changed, 52 insertions(+), 43 deletions(-)
>>>>
>>>> diff --git a/src/android/camera_device.cpp
>>>> b/src/android/camera_device.cpp
>>>> index 9287ea07..a3b8a549 100644
>>>> --- a/src/android/camera_device.cpp
>>>> +++ b/src/android/camera_device.cpp
>>>> @@ -457,7 +457,9 @@ void CameraDevice::stop()
>>>> worker_.stop();
>>>> camera_->stop();
>>>>
>>>> - descriptors_.clear();
>>>> + while (!descriptors_.empty())
>>>> + descriptors_.pop();
>>> You can simplify this with
>>>
>>> descriptors_ = {};
>>>
>>>> +
>>>> state_ = State::Stopped;
>>>> }
>>>>
>>>> @@ -955,7 +957,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 in the type declaration in using
>> std::make_unique because the type is obvious from the right formula.
>>
>>>> /*
>>>> * \todo The Android request model is incremental, settings
>>>> passed in
>>>> @@ -966,12 +970,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";
>>>>
>>>> - 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];
>>>> + for (const auto &[i, camera3Buffer] :
>>>> utils::enumerate(descriptor->buffers_)) {
>>>> camera3_stream *camera3Stream = camera3Buffer.stream;
>>>> CameraStream *cameraStream =
>>>> static_cast<CameraStream *>(camera3Stream->priv);
>>>>
>>>> @@ -1007,12 +1011,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();
>>>> acquireFence = camera3Buffer.acquire_fence;
>>>> LOG(HAL, Debug) << ss.str() << " (direct)";
>>>> break;
>>>> @@ -1035,7 +1039,7 @@ int
>>>> CameraDevice::processCaptureRequest(camera3_capture_request_t
>>>> *camera3Reques
>>>> return -ENOMEM;
>>>> }
>>>>
>>>> - descriptor.request_->addBuffer(cameraStream->stream(), buffer,
>>>> + descriptor->request_->addBuffer(cameraStream->stream(), buffer,
>>>> acquireFence);
>>>> }
>>>>
>>>> @@ -1043,7 +1047,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;
>>>>
>>>> @@ -1071,11 +1075,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(std::move(descriptor));
>>>> }
>>>>
>>>> worker_.queueRequest(request);
>>>> @@ -1085,26 +1089,26 @@ int
>>>> CameraDevice::processCaptureRequest(camera3_capture_request_t
>>>> *camera3Reques
>>>>
>>>> void CameraDevice::requestComplete(Request *request)
>>>> {
>>>> - decltype(descriptors_)::node_type node;
>>>> + Camera3RequestDescriptor *descriptor;
>>>> {
>>>> 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();
>>>> + ASSERT(!descriptors_.empty());
>>>> + descriptor = descriptors_.front().get();
>> Why isn't descriptor popped here?
>> Is there any case that a descriptor should be kept?
>> The original code removes the descriptor from descriptors_.
>> This changes the logic.
>
>
> A bit yes, but it does respect the lifetime of the descriptor as
> before (as the std::map)
>
> Provided that this is a transient patch that works on descriptors_, I
> prefer to be a little flexible here with logic. The popping of
> descriptors_ has been moved to a centralized location in 3/3
>
>> I also think removing here is safe because although a pointer of node
>> in std::queue() is not invalidated by push operation, but indeed can
>> be invalidated by pop operation.
>> So removing here (while acquiring a lock) avoids the problem of using
>> an invalid pointer in the case descriptors_ are cleared in parallel.
>
>
> Yes there might be cases in the future where this can happen, I can
> think of some stop() or flush() clearing the descriptors_, but we are
> going to address them step by step.
>
> The clearing of descriptors_ is something that shouldn't be happening
> by design, so we need to handle it as case-by-case basis (which is one
> the goals we intend to pursue as part of this HAL rework), in
> near-future patch series.
s/ shouldn't be happening / shouldn't be happening in parallel /
>
>
>>
>> -Hiro
>>>> + }
>>>>
>>>> - return;
>>>> - }
>>>> + 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)
>>>> + << "Out-of-order completion for request: "
>>> s/://
>>>
>>> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
>>>
>>>> + << utils::hex(request->cookie());
>>>>
>>>> - node = descriptors_.extract(it);
>>>> + return;
>>>> }
>>>> - Camera3RequestDescriptor &descriptor = node.mapped();
>>>>
>>>> /*
>>>> * Prepare the capture result for the Android camera stack.
>>>> @@ -1113,9 +1117,9 @@ 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_) {
>>>> CameraStream *cameraStream =
>>>> static_cast<CameraStream
>>>> *>(buffer.stream->priv);
>>>>
>>>> @@ -1135,7 +1139,7 @@ void CameraDevice::requestComplete(Request
>>>> *request)
>>>> 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;
>>>>
>>>> /*
>>>> @@ -1147,11 +1151,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_) {
>>>> /*
>>>> * Signal to the framework it has to handle
>>>> fences that
>>>> * have not been waited on by setting the
>>>> release fence
>>>> @@ -1164,6 +1168,7 @@ void CameraDevice::requestComplete(Request
>>>> *request)
>>>>
>>>> callbacks_->process_capture_result(callbacks_, &captureResult);
>>>>
>> A lock is not acquired here?
>> MutexLocker descriptorsLock(descriptorsMutex_);
>
>
> ops, yeah, I'll fix it!
>
>>
>>>> + descriptors_.pop();
>>>> return;
>>>> }
>>>>
>>>> @@ -1175,10 +1180,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.
>>>> @@ -1186,16 +1191,16 @@ void CameraDevice::requestComplete(Request
>>>> *request)
>>>> * Notify if the metadata generation has failed, but
>>>> continue processing
>>>> * buffers and return an empty metadata pack.
>>>> */
>>>> - std::unique_ptr<CameraMetadata> resultMetadata =
>>>> getResultMetadata(descriptor);
>>>> + 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);
>>>>
>>>> @@ -1206,13 +1211,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
>>>> @@ -1223,13 +1228,16 @@ 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);
>>>> +
>>>> + MutexLocker descriptorsLock(descriptorsMutex_);
>>>> + descriptors_.pop();
>>>> }
>>>>
>>>> std::string CameraDevice::logPrefix() const
>>>> diff --git a/src/android/camera_device.h b/src/android/camera_device.h
>>>> index 43eb5895..9ec510d5 100644
>>>> --- a/src/android/camera_device.h
>>>> +++ b/src/android/camera_device.h
>>>> @@ -10,6 +10,7 @@
>>>> #include <map>
>>>> #include <memory>
>>>> #include <mutex>
>>>> +#include <queue>
>>>> #include <vector>
>>>>
>>>> #include <hardware/camera3.h>
>>>> @@ -125,7 +126,7 @@ private:
>>>> std::vector<CameraStream> streams_;
>>>>
>>>> libcamera::Mutex descriptorsMutex_; /* Protects
>>>> descriptors_. */
>>>> - std::map<uint64_t, Camera3RequestDescriptor> descriptors_;
>>>> + std::queue<std::unique_ptr<Camera3RequestDescriptor>> descriptors_;
>>>>
>>>> std::string maker_;
>>>> std::string model_;
>>> --
>>> Regards,
>>>
>>> Laurent Pinchart
More information about the libcamera-devel
mailing list