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
