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