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

13928 diff mbox series

When a request has been completed and a new capture_result is created
to be sent to the framework through the process_capture_result()
callback we assumed all fences had been waited on, hence we set both
the release and acquisition fences to -1.

Now that the acquisition fence of streams generated through
post-processing are handled by CameraStream::process(), resetting fences
too early would invalidate them before they get handled.

Post-pone setting fences to -1 for successful capture results, and
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>
---
 src/android/camera_device.cpp | 44 ++++++++++++++++++++++++-----------
 1 file changed, 31 insertions(+), 13 deletions(-)

Hi Jacopo,

On Sat, Sep 25, 2021 at 2:01 AM Jacopo Mondi <jacopo@jmondi.org> wrote:
>
> When a request has been completed and a new capture_result is created
> to be sent to the framework through the process_capture_result()
> callback we assumed all fences had been waited on, hence we set both
> the release and acquisition fences to -1.
>
> Now that the acquisition fence of streams generated through
> post-processing are handled by CameraStream::process(), resetting fences
> too early would invalidate them before they get handled.
>
> Post-pone setting fences to -1 for successful capture results, and
> 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>

The change looks good.
Reviewed-by: Hirokazu Honda <hiroh@chromium.org>

But I think the patch order should be reversed. Otherwise, a passed
acquire_fence in CameraStream::process() is always -1.

-Hiro
> ---
>  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 db35947afc2f..8ca2353e5f33 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
> +                        * 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;
> +               }
>                 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;
> +               buffer.release_fence = -1;
> +               buffer.status = CAMERA3_BUFFER_STATUS_OK;
> +       }
> +       captureResult.partial_result = 1;
> +
>         captureResult.result = resultMetadata->get();
>         callbacks_->process_capture_result(callbacks_, &captureResult);
>  }
> --
> 2.32.0
>