| Message ID | 20210928162536.227475-4-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:36PM +0530, Umang Jain wrote: > There is a possibility that an out-of-order completion of capture > request happens by calling process_capture_result() directly on error > paths. The framework expects that errors should be notified as soon as > possible, but the request completion order should remain intact. > An existing instance of this is abortRequest(), which sends the capture > results on flushing state, without considering order-of-completion. > > Since we have a queue of Camera3RequestDescriptor tracking each > capture request placed by framework to libcamera HAL, we should be only > sending back capture results from a single location, by inspecting > the queue. As per the patch, this now happens in > CameraDevice::sendCaptureResults(). > > Each descriptor is now equipped with its own status to denote whether > the capture request is complete and ready to send back to the framework > or needs to be waited upon. This ensures that the order of completion is > respected for the requests. > > Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> > --- > src/android/camera_device.cpp | 44 +++++++++++++++++++++++++---------- > src/android/camera_device.h | 15 +++++++++++- > 2 files changed, 46 insertions(+), 13 deletions(-) > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp > index a3b8a549..83030366 100644 > --- a/src/android/camera_device.cpp > +++ b/src/android/camera_device.cpp > @@ -861,11 +861,12 @@ int CameraDevice::processControls(Camera3RequestDescriptor *descriptor) > return 0; > } > > -void CameraDevice::abortRequest(camera3_capture_request_t *request) > +void CameraDevice::abortRequest(Camera3RequestDescriptor *descriptor, > + camera3_capture_request_t *request) I don't think I've seen a reply to my review comment in v1: Could this function take a Camera3RequestDescriptor pointer only ? It should contain all the needed data. This can be done as a patch before this one if desired, or here as it shouldn't be much extra work. This function uses the num_output_buffers, frame_number and output_buffers members of camera3_capture_request_t. Those are copied to the Camera3RequestDescriptor buffers_ and frameNumber_ members which you can use here. Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > { > notifyError(request->frame_number, nullptr, CAMERA3_MSG_ERROR_REQUEST); > > - camera3_capture_result_t result = {}; > + camera3_capture_result_t &result = descriptor->captureResult_; > result.num_output_buffers = request->num_output_buffers; > result.frame_number = request->frame_number; > result.partial_result = 0; > @@ -879,7 +880,7 @@ void CameraDevice::abortRequest(camera3_capture_request_t *request) > } > result.output_buffers = resultBuffers.data(); > > - callbacks_->process_capture_result(callbacks_, &result); > + descriptor->status_ = Camera3RequestDescriptor::Status::Error; > } > > bool CameraDevice::isValidRequest(camera3_capture_request_t *camera3Request) const > @@ -1052,13 +1053,19 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques > return ret; > > /* > - * If flush is in progress abort the request. If the camera has been > - * stopped we have to re-start it to be able to process the request. > + * If flush is in progress abort the request and push the descriptor to > + * the queue. If the camera has been stopped we have to re-start it to > + * be able to process the request. > */ > MutexLocker stateLock(stateMutex_); > > if (state_ == State::Flushing) { > - abortRequest(camera3Request); > + abortRequest(descriptors_.back().get(), camera3Request); > + { > + MutexLocker descriptorsLock(descriptorsMutex_); > + descriptors_.push(std::move(descriptor)); > + } > + sendCaptureResults(); > return 0; > } > > @@ -1116,7 +1123,7 @@ void CameraDevice::requestComplete(Request *request) > * The buffer status is set to OK and later changed to ERROR if > * post-processing/compression fails. > */ > - camera3_capture_result_t captureResult = {}; > + camera3_capture_result_t &captureResult = descriptor->captureResult_; > captureResult.frame_number = descriptor->frameNumber_; > captureResult.num_output_buffers = descriptor->buffers_.size(); > for (camera3_stream_buffer_t &buffer : descriptor->buffers_) { > @@ -1166,9 +1173,9 @@ void CameraDevice::requestComplete(Request *request) > buffer.status = CAMERA3_BUFFER_STATUS_ERROR; > } > > - callbacks_->process_capture_result(callbacks_, &captureResult); > + descriptor->status_ = Camera3RequestDescriptor::Status::Error; > + sendCaptureResults(); > > - descriptors_.pop(); > return; > } > > @@ -1234,10 +1241,23 @@ void CameraDevice::requestComplete(Request *request) > } > > captureResult.result = resultMetadata->get(); > - callbacks_->process_capture_result(callbacks_, &captureResult); > + descriptor->status_ = Camera3RequestDescriptor::Status::Success; > + sendCaptureResults(); > +} > > - MutexLocker descriptorsLock(descriptorsMutex_); > - descriptors_.pop(); > +void CameraDevice::sendCaptureResults() > +{ > + MutexLocker lock(descriptorsMutex_); > + while (!descriptors_.empty() && !descriptors_.front()->isPending()) { > + std::unique_ptr<Camera3RequestDescriptor> descriptor = > + std::move(descriptors_.front()); > + descriptors_.pop(); > + > + lock.unlock(); > + callbacks_->process_capture_result(callbacks_, > + &descriptor->captureResult_); > + lock.lock(); > + } > } > > std::string CameraDevice::logPrefix() const > diff --git a/src/android/camera_device.h b/src/android/camera_device.h > index 9ec510d5..dbfa7431 100644 > --- a/src/android/camera_device.h > +++ b/src/android/camera_device.h > @@ -74,17 +74,28 @@ private: > CameraDevice(unsigned int id, std::shared_ptr<libcamera::Camera> camera); > > struct Camera3RequestDescriptor { > + enum class Status { > + Pending, > + Success, > + Error, > + }; > + > Camera3RequestDescriptor() = default; > ~Camera3RequestDescriptor() = default; > Camera3RequestDescriptor(libcamera::Camera *camera, > const camera3_capture_request_t *camera3Request); > Camera3RequestDescriptor &operator=(Camera3RequestDescriptor &&) = default; > > + bool isPending() const { return status_ == Status::Pending; } > + > uint32_t frameNumber_ = 0; > std::vector<camera3_stream_buffer_t> buffers_; > std::vector<std::unique_ptr<libcamera::FrameBuffer>> frameBuffers_; > CameraMetadata settings_; > std::unique_ptr<CaptureRequest> request_; > + > + camera3_capture_result_t captureResult_ = {}; > + Status status_ = Status::Pending; > }; > > enum class State { > @@ -99,12 +110,14 @@ private: > createFrameBuffer(const buffer_handle_t camera3buffer, > libcamera::PixelFormat pixelFormat, > const libcamera::Size &size); > - void abortRequest(camera3_capture_request_t *request); > + void abortRequest(Camera3RequestDescriptor *descriptor, > + camera3_capture_request_t *request); > bool isValidRequest(camera3_capture_request_t *request) const; > void notifyShutter(uint32_t frameNumber, uint64_t timestamp); > void notifyError(uint32_t frameNumber, camera3_stream_t *stream, > camera3_error_msg_code code); > int processControls(Camera3RequestDescriptor *descriptor); > + void sendCaptureResults(); > std::unique_ptr<CameraMetadata> getResultMetadata( > const Camera3RequestDescriptor &descriptor) const; >
Hi Umang, On Wed, Sep 29, 2021 at 6:30 AM Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hi Umang, > > Thank you for the patch. > > On Tue, Sep 28, 2021 at 09:55:36PM +0530, Umang Jain wrote: > > There is a possibility that an out-of-order completion of capture > > request happens by calling process_capture_result() directly on error > > paths. The framework expects that errors should be notified as soon as > > possible, but the request completion order should remain intact. > > An existing instance of this is abortRequest(), which sends the capture > > results on flushing state, without considering order-of-completion. > > > > Since we have a queue of Camera3RequestDescriptor tracking each > > capture request placed by framework to libcamera HAL, we should be only > > sending back capture results from a single location, by inspecting > > the queue. As per the patch, this now happens in > > CameraDevice::sendCaptureResults(). > > > > Each descriptor is now equipped with its own status to denote whether > > the capture request is complete and ready to send back to the framework > > or needs to be waited upon. This ensures that the order of completion is > > respected for the requests. > > > > Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> > > --- > > src/android/camera_device.cpp | 44 +++++++++++++++++++++++++---------- > > src/android/camera_device.h | 15 +++++++++++- > > 2 files changed, 46 insertions(+), 13 deletions(-) > > > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp > > index a3b8a549..83030366 100644 > > --- a/src/android/camera_device.cpp > > +++ b/src/android/camera_device.cpp > > @@ -861,11 +861,12 @@ int CameraDevice::processControls(Camera3RequestDescriptor *descriptor) > > return 0; > > } > > > > -void CameraDevice::abortRequest(camera3_capture_request_t *request) > > +void CameraDevice::abortRequest(Camera3RequestDescriptor *descriptor, > > + camera3_capture_request_t *request) > > I don't think I've seen a reply to my review comment in v1: > > Could this function take a Camera3RequestDescriptor pointer only ? It > should contain all the needed data. This can be done as a patch before > this one if desired, or here as it shouldn't be much extra work. > > This function uses the num_output_buffers, frame_number and > output_buffers members of camera3_capture_request_t. Those are copied to > the Camera3RequestDescriptor buffers_ and frameNumber_ members which you > can use here. > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > { > > notifyError(request->frame_number, nullptr, CAMERA3_MSG_ERROR_REQUEST); > > > > - camera3_capture_result_t result = {}; > > + camera3_capture_result_t &result = descriptor->captureResult_; > > result.num_output_buffers = request->num_output_buffers; > > result.frame_number = request->frame_number; > > result.partial_result = 0; > > @@ -879,7 +880,7 @@ void CameraDevice::abortRequest(camera3_capture_request_t *request) > > } > > result.output_buffers = resultBuffers.data(); > > > > - callbacks_->process_capture_result(callbacks_, &result); > > + descriptor->status_ = Camera3RequestDescriptor::Status::Error; > > } > > > > bool CameraDevice::isValidRequest(camera3_capture_request_t *camera3Request) const > > @@ -1052,13 +1053,19 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques > > return ret; > > > > /* > > - * If flush is in progress abort the request. If the camera has been > > - * stopped we have to re-start it to be able to process the request. > > + * If flush is in progress abort the request and push the descriptor to > > + * the queue. If the camera has been stopped we have to re-start it to > > + * be able to process the request. > > */ > > MutexLocker stateLock(stateMutex_); > > > > if (state_ == State::Flushing) { > > - abortRequest(camera3Request); > > + abortRequest(descriptors_.back().get(), camera3Request); > > + { > > + MutexLocker descriptorsLock(descriptorsMutex_); > > + descriptors_.push(std::move(descriptor)); > > + } Could you tell me what happens here? I think the current request, camear3Request needs to be cancelled. But abortRequest(descriptors_.back().get(), camera3Request) is called before pushing. So it aborts the last pushed request with the current request. I wonder if the correct code is below. if (state_ == State::Flushing) { { MutexLocker descriptorsLock(descriptorsMutex_); descriptors_.push(std::move(descriptor)); abortRequest(descriptors_.back().get(), camera3Request); } sendCaptureResults(); return 0; } > > + sendCaptureResults(); > > return 0; > > } > > > > @@ -1116,7 +1123,7 @@ void CameraDevice::requestComplete(Request *request) > > * The buffer status is set to OK and later changed to ERROR if > > * post-processing/compression fails. > > */ > > - camera3_capture_result_t captureResult = {}; > > + camera3_capture_result_t &captureResult = descriptor->captureResult_; > > captureResult.frame_number = descriptor->frameNumber_; > > captureResult.num_output_buffers = descriptor->buffers_.size(); > > for (camera3_stream_buffer_t &buffer : descriptor->buffers_) { > > @@ -1166,9 +1173,9 @@ void CameraDevice::requestComplete(Request *request) > > buffer.status = CAMERA3_BUFFER_STATUS_ERROR; > > } > > > > - callbacks_->process_capture_result(callbacks_, &captureResult); > > + descriptor->status_ = Camera3RequestDescriptor::Status::Error; > > + sendCaptureResults(); > > > > - descriptors_.pop(); > > return; > > } > > > > @@ -1234,10 +1241,23 @@ void CameraDevice::requestComplete(Request *request) > > } > > > > captureResult.result = resultMetadata->get(); > > - callbacks_->process_capture_result(callbacks_, &captureResult); > > + descriptor->status_ = Camera3RequestDescriptor::Status::Success; > > + sendCaptureResults(); > > +} > > > > - MutexLocker descriptorsLock(descriptorsMutex_); > > - descriptors_.pop(); > > +void CameraDevice::sendCaptureResults() > > +{ > > + MutexLocker lock(descriptorsMutex_); > > + while (!descriptors_.empty() && !descriptors_.front()->isPending()) { > > + std::unique_ptr<Camera3RequestDescriptor> descriptor = > > + std::move(descriptors_.front()); nit: I would use auto here. > > + descriptors_.pop(); > > + > > + lock.unlock(); > > + callbacks_->process_capture_result(callbacks_, > > + &descriptor->captureResult_); > > + lock.lock(); Why is lock released during calling the callback? Is there any deadlock here? I wonder if it might be less efficient to release and re-acquire lock every call than just holding a lock entirely. -Hiro > > + } > > } > > > > std::string CameraDevice::logPrefix() const > > diff --git a/src/android/camera_device.h b/src/android/camera_device.h > > index 9ec510d5..dbfa7431 100644 > > --- a/src/android/camera_device.h > > +++ b/src/android/camera_device.h > > @@ -74,17 +74,28 @@ private: > > CameraDevice(unsigned int id, std::shared_ptr<libcamera::Camera> camera); > > > > struct Camera3RequestDescriptor { > > + enum class Status { > > + Pending, > > + Success, > > + Error, > > + }; > > + > > Camera3RequestDescriptor() = default; > > ~Camera3RequestDescriptor() = default; > > Camera3RequestDescriptor(libcamera::Camera *camera, > > const camera3_capture_request_t *camera3Request); > > Camera3RequestDescriptor &operator=(Camera3RequestDescriptor &&) = default; > > > > + bool isPending() const { return status_ == Status::Pending; } > > + > > uint32_t frameNumber_ = 0; > > std::vector<camera3_stream_buffer_t> buffers_; > > std::vector<std::unique_ptr<libcamera::FrameBuffer>> frameBuffers_; > > CameraMetadata settings_; > > std::unique_ptr<CaptureRequest> request_; > > + > > + camera3_capture_result_t captureResult_ = {}; > > + Status status_ = Status::Pending; > > }; > > > > enum class State { > > @@ -99,12 +110,14 @@ private: > > createFrameBuffer(const buffer_handle_t camera3buffer, > > libcamera::PixelFormat pixelFormat, > > const libcamera::Size &size); > > - void abortRequest(camera3_capture_request_t *request); > > + void abortRequest(Camera3RequestDescriptor *descriptor, > > + camera3_capture_request_t *request); > > bool isValidRequest(camera3_capture_request_t *request) const; > > void notifyShutter(uint32_t frameNumber, uint64_t timestamp); > > void notifyError(uint32_t frameNumber, camera3_stream_t *stream, > > camera3_error_msg_code code); > > int processControls(Camera3RequestDescriptor *descriptor); > > + void sendCaptureResults(); > > std::unique_ptr<CameraMetadata> getResultMetadata( > > const Camera3RequestDescriptor &descriptor) const; > > > > -- > Regards, > > Laurent Pinchart
Hi On 9/29/21 3:00 AM, Laurent Pinchart wrote: > Hi Umang, > > Thank you for the patch. > > On Tue, Sep 28, 2021 at 09:55:36PM +0530, Umang Jain wrote: >> There is a possibility that an out-of-order completion of capture >> request happens by calling process_capture_result() directly on error >> paths. The framework expects that errors should be notified as soon as >> possible, but the request completion order should remain intact. >> An existing instance of this is abortRequest(), which sends the capture >> results on flushing state, without considering order-of-completion. >> >> Since we have a queue of Camera3RequestDescriptor tracking each >> capture request placed by framework to libcamera HAL, we should be only >> sending back capture results from a single location, by inspecting >> the queue. As per the patch, this now happens in >> CameraDevice::sendCaptureResults(). >> >> Each descriptor is now equipped with its own status to denote whether >> the capture request is complete and ready to send back to the framework >> or needs to be waited upon. This ensures that the order of completion is >> respected for the requests. >> >> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> >> --- >> src/android/camera_device.cpp | 44 +++++++++++++++++++++++++---------- >> src/android/camera_device.h | 15 +++++++++++- >> 2 files changed, 46 insertions(+), 13 deletions(-) >> >> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp >> index a3b8a549..83030366 100644 >> --- a/src/android/camera_device.cpp >> +++ b/src/android/camera_device.cpp >> @@ -861,11 +861,12 @@ int CameraDevice::processControls(Camera3RequestDescriptor *descriptor) >> return 0; >> } >> >> -void CameraDevice::abortRequest(camera3_capture_request_t *request) >> +void CameraDevice::abortRequest(Camera3RequestDescriptor *descriptor, >> + camera3_capture_request_t *request) > I don't think I've seen a reply to my review comment in v1: Oh, I think I did reply and had a brief discussion with you about it on IRC. Nevermind, I think I got confused a bit yesterday and now I have addressed the comment locally here as well. Works fine. > > Could this function take a Camera3RequestDescriptor pointer only ? It > should contain all the needed data. This can be done as a patch before > this one if desired, or here as it shouldn't be much extra work. > > This function uses the num_output_buffers, frame_number and > output_buffers members of camera3_capture_request_t. Those are copied to > the Camera3RequestDescriptor buffers_ and frameNumber_ members which you > can use here. > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > >> { >> notifyError(request->frame_number, nullptr, CAMERA3_MSG_ERROR_REQUEST); >> >> - camera3_capture_result_t result = {}; >> + camera3_capture_result_t &result = descriptor->captureResult_; >> result.num_output_buffers = request->num_output_buffers; >> result.frame_number = request->frame_number; >> result.partial_result = 0; >> @@ -879,7 +880,7 @@ void CameraDevice::abortRequest(camera3_capture_request_t *request) >> } >> result.output_buffers = resultBuffers.data(); >> >> - callbacks_->process_capture_result(callbacks_, &result); >> + descriptor->status_ = Camera3RequestDescriptor::Status::Error; >> } >> >> bool CameraDevice::isValidRequest(camera3_capture_request_t *camera3Request) const >> @@ -1052,13 +1053,19 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques >> return ret; >> >> /* >> - * If flush is in progress abort the request. If the camera has been >> - * stopped we have to re-start it to be able to process the request. >> + * If flush is in progress abort the request and push the descriptor to >> + * the queue. If the camera has been stopped we have to re-start it to >> + * be able to process the request. >> */ >> MutexLocker stateLock(stateMutex_); >> >> if (state_ == State::Flushing) { >> - abortRequest(camera3Request); >> + abortRequest(descriptors_.back().get(), camera3Request); >> + { >> + MutexLocker descriptorsLock(descriptorsMutex_); >> + descriptors_.push(std::move(descriptor)); >> + } >> + sendCaptureResults(); >> return 0; >> } >> >> @@ -1116,7 +1123,7 @@ void CameraDevice::requestComplete(Request *request) >> * The buffer status is set to OK and later changed to ERROR if >> * post-processing/compression fails. >> */ >> - camera3_capture_result_t captureResult = {}; >> + camera3_capture_result_t &captureResult = descriptor->captureResult_; >> captureResult.frame_number = descriptor->frameNumber_; >> captureResult.num_output_buffers = descriptor->buffers_.size(); >> for (camera3_stream_buffer_t &buffer : descriptor->buffers_) { >> @@ -1166,9 +1173,9 @@ void CameraDevice::requestComplete(Request *request) >> buffer.status = CAMERA3_BUFFER_STATUS_ERROR; >> } >> >> - callbacks_->process_capture_result(callbacks_, &captureResult); >> + descriptor->status_ = Camera3RequestDescriptor::Status::Error; >> + sendCaptureResults(); >> >> - descriptors_.pop(); >> return; >> } >> >> @@ -1234,10 +1241,23 @@ void CameraDevice::requestComplete(Request *request) >> } >> >> captureResult.result = resultMetadata->get(); >> - callbacks_->process_capture_result(callbacks_, &captureResult); >> + descriptor->status_ = Camera3RequestDescriptor::Status::Success; >> + sendCaptureResults(); >> +} >> >> - MutexLocker descriptorsLock(descriptorsMutex_); >> - descriptors_.pop(); >> +void CameraDevice::sendCaptureResults() >> +{ >> + MutexLocker lock(descriptorsMutex_); >> + while (!descriptors_.empty() && !descriptors_.front()->isPending()) { >> + std::unique_ptr<Camera3RequestDescriptor> descriptor = >> + std::move(descriptors_.front()); >> + descriptors_.pop(); >> + >> + lock.unlock(); >> + callbacks_->process_capture_result(callbacks_, >> + &descriptor->captureResult_); >> + lock.lock(); >> + } >> } >> >> std::string CameraDevice::logPrefix() const >> diff --git a/src/android/camera_device.h b/src/android/camera_device.h >> index 9ec510d5..dbfa7431 100644 >> --- a/src/android/camera_device.h >> +++ b/src/android/camera_device.h >> @@ -74,17 +74,28 @@ private: >> CameraDevice(unsigned int id, std::shared_ptr<libcamera::Camera> camera); >> >> struct Camera3RequestDescriptor { >> + enum class Status { >> + Pending, >> + Success, >> + Error, >> + }; >> + >> Camera3RequestDescriptor() = default; >> ~Camera3RequestDescriptor() = default; >> Camera3RequestDescriptor(libcamera::Camera *camera, >> const camera3_capture_request_t *camera3Request); >> Camera3RequestDescriptor &operator=(Camera3RequestDescriptor &&) = default; >> >> + bool isPending() const { return status_ == Status::Pending; } >> + >> uint32_t frameNumber_ = 0; >> std::vector<camera3_stream_buffer_t> buffers_; >> std::vector<std::unique_ptr<libcamera::FrameBuffer>> frameBuffers_; >> CameraMetadata settings_; >> std::unique_ptr<CaptureRequest> request_; >> + >> + camera3_capture_result_t captureResult_ = {}; >> + Status status_ = Status::Pending; >> }; >> >> enum class State { >> @@ -99,12 +110,14 @@ private: >> createFrameBuffer(const buffer_handle_t camera3buffer, >> libcamera::PixelFormat pixelFormat, >> const libcamera::Size &size); >> - void abortRequest(camera3_capture_request_t *request); >> + void abortRequest(Camera3RequestDescriptor *descriptor, >> + camera3_capture_request_t *request); >> bool isValidRequest(camera3_capture_request_t *request) const; >> void notifyShutter(uint32_t frameNumber, uint64_t timestamp); >> void notifyError(uint32_t frameNumber, camera3_stream_t *stream, >> camera3_error_msg_code code); >> int processControls(Camera3RequestDescriptor *descriptor); >> + void sendCaptureResults(); >> std::unique_ptr<CameraMetadata> getResultMetadata( >> const Camera3RequestDescriptor &descriptor) const; >>
Hi Hiro, On Wed, Sep 29, 2021 at 08:45:24AM +0900, Hirokazu Honda wrote: > On Wed, Sep 29, 2021 at 6:30 AM Laurent Pinchart wrote: > > On Tue, Sep 28, 2021 at 09:55:36PM +0530, Umang Jain wrote: > > > There is a possibility that an out-of-order completion of capture > > > request happens by calling process_capture_result() directly on error > > > paths. The framework expects that errors should be notified as soon as > > > possible, but the request completion order should remain intact. > > > An existing instance of this is abortRequest(), which sends the capture > > > results on flushing state, without considering order-of-completion. > > > > > > Since we have a queue of Camera3RequestDescriptor tracking each > > > capture request placed by framework to libcamera HAL, we should be only > > > sending back capture results from a single location, by inspecting > > > the queue. As per the patch, this now happens in > > > CameraDevice::sendCaptureResults(). > > > > > > Each descriptor is now equipped with its own status to denote whether > > > the capture request is complete and ready to send back to the framework > > > or needs to be waited upon. This ensures that the order of completion is > > > respected for the requests. > > > > > > Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> > > > --- > > > src/android/camera_device.cpp | 44 +++++++++++++++++++++++++---------- > > > src/android/camera_device.h | 15 +++++++++++- > > > 2 files changed, 46 insertions(+), 13 deletions(-) > > > > > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp > > > index a3b8a549..83030366 100644 > > > --- a/src/android/camera_device.cpp > > > +++ b/src/android/camera_device.cpp > > > @@ -861,11 +861,12 @@ int CameraDevice::processControls(Camera3RequestDescriptor *descriptor) > > > return 0; > > > } > > > > > > -void CameraDevice::abortRequest(camera3_capture_request_t *request) > > > +void CameraDevice::abortRequest(Camera3RequestDescriptor *descriptor, > > > + camera3_capture_request_t *request) > > > > I don't think I've seen a reply to my review comment in v1: > > > > Could this function take a Camera3RequestDescriptor pointer only ? It > > should contain all the needed data. This can be done as a patch before > > this one if desired, or here as it shouldn't be much extra work. > > > > This function uses the num_output_buffers, frame_number and > > output_buffers members of camera3_capture_request_t. Those are copied to > > the Camera3RequestDescriptor buffers_ and frameNumber_ members which you > > can use here. > > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > > { > > > notifyError(request->frame_number, nullptr, CAMERA3_MSG_ERROR_REQUEST); > > > > > > - camera3_capture_result_t result = {}; > > > + camera3_capture_result_t &result = descriptor->captureResult_; > > > result.num_output_buffers = request->num_output_buffers; > > > result.frame_number = request->frame_number; > > > result.partial_result = 0; > > > @@ -879,7 +880,7 @@ void CameraDevice::abortRequest(camera3_capture_request_t *request) > > > } > > > result.output_buffers = resultBuffers.data(); > > > > > > - callbacks_->process_capture_result(callbacks_, &result); > > > + descriptor->status_ = Camera3RequestDescriptor::Status::Error; > > > } > > > > > > bool CameraDevice::isValidRequest(camera3_capture_request_t *camera3Request) const > > > @@ -1052,13 +1053,19 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques > > > return ret; > > > > > > /* > > > - * If flush is in progress abort the request. If the camera has been > > > - * stopped we have to re-start it to be able to process the request. > > > + * If flush is in progress abort the request and push the descriptor to > > > + * the queue. If the camera has been stopped we have to re-start it to > > > + * be able to process the request. > > > */ > > > MutexLocker stateLock(stateMutex_); > > > > > > if (state_ == State::Flushing) { > > > - abortRequest(camera3Request); > > > + abortRequest(descriptors_.back().get(), camera3Request); > > > + { > > > + MutexLocker descriptorsLock(descriptorsMutex_); > > > + descriptors_.push(std::move(descriptor)); > > > + } > > Could you tell me what happens here? > I think the current request, camear3Request needs to be cancelled. > But abortRequest(descriptors_.back().get(), camera3Request) is called > before pushing. > So it aborts the last pushed request with the current request. Oops indeed. > I wonder if the correct code is below. > if (state_ == State::Flushing) > { > { > MutexLocker descriptorsLock(descriptorsMutex_); > descriptors_.push(std::move(descriptor)); > abortRequest(descriptors_.back().get(), camera3Request); > } > sendCaptureResults(); > return 0; > } abortRequest() is supposed to operator on the request it receives as a parameter, so I don't think it needs to be covered by the lock. I think abortRequest(descriptor.get(), camera3Request); { MutexLocker descriptorsLock(descriptorsMutex_); descriptors_.push(std::move(descriptor)); } sendCaptureResults(); may be enough to fix the issue. > > > + sendCaptureResults(); > > > return 0; > > > } > > > > > > @@ -1116,7 +1123,7 @@ void CameraDevice::requestComplete(Request *request) > > > * The buffer status is set to OK and later changed to ERROR if > > > * post-processing/compression fails. > > > */ > > > - camera3_capture_result_t captureResult = {}; > > > + camera3_capture_result_t &captureResult = descriptor->captureResult_; > > > captureResult.frame_number = descriptor->frameNumber_; > > > captureResult.num_output_buffers = descriptor->buffers_.size(); > > > for (camera3_stream_buffer_t &buffer : descriptor->buffers_) { > > > @@ -1166,9 +1173,9 @@ void CameraDevice::requestComplete(Request *request) > > > buffer.status = CAMERA3_BUFFER_STATUS_ERROR; > > > } > > > > > > - callbacks_->process_capture_result(callbacks_, &captureResult); > > > + descriptor->status_ = Camera3RequestDescriptor::Status::Error; > > > + sendCaptureResults(); > > > > > > - descriptors_.pop(); > > > return; > > > } > > > > > > @@ -1234,10 +1241,23 @@ void CameraDevice::requestComplete(Request *request) > > > } > > > > > > captureResult.result = resultMetadata->get(); > > > - callbacks_->process_capture_result(callbacks_, &captureResult); > > > + descriptor->status_ = Camera3RequestDescriptor::Status::Success; > > > + sendCaptureResults(); > > > +} > > > > > > - MutexLocker descriptorsLock(descriptorsMutex_); > > > - descriptors_.pop(); > > > +void CameraDevice::sendCaptureResults() > > > +{ > > > + MutexLocker lock(descriptorsMutex_); > > > + while (!descriptors_.empty() && !descriptors_.front()->isPending()) { > > > + std::unique_ptr<Camera3RequestDescriptor> descriptor = > > > + std::move(descriptors_.front()); > > nit: I would use auto here. > > > > + descriptors_.pop(); > > > + > > > + lock.unlock(); > > > + callbacks_->process_capture_result(callbacks_, > > > + &descriptor->captureResult_); > > > + lock.lock(); > > Why is lock released during calling the callback? Is there any deadlock here? The Android camera HAL API doesn't specify this as far as I know, so we decided to minmize lock contention as the default rule. > I wonder if it might be less efficient to release and re-acquire lock > every call than just holding a lock entirely. That's a good question, the only way to know would be to measure performances for both options I suppose. > > > + } > > > } > > > > > > std::string CameraDevice::logPrefix() const > > > diff --git a/src/android/camera_device.h b/src/android/camera_device.h > > > index 9ec510d5..dbfa7431 100644 > > > --- a/src/android/camera_device.h > > > +++ b/src/android/camera_device.h > > > @@ -74,17 +74,28 @@ private: > > > CameraDevice(unsigned int id, std::shared_ptr<libcamera::Camera> camera); > > > > > > struct Camera3RequestDescriptor { > > > + enum class Status { > > > + Pending, > > > + Success, > > > + Error, > > > + }; > > > + > > > Camera3RequestDescriptor() = default; > > > ~Camera3RequestDescriptor() = default; > > > Camera3RequestDescriptor(libcamera::Camera *camera, > > > const camera3_capture_request_t *camera3Request); > > > Camera3RequestDescriptor &operator=(Camera3RequestDescriptor &&) = default; > > > > > > + bool isPending() const { return status_ == Status::Pending; } > > > + > > > uint32_t frameNumber_ = 0; > > > std::vector<camera3_stream_buffer_t> buffers_; > > > std::vector<std::unique_ptr<libcamera::FrameBuffer>> frameBuffers_; > > > CameraMetadata settings_; > > > std::unique_ptr<CaptureRequest> request_; > > > + > > > + camera3_capture_result_t captureResult_ = {}; > > > + Status status_ = Status::Pending; > > > }; > > > > > > enum class State { > > > @@ -99,12 +110,14 @@ private: > > > createFrameBuffer(const buffer_handle_t camera3buffer, > > > libcamera::PixelFormat pixelFormat, > > > const libcamera::Size &size); > > > - void abortRequest(camera3_capture_request_t *request); > > > + void abortRequest(Camera3RequestDescriptor *descriptor, > > > + camera3_capture_request_t *request); > > > bool isValidRequest(camera3_capture_request_t *request) const; > > > void notifyShutter(uint32_t frameNumber, uint64_t timestamp); > > > void notifyError(uint32_t frameNumber, camera3_stream_t *stream, > > > camera3_error_msg_code code); > > > int processControls(Camera3RequestDescriptor *descriptor); > > > + void sendCaptureResults(); > > > std::unique_ptr<CameraMetadata> getResultMetadata( > > > const Camera3RequestDescriptor &descriptor) const; > > >
Hello, On 9/29/21 12:40 PM, Laurent Pinchart wrote: > Hi Hiro, > > On Wed, Sep 29, 2021 at 08:45:24AM +0900, Hirokazu Honda wrote: >> On Wed, Sep 29, 2021 at 6:30 AM Laurent Pinchart wrote: >>> On Tue, Sep 28, 2021 at 09:55:36PM +0530, Umang Jain wrote: >>>> There is a possibility that an out-of-order completion of capture >>>> request happens by calling process_capture_result() directly on error >>>> paths. The framework expects that errors should be notified as soon as >>>> possible, but the request completion order should remain intact. >>>> An existing instance of this is abortRequest(), which sends the capture >>>> results on flushing state, without considering order-of-completion. >>>> >>>> Since we have a queue of Camera3RequestDescriptor tracking each >>>> capture request placed by framework to libcamera HAL, we should be only >>>> sending back capture results from a single location, by inspecting >>>> the queue. As per the patch, this now happens in >>>> CameraDevice::sendCaptureResults(). >>>> >>>> Each descriptor is now equipped with its own status to denote whether >>>> the capture request is complete and ready to send back to the framework >>>> or needs to be waited upon. This ensures that the order of completion is >>>> respected for the requests. >>>> >>>> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> >>>> --- >>>> src/android/camera_device.cpp | 44 +++++++++++++++++++++++++---------- >>>> src/android/camera_device.h | 15 +++++++++++- >>>> 2 files changed, 46 insertions(+), 13 deletions(-) >>>> >>>> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp >>>> index a3b8a549..83030366 100644 >>>> --- a/src/android/camera_device.cpp >>>> +++ b/src/android/camera_device.cpp >>>> @@ -861,11 +861,12 @@ int CameraDevice::processControls(Camera3RequestDescriptor *descriptor) >>>> return 0; >>>> } >>>> >>>> -void CameraDevice::abortRequest(camera3_capture_request_t *request) >>>> +void CameraDevice::abortRequest(Camera3RequestDescriptor *descriptor, >>>> + camera3_capture_request_t *request) >>> I don't think I've seen a reply to my review comment in v1: >>> >>> Could this function take a Camera3RequestDescriptor pointer only ? It >>> should contain all the needed data. This can be done as a patch before >>> this one if desired, or here as it shouldn't be much extra work. >>> >>> This function uses the num_output_buffers, frame_number and >>> output_buffers members of camera3_capture_request_t. Those are copied to >>> the Camera3RequestDescriptor buffers_ and frameNumber_ members which you >>> can use here. >>> >>> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> >>> >>>> { >>>> notifyError(request->frame_number, nullptr, CAMERA3_MSG_ERROR_REQUEST); >>>> >>>> - camera3_capture_result_t result = {}; >>>> + camera3_capture_result_t &result = descriptor->captureResult_; >>>> result.num_output_buffers = request->num_output_buffers; >>>> result.frame_number = request->frame_number; >>>> result.partial_result = 0; >>>> @@ -879,7 +880,7 @@ void CameraDevice::abortRequest(camera3_capture_request_t *request) >>>> } >>>> result.output_buffers = resultBuffers.data(); >>>> >>>> - callbacks_->process_capture_result(callbacks_, &result); >>>> + descriptor->status_ = Camera3RequestDescriptor::Status::Error; >>>> } >>>> >>>> bool CameraDevice::isValidRequest(camera3_capture_request_t *camera3Request) const >>>> @@ -1052,13 +1053,19 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques >>>> return ret; >>>> >>>> /* >>>> - * If flush is in progress abort the request. If the camera has been >>>> - * stopped we have to re-start it to be able to process the request. >>>> + * If flush is in progress abort the request and push the descriptor to >>>> + * the queue. If the camera has been stopped we have to re-start it to >>>> + * be able to process the request. >>>> */ >>>> MutexLocker stateLock(stateMutex_); >>>> >>>> if (state_ == State::Flushing) { >>>> - abortRequest(camera3Request); >>>> + abortRequest(descriptors_.back().get(), camera3Request); >>>> + { >>>> + MutexLocker descriptorsLock(descriptorsMutex_); >>>> + descriptors_.push(std::move(descriptor)); >>>> + } >> Could you tell me what happens here? >> I think the current request, camear3Request needs to be cancelled. >> But abortRequest(descriptors_.back().get(), camera3Request) is called >> before pushing. >> So it aborts the last pushed request with the current request. > Oops indeed. > >> I wonder if the correct code is below. >> if (state_ == State::Flushing) >> { >> { >> MutexLocker descriptorsLock(descriptorsMutex_); >> descriptors_.push(std::move(descriptor)); >> abortRequest(descriptors_.back().get(), camera3Request); >> } >> sendCaptureResults(); >> return 0; >> } > abortRequest() is supposed to operator on the request it receives as a > parameter, so I don't think it needs to be covered by the lock. I think > > abortRequest(descriptor.get(), camera3Request); > { > MutexLocker descriptorsLock(descriptorsMutex_); > descriptors_.push(std::move(descriptor)); > } > sendCaptureResults(); > > may be enough to fix the issue. Yep, addressed in v3, thanks for pointing it out > >>>> + sendCaptureResults(); >>>> return 0; >>>> } >>>> >>>> @@ -1116,7 +1123,7 @@ void CameraDevice::requestComplete(Request *request) >>>> * The buffer status is set to OK and later changed to ERROR if >>>> * post-processing/compression fails. >>>> */ >>>> - camera3_capture_result_t captureResult = {}; >>>> + camera3_capture_result_t &captureResult = descriptor->captureResult_; >>>> captureResult.frame_number = descriptor->frameNumber_; >>>> captureResult.num_output_buffers = descriptor->buffers_.size(); >>>> for (camera3_stream_buffer_t &buffer : descriptor->buffers_) { >>>> @@ -1166,9 +1173,9 @@ void CameraDevice::requestComplete(Request *request) >>>> buffer.status = CAMERA3_BUFFER_STATUS_ERROR; >>>> } >>>> >>>> - callbacks_->process_capture_result(callbacks_, &captureResult); >>>> + descriptor->status_ = Camera3RequestDescriptor::Status::Error; >>>> + sendCaptureResults(); >>>> >>>> - descriptors_.pop(); >>>> return; >>>> } >>>> >>>> @@ -1234,10 +1241,23 @@ void CameraDevice::requestComplete(Request *request) >>>> } >>>> >>>> captureResult.result = resultMetadata->get(); >>>> - callbacks_->process_capture_result(callbacks_, &captureResult); >>>> + descriptor->status_ = Camera3RequestDescriptor::Status::Success; >>>> + sendCaptureResults(); >>>> +} >>>> >>>> - MutexLocker descriptorsLock(descriptorsMutex_); >>>> - descriptors_.pop(); >>>> +void CameraDevice::sendCaptureResults() >>>> +{ >>>> + MutexLocker lock(descriptorsMutex_); >>>> + while (!descriptors_.empty() && !descriptors_.front()->isPending()) { >>>> + std::unique_ptr<Camera3RequestDescriptor> descriptor = >>>> + std::move(descriptors_.front()); >> nit: I would use auto here. >> >>>> + descriptors_.pop(); >>>> + >>>> + lock.unlock(); >>>> + callbacks_->process_capture_result(callbacks_, >>>> + &descriptor->captureResult_); >>>> + lock.lock(); >> Why is lock released during calling the callback? Is there any deadlock here? > The Android camera HAL API doesn't specify this as far as I know, so we > decided to minmize lock contention as the default rule. > >> I wonder if it might be less efficient to release and re-acquire lock >> every call than just holding a lock entirely. > That's a good question, the only way to know would be to measure > performances for both options I suppose. The process_capture_resultperformance is specified as: * This is a non-blocking call. The framework will return this call in 5ms. * * The pipeline latency (see S7 for definition) should be less than or equal to * 4 frame intervals, and must be less than or equal to 8 frame intervals. https://android.googlesource.com/platform/hardware/libhardware/+/master/include/hardware/camera3.h#2781 I don't think bring callback under the lock would have much impact on the performance, fps-wise? > >>>> + } >>>> } >>>> >>>> std::string CameraDevice::logPrefix() const >>>> diff --git a/src/android/camera_device.h b/src/android/camera_device.h >>>> index 9ec510d5..dbfa7431 100644 >>>> --- a/src/android/camera_device.h >>>> +++ b/src/android/camera_device.h >>>> @@ -74,17 +74,28 @@ private: >>>> CameraDevice(unsigned int id, std::shared_ptr<libcamera::Camera> camera); >>>> >>>> struct Camera3RequestDescriptor { >>>> + enum class Status { >>>> + Pending, >>>> + Success, >>>> + Error, >>>> + }; >>>> + >>>> Camera3RequestDescriptor() = default; >>>> ~Camera3RequestDescriptor() = default; >>>> Camera3RequestDescriptor(libcamera::Camera *camera, >>>> const camera3_capture_request_t *camera3Request); >>>> Camera3RequestDescriptor &operator=(Camera3RequestDescriptor &&) = default; >>>> >>>> + bool isPending() const { return status_ == Status::Pending; } >>>> + >>>> uint32_t frameNumber_ = 0; >>>> std::vector<camera3_stream_buffer_t> buffers_; >>>> std::vector<std::unique_ptr<libcamera::FrameBuffer>> frameBuffers_; >>>> CameraMetadata settings_; >>>> std::unique_ptr<CaptureRequest> request_; >>>> + >>>> + camera3_capture_result_t captureResult_ = {}; >>>> + Status status_ = Status::Pending; >>>> }; >>>> >>>> enum class State { >>>> @@ -99,12 +110,14 @@ private: >>>> createFrameBuffer(const buffer_handle_t camera3buffer, >>>> libcamera::PixelFormat pixelFormat, >>>> const libcamera::Size &size); >>>> - void abortRequest(camera3_capture_request_t *request); >>>> + void abortRequest(Camera3RequestDescriptor *descriptor, >>>> + camera3_capture_request_t *request); >>>> bool isValidRequest(camera3_capture_request_t *request) const; >>>> void notifyShutter(uint32_t frameNumber, uint64_t timestamp); >>>> void notifyError(uint32_t frameNumber, camera3_stream_t *stream, >>>> camera3_error_msg_code code); >>>> int processControls(Camera3RequestDescriptor *descriptor); >>>> + void sendCaptureResults(); >>>> std::unique_ptr<CameraMetadata> getResultMetadata( >>>> const Camera3RequestDescriptor &descriptor) const; >>>>
diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp index a3b8a549..83030366 100644 --- a/src/android/camera_device.cpp +++ b/src/android/camera_device.cpp @@ -861,11 +861,12 @@ int CameraDevice::processControls(Camera3RequestDescriptor *descriptor) return 0; } -void CameraDevice::abortRequest(camera3_capture_request_t *request) +void CameraDevice::abortRequest(Camera3RequestDescriptor *descriptor, + camera3_capture_request_t *request) { notifyError(request->frame_number, nullptr, CAMERA3_MSG_ERROR_REQUEST); - camera3_capture_result_t result = {}; + camera3_capture_result_t &result = descriptor->captureResult_; result.num_output_buffers = request->num_output_buffers; result.frame_number = request->frame_number; result.partial_result = 0; @@ -879,7 +880,7 @@ void CameraDevice::abortRequest(camera3_capture_request_t *request) } result.output_buffers = resultBuffers.data(); - callbacks_->process_capture_result(callbacks_, &result); + descriptor->status_ = Camera3RequestDescriptor::Status::Error; } bool CameraDevice::isValidRequest(camera3_capture_request_t *camera3Request) const @@ -1052,13 +1053,19 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques return ret; /* - * If flush is in progress abort the request. If the camera has been - * stopped we have to re-start it to be able to process the request. + * If flush is in progress abort the request and push the descriptor to + * the queue. If the camera has been stopped we have to re-start it to + * be able to process the request. */ MutexLocker stateLock(stateMutex_); if (state_ == State::Flushing) { - abortRequest(camera3Request); + abortRequest(descriptors_.back().get(), camera3Request); + { + MutexLocker descriptorsLock(descriptorsMutex_); + descriptors_.push(std::move(descriptor)); + } + sendCaptureResults(); return 0; } @@ -1116,7 +1123,7 @@ void CameraDevice::requestComplete(Request *request) * The buffer status is set to OK and later changed to ERROR if * post-processing/compression fails. */ - camera3_capture_result_t captureResult = {}; + camera3_capture_result_t &captureResult = descriptor->captureResult_; captureResult.frame_number = descriptor->frameNumber_; captureResult.num_output_buffers = descriptor->buffers_.size(); for (camera3_stream_buffer_t &buffer : descriptor->buffers_) { @@ -1166,9 +1173,9 @@ void CameraDevice::requestComplete(Request *request) buffer.status = CAMERA3_BUFFER_STATUS_ERROR; } - callbacks_->process_capture_result(callbacks_, &captureResult); + descriptor->status_ = Camera3RequestDescriptor::Status::Error; + sendCaptureResults(); - descriptors_.pop(); return; } @@ -1234,10 +1241,23 @@ void CameraDevice::requestComplete(Request *request) } captureResult.result = resultMetadata->get(); - callbacks_->process_capture_result(callbacks_, &captureResult); + descriptor->status_ = Camera3RequestDescriptor::Status::Success; + sendCaptureResults(); +} - MutexLocker descriptorsLock(descriptorsMutex_); - descriptors_.pop(); +void CameraDevice::sendCaptureResults() +{ + MutexLocker lock(descriptorsMutex_); + while (!descriptors_.empty() && !descriptors_.front()->isPending()) { + std::unique_ptr<Camera3RequestDescriptor> descriptor = + std::move(descriptors_.front()); + descriptors_.pop(); + + lock.unlock(); + callbacks_->process_capture_result(callbacks_, + &descriptor->captureResult_); + lock.lock(); + } } std::string CameraDevice::logPrefix() const diff --git a/src/android/camera_device.h b/src/android/camera_device.h index 9ec510d5..dbfa7431 100644 --- a/src/android/camera_device.h +++ b/src/android/camera_device.h @@ -74,17 +74,28 @@ private: CameraDevice(unsigned int id, std::shared_ptr<libcamera::Camera> camera); struct Camera3RequestDescriptor { + enum class Status { + Pending, + Success, + Error, + }; + Camera3RequestDescriptor() = default; ~Camera3RequestDescriptor() = default; Camera3RequestDescriptor(libcamera::Camera *camera, const camera3_capture_request_t *camera3Request); Camera3RequestDescriptor &operator=(Camera3RequestDescriptor &&) = default; + bool isPending() const { return status_ == Status::Pending; } + uint32_t frameNumber_ = 0; std::vector<camera3_stream_buffer_t> buffers_; std::vector<std::unique_ptr<libcamera::FrameBuffer>> frameBuffers_; CameraMetadata settings_; std::unique_ptr<CaptureRequest> request_; + + camera3_capture_result_t captureResult_ = {}; + Status status_ = Status::Pending; }; enum class State { @@ -99,12 +110,14 @@ private: createFrameBuffer(const buffer_handle_t camera3buffer, libcamera::PixelFormat pixelFormat, const libcamera::Size &size); - void abortRequest(camera3_capture_request_t *request); + void abortRequest(Camera3RequestDescriptor *descriptor, + camera3_capture_request_t *request); bool isValidRequest(camera3_capture_request_t *request) const; void notifyShutter(uint32_t frameNumber, uint64_t timestamp); void notifyError(uint32_t frameNumber, camera3_stream_t *stream, camera3_error_msg_code code); int processControls(Camera3RequestDescriptor *descriptor); + void sendCaptureResults(); std::unique_ptr<CameraMetadata> getResultMetadata( const Camera3RequestDescriptor &descriptor) const;
There is a possibility that an out-of-order completion of capture request happens by calling process_capture_result() directly on error paths. The framework expects that errors should be notified as soon as possible, but the request completion order should remain intact. An existing instance of this is abortRequest(), which sends the capture results on flushing state, without considering order-of-completion. Since we have a queue of Camera3RequestDescriptor tracking each capture request placed by framework to libcamera HAL, we should be only sending back capture results from a single location, by inspecting the queue. As per the patch, this now happens in CameraDevice::sendCaptureResults(). Each descriptor is now equipped with its own status to denote whether the capture request is complete and ready to send back to the framework or needs to be waited upon. This ensures that the order of completion is respected for the requests. Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> --- src/android/camera_device.cpp | 44 +++++++++++++++++++++++++---------- src/android/camera_device.h | 15 +++++++++++- 2 files changed, 46 insertions(+), 13 deletions(-)