[FFmpeg-devel] [RFC] Questions and suggestions regarding AVSEEK_FORCE?

[Tomas Härdin]

On Wed, 2010-03-31 at 17:03 +0200, Michael Niedermayer wrote:
> On Wed, Mar 31, 2010 at 04:56:11PM +0200, Tomas H?rdin wrote:
> > On Wed, 2010-03-31 at 12:34 +0200, Michael Niedermayer wrote:
> > > On Tue, Mar 30, 2010 at 01:56:24PM +0200, Tomas H?rdin wrote:
> > > > 
> > > > Digging around some more it seems that offset stems from
> > > > dv_read_header(). For some reason the four-byte state that should be
> > > > 0x1f07003f (after anding) at dv.c:414 is 0x0a2feb0f instead. That does
> > > > not match what the file says at all (which says 0x1f0700bf).
> > > > Unsurprisingly, the proper value shows up 12928 B later. This leads me
> > > > to believe something messes up the ByteIOContext buffer before
> > > > dv_read_header() is called - probably after the probing is done.
> > > 
> > > maybe its:
> > >     if (url_fseek(*pb, 0, SEEK_SET) < 0) {
> > >         url_fclose(*pb);
> > >         if (url_fopen(pb, filename, URL_RDONLY) < 0)
> > >             return AVERROR(EIO);
> > >     }
> > > from ff_probe_input_buffer()
> > > 
> > > this doesnt look like it could work with pipes
> > > what happens if you limit the max probing size there to a value smaller than
> > > our internal buffer? (so seek back would be guranteed to work)
> > 
> > Limiting PROBE_BUF_MAX to 16 KiB makes it work, which makes sense since
> > that seek will fail for probe buffers. I've figured out a simple
> > solution: rewind the ByteIOContext by joining the probe buffer and the
> > context's buffer, taking the overlap between them into consideration.	
> > 
> > I've attached a patch that replaces the above block of code with a call
> > to a new function: url_rewind_with_probe_data()
> > This basically means a seek is avoided in all cases, not just for pipes,
> > which reduce the initial delay when demuxing most sources (especially
> > HTTP).
> > 
> > The patch can also be used with the patch I attached earlier in this
> > thread (skipping_stream_seek.patch). In fact, that patch is fairly
> > useless without this one.
> > 
> > Finally, the patch passes the regression tests.
> > 
> > /Tomas
> 
> >  avio.h    |   14 ++++++++++++++
> >  aviobuf.c |   36 ++++++++++++++++++++++++++++++++++++
> >  utils.c   |   13 +++++--------
> >  3 files changed, 55 insertions(+), 8 deletions(-)
> > 62925f155107c57c939a508930d56a09b17e2707  nonseeking_probe_rewind.patch
> 
> this is quite a nice solution it needs a few changes though, see below
> 
> 
> > diff --git a/libavformat/avio.h b/libavformat/avio.h
> > index 649de15..b53b015 100644
> > --- a/libavformat/avio.h
> > +++ b/libavformat/avio.h
> > @@ -433,6 +433,20 @@ int url_resetbuf(ByteIOContext *s, int flags);
> >  #endif
> >  
> >  /**
> > + * Rewinds the ByteIOContext using the specified buffer containing the first buf_size bytes of the file.
> > + * Used after probing to avoid seeking.
> > + * Joins buf and s->buffer, taking any overlap into consideration.
> > + * @note s->buffer must overlap with buf or they can't be joined and the function fails
> > + *
> > + * @param s The read-only ByteIOContext to rewind
> > + * @param buf The probe buffer containing the first buf_size bytes of the file
> > + * @param buf_size The size of buf
> > + * @return 0 in case of success, a negative value corresponding to an
> > + * AVERROR code in case of failure
> > + */
> > +int url_rewind_with_probe_data(ByteIOContext *s, unsigned char *buf, int buf_size);
> 
> this should have a ff_ prefix or be static as such quite tricky things
> should not be public API
> 

I opted for renaming it to ff_rewind_with_probe_data() and putting a
"@note This function is NOT part of the public API" in the header.

> 
> > +
> > +/**
> >   * Creates and initializes a ByteIOContext for accessing the
> >   * resource indicated by url.
> >   * @note When the resource indicated by url has been opened in
> > diff --git a/libavformat/aviobuf.c b/libavformat/aviobuf.c
> > index 37a6f6d..dd266ad 100644
> > --- a/libavformat/aviobuf.c
> > +++ b/libavformat/aviobuf.c
> > @@ -611,6 +611,42 @@ static int url_resetbuf(ByteIOContext *s, int flags)
> >      return 0;
> >  }
> >  
> > +int url_rewind_with_probe_data(ByteIOContext *s, unsigned char *buf, int buf_size)
> > +{
> > +    int64_t buffer_start;
> > +    int buffer_size;
> > +    int overlap, new_size;
> > +
> > +    if (s->write_flag)
> > +        return AVERROR(EINVAL);
> > +
> > +    buffer_size = s->buf_end - s->buffer;
> > +
> > +    /* the buffers must touch or overlap */
> > +    if ((buffer_start = s->pos - buffer_size) > buf_size)
> > +        return AVERROR(EINVAL);
> > +
> 
> > +    overlap = (int)(buf_size - buffer_start);
> 
> the cast seems unneeded
> 

Fixed. I was trying to avoid compiler warnings, but that operation
doesn't cause any.

> 
> > +    new_size = buf_size + buffer_size - overlap;
> > +
> > +    if (new_size > buf_size) {
> > +        if (!(buf = av_realloc(buf, new_size)))
> > +            return AVERROR(ENOMEM);
> > +
> > +        memcpy(buf + buf_size, s->buffer + overlap, buffer_size - overlap);
> > +        buf_size = new_size;
> > +    }
> > +
> > +    av_free(s->buffer);
> > +    s->buf_ptr = s->buffer = buf;
> > +    s->pos = s->buffer_size = buf_size;
> > +    s->buf_end = s->buf_ptr + buf_size;
> > +    s->eof_reached = 0;
> > +    s->must_flush = 0;
> 
> this code seems to leave the ByteIOContext buffer at the reallocated size
> this could be annoying if probing ends up needing something like 5mb

That's by design - since this method doesn't seek it needs to keep the
probe buffer and whatever extra data (less than 32 KiB) is left in the
ByteIOContext after the overlap.

I think what you're getting at is that it should only remain that size
as long as the demuxer is using the probe data - once exhausted the size
of the buffer can be decreased.

Assuming memory usage is a problem I did some quick calculations
regarding peak memory usage:
 * ff_probe_input_buffer() uses at most 1.5 MiB - during its
av_realloc() call in the last iteration
 * ff_rewind_with_probe_data() will similarly use at most 2 MiB (+ 2 x
IO_BUFFER_SIZE = 64 KiB) durings its call to av_realloc(), but will
eventually end up using no more than PROBE_BUF_MAX + IO_BUFFER_SIZE or
just over 1 MiB

The 2 MiB peak usage in ff_rewind_with_probe_data() could be avoided if
IO_BUFFER_SIZE extra space is reserved in ff_probe_input_buffer(), in
addition to AVPROBE_PADDING_SIZE. Letting the function know about this
extra space should eliminate the need for the av_realloc() call.

I suspect memory usage is not the main issue though - a large buffer
might reduce performance a bit due to worse cache coherency. In that
case fill_buffer() could be slightly modified to reallocate the buffer
if it's larger than IO_BUFFER_SIZE. I've attached an updated patch that
takes this approach. It also passes the tests.

/Tomas
-------------- next part --------------
A non-text attachment was scrubbed...
Name: nonseeking_probe_rewind_v2.patch
Type: text/x-patch
Size: 3955 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20100331/fa4ce083/attachment.bin>