GNOME Bugzilla – Bug 679115
Windows: Make the GStreamer DLLs/data relocatable
Last modified: 2017-07-14 15:13:52 UTC
Created attachment 217594 [details] [review] Windows: Make GStreamer Data and plug-ins relocatable. Hi, It seems from the win32/common/config.h that the paths of plugins, libraries, data, locale stuff are hardcoded to some sub-folder under c:\gstreamer on Windows builds. This patch adds functionality to the various sources in gst/ to make up the corresponding paths dynamically at run-time, so that the plug in DLLs and other data can be moved without rebuilding the code. This is done in a similar way to code in the GTK+ stack, as installation destinations of Windows programs are subject to change from installation to installation. Thanks for the time, with blessings!
Comment on attachment 217594 [details] [review] Windows: Make GStreamer Data and plug-ins relocatable. If I'm not mistaken this leaks directory strings all over the place, e.g. the return values of g_win32_get_package_installation_directory_of_module(), but also the return values of _get_{libdir,plugin_scananer_dir}().
Created attachment 218883 [details] [review] Make GStreamer relocatable on Windows (take 2) Hello Tim-Philipp, Yeah, I missed out on the g_free() calls that is needed, so I redid the patch here. Note that I did not do the g_free() calls in gstpluginloader.c and gstpreset.c as their respective return variables were g_free'd initially. With blessings, thanks.
Ok, I think this still leaks the return values of g_win32_get_package_installation_directory_of_module(), but we can fix this up. I think I might prefer a solution where get_foo_dir() is always called, and for the non-win32 case it just returns g_strdup (FOODIR) then or so, that way the code pa.ths and g_free()s etc. are consistent and in the unconditional code path, but that's also easy to do. Adding Andoni to CC in case he's got an opinion on this as well. Setting target to 1.1.x dev cycle.
Review of attachment 218883 [details] [review]: Sounds like a good idea in general but: ::: gst/gst.c @@ +124,3 @@ +{ + return g_build_filename ( + g_win32_get_package_installation_directory_of_module (_priv_gst_dll_handle), Leaks return value of g_win32_get...() @@ +125,3 @@ + return g_build_filename ( + g_win32_get_package_installation_directory_of_module (_priv_gst_dll_handle), + "lib", If it is called lib or not depends on the parameters passed to configure, i.e. --libdir. Could also be called anything else @@ +1129,2 @@ g_type_class_unref (g_type_class_peek (gst_allocator_flags_get_type ())); Why these unrelated changes? :) ::: gst/gstpluginloader.c @@ +65,3 @@ +{ + return g_build_filename ( + g_win32_get_package_installation_directory_of_module (_priv_gst_dll_handle), Leaking return value @@ +67,3 @@ + g_win32_get_package_installation_directory_of_module (_priv_gst_dll_handle), +#ifdef _DEBUG + "debug", Is this standard? @@ +69,3 @@ + "debug", +#endif + "lib", This one is in libexec, not lib. Also configurable with --libexecdir at configure @@ +73,3 @@ + NULL); +} +#define GST_PLUGIN_SCANNER_INSTALLED get_plugin_scanner_dir() You'll leak the memory here because the code below assumes it's a static string ::: gst/gstpreset.c @@ +110,3 @@ +{ + return g_build_filename ( + g_win32_get_package_installation_directory_of_module (_priv_gst_dll_handle), Leaking return value @@ +118,3 @@ +#undef GST_DATADIR +#endif +#define GST_DATADIR get_datadir() Leaking the memory below ::: gst/gstregistry.c @@ +142,3 @@ +{ + return g_build_filename ( + g_win32_get_package_installation_directory_of_module (_priv_gst_dll_handle), Leaking return value @@ +144,3 @@ + g_win32_get_package_installation_directory_of_module (_priv_gst_dll_handle), +#ifdef _DEBUG + "debug", Is this standard? @@ +1609,3 @@ /* add the main (installed) library path */ + GST_DEBUG ("scanning main plugins %s", plugin_dir); + changed |= gst_registry_scan_path_internal (&context, plugin_dir); There is already code like this in gstregistry.c, 5 lines below :)
Created attachment 265680 [details] [review] Make The GStreamer Installation Relocatable on Windows (take iii) Hi, Sorry it took much longer than I liked to come back to this issue, but here's my other take on this, hopefully addressing the issues that have been brought up. :) (In reply to comment #4) > ::: gst/gst.c > @@ +124,3 @@ > +{ > + return g_build_filename ( > + g_win32_get_package_installation_directory_of_module > (_priv_gst_dll_handle), > > Leaks return value of g_win32_get...() I think it's fixed in this patch of mine. > @@ +125,3 @@ > + return g_build_filename ( > + g_win32_get_package_installation_directory_of_module > (_priv_gst_dll_handle), > + "lib", > > If it is called lib or not depends on the parameters passed to configure, i.e. > --libdir. Could also be called anything else I think this is the general convention that is used on Windows for the GTK+, so following suit here:) This also applies to MSVC builds, which do not use configure to build. > @@ +1129,2 @@ > g_type_class_unref (g_type_class_peek (gst_allocator_flags_get_type ())); > > > Why these unrelated changes? :) Oops, this patch does not have this anymore :) > ::: gst/gstpluginloader.c > @@ +65,3 @@ > +{ > + return g_build_filename ( > + g_win32_get_package_installation_directory_of_module > (_priv_gst_dll_handle), > > Leaking return value Fixed in the latest version of my patch. > @@ +67,3 @@ > + g_win32_get_package_installation_directory_of_module > (_priv_gst_dll_handle), > +#ifdef _DEBUG > + "debug", > > Is this standard? Nope, but I am following suit from the pre-generated config.h in win32/common. We could always purge this part, if needed. :) > @@ +69,3 @@ > + "debug", > +#endif > + "lib", > > This one is in libexec, not lib. Also configurable with --libexecdir at > configure Putting it here as "lib", as it is a helper. Please advise if libexec is preferred to be set as something else. (perhaps with the other .exe's?) > @@ +73,3 @@ > + NULL); > +} > +#define GST_PLUGIN_SCANNER_INSTALLED get_plugin_scanner_dir() > > You'll leak the memory here because the code below assumes it's a static string Fixed in the latest version of my patch. > ::: gst/gstpreset.c > @@ +110,3 @@ > +{ > + return g_build_filename ( > + g_win32_get_package_installation_directory_of_module > (_priv_gst_dll_handle), > > Leaking return value Fixed in the latest version of my patch. > @@ +118,3 @@ > +#undef GST_DATADIR > +#endif > +#define GST_DATADIR get_datadir() > > Leaking the memory below Fixed in the latest version of my patch. > ::: gst/gstregistry.c > @@ +142,3 @@ > +{ > + return g_build_filename ( > + g_win32_get_package_installation_directory_of_module > (_priv_gst_dll_handle), > > Leaking return value Fixed in the latest version of my patch. > @@ +144,3 @@ > + g_win32_get_package_installation_directory_of_module > (_priv_gst_dll_handle), > +#ifdef _DEBUG > + "debug", > > Is this standard? Same as the "debug" thing above. Let me know if this is preferred without :). > @@ +1609,3 @@ > /* add the main (installed) library path */ > + GST_DEBUG ("scanning main plugins %s", plugin_dir); > + changed |= gst_registry_scan_path_internal (&context, plugin_dir); > > There is already code like this in gstregistry.c, 5 lines below :) Noticed that, so moving a block down:) With blessings, thank you!
Thanks for updating the patch, just one comment then I think it can go in (In reply to comment #5) > > @@ +125,3 @@ > > + return g_build_filename ( > > + g_win32_get_package_installation_directory_of_module > > (_priv_gst_dll_handle), > > + "lib", > > > > If it is called lib or not depends on the parameters passed to configure, i.e. > > --libdir. Could also be called anything else > > I think this is the general convention that is used on Windows for the GTK+, so > following suit here:) This also applies to MSVC builds, which do not use > configure to build. I think you should use something from config.h for this, which is based on the configure parameter. But it's probably ok as is > > @@ +67,3 @@ > > + g_win32_get_package_installation_directory_of_module > > (_priv_gst_dll_handle), > > +#ifdef _DEBUG > > + "debug", > > > > Is this standard? > > Nope, but I am following suit from the pre-generated config.h in win32/common. > We could always purge this part, if needed. :) Sounds ok then :) > > @@ +69,3 @@ > > + "debug", > > +#endif > > + "lib", > > > > This one is in libexec, not lib. Also configurable with --libexecdir at > > configure > > Putting it here as "lib", as it is a helper. Please advise if libexec is > preferred to be set as something else. (perhaps with the other .exe's?) libexec is for executables that are run by the library only and not meant for users to run. bin should contain executables that the user should run (gst-launch, etc) This here should be the path where gst-plugin-scanner.exe is installed.
(In reply to comment #6) > Thanks for updating the patch, just one comment then I think it can go in > > (In reply to comment #5) > > > > @@ +125,3 @@ > > > + return g_build_filename ( > > > + g_win32_get_package_installation_directory_of_module > > > (_priv_gst_dll_handle), > > > + "lib", > > > > > > If it is called lib or not depends on the parameters passed to configure, i.e. > > > --libdir. Could also be called anything else > > > > I think this is the general convention that is used on Windows for the GTK+, so > > following suit here:) This also applies to MSVC builds, which do not use > > configure to build. > > I think you should use something from config.h for this, which is based on the > configure parameter. But it's probably ok as is > I think it's safe to assume that in Windows the directory structure is preserved as bin/ lib/
Review of attachment 265680 [details] [review]: ::: gst/gst.c @@ -502,0 +503,12 @@ +#ifdef G_OS_WIN32 + { + gchar *basedir = g_win32_get_package_installation_directory_of_module (_priv_gst_dll_handle); ... 9 more ... libdir must show the location of the libs directory and not the plugins dir, so basedir/lib instead of basedir/lib/gstreamer-1.0/
I believe this relocation technique can be also very useful in OS X for Applications, as the redistribution of GStreamer in the App implies some relocations too. Maybe a configure option --with-relocation that's enabled by default in Windows and can be turned on/off for other platforms?
(In reply to comment #9) > I believe this relocation technique can be also very useful in OS X for > Applications, as the redistribution of GStreamer in the App implies some > relocations too. > Maybe a configure option --with-relocation that's enabled by default in Windows > and can be turned on/off for other platforms? Getting the location of the GStreamer library (and I mean library, not the application executable) requires platform specific API though. Does something exist for OSX? Then I think we can even enable that by default like in the above patch.
Something like: static gchar * get_prefix_dir (void) { gchar *prefix_dir = NULL; #if defined(HAVE_OS_WINDOWS) prefix_dir = g_win32_get_package_installation_directory_of_module (NULL); #elif defined (HAVE_OS_DARWIN) char pathbuf[PATH_MAX + 1]; uint32_t bufsize = sizeof(pathbuf); gchar *bin_dir; _NSGetExecutablePath(pathbuf, &bufsize); bin_dir = g_dirname(pathbuf); prefix_dir = g_build_path (G_DIR_SEPARATOR_S, bin_dir, "..", NULL); g_free (bin_dir); #elif defined (HAVE_OS_LINUX) gchar *exe_path, *bin_dir; exe_path = g_file_read_link ("/proc/self/exe", NULL); bin_dir = g_dirname(exe_path); prefix_dir = g_build_path (G_DIR_SEPARATOR_S, bin_dir, "..", NULL); g_free (exe_path); g_free (bin_dir); #endif return prerix_dir; }
Can you open a new bug for that for OSX? Also _NSGetExecutablePath() probably returns the application path, not the library path. And same story for /proc/self/exe
Yes, this assume the application is installed in bin/ and libgstreamer-0.10.dll in lib/, that's why prefix is built with bin_dir/.. It's not completely correct though as we should try to get the path of the library and not guess it from the executable
I would refactor than the following patch to: 1) Define a function that returns the installation prefix 2) In Windows and OS X try with the relocated paths. 3) In OS X, try also with the configuration hardcoded path And I wouldn't change anything on Linux. Since the location of the library in OS X can only be guessed through the application dir, I would check both the relocated path and the hardcoded one so that it works too with applications using a system-wide installation of GStreamer in /Library/Frameworks/GStreamer.framework
We could have the following helper functions: get_installation_prefix get_installation_lib_dir get_installation_plugins_dir get_installation_scanner_path if it returns NULL (for Linux or when the path does not exists), we try with directly with the harcoded one, if it returns a path we try first with this path. The OS-specific code is kept in a single place: in get_installation_prefix
Created attachment 268254 [details] [review] Make the GStreamer installation relocatable (take iv) Hi, Here comes another version of my patch for this... > libdir must show the location of the libs directory and not the plugins dir, so > basedir/lib instead of basedir/lib/gstreamer-1.0/ Yup, got that, so this new patch fixes this. Thanks though. > libexec is for executables that are run by the library only and not meant for > users to run. bin should contain executables that the user should run > (gst-launch, etc) > > This here should be the path where gst-plugin-scanner.exe is installed. Hmm, seems that my patch points to <win32_prefix>\lib\gstreamer-1.0\gst-plugin-scanner.exe already, or did I miss something? p.s. I'm not too sure about the Mac or Linux situation regarding relocation, so can't say much about them here. But anyways, thanks for the reviews. With blessings!
commit 043c629e72946f80e251ce149777ab91f184cb29 Author: Chun-wei Fan <fanchunwei@src.gnome.org> Date: Thu Feb 6 14:18:31 2014 +0800 windows: Make GStreamer installation relocatable Use the technique that is now done in GTK+ so that the plugins do not have to be installed in c:\gstreamer\lib\<debug>\gstreamer-$(GSTApiVersion), but can be installed in <parent_folder_of_gstreamer_main_dll>\lib\<debug>\gstreamer-$(GSTApiVersion) or as per g_win32_get_package_installation_directory_of_module() allows. https://bugzilla.gnome.org/show_bug.cgi?id=679115
This patch is still using "lib" instead of "libexec" for the gst-plugin-scanner path. And I think the indentation is not correct either.
commit d12329118377f99e7e3d4a87fd97c28c79b739b2 (HEAD -> master) Author: Tim-Philipp Müller <tim@centricular.com> Date: Fri Jul 14 16:12:25 2017 +0100 win32: find plugin scanner in libexecdir subdir as configured https://bugzilla.gnome.org/show_bug.cgi?id=679115 tpm@xps:~/gst/master/gstreamer$ git push