[libcamera-devel] [PATCH v2 2/3] android: camera_device: Transform descriptors_ map to queue
Hirokazu Honda
hiroh at chromium.org
Wed Sep 29 09:18:38 CEST 2021
Hi Umang, thank you for replying.
On Wed, Sep 29, 2021 at 4:02 PM Laurent Pinchart
<laurent.pinchart at ideasonboard.com> wrote:
>
> Hello,
>
> On Wed, Sep 29, 2021 at 11:10:08AM +0530, Umang Jain wrote:
> > On 9/29/21 4:59 AM, Hirokazu Honda wrote:
> > > On Wed, Sep 29, 2021 at 6:07 AM Laurent Pinchart wrote:
> > >> 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
>
> Let's also note that the upcoming async post-processing series will
> require keeping the descriptor in the queue until the post-processing is
> complete.
>
> > > 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.
> >
Doesn't the issue of clearing in parallel happen at this moment?
> > 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.
> >
> > >>> + }
> > >>>
> > >>> - 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