[libcamera-devel] [PATCH v2 1/2] android: Post-pone fences reset in capture result
Umang Jain
umang.jain at ideasonboard.com
Tue Sep 28 13:25:57 CEST 2021
Hi,
On 9/28/21 3:32 PM, Jacopo Mondi wrote:
> Hi Laurent,
>
> On Tue, Sep 28, 2021 at 03:51:52AM +0300, Laurent Pinchart wrote:
>> Hi Jacopo,
>>
>> Thank you for the patch.
>>
>> On Mon, Sep 27, 2021 at 11:36:59PM +0200, Jacopo Mondi wrote:
>>> When a request has been completed and a new capture_result is created
>>> we assumed all fences had been waited on, hence we set both
>> Maybe "we assumed we have waited for all the fences to be signalled" ?
>>
>>> the release and acquisition fences to -1.
>> s/acquisition/acquire/
>>
>> same below
>>
>>> As no buffer is queued to libcamera::Camera for streams of type Mapped,
>>> their acquisition fences went ignored. Prepare to fix that by
>>> by moving fences resetting after post-processing, that will be
>>> instrumented to handle fences in the next patch.
>>>
>>> Also correct the release_fence handling for failed captures, as the
>>> framework requires the release fences to be set to the acquire fence
>>> value if the acquire fence has not been waited on.
>>>
>>> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
>>> Reviewed-by: Hirokazu Honda <hiroh at chromium.org>
>>> ---
>>> src/android/camera_device.cpp | 44 ++++++++++++++++++++++++-----------
>>> 1 file changed, 31 insertions(+), 13 deletions(-)
>>>
>>> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
>>> index 21844e5114a9..3c9609d74402 100644
>>> --- a/src/android/camera_device.cpp
>>> +++ b/src/android/camera_device.cpp
>>> @@ -1098,22 +1098,10 @@ void CameraDevice::requestComplete(Request *request)
>>> }
>>> Camera3RequestDescriptor &descriptor = node.mapped();
>>>
>>> - /*
>>> - * Prepare the capture result for the Android camera stack.
>>> - *
>>> - * The buffer status is set to OK and later changed to ERROR if
>>> - * post-processing/compression fails.
>>> - */
>>> camera3_capture_result_t captureResult = {};
>>> captureResult.frame_number = descriptor.frameNumber_;
>>> captureResult.num_output_buffers = descriptor.buffers_.size();
>>> - for (camera3_stream_buffer_t &buffer : descriptor.buffers_) {
>>> - buffer.acquire_fence = -1;
>>> - buffer.release_fence = -1;
>>> - buffer.status = CAMERA3_BUFFER_STATUS_OK;
>>> - }
>>> captureResult.output_buffers = descriptor.buffers_.data();
>>> - captureResult.partial_result = 1;
>>>
>>> /*
>>> * If the Request has failed, abort the request by notifying the error
>>> @@ -1128,8 +1116,27 @@ void CameraDevice::requestComplete(Request *request)
>>> CAMERA3_MSG_ERROR_REQUEST);
>>>
>>> captureResult.partial_result = 0;
>>> - for (camera3_stream_buffer_t &buffer : descriptor.buffers_)
>>> + for (camera3_stream_buffer_t &buffer : descriptor.buffers_) {
>>> + CameraStream *cameraStream =
>>> + static_cast<CameraStream *>(buffer.stream->priv);
>>> +
>>> + /*
>>> + * Streams of type Direct have been queued to the
>>> + * libcamera::Camera and their acquisition fences has
>> s/has/have/
>>
>>> + * already been waited on by the CameraWorker.
>>> + *
>>> + * For other stream types signal to the framework the
>>> + * acquisition fence has not been waited on, by setting
>>> + * the release fence to its value.
>>> + */
>>> + if (cameraStream->type() == CameraStream::Type::Direct)
>>> + buffer.release_fence = -1;
>>> + else
>>> + buffer.release_fence = buffer.acquire_fence;
>>> +
>>> + buffer.acquire_fence = -1;
>>> buffer.status = CAMERA3_BUFFER_STATUS_ERROR;
>>> + }
>> Blank line here.
>>
>>> callbacks_->process_capture_result(callbacks_, &captureResult);
>>>
>>> return;
>>> @@ -1196,6 +1203,17 @@ void CameraDevice::requestComplete(Request *request)
>>> }
>>> }
>>>
>>> + /*
>>> + * Finalize the capture result by setting fences and buffer status
>>> + * before executing the callback.
>>> + */
>>> + for (camera3_stream_buffer_t &buffer : descriptor.buffers_) {
>>> + buffer.acquire_fence = -1;
>> Ideally, it would be nice to set the acquire_fence to -1 in
>> CameraWorker::Worker::waitFence(), just after closing the fence fd.
>> That's not possible yet, as the CameraWorker doesn't have access to the
>> descriptor. This will be fixed by Umang's work, but I don't think we
>> should wait until it gets merged. A \todo comment here would be good
>> though.
>>
>> If we could set acquire_fence to -1 right after waiting, the above code
>> would also be simplified, you could write
>>
>> buffer.release_fence = buffer.acquire_fence;
>>
>> regardless of the stream type.
> I considered that too, but it required giving the full descriptor to
> the camera worker, something that would conflict with what Umang is
> doing.
Yes, it seems so. I wouldn't mind having a \todo here, plus, a patch to
address this shall go on top of my map => deque work, because I do not
think my series will address the patch to share descriptor to
CameraWorker (hence, I am advocating a \todo)
>
>>> + buffer.release_fence = -1;
>>> + buffer.status = CAMERA3_BUFFER_STATUS_OK;
>> This will override the buffer status set to CAMERA3_BUFFER_STATUS_ERROR
>> in case post-processing fails. I think it would be easier to keep
>> setting buffer.status to CAMERA3_BUFFER_STATUS_OK at the beginning of
>> this function to avoid this problem. And if you keep the loop, you could
>> as well set release_fence to -1 there too, but that's up to you.
> Ah crabs, yes, we call the callback in the same path, sorry, I'll fix!
>
>>> + }
>>> + captureResult.partial_result = 1;
>>> +
>>> captureResult.result = resultMetadata->get();
>>> callbacks_->process_capture_result(callbacks_, &captureResult);
>>> }
>> --
>> Regards,
>>
>> Laurent Pinchart
More information about the libcamera-devel
mailing list