[FFmpeg-devel] [RFC] Questions and suggestions regarding AVSEEK_FORCE?
Michael Niedermayer
michaelni
Thu Apr 1 00:11:22 CEST 2010
On Wed, Mar 31, 2010 at 06:38:11PM +0200, Tomas H?rdin wrote:
> 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.
exactly
>
> 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.
i was actually thinking of a sigle threaded player that stops playback
while filling a multi megabyte buffer from slow media
>
> /Tomas
> avio.h | 15 +++++++++++++++
> aviobuf.c | 45 +++++++++++++++++++++++++++++++++++++++++++++
> utils.c | 13 +++++--------
> 3 files changed, 65 insertions(+), 8 deletions(-)
> e71138cfe462f38d8d5c3cfbfc85a93499e8b47d nonseeking_probe_rewind_v2.patch
great work and looks ok
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
No human being will ever know the Truth, for even if they happen to say it
by chance, they would not even known they had done so. -- Xenophanes
-------------- 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/20100401/68c16756/attachment.pgp>
More information about the ffmpeg-devel
mailing list