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

[Michael Niedermayer]

On Mon, Jan 03, 2011 at 05:26:03PM +0200, Martin Storsj? wrote:
> 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.

If none of the current protocols can generate EAGAIN on write_packet() then
we could just disallow it for future protocols to do it

If some of the current protocols can generate EAGAIN on write_packet() then we
need to handle this somewhere, either in the protocols or the callers. Where
its easier depends probably on how many protocols are affected and if the
code could benefit from being protocol specific

We also could change the API to use 2 size parameters.
One that specifies the minimum that we need and one the maximum we could accept
then all the retry code could be put inside protocols and still allow flexible
blocking and non blocking reads. It of course doesnt really avoid the retry
need but it might simplify API by reducing the duplication of complete and non
complete IO functions

In the end i dont know what is best, i know though what is worst and that is
leaving it buggy ...

[...]
--
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Into a blind darkness they enter who follow after the Ignorance,
they as if into a greater darkness enter who devote themselves
to the Knowledge alone. -- Isha Upanishad
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20110107/15f58da3/attachment.pgp>