[FFmpeg-devel] [PATCH] avformat: add option to parse/store ID3 PRIV tags in metadata.

Richard Shaffer rshaffer at tunein.com
Wed Jan 17 21:10:26 EET 2018


Thanks for reviewing; it's much appreciated. I responded to one of the
comments in-line. I'll work on updating the patch to address your comments.

On Wed, Jan 17, 2018 at 10:21 AM, wm4 <nfxjfg at googlemail.com> wrote:

> On Fri, 12 Jan 2018 13:13:05 -0800
> rshaffer at tunein.com wrote:
>
> > From: Richard Shaffer <rshaffer at tunein.com>
> >
> > Enables getting access to ID3 PRIV tags from the command-line or
> metadata API
> > when demuxing. The PRIV owner is stored as the metadata key, and the
> data is
> > stored as the metadata value. As PRIV tags may contain arbitrary data,
> non-
> > printable characters, including NULL bytes, are escaped as \xXX.
> >
> > As this introduces a change in behavior, it must be enabled by setting
> the
> > 'id3v2_parse_priv' option.
> > ---
> >
> > I want to be able to expose PRIV tags using an existing API, but not
> sure if
> > this is the best approach. In particular, PRIV data may be of any type,
> while
> > metadata (and the AVDictionary type it uses) expresses values as
> strings. Any
> > feedback on the approach or specifics would be much appreciated,
> especially if
> > there is a suggestion for a better way to accomplish this.
> >
> > -Richard
> >
> >  libavformat/aacdec.c | 40 +++++++++++++++++++++++++++++++---------
> >  libavformat/id3v2.c  | 40 ++++++++++++++++++++++++++++++++++++++++
> >  libavformat/id3v2.h  | 13 +++++++++++++
> >  libavformat/mp3dec.c |  2 ++
> >  libavformat/utils.c  |  4 ++++
> >  5 files changed, 90 insertions(+), 9 deletions(-)
> >
> > diff --git a/libavformat/aacdec.c b/libavformat/aacdec.c
> > index 36d558ff54..46e10f34af 100644
> > --- a/libavformat/aacdec.c
> > +++ b/libavformat/aacdec.c
> > @@ -21,6 +21,7 @@
> >   */
> >
> >  #include "libavutil/intreadwrite.h"
> > +#include "libavutil/opt.h"
> >  #include "avformat.h"
> >  #include "internal.h"
> >  #include "id3v1.h"
> > @@ -28,6 +29,11 @@
> >
> >  #define ADTS_HEADER_SIZE 7
> >
> > +typedef struct AACDemuxContext {
> > +    AVClass *class;
> > +    int id3v2_parse_priv;
> > +} AACDemuxContext;
> > +
> >  static int adts_aac_probe(AVProbeData *p)
> >  {
> >      int max_frames = 0, first_frames = 0;
> > @@ -146,14 +152,30 @@ static int adts_aac_read_packet(AVFormatContext
> *s, AVPacket *pkt)
> >      return ret;
> >  }
> >
> > +static const AVOption aac_options[] = {
> > +    { "id3v2_parse_priv",
> > +        "parse ID3v2 PRIV tags", offsetof(AACDemuxContext,
> id3v2_parse_priv),
> > +        AV_OPT_TYPE_BOOL, { .i64 = 0 }, 0, 1,
> AV_OPT_FLAG_DECODING_PARAM },
> > +    { NULL },
> > +};
> > +
> > +static const AVClass aac_class = {
> > +    .class_name = "aac",
> > +    .item_name  = av_default_item_name,
> > +    .option     = aac_options,
> > +    .version    = LIBAVUTIL_VERSION_INT,
> > +};
> > +
> >  AVInputFormat ff_aac_demuxer = {
> > -    .name         = "aac",
> > -    .long_name    = NULL_IF_CONFIG_SMALL("raw ADTS AAC (Advanced Audio
> Coding)"),
> > -    .read_probe   = adts_aac_probe,
> > -    .read_header  = adts_aac_read_header,
> > -    .read_packet  = adts_aac_read_packet,
> > -    .flags        = AVFMT_GENERIC_INDEX,
> > -    .extensions   = "aac",
> > -    .mime_type    = "audio/aac,audio/aacp,audio/x-aac",
> > -    .raw_codec_id = AV_CODEC_ID_AAC,
> > +    .name           = "aac",
> > +    .long_name      = NULL_IF_CONFIG_SMALL("raw ADTS AAC (Advanced
> Audio Coding)"),
> > +    .read_probe     = adts_aac_probe,
> > +    .read_header    = adts_aac_read_header,
> > +    .read_packet    = adts_aac_read_packet,
> > +    .flags          = AVFMT_GENERIC_INDEX,
> > +    .priv_class     = &aac_class,
> > +    .priv_data_size = sizeof(AACDemuxContext),
> > +    .extensions     = "aac",
> > +    .mime_type      = "audio/aac,audio/aacp,audio/x-aac",
> > +    .raw_codec_id   = AV_CODEC_ID_AAC,
> >  };
>
> AAC and mp3 are by far not the only formats that can use ID3v2. FFmpeg
> accepts ID3v2 tags on basically all file formats. So just adding a
> private option to aac and mp3 doesn't make that much sense.


Fair point.

>
>
I'm also not sure if an option for compatibility is needed. It's
> probably fine to prefix the name with something (maybe "id3v2_priv."?),
> and always export it.
>

I guess my thought was that users of ffmpeg/ffprobe might have some
scripting or programming around the metadata API, such as dumping metadata
with -f ffmetadata and then mapping it to a stream later. In those cases,
this change might suddenly cause behavior that they weren't expecting.
Maybe that would be mitigated to some degree if we could also map
'id3v2_priv' back to PRIV tags on output. That would actually address a use
case that I have and would be super from my standpoint. I'll work on
implementing that. Please let me know if you have additional thoughts.

>
> > diff --git a/libavformat/id3v2.c b/libavformat/id3v2.c
> > index 6c216ba7a2..dd151dd7f2 100644
> > --- a/libavformat/id3v2.c
> > +++ b/libavformat/id3v2.c
> > @@ -33,6 +33,7 @@
> >  #endif
> >
> >  #include "libavutil/avstring.h"
> > +#include "libavutil/bprint.h"
> >  #include "libavutil/dict.h"
> >  #include "libavutil/intreadwrite.h"
> >  #include "avio_internal.h"
> > @@ -1224,3 +1225,42 @@ end:
> >      av_freep(&chapters);
> >      return ret;
> >  }
> > +
> > +int ff_id3v2_parse_priv_dict(AVDictionary **metadata, ID3v2ExtraMeta
> **extra_meta)
> > +{
> > +    ID3v2ExtraMeta *cur;
> > +    int dict_flags = AV_DICT_DONT_OVERWRITE | AV_DICT_DONT_STRDUP_VAL;
> > +
> > +    for (cur = *extra_meta; cur; cur = cur->next) {
> > +        if (!strcmp(cur->tag, "PRIV")) {
> > +            ID3v2ExtraMetaPRIV *priv = cur->data;
> > +            AVBPrint bprint;
> > +            char * escaped;
> > +            int i, ret;
> > +
> > +            av_bprint_init(&bprint, priv->datasize + sizeof(char),
> AV_BPRINT_SIZE_UNLIMITED);
>
> sizeof(char) makes no sense - it's always 1, and it's better to use 1.
>
> > +
> > +            for (i = 0; i < priv->datasize; i++) {
> > +                if (priv->data[i] < 32 || priv->data[i] > 126) {
> > +                    av_bprintf(&bprint, "\\x%02x", priv->data[i]);
> > +                } else if (priv->data[i] == '\\') {
> > +                    av_bprint_chars(&bprint, '\\', 2);
>
> Really not particularly fond of exporting binary data like this.
> There's probably no better way though, unless we just make this side
> data, which would come with other problems.
>
> I'd still argue that \ should be escaped in the same way as the
> binary chars for simplicity.
>
> > +                } else {
> > +                    av_bprint_chars(&bprint, priv->data[i], 1);
> > +                }
> > +            }
> > +
> > +            if ((ret = av_bprint_finalize(&bprint, &escaped)) < 0)
> > +                return ret;
> > +
> > +            av_dict_set(metadata, priv->owner, escaped, dict_flags);
>
> In theory you need to check the return value, although nobody in FFmpeg
> seems to do that.




> > +        }
> > +    }
> > +
> > +    return 0;
> > +}
> > +
> > +int ff_id3v2_parse_priv(AVFormatContext *s, ID3v2ExtraMeta
> **extra_meta)
> > +{
> > +    return ff_id3v2_parse_priv_dict(&s->metadata, extra_meta);
> > +}
> > \ No newline at end of file
> > diff --git a/libavformat/id3v2.h b/libavformat/id3v2.h
> > index 5e64ead096..5f46a46115 100644
> > --- a/libavformat/id3v2.h
> > +++ b/libavformat/id3v2.h
> > @@ -167,6 +167,19 @@ int ff_id3v2_parse_apic(AVFormatContext *s,
> ID3v2ExtraMeta **extra_meta);
> >   */
> >  int ff_id3v2_parse_chapters(AVFormatContext *s, ID3v2ExtraMeta
> **extra_meta);
> >
> > +/**
> > + * Parse PRIV tags into a dictionary. The PRIV owner is the metadata
> key. The
> > + * PRIV data is the value, with non-printable characters escaped.
> > + */
> > +int ff_id3v2_parse_priv_dict(AVDictionary **d, ID3v2ExtraMeta
> **extra_meta);
> > +
> > +/**
> > + * Add metadata for all PRIV tags in the ID3v2 header. The PRIV owner
> is the
> > + * metadata key. The PRIV data is the value, with non-printable
> characters
> > + * escaped.
> > + */
> > +int ff_id3v2_parse_priv(AVFormatContext *s, ID3v2ExtraMeta
> **extra_meta);
> > +
> >  extern const AVMetadataConv ff_id3v2_34_metadata_conv[];
> >  extern const AVMetadataConv ff_id3v2_4_metadata_conv[];
> >
> > diff --git a/libavformat/mp3dec.c b/libavformat/mp3dec.c
> > index a76fe32e59..d2041d0c44 100644
> > --- a/libavformat/mp3dec.c
> > +++ b/libavformat/mp3dec.c
> > @@ -55,6 +55,7 @@ typedef struct {
> >      unsigned frames; /* Total number of frames in file */
> >      unsigned header_filesize;   /* Total number of bytes in the stream
> */
> >      int is_cbr;
> > +    int id3v2_parse_priv;
> >  } MP3DecContext;
> >
> >  enum CheckRet {
> > @@ -579,6 +580,7 @@ static int mp3_seek(AVFormatContext *s, int
> stream_index, int64_t timestamp,
> >
> >  static const AVOption options[] = {
> >      { "usetoc", "use table of contents", offsetof(MP3DecContext,
> usetoc), AV_OPT_TYPE_BOOL, {.i64 = 0}, 0, 1, AV_OPT_FLAG_DECODING_PARAM},
> > +    { "id3v2_parse_priv", "parse ID3v2 PRIV tags",
> offsetof(MP3DecContext, id3v2_parse_priv), AV_OPT_TYPE_BOOL, { .i64 = 0 },
> 0, 1, AV_OPT_FLAG_DECODING_PARAM },
> >      { NULL },
> >  };
> >
> > diff --git a/libavformat/utils.c b/libavformat/utils.c
> > index 2185a6f05b..207628161e 100644
> > --- a/libavformat/utils.c
> > +++ b/libavformat/utils.c
> > @@ -629,10 +629,14 @@ int avformat_open_input(AVFormatContext **ps,
> const char *filename,
> >      if (id3v2_extra_meta) {
> >          if (!strcmp(s->iformat->name, "mp3") ||
> !strcmp(s->iformat->name, "aac") ||
> >              !strcmp(s->iformat->name, "tta")) {
> > +            int64_t id3v2_parse_priv = 0;
> > +            av_opt_get_int(s, "id3v2_parse_priv",
> AV_OPT_SEARCH_CHILDREN, &id3v2_parse_priv);
> >              if ((ret = ff_id3v2_parse_apic(s, &id3v2_extra_meta)) < 0)
> >                  goto fail;
> >              if ((ret = ff_id3v2_parse_chapters(s, &id3v2_extra_meta)) <
> 0)
> >                  goto fail;
> > +            if (id3v2_parse_priv && (ret = ff_id3v2_parse_priv(s,
> &id3v2_extra_meta)) < 0)
> > +                goto fail;
> >          } else
> >              av_log(s, AV_LOG_DEBUG, "demuxer does not support
> additional id3 data, skipping\n");
> >      }
>
> If the option really needs to be kept for whatever reason, it should
> probably be a global libavformat option.
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>


More information about the ffmpeg-devel mailing list