Message ID | 20230321123729.74124-1-martin@martin.st |
---|---|
State | New |
Headers | show |
Series | [FFmpeg-devel,1/2] libavformat: Improve ff_configure_buffers_for_index for excessive deltas | expand |
Context | Check | Description |
---|---|---|
andriy/make_x86 | success | Make finished |
andriy/make_fate_x86 | success | Make fate finished |
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
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)
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(-)