[libcamera-devel] [PATCH v3 02/10] android: camera_device: Transform descriptors_ map to queue
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Mon Sep 20 21:06:32 CEST 2021
Hi Umang,
Thank you for the patch.
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 ?
>
> 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.
> + /*
> + * \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() ?
>
> - 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.
> 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).
Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
>
> std::string maker_;
> std::string model_;
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list