| Message ID | 20160910010640.68408-1-rodger.combs@gmail.com |
|---|---|
| State | Withdrawn, archived |
| Headers | show |
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 ? [...]
> 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 [...]
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;
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(-)