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 <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.
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 at mail.0pointer.de
> https://tango.0pointer.de/mailman/listinfo/pulseaudio-discuss
