[libcamera-devel] [PATCH v3 3/3] android: camera_device: Send capture results by inspecting the queue
Umang Jain
umang.jain at ideasonboard.com
Wed Sep 29 10:34:50 CEST 2021
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 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;
>> }
>>
>> 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
>>
More information about the libcamera-devel
mailing list