| Message ID | 20210927111149.692004-3-umang.jain@ideasonboard.com |
|---|---|
| State | Superseded |
| Delegated to: | Umang Jain |
| Headers | show |
| Series |
|
| Related | show |
Hi Umang, On Mon, Sep 27, 2021 at 04:41:48PM +0530, Umang Jain wrote: > The descriptors_ map hold Camera3RequestDescriptor(s) which are > per-capture request placed by the framework to libcamera HAL. > CameraDevice::requestComplete() looks for the descriptor for which the > camera request has been completed and removes it from the map. > Since, the request is 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 is > extracted from the map and its lifetime is 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 | 91 ++++++++++++++++++----------------- > src/android/camera_device.h | 5 +- > 2 files changed, 49 insertions(+), 47 deletions(-) > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp > index 8b5dd7a1..b0b7f4fd 100644 > --- a/src/android/camera_device.cpp > +++ b/src/android/camera_device.cpp > @@ -955,7 +955,9 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques > * The descriptor and the associated memory reserved here are freed > * at request complete time. > */ > - Camera3RequestDescriptor descriptor(camera_.get(), camera3Request); > + std::unique_ptr<Camera3RequestDescriptor> descriptor = > + std::make_unique<Camera3RequestDescriptor>(camera_.get(), > + camera3Request); > > /* > * \todo The Android request model is incremental, settings passed in > @@ -966,12 +968,12 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques > if (camera3Request->settings) > lastSettings_ = camera3Request->settings; > else > - descriptor.settings_ = lastSettings_; > + descriptor->settings_ = lastSettings_; > > - LOG(HAL, Debug) << "Queueing request " << descriptor.request_->cookie() > - << " with " << descriptor.buffers_.size() << " streams"; > - for (unsigned int i = 0; i < descriptor.buffers_.size(); ++i) { > - const camera3_stream_buffer_t &camera3Buffer = descriptor.buffers_[i]; > + LOG(HAL, Debug) << "Queueing request " << descriptor->request_->cookie() > + << " with " << descriptor->buffers_.size() << " streams"; Many times I touched this file I wanted to insert an empty line here but didn't dare not to receive an "is this related?" comment, but if you have more guts than me, please go ahead. I surely won't complain :D > + for (unsigned int i = 0; i < descriptor->buffers_.size(); ++i) { Should we also take the occasion to for (auto &[i, camera3Buffer] : utils::enumerate(descriptor.buffers_)) { > + const camera3_stream_buffer_t &camera3Buffer = descriptor->buffers_[i]; and drop this ? > camera3_stream *camera3Stream = camera3Buffer.stream; > CameraStream *cameraStream = static_cast<CameraStream *>(camera3Stream->priv); > > @@ -1003,12 +1005,12 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques > * associate it with the Camera3RequestDescriptor for > * lifetime management only. > */ > - descriptor.frameBuffers_.push_back( > + descriptor->frameBuffers_.push_back( > createFrameBuffer(*camera3Buffer.buffer, > cameraStream->configuration().pixelFormat, > cameraStream->configuration().size)); > > - buffer = descriptor.frameBuffers_.back().get(); > + buffer = descriptor->frameBuffers_.back().get(); > LOG(HAL, Debug) << ss.str() << " (direct)"; > break; > > @@ -1030,7 +1032,7 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques > return -ENOMEM; > } > > - descriptor.request_->addBuffer(cameraStream->stream(), buffer, > + descriptor->request_->addBuffer(cameraStream->stream(), buffer, > camera3Buffer.acquire_fence); > } > > @@ -1038,7 +1040,7 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques > * Translate controls from Android to libcamera and queue the request > * to the CameraWorker thread. > */ > - int ret = processControls(&descriptor); > + int ret = processControls(descriptor.get()); > if (ret) > return ret; > > @@ -1066,11 +1068,11 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques > state_ = State::Running; > } > > - CaptureRequest *request = descriptor.request_.get(); > + CaptureRequest *request = descriptor->request_.get(); > > { > MutexLocker descriptorsLock(descriptorsMutex_); > - descriptors_[descriptor.request_->cookie()] = std::move(descriptor); > + descriptors_.push_back(std::move(descriptor)); > } > > worker_.queueRequest(request); > @@ -1080,31 +1082,27 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques > > void CameraDevice::requestComplete(Request *request) > { > - decltype(descriptors_)::node_type node; > - { > - MutexLocker descriptorsLock(descriptorsMutex_); > - auto it = descriptors_.find(request->cookie()); > - if (it == descriptors_.end()) { > - /* > - * \todo Clarify if the Camera has to be closed on > - * ERROR_DEVICE and possibly demote the Fatal to simple > - * Error. > - */ > - notifyError(0, nullptr, CAMERA3_MSG_ERROR_DEVICE); > - LOG(HAL, Fatal) > - << "Unknown request: " << request->cookie(); > + if (descriptors_.empty()) > + return; Access to descriptor_ should be locked. Can't you std::unique_ptr<Camera3RequestDescriptor> descriptor; { MutexLocker descriptorsLock(descriptorsMutex_); if (descriptors_.empty()) return; descriptor = std::move(descriptors_.front()); descriptors_.pop_front(); } So that you have descrptor managed by a unique_ptr here and you can save the pop_front() at the end ? > > - return; > - } > + Camera3RequestDescriptor *descriptor = descriptors_.front().get(); > + if (descriptor->request_->cookie() != request->cookie()) { > + /* > + * \todo Clarify if the Camera has to be closed on > + * ERROR_DEVICE and possibly demote the Fatal to simple > + * Error. > + */ > + notifyError(0, nullptr, CAMERA3_MSG_ERROR_DEVICE); > + LOG(HAL, Fatal) > + << "Unknown request: " << request->cookie(); > > - node = descriptors_.extract(it); > + return; > } > - Camera3RequestDescriptor &descriptor = node.mapped(); > > camera3_capture_result_t captureResult = {}; > - captureResult.frame_number = descriptor.frameNumber_; > - captureResult.num_output_buffers = descriptor.buffers_.size(); > - captureResult.output_buffers = descriptor.buffers_.data(); > + captureResult.frame_number = descriptor->frameNumber_; > + captureResult.num_output_buffers = descriptor->buffers_.size(); > + captureResult.output_buffers = descriptor->buffers_.data(); > > /* > * If the Request has failed, abort the request by notifying the error > @@ -1115,11 +1113,11 @@ void CameraDevice::requestComplete(Request *request) > << " not successfully completed: " > << request->status(); > > - notifyError(descriptor.frameNumber_, nullptr, > + notifyError(descriptor->frameNumber_, nullptr, > CAMERA3_MSG_ERROR_REQUEST); > > captureResult.partial_result = 0; > - for (camera3_stream_buffer_t &buffer : descriptor.buffers_) { > + for (camera3_stream_buffer_t &buffer : descriptor->buffers_) { > CameraStream *cameraStream = > static_cast<CameraStream *>(buffer.stream->priv); > > @@ -1142,6 +1140,7 @@ void CameraDevice::requestComplete(Request *request) > } > callbacks_->process_capture_result(callbacks_, &captureResult); > > + descriptors_.pop_front(); > return; > } > > @@ -1153,10 +1152,10 @@ void CameraDevice::requestComplete(Request *request) > */ > uint64_t sensorTimestamp = static_cast<uint64_t>(request->metadata() > .get(controls::SensorTimestamp)); > - notifyShutter(descriptor.frameNumber_, sensorTimestamp); > + notifyShutter(descriptor->frameNumber_, sensorTimestamp); > > LOG(HAL, Debug) << "Request " << request->cookie() << " completed with " > - << descriptor.buffers_.size() << " streams"; > + << descriptor->buffers_.size() << " streams"; > > /* > * Generate the metadata associated with the captured buffers. > @@ -1166,14 +1165,14 @@ void CameraDevice::requestComplete(Request *request) > */ > std::unique_ptr<CameraMetadata> resultMetadata = getResultMetadata(descriptor); > if (!resultMetadata) { > - notifyError(descriptor.frameNumber_, nullptr, CAMERA3_MSG_ERROR_RESULT); > + notifyError(descriptor->frameNumber_, nullptr, CAMERA3_MSG_ERROR_RESULT); > > /* The camera framework expects an empty metadata pack on error. */ > resultMetadata = std::make_unique<CameraMetadata>(0, 0); > } > > /* Handle post-processing. */ > - for (camera3_stream_buffer_t &buffer : descriptor.buffers_) { > + for (camera3_stream_buffer_t &buffer : descriptor->buffers_) { > CameraStream *cameraStream = > static_cast<CameraStream *>(buffer.stream->priv); > > @@ -1184,13 +1183,13 @@ void CameraDevice::requestComplete(Request *request) > if (!src) { > LOG(HAL, Error) << "Failed to find a source stream buffer"; > buffer.status = CAMERA3_BUFFER_STATUS_ERROR; > - notifyError(descriptor.frameNumber_, buffer.stream, > + notifyError(descriptor->frameNumber_, buffer.stream, > CAMERA3_MSG_ERROR_BUFFER); > continue; > } > > int ret = cameraStream->process(*src, buffer, > - descriptor.settings_, > + descriptor->settings_, > resultMetadata.get()); > /* > * Return the FrameBuffer to the CameraStream now that we're > @@ -1201,7 +1200,7 @@ void CameraDevice::requestComplete(Request *request) > > if (ret) { > buffer.status = CAMERA3_BUFFER_STATUS_ERROR; > - notifyError(descriptor.frameNumber_, buffer.stream, > + notifyError(descriptor->frameNumber_, buffer.stream, > CAMERA3_MSG_ERROR_BUFFER); > } > } > @@ -1210,7 +1209,7 @@ void CameraDevice::requestComplete(Request *request) > * Finalize the capture result by setting fences and buffer status > * before executing the callback. > */ > - for (camera3_stream_buffer_t &buffer : descriptor.buffers_) { > + for (camera3_stream_buffer_t &buffer : descriptor->buffers_) { > buffer.acquire_fence = -1; > buffer.release_fence = -1; > buffer.status = CAMERA3_BUFFER_STATUS_OK; > @@ -1219,6 +1218,8 @@ void CameraDevice::requestComplete(Request *request) > > captureResult.result = resultMetadata->get(); > callbacks_->process_capture_result(callbacks_, &captureResult); > + > + descriptors_.pop_front(); > } > > std::string CameraDevice::logPrefix() const > @@ -1254,10 +1255,10 @@ void CameraDevice::notifyError(uint32_t frameNumber, camera3_stream_t *stream, > * Produce a set of fixed result metadata. > */ > std::unique_ptr<CameraMetadata> > -CameraDevice::getResultMetadata(const Camera3RequestDescriptor &descriptor) const > +CameraDevice::getResultMetadata(const Camera3RequestDescriptor *descriptor) const Not sure it's better to make a pointer here, or keep a reference and dereference the argument in the caller... A minor anyway... > { > - const ControlList &metadata = descriptor.request_->metadata(); > - const CameraMetadata &settings = descriptor.settings_; > + const ControlList &metadata = descriptor->request_->metadata(); > + const CameraMetadata &settings = descriptor->settings_; > camera_metadata_ro_entry_t entry; > bool found; > > diff --git a/src/android/camera_device.h b/src/android/camera_device.h > index 43eb5895..5889a0e7 100644 > --- a/src/android/camera_device.h > +++ b/src/android/camera_device.h > @@ -7,6 +7,7 @@ > #ifndef __ANDROID_CAMERA_DEVICE_H__ > #define __ANDROID_CAMERA_DEVICE_H__ > > +#include <deque> > #include <map> > #include <memory> > #include <mutex> > @@ -105,7 +106,7 @@ private: > camera3_error_msg_code code); > int processControls(Camera3RequestDescriptor *descriptor); > std::unique_ptr<CameraMetadata> getResultMetadata( > - const Camera3RequestDescriptor &descriptor) const; > + const Camera3RequestDescriptor *descriptor) const; > > unsigned int id_; > camera3_device_t camera3Device_; > @@ -125,7 +126,7 @@ private: > std::vector<CameraStream> streams_; > > libcamera::Mutex descriptorsMutex_; /* Protects descriptors_. */ > - std::map<uint64_t, Camera3RequestDescriptor> descriptors_; > + std::deque<std::unique_ptr<Camera3RequestDescriptor>> descriptors_; > > std::string maker_; > std::string model_; > -- > 2.31.1 >
Hello, On Mon, Sep 27, 2021 at 10:38:25PM +0200, Jacopo Mondi wrote: > On Mon, Sep 27, 2021 at 04:41:48PM +0530, Umang Jain wrote: > > The descriptors_ map hold Camera3RequestDescriptor(s) which are s/hold/holds/ > > per-capture request placed by the framework to libcamera HAL. s/request/requests/ > > CameraDevice::requestComplete() looks for the descriptor for which the > > camera request has been completed and removes it from the map. > > Since, the request is placed in form of FIFO and the framework expects s/Since, the request is/Since the requests are/ > > the order of completion to be FIFO as well, this calls for a need of > > a queue rather than a std::map. > > > > This patch still keeps the same lifetime of Camera3RequestDescriptor as > > before i.e. in the requestComplete(). Previously, a descriptor is s/ is/ was/ > > extracted from the map and its lifetime is bound to requestComplete(). s/ is / was / > > The lifetime is kept same by manually calling .pop_front() on the > > queue. In the subsequent commit, this is likely to change with a > > centralized location of dropping descriptors from the queue for request > > completion. > > > > Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> > > --- > > src/android/camera_device.cpp | 91 ++++++++++++++++++----------------- > > src/android/camera_device.h | 5 +- > > 2 files changed, 49 insertions(+), 47 deletions(-) > > > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp > > index 8b5dd7a1..b0b7f4fd 100644 > > --- a/src/android/camera_device.cpp > > +++ b/src/android/camera_device.cpp > > @@ -955,7 +955,9 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques > > * The descriptor and the associated memory reserved here are freed > > * at request complete time. > > */ > > - Camera3RequestDescriptor descriptor(camera_.get(), camera3Request); > > + std::unique_ptr<Camera3RequestDescriptor> descriptor = > > + std::make_unique<Camera3RequestDescriptor>(camera_.get(), > > + camera3Request); > > > > /* > > * \todo The Android request model is incremental, settings passed in > > @@ -966,12 +968,12 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques > > if (camera3Request->settings) > > lastSettings_ = camera3Request->settings; > > else > > - descriptor.settings_ = lastSettings_; > > + descriptor->settings_ = lastSettings_; > > > > - LOG(HAL, Debug) << "Queueing request " << descriptor.request_->cookie() > > - << " with " << descriptor.buffers_.size() << " streams"; > > - for (unsigned int i = 0; i < descriptor.buffers_.size(); ++i) { > > - const camera3_stream_buffer_t &camera3Buffer = descriptor.buffers_[i]; > > + LOG(HAL, Debug) << "Queueing request " << descriptor->request_->cookie() > > + << " with " << descriptor->buffers_.size() << " streams"; > > Many times I touched this file I wanted to insert an empty line here > but didn't dare not to receive an "is this related?" comment, but if > you have more guts than me, please go ahead. I surely won't complain :D Neither will I :-) > > + for (unsigned int i = 0; i < descriptor->buffers_.size(); ++i) { > > Should we also take the occasion to > > for (auto &[i, camera3Buffer] : utils::enumerate(descriptor.buffers_)) { > > > + const camera3_stream_buffer_t &camera3Buffer = descriptor->buffers_[i]; > > and drop this ? Fine with me. > > camera3_stream *camera3Stream = camera3Buffer.stream; > > CameraStream *cameraStream = static_cast<CameraStream *>(camera3Stream->priv); > > > > @@ -1003,12 +1005,12 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques > > * associate it with the Camera3RequestDescriptor for > > * lifetime management only. > > */ > > - descriptor.frameBuffers_.push_back( > > + descriptor->frameBuffers_.push_back( > > createFrameBuffer(*camera3Buffer.buffer, > > cameraStream->configuration().pixelFormat, > > cameraStream->configuration().size)); > > > > - buffer = descriptor.frameBuffers_.back().get(); > > + buffer = descriptor->frameBuffers_.back().get(); > > LOG(HAL, Debug) << ss.str() << " (direct)"; > > break; > > > > @@ -1030,7 +1032,7 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques > > return -ENOMEM; > > } > > > > - descriptor.request_->addBuffer(cameraStream->stream(), buffer, > > + descriptor->request_->addBuffer(cameraStream->stream(), buffer, > > camera3Buffer.acquire_fence); > > } > > > > @@ -1038,7 +1040,7 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques > > * Translate controls from Android to libcamera and queue the request > > * to the CameraWorker thread. > > */ > > - int ret = processControls(&descriptor); > > + int ret = processControls(descriptor.get()); > > if (ret) > > return ret; > > > > @@ -1066,11 +1068,11 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques > > state_ = State::Running; > > } > > > > - CaptureRequest *request = descriptor.request_.get(); > > + CaptureRequest *request = descriptor->request_.get(); > > > > { > > MutexLocker descriptorsLock(descriptorsMutex_); > > - descriptors_[descriptor.request_->cookie()] = std::move(descriptor); > > + descriptors_.push_back(std::move(descriptor)); > > } > > > > worker_.queueRequest(request); > > @@ -1080,31 +1082,27 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques > > > > void CameraDevice::requestComplete(Request *request) > > { > > - decltype(descriptors_)::node_type node; > > - { > > - MutexLocker descriptorsLock(descriptorsMutex_); > > - auto it = descriptors_.find(request->cookie()); > > - if (it == descriptors_.end()) { > > - /* > > - * \todo Clarify if the Camera has to be closed on > > - * ERROR_DEVICE and possibly demote the Fatal to simple > > - * Error. > > - */ > > - notifyError(0, nullptr, CAMERA3_MSG_ERROR_DEVICE); > > - LOG(HAL, Fatal) > > - << "Unknown request: " << request->cookie(); > > + if (descriptors_.empty()) > > + return; > > Access to descriptor_ should be locked. > > Can't you > > std::unique_ptr<Camera3RequestDescriptor> descriptor; > { > MutexLocker descriptorsLock(descriptorsMutex_); > > if (descriptors_.empty()) > return; > > descriptor = std::move(descriptors_.front()); > descriptors_.pop_front(); > } > > So that you have descrptor managed by a unique_ptr here and you can > save the pop_front() at the end ? It may conflict with patch 3/3. Another option is Camera3RequestDescriptor *descriptor; { MutexLocker descriptorsLock(descriptorsMutex_); ASSERT(!descriptors_.empty()); descriptor = descriptors_.front().get(); } with a pop at the end. I've used an ASSERT() here as it's really not supposed to happen, but maybe an error message would be better ? In that case, you should call notifyError(). I don't think we would be able to recover from this. > > - return; > > - } > > + Camera3RequestDescriptor *descriptor = descriptors_.front().get(); > > + if (descriptor->request_->cookie() != request->cookie()) { > > + /* > > + * \todo Clarify if the Camera has to be closed on > > + * ERROR_DEVICE and possibly demote the Fatal to simple > > + * Error. > > + */ > > + notifyError(0, nullptr, CAMERA3_MSG_ERROR_DEVICE); > > + LOG(HAL, Fatal) > > + << "Unknown request: " << request->cookie(); > > > > - node = descriptors_.extract(it); > > + return; > > } > > - Camera3RequestDescriptor &descriptor = node.mapped(); > > > > camera3_capture_result_t captureResult = {}; > > - captureResult.frame_number = descriptor.frameNumber_; > > - captureResult.num_output_buffers = descriptor.buffers_.size(); > > - captureResult.output_buffers = descriptor.buffers_.data(); > > + captureResult.frame_number = descriptor->frameNumber_; > > + captureResult.num_output_buffers = descriptor->buffers_.size(); > > + captureResult.output_buffers = descriptor->buffers_.data(); > > > > /* > > * If the Request has failed, abort the request by notifying the error > > @@ -1115,11 +1113,11 @@ void CameraDevice::requestComplete(Request *request) > > << " not successfully completed: " > > << request->status(); > > > > - notifyError(descriptor.frameNumber_, nullptr, > > + notifyError(descriptor->frameNumber_, nullptr, > > CAMERA3_MSG_ERROR_REQUEST); > > > > captureResult.partial_result = 0; > > - for (camera3_stream_buffer_t &buffer : descriptor.buffers_) { > > + for (camera3_stream_buffer_t &buffer : descriptor->buffers_) { > > CameraStream *cameraStream = > > static_cast<CameraStream *>(buffer.stream->priv); > > > > @@ -1142,6 +1140,7 @@ void CameraDevice::requestComplete(Request *request) > > } > > callbacks_->process_capture_result(callbacks_, &captureResult); > > > > + descriptors_.pop_front(); > > return; > > } > > > > @@ -1153,10 +1152,10 @@ void CameraDevice::requestComplete(Request *request) > > */ > > uint64_t sensorTimestamp = static_cast<uint64_t>(request->metadata() > > .get(controls::SensorTimestamp)); > > - notifyShutter(descriptor.frameNumber_, sensorTimestamp); > > + notifyShutter(descriptor->frameNumber_, sensorTimestamp); > > > > LOG(HAL, Debug) << "Request " << request->cookie() << " completed with " > > - << descriptor.buffers_.size() << " streams"; > > + << descriptor->buffers_.size() << " streams"; > > > > /* > > * Generate the metadata associated with the captured buffers. > > @@ -1166,14 +1165,14 @@ void CameraDevice::requestComplete(Request *request) > > */ > > std::unique_ptr<CameraMetadata> resultMetadata = getResultMetadata(descriptor); > > if (!resultMetadata) { > > - notifyError(descriptor.frameNumber_, nullptr, CAMERA3_MSG_ERROR_RESULT); > > + notifyError(descriptor->frameNumber_, nullptr, CAMERA3_MSG_ERROR_RESULT); > > > > /* The camera framework expects an empty metadata pack on error. */ > > resultMetadata = std::make_unique<CameraMetadata>(0, 0); > > } > > > > /* Handle post-processing. */ > > - for (camera3_stream_buffer_t &buffer : descriptor.buffers_) { > > + for (camera3_stream_buffer_t &buffer : descriptor->buffers_) { > > CameraStream *cameraStream = > > static_cast<CameraStream *>(buffer.stream->priv); > > > > @@ -1184,13 +1183,13 @@ void CameraDevice::requestComplete(Request *request) > > if (!src) { > > LOG(HAL, Error) << "Failed to find a source stream buffer"; > > buffer.status = CAMERA3_BUFFER_STATUS_ERROR; > > - notifyError(descriptor.frameNumber_, buffer.stream, > > + notifyError(descriptor->frameNumber_, buffer.stream, > > CAMERA3_MSG_ERROR_BUFFER); > > continue; > > } > > > > int ret = cameraStream->process(*src, buffer, > > - descriptor.settings_, > > + descriptor->settings_, > > resultMetadata.get()); > > /* > > * Return the FrameBuffer to the CameraStream now that we're > > @@ -1201,7 +1200,7 @@ void CameraDevice::requestComplete(Request *request) > > > > if (ret) { > > buffer.status = CAMERA3_BUFFER_STATUS_ERROR; > > - notifyError(descriptor.frameNumber_, buffer.stream, > > + notifyError(descriptor->frameNumber_, buffer.stream, > > CAMERA3_MSG_ERROR_BUFFER); > > } > > } > > @@ -1210,7 +1209,7 @@ void CameraDevice::requestComplete(Request *request) > > * Finalize the capture result by setting fences and buffer status > > * before executing the callback. > > */ > > - for (camera3_stream_buffer_t &buffer : descriptor.buffers_) { > > + for (camera3_stream_buffer_t &buffer : descriptor->buffers_) { > > buffer.acquire_fence = -1; > > buffer.release_fence = -1; > > buffer.status = CAMERA3_BUFFER_STATUS_OK; > > @@ -1219,6 +1218,8 @@ void CameraDevice::requestComplete(Request *request) > > > > captureResult.result = resultMetadata->get(); > > callbacks_->process_capture_result(callbacks_, &captureResult); > > + You need MutexLocker descriptorsLock(descriptorsMutex_); here. > > + descriptors_.pop_front(); > > } > > > > std::string CameraDevice::logPrefix() const > > @@ -1254,10 +1255,10 @@ void CameraDevice::notifyError(uint32_t frameNumber, camera3_stream_t *stream, > > * Produce a set of fixed result metadata. > > */ > > std::unique_ptr<CameraMetadata> > > -CameraDevice::getResultMetadata(const Camera3RequestDescriptor &descriptor) const > > +CameraDevice::getResultMetadata(const Camera3RequestDescriptor *descriptor) const > > Not sure it's better to make a pointer here, or keep a reference and > dereference the argument in the caller... A minor anyway... Keeping a reference would be better I think, as the argument can't be null. > > { > > - const ControlList &metadata = descriptor.request_->metadata(); > > - const CameraMetadata &settings = descriptor.settings_; > > + const ControlList &metadata = descriptor->request_->metadata(); > > + const CameraMetadata &settings = descriptor->settings_; > > camera_metadata_ro_entry_t entry; > > bool found; > > > > diff --git a/src/android/camera_device.h b/src/android/camera_device.h > > index 43eb5895..5889a0e7 100644 > > --- a/src/android/camera_device.h > > +++ b/src/android/camera_device.h > > @@ -7,6 +7,7 @@ > > #ifndef __ANDROID_CAMERA_DEVICE_H__ > > #define __ANDROID_CAMERA_DEVICE_H__ > > > > +#include <deque> > > #include <map> > > #include <memory> > > #include <mutex> > > @@ -105,7 +106,7 @@ private: > > camera3_error_msg_code code); > > int processControls(Camera3RequestDescriptor *descriptor); > > std::unique_ptr<CameraMetadata> getResultMetadata( > > - const Camera3RequestDescriptor &descriptor) const; > > + const Camera3RequestDescriptor *descriptor) const; > > > > unsigned int id_; > > camera3_device_t camera3Device_; > > @@ -125,7 +126,7 @@ private: > > std::vector<CameraStream> streams_; > > > > libcamera::Mutex descriptorsMutex_; /* Protects descriptors_. */ > > - std::map<uint64_t, Camera3RequestDescriptor> descriptors_; > > + std::deque<std::unique_ptr<Camera3RequestDescriptor>> descriptors_; You can use std::queue as it's a FIFO. > > > > std::string maker_; > > std::string model_;
Hi Laurent, On 9/28/21 3:53 AM, Laurent Pinchart wrote: > Hello, > > On Mon, Sep 27, 2021 at 10:38:25PM +0200, Jacopo Mondi wrote: >> On Mon, Sep 27, 2021 at 04:41:48PM +0530, Umang Jain wrote: >>> The descriptors_ map hold Camera3RequestDescriptor(s) which are > s/hold/holds/ > >>> per-capture request placed by the framework to libcamera HAL. > s/request/requests/ > >>> CameraDevice::requestComplete() looks for the descriptor for which the >>> camera request has been completed and removes it from the map. >>> Since, the request is placed in form of FIFO and the framework expects > s/Since, the request is/Since the requests are/ > >>> the order of completion to be FIFO as well, this calls for a need of >>> a queue rather than a std::map. >>> >>> This patch still keeps the same lifetime of Camera3RequestDescriptor as >>> before i.e. in the requestComplete(). Previously, a descriptor is > s/ is/ was/ > >>> extracted from the map and its lifetime is bound to requestComplete(). > s/ is / was / > >>> The lifetime is kept same by manually calling .pop_front() on the >>> queue. In the subsequent commit, this is likely to change with a >>> centralized location of dropping descriptors from the queue for request >>> completion. >>> >>> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> >>> --- >>> src/android/camera_device.cpp | 91 ++++++++++++++++++----------------- >>> src/android/camera_device.h | 5 +- >>> 2 files changed, 49 insertions(+), 47 deletions(-) >>> >>> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp >>> index 8b5dd7a1..b0b7f4fd 100644 >>> --- a/src/android/camera_device.cpp >>> +++ b/src/android/camera_device.cpp >>> @@ -955,7 +955,9 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques >>> * The descriptor and the associated memory reserved here are freed >>> * at request complete time. >>> */ >>> - Camera3RequestDescriptor descriptor(camera_.get(), camera3Request); >>> + std::unique_ptr<Camera3RequestDescriptor> descriptor = >>> + std::make_unique<Camera3RequestDescriptor>(camera_.get(), >>> + camera3Request); >>> >>> /* >>> * \todo The Android request model is incremental, settings passed in >>> @@ -966,12 +968,12 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques >>> if (camera3Request->settings) >>> lastSettings_ = camera3Request->settings; >>> else >>> - descriptor.settings_ = lastSettings_; >>> + descriptor->settings_ = lastSettings_; >>> >>> - LOG(HAL, Debug) << "Queueing request " << descriptor.request_->cookie() >>> - << " with " << descriptor.buffers_.size() << " streams"; >>> - for (unsigned int i = 0; i < descriptor.buffers_.size(); ++i) { >>> - const camera3_stream_buffer_t &camera3Buffer = descriptor.buffers_[i]; >>> + LOG(HAL, Debug) << "Queueing request " << descriptor->request_->cookie() >>> + << " with " << descriptor->buffers_.size() << " streams"; >> Many times I touched this file I wanted to insert an empty line here >> but didn't dare not to receive an "is this related?" comment, but if >> you have more guts than me, please go ahead. I surely won't complain :D > Neither will I :-) > >>> + for (unsigned int i = 0; i < descriptor->buffers_.size(); ++i) { >> Should we also take the occasion to >> >> for (auto &[i, camera3Buffer] : utils::enumerate(descriptor.buffers_)) { >> >>> + const camera3_stream_buffer_t &camera3Buffer = descriptor->buffers_[i]; >> and drop this ? > Fine with me. > >>> camera3_stream *camera3Stream = camera3Buffer.stream; >>> CameraStream *cameraStream = static_cast<CameraStream *>(camera3Stream->priv); >>> >>> @@ -1003,12 +1005,12 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques >>> * associate it with the Camera3RequestDescriptor for >>> * lifetime management only. >>> */ >>> - descriptor.frameBuffers_.push_back( >>> + descriptor->frameBuffers_.push_back( >>> createFrameBuffer(*camera3Buffer.buffer, >>> cameraStream->configuration().pixelFormat, >>> cameraStream->configuration().size)); >>> >>> - buffer = descriptor.frameBuffers_.back().get(); >>> + buffer = descriptor->frameBuffers_.back().get(); >>> LOG(HAL, Debug) << ss.str() << " (direct)"; >>> break; >>> >>> @@ -1030,7 +1032,7 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques >>> return -ENOMEM; >>> } >>> >>> - descriptor.request_->addBuffer(cameraStream->stream(), buffer, >>> + descriptor->request_->addBuffer(cameraStream->stream(), buffer, >>> camera3Buffer.acquire_fence); >>> } >>> >>> @@ -1038,7 +1040,7 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques >>> * Translate controls from Android to libcamera and queue the request >>> * to the CameraWorker thread. >>> */ >>> - int ret = processControls(&descriptor); >>> + int ret = processControls(descriptor.get()); >>> if (ret) >>> return ret; >>> >>> @@ -1066,11 +1068,11 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques >>> state_ = State::Running; >>> } >>> >>> - CaptureRequest *request = descriptor.request_.get(); >>> + CaptureRequest *request = descriptor->request_.get(); >>> >>> { >>> MutexLocker descriptorsLock(descriptorsMutex_); >>> - descriptors_[descriptor.request_->cookie()] = std::move(descriptor); >>> + descriptors_.push_back(std::move(descriptor)); >>> } >>> >>> worker_.queueRequest(request); >>> @@ -1080,31 +1082,27 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques >>> >>> void CameraDevice::requestComplete(Request *request) >>> { >>> - decltype(descriptors_)::node_type node; >>> - { >>> - MutexLocker descriptorsLock(descriptorsMutex_); >>> - auto it = descriptors_.find(request->cookie()); >>> - if (it == descriptors_.end()) { >>> - /* >>> - * \todo Clarify if the Camera has to be closed on >>> - * ERROR_DEVICE and possibly demote the Fatal to simple >>> - * Error. >>> - */ >>> - notifyError(0, nullptr, CAMERA3_MSG_ERROR_DEVICE); >>> - LOG(HAL, Fatal) >>> - << "Unknown request: " << request->cookie(); >>> + if (descriptors_.empty()) >>> + return; >> Access to descriptor_ should be locked. >> >> Can't you >> >> std::unique_ptr<Camera3RequestDescriptor> descriptor; >> { >> MutexLocker descriptorsLock(descriptorsMutex_); >> >> if (descriptors_.empty()) >> return; >> >> descriptor = std::move(descriptors_.front()); >> descriptors_.pop_front(); >> } >> >> So that you have descrptor managed by a unique_ptr here and you can >> save the pop_front() at the end ? > It may conflict with patch 3/3. Another option is > > Camera3RequestDescriptor *descriptor; > > { > MutexLocker descriptorsLock(descriptorsMutex_); > ASSERT(!descriptors_.empty()); > descriptor = descriptors_.front().get(); > } > > with a pop at the end. Yes, I am inclined towards popping the descriptor at the end > > I've used an ASSERT() here as it's really not supposed to happen, but > maybe an error message would be better ? In that case, you should call > notifyError(). I don't think we would be able to recover from this. Indeed, there was another suggestion on this in async post-processing series, which I am inclining towards. I'd change the message to << "Out-of-order completion for request " << request->cookie(); By the way, with the cookie containing a pointer, I think it would be more readable in hex. Maybe a patch on top to use utils::hex() ? We are logging with Fatal currently, so it's similar to ASSERT with an error log printed. I will keep the choice of Fatal over ASSERT(), should be ok? > >>> - return; >>> - } >>> + Camera3RequestDescriptor *descriptor = descriptors_.front().get(); >>> + if (descriptor->request_->cookie() != request->cookie()) { >>> + /* >>> + * \todo Clarify if the Camera has to be closed on >>> + * ERROR_DEVICE and possibly demote the Fatal to simple >>> + * Error. >>> + */ >>> + notifyError(0, nullptr, CAMERA3_MSG_ERROR_DEVICE); >>> + LOG(HAL, Fatal) >>> + << "Unknown request: " << request->cookie(); >>> >>> - node = descriptors_.extract(it); >>> + return; >>> } >>> - Camera3RequestDescriptor &descriptor = node.mapped(); >>> >>> camera3_capture_result_t captureResult = {}; >>> - captureResult.frame_number = descriptor.frameNumber_; >>> - captureResult.num_output_buffers = descriptor.buffers_.size(); >>> - captureResult.output_buffers = descriptor.buffers_.data(); >>> + captureResult.frame_number = descriptor->frameNumber_; >>> + captureResult.num_output_buffers = descriptor->buffers_.size(); >>> + captureResult.output_buffers = descriptor->buffers_.data(); >>> >>> /* >>> * If the Request has failed, abort the request by notifying the error >>> @@ -1115,11 +1113,11 @@ void CameraDevice::requestComplete(Request *request) >>> << " not successfully completed: " >>> << request->status(); >>> >>> - notifyError(descriptor.frameNumber_, nullptr, >>> + notifyError(descriptor->frameNumber_, nullptr, >>> CAMERA3_MSG_ERROR_REQUEST); >>> >>> captureResult.partial_result = 0; >>> - for (camera3_stream_buffer_t &buffer : descriptor.buffers_) { >>> + for (camera3_stream_buffer_t &buffer : descriptor->buffers_) { >>> CameraStream *cameraStream = >>> static_cast<CameraStream *>(buffer.stream->priv); >>> >>> @@ -1142,6 +1140,7 @@ void CameraDevice::requestComplete(Request *request) >>> } >>> callbacks_->process_capture_result(callbacks_, &captureResult); >>> >>> + descriptors_.pop_front(); >>> return; >>> } >>> >>> @@ -1153,10 +1152,10 @@ void CameraDevice::requestComplete(Request *request) >>> */ >>> uint64_t sensorTimestamp = static_cast<uint64_t>(request->metadata() >>> .get(controls::SensorTimestamp)); >>> - notifyShutter(descriptor.frameNumber_, sensorTimestamp); >>> + notifyShutter(descriptor->frameNumber_, sensorTimestamp); >>> >>> LOG(HAL, Debug) << "Request " << request->cookie() << " completed with " >>> - << descriptor.buffers_.size() << " streams"; >>> + << descriptor->buffers_.size() << " streams"; >>> >>> /* >>> * Generate the metadata associated with the captured buffers. >>> @@ -1166,14 +1165,14 @@ void CameraDevice::requestComplete(Request *request) >>> */ >>> std::unique_ptr<CameraMetadata> resultMetadata = getResultMetadata(descriptor); >>> if (!resultMetadata) { >>> - notifyError(descriptor.frameNumber_, nullptr, CAMERA3_MSG_ERROR_RESULT); >>> + notifyError(descriptor->frameNumber_, nullptr, CAMERA3_MSG_ERROR_RESULT); >>> >>> /* The camera framework expects an empty metadata pack on error. */ >>> resultMetadata = std::make_unique<CameraMetadata>(0, 0); >>> } >>> >>> /* Handle post-processing. */ >>> - for (camera3_stream_buffer_t &buffer : descriptor.buffers_) { >>> + for (camera3_stream_buffer_t &buffer : descriptor->buffers_) { >>> CameraStream *cameraStream = >>> static_cast<CameraStream *>(buffer.stream->priv); >>> >>> @@ -1184,13 +1183,13 @@ void CameraDevice::requestComplete(Request *request) >>> if (!src) { >>> LOG(HAL, Error) << "Failed to find a source stream buffer"; >>> buffer.status = CAMERA3_BUFFER_STATUS_ERROR; >>> - notifyError(descriptor.frameNumber_, buffer.stream, >>> + notifyError(descriptor->frameNumber_, buffer.stream, >>> CAMERA3_MSG_ERROR_BUFFER); >>> continue; >>> } >>> >>> int ret = cameraStream->process(*src, buffer, >>> - descriptor.settings_, >>> + descriptor->settings_, >>> resultMetadata.get()); >>> /* >>> * Return the FrameBuffer to the CameraStream now that we're >>> @@ -1201,7 +1200,7 @@ void CameraDevice::requestComplete(Request *request) >>> >>> if (ret) { >>> buffer.status = CAMERA3_BUFFER_STATUS_ERROR; >>> - notifyError(descriptor.frameNumber_, buffer.stream, >>> + notifyError(descriptor->frameNumber_, buffer.stream, >>> CAMERA3_MSG_ERROR_BUFFER); >>> } >>> } >>> @@ -1210,7 +1209,7 @@ void CameraDevice::requestComplete(Request *request) >>> * Finalize the capture result by setting fences and buffer status >>> * before executing the callback. >>> */ >>> - for (camera3_stream_buffer_t &buffer : descriptor.buffers_) { >>> + for (camera3_stream_buffer_t &buffer : descriptor->buffers_) { >>> buffer.acquire_fence = -1; >>> buffer.release_fence = -1; >>> buffer.status = CAMERA3_BUFFER_STATUS_OK; >>> @@ -1219,6 +1218,8 @@ void CameraDevice::requestComplete(Request *request) >>> >>> captureResult.result = resultMetadata->get(); >>> callbacks_->process_capture_result(callbacks_, &captureResult); >>> + > You need > > MutexLocker descriptorsLock(descriptorsMutex_); > > here. > >>> + descriptors_.pop_front(); >>> } >>> >>> std::string CameraDevice::logPrefix() const >>> @@ -1254,10 +1255,10 @@ void CameraDevice::notifyError(uint32_t frameNumber, camera3_stream_t *stream, >>> * Produce a set of fixed result metadata. >>> */ >>> std::unique_ptr<CameraMetadata> >>> -CameraDevice::getResultMetadata(const Camera3RequestDescriptor &descriptor) const >>> +CameraDevice::getResultMetadata(const Camera3RequestDescriptor *descriptor) const >> Not sure it's better to make a pointer here, or keep a reference and >> dereference the argument in the caller... A minor anyway... > Keeping a reference would be better I think, as the argument can't be > null. > >>> { >>> - const ControlList &metadata = descriptor.request_->metadata(); >>> - const CameraMetadata &settings = descriptor.settings_; >>> + const ControlList &metadata = descriptor->request_->metadata(); >>> + const CameraMetadata &settings = descriptor->settings_; >>> camera_metadata_ro_entry_t entry; >>> bool found; >>> >>> diff --git a/src/android/camera_device.h b/src/android/camera_device.h >>> index 43eb5895..5889a0e7 100644 >>> --- a/src/android/camera_device.h >>> +++ b/src/android/camera_device.h >>> @@ -7,6 +7,7 @@ >>> #ifndef __ANDROID_CAMERA_DEVICE_H__ >>> #define __ANDROID_CAMERA_DEVICE_H__ >>> >>> +#include <deque> >>> #include <map> >>> #include <memory> >>> #include <mutex> >>> @@ -105,7 +106,7 @@ private: >>> camera3_error_msg_code code); >>> int processControls(Camera3RequestDescriptor *descriptor); >>> std::unique_ptr<CameraMetadata> getResultMetadata( >>> - const Camera3RequestDescriptor &descriptor) const; >>> + const Camera3RequestDescriptor *descriptor) const; >>> >>> unsigned int id_; >>> camera3_device_t camera3Device_; >>> @@ -125,7 +126,7 @@ private: >>> std::vector<CameraStream> streams_; >>> >>> libcamera::Mutex descriptorsMutex_; /* Protects descriptors_. */ >>> - std::map<uint64_t, Camera3RequestDescriptor> descriptors_; >>> + std::deque<std::unique_ptr<Camera3RequestDescriptor>> descriptors_; > You can use std::queue as it's a FIFO. Ack, > >>> std::string maker_; >>> std::string model_;
diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp index 8b5dd7a1..b0b7f4fd 100644 --- a/src/android/camera_device.cpp +++ b/src/android/camera_device.cpp @@ -955,7 +955,9 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques * The descriptor and the associated memory reserved here are freed * at request complete time. */ - Camera3RequestDescriptor descriptor(camera_.get(), camera3Request); + std::unique_ptr<Camera3RequestDescriptor> descriptor = + std::make_unique<Camera3RequestDescriptor>(camera_.get(), + camera3Request); /* * \todo The Android request model is incremental, settings passed in @@ -966,12 +968,12 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques if (camera3Request->settings) lastSettings_ = camera3Request->settings; else - descriptor.settings_ = lastSettings_; + descriptor->settings_ = lastSettings_; - LOG(HAL, Debug) << "Queueing request " << descriptor.request_->cookie() - << " with " << descriptor.buffers_.size() << " streams"; - for (unsigned int i = 0; i < descriptor.buffers_.size(); ++i) { - const camera3_stream_buffer_t &camera3Buffer = descriptor.buffers_[i]; + LOG(HAL, Debug) << "Queueing request " << descriptor->request_->cookie() + << " with " << descriptor->buffers_.size() << " streams"; + for (unsigned int i = 0; i < descriptor->buffers_.size(); ++i) { + const camera3_stream_buffer_t &camera3Buffer = descriptor->buffers_[i]; camera3_stream *camera3Stream = camera3Buffer.stream; CameraStream *cameraStream = static_cast<CameraStream *>(camera3Stream->priv); @@ -1003,12 +1005,12 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques * associate it with the Camera3RequestDescriptor for * lifetime management only. */ - descriptor.frameBuffers_.push_back( + descriptor->frameBuffers_.push_back( createFrameBuffer(*camera3Buffer.buffer, cameraStream->configuration().pixelFormat, cameraStream->configuration().size)); - buffer = descriptor.frameBuffers_.back().get(); + buffer = descriptor->frameBuffers_.back().get(); LOG(HAL, Debug) << ss.str() << " (direct)"; break; @@ -1030,7 +1032,7 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques return -ENOMEM; } - descriptor.request_->addBuffer(cameraStream->stream(), buffer, + descriptor->request_->addBuffer(cameraStream->stream(), buffer, camera3Buffer.acquire_fence); } @@ -1038,7 +1040,7 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques * Translate controls from Android to libcamera and queue the request * to the CameraWorker thread. */ - int ret = processControls(&descriptor); + int ret = processControls(descriptor.get()); if (ret) return ret; @@ -1066,11 +1068,11 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques state_ = State::Running; } - CaptureRequest *request = descriptor.request_.get(); + CaptureRequest *request = descriptor->request_.get(); { MutexLocker descriptorsLock(descriptorsMutex_); - descriptors_[descriptor.request_->cookie()] = std::move(descriptor); + descriptors_.push_back(std::move(descriptor)); } worker_.queueRequest(request); @@ -1080,31 +1082,27 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques void CameraDevice::requestComplete(Request *request) { - decltype(descriptors_)::node_type node; - { - MutexLocker descriptorsLock(descriptorsMutex_); - auto it = descriptors_.find(request->cookie()); - if (it == descriptors_.end()) { - /* - * \todo Clarify if the Camera has to be closed on - * ERROR_DEVICE and possibly demote the Fatal to simple - * Error. - */ - notifyError(0, nullptr, CAMERA3_MSG_ERROR_DEVICE); - LOG(HAL, Fatal) - << "Unknown request: " << request->cookie(); + if (descriptors_.empty()) + return; - return; - } + Camera3RequestDescriptor *descriptor = descriptors_.front().get(); + if (descriptor->request_->cookie() != request->cookie()) { + /* + * \todo Clarify if the Camera has to be closed on + * ERROR_DEVICE and possibly demote the Fatal to simple + * Error. + */ + notifyError(0, nullptr, CAMERA3_MSG_ERROR_DEVICE); + LOG(HAL, Fatal) + << "Unknown request: " << request->cookie(); - node = descriptors_.extract(it); + return; } - Camera3RequestDescriptor &descriptor = node.mapped(); camera3_capture_result_t captureResult = {}; - captureResult.frame_number = descriptor.frameNumber_; - captureResult.num_output_buffers = descriptor.buffers_.size(); - captureResult.output_buffers = descriptor.buffers_.data(); + captureResult.frame_number = descriptor->frameNumber_; + captureResult.num_output_buffers = descriptor->buffers_.size(); + captureResult.output_buffers = descriptor->buffers_.data(); /* * If the Request has failed, abort the request by notifying the error @@ -1115,11 +1113,11 @@ void CameraDevice::requestComplete(Request *request) << " not successfully completed: " << request->status(); - notifyError(descriptor.frameNumber_, nullptr, + notifyError(descriptor->frameNumber_, nullptr, CAMERA3_MSG_ERROR_REQUEST); captureResult.partial_result = 0; - for (camera3_stream_buffer_t &buffer : descriptor.buffers_) { + for (camera3_stream_buffer_t &buffer : descriptor->buffers_) { CameraStream *cameraStream = static_cast<CameraStream *>(buffer.stream->priv); @@ -1142,6 +1140,7 @@ void CameraDevice::requestComplete(Request *request) } callbacks_->process_capture_result(callbacks_, &captureResult); + descriptors_.pop_front(); return; } @@ -1153,10 +1152,10 @@ void CameraDevice::requestComplete(Request *request) */ uint64_t sensorTimestamp = static_cast<uint64_t>(request->metadata() .get(controls::SensorTimestamp)); - notifyShutter(descriptor.frameNumber_, sensorTimestamp); + notifyShutter(descriptor->frameNumber_, sensorTimestamp); LOG(HAL, Debug) << "Request " << request->cookie() << " completed with " - << descriptor.buffers_.size() << " streams"; + << descriptor->buffers_.size() << " streams"; /* * Generate the metadata associated with the captured buffers. @@ -1166,14 +1165,14 @@ void CameraDevice::requestComplete(Request *request) */ std::unique_ptr<CameraMetadata> resultMetadata = getResultMetadata(descriptor); if (!resultMetadata) { - notifyError(descriptor.frameNumber_, nullptr, CAMERA3_MSG_ERROR_RESULT); + notifyError(descriptor->frameNumber_, nullptr, CAMERA3_MSG_ERROR_RESULT); /* The camera framework expects an empty metadata pack on error. */ resultMetadata = std::make_unique<CameraMetadata>(0, 0); } /* Handle post-processing. */ - for (camera3_stream_buffer_t &buffer : descriptor.buffers_) { + for (camera3_stream_buffer_t &buffer : descriptor->buffers_) { CameraStream *cameraStream = static_cast<CameraStream *>(buffer.stream->priv); @@ -1184,13 +1183,13 @@ void CameraDevice::requestComplete(Request *request) if (!src) { LOG(HAL, Error) << "Failed to find a source stream buffer"; buffer.status = CAMERA3_BUFFER_STATUS_ERROR; - notifyError(descriptor.frameNumber_, buffer.stream, + notifyError(descriptor->frameNumber_, buffer.stream, CAMERA3_MSG_ERROR_BUFFER); continue; } int ret = cameraStream->process(*src, buffer, - descriptor.settings_, + descriptor->settings_, resultMetadata.get()); /* * Return the FrameBuffer to the CameraStream now that we're @@ -1201,7 +1200,7 @@ void CameraDevice::requestComplete(Request *request) if (ret) { buffer.status = CAMERA3_BUFFER_STATUS_ERROR; - notifyError(descriptor.frameNumber_, buffer.stream, + notifyError(descriptor->frameNumber_, buffer.stream, CAMERA3_MSG_ERROR_BUFFER); } } @@ -1210,7 +1209,7 @@ void CameraDevice::requestComplete(Request *request) * Finalize the capture result by setting fences and buffer status * before executing the callback. */ - for (camera3_stream_buffer_t &buffer : descriptor.buffers_) { + for (camera3_stream_buffer_t &buffer : descriptor->buffers_) { buffer.acquire_fence = -1; buffer.release_fence = -1; buffer.status = CAMERA3_BUFFER_STATUS_OK; @@ -1219,6 +1218,8 @@ void CameraDevice::requestComplete(Request *request) captureResult.result = resultMetadata->get(); callbacks_->process_capture_result(callbacks_, &captureResult); + + descriptors_.pop_front(); } std::string CameraDevice::logPrefix() const @@ -1254,10 +1255,10 @@ void CameraDevice::notifyError(uint32_t frameNumber, camera3_stream_t *stream, * Produce a set of fixed result metadata. */ std::unique_ptr<CameraMetadata> -CameraDevice::getResultMetadata(const Camera3RequestDescriptor &descriptor) const +CameraDevice::getResultMetadata(const Camera3RequestDescriptor *descriptor) const { - const ControlList &metadata = descriptor.request_->metadata(); - const CameraMetadata &settings = descriptor.settings_; + const ControlList &metadata = descriptor->request_->metadata(); + const CameraMetadata &settings = descriptor->settings_; camera_metadata_ro_entry_t entry; bool found; diff --git a/src/android/camera_device.h b/src/android/camera_device.h index 43eb5895..5889a0e7 100644 --- a/src/android/camera_device.h +++ b/src/android/camera_device.h @@ -7,6 +7,7 @@ #ifndef __ANDROID_CAMERA_DEVICE_H__ #define __ANDROID_CAMERA_DEVICE_H__ +#include <deque> #include <map> #include <memory> #include <mutex> @@ -105,7 +106,7 @@ private: camera3_error_msg_code code); int processControls(Camera3RequestDescriptor *descriptor); std::unique_ptr<CameraMetadata> getResultMetadata( - const Camera3RequestDescriptor &descriptor) const; + const Camera3RequestDescriptor *descriptor) const; unsigned int id_; camera3_device_t camera3Device_; @@ -125,7 +126,7 @@ private: std::vector<CameraStream> streams_; libcamera::Mutex descriptorsMutex_; /* Protects descriptors_. */ - std::map<uint64_t, Camera3RequestDescriptor> descriptors_; + std::deque<std::unique_ptr<Camera3RequestDescriptor>> descriptors_; std::string maker_; std::string model_;
The descriptors_ map hold Camera3RequestDescriptor(s) which are per-capture request placed by the framework to libcamera HAL. CameraDevice::requestComplete() looks for the descriptor for which the camera request has been completed and removes it from the map. Since, the request is 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 is extracted from the map and its lifetime is 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 | 91 ++++++++++++++++++----------------- src/android/camera_device.h | 5 +- 2 files changed, 49 insertions(+), 47 deletions(-)