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

[Ulion]

first of all, thanks for this long review.

2007/8/13, Reimar Döffinger <Reimar.Doeffinger at stud.uni-karlsruhe.de>:
>
> > +  cc_check -framework IOKit && tmp_run && _apple_remote=yes
>
> configure checks should not execute anything unless absolutely
> necessary.
I think this run check is necessary when apple-remote support is setted 'auto'.

>
>
> > +#ifdef HAVE_APPLE_REMOTE
> > +  { { AR_PLAY, 0}, "pause" },
> > ...
> > +  { { AR_VDOWN, 0 }, "volume -1"},
> > +#endif
>
> I think the #ifdef is not needed.
in the context, other codes all have this '#ifdef' such as joystick's,
so we follow the style.

>
> [...]
> > +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?
fixed. ar_codes must also be static const.

>
> > +static IOHIDDeviceInterface **hidDeviceInterface = NULL;
> > +static int inited = 0;
> > +static int hidDeviceIsOpen = 0;
> > +static io_iterator_t hidObjectIterator = (io_iterator_t)NULL;
>
> Useless initializations.
hidObjectIterator moved into init function.
other init maybe useless, but for make sure.

>
> [...]
> > +    CFMutableDictionaryRef hidMatchDictionary = NULL;
> > +    IOReturn ioReturnValue = kIOReturnSuccess;
>
> These initializations are useless as well
removed.

>
> > +    // 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.
fixed.

>
> > +IOHIDElementCookie *cookies = NULL;
> > +int nr_cookies = 0;
>
> Either make them static or use less generic names.
removed, these two variables only for getHIDCookies' return, just use
function paramters instead of global variables.

>
> > +    IOHIDElementCookie *cookies = calloc(256, sizeof(IOHIDElementCookie));
>
> > +        object = (CFDictionaryGetValue(element, CFSTR(kIOHIDElementCookieKey)));
>
> Outermost () is useless.
removed.

>
> > +        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?
fixed.

>
> > +    io_object_t hidDevice = (io_object_t)NULL;
>
> There should be no need to cast NULL.
yes, no need to init this variable.

>
> > +    if (nr_cookies) {
> > +        if (cookies)
> > +            free(cookies);
> > +        nr_cookies = 0;
> > +    }
>
> Why don't you free them on close instead?
global variables, removed.

>
> > +    if (hidObjectIterator == (io_iterator_t)NULL)
> > +        return -1;
>
> IOIteratorIsValid?
fixe.

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

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

> > +            {
> > +                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.
it's not steal, events from Apple Remote can be grabbed by any
process, certainly mplayer in slave mode also can grab the events.
also, if the master of the slaved mplayer want these events, it will
open the device interface first in exclusive mode with
kIOHIDOptionsTypeSeizeDevice paramter before mplayer does, so mplayer
will start with mp_input_ar_init() failed, and won't possible to steal
master's events.
also, for make sure, master can start mplayer with noar paramter to
disable Apple Remote for slaved mplayer.

>
> [...]
> > +        if (event.elementCookie == (IOHIDElementCookie)5)
>
> This definitely demands for an explaining comment
there is no official document for Apple Remote support, only some
no-official codes such as Remote Control Wrapper introduced a way to
identity different input from Apple Remote, skip value 5 is probably
for identify all different inputs from Apple Remote by each different
sum cookie values to prevent from has to detect each member of squence
of cookies.

>
> > +    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...
yes, has to do it.
in current mac osx, every process can open the device for Apple
Remote, but only one can in exclusive mode, since we need exclusive to
prevent triger system's or other apps' response for Apple Remote when
mplayer is running front, we has to open the device in exclusive mode.
and if mplayer or the master of slaved mplayer lost focus, mplayer
should release the device from current exclusive mode, since we need
not input when mplayer or master lost focus, so close it is  the right
choice.
as your question, why require each app to handle the focus stuff,
because some apps, need keep open the device whenever it foreground or
background, such as some remote control apps, so here system leave the
app itself to handle focus and release stuff.

>
> [...]
> > +    // dispose of queue
> > +    (*queue)->dispose(queue);
> > +    // release the queue we allocated
> > +//    (*queue)->Release(queue);
>
> Why is it commented out?
fixed.

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

> > +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.
modified as you advice.

such a long grammer check, thank you, and here is the updated patch.
-- 
Ulion
-------------- next part --------------
A non-text attachment was scrubbed...
Name: apple_remote_grammer_fixed.patch
Type: application/octet-stream
Size: 27941 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/mplayer-dev-eng/attachments/20070813/a4708e89/attachment.obj>