[MPlayer-dev-eng] [PATCH] (v2) Multi-code events get LIRC input out of sync

Dennis Vshivkov jaimor at orcon.net.nz
Mon Jan 26 14:34:47 CET 2009


On Sun, Jan 25, 2009 at 07:46:52PM +0100, Reimar Dцffinger wrote:

 > On Sat, Jan 24, 2009 at 03:56:08AM +1300, Dennis Vshivkov
 > wrote:
 >> attached is a patch I've been using for a few months in my
 >> personal MPlayer Debian package.

 > I have seen it, but I did and still do not like it, not being
 > able to use select also means that proper "sleep until input
 > or next frame" behaviour will not be possible.

Sure thing. The select(2), in a sense, is already being not
used, but rather abused by input/lirc.c.

 > Unfortunately there seems to be no other way to fix it, so
 > basically this is ok.

Yeah, the `right' way seems:

[1] Make lirc_sock non-blocking.

[2] Rework current LIRC handling so that it's not polled
constantly but made a proper part of top-level select(2).

[3] When top-level select(2) indicates data on lirc_sock, poll
lirc_nextcode() until no code is returned.

Keeping solutions of separate problems separate seems favoured
on the ML, which is good.  The [2], of course, solves a problem
that is rather orthogonal to what made me write the patch, which
basically implemented what [1] and [3] were in absence of [2].

The [2] can be added later, provided the polling, despite having
shrunk from repeated select(2) to repeated read(2), keeps
bothering whoever's knowledgeable enough to do it.

 >> @@ -21,7 +22,8 @@
 >>  
 >>  int 
 >>  mp_input_lirc_init(void) {
 >> -  int lirc_sock;
 >> +  int lirc_sock,
 >> +      mode;
 >>  
 >>    mp_msg(MSGT_LIRC,MSGL_V,MSGTR_SettingUpLIRC);
 >>    if((lirc_sock=lirc_init("mplayer",1))==-1){

 > The lirc_sock line should not be modified.

Fixed.

 >> @@ -36,12 +38,18 @@
 >>      return -1;
 >>    }
 >>  
 >> +  if ((mode = fcntl(lirc_sock, F_GETFL)) < 0 ||
 >> +      fcntl(lirc_sock, F_SETFL, mode | O_NONBLOCK) < 0) {
 >> +    mp_msg(MSGT_LIRC, MSGL_ERR, "setting non-blocking mode failed: %s\n",
 >> +	strerror(errno));
 >> +    lirc_deinit();
 >> +    return -1;
 >> +  }

 > This is not very readable, IMO it should be
 >> mode = fcntl(lirc_sock, F_GETFL);
 >> if (mode < 0 || fcntl(lirc_sock, F_SETFL, mode | O_NONBLOCK) < 0) {
 >> ...

Fixed.

 >> -  fd_set fds;
 >> -  struct timeval tv;
 >>    int r,cl = 0;
 >>    char *code = NULL,*c = NULL;
 >>  
 >> @@ -59,21 +67,7 @@
 >>      return w;
 >>    }
 >>        
 >> -  // Nothing in the buffer, pool the lirc fd
 >> -  FD_ZERO(&fds);
 >> -  FD_SET(fd,&fds);
 >> -  memset(&tv,0,sizeof(tv));
 >> -  while((r = select(fd+1,&fds,NULL,NULL,&tv)) <= 0) {
 >> -    if(r < 0) {
 >> -      if(errno == EINTR)
 >> -	continue;
 >> -      mp_msg(MSGT_INPUT,MSGL_ERR,"Select error : %s\n",strerror(errno));
 >> -      return MP_INPUT_ERROR;
 >> -    } else
 >> -      return MP_INPUT_NOTHING;
 >> -  }
 >> -  
 >> -  // There's something to read
 >> +  // Nothing in the buffer, poll the lirc fd

 > I think this means at least the
 >> #include <sys/types.h>
 >> #include <sys/time.h>
 > can go,

Seems so (compiles on me).  Fixed.

 > also the pool -> poll fix should not be part of this patch
 > (and I just fixed that in SVN).

Fixed.

The v2 of the patch is attached.

-- 
/Dennis Vshivkov <jaimor at orcon.net.nz>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: lirc-input-fully-consume.diff
Type: text/x-diff
Size: 3362 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/mplayer-dev-eng/attachments/20090127/6342b81a/attachment.diff>


More information about the MPlayer-dev-eng mailing list