[FFmpeg-devel] [PATCHv3 1/2] libavformat: Account for negative position differences in ff_configure_buffers_for_index

[FFmpeg-devel] [PATCHv3 1/2] libavformat: Account for negative position differences in ff_configure_buffers_for_index

[Martin Storsjö]

When scanning through the index, account for the fact that the
compared samples may be located in an unexpected order in the file;
this function is mainly interested in the absolute difference between
file locations.

Signed-off-by: Martin Storsjö <martin@martin.st>
---
v3: Avoid mixed declarations and statements.

Reposting as part of the updated other patch, as it the previously posted
v3 didn't get any comments.
---
 libavformat/seek.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/libavformat/seek.c b/libavformat/seek.c
index a236e285c0..faa47f961f 100644
--- a/libavformat/seek.c
+++ b/libavformat/seek.c
@@ -208,9 +208,11 @@ void ff_configure_buffers_for_index(AVFormatContext *s, int64_t time_tolerance)
                 for (; i2 < sti2->nb_index_entries; i2++) {
                     const AVIndexEntry *const e2 = &sti2->index_entries[i2];
                     int64_t e2_pts = av_rescale_q(e2->timestamp, st2->time_base, AV_TIME_BASE_Q);
+                    int64_t cur_delta;
                     if (e2_pts < e1_pts || e2_pts - (uint64_t)e1_pts < time_tolerance)
                         continue;
-                    pos_delta = FFMAX(pos_delta, e1->pos - e2->pos);
+                    cur_delta = FFABS(e1->pos - e2->pos);
+                    pos_delta = FFMAX(pos_delta, cur_delta);
                     break;
                 }
             }
-- 
2.37.1 (Apple Git-137.1)

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

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

[Martin Storsjö]

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 contained entries with a much larger
delta, setting pos_delta to a value larger than the sanity
limit, we would end up not increasing the buffer size at all.

Instead, ignore the individual deltas that are excessive, but
increase the buffer size based on the deltas that are below the
sanity limit.

Only count deltas that are below 1<<23, 8 MB; pos_delta gets doubled
before setting the buffer size - this matches the previous maximum
buffer size of 1<<24, 16 MB.

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>
---
v2: Ignore entries that are out of range instead of clipping to
the maximum allowed.
---
 libavformat/seek.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/libavformat/seek.c b/libavformat/seek.c
index faa47f961f..386312cd3a 100644
--- a/libavformat/seek.c
+++ b/libavformat/seek.c
@@ -204,7 +204,9 @@ void ff_configure_buffers_for_index(AVFormatContext *s, int64_t time_tolerance)
                 const AVIndexEntry *const e1 = &sti1->index_entries[i1];
                 int64_t e1_pts = av_rescale_q(e1->timestamp, st1->time_base, AV_TIME_BASE_Q);
 
-                skip = FFMAX(skip, e1->size);
+                if (e1->size < (1 << 23))
+                    skip = FFMAX(skip, e1->size);
+
                 for (; i2 < sti2->nb_index_entries; i2++) {
                     const AVIndexEntry *const e2 = &sti2->index_entries[i2];
                     int64_t e2_pts = av_rescale_q(e2->timestamp, st2->time_base, AV_TIME_BASE_Q);
@@ -212,7 +214,8 @@ void ff_configure_buffers_for_index(AVFormatContext *s, int64_t time_tolerance)
                     if (e2_pts < e1_pts || e2_pts - (uint64_t)e1_pts < time_tolerance)
                         continue;
                     cur_delta = FFABS(e1->pos - e2->pos);
-                    pos_delta = FFMAX(pos_delta, cur_delta);
+                    if (cur_delta < (1 << 23))
+                        pos_delta = FFMAX(pos_delta, cur_delta);
                     break;
                 }
             }
@@ -222,7 +225,7 @@ 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)) {
+    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 */
@@ -234,9 +237,7 @@ 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);
-    }
+    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)

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCHv2 2/2] libavformat: Improve ff_configure_buffers_for_index for excessive deltas

[Martin Storsjö]

On Tue, 28 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 contained entries with a much larger
> delta, setting pos_delta to a value larger than the sanity
> limit, we would end up not increasing the buffer size at all.
>
> Instead, ignore the individual deltas that are excessive, but
> increase the buffer size based on the deltas that are below the
> sanity limit.
>
> Only count deltas that are below 1<<23, 8 MB; pos_delta gets doubled
> before setting the buffer size - this matches the previous maximum
> buffer size of 1<<24, 16 MB.
>
> 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>
> ---
> v2: Ignore entries that are out of range instead of clipping to
> the maximum allowed.
> ---
> libavformat/seek.c | 13 +++++++------
> 1 file changed, 7 insertions(+), 6 deletions(-)
>
> diff --git a/libavformat/seek.c b/libavformat/seek.c
> index faa47f961f..386312cd3a 100644
> --- a/libavformat/seek.c
> +++ b/libavformat/seek.c
> @@ -204,7 +204,9 @@ void ff_configure_buffers_for_index(AVFormatContext *s, int64_t time_tolerance)
>                 const AVIndexEntry *const e1 = &sti1->index_entries[i1];
>                 int64_t e1_pts = av_rescale_q(e1->timestamp, st1->time_base, AV_TIME_BASE_Q);
>
> -                skip = FFMAX(skip, e1->size);
> +                if (e1->size < (1 << 23))
> +                    skip = FFMAX(skip, e1->size);
> +
>                 for (; i2 < sti2->nb_index_entries; i2++) {
>                     const AVIndexEntry *const e2 = &sti2->index_entries[i2];
>                     int64_t e2_pts = av_rescale_q(e2->timestamp, st2->time_base, AV_TIME_BASE_Q);
> @@ -212,7 +214,8 @@ void ff_configure_buffers_for_index(AVFormatContext *s, int64_t time_tolerance)
>                     if (e2_pts < e1_pts || e2_pts - (uint64_t)e1_pts < time_tolerance)
>                         continue;
>                     cur_delta = FFABS(e1->pos - e2->pos);
> -                    pos_delta = FFMAX(pos_delta, cur_delta);
> +                    if (cur_delta < (1 << 23))
> +                        pos_delta = FFMAX(pos_delta, cur_delta);
>                     break;
>                 }
>             }
> @@ -222,7 +225,7 @@ 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)) {
> +    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 */
> @@ -234,9 +237,7 @@ 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);
> -    }
> +    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)

Will push these two patches today, if there's no further input on it; the 
previous iteration was already accepted by some, and this version includes 
the other feedback that I got.

// Martin
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".