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

[Anton Khirnov]

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.

-- 
Anton Khirnov