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

[Marton Balint]

On Fri, 24 Mar 2023, Anton Khirnov wrote:

> Quoting Martin Storsjö (2023-03-24 12:25:37)
>> On Fri, 24 Mar 2023, Anton Khirnov wrote:
>> 
>> > Quoting Martin Storsjö (2023-03-21 21:24:25)
>> >> On Tue, 21 Mar 2023, Marton Balint wrote:
>> >> 
>> >> >
>> >> >
>> >> > On Tue, 21 Mar 2023, Martin Storsjö wrote:
>> >> >
>> >> >> Normally, fill_buffer reads in one max_packet_size/IO_BUFFER_SIZE
>> >> >> worth of data into the buffer, slowly filling the buffer until it
>> >> >> is full.
>> >> >>
>> >> >> Previously, when the buffer was full, fill_buffer would start over
>> >> >> from the start, effectively discarding all the previously buffered
>> >> >> data.
>> >> >>
>> >> >> For files that are read linearly, the previous behaviour was fine.
>> >> >>
>> >> >> For files that exhibit some amount of nonlinear read patterns,
>> >> >> especially mov files (where ff_configure_buffers_for_index
>> >> >> increases the buffer size to accomodate for the nonlinear reading!)
>> >> >> we would mostly be able to seek within the buffer - but whenever
>> >> >> we've hit the maximum buffer size, we'd discard most of the buffer
>> >> >> and start over with a very small buffer, so the next seek backwards
>> >> >> would end up outside of the buffer.
>> >> >>
>> >> >> Keep one fourth of the buffered data, moving it to the start of
>> >> >> the buffer, freeing the rest to be refilled with future data.
>> >> >>
>> >> >> For mov files with nonlinear read patterns, this almost entirely
>> >> >> avoids doing seeks on the lower IO level, where we previously would
>> >> >> end up doing seeks occasionally.
>> >> >
>> >> > Maybe the demuxer should use ffio_ensure_seekback() instead if it knows
>> >> > that a seekback will happen? Unconditional memmove of even fourth of all 
>> >> > data does not seem like a good idea.
>> >> 
>> >> Right, it's probably not ideal to do this unconditionally.
>> >> 
>> >> However, it's not that the demuxer really knows that a seekback _will_ 
>> >> happen - unless we make it inspect the next couple index entries. And I 
>> >> don't think we should make the demuxer pre-analyze the next access 
>> >> locations, but keep optimization like this on the separate layer. That 
>> >> way, it works as expected as long as the seeks are short enough within the 
>> >> expected tolerance, and falls back graciously on regular seeking for the 
>> >> accesses that are weirder than that.
>> >
>> > I suppose changing the buffer into a ring buffer so you don't have to
>> > move the data is not feasible?
>> 
>> Something like that would probably be ideal, yes - so we'd have a 
>> constantly sliding window of data available behind the current position.
>> 
>> I think that would be more work than I'm able to invest in the issue at 
>> the moment, though. (That doesn't mean I think everyone should suffer the 
>> overhead of this patch in this form, but I'm more interested in looking at 
>> heuristic based solutions for triggering this case rather than a full 
>> rewrite.)
>
> As a (hopefully) temporary heuristic, triggering this after observing a
> few backward seeks under buffer size sounds reasonable to me.

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?

Thanks,
Marton