diff mbox

[FFmpeg-devel] Make the process of uuid-xmp atom faster.

Message ID B2C9866A-99B1-4ABE-BB68-CA0F0776897E@alibaba-inc.com
State Accepted
Headers show

Commit Message

=?GBK?B?w8+zvSjj1bfqKQ==?= Nov. 11, 2016, 3:42 a.m. UTC
Ya. It’s really annoying everyone using patchwork. (So loooong text.) I’m trying to fix it.
Please use my name, Chen Meng, if nothing changed in a short time.

2016-11-11 10:36 GMT+08:00 Chen Meng <mengchen.mc@alibaba-inc.com>:
---
 libavformat/mov.c | 32 ++++++++++++++++++--------------
 1 file changed, 18 insertions(+), 14 deletions(-)

--
2.10.2
LGTM,   Except Submitter name used GBK at https://patchwork.ffmpeg.org/project/ffmpeg/list/

Comments

Michael Niedermayer Nov. 11, 2016, 2:40 p.m. UTC | #1
On Fri, Nov 11, 2016 at 11:42:26AM +0800, Chen Meng wrote:
> Ya. It’s really annoying everyone using patchwork. (So loooong text.) I’m trying to fix it.
> Please use my name, Chen Meng, if nothing changed in a short time.
> 
> 2016-11-11 10:36 GMT+08:00 Chen Meng <mengchen.mc@alibaba-inc.com>:
> ---
>  libavformat/mov.c | 32 ++++++++++++++++++--------------
>  1 file changed, 18 insertions(+), 14 deletions(-)
> 
> diff --git a/libavformat/mov.c b/libavformat/mov.c
> index 9ec7d03..436c234 100644
> --- a/libavformat/mov.c
> +++ b/libavformat/mov.c
> @@ -4549,24 +4549,28 @@ static int mov_read_uuid(MOVContext *c, AVIOContext *pb, MOVAtom atom)
>      } else if (!memcmp(uuid, uuid_xmp, sizeof(uuid))) {
>          uint8_t *buffer;
>          size_t len = atom.size - sizeof(uuid);
> -
> -        buffer = av_mallocz(len + 1);
> -        if (!buffer) {
> -            return AVERROR(ENOMEM);
> -        }
> -        ret = avio_read(pb, buffer, len);
> -        if (ret < 0) {
> -            av_free(buffer);
> -            return ret;
> -        } else if (ret != len) {
> -            av_free(buffer);
> -            return AVERROR_INVALIDDATA;
[...]
> +            ret = avio_skip(pb, len);
> +            if (ret < 0)
> +                return ret;

this treats ret != len differently than before
is that intended ?

[...]
=?GBK?B?w8+zvSjj1bfqKQ==?= Nov. 14, 2016, 2:26 a.m. UTC | #2
Yes, this change is intended.

Previously, the mov_read_uuid always read the uuid-xmp byte by byte even if, c->export_xmp == false, it’s no need to export xmp data by default.
It’s OK for a small size uuid-xmp atom. But for a large size of that atom, which is, for example, exported by non-linear editing system like Premiere, it’s really cost a lot of time.
One of mov file I met has a 56MB uuid-xmp atom, which ffmpeg takes about 5 minutes to operate it in network job.

So, for speeding up, I take the avio_read only if needed, which probe the large size of uuid-xmp atom mov file in seconds.

    On Fri, Nov 11, 2016 at 11:42:26AM +0800, Chen Meng wrote:
    > Ya. It’s really annoying everyone using patchwork. (So loooong text.) I’m trying to fix it.
    > Please use my name, Chen Meng, if nothing changed in a short time.
    > 
    > 2016-11-11 10:36 GMT+08:00 Chen Meng <mengchen.mc@alibaba-inc.com>:
    > ---
    >  libavformat/mov.c | 32 ++++++++++++++++++--------------
    >  1 file changed, 18 insertions(+), 14 deletions(-)
    > 
    > diff --git a/libavformat/mov.c b/libavformat/mov.c
    > index 9ec7d03..436c234 100644
    > --- a/libavformat/mov.c
    > +++ b/libavformat/mov.c
    > @@ -4549,24 +4549,28 @@ static int mov_read_uuid(MOVContext *c, AVIOContext *pb, MOVAtom atom)
    >      } else if (!memcmp(uuid, uuid_xmp, sizeof(uuid))) {
    >          uint8_t *buffer;
    >          size_t len = atom.size - sizeof(uuid);
    > -
    > -        buffer = av_mallocz(len + 1);
    > -        if (!buffer) {
    > -            return AVERROR(ENOMEM);
    > -        }
    > -        ret = avio_read(pb, buffer, len);
    > -        if (ret < 0) {
    > -            av_free(buffer);
    > -            return ret;
    > -        } else if (ret != len) {
    > -            av_free(buffer);
    > -            return AVERROR_INVALIDDATA;
    [...]
    > +            ret = avio_skip(pb, len);
    > +            if (ret < 0)
    > +                return ret;
    
    this treats ret != len differently than before
    is that intended ?
    
    [...]
    -- 
    Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
    
    Does the universe only have a finite lifespan? No, its going to go on
    forever, its just that you wont like living in it. -- Hiranya Peiri
    _______________________________________________
    ffmpeg-devel mailing list
    ffmpeg-devel@ffmpeg.org
    http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Michael Niedermayer Dec. 19, 2016, 10:08 p.m. UTC | #3
On Fri, Nov 11, 2016 at 11:42:26AM +0800, Chen Meng wrote:
> Ya. It’s really annoying everyone using patchwork. (So loooong text.) I’m trying to fix it.
> Please use my name, Chen Meng, if nothing changed in a short time.
> 
> 2016-11-11 10:36 GMT+08:00 Chen Meng <mengchen.mc@alibaba-inc.com>:
> ---
>  libavformat/mov.c | 32 ++++++++++++++++++--------------
>  1 file changed, 18 insertions(+), 14 deletions(-)

applied

thx

[...]
diff mbox

Patch

diff --git a/libavformat/mov.c b/libavformat/mov.c
index 9ec7d03..436c234 100644
--- a/libavformat/mov.c
+++ b/libavformat/mov.c
@@ -4549,24 +4549,28 @@  static int mov_read_uuid(MOVContext *c, AVIOContext *pb, MOVAtom atom)
     } else if (!memcmp(uuid, uuid_xmp, sizeof(uuid))) {
         uint8_t *buffer;
         size_t len = atom.size - sizeof(uuid);
-
-        buffer = av_mallocz(len + 1);
-        if (!buffer) {
-            return AVERROR(ENOMEM);
-        }
-        ret = avio_read(pb, buffer, len);
-        if (ret < 0) {
-            av_free(buffer);
-            return ret;
-        } else if (ret != len) {
-            av_free(buffer);
-            return AVERROR_INVALIDDATA;
-        }
         if (c->export_xmp) {
+            buffer = av_mallocz(len + 1);
+            if (!buffer) {
+                return AVERROR(ENOMEM);
+            }
+            ret = avio_read(pb, buffer, len);
+            if (ret < 0) {
+                av_free(buffer);
+                return ret;
+            } else if (ret != len) {
+                av_free(buffer);
+                return AVERROR_INVALIDDATA;
+            }
             buffer[len] = '\0';
             av_dict_set(&c->fc->metadata, "xmp", buffer, 0);
+            av_free(buffer);
+        } else {
+            // skip all uuid atom, which makes it fast for long uuid-xmp file
+            ret = avio_skip(pb, len);
+            if (ret < 0)
+                return ret;
         }
-        av_free(buffer);
     }
     return 0;
 }