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
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
