[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