[FFmpeg-devel,1/2] libavformat: Improve ff_configure_buffers_for_index for excessive deltas

Previously, the ff_configure_buffers_for_index function had
upper sanity limits of 16 MB (1<<24) for buffer_size and
8 MB (1<<23) for short_seek_threshold.

However, if the index analysis showed a need for even larger
buffer sizes (for a really badly interleaved file), over the
sanity limits, we previously didn't increase the buffer sizes
at all.

Instead, if the file shows a need for really large buffers and
short_seek_threshold, just set them to the maximum sanity limit;
while it might not be enough for all cases in the file, it might
be enough for most of it.

This can happen e.g. with a mov file with some tracks containing
some samples that belong in the start of the file, at the end of
the mdat, while the rest of the file is mostly reasonably interleaved;
previously those samples caused the maximum pos_delta to skyrocket,
skipping any buffer size enlargement.

Signed-off-by: Martin Storsjö <martin@martin.st>
---
 libavformat/seek.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

On Tue, 21 Mar 2023, Martin Storsjö wrote:

> Previously, the ff_configure_buffers_for_index function had
> upper sanity limits of 16 MB (1<<24) for buffer_size and
> 8 MB (1<<23) for short_seek_threshold.
>
> However, if the index analysis showed a need for even larger
> buffer sizes (for a really badly interleaved file), over the
> sanity limits, we previously didn't increase the buffer sizes
> at all.
>
> Instead, if the file shows a need for really large buffers and
> short_seek_threshold, just set them to the maximum sanity limit;
> while it might not be enough for all cases in the file, it might
> be enough for most of it.
>
> This can happen e.g. with a mov file with some tracks containing
> some samples that belong in the start of the file, at the end of
> the mdat, while the rest of the file is mostly reasonably interleaved;
> previously those samples caused the maximum pos_delta to skyrocket,
> skipping any buffer size enlargement.
>
> Signed-off-by: Martin Storsjö <martin@martin.st>
> ---
> libavformat/seek.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/libavformat/seek.c b/libavformat/seek.c
> index a236e285c0..818549dfef 100644
> --- a/libavformat/seek.c
> +++ b/libavformat/seek.c
> @@ -220,7 +220,8 @@ void ff_configure_buffers_for_index(AVFormatContext *s, int64_t time_tolerance)
>     pos_delta *= 2;
>     ctx = ffiocontext(s->pb);
>     /* XXX This could be adjusted depending on protocol*/
> -    if (s->pb->buffer_size < pos_delta && pos_delta < (1<<24)) {
> +    pos_delta = FFMIN(pos_delta, 1<<24);
> +    if (s->pb->buffer_size < pos_delta) {
>         av_log(s, AV_LOG_VERBOSE, "Reconfiguring buffers to size %"PRId64"\n", pos_delta);
>
>         /* realloc the buffer and the original data will be retained */
> @@ -232,9 +233,8 @@ void ff_configure_buffers_for_index(AVFormatContext *s, int64_t time_tolerance)
>         ctx->short_seek_threshold = FFMAX(ctx->short_seek_threshold, pos_delta/2);
>     }
> 
> -    if (skip < (1<<23)) {
> -        ctx->short_seek_threshold = FFMAX(ctx->short_seek_threshold, skip);
> -    }
> +    skip = FFMIN(skip, 1<<23);
> +    ctx->short_seek_threshold = FFMAX(ctx->short_seek_threshold, skip);
> }
> 
> int av_index_search_timestamp(AVStream *st, int64_t wanted_timestamp, int flags)
> -- 
> 2.37.1 (Apple Git-137.1)

LGTM, thanks.

Marton

On Tue, Mar 21, 2023 at 02:37:28PM +0200, Martin Storsjö wrote:
> Previously, the ff_configure_buffers_for_index function had
> upper sanity limits of 16 MB (1<<24) for buffer_size and
> 8 MB (1<<23) for short_seek_threshold.
> 
> However, if the index analysis showed a need for even larger
> buffer sizes (for a really badly interleaved file), over the
> sanity limits, we previously didn't increase the buffer sizes
> at all.
> 
> Instead, if the file shows a need for really large buffers and
> short_seek_threshold, just set them to the maximum sanity limit;
> while it might not be enough for all cases in the file, it might
> be enough for most of it.
> 
> This can happen e.g. with a mov file with some tracks containing
> some samples that belong in the start of the file, at the end of
> the mdat, while the rest of the file is mostly reasonably interleaved;
> previously those samples caused the maximum pos_delta to skyrocket,
> skipping any buffer size enlargement.
> 
> Signed-off-by: Martin Storsjö <martin@martin.st>
> ---
>  libavformat/seek.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)

I think, if iam not too tired ATM that
instead of taking the maximum it would be better to ignore the cases
which are above the maximum, so the value would be optimal for the
cases that it can help

[...]

thx

On Sat, 25 Mar 2023, Michael Niedermayer wrote:

> On Tue, Mar 21, 2023 at 02:37:28PM +0200, Martin Storsjö wrote:
>> Previously, the ff_configure_buffers_for_index function had
>> upper sanity limits of 16 MB (1<<24) for buffer_size and
>> 8 MB (1<<23) for short_seek_threshold.
>>
>> However, if the index analysis showed a need for even larger
>> buffer sizes (for a really badly interleaved file), over the
>> sanity limits, we previously didn't increase the buffer sizes
>> at all.
>>
>> Instead, if the file shows a need for really large buffers and
>> short_seek_threshold, just set them to the maximum sanity limit;
>> while it might not be enough for all cases in the file, it might
>> be enough for most of it.
>>
>> This can happen e.g. with a mov file with some tracks containing
>> some samples that belong in the start of the file, at the end of
>> the mdat, while the rest of the file is mostly reasonably interleaved;
>> previously those samples caused the maximum pos_delta to skyrocket,
>> skipping any buffer size enlargement.
>>
>> Signed-off-by: Martin Storsjö <martin@martin.st>
>> ---
>>  libavformat/seek.c | 8 ++++----
>>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> I think, if iam not too tired ATM that
> instead of taking the maximum it would be better to ignore the cases
> which are above the maximum, so the value would be optimal for the
> cases that it can help

Sure, I guess that could be reasonable too - I can update the patch with 
that.

For the tricky file in my case, this algorithm currently observes mostly 
deltas around 2-32 MB here, but with a few outliers around 2 GB. So the 2 
GB outliers clearly are irrelevant regardless of whether we keep the limit 
at the current 16 MB or raise it a little bit.

This heuristic algorithm currently does observe some differences over 32 
MB - but even with the buffer size set to the maximum of 16 MB, it's quite 
enough to avoid most of the seeking.

One thing I noted is that this function is called with time_tolerance set 
to 1 second - but shouldn't it be quite enough to just have time_tolerance 
set to 0? I.e. check exactly the next sample in the other tracks (since 
that's what's going to be read in most cases), instead of checking a 
sample 1 second in the future in other tracks?

// Martin