[FFmpeg-devel] avformat/mov: export xml metadata

Submitted by Paul B Mahol on Feb. 1, 2017, 11:50 a.m.

Details

Message ID 20170201115051.13328-1-onemda@gmail.com
State New
Headers show

Commit Message

Paul B Mahol Feb. 1, 2017, 11:50 a.m.
Signed-off-by: Paul B Mahol <onemda@gmail.com>
---
 libavformat/mov.c | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

Comments

wm4 Feb. 1, 2017, 12:07 p.m.
On Wed,  1 Feb 2017 12:50:51 +0100
Paul B Mahol <onemda@gmail.com> wrote:

> Signed-off-by: Paul B Mahol <onemda@gmail.com>
> ---
>  libavformat/mov.c | 25 +++++++++++++++++++++++++
>  1 file changed, 25 insertions(+)
> 
> diff --git a/libavformat/mov.c b/libavformat/mov.c
> index 9ae4f8c..75e1c9c60 100644
> --- a/libavformat/mov.c
> +++ b/libavformat/mov.c
> @@ -3764,6 +3764,25 @@ static int mov_read_keys(MOVContext *c, AVIOContext *pb, MOVAtom atom)
>      return 0;
>  }
>  
> +static int mov_read_xml(MOVContext *c, AVIOContext *pb, MOVAtom atom)
> +{
> +    uint8_t *xml;
> +
> +    if (atom.size < 5)
> +        return 0;
> +
> +    avio_skip(pb, 4);
> +    xml = av_calloc(atom.size - 4 + 1, sizeof(uint8_t));
> +    if (!xml)
> +        return AVERROR(ENOMEM);
> +
> +    avio_read(pb, xml, atom.size - 4);
> +    av_dict_set(&c->fc->metadata, "xml", xml, 0);
> +    av_free(xml);
> +
> +    return 0;
> +}
> +
>  static int mov_read_custom(MOVContext *c, AVIOContext *pb, MOVAtom atom)
>  {
>      int64_t end = avio_tell(pb) + atom.size;
> @@ -5280,6 +5299,12 @@ static int mov_read_default(MOVContext *c, AVIOContext *pb, MOVAtom atom)
>              parse = mov_read_keys;
>          }
>  
> +        if (!parse &&
> +            atom.type == MKTAG('m','e','t','a') &&
> +            a.type == MKTAG('x','m','l',' ')) {
> +            parse = mov_read_xml;
> +        }
> +
>          if (!parse) { /* skip leaf atoms data */
>              avio_skip(pb, a.size);
>          } else {

We had this discussion recently: the metadata AVDictionary should
probably only contain real tags, not other information that's contained
in the format but for which libavformat has no good way to export yet.

There needs to be a concept for distinguishing user-visible tags and
other data.

(You don't want to show a xml dump in a GUI that happens to use ffmpeg,
do you?)
Paul B Mahol Feb. 1, 2017, 12:21 p.m.
On 2/1/17, wm4 <nfxjfg@googlemail.com> wrote:
> On Wed,  1 Feb 2017 12:50:51 +0100
> Paul B Mahol <onemda@gmail.com> wrote:
>
>> Signed-off-by: Paul B Mahol <onemda@gmail.com>
>> ---
>>  libavformat/mov.c | 25 +++++++++++++++++++++++++
>>  1 file changed, 25 insertions(+)
>>
>> diff --git a/libavformat/mov.c b/libavformat/mov.c
>> index 9ae4f8c..75e1c9c60 100644
>> --- a/libavformat/mov.c
>> +++ b/libavformat/mov.c
>> @@ -3764,6 +3764,25 @@ static int mov_read_keys(MOVContext *c, AVIOContext
>> *pb, MOVAtom atom)
>>      return 0;
>>  }
>>
>> +static int mov_read_xml(MOVContext *c, AVIOContext *pb, MOVAtom atom)
>> +{
>> +    uint8_t *xml;
>> +
>> +    if (atom.size < 5)
>> +        return 0;
>> +
>> +    avio_skip(pb, 4);
>> +    xml = av_calloc(atom.size - 4 + 1, sizeof(uint8_t));
>> +    if (!xml)
>> +        return AVERROR(ENOMEM);
>> +
>> +    avio_read(pb, xml, atom.size - 4);
>> +    av_dict_set(&c->fc->metadata, "xml", xml, 0);
>> +    av_free(xml);
>> +
>> +    return 0;
>> +}
>> +
>>  static int mov_read_custom(MOVContext *c, AVIOContext *pb, MOVAtom atom)
>>  {
>>      int64_t end = avio_tell(pb) + atom.size;
>> @@ -5280,6 +5299,12 @@ static int mov_read_default(MOVContext *c,
>> AVIOContext *pb, MOVAtom atom)
>>              parse = mov_read_keys;
>>          }
>>
>> +        if (!parse &&
>> +            atom.type == MKTAG('m','e','t','a') &&
>> +            a.type == MKTAG('x','m','l',' ')) {
>> +            parse = mov_read_xml;
>> +        }
>> +
>>          if (!parse) { /* skip leaf atoms data */
>>              avio_skip(pb, a.size);
>>          } else {
>
> We had this discussion recently: the metadata AVDictionary should
> probably only contain real tags, not other information that's contained
> in the format but for which libavformat has no good way to export yet.
>
> There needs to be a concept for distinguishing user-visible tags and
> other data.
>
> (You don't want to show a xml dump in a GUI that happens to use ffmpeg,
> do you?)

You are being just rude, and do not want FFmpeg to improve.
Robert Krüger Feb. 1, 2017, 12:31 p.m.
On Wed, Feb 1, 2017 at 1:07 PM, wm4 <nfxjfg@googlemail.com> wrote:

> On Wed,  1 Feb 2017 12:50:51 +0100
> Paul B Mahol <onemda@gmail.com> wrote:
>
> > Signed-off-by: Paul B Mahol <onemda@gmail.com>
> > ---
> >  libavformat/mov.c | 25 +++++++++++++++++++++++++
> >  1 file changed, 25 insertions(+)
> >
> > diff --git a/libavformat/mov.c b/libavformat/mov.c
> > index 9ae4f8c..75e1c9c60 100644
> > --- a/libavformat/mov.c
> > +++ b/libavformat/mov.c
> > @@ -3764,6 +3764,25 @@ static int mov_read_keys(MOVContext *c,
> AVIOContext *pb, MOVAtom atom)
> >      return 0;
> >  }
> >
> > +static int mov_read_xml(MOVContext *c, AVIOContext *pb, MOVAtom atom)
> > +{
> > +    uint8_t *xml;
> > +
> > +    if (atom.size < 5)
> > +        return 0;
> > +
> > +    avio_skip(pb, 4);
> > +    xml = av_calloc(atom.size - 4 + 1, sizeof(uint8_t));
> > +    if (!xml)
> > +        return AVERROR(ENOMEM);
> > +
> > +    avio_read(pb, xml, atom.size - 4);
> > +    av_dict_set(&c->fc->metadata, "xml", xml, 0);
> > +    av_free(xml);
> > +
> > +    return 0;
> > +}
> > +
> >  static int mov_read_custom(MOVContext *c, AVIOContext *pb, MOVAtom atom)
> >  {
> >      int64_t end = avio_tell(pb) + atom.size;
> > @@ -5280,6 +5299,12 @@ static int mov_read_default(MOVContext *c,
> AVIOContext *pb, MOVAtom atom)
> >              parse = mov_read_keys;
> >          }
> >
> > +        if (!parse &&
> > +            atom.type == MKTAG('m','e','t','a') &&
> > +            a.type == MKTAG('x','m','l',' ')) {
> > +            parse = mov_read_xml;
> > +        }
> > +
> >          if (!parse) { /* skip leaf atoms data */
> >              avio_skip(pb, a.size);
> >          } else {
>
> We had this discussion recently: the metadata AVDictionary should
> probably only contain real tags, not other information that's contained
> in the format but for which libavformat has no good way to export yet.
>
> There needs to be a concept for distinguishing user-visible tags and
> other data.
>
> (You don't want to show a xml dump in a GUI that happens to use ffmpeg,
> do you?)
>
>
I thought that was already the case for XMP (i.e. xml) metadata embedded in
mov. I must look up how that was handled. I seem to remember there was a
flag to suppress this on the command line. With the policy you propose,
libavformat will be of a lot less use for api users. Not that I had a vote
here. It's just 2c of an api user.
Paul B Mahol Feb. 1, 2017, 12:47 p.m.
On 2/1/17, Robert Krueger <krueger@lesspain.software> wrote:
> On Wed, Feb 1, 2017 at 1:07 PM, wm4 <nfxjfg@googlemail.com> wrote:
>
>> On Wed,  1 Feb 2017 12:50:51 +0100
>> Paul B Mahol <onemda@gmail.com> wrote:
>>
>> > Signed-off-by: Paul B Mahol <onemda@gmail.com>
>> > ---
>> >  libavformat/mov.c | 25 +++++++++++++++++++++++++
>> >  1 file changed, 25 insertions(+)
>> >
>> > diff --git a/libavformat/mov.c b/libavformat/mov.c
>> > index 9ae4f8c..75e1c9c60 100644
>> > --- a/libavformat/mov.c
>> > +++ b/libavformat/mov.c
>> > @@ -3764,6 +3764,25 @@ static int mov_read_keys(MOVContext *c,
>> AVIOContext *pb, MOVAtom atom)
>> >      return 0;
>> >  }
>> >
>> > +static int mov_read_xml(MOVContext *c, AVIOContext *pb, MOVAtom atom)
>> > +{
>> > +    uint8_t *xml;
>> > +
>> > +    if (atom.size < 5)
>> > +        return 0;
>> > +
>> > +    avio_skip(pb, 4);
>> > +    xml = av_calloc(atom.size - 4 + 1, sizeof(uint8_t));
>> > +    if (!xml)
>> > +        return AVERROR(ENOMEM);
>> > +
>> > +    avio_read(pb, xml, atom.size - 4);
>> > +    av_dict_set(&c->fc->metadata, "xml", xml, 0);
>> > +    av_free(xml);
>> > +
>> > +    return 0;
>> > +}
>> > +
>> >  static int mov_read_custom(MOVContext *c, AVIOContext *pb, MOVAtom
>> > atom)
>> >  {
>> >      int64_t end = avio_tell(pb) + atom.size;
>> > @@ -5280,6 +5299,12 @@ static int mov_read_default(MOVContext *c,
>> AVIOContext *pb, MOVAtom atom)
>> >              parse = mov_read_keys;
>> >          }
>> >
>> > +        if (!parse &&
>> > +            atom.type == MKTAG('m','e','t','a') &&
>> > +            a.type == MKTAG('x','m','l',' ')) {
>> > +            parse = mov_read_xml;
>> > +        }
>> > +
>> >          if (!parse) { /* skip leaf atoms data */
>> >              avio_skip(pb, a.size);
>> >          } else {
>>
>> We had this discussion recently: the metadata AVDictionary should
>> probably only contain real tags, not other information that's contained
>> in the format but for which libavformat has no good way to export yet.
>>
>> There needs to be a concept for distinguishing user-visible tags and
>> other data.
>>
>> (You don't want to show a xml dump in a GUI that happens to use ffmpeg,
>> do you?)
>>
>>
> I thought that was already the case for XMP (i.e. xml) metadata embedded in
> mov. I must look up how that was handled. I seem to remember there was a
> flag to suppress this on the command line. With the policy you propose,
> libavformat will be of a lot less use for api users. Not that I had a vote
> here. It's just 2c of an api user.
>

Added export_xml option locally.
wm4 Feb. 1, 2017, 1:08 p.m.
On Wed, 1 Feb 2017 13:31:46 +0100
Robert Krüger <krueger@lesspain.software> wrote:

> On Wed, Feb 1, 2017 at 1:07 PM, wm4 <nfxjfg@googlemail.com> wrote:
> 
> > On Wed,  1 Feb 2017 12:50:51 +0100
> > Paul B Mahol <onemda@gmail.com> wrote:
> >  
> > > Signed-off-by: Paul B Mahol <onemda@gmail.com>
> > > ---
> > >  libavformat/mov.c | 25 +++++++++++++++++++++++++
> > >  1 file changed, 25 insertions(+)
> > >
> > > diff --git a/libavformat/mov.c b/libavformat/mov.c
> > > index 9ae4f8c..75e1c9c60 100644
> > > --- a/libavformat/mov.c
> > > +++ b/libavformat/mov.c
> > > @@ -3764,6 +3764,25 @@ static int mov_read_keys(MOVContext *c,  
> > AVIOContext *pb, MOVAtom atom)  
> > >      return 0;
> > >  }
> > >
> > > +static int mov_read_xml(MOVContext *c, AVIOContext *pb, MOVAtom atom)
> > > +{
> > > +    uint8_t *xml;
> > > +
> > > +    if (atom.size < 5)
> > > +        return 0;
> > > +
> > > +    avio_skip(pb, 4);
> > > +    xml = av_calloc(atom.size - 4 + 1, sizeof(uint8_t));
> > > +    if (!xml)
> > > +        return AVERROR(ENOMEM);
> > > +
> > > +    avio_read(pb, xml, atom.size - 4);
> > > +    av_dict_set(&c->fc->metadata, "xml", xml, 0);
> > > +    av_free(xml);
> > > +
> > > +    return 0;
> > > +}
> > > +
> > >  static int mov_read_custom(MOVContext *c, AVIOContext *pb, MOVAtom atom)
> > >  {
> > >      int64_t end = avio_tell(pb) + atom.size;
> > > @@ -5280,6 +5299,12 @@ static int mov_read_default(MOVContext *c,  
> > AVIOContext *pb, MOVAtom atom)  
> > >              parse = mov_read_keys;
> > >          }
> > >
> > > +        if (!parse &&
> > > +            atom.type == MKTAG('m','e','t','a') &&
> > > +            a.type == MKTAG('x','m','l',' ')) {
> > > +            parse = mov_read_xml;
> > > +        }
> > > +
> > >          if (!parse) { /* skip leaf atoms data */
> > >              avio_skip(pb, a.size);
> > >          } else {  
> >
> > We had this discussion recently: the metadata AVDictionary should
> > probably only contain real tags, not other information that's contained
> > in the format but for which libavformat has no good way to export yet.
> >
> > There needs to be a concept for distinguishing user-visible tags and
> > other data.
> >
> > (You don't want to show a xml dump in a GUI that happens to use ffmpeg,
> > do you?)
> >
> >  
> I thought that was already the case for XMP (i.e. xml) metadata embedded in
> mov. I must look up how that was handled. I seem to remember there was a
> flag to suppress this on the command line. With the policy you propose,
> libavformat will be of a lot less use for api users. Not that I had a vote
> here. It's just 2c of an api user.

I'm not voting for removing it, just that there should be a separate
mechanism for this.

And I as API user are very bothered by irrelevant crap being dumped
in what should return user tags (that can be displayed to a user), or
the fact that these can even show up in remuxes.

Example: just what is the use of "crap" like MAJOR_BRAND or
COMPATIBLE_BRANDS in Matroska tags? Yeah, let's add some
meaningless XML garbage to produced files as well.
Paul B Mahol Feb. 1, 2017, 1:17 p.m.
On 2/1/17, wm4 <nfxjfg@googlemail.com> wrote:
> On Wed, 1 Feb 2017 13:31:46 +0100
> Robert Krueger <krueger@lesspain.software> wrote:
>
>> On Wed, Feb 1, 2017 at 1:07 PM, wm4 <nfxjfg@googlemail.com> wrote:
>>
>> > On Wed,  1 Feb 2017 12:50:51 +0100
>> > Paul B Mahol <onemda@gmail.com> wrote:
>> >
>> > > Signed-off-by: Paul B Mahol <onemda@gmail.com>
>> > > ---
>> > >  libavformat/mov.c | 25 +++++++++++++++++++++++++
>> > >  1 file changed, 25 insertions(+)
>> > >
>> > > diff --git a/libavformat/mov.c b/libavformat/mov.c
>> > > index 9ae4f8c..75e1c9c60 100644
>> > > --- a/libavformat/mov.c
>> > > +++ b/libavformat/mov.c
>> > > @@ -3764,6 +3764,25 @@ static int mov_read_keys(MOVContext *c,
>> > AVIOContext *pb, MOVAtom atom)
>> > >      return 0;
>> > >  }
>> > >
>> > > +static int mov_read_xml(MOVContext *c, AVIOContext *pb, MOVAtom atom)
>> > > +{
>> > > +    uint8_t *xml;
>> > > +
>> > > +    if (atom.size < 5)
>> > > +        return 0;
>> > > +
>> > > +    avio_skip(pb, 4);
>> > > +    xml = av_calloc(atom.size - 4 + 1, sizeof(uint8_t));
>> > > +    if (!xml)
>> > > +        return AVERROR(ENOMEM);
>> > > +
>> > > +    avio_read(pb, xml, atom.size - 4);
>> > > +    av_dict_set(&c->fc->metadata, "xml", xml, 0);
>> > > +    av_free(xml);
>> > > +
>> > > +    return 0;
>> > > +}
>> > > +
>> > >  static int mov_read_custom(MOVContext *c, AVIOContext *pb, MOVAtom
>> > > atom)
>> > >  {
>> > >      int64_t end = avio_tell(pb) + atom.size;
>> > > @@ -5280,6 +5299,12 @@ static int mov_read_default(MOVContext *c,
>> > AVIOContext *pb, MOVAtom atom)
>> > >              parse = mov_read_keys;
>> > >          }
>> > >
>> > > +        if (!parse &&
>> > > +            atom.type == MKTAG('m','e','t','a') &&
>> > > +            a.type == MKTAG('x','m','l',' ')) {
>> > > +            parse = mov_read_xml;
>> > > +        }
>> > > +
>> > >          if (!parse) { /* skip leaf atoms data */
>> > >              avio_skip(pb, a.size);
>> > >          } else {
>> >
>> > We had this discussion recently: the metadata AVDictionary should
>> > probably only contain real tags, not other information that's contained
>> > in the format but for which libavformat has no good way to export yet.
>> >
>> > There needs to be a concept for distinguishing user-visible tags and
>> > other data.
>> >
>> > (You don't want to show a xml dump in a GUI that happens to use ffmpeg,
>> > do you?)
>> >
>> >
>> I thought that was already the case for XMP (i.e. xml) metadata embedded
>> in
>> mov. I must look up how that was handled. I seem to remember there was a
>> flag to suppress this on the command line. With the policy you propose,
>> libavformat will be of a lot less use for api users. Not that I had a vote
>> here. It's just 2c of an api user.
>
> I'm not voting for removing it, just that there should be a separate
> mechanism for this.

Can you define/explain such thing in detail?
If not I will use current approach.

>
> And I as API user are very bothered by irrelevant crap being dumped
> in what should return user tags (that can be displayed to a user), or
> the fact that these can even show up in remuxes.
>
> Example: just what is the use of "crap" like MAJOR_BRAND or
> COMPATIBLE_BRANDS in Matroska tags? Yeah, let's add some
> meaningless XML garbage to produced files as well.
wm4 Feb. 1, 2017, 1:28 p.m.
On Wed, 1 Feb 2017 14:17:19 +0100
Paul B Mahol <onemda@gmail.com> wrote:

> On 2/1/17, wm4 <nfxjfg@googlemail.com> wrote:
> > On Wed, 1 Feb 2017 13:31:46 +0100
> > Robert Krueger <krueger@lesspain.software> wrote:
> >  
> >> On Wed, Feb 1, 2017 at 1:07 PM, wm4 <nfxjfg@googlemail.com> wrote:
> >>  
> >> > On Wed,  1 Feb 2017 12:50:51 +0100
> >> > Paul B Mahol <onemda@gmail.com> wrote:
> >> >  
> >> > > Signed-off-by: Paul B Mahol <onemda@gmail.com>
> >> > > ---
> >> > >  libavformat/mov.c | 25 +++++++++++++++++++++++++
> >> > >  1 file changed, 25 insertions(+)
> >> > >
> >> > > diff --git a/libavformat/mov.c b/libavformat/mov.c
> >> > > index 9ae4f8c..75e1c9c60 100644
> >> > > --- a/libavformat/mov.c
> >> > > +++ b/libavformat/mov.c
> >> > > @@ -3764,6 +3764,25 @@ static int mov_read_keys(MOVContext *c,  
> >> > AVIOContext *pb, MOVAtom atom)  
> >> > >      return 0;
> >> > >  }
> >> > >
> >> > > +static int mov_read_xml(MOVContext *c, AVIOContext *pb, MOVAtom atom)
> >> > > +{
> >> > > +    uint8_t *xml;
> >> > > +
> >> > > +    if (atom.size < 5)
> >> > > +        return 0;
> >> > > +
> >> > > +    avio_skip(pb, 4);
> >> > > +    xml = av_calloc(atom.size - 4 + 1, sizeof(uint8_t));
> >> > > +    if (!xml)
> >> > > +        return AVERROR(ENOMEM);
> >> > > +
> >> > > +    avio_read(pb, xml, atom.size - 4);
> >> > > +    av_dict_set(&c->fc->metadata, "xml", xml, 0);
> >> > > +    av_free(xml);
> >> > > +
> >> > > +    return 0;
> >> > > +}
> >> > > +
> >> > >  static int mov_read_custom(MOVContext *c, AVIOContext *pb, MOVAtom
> >> > > atom)
> >> > >  {
> >> > >      int64_t end = avio_tell(pb) + atom.size;
> >> > > @@ -5280,6 +5299,12 @@ static int mov_read_default(MOVContext *c,  
> >> > AVIOContext *pb, MOVAtom atom)  
> >> > >              parse = mov_read_keys;
> >> > >          }
> >> > >
> >> > > +        if (!parse &&
> >> > > +            atom.type == MKTAG('m','e','t','a') &&
> >> > > +            a.type == MKTAG('x','m','l',' ')) {
> >> > > +            parse = mov_read_xml;
> >> > > +        }
> >> > > +
> >> > >          if (!parse) { /* skip leaf atoms data */
> >> > >              avio_skip(pb, a.size);
> >> > >          } else {  
> >> >
> >> > We had this discussion recently: the metadata AVDictionary should
> >> > probably only contain real tags, not other information that's contained
> >> > in the format but for which libavformat has no good way to export yet.
> >> >
> >> > There needs to be a concept for distinguishing user-visible tags and
> >> > other data.
> >> >
> >> > (You don't want to show a xml dump in a GUI that happens to use ffmpeg,
> >> > do you?)
> >> >
> >> >  
> >> I thought that was already the case for XMP (i.e. xml) metadata embedded
> >> in
> >> mov. I must look up how that was handled. I seem to remember there was a
> >> flag to suppress this on the command line. With the policy you propose,
> >> libavformat will be of a lot less use for api users. Not that I had a vote
> >> here. It's just 2c of an api user.  
> >
> > I'm not voting for removing it, just that there should be a separate
> > mechanism for this.  
> 
> Can you define/explain such thing in detail?
> If not I will use current approach.

Using side data. Or maybe just a different metadata field. Or clearly
defining which keys are internal and which are meant to be shown to the
user as text (as far as specified by the file format).

Patch hide | download patch | download mbox

diff --git a/libavformat/mov.c b/libavformat/mov.c
index 9ae4f8c..75e1c9c60 100644
--- a/libavformat/mov.c
+++ b/libavformat/mov.c
@@ -3764,6 +3764,25 @@  static int mov_read_keys(MOVContext *c, AVIOContext *pb, MOVAtom atom)
     return 0;
 }
 
+static int mov_read_xml(MOVContext *c, AVIOContext *pb, MOVAtom atom)
+{
+    uint8_t *xml;
+
+    if (atom.size < 5)
+        return 0;
+
+    avio_skip(pb, 4);
+    xml = av_calloc(atom.size - 4 + 1, sizeof(uint8_t));
+    if (!xml)
+        return AVERROR(ENOMEM);
+
+    avio_read(pb, xml, atom.size - 4);
+    av_dict_set(&c->fc->metadata, "xml", xml, 0);
+    av_free(xml);
+
+    return 0;
+}
+
 static int mov_read_custom(MOVContext *c, AVIOContext *pb, MOVAtom atom)
 {
     int64_t end = avio_tell(pb) + atom.size;
@@ -5280,6 +5299,12 @@  static int mov_read_default(MOVContext *c, AVIOContext *pb, MOVAtom atom)
             parse = mov_read_keys;
         }
 
+        if (!parse &&
+            atom.type == MKTAG('m','e','t','a') &&
+            a.type == MKTAG('x','m','l',' ')) {
+            parse = mov_read_xml;
+        }
+
         if (!parse) { /* skip leaf atoms data */
             avio_skip(pb, a.size);
         } else {