Merge ~sergiodj/ubuntu/+source/gstreamer1.0:use-g_memdup2-where-available into ubuntu/+source/gstreamer1.0:ubuntu/devel

[Sergio Durigan Junior]

Currently, libcamera FTBFS because gstreamer is using a deprecated g_memdup:

/usr/include/gstreamer-1.0/gst/base/gstbytereader.h: In function ‘guint8* gst_byte_reader_dup_data_unchecked(GstByteReader*, guint)’:
/usr/include/gstreamer-1.0/gst/base/gstbytereader.h:365:41: error: ‘void* g_memdup(gconstpointer, guint)’ is deprecated: Use 'g_memdup2' instead [-Werror=deprecated-declarations]
  365 | return (guint8 *) g_memdup (data, size);
      | ^
In file included from /usr/include/glib-2.0/glib.h:82,
                 from /usr/include/gstreamer-1.0/gst/gst.h:27,
                 from ../src/gstreamer/gstlibcamerasrc.h:12,
                 from ../src/gstreamer/gstlibcamerasrc.cpp:34:
/usr/include/glib-2.0/glib/gstrfuncs.h:257:23: note: declared here
  257 | gpointer g_memdup (gconstpointer mem,

This has been fixed by upstream gstreamer, and the patch is simple enough and I think we should adopt it as a delta while Debian doesn't pick it up:

https://gitlab.freedesktop.org/gstreamer/gstreamer/-/commit/b16e96dd878e0c5e7baeb8fad62ca43de1f66982

This is what this MP is about.

I tested rebuilding gstreamer with the upstream patch and then rebuilding libcamera with the new gstreamer package, and the build succeeded.

There's a PPA with the proposed gstreamer package and the libcamera rebuild here:

https://launchpad.net/~sergiodj/+archive/ubuntu/gstreamer-bug

[Utkarsh Gupta (utkarsh)]

* Changelog:
  - [-] old content and logical tag match as expected
  - [✔] changelog entry correct version and targeted codename
  - [✔] changelog entries correct
  - [✔] update-maintainer has been run

* Actual changes:
  - [-] no upstream changes to consider
  - [✔] no further upstream version to consider
  - [-] debian changes look safe

* New Delta:
  - [✔] patches match what was proposed upstream
  - [✔] patches correctly included in debian/patches/series
  - [✔] patches have correct DEP3 metadata
  - [x] patches forwarded to Debian

I don't see the patches being forwarded however I believe that the new version that Debian will incorporate after the freeze should automatically pick this up. So not a problem, I believe. In case you've had different reasons for not forwarding, I'd be interested to hear them. Thanks!

* Build/Test:
  - [✔] build is ok
  - [✔] verified PPA package installs/uninstalls
  - [-] autopkgtest against the PPA package passes
  - [-] sanity checks test fine

I suppose you've done the above two things here, as mentioned in the comment, so I didn't re-do the whole thing.

+1. Thank you!

review: Approve

[Sebastien Bacher (seb128)]

Thanks but the package is currently in sync with Debian and it would be nice to stay this way, could you try to propose the fix there?

[Sergio Durigan Junior (sergiodj)]

Thanks for the review, Utkarsh and Sebastien.

I agree it would be nice to keep the package as a sync. As I said, my rationale for proposing it as an MP is that (a) gstreamer has been blocking the migration of libcamera for a long time now, and (b) given that the fix comes from upstream, it should reach Ubuntu soon when Debian updates the package.

Either way, I will propose a Merge Request to the Debian package and see how it goes.

[Sergio Durigan Junior (sergiodj)]

https://salsa.debian.org/gstreamer-team/gstreamer1.0/-/merge_requests/6

[Heather Ellsworth (hellsworth)]

In an update impish vm, I installed the gstreamer and libcamera packages from the ppa. I installed libcamera-tools (built from the same source) and ran qcam (just from the cli) to test it and it segfaulted and this was logged in journalctl:

Jul 09 11:55:22 impish-fresh kernel: qcam[3176]: segfault at c ip 00007fa604e6c9e6 sp 00007ffee1c4f6b0 error 6 in libcamera.so.0.1.0[7fa604e27000+8d000]
Jul 09 11:55:22 impish-fresh kernel: Code: 00 00 4d 85 e4 74 0d 48 83 c4 08 4c 89 e0 5b 41 5c c3 66 90 4c 8b 25 31 88 07 00 48 89 c3 bf ba 00 00 00 31 c0 e8 da b3 fb ff <41> 89 44 24 0c 4c 89 e0 4c 89 a3 20 00 00 00 48 83 c4 08 5b 41 5c

I don't see any libcamera debugging symbols to dive down further.

Just to make sure qcam hasn't been broken, I went and made sure qcam worked worked in hirsute with the libcamera in the hirsute archive.

[Sebastien Bacher (seb128)]

Thanks Heather for the testing, that's useful and sounds like something to investigate before uploading

Sergio, one other way to deal with the build failure which is due to '[-Werror=deprecated-declarations' would be to patch libcamera to not used that Werror...

still we should confirm that a rebuild is working and not leading also to a segfault

[Sergio Durigan Junior (sergiodj)]

OK, thanks for the further comments guys, and thanks for testing, Heather.

To be totally honest with you guys, I wasn't expecting this MP to blow up like this. Of course, this is an apparent valid backport for gstreamer1.0, but if it affects libcamera and renders it unusable, then I totally agree that this needs more investigation.

This week was my +1 maintenance turn, and this MP was filed because of it. I have other higher priority tasks to complete in the upcoming weeks and I cannot promise that I will be able to continue investigating this issue.

I will put this MP in my TODO list, but it may be a while until I have the time to come back to it. So feel free to take it over from here, or to reject it and pursue another approach, of course. Bear in mind that it may be worth taking this discussion to the Merge Request I filed against Debian's gstreamer1.0, because if they accept and upload the change there it will quietly find its way to Ubuntu and then we will have a buildable-but-useless libcamera in the archive.

Lastly, I can happily enable the building of debuginfo in my PPA if Heather wants. Just let me know.

[Sergio Durigan Junior (sergiodj)]

I won't have time to tackle this anytime soon, so I'm marking this MP as rejected. Hopefully it can be of use to someone who wants to continue the investigation.

Unmerged commits

bde453a... by Sergio Durigan Junior on 2021-07-08

changelog for 1.18.4-2ubuntu1

f61f468... by Sergio Durigan Junior on 2021-07-08

update-maintainer

6147cef... by Sergio Durigan Junior on 2021-07-08

  * d/p/0002-Use-g_memdup2-where-available-and-add-fallback-for-o.patch:
    Use g_memdup2 where available and add fallback to older GLib
    versions. (LP: #1931958)

diff --git a/debian/changelog b/debian/changelog
index 1c5d33b..8bd2ff6 100644
--- a/debian/changelog
+++ b/debian/changelog
@@ -1,3 +1,11 @@
+gstreamer1.0 (1.18.4-2ubuntu1) impish; urgency=medium
+
+  * d/p/0002-Use-g_memdup2-where-available-and-add-fallback-for-o.patch:
+    Use g_memdup2 where available and add fallback to older GLib
+    versions. (LP: #1931958)
+
+ -- Sergio Durigan Junior <[e-mail]>  Thu, 08 Jul 2021 12:34:52 -0400
+
 gstreamer1.0 (1.18.4-2) unstable; urgency=medium
 
   * Upload to unstable.
diff --git a/debian/control b/debian/control
index a5954eb..e70c1d4 100644
--- a/debian/control
+++ b/debian/control
@@ -1,7 +1,8 @@
 Source: gstreamer1.0
 Section: libs
 Priority: optional
-Maintainer: Maintainers of GStreamer packages <[e-mail]>
+Maintainer: Ubuntu Developers <[e-mail]>
+XSBC-Original-Maintainer: Maintainers of GStreamer packages <[e-mail]>
 Uploaders: Sebastian Dröge <[e-mail]>,
            Sjoerd Simons <[e-mail]>
 Build-Depends: debhelper,
diff --git a/debian/patches/0002-Use-g_memdup2-where-available-and-add-fallback-for-o.patch b/debian/patches/0002-Use-g_memdup2-where-available-and-add-fallback-for-o.patch
 new file mode 100644
index 0000000..6d35e3b
--- /dev/null
+++ b/debian/patches/0002-Use-g_memdup2-where-available-and-add-fallback-for-o.patch
@@ -0,0 +1,244 @@
+From: Doug Nazar <[e-mail]>
+Date: Fri, 23 Apr 2021 12:12:58 -0400
+Subject: Use g_memdup2() where available and add fallback for older GLib
+ versions
+
+glib 2.68 deprecates g_memdup(). Replace with g_memdup2() and
+add fallback if compiling against older versions, since we
+want to avoid deprecation warnings.
+
+Part-of: <https://gitlab.freedesktop.org/gstreamer/gstreamer/-/merge_requests/803>
+
+Origin: upstream, https://gitlab.freedesktop.org/gstreamer/gstreamer/-/commit/b16e96dd878e0c5e7baeb8fad62ca43de1f66982
+Bug-Ubuntu: https://bugs.launchpad.net/ubuntu/+source/libcamera/+bug/1931958
+---
+ gst/glib-compat-private.h     | 3 +++
+ gst/gstregistrychunks.c       | 3 ++-
+ libs/gst/base/gstbitwriter.c  | 6 ++++--
+ libs/gst/base/gstbytereader.c | 3 ++-
+ libs/gst/base/gstbytereader.h | 5 ++++-
+ libs/gst/base/gstbytewriter.c | 4 +++-
+ libs/gst/base/gstindex.c      | 3 ++-
+ tests/check/libs/bitwriter.c  | 3 ++-
+ tests/check/libs/bytereader.c | 3 ++-
+ tests/check/libs/bytewriter.c | 5 +++--
+ 10 files changed, 27 insertions(+), 11 deletions(-)
+
+diff --git a/gst/glib-compat-private.h b/gst/glib-compat-private.h
+index 8f37de2..617a50e 100644
+--- a/gst/glib-compat-private.h
++++ b/gst/glib-compat-private.h
+@@ -30,6 +30,9 @@ G_BEGIN_DECLS
+ /* copies */
+ 
+ /* adaptations */
++#if !GLIB_CHECK_VERSION(2, 67, 4)
++#define g_memdup2(ptr,sz) ((G_LIKELY(((guint64)(sz)) < G_MAXUINT)) ? g_memdup(ptr,sz) : (g_abort(),NULL))
++#endif
+ 
+ G_END_DECLS
+ 
+diff --git a/gst/gstregistrychunks.c b/gst/gstregistrychunks.c
+index b352c45..80d0d4f 100644
+--- a/gst/gstregistrychunks.c
++++ b/gst/gstregistrychunks.c
+@@ -40,6 +40,7 @@
+ #include <gst/gstinfo.h>
+ #include <gst/gstenumtypes.h>
+ #include <gst/gstpadtemplate.h>
++#include "glib-compat-private.h"
+ 
+ #include <gst/gstregistrychunks.h>
+ 
+@@ -95,7 +96,7 @@ _strnlen (const gchar * str, gint maxlen)
+   gint _len = _strnlen (inptr, (endptr-inptr)); \
+   if (_len == -1) \
+     goto error_label; \
+-  outptr = g_memdup ((gconstpointer)inptr, _len + 1); \
++  outptr = g_memdup2 ((gconstpointer)inptr, _len + 1); \
+   inptr += _len + 1; \
+ }G_STMT_END
+ 
+diff --git a/libs/gst/base/gstbitwriter.c b/libs/gst/base/gstbitwriter.c
+index 2f7f56b..0947a68 100644
+--- a/libs/gst/base/gstbitwriter.c
++++ b/libs/gst/base/gstbitwriter.c
+@@ -27,6 +27,8 @@
+ #define GST_BIT_WRITER_DISABLE_INLINES
+ #include "gstbitwriter.h"
+ 
++#include "gst/glib-compat-private.h"
++
+ /**
+  * SECTION:gstbitwriter
+  * @title: GstBitWriter
+@@ -200,7 +202,7 @@ gst_bit_writer_reset_and_get_data (GstBitWriter * bitwriter)
+ 
+   data = bitwriter->data;
+   if (bitwriter->owned)
+-    data = g_memdup (data, bitwriter->bit_size >> 3);
++    data = g_memdup2 (data, bitwriter->bit_size >> 3);
+   gst_bit_writer_reset (bitwriter);
+ 
+   return data;
+@@ -232,7 +234,7 @@ gst_bit_writer_reset_and_get_buffer (GstBitWriter * bitwriter)
+   /* we cannot rely on buffers allocated externally, thus let's dup
+    * the data */
+   if (data && !bitwriter->owned)
+-    data = g_memdup (data, size);
++    data = g_memdup2 (data, size);
+ 
+   buffer = gst_buffer_new ();
+   if (data != NULL) {
+diff --git a/libs/gst/base/gstbytereader.c b/libs/gst/base/gstbytereader.c
+index e9c2ec0..c0a16ef 100644
+--- a/libs/gst/base/gstbytereader.c
++++ b/libs/gst/base/gstbytereader.c
+@@ -26,6 +26,7 @@
+ #define GST_BYTE_READER_DISABLE_INLINES
+ #include "gstbytereader.h"
+ 
++#include "gst/glib-compat-private.h"
+ #include <string.h>
+ 
+ /**
+@@ -1222,7 +1223,7 @@ gst_byte_reader_dup_string_utf##bits (GstByteReader * reader, type ** str) \
+     *str = NULL; \
+     return FALSE; \
+   } \
+-  *str = g_memdup (reader->data + reader->byte, size); \
++  *str = g_memdup2 (reader->data + reader->byte, size); \
+   reader->byte += size; \
+   return TRUE; \
+ }
+diff --git a/libs/gst/base/gstbytereader.h b/libs/gst/base/gstbytereader.h
+index b988739..2130a8a 100644
+--- a/libs/gst/base/gstbytereader.h
++++ b/libs/gst/base/gstbytereader.h
+@@ -362,7 +362,10 @@ static inline guint8 *
+ gst_byte_reader_dup_data_unchecked (GstByteReader * reader, guint size)
+ {
+   gconstpointer data = gst_byte_reader_get_data_unchecked (reader, size);
+-  return (guint8 *) g_memdup (data, size);
++  guint8 *dup_data = (guint8 *) g_malloc (size);
++
++  memcpy (dup_data, data, size);
++  return dup_data;
+ }
+ 
+ /* Unchecked variants that should not be used */
+diff --git a/libs/gst/base/gstbytewriter.c b/libs/gst/base/gstbytewriter.c
+index 335f22f..aec1093 100644
+--- a/libs/gst/base/gstbytewriter.c
++++ b/libs/gst/base/gstbytewriter.c
+@@ -25,6 +25,8 @@
+ #define GST_BYTE_WRITER_DISABLE_INLINES
+ #include "gstbytewriter.h"
+ 
++#include "gst/glib-compat-private.h"
++
+ /**
+  * SECTION:gstbytewriter
+  * @title: GstByteWriter
+@@ -236,7 +238,7 @@ gst_byte_writer_reset_and_get_data (GstByteWriter * writer)
+ 
+   data = (guint8 *) writer->parent.data;
+   if (!writer->owned)
+-    data = g_memdup (data, writer->parent.size);
++    data = g_memdup2 (data, writer->parent.size);
+   writer->parent.data = NULL;
+   gst_byte_writer_reset (writer);
+ 
+diff --git a/libs/gst/base/gstindex.c b/libs/gst/base/gstindex.c
+index d54e0fe..964508e 100644
+--- a/libs/gst/base/gstindex.c
++++ b/libs/gst/base/gstindex.c
+@@ -61,6 +61,7 @@
+ #endif
+ 
+ #include <gst/gst.h>
++#include "gst/glib-compat-private.h"
+ 
+ /* Index signals and args */
+ enum
+@@ -798,7 +799,7 @@ gst_index_add_associationv (GstIndex * index, gint id,
+   entry->type = GST_INDEX_ENTRY_ASSOCIATION;
+   entry->id = id;
+   entry->data.assoc.flags = flags;
+-  entry->data.assoc.assocs = g_memdup (list, sizeof (GstIndexAssociation) * n);
++  entry->data.assoc.assocs = g_memdup2 (list, sizeof (GstIndexAssociation) * n);
+   entry->data.assoc.nassocs = n;
+ 
+   gst_index_add_entry (index, entry);
+diff --git a/tests/check/libs/bitwriter.c b/tests/check/libs/bitwriter.c
+index d6f68d3..4628a5b 100644
+--- a/tests/check/libs/bitwriter.c
++++ b/tests/check/libs/bitwriter.c
+@@ -29,6 +29,7 @@
+ #include <gst/check/gstcheck.h>
+ #include <gst/base/gstbitwriter.h>
+ #include <gst/base/gstbitreader.h>
++#include "gst/glib-compat-private.h"
+ 
+ GST_START_TEST (test_initialization)
+ {
+@@ -76,7 +77,7 @@ GST_START_TEST (test_data)
+   fail_unless (gst_bit_writer_put_bits_uint64 (&writer, 0x45, 48));
+   fail_unless_equals_int (gst_bit_writer_get_remaining (&writer), 2048 - 71);
+   fail_unless (gst_bit_writer_align_bytes (&writer, 0));
+-  data = g_memdup (sdata, sizeof (sdata));
++  data = g_memdup2 (sdata, sizeof (sdata));
+   fail_unless (gst_bit_writer_put_bytes (&writer, data, sizeof (sdata)));
+ 
+   gst_bit_reader_init (&reader, gst_bit_writer_get_data (&writer), 256);
+diff --git a/tests/check/libs/bytereader.c b/tests/check/libs/bytereader.c
+index 24a2971..bc4c406 100644
+--- a/tests/check/libs/bytereader.c
++++ b/tests/check/libs/bytereader.c
+@@ -27,6 +27,7 @@
+ #include <gst/gst.h>
+ #include <gst/check/gstcheck.h>
+ #include <gst/base/gstbytereader.h>
++#include "gst/glib-compat-private.h"
+ 
+ #ifndef fail_unless_equals_int64
+ #define fail_unless_equals_int64(a, b)					\
+@@ -574,7 +575,7 @@ GST_START_TEST (test_scan)
+     gint found;
+ 
+     /* dup so valgrind can detect out of bounds access more easily */
+-    m = g_memdup (sync_data, sizeof (sync_data));
++    m = g_memdup2 (sync_data, sizeof (sync_data));
+     gst_byte_reader_init (&reader, m, sizeof (sync_data));
+ 
+     found = gst_byte_reader_masked_scan_uint32_peek (&reader, 0xffffff00,
+diff --git a/tests/check/libs/bytewriter.c b/tests/check/libs/bytewriter.c
+index 89999af..c8017b8 100644
+--- a/tests/check/libs/bytewriter.c
++++ b/tests/check/libs/bytewriter.c
+@@ -27,6 +27,7 @@
+ #include <gst/gst.h>
+ #include <gst/check/gstcheck.h>
+ #include <gst/base/gstbytewriter.h>
++#include "gst/glib-compat-private.h"
+ 
+ GST_START_TEST (test_initialization)
+ {
+@@ -42,7 +43,7 @@ GST_START_TEST (test_initialization)
+           (&writer)), 0);
+   gst_byte_writer_reset (&writer);
+ 
+-  data = g_memdup (sdata, sizeof (sdata));
++  data = g_memdup2 (sdata, sizeof (sdata));
+   gst_byte_writer_init_with_data (&writer, data, sizeof (sdata), FALSE);
+   fail_unless_equals_int (gst_byte_writer_get_pos (&writer), 0);
+   fail_unless_equals_int (gst_byte_writer_get_size (&writer), 0);
+@@ -56,7 +57,7 @@ GST_START_TEST (test_initialization)
+   g_free (data);
+   data = tmp = NULL;
+ 
+-  data = g_memdup (sdata, sizeof (sdata));
++  data = g_memdup2 (sdata, sizeof (sdata));
+   gst_byte_writer_init_with_data (&writer, data, sizeof (sdata), TRUE);
+   fail_unless_equals_int (gst_byte_writer_get_pos (&writer), 0);
+   fail_unless_equals_int (gst_byte_writer_get_size (&writer), sizeof (sdata));
diff --git a/debian/patches/series b/debian/patches/series
index cbeba03..bc3b6bc 100644
--- a/debian/patches/series
+++ b/debian/patches/series
@@ -1 +1,2 @@
 0001-registrybinary-Update-magic-version-string.patch
+0002-Use-g_memdup2-where-available-and-add-fallback-for-o.patch