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

[Tomas Härdin]

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.

> +
> +    buf = av_malloc(size + 1);

*str = av_malloc() and drop buf altogether?

> +    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.

> +        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):

    {3c5ae397-f4b5-4d0f-8a4b-82107242e4ee}

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...

> +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 :)

/Tomas
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20130404/21c9244b/attachment.asc>