[FFmpeg-devel] [PATCH] lavf/mxfdec: handle identification metadata

Matthieu Bouron matthieu.bouron at gmail.com
Thu Apr 4 15:33:56 CEST 2013


On Thu, Apr 04, 2013 at 02:56:17PM +0200, Matthieu Bouron wrote:
> On Thu, Apr 04, 2013 at 01:43:36PM +0200, Tomas Härdin wrote:
> > On Fri, 2013-03-29 at 20:21 +0100, Matthieu Bouron wrote:
> > > ---
> > >  libavformat/mxfdec.c | 131 +++++++++++++++++++++++++++++++++++++++++++++++++++
> > >  1 file changed, 131 insertions(+)
> > > 
> > > diff --git a/libavformat/mxfdec.c b/libavformat/mxfdec.c
> > > index 4580e1b..fa1a0fe 100644
> > > --- a/libavformat/mxfdec.c
> > > +++ b/libavformat/mxfdec.c
> > > @@ -1619,6 +1619,136 @@ fail_and_free:
> > >      return ret;
> > >  }
> > >  
> > > +static int mxf_read_uid(AVIOContext *pb, int size, UID uid)
> > > +{
> > > +    if (size != 16)
> > > +        return AVERROR(EINVAL);
> > > +
> > > +    avio_read(pb, uid, size);
> > > +
> > > +    return 0;
> > > +}
> > 
> > Not required - just avio_read() in the macro. The spec mandates the size
> > of this field, and accidental overreading on bad files is already
> > handled.
> > 
> > > +static int mxf_read_utf16_string(AVIOContext *pb, int size, char** str)
> > > +{
> > > +    int ret;
> > > +    char *buf;
> > > +    if (size <= 0)
> > > +        return AVERROR(EINVAL);
> > 
> > Strings can be empty, and size is never (?) negative.
> 
> Switched to strict comparison. size is never negative but since it is
> used by av_mallocz and avio_read_str16be i kept the check.
> 
> > 
> > > +
> > > +    buf = av_malloc(size + 1);
> > 
> > *str = av_malloc() and drop buf altogether?
> 
> Done.
> 
> > 
> > > +    if (!buf)
> > > +        return AVERROR(ENOMEM);
> > > +
> > > +    if ((ret = avio_get_str16be(pb, size, buf, size + 1)) < 0) {
> > 
> > Size might need to be larger, or this might crap out certain strings.
> 
> buffer size should be ok since it is size + 1 large (twice the size needed
> for the utf8 version of the string since avio_get_str16be performs a
> convertion).
> 
> > 
> > > +        av_free(buf);
> > > +        return ret;
> > > +    }
> > > +
> > > +    avio_skip(pb, size - ret);
> > 
> > Pointless skip.
> > 
> > > +    *str = buf;
> > > +
> > > +    return ret;
> > > +}
> > > +
> > > +static int mxf_read_timestamp(AVIOContext *pb, int size, uint64_t *ts)
> > > +{
> > > +    if (size != 8)
> > > +        return AVERROR(EINVAL);
> > > +
> > > +    *ts = avio_rb64(pb);
> > > +    return 0;
> > > +}
> > 
> > Again, no need for functions for reading constant-size fields. Just
> > avio_rb64().
> > 
> > > +static int mxf_uid_to_str(UID uid, char **str)
> > > +{
> > > +    int i;
> > > +    char *p;
> > > +    p = *str = av_mallocz(sizeof(UID) * 2 + 1);
> > > +    if (!p)
> > > +        return AVERROR(ENOMEM);
> > > +    for (i = 0; i < sizeof(UID); i++) {
> > > +        snprintf(p, 2 + 1, "%.2x", uid[i]);
> > > +        p += 2;
> > > +    }
> > > +    return 0;
> > > +}
> > 
> > Don't we do this elsewhere in mxfdec.c? On a related note, I believe
> > UIDs should be formatted like this (via mxfdump):
> 
> The formatting is already done for ULs not UIDs.
> 
> > 
> >     {3c5ae397-f4b5-4d0f-8a4b-82107242e4ee}
> 
> Done.
> 
> > 
> > I haven't located the spec for it though.
> > 
> > > +static int mxf_timestamp_to_str(uint64_t timestamp, char **str)
> > > +{
> > > +    struct tm time;
> > > +    time.tm_year = (timestamp >> 48) - 1900;
> > > +    time.tm_mon  = (timestamp >> 48 & 0xF) - 1;
> > > +    time.tm_mday = (timestamp >> 32 & 0xF);
> > > +    time.tm_hour = (timestamp >> 24 & 0XF);
> > > +    time.tm_min  = (timestamp >> 16 & 0xF);
> > > +    time.tm_sec  = (timestamp >> 8  & 0xF);
> > > +
> > > +    *str = av_mallocz(32);
> > > +    if (!*str)
> > > +        return AVERROR(ENOMEM);
> > > +    strftime(*str, 32, "%F %T", &time);
> > > +
> > > +    return 0;
> > > +}
> > > +
> > > +#define SET_STR_METADATA(name, str) \
> > > +    if ((ret = mxf_read_utf16_string(pb, size, &str)) < 0) \
> > > +        return ret; \
> > > +    av_dict_set(&s->metadata, name, str, AV_DICT_DONT_STRDUP_VAL);
> > > +#define SET_UID_METADATA(name, var, str) \
> > > +    if ((ret = mxf_read_uid(pb, size, var)) < 0) \
> > > +        return ret; \
> > > +    if ((ret = mxf_uid_to_str(var, &str)) < 0) \
> > > +        return ret; \
> > > +    av_dict_set(&s->metadata, name, str, AV_DICT_DONT_STRDUP_VAL);
> > > +
> > > +#define SET_TS_METADATA(name, var, str) \
> > > +    if ((ret = mxf_read_timestamp(pb, size, &var)) < 0) \
> > > +        return ret; \
> > > +    if ((ret = mxf_timestamp_to_str(var, &str)) < 0) \
> > > +        return ret; \
> > > +    av_dict_set(&s->metadata, name, str, AV_DICT_DONT_STRDUP_VAL);
> > 
> > I prefer omitting final semicolons on macros like these, and putting
> > them on the end of the macro invocations instead...
> 
> Done.
> 
> > 
> > > +static int mxf_read_identification_metadata(void *arg, AVIOContext *pb, int tag, int size, UID _uid, int64_t klv_offset)
> > > +{
> > > +    MXFContext *mxf = arg;
> > > +    AVFormatContext *s = mxf->fc;
> > > +    int ret;
> > > +    UID uid = { 0 };
> > > +    char *str = NULL;
> > > +    uint64_t ts;
> > > +    switch (tag) {
> > > +    case 0x3C0A:
> > > +        SET_UID_METADATA("uid", uid, str)
> > 
> > .. like here:
> > 
> >     SET_UID_METADATA("uid", uid, str);
> > 
> > > +    break;
> > 
> > Indent the breaks.
> > 
> > Good job overall :)
> > 
> 
> Patch updated.
> Thanks for the review !

> From b24c8d1369ca333edd633f0effc7086b1265c35f Mon Sep 17 00:00:00 2001
> From: Matthieu Bouron <matthieu.bouron at gmail.com>
> Date: Fri, 29 Mar 2013 14:09:31 +0100
> Subject: [PATCH] lavf/mxfdec: handle identification metadata
> 
> ---
>  libavformat/mxfdec.c | 109 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 109 insertions(+)
> 
> diff --git a/libavformat/mxfdec.c b/libavformat/mxfdec.c
> index fb517dd..d457c48 100644
> --- a/libavformat/mxfdec.c
> +++ b/libavformat/mxfdec.c
> @@ -1667,6 +1667,114 @@ fail_and_free:
>      return ret;
>  }
>  
> +static int mxf_read_utf16_string(AVIOContext *pb, int size, char** str)
> +{
> +    int ret;
> +    if (size < 0)
> +        return AVERROR(EINVAL);
> +
> +    *str = av_malloc(size + 1);
> +    if (!*str)
> +        return AVERROR(ENOMEM);
> +
> +    if ((ret = avio_get_str16be(pb, size, *str, size + 1)) < 0) {
> +        av_freep(str);
> +        return ret;
> +    }
> +
> +    return ret;
> +}
> +
> +static int mxf_uid_to_str(UID uid, char **str)
> +{
> +    int i;
> +    char *p;
> +    p = *str = av_mallocz(sizeof(UID) * 2 + 4 + 1);
> +    if (!p)
> +        return AVERROR(ENOMEM);
> +    for (i = 0; i < sizeof(UID); i++) {
> +        snprintf(p, 2 + 1, "%.2x", uid[i]);
> +        p += 2;
> +        if (i == 3 || i == 5 || i == 7 || i == 9) {
> +            snprintf(p, 1 + 1, "-");
> +            p++;
> +        }
> +    }
> +    return 0;
> +}
> +
> +static int mxf_timestamp_to_str(uint64_t timestamp, char **str)
> +{
> +    struct tm time;
> +    time.tm_year = (timestamp >> 48) - 1900;
> +    time.tm_mon  = (timestamp >> 48 & 0xF) - 1;
> +    time.tm_mday = (timestamp >> 32 & 0xF);
> +    time.tm_hour = (timestamp >> 24 & 0XF);
> +    time.tm_min  = (timestamp >> 16 & 0xF);
> +    time.tm_sec  = (timestamp >> 8  & 0xF);
> +
> +    *str = av_mallocz(32);
> +    if (!*str)
> +        return AVERROR(ENOMEM);
> +    strftime(*str, 32, "%F %T", &time);
> +
> +    return 0;
> +}
> +
> +#define SET_STR_METADATA(pb, name, str) \
> +    if ((ret = mxf_read_utf16_string(pb, size, &str)) < 0) \
> +        return ret; \
> +    av_dict_set(&s->metadata, name, str, AV_DICT_DONT_STRDUP_VAL)
> +
> +#define SET_UID_METADATA(pb, name, var, str) \
> +    avio_read(pb, var, 16); \
> +    if ((ret = mxf_uid_to_str(var, &str)) < 0) \
> +        return ret; \
> +    av_dict_set(&s->metadata, name, str, AV_DICT_DONT_STRDUP_VAL)
> +
> +#define SET_TS_METADATA(pb, name, var, str) \
> +    var = avio_rb64(pb); \
> +    if ((ret = mxf_timestamp_to_str(var, &str)) < 0) \
> +        return ret; \
> +    av_dict_set(&s->metadata, name, str, AV_DICT_DONT_STRDUP_VAL)
> +
> +static int mxf_read_identification_metadata(void *arg, AVIOContext *pb, int tag, int size, UID _uid, int64_t klv_offset)
> +{
> +    MXFContext *mxf = arg;
> +    AVFormatContext *s = mxf->fc;
> +    int ret;
> +    UID uid = { 0 };
> +    char *str = NULL;
> +    uint64_t ts;
> +    switch (tag) {
> +    case 0x3C0A:
> +        SET_UID_METADATA(pb, "uid", uid, str);
> +        break;
> +    case 0x3C09:
> +        SET_UID_METADATA(pb, "generation_uid", uid, str);
> +        break;
> +    case 0x3C01:
> +        SET_STR_METADATA(pb, "company_name", str);
> +        break;
> +    case 0x3C02:
> +        SET_STR_METADATA(pb, "product_name", str);
> +        break;
> +    case 0x3C04:
> +        SET_STR_METADATA(pb, "product_version", str);
> +        break;
> +    case 0x3C05:
> +        SET_UID_METADATA(pb, "product_uid", uid, str);
> +        break;
> +    case 0x3C06:
> +        SET_TS_METADATA(pb, "modification_date", ts, str);
> +        break;
> +    case 0x3C08:
> +        SET_STR_METADATA(pb, "application_platform", str);
> +        break;
> +    }
> +    return 0;
> +}
> +
>  static const MXFMetadataReadTableEntry mxf_metadata_read_table[] = {
>      { { 0x06,0x0E,0x2B,0x34,0x02,0x05,0x01,0x01,0x0d,0x01,0x02,0x01,0x01,0x05,0x01,0x00 }, mxf_read_primer_pack },
>      { { 0x06,0x0E,0x2B,0x34,0x02,0x05,0x01,0x01,0x0d,0x01,0x02,0x01,0x01,0x02,0x01,0x00 }, mxf_read_partition_pack },
> @@ -1679,6 +1787,7 @@ static const MXFMetadataReadTableEntry mxf_metadata_read_table[] = {
>      { { 0x06,0x0E,0x2B,0x34,0x02,0x05,0x01,0x01,0x0d,0x01,0x02,0x01,0x01,0x03,0x04,0x00 }, mxf_read_partition_pack },
>      { { 0x06,0x0E,0x2B,0x34,0x02,0x05,0x01,0x01,0x0d,0x01,0x02,0x01,0x01,0x04,0x02,0x00 }, mxf_read_partition_pack },
>      { { 0x06,0x0E,0x2B,0x34,0x02,0x05,0x01,0x01,0x0d,0x01,0x02,0x01,0x01,0x04,0x04,0x00 }, mxf_read_partition_pack },
> +    { { 0x06,0x0E,0x2B,0x34,0x02,0x53,0x01,0x01,0x0D,0x01,0x01,0x01,0x01,0x01,0x30,0x00 }, mxf_read_identification_metadata },
>      { { 0x06,0x0E,0x2B,0x34,0x02,0x53,0x01,0x01,0x0d,0x01,0x01,0x01,0x01,0x01,0x18,0x00 }, mxf_read_content_storage, 0, AnyType },
>      { { 0x06,0x0E,0x2B,0x34,0x02,0x53,0x01,0x01,0x0d,0x01,0x01,0x01,0x01,0x01,0x37,0x00 }, mxf_read_source_package, sizeof(MXFPackage), SourcePackage },
>      { { 0x06,0x0E,0x2B,0x34,0x02,0x53,0x01,0x01,0x0d,0x01,0x01,0x01,0x01,0x01,0x36,0x00 }, mxf_read_material_package, sizeof(MXFPackage), MaterialPackage },
> -- 
> 1.8.2
> 

New patch attached.
Added missing do { } while (0) statement to macros (thanks to Clément).
-------------- next part --------------
>From 91b062d50e4bd008263c60b22e72b78c17cc10bd Mon Sep 17 00:00:00 2001
From: Matthieu Bouron <matthieu.bouron at gmail.com>
Date: Fri, 29 Mar 2013 14:09:31 +0100
Subject: [PATCH] lavf/mxfdec: handle identification metadata

---
 libavformat/mxfdec.c | 112 +++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 112 insertions(+)

diff --git a/libavformat/mxfdec.c b/libavformat/mxfdec.c
index fb517dd..86c23b3 100644
--- a/libavformat/mxfdec.c
+++ b/libavformat/mxfdec.c
@@ -1667,6 +1667,117 @@ fail_and_free:
     return ret;
 }
 
+static int mxf_read_utf16_string(AVIOContext *pb, int size, char** str)
+{
+    int ret;
+    if (size < 0)
+        return AVERROR(EINVAL);
+
+    *str = av_malloc(size + 1);
+    if (!*str)
+        return AVERROR(ENOMEM);
+
+    if ((ret = avio_get_str16be(pb, size, *str, size + 1)) < 0) {
+        av_freep(str);
+        return ret;
+    }
+
+    return ret;
+}
+
+static int mxf_uid_to_str(UID uid, char **str)
+{
+    int i;
+    char *p;
+    p = *str = av_mallocz(sizeof(UID) * 2 + 4 + 1);
+    if (!p)
+        return AVERROR(ENOMEM);
+    for (i = 0; i < sizeof(UID); i++) {
+        snprintf(p, 2 + 1, "%.2x", uid[i]);
+        p += 2;
+        if (i == 3 || i == 5 || i == 7 || i == 9) {
+            snprintf(p, 1 + 1, "-");
+            p++;
+        }
+    }
+    return 0;
+}
+
+static int mxf_timestamp_to_str(uint64_t timestamp, char **str)
+{
+    struct tm time;
+    time.tm_year = (timestamp >> 48) - 1900;
+    time.tm_mon  = (timestamp >> 48 & 0xF) - 1;
+    time.tm_mday = (timestamp >> 32 & 0xF);
+    time.tm_hour = (timestamp >> 24 & 0XF);
+    time.tm_min  = (timestamp >> 16 & 0xF);
+    time.tm_sec  = (timestamp >> 8  & 0xF);
+
+    *str = av_mallocz(32);
+    if (!*str)
+        return AVERROR(ENOMEM);
+    strftime(*str, 32, "%F %T", &time);
+
+    return 0;
+}
+
+#define SET_STR_METADATA(pb, name, str) do { \
+    if ((ret = mxf_read_utf16_string(pb, size, &str)) < 0) \
+        return ret; \
+    av_dict_set(&s->metadata, name, str, AV_DICT_DONT_STRDUP_VAL); \
+} while (0)
+
+#define SET_UID_METADATA(pb, name, var, str) do { \
+    avio_read(pb, var, 16); \
+    if ((ret = mxf_uid_to_str(var, &str)) < 0) \
+        return ret; \
+    av_dict_set(&s->metadata, name, str, AV_DICT_DONT_STRDUP_VAL); \
+} while (0)
+
+#define SET_TS_METADATA(pb, name, var, str) do { \
+    var = avio_rb64(pb); \
+    if ((ret = mxf_timestamp_to_str(var, &str)) < 0) \
+        return ret; \
+    av_dict_set(&s->metadata, name, str, AV_DICT_DONT_STRDUP_VAL); \
+} while (0)
+
+static int mxf_read_identification_metadata(void *arg, AVIOContext *pb, int tag, int size, UID _uid, int64_t klv_offset)
+{
+    MXFContext *mxf = arg;
+    AVFormatContext *s = mxf->fc;
+    int ret;
+    UID uid = { 0 };
+    char *str = NULL;
+    uint64_t ts;
+    switch (tag) {
+    case 0x3C0A:
+        SET_UID_METADATA(pb, "uid", uid, str);
+        break;
+    case 0x3C09:
+        SET_UID_METADATA(pb, "generation_uid", uid, str);
+        break;
+    case 0x3C01:
+        SET_STR_METADATA(pb, "company_name", str);
+        break;
+    case 0x3C02:
+        SET_STR_METADATA(pb, "product_name", str);
+        break;
+    case 0x3C04:
+        SET_STR_METADATA(pb, "product_version", str);
+        break;
+    case 0x3C05:
+        SET_UID_METADATA(pb, "product_uid", uid, str);
+        break;
+    case 0x3C06:
+        SET_TS_METADATA(pb, "modification_date", ts, str);
+        break;
+    case 0x3C08:
+        SET_STR_METADATA(pb, "application_platform", str);
+        break;
+    }
+    return 0;
+}
+
 static const MXFMetadataReadTableEntry mxf_metadata_read_table[] = {
     { { 0x06,0x0E,0x2B,0x34,0x02,0x05,0x01,0x01,0x0d,0x01,0x02,0x01,0x01,0x05,0x01,0x00 }, mxf_read_primer_pack },
     { { 0x06,0x0E,0x2B,0x34,0x02,0x05,0x01,0x01,0x0d,0x01,0x02,0x01,0x01,0x02,0x01,0x00 }, mxf_read_partition_pack },
@@ -1679,6 +1790,7 @@ static const MXFMetadataReadTableEntry mxf_metadata_read_table[] = {
     { { 0x06,0x0E,0x2B,0x34,0x02,0x05,0x01,0x01,0x0d,0x01,0x02,0x01,0x01,0x03,0x04,0x00 }, mxf_read_partition_pack },
     { { 0x06,0x0E,0x2B,0x34,0x02,0x05,0x01,0x01,0x0d,0x01,0x02,0x01,0x01,0x04,0x02,0x00 }, mxf_read_partition_pack },
     { { 0x06,0x0E,0x2B,0x34,0x02,0x05,0x01,0x01,0x0d,0x01,0x02,0x01,0x01,0x04,0x04,0x00 }, mxf_read_partition_pack },
+    { { 0x06,0x0E,0x2B,0x34,0x02,0x53,0x01,0x01,0x0D,0x01,0x01,0x01,0x01,0x01,0x30,0x00 }, mxf_read_identification_metadata },
     { { 0x06,0x0E,0x2B,0x34,0x02,0x53,0x01,0x01,0x0d,0x01,0x01,0x01,0x01,0x01,0x18,0x00 }, mxf_read_content_storage, 0, AnyType },
     { { 0x06,0x0E,0x2B,0x34,0x02,0x53,0x01,0x01,0x0d,0x01,0x01,0x01,0x01,0x01,0x37,0x00 }, mxf_read_source_package, sizeof(MXFPackage), SourcePackage },
     { { 0x06,0x0E,0x2B,0x34,0x02,0x53,0x01,0x01,0x0d,0x01,0x01,0x01,0x01,0x01,0x36,0x00 }, mxf_read_material_package, sizeof(MXFPackage), MaterialPackage },
-- 
1.8.2



More information about the ffmpeg-devel mailing list