| Message ID | 20210928162536.227475-3-umang.jain@ideasonboard.com |
|---|---|
| State | Superseded |
| Delegated to: | Umang Jain |
| Headers | show |
| Series |
|
| Related | show |
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@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); > > /* > * \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(); > + } > > - 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@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); > > + 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_;
Hi Umang, thank you for the patch. On Wed, Sep 29, 2021 at 6:07 AM Laurent Pinchart <laurent.pinchart@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@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. 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. -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@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_); > > + 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
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@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@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. > > -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@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
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@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@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@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
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@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. > > 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@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_;
Hi Umang, thank you for replying. On Wed, Sep 29, 2021 at 4:02 PM Laurent Pinchart <laurent.pinchart@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@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@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
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(); + 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); /* * \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(); + } - 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: " + << 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); + 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_;
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@ideasonboard.com> --- src/android/camera_device.cpp | 92 +++++++++++++++++++---------------- src/android/camera_device.h | 3 +- 2 files changed, 52 insertions(+), 43 deletions(-)