diff mbox series

[FFmpeg-devel,libavformat/mov.c] Read the QT Metadata Keys only once

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
Related show

Checks

Context Check Description
andriy/default pending
andriy/make success Make finished
andriy/make_fate success Make fate finished

Commit Message

Thierry Foucu May 11, 2020, 4:35 p.m. UTC
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(-)

Comments

Thierry Foucu May 13, 2020, 11:06 p.m. UTC | #1
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
Derek Buitenhuis May 14, 2020, 12:09 p.m. UTC | #2
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
Thierry Foucu May 14, 2020, 5:19 p.m. UTC | #3
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".
Derek Buitenhuis May 14, 2020, 7:26 p.m. UTC | #4
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
Thierry Foucu May 15, 2020, 5 p.m. UTC | #5
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".
Michael Niedermayer May 15, 2020, 7:56 p.m. UTC | #6
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 mbox series

Patch

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;
         }