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

[Marton Balint]

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