[pulseaudio-discuss] [RFC][PATCH] CoreAudio: add device detection module

[Daniel Mack]

On Tue, Nov 03, 2009 at 11:02:58PM +0100, Lennart Poettering wrote:
> On Tue, 03.11.09 14:40, Daniel Mack (daniel at caiaq.de) wrote:
> 
> > +typedef struct ca_device ca_device;
> > +
> > +struct ca_device {
> > +    AudioDeviceID id;
> > +    unsigned int  module_index;
> > +    PA_LLIST_FIELDS(ca_device);
> > +};
> 
> In PA we generally don't prefix static variables or file-local
> structs, since they are unlikely to cause namespace collisions and
> this is a nice way to determine easily if something is exported or not
> exported API. But that's just nitpicking.

Which prefix do you mean? I just didn't want to call it 'device', and
'ca_device' was short enough for me to be 'local' :)

> > +struct userdata {
> > +    PA_LLIST_HEAD(ca_device, devices);
> > +};
> > +
> > +static int ca_device_added(struct pa_module *m, AudioDeviceID id) {
> > +    char args[32];
> > +    pa_module *mod;
> > +    struct userdata *u = m->userdata;
> > +    struct ca_device *dev;
> > +
> > +    pa_assert(u);
> > +
> > +    pa_snprintf(args, sizeof(args), "device_id=%d", id);
> 
> Using pa_malloc_sprintf() is recommended. I am actually trying to get
> rid of the use of pa_snprintf() in PA entirely and am replacing it
> with the malloc version wherever I encounter it and it makes sense
> (i.e. outside of RT threads).

Ok, will fix.

> > +    if (!mod) {
> > +        pa_log_info("Failed to load module %s with arguments '%s'", DEVICE_MODULE_NAME, args);
> > +        return -1;
> > +    }
> > +
> > +    dev = pa_xnew0(ca_device, 1);
> > +    pa_assert(dev);
> 
> No need for this assert. The "x" in "pa_xnew0" should make clear that
> this is an aborting allocation anyway. 

Ok.

> > +    if (AudioHardwareAddPropertyListener(kAudioHardwarePropertyDevices, property_listener_proc, m)) {
> > +        pa_log("AudioHardwareAddPropertyListener() failed.");
> > +        goto fail;
> > +    }
> 
> Hmm, how are these event callbacks called? From a background thread?
> Or is this hooked up to the event loop in some way? Or is this only
> one-time during initialization?

They're called from a background thread, indeed. CoreAudio has no clue
about PA's mainloop implementation, so it can't hook up in there.

So yes, we might need some locking here. I just wasn't sure how PA likes
that (iow, which resources I need to lock and how), so I ignored it for
now. Any hint?

Thanks,
Daniel