diff mbox series

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

Message ID 20200920085253.7107-2-cus@passwd.hu
State Accepted
Headers show
Series [FFmpeg-devel,1/3] avformat/aviobuf: read till the very end of the IO buffer
Related show

Checks

Context Check Description
andriy/default pending
andriy/make success Make finished
andriy/make_fate success Make fate finished

Commit Message

Marton Balint Sept. 20, 2020, 8:52 a.m. UTC
Signed-off-by: Marton Balint <cus@passwd.hu>
---
 libavformat/aviobuf.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

Comments

Paul B Mahol Sept. 20, 2020, 12:40 p.m. UTC | #1
On Sun, Sep 20, 2020 at 10:52:52AM +0200, Marton Balint wrote:
> Signed-off-by: Marton Balint <cus@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
[...]

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.

Lines with XXX, means that allocation and memcpy was not needed.

I can also post behavior of my solution, which work just fine, and I yet need to find scenario when it does not work.

>  
> -- 
> 2.26.2
> 
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
Marton Balint Sept. 20, 2020, 1:16 p.m. UTC | #2
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@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
Paul B Mahol Sept. 20, 2020, 1:44 p.m. UTC | #3
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@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.
Marton Balint Sept. 20, 2020, 2:09 p.m. UTC | #4
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@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@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
Marton Balint Sept. 26, 2020, 9:18 a.m. UTC | #5
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@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@ffmpeg.org
>> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>>
>> To unsubscribe, visit link above, or email
>> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
Michael Niedermayer Sept. 26, 2020, 10:14 a.m. UTC | #6
On Sun, Sep 20, 2020 at 10:52:52AM +0200, Marton Balint wrote:
> Signed-off-by: Marton Balint <cus@passwd.hu>
> ---
>  libavformat/aviobuf.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)

The commit message is too terse. It is not clear from it what the problem
is that is being fixed and how it is fixed.
Also this feels like it fixes multiple issues so it probably should be spit
unless iam missing some interdependancies

ill also take a look at the "competing" patch, so i dont yet know
how they relate ...


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

Did you check that this doesnt interfere with the s->buffer_size reduction
we do when it was larger from probing ?
Its maybe ok, just checking that this was considered

thx


[...]
Marton Balint Sept. 26, 2020, 10:46 a.m. UTC | #7
On Sat, 26 Sep 2020, Michael Niedermayer wrote:

> On Sun, Sep 20, 2020 at 10:52:52AM +0200, Marton Balint wrote:
>> Signed-off-by: Marton Balint <cus@passwd.hu>
>> ---
>>  libavformat/aviobuf.c | 7 +++++--
>>  1 file changed, 5 insertions(+), 2 deletions(-)
>
> The commit message is too terse. It is not clear from it what the problem
> is that is being fixed and how it is fixed.
> Also this feels like it fixes multiple issues so it probably should be spit
> unless iam missing some interdependancies

It can be seperated to two patches if that is preferred:

1) existing code does not check if the requested seekback buffer is 
already read entirely. In this case, nothing has to be done. This is the 
newly added if() at the beginning of the patch.

2) the new buf_size is detemined too conservatively, maybe because of the 
issue which was fixed in patch 1/3. So we can safely substract 1 more from 
the new buffer size. Comparing the new buf_size against filled does not 
make a lot of sense to me, that just seems wrong. What makes sense is that 
we want to reallocate the buffer if the new buf_size is bigger than the 
old, therefore the change in the check.

>
> ill also take a look at the "competing" patch, so i dont yet know
> how they relate ...
>
>
>>
>> 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;
>
> Did you check that this doesnt interfere with the s->buffer_size reduction
> we do when it was larger from probing ?
> Its maybe ok, just checking that this was considered

I don't see how that can be affected by this change, so I am not sure what 
you mean here. If the new buffer size is bigger than s->orig_buffer_size, 
then eventually it will be reduced, it does not seem more complicated than 
that to me.

Regards,
Marton
Michael Niedermayer Sept. 26, 2020, 4 p.m. UTC | #8
On Sat, Sep 26, 2020 at 12:46:40PM +0200, Marton Balint wrote:
> 
> 
> On Sat, 26 Sep 2020, Michael Niedermayer wrote:
> 
> > On Sun, Sep 20, 2020 at 10:52:52AM +0200, Marton Balint wrote:
> > > Signed-off-by: Marton Balint <cus@passwd.hu>
> > > ---
> > >  libavformat/aviobuf.c | 7 +++++--
> > >  1 file changed, 5 insertions(+), 2 deletions(-)
> > 
> > The commit message is too terse. It is not clear from it what the problem
> > is that is being fixed and how it is fixed.
> > Also this feels like it fixes multiple issues so it probably should be spit
> > unless iam missing some interdependancies
> 
> It can be seperated to two patches if that is preferred:
> 
> 1) existing code does not check if the requested seekback buffer is already
> read entirely. In this case, nothing has to be done. This is the newly added
> if() at the beginning of the patch.
> 
> 2) the new buf_size is detemined too conservatively, maybe because of the
> issue which was fixed in patch 1/3. So we can safely substract 1 more from
> the new buffer size. Comparing the new buf_size against filled does not make
> a lot of sense to me, that just seems wrong. What makes sense is that we
> want to reallocate the buffer if the new buf_size is bigger than the old,
> therefore the change in the check.

i would prefer it split yes


> 
> > 
> > ill also take a look at the "competing" patch, so i dont yet know
> > how they relate ...
> > 
> > 
> > > 
> > > 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;
> > 
> > Did you check that this doesnt interfere with the s->buffer_size reduction
> > we do when it was larger from probing ?
> > Its maybe ok, just checking that this was considered
> 
> I don't see how that can be affected by this change, so I am not sure what
> you mean here. If the new buffer size is bigger than s->orig_buffer_size,
> then eventually it will be reduced, it does not seem more complicated than
> that to me.

what i meant was that if its reduced then at that point the seekback 
gurantee changes relative to it never being reduced.
I think you consider this in your code, i just wanted to double check.

thx


[...]
Marton Balint Sept. 26, 2020, 5:15 p.m. UTC | #9
On Sat, 26 Sep 2020, Michael Niedermayer wrote:

> On Sat, Sep 26, 2020 at 12:46:40PM +0200, Marton Balint wrote:
>>
>>
>> On Sat, 26 Sep 2020, Michael Niedermayer wrote:
>>
>>> On Sun, Sep 20, 2020 at 10:52:52AM +0200, Marton Balint wrote:
>>>> Signed-off-by: Marton Balint <cus@passwd.hu>
>>>> ---
>>>>  libavformat/aviobuf.c | 7 +++++--
>>>>  1 file changed, 5 insertions(+), 2 deletions(-)
>>>
>>> The commit message is too terse. It is not clear from it what the problem
>>> is that is being fixed and how it is fixed.
>>> Also this feels like it fixes multiple issues so it probably should be spit
>>> unless iam missing some interdependancies
>>
>> It can be seperated to two patches if that is preferred:
>>
>> 1) existing code does not check if the requested seekback buffer is already
>> read entirely. In this case, nothing has to be done. This is the newly added
>> if() at the beginning of the patch.
>>
>> 2) the new buf_size is detemined too conservatively, maybe because of the
>> issue which was fixed in patch 1/3. So we can safely substract 1 more from
>> the new buffer size. Comparing the new buf_size against filled does not make
>> a lot of sense to me, that just seems wrong. What makes sense is that we
>> want to reallocate the buffer if the new buf_size is bigger than the old,
>> therefore the change in the check.
>
> i would prefer it split yes

Ok, will send new series.

>
>
>>
>>>
>>> ill also take a look at the "competing" patch, so i dont yet know
>>> how they relate ...
>>>
>>>
>>>>
>>>> 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;
>>>
>>> Did you check that this doesnt interfere with the s->buffer_size reduction
>>> we do when it was larger from probing ?
>>> Its maybe ok, just checking that this was considered
>>
>> I don't see how that can be affected by this change, so I am not sure what
>> you mean here. If the new buffer size is bigger than s->orig_buffer_size,
>> then eventually it will be reduced, it does not seem more complicated than
>> that to me.
>
> what i meant was that if its reduced then at that point the seekback
> gurantee changes relative to it never being reduced.
> I think you consider this in your code, i just wanted to double check.

I think that is already ensured by the check in fill_buffer before 
decreaseing the buffer:

         if (dst == s->buffer && s->buf_ptr != dst)

This checks that buffer size decrease can only happen when we are writing 
to the start of the buffer and buf_ptr > buffer, so we are flushing 
anyways.

Regards,
Marton
diff mbox series

Patch

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);