[PATCH 1/2] lavf/mov: fix parsing QuickTime meta after the first track
I'm not entirely sure why found_hdlr_mdta existed to begin with, so cc-ing Tinglin Liu (who originally wrote the patch) and Derek Buitenhuis (who signed off on it) hoping for some background. If these checks actually do have a purpose, then the `type == MKTAG('m','d','t','a')` check should be moved to before the `c->fc->nb_streams < 1` check instead of inside it. --- libavformat/isom.h | 1 - libavformat/mov.c | 10 +++------- 2 files changed, 3 insertions(+), 8 deletions(-) diff --git a/libavformat/isom.h b/libavformat/isom.h index 2246fed..6b6f678 100644 --- a/libavformat/isom.h +++ b/libavformat/isom.h @@ -198,7 +198,6 @@ 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 int trak_index; ///< Index of the current 'trak' char **meta_keys; unsigned meta_keys_count; diff --git a/libavformat/mov.c b/libavformat/mov.c index 6e80b93..54530e3 100644 --- a/libavformat/mov.c +++ b/libavformat/mov.c @@ -381,7 +381,7 @@ 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) { + } else if (!key && c->meta_keys) { uint32_t index = AV_RB32(&atom.type); if (index < c->meta_keys_count) { key = c->meta_keys[index]; @@ -694,12 +694,8 @@ 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->trak_index < 0) { // meta not inside a trak - if (type == MKTAG('m','d','t','a')) { - c->found_hdlr_mdta = 1; - } + if (c->fc->nb_streams < 1) // meta before first trak return 0; - } st = c->fc->streams[c->fc->nb_streams-1]; @@ -4505,7 +4501,7 @@ static int mov_read_default(MOVContext *c, AVIOContext *pb, MOVAtom atom) // Supports parsing the QuickTime Metadata Keys. // https://developer.apple.com/library/mac/documentation/QuickTime/QTFF/Metadat... - if (!parse && c->found_hdlr_mdta && + if (!parse && atom.type == MKTAG('m','e','t','a') && a.type == MKTAG('k','e','y','s')) { parse = mov_read_keys; -- 2.10.0
--- libavformat/mov.c | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/libavformat/mov.c b/libavformat/mov.c index 54530e3..b75acd2 100644 --- a/libavformat/mov.c +++ b/libavformat/mov.c @@ -42,6 +42,7 @@ #include "libavutil/aes_ctr.h" #include "libavutil/sha.h" #include "libavutil/timecode.h" +#include "libavutil/parseutils.h" #include "libavcodec/ac3tab.h" #include "libavcodec/mpegaudiodecheader.h" #include "avformat.h" @@ -476,6 +477,21 @@ retry: return ret; } str[str_size] = 0; + if (!strcmp(key, "com.apple.quicktime.creationdate")) { + struct tm *ptm, tmbuf; + int64_t timeval; + if (av_parse_time(&timeval, str, 0) >= 0) { + time_t timet = timeval / 1000000; + if (ptm = gmtime_r(&timet, &tmbuf)) { + strftime(str, str_size, "%Y-%m-%d %H:%M:%S", ptm); + key = "creation_time"; + } + } + } else if (!strcmp(key, "com.apple.quicktime.location.ISO6709")) { + key = "location"; + } else if (!strncmp(key, "com.apple.quicktime.", 20)) { + key += 20; + } } c->fc->event_flags |= AVFMT_EVENT_FLAG_METADATA_UPDATED; av_dict_set(&c->fc->metadata, key, str, 0); -- 2.10.0
On Fri, Sep 09, 2016 at 08:06:40PM -0500, Rodger Combs wrote:
--- libavformat/mov.c | 16 ++++++++++++++++ 1 file changed, 16 insertions(+)
diff --git a/libavformat/mov.c b/libavformat/mov.c index 54530e3..b75acd2 100644 --- a/libavformat/mov.c +++ b/libavformat/mov.c @@ -42,6 +42,7 @@ #include "libavutil/aes_ctr.h" #include "libavutil/sha.h" #include "libavutil/timecode.h" +#include "libavutil/parseutils.h" #include "libavcodec/ac3tab.h" #include "libavcodec/mpegaudiodecheader.h" #include "avformat.h" @@ -476,6 +477,21 @@ retry: return ret; } str[str_size] = 0; + if (!strcmp(key, "com.apple.quicktime.creationdate")) { + struct tm *ptm, tmbuf; + int64_t timeval; + if (av_parse_time(&timeval, str, 0) >= 0) { + time_t timet = timeval / 1000000; + if (ptm = gmtime_r(&timet, &tmbuf)) { + strftime(str, str_size, "%Y-%m-%d %H:%M:%S", ptm); + key = "creation_time"; + } + } + } else if (!strcmp(key, "com.apple.quicktime.location.ISO6709")) { + key = "location"; + } else if (!strncmp(key, "com.apple.quicktime.", 20)) { + key += 20; + } }
this causes language specific entries no longer to be listed together with their "parent" entries: is this missing some hadling of langauge specific stuff ? also off topic but we should sort metadata for display so related tags appear together ticket/4012/IMG_4596.MOV major_brand : qt minor_version : 0 - creation_time : 2014-10-05T10:06:36.000000Z + make-eng : Apple encoder : 8.0.2 encoder-eng : 8.0.2 date : 2014-10-05T18:06:36+0800 date-eng : 2014-10-05T18:06:36+0800 - location : +06.2271+099.7476+018.026/ + software : 8.0.2 location-eng : +06.2271+099.7476+018.026/ - model : iPhone 6 + creation_time : 2014-10-05T10:06:36.000 model-eng : iPhone 6 + location : +06.2271+099.7476+018.026/ + model : iPhone 6 make : Apple - make-eng : Apple rotate : 180 creation_time : 2014-10-05T10:06:36.000000Z handler_name : Core Media Data Handler [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Why not whip the teacher when the pupil misbehaves? -- Diogenes of Sinope
On Sep 10, 2016, at 05:55, Michael Niedermayer <michael@niedermayer.cc> wrote:
On Fri, Sep 09, 2016 at 08:06:40PM -0500, Rodger Combs wrote:
--- libavformat/mov.c | 16 ++++++++++++++++ 1 file changed, 16 insertions(+)
diff --git a/libavformat/mov.c b/libavformat/mov.c index 54530e3..b75acd2 100644 --- a/libavformat/mov.c +++ b/libavformat/mov.c @@ -42,6 +42,7 @@ #include "libavutil/aes_ctr.h" #include "libavutil/sha.h" #include "libavutil/timecode.h" +#include "libavutil/parseutils.h" #include "libavcodec/ac3tab.h" #include "libavcodec/mpegaudiodecheader.h" #include "avformat.h" @@ -476,6 +477,21 @@ retry: return ret; } str[str_size] = 0; + if (!strcmp(key, "com.apple.quicktime.creationdate")) { + struct tm *ptm, tmbuf; + int64_t timeval; + if (av_parse_time(&timeval, str, 0) >= 0) { + time_t timet = timeval / 1000000; + if (ptm = gmtime_r(&timet, &tmbuf)) { + strftime(str, str_size, "%Y-%m-%d %H:%M:%S", ptm); + key = "creation_time"; + } + } + } else if (!strcmp(key, "com.apple.quicktime.location.ISO6709")) { + key = "location"; + } else if (!strncmp(key, "com.apple.quicktime.", 20)) { + key += 20; + } }
this causes language specific entries no longer to be listed together with their "parent" entries:
Huh; interesting. Not sure why this happens, but also not sure if it matters?
is this missing some hadling of langauge specific stuff ?
Nothing I'm aware of, but I could easily be missing something.
also off topic but we should sort metadata for display so related tags appear together
Sounds worthwhile, but yeah out-of-scope here.
ticket/4012/IMG_4596.MOV
major_brand : qt minor_version : 0 - creation_time : 2014-10-05T10:06:36.000000Z + make-eng : Apple encoder : 8.0.2 encoder-eng : 8.0.2 date : 2014-10-05T18:06:36+0800 date-eng : 2014-10-05T18:06:36+0800 - location : +06.2271+099.7476+018.026/ + software : 8.0.2 location-eng : +06.2271+099.7476+018.026/ - model : iPhone 6 + creation_time : 2014-10-05T10:06:36.000 model-eng : iPhone 6 + location : +06.2271+099.7476+018.026/ + model : iPhone 6 make : Apple - make-eng : Apple rotate : 180 creation_time : 2014-10-05T10:06:36.000000Z handler_name : Core Media Data Handler
[...]
-- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
Why not whip the teacher when the pupil misbehaves? -- Diogenes of Sinope _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org <mailto:ffmpeg-devel@ffmpeg.org> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel <http://ffmpeg.org/mailman/listinfo/ffmpeg-devel>
On Mon, Sep 12, 2016 at 08:24:44PM -0500, Rodger Combs wrote:
On Sep 10, 2016, at 05:55, Michael Niedermayer <michael@niedermayer.cc> wrote:
On Fri, Sep 09, 2016 at 08:06:40PM -0500, Rodger Combs wrote:
--- libavformat/mov.c | 16 ++++++++++++++++ 1 file changed, 16 insertions(+)
diff --git a/libavformat/mov.c b/libavformat/mov.c index 54530e3..b75acd2 100644 --- a/libavformat/mov.c +++ b/libavformat/mov.c @@ -42,6 +42,7 @@ #include "libavutil/aes_ctr.h" #include "libavutil/sha.h" #include "libavutil/timecode.h" +#include "libavutil/parseutils.h" #include "libavcodec/ac3tab.h" #include "libavcodec/mpegaudiodecheader.h" #include "avformat.h" @@ -476,6 +477,21 @@ retry: return ret; } str[str_size] = 0; + if (!strcmp(key, "com.apple.quicktime.creationdate")) { + struct tm *ptm, tmbuf; + int64_t timeval; + if (av_parse_time(&timeval, str, 0) >= 0) { + time_t timet = timeval / 1000000; + if (ptm = gmtime_r(&timet, &tmbuf)) { + strftime(str, str_size, "%Y-%m-%d %H:%M:%S", ptm); + key = "creation_time"; + } + } + } else if (!strcmp(key, "com.apple.quicktime.location.ISO6709")) { + key = "location"; + } else if (!strncmp(key, "com.apple.quicktime.", 20)) { + key += 20; + } }
this causes language specific entries no longer to be listed together with their "parent" entries:
Huh; interesting. Not sure why this happens, but also not sure if it matters?
doesnt matter btw this patch seems to have been forgotten [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB You can kill me, but you cannot change the truth.
On Fri, Sep 09, 2016 at 08:06:39PM -0500, Rodger Combs wrote:
I'm not entirely sure why found_hdlr_mdta existed to begin with, so cc-ing Tinglin Liu (who originally wrote the patch) and Derek Buitenhuis (who signed off on it) hoping for some background. If these checks actually do have a purpose, then the `type == MKTAG('m','d','t','a')` check should be moved to before the `c->fc->nb_streams < 1` check instead of inside it. --- libavformat/isom.h | 1 - libavformat/mov.c | 10 +++------- 2 files changed, 3 insertions(+), 8 deletions(-)
This seems to change "handler_name : sound handler" to "handler_name : ilst handler" for http://samples.ffmpeg.org/MPEG-4/replaygain/sample.mp4 is that intended ? [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Modern terrorism, a quick summary: Need oil, start war with country that has oil, kill hundread thousand in war. Let country fall into chaos, be surprised about raise of fundamantalists. Drop more bombs, kill more people, be surprised about them taking revenge and drop even more bombs and strip your own citizens of their rights and freedoms. to be continued
On Sep 10, 2016, at 05:10, Michael Niedermayer <michael@niedermayer.cc> wrote:
On Fri, Sep 09, 2016 at 08:06:39PM -0500, Rodger Combs wrote:
I'm not entirely sure why found_hdlr_mdta existed to begin with, so cc-ing Tinglin Liu (who originally wrote the patch) and Derek Buitenhuis (who signed off on it) hoping for some background. If these checks actually do have a purpose, then the `type == MKTAG('m','d','t','a')` check should be moved to before the `c->fc->nb_streams < 1` check instead of inside it. --- libavformat/isom.h | 1 - libavformat/mov.c | 10 +++------- 2 files changed, 3 insertions(+), 8 deletions(-)
This seems to change "handler_name : sound handler" to "handler_name : ilst handler"
for http://samples.ffmpeg.org/MPEG-4/replaygain/sample.mp4
is that intended ?
Hmm, this looks like a consequence of the `handler_name` key being provided in multiple places on the same track, but I'm not sure `handler_name` is ever meaningful to the consumer to begin with, so I'm not sure if this matters?
[...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
Modern terrorism, a quick summary: Need oil, start war with country that has oil, kill hundread thousand in war. Let country fall into chaos, be surprised about raise of fundamantalists. Drop more bombs, kill more people, be surprised about them taking revenge and drop even more bombs and strip your own citizens of their rights and freedoms. to be continued _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
On Mon, Sep 12, 2016 at 09:02:25PM -0500, Rodger Combs wrote:
On Sep 10, 2016, at 05:10, Michael Niedermayer <michael@niedermayer.cc> wrote:
On Fri, Sep 09, 2016 at 08:06:39PM -0500, Rodger Combs wrote:
I'm not entirely sure why found_hdlr_mdta existed to begin with, so cc-ing Tinglin Liu (who originally wrote the patch) and Derek Buitenhuis (who signed off on it) hoping for some background. If these checks actually do have a purpose, then the `type == MKTAG('m','d','t','a')` check should be moved to before the `c->fc->nb_streams < 1` check instead of inside it. --- libavformat/isom.h | 1 - libavformat/mov.c | 10 +++------- 2 files changed, 3 insertions(+), 8 deletions(-)
This seems to change "handler_name : sound handler" to "handler_name : ilst handler"
for http://samples.ffmpeg.org/MPEG-4/replaygain/sample.mp4
is that intended ?
Hmm, this looks like a consequence of the `handler_name` key being provided in multiple places on the same track, but I'm not sure `handler_name` is ever meaningful to the consumer to begin with, so I'm not sure if this matters?
no idea iam not objecting to the patch, just pointing out what i found [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Does the universe only have a finite lifespan? No, its going to go on forever, its just that you wont like living in it. -- Hiranya Peiri
participants (2)
-
Michael Niedermayer -
Rodger Combs