[pulseaudio-discuss] added two new commands to native API to control the combine sink slaves after the combine sink has been created

Submitted by Steffen Pfendtner on Jan. 5, 2017, 7:13 p.m.

Details

Message ID 1483643606-23916-2-git-send-email-steffen@pfendtner.de
State New
Headers show
Series "added two new commands to native API to control the combine sink slaves after the combine sink has been created" ( rev: 1 ) in PulseAudio

Not browsing as part of any series.

Commit Message

Steffen Pfendtner Jan. 5, 2017, 7:13 p.m.
---
 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(+)

Patch hide | download patch | download mbox

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 specified combine sink (args: ?, ?", 3},
+    { "combine-sink-del-output", pa_cli_command_combine_sink_del_output, "Delete a output sink from the specified combine sink (args: ?, ?", 3},
     { "play-file",               pa_cli_command_play_file,          "Play a sound file (args: filename, sink|index)", 3},
     { "dump",                    pa_cli_command_dump,               "Dump daemon configuration", 1},
     { "dump-volumes",            pa_cli_command_dump_volumes,       "Debug: Show the state of all volumes", 1 },
@@ -1992,6 +1996,80 @@  static int pa_cli_command_dump_volumes(pa_core *c, pa_tokenizer *t, pa_strbuf *b
     return 0;
 }
 
+int pa_cli_command_combine_sink_add_output(pa_core *c, pa_tokenizer *t, pa_strbuf *buf, bool *fail) {
+    const char *cs, *os;
+    pa_sink *combine_sink, *out_sink;
+
+    pa_core_assert_ref(c);
+    pa_assert(t);
+    pa_assert(buf);
+    pa_assert(fail);
+
+    if (!(cs = pa_tokenizer_get(t, 1))) {
+        pa_strbuf_puts(buf, "You need to specify a combine sink either by its name or its index.\n");
+        return -1;
+    }
+
+    if (!(os = pa_tokenizer_get(t, 2))) {
+        pa_strbuf_puts(buf, "You need to specify an output sink either by its name or its index.\n");
+        return -1;
+    }
+
+    if (!(combine_sink = pa_namereg_get(c, cs, PA_NAMEREG_SINK))) {
+        pa_strbuf_puts(buf, "No combine sink found by this name or index.\n");
+        return -1;
+    }
+
+    if (!(out_sink = pa_namereg_get(c, os, PA_NAMEREG_SINK))) {
+        pa_strbuf_puts(buf, "No output sink found by this name or index.\n");
+        return -1;
+    }
+
+    if (pa_sink_combine_add_output(combine_sink, out_sink) < 0) {
+        pa_strbuf_puts(buf, "Add to combine sink failed!\n");
+        return -1;
+    }
+
+    return 0;
+}
+
+int pa_cli_command_combine_sink_del_output(pa_core *c, pa_tokenizer *t, pa_strbuf *buf, bool *fail) {
+    const char *cs, *os;
+    pa_sink *combine_sink, *out_sink;
+
+    pa_core_assert_ref(c);
+    pa_assert(t);
+    pa_assert(buf);
+    pa_assert(fail);
+
+    if (!(cs = pa_tokenizer_get(t, 1))) {
+        pa_strbuf_puts(buf, "You need to specify a combine sink either by its name or its index.\n");
+        return -1;
+    }
+
+    if (!(os = pa_tokenizer_get(t, 2))) {
+        pa_strbuf_puts(buf, "You need to specify an output sink either by its name or its index.\n");
+        return -1;
+    }
+
+    if (!(combine_sink = pa_namereg_get(c, cs, PA_NAMEREG_SINK))) {
+        pa_strbuf_puts(buf, "No combine sink found by this name or index.\n");
+        return -1;
+    }
+
+    if (!(out_sink = pa_namereg_get(c, os, PA_NAMEREG_SINK))) {
+        pa_strbuf_puts(buf, "No output sink found by this name or index.\n");
+        return -1;
+    }
+
+    if (pa_sink_combine_del_output(combine_sink, out_sink) < 0) {
+        pa_strbuf_puts(buf, "Delete from combine sink failed!\n");
+        return -1;
+    }
+
+    return 0;
+}
+
 int pa_cli_command_execute_line_stateful(pa_core *c, const char *s, pa_strbuf *buf, bool *fail, int *ifstate) {
     const char *cs;
 
diff --git a/src/pulsecore/sink.c b/src/pulsecore/sink.c
index 5c6a9c6..6379504 100644
--- a/src/pulsecore/sink.c
+++ b/src/pulsecore/sink.c
@@ -3835,3 +3835,35 @@  void pa_sink_set_reference_volume_direct(pa_sink *s, const pa_cvolume *volume) {
     pa_subscription_post(s->core, PA_SUBSCRIPTION_EVENT_SINK|PA_SUBSCRIPTION_EVENT_CHANGE, s->index);
     pa_hook_fire(&s->core->hooks[PA_CORE_HOOK_SINK_VOLUME_CHANGED], s);
 }
+
+int pa_sink_combine_add_output(pa_sink *combine_sink, pa_sink *slave_sink) {
+    int r;
+
+    pa_assert_ctl_context();
+    pa_assert(combine_sink);
+    pa_assert(slave_sink);
+
+    if (combine_sink->combine_add_output != NULL) {
+        r = combine_sink->combine_add_output(combine_sink, slave_sink);
+        return r;
+    } else {
+        return -1;
+    }
+
+    return 0;
+}
+
+int pa_sink_combine_del_output(pa_sink *combine_sink, pa_sink *slave_sink) {
+    int r;
+
+    pa_assert_ctl_context();
+    pa_assert(combine_sink);
+    pa_assert(slave_sink);
+
+    if (combine_sink->combine_del_output != NULL) {
+        r = combine_sink->combine_del_output(combine_sink, slave_sink);
+        return r;
+    } else {
+        return -1;
+    }
+}
diff --git a/src/pulsecore/sink.h b/src/pulsecore/sink.h
index c549869..756a0f6 100644
--- a/src/pulsecore/sink.h
+++ b/src/pulsecore/sink.h
@@ -248,6 +248,11 @@  struct pa_sink {
      * main thread. */
     int (*update_rate)(pa_sink *s, uint32_t rate);
 
+    /* Called for manual combine sink change. Must not be NULL for a combine
+     * sink. Can be NULL for all other sink types. */
+    int (*combine_add_output)(pa_sink *combine_sink, pa_sink *slave_sink);
+    int (*combine_del_output)(pa_sink *combine_sink, pa_sink *slave_sink);
+
     /* Contains copies of the above data so that the real-time worker
      * thread can work without access locking */
     struct {
@@ -528,6 +533,9 @@  pa_usec_t pa_sink_get_latency_within_thread(pa_sink *s);
  * s->reference_volume and fires change notifications. */
 void pa_sink_set_reference_volume_direct(pa_sink *s, const pa_cvolume *volume);
 
+int pa_sink_combine_add_output(pa_sink *combine_sink, pa_sink *slave_sink);
+int pa_sink_combine_del_output(pa_sink *combine_sink, pa_sink *slave_sink);
+
 /* Verify that we called in IO context (aka 'thread context), or that
  * the sink is not yet set up, i.e. the thread not set up yet. See
  * pa_assert_io_context() in thread-mq.h for more information. */

Comments

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
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
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 quite open to being convinced.
> :)

My "strong reasons" boil down to avoiding unnecessary complexity in the
client API. Having the API in the core should also make the
implementation quite a bit simpler, but that carries less weight in my
consideration.
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 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 quite open to being convinced.
> > :)
> 
> My "strong reasons" boil down to avoiding unnecessary complexity in the
> client API. Having the API in the core should also make the
> implementation quite a bit simpler, but that carries less weight in my
> consideration.
>
On Fri, 2017-01-27 at 19:34 +0100, Steffen Pfendtner wrote:
> 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.

It's not clear to me if you're planning to make a protocol extension,
or put this stuff in the core protocol. If it's going to be an
extension, then there's no need for pulsecore/combine-sink.[ch].
module-combine-sink will use pa_native_protocol_install_ext() to set a
callback for receiving the messages from clients.

> Regarding multiple instances of module combine sink.
> I though I've added the instance id of the combine module to the API.

By "the instance id of the combine module", do you mean the sink name?
Or the module index? I don't think the module index needs to be part of
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.

If an application wants to keep track of what combine sinks exist, how
do you plan to support that without a central management API? Is the
application expected to get a list of all sinks and filter based on
which module owns the sink? The problem with that is that I dislike the
idea of mandating that every combine sink has to have a module-combine-
sink instance associated with it.

I've been involved with a project that made a routing module that
automatically created combine sinks, and having to deal with the module
layer was an extra pain that could have been avoided if there was a way
to create combine sinks without having to load a module every time. I
think that project is dead now (I'm certainly not involved any more),
but I wouldn't be surprised if something similar will be created in the
future. That's why I don't want the API to assume that there will be
always a module-combine-sink instance associated with a combine sink.