[FFmpeg-devel] [PATCH] libavformat: Don't return errors if select is interrupted

[Martin Storsjö]

On Thu, 4 Mar 2010, Ronald S. Bultje wrote:

> On Thu, Mar 4, 2010 at 5:23 PM, Martin Storsj? <martin at martin.st> wrote:
> > Currently, most network protocols wait for data with a select loop, with a
> > 100 ms timeout and recheck url_interrupt_cb() for each round in the loop.
> > If the select is interrupted (e.g. by a signal), most of the protocols
> > return an error, even if a similar interruption in recv is handled by
> > retrying. The attached patch makes the protocols simply retry select if it
> > returned with EINTR or EAGAIN, instead of returning an error in these
> > cases. This makes the interruption behaviour consistent between select and
> > recv (or send or similar).
> >
> > Ronald, does this sound sensible to you?
> 
> +        } else if (n < 0) {
> [..]
> +            return AVERROR(EIO);
> 
> That should be applied separately and is OK.

Applied this part.

> For the rest, does it fix an actual bug? Let's start with EAGAIN:

I can't say that I've experienced this as a bug, but I'd imagine that this 
would be a nightmare to track down if it actually was the cause of 
spurious errors.

> EAGAIN, according to man 2 select:
>      [EAGAIN]           The kernel was (perhaps temporarily) unable to allo-
>                         cate the requested number of file descriptors.
> This is not the same as man 2 recv:
>      [EAGAIN]           The socket is marked non-blocking, and the receive
>                         operation would block, or a receive timeout had been
>                         set, and the timeout expired before data were
>                         received.
> 
> Since we're blocking, what happens here for recv() is that no data was
> received and the timer expired. For select(), then the return value is
> 0 (see man 2 recv:)
> 
> RETURN VALUES
> [..] If the time limit expires, select() returns 0. [..]
> 
> Because of that, IMO the EAGAIN part is not OK, the difference is intended.

Ah, yes, EAGAIN isn't relevant for select, my bad. Updated patch attached.

> As for EINTR, I in fact consider it bad behaviour to continue a loop
> when interrupted, so I've always frowned upon its use in recv(). My
> hypothesis is that people use other functions to manually interrupt,
> in combination with url_interrupt_cb(), and then want the function to
> return EINTR instead of EIO (default return value), but I'm not sure.
> Maybe somebody else knows? Tabled, for now... :-).

Well, the interruption could be anything (any signal delivered to the 
process, totally unrelated to interrupting/stopping reading from the 
network). If the caller really wants to stop the reading, that should be 
done through url_interrupt_cb and not through just any signal being 
delivered to the process, otherwise this may simply lead to spurious 
errors.

On Fri, 5 Mar 2010, Luca Abeni wrote:

> I think the idea was to let the application cope with EINTR and EAGAIN,
> but I might be misremembering (and I have no strong opinions on it :)

Yes, assuming that all the code (between the lowest level protocol 
receiving the data and the original av_read_frame call from the user 
application) is ready to handle EINTR/EAGAIN properly, that's probably the 
best solution.

One problem is that a very large portion of the code don't pass the 
original error codes through properly but simply return AVERROR(EIO) if 
something occurred.

Another problem is that code reading a few bytes at a time seldom is ready 
to actually be interrupted at any time, return an error code and then 
continue properly when called the next time again.

One (quite randomly chosen) example of this is ff_rtmp_packet_read, in 
libavformat/rtmppkt.c, lines 72 and onwards. This reads a packet, a few 
bytes at a time, using url_read and url_read_complete. On errors, it 
simply returns AVERROR(EIO), so the original EINTR is lost. These problems 
generally are quite simple to solve.

But what if the function has passed halfway through, has read part of the 
packet, but one of the url_read_complete calls fail with EINTR (since the 
process received some totally unrelated signal at this time)? Modifying 
this code to save the state (half of the packet already received, reuse 
that data on next call) so that it can be called again, since the caller 
didn't want to interrupt it. I guess that would require some kind of 
rewind-functionality for the URLContext.

I guess the same goes for all demuxers, too. Are they ready to handle 
reading functions spuriously returning EINTR/EAGAIN randomly, saving the 
state halfway through whatever packet was being read to return the EINTR 
error, so that the caller can try to read again in order to get the full 
packet that was being received. Or do they return an incomplete packet 
with as much data as had been received up to that point? As a caller of 
av_read_frame(), I don't want to receive an incomplete packet just since 
my process was interrupted by something unrelated while receiving it.

// Martin
-------------- next part --------------
A non-text attachment was scrubbed...
Name: select-interrupted.patch
Type: text/x-diff
Size: 2061 bytes
Desc: 
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20100305/46a7f731/attachment.patch>