Thank you Derek for the reply. Basically the metadata is stored in the structure like:
|--meta |----hdlr |----keys |----ilst 1) Firstly, we check if the handler type in the metadata handler atom is ‘mdta’. If it is, we set found_hdlr_mdta in MOVContext be 1. 2) The key and value are stored separately for each key-value pair. 'keys' atom stores the key name table, while 'ilst' atom stores the values corresponding to the indices in the key table. And since they are stored in two different atoms, I have to store the name and count of the keys in the MOVContext, and use them when parsing 'ilst'. Cheers, Tinglin On Thu, Oct 22, 2015 at 11:43 AM, Derek Buitenhuis < derek.buitenh...@gmail.com> wrote: > 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.) > c->found_hdlr_mdta checks the handler type of 'hdlr' is good, but c->meta_keys is read from 'keys'. I believe c->meta_keys is necessary in case the 'keys' atom is missing. > > > + 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. > Updated in the patch, thanks. > > > + key = c->meta_keys[index]; > > + } > > Perhaps some sort of warning if the index is out-of-range? > Updated in the patch, thanks. > > > + // 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. > Updated in the patch, thanks. > > + 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? > Because data_type 23 indicates it's a float32 data stored, so we need make sure the size of data >= (32/8 = 4). > > > + 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. > Updated in the patch, thanks. > > > @@ -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? > Because we need check the type of the hdlr before first trak. > > > +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. > Updated in the patch, thanks. > > > + 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? > Yes, it shouldn't be set at all, I just make sure it's null, but it doesn't matter. Removed in the patch, thanks. > > > + 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. > If you don't feel strong about it, I would prefer keeping as it because the logic is more clear to me to keep the same index values. > > + 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? > Updated in the patch, thanks. > > 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 > _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
