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

[Kieran Bingham]

Quoting Umang Jain (2022-11-30 10:35:47)
> Hi Kieran,
> 
> On 11/30/22 1:39 AM, Kieran Bingham via libcamera-devel wrote:
> > 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.
> 
> If the buffers to be associated with the Requests are allocated and 
> mis-managed by the application themselves - how would that be address by 
> libcamera-core?
> 
> I am reading the Request->addBuffer() documentation and clearly mentioned:
> 
>   *
>   * A reference to the buffer is stored in the request. The caller is 
> responsible
>   * for ensuring that the buffer will remain valid until the request 
> complete
>   * callback is called.
> *
> 
> In this case the buffers from manually freed before the request got 
> completed (via set 'cancelled' status and completeRequest() codepath in 
> PIpelineHandler::Stop()). So that's not the right thing to do / happen.

aha, ok - if it's documented, then we're probably good with this for
now.

I do wonder if we should have some reverse association such that when
you add a buffer to a request, the buffer knows which request it's
contained in, and can 'remove' itself when it's freed... but ... that
makes me wonder if we could legitimately have a single buffer in
multiple requests which would break that possibility ... so maybe we're
fine with just the documentation as we have it.

--
Kieran


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