[FFmpeg-devel] [PATCH] lavf/mxfdec: handle identification metadata
Matthieu Bouron
matthieu.bouron at gmail.com
Thu Apr 4 14:56:17 CEST 2013
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 !
-------------- next part --------------
>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
More information about the ffmpeg-devel
mailing list