[FFmpeg-devel] [PATCH] libavformat/mxfdec: export user comments metadata

[Mark Reid]

On Fri, Mar 13, 2015 at 3:57 AM, Tomas Härdin <tomas.hardin at codemill.se>
wrote:

> On Fri, 2015-03-06 at 13:24 -0800, Mark Reid wrote:
> > +static int mxf_read_indirect_value(void *arg, AVIOContext *pb, int size)
> > +{
> > +    MXFTaggedValue *tagged_value = arg;
> > +    uint8_t key[17];
> > +
> > +    if (size <= 17)
> > +        return 0;
> > +
> > +    avio_read(pb, key, 17);
>
> Really Avid, 17 byte keys? OK..
>

Its not really a 17 byte key, the first byte is the endianness, (0x4c == le
and 0x42 == be). I added the byte to the front of the key because I thought
it made identifying the indirect type logic simpler.


> > +    /* TODO: handle other types of of indirect values */
> > +    if (memcmp(key, mxf_indirect_value_utf16le, 17) == 0) {
> > +        return mxf_read_utf16le_string(pb, size - 17,
> &tagged_value->value);
> > +    } else if (memcmp(key, mxf_indirect_value_utf16be, 17) == 0) {
> > +        return mxf_read_utf16be_string(pb, size - 17,
> &tagged_value->value);
> > +    }
> > +    return 0;
> > +}
>
> > +static int mxf_parse_package_comments(MXFContext *mxf, AVDictionary
> **pm, MXFPackage *package)
> > +{
> > +    MXFTaggedValue *tag;
> > +    int size, i;
> > +    const char *prefix = "comment_";
> > +    char *key = NULL;
> > +
> > +    for (i = 0; i < package->comment_count; i++) {
> > +        tag = mxf_resolve_strong_ref(mxf, &package->comment_refs[i],
> TaggedValue);
> > +        if (!tag || !tag->name || !tag->value)
> > +            continue;
> > +
> > +        size = strlen(prefix) + strlen(tag->name) + 1;
> > +        key = av_mallocz(size);
> > +        if (!key)
> > +            return AVERROR(ENOMEM);
> > +
> > +        strcpy(key, prefix);
> > +        strlcat(key, tag->name, size);
>
> snprintf() would make this one line only, or use av_strlcat() like
> Michael suggests. Come to think of it, I'm not sure snprintf() exists on
> all platforms..
>
>
I see snprintf being used quite a lot used throughout the project, I'll try
using that instead,


> > +        av_dict_set(pm, key, tag->value, AV_DICT_DONT_STRDUP_KEY);
> > +    }
> > +    return 0;
> > +}
>
> Looks good overall, even if I wasn't aware of this Avid feature before.
>
>
great, I'm hoping to add setting these metadata keys to mxfenc as well,
because these tags show up in Avid bins as new columns.
I'll submit a new patch fixing the strlcat, thanks for taking the time to
review.