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 <nfx...@googlemail.com> wrote:
> On Fri, 12 Jan 2018 13:13:05 -0800
> rshaf...@tunein.com wrote:
>
> > From: Richard Shaffer <rshaf...@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@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
