| Message ID | 20210929071708.299946-4-umang.jain@ideasonboard.com |
|---|---|
| State | Superseded |
| Delegated to: | Umang Jain |
| Headers | show |
| Series |
|
| Related | show |
Hi Umang, thank you for the patch. On Wed, Sep 29, 2021 at 4:17 PM Umang Jain <umang.jain@ideasonboard.com> 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. > > Since we are fixing out-of-order request completion in abortRequest(), > change the function to read from the Camera3RequestDescriptor directly, > instead of camera3_capture_request_t. The descriptor should have all the > information necessary to set the request buffers' state to error. > > Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > src/android/camera_device.cpp | 52 +++++++++++++++++++++++------------ > src/android/camera_device.h | 14 +++++++++- > 2 files changed, 48 insertions(+), 18 deletions(-) > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp > index 499baec8..fab5a854 100644 > --- a/src/android/camera_device.cpp > +++ b/src/android/camera_device.cpp > @@ -860,25 +860,25 @@ int CameraDevice::processControls(Camera3RequestDescriptor *descriptor) > return 0; > } > > -void CameraDevice::abortRequest(camera3_capture_request_t *request) > +void CameraDevice::abortRequest(Camera3RequestDescriptor *descriptor) > { > - notifyError(request->frame_number, nullptr, CAMERA3_MSG_ERROR_REQUEST); > + notifyError(descriptor->frameNumber_, nullptr, CAMERA3_MSG_ERROR_REQUEST); > > - camera3_capture_result_t result = {}; > - result.num_output_buffers = request->num_output_buffers; > - result.frame_number = request->frame_number; > + camera3_capture_result_t &result = descriptor->captureResult_; > + result.num_output_buffers = descriptor->buffers_.size(); > + result.frame_number = descriptor->frameNumber_; > result.partial_result = 0; > > std::vector<camera3_stream_buffer_t> resultBuffers(result.num_output_buffers); > for (auto [i, buffer] : utils::enumerate(resultBuffers)) { > - buffer = request->output_buffers[i]; > - buffer.release_fence = request->output_buffers[i].acquire_fence; > + buffer = descriptor->buffers_[i]; > + buffer.release_fence = descriptor->buffers_[i].acquire_fence; > buffer.acquire_fence = -1; > buffer.status = CAMERA3_BUFFER_STATUS_ERROR; > } > result.output_buffers = resultBuffers.data(); > > - callbacks_->process_capture_result(callbacks_, &result); > + descriptor->status_ = Camera3RequestDescriptor::Status::Error; > } Looks like abortRequest() does no longer touch any member variable. I would at least makes this const or aggressively static const or move to anonymous space. > > bool CameraDevice::isValidRequest(camera3_capture_request_t *camera3Request) const > @@ -1051,13 +1051,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(descriptor.get()); > + { > + MutexLocker descriptorsLock(descriptorsMutex_); > + descriptors_.push(std::move(descriptor)); > + } > + sendCaptureResults(); > return 0; > } > > @@ -1115,7 +1121,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_) { > @@ -1165,9 +1171,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; > } > > @@ -1233,10 +1239,22 @@ 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()) { > + auto descriptor = std::move(descriptors_.front()); > + descriptors_.pop(); > + Can you leave a TODO about trying to measure the performance coarse vs. grained lock? > + 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..69c78f08 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; side note: Since we don't do this map, I think this default constructor can be removed. With these suggestions, Reviewed-by: Hirokazu Honda <hiroh@chromium.org> -Hiro > 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,13 @@ 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); > 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; > > -- > 2.31.0 >
Hi Umang, On Wed, Sep 29, 2021 at 12:47:08PM +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. > > Since we are fixing out-of-order request completion in abortRequest(), > change the function to read from the Camera3RequestDescriptor directly, > instead of camera3_capture_request_t. The descriptor should have all the > information necessary to set the request buffers' state to error. > > Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > src/android/camera_device.cpp | 52 +++++++++++++++++++++++------------ > src/android/camera_device.h | 14 +++++++++- > 2 files changed, 48 insertions(+), 18 deletions(-) > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp > index 499baec8..fab5a854 100644 > --- a/src/android/camera_device.cpp > +++ b/src/android/camera_device.cpp > @@ -860,25 +860,25 @@ int CameraDevice::processControls(Camera3RequestDescriptor *descriptor) > return 0; > } > > -void CameraDevice::abortRequest(camera3_capture_request_t *request) > +void CameraDevice::abortRequest(Camera3RequestDescriptor *descriptor) > { > - notifyError(request->frame_number, nullptr, CAMERA3_MSG_ERROR_REQUEST); > + notifyError(descriptor->frameNumber_, nullptr, CAMERA3_MSG_ERROR_REQUEST); > > - camera3_capture_result_t result = {}; > - result.num_output_buffers = request->num_output_buffers; > - result.frame_number = request->frame_number; > + camera3_capture_result_t &result = descriptor->captureResult_; > + result.num_output_buffers = descriptor->buffers_.size(); > + result.frame_number = descriptor->frameNumber_; > result.partial_result = 0; > > std::vector<camera3_stream_buffer_t> resultBuffers(result.num_output_buffers); > for (auto [i, buffer] : utils::enumerate(resultBuffers)) { > - buffer = request->output_buffers[i]; > - buffer.release_fence = request->output_buffers[i].acquire_fence; > + buffer = descriptor->buffers_[i]; > + buffer.release_fence = descriptor->buffers_[i].acquire_fence; > buffer.acquire_fence = -1; > buffer.status = CAMERA3_BUFFER_STATUS_ERROR; > } > 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 > @@ -1051,13 +1051,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 I would * If flush is in progress set the request status to error and place it * on the queue to be later completed. 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(descriptor.get()); > + { > + MutexLocker descriptorsLock(descriptorsMutex_); > + descriptors_.push(std::move(descriptor)); > + } > + sendCaptureResults(); Empty lines around sendCaptureResults() maybe to give the code some more room to breath > return 0; > } > > @@ -1115,7 +1121,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_) { > @@ -1165,9 +1171,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; > } > > @@ -1233,10 +1239,22 @@ void CameraDevice::requestComplete(Request *request) > } > > captureResult.result = resultMetadata->get(); This now becomes a problem. Before this patch we called back the framework right here, and we were guaranteed resultMetadata was still in scope. During the callback the framework copied the result metadata and when process_capture_result() returned captureResult went out of scope and got released. Now that a request sits in the queue and can be completed at a later time, when it is passed to the framework captureResult.result points to a memory location that has been invalidated because the here declared unique_ptr<> has gone out of scope. I think we should make just store a std::unique_ptr<CameraMetadata> resultMetadata; in the Camera3RequestDescriptor and set captureResult.result before calling the process_capture_result callback. Thanks j > - 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()) { > + auto 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..69c78f08 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,13 @@ 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); > 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; > > -- > 2.31.0 >
Hi Jacopo, On 9/29/21 1:47 PM, Jacopo Mondi wrote: > Hi Umang, > > On Wed, Sep 29, 2021 at 12:47:08PM +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. >> >> Since we are fixing out-of-order request completion in abortRequest(), >> change the function to read from the Camera3RequestDescriptor directly, >> instead of camera3_capture_request_t. The descriptor should have all the >> information necessary to set the request buffers' state to error. >> >> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> >> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> >> --- >> src/android/camera_device.cpp | 52 +++++++++++++++++++++++------------ >> src/android/camera_device.h | 14 +++++++++- >> 2 files changed, 48 insertions(+), 18 deletions(-) >> >> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp >> index 499baec8..fab5a854 100644 >> --- a/src/android/camera_device.cpp >> +++ b/src/android/camera_device.cpp >> @@ -860,25 +860,25 @@ int CameraDevice::processControls(Camera3RequestDescriptor *descriptor) >> return 0; >> } >> >> -void CameraDevice::abortRequest(camera3_capture_request_t *request) >> +void CameraDevice::abortRequest(Camera3RequestDescriptor *descriptor) >> { >> - notifyError(request->frame_number, nullptr, CAMERA3_MSG_ERROR_REQUEST); >> + notifyError(descriptor->frameNumber_, nullptr, CAMERA3_MSG_ERROR_REQUEST); >> >> - camera3_capture_result_t result = {}; >> - result.num_output_buffers = request->num_output_buffers; >> - result.frame_number = request->frame_number; >> + camera3_capture_result_t &result = descriptor->captureResult_; >> + result.num_output_buffers = descriptor->buffers_.size(); >> + result.frame_number = descriptor->frameNumber_; >> result.partial_result = 0; >> >> std::vector<camera3_stream_buffer_t> resultBuffers(result.num_output_buffers); >> for (auto [i, buffer] : utils::enumerate(resultBuffers)) { >> - buffer = request->output_buffers[i]; >> - buffer.release_fence = request->output_buffers[i].acquire_fence; >> + buffer = descriptor->buffers_[i]; >> + buffer.release_fence = descriptor->buffers_[i].acquire_fence; >> buffer.acquire_fence = -1; >> buffer.status = CAMERA3_BUFFER_STATUS_ERROR; >> } >> 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 >> @@ -1051,13 +1051,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 > I would > > * If flush is in progress set the request status to error and place it > * on the queue to be later completed. 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(descriptor.get()); >> + { >> + MutexLocker descriptorsLock(descriptorsMutex_); >> + descriptors_.push(std::move(descriptor)); >> + } >> + sendCaptureResults(); > Empty lines around sendCaptureResults() maybe to give the code some > more room to breath > >> return 0; >> } >> >> @@ -1115,7 +1121,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_) { >> @@ -1165,9 +1171,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; >> } >> >> @@ -1233,10 +1239,22 @@ void CameraDevice::requestComplete(Request *request) >> } >> >> captureResult.result = resultMetadata->get(); > This now becomes a problem. Before this patch we called back the > framework right here, and we were guaranteed resultMetadata was still > in scope. During the callback the framework copied the result metadata > and when process_capture_result() returned captureResult went out > of scope and got released. > > Now that a request sits in the queue and can be completed at a later > time, when it is passed to the framework captureResult.result points I have considered this. Please look at the sendCaptureResults() right after we set the state to ::Success The "at a later time" scenario, according to me, emerges when something is asynchronous, making a descriptor block while other ::Success descriptor queued. Currently everything is synchronous and every descriptor is synchronously processed as soon as we set a state on it. So, the resultMetdata should be valid until sendCaptureResults() returns. Hence, we should be handling this in upcoming post-processing series rather than this? Does it make sense? > to a memory location that has been invalidated because the here > declared unique_ptr<> has gone out of scope. > > I think we should make just store a > std::unique_ptr<CameraMetadata> resultMetadata; > in the Camera3RequestDescriptor and set captureResult.result before > calling the process_capture_result callback. Yep, that's what should be happening as here: https://patchwork.libcamera.org/patch/13869/ > > Thanks > j > >> - 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()) { >> + auto 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..69c78f08 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,13 @@ 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); >> 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; >> >> -- >> 2.31.0 >>
Hi Umang, On Wed, Sep 29, 2021 at 02:04:50PM +0530, Umang Jain wrote: > Hi Jacopo, > > On 9/29/21 1:47 PM, Jacopo Mondi wrote: > > Hi Umang, > > > > On Wed, Sep 29, 2021 at 12:47:08PM +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. > > > > > > Since we are fixing out-of-order request completion in abortRequest(), > > > change the function to read from the Camera3RequestDescriptor directly, > > > instead of camera3_capture_request_t. The descriptor should have all the > > > information necessary to set the request buffers' state to error. > > > > > > Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > --- > > > src/android/camera_device.cpp | 52 +++++++++++++++++++++++------------ > > > src/android/camera_device.h | 14 +++++++++- > > > 2 files changed, 48 insertions(+), 18 deletions(-) > > > > > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp > > > index 499baec8..fab5a854 100644 > > > --- a/src/android/camera_device.cpp > > > +++ b/src/android/camera_device.cpp > > > @@ -860,25 +860,25 @@ int CameraDevice::processControls(Camera3RequestDescriptor *descriptor) > > > return 0; > > > } > > > > > > -void CameraDevice::abortRequest(camera3_capture_request_t *request) > > > +void CameraDevice::abortRequest(Camera3RequestDescriptor *descriptor) > > > { > > > - notifyError(request->frame_number, nullptr, CAMERA3_MSG_ERROR_REQUEST); > > > + notifyError(descriptor->frameNumber_, nullptr, CAMERA3_MSG_ERROR_REQUEST); > > > > > > - camera3_capture_result_t result = {}; > > > - result.num_output_buffers = request->num_output_buffers; > > > - result.frame_number = request->frame_number; > > > + camera3_capture_result_t &result = descriptor->captureResult_; > > > + result.num_output_buffers = descriptor->buffers_.size(); > > > + result.frame_number = descriptor->frameNumber_; > > > result.partial_result = 0; > > > > > > std::vector<camera3_stream_buffer_t> resultBuffers(result.num_output_buffers); > > > for (auto [i, buffer] : utils::enumerate(resultBuffers)) { > > > - buffer = request->output_buffers[i]; > > > - buffer.release_fence = request->output_buffers[i].acquire_fence; > > > + buffer = descriptor->buffers_[i]; > > > + buffer.release_fence = descriptor->buffers_[i].acquire_fence; > > > buffer.acquire_fence = -1; > > > buffer.status = CAMERA3_BUFFER_STATUS_ERROR; > > > } > > > 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 > > > @@ -1051,13 +1051,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 > > I would > > > > * If flush is in progress set the request status to error and place it > > * on the queue to be later completed. 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(descriptor.get()); > > > + { > > > + MutexLocker descriptorsLock(descriptorsMutex_); > > > + descriptors_.push(std::move(descriptor)); > > > + } > > > + sendCaptureResults(); > > Empty lines around sendCaptureResults() maybe to give the code some > > more room to breath > > > > > return 0; > > > } > > > > > > @@ -1115,7 +1121,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_) { > > > @@ -1165,9 +1171,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; > > > } > > > > > > @@ -1233,10 +1239,22 @@ void CameraDevice::requestComplete(Request *request) > > > } > > > > > > captureResult.result = resultMetadata->get(); > > This now becomes a problem. Before this patch we called back the > > framework right here, and we were guaranteed resultMetadata was still > > in scope. During the callback the framework copied the result metadata > > and when process_capture_result() returned captureResult went out > > of scope and got released. > > > > Now that a request sits in the queue and can be completed at a later > > time, when it is passed to the framework captureResult.result points > > > I have considered this. Please look at the sendCaptureResults() right after > we set the state to ::Success > > The "at a later time" scenario, according to me, emerges when something is > asynchronous, making a descriptor block while other ::Success descriptor > queued. Currently everything is synchronous and every descriptor is > synchronously processed as soon as we set a state on it. So, the > resultMetdata should be valid until sendCaptureResults() returns. > > Hence, we should be handling this in upcoming post-processing series rather > than this? Does it make sense? Indeed it does, sorry I was thinking already in the async post-processing mode :) As long as this is under your radar when moving the post-processing to be async, it's all good! Thanks j > > > to a memory location that has been invalidated because the here > > declared unique_ptr<> has gone out of scope. > > > > I think we should make just store a > > std::unique_ptr<CameraMetadata> resultMetadata; > > in the Camera3RequestDescriptor and set captureResult.result before > > calling the process_capture_result callback. > > > Yep, that's what should be happening as here: > > https://patchwork.libcamera.org/patch/13869/ > > > > > Thanks > > j > > > > > - 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()) { > > > + auto 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..69c78f08 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,13 @@ 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); > > > 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; > > > > > > -- > > > 2.31.0 > > >
Hi Hiro On 9/29/21 12:59 PM, Hirokazu Honda wrote: > Hi Umang, thank you for the patch. > > On Wed, Sep 29, 2021 at 4:17 PM Umang Jain <umang.jain@ideasonboard.com> 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. >> >> Since we are fixing out-of-order request completion in abortRequest(), >> change the function to read from the Camera3RequestDescriptor directly, >> instead of camera3_capture_request_t. The descriptor should have all the >> information necessary to set the request buffers' state to error. >> >> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> >> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> >> --- >> src/android/camera_device.cpp | 52 +++++++++++++++++++++++------------ >> src/android/camera_device.h | 14 +++++++++- >> 2 files changed, 48 insertions(+), 18 deletions(-) >> >> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp >> index 499baec8..fab5a854 100644 >> --- a/src/android/camera_device.cpp >> +++ b/src/android/camera_device.cpp >> @@ -860,25 +860,25 @@ int CameraDevice::processControls(Camera3RequestDescriptor *descriptor) >> return 0; >> } >> >> -void CameraDevice::abortRequest(camera3_capture_request_t *request) >> +void CameraDevice::abortRequest(Camera3RequestDescriptor *descriptor) >> { >> - notifyError(request->frame_number, nullptr, CAMERA3_MSG_ERROR_REQUEST); >> + notifyError(descriptor->frameNumber_, nullptr, CAMERA3_MSG_ERROR_REQUEST); >> >> - camera3_capture_result_t result = {}; >> - result.num_output_buffers = request->num_output_buffers; >> - result.frame_number = request->frame_number; >> + camera3_capture_result_t &result = descriptor->captureResult_; >> + result.num_output_buffers = descriptor->buffers_.size(); >> + result.frame_number = descriptor->frameNumber_; >> result.partial_result = 0; >> >> std::vector<camera3_stream_buffer_t> resultBuffers(result.num_output_buffers); >> for (auto [i, buffer] : utils::enumerate(resultBuffers)) { >> - buffer = request->output_buffers[i]; >> - buffer.release_fence = request->output_buffers[i].acquire_fence; >> + buffer = descriptor->buffers_[i]; >> + buffer.release_fence = descriptor->buffers_[i].acquire_fence; >> buffer.acquire_fence = -1; >> buffer.status = CAMERA3_BUFFER_STATUS_ERROR; >> } >> result.output_buffers = resultBuffers.data(); >> >> - callbacks_->process_capture_result(callbacks_, &result); >> + descriptor->status_ = Camera3RequestDescriptor::Status::Error; >> } > Looks like abortRequest() does no longer touch any member variable. I > would at least makes this const or aggressively static const or move > to anonymous space. I don't think it should go to anonymous space. I would opt for const, with making notifyError() as const as well, otherwise it will complain. Possible candidate for separate patch? >> bool CameraDevice::isValidRequest(camera3_capture_request_t *camera3Request) const >> @@ -1051,13 +1051,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(descriptor.get()); >> + { >> + MutexLocker descriptorsLock(descriptorsMutex_); >> + descriptors_.push(std::move(descriptor)); >> + } >> + sendCaptureResults(); >> return 0; >> } >> >> @@ -1115,7 +1121,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_) { >> @@ -1165,9 +1171,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; >> } >> >> @@ -1233,10 +1239,22 @@ 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()) { >> + auto descriptor = std::move(descriptors_.front()); >> + descriptors_.pop(); >> + > Can you leave a TODO about trying to measure the performance coarse > vs. grained lock? The process_capture_result() documentation seems to state the This is a non-blocking call. The framework will return this call in 5ms. I'll try to give some thought to performance implications, if it turns out to be rabbit hole, I'll discuss with Jacopo and leave a \todo in that case. >> + 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..69c78f08 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; > side note: Since we don't do this map, I think this default > constructor can be removed. Possible candidate for separate patch > > With these suggestions, > Reviewed-by: Hirokazu Honda <hiroh@chromium.org> > > -Hiro >> 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,13 @@ 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); >> 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; >> >> -- >> 2.31.0 >>
Hi Umang, On Wed, Sep 29, 2021 at 02:34:50PM +0530, Umang Jain wrote: > On 9/29/21 12:59 PM, Hirokazu Honda wrote: > > On Wed, Sep 29, 2021 at 4:17 PM Umang Jain <umang.jain@ideasonboard.com> 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. > >> > >> Since we are fixing out-of-order request completion in abortRequest(), > >> change the function to read from the Camera3RequestDescriptor directly, > >> instead of camera3_capture_request_t. The descriptor should have all the > >> information necessary to set the request buffers' state to error. > >> > >> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> > >> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > >> --- > >> src/android/camera_device.cpp | 52 +++++++++++++++++++++++------------ > >> src/android/camera_device.h | 14 +++++++++- > >> 2 files changed, 48 insertions(+), 18 deletions(-) > >> > >> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp > >> index 499baec8..fab5a854 100644 > >> --- a/src/android/camera_device.cpp > >> +++ b/src/android/camera_device.cpp > >> @@ -860,25 +860,25 @@ int CameraDevice::processControls(Camera3RequestDescriptor *descriptor) > >> return 0; > >> } > >> > >> -void CameraDevice::abortRequest(camera3_capture_request_t *request) > >> +void CameraDevice::abortRequest(Camera3RequestDescriptor *descriptor) > >> { > >> - notifyError(request->frame_number, nullptr, CAMERA3_MSG_ERROR_REQUEST); > >> + notifyError(descriptor->frameNumber_, nullptr, CAMERA3_MSG_ERROR_REQUEST); > >> > >> - camera3_capture_result_t result = {}; > >> - result.num_output_buffers = request->num_output_buffers; > >> - result.frame_number = request->frame_number; > >> + camera3_capture_result_t &result = descriptor->captureResult_; > >> + result.num_output_buffers = descriptor->buffers_.size(); > >> + result.frame_number = descriptor->frameNumber_; > >> result.partial_result = 0; > >> > >> std::vector<camera3_stream_buffer_t> resultBuffers(result.num_output_buffers); > >> for (auto [i, buffer] : utils::enumerate(resultBuffers)) { > >> - buffer = request->output_buffers[i]; > >> - buffer.release_fence = request->output_buffers[i].acquire_fence; > >> + buffer = descriptor->buffers_[i]; > >> + buffer.release_fence = descriptor->buffers_[i].acquire_fence; > >> buffer.acquire_fence = -1; > >> buffer.status = CAMERA3_BUFFER_STATUS_ERROR; > >> } > >> result.output_buffers = resultBuffers.data(); > >> > >> - callbacks_->process_capture_result(callbacks_, &result); > >> + descriptor->status_ = Camera3RequestDescriptor::Status::Error; > >> } > > > > Looks like abortRequest() does no longer touch any member variable. I > > would at least makes this const or aggressively static const or move > > to anonymous space. > > I don't think it should go to anonymous space. > > I would opt for const, with making notifyError() as const as well, > otherwise it will complain. Possible candidate for separate patch? If you post a v4 of this, I'd make abortRequest() const already. In a future series, I'd move abortRequest() to Camera3RequestDescriptor and rename it to abort(). > >> bool CameraDevice::isValidRequest(camera3_capture_request_t *camera3Request) const > >> @@ -1051,13 +1051,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(descriptor.get()); > >> + { > >> + MutexLocker descriptorsLock(descriptorsMutex_); > >> + descriptors_.push(std::move(descriptor)); > >> + } > >> + sendCaptureResults(); > >> return 0; > >> } > >> > >> @@ -1115,7 +1121,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_) { > >> @@ -1165,9 +1171,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; > >> } > >> > >> @@ -1233,10 +1239,22 @@ 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()) { > >> + auto descriptor = std::move(descriptors_.front()); > >> + descriptors_.pop(); > >> + > > > > Can you leave a TODO about trying to measure the performance coarse > > vs. grained lock? > > The process_capture_result() documentation seems to state the > > This is a non-blocking call. The framework will return this > call in 5ms. > > I'll try to give some thought to performance implications, if it turns > out to be rabbit hole, I'll discuss with Jacopo and leave a \todo in > that case. > > >> + 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..69c78f08 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; > > > > side note: Since we don't do this map, I think this default > > constructor can be removed. > > Possible candidate for separate patch > > > With these suggestions, > > Reviewed-by: Hirokazu Honda <hiroh@chromium.org> > > > >> 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,13 @@ 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); > >> 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 Laurent, On 9/29/21 2:39 PM, Laurent Pinchart wrote: > Hi Umang, > > On Wed, Sep 29, 2021 at 02:34:50PM +0530, Umang Jain wrote: >> On 9/29/21 12:59 PM, Hirokazu Honda wrote: >>> On Wed, Sep 29, 2021 at 4:17 PM Umang Jain <umang.jain@ideasonboard.com> 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. >>>> >>>> Since we are fixing out-of-order request completion in abortRequest(), >>>> change the function to read from the Camera3RequestDescriptor directly, >>>> instead of camera3_capture_request_t. The descriptor should have all the >>>> information necessary to set the request buffers' state to error. >>>> >>>> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> >>>> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> >>>> --- >>>> src/android/camera_device.cpp | 52 +++++++++++++++++++++++------------ >>>> src/android/camera_device.h | 14 +++++++++- >>>> 2 files changed, 48 insertions(+), 18 deletions(-) >>>> >>>> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp >>>> index 499baec8..fab5a854 100644 >>>> --- a/src/android/camera_device.cpp >>>> +++ b/src/android/camera_device.cpp >>>> @@ -860,25 +860,25 @@ int CameraDevice::processControls(Camera3RequestDescriptor *descriptor) >>>> return 0; >>>> } >>>> >>>> -void CameraDevice::abortRequest(camera3_capture_request_t *request) >>>> +void CameraDevice::abortRequest(Camera3RequestDescriptor *descriptor) >>>> { >>>> - notifyError(request->frame_number, nullptr, CAMERA3_MSG_ERROR_REQUEST); >>>> + notifyError(descriptor->frameNumber_, nullptr, CAMERA3_MSG_ERROR_REQUEST); >>>> >>>> - camera3_capture_result_t result = {}; >>>> - result.num_output_buffers = request->num_output_buffers; >>>> - result.frame_number = request->frame_number; >>>> + camera3_capture_result_t &result = descriptor->captureResult_; >>>> + result.num_output_buffers = descriptor->buffers_.size(); >>>> + result.frame_number = descriptor->frameNumber_; >>>> result.partial_result = 0; >>>> >>>> std::vector<camera3_stream_buffer_t> resultBuffers(result.num_output_buffers); >>>> for (auto [i, buffer] : utils::enumerate(resultBuffers)) { >>>> - buffer = request->output_buffers[i]; >>>> - buffer.release_fence = request->output_buffers[i].acquire_fence; >>>> + buffer = descriptor->buffers_[i]; >>>> + buffer.release_fence = descriptor->buffers_[i].acquire_fence; >>>> buffer.acquire_fence = -1; >>>> buffer.status = CAMERA3_BUFFER_STATUS_ERROR; >>>> } >>>> result.output_buffers = resultBuffers.data(); >>>> >>>> - callbacks_->process_capture_result(callbacks_, &result); >>>> + descriptor->status_ = Camera3RequestDescriptor::Status::Error; >>>> } >>> Looks like abortRequest() does no longer touch any member variable. I >>> would at least makes this const or aggressively static const or move >>> to anonymous space. >> I don't think it should go to anonymous space. >> >> I would opt for const, with making notifyError() as const as well, >> otherwise it will complain. Possible candidate for separate patch? > If you post a v4 of this, I'd make abortRequest() const already. In a > future series, I'd move abortRequest() to Camera3RequestDescriptor and > rename it to abort(). I was already thinking this and noted down for future series (separate ofcourse) Though we will need to move notifyError() to Camera3RequestDescriptor as well, which is currently inside CameraDevice & possibly devise a way to access callbacks_ ! So, will need some thinking there, which I don't want to get into right now For v4, lets move to const already, makes sense. > >>>> bool CameraDevice::isValidRequest(camera3_capture_request_t *camera3Request) const >>>> @@ -1051,13 +1051,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(descriptor.get()); >>>> + { >>>> + MutexLocker descriptorsLock(descriptorsMutex_); >>>> + descriptors_.push(std::move(descriptor)); >>>> + } >>>> + sendCaptureResults(); >>>> return 0; >>>> } >>>> >>>> @@ -1115,7 +1121,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_) { >>>> @@ -1165,9 +1171,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; >>>> } >>>> >>>> @@ -1233,10 +1239,22 @@ 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()) { >>>> + auto descriptor = std::move(descriptors_.front()); >>>> + descriptors_.pop(); >>>> + >>> Can you leave a TODO about trying to measure the performance coarse >>> vs. grained lock? >> The process_capture_result() documentation seems to state the >> >> This is a non-blocking call. The framework will return this >> call in 5ms. >> >> I'll try to give some thought to performance implications, if it turns >> out to be rabbit hole, I'll discuss with Jacopo and leave a \todo in >> that case. >> >>>> + 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..69c78f08 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; >>> side note: Since we don't do this map, I think this default >>> constructor can be removed. >> Possible candidate for separate patch >> >>> With these suggestions, >>> Reviewed-by: Hirokazu Honda <hiroh@chromium.org> >>> >>>> 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,13 @@ 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); >>>> 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 499baec8..fab5a854 100644 --- a/src/android/camera_device.cpp +++ b/src/android/camera_device.cpp @@ -860,25 +860,25 @@ int CameraDevice::processControls(Camera3RequestDescriptor *descriptor) return 0; } -void CameraDevice::abortRequest(camera3_capture_request_t *request) +void CameraDevice::abortRequest(Camera3RequestDescriptor *descriptor) { - notifyError(request->frame_number, nullptr, CAMERA3_MSG_ERROR_REQUEST); + notifyError(descriptor->frameNumber_, nullptr, CAMERA3_MSG_ERROR_REQUEST); - camera3_capture_result_t result = {}; - result.num_output_buffers = request->num_output_buffers; - result.frame_number = request->frame_number; + camera3_capture_result_t &result = descriptor->captureResult_; + result.num_output_buffers = descriptor->buffers_.size(); + result.frame_number = descriptor->frameNumber_; result.partial_result = 0; std::vector<camera3_stream_buffer_t> resultBuffers(result.num_output_buffers); for (auto [i, buffer] : utils::enumerate(resultBuffers)) { - buffer = request->output_buffers[i]; - buffer.release_fence = request->output_buffers[i].acquire_fence; + buffer = descriptor->buffers_[i]; + buffer.release_fence = descriptor->buffers_[i].acquire_fence; buffer.acquire_fence = -1; buffer.status = CAMERA3_BUFFER_STATUS_ERROR; } 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 @@ -1051,13 +1051,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(descriptor.get()); + { + MutexLocker descriptorsLock(descriptorsMutex_); + descriptors_.push(std::move(descriptor)); + } + sendCaptureResults(); return 0; } @@ -1115,7 +1121,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_) { @@ -1165,9 +1171,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; } @@ -1233,10 +1239,22 @@ 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()) { + auto 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..69c78f08 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,13 @@ 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); 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;