[libcamera-devel,v2,1/2] android: Post-pone fences reset in capture result

13957 diff mbox series

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
the release and acquisition fences to -1.

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@jmondi.org>
Reviewed-by: Hirokazu Honda <hiroh@chromium.org>
---
 src/android/camera_device.cpp | 44 ++++++++++++++++++++++++-----------
 1 file changed, 31 insertions(+), 13 deletions(-)

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@jmondi.org>
> Reviewed-by: Hirokazu Honda <hiroh@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.

> +		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.

> +	}
> +	captureResult.partial_result = 1;
> +
>  	captureResult.result = resultMetadata->get();
>  	callbacks_->process_capture_result(callbacks_, &captureResult);
>  }

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@jmondi.org>
> > Reviewed-by: Hirokazu Honda <hiroh@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.

>
> > +		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

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@jmondi.org>
>>> Reviewed-by: Hirokazu Honda <hiroh@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