[FFmpeg-devel] [PATCH] avformat: add option to parse/store ID3 PRIV tags in metadata.
wm4
nfxjfg at googlemail.com
Wed Jan 17 20:21:44 EET 2018
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.
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.
More information about the ffmpeg-devel
mailing list