[libcamera-devel] [PATCH v2 3/3] android: camera_device: Send capture results by inspecting the queue
Hirokazu Honda
hiroh at chromium.org
Wed Sep 29 01:45:24 CEST 2021
Hi Umang,
On Wed, Sep 29, 2021 at 6:30 AM Laurent Pinchart
<laurent.pinchart at 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 at 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 at 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
More information about the libcamera-devel
mailing list