| Message ID | 20210914121700.122591-3-umang.jain@ideasonboard.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Le mardi 14 septembre 2021 à 17:47 +0530, Umang Jain a écrit : > The test hold a valid reference to convert0_ and sink0_ but > not released. This results in a memory leak and can be checked > via valgrind. Drop the references with test cleanup() virtual > function. > > Valgrind log: > ==95352== definitely lost: 1,688 bytes in 2 blocks > ==95352== indirectly lost: 11,901 bytes in 37 blocks > > The patch fixes the leaks reported by valgrind above to: > ==120397== definitely lost: 0 bytes in 0 blocks > ==120397== indirectly lost: 0 bytes in 0 blocks > > Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> > --- > test/gstreamer/gstreamer_single_stream_test.cpp | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/test/gstreamer/gstreamer_single_stream_test.cpp b/test/gstreamer/gstreamer_single_stream_test.cpp > index d992455c..763a77b1 100644 > --- a/test/gstreamer/gstreamer_single_stream_test.cpp > +++ b/test/gstreamer/gstreamer_single_stream_test.cpp > @@ -70,6 +70,12 @@ protected: > return TestPass; > } > > + void cleanup() override > + { > + gst_object_unref(convert0_); > + gst_object_unref(sink0_); > + } This is correct of course, but I think we could cleanup more the test. With a run/cleanup() set of function, I would expect to be able to loop over and run the test multiple times, but it does not seem to be the case due to the fact the pipeline is not fully constructed in run(). For the leak being fixed: Reviewed-by: Nicolas Dufresne <nicolas.dufresne@collabora.com> > + > private: > GstElement *convert0_; > GstElement *sink0_;
Hi Nicolas, On 9/14/21 11:15 PM, Nicolas Dufresne wrote: > Le mardi 14 septembre 2021 à 17:47 +0530, Umang Jain a écrit : >> The test hold a valid reference to convert0_ and sink0_ but >> not released. This results in a memory leak and can be checked >> via valgrind. Drop the references with test cleanup() virtual >> function. >> >> Valgrind log: >> ==95352== definitely lost: 1,688 bytes in 2 blocks >> ==95352== indirectly lost: 11,901 bytes in 37 blocks >> >> The patch fixes the leaks reported by valgrind above to: >> ==120397== definitely lost: 0 bytes in 0 blocks >> ==120397== indirectly lost: 0 bytes in 0 blocks >> >> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> >> --- >> test/gstreamer/gstreamer_single_stream_test.cpp | 6 ++++++ >> 1 file changed, 6 insertions(+) >> >> diff --git a/test/gstreamer/gstreamer_single_stream_test.cpp b/test/gstreamer/gstreamer_single_stream_test.cpp >> index d992455c..763a77b1 100644 >> --- a/test/gstreamer/gstreamer_single_stream_test.cpp >> +++ b/test/gstreamer/gstreamer_single_stream_test.cpp >> @@ -70,6 +70,12 @@ protected: >> return TestPass; >> } >> >> + void cleanup() override >> + { >> + gst_object_unref(convert0_); >> + gst_object_unref(sink0_); >> + } > This is correct of course, but I think we could cleanup more the test. With a > run/cleanup() set of function, I would expect to be able to loop over and run > the test multiple times, but it does not seem to be the case due to the fact the Indeed, we can and should. I went in a rush to fix the leak, to see if that's all we leak or more! +cc Vedant, Vedant, could you assume ownership of these patches/series and work upon it? I won't have more time to allocate to this and it will generally help you as well to fix the multi stream test on top of this as well and make sure we don't have leaks anywhere. > pipeline is not fully constructed in run(). > > For the leak being fixed: > Reviewed-by: Nicolas Dufresne <nicolas.dufresne@collabora.com> Thanks for taking a look Nicolas. > >> + >> private: >> GstElement *convert0_; >> GstElement *sink0_; >
Hi Umang, I'll take ownership of these patches. Thanks for the fix. I was missing a key info about floating refs, so I assumed that pipeline will take care of lifetime of other elements. Regards, Vedant Paranjape On Tue, 14 Sep, 2021, 23:24 Umang Jain, <umang.jain@ideasonboard.com> wrote: > Hi Nicolas, > > On 9/14/21 11:15 PM, Nicolas Dufresne wrote: > > Le mardi 14 septembre 2021 à 17:47 +0530, Umang Jain a écrit : > >> The test hold a valid reference to convert0_ and sink0_ but > >> not released. This results in a memory leak and can be checked > >> via valgrind. Drop the references with test cleanup() virtual > >> function. > >> > >> Valgrind log: > >> ==95352== definitely lost: 1,688 bytes in 2 blocks > >> ==95352== indirectly lost: 11,901 bytes in 37 blocks > >> > >> The patch fixes the leaks reported by valgrind above to: > >> ==120397== definitely lost: 0 bytes in 0 blocks > >> ==120397== indirectly lost: 0 bytes in 0 blocks > >> > >> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> > >> --- > >> test/gstreamer/gstreamer_single_stream_test.cpp | 6 ++++++ > >> 1 file changed, 6 insertions(+) > >> > >> diff --git a/test/gstreamer/gstreamer_single_stream_test.cpp > b/test/gstreamer/gstreamer_single_stream_test.cpp > >> index d992455c..763a77b1 100644 > >> --- a/test/gstreamer/gstreamer_single_stream_test.cpp > >> +++ b/test/gstreamer/gstreamer_single_stream_test.cpp > >> @@ -70,6 +70,12 @@ protected: > >> return TestPass; > >> } > >> > >> + void cleanup() override > >> + { > >> + gst_object_unref(convert0_); > >> + gst_object_unref(sink0_); > >> + } > > This is correct of course, but I think we could cleanup more the test. > With a > > run/cleanup() set of function, I would expect to be able to loop over > and run > > the test multiple times, but it does not seem to be the case due to the > fact the > > > Indeed, we can and should. I went in a rush to fix the leak, to see if > that's all we leak or more! > > +cc Vedant, > > Vedant, could you assume ownership of these patches/series and work upon > it? I won't have more time to allocate to this and it will generally > help you as well to fix the multi stream test on top of this as well and > make sure we don't have leaks anywhere. > > > pipeline is not fully constructed in run(). > > > > For the leak being fixed: > > Reviewed-by: Nicolas Dufresne <nicolas.dufresne@collabora.com> > > > Thanks for taking a look Nicolas. > > > > >> + > >> private: > >> GstElement *convert0_; > >> GstElement *sink0_; > > >
diff --git a/test/gstreamer/gstreamer_single_stream_test.cpp b/test/gstreamer/gstreamer_single_stream_test.cpp index d992455c..763a77b1 100644 --- a/test/gstreamer/gstreamer_single_stream_test.cpp +++ b/test/gstreamer/gstreamer_single_stream_test.cpp @@ -70,6 +70,12 @@ protected: return TestPass; } + void cleanup() override + { + gst_object_unref(convert0_); + gst_object_unref(sink0_); + } + private: GstElement *convert0_; GstElement *sink0_;
The test hold a valid reference to convert0_ and sink0_ but not released. This results in a memory leak and can be checked via valgrind. Drop the references with test cleanup() virtual function. Valgrind log: ==95352== definitely lost: 1,688 bytes in 2 blocks ==95352== indirectly lost: 11,901 bytes in 37 blocks The patch fixes the leaks reported by valgrind above to: ==120397== definitely lost: 0 bytes in 0 blocks ==120397== indirectly lost: 0 bytes in 0 blocks Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> --- test/gstreamer/gstreamer_single_stream_test.cpp | 6 ++++++ 1 file changed, 6 insertions(+)