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

[Dennis Vshivkov]

On Mon, Jan 26, 2009 at 02:59:55PM +0100, Reimar Dцffinger wrote:

 > On Tue, Jan 27, 2009 at 02:34:47AM +1300, Dennis Vshivkov
 > wrote:
 >> 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.

 > The reason I mentioned this is because I think this is not
 > all.  mp_input_lirc_read must be modified to return _all_
 > buffered commands, both those by lirc_nextcode, by
 > lirc_code2char or the cmd_buf itself, otherwise using select
 > in the top-level will not work.

Well, of course it won't otherwise, that's the reason I simply
said [2]: it includes all that.

Also, just for clarity: lirc_nextcode() returns not commands but
LIRC `codes', which are akin to keypresses (in case of my remote
they're equal to keypresses).  They're fed one by one to
lirc_code2char() that produces actual commands.

 > I wonder why that lirc.c cmd_buf is there anyway,

There's two twists to lirc_code2char():

    - it can produce multiple commands per code, however it
      needs to be called multiple times for that, producing one
      command per call, hence the presence of cmd_buf at all;

    - it can produce NO commands for a code, only changing its
      internal state or simply ignoring it; hence the existence
      of MP_INPUT_RETRY (lirc.c is its only user).

 > is this outdated code or is MP_CMD_MAX_SIZE too small or...?

These hunches seem more connected not to cmd_buf, but other
circumstances.

Very much like the first twist of lirc_code2char() above is the
same piecemeal behaviour of mp_input_lirc_read() passing the
results up (that's why cmd_buf is global).  Why?  I don't know,
maybe you're right and MP_CMD_MAX_SIZE can be too small.  Or
maybe it simply replicated how commands were submitted up by the
dedicated child process that used to handle LIRC input before.
In any case, it made top level select(2) inapplicable.

Maybe some time ago LIRC library used to read input codes one by
one, say, by reading a byte at a time, and the select(2) in
mp_input_lirc_read() was harmless to remotes like mine?  If so,
then the other guess of yours is right: the code is outdated.

 >> The v2 of the patch is attached.

 > Looks good to me, any objections? A well-tested patch to get
 > rid of the cmd_buf in lirc.c would be welcome :-).

I'd welcome the end of that piece of cruft, too.

 > Btw. using select for lirc can be enbled in input.c by
 > changing the 0 to 1 in the mp_input_add_cmd_fd in line 1771,

 > I just think it needs more changes to handle very quickly
 > consecutive keypresses correctly.

You mean, to handle cases when MP_CMD_MAX_SIZE is indeed too
small?


I see three ways out:

[1] The mp_input_read_cmd() code already drops commands that are
longer than that.  Since that's acceptable, perhaps extending
this lack of care for overly long commands to trailing parts of
overly long sequences of commands that come at once can be
tolerated?

[2] Instead of dropping commands that don't fit into the buffer,
they can be (sic!) delayed until more LIRC input comes.  Just
like my current problem, but, naturally, much less likely.

[3] The interface between mp_input_read_cmd() and
cmd_fd#read_func() methods changes.  The second value,
indicating whether read_func() should be called again right
away, should be returned in a way.


Like I originally said, this kind of decision needs someone more
knowledgeable, or perhaps authoritative.  I personally prefer
[3] for the long term, but, ironically, [2] for the short.

Your call.

-- 
/Dennis Vshivkov <jaimor at orcon.net.nz>