[FFmpeg-devel,1/2] lavf/mov: fix parsing QuickTime meta after the first track

Submitted by Rodger Combs on Sept. 10, 2016, 1:06 a.m.

Details

Message ID 20160910010640.68408-1-rodger.combs@gmail.com
State New
Headers show

Commit Message

Rodger Combs Sept. 10, 2016, 1:06 a.m.
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(-)

Comments

Michael Niedermayer Sept. 10, 2016, 10:10 a.m.
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 ?

[...]
Rodger Combs Sept. 13, 2016, 2:02 a.m.
> 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
Michael Niedermayer Sept. 13, 2016, 11:11 a.m.
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

[...]

Patch hide | download patch | download mbox

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/Metadata/Metadata.html
-        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;