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

[Martin Storsjö]

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

// Martin