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

[Marton Balint <cus@passwd.hu>]

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.

Regards,
Marton

>
> Signed-off-by: Martin Storsjö <martin@martin.st>
> ---
> I'm open to suggestions on whether 1/4 of the buffer is a reasonable
> amount to keep. It does of course incur some amount of overhead
> for well behaved linear files, but is a decent improvement for
> nonlinear mov files.
>
> Alternatively we could trigger this behaviour only after we've
> observed a couple seeks backwards?
> ---
> libavformat/aviobuf.c | 46 +++++++++++++++++++++++++++++++++++++------
> 1 file changed, 40 insertions(+), 6 deletions(-)
>
> diff --git a/libavformat/aviobuf.c b/libavformat/aviobuf.c
> index 4ad734a3c3..dfc3e77016 100644
> --- a/libavformat/aviobuf.c
> +++ b/libavformat/aviobuf.c
> @@ -534,8 +534,7 @@ static void fill_buffer(AVIOContext *s)
>     FFIOContext *const ctx = (FFIOContext *)s;
>     int max_buffer_size = s->max_packet_size ?
>                           s->max_packet_size : IO_BUFFER_SIZE;
> -    uint8_t *dst        = s->buf_end - s->buffer + max_buffer_size <= s->buffer_size ?
> -                          s->buf_end : s->buffer;
> +    uint8_t *dst        = s->buf_end;
>     int len             = s->buffer_size - (dst - s->buffer);
>
>     /* can't fill the buffer without read_packet, just set EOF if appropriate */
> @@ -546,11 +545,46 @@ static void fill_buffer(AVIOContext *s)
>     if (s->eof_reached)
>         return;
> 
> -    if (s->update_checksum && dst == s->buffer) {
> -        if (s->buf_end > s->checksum_ptr)
> +    if (len < max_buffer_size && s->buffer_size > max_buffer_size) {
> +        /* If the buffer is almost full and we're not trying to read
> +           one whole buffer worth of data at once; keep some amount of
> +           the currently buffered data, but move it to the start of the
> +           buffer, to allow filling the buffer with more data. */
> +        int keep = (s->buf_end - s->buffer)/4;
> +        int shift = s->buf_end - keep - s->buffer;
> +
> +        if (s->update_checksum && s->checksum_ptr - s->buffer < shift) {
> +            /* Checksum up to the buffer + shift position (that we're
> +               shifting out of the buffer. */
>             s->checksum = s->update_checksum(s->checksum, s->checksum_ptr,
> -                                             s->buf_end - s->checksum_ptr);
> -        s->checksum_ptr = s->buffer;
> +                                             s->buffer + shift - s->checksum_ptr);
> +        }
> +
> +        memmove(s->buffer, s->buf_end - keep, keep);
> +        s->buf_end -= shift;
> +        s->buf_ptr -= shift;
> +        if (s->update_checksum) {
> +            if (s->checksum_ptr - s->buffer < shift)
> +                s->checksum_ptr = s->buffer;
> +            else
> +                s->checksum_ptr -= shift;
> +        }
> +
> +        dst = s->buf_end;
> +        len = s->buffer_size - (dst - s->buffer);
> +    } else if (len < max_buffer_size) {
> +        /* If the buffer is full so we can't fit a whole write of max_buffer_size,
> +           just restart the pointers from the start of the buffer. */
> +        dst = s->buffer;
> +        len = s->buffer_size;
> +
> +        if (s->update_checksum) {
> +            /* Checksum all data that gets shifted out of the buffer. */
> +            if (s->buf_end > s->checksum_ptr)
> +                s->checksum = s->update_checksum(s->checksum, s->checksum_ptr,
> +                                                 s->buf_end - s->checksum_ptr);
> +            s->checksum_ptr = s->buffer;
> +        }
>     }
>
>     /* make buffer smaller in case it ended up large after probing */
> -- 
> 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 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".