On May 19, 2011, at 3:11 PM, Tanu Kaskinen wrote:

> 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).

I think the best solution for now would be to move it to pulsecore.

> 
> 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 <m...@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 <m...@slimlogic.co.uk>
>> Signed-off-by: Jorge Eduardo Candelaria <j...@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 <m...@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.

It seemed unnecessary since only the card name is being extracted from the
struct, but I get your point. I will change this.

> 
>> +} 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/?

Then these structs should defined the alsa-jack.h header, since it's going
to be moved to 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.

Agreed, I will change this.

> 
>> +#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.

I'm actually not very familiar with hooks, however do we need to signal the 
jack 
insertion/removal events from jack_report().

What would be a more clean solution?

> 
>> +}
>> +
>> +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()?

Again, not very familiar with rtpoll_item, but after your comment I realized how
to properly use it to get a non_blocking thread. I will also change this now.

> 
>> +
>> +    /* 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;
>> 
> 
> 
> _______________________________________________
> pulseaudio-discuss mailing list
> pulseaudio-discuss@mail.0pointer.de
> https://tango.0pointer.de/mailman/listinfo/pulseaudio-discuss

_______________________________________________
pulseaudio-discuss mailing list
pulseaudio-discuss@mail.0pointer.de
https://tango.0pointer.de/mailman/listinfo/pulseaudio-discuss

Reply via email to