[FFmpeg-devel] [PATCH 2/2] aviobuf: Avoid clearing the whole buffer in fill_buffer

[Martin Storsjö]

On Fri, 24 Mar 2023, Marton Balint wrote:

>
>
> On Fri, 24 Mar 2023, Martin Storsjö wrote:
>
>> On Fri, 24 Mar 2023, Marton Balint wrote:
>>
>>>  I am uneasy about complicating an already complicated and hard-to-follow
>>>  AVIO layer with heuristics which activate on magic behaviour. And we all
>>>  know how long temporary solutions last :)
>>>
>>>  I guess we could add some new parameter to AVIOContext end enable this
>>>  data-shifting behaviour explicitly when you reconfigure the buffer size
>>>  for index in the MOV demuxer. But is it worth it? How significant is the
>>>  "improvement" this patch provides over the previous one in the series?
>>
>> With the 2.6 GB, 40 minute mov file I'm looking at, originally, due to the 
>> issue fixed in patch 1/2, the buffer size was never increased from the 
>> original 32 KB, so when reading the file linearly, we would do many tens of 
>> thousands of seek requests, giving absolutely abysmal performance. (I saw a 
>> server side log number saying 120 000 requests.)
>>
>> With patch 1/2 applied, while reading the bulk of the file, it does ~170 
>> seeks. So nothing terrible, but it still feels unnecessarily inefficient to 
>> do >4 seeks per minute due to the fact that the aviobuf layer is throwing 
>> away good data that it already had buffered.
>>
>> In this case, it used a buffer size of 16 MB, and calculating 2.6 GB / 16 
> MB 
>> ends up very near 170. So every time the 16 MB aviobuf buffer gets full and 
>> aviobuf clears it, we end up doing a seek backwards.
>
> Thanks for the details. Patch/1 already made the significant improvement,

Yeah - can I get someone to approve that one too? :-)

> so yeah, I am not sure about Patch/2 knowing it is not the "right" way.

Yeah, I'm a bit on the fence myself. On one hand, it's been a pet peeve of 
mine for years, that we throw away the buffered data regularly like this, 
but the implementation maybe isn't the best. So for the practical 
performance issue it's probably not essential - it's just an annoying wart 
that is left :-)

// Martin