diff mbox series

[FFmpeg-devel] avformat/aviobuf: fix broken logic in ffio_ensure_seekback()

Message ID 20200917103106.29078-1-onemda@gmail.com
State New
Headers show
Series [FFmpeg-devel] avformat/aviobuf: fix broken logic in ffio_ensure_seekback()
Related show

Checks

Context Check Description
andriy/default pending
andriy/make success Make finished
andriy/make_fate success Make fate finished

Commit Message

Paul B Mahol Sept. 17, 2020, 10:31 a.m. UTC
This removes big CPU overhead for demuxing chained ogg streams.

Signed-off-by: Paul B Mahol <onemda@gmail.com>
---
 libavformat/aviobuf.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

Comments

Marton Balint Sept. 17, 2020, 7:44 p.m. UTC | #1
On Thu, 17 Sep 2020, Paul B Mahol wrote:

> This removes big CPU overhead for demuxing chained ogg streams.
>
> Signed-off-by: Paul B Mahol <onemda@gmail.com>
> ---
> libavformat/aviobuf.c | 10 +++++-----
> 1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/libavformat/aviobuf.c b/libavformat/aviobuf.c
> index a77517d712..ce9b7d59c9 100644
> --- a/libavformat/aviobuf.c
> +++ b/libavformat/aviobuf.c
> @@ -996,20 +996,20 @@ int ffio_ensure_seekback(AVIOContext *s, int64_t buf_size)
>     uint8_t *buffer;
>     int max_buffer_size = s->max_packet_size ?
>                           s->max_packet_size : IO_BUFFER_SIZE;
> -    int filled = s->buf_end - s->buffer;
>     ptrdiff_t checksum_ptr_offset = s->checksum_ptr ? s->checksum_ptr - s->buffer : -1;
> 
> -    buf_size += s->buf_ptr - s->buffer + max_buffer_size;
> -
> -    if (buf_size < filled || s->seekable || !s->read_packet)
> +    if (buf_size <= s->buffer_size || s->seekable || !s->read_packet)
>         return 0;

This is still not correct. You can only return if
buf_size <= s->buf_end - s->buf_ptr
OR
buf_size <= s->buffer + s->buffer_size - s->buf_ptr - max_buffer_size + 1

And the new minimum buffer size is what is calculated now, so that should 
not be changed.

I am not sure if this fixes you original issue. It might make sense to 
allocate max_buffer_size*2 length buffers by default, but the buffer size 
revert logic must be fixed to support that...

Regards,
Marton



> +    buf_size += s->buffer_size;
> +    buf_size = FFMAX(buf_size, max_buffer_size);
> +
>     av_assert0(!s->write_flag);
>
>     buffer = av_malloc(buf_size);
>     if (!buffer)
>         return AVERROR(ENOMEM);
> 
> -    memcpy(buffer, s->buffer, filled);
> +    memcpy(buffer, s->buffer, s->buffer_size);
>     av_free(s->buffer);
>     s->buf_ptr = buffer + (s->buf_ptr - s->buffer);
>     s->buf_end = buffer + (s->buf_end - s->buffer);
> -- 
> 2.17.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".
Paul B Mahol Sept. 17, 2020, 9:32 p.m. UTC | #2
On Thu, Sep 17, 2020 at 09:44:52PM +0200, Marton Balint wrote:
> 
> 
> On Thu, 17 Sep 2020, Paul B Mahol wrote:
> 
> > This removes big CPU overhead for demuxing chained ogg streams.
> > 
> > Signed-off-by: Paul B Mahol <onemda@gmail.com>
> > ---
> > libavformat/aviobuf.c | 10 +++++-----
> > 1 file changed, 5 insertions(+), 5 deletions(-)
> > 
> > diff --git a/libavformat/aviobuf.c b/libavformat/aviobuf.c
> > index a77517d712..ce9b7d59c9 100644
> > --- a/libavformat/aviobuf.c
> > +++ b/libavformat/aviobuf.c
> > @@ -996,20 +996,20 @@ int ffio_ensure_seekback(AVIOContext *s, int64_t buf_size)
> >     uint8_t *buffer;
> >     int max_buffer_size = s->max_packet_size ?
> >                           s->max_packet_size : IO_BUFFER_SIZE;
> > -    int filled = s->buf_end - s->buffer;
> >     ptrdiff_t checksum_ptr_offset = s->checksum_ptr ? s->checksum_ptr - s->buffer : -1;
> > 
> > -    buf_size += s->buf_ptr - s->buffer + max_buffer_size;
> > -
> > -    if (buf_size < filled || s->seekable || !s->read_packet)
> > +    if (buf_size <= s->buffer_size || s->seekable || !s->read_packet)
> >         return 0;
> 
> This is still not correct. You can only return if
> buf_size <= s->buf_end - s->buf_ptr

That one gives you original issue.

> OR
> buf_size <= s->buffer + s->buffer_size - s->buf_ptr - max_buffer_size + 1
> 

That one gives you also original issue but at slower peace.

> And the new minimum buffer size is what is calculated now, so that should
> not be changed.
> 
> I am not sure if this fixes you original issue. It might make sense to
> allocate max_buffer_size*2 length buffers by default, but the buffer size
> revert logic must be fixed to support that...
> 
> Regards,
> Marton
> 
> 
> 
> > +    buf_size += s->buffer_size;
> > +    buf_size = FFMAX(buf_size, max_buffer_size);
> > +
> >     av_assert0(!s->write_flag);
> > 
> >     buffer = av_malloc(buf_size);
> >     if (!buffer)
> >         return AVERROR(ENOMEM);
> > 
> > -    memcpy(buffer, s->buffer, filled);
> > +    memcpy(buffer, s->buffer, s->buffer_size);
> >     av_free(s->buffer);
> >     s->buf_ptr = buffer + (s->buf_ptr - s->buffer);
> >     s->buf_end = buffer + (s->buf_end - s->buffer);
> > -- 
> > 2.17.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".
Paul B Mahol Sept. 19, 2020, 7:15 p.m. UTC | #3
On Thu, Sep 17, 2020 at 12:31:06PM +0200, Paul B Mahol wrote:
> This removes big CPU overhead for demuxing chained ogg streams.
> 
> Signed-off-by: Paul B Mahol <onemda@gmail.com>
> ---
>  libavformat/aviobuf.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 

will apply soon.
Marton Balint Sept. 19, 2020, 9:46 p.m. UTC | #4
On Sat, 19 Sep 2020, Paul B Mahol wrote:

> On Thu, Sep 17, 2020 at 12:31:06PM +0200, Paul B Mahol wrote:
>> This removes big CPU overhead for demuxing chained ogg streams.
>> 
>> Signed-off-by: Paul B Mahol <onemda@gmail.com>
>> ---
>>  libavformat/aviobuf.c | 10 +++++-----
>>  1 file changed, 5 insertions(+), 5 deletions(-)
>> 
>
> will apply soon.

Don't apply it, as I explained, it is not correct.

Regards,
Marton
Paul B Mahol Sept. 19, 2020, 11:27 p.m. UTC | #5
On Sat, Sep 19, 2020 at 11:46:50PM +0200, Marton Balint wrote:
> 
> 
> On Sat, 19 Sep 2020, Paul B Mahol wrote:
> 
> > On Thu, Sep 17, 2020 at 12:31:06PM +0200, Paul B Mahol wrote:
> > > This removes big CPU overhead for demuxing chained ogg streams.
> > > 
> > > Signed-off-by: Paul B Mahol <onemda@gmail.com>
> > > ---
> > >  libavformat/aviobuf.c | 10 +++++-----
> > >  1 file changed, 5 insertions(+), 5 deletions(-)
> > > 
> > 
> > will apply soon.
> 
> Don't apply it, as I explained, it is not correct.

I carefully examined your explanation and I already reported
here on mailing list for every one to see that it does not work.

So please reconsider your blocking statement and provide
correct solution to problem.

I can even upload relevant sample somewhere privately
for you so you can freely produce correct solution
and compare it with my solution for everyones benefit.
Marton Balint Sept. 20, 2020, 8:49 a.m. UTC | #6
On Sun, 20 Sep 2020, Paul B Mahol wrote:

> On Sat, Sep 19, 2020 at 11:46:50PM +0200, Marton Balint wrote:
>> 
>> 
>> On Sat, 19 Sep 2020, Paul B Mahol wrote:
>> 
>> > On Thu, Sep 17, 2020 at 12:31:06PM +0200, Paul B Mahol wrote:
>> > > This removes big CPU overhead for demuxing chained ogg streams.
>> > > 
>> > > Signed-off-by: Paul B Mahol <onemda@gmail.com>
>> > > ---
>> > >  libavformat/aviobuf.c | 10 +++++-----
>> > >  1 file changed, 5 insertions(+), 5 deletions(-)
>> > > 
>> > 
>> > will apply soon.
>> 
>> Don't apply it, as I explained, it is not correct.
>
> I carefully examined your explanation and I already reported
> here on mailing list for every one to see that it does not work.
>
> So please reconsider your blocking statement and provide
> correct solution to problem.

Your solution will break the fundamentals of ffio_ensure_seekback, so it 
is not OK. Checking if (buf_size <= s->buffer_size) will not guarantee 
that the buffer will not be flushed when reading the next buf_size bytes 
because buf_ptr can be anywhere in the buffer. See how fill_buffer() 
works. It not only makes sure the buffer has space, it makes sure that the 
buffer has max_buffer_size space available. If not, then it gets flushed.

There are some issues with current code, I will send a patch to fix them. 
Unfortunately it will NOT fix your problem entirely. As I suggested, if 
you use 2*max_buffer_size buffers by default, that might help you, but the 
fundamental problem is that ffio_ensure_seekback simply cannot be 
implemented efficiently with the current design. The guarantees should be 
reduced (e.g. ffio_ensure_seekback can flush the buffer and invalidate 
previous ffio_ensure_seekback request)

Regards,
Marton
Michael Niedermayer Sept. 26, 2020, 10:37 a.m. UTC | #7
On Sun, Sep 20, 2020 at 10:49:12AM +0200, Marton Balint wrote:
[...]

> fundamental problem is that ffio_ensure_seekback simply cannot be
> implemented efficiently with the current design. The guarantees should be
> reduced (e.g. ffio_ensure_seekback can flush the buffer and invalidate
> previous ffio_ensure_seekback request)

this probably makes sense, would this cause a problem to any code using it ?

thx

[...]
Michael Niedermayer Sept. 26, 2020, 10:50 a.m. UTC | #8
On Thu, Sep 17, 2020 at 12:31:06PM +0200, Paul B Mahol wrote:
> This removes big CPU overhead for demuxing chained ogg streams.
> 
> Signed-off-by: Paul B Mahol <onemda@gmail.com>
> ---
>  libavformat/aviobuf.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)

I think we need some fate test for ffio_ensure_seekback()
as our tests are ATM we generally have fully seekable inputs so
failure of seekback would not be noticed.

This is a problem because the buffer IO stuff is not easy to
understand and so it is time consuming and error prone to review these patches
just by eye. And as we see developers disagree on what is correct

Such fate test should at least test various sequentially occuring
ffio_ensure_seekback(), a possibly larger initial buffer from probing
as well as its later reduction. And also reading requests failing, early EOF
seeking, and IO errors

thanks

[...]
Paul B Mahol Sept. 26, 2020, 11:11 a.m. UTC | #9
On Sat, Sep 26, 2020 at 12:50:34PM +0200, Michael Niedermayer wrote:
> On Thu, Sep 17, 2020 at 12:31:06PM +0200, Paul B Mahol wrote:
> > This removes big CPU overhead for demuxing chained ogg streams.
> > 
> > Signed-off-by: Paul B Mahol <onemda@gmail.com>
> > ---
> >  libavformat/aviobuf.c | 10 +++++-----
> >  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> I think we need some fate test for ffio_ensure_seekback()
> as our tests are ATM we generally have fully seekable inputs so
> failure of seekback would not be noticed.
> 
> This is a problem because the buffer IO stuff is not easy to
> understand and so it is time consuming and error prone to review these patches
> just by eye. And as we see developers disagree on what is correct
> 
> Such fate test should at least test various sequentially occuring
> ffio_ensure_seekback(), a possibly larger initial buffer from probing
> as well as its later reduction. And also reading requests failing, early EOF
> seeking, and IO errors

Well, if its documented that ffio_ensure_seekback() can not flush buffer
than there should be added code/function to do allow flushing.

Because oggdec works fine with assumed flushing and my patch.

> 
> thanks
> 
> [...]
> 
> -- 
> Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
> 
> The day soldiers stop bringing you their problems is the day you have stopped 
> leading them. They have either lost confidence that you can help or concluded 
> you do not care. Either case is a failure of leadership. - Colin Powell



> _______________________________________________
> 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".
diff mbox series

Patch

diff --git a/libavformat/aviobuf.c b/libavformat/aviobuf.c
index a77517d712..ce9b7d59c9 100644
--- a/libavformat/aviobuf.c
+++ b/libavformat/aviobuf.c
@@ -996,20 +996,20 @@  int ffio_ensure_seekback(AVIOContext *s, int64_t buf_size)
     uint8_t *buffer;
     int max_buffer_size = s->max_packet_size ?
                           s->max_packet_size : IO_BUFFER_SIZE;
-    int filled = s->buf_end - s->buffer;
     ptrdiff_t checksum_ptr_offset = s->checksum_ptr ? s->checksum_ptr - s->buffer : -1;
 
-    buf_size += s->buf_ptr - s->buffer + max_buffer_size;
-
-    if (buf_size < filled || s->seekable || !s->read_packet)
+    if (buf_size <= s->buffer_size || s->seekable || !s->read_packet)
         return 0;
+    buf_size += s->buffer_size;
+    buf_size = FFMAX(buf_size, max_buffer_size);
+
     av_assert0(!s->write_flag);
 
     buffer = av_malloc(buf_size);
     if (!buffer)
         return AVERROR(ENOMEM);
 
-    memcpy(buffer, s->buffer, filled);
+    memcpy(buffer, s->buffer, s->buffer_size);
     av_free(s->buffer);
     s->buf_ptr = buffer + (s->buf_ptr - s->buffer);
     s->buf_end = buffer + (s->buf_end - s->buffer);