-
Notifications
You must be signed in to change notification settings - Fork 205
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
output modules #210
base: master
Are you sure you want to change the base?
output modules #210
Conversation
This allow to add dynamicly output inside the list of output. A new function output_append_module does this
The version of gst and the version of the output is displayed with the --list-outputs option. The core of gmedia_render doesn't dependence on gst any more.
gstreamer_output structure is inside an independent library. The core application dependences on the library only with one line in the main function to call ouput_append_module.
The initialization should be done whith the glib argument parser. But now the gstreamer is a library and in the next commit the module is loaded during the arguments parser.
By default gmrender hasn't any ouput's module, and should load at least one. Two way are availables: - use LD_PRELOAD to load the library before the application's launch: $ LD_PRELOAD=/usr/local/lib/libgmrender_gst.so /usr/local/bin/gmediarender - use the output option to set the name of the module to load: $ /usr/local/bin/gmediarender -o gst The second solution disallows the use of gstreamer's options on the command line. This point must be corrected in the next commit. This solution looks the library inside the LIBDIR directory and the name must have the format of libgmrender_<shortname>.so Because the core application doesn't use directly module API is impossible the link this binary to a module library. The linker disallows that.
The argument parser (add_options) must be call before the initialization. But if the initialization load the module, it is too late to parse the arguments.
To pass arguments to the module, the commandline must separate the arguments for the application. glib stop to parse when the argument is '--' $ /usr/local/bin/gmediarender -o gst -- --gstout-videosink=qtvideosink
This patch allows to write modules without link with glib.
The output module was in fact a specific library. This patch creates a real module, a dynamic binary that only gmediarender may load dynamicly with dlopen.
This contribs is just an example how to create a new output module. It's not stable. output_mpg123.c is the real module sound_alsa and sound_module manages the sound playback with alsa (a version with tinyalsa may be contributed too in the future) webclient allows to send GET request to the server and to receive the audio file data.
Ok...but why? I understand you have an application where gstreamer is an undesirable output backend. But is there really a common use case for having an output module that can be loaded at run-time? |
Hello,
My motivation is to add new modules without modification of the core
application. Gmrender contains a mechanism to use output modules, but only
gstreamer has been available for a long time.
I tried to send a support of mpg123 without success, and now I would like
to use gmediarender to drive my own playback application (
https://github.com/mchalain/putv).
The load of an external library allows to extend the usage of gmrender.
Break the link between the UPnP features and the Output, allows to
integrate Gmrender in different projects on different targets.
If gmrender may be used by more people, the application may receive more
contributions.
My goal is a connected speaker (
https://www.lesimprimantes3d.fr/forum/topic/31906-enceinte-wifi-raspberry-pi-0-w/
). All softwares is in Open-Source. I need to keep the memory and the CPU
resource for the streaming and keep the MMC space to store music. Gstreamer
is too fat, and yet glib is useless for me.
Le dim. 28 juin 2020 à 01:17, Tucker Kern <[email protected]> a
écrit :
… Ok...but why?
I understand you have an application where gstreamer is an undesirable
output backend. But is there really a common use case for having an output
module that can be loaded at run-time?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#210 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AA7UFJC2IQXB3NB3TRIWUJ3RYZ4ZBANCNFSM4OJKH3QQ>
.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your contribution; sorry I have been busy last year with Covid related things and didn't notice this pull request.
I am not sure if providing dynamic loading of output modules will be very beneficial. Usually these kind of things are cool, but order to support them properly, there needs to be a mechanism to figure out against which version one links; in general, the situation is more brittle to support, it depends on if the installation has the right shared object files at the right place etc.
So in a first step, I would advise against considering dynamic loading of modules. It is cool, I know, and I also like to use dlopen() for all kinds of fun parts; but in situations where we add unnecessary brittleness I'd like to avoid it. gmrender is typically installed on very small devices, so you want ideally some statically build small thing. I don't like gstreamer very much for that reason, as they have a lot of not very robust dynamic loading going on.
Using alternative output libraries such as mpg123 is exciting and very welcome (I think there was another pull request of that a few years back but there was an issue with merging it I think).
So as a first step I suggest
- Provide in the pull request what is needed to further separate bulid-in things; You added
version()
to the output module struct, I think this is good. Maybe without the buffer passed in (see comment below). - Avoid the dynamic loading with dlopen(); instead make these compiled-in modules, chosen at configure time. We can revisit the dynamic loading separately.
- I like the small commits in the pull request (which makes it ok to review), though the final pull request is somewhat big. Maybe we should have a few pull requests that are somewhat targeting smaller sub-problems (for instance you have provided the glib pull request, once that is somewhat through, other changes will be smaller).
Anyway, a few initial comments below:
@@ -46,6 +46,8 @@ struct output_module { | |||
int (*set_volume)(float); | |||
int (*get_mute)(int *); | |||
int (*set_mute)(int); | |||
|
|||
struct output_module *next; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given that this struct is part of the public interface how to implement an output module, the linked list pointer is probably not part of what we'd like to expose.
So the maintenance of the list should be outside of this struct, even if it is more cumbersome than an intrusive data structure. Public interfaces should be as lean and clean as possible and not leak internal implementation details.
src/output_gstreamer.c
Outdated
@@ -577,12 +577,22 @@ static int output_gstreamer_init(void) | |||
return 0; | |||
} | |||
|
|||
static const char *output_gstreamer_version(char *buffer, size_t len) | |||
{ | |||
snprintf(buffer, len, "%s (glib-%d.%d.%d; gstreamer-%d.%d.%d)", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I'd be also fine if the module would return a statically allocated string. We could either have a static allocated buffer and snprintf() into it, or just assemble the stirng with macro tricks.
Providing less headache for the reader of the calling code is a good thing.
Also, it keeps our API interface lean. It is not that we have a heavy lifting function here that requires some external megabyte buffer to be filled, but something that for essentially all known implementation will return a static string in the first place.
@@ -33,6 +33,7 @@ struct output_module { | |||
|
|||
// Commands. | |||
int (*init)(void); | |||
const char *(*version)(char *buffer, size_t len); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(here would be just a simpler const char *(*version)();
)
typedef struct st_output_mpg123_uri output_mpg123_uri_t; | ||
struct st_output_mpg123_uri | ||
{ | ||
char *uri; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should this be const char* ?
|
||
struct output_module mpg123_output = { | ||
.shortname = "mpg123", | ||
.description = "daemon framework", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
indentation looks funny here.
This collection of patches allows to use external output module.
The module is a dynamic binary installed is @Prefix@/lib/gmediarender and the core application is able to load it dynamically with dlopen.
output_gstreamer.c is not inside the core application and move to the gmrender_gst.so file.
Now it exists two way to load the module:
$ /usr/local/bin/gmediarender -o : this command looks for the file /usr/local/lib/gmediarender/gmrender_.so
or:
$ LD_PRELOAD=/usr/local/lib/gmediarender/gmrender_gst.so /usr/local/bin/gmediarender
But as the module may be load from the arguments of the application, the argument of the module needs to be separate from the others by '--'.
$ /usr/local/bin/gmediarender -o gst --logfile=stderr -- --gstout-videosink=qtvideosink
To check the capabilities of the module, the gmrender_mp123.so module is available in the contribs directory.
Others modules should be available in the future (mpd and another for my own application putv).