| Message ID | 20200511163500.144007-1-tfoucu@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel,libavformat/mov.c] Read the QT Metadata Keys only once | expand |
| Context | Check | Description |
|---|---|---|
| andriy/default | pending | |
| andriy/make | success | Make finished |
| andriy/make_fate | success | Make fate finished |
On Mon, May 11, 2020 at 9:35 AM Thierry Foucu <tfoucu@gmail.com> wrote: > If you have a file with multiple Metadata Keys, the second time you parse > the keys, you will re-alloc c->meta_keys without freeing the old one. > This change will avoid parsing all the consecutive Metadata keys. > --- > libavformat/mov.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/libavformat/mov.c b/libavformat/mov.c > index ad718cdaa2..062a62d93b 100644 > --- a/libavformat/mov.c > +++ b/libavformat/mov.c > @@ -7022,7 +7022,8 @@ static int mov_read_default(MOVContext *c, > AVIOContext *pb, MOVAtom atom) > // > https://developer.apple.com/library/mac/documentation/QuickTime/QTFF/Metadata/Metadata.html > if (!parse && c->found_hdlr_mdta && > atom.type == MKTAG('m','e','t','a') && > - a.type == MKTAG('k','e','y','s')) { > + a.type == MKTAG('k','e','y','s') && > + c->meta_keys_count == 0) { > parse = mov_read_keys; > } > > -- > 2.26.2.526.g744177e7f7-goog > > ping? Thanks
On 11/05/2020 17:35, Thierry Foucu wrote: > If you have a file with multiple Metadata Keys, the second time you parse > the keys, you will re-alloc c->meta_keys without freeing the old one. > This change will avoid parsing all the consecutive Metadata keys. > --- > libavformat/mov.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) Is it correct behavior to skip sunsequent ones entirely (per QTFF spec)? - Derek
On Thu, May 14, 2020 at 5:09 AM Derek Buitenhuis <derek.buitenhuis@gmail.com> wrote: > On 11/05/2020 17:35, Thierry Foucu wrote: > > If you have a file with multiple Metadata Keys, the second time you parse > > the keys, you will re-alloc c->meta_keys without freeing the old one. > > This change will avoid parsing all the consecutive Metadata keys. > > --- > > libavformat/mov.c | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > Is it correct behavior to skip sunsequent ones entirely (per QTFF spec)? > Looking at https://developer.apple.com/library/archive/documentation/QuickTime/QTFF/Metadata/Metadata.html The spec does not seem to say 1 or more. But because the `keys` atom is a list of indexes used by the `ilst` and the spec for `keys` said: Indexes into the metadata item keys atom are 1-based (1…entry_count). having 2+ `keys` atoms will conflict with this, as which one will be index 1 I'm assuming that only `keys` could only work. instead of skipping the consecutive `keys` atom, I could change the code to free the keys and re-alloc it for each `keys` atom found. So you will get only the data from the last `keys` atom. so, it is either the 1st `keys` or the last `keys`. Up to you. > - Derek > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
On 14/05/2020 18:19, Thierry Foucu wrote: > Looking at > https://developer.apple.com/library/archive/documentation/QuickTime/QTFF/Metadata/Metadata.html > The spec does not seem to say 1 or more. But because the `keys` atom is a > list of indexes used by the `ilst` and the spec for `keys` said: > Indexes into the metadata item keys atom are 1-based (1…entry_count). > > having 2+ `keys` atoms will conflict with this, as which one will be index 1 > > I'm assuming that only `keys` could only work. > instead of skipping the consecutive `keys` atom, I could change the code to > free the keys and re-alloc it for each `keys` atom found. So you will get > only the data from the last `keys` atom. > > so, it is either the 1st `keys` or the last `keys`. Up to you. LGTM as-is then. - Derek
On Thu, May 14, 2020 at 12:26 PM Derek Buitenhuis < derek.buitenhuis@gmail.com> wrote: > On 14/05/2020 18:19, Thierry Foucu wrote: > > Looking at > > > https://developer.apple.com/library/archive/documentation/QuickTime/QTFF/Metadata/Metadata.html > > The spec does not seem to say 1 or more. But because the `keys` atom is a > > list of indexes used by the `ilst` and the spec for `keys` said: > > Indexes into the metadata item keys atom are 1-based (1…entry_count). > > > > having 2+ `keys` atoms will conflict with this, as which one will be > index 1 > > > > I'm assuming that only `keys` could only work. > > instead of skipping the consecutive `keys` atom, I could change the code > to > > free the keys and re-alloc it for each `keys` atom found. So you will get > > only the data from the last `keys` atom. > > > > so, it is either the 1st `keys` or the last `keys`. Up to you. > > LGTM as-is then. > Thanks for the review, Derek. Any chance to have someone to submit it? > > - Derek > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
On Fri, May 15, 2020 at 10:00:02AM -0700, Thierry Foucu wrote: > On Thu, May 14, 2020 at 12:26 PM Derek Buitenhuis < > derek.buitenhuis@gmail.com> wrote: > > > On 14/05/2020 18:19, Thierry Foucu wrote: > > > Looking at > > > > > https://developer.apple.com/library/archive/documentation/QuickTime/QTFF/Metadata/Metadata.html > > > The spec does not seem to say 1 or more. But because the `keys` atom is a > > > list of indexes used by the `ilst` and the spec for `keys` said: > > > Indexes into the metadata item keys atom are 1-based (1…entry_count). > > > > > > having 2+ `keys` atoms will conflict with this, as which one will be > > index 1 > > > > > > I'm assuming that only `keys` could only work. > > > instead of skipping the consecutive `keys` atom, I could change the code > > to > > > free the keys and re-alloc it for each `keys` atom found. So you will get > > > only the data from the last `keys` atom. > > > > > > so, it is either the 1st `keys` or the last `keys`. Up to you. > > > > LGTM as-is then. > > > > Thanks for the review, Derek. > Any chance to have someone to submit it? will apply thx [...]
diff --git a/libavformat/mov.c b/libavformat/mov.c index ad718cdaa2..062a62d93b 100644 --- a/libavformat/mov.c +++ b/libavformat/mov.c @@ -7022,7 +7022,8 @@ static int mov_read_default(MOVContext *c, AVIOContext *pb, MOVAtom atom) // https://developer.apple.com/library/mac/documentation/QuickTime/QTFF/Metadata/Metadata.html if (!parse && c->found_hdlr_mdta && atom.type == MKTAG('m','e','t','a') && - a.type == MKTAG('k','e','y','s')) { + a.type == MKTAG('k','e','y','s') && + c->meta_keys_count == 0) { parse = mov_read_keys; }