| Message ID | 20170201115051.13328-1-onemda@gmail.com |
|---|---|
| State | New |
| Headers | show |
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?)
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.
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.
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.
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.
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.
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).
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 {
Signed-off-by: Paul B Mahol <onemda@gmail.com> --- libavformat/mov.c | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+)