I tried to apply the patch for easier review in meld, but unfortunately
it seems that some helpful software has eaten the first space on lines
that begin with one or more spaces. Because of that, "git am" doesn't
accept the patch.
I'll review this anyway, without meld.
Overall comments: there is some confusion about whether the jack stuff
is a core concept (hooks in pa_core) or an alsa specific thing (all jack
related type definitions are in alsa-jack.h). I think it probably makes
sense to keep alsa-jack.h as an alsa specific thing, but then the core
hooks shouldn't be tied to alsa-jack.h like they are now. In the longer
term I hope the thing how policy modules interact with jack detection
will be monitoring the (currently not existing) available status of the
port objects. However, if you don't want to start working on that now,
I'd just move alsa-jack.h under pulsecore (and of course rename it).
The only major issue in my opinion is the jack_fd reading. It should be
made non-blocking.
--
Tanu
On Thu, 2011-05-19 at 11:44 -0500, Jorge Eduardo Candelaria wrote:
> From: Margarita Olaya Cabrera <magi at slimlogic.co.uk>
>
> This patch adds support to module-alsa-card so that we can read jack insertion
> and removal events using the input device subsystem. i.e. we can detect
> headphone insertions and removals.
>
> This patch adds a new module parameter called jack_id that contains the ID of
> the jack input device associated with this sound card. If we receive a valid
> jack_id during module init then we start a reader thread that will read the
> jack input device and fire a hook on every removal and insertion event.
>
> Jack support development was kindly sponsored by Wolfson Microelectronics PLC
>
> Signed-off-by: Margarita Olaya Cabrera <magi at slimlogic.co.uk>
> Signed-off-by: Jorge Eduardo Candelaria <jedu at slimlogic.co.uk>
> ---
> src/modules/alsa/alsa-jack.h | 42 ++++++++++++
> src/modules/alsa/module-alsa-card.c | 120 +++++++++++++++++++++++++++++++++++
> src/pulsecore/core.h | 2 +
> 3 files changed, 164 insertions(+), 0 deletions(-)
> create mode 100644 src/modules/alsa/alsa-jack.h
>
> diff --git a/src/modules/alsa/alsa-jack.h b/src/modules/alsa/alsa-jack.h
> new file mode 100644
> index 0000000..4fc67c6
> --- /dev/null
> +++ b/src/modules/alsa/alsa-jack.h
> @@ -0,0 +1,42 @@
> +#ifndef foopulsejackdetecthfoo
> +#define foopulsejackdetecthfoo
> +
> +/***
> + This file is part of PulseAudio.
> +
> + Copyright 2011 Wolfson Microelectronics PLC
> + Author Margarita Olaya <magi at slimlogic.co.uk>
> +
> + PulseAudio is free software; you can redistribute it and/or modify
> + it under the terms of the GNU Lesser General Public License as published
> + by the Free Software Foundation; either version 2.1 of the License,
> + or (at your option) any later version.
> +
> + PulseAudio is distributed in the hope that it will be useful, but
> + WITHOUT ANY WARRANTY; without even the implied warranty of
> + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> + General Public License for more details.
> +
> + You should have received a copy of the GNU Lesser General Public License
> + along with PulseAudio; if not, write to the Free Software
> + Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307
> + USA.
> +***/
> +
> +#include <inttypes.h>
> +
> +typedef enum pa_jack_event {
> + PA_JACK_HEADPHONES,
> + PA_JACK_HEADSET,
> + PA_JACK_MICROPHONE,
> + PA_JACK_LINEOUT,
> + PA_JACK_UNKNOWN,
> + PA_JACK_MAX
> +} pa_jack_event_t;
> +
> +typedef struct pa_jack_detect {
> + pa_jack_event_t event;
> + char *card;
Instead of the card name, would it be better to store a pa_card pointer
here? It seems that you have the card object available when you
initialize this struct.
> +} pa_jack_detect_t;
Structs shouldn't have the _t suffix. (Enum typedefs should, so the
pa_jack_event_t name is correct.)
If this is alsa specific, like it seems to be, judging from the file
location and name, I recommend prefixing the types with "pa_alsa_jack_",
because all other alsa stuff uses "pa_alsa_" prefix too. Hmm... on the
other hand, this struct is passed to a core hook, so maybe these types
are not really alsa specific, and should be defined in a header under
src/pulsecore/?
Also, I think pa_jack_event would be a better name for what you've now
named pa_jack_detect, and the enum could be pa_jack_type_t.
> +#endif
> diff --git a/src/modules/alsa/module-alsa-card.c b/src/modules/alsa/module-alsa-card.c
> index e60aa5e..75202fb 100644
> --- a/src/modules/alsa/module-alsa-card.c
> +++ b/src/modules/alsa/module-alsa-card.c
> @@ -29,6 +29,9 @@
> #include <pulsecore/core-util.h>
> #include <pulsecore/modargs.h>
> #include <pulsecore/queue.h>
> +#include <pulsecore/thread.h>
> +
> +#include <linux/input.h>
>
> #include <modules/reserve-wrap.h>
>
> @@ -39,6 +42,7 @@
> #include "alsa-util.h"
> #include "alsa-sink.h"
> #include "alsa-source.h"
> +#include "alsa-jack.h"
> #include "module-alsa-card-symdef.h"
>
> PA_MODULE_AUTHOR("Lennart Poettering");
> @@ -55,6 +59,7 @@ PA_MODULE_USAGE(
> "source_properties=<properties for the source> "
> "namereg_fail=<pa_namereg_register() fail parameter value> "
> "device_id=<ALSA card index> "
> + "jack_id=<Jack device index> "
> "format=<sample format> "
> "rate=<sample rate> "
> "fragments=<number of fragments> "
> @@ -90,6 +95,7 @@ static const char* const valid_modargs[] = {
> "ignore_dB",
> "sync_volume",
> "profile_set",
> + "jack_id",
> NULL
> };
>
> @@ -106,6 +112,14 @@ struct userdata {
> pa_modargs *modargs;
>
> pa_alsa_profile_set *profile_set;
> +
> + /*userdata needed for jack detection */
> + int jack_fd;
> + char *jack_id;
> + pa_thread *thread;
> + pa_thread_mq thread_mq;
> + pa_rtpoll *rtpoll;
> + pa_rtpoll_item *rtpoll_item;
> };
>
> struct profile_data {
> @@ -284,11 +298,89 @@ static void set_card_name(pa_card_new_data *data, pa_modargs *ma, const char *de
> pa_xfree(t);
> }
>
> +/* Jack detection API */
> +static void jack_report(struct userdata *u, struct input_event *event)
> +{
> + pa_jack_detect_t jack;
> +
> + jack.card = u->card->name ;
> +
> + pa_log_debug("jack: card %s event type %x code %x value %x", u->card->name, event->type, event->code, event->value);
> +
> + /* only process switch events */
> + if (event->type != EV_SW) {
> + pa_log_debug("jack: card %s ignored event type %x", u->card->name, event->type);
> + return;
> + }
> +
> + switch (event->code) {
> + case SW_HEADPHONE_INSERT:
> + jack.event = PA_JACK_HEADPHONES;
> + break;
> + case SW_MICROPHONE_INSERT:
> + jack.event = PA_JACK_MICROPHONE;
> + break;
> + case SW_LINEOUT_INSERT:
> + jack.event = PA_JACK_LINEOUT;
> + break;
> + case SW_JACK_PHYSICAL_INSERT:
> + jack.event = PA_JACK_UNKNOWN;
> + break;
> + default:
> + pa_log_debug("jack: card %s ignored event code %x", u->card->name, event->code);
> + break;
> + }
> +
> + if (event->value)
> + pa_hook_fire(&u->core->hooks[PA_CORE_HOOK_JACK_INSERT], &jack);
> + else
> + pa_hook_fire(&u->core->hooks[PA_CORE_HOOK_JACK_REMOVE], &jack);
Firing core hooks directly from modules looks a bit dirty to me.
> +}
> +
> +static void *jack_detect_thread(void *userdata)
> +{
> + struct userdata *u = userdata;
> + struct input_event event;
> +
> + pa_assert(u);
> +
> + pa_log_debug("jack thread started for card %s", u->card->name);
> +
> + /* Install pa_thread_mq object in this thread */
> + pa_thread_mq_install(&u->thread_mq);
> +
> + while (pa_read(u->jack_fd, &event, sizeof(event), NULL) == sizeof(event)) {
> + jack_report(u, &event);
> + }
This looks like this can block indefinetely, so when trying to unload
the module, pa__done() will get stuck in the pa_memblockq_send() call.
You have the pa_rtpoll_item variable added to the userdata, but you
don't seem to be using it. Did you intend to create an rtpoll item for
the file descriptor instead of blocking in pa_read()?
> +
> + /* If this was no regular exit from the loop we have to continue
> + * processing messages until we receive PA_MESSAGE_SHUTDOWN */
> + pa_asyncmsgq_post(u->thread_mq.outq, PA_MSGOBJECT(u->core), PA_CORE_MESSAGE_UNLOAD_MODULE, u->module, 0, NULL, NULL);
> + pa_asyncmsgq_wait_for(u->thread_mq.inq, PA_MESSAGE_SHUTDOWN);
> +
> + pa_log_debug("jack thread shutting down");
> +}
> +
> +static void jack_get_initial_state(struct userdata *u)
> +{
> + struct input_event event;
> + int err;
> +
> + err = ioctl(u->jack_fd, EVIOCGSW(sizeof(event)), &event);
> + if (err < 0) {
> + pa_log("Failed to read initial %s jack status %d", u->jack_id, err);
> + return;
> + }
> +
> + jack_report(u, &event);
> +}
> +
> int pa__init(pa_module *m) {
> pa_card_new_data data;
> pa_modargs *ma;
> int alsa_card_index;
> struct userdata *u;
> + struct udev *udev;
> pa_reserve_wrapper *reserve = NULL;
> const char *description;
> char *fn = NULL;
> @@ -409,6 +501,26 @@ int pa__init(pa_module *m) {
> "is abused (i.e. fixes are not pushed to ALSA), the decibel fix feature may be removed in some future "
> "Pulseaudio version.", u->card->name);
>
> + /* Initialize pa_thread_mq object to communicate with jack_detect_thread */
> + u->rtpoll = pa_rtpoll_new();
> + pa_thread_mq_init(&u->thread_mq, m->core->mainloop, u->rtpoll);
> + u->rtpoll_item = NULL;
> +
> + /* Start the jack reader if we have a valid jack ID */
> + if (!pa_streq(pa_modargs_get_value(ma, "jack_id", NULL), "(null)")) {
> + u->jack_id = pa_sprintf_malloc("/dev/input/event%s",
> + pa_modargs_get_value(ma, "jack_id", NULL));
> +
> + /* open jack input event device */
> + if ((u->jack_fd = open(u->jack_id, O_RDONLY)) > 0) {
> + /* read the initial jack state */
> + jack_get_initial_state(u);
> +
> + /* start the jack reader thread */
> + if (!(u->thread = pa_thread_new("jack-reader", jack_detect_thread, u)))
> + pa_log("Failed to create jack reader thread");
> + }
> + }
> return 0;
>
> fail:
> @@ -471,6 +583,14 @@ void pa__done(pa_module*m) {
> if (u->profile_set)
> pa_alsa_profile_set_free(u->profile_set);
>
> + if (u->thread) {
> + pa_asyncmsgq_send(u->thread_mq.inq, NULL, PA_MESSAGE_SHUTDOWN, NULL, 0, NULL);
> + pa_thread_free(u->thread);
> + }
> +
> + if (u->jack_fd)
> + pa_close(u->jack_fd);
> +
> pa_xfree(u->device_id);
> pa_xfree(u);
>
> diff --git a/src/pulsecore/core.h b/src/pulsecore/core.h
> index 358b98d..830145a 100644
> --- a/src/pulsecore/core.h
> +++ b/src/pulsecore/core.h
> @@ -114,6 +114,8 @@ typedef enum pa_core_hook {
> PA_CORE_HOOK_CARD_PUT,
> PA_CORE_HOOK_CARD_UNLINK,
> PA_CORE_HOOK_CARD_PROFILE_CHANGED,
> + PA_CORE_HOOK_JACK_INSERT,
> + PA_CORE_HOOK_JACK_REMOVE,
> PA_CORE_HOOK_MAX
> } pa_core_hook_t;
>
