| Message ID | 20200915113525.30806-1-onemda@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel] avformat/aviobuf: realloc memory in ffio_ensure_seekback() | expand |
| Context | Check | Description |
|---|---|---|
| andriy/default | pending | |
| andriy/make | success | Make finished |
| andriy/make_fate | success | Make fate finished |
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;
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".
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.
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".
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
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
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 --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;
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(-)