[FFmpeg-devel] [PATCH] Adds support parsing the QuickTime Metadata Keys.

[Derek Buitenhuis]

On 10/20/2015 7:29 PM, Tinglin Liu wrote:
> The Apple dev specification:
> https://developer.apple.com/library/mac/documentation/QuickTime/QTFF/Metadata/Metadata.html
> ---
>  libavformat/isom.h |  3 +++
>  libavformat/mov.c  | 77 +++++++++++++++++++++++++++++++++++++++++++++++++-----
>  2 files changed, 74 insertions(+), 6 deletions(-)

Is there a test file around we can add to FATE?

> @@ -177,6 +177,9 @@ typedef struct MOVContext {
>      int64_t duration;     ///< duration of the longest track
>      int found_moov;       ///< 'moov' atom has been found
>      int found_mdat;       ///< 'mdat' atom has been found
> +    int found_hdlr_mdta;  ///< 'hdlr' atom with type 'mdta' has been found
> +    char **meta_keys;
> +    unsigned meta_keys_count;

How exactly is it intended for these gets to be accessed? MOVContext is an internal
structure only. Perhaps this should be exported via side-data?

> @@ -368,6 +369,11 @@ retry:
>                      av_log(c->fc, AV_LOG_ERROR, "Error parsing cover art.\n");
>                  }
>                  return ret;
> +            } else if (!key && c->found_hdlr_mdta && c->meta_keys) {

Is it ever possible for c->meta_keys to not be allocated here? (Patch doesn't
provide me enough context liens to determine that.)

> +                uint32_t index = AV_RB32(&atom.type);
> +                if (index < c->meta_keys_count && c->meta_keys[index]) {

If we have already allocated c->meta_keys_count * sizeof(*c->meta_keys) entries,
the latter check is redundant.

> +                    key = c->meta_keys[index];
> +                }

Perhaps some sort of warning if the index is out-of-range?

> +    // Allocates enough space if data_type is a float32 number, otherwise
>      // worst-case requirement for output string in case of utf8 coded input
> -    str_size_alloc = (raw ? str_size : str_size * 2) + 1;
> +    num = (data_type == 23) ? 1 : 0;

Redundant ternary operator.

> +    str_size_alloc = (num ? 512 : (raw ? str_size : str_size * 2)) + 1;
>      str = av_mallocz(str_size_alloc);
>      if (!str)
>          return AVERROR(ENOMEM);
> @@ -405,6 +413,10 @@ retry:
>      else {
>          if (!raw && (data_type == 3 || (data_type == 0 && (langcode < 0x400 || langcode == 0x7fff)))) { // MAC Encoded
>              mov_read_mac_string(c, pb, str_size, str, str_size_alloc);
> +        } else if (data_type == 23 && str_size >= 4) {  // BE float32

Where does 4 come from?

> +            union av_intfloat32 val;
> +            val.i = avio_rb32(pb);

I'm not entirely sure of the portability of this code. Would this not fail on
any system without IEEE floats?

> +            snprintf(str, str_size_alloc, "%f", val.f);

Return value should be checked when using %f, in case of insane input.

> @@ -614,6 +621,15 @@ static int mov_read_hdlr(MOVContext *c, AVIOContext *pb, MOVAtom atom)
>      av_log(c->fc, AV_LOG_TRACE, "ctype= %.4s (0x%08x)\n", (char*)&ctype, ctype);
>      av_log(c->fc, AV_LOG_TRACE, "stype= %.4s\n", (char*)&type);
>  
> +    if (c->fc->nb_streams < 1) {  // meta before first trak
> +        if (type == MKTAG('m','d','t','a')) {
> +            c->found_hdlr_mdta = 1;
> +        }
> +        return 0;
> +    }
> +
> +    st = c->fc->streams[c->fc->nb_streams-1];

Any reason this was moved lower?

> +static int mov_read_keys(MOVContext *c, AVIOContext *pb, MOVAtom atom)
> +{
> +    uint32_t count;
> +    uint32_t i;
> +
> +    if (atom.size < 8)
> +        return 0;
> +
> +    avio_skip(pb, 4);
> +    count = avio_rb32(pb);
> +    if (count > UINT_MAX / sizeof(*c->meta_keys))
> +        return 0;

This shouldn't really return success.

Also, probably could do with a warning.

> +    av_freep(&c->meta_keys);

Why are we freeing this here? It should not be set at all, or it should not be
overwritten silently, no?

> +    c->meta_keys_count = count + 1;

Some may complain we are wasting 1 entry's worth of memory ;)... not that I particularily
care, but it may end up with some funny bugs later on if others assume a 0-origin.

> +    c->meta_keys = av_mallocz(c->meta_keys_count * sizeof(*c->meta_keys));
> +    if (!c->meta_keys)
> +        return AVERROR(ENOMEM);
> +
> +    for (i = 1; i <= count; ++i) {
> +        uint32_t key_size = avio_rb32(pb);
> +        uint32_t type = avio_rl32(pb);
> +        if (type != MKTAG('m','d','t','a') || key_size < 8)
> +            return 0;

Logging? Also, is it reasonable to return success here as you do?

The rest is just simple stuff like inconsistent use of braces and post/pre-increment,
and not worth noting.

- Derek