[MPlayer-dev-eng] [PATCH] LIRC input: minor fix, cleanups, and proper select(2)

[Dennis Vshivkov]

On Tue, Jan 27, 2009 at 08:28:44PM +0100, Reimar Dцffinger wrote:

 > On Wed, Jan 28, 2009 at 05:03:04AM +1300, Dennis Vshivkov
 > wrote:
 >>> 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;

 > That does not explain why cmd_buf is used instead of writing
 > into dest directly...

Indeed, it doesn't.  You were right, then, cmd_buf seems linked
to the MP_CMD_MAX_SIZE consideration.  To read _all_ commands
out of a code, either ample cmd_buf is needed, or `code'
variable needs be saved across calls.  In the vein of much code
there, the worse way made it.

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

 > That does not make any sense to me. First, there is already a
 > loop to call lirc_code2char multiple times. Second,
 > MP_INPUT_RETRY can not work for that purpose since
 > lirc_nextcode would return NULL and lirc_code2char would not
 > be called again.  Which of course raises the additional
 > question: what is the point of returning MP_INPUT_RETRY...

I have only one explanation that makes any sense out of
MP_INPUT_RETRY.  As I supposed before already, in the past LIRC
library might have had no buffer internal to lirc_nextcode(),
and my original problem wasn't so much of a problem.  Codes that
produced NO commands would make mp_input_lirc_read() return
MP_INPUT_NOTHING.  The next poll would come in a short while,
get through internal select(2), and retrieve the next code.

Someone with lots of empty codes from their remote might have
been irritated by these short but perceivable delays enough to
come up with MP_INPUT_RETRY idea.  Not the best idea, but before
the MP_INPUT_RETRY hoop went in, up to only one LIRC code, empty
or not, per call of mp_input_read_cmd() was handled.  With it,
at least one command was obtained, if there was any pending on
lirc_sock.

Of course, if there ever was any sense to MP_INPUT_RETRY, it
definitely isn't there anymore.

 > I think MP_INPUT_RETRY is just a way to avoid another loop
 > around lirc_nextcode, so it is not something really
 > necessary.

Sure, and a bad way, too.  There's so much cruft I've already
seen around ./input, no wonder nowadays patches undergo such
pleasantly draconian reviews for readability, etc. =)

 >>> 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?

 > No, I mean this scenario:
 > keys A and B were pressed, both events are waiting to be read
 > from teh fd.
 > Select sees there is data available, calls mp_input_lirc_read.
 > lirc_nextcode reads both, returns only A.
 > A creates a command, which is returned.
 > Since a command was returned (thus no MP_INPUT_RETRY) and select does
 > not show any data waiting, mp_input_lirc_read will not be called, thus
 > key B will not be processed.

Well, this is of course a part of making mp_input_lirc_read()
usable with top-level select(2).

 > Getting rid of MP_INPUT_RETRY and adding a loop around
 > lirc_nextcode instead would fix this...

Getting rid of MP_INPUT_RETRY is good on its own.  A loop around
lirc_nextcode() to do what MP_INPUT_RETRY _might_ have done in
the past would skip `empty' codes, yes.  But why not fix all of
the piecemeal mess at once?  See below.

 >> [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?

 > Actually I think this is mostly desirable, it avoids that a
 > single key press with a far too high repeat rate can make
 > MPlayer unusable (e.g.  consider a single keypress that
 > creates 1000 seek events, that will make MPlayer hang for
 > quite some time).

Alright, here's another patchset.  Damn, it was hard to
adhere to the strange lack of spaces after commas, in
arithmetics, or after keywords...  Other files are hardly
consistent either.  Is there a single code style document at
all?

(I noticed my patch was applied.  Yay!  Thanks for your help.)

There's three parts.  The first is (sic!) minor fixes for v2 of
my original patch, the second is some cleanups before a rewrite;
the last is re-select(2)ification and cruft removal.

  Legend: ! fix, + addition, - removal, * change.

  lirc-input-1-no-delay-fix.diff:

    ! O_NONBLOCK part of mp_input_lirc_init() moved to before
      lirc_readconfig(): where it was, it missed
      lirc_freeconfig() upon error.
    * Renamed `mode' variable in mp_input_lirc_init() to
      `flags', which it really is.

  lirc-input-2-janitorial.diff:

    * Readability: renamed variables in mp_input_lirc_read() to
      better reflect their meaning: c -> cmd, cl -> cmd_len,
      r -> ret, s -> dest_size.
    * Consistency: mp_input_lirc_read() is not a static, so
      should have its name at the beginning of a line.
    * Consistency: messages specific to LIRC in input/lirc.c
      should be MSGT_LIRC, not the generic MSGT_INPUT.
    * Consistency: report a LIRC error not only when it happens
      in lirc_nextcode(), but also when it happens in
      lirc_code2char(); also, for clarity, report where it
      happened.

  lirc-input-3-proper-select.diff:
    - cmd_buf global in input/lirc.c: mp_input_lirc_read()
      writes directly to the passed buffer.
    - MP_INPUT_RETRY return code for mp_input_fd_t#read_func()
      method: long unfunctional and now entirely redundant,
      mp_input_lirc_read() ceases its only use.
    * mp_input_lirc_read() now processes all of input pending on
      LIRC socket and returns all resulting commands in one go.
    * LIRC socket is made part of top-level select(2) in
      input.c, thus mp_input_lirc_read() is only polled when
      there's new input on the socket.

Comments?

-- 
/Dennis Vshivkov <jaimor at orcon.net.nz>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: lirc-input-1-no-delay-fix.diff
Type: text/x-diff
Size: 1601 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/mplayer-dev-eng/attachments/20090129/9c44ab87/attachment.diff>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: lirc-input-2-janitorial.diff
Type: text/x-diff
Size: 2980 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/mplayer-dev-eng/attachments/20090129/9c44ab87/attachment-0001.diff>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: lirc-input-3-proper-select.diff
Type: text/x-diff
Size: 4770 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/mplayer-dev-eng/attachments/20090129/9c44ab87/attachment-0002.diff>