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

[Marton Balint]

On Sun, 20 Sep 2020, Marton Balint wrote:

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

Ping for this as well.

Thanks,
Marton


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