[FFmpeg-devel] [PATCH] aviobuf: Retry if the read/write function returns AVERROR(EAGAIN)

[Martin Storsjö]

On Mon, 3 Jan 2011, Reimar D?ffinger wrote:

> On 3 jan 2011, at 13:27, Martin Storsjo <martin at martin.st> wrote:
> > ---
> > libavformat/aviobuf.c |   28 +++++++++++++++++++++++++---
> > 1 files changed, 25 insertions(+), 3 deletions(-)
> > 
> > diff --git a/libavformat/aviobuf.c b/libavformat/aviobuf.c
> > index df76507..5aff31c 100644
> > --- a/libavformat/aviobuf.c
> > +++ b/libavformat/aviobuf.c
> > @@ -19,6 +19,9 @@
> >  * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
> >  */
> > 
> > +/* needed for usleep() */
> > +#define _XOPEN_SOURCE 600
> > +#include <unistd.h>
> > #include "libavutil/crc.h"
> > #include "libavutil/intreadwrite.h"
> > #include "avformat.h"
> > @@ -88,11 +91,29 @@ ByteIOContext *av_alloc_put_byte(
> >     return s;
> > }
> > 
> > +static inline int retry_transfer_wrapper(void *opaque, unsigned char *buf,
> > +                                         int size,
> > +                                         int (*transfer_func)(void *, unsigned char *, int)) {
> > +    int fast_retries = 5;
> > +    while (1) {
> > +        int ret = transfer_func(opaque, buf, size);
> > +        if (ret == AVERROR(EAGAIN)) {
> > +            if (fast_retries)
> > +                fast_retries--;
> > +            else
> > +                usleep(1000);
> > +        } else
> > +            return ret;
> > +    }
> > +}
> > +
> > static void flush_buffer(ByteIOContext *s)
> > {
> >     if (s->buf_ptr > s->buffer) {
> >         if (s->write_packet && !s->error){
> > -            int ret= s->write_packet(s->opaque, s->buffer, s->buf_ptr - s->buffer);
> > +            int ret= retry_transfer_wrapper(s->opaque, s->buffer,
> > +                                            s->buf_ptr - s->buffer,
> > +                                            s->write_packet);
> 
> This duplicates code, also the question is why the read_packet and 
> write_packet functions should be allowed to return EAGAIN and if this is 
> sufficiently documented.

It duplicates a little of the logic from avio.c:retry_transfer_wrapper, 
yes, but this version is a bit simpler, since the re-reading for short 
reads already exist in aviobuf.

If we'd want to avoid this duplication, a third function (e.g. 
url_read_retry) could be added which would retry on EAGAIN but return as 
soon as some data is available. Or sharing the code from avio.c in some 
other way... (Just setting url_read_complete as packet_read in url_fdopen, 
instead of url_read, wouldn't work properly, since fill_buffer() then 
would block needlessly long, filling the whole buffer even if only a 
single byte was needed.) But that would only work for the url_fdopen() 
case, not for other functions wrapped in ByteIOContext.

As for whether they are allowed to return EAGAIN - url_read_complete 
already handles EAGAIN, so url_read seems to be allowed to return it, 
which ByteIOContext uses directly at the moment. Not sure if write_packet 
is allowed to return it though - url_write hides it at least.

// Martin