[FFmpeg-devel] [PATCH 2/3] avformat/aviobuf: fix checks in ffio_ensure_seekback

[Paul B Mahol]

On Sun, Sep 20, 2020 at 03:16:15PM +0200, Marton Balint wrote:
> 
> 
> On Sun, 20 Sep 2020, Paul B Mahol wrote:
> 
> > On Sun, Sep 20, 2020 at 10:52:52AM +0200, Marton Balint wrote:
> > > Signed-off-by: Marton Balint <cus at passwd.hu>
> > > ---
> > >  libavformat/aviobuf.c | 7 +++++--
> > >  1 file changed, 5 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/libavformat/aviobuf.c b/libavformat/aviobuf.c
> > > index 9675425349..aa1d6c0830 100644
> > > --- a/libavformat/aviobuf.c
> > > +++ b/libavformat/aviobuf.c
> > > @@ -999,9 +999,12 @@ int ffio_ensure_seekback(AVIOContext *s, int64_t buf_size)
> > >      int filled = s->buf_end - s->buffer;
> > >      ptrdiff_t checksum_ptr_offset = s->checksum_ptr ? s->checksum_ptr - s->buffer : -1;
> > > 
> > > -    buf_size += s->buf_ptr - s->buffer + max_buffer_size;
> > > +    if (buf_size <= s->buf_end - s->buf_ptr)
> > > +        return 0;
> > > +
> > > +    buf_size += s->buf_ptr - s->buffer + max_buffer_size - 1;
> > > 
> > > -    if (buf_size < filled || s->seekable || !s->read_packet)
> > > +    if (buf_size <= s->buffer_size || s->seekable || !s->read_packet)
> > >          return 0;
> > >      av_assert0(!s->write_flag);
> > 
> > 
> > Not acceptable change.
> > 
> > Your code does this to chained ogg files:
> > 
> > XXX 10
> > XXX 65307
> > XXX 65307
> > 102031
> > 106287
> > 110527
> > 114745
> > 119319
> > [...]
> 
> This was also the case before the patch, no? So this alone is no reason
> to reject the patch.

Exactly the reson for patch rejection is that it does not improve code at all.

> 
> > 
> > It continues allocating excessive small extra chunks of bytes for no apparent reason in each and every call
> > which slowly and gradually increases memory usage, but every call causes unnecessary memcpy calls thus causing
> > almost exponential slowdown of file processing.
> 
> And when I say ffio_ensure_seekback() has a design issue, this is exactly
> what I mean. It is not that suprising, consider this:
> 
> I have 32k buffer, and I read at most 32k at once.
> I want seekback of 64k. Buffer got increased to 96k
> I read 64k.
> I want seekback of 64k. Buffer got increased to 160k.
> I read 64k.
> ... and so on.

My patch exactly does that and it proves it works.

> 
> a read call cannot flush the buffer because I am always whitin my requested
> boundary. ffio_ensure_seekback() cannot flush the buffer either, because it
> is not allowed to do that. Therefore I consume infinite memory.

This explanation is flawed.

> 
> > 
> > Lines with XXX, means that allocation and memcpy was not needed.
> 
> Are you sure about that? Feel free to give an example with buffer sizes and
> buffer positions, and prove that reallocation is uneeded. But please be
> aware how fill_buffer() works and make sure that when reading sequentially
> up to buf_size, seeking within the current pos and pos+buf_size is possible.

Seeking should be possible anywhere between buffer start and buffer end.
current buffer ptr is not important as it just points within buffer.