[FFmpeg-devel] [PATCH] avio_read data loss with multiple calls to fill_buffer

[Michael Niedermayer]

hi

On Thu, May 18, 2017 at 10:58:39PM +0000, Rob Meyers wrote:
> avio_read does multiple calls to fill_buffer(). Adjusting things in
> fill_buffer was frowned upon; how would you suggest I approach this?

Its not frowned upon to fix a bug in fill_buffer(), its frowned
upon to make a random change that fixes a testcase without fixing
the underlaing bug. (there is a bug somewhere)


> The
> id3v2 seek doesn't come into play; the return from avio_read is the "full"
> read.

Just tried, if i comment
ff_id3v2_read_dict(s->pb, &s->internal->id3v2_meta, ID3v2_DEFAULT_MAGIC, &id3v2_extra_meta);
out from avformat_open_input()
your testcase works


> 
> The call I added in avio_read is only done when there was accumulated data,
> so avoid reallocating the buffer the first time through.

> 
> On Thu, May 18, 2017 at 3:53 PM Michael Niedermayer <michael at niedermayer.cc>
> wrote:
> 
> > On Thu, May 18, 2017 at 01:12:49PM -0700, Rob Meyers wrote:
> > > avio_read may make multiple calls to fill_buffer to accumulate bytes
> > > to fulfill a request. fill_buffer will overwrite previously read data
> > > if it is not prepped. Added a call to 'ffio_ensure_seekback' to adjust
> > > the s->buffer to fill_buffer's liking. This isn't necessary for the
> > > very first call to fill_buffer, thus the "size1 != size" check.
> > > ---
> > >  libavformat/aviobuf.c | 5 +++++
> > >  1 file changed, 5 insertions(+)
> > >
> > > diff --git a/libavformat/aviobuf.c b/libavformat/aviobuf.c
> > > index 0a7c39eacd..8e9594cab8 100644
> > > --- a/libavformat/aviobuf.c
> > > +++ b/libavformat/aviobuf.c
> > > @@ -648,6 +648,11 @@ int avio_read(AVIOContext *s, unsigned char *buf,
> > int size)
> > >                      s->buf_end = s->buffer/* + len*/;
> > >                  }
> > >              } else {
> > > +                if (size1 != size)
> > > +                    /* this call will ensure fill_buffer will not
> > overwrite previously
> > > +                    read data. */
> > > +                    ffio_ensure_seekback(s, size1);
> >
> > ffio_ensure_seekback() should be called when theres a potential
> > seek back in the following code.
> >
> > There is no seekback in avio_read(), avio_read only moves forward.
> > Some callers may do a avio_read() and or other function calls and
> > then seek back. A caller might do 3 avio_read() and seek back over all
> >
> > ffio_ensure_seekback() generally should be called by the code that
> > later initiates teh seek back
> >
> > ffio_ensure_seekback() is not free it should only be used when it is
> > needed.
> >
> >
> >
> > [...]
> > --
> > Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
> >
> > Those who are too smart to engage in politics are punished by being
> > governed by those who are dumber. -- Plato
> > _______________________________________________
> > ffmpeg-devel mailing list
> > ffmpeg-devel at ffmpeg.org
> > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> >
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Many things microsoft did are stupid, but not doing something just because
microsoft did it is even more stupid. If everything ms did were stupid they
would be bankrupt already.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20170519/92b22f20/attachment.sig>