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

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

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.

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

Regards,
Marton