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

[Michael Niedermayer]

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


> +
> +/**
>   * 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


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

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

Everything should be made as simple as possible, but not simpler.
-- Albert Einstein
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20100331/716df1de/attachment.pgp>