diff mbox

[FFmpeg-devel] libavformat/mov: Always use av_realloc() for AVCodecParameters.extradata

Message ID CAG4UmKvsY1F-=Y_0X70HRsL8L0gntucHn7uhbUtyd5Cf8UBJkg@mail.gmail.com
State New
Headers show

Commit Message

John Rummell Aug. 25, 2017, 6:25 p.m. UTC 
Chromium uses tcmalloc which doesn't like mixing calls to posix_memalign()
and realloc(). This change updates mov.c to only use av_realloc() when
allocating memory for AVCodecParameters.extradata.
---
 libavformat/mov.c | 21 ++++++++++++---------
 1 file changed, 12 insertions(+), 9 deletions(-)

+                memset(st->codecpar->extradata, 0, size);
                 st->codecpar->extradata_size = ALAC_EXTRADATA_SIZE;
                 AV_WB32(st->codecpar->extradata    , ALAC_EXTRADATA_SIZE);
                 AV_WB32(st->codecpar->extradata + 4,
MKTAG('a','l','a','c'));
@@ -2054,6 +2055,8 @@ static int mov_rewrite_dvd_sub_extradata(AVStream *st)
     char buf[256] = {0};
     uint8_t *src = st->codecpar->extradata;
     int i;
+    uint64_t size;
+    int ret;

     if (st->codecpar->extradata_size != 64)
         return 0;
@@ -2075,9 +2078,9 @@ static int mov_rewrite_dvd_sub_extradata(AVStream *st)

     av_freep(&st->codecpar->extradata);
     st->codecpar->extradata_size = 0;
-    st->codecpar->extradata = av_mallocz(strlen(buf) +
AV_INPUT_BUFFER_PADDING_SIZE);
-    if (!st->codecpar->extradata)
-        return AVERROR(ENOMEM);
+    size = strlen(buf) + AV_INPUT_BUFFER_PADDING_SIZE;
+    if ((ret = av_reallocp(&st->codecpar->extradata, size)) < 0)
+        return ret;
     st->codecpar->extradata_size = strlen(buf);
     memcpy(st->codecpar->extradata, buf, st->codecpar->extradata_size);

@@ -2392,9 +2395,9 @@ static int mov_read_stsd(MOVContext *c, AVIOContext
*pb, MOVAtom atom)
     av_freep(&st->codecpar->extradata);
     st->codecpar->extradata_size = sc->extradata_size[0];
     if (sc->extradata_size[0]) {
-        st->codecpar->extradata = av_mallocz(sc->extradata_size[0] +
AV_INPUT_BUFFER_PADDING_SIZE);
-        if (!st->codecpar->extradata)
-            return AVERROR(ENOMEM);
+        uint64_t size = sc->extradata_size[0] +
AV_INPUT_BUFFER_PADDING_SIZE;
+        if ((ret = av_reallocp(&st->codecpar->extradata, size)) < 0)
+            return ret;
         memcpy(st->codecpar->extradata, sc->extradata[0],
sc->extradata_size[0]);
     }

Comments

Michael Niedermayer Aug. 26, 2017, 9:43 a.m. UTC | #1 
On Fri, Aug 25, 2017 at 11:25:11AM -0700, John Rummell wrote:
> Chromium uses tcmalloc which doesn't like mixing calls to posix_memalign()
> and realloc(). This change updates mov.c to only use av_realloc() when
> allocating memory for AVCodecParameters.extradata.
> ---
>  libavformat/mov.c | 21 ++++++++++++---------
>  1 file changed, 12 insertions(+), 9 deletions(-)

I dont think this is a practical solution.
having av_malloc and av_realloc incompatible is not a path that is
maintainable. No matter how hard people want that.

The fix to this is to make av_malloc() and av_realloc() compatible.

Does tcmalloc() provide sufficient alignment ? if so there is no
need for posix_memalign().
If not thats a shortcomming of tcmalloc() and something that especially
for a performance oriented malloc would seem out of place.

Fixing tcmalloc() so it can interoperate with posix_memalign(), for
example by also overriding posix_memalign() or whatever would also be
an option

but the code removed in
3835283293bfd38ba69203f4618f0f0f21377bcc
also would be a fix for this

again, i dont object to the patch, i just dont think this is practical
maintaince wise.

having av_malloc() and av_realloc* incompatible is a minefield and
something that creates an endless stream of bugs, work and bugfixes.
Making them compatible would lead to much more robust code


[...]
Carl Eugen Hoyos Aug. 26, 2017, 10:05 a.m. UTC | #2 
2017-08-26 11:43 GMT+02:00 Michael Niedermayer <michael@niedermayer.cc>:

> The fix to this is to make av_malloc() and av_realloc() compatible.
>
> Does tcmalloc() provide sufficient alignment ?

This report indicates it defaults to 16 bytes:
https://github.com/gperftools/gperftools/issues/433

But they intend to raise the alignment to 64 bytes iiuc:
https://github.com/gperftools/gperftools/issues/808

Carl Eugen
wm4 Aug. 28, 2017, 9:32 a.m. UTC | #3 
On Sat, 26 Aug 2017 11:43:26 +0200
Michael Niedermayer <michael@niedermayer.cc> wrote:

> but the code removed in
> 3835283293bfd38ba69203f4618f0f0f21377bcc
> also would be a fix for this

That code won't come back.
diff mbox

Patch

From 1e34019d625d4b33a1767af70bc6c7e2c6b42a7f Mon Sep 17 00:00:00 2001
From: John Rummell <jrummell@chromium.org>
Date: Wed, 21 Jun 2017 14:39:20 -0700
Subject: [PATCH] libavformat/mov: Always use av_realloc() for
 AVCodecParameters.extradata

Chromium uses tcmalloc which doesn't like mixing calls to posix_memalign()
and realloc(). This change updates mov.c to only use av_realloc() when
allocating memory for AVCodecParameters.extradata.
---
 libavformat/mov.c | 21 ++++++++++++---------
 1 file changed, 12 insertions(+), 9 deletions(-)

diff --git a/libavformat/mov.c b/libavformat/mov.c
index 876f48d912..6e226c8ce4 100644
--- a/libavformat/mov.c
+++ b/libavformat/mov.c
@@ -1652,9 +1652,10 @@  static int mov_read_wave(MOVContext *c, AVIOContext *pb, MOVAtom atom)
                 atom.size += 8;
             } else if (!st->codecpar->extradata_size) {
 #define ALAC_EXTRADATA_SIZE 36
-                st->codecpar->extradata = av_mallocz(ALAC_EXTRADATA_SIZE + AV_INPUT_BUFFER_PADDING_SIZE);
-                if (!st->codecpar->extradata)
-                    return AVERROR(ENOMEM);
+                uint64_t size = ALAC_EXTRADATA_SIZE + AV_INPUT_BUFFER_PADDING_SIZE;
+                if ((ret = av_reallocp(&st->codecpar->extradata, size)) < 0)
+                    return ret;
+                memset(st->codecpar->extradata, 0, size);
                 st->codecpar->extradata_size = ALAC_EXTRADATA_SIZE;
                 AV_WB32(st->codecpar->extradata    , ALAC_EXTRADATA_SIZE);
                 AV_WB32(st->codecpar->extradata + 4, MKTAG('a','l','a','c'));
@@ -2054,6 +2055,8 @@  static int mov_rewrite_dvd_sub_extradata(AVStream *st)
     char buf[256] = {0};
     uint8_t *src = st->codecpar->extradata;
     int i;
+    uint64_t size;
+    int ret;
 
     if (st->codecpar->extradata_size != 64)
         return 0;
@@ -2075,9 +2078,9 @@  static int mov_rewrite_dvd_sub_extradata(AVStream *st)
 
     av_freep(&st->codecpar->extradata);
     st->codecpar->extradata_size = 0;
-    st->codecpar->extradata = av_mallocz(strlen(buf) + AV_INPUT_BUFFER_PADDING_SIZE);
-    if (!st->codecpar->extradata)
-        return AVERROR(ENOMEM);
+    size = strlen(buf) + AV_INPUT_BUFFER_PADDING_SIZE;
+    if ((ret = av_reallocp(&st->codecpar->extradata, size)) < 0)
+        return ret;
     st->codecpar->extradata_size = strlen(buf);
     memcpy(st->codecpar->extradata, buf, st->codecpar->extradata_size);
 
@@ -2392,9 +2395,9 @@  static int mov_read_stsd(MOVContext *c, AVIOContext *pb, MOVAtom atom)
     av_freep(&st->codecpar->extradata);
     st->codecpar->extradata_size = sc->extradata_size[0];
     if (sc->extradata_size[0]) {
-        st->codecpar->extradata = av_mallocz(sc->extradata_size[0] + AV_INPUT_BUFFER_PADDING_SIZE);
-        if (!st->codecpar->extradata)
-            return AVERROR(ENOMEM);
+        uint64_t size = sc->extradata_size[0] + AV_INPUT_BUFFER_PADDING_SIZE;
+        if ((ret = av_reallocp(&st->codecpar->extradata, size)) < 0)
+            return ret;
         memcpy(st->codecpar->extradata, sc->extradata[0], sc->extradata_size[0]);
     }
 
-- 
2.14.1.342.g6490525c54-goog