On 03.10.2017 14:58, Tanu Kaskinen wrote:
On Mon, 2017-10-02 at 14:32 +0200, Georg Chini wrote:
On 02.10.2017 11:31, Tanu Kaskinen wrote:
On Sun, 2017-10-01 at 20:31 +0200, Georg Chini wrote:
On 01.10.2017 18:16, Tanu Kaskinen wrote:
On Sat, 2017-08-19 at 17:48 +0200, Georg Chini wrote:
+/* List handlers */
+char *pa_core_message_handler_list(pa_core *c);
Putting this function to core-messages.h doesn't seem right to me. The
function will never be used outside core.c, so the it should be a
private function in core.c.
If I put it in core.c, I would need to expose the pa_core_message_handler
structure, which you wanted to keep private.
Something that didn't occur to me when I reviewed this patch yesterday
is that the string that the list-handlers returns is tailor-made for
pactl/pacmd, which is not how the message handlers are expected to
behave. What if an application wants to not only print a message (of
which layout is already decided by the server), but to get a list of
handlers and do something else with that information?
I don't understand. Message handlers return a single string, so
parsing the result is the task of the sender. The format returned
by the handler is rather simple, apart from the header line
it only consists of lines of the form
handler-path, " ", description
This should be easy to parse.
Not as easy as it would be with a standardized serialization format.
If we assume that the information that list-handlers returns is
basically a list of (path,description) pairs (which IMO is not a good
idea, I'll discuss that later), as an application developer I'd want to
be able to do this rather than dealing with the details of string
parsing:
const char *state = message_parameters;
if (pa_message_read_start_list(&state) < 0)
fail("Expected a list.");
while (pa_message_read_end_list(&state) < 0) {
char *path;
char *description;
if (pa_message_read_path(&state, &path) < 0)
fail("Expected a path.");
if (pa_message_read_string(&state, &description) < 0)
fail("Expected a string.")
/* Do something with path and description. */
pa_xfree(description);
pa_xfree(path);
}
The documentation becomes much simpler too. You don't need to describe
the string in detail (what are the separators, how to deal with
escaping etc.), this is sufficient to document the exact format of the
message:
list-handlers
parameters: none
response: [Path String]
The message returns all message handlers as a list. The list
items contain the handler path and the handler description.
It can be explained just once how to read this notation. The
"parameters:" section describes the parameters of the message (in this
case there are none) and the "response:" section describes type of the
response parameters. Square brackets denote a list, and inside the
square brackets is the list item type (in this case the items consist
of a Path and a String value).
I still don't understand. The return value of a message is always a
string, so all results that are passed back to the caller must be part
of a single string. The message handler is not passing back any
structures or lists.
What you propose could only be done with the introspection API.
Now, on the topic why (path,description) pairs is not a very good idea:
choosing that structure assumes that message handlers won't have any
other properties in the future. We can't change the response format
specification, so it should be flexible enough so that we can add new
properties without breaking applications that were written before the
new properties existed.
If we use (path,description) pairs now, and for example add the handler
type (e.g. sink, loopback, etc) as a new property later, we'll have to
introduce a new message to be able to deal with that. Not ideal.
Yes, not ideal, but it would be the only way to deal with it.
See below for some reasoning why I still think it should be
implemented using messages.
Also, it's questionable why the list-handlers command is implemented
with the messaging API in the first place. It's core functionality, so
why is it not implemented using the normal introspection API?
Two reasons:
a) It's much simpler than using the introspection API
Ok, but didn't we agree earlier that the messaging API is only meant
for modules, and new core features shall be added using the existing
API conventions?
No, we did not. If that was the purpose, my first approach to pass
parameters to modules would have been fully sufficient. On the
contrary, you wanted to have something more general, so that
any object could have a message handler.
The rationale, if you've forgotten it, was that it's
annoying for application developers if the core API uses inconsistent
conventions and introspection functionality needs to be searched from
two different places.
I might accept it if you put the client-facing bits in the
introspection API, but use the messaging system behind the scenes.
I think it does not really make sense to use the messaging API
at all in that case. It's either via messaging or introspection API
and I have to admit that adding to the introspection API for such
a simple information request is exactly what I wanted to avoid.
The introspection API has massive overhead. Take a look how
many places you have to touch to add some tiny functionality.
The messaging system provides a simple way to get or send
information whenever this information can be expressed as
a string. This is where the messaging system can add real
value and getting the handlers and description is a typical
example for that.
The point is, that functionality can be added by simply writing
a message handler without having to deal with the complexity
of the introspection API.
If
you want pursue that route, I'd like get an ok from Arun as well. This
is not only about the list-handlers message, it's about all future core
features.
Another possibility is to deprecate the whole introspection API and
reimplement it using the messaging API, but you probably don't want to
take such a big project.
I wonder why you are talking black and white here. As said above,
I think the messaging API could be used for simple tasks, while
the introspection API is useful if the information exchanged is
rather complex. I don't think developers are overburdened if they
have to look in two places for some functionality.
b) We should at least have one example in the code
which shows how a message handler i implemented.
It's certainly good to have an example now so that we can discuss how
the message parameter serialization is supposed to work, but I don't
think we need to apply that example to master. I hope we'll have some
other example by the time 12.0 is released (IIRC, you plan to add a
message handler to module-loopback), but it's not a big deal if there
are no examples.
Yes, I will add a message handler to module-loopback, but there
are still a lot of outstanding patches for it and first tests showed
that adding a message handler to change the latency on the fly
has non-trivial implications. So not sure if I will have it for 12.0.
The original motivation for writing the patches was an equalizer
module developed by somebody in Italy which looked very
promising to me. But the equalizer would need a GUI to change
parameters to be really useful and I was looking for an easy
way to do the parameter exchange between the module and
the GUI. So maybe this might go into 12.0.
And one more thing: if the handler descriptions are visible to clients
and the descriptions are mutable, clients should be notified about
description changes.
How? Sending a signal?
A signal would be appropriate if the list-handlers operation is
implemented only as a message, but if it's implemented using the
introspection API, then the subscription system should be used for the
notifications.
Another thing that came to my mind is that notifications should also be
sent when message handlers are added and removed.
So you mean adding a PA_SUBSCRIPTION_EVENT_MESSAGE_HANDLER?
This would be subject of an additional patch I think.
_______________________________________________
pulseaudio-discuss mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
