| Message ID | 20210311093325.8933-1-m.cichy@pengutronix.de |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Hi Marian, I noticed a few potential null/segfault issues with this approach On 3/11/21 3:03 PM, Marian Cichy wrote: > If gst_buffer_pool_acquire_buffer in gst_libcamera_task_run fails, the > unique_ptr to the request-object gets reset and hence, its destructor > is called. However, the wrap-object points to the same object and is > still alive at this moment. When the task_run-function is finished, the > destructor of the wrap-object is called, which in return calls the > destructor of the request-object again. > > Instead of taking care of both, the request and the wrap-object, we can > move the request to the wrap which will then effectively take care of > the request object automatically. ...take care of the request s/object/object-deletion/ maybe? > > Signed-off-by: Marian Cichy <m.cichy@pengutronix.de> > Suggested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > src/gstreamer/gstlibcamerasrc.cpp | 28 ++++++++++++---------------- > 1 file changed, 12 insertions(+), 16 deletions(-) > > diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp > index 636c14df..e86c3d7f 100644 > --- a/src/gstreamer/gstlibcamerasrc.cpp > +++ b/src/gstreamer/gstlibcamerasrc.cpp > @@ -52,19 +52,19 @@ GST_DEBUG_CATEGORY_STATIC(source_debug); > #define GST_CAT_DEFAULT source_debug > > struct RequestWrap { > - RequestWrap(Request *request); > + RequestWrap(std::unique_ptr<Request> request); > ~RequestWrap(); > > void attachBuffer(GstBuffer *buffer); > GstBuffer *detachBuffer(Stream *stream); > > /* For ptr comparison only. */ I think you need to remove this comment too > - Request *request_; > + std::unique_ptr<Request> request_; > std::map<Stream *, GstBuffer *> buffers_; > }; > > -RequestWrap::RequestWrap(Request *request) > - : request_(request) > +RequestWrap::RequestWrap(std::unique_ptr<Request> request) > + : request_(std::move(request)) > { > } > > @@ -74,8 +74,6 @@ RequestWrap::~RequestWrap() > if (item.second) > gst_buffer_unref(item.second); > } > - > - delete request_; > } > > void RequestWrap::attachBuffer(GstBuffer *buffer) > @@ -164,7 +162,7 @@ GstLibcameraSrcState::requestCompleted(Request *request) > std::unique_ptr<RequestWrap> wrap = std::move(requests_.front()); > requests_.pop(); > > - g_return_if_fail(wrap->request_ == request); > + g_return_if_fail(wrap->request_.get() == request); > > if ((request->status() == Request::RequestCancelled)) { > GST_DEBUG_OBJECT(src_, "Request was cancelled"); > @@ -268,8 +266,7 @@ gst_libcamera_src_task_run(gpointer user_data) > GstLibcameraSrc *self = GST_LIBCAMERA_SRC(user_data); > GstLibcameraSrcState *state = self->state; > > - std::unique_ptr<Request> request = state->cam_->createRequest(); > - auto wrap = std::make_unique<RequestWrap>(request.get()); > + auto wrap = std::make_unique<RequestWrap>(state->cam_->createRequest()); *1 for marker below > for (GstPad *srcpad : state->srcpads_) { > GstLibcameraPool *pool = gst_libcamera_pad_get_pool(srcpad); > GstBuffer *buffer; > @@ -279,24 +276,23 @@ gst_libcamera_src_task_run(gpointer user_data) > &buffer, nullptr); > if (ret != GST_FLOW_OK) { > /* > - * RequestWrap does not take ownership, and we won't be > - * queueing this one due to lack of buffers. > + * RequestWrap has ownership of the rquest, and we s/rquest/request/ > + * won't be queueing this one due to lack of buffers. > */ > - request.reset(); > + wrap.release(); > break; > } > > wrap->attachBuffer(buffer); > } > > - if (request) { > + if (wrap) { I think this should be `if (wrap.get())` no? `wrap.release()` if hit, will set nullptr to the _managed_ object, so we need to check the managed object for null > GLibLocker lock(GST_OBJECT(self)); > GST_TRACE_OBJECT(self, "Requesting buffers"); > - state->cam_->queueRequest(request.get()); > + state->cam_->queueRequest(wrap->request_.get()); from *1, createRequest() call can return nullptr, hence you have a std::make_unique<RequestWrap>(nullptr); which will set request_ as nullptr on the wrap object. It is fine as long as wrap->request_ is not used unlike here, in this block. > state->requests_.push(std::move(wrap)); > > - /* The request will be deleted in the completion handler. */ > - request.release(); > + /* The RequestWrap will be deleted in the completion handler. */ > } > > GstFlowReturn ret = GST_FLOW_OK;
Hello, On Thu, Mar 11, 2021 at 05:05:39PM +0530, Umang Jain wrote: > > Hi Marian, > > I noticed a few potential null/segfault issues with this approach > > On 3/11/21 3:03 PM, Marian Cichy wrote: > > If gst_buffer_pool_acquire_buffer in gst_libcamera_task_run fails, the > > unique_ptr to the request-object gets reset and hence, its destructor > > is called. However, the wrap-object points to the same object and is > > still alive at this moment. When the task_run-function is finished, the > > destructor of the wrap-object is called, which in return calls the > > destructor of the request-object again. > > > > Instead of taking care of both, the request and the wrap-object, we can > > move the request to the wrap which will then effectively take care of > > the request object automatically. > > ...take care of the request s/object/object-deletion/ maybe? > > > > > Signed-off-by: Marian Cichy <m.cichy@pengutronix.de> > > Suggested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > --- > > src/gstreamer/gstlibcamerasrc.cpp | 28 ++++++++++++---------------- > > 1 file changed, 12 insertions(+), 16 deletions(-) > > > > diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp > > index 636c14df..e86c3d7f 100644 > > --- a/src/gstreamer/gstlibcamerasrc.cpp > > +++ b/src/gstreamer/gstlibcamerasrc.cpp > > @@ -52,19 +52,19 @@ GST_DEBUG_CATEGORY_STATIC(source_debug); > > #define GST_CAT_DEFAULT source_debug > > > > struct RequestWrap { > > - RequestWrap(Request *request); > > + RequestWrap(std::unique_ptr<Request> request); > > ~RequestWrap(); > > > > void attachBuffer(GstBuffer *buffer); > > GstBuffer *detachBuffer(Stream *stream); > > > > /* For ptr comparison only. */ > > I think you need to remove this comment too Indeed :-) > > - Request *request_; > > + std::unique_ptr<Request> request_; > > std::map<Stream *, GstBuffer *> buffers_; > > }; > > > > -RequestWrap::RequestWrap(Request *request) > > - : request_(request) > > +RequestWrap::RequestWrap(std::unique_ptr<Request> request) > > + : request_(std::move(request)) > > { > > } > > > > @@ -74,8 +74,6 @@ RequestWrap::~RequestWrap() > > if (item.second) > > gst_buffer_unref(item.second); > > } > > - > > - delete request_; > > } > > > > void RequestWrap::attachBuffer(GstBuffer *buffer) > > @@ -164,7 +162,7 @@ GstLibcameraSrcState::requestCompleted(Request *request) > > std::unique_ptr<RequestWrap> wrap = std::move(requests_.front()); > > requests_.pop(); > > > > - g_return_if_fail(wrap->request_ == request); > > + g_return_if_fail(wrap->request_.get() == request); > > > > if ((request->status() == Request::RequestCancelled)) { > > GST_DEBUG_OBJECT(src_, "Request was cancelled"); > > @@ -268,8 +266,7 @@ gst_libcamera_src_task_run(gpointer user_data) > > GstLibcameraSrc *self = GST_LIBCAMERA_SRC(user_data); > > GstLibcameraSrcState *state = self->state; > > > > - std::unique_ptr<Request> request = state->cam_->createRequest(); > > - auto wrap = std::make_unique<RequestWrap>(request.get()); > > + auto wrap = std::make_unique<RequestWrap>(state->cam_->createRequest()); > > *1 for marker below > > > for (GstPad *srcpad : state->srcpads_) { > > GstLibcameraPool *pool = gst_libcamera_pad_get_pool(srcpad); > > GstBuffer *buffer; > > @@ -279,24 +276,23 @@ gst_libcamera_src_task_run(gpointer user_data) > > &buffer, nullptr); > > if (ret != GST_FLOW_OK) { > > /* > > - * RequestWrap does not take ownership, and we won't be > > - * queueing this one due to lack of buffers. > > + * RequestWrap has ownership of the rquest, and we > > s/rquest/request/ > > > + * won't be queueing this one due to lack of buffers. > > */ > > - request.reset(); > > + wrap.release(); > > break; > > } > > > > wrap->attachBuffer(buffer); > > } > > > > - if (request) { > > + if (wrap) { > > I think this should be `if (wrap.get())` no? `wrap.release()` if hit, > will set nullptr to the _managed_ object, so we need to check the > managed object for null This is what is happening already: https://en.cppreference.com/w/cpp/memory/unique_ptr/operator_bool > > GLibLocker lock(GST_OBJECT(self)); > > GST_TRACE_OBJECT(self, "Requesting buffers"); > > - state->cam_->queueRequest(request.get()); > > + state->cam_->queueRequest(wrap->request_.get()); > > from *1, createRequest() call can return nullptr, hence you have a > > std::make_unique<RequestWrap>(nullptr); > > which will set request_ as nullptr on the wrap object. It is fine as > long as wrap->request_ is not used unlike here, in this block. That's indeed a problem. Maybe if (wrap && wrap->request_) above ? Nicolas, any opinion ? > > state->requests_.push(std::move(wrap)); > > > > - /* The request will be deleted in the completion handler. */ > > - request.release(); > > + /* The RequestWrap will be deleted in the completion handler. */ > > } > > > > GstFlowReturn ret = GST_FLOW_OK;
Hi Laurent, On 3/11/21 5:29 PM, Laurent Pinchart wrote: > Hello, > > On Thu, Mar 11, 2021 at 05:05:39PM +0530, Umang Jain wrote: >> Hi Marian, >> >> I noticed a few potential null/segfault issues with this approach >> >> On 3/11/21 3:03 PM, Marian Cichy wrote: >>> If gst_buffer_pool_acquire_buffer in gst_libcamera_task_run fails, the >>> unique_ptr to the request-object gets reset and hence, its destructor >>> is called. However, the wrap-object points to the same object and is >>> still alive at this moment. When the task_run-function is finished, the >>> destructor of the wrap-object is called, which in return calls the >>> destructor of the request-object again. >>> >>> Instead of taking care of both, the request and the wrap-object, we can >>> move the request to the wrap which will then effectively take care of >>> the request object automatically. >> ...take care of the request s/object/object-deletion/ maybe? >> >>> Signed-off-by: Marian Cichy <m.cichy@pengutronix.de> >>> Suggested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> >>> --- >>> src/gstreamer/gstlibcamerasrc.cpp | 28 ++++++++++++---------------- >>> 1 file changed, 12 insertions(+), 16 deletions(-) >>> >>> diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp >>> index 636c14df..e86c3d7f 100644 >>> --- a/src/gstreamer/gstlibcamerasrc.cpp >>> +++ b/src/gstreamer/gstlibcamerasrc.cpp >>> @@ -52,19 +52,19 @@ GST_DEBUG_CATEGORY_STATIC(source_debug); >>> #define GST_CAT_DEFAULT source_debug >>> >>> struct RequestWrap { >>> - RequestWrap(Request *request); >>> + RequestWrap(std::unique_ptr<Request> request); >>> ~RequestWrap(); >>> >>> void attachBuffer(GstBuffer *buffer); >>> GstBuffer *detachBuffer(Stream *stream); >>> >>> /* For ptr comparison only. */ >> I think you need to remove this comment too > Indeed :-) > >>> - Request *request_; >>> + std::unique_ptr<Request> request_; >>> std::map<Stream *, GstBuffer *> buffers_; >>> }; >>> >>> -RequestWrap::RequestWrap(Request *request) >>> - : request_(request) >>> +RequestWrap::RequestWrap(std::unique_ptr<Request> request) >>> + : request_(std::move(request)) >>> { >>> } >>> >>> @@ -74,8 +74,6 @@ RequestWrap::~RequestWrap() >>> if (item.second) >>> gst_buffer_unref(item.second); >>> } >>> - >>> - delete request_; >>> } >>> >>> void RequestWrap::attachBuffer(GstBuffer *buffer) >>> @@ -164,7 +162,7 @@ GstLibcameraSrcState::requestCompleted(Request *request) >>> std::unique_ptr<RequestWrap> wrap = std::move(requests_.front()); >>> requests_.pop(); >>> >>> - g_return_if_fail(wrap->request_ == request); >>> + g_return_if_fail(wrap->request_.get() == request); >>> >>> if ((request->status() == Request::RequestCancelled)) { >>> GST_DEBUG_OBJECT(src_, "Request was cancelled"); >>> @@ -268,8 +266,7 @@ gst_libcamera_src_task_run(gpointer user_data) >>> GstLibcameraSrc *self = GST_LIBCAMERA_SRC(user_data); >>> GstLibcameraSrcState *state = self->state; >>> >>> - std::unique_ptr<Request> request = state->cam_->createRequest(); >>> - auto wrap = std::make_unique<RequestWrap>(request.get()); >>> + auto wrap = std::make_unique<RequestWrap>(state->cam_->createRequest()); >> *1 for marker below >> >>> for (GstPad *srcpad : state->srcpads_) { >>> GstLibcameraPool *pool = gst_libcamera_pad_get_pool(srcpad); >>> GstBuffer *buffer; >>> @@ -279,24 +276,23 @@ gst_libcamera_src_task_run(gpointer user_data) >>> &buffer, nullptr); >>> if (ret != GST_FLOW_OK) { >>> /* >>> - * RequestWrap does not take ownership, and we won't be >>> - * queueing this one due to lack of buffers. >>> + * RequestWrap has ownership of the rquest, and we >> s/rquest/request/ >> >>> + * won't be queueing this one due to lack of buffers. >>> */ >>> - request.reset(); >>> + wrap.release(); >>> break; >>> } >>> >>> wrap->attachBuffer(buffer); >>> } >>> >>> - if (request) { >>> + if (wrap) { >> I think this should be `if (wrap.get())` no? `wrap.release()` if hit, >> will set nullptr to the _managed_ object, so we need to check the >> managed object for null > This is what is happening already: > https://en.cppreference.com/w/cpp/memory/unique_ptr/operator_bool > >>> GLibLocker lock(GST_OBJECT(self)); >>> GST_TRACE_OBJECT(self, "Requesting buffers"); >>> - state->cam_->queueRequest(request.get()); >>> + state->cam_->queueRequest(wrap->request_.get()); >> from *1, createRequest() call can return nullptr, hence you have a >> >> std::make_unique<RequestWrap>(nullptr); >> >> which will set request_ as nullptr on the wrap object. It is fine as >> long as wrap->request_ is not used unlike here, in this block. > That's indeed a problem. Maybe > > if (wrap && wrap->request_) > > above ? Nicolas, any opinion ? OR null-check createRequest()'s returned object before wrapping it (i.e. ensure RequestWrap doesn't wrap a nullptr from the start), so we don't have to check everything for the wrap->request_ is valid or not. > >>> state->requests_.push(std::move(wrap)); >>> >>> - /* The request will be deleted in the completion handler. */ >>> - request.release(); >>> + /* The RequestWrap will be deleted in the completion handler. */ >>> } >>> >>> GstFlowReturn ret = GST_FLOW_OK;
Hi Umang, On Thu, Mar 11, 2021 at 05:47:59PM +0530, Umang Jain wrote: > On 3/11/21 5:29 PM, Laurent Pinchart wrote: > > On Thu, Mar 11, 2021 at 05:05:39PM +0530, Umang Jain wrote: > >> On 3/11/21 3:03 PM, Marian Cichy wrote: > >>> If gst_buffer_pool_acquire_buffer in gst_libcamera_task_run fails, the > >>> unique_ptr to the request-object gets reset and hence, its destructor > >>> is called. However, the wrap-object points to the same object and is > >>> still alive at this moment. When the task_run-function is finished, the > >>> destructor of the wrap-object is called, which in return calls the > >>> destructor of the request-object again. > >>> > >>> Instead of taking care of both, the request and the wrap-object, we can > >>> move the request to the wrap which will then effectively take care of > >>> the request object automatically. > >> ...take care of the request s/object/object-deletion/ maybe? > >> > >>> Signed-off-by: Marian Cichy <m.cichy@pengutronix.de> > >>> Suggested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > >>> --- > >>> src/gstreamer/gstlibcamerasrc.cpp | 28 ++++++++++++---------------- > >>> 1 file changed, 12 insertions(+), 16 deletions(-) > >>> > >>> diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp > >>> index 636c14df..e86c3d7f 100644 > >>> --- a/src/gstreamer/gstlibcamerasrc.cpp > >>> +++ b/src/gstreamer/gstlibcamerasrc.cpp > >>> @@ -52,19 +52,19 @@ GST_DEBUG_CATEGORY_STATIC(source_debug); > >>> #define GST_CAT_DEFAULT source_debug > >>> > >>> struct RequestWrap { > >>> - RequestWrap(Request *request); > >>> + RequestWrap(std::unique_ptr<Request> request); > >>> ~RequestWrap(); > >>> > >>> void attachBuffer(GstBuffer *buffer); > >>> GstBuffer *detachBuffer(Stream *stream); > >>> > >>> /* For ptr comparison only. */ > >> > >> I think you need to remove this comment too > > > > Indeed :-) > > > >>> - Request *request_; > >>> + std::unique_ptr<Request> request_; > >>> std::map<Stream *, GstBuffer *> buffers_; > >>> }; > >>> > >>> -RequestWrap::RequestWrap(Request *request) > >>> - : request_(request) > >>> +RequestWrap::RequestWrap(std::unique_ptr<Request> request) > >>> + : request_(std::move(request)) > >>> { > >>> } > >>> > >>> @@ -74,8 +74,6 @@ RequestWrap::~RequestWrap() > >>> if (item.second) > >>> gst_buffer_unref(item.second); > >>> } > >>> - > >>> - delete request_; > >>> } > >>> > >>> void RequestWrap::attachBuffer(GstBuffer *buffer) > >>> @@ -164,7 +162,7 @@ GstLibcameraSrcState::requestCompleted(Request *request) > >>> std::unique_ptr<RequestWrap> wrap = std::move(requests_.front()); > >>> requests_.pop(); > >>> > >>> - g_return_if_fail(wrap->request_ == request); > >>> + g_return_if_fail(wrap->request_.get() == request); > >>> > >>> if ((request->status() == Request::RequestCancelled)) { > >>> GST_DEBUG_OBJECT(src_, "Request was cancelled"); > >>> @@ -268,8 +266,7 @@ gst_libcamera_src_task_run(gpointer user_data) > >>> GstLibcameraSrc *self = GST_LIBCAMERA_SRC(user_data); > >>> GstLibcameraSrcState *state = self->state; > >>> > >>> - std::unique_ptr<Request> request = state->cam_->createRequest(); > >>> - auto wrap = std::make_unique<RequestWrap>(request.get()); > >>> + auto wrap = std::make_unique<RequestWrap>(state->cam_->createRequest()); > > > >> *1 for marker below > >> > >>> for (GstPad *srcpad : state->srcpads_) { > >>> GstLibcameraPool *pool = gst_libcamera_pad_get_pool(srcpad); > >>> GstBuffer *buffer; > >>> @@ -279,24 +276,23 @@ gst_libcamera_src_task_run(gpointer user_data) > >>> &buffer, nullptr); > >>> if (ret != GST_FLOW_OK) { > >>> /* > >>> - * RequestWrap does not take ownership, and we won't be > >>> - * queueing this one due to lack of buffers. > >>> + * RequestWrap has ownership of the rquest, and we > >> > >> s/rquest/request/ > >> > >>> + * won't be queueing this one due to lack of buffers. > >>> */ > >>> - request.reset(); > >>> + wrap.release(); > >>> break; > >>> } > >>> > >>> wrap->attachBuffer(buffer); > >>> } > >>> > >>> - if (request) { > >>> + if (wrap) { > >> > >> I think this should be `if (wrap.get())` no? `wrap.release()` if hit, > >> will set nullptr to the _managed_ object, so we need to check the > >> managed object for null > > > > This is what is happening already: > > https://en.cppreference.com/w/cpp/memory/unique_ptr/operator_bool > > > >>> GLibLocker lock(GST_OBJECT(self)); > >>> GST_TRACE_OBJECT(self, "Requesting buffers"); > >>> - state->cam_->queueRequest(request.get()); > >>> + state->cam_->queueRequest(wrap->request_.get()); > >> > >> from *1, createRequest() call can return nullptr, hence you have a > >> > >> std::make_unique<RequestWrap>(nullptr); > >> > >> which will set request_ as nullptr on the wrap object. It is fine as > >> long as wrap->request_ is not used unlike here, in this block. > > > > That's indeed a problem. Maybe > > > > if (wrap && wrap->request_) > > > > above ? Nicolas, any opinion ? > > OR null-check createRequest()'s returned object before wrapping it (i.e. > ensure RequestWrap doesn't wrap a nullptr from the start), so we don't > have to check everything for the wrap->request_ is valid or not. But, as far as I understand, code further down this function still have to run in that case, we can't just do an early return using a null check. That's why I've asked for Nicolas' feedback, he knows this code much better than I do. > >>> state->requests_.push(std::move(wrap)); > >>> > >>> - /* The request will be deleted in the completion handler. */ > >>> - request.release(); > >>> + /* The RequestWrap will be deleted in the completion handler. */ > >>> } > >>> > >>> GstFlowReturn ret = GST_FLOW_OK;
Le jeudi 11 mars 2021 à 13:59 +0200, Laurent Pinchart a écrit : > Hello, > > On Thu, Mar 11, 2021 at 05:05:39PM +0530, Umang Jain wrote: > > > > Hi Marian, > > > > I noticed a few potential null/segfault issues with this approach > > > > On 3/11/21 3:03 PM, Marian Cichy wrote: > > > If gst_buffer_pool_acquire_buffer in gst_libcamera_task_run fails, the > > > unique_ptr to the request-object gets reset and hence, its destructor > > > is called. However, the wrap-object points to the same object and is > > > still alive at this moment. When the task_run-function is finished, the > > > destructor of the wrap-object is called, which in return calls the > > > destructor of the request-object again. > > > > > > Instead of taking care of both, the request and the wrap-object, we can > > > move the request to the wrap which will then effectively take care of > > > the request object automatically. > > > > ...take care of the request s/object/object-deletion/ maybe? > > > > > > > > Signed-off-by: Marian Cichy <m.cichy@pengutronix.de> > > > Suggested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > --- > > > src/gstreamer/gstlibcamerasrc.cpp | 28 ++++++++++++---------------- > > > 1 file changed, 12 insertions(+), 16 deletions(-) > > > > > > diff --git a/src/gstreamer/gstlibcamerasrc.cpp > > > b/src/gstreamer/gstlibcamerasrc.cpp > > > index 636c14df..e86c3d7f 100644 > > > --- a/src/gstreamer/gstlibcamerasrc.cpp > > > +++ b/src/gstreamer/gstlibcamerasrc.cpp > > > @@ -52,19 +52,19 @@ GST_DEBUG_CATEGORY_STATIC(source_debug); > > > #define GST_CAT_DEFAULT source_debug > > > > > > struct RequestWrap { > > > - RequestWrap(Request *request); > > > + RequestWrap(std::unique_ptr<Request> request); > > > ~RequestWrap(); > > > > > > void attachBuffer(GstBuffer *buffer); > > > GstBuffer *detachBuffer(Stream *stream); > > > > > > /* For ptr comparison only. */ > > > > I think you need to remove this comment too > > Indeed :-) > > > > - Request *request_; > > > + std::unique_ptr<Request> request_; > > > std::map<Stream *, GstBuffer *> buffers_; > > > }; > > > > > > -RequestWrap::RequestWrap(Request *request) > > > - : request_(request) > > > +RequestWrap::RequestWrap(std::unique_ptr<Request> request) > > > + : request_(std::move(request)) > > > { > > > } > > > > > > @@ -74,8 +74,6 @@ RequestWrap::~RequestWrap() > > > if (item.second) > > > gst_buffer_unref(item.second); > > > } > > > - > > > - delete request_; > > > } > > > > > > void RequestWrap::attachBuffer(GstBuffer *buffer) > > > @@ -164,7 +162,7 @@ GstLibcameraSrcState::requestCompleted(Request > > > *request) > > > std::unique_ptr<RequestWrap> wrap = std::move(requests_.front()); > > > requests_.pop(); > > > > > > - g_return_if_fail(wrap->request_ == request); > > > + g_return_if_fail(wrap->request_.get() == request); > > > > > > if ((request->status() == Request::RequestCancelled)) { > > > GST_DEBUG_OBJECT(src_, "Request was cancelled"); > > > @@ -268,8 +266,7 @@ gst_libcamera_src_task_run(gpointer user_data) > > > GstLibcameraSrc *self = GST_LIBCAMERA_SRC(user_data); > > > GstLibcameraSrcState *state = self->state; > > > > > > - std::unique_ptr<Request> request = state->cam_->createRequest(); > > > - auto wrap = std::make_unique<RequestWrap>(request.get()); > > > + auto wrap = Please keep seperate, and add missing nullptr check in between. > > > std::make_unique<RequestWrap>(state->cam_->createRequest()); > > > > *1 for marker below > > > > > for (GstPad *srcpad : state->srcpads_) { > > > GstLibcameraPool *pool = > > > gst_libcamera_pad_get_pool(srcpad); > > > GstBuffer *buffer; > > > @@ -279,24 +276,23 @@ gst_libcamera_src_task_run(gpointer user_data) > > > &buffer, nullptr); > > > if (ret != GST_FLOW_OK) { > > > /* > > > - * RequestWrap does not take ownership, and we > > > won't be > > > - * queueing this one due to lack of buffers. > > > + * RequestWrap has ownership of the rquest, and we > > > > s/rquest/request/ > > > > > + * won't be queueing this one due to lack of > > > buffers. > > > */ > > > - request.reset(); > > > + wrap.release(); > > > break; > > > } > > > > > > wrap->attachBuffer(buffer); > > > } > > > > > > - if (request) { > > > + if (wrap) { > > > > I think this should be `if (wrap.get())` no? `wrap.release()` if hit, > > will set nullptr to the _managed_ object, so we need to check the > > managed object for null > > This is what is happening already: > https://en.cppreference.com/w/cpp/memory/unique_ptr/operator_bool > > > > GLibLocker lock(GST_OBJECT(self)); > > > GST_TRACE_OBJECT(self, "Requesting buffers"); > > > - state->cam_->queueRequest(request.get()); > > > + state->cam_->queueRequest(wrap->request_.get()); > > > > from *1, createRequest() call can return nullptr, hence you have a > > > > std::make_unique<RequestWrap>(nullptr); > > > > which will set request_ as nullptr on the wrap object. It is fine as > > long as wrap->request_ is not used unlike here, in this block. > > That's indeed a problem. Maybe > > if (wrap && wrap->request_) > > above ? Nicolas, any opinion ? This way we don't have to be defensive in so many places. > > > > state->requests_.push(std::move(wrap)); > > > > > > - /* The request will be deleted in the completion handler. > > > */ > > > - request.release(); > > > + /* The RequestWrap will be deleted in the completion > > > handler. */ > > > } > > > > > > GstFlowReturn ret = GST_FLOW_OK; >
Le jeudi 11 mars 2021 à 17:47 +0530, Umang Jain a écrit : > Hi Laurent, > > On 3/11/21 5:29 PM, Laurent Pinchart wrote: > > Hello, > > > > On Thu, Mar 11, 2021 at 05:05:39PM +0530, Umang Jain wrote: > > > Hi Marian, > > > > > > I noticed a few potential null/segfault issues with this approach > > > > > > On 3/11/21 3:03 PM, Marian Cichy wrote: > > > > If gst_buffer_pool_acquire_buffer in gst_libcamera_task_run fails, the > > > > unique_ptr to the request-object gets reset and hence, its destructor > > > > is called. However, the wrap-object points to the same object and is > > > > still alive at this moment. When the task_run-function is finished, the > > > > destructor of the wrap-object is called, which in return calls the > > > > destructor of the request-object again. > > > > > > > > Instead of taking care of both, the request and the wrap-object, we can > > > > move the request to the wrap which will then effectively take care of > > > > the request object automatically. > > > ...take care of the request s/object/object-deletion/ maybe? > > > > > > > Signed-off-by: Marian Cichy <m.cichy@pengutronix.de> > > > > Suggested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > --- > > > > src/gstreamer/gstlibcamerasrc.cpp | 28 ++++++++++++---------------- > > > > 1 file changed, 12 insertions(+), 16 deletions(-) > > > > > > > > diff --git a/src/gstreamer/gstlibcamerasrc.cpp > > > > b/src/gstreamer/gstlibcamerasrc.cpp > > > > index 636c14df..e86c3d7f 100644 > > > > --- a/src/gstreamer/gstlibcamerasrc.cpp > > > > +++ b/src/gstreamer/gstlibcamerasrc.cpp > > > > @@ -52,19 +52,19 @@ GST_DEBUG_CATEGORY_STATIC(source_debug); > > > > #define GST_CAT_DEFAULT source_debug > > > > > > > > struct RequestWrap { > > > > - RequestWrap(Request *request); > > > > + RequestWrap(std::unique_ptr<Request> request); > > > > ~RequestWrap(); > > > > > > > > void attachBuffer(GstBuffer *buffer); > > > > GstBuffer *detachBuffer(Stream *stream); > > > > > > > > /* For ptr comparison only. */ > > > I think you need to remove this comment too > > Indeed :-) > > > > > > - Request *request_; > > > > + std::unique_ptr<Request> request_; > > > > std::map<Stream *, GstBuffer *> buffers_; > > > > }; > > > > > > > > -RequestWrap::RequestWrap(Request *request) > > > > - : request_(request) > > > > +RequestWrap::RequestWrap(std::unique_ptr<Request> request) > > > > + : request_(std::move(request)) > > > > { > > > > } > > > > > > > > @@ -74,8 +74,6 @@ RequestWrap::~RequestWrap() > > > > if (item.second) > > > > gst_buffer_unref(item.second); > > > > } > > > > - > > > > - delete request_; > > > > } > > > > > > > > void RequestWrap::attachBuffer(GstBuffer *buffer) > > > > @@ -164,7 +162,7 @@ GstLibcameraSrcState::requestCompleted(Request > > > > *request) > > > > std::unique_ptr<RequestWrap> wrap = > > > > std::move(requests_.front()); > > > > requests_.pop(); > > > > > > > > - g_return_if_fail(wrap->request_ == request); > > > > + g_return_if_fail(wrap->request_.get() == request); > > > > > > > > if ((request->status() == Request::RequestCancelled)) { > > > > GST_DEBUG_OBJECT(src_, "Request was cancelled"); > > > > @@ -268,8 +266,7 @@ gst_libcamera_src_task_run(gpointer user_data) > > > > GstLibcameraSrc *self = GST_LIBCAMERA_SRC(user_data); > > > > GstLibcameraSrcState *state = self->state; > > > > > > > > - std::unique_ptr<Request> request = state->cam_->createRequest(); > > > > - auto wrap = std::make_unique<RequestWrap>(request.get()); > > > > + auto wrap = std::make_unique<RequestWrap>(state->cam_- > > > > >createRequest()); > > > *1 for marker below > > > > > > > for (GstPad *srcpad : state->srcpads_) { > > > > GstLibcameraPool *pool = > > > > gst_libcamera_pad_get_pool(srcpad); > > > > GstBuffer *buffer; > > > > @@ -279,24 +276,23 @@ gst_libcamera_src_task_run(gpointer user_data) > > > > &buffer, nullptr); > > > > if (ret != GST_FLOW_OK) { > > > > /* > > > > - * RequestWrap does not take ownership, and we > > > > won't be > > > > - * queueing this one due to lack of buffers. > > > > + * RequestWrap has ownership of the rquest, and > > > > we > > > s/rquest/request/ > > > > > > > + * won't be queueing this one due to lack of > > > > buffers. > > > > */ > > > > - request.reset(); > > > > + wrap.release(); > > > > break; > > > > } > > > > > > > > wrap->attachBuffer(buffer); > > > > } > > > > > > > > - if (request) { > > > > + if (wrap) { > > > I think this should be `if (wrap.get())` no? `wrap.release()` if hit, > > > will set nullptr to the _managed_ object, so we need to check the > > > managed object for null > > This is what is happening already: > > https://en.cppreference.com/w/cpp/memory/unique_ptr/operator_bool > > > > > > GLibLocker lock(GST_OBJECT(self)); > > > > GST_TRACE_OBJECT(self, "Requesting buffers"); > > > > - state->cam_->queueRequest(request.get()); > > > > + state->cam_->queueRequest(wrap->request_.get()); > > > from *1, createRequest() call can return nullptr, hence you have a > > > > > > std::make_unique<RequestWrap>(nullptr); > > > > > > which will set request_ as nullptr on the wrap object. It is fine as > > > long as wrap->request_ is not used unlike here, in this block. > > That's indeed a problem. Maybe > > > > if (wrap && wrap->request_) > > > > above ? Nicolas, any opinion ? > OR null-check createRequest()'s returned object before wrapping it (i.e. > ensure RequestWrap doesn't wrap a nullptr from the start), so we don't > have to check everything for the wrap->request_ is valid or not. Agreed! I've just replied the same in previous email. > > > > > > state->requests_.push(std::move(wrap)); > > > > > > > > - /* The request will be deleted in the completion > > > > handler. */ > > > > - request.release(); > > > > + /* The RequestWrap will be deleted in the completion > > > > handler. */ > > > > } > > > > > > > > GstFlowReturn ret = GST_FLOW_OK; > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Le jeudi 11 mars 2021 à 14:22 +0200, Laurent Pinchart a écrit : > Hi Umang, > > On Thu, Mar 11, 2021 at 05:47:59PM +0530, Umang Jain wrote: > > On 3/11/21 5:29 PM, Laurent Pinchart wrote: > > > On Thu, Mar 11, 2021 at 05:05:39PM +0530, Umang Jain wrote: > > > > On 3/11/21 3:03 PM, Marian Cichy wrote: > > > > > If gst_buffer_pool_acquire_buffer in gst_libcamera_task_run fails, the > > > > > unique_ptr to the request-object gets reset and hence, its destructor > > > > > is called. However, the wrap-object points to the same object and is > > > > > still alive at this moment. When the task_run-function is finished, > > > > > the > > > > > destructor of the wrap-object is called, which in return calls the > > > > > destructor of the request-object again. > > > > > > > > > > Instead of taking care of both, the request and the wrap-object, we > > > > > can > > > > > move the request to the wrap which will then effectively take care of > > > > > the request object automatically. > > > > ...take care of the request s/object/object-deletion/ maybe? > > > > > > > > > Signed-off-by: Marian Cichy <m.cichy@pengutronix.de> > > > > > Suggested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > > --- > > > > > src/gstreamer/gstlibcamerasrc.cpp | 28 ++++++++++++---------------- > > > > > 1 file changed, 12 insertions(+), 16 deletions(-) > > > > > > > > > > diff --git a/src/gstreamer/gstlibcamerasrc.cpp > > > > > b/src/gstreamer/gstlibcamerasrc.cpp > > > > > index 636c14df..e86c3d7f 100644 > > > > > --- a/src/gstreamer/gstlibcamerasrc.cpp > > > > > +++ b/src/gstreamer/gstlibcamerasrc.cpp > > > > > @@ -52,19 +52,19 @@ GST_DEBUG_CATEGORY_STATIC(source_debug); > > > > > #define GST_CAT_DEFAULT source_debug > > > > > > > > > > struct RequestWrap { > > > > > - RequestWrap(Request *request); > > > > > + RequestWrap(std::unique_ptr<Request> request); > > > > > ~RequestWrap(); > > > > > > > > > > void attachBuffer(GstBuffer *buffer); > > > > > GstBuffer *detachBuffer(Stream *stream); > > > > > > > > > > /* For ptr comparison only. */ > > > > > > > > I think you need to remove this comment too > > > > > > Indeed :-) > > > > > > > > - Request *request_; > > > > > + std::unique_ptr<Request> request_; > > > > > std::map<Stream *, GstBuffer *> buffers_; > > > > > }; > > > > > > > > > > -RequestWrap::RequestWrap(Request *request) > > > > > - : request_(request) > > > > > +RequestWrap::RequestWrap(std::unique_ptr<Request> request) > > > > > + : request_(std::move(request)) > > > > > { > > > > > } > > > > > > > > > > @@ -74,8 +74,6 @@ RequestWrap::~RequestWrap() > > > > > if (item.second) > > > > > gst_buffer_unref(item.second); > > > > > } > > > > > - > > > > > - delete request_; > > > > > } > > > > > > > > > > void RequestWrap::attachBuffer(GstBuffer *buffer) > > > > > @@ -164,7 +162,7 @@ GstLibcameraSrcState::requestCompleted(Request > > > > > *request) > > > > > std::unique_ptr<RequestWrap> wrap = > > > > > std::move(requests_.front()); > > > > > requests_.pop(); > > > > > > > > > > - g_return_if_fail(wrap->request_ == request); > > > > > + g_return_if_fail(wrap->request_.get() == request); > > > > > > > > > > if ((request->status() == Request::RequestCancelled)) { > > > > > GST_DEBUG_OBJECT(src_, "Request was cancelled"); > > > > > @@ -268,8 +266,7 @@ gst_libcamera_src_task_run(gpointer user_data) > > > > > GstLibcameraSrc *self = GST_LIBCAMERA_SRC(user_data); > > > > > GstLibcameraSrcState *state = self->state; > > > > > > > > > > - std::unique_ptr<Request> request = state->cam_- > > > > > >createRequest(); > > > > > - auto wrap = std::make_unique<RequestWrap>(request.get()); > > > > > + auto wrap = std::make_unique<RequestWrap>(state->cam_- > > > > > >createRequest()); > > > > > > > *1 for marker below > > > > > > > > > for (GstPad *srcpad : state->srcpads_) { > > > > > GstLibcameraPool *pool = > > > > > gst_libcamera_pad_get_pool(srcpad); > > > > > GstBuffer *buffer; > > > > > @@ -279,24 +276,23 @@ gst_libcamera_src_task_run(gpointer user_data) > > > > > &buffer, > > > > > nullptr); > > > > > if (ret != GST_FLOW_OK) { > > > > > /* > > > > > - * RequestWrap does not take ownership, and we > > > > > won't be > > > > > - * queueing this one due to lack of buffers. > > > > > + * RequestWrap has ownership of the rquest, > > > > > and we > > > > > > > > s/rquest/request/ > > > > > > > > > + * won't be queueing this one due to lack of > > > > > buffers. > > > > > */ > > > > > - request.reset(); > > > > > + wrap.release(); > > > > > break; > > > > > } > > > > > > > > > > wrap->attachBuffer(buffer); > > > > > } > > > > > > > > > > - if (request) { > > > > > + if (wrap) { > > > > > > > > I think this should be `if (wrap.get())` no? `wrap.release()` if hit, > > > > will set nullptr to the _managed_ object, so we need to check the > > > > managed object for null > > > > > > This is what is happening already: > > > https://en.cppreference.com/w/cpp/memory/unique_ptr/operator_bool > > > > > > > > GLibLocker lock(GST_OBJECT(self)); > > > > > GST_TRACE_OBJECT(self, "Requesting buffers"); > > > > > - state->cam_->queueRequest(request.get()); > > > > > + state->cam_->queueRequest(wrap->request_.get()); > > > > > > > > from *1, createRequest() call can return nullptr, hence you have a > > > > > > > > std::make_unique<RequestWrap>(nullptr); > > > > > > > > which will set request_ as nullptr on the wrap object. It is fine as > > > > long as wrap->request_ is not used unlike here, in this block. > > > > > > That's indeed a problem. Maybe > > > > > > if (wrap && wrap->request_) > > > > > > above ? Nicolas, any opinion ? > > > > OR null-check createRequest()'s returned object before wrapping it (i.e. > > ensure RequestWrap doesn't wrap a nullptr from the start), so we don't > > have to check everything for the wrap->request_ is valid or not. > > But, as far as I understand, code further down this function still have > to run in that case, we can't just do an early return using a null > check. That's why I've asked for Nicolas' feedback, he knows this code > much better than I do. Hmm, right. Something like this should do: if (!request) { GST_ELEMENT_ERROR(self, RESOURCE, NO_SPACE_LEFT,...; gst_task_stop(task); return; } > > > > > > state->requests_.push(std::move(wrap)); > > > > > > > > > > - /* The request will be deleted in the completion > > > > > handler. */ > > > > > - request.release(); > > > > > + /* The RequestWrap will be deleted in the completion > > > > > handler. */ > > > > > } > > > > > > > > > > GstFlowReturn ret = GST_FLOW_OK; >
Hi Nicolas, On Thu, Mar 11, 2021 at 04:02:20PM -0500, Nicolas Dufresne wrote: > Le jeudi 11 mars 2021 à 14:22 +0200, Laurent Pinchart a écrit : > > On Thu, Mar 11, 2021 at 05:47:59PM +0530, Umang Jain wrote: > > > On 3/11/21 5:29 PM, Laurent Pinchart wrote: > > > > On Thu, Mar 11, 2021 at 05:05:39PM +0530, Umang Jain wrote: > > > > > On 3/11/21 3:03 PM, Marian Cichy wrote: > > > > > > If gst_buffer_pool_acquire_buffer in gst_libcamera_task_run fails, the > > > > > > unique_ptr to the request-object gets reset and hence, its destructor > > > > > > is called. However, the wrap-object points to the same object and is > > > > > > still alive at this moment. When the task_run-function is finished, the > > > > > > destructor of the wrap-object is called, which in return calls the > > > > > > destructor of the request-object again. > > > > > > > > > > > > Instead of taking care of both, the request and the wrap-object, we can > > > > > > move the request to the wrap which will then effectively take care of > > > > > > the request object automatically. > > > > > > > > > > ...take care of the request s/object/object-deletion/ maybe? > > > > > > > > > > > Signed-off-by: Marian Cichy <m.cichy@pengutronix.de> > > > > > > Suggested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > > > --- > > > > > > src/gstreamer/gstlibcamerasrc.cpp | 28 ++++++++++++---------------- > > > > > > 1 file changed, 12 insertions(+), 16 deletions(-) > > > > > > > > > > > > diff --git a/src/gstreamer/gstlibcamerasrc.cpp > > > > > > b/src/gstreamer/gstlibcamerasrc.cpp > > > > > > index 636c14df..e86c3d7f 100644 > > > > > > --- a/src/gstreamer/gstlibcamerasrc.cpp > > > > > > +++ b/src/gstreamer/gstlibcamerasrc.cpp > > > > > > @@ -52,19 +52,19 @@ GST_DEBUG_CATEGORY_STATIC(source_debug); > > > > > > #define GST_CAT_DEFAULT source_debug > > > > > > > > > > > > struct RequestWrap { > > > > > > - RequestWrap(Request *request); > > > > > > + RequestWrap(std::unique_ptr<Request> request); > > > > > > ~RequestWrap(); > > > > > > > > > > > > void attachBuffer(GstBuffer *buffer); > > > > > > GstBuffer *detachBuffer(Stream *stream); > > > > > > > > > > > > /* For ptr comparison only. */ > > > > > > > > > > I think you need to remove this comment too > > > > > > > > Indeed :-) > > > > > > > > > > - Request *request_; > > > > > > + std::unique_ptr<Request> request_; > > > > > > std::map<Stream *, GstBuffer *> buffers_; > > > > > > }; > > > > > > > > > > > > -RequestWrap::RequestWrap(Request *request) > > > > > > - : request_(request) > > > > > > +RequestWrap::RequestWrap(std::unique_ptr<Request> request) > > > > > > + : request_(std::move(request)) > > > > > > { > > > > > > } > > > > > > > > > > > > @@ -74,8 +74,6 @@ RequestWrap::~RequestWrap() > > > > > > if (item.second) > > > > > > gst_buffer_unref(item.second); > > > > > > } > > > > > > - > > > > > > - delete request_; > > > > > > } > > > > > > > > > > > > void RequestWrap::attachBuffer(GstBuffer *buffer) > > > > > > @@ -164,7 +162,7 @@ GstLibcameraSrcState::requestCompleted(Request *request) > > > > > > std::unique_ptr<RequestWrap> wrap = std::move(requests_.front()); > > > > > > requests_.pop(); > > > > > > > > > > > > - g_return_if_fail(wrap->request_ == request); > > > > > > + g_return_if_fail(wrap->request_.get() == request); > > > > > > > > > > > > if ((request->status() == Request::RequestCancelled)) { > > > > > > GST_DEBUG_OBJECT(src_, "Request was cancelled"); > > > > > > @@ -268,8 +266,7 @@ gst_libcamera_src_task_run(gpointer user_data) > > > > > > GstLibcameraSrc *self = GST_LIBCAMERA_SRC(user_data); > > > > > > GstLibcameraSrcState *state = self->state; > > > > > > > > > > > > - std::unique_ptr<Request> request = state->cam_->createRequest(); > > > > > > - auto wrap = std::make_unique<RequestWrap>(request.get()); > > > > > > + auto wrap = std::make_unique<RequestWrap>(state->cam_->createRequest()); > > > > > > > > > *1 for marker below > > > > > > > > > > > for (GstPad *srcpad : state->srcpads_) { > > > > > > GstLibcameraPool *pool = gst_libcamera_pad_get_pool(srcpad); > > > > > > GstBuffer *buffer; > > > > > > @@ -279,24 +276,23 @@ gst_libcamera_src_task_run(gpointer user_data) > > > > > > &buffer, nullptr); > > > > > > if (ret != GST_FLOW_OK) { > > > > > > /* > > > > > > - * RequestWrap does not take ownership, and we won't be > > > > > > - * queueing this one due to lack of buffers. > > > > > > + * RequestWrap has ownership of the rquest, and we > > > > > > > > > > s/rquest/request/ > > > > > > > > > > > + * won't be queueing this one due to lack of buffers. > > > > > > */ > > > > > > - request.reset(); > > > > > > + wrap.release(); > > > > > > break; > > > > > > } > > > > > > > > > > > > wrap->attachBuffer(buffer); > > > > > > } > > > > > > > > > > > > - if (request) { > > > > > > + if (wrap) { > > > > > > > > > > I think this should be `if (wrap.get())` no? `wrap.release()` if hit, > > > > > will set nullptr to the _managed_ object, so we need to check the > > > > > managed object for null > > > > > > > > This is what is happening already: > > > > https://en.cppreference.com/w/cpp/memory/unique_ptr/operator_bool > > > > > > > > > > GLibLocker lock(GST_OBJECT(self)); > > > > > > GST_TRACE_OBJECT(self, "Requesting buffers"); > > > > > > - state->cam_->queueRequest(request.get()); > > > > > > + state->cam_->queueRequest(wrap->request_.get()); > > > > > > > > > > from *1, createRequest() call can return nullptr, hence you have a > > > > > > > > > > std::make_unique<RequestWrap>(nullptr); > > > > > > > > > > which will set request_ as nullptr on the wrap object. It is fine as > > > > > long as wrap->request_ is not used unlike here, in this block. > > > > > > > > That's indeed a problem. Maybe > > > > > > > > if (wrap && wrap->request_) > > > > > > > > above ? Nicolas, any opinion ? > > > > > > OR null-check createRequest()'s returned object before wrapping it (i.e. > > > ensure RequestWrap doesn't wrap a nullptr from the start), so we don't > > > have to check everything for the wrap->request_ is valid or not. > > > > But, as far as I understand, code further down this function still have > > to run in that case, we can't just do an early return using a null > > check. That's why I've asked for Nicolas' feedback, he knows this code > > much better than I do. > > Hmm, right. Something like this should do: > > if (!request) { > GST_ELEMENT_ERROR(self, RESOURCE, NO_SPACE_LEFT,...; > gst_task_stop(task); > return; > } Something along those lines ? commit be081f85bbab0c8b3b5f08d73c5bda504be7f2e6 Author: Marian Cichy <m.cichy@pengutronix.de> Date: Thu Mar 11 10:33:25 2021 +0100 libcamera: gst: Fix double-free when acquire_buffer fails If gst_buffer_pool_acquire_buffer in gst_libcamera_task_run fails, the unique_ptr to the request-object gets reset and hence, its destructor is called. However, the wrap-object points to the same object and is still alive at this moment. When the task_run-function is finished, the destructor of the wrap-object is called, which in return calls the destructor of the request-object again. Instead of taking care of both, the request and the wrap-object, we can move the request to the wrap which will then effectively take care of the request object automatically. Signed-off-by: Marian Cichy <m.cichy@pengutronix.de> Suggested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp index 636c14dff64e..7b9671200f87 100644 --- a/src/gstreamer/gstlibcamerasrc.cpp +++ b/src/gstreamer/gstlibcamerasrc.cpp @@ -52,19 +52,18 @@ GST_DEBUG_CATEGORY_STATIC(source_debug); #define GST_CAT_DEFAULT source_debug struct RequestWrap { - RequestWrap(Request *request); + RequestWrap(std::unique_ptr<Request> request); ~RequestWrap(); void attachBuffer(GstBuffer *buffer); GstBuffer *detachBuffer(Stream *stream); - /* For ptr comparison only. */ - Request *request_; + std::unique_ptr<Request> request_; std::map<Stream *, GstBuffer *> buffers_; }; -RequestWrap::RequestWrap(Request *request) - : request_(request) +RequestWrap::RequestWrap(std::unique_ptr<Request> request) + : request_(std::move(request)) { } @@ -74,8 +73,6 @@ RequestWrap::~RequestWrap() if (item.second) gst_buffer_unref(item.second); } - - delete request_; } void RequestWrap::attachBuffer(GstBuffer *buffer) @@ -164,7 +161,7 @@ GstLibcameraSrcState::requestCompleted(Request *request) std::unique_ptr<RequestWrap> wrap = std::move(requests_.front()); requests_.pop(); - g_return_if_fail(wrap->request_ == request); + g_return_if_fail(wrap->request_.get() == request); if ((request->status() == Request::RequestCancelled)) { GST_DEBUG_OBJECT(src_, "Request was cancelled"); @@ -269,7 +266,18 @@ gst_libcamera_src_task_run(gpointer user_data) GstLibcameraSrcState *state = self->state; std::unique_ptr<Request> request = state->cam_->createRequest(); - auto wrap = std::make_unique<RequestWrap>(request.get()); + if (!request) { + GST_ELEMENT_ERROR(self, RESOURCE, NO_SPACE_LEFT, + ("Failed to allocate request for camera '%s'.", + state->cam_->id().c_str()), + ("libcamera::Camera::createRequest() failed")); + gst_task_stop(self->task); + return; + } + + std::unique_ptr<RequestWrap> wrap = + std::make_unique<RequestWrap>(std::move(request)); + for (GstPad *srcpad : state->srcpads_) { GstLibcameraPool *pool = gst_libcamera_pad_get_pool(srcpad); GstBuffer *buffer; @@ -279,24 +287,23 @@ gst_libcamera_src_task_run(gpointer user_data) &buffer, nullptr); if (ret != GST_FLOW_OK) { /* - * RequestWrap does not take ownership, and we won't be - * queueing this one due to lack of buffers. + * RequestWrap has ownership of the rquest, and we + * won't be queueing this one due to lack of buffers. */ - request.reset(); + wrap.release(); break; } wrap->attachBuffer(buffer); } - if (request) { + if (wrap) { GLibLocker lock(GST_OBJECT(self)); GST_TRACE_OBJECT(self, "Requesting buffers"); - state->cam_->queueRequest(request.get()); + state->cam_->queueRequest(wrap->request_.get()); state->requests_.push(std::move(wrap)); - /* The request will be deleted in the completion handler. */ - request.release(); + /* The RequestWrap will be deleted in the completion handler. */ } GstFlowReturn ret = GST_FLOW_OK; Marian, would you be able to test this ? Should the same be done in case wrap becomes null, due to a call to gst_buffer_pool_acquire_buffer() failing, insted of continuing to processing the flow ? The lack of symmetry between the two codes paths bothers me, either because there's an issue there, or because I don't understand why they need to be different :-) This can be addressed in a subsequent patch, as it's not an issue introduced by this patch. > > > > > > state->requests_.push(std::move(wrap)); > > > > > > > > > > > > - /* The request will be deleted in the completion handler. */ > > > > > > - request.release(); > > > > > > + /* The RequestWrap will be deleted in the completion handler. */ > > > > > > } > > > > > > > > > > > > GstFlowReturn ret = GST_FLOW_OK;
Le vendredi 12 mars 2021 à 01:14 +0200, Laurent Pinchart a écrit : > Hi Nicolas, > > On Thu, Mar 11, 2021 at 04:02:20PM -0500, Nicolas Dufresne wrote: > > Le jeudi 11 mars 2021 à 14:22 +0200, Laurent Pinchart a écrit : > > > On Thu, Mar 11, 2021 at 05:47:59PM +0530, Umang Jain wrote: > > > > On 3/11/21 5:29 PM, Laurent Pinchart wrote: > > > > > On Thu, Mar 11, 2021 at 05:05:39PM +0530, Umang Jain wrote: > > > > > > On 3/11/21 3:03 PM, Marian Cichy wrote: > > > > > > > If gst_buffer_pool_acquire_buffer in gst_libcamera_task_run fails, > > > > > > > the > > > > > > > unique_ptr to the request-object gets reset and hence, its > > > > > > > destructor > > > > > > > is called. However, the wrap-object points to the same object and > > > > > > > is > > > > > > > still alive at this moment. When the task_run-function is > > > > > > > finished, the > > > > > > > destructor of the wrap-object is called, which in return calls the > > > > > > > destructor of the request-object again. > > > > > > > > > > > > > > Instead of taking care of both, the request and the wrap-object, > > > > > > > we can > > > > > > > move the request to the wrap which will then effectively take care > > > > > > > of > > > > > > > the request object automatically. > > > > > > > > > > > > ...take care of the request s/object/object-deletion/ maybe? > > > > > > > > > > > > > Signed-off-by: Marian Cichy <m.cichy@pengutronix.de> > > > > > > > Suggested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > > > > --- > > > > > > > src/gstreamer/gstlibcamerasrc.cpp | 28 ++++++++++++------------ > > > > > > > ---- > > > > > > > 1 file changed, 12 insertions(+), 16 deletions(-) > > > > > > > > > > > > > > diff --git a/src/gstreamer/gstlibcamerasrc.cpp > > > > > > > b/src/gstreamer/gstlibcamerasrc.cpp > > > > > > > index 636c14df..e86c3d7f 100644 > > > > > > > --- a/src/gstreamer/gstlibcamerasrc.cpp > > > > > > > +++ b/src/gstreamer/gstlibcamerasrc.cpp > > > > > > > @@ -52,19 +52,19 @@ GST_DEBUG_CATEGORY_STATIC(source_debug); > > > > > > > #define GST_CAT_DEFAULT source_debug > > > > > > > > > > > > > > struct RequestWrap { > > > > > > > - RequestWrap(Request *request); > > > > > > > + RequestWrap(std::unique_ptr<Request> request); > > > > > > > ~RequestWrap(); > > > > > > > > > > > > > > void attachBuffer(GstBuffer *buffer); > > > > > > > GstBuffer *detachBuffer(Stream *stream); > > > > > > > > > > > > > > /* For ptr comparison only. */ > > > > > > > > > > > > I think you need to remove this comment too > > > > > > > > > > Indeed :-) > > > > > > > > > > > > - Request *request_; > > > > > > > + std::unique_ptr<Request> request_; > > > > > > > std::map<Stream *, GstBuffer *> buffers_; > > > > > > > }; > > > > > > > > > > > > > > -RequestWrap::RequestWrap(Request *request) > > > > > > > - : request_(request) > > > > > > > +RequestWrap::RequestWrap(std::unique_ptr<Request> request) > > > > > > > + : request_(std::move(request)) > > > > > > > { > > > > > > > } > > > > > > > > > > > > > > @@ -74,8 +74,6 @@ RequestWrap::~RequestWrap() > > > > > > > if (item.second) > > > > > > > gst_buffer_unref(item.second); > > > > > > > } > > > > > > > - > > > > > > > - delete request_; > > > > > > > } > > > > > > > > > > > > > > void RequestWrap::attachBuffer(GstBuffer *buffer) > > > > > > > @@ -164,7 +162,7 @@ GstLibcameraSrcState::requestCompleted(Request > > > > > > > *request) > > > > > > > std::unique_ptr<RequestWrap> wrap = > > > > > > > std::move(requests_.front()); > > > > > > > requests_.pop(); > > > > > > > > > > > > > > - g_return_if_fail(wrap->request_ == request); > > > > > > > + g_return_if_fail(wrap->request_.get() == request); > > > > > > > > > > > > > > if ((request->status() == Request::RequestCancelled)) { > > > > > > > GST_DEBUG_OBJECT(src_, "Request was cancelled"); > > > > > > > @@ -268,8 +266,7 @@ gst_libcamera_src_task_run(gpointer user_data) > > > > > > > GstLibcameraSrc *self = GST_LIBCAMERA_SRC(user_data); > > > > > > > GstLibcameraSrcState *state = self->state; > > > > > > > > > > > > > > - std::unique_ptr<Request> request = state->cam_- > > > > > > > >createRequest(); > > > > > > > - auto wrap = std::make_unique<RequestWrap>(request.get()); > > > > > > > + auto wrap = std::make_unique<RequestWrap>(state->cam_- > > > > > > > >createRequest()); > > > > > > > > > > > *1 for marker below > > > > > > > > > > > > > for (GstPad *srcpad : state->srcpads_) { > > > > > > > GstLibcameraPool *pool = > > > > > > > gst_libcamera_pad_get_pool(srcpad); > > > > > > > GstBuffer *buffer; > > > > > > > @@ -279,24 +276,23 @@ gst_libcamera_src_task_run(gpointer > > > > > > > user_data) > > > > > > > &buffer, > > > > > > > nullptr); > > > > > > > if (ret != GST_FLOW_OK) { > > > > > > > /* > > > > > > > - * RequestWrap does not take ownership, > > > > > > > and we won't be > > > > > > > - * queueing this one due to lack of > > > > > > > buffers. > > > > > > > + * RequestWrap has ownership of the > > > > > > > rquest, and we > > > > > > > > > > > > s/rquest/request/ > > > > > > > > > > > > > + * won't be queueing this one due to lack > > > > > > > of buffers. > > > > > > > */ > > > > > > > - request.reset(); > > > > > > > + wrap.release(); > > > > > > > break; > > > > > > > } > > > > > > > > > > > > > > wrap->attachBuffer(buffer); > > > > > > > } > > > > > > > > > > > > > > - if (request) { > > > > > > > + if (wrap) { > > > > > > > > > > > > I think this should be `if (wrap.get())` no? `wrap.release()` if > > > > > > hit, > > > > > > will set nullptr to the _managed_ object, so we need to check the > > > > > > managed object for null > > > > > > > > > > This is what is happening already: > > > > > https://en.cppreference.com/w/cpp/memory/unique_ptr/operator_bool > > > > > > > > > > > > GLibLocker lock(GST_OBJECT(self)); > > > > > > > GST_TRACE_OBJECT(self, "Requesting buffers"); > > > > > > > - state->cam_->queueRequest(request.get()); > > > > > > > + state->cam_->queueRequest(wrap->request_.get()); > > > > > > > > > > > > from *1, createRequest() call can return nullptr, hence you have a > > > > > > > > > > > > std::make_unique<RequestWrap>(nullptr); > > > > > > > > > > > > which will set request_ as nullptr on the wrap object. It is fine > > > > > > as > > > > > > long as wrap->request_ is not used unlike here, in this block. > > > > > > > > > > That's indeed a problem. Maybe > > > > > > > > > > if (wrap && wrap->request_) > > > > > > > > > > above ? Nicolas, any opinion ? > > > > > > > > OR null-check createRequest()'s returned object before wrapping it (i.e. > > > > ensure RequestWrap doesn't wrap a nullptr from the start), so we don't > > > > have to check everything for the wrap->request_ is valid or not. > > > > > > But, as far as I understand, code further down this function still have > > > to run in that case, we can't just do an early return using a null > > > check. That's why I've asked for Nicolas' feedback, he knows this code > > > much better than I do. > > > > Hmm, right. Something like this should do: > > > > if (!request) { > > GST_ELEMENT_ERROR(self, RESOURCE, NO_SPACE_LEFT,...; > > gst_task_stop(task); > > return; > > } > > Something along those lines ? > > commit be081f85bbab0c8b3b5f08d73c5bda504be7f2e6 > Author: Marian Cichy <m.cichy@pengutronix.de> > Date: Thu Mar 11 10:33:25 2021 +0100 > > libcamera: gst: Fix double-free when acquire_buffer fails > > If gst_buffer_pool_acquire_buffer in gst_libcamera_task_run fails, the > unique_ptr to the request-object gets reset and hence, its destructor > is called. However, the wrap-object points to the same object and is > still alive at this moment. When the task_run-function is finished, the > destructor of the wrap-object is called, which in return calls the > destructor of the request-object again. > > Instead of taking care of both, the request and the wrap-object, we can > move the request to the wrap which will then effectively take care of > the request object automatically. > > Signed-off-by: Marian Cichy <m.cichy@pengutronix.de> > Suggested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > diff --git a/src/gstreamer/gstlibcamerasrc.cpp > b/src/gstreamer/gstlibcamerasrc.cpp > index 636c14dff64e..7b9671200f87 100644 > --- a/src/gstreamer/gstlibcamerasrc.cpp > +++ b/src/gstreamer/gstlibcamerasrc.cpp > @@ -52,19 +52,18 @@ GST_DEBUG_CATEGORY_STATIC(source_debug); > #define GST_CAT_DEFAULT source_debug > > struct RequestWrap { > - RequestWrap(Request *request); > + RequestWrap(std::unique_ptr<Request> request); > ~RequestWrap(); > > void attachBuffer(GstBuffer *buffer); > GstBuffer *detachBuffer(Stream *stream); > > - /* For ptr comparison only. */ > - Request *request_; > + std::unique_ptr<Request> request_; > std::map<Stream *, GstBuffer *> buffers_; > }; > > -RequestWrap::RequestWrap(Request *request) > - : request_(request) > +RequestWrap::RequestWrap(std::unique_ptr<Request> request) > + : request_(std::move(request)) > { > } > > @@ -74,8 +73,6 @@ RequestWrap::~RequestWrap() > if (item.second) > gst_buffer_unref(item.second); > } > - > - delete request_; > } > > void RequestWrap::attachBuffer(GstBuffer *buffer) > @@ -164,7 +161,7 @@ GstLibcameraSrcState::requestCompleted(Request *request) > std::unique_ptr<RequestWrap> wrap = std::move(requests_.front()); > requests_.pop(); > > - g_return_if_fail(wrap->request_ == request); > + g_return_if_fail(wrap->request_.get() == request); > > if ((request->status() == Request::RequestCancelled)) { > GST_DEBUG_OBJECT(src_, "Request was cancelled"); > @@ -269,7 +266,18 @@ gst_libcamera_src_task_run(gpointer user_data) > GstLibcameraSrcState *state = self->state; > > std::unique_ptr<Request> request = state->cam_->createRequest(); > - auto wrap = std::make_unique<RequestWrap>(request.get()); > + if (!request) { > + GST_ELEMENT_ERROR(self, RESOURCE, NO_SPACE_LEFT, > + ("Failed to allocate request for camera > '%s'.", > + state->cam_->id().c_str()), > + ("libcamera::Camera::createRequest() > failed")); > + gst_task_stop(self->task); > + return; > + } > + > + std::unique_ptr<RequestWrap> wrap = > + std::make_unique<RequestWrap>(std::move(request)); > + > for (GstPad *srcpad : state->srcpads_) { > GstLibcameraPool *pool = gst_libcamera_pad_get_pool(srcpad); > GstBuffer *buffer; > @@ -279,24 +287,23 @@ gst_libcamera_src_task_run(gpointer user_data) > &buffer, nullptr); > if (ret != GST_FLOW_OK) { > /* > - * RequestWrap does not take ownership, and we won't > be > - * queueing this one due to lack of buffers. > + * RequestWrap has ownership of the rquest, and we request > + * won't be queueing this one due to lack of buffers. > */ > - request.reset(); > + wrap.release(); > break; > } > > wrap->attachBuffer(buffer); > } > > - if (request) { > + if (wrap) { > GLibLocker lock(GST_OBJECT(self)); > GST_TRACE_OBJECT(self, "Requesting buffers"); > - state->cam_->queueRequest(request.get()); > + state->cam_->queueRequest(wrap->request_.get()); > state->requests_.push(std::move(wrap)); > > - /* The request will be deleted in the completion handler. */ > - request.release(); > + /* The RequestWrap will be deleted in the completion handler. > */ > } > > GstFlowReturn ret = GST_FLOW_OK; Reviewed-by: Nicolas Dufresne <nicolas.dufresne@collabora.com> > > Marian, would you be able to test this ? > > Should the same be done in case wrap becomes null, due to a call to > gst_buffer_pool_acquire_buffer() failing, insted of continuing to > processing the flow ? The lack of symmetry between the two codes paths > bothers me, either because there's an issue there, or because I don't > understand why they need to be different :-) This can be addressed in a > subsequent patch, as it's not an issue introduced by this patch. I think this code is a bit incomplete, specially since it supports multiple pads, while you cannot yet request a second pad. In general, my idea was that we'd try and ensure we have at least one pending request per stream/pad before pausing the task. Pausing the task, which just mean our task function will stop being called repeatedly until gst_libcamera_resume_task() is called. For the case we run-out of buffer in our pool (which is not as dramatic as failing to create a request), we should only stop / fail if there is no pending request already queued for that pad. I'll take care of this one, it also need a locking fix, and reversing the pending condition check lower (which is upside down). > > > > > > > > state->requests_.push(std::move(wrap)); > > > > > > > > > > > > > > - /* The request will be deleted in the completion > > > > > > > handler. */ > > > > > > > - request.release(); > > > > > > > + /* The RequestWrap will be deleted in the > > > > > > > completion handler. */ > > > > > > > } > > > > > > > > > > > > > > GstFlowReturn ret = GST_FLOW_OK; >
On 3/12/21 12:14 AM, Laurent Pinchart wrote: > Hi Nicolas, > > On Thu, Mar 11, 2021 at 04:02:20PM -0500, Nicolas Dufresne wrote: >> Le jeudi 11 mars 2021 à 14:22 +0200, Laurent Pinchart a écrit : >>> On Thu, Mar 11, 2021 at 05:47:59PM +0530, Umang Jain wrote: >>>> On 3/11/21 5:29 PM, Laurent Pinchart wrote: >>>>> On Thu, Mar 11, 2021 at 05:05:39PM +0530, Umang Jain wrote: >>>>>> On 3/11/21 3:03 PM, Marian Cichy wrote: >>>>>>> If gst_buffer_pool_acquire_buffer in gst_libcamera_task_run fails, the >>>>>>> unique_ptr to the request-object gets reset and hence, its destructor >>>>>>> is called. However, the wrap-object points to the same object and is >>>>>>> still alive at this moment. When the task_run-function is finished, the >>>>>>> destructor of the wrap-object is called, which in return calls the >>>>>>> destructor of the request-object again. >>>>>>> >>>>>>> Instead of taking care of both, the request and the wrap-object, we can >>>>>>> move the request to the wrap which will then effectively take care of >>>>>>> the request object automatically. >>>>>> ...take care of the request s/object/object-deletion/ maybe? >>>>>> >>>>>>> Signed-off-by: Marian Cichy <m.cichy@pengutronix.de> >>>>>>> Suggested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> >>>>>>> --- >>>>>>> src/gstreamer/gstlibcamerasrc.cpp | 28 ++++++++++++---------------- >>>>>>> 1 file changed, 12 insertions(+), 16 deletions(-) >>>>>>> >>>>>>> diff --git a/src/gstreamer/gstlibcamerasrc.cpp >>>>>>> b/src/gstreamer/gstlibcamerasrc.cpp >>>>>>> index 636c14df..e86c3d7f 100644 >>>>>>> --- a/src/gstreamer/gstlibcamerasrc.cpp >>>>>>> +++ b/src/gstreamer/gstlibcamerasrc.cpp >>>>>>> @@ -52,19 +52,19 @@ GST_DEBUG_CATEGORY_STATIC(source_debug); >>>>>>> #define GST_CAT_DEFAULT source_debug >>>>>>> >>>>>>> struct RequestWrap { >>>>>>> - RequestWrap(Request *request); >>>>>>> + RequestWrap(std::unique_ptr<Request> request); >>>>>>> ~RequestWrap(); >>>>>>> >>>>>>> void attachBuffer(GstBuffer *buffer); >>>>>>> GstBuffer *detachBuffer(Stream *stream); >>>>>>> >>>>>>> /* For ptr comparison only. */ >>>>>> I think you need to remove this comment too >>>>> Indeed :-) >>>>> >>>>>>> - Request *request_; >>>>>>> + std::unique_ptr<Request> request_; >>>>>>> std::map<Stream *, GstBuffer *> buffers_; >>>>>>> }; >>>>>>> >>>>>>> -RequestWrap::RequestWrap(Request *request) >>>>>>> - : request_(request) >>>>>>> +RequestWrap::RequestWrap(std::unique_ptr<Request> request) >>>>>>> + : request_(std::move(request)) >>>>>>> { >>>>>>> } >>>>>>> >>>>>>> @@ -74,8 +74,6 @@ RequestWrap::~RequestWrap() >>>>>>> if (item.second) >>>>>>> gst_buffer_unref(item.second); >>>>>>> } >>>>>>> - >>>>>>> - delete request_; >>>>>>> } >>>>>>> >>>>>>> void RequestWrap::attachBuffer(GstBuffer *buffer) >>>>>>> @@ -164,7 +162,7 @@ GstLibcameraSrcState::requestCompleted(Request *request) >>>>>>> std::unique_ptr<RequestWrap> wrap = std::move(requests_.front()); >>>>>>> requests_.pop(); >>>>>>> >>>>>>> - g_return_if_fail(wrap->request_ == request); >>>>>>> + g_return_if_fail(wrap->request_.get() == request); >>>>>>> >>>>>>> if ((request->status() == Request::RequestCancelled)) { >>>>>>> GST_DEBUG_OBJECT(src_, "Request was cancelled"); >>>>>>> @@ -268,8 +266,7 @@ gst_libcamera_src_task_run(gpointer user_data) >>>>>>> GstLibcameraSrc *self = GST_LIBCAMERA_SRC(user_data); >>>>>>> GstLibcameraSrcState *state = self->state; >>>>>>> >>>>>>> - std::unique_ptr<Request> request = state->cam_->createRequest(); >>>>>>> - auto wrap = std::make_unique<RequestWrap>(request.get()); >>>>>>> + auto wrap = std::make_unique<RequestWrap>(state->cam_->createRequest()); >>>>>> *1 for marker below >>>>>> >>>>>>> for (GstPad *srcpad : state->srcpads_) { >>>>>>> GstLibcameraPool *pool = gst_libcamera_pad_get_pool(srcpad); >>>>>>> GstBuffer *buffer; >>>>>>> @@ -279,24 +276,23 @@ gst_libcamera_src_task_run(gpointer user_data) >>>>>>> &buffer, nullptr); >>>>>>> if (ret != GST_FLOW_OK) { >>>>>>> /* >>>>>>> - * RequestWrap does not take ownership, and we won't be >>>>>>> - * queueing this one due to lack of buffers. >>>>>>> + * RequestWrap has ownership of the rquest, and we >>>>>> s/rquest/request/ >>>>>> >>>>>>> + * won't be queueing this one due to lack of buffers. >>>>>>> */ >>>>>>> - request.reset(); >>>>>>> + wrap.release(); >>>>>>> break; >>>>>>> } >>>>>>> >>>>>>> wrap->attachBuffer(buffer); >>>>>>> } >>>>>>> >>>>>>> - if (request) { >>>>>>> + if (wrap) { >>>>>> I think this should be `if (wrap.get())` no? `wrap.release()` if hit, >>>>>> will set nullptr to the _managed_ object, so we need to check the >>>>>> managed object for null >>>>> This is what is happening already: >>>>> https://en.cppreference.com/w/cpp/memory/unique_ptr/operator_bool >>>>> >>>>>>> GLibLocker lock(GST_OBJECT(self)); >>>>>>> GST_TRACE_OBJECT(self, "Requesting buffers"); >>>>>>> - state->cam_->queueRequest(request.get()); >>>>>>> + state->cam_->queueRequest(wrap->request_.get()); >>>>>> from *1, createRequest() call can return nullptr, hence you have a >>>>>> >>>>>> std::make_unique<RequestWrap>(nullptr); >>>>>> >>>>>> which will set request_ as nullptr on the wrap object. It is fine as >>>>>> long as wrap->request_ is not used unlike here, in this block. >>>>> That's indeed a problem. Maybe >>>>> >>>>> if (wrap && wrap->request_) >>>>> >>>>> above ? Nicolas, any opinion ? >>>> OR null-check createRequest()'s returned object before wrapping it (i.e. >>>> ensure RequestWrap doesn't wrap a nullptr from the start), so we don't >>>> have to check everything for the wrap->request_ is valid or not. >>> But, as far as I understand, code further down this function still have >>> to run in that case, we can't just do an early return using a null >>> check. That's why I've asked for Nicolas' feedback, he knows this code >>> much better than I do. >> Hmm, right. Something like this should do: >> >> if (!request) { >> GST_ELEMENT_ERROR(self, RESOURCE, NO_SPACE_LEFT,...; >> gst_task_stop(task); >> return; >> } > Something along those lines ? > > commit be081f85bbab0c8b3b5f08d73c5bda504be7f2e6 > Author: Marian Cichy <m.cichy@pengutronix.de> > Date: Thu Mar 11 10:33:25 2021 +0100 > > libcamera: gst: Fix double-free when acquire_buffer fails > > If gst_buffer_pool_acquire_buffer in gst_libcamera_task_run fails, the > unique_ptr to the request-object gets reset and hence, its destructor > is called. However, the wrap-object points to the same object and is > still alive at this moment. When the task_run-function is finished, the > destructor of the wrap-object is called, which in return calls the > destructor of the request-object again. > > Instead of taking care of both, the request and the wrap-object, we can > move the request to the wrap which will then effectively take care of > the request object automatically. > > Signed-off-by: Marian Cichy <m.cichy@pengutronix.de> > Suggested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp > index 636c14dff64e..7b9671200f87 100644 > --- a/src/gstreamer/gstlibcamerasrc.cpp > +++ b/src/gstreamer/gstlibcamerasrc.cpp > @@ -52,19 +52,18 @@ GST_DEBUG_CATEGORY_STATIC(source_debug); > #define GST_CAT_DEFAULT source_debug > > struct RequestWrap { > - RequestWrap(Request *request); > + RequestWrap(std::unique_ptr<Request> request); > ~RequestWrap(); > > void attachBuffer(GstBuffer *buffer); > GstBuffer *detachBuffer(Stream *stream); > > - /* For ptr comparison only. */ > - Request *request_; > + std::unique_ptr<Request> request_; > std::map<Stream *, GstBuffer *> buffers_; > }; > > -RequestWrap::RequestWrap(Request *request) > - : request_(request) > +RequestWrap::RequestWrap(std::unique_ptr<Request> request) > + : request_(std::move(request)) > { > } > > @@ -74,8 +73,6 @@ RequestWrap::~RequestWrap() > if (item.second) > gst_buffer_unref(item.second); > } > - > - delete request_; > } > > void RequestWrap::attachBuffer(GstBuffer *buffer) > @@ -164,7 +161,7 @@ GstLibcameraSrcState::requestCompleted(Request *request) > std::unique_ptr<RequestWrap> wrap = std::move(requests_.front()); > requests_.pop(); > > - g_return_if_fail(wrap->request_ == request); > + g_return_if_fail(wrap->request_.get() == request); > > if ((request->status() == Request::RequestCancelled)) { > GST_DEBUG_OBJECT(src_, "Request was cancelled"); > @@ -269,7 +266,18 @@ gst_libcamera_src_task_run(gpointer user_data) > GstLibcameraSrcState *state = self->state; > > std::unique_ptr<Request> request = state->cam_->createRequest(); > - auto wrap = std::make_unique<RequestWrap>(request.get()); > + if (!request) { > + GST_ELEMENT_ERROR(self, RESOURCE, NO_SPACE_LEFT, > + ("Failed to allocate request for camera '%s'.", > + state->cam_->id().c_str()), > + ("libcamera::Camera::createRequest() failed")); > + gst_task_stop(self->task); > + return; > + } > + > + std::unique_ptr<RequestWrap> wrap = > + std::make_unique<RequestWrap>(std::move(request)); > + > for (GstPad *srcpad : state->srcpads_) { > GstLibcameraPool *pool = gst_libcamera_pad_get_pool(srcpad); > GstBuffer *buffer; > @@ -279,24 +287,23 @@ gst_libcamera_src_task_run(gpointer user_data) > &buffer, nullptr); > if (ret != GST_FLOW_OK) { > /* > - * RequestWrap does not take ownership, and we won't be > - * queueing this one due to lack of buffers. > + * RequestWrap has ownership of the rquest, and we > + * won't be queueing this one due to lack of buffers. > */ > - request.reset(); > + wrap.release(); > break; > } > > wrap->attachBuffer(buffer); > } > > - if (request) { > + if (wrap) { > GLibLocker lock(GST_OBJECT(self)); > GST_TRACE_OBJECT(self, "Requesting buffers"); > - state->cam_->queueRequest(request.get()); > + state->cam_->queueRequest(wrap->request_.get()); > state->requests_.push(std::move(wrap)); > > - /* The request will be deleted in the completion handler. */ > - request.release(); > + /* The RequestWrap will be deleted in the completion handler. */ > } > > GstFlowReturn ret = GST_FLOW_OK; > > Marian, would you be able to test this ? > > Should the same be done in case wrap becomes null, due to a call to > gst_buffer_pool_acquire_buffer() failing, insted of continuing to > processing the flow ? The lack of symmetry between the two codes paths > bothers me, either because there's an issue there, or because I don't > understand why they need to be different :-) This can be addressed in a > subsequent patch, as it's not an issue introduced by this patch. Hi Laurent, I tested this, using: @@ -267,6 +265,7 @@ gst_libcamera_src_task_run(gpointer user_data) { GstLibcameraSrc *self = GST_LIBCAMERA_SRC(user_data); GstLibcameraSrcState *state = self->state; + static int cnt = 0; std::unique_ptr<Request> request = state->cam_->createRequest(); auto wrap = std::make_unique<RequestWrap>(request.get()); @@ -275,7 +274,10 @@ gst_libcamera_src_task_run(gpointer user_data) GstBuffer *buffer; GstFlowReturn ret; - ret = gst_buffer_pool_acquire_buffer(GST_BUFFER_POOL(pool), + if (++cnt == 10) + ret = GST_FLOW_ERROR; + else + ret = gst_buffer_pool_acquire_buffer(GST_BUFFER_POOL(pool), as expected, the code skips the failing buffer, than continues it works, no problems whatsoever. So it's fine. I see there is still ongoing discussion with you and Nicolas, so I'd say I wait before sending V3 until it's clear? Marian >>>>>>> state->requests_.push(std::move(wrap)); >>>>>>> >>>>>>> - /* The request will be deleted in the completion handler. */ >>>>>>> - request.release(); >>>>>>> + /* The RequestWrap will be deleted in the completion handler. */ >>>>>>> } >>>>>>> >>>>>>> GstFlowReturn ret = GST_FLOW_OK;
Hi Marian, On Fri, Mar 12, 2021 at 06:59:11PM +0100, Marian Cichy wrote: > On 3/12/21 12:14 AM, Laurent Pinchart wrote: > > On Thu, Mar 11, 2021 at 04:02:20PM -0500, Nicolas Dufresne wrote: > >> Le jeudi 11 mars 2021 à 14:22 +0200, Laurent Pinchart a écrit : > >>> On Thu, Mar 11, 2021 at 05:47:59PM +0530, Umang Jain wrote: > >>>> On 3/11/21 5:29 PM, Laurent Pinchart wrote: > >>>>> On Thu, Mar 11, 2021 at 05:05:39PM +0530, Umang Jain wrote: > >>>>>> On 3/11/21 3:03 PM, Marian Cichy wrote: > >>>>>>> If gst_buffer_pool_acquire_buffer in gst_libcamera_task_run fails, the > >>>>>>> unique_ptr to the request-object gets reset and hence, its destructor > >>>>>>> is called. However, the wrap-object points to the same object and is > >>>>>>> still alive at this moment. When the task_run-function is finished, the > >>>>>>> destructor of the wrap-object is called, which in return calls the > >>>>>>> destructor of the request-object again. > >>>>>>> > >>>>>>> Instead of taking care of both, the request and the wrap-object, we can > >>>>>>> move the request to the wrap which will then effectively take care of > >>>>>>> the request object automatically. > >>>>>> ...take care of the request s/object/object-deletion/ maybe? > >>>>>> > >>>>>>> Signed-off-by: Marian Cichy <m.cichy@pengutronix.de> > >>>>>>> Suggested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > >>>>>>> --- > >>>>>>> src/gstreamer/gstlibcamerasrc.cpp | 28 ++++++++++++---------------- > >>>>>>> 1 file changed, 12 insertions(+), 16 deletions(-) > >>>>>>> > >>>>>>> diff --git a/src/gstreamer/gstlibcamerasrc.cpp > >>>>>>> b/src/gstreamer/gstlibcamerasrc.cpp > >>>>>>> index 636c14df..e86c3d7f 100644 > >>>>>>> --- a/src/gstreamer/gstlibcamerasrc.cpp > >>>>>>> +++ b/src/gstreamer/gstlibcamerasrc.cpp > >>>>>>> @@ -52,19 +52,19 @@ GST_DEBUG_CATEGORY_STATIC(source_debug); > >>>>>>> #define GST_CAT_DEFAULT source_debug > >>>>>>> > >>>>>>> struct RequestWrap { > >>>>>>> - RequestWrap(Request *request); > >>>>>>> + RequestWrap(std::unique_ptr<Request> request); > >>>>>>> ~RequestWrap(); > >>>>>>> > >>>>>>> void attachBuffer(GstBuffer *buffer); > >>>>>>> GstBuffer *detachBuffer(Stream *stream); > >>>>>>> > >>>>>>> /* For ptr comparison only. */ > >>>>>> I think you need to remove this comment too > >>>>> Indeed :-) > >>>>> > >>>>>>> - Request *request_; > >>>>>>> + std::unique_ptr<Request> request_; > >>>>>>> std::map<Stream *, GstBuffer *> buffers_; > >>>>>>> }; > >>>>>>> > >>>>>>> -RequestWrap::RequestWrap(Request *request) > >>>>>>> - : request_(request) > >>>>>>> +RequestWrap::RequestWrap(std::unique_ptr<Request> request) > >>>>>>> + : request_(std::move(request)) > >>>>>>> { > >>>>>>> } > >>>>>>> > >>>>>>> @@ -74,8 +74,6 @@ RequestWrap::~RequestWrap() > >>>>>>> if (item.second) > >>>>>>> gst_buffer_unref(item.second); > >>>>>>> } > >>>>>>> - > >>>>>>> - delete request_; > >>>>>>> } > >>>>>>> > >>>>>>> void RequestWrap::attachBuffer(GstBuffer *buffer) > >>>>>>> @@ -164,7 +162,7 @@ GstLibcameraSrcState::requestCompleted(Request *request) > >>>>>>> std::unique_ptr<RequestWrap> wrap = std::move(requests_.front()); > >>>>>>> requests_.pop(); > >>>>>>> > >>>>>>> - g_return_if_fail(wrap->request_ == request); > >>>>>>> + g_return_if_fail(wrap->request_.get() == request); > >>>>>>> > >>>>>>> if ((request->status() == Request::RequestCancelled)) { > >>>>>>> GST_DEBUG_OBJECT(src_, "Request was cancelled"); > >>>>>>> @@ -268,8 +266,7 @@ gst_libcamera_src_task_run(gpointer user_data) > >>>>>>> GstLibcameraSrc *self = GST_LIBCAMERA_SRC(user_data); > >>>>>>> GstLibcameraSrcState *state = self->state; > >>>>>>> > >>>>>>> - std::unique_ptr<Request> request = state->cam_->createRequest(); > >>>>>>> - auto wrap = std::make_unique<RequestWrap>(request.get()); > >>>>>>> + auto wrap = std::make_unique<RequestWrap>(state->cam_->createRequest()); > >>>>>> *1 for marker below > >>>>>> > >>>>>>> for (GstPad *srcpad : state->srcpads_) { > >>>>>>> GstLibcameraPool *pool = gst_libcamera_pad_get_pool(srcpad); > >>>>>>> GstBuffer *buffer; > >>>>>>> @@ -279,24 +276,23 @@ gst_libcamera_src_task_run(gpointer user_data) > >>>>>>> &buffer, nullptr); > >>>>>>> if (ret != GST_FLOW_OK) { > >>>>>>> /* > >>>>>>> - * RequestWrap does not take ownership, and we won't be > >>>>>>> - * queueing this one due to lack of buffers. > >>>>>>> + * RequestWrap has ownership of the rquest, and we > >>>>>> s/rquest/request/ > >>>>>> > >>>>>>> + * won't be queueing this one due to lack of buffers. > >>>>>>> */ > >>>>>>> - request.reset(); > >>>>>>> + wrap.release(); > >>>>>>> break; > >>>>>>> } > >>>>>>> > >>>>>>> wrap->attachBuffer(buffer); > >>>>>>> } > >>>>>>> > >>>>>>> - if (request) { > >>>>>>> + if (wrap) { > >>>>>> I think this should be `if (wrap.get())` no? `wrap.release()` if hit, > >>>>>> will set nullptr to the _managed_ object, so we need to check the > >>>>>> managed object for null > >>>>> This is what is happening already: > >>>>> https://en.cppreference.com/w/cpp/memory/unique_ptr/operator_bool > >>>>> > >>>>>>> GLibLocker lock(GST_OBJECT(self)); > >>>>>>> GST_TRACE_OBJECT(self, "Requesting buffers"); > >>>>>>> - state->cam_->queueRequest(request.get()); > >>>>>>> + state->cam_->queueRequest(wrap->request_.get()); > >>>>>> from *1, createRequest() call can return nullptr, hence you have a > >>>>>> > >>>>>> std::make_unique<RequestWrap>(nullptr); > >>>>>> > >>>>>> which will set request_ as nullptr on the wrap object. It is fine as > >>>>>> long as wrap->request_ is not used unlike here, in this block. > >>>>> That's indeed a problem. Maybe > >>>>> > >>>>> if (wrap && wrap->request_) > >>>>> > >>>>> above ? Nicolas, any opinion ? > >>>> OR null-check createRequest()'s returned object before wrapping it (i.e. > >>>> ensure RequestWrap doesn't wrap a nullptr from the start), so we don't > >>>> have to check everything for the wrap->request_ is valid or not. > >>> But, as far as I understand, code further down this function still have > >>> to run in that case, we can't just do an early return using a null > >>> check. That's why I've asked for Nicolas' feedback, he knows this code > >>> much better than I do. > >> Hmm, right. Something like this should do: > >> > >> if (!request) { > >> GST_ELEMENT_ERROR(self, RESOURCE, NO_SPACE_LEFT,...; > >> gst_task_stop(task); > >> return; > >> } > > Something along those lines ? > > > > commit be081f85bbab0c8b3b5f08d73c5bda504be7f2e6 > > Author: Marian Cichy <m.cichy@pengutronix.de> > > Date: Thu Mar 11 10:33:25 2021 +0100 > > > > libcamera: gst: Fix double-free when acquire_buffer fails > > > > If gst_buffer_pool_acquire_buffer in gst_libcamera_task_run fails, the > > unique_ptr to the request-object gets reset and hence, its destructor > > is called. However, the wrap-object points to the same object and is > > still alive at this moment. When the task_run-function is finished, the > > destructor of the wrap-object is called, which in return calls the > > destructor of the request-object again. > > > > Instead of taking care of both, the request and the wrap-object, we can > > move the request to the wrap which will then effectively take care of > > the request object automatically. > > > > Signed-off-by: Marian Cichy <m.cichy@pengutronix.de> > > Suggested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp > > index 636c14dff64e..7b9671200f87 100644 > > --- a/src/gstreamer/gstlibcamerasrc.cpp > > +++ b/src/gstreamer/gstlibcamerasrc.cpp > > @@ -52,19 +52,18 @@ GST_DEBUG_CATEGORY_STATIC(source_debug); > > #define GST_CAT_DEFAULT source_debug > > > > struct RequestWrap { > > - RequestWrap(Request *request); > > + RequestWrap(std::unique_ptr<Request> request); > > ~RequestWrap(); > > > > void attachBuffer(GstBuffer *buffer); > > GstBuffer *detachBuffer(Stream *stream); > > > > - /* For ptr comparison only. */ > > - Request *request_; > > + std::unique_ptr<Request> request_; > > std::map<Stream *, GstBuffer *> buffers_; > > }; > > > > -RequestWrap::RequestWrap(Request *request) > > - : request_(request) > > +RequestWrap::RequestWrap(std::unique_ptr<Request> request) > > + : request_(std::move(request)) > > { > > } > > > > @@ -74,8 +73,6 @@ RequestWrap::~RequestWrap() > > if (item.second) > > gst_buffer_unref(item.second); > > } > > - > > - delete request_; > > } > > > > void RequestWrap::attachBuffer(GstBuffer *buffer) > > @@ -164,7 +161,7 @@ GstLibcameraSrcState::requestCompleted(Request *request) > > std::unique_ptr<RequestWrap> wrap = std::move(requests_.front()); > > requests_.pop(); > > > > - g_return_if_fail(wrap->request_ == request); > > + g_return_if_fail(wrap->request_.get() == request); > > > > if ((request->status() == Request::RequestCancelled)) { > > GST_DEBUG_OBJECT(src_, "Request was cancelled"); > > @@ -269,7 +266,18 @@ gst_libcamera_src_task_run(gpointer user_data) > > GstLibcameraSrcState *state = self->state; > > > > std::unique_ptr<Request> request = state->cam_->createRequest(); > > - auto wrap = std::make_unique<RequestWrap>(request.get()); > > + if (!request) { > > + GST_ELEMENT_ERROR(self, RESOURCE, NO_SPACE_LEFT, > > + ("Failed to allocate request for camera '%s'.", > > + state->cam_->id().c_str()), > > + ("libcamera::Camera::createRequest() failed")); > > + gst_task_stop(self->task); > > + return; > > + } > > + > > + std::unique_ptr<RequestWrap> wrap = > > + std::make_unique<RequestWrap>(std::move(request)); > > + > > for (GstPad *srcpad : state->srcpads_) { > > GstLibcameraPool *pool = gst_libcamera_pad_get_pool(srcpad); > > GstBuffer *buffer; > > @@ -279,24 +287,23 @@ gst_libcamera_src_task_run(gpointer user_data) > > &buffer, nullptr); > > if (ret != GST_FLOW_OK) { > > /* > > - * RequestWrap does not take ownership, and we won't be > > - * queueing this one due to lack of buffers. > > + * RequestWrap has ownership of the rquest, and we > > + * won't be queueing this one due to lack of buffers. > > */ > > - request.reset(); > > + wrap.release(); > > break; > > } > > > > wrap->attachBuffer(buffer); > > } > > > > - if (request) { > > + if (wrap) { > > GLibLocker lock(GST_OBJECT(self)); > > GST_TRACE_OBJECT(self, "Requesting buffers"); > > - state->cam_->queueRequest(request.get()); > > + state->cam_->queueRequest(wrap->request_.get()); > > state->requests_.push(std::move(wrap)); > > > > - /* The request will be deleted in the completion handler. */ > > - request.release(); > > + /* The RequestWrap will be deleted in the completion handler. */ > > } > > > > GstFlowReturn ret = GST_FLOW_OK; > > > > Marian, would you be able to test this ? > > > > Should the same be done in case wrap becomes null, due to a call to > > gst_buffer_pool_acquire_buffer() failing, insted of continuing to > > processing the flow ? The lack of symmetry between the two codes paths > > bothers me, either because there's an issue there, or because I don't > > understand why they need to be different :-) This can be addressed in a > > subsequent patch, as it's not an issue introduced by this patch. > > > Hi Laurent, > > I tested this, using: > > > @@ -267,6 +265,7 @@ gst_libcamera_src_task_run(gpointer user_data) > { > GstLibcameraSrc *self = GST_LIBCAMERA_SRC(user_data); > GstLibcameraSrcState *state = self->state; > + static int cnt = 0; > > std::unique_ptr<Request> request = state->cam_->createRequest(); > auto wrap = std::make_unique<RequestWrap>(request.get()); > @@ -275,7 +274,10 @@ gst_libcamera_src_task_run(gpointer user_data) > GstBuffer *buffer; > GstFlowReturn ret; > > - ret = gst_buffer_pool_acquire_buffer(GST_BUFFER_POOL(pool), > + if (++cnt == 10) > + ret = GST_FLOW_ERROR; > + else > + ret = > gst_buffer_pool_acquire_buffer(GST_BUFFER_POOL(pool), > > > as expected, the code skips the failing buffer, than continues it works, > no problems whatsoever. So it's fine. > > I see there is still ongoing discussion with you and Nicolas, so I'd say > I wait before sending V3 until it's clear? Nicolas gave a Reviewed-by on this, so I think the other issues can be addressed on top. I'll push this. > >>>>>>> state->requests_.push(std::move(wrap)); > >>>>>>> > >>>>>>> - /* The request will be deleted in the completion handler. */ > >>>>>>> - request.release(); > >>>>>>> + /* The RequestWrap will be deleted in the completion handler. */ > >>>>>>> } > >>>>>>> > >>>>>>> GstFlowReturn ret = GST_FLOW_OK;
diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp index 636c14df..e86c3d7f 100644 --- a/src/gstreamer/gstlibcamerasrc.cpp +++ b/src/gstreamer/gstlibcamerasrc.cpp @@ -52,19 +52,19 @@ GST_DEBUG_CATEGORY_STATIC(source_debug); #define GST_CAT_DEFAULT source_debug struct RequestWrap { - RequestWrap(Request *request); + RequestWrap(std::unique_ptr<Request> request); ~RequestWrap(); void attachBuffer(GstBuffer *buffer); GstBuffer *detachBuffer(Stream *stream); /* For ptr comparison only. */ - Request *request_; + std::unique_ptr<Request> request_; std::map<Stream *, GstBuffer *> buffers_; }; -RequestWrap::RequestWrap(Request *request) - : request_(request) +RequestWrap::RequestWrap(std::unique_ptr<Request> request) + : request_(std::move(request)) { } @@ -74,8 +74,6 @@ RequestWrap::~RequestWrap() if (item.second) gst_buffer_unref(item.second); } - - delete request_; } void RequestWrap::attachBuffer(GstBuffer *buffer) @@ -164,7 +162,7 @@ GstLibcameraSrcState::requestCompleted(Request *request) std::unique_ptr<RequestWrap> wrap = std::move(requests_.front()); requests_.pop(); - g_return_if_fail(wrap->request_ == request); + g_return_if_fail(wrap->request_.get() == request); if ((request->status() == Request::RequestCancelled)) { GST_DEBUG_OBJECT(src_, "Request was cancelled"); @@ -268,8 +266,7 @@ gst_libcamera_src_task_run(gpointer user_data) GstLibcameraSrc *self = GST_LIBCAMERA_SRC(user_data); GstLibcameraSrcState *state = self->state; - std::unique_ptr<Request> request = state->cam_->createRequest(); - auto wrap = std::make_unique<RequestWrap>(request.get()); + auto wrap = std::make_unique<RequestWrap>(state->cam_->createRequest()); for (GstPad *srcpad : state->srcpads_) { GstLibcameraPool *pool = gst_libcamera_pad_get_pool(srcpad); GstBuffer *buffer; @@ -279,24 +276,23 @@ gst_libcamera_src_task_run(gpointer user_data) &buffer, nullptr); if (ret != GST_FLOW_OK) { /* - * RequestWrap does not take ownership, and we won't be - * queueing this one due to lack of buffers. + * RequestWrap has ownership of the rquest, and we + * won't be queueing this one due to lack of buffers. */ - request.reset(); + wrap.release(); break; } wrap->attachBuffer(buffer); } - if (request) { + if (wrap) { GLibLocker lock(GST_OBJECT(self)); GST_TRACE_OBJECT(self, "Requesting buffers"); - state->cam_->queueRequest(request.get()); + state->cam_->queueRequest(wrap->request_.get()); state->requests_.push(std::move(wrap)); - /* The request will be deleted in the completion handler. */ - request.release(); + /* The RequestWrap will be deleted in the completion handler. */ } GstFlowReturn ret = GST_FLOW_OK;
If gst_buffer_pool_acquire_buffer in gst_libcamera_task_run fails, the unique_ptr to the request-object gets reset and hence, its destructor is called. However, the wrap-object points to the same object and is still alive at this moment. When the task_run-function is finished, the destructor of the wrap-object is called, which in return calls the destructor of the request-object again. Instead of taking care of both, the request and the wrap-object, we can move the request to the wrap which will then effectively take care of the request object automatically. Signed-off-by: Marian Cichy <m.cichy@pengutronix.de> Suggested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> --- src/gstreamer/gstlibcamerasrc.cpp | 28 ++++++++++++---------------- 1 file changed, 12 insertions(+), 16 deletions(-)