[PATCH v1] gstreamer: Fix string memory leak
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Tue May 14 21:57:37 CEST 2024
On Tue, May 14, 2024 at 03:18:50PM -0400, Nicolas Dufresne wrote:
> Hi,
>
> Le mardi 14 mai 2024 à 21:50 +0300, Laurent Pinchart a écrit :
> > Hi Barnabás,
> >
> > Thank you for the patch.
> >
> > On Tue, May 14, 2024 at 05:45:39PM +0000, Barnabás Pőcze wrote:
> > > The string returned by `gst_video_colorimetry_to_string()`
> > > has to be freed,
> >
> > This is right.
> >
> > > this was missing.
> >
> > But are you sure about this ? The allocated colorimetry_str is passed to
> > gst_structure_set(), which I *think* doesn't copy the string but takes
> > ownership of it.
>
> In glib, default transfer method is "none", which means the caller keeps
> ownership. gst_structure_set() has no annotation thus it implies that it will
> have to copy foreign strings. In general, glib API that takes ownership of a
> string would use the term "take".
Thank you for the explanation. I had a closer look at the code (for my
own education) and it appears to match.
> > > Fixes: fc9783acc6083a ("gstreamer: Provide colorimetry <> ColorSpace mappings")
> > > Signed-off-by: Barnabás Pőcze <pobrn at protonmail.com>
> > > ---
> > > src/gstreamer/gstlibcamera-utils.cpp | 2 +-
> > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/src/gstreamer/gstlibcamera-utils.cpp b/src/gstreamer/gstlibcamera-utils.cpp
> > > index e163ce41..ec4da435 100644
> > > --- a/src/gstreamer/gstlibcamera-utils.cpp
> > > +++ b/src/gstreamer/gstlibcamera-utils.cpp
> > > @@ -385,7 +385,7 @@ gst_libcamera_stream_configuration_to_caps(const StreamConfiguration &stream_cfg
> > >
> > > if (stream_cfg.colorSpace) {
> > > GstVideoColorimetry colorimetry = colorimetry_from_colorspace(stream_cfg.colorSpace.value());
> > > - gchar *colorimetry_str = gst_video_colorimetry_to_string(&colorimetry);
> > > + g_autofree gchar *colorimetry_str = gst_video_colorimetry_to_string(&colorimetry);
> > >
> > > if (colorimetry_str)
> > > gst_structure_set(s, "colorimetry", G_TYPE_STRING, colorimetry_str, nullptr);
>
> Reviewed-by: Nicolas Dufresne <nicolas.dufresne at collabora.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> Note that we should stop hand writing GstCaps/GstStructure, it makes the
> libcamerasrc produce incomplete Raw video caps. This my mistake, I apology.
> Instead we should fill a GstVideoInfo and make the caps using
> gst_video_info_to_caps(). The obvious benefit is that we no longer have to do
> that GstVideoColorimetry to string conversion.
>
> So we should do, StreamConfiguration -> GstVideoInfo -> caps. This will fix many
> issues we are having these days.
Patches are welcome :-)
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list