[libcamera-devel] [PATCH] lc-compliance: simple_capture: Free Requests properly
Umang Jain
umang.jain at ideasonboard.com
Wed Nov 30 11:35:47 CET 2022
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.
>
> 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
>>
More information about the libcamera-devel
mailing list