[libcamera-devel] [PATCH v1 2/3] android: camera_device: Transform descriptors_ map to queue
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Tue Sep 28 00:23:56 CEST 2021
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.
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.
> > - 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.
> >
> > std::string maker_;
> > std::string model_;
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list