Remove dependencies on glib from core application #211
Conversation
webserver used only glib types.
upnp_device and upnp_transport used only glib types
The typedef is used only inside upnp_connmgr.c file. It is useless to stay it inside the header file
add_options feature was mixed inside main.c between the options for the core application and the options for the modules. To keep the use of glib API inside the module and to remove the glib from main.c file, the feature has to be splited. The consequence of this patch is that the options must be separated on the command line with "--". ex: $ /usr/local/bin/gmediarender -o gst -- --gstout-videosink=qtvideosink
This patch uses the same arguments just change the tool to parse the command line.
The output's modules are allowed to fet a version string, that output.c displays with output_dump function.
The versions of glib and gst are inside the output module.
The module manages a large part of the system. The UPnP system has its own thread management without any link with glib. Only the gstreamer threads need to use glib API and output already managed the main loop. This patch create a new API inside the output module (loop).
ouput used gint64 type definition, this one is fully equal to int64_t the standard type definition.
This modification may does issues if the string contains no eASCII characters. But the modification is only for protoInfo which is a standard ASCII string. Then no issue should occure. The usage of asprintf and __GNU_SOURCE flag may generate build problem on some platform.
This patch replace the use of gpointer to void *
upnp_connmgr uses GSLIST object. The file gmrender_list.c emulates the GSLIST object from glib2. GmSLIST is less efficient than GSLIST to create new entries, but GmRender creates the list of mime only during the startup. For the rest of functions, the performances should be approximately the same.
There was a problem hiding this comment.
Sorry, I think I missed that in the middle of last year, which was a bit crazy. And github is also not very good bringing new pull requests to an attention.
In general I like the premise to reduce dependencies and clean up code from things that we don't need from dependencies. So all the changes from gint64 -> int64_t and remove of use of gboolean (albeit it should be replace with stdbool not int ) are very welcome.
In the current state, some features are lost, such as documentation of options; this funcitonality would need to be re-implemented.
In the end, there might not be as much gain as you hope there is: an unstripped binary of master is about 368k, while with your version it is about 376k (on 5.10.5-1 (2021-01-09) x86_64 GNU/Linux). However, after stripping, both binaries are 104k (in fact, they only differ by 288 bytes). And these are probably just the strings that are removed from the options.
I guess it only really makes a slight difference, if you're not using gstreamer.
| static gboolean show_transport_scpd = FALSE; | ||
| static gboolean show_outputs = FALSE; | ||
| static gboolean daemon_mode = FALSE; | ||
| static int show_version = FALSE; |
There was a problem hiding this comment.
Instead of changing gboolean to int -- which looses all the idea behind using them in the first place, some user-visible type that indicates what this is about -- I suggest to include <stdbool.h> and use the bool type, as well as the true and false constants that come with it.
There are quite a few places where you had to downgrade a gboolean to an int; with using stdbool.h, we can keep the sanity.
|
|
||
| // IP-address seems strange in libupnp: they actually don't bind to | ||
| // that address, but to INADDR_ANY (miniserver.c in upnp library). | ||
| // Apparently they just use this for the advertisement ? Anyway, 0.0.0.0 would | ||
| // not work. | ||
| static const gchar *ip_address = NULL; | ||
| static const char *ip_address = NULL; |
There was a problem hiding this comment.
sorry, this changed to interface_name in the meantime (didn't see your pull request until now)
| { "dump-transport-scpd", 0, 0, G_OPTION_ARG_NONE, &show_transport_scpd, | ||
| "Dump A/V Transport service description XML and exit.", NULL }, | ||
| { NULL } | ||
| static struct option option_entries[] = { |
There was a problem hiding this comment.
We're now loosing all the commandline option description provided with gmediarender -h; in fact this new version does not have a help anymore.
So we need to get this functionality back somehow differently.
| @@ -577,12 +585,48 @@ static int output_gstreamer_init(void) | |||
| return 0; | |||
| } | |||
|
|
|||
| static const char *output_gstreamer_version(char *buffer, size_t len) | |||
There was a problem hiding this comment.
this version string also needs to make it all the way up to the init_logging() and do_show_version() in main. So some more plumbing is needed for that.
| #endif | ||
|
|
||
| /* Create a main loop that runs the default GLib main context */ | ||
| main_loop_ = g_main_loop_new(NULL, FALSE); |
This collection of patches remove all dependencies on glib except inside output_gstreamer.
The binary loses weight by 17ko without gstreamer and by 15ko with gstreamer.
It is also possible to integrate gmediarender on systems without glib, like distributions build with BuildRoot or Yocto.
This modification is targeted for embedded system like Raspberry Pi.