diff mbox series

[FFmpeg-devel] avformat/aviobuf: realloc memory in ffio_ensure_seekback()

Message ID 20200915113525.30806-1-onemda@gmail.com
State New
Headers show
Series [FFmpeg-devel] avformat/aviobuf: realloc memory 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. 15, 2020, 11:35 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 | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

Comments

Nicolas George Sept. 15, 2020, 11:39 a.m. UTC | #1
Paul B Mahol (12020-09-15):
> This removes big CPU overhead for demuxing chained ogg streams.
> 
> Signed-off-by: Paul B Mahol <onemda@gmail.com>
> ---
>  libavformat/aviobuf.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/libavformat/aviobuf.c b/libavformat/aviobuf.c
> index a77517d712..6d01150f66 100644
> --- a/libavformat/aviobuf.c
> +++ b/libavformat/aviobuf.c
> @@ -1005,12 +1005,11 @@ int ffio_ensure_seekback(AVIOContext *s, int64_t buf_size)
>          return 0;
>      av_assert0(!s->write_flag);
>  
> -    buffer = av_malloc(buf_size);
> +    buffer = s->buffer;

> +    buffer = av_realloc(buffer, buf_size);
>      if (!buffer)
>          return AVERROR(ENOMEM);

Leaks in case of failure.

>  
> -    memcpy(buffer, s->buffer, filled);
> -    av_free(s->buffer);
>      s->buf_ptr = buffer + (s->buf_ptr - s->buffer);
>      s->buf_end = buffer + (s->buf_end - s->buffer);
>      s->buffer = buffer;
Paul B Mahol Sept. 15, 2020, 11:56 a.m. UTC | #2
On Tue, Sep 15, 2020 at 01:39:45PM +0200, Nicolas George wrote:
> Paul B Mahol (12020-09-15):
> > This removes big CPU overhead for demuxing chained ogg streams.
> > 
> > Signed-off-by: Paul B Mahol <onemda@gmail.com>
> > ---
> >  libavformat/aviobuf.c | 5 ++---
> >  1 file changed, 2 insertions(+), 3 deletions(-)
> > 
> > diff --git a/libavformat/aviobuf.c b/libavformat/aviobuf.c
> > index a77517d712..6d01150f66 100644
> > --- a/libavformat/aviobuf.c
> > +++ b/libavformat/aviobuf.c
> > @@ -1005,12 +1005,11 @@ int ffio_ensure_seekback(AVIOContext *s, int64_t buf_size)
> >          return 0;
> >      av_assert0(!s->write_flag);
> >  
> > -    buffer = av_malloc(buf_size);
> > +    buffer = s->buffer;
> 
> > +    buffer = av_realloc(buffer, buf_size);
> >      if (!buffer)
> >          return AVERROR(ENOMEM);
> 
> Leaks in case of failure.

It leaks nothing in case of failure.
In case of failure old memory is kept and
seeking back will fail and thus give errors when syncing.

> 
> >  
> > -    memcpy(buffer, s->buffer, filled);
> > -    av_free(s->buffer);
> >      s->buf_ptr = buffer + (s->buf_ptr - s->buffer);
> >      s->buf_end = buffer + (s->buf_end - s->buffer);
> >      s->buffer = buffer;
> 
> -- 
>   Nicolas George



> _______________________________________________
> 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".
Nicolas George Sept. 15, 2020, 11:57 a.m. UTC | #3
Paul B Mahol (12020-09-15):
> It leaks nothing in case of failure.
> In case of failure old memory is kept and
> seeking back will fail and thus give errors when syncing.

My bad, it was a common pattern, but avoided here.
Marton Balint Sept. 15, 2020, 8:18 p.m. UTC | #4
On Tue, 15 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 | 5 ++---
> 1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/libavformat/aviobuf.c b/libavformat/aviobuf.c
> index a77517d712..6d01150f66 100644
> --- a/libavformat/aviobuf.c
> +++ b/libavformat/aviobuf.c
> @@ -1005,12 +1005,11 @@ int ffio_ensure_seekback(AVIOContext *s, int64_t buf_size)
>         return 0;
>     av_assert0(!s->write_flag);
> 
> -    buffer = av_malloc(buf_size);
> +    buffer = s->buffer;
> +    buffer = av_realloc(buffer, buf_size);

It is not guaranteed that buffer is allocated with av_realloc, so you 
cannot realloc it.

Regards,
Marton

>     if (!buffer)
>         return AVERROR(ENOMEM);
> 
> -    memcpy(buffer, s->buffer, filled);
> -    av_free(s->buffer);
>     s->buf_ptr = buffer + (s->buf_ptr - s->buffer);
>     s->buf_end = buffer + (s->buf_end - s->buffer);
>     s->buffer = 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".
Andreas Rheinhardt Sept. 15, 2020, 8:22 p.m. UTC | #5
Marton Balint:
> 
> 
> On Tue, 15 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 | 5 ++---
>> 1 file changed, 2 insertions(+), 3 deletions(-)
>>
>> diff --git a/libavformat/aviobuf.c b/libavformat/aviobuf.c
>> index a77517d712..6d01150f66 100644
>> --- a/libavformat/aviobuf.c
>> +++ b/libavformat/aviobuf.c
>> @@ -1005,12 +1005,11 @@ int ffio_ensure_seekback(AVIOContext *s,
>> int64_t buf_size)
>>         return 0;
>>     av_assert0(!s->write_flag);
>>
>> -    buffer = av_malloc(buf_size);
>> +    buffer = s->buffer;
>> +    buffer = av_realloc(buffer, buf_size);
> 
> It is not guaranteed that buffer is allocated with av_realloc, so you
> cannot realloc it.
> 

This isn't true since 21f70940ae106bfffa07e73057cdb4b5e81a767a any more.

- Andreas
Marton Balint Sept. 15, 2020, 8:40 p.m. UTC | #6
On Tue, 15 Sep 2020, Andreas Rheinhardt wrote:

> Marton Balint:
>> 
>> 
>> On Tue, 15 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 | 5 ++---
>>> 1 file changed, 2 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/libavformat/aviobuf.c b/libavformat/aviobuf.c
>>> index a77517d712..6d01150f66 100644
>>> --- a/libavformat/aviobuf.c
>>> +++ b/libavformat/aviobuf.c
>>> @@ -1005,12 +1005,11 @@ int ffio_ensure_seekback(AVIOContext *s,
>>> int64_t buf_size)
>>>         return 0;
>>>     av_assert0(!s->write_flag);
>>>
>>> -    buffer = av_malloc(buf_size);
>>> +    buffer = s->buffer;
>>> +    buffer = av_realloc(buffer, buf_size);
>> 
>> It is not guaranteed that buffer is allocated with av_realloc, so you
>> cannot realloc it.
>> 
>
> This isn't true since 21f70940ae106bfffa07e73057cdb4b5e81a767a any more.

Then av_realloc() docs need updating as well.

Regards,
Marton
Michael Niedermayer Sept. 16, 2020, 6:02 p.m. UTC | #7
On Tue, Sep 15, 2020 at 01:35:25PM +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 | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/libavformat/aviobuf.c b/libavformat/aviobuf.c
> index a77517d712..6d01150f66 100644
> --- a/libavformat/aviobuf.c
> +++ b/libavformat/aviobuf.c
> @@ -1005,12 +1005,11 @@ int ffio_ensure_seekback(AVIOContext *s, int64_t buf_size)
>          return 0;
>      av_assert0(!s->write_flag);
>  
> -    buffer = av_malloc(buf_size);
> +    buffer = s->buffer;
> +    buffer = av_realloc(buffer, buf_size);
>      if (!buffer)
>          return AVERROR(ENOMEM);

This would reduce the guranteed alignment.
If that is not an issue then this patch could be ok

thx

[...]
diff mbox series

Patch

diff --git a/libavformat/aviobuf.c b/libavformat/aviobuf.c
index a77517d712..6d01150f66 100644
--- a/libavformat/aviobuf.c
+++ b/libavformat/aviobuf.c
@@ -1005,12 +1005,11 @@  int ffio_ensure_seekback(AVIOContext *s, int64_t buf_size)
         return 0;
     av_assert0(!s->write_flag);
 
-    buffer = av_malloc(buf_size);
+    buffer = s->buffer;
+    buffer = av_realloc(buffer, buf_size);
     if (!buffer)
         return AVERROR(ENOMEM);
 
-    memcpy(buffer, s->buffer, filled);
-    av_free(s->buffer);
     s->buf_ptr = buffer + (s->buf_ptr - s->buffer);
     s->buf_end = buffer + (s->buf_end - s->buffer);
     s->buffer = buffer;