| Message ID | 20180112211305.5812-1-rshaffer@tunein.com |
|---|---|
| State | Superseded |
| Headers | show |
I just want to ping the list again to see if anyone would be willing to have a look at this change set. I sent it off last time on a Friday evening, so I'm not sure if maybe it was just forgotten or missed over the weekend, or if it's just not interesting to anyone else. If this isn't useful to anyone else, that would also be good to know. Any feedback at all would be appreciated if anyone is willing. Much thanks, -Richard On Fri, Jan 12, 2018 at 1:13 PM, <rshaffer@tunein.com> wrote: > From: Richard Shaffer <rshaffer@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, > }; > 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); > + > + 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); > + } 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); > + } > + } > + > + 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"); > } > -- > 2.14.3 (Apple Git-98) > >
On Fri, 12 Jan 2018 13:13:05 -0800 rshaffer@tunein.com wrote: > From: Richard Shaffer <rshaffer@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. 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. > 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.
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@googlemail.com> wrote: > On Fri, 12 Jan 2018 13:13:05 -0800 > rshaffer@tunein.com wrote: > > > From: Richard Shaffer <rshaffer@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 >
On Wed, 17 Jan 2018 11:10:26 -0800 Richard Shaffer <rshaffer@tunein.com> wrote: > [...] > 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. That's probably a lost cause. The "metadata" the FFmpeg API is full of container specific things and rather arbitrary. I don't think it matters to invent and export a few new keys. What needs to be assured is that the entry names can't clash with generic tag names (there's a list in avformat.h), which is why I suggested adding a prefix to the name. Although if metadata entries are very long (like big binary blobs) there might be a problem. I don't know what's the use of those priv tags though. > 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. Probably makes sense.
On Wed, Jan 17, 2018 at 11:38 AM, wm4 <nfxjfg@googlemail.com> wrote: > On Wed, 17 Jan 2018 11:10:26 -0800 > Richard Shaffer <rshaffer@tunein.com> wrote: > > > [...] > > 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. > > That's probably a lost cause. The "metadata" the FFmpeg API is full of > container specific things and rather arbitrary. I don't think it > matters to invent and export a few new keys. > > What needs to be assured is that the entry names can't clash with > generic tag names (there's a list in avformat.h), which is why I > suggested adding a prefix to the name. > > Although if metadata entries are very long (like big binary blobs) > there might be a problem. I don't know what's the use of those priv > tags though. > The tags that I have seen are short. However, most ID3v2 frames have a maximum length of 256MB, even text information frames. If it is a concern, we should probably implement a limit for ID3 tags generally. > > > 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. > > Probably makes sense. > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel >
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, }; 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); + + 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); + } 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); + } + } + + 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"); }
From: Richard Shaffer <rshaffer@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(-)