Hi Ismo!
Sorry for the extreme delay with this review. If you want, I can do the
suggested changes - they're mostly cosmetic anyway. Of course, if you
don't agree with all the suggestions, please complain.
The interface that you propose is rather limited - in order to use it,
the user has to know the sink name and the plugin type beforehand. Doing
a nice generic GUI isn't possible with an interface like this - there's
no way to find all available ladspa sinks (except by checking the
introspection XML of all sinks - yuck) and there's no way to introspect
the plugin interface. That said, if the interface solves some problem
for you, I'm fine with it, because it's still an improvement to the
previous situation.
On Wed, 2012-01-25 at 13:57 +0200, Ismo Puustinen wrote:
> diff --git a/src/modules/ladspa.h b/src/modules/ladspa.h
> index b1a9c4e..7971160 100644
> --- a/src/modules/ladspa.h
> +++ b/src/modules/ladspa.h
> @@ -353,6 +353,17 @@ typedef struct _LADSPA_PortRangeHint {
>
> /*****************************************************************************/
>
> +/* Extended messages:
> +
> + The PA_SINK_MESSAGE types that extend the predefined messages. */
> +
> +enum {
> + LADSPA_SINK_MESSAGE_UPDATE_PARAMETERS = PA_SINK_MESSAGE_MAX
> +};
> +
> +
> +/*****************************************************************************/
> +
This doesn't really belong here, because ladspa.h is sort of
"immutable". It's copied from here:
http://www.ladspa.org/ladspa_sdk/ladspa.h.txt
The enum should be in module-ladspa-sink.c.
> /* Plugin Handles:
>
> This plugin handle indicates a particular instance of the plugin
> diff --git a/src/modules/module-ladspa-sink.c b/src/modules/module-ladspa-sink.c
> index be05715..35fb3db 100644
> --- a/src/modules/module-ladspa-sink.c
> +++ b/src/modules/module-ladspa-sink.c
> @@ -41,6 +41,11 @@
> #include <pulsecore/sample-util.h>
> #include <pulsecore/ltdl-helper.h>
>
> +#ifdef HAVE_DBUS
> +#include <pulsecore/protocol-dbus.h>
> +#include <pulsecore/dbus-util.h>
> +#endif
> +
> #include "module-ladspa-sink-symdef.h"
> #include "ladspa.h"
>
> @@ -79,6 +84,7 @@ struct userdata {
> LADSPA_Data **input, **output;
> size_t block_size;
> LADSPA_Data *control;
> + long unsigned int n_control;
The convention is to leave "int" out of unsigned ints.
>
> /* This is a dummy buffer. Every port must be connected, but we don't care
> about control out ports. We connect them all to this single buffer. */
> @@ -86,6 +92,14 @@ struct userdata {
>
> pa_memblockq *memblockq;
>
> + pa_bool_t *use_default;
> + pa_sample_spec ss;
> +
> +#ifdef HAVE_DBUS
> + pa_dbus_protocol *dbus_protocol;
> + char *dbus_path;
> +#endif
> +
> pa_bool_t auto_desc;
> };
>
> @@ -105,6 +119,221 @@ static const char* const valid_modargs[] = {
> NULL
> };
>
> +static pa_bool_t write_control_parameters(double *control_values, pa_bool_t *use_default, struct userdata *u);
The convention is to return a negative int when function fails, not a
boolean.
> +static pa_bool_t connect_control_ports(struct userdata *u);
It seems that connect_control_ports() always returns TRUE, so the return
type should probably be void.
> +
> +#ifdef HAVE_DBUS
> +
> +#define LADSPA_IFACE "org.PulseAudio.Ext.Ladspa1"
> +#define LADSPA_ALGORITHM_PARAMETERS "AlgorithmParameters"
> +
> +/* TODO: add a PropertyChanged signal to tell that the algorithm parameters have been changed */
> +
> +enum ladspa_handler_index {
> + LADSPA_HANDLER_ALGORITHM_PARAMETERS,
> + LADSPA_HANDLER_MAX
> +};
> +
> +static void get_algorithm_parameters(DBusConnection *conn, DBusMessage *msg, void *_u) {
> + struct userdata *u;
> + DBusMessage *reply = NULL;
> + DBusMessageIter msg_iter, struct_iter;
> + unsigned long i;
> + double *control;
> + dbus_bool_t *use_default;
> +
> + pa_assert(conn);
> + pa_assert(msg);
> + pa_assert_se(u = _u);
> +
> + pa_assert_se((reply = dbus_message_new_method_return(msg)));
> + dbus_message_iter_init_append(reply, &msg_iter);
> +
> + dbus_message_iter_open_container(&msg_iter, DBUS_TYPE_STRUCT, NULL, &struct_iter);
> +
> + /* copying because of the D-Bus type mapping */
> + control = pa_xnew(double, u->n_control);
> + use_default = pa_xnew(dbus_bool_t, u->n_control);
> +
> + for (i = 0; i < u->n_control; i++) {
> + control[i] = (double) u->control[i];
> + use_default[i] = u->use_default[i];
> + }
> +
> + pa_dbus_append_basic_array(&struct_iter, DBUS_TYPE_DOUBLE, control, u->n_control);
> + pa_dbus_append_basic_array(&struct_iter, DBUS_TYPE_BOOLEAN, use_default, u->n_control);
> +
> + dbus_message_iter_close_container(&msg_iter, &struct_iter);
> +
> + pa_assert_se(dbus_connection_send(conn, reply, NULL));
> +
> + dbus_message_unref(reply);
> + pa_xfree(control);
> + pa_xfree(use_default);
> +
> + return;
Redundant return.
> +}
> +
> +static void set_algorithm_parameters(DBusConnection *conn, DBusMessage *msg, DBusMessageIter *iter, void *_u) {
> + struct userdata *u;
> + DBusMessageIter array_iter, struct_iter;
> + int n_control = 0, n_use_default;
> + unsigned int n_dbus_control, n_dbus_use_default;
"unsigned int" -> "unsigned"
> + double *read_values = NULL;
> + dbus_bool_t *read_defaults = NULL;
> + pa_bool_t *use_defaults = NULL;
> + unsigned long i;
> +
> + pa_assert(conn);
> + pa_assert(msg);
> + pa_assert_se(u = _u);
> +
> + /* The property we are expecting has signature (adab), meaning that it's a
> + struct of two arrays, the first containing doubles and the second containing
> + booleans. The first array has the algorithm configuration values and the
> + second array has booleans indicating whether the matching algorithm
> + configuration value should use (or try to use) the default value provided by
> + the algorithm module. The PulseAudio D-Bus infrastructure will take care of
> + checking the argument types for us. */
> +
> + dbus_message_iter_recurse(iter, &struct_iter);
> +
> + dbus_message_iter_recurse(&struct_iter, &array_iter);
> + dbus_message_iter_get_fixed_array(&array_iter, &read_values, &n_control);
> +
> + dbus_message_iter_next(&struct_iter);
> + dbus_message_iter_recurse(&struct_iter, &array_iter);
> + dbus_message_iter_get_fixed_array(&array_iter, &read_defaults, &n_use_default);
> +
> + n_dbus_control = n_control; /* handle the unsignedness */
> + n_dbus_use_default = n_use_default;
> +
> + if (n_dbus_control != u->n_control || n_dbus_use_default != u->n_control) {
> + pa_dbus_send_error(conn, msg, DBUS_ERROR_INVALID_ARGS, "Wrong number of array values (expected %lu)", u->n_control);
> + return;
> + }
> +
> + use_defaults = pa_xnew(pa_bool_t, n_control);
> + for (i = 0; i < u->n_control; i++)
> + use_defaults[i] = read_defaults[i];
> +
> + if (!write_control_parameters(read_values, use_defaults, u)) {
> + pa_log("Failed writing control parameters");
> + goto error;
> + }
> +
> + /* TODO: if sink is not active, we can change the values here and now */
> + pa_asyncmsgq_send(u->sink->asyncmsgq, PA_MSGOBJECT(u->sink), LADSPA_SINK_MESSAGE_UPDATE_PARAMETERS, NULL, 0, NULL);
I don't think the suggested optimization would buy much. I like the
clarity of updating the parameters always from the same thread, so I'd
remove the TODO comment.
> +
> + pa_dbus_send_empty_reply(conn, msg);
> +
> + pa_xfree(use_defaults);
> + return;
> +
> +error:
> + pa_xfree(use_defaults);
> + pa_dbus_send_error(conn, msg, DBUS_ERROR_FAILED, "Internal error");
> +
> + return;
Redundant return.
> +}
> +
> +static pa_dbus_property_handler ladspa_property_handlers[LADSPA_HANDLER_MAX] = {
> + [LADSPA_HANDLER_ALGORITHM_PARAMETERS] = {
> + .property_name=LADSPA_ALGORITHM_PARAMETERS,
> + .type="(adab)",
> + .get_cb=get_algorithm_parameters,
> + .set_cb=set_algorithm_parameters
I'd very much prefer spaces around the equal signs.
> + }
> +};
> +
> +static void ladspa_get_all(DBusConnection *conn, DBusMessage *msg, void *_u) {
> + struct userdata *u;
> + DBusMessage *reply = NULL;
> + DBusMessageIter msg_iter, dict_iter, dict_entry_iter, variant_iter, struct_iter;
> + const char *key = LADSPA_ALGORITHM_PARAMETERS;
> + double *control;
> + dbus_bool_t *use_default;
> + long unsigned i;
> +
> + pa_assert(conn);
> + pa_assert(msg);
> + pa_assert_se(u = _u);
> +
> + pa_assert_se((reply = dbus_message_new_method_return(msg)));
> +
> + /* Currently, on this interface, only a single property is returned. */
> +
> + dbus_message_iter_init_append(reply, &msg_iter);
> + pa_assert_se(dbus_message_iter_open_container(&msg_iter, DBUS_TYPE_ARRAY, "{sv}", &dict_iter));
> + pa_assert_se(dbus_message_iter_open_container(&dict_iter, DBUS_TYPE_DICT_ENTRY, NULL, &dict_entry_iter));
> + pa_assert_se(dbus_message_iter_append_basic(&dict_entry_iter, DBUS_TYPE_STRING, &key));
> +
> + pa_assert_se(dbus_message_iter_open_container(&dict_entry_iter, DBUS_TYPE_VARIANT, "(adab)", &variant_iter));
> + pa_assert_se(dbus_message_iter_open_container(&variant_iter, DBUS_TYPE_STRUCT, NULL, &struct_iter));
> +
> + control = pa_xnew(double, u->n_control);
> + use_default = pa_xnew(dbus_bool_t, u->n_control);
> +
> + for (i = 0; i < u->n_control; i++) {
> + control[i] = (double) u->control[i];
> + use_default[i] = u->use_default[i];
> + }
> +
> + pa_dbus_append_basic_array(&struct_iter, DBUS_TYPE_DOUBLE, control, u->n_control);
> + pa_dbus_append_basic_array(&struct_iter, DBUS_TYPE_BOOLEAN, use_default, u->n_control);
> +
> + pa_assert_se(dbus_message_iter_close_container(&variant_iter, &struct_iter));
> + pa_assert_se(dbus_message_iter_close_container(&dict_entry_iter, &variant_iter));
> + pa_assert_se(dbus_message_iter_close_container(&dict_iter, &dict_entry_iter));
> + pa_assert_se(dbus_message_iter_close_container(&msg_iter, &dict_iter));
> +
> + pa_assert_se(dbus_connection_send(conn, reply, NULL));
> + dbus_message_unref(reply);
> + pa_xfree(control);
> + pa_xfree(use_default);
> +
> + return;
Redundant return.
> +}
> +
> +static pa_dbus_interface_info ladspa_info={
> + .name=LADSPA_IFACE,
> + .method_handlers=NULL,
> + .n_method_handlers=0,
> + .property_handlers=ladspa_property_handlers,
> + .n_property_handlers=LADSPA_HANDLER_MAX,
> + .get_all_properties_cb=ladspa_get_all,
> + .signals=NULL,
> + .n_signals=0
> +};
Missing spaces around equal signs.
> +
> +static void dbus_init(struct userdata *u) {
> +
Extra empty line.
> + pa_assert_se(u);
> +
> + u->dbus_protocol=pa_dbus_protocol_get(u->sink->core);
> + u->dbus_path=pa_sprintf_malloc("/org/pulseaudio/core1/sink%d", u->sink->index);
Spaces around equal signs.
> +
> + pa_dbus_protocol_add_interface(u->dbus_protocol, u->dbus_path, &ladspa_info, u);
> +
> + return;
> +}
> +
> +static void dbus_done(struct userdata *u) {
> +
Extra space.
> + pa_assert_se(u);
> +
> + if (!u->dbus_path || !u->dbus_protocol)
> + return;
This check doesn't look very good. If dbus_path is set but dbus_protocol
is not, then we will leak dbus_path. Of course, that's something that
should never happen, so there's no real bug here, but I think it would
be clearer to check only for dbus_protocol and maybe add an assertion
that if dbus_protocol is NULL, then also dbus_path must be NULL.
> +
> + pa_dbus_protocol_remove_interface(u->dbus_protocol, u->dbus_path, ladspa_info.name);
> + pa_xfree(u->dbus_path);
> + pa_dbus_protocol_unref(u->dbus_protocol);
I'd set dbus_path and dbus_protocol to NULL here, so that it will be
safe to call this function multiple times if it some day becomes
necessary (if someone adds another dbus_done() call somewhere, it's very
easy to forget to fix this at that time).
> +
> + return;
Redundant return.
> +}
> +
> +#endif
This could have a /* HAVE_DBUS */ comment, since the start of this ifdef
is far away.
> +
> /* Called from I/O thread context */
> static int sink_process_msg_cb(pa_msgobject *o, int code, void *data, int64_t offset, pa_memchunk *chunk) {
> struct userdata *u = PA_SINK(o)->userdata;
> @@ -131,6 +360,18 @@ static int sink_process_msg_cb(pa_msgobject *o, int code, void *data, int64_t of
> pa_bytes_to_usec(pa_memblockq_get_length(u->sink_input->thread_info.render_memblockq), &u->sink_input->sink->sample_spec);
>
> return 0;
> +
> + case LADSPA_SINK_MESSAGE_UPDATE_PARAMETERS:
> +
> + /* rewind the stream to throw away the previously rendered data */
> +
> + pa_log_debug("Requesting rewind due to parameter update.");
> + pa_sink_request_rewind(u->sink, -1);
> +
> + /* change the sink parameters */
> + connect_control_ports(u);
> +
> + return 0;
> }
>
> return pa_sink_process_msg(o, code, data, offset, chunk);
> @@ -469,6 +710,220 @@ static void sink_input_mute_changed_cb(pa_sink_input *i) {
> pa_sink_mute_changed(u->sink, i->muted);
> }
>
> +static pa_bool_t parse_control_parameters(const char *cdata, double *read_values, pa_bool_t *use_default, struct userdata *u) {
Report errors with negative integers. Also, I'd prefer to have userdata
as the first argument for these internal helper functions.
> +
Extra empty line.
> + unsigned long p = 0;
> + const char *state = NULL;
> + char *k;
It's a convention to have an empty line after the variable declarations.
> + pa_assert(read_values);
> + pa_assert(use_default);
> + pa_assert(u);
> +
> + pa_log_debug("Trying to read %lu control values", u->n_control);
> +
> + if (!cdata && u->n_control > 0) {
> + return FALSE;
> + }
Redundant braces.
> +
> + pa_log_debug("cdata: '%s'", cdata);
> +
> + while ((k = pa_split(cdata, ",", &state)) && p < u->n_control) {
> + double f;
> +
> + if (*k == 0) {
> + pa_log_debug("Read empty config value (p=%lu)", p);
> + use_default[p++] = TRUE;
> + pa_xfree(k);
> + continue;
> + }
> +
> + if (pa_atod(k, &f) < 0) {
> + pa_log_debug("Failed to parse control value '%s' (p=%lu)", k, p);
> + pa_xfree(k);
> + goto fail;
> + }
> +
> + pa_xfree(k);
> +
> + pa_log_debug("Read config value %f (p=%lu)", f, p);
> +
> + use_default[p] = FALSE;
> + read_values[p++] = f;
> + }
> +
> + /* The previous loop doesn't take the last control value into account
> + if it is left empty, so we do it here. */
> + if (*cdata == 0 || cdata[strlen(cdata) - 1] == ',') {
> + if (p < u->n_control)
> + use_default[p] = TRUE;
> + p++;
> + }
> +
> + if (p > u->n_control || k) {
> + pa_log("Too many control values passed, %lu expected.", u->n_control);
> + pa_xfree(k);
> + goto fail;
> + }
> +
> + if (p < u->n_control) {
> + pa_log("Not enough control values passed, %lu expected, %lu passed.", u->n_control, p);
> + goto fail;
> + }
> +
> + return TRUE;
> +
> +fail:
> + return FALSE;
> +}
> +
> +static pa_bool_t connect_control_ports(struct userdata *u) {
> +
Extra empty line.
> + unsigned long p = 0, h = 0, c;
> + const LADSPA_Descriptor *d;
It's a convention to have an empty line after the variable declarations.
> + pa_assert(u);
> + d = u->descriptor;
> + pa_assert(d);
I'd prefer pa_assert_se(d = u->descriptor).
> +
> + for (p = 0; p < d->PortCount; p++) {
> + if (!LADSPA_IS_PORT_CONTROL(d->PortDescriptors[p]))
> + continue;
> +
> + if (LADSPA_IS_PORT_OUTPUT(d->PortDescriptors[p])) {
> + for (c = 0; c < (u->channels / u->max_ladspaport_count); c++)
> + d->connect_port(u->handle[c], p, &u->control_out);
> + continue;
> + }
> +
> + /* input control port */
> +
> + pa_log_debug("Binding %f to port %s", u->control[h], d->PortNames[p]);
> +
> + for (c = 0; c < (u->channels / u->max_ladspaport_count); c++)
> + d->connect_port(u->handle[c], p, &u->control[h]);
> +
> + h++;
> + }
> +
> + return TRUE;
> +}
> +
> +static pa_bool_t write_control_parameters(double *control_values, pa_bool_t *use_default, struct userdata *u) {
I'd prefer to have userdata as the first argument for these internal
helper functions.
> +
> + unsigned long p = 0, h = 0, c;
> + const LADSPA_Descriptor *d;
> + pa_sample_spec ss;
It's a convention to have an empty line after the variable declarations.
> + pa_assert(control_values);
> + pa_assert(use_default);
> + pa_assert(u);
> + ss = u->ss;
> + d = u->descriptor;
> + pa_assert(d);
I'd prefer pa_assert_se(d = u->descriptor).
> +
> + /* p iterates over all ports, h is the control port iterator */
> +
> + for (p = 0; p < d->PortCount; p++) {
> + LADSPA_PortRangeHintDescriptor hint = d->PortRangeHints[p].HintDescriptor;
> +
> + if (!LADSPA_IS_PORT_CONTROL(d->PortDescriptors[p]))
> + continue;
> +
> + if (LADSPA_IS_PORT_OUTPUT(d->PortDescriptors[p])) {
> + for (c = 0; c < (u->channels / u->max_ladspaport_count); c++)
> + d->connect_port(u->handle[c], p, &u->control_out);
> + continue;
> + }
> +
> + if (use_default[h]) {
> +
> + LADSPA_Data lower, upper;
> +
> + if (!LADSPA_IS_HINT_HAS_DEFAULT(hint)) {
> + pa_log("Control port value left empty but plugin defines no default.");
> + goto fail;
This may happen in the middle of updating the control values. It would
be nice if nothing would be updated on failure. Maybe there should be a
table of booleans, indicating whether a given control port defines a
default (or maybe all the tables should be replaced with a single table
that would contains structs consisting of the current control value, the
default value and a "has default" boolean). That table could then be
checked before write_control_parameters() is called, and
write_control_parameters() would never fail.
> + }
> +
> + lower = d->PortRangeHints[p].LowerBound;
> + upper = d->PortRangeHints[p].UpperBound;
> +
> + if (LADSPA_IS_HINT_SAMPLE_RATE(hint)) {
> + lower *= (LADSPA_Data) ss.rate;
> + upper *= (LADSPA_Data) ss.rate;
> + }
> +
> + switch (hint & LADSPA_HINT_DEFAULT_MASK) {
> +
> + case LADSPA_HINT_DEFAULT_MINIMUM:
> + u->control[h] = lower;
> + break;
> +
> + case LADSPA_HINT_DEFAULT_MAXIMUM:
> + u->control[h] = upper;
> + break;
> +
> + case LADSPA_HINT_DEFAULT_LOW:
> + if (LADSPA_IS_HINT_LOGARITHMIC(hint))
> + u->control[h] = (LADSPA_Data) exp(log(lower) * 0.75 + log(upper) * 0.25);
> + else
> + u->control[h] = (LADSPA_Data) (lower * 0.75 + upper * 0.25);
> + break;
> +
> + case LADSPA_HINT_DEFAULT_MIDDLE:
> + if (LADSPA_IS_HINT_LOGARITHMIC(hint))
> + u->control[h] = (LADSPA_Data) exp(log(lower) * 0.5 + log(upper) * 0.5);
> + else
> + u->control[h] = (LADSPA_Data) (lower * 0.5 + upper * 0.5);
> + break;
> +
> + case LADSPA_HINT_DEFAULT_HIGH:
> + if (LADSPA_IS_HINT_LOGARITHMIC(hint))
> + u->control[h] = (LADSPA_Data) exp(log(lower) * 0.25 + log(upper) * 0.75);
> + else
> + u->control[h] = (LADSPA_Data) (lower * 0.25 + upper * 0.75);
> + break;
> +
> + case LADSPA_HINT_DEFAULT_0:
> + u->control[h] = 0;
> + break;
> +
> + case LADSPA_HINT_DEFAULT_1:
> + u->control[h] = 1;
> + break;
> +
> + case LADSPA_HINT_DEFAULT_100:
> + u->control[h] = 100;
> + break;
> +
> + case LADSPA_HINT_DEFAULT_440:
> + u->control[h] = 440;
> + break;
> +
> + default:
> + pa_assert_not_reached();
> + }
> + }
> + else {
> + if (LADSPA_IS_HINT_INTEGER(hint)) {
> + u->control[h] = roundf(control_values[h]);
> + }
> + else {
> + u->control[h] = control_values[h];
> + }
> + }
> +
> + h++;
> + }
> +
> + /* set the use_default array to the user data */
> +
> + memcpy(u->use_default, use_default, u->n_control);
Shouldn't the size argument be u->n_control * sizeof(pa_bool_t)?
--
Tanu
