gst_all_1.gst-plugins-good: restore support for dlopen libsoup on Darwin#317737
gst_all_1.gst-plugins-good: restore support for dlopen libsoup on Darwin#317737reckenrode merged 2 commits intoNixOS:stagingfrom
Conversation
ofborg
bot
added
10.rebuild-darwin: 11-100
paparodeo
left a comment
paparodeo
left a comment
There was a problem hiding this comment.
there seems to be a subtle difference in dlopen behavior on linux which could cause issues when a libsoup from a different derivation is loaded than the one specified in the full path.
why does darwin prefer libsoup-2?
There was a problem hiding this comment.
shouldn't the block that sets gstsouphttpsrc_static also be here?
There was a problem hiding this comment.
It’s set on line 133 of the patch. Might be more obvious if I switch the if host_system != 'linux' lines to if false.
There was a problem hiding this comment.
using the full path here will fail for linux when the resolved files differ (hardlinks ok, copies fail) but will succeed when just using the basename.
dlopen tests
#include <dlfcn.h>
#include <stdbool.h>
#include <stdio.h>
#ifdef __APPLE__
#define SO_SUFFIX "dylib"
#define LIBSOUP_2 "libsoup-2.4.1.dylib"
#define LIBSOUP_3 "libsoup-3.0.0.dylib"
#else
#define SO_SUFFIX "so"
#define LIBSOUP_2 "libsoup-2.4.so.1"
#define LIBSOUP_3 "libsoup-3.0.so.0"
#endif
#define LIBSOUP_2_SYMLNK "libsoup-2.4." SO_SUFFIX
#define LIBSOUP_3_SYMLNK "libsoup-3.0." SO_SUFFIX
int main(void)
{
const char * const libs [] = {
LIBSOUP_3,
LIBSOUP_3_SYMLNK,
LIBSOUP_2,
LIBSOUP_2_SYMLNK,
SOUP3_PATH "/" LIBSOUP_3,
SOUP3_PATH "/" LIBSOUP_3_SYMLNK,
SOUP2_PATH "/" LIBSOUP_2,
SOUP2_PATH "/" LIBSOUP_2_SYMLNK,
};
for (size_t i = 0; i < sizeof libs / sizeof *libs; i++) {
bool opened = dlopen(libs[i], RTLD_NOW | RTLD_NOLOAD) != NULL;
printf(opened ? "opened succeeded: %s\n" :
"open failed: %s\n", libs[i]);
}
return 0;
}$ nix-build '<nixpkgs>' -A libsoup_3 -A libsoup
$ $CC -Wall main.c -L$PWD/result-2/lib -Wl,-R$PWD/result-2/lib -lsoup-2.4 -o test -DSOUP3_PATH=\"$PWD/result/lib\" -DSOUP2_PATH=\"$PWD/result-2/lib\"
$ ./test
open failed: libsoup-3.0.so.0
open failed: libsoup-3.0.so
opened succeeded: libsoup-2.4.so.1
opened succeeded: libsoup-2.4.so
open failed: /home/anna/src/dlopen/result/lib/libsoup-3.0.so.0
open failed: /home/anna/src/dlopen/result/lib/libsoup-3.0.so
opened succeeded: /home/anna/src/dlopen/result-2/lib/libsoup-2.4.so.1
opened succeeded: /home/anna/src/dlopen/result-2/lib/libsoup-2.4.so
$ cp -r result-2/ copy
$ $CC -Wall main.c -L$PWD/result-2/lib -Wl,-R$PWD/result-2/lib -lsoup-2.4 -o test -DSOUP3_PATH=\"$PWD/result/lib\" -DSOUP2_PATH=\"$PWD/copy/lib\"
$ ./test
open failed: libsoup-3.0.so.0
open failed: libsoup-3.0.so
opened succeeded: libsoup-2.4.so.1
opened succeeded: libsoup-2.4.so
open failed: /home/anna/src/dlopen/result/lib/libsoup-3.0.so.0
open failed: /home/anna/src/dlopen/result/lib/libsoup-3.0.so
open failed: /home/anna/src/dlopen/copy/lib/libsoup-2.4.so.1
open failed: /home/anna/src/dlopen/copy/lib/libsoup-2.4.soThere was a problem hiding this comment.
Linux doesn’t use the full path for libsoup 2.4. It only uses it for libsoup3, which is duplicating the behavior of #276408 (though I wonder if the trailing / should be included in the substitution rather than in the patch).
There was a problem hiding this comment.
which libsoup the testcode is linking is irrelevant as the same issue happens when linking to libsoup_3 and then trying to dlopen libsoup_3 using the fullpath of a copy.
dlopen tests with libsoup_3
$ $CC -Wall main.c -L$PWD/result/lib -Wl,-R$PWD/result/lib -lsoup-3.0 -o test -DSOUP3_PATH=\"$PWD/result/lib\" -DSOUP2_PATH=\"$PWD/result-2/lib\"
$ ./test
opened succeeded: libsoup-3.0.so.0
opened succeeded: libsoup-3.0.so
open failed: libsoup-2.4.so.1
open failed: libsoup-2.4.so
opened succeeded: /home/anna/src/dlopen/result/lib/libsoup-3.0.so.0
opened succeeded: /home/anna/src/dlopen/result/lib/libsoup-3.0.so
open failed: /home/anna/src/dlopen/result-2/lib/libsoup-2.4.so.1
open failed: /home/anna/src/dlopen/result-2/lib/libsoup-2.4.so
$ cp -r result/ libsoup3-copy
$ $CC -Wall main.c -L$PWD/result/lib -Wl,-R$PWD/result/lib -lsoup-3.0 -o test -DSOUP3_PATH=\"$PWD/libsoup3-copy/lib\" -DSOUP2_PATH=\"$PWD/result-2/lib\"
$ ./test
opened succeeded: libsoup-3.0.so.0
opened succeeded: libsoup-3.0.so
open failed: libsoup-2.4.so.1
open failed: libsoup-2.4.so
open failed: /home/anna/src/dlopen/libsoup3-copy/lib/libsoup-3.0.so.0
open failed: /home/anna/src/dlopen/libsoup3-copy/lib/libsoup-3.0.so
open failed: /home/anna/src/dlopen/result-2/lib/libsoup-2.4.so.1
open failed: /home/anna/src/dlopen/result-2/lib/libsoup-2.4.soThere was a problem hiding this comment.
In the update, I populate the trailing slash in the substitution. Should I have separate patches for both Linux and Darwin (to preserve the libsoup3 first checks for Linux)?
There was a problem hiding this comment.
switching the order of the search for loaded libsoup doesn't make a difference -- one libsoup is loaded or none are search order doesn't change that. when no libsoups are loaded, linux will load libsoup3 since the path for libsoup2 is empty. I'm not clear why darwin needs to load libsoup2 if no libsoups are loaded as it changes the default and diverges from linux behavior.
to fix the subtle linux issue could do something like (untested)
diff --git a/pkgs/development/libraries/gstreamer/good/souploader.diff b/pkgs/development/libraries/gstreamer/good/souploader.diff
index e8625191f900..7e953648d051 100644
--- a/pkgs/development/libraries/gstreamer/good/souploader.diff
+++ b/pkgs/development/libraries/gstreamer/good/souploader.diff
@@ -24,7 +24,7 @@ diff --git a/subprojects/gst-plugins-good/ext/soup/gstsouploader.c b/subprojects
index 9192e4dac5..d91b466489 100644
--- a/subprojects/gst-plugins-good/ext/soup/gstsouploader.c
+++ b/subprojects/gst-plugins-good/ext/soup/gstsouploader.c
-@@ -34,12 +34,17 @@ GST_DEBUG_CATEGORY (gst_soup_debug);
+@@ -34,12 +34,21 @@ GST_DEBUG_CATEGORY (gst_soup_debug);
#ifndef LINK_SOUP
@@ -35,16 +35,20 @@ index 9192e4dac5..d91b466489 100644
#endif
+#if defined(__APPLE__)
++#define MAKE_LIB_PATH(dirname, basename) (dirname basename)
+#define LIBSOUP_3_SONAME "libsoup-3.0.0.dylib"
+#define LIBSOUP_2_SONAME "libsoup-2.4.1.dylib"
+#else
++#define MAKE_LIB_PATH(dirname, basename) (basename)
#define LIBSOUP_3_SONAME "libsoup-3.0.so.0"
#define LIBSOUP_2_SONAME "libsoup-2.4.so.1"
+#endif
++#define LIBSOUP_2_SONAME_SEARCH MAKE_LIB_PATH("@nixLibSoup2Path@", LIBSOUP_2_SONAME)
++#define LIBSOUP_3_SONAME_SEARCH MAKE_LIB_PATH("@nixLibSoup3Path@", LIBSOUP_3_SONAME)
#define LOAD_SYMBOL(name) G_STMT_START { \
if (!g_module_symbol (module, G_STRINGIFY (name), (gpointer *) &G_PASTE (vtable->_, name))) { \
-@@ -163,16 +168,16 @@ gst_soup_load_library (void)
+@@ -163,16 +172,17 @@ gst_soup_load_library (void)
/* In order to avoid causing conflicts we detect if libsoup 2 or 3 is loaded already.
* If so use that. Otherwise we will try to load our own version to use preferring 3. */
@@ -53,16 +57,17 @@ index 9192e4dac5..d91b466489 100644
- GST_DEBUG ("LibSoup 3 found");
- } else if ((handle = dlopen (LIBSOUP_2_SONAME, RTLD_NOW | RTLD_NOLOAD))) {
- libsoup_sonames[0] = LIBSOUP_2_SONAME;
-+ if ((handle = dlopen ("@nixLibSoup2Path@" LIBSOUP_2_SONAME, RTLD_NOW | RTLD_NOLOAD))) {
-+ libsoup_sonames[0] = "@nixLibSoup2Path@" LIBSOUP_2_SONAME;
++ if ((handle = dlopen (LIBSOUP_2_SONAME_SEARCH, RTLD_NOW | RTLD_NOLOAD))) {
++ libsoup_sonames[0] = LIBSOUP_2_SONAME_SEARCH;
GST_DEBUG ("LibSoup 2 found");
-+ } else if ((handle = dlopen ("@nixLibSoup3Path@" LIBSOUP_3_SONAME, RTLD_NOW | RTLD_NOLOAD))) {
-+ libsoup_sonames[0] = "@nixLibSoup3Path@" LIBSOUP_3_SONAME;
++ } else if ((handle = dlopen (LIBSOUP_3_SONAME_SEARCH, RTLD_NOW | RTLD_NOLOAD))) {
++ libsoup_sonames[0] = LIBSOUP_3_SONAME_SEARCH;
+ GST_DEBUG ("LibSoup 3 found");
} else {
GST_DEBUG ("Trying all libsoups");
- libsoup_sonames[0] = LIBSOUP_3_SONAME;
- libsoup_sonames[1] = LIBSOUP_2_SONAME;
++ // full path required for all platforms
+ libsoup_sonames[0] = "@nixLibSoup2Path@" LIBSOUP_2_SONAME;
+ libsoup_sonames[1] = "@nixLibSoup3Path@" LIBSOUP_3_SONAME;
}should work so linux uses the basename and apple uses the full path.
There was a problem hiding this comment.
@paparodeo I pushed an update taking a different approach. On Darwin, it uses dyld APIs to check the loaded images for libsoup. If it finds one, it uses that. Otherwise, it defaults to libsoup3 from the store.
Linux should work exactly as it does currently in nixpkgs.
There was a problem hiding this comment.
Technically, this approach isn’t thread safe. An image could be added or removed while the search is being performed. Unfortunately, I don’t know enough about how gstreamer loads plugins whether it is safe to assume that once loaded they remain loaded, and the thread safe APIs that are available require a newer SDK than is available in nixpkgs. However, I was able to successfully use GStreamer with Wine with this patch.
There was a problem hiding this comment.
that looks good! i was playing around with dladdr(dlsym(RTLD_DEFAULT, "soup_get_major_version"), &info) earlier which seems to also provide the image name, though has downside of breaking if the symbol name gets changed or removed.
There was a problem hiding this comment.
Technically, this approach isn’t thread safe. An image could be added or removed while the search is being performed.
this seems like an unlikely event that we can live with as the current version (linux) isn't thread safe either. the image can be unloaded after detection and then will fail to load in the g_module_open call.
I don't think the gstreamer plugins will get unloaded. i thought that at init time the plugin list is searched and loaded and then that's it. tho i could be misremembering as it was 6-7 months ago since i was looking at this code.
[edit] i think i am misremembering.
reckenrode
force-pushed
the
gstreamer-libsoup-darwin-fix
branch
3 times, most recently
from
efd7c49 to
3d1bbca
Compare
There was a problem hiding this comment.
perhaps overly cautious but might want to check that image_path is non-null
There was a problem hiding this comment.
That should only happen when an image is removed, and idx is now out of bounds. I assume by your edit that GStreamer plugins can actually get unloaded?
I force-pushed an implementation of your idea of using dladdr(dlsym(RTLD_DEFAULT, "soup_get_major_version"), &info) to probe for libsoup. If libsoup drops soup_get_major_version, we probably have more problems than just not finding the dylib anymore. It’s only for Darwin because of the glibc bug note at the end of https://linux.die.net/man/3/dladdr.
There was a problem hiding this comment.
I don’t think that’s right for Darwin. glibc defines RTLD_LOCAL as 0x0, but Darwin defines it as 0x4.
https://sourceware.org/git/?p=glibc.git;a=blob;f=bits/dlfcn.h;h=dd5bb46a182d93a6d7217cb1e575456b4b64bf70;hb=HEAD#l38
https://github.com/apple-oss-distributions/dyld/blob/d552c40cd1de105f0ec95008e0e0c0972de43456/include/dlfcn.h#L87
According to POSIX, if you don’t specify RLTD_LOCAL or RTLD_GLOBAL, the behavior is unspecified. On Linux (and FreeBSD), the default is RTLD_LOCAL. On Darwin, it’s apparently RTLD_GLOBAL.
https://pubs.opengroup.org/onlinepubs/009696799/functions/dlopen.html
I’m testing a glib patch to use RTLD_LOCAL explicitly to see if it fits the plugin ordering problem I was still observing.
There was a problem hiding this comment.
See the latest force push. I’m pretty sure that’s a long-standing glib bug with g_module_open on Darwin. I included a fix for that and cleaned up the probing. Darwin now loads libsoup without issue regardless of the order of folders specified on GST_PLUGIN_PATH. Notice how it uses libsoup2 when gst-plugins-bad is first and libsoup3 (with RTLD_LOCAL) when gst-plugins-good is first.
$ rm -f foo.bin && GST_REGISTRY=foo.bin GST_DEBUG='*soup*:6' GST_PLUGIN_PATH=gst-plugins-bad/lib/gstreamer-1.0:gst-plugins-good/lib/gstreamer-1.0 result-gst-2-bin/bin/gst-inspect-1.0 adaptivedemux2
0:00:00.340118125 88901 0x60000077ca50 DEBUG adaptivedemux2-soup gstsouploader.c:184:gst_soup_load_library: LibSoup 2 found
0:00:00.340414708 88901 0x60000077ca50 DEBUG adaptivedemux2-soup gstsouploader.c:229:gst_soup_load_library: Loaded /nix/store/3kp87dcj8r9zrsz6kfnr7ds2y3s4h7zs-libsoup-2.74.3/lib/libsoup-2.4.1.dylib
0:00:00.349393375 88901 0x60000077ca50 DEBUG souputils gstsoupelement.c:38:soup_element_init: binding text domain gst-plugins-good-1.0 to locale dir /nix/store/6vinyky0acyww6a767xq7n1w7zzxm5m9-gst-plugins-good-1.24.2/share/locale
0:00:00.349510500 88901 0x60000077ca50 DEBUG soup gstsouploader.c:184:gst_soup_load_library: LibSoup 2 found
0:00:00.349515500 88901 0x60000077ca50 DEBUG soup gstsouploader.c:229:gst_soup_load_library: Loaded /nix/store/3kp87dcj8r9zrsz6kfnr7ds2y3s4h7zs-libsoup-2.74.3/lib/libsoup-2.4.1.dylib
Plugin Details:
Name adaptivedemux2
Description Adaptive Streaming 2 plugin
Filename gst-plugins-good/lib/gstreamer-1.0/libgstadaptivedemux2.dylib
Version 1.24.2
License LGPL
Source module gst-plugins-good
Documentation https://gstreamer.freedesktop.org/documentation/adaptivedemux2/
Source release date 2024-04-09
Binary package GStreamer Good Plug-ins source release
Origin URL Unknown package origin
dashdemux2: DASH Demuxer
hlsdemux2: HLS Demuxer
mssdemux2: Smooth Streaming demuxer (v2)
3 features:
+-- 3 elements
$ rm -f foo.bin && GST_REGISTRY=foo.bin GST_DEBUG='*soup*:6' GST_PLUGIN_PATH=gst-plugins-good/lib/gstreamer-1.0:gst-plugins-bad/lib/gstreamer-1.0 result-gst-2-bin/bin/gst-inspect-1.0 adaptivedemux2
0:00:00.016612208 92549 0x60000048a1f0 DEBUG adaptivedemux2-soup gstsouploader.c:190:gst_soup_load_library: Trying all libsoups
0:00:00.018981792 92549 0x60000048a1f0 DEBUG adaptivedemux2-soup gstsouploader.c:229:gst_soup_load_library: Loaded /nix/store/fq4a5slwr3m9vyprw6cwhfy4hq0b1ns7-libsoup-3.4.4/lib/libsoup-3.0.0.dylib
0:00:00.023456417 92549 0x60000048a1f0 DEBUG souputils gstsoupelement.c:38:soup_element_init: binding text domain gst-plugins-good-1.0 to locale dir /nix/store/6vinyky0acyww6a767xq7n1w7zzxm5m9-gst-plugins-good-1.24.2/share/locale
0:00:00.023546292 92549 0x60000048a1f0 DEBUG soup gstsouploader.c:190:gst_soup_load_library: Trying all libsoups
0:00:00.023550083 92549 0x60000048a1f0 DEBUG soup gstsouploader.c:229:gst_soup_load_library: Loaded /nix/store/fq4a5slwr3m9vyprw6cwhfy4hq0b1ns7-libsoup-3.4.4/lib/libsoup-3.0.0.dylib
Plugin Details:
Name adaptivedemux2
Description Adaptive Streaming 2 plugin
Filename gst-plugins-good/lib/gstreamer-1.0/libgstadaptivedemux2.dylib
Version 1.24.2
License LGPL
Source module gst-plugins-good
Documentation https://gstreamer.freedesktop.org/documentation/adaptivedemux2/
Source release date 2024-04-09
Binary package GStreamer Good Plug-ins source release
Origin URL Unknown package origin
dashdemux2: DASH Demuxer
hlsdemux2: HLS Demuxer
mssdemux2: Smooth Streaming demuxer (v2)
3 features:
+-- 3 elements
There was a problem hiding this comment.
i thought i was mixing up scanning vs loading and i'd need to look at the code, thus the edit. the reason to check for null is just because the number of loaded objects can decrease even if gstreamer objects only increase.
if protecting against the race when the libsoup module gets unloaded we're not going to be able to avoid the case where we are reading the image path string and the page it is on gets unmapped and we segfault but could limit the damage from reading a garbage string. we also can't do anything if a different libsoup gets loaded after the detection code and before the final g_module_open call.
a couple of minor modifications with your current approach will protect against loading a bad module.
- perform the strdup before the basename / strcmp. this way the path name will not change from under us
- if the name matches do a
dlopen(image_path, RTLD_NOW | RTLD_NOLOAD)to verify that the path we got points to a loaded object -- this mainly is to protect against somehow getting a path that looks like/evil/location/libsoup-2.4.1.dylib. - maybe rather than using strdup do a malloc / strlcpy so if we are reading garbage we don't end up allocating way too much -- this is somewhat paranoid.
this is my hacked version. it uses memcmp rather than basename just cause of testcode not using glib but should get the gist.
somewhat-resiliant-dlopen
bool
somewhat_resilant_thing(void)
{
_Static_assert(sizeof LIBSOUP_3_SONAME > sizeof(char *), "LIBSOUP_3_SONAME is not a char[]");
_Static_assert(sizeof LIBSOUP_2_SONAME > sizeof(char *), "LIBSOUP_2_SONAME is not a char[]");
Dl_info info = { 0 };
char *image_path = NULL;
libsoup_sonames[0] = /* "@nixLibSoup3Path@/" */ LIBSOUP_3_SONAME;
libsoup_sonames[1] = LIBSOUP_2_SONAME;
if (dladdr(dlsym(RTLD_DEFAULT, "soup_get_major_version"), &info) && info.dli_fname != NULL) {
// excessively large path
const size_t bufSize = 4096;
image_path = malloc(bufSize);
if (image_path == NULL) {
return false;
}
// copy path (could segfault if info.dli_fname ends up getting unmapped
strlcpy(image_path, info.dli_fname, bufSize);
const struct {
const char * const name;
const size_t size;
} libs[] = {
{ LIBSOUP_3_SONAME, sizeof LIBSOUP_3_SONAME },
{ LIBSOUP_2_SONAME, sizeof LIBSOUP_2_SONAME },
};
for (size_t i = 0; i < sizeof libs / sizeof *libs; i++) {
// can switch to basename / strcmp here rather than using sizes and memcmp
const char * const end = strchr(image_path, '\0');
const size_t pathSize = end - image_path;
if (pathSize >= libs[i].size
&& memcmp(end - (libs[i].size - 1), libs[i].name, libs[i].size) == 0) {
// ok. now that basename(inage_path) == LIBSOUP_X_SONAME
// verify the image is still loaded to verify it is valid
// and doesn't point somewhere evil and contains the key symbol
void *hndl = dlopen(image_path, RTLD_NOW | RTLD_NOLOAD);
if (hndl != NULL && dlsym(hndl, "soup_get_major_version") != NULL) {
libsoup_sonames[0] = image_path;
}
if (hndl != NULL) {
dlclose(hndl);
}
break;
}
}
}
printf("soname 0 = %s\n", libsoup_sonames[0]);
printf("soname 1 = %s\n", libsoup_sonames[1]);
free(image_path);
return true;
}There was a problem hiding this comment.
was it crashing / giving an error when using 0 rather than RTLD_LOCAL? i'm not clear what's happening with gst-inspect, why does it load so many things? on linux i looked at the loaded modules pldd when gst-play is streaming
The gst-inspect command is the test case from the upstream issue that prompted them to restrict loading libsoup to Linux. It doesn’t cause a crash, though it breaks Wine, but it causes libsoup to issue the following error.
(gst-plugin-scanner:65699): libsoup-ERROR **: 15:11:40.167: libsoup3 symbols detected. Using libsoup2 and libsoup3 in the same process is not supported.
While working on a fix, I noticed that whether the error triggers on Darwin is dependent on the load order of plugins in GST_PLUGIN_PATH. That’s why I initially had it defaulting to libsoup 2.4 on Darwin, which ended up being a workaround for the actual problem (see below). Note that this does not happen on Linux (also see below).
- If gst-bad-plugins is loaded first, which includes the webrtc plugin that indirectly links against libsoup 2.4 via libnice, then libsoup 2.4 will be detected by the soup loader. When the adaptivedemux2 and soup plugins are loaded, they will use libsoup 2.4 instead of libsoup 3.
- If gst-good-plugins is loaded first, then libsoup will raise the error when webrtc is loaded because adaptivedemux and soup did not detect libsoup 2.4 and defaulted to libsoup 3. Due to the Glib bug, this libsoup 3 is loaded globally when it should be loaded locally.
Linux does not experience this problem because 0 is RTLD_LOCAL. Darwin defines RTLD_LOCAL as 0x4, so when it gets 0, it defaults to RTLD_GLOBAL, which is the opposite of what’s wanted by GStreamer. Linux can default to libsoup3 because the symbols it loads are not global. Darwin (without the fix) breaks because they are.
In essence, G_MODULE_BIND_LOCAL does not work on Darwin. GStreamer tries to use it to load libsoup, but it ends up accidentally loading libsoup globally instead. The fix is trivial, but I’d like to go through a staging cycle and see if anything breaks once Glib starts working correctly on Darwin before trying to upstream the fix.
There was a problem hiding this comment.
If gst-good-plugins is loaded first, then libsoup will raise the error when webrtc is loaded because adaptivedemux and soup did not detect libsoup 2.4 and defaulted to libsoup 3. Due to the Glib bug, this libsoup 3 is loaded globally when it should be loaded locally.
got it, it think -- thanks. so libsoup-2.4 is linked in at compile time and isn't loading using g_module_open then, so RTLD_LOCAL matters. and now that makes sense that you're using the g_find_symbol so it is run in the context of the gmodule.dylib.
seems like a good bugfix. nice work!
There was a problem hiding this comment.
so libsoup-2.4 is linked in at compile time and isn't loading using g_module_open then
Right. Plugins (and applications) can link directly to libsoup, which loads libsoup’s symbols globally (and even using two-level namespaces doesn’t seem to matter). The g_find_symbol check I’m doing is more or less how libsoup does its check. If it fails to detect anything, then libsoup shouldn’t find anything either.
There was a problem hiding this comment.
I chose
soup_uri_newas the symbol to check because that’s what libsoup checks.
i think soup_uri_new is only in libsoup-2 https://gitlab.freedesktop.org/gstreamer/gstreamer/-/blob/main/subprojects/gst-plugins-good/ext/soup/gstsouploader.c?ref_type=heads#L199 (it is part of versioned symbols). also not detected on libsoup-3 looking at nm output.
There was a problem hiding this comment.
You’re right. libsoup2 and libsoup3 check different symbols. I’ll change it back to checking soup_get_major_version.
reckenrode
force-pushed
the
gstreamer-libsoup-darwin-fix
branch
2 times, most recently
from
72fe2e5 to
6148c67
Compare
ofborg
bot
added
the
2.status: merge conflict
reckenrode
force-pushed
the
gstreamer-libsoup-darwin-fix
branch
3 times, most recently
from
44320b8 to
c20a288
Compare
There was a problem hiding this comment.
change looks good to me sans merge conflict. tested on master for x64 linux / darwin and seems good. minor nit would be to zero initialize info = {0}; and func = NULL; for paranoia.
tested on x64 linux/darwin (linux output shown below)
$ nix-build -I nixpkgs=. - <<\ENV
let
inherit (import <nixpkgs> {})
buildEnv
glib-networking
gst_all_1
wrapGAppsNoGuiHook
;
in
buildEnv rec {
name = "gstreamer-env";
paths = with gst_all_1; [
gstreamer
gst-plugins-base
gst-plugins-bad
gst-plugins-good
];
nativeBuildInputs = [ wrapGAppsNoGuiHook ];
buildInputs = paths ++ [ glib-networking ];
pathsToLink = [ "/bin" "/share/man" ];
postBuild = ''
rm -- $out/bin/.*
gappsWrapperArgsHook
wrapGAppsHook
'';
}
ENV
$ rm -rf ~/.cache/gstream* ; GST_DEBUG='*soup*:6' result/bin/gst-inspect-1.0 adaptivedemux2
0:00:00.131779254 2205998 0x2629af0 DEBUG adaptivedemux2-soup gstsouploader.c:216:gst_soup_load_library: LibSoup 2 found
0:00:00.131958380 2205998 0x2629af0 DEBUG adaptivedemux2-soup gstsouploader.c:238:gst_soup_load_library: Loaded libsoup-2.4.so.1
0:00:00.137940138 2205998 0x2629af0 DEBUG souputils gstsoupelement.c:37:soup_element_init: binding text domain gst-plugins-good-1.0 to locale dir /nix/store/iwx4012karfxfc18laafdzpym613rky1-gst-plugins-good-1.24.2/share/locale
0:00:00.138040111 2205998 0x2629af0 DEBUG soup gstsouploader.c:216:gst_soup_load_library: LibSoup 2 found
0:00:00.138049501 2205998 0x2629af0 DEBUG soup gstsouploader.c:238:gst_soup_load_library: Loaded libsoup-2.4.so.1
Plugin Details:
Name adaptivedemux2
Description Adaptive Streaming 2 plugin
Filename /nix/store/iwx4012karfxfc18laafdzpym613rky1-gst-plugins-good-1.24.2/lib/gstreamer-1.0/libgstadaptivedemux2.so
Version 1.24.2
License LGPL
Source module gst-plugins-good
Documentation https://gstreamer.freedesktop.org/documentation/adaptivedemux2/
Source release date 2024-04-09
Binary package GStreamer Good Plug-ins source release
Origin URL Unknown package origin
dashdemux2: DASH Demuxer
hlsdemux2: HLS Demuxer
mssdemux2: Smooth Streaming demuxer (v2)
3 features:
+-- 3 elements
$ ./result/bin/gst-play-1.0 "https://ia803206.us.archive.org/9/items/odnt_20200806/02. Marlena Shaw - California Soul.mp3"
Press 'k' to see a list of keyboard shortcuts.
Now playing https://ia803206.us.archive.org/9/items/odnt_20200806/02. Marlena Shaw - California Soul.mp3
Prerolling...
Redistribute latency...
Redistribute latency...
0:00:45.2 / 0:02:57.6
Unlike Linux, Darwin defines `RTLD_LOCAL` as a non-zero value.
reckenrode
force-pushed
the
gstreamer-libsoup-darwin-fix
branch
from
c20a288 to
b3d8d9d
Compare
I fixed the merge conflict and updated the patch to zero-initialize |
|
I also added the backport label since this issue would affect 24.05, and it’s just released. |
|
@ofborg eval |
label needs to target staging-release-24.05 (or whatever the staging label is named) |
ofborg
bot
removed
the
2.status: merge conflict
ofborg
bot
added
10.rebuild-darwin: 501+
There was a problem hiding this comment.
This should really be upstreamed. Looks okay to me at a glance but I am not confident about possible effects.
There was a problem hiding this comment.
I agree, but I want to go through a staging cycle before doing that given that it’s a big semantic change (because I think, “What does this break?” is an obvious question, and having a successful cycle helps answer that).
There was a problem hiding this comment.
I am not really comfortable with large patches like this in-tree. Is it possible to split it into an upstreamable part and keep only a small patch that substitutes path?
There was a problem hiding this comment.
Upstream removed support for dynamically loading libsoup on non-Linux platforms under the assumption only Linux distributions ship both libsoup 2 and libsoup 3. While that’s true (more or less) of Homebrew and MacPorts, nixpkgs is still shipping both even on Darwin.
I plan to try to upstream this patch after the Glib patch is merged, but there’s no guarantee they’ll accept it.
Is it possible to split it into an upstreamable part and keep only a small patch that substitutes path?
Just to make sure I’m understanding correctly: keep souploader.diff as-is (with the substitution patch) and provide a separate patch with the changes I plan to upstream (after the Glib patch is merged)?
There was a problem hiding this comment.
Just to make sure I’m understanding correctly: keep
souploader.diffas-is (with the substitution patch) and provide a separate patch with the changes I plan to upstream (after the Glib patch is merged)?
Yes.
There was a problem hiding this comment.
Sorry about the delay, but I split out the Darwin-specific change to a separate patch in the latest force push.
There was a problem hiding this comment.
I still only see a single GStreamer patch with non-upstreamable substitution.
There was a problem hiding this comment.
Ah, right. I was counting the existing patch for Linux as the second patch, but there also needs to be a separate one for Darwin. I’ll fix that (by including it in the other patch with the substitution).
wegank
added
the
12.approvals: 1
reckenrode
force-pushed
the
gstreamer-libsoup-darwin-fix
branch
from
b3d8d9d to
8196c61
Compare
wegank
removed
the
12.approvals: 1
|
@lilyinstarlight It was suggested I ping you for a review. This fixed GStreamer on Darwin with plugins that use libsoup. |
There was a problem hiding this comment.
Finally getting a chance to look at this, I'm broadly okay with the change and it does appear like it should work around (at least for nixpkgs) the issue outlined at https://gitlab.freedesktop.org/gstreamer/gstreamer/-/issues/1171#note_2290789
I will echo @jtojnar that the gst-plugins-good patch should probably be split into one for the functionality (that could at least be submitted upstream for review) and another for the substitution to embed an absolute fallback path. But I'm not going to personally block this PR on that
wegank
added
12.approvals: 2
Upstream removed support for using dlopen on platforms other than Linux to fix https://gitlab.freedesktop.org/gstreamer/gstreamer/-/issues/1171, but Darwin in nixpkgs still needs to find the libsoup dynamically because nixpkgs packages both. Instead of using `dlopen`, Darwin probes for libsoup symbols.
reckenrode
force-pushed
the
gstreamer-libsoup-darwin-fix
branch
from
8196c61 to
f806d30
Compare
|
@jtojnar @lilyinstarlight I updated the PR to isolate the absolute path patch to |
Looks good to me! We should probably open MRs for upstream review the glib and soup loader patches and at least (assuming they aren't outright accepted) link their discussions in a comment in the package But I'm not interested in blocking this PR over that Thank you for your work on this! |
|
FYI: the backport tag still needs to be changed from |
Thanks for catching this and @lilyinstarlight for fixing the tags! I’m going to merge this, so it makes the next staging cycle. I’m at a gaming convention this weekend, but I can start working on the upstreaming stuff next week. |
|
Backport failed for Please cherry-pick the changes locally and resolve any conflicts. git fetch origin staging-24.05
git worktree add -d .worktree/backport-317737-to-staging-24.05 origin/staging-24.05
cd .worktree/backport-317737-to-staging-24.05
git switch --create backport-317737-to-staging-24.05
git cherry-pick -x bdc870b17ac2f548120194415a67acebc4390441 f806d30ae28a1df7708f2b21ad186255d94d52ff |
|
I created https://gitlab.gnome.org/GNOME/glib/-/merge_requests/4154 to upstream the GLib patch. Update: It’s been merged. |
5 participants
I ran into this issue when trying to enable and use GStreamer in Wine. Wine because gst-plugins-bad links libsoup 2.4 while gst-plugins-good is linked against libsoup 3.
Upstream removed support for using dlopen on platforms other than Linux to fix https://gitlab.freedesktop.org/gstreamer/gstreamer/-/issues/1171, but Darwin in nixpkgs also needs to
dlopenlibsoup because gst-plugins-bad depends on libsoup 2.4 indirectly via libnice.#276408 fixed the issue for Linux, but this PR expands the patch to work with Darwin as well by restoring support for using
dlopenon Darwin. I have tried to maintain the spirit of preferring libsoup3 on Linux when no other libsoup is in use, but I have not tested it. On Darwin, libsoup 2.4 is preferred.Description of changes
Things done
nix.conf? (See Nix manual)sandbox = relaxedsandbox = truenix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/)Add a 👍 reaction to pull requests you find important.