[MPlayer-dev-eng] [PATCH] Apple Remote support

[Reimar Döffinger]

Hello,

First of all, all new functions are supposed to have doxygen-compatible
documentation/comments.

> +  cc_check -framework IOKit && tmp_run && _apple_remote=yes

configure checks should not execute anything unless absolutely
necessary.


> +#ifdef HAVE_APPLE_REMOTE
> +  { { AR_PLAY, 0}, "pause" },
> +  { { AR_PLAY_HOLD, 0}, "quit" },
> +  { { AR_NEXT, 0 }, "seek 30" },
> +  { { AR_NEXT_HOLD, 0 }, "seek 120" },
> +  { { AR_PREV, 0 }, "seek -10" },
> +  { { AR_PREV_HOLD, 0 }, "seek -120" },
> +  { { AR_MENU, 0 }, "osd" },
> +  { { AR_MENU_HOLD, 0 }, "mute" },
> +  { { AR_VUP, 0 }, "volume 1"},
> +  { { AR_VDOWN, 0 }, "volume -1"},
> +#endif

I think the #ifdef is not needed.

[...]
> +cookie_keycode_map_t ar_codes_tiger[] = {
[...]
> +cookie_keycode_map_t ar_codes_leopard[] = {

should be static const?

> +cookie_keycode_map_t *ar_codes = &ar_codes_tiger[0];

should be static?

> +static IOHIDDeviceInterface **hidDeviceInterface = NULL;
> +static int inited = 0;
> +static int hidDeviceIsOpen = 0;
> +static io_iterator_t hidObjectIterator = (io_iterator_t)NULL;

Useless initializations.

[...]
> +    CFMutableDictionaryRef hidMatchDictionary = NULL;
> +    IOReturn ioReturnValue = kIOReturnSuccess;

These initializations are useless as well

> +    // Set up a matching dictionary to search the I/O Registry by class
> +    // name for all HID class devices
> +    hidMatchDictionary = IOServiceMatching("AppleIRController");
> +
> +    // Now search I/O Registry for matching devices.
> +    ioReturnValue = IOServiceGetMatchingServices(masterPort, hidMatchDictionary, hidObjectIterator);
> +
> +    // If search is unsuccessful, print message and hang.
> +    if ((ioReturnValue != kIOReturnSuccess) | (*hidObjectIterator == (io_iterator_t)NULL)) {

makes more sense to use || instead of |.
Also according to the documentation,
IOIteratorIsValid(*hidObjectIterator) is the right way to check.
And the {} are not needed.

> +IOHIDElementCookie *cookies = NULL;
> +int nr_cookies = 0;

Either make them static or use less generic names.

> +    IOHIDElementCookie *cookies = calloc(256, sizeof(IOHIDElementCookie));

> +        object = (CFDictionaryGetValue(element, CFSTR(kIOHIDElementCookieKey)));

Outermost () is useless.

> +        cookies[nr_cookies++] = (IOHIDElementCookie)number;

Shouldn't you check that nr_cookies never can reach 256? Or is that
given due to some other condition?

> +    io_object_t hidDevice = (io_object_t)NULL;

There should be no need to cast NULL.

> +    if (nr_cookies) {
> +        if (cookies)
> +            free(cookies);
> +        nr_cookies = 0;
> +    }

Why don't you free them on close instead?

> +    if (hidObjectIterator == (io_iterator_t)NULL)
> +        return -1;

IOIteratorIsValid?

> +    while ((hidDevice = IOIteratorNext(hidObjectIterator))) {
> +        if (CreateHIDDeviceInterface(hidDevice, &hidDeviceInterface) < 0)
> +            return -1;
> +        if ((cookies = getHIDCookies((IOHIDDeviceInterface122 **)hidDeviceInterface)) == NULL)
> +            return -1;

Why not just continue? And both cases miss to free at least hidDevice,
possibly also hidDeviceInterface.

> +    if (GetFrontProcess(&frProc) == noErr)
> +    {               
> +        if (GetCurrentProcess(&myProc) == noErr)
> +        {       
> +            if (SameProcess(&frProc, &myProc, &sameProc) == noErr)

Three nesting levels is really overkill for this:
> if (GetFrontProcess(&frProc) == noErr && GetCurrentProcess(&myProc) == noErr &&
>     SameProcess(&frProc, &myProc, &sameProc) == noErr)

> +            {
> +                if (sameProc)
> +                    return 1;
> +                // if mplayer running in slave mode, also check parent process
> +                if (slave_mode && GetProcessPID(&frProc, &parentPID) == noErr)
> +                    return parentPID==getppid();

Do I understand this right, that in slave mode you steal events from the
parent process? This might do what you want in this case, but it's not
sane behaviour for a program.

[...]
> +        if (event.elementCookie == (IOHIDElementCookie)5)

This definitely demands for an explaining comment

> +    if (!is_mplayer_front()) {
> +        if (hidDeviceIsOpen) {
> +            (*hidDeviceInterface)->close(hidDeviceInterface);
> +            hidDeviceIsOpen = 0;
> +        }
> +        return MP_INPUT_NOTHING;
> +    }
> +    // current front, if closed, re-open the device to make sure not affected by other apps or system use apple remote
> +    else if (!hidDeviceIsOpen) {
> +        if( kIOReturnSuccess == (*hidDeviceInterface)->open(hidDeviceInterface, kIOHIDOptionsTypeSeizeDevice) )
> +            hidDeviceIsOpen = 1;
> +    }

Is it really necessary to close and reopen the device on each focus
change? If so, isn't there some at least remotely sane interface for
using this device? It is rather stupid to require each application to
reinvent the focus handling the user interface is already doing for all
other input devices...

[...]
> +    // dispose of queue
> +    (*queue)->dispose(queue);
> +    // release the queue we allocated
> +//    (*queue)->Release(queue);

Why is it commented out?

> +#ifndef __INPUT_AR_H_
> +#define __INPUT_AR_H_

Not sure if someone else already said so, but names starting with __ or
_ and uppercase letter are reserved and may not be used.

> +extern int mp_input_ar_init(void);
> +extern int mp_input_ar_read(int fd);
> +extern void mp_input_ar_close(int fd);

The extern is not really needed for functions, but feel free to keep it
if you like.

Greetings,
Reimar Döffinger