[libcamera-devel] [PATCH] lc-compliance: simple_capture: Free Requests properly

[Kieran Bingham]

Quoting Paul Elder via libcamera-devel (2022-11-29 11:00:05)
> In the simple capture tests, in the capture functions, the Requests were
> auto-deallocated by the function going out of scope after the test
> completed. However, before the end of the scope, the Framebuffers that
> the Requests referred to were manually freed. Thus when the Requests
> were deallocated, they tried to cancel their Framebuffers, which involve
> setting the status to cancelled, which obviously causes a segfault
> because the Framebuffers had already been freed.

This worries me that we have a path that could let application cause a
segfault in libcamera. That's not good.

> Fix this by moving the list of Requests to a member variable and
> deallocating them before deallocating the Framebuffers at stop() time.

Fixing this here seems reasonable, but I'm concerned that we now need a
unit test in libcamera unit tests which ensure or validate the lifetimes
of freeing buffers/requests.

If I understand it, that means that we have a lifetime management issue
still between the Request and the Buffer object that may be associated
with it.

Bug: https://bugs.libcamera.org/show_bug.cgi?id=171
Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
Tested-by: Kieran Bingham <kieran.bingham at ideasonboard.com>

But I am not sure we should consider the underlying issue resolved yet.
I feel like the crash in lc-compliance is simply telling us we have a
fault with our API.

The question is if we should still fix lc-compliance here or not - or if
we should keep it 'broken' so that we can validate whatever fix we do
internally in libcamera?

--
Kieran


> Signed-off-by: Paul Elder <paul.elder at ideasonboard.com>
> ---
>  src/apps/lc-compliance/simple_capture.cpp | 7 +++----
>  src/apps/lc-compliance/simple_capture.h   | 1 +
>  2 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/src/apps/lc-compliance/simple_capture.cpp b/src/apps/lc-compliance/simple_capture.cpp
> index ab5cb35c..cf4d7cf3 100644
> --- a/src/apps/lc-compliance/simple_capture.cpp
> +++ b/src/apps/lc-compliance/simple_capture.cpp
> @@ -65,6 +65,7 @@ void SimpleCapture::stop()
>         camera_->requestCompleted.disconnect(this);
>  
>         Stream *stream = config_->at(0).stream();
> +       requests_.clear();
>         allocator_->free(stream);
>  }
>  
> @@ -95,7 +96,6 @@ void SimpleCaptureBalanced::capture(unsigned int numRequests)
>         captureLimit_ = numRequests;
>  
>         /* Queue the recommended number of reqeuests. */
> -       std::vector<std::unique_ptr<libcamera::Request>> requests;
>         for (const std::unique_ptr<FrameBuffer> &buffer : buffers) {
>                 std::unique_ptr<Request> request = camera_->createRequest();
>                 ASSERT_TRUE(request) << "Can't create request";
> @@ -104,7 +104,7 @@ void SimpleCaptureBalanced::capture(unsigned int numRequests)
>  
>                 ASSERT_EQ(queueRequest(request.get()), 0) << "Failed to queue request";
>  
> -               requests.push_back(std::move(request));
> +               requests_.push_back(std::move(request));
>         }
>  
>         /* Run capture session. */
> @@ -156,7 +156,6 @@ void SimpleCaptureUnbalanced::capture(unsigned int numRequests)
>         captureLimit_ = numRequests;
>  
>         /* Queue the recommended number of reqeuests. */
> -       std::vector<std::unique_ptr<libcamera::Request>> requests;
>         for (const std::unique_ptr<FrameBuffer> &buffer : buffers) {
>                 std::unique_ptr<Request> request = camera_->createRequest();
>                 ASSERT_TRUE(request) << "Can't create request";
> @@ -165,7 +164,7 @@ void SimpleCaptureUnbalanced::capture(unsigned int numRequests)
>  
>                 ASSERT_EQ(camera_->queueRequest(request.get()), 0) << "Failed to queue request";
>  
> -               requests.push_back(std::move(request));
> +               requests_.push_back(std::move(request));
>         }
>  
>         /* Run capture session. */
> diff --git a/src/apps/lc-compliance/simple_capture.h b/src/apps/lc-compliance/simple_capture.h
> index fd9d2a97..2911d601 100644
> --- a/src/apps/lc-compliance/simple_capture.h
> +++ b/src/apps/lc-compliance/simple_capture.h
> @@ -32,6 +32,7 @@ protected:
>         std::shared_ptr<libcamera::Camera> camera_;
>         std::unique_ptr<libcamera::FrameBufferAllocator> allocator_;
>         std::unique_ptr<libcamera::CameraConfiguration> config_;
> +       std::vector<std::unique_ptr<libcamera::Request>> requests_;
>  };
>  
>  class SimpleCaptureBalanced : public SimpleCapture
> -- 
> 2.35.1
>