Re: [pulseaudio-discuss] [PATCH] added two new commands to native API to control the combine sink slaves after the combine sink has been created
To catch up your points: I will synchronise the pactl to pacmd. pulseaudio-dlna is using pactl too. I just missed that point. I share your view regarding the location of the new functions. A new file set src/pulsecore/combine-sink.[ch] is a good idea to remove this from the standard sink stuff. Regarding multiple instances of module combine sink. I though I've added the instance id of the combine module to the API. This way the user or an external application can address which of the instances he would like to modify or query. This way no central management inside pulseaudio over all sinks on all combined sinks is needed. In this way the patch is as essential as it can be. You can even add a sink to two different combine sinks or a combine sink as input sink to another one if you like to. If the module combine is not loaded and you call the API functions they will simply return an error. If you unload the module while its running it is part of the module instance to clean up on exit. Simple and stupid and not as complex as some dynamic API stuff. On Thu, 2017-01-12 at 03:56 +0200 Tanu Kaskinen wrote: > On Wed, 2017-01-11 at 17:44 +0530, Arun Raghavan wrote: > > On Wed, 11 Jan 2017, at 03:45 AM, Tanu Kaskinen wrote: > > > Thanks for the patch! > > > > > > On Thu, 2017-01-05 at 20:13 +0100, Steffen Pfendtner wrote: > > > > Subject: [PATCH] added two new commands to native API to control the > > > > combine sink slaves after the combine sink has been created > > > > > > There's a misunderstanding: you edited the command line interface, not > > > the native protocol. Applications use libpulse, which implements the > > > native protocol, so that's where the client interface should be added. > > > The command line interface is used by pacmd and the startup script > > > interpreter. If you already integrated this feature in pulseaudio-dlna, > > > I guess you run pacmd commands from pulseaudio-dlna? > > > > > > We have two similar tools: pacmd and pactl. Unlike pacmd, pactl is a > > > regular client that uses libpulse to interact with the server. It's > > > pretty pointless to have two tools that do the same thing, so I hope we > > > can get rid of pacmd some day. pacmd doesn't work over the network, and > > > also doesn't work when pulseaudio runs in the system mode. > > > > > > You can add this functionality in the command line interface if you > > > really want to, but if you do that, you have to add it to pactl as > > > well, because I don't want new features in pacmd that are not supported > > > by pactl. Adding the functionality to pactl is highly desirable even if > > > you don't add the functionality to the command line interface, though. > > > > > > Assuming that the API is added to the "core" instead as a protocol > > > extension (see my previous mails: [1][2][3]), you'll need to add new > > > commands that are sent from src/pulse/introspect.c and handled in > > > src/pulsecore/protocol-native.c. I would suggest adding > > > src/pulsecore/combine-sink.[ch] that has pa_combine_sink_add_output() > > > and pa_combine_sink_remove_output() along with anything else protocol- > > > native.c needs for dealing with the new commands. The API shouldn't be > > > in sink.h, because that's for common sink functionality, while this API > > > is only for one specific sink implementation. > > > > First, to make this concrete. module-combine currently allows multiple > > instances, so I guess we would either need to change it to allow a > > single instance, or add a module-combine-manager module to implement the > > extension API. I prefer the former. > > Presumably you'd then add some new argument to module-combine-sink to > signal that it should run in the "manager mode"? If we are going the > extension route, I would prefer a separate manager module, but I don't > care about that detail very much. > > > While I'm not entirely against the idea of having this in core, I look > > at this in terms of what do we gain, and what do we lose. If we were to > > make this API core: > > > > 1. We gain a little bit by not jumping through the extension API hoops > > In my opinion avoiding the extension API hoops is a pretty big > advantage. Normally an extension can disappear at any time, because the > module can be unloaded. Do you think this should also be the case for > the combine-manager API? If the extension can be or become unavailable, > that will inherently make the API more complex. > > One option would be to implement some kind of transparent on-demand > loading of the manager module, and prevent it from being unloaded while > there are clients using it. That would still require applications to > register themselves as users of the API, which could be avoided if the > API was always available. > > > 2. We lose a little flexibility, because IMO the core API is more of a > > commitment for us than an extension API (we would still want to be very > > very reluctant to ever break it) > > I don't think
Re: [pulseaudio-discuss] [PATCH] added two new commands to native API to control the combine sink slaves after the combine sink has been created
On Wed, 2017-01-11 at 17:44 +0530, Arun Raghavan wrote: > On Wed, 11 Jan 2017, at 03:45 AM, Tanu Kaskinen wrote: > > Thanks for the patch! > > > > On Thu, 2017-01-05 at 20:13 +0100, Steffen Pfendtner wrote: > > > Subject: [PATCH] added two new commands to native API to control the > > > combine sink slaves after the combine sink has been created > > > > There's a misunderstanding: you edited the command line interface, not > > the native protocol. Applications use libpulse, which implements the > > native protocol, so that's where the client interface should be added. > > The command line interface is used by pacmd and the startup script > > interpreter. If you already integrated this feature in pulseaudio-dlna, > > I guess you run pacmd commands from pulseaudio-dlna? > > > > We have two similar tools: pacmd and pactl. Unlike pacmd, pactl is a > > regular client that uses libpulse to interact with the server. It's > > pretty pointless to have two tools that do the same thing, so I hope we > > can get rid of pacmd some day. pacmd doesn't work over the network, and > > also doesn't work when pulseaudio runs in the system mode. > > > > You can add this functionality in the command line interface if you > > really want to, but if you do that, you have to add it to pactl as > > well, because I don't want new features in pacmd that are not supported > > by pactl. Adding the functionality to pactl is highly desirable even if > > you don't add the functionality to the command line interface, though. > > > > Assuming that the API is added to the "core" instead as a protocol > > extension (see my previous mails: [1][2][3]), you'll need to add new > > commands that are sent from src/pulse/introspect.c and handled in > > src/pulsecore/protocol-native.c. I would suggest adding > > src/pulsecore/combine-sink.[ch] that has pa_combine_sink_add_output() > > and pa_combine_sink_remove_output() along with anything else protocol- > > native.c needs for dealing with the new commands. The API shouldn't be > > in sink.h, because that's for common sink functionality, while this API > > is only for one specific sink implementation. > > First, to make this concrete. module-combine currently allows multiple > instances, so I guess we would either need to change it to allow a > single instance, or add a module-combine-manager module to implement the > extension API. I prefer the former. Presumably you'd then add some new argument to module-combine-sink to signal that it should run in the "manager mode"? If we are going the extension route, I would prefer a separate manager module, but I don't care about that detail very much. > While I'm not entirely against the idea of having this in core, I look > at this in terms of what do we gain, and what do we lose. If we were to > make this API core: > > 1. We gain a little bit by not jumping through the extension API hoops In my opinion avoiding the extension API hoops is a pretty big advantage. Normally an extension can disappear at any time, because the module can be unloaded. Do you think this should also be the case for the combine-manager API? If the extension can be or become unavailable, that will inherently make the API more complex. One option would be to implement some kind of transparent on-demand loading of the manager module, and prevent it from being unloaded while there are clients using it. That would still require applications to register themselves as users of the API, which could be avoided if the API was always available. > 2. We lose a little flexibility, because IMO the core API is more of a > commitment for us than an extension API (we would still want to be very > very reluctant to ever break it) I don't think there's any real difference in commitment. An API break is an API break, the effects are the same regardless of whether the API is labeled as "core" or "extension". > 3. We lose a little more flexibility because combine becomes THE way to > group outputs in the core API, or we have API duplication if we revisit > other mechanisms (such as the node routing layer) I don't have high hopes for the new routing layer being resurrected anytime soon, but even if it will be, it will anyway duplicate all existing routing APIs, so I don't feel it's that bad adding another bit of routing APIs that might get duplicated by the new layer. > There are minor concerns around what happens if-module-combine-sink > isn't available for some reason in both cases, but that's not something > that would swing my opinion either way. I'm not sure what you mean by "not available". Do you mean that the module is not installed, or not loaded? If you mean not installed, I think that case is safe to ignore. If a distribution doesn't install module-combine-sink along with the main pulseaudio package, that's their fault. I don't think we need to support that. > Given this, I lean towards having an extension API. If you have strong > reasons to prefer the core API, I'm still
Re: [pulseaudio-discuss] [PATCH] added two new commands to native API to control the combine sink slaves after the combine sink has been created
On Wed, 11 Jan 2017, at 03:45 AM, Tanu Kaskinen wrote: > Thanks for the patch! > > On Thu, 2017-01-05 at 20:13 +0100, Steffen Pfendtner wrote: > > Subject: [PATCH] added two new commands to native API to control the > > combine sink slaves after the combine sink has been created > > There's a misunderstanding: you edited the command line interface, not > the native protocol. Applications use libpulse, which implements the > native protocol, so that's where the client interface should be added. > The command line interface is used by pacmd and the startup script > interpreter. If you already integrated this feature in pulseaudio-dlna, > I guess you run pacmd commands from pulseaudio-dlna? > > We have two similar tools: pacmd and pactl. Unlike pacmd, pactl is a > regular client that uses libpulse to interact with the server. It's > pretty pointless to have two tools that do the same thing, so I hope we > can get rid of pacmd some day. pacmd doesn't work over the network, and > also doesn't work when pulseaudio runs in the system mode. > > You can add this functionality in the command line interface if you > really want to, but if you do that, you have to add it to pactl as > well, because I don't want new features in pacmd that are not supported > by pactl. Adding the functionality to pactl is highly desirable even if > you don't add the functionality to the command line interface, though. > > Assuming that the API is added to the "core" instead as a protocol > extension (see my previous mails: [1][2][3]), you'll need to add new > commands that are sent from src/pulse/introspect.c and handled in > src/pulsecore/protocol-native.c. I would suggest adding > src/pulsecore/combine-sink.[ch] that has pa_combine_sink_add_output() > and pa_combine_sink_remove_output() along with anything else protocol- > native.c needs for dealing with the new commands. The API shouldn't be > in sink.h, because that's for common sink functionality, while this API > is only for one specific sink implementation. First, to make this concrete. module-combine currently allows multiple instances, so I guess we would either need to change it to allow a single instance, or add a module-combine-manager module to implement the extension API. I prefer the former. While I'm not entirely against the idea of having this in core, I look at this in terms of what do we gain, and what do we lose. If we were to make this API core: 1. We gain a little bit by not jumping through the extension API hoops 2. We lose a little flexibility, because IMO the core API is more of a commitment for us than an extension API (we would still want to be very very reluctant to ever break it) 3. We lose a little more flexibility because combine becomes THE way to group outputs in the core API, or we have API duplication if we revisit other mechanisms (such as the node routing layer) There are minor concerns around what happens if-module-combine-sink isn't available for some reason in both cases, but that's not something that would swing my opinion either way. Given this, I lean towards having an extension API. If you have strong reasons to prefer the core API, I'm still quite open to being convinced. :) Cheers, Arun ___ pulseaudio-discuss mailing list pulseaudio-discuss@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
Re: [pulseaudio-discuss] [PATCH] added two new commands to native API to control the combine sink slaves after the combine sink has been created
Thanks for the patch! On Thu, 2017-01-05 at 20:13 +0100, Steffen Pfendtner wrote: > Subject: [PATCH] added two new commands to native API to control the combine > sink slaves after the combine sink has been created There's a misunderstanding: you edited the command line interface, not the native protocol. Applications use libpulse, which implements the native protocol, so that's where the client interface should be added. The command line interface is used by pacmd and the startup script interpreter. If you already integrated this feature in pulseaudio-dlna, I guess you run pacmd commands from pulseaudio-dlna? We have two similar tools: pacmd and pactl. Unlike pacmd, pactl is a regular client that uses libpulse to interact with the server. It's pretty pointless to have two tools that do the same thing, so I hope we can get rid of pacmd some day. pacmd doesn't work over the network, and also doesn't work when pulseaudio runs in the system mode. You can add this functionality in the command line interface if you really want to, but if you do that, you have to add it to pactl as well, because I don't want new features in pacmd that are not supported by pactl. Adding the functionality to pactl is highly desirable even if you don't add the functionality to the command line interface, though. Assuming that the API is added to the "core" instead as a protocol extension (see my previous mails: [1][2][3]), you'll need to add new commands that are sent from src/pulse/introspect.c and handled in src/pulsecore/protocol-native.c. I would suggest adding src/pulsecore/combine-sink.[ch] that has pa_combine_sink_add_output() and pa_combine_sink_remove_output() along with anything else protocol- native.c needs for dealing with the new commands. The API shouldn't be in sink.h, because that's for common sink functionality, while this API is only for one specific sink implementation. [1] https://lists.freedesktop.org/archives/pulseaudio-discuss/2016-December/027234.html [2] https://lists.freedesktop.org/archives/pulseaudio-discuss/2016-December/027235.html [3] https://lists.freedesktop.org/archives/pulseaudio-discuss/2016-December/027236.html -- Tanu https://www.patreon.com/tanuk ___ pulseaudio-discuss mailing list pulseaudio-discuss@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
[pulseaudio-discuss] [PATCH] added two new commands to native API to control the combine sink slaves after the combine sink has been created
---
src/modules/module-combine-sink.c | 92 +++
src/pulsecore/cli-command.c | 78 +
src/pulsecore/sink.c | 32 ++
src/pulsecore/sink.h | 8
4 files changed, 210 insertions(+)
diff --git a/src/modules/module-combine-sink.c
b/src/modules/module-combine-sink.c
index b6322c6..4a91901 100644
--- a/src/modules/module-combine-sink.c
+++ b/src/modules/module-combine-sink.c
@@ -1249,6 +1249,95 @@ static pa_hook_result_t
sink_state_changed_hook_cb(pa_core *c, pa_sink *s, struc
return PA_HOOK_OK;
}
+void update_slaves_prop(pa_sink *combine_sink, struct userdata *u) {
+
+uint32_t idx;
+char *t;
+bool first = true;
+struct output *o;
+pa_proplist *pl;
+
+PA_IDXSET_FOREACH(o, u->outputs, idx) {
+char *e;
+if (first) {
+e = pa_sprintf_malloc("%s", pa_strnull(o->sink->name));
+first = false;
+} else {
+e = pa_sprintf_malloc("%s, %s", t, pa_strnull(o->sink->name));
+pa_xfree(t);
+}
+t = e;
+}
+/* if still first we have no outputs in the list */
+if (first) {
+t = pa_sprintf_malloc("");
+}
+
+pl = pa_proplist_new();
+pa_proplist_setf(pl, "combine.slaves", t);
+pa_xfree(t);
+pa_sink_update_proplist(combine_sink, PA_UPDATE_REPLACE, pl);
+pa_proplist_free(pl);
+}
+
+/* Called from main context on native API call */
+static int add_output(pa_sink *combine_sink, pa_sink *slave_sink) {
+struct userdata *u;
+struct output *o;
+
+pa_sink_assert_ref(combine_sink);
+pa_sink_assert_ref(slave_sink);
+pa_assert_se(u = combine_sink->userdata);
+
+if (u->automatic) {
+pa_log("combine sink is automatic, cannot add: '%s'.",
slave_sink->name);
+return -2;
+}
+
+if ((o = find_output(u, slave_sink))) {
+pa_log("Sink already on combine sink: '%s'.", slave_sink->name);
+return -3;
+}
+
+if (!(o = output_new(u, slave_sink))) {
+pa_log("Failed to add sink to combine sink: '%s'.", slave_sink->name);
+return -4;
+}
+output_verify(o);
+update_slaves_prop(combine_sink, u);
+
+pa_log("Add output sink");
+return 0;
+}
+
+/* Called from main context on native API call */
+static int del_output(pa_sink *combine_sink, pa_sink *slave_sink) {
+struct userdata *u;
+struct output *o;
+
+pa_sink_assert_ref(combine_sink);
+pa_sink_assert_ref(slave_sink);
+pa_assert_se(u = combine_sink->userdata);
+
+if (u->automatic) {
+pa_log("combine sink is automatic, cannot del: '%s'.",
slave_sink->name);
+return -2;
+}
+
+if (!(o = find_output(u, slave_sink))) {
+pa_log("Could not remove %s from combine sink, output was not found",
+slave_sink->name);
+return -3;
+}
+
+pa_idxset_remove_by_data(u->outputs, o, NULL);
+output_free(o);
+update_description(u);
+update_slaves_prop(combine_sink, u);
+pa_log("Del output sink");
+return 0;
+}
+
int pa__init(pa_module*m) {
struct userdata *u;
pa_modargs *ma = NULL;
@@ -1401,6 +1490,9 @@ int pa__init(pa_module*m) {
u->sink->set_state = sink_set_state;
u->sink->update_requested_latency = sink_update_requested_latency;
u->sink->userdata = u;
+u->sink->module = m;
+u->sink->combine_add_output = add_output;
+u->sink->combine_del_output = del_output;
pa_sink_set_rtpoll(u->sink, u->rtpoll);
pa_sink_set_asyncmsgq(u->sink, u->thread_mq.inq);
diff --git a/src/pulsecore/cli-command.c b/src/pulsecore/cli-command.c
index 9a73605..ba168d5 100644
--- a/src/pulsecore/cli-command.c
+++ b/src/pulsecore/cli-command.c
@@ -135,6 +135,8 @@ static int pa_cli_command_sink_port(pa_core *c,
pa_tokenizer *t, pa_strbuf *buf,
static int pa_cli_command_source_port(pa_core *c, pa_tokenizer *t, pa_strbuf
*buf, bool *fail);
static int pa_cli_command_port_offset(pa_core *c, pa_tokenizer *t, pa_strbuf
*buf, bool *fail);
static int pa_cli_command_dump_volumes(pa_core *c, pa_tokenizer *t, pa_strbuf
*buf, bool *fail);
+static int pa_cli_command_combine_sink_add_output(pa_core *c, pa_tokenizer *t,
pa_strbuf *buf, bool *fail);
+static int pa_cli_command_combine_sink_del_output(pa_core *c, pa_tokenizer *t,
pa_strbuf *buf, bool *fail);
/* A method table for all available commands */
@@ -191,6 +193,8 @@ static const struct command commands[] = {
{ "set-log-meta",pa_cli_command_log_meta, "Show
source code location in log messages (args: bool)", 2},
{ "set-log-time",pa_cli_command_log_time, "Show
timestamps in log messages (args: bool)", 2},
{ "set-log-backtrace", pa_cli_command_log_backtrace, "Show
backtrace in log messages (args: frames)", 2},
+{ "combine-sink-add-output", pa_cli_command_combine_sink_add_output, "Add
a output sink to the
