| Message ID | 20240514174538.195350-1-pobrn@protonmail.com |
|---|---|
| State | Accepted |
| Commit | f113464b3481d76dee158a00c790b44671b382b7 |
| Headers | show |
| Series |
|
| Related | show |
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. > Fixes: fc9783acc6083a ("gstreamer: Provide colorimetry <> ColorSpace mappings") > Signed-off-by: Barnabás Pőcze <pobrn@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);
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". > > > Fixes: fc9783acc6083a ("gstreamer: Provide colorimetry <> ColorSpace mappings") > > Signed-off-by: Barnabás Pőcze <pobrn@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@collabora.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. :-D Nicolas
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@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@collabora.com> Reviewed-by: Laurent Pinchart <laurent.pinchart@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 :-)
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);
The string returned by `gst_video_colorimetry_to_string()` has to be freed, this was missing. Fixes: fc9783acc6083a ("gstreamer: Provide colorimetry <> ColorSpace mappings") Signed-off-by: Barnabás Pőcze <pobrn@protonmail.com> --- src/gstreamer/gstlibcamera-utils.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)