[libcamera-devel] [PATCH v2 3/3] android: camera_device: Send capture results by inspecting the queue
Umang Jain
umang.jain at ideasonboard.com
Wed Sep 29 09:34:57 CEST 2021
Hello,
On 9/29/21 12:40 PM, Laurent Pinchart wrote:
> 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.
Yep, addressed in v3, thanks for pointing it out
>
>>>> + 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.
The process_capture_resultperformance is specified as:
* This is a non-blocking call. The framework will return this call
in 5ms.
*
* The pipeline latency (see S7 for definition) should be less than
or equal to
* 4 frame intervals, and must be less than or equal to 8 frame
intervals.
https://android.googlesource.com/platform/hardware/libhardware/+/master/include/hardware/camera3.h#2781
I don't think bring callback under the lock would have much impact on
the performance, fps-wise?
>
>>>> + }
>>>> }
>>>>
>>>> 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;
>>>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20210929/84ce8e22/attachment-0001.htm>
More information about the libcamera-devel
mailing list