[libcamera-devel] [PATCH v3 3/3] android: camera_device: Send capture results by inspecting the queue
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Wed Sep 29 11:09:49 CEST 2021
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 at 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 at ideasonboard.com>
> >> Reviewed-by: Laurent Pinchart <laurent.pinchart at 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 at 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;
> >>
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list