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

[Marton Balint]

On Sun, 20 Sep 2020, Paul B Mahol wrote:

> 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 might not fix your issue, but it certainly improves the code.

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

Please point out where the flaw is.

Thanks,
Marton

>
>> 
>> > 
>> > 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.
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".