[FFmpeg-devel] [PATCH 1/3] fftools/ffmpeg: rewrite checking whether codec AVOptions have been used
Marton Balint
cus at passwd.hu
Thu Jun 27 21:36:30 EEST 2024
On Thu, 27 Jun 2024, Anton Khirnov wrote:
> Share the code between encoding and decoding. Instead of checking every
> stream's options dictionary (which is also used for other purposes),
> track all used options in a dedicated dictionary.
> ---
> fftools/cmdutils.c | 17 ++++++++----
> fftools/cmdutils.h | 4 ++-
> fftools/ffmpeg.c | 50 ++++++++++++++++++++++++++++++++++
> fftools/ffmpeg.h | 4 ++-
> fftools/ffmpeg_demux.c | 50 ++++++++--------------------------
> fftools/ffmpeg_mux.c | 1 +
> fftools/ffmpeg_mux.h | 3 +++
> fftools/ffmpeg_mux_init.c | 56 +++++----------------------------------
> fftools/ffmpeg_opt.c | 18 -------------
> fftools/ffplay.c | 2 +-
> fftools/ffprobe.c | 2 +-
> 11 files changed, 91 insertions(+), 116 deletions(-)
>
> diff --git a/fftools/cmdutils.c b/fftools/cmdutils.c
> index a504acb3e4..9b18cf5e4d 100644
> --- a/fftools/cmdutils.c
> +++ b/fftools/cmdutils.c
> @@ -986,7 +986,7 @@ int check_stream_specifier(AVFormatContext *s, AVStream *st, const char *spec)
>
> int filter_codec_opts(const AVDictionary *opts, enum AVCodecID codec_id,
> AVFormatContext *s, AVStream *st, const AVCodec *codec,
> - AVDictionary **dst)
> + AVDictionary **dst, AVDictionary **opts_used)
> {
> AVDictionary *ret = NULL;
> const AVDictionaryEntry *t = NULL;
> @@ -1013,6 +1013,7 @@ int filter_codec_opts(const AVDictionary *opts, enum AVCodecID codec_id,
> while (t = av_dict_iterate(opts, t)) {
> const AVClass *priv_class;
> char *p = strchr(t->key, ':');
> + int used = 0;
>
> /* check stream specification in opt name */
> if (p) {
> @@ -1030,15 +1031,21 @@ int filter_codec_opts(const AVDictionary *opts, enum AVCodecID codec_id,
> !codec ||
> ((priv_class = codec->priv_class) &&
> av_opt_find(&priv_class, t->key, NULL, flags,
> - AV_OPT_SEARCH_FAKE_OBJ)))
> + AV_OPT_SEARCH_FAKE_OBJ))) {
> av_dict_set(&ret, t->key, t->value, 0);
> - else if (t->key[0] == prefix &&
> + used = 1;
> + } else if (t->key[0] == prefix &&
> av_opt_find(&cc, t->key + 1, NULL, flags,
> - AV_OPT_SEARCH_FAKE_OBJ))
> + AV_OPT_SEARCH_FAKE_OBJ)) {
> av_dict_set(&ret, t->key + 1, t->value, 0);
> + used = 1;
> + }
>
> if (p)
> *p = ':';
> +
> + if (used && opts_used)
> + av_dict_set(opts_used, t->key, "", 0);
> }
>
> *dst = ret;
> @@ -1063,7 +1070,7 @@ int setup_find_stream_info_opts(AVFormatContext *s,
>
> for (int i = 0; i < s->nb_streams; i++) {
> ret = filter_codec_opts(codec_opts, s->streams[i]->codecpar->codec_id,
> - s, s->streams[i], NULL, &opts[i]);
> + s, s->streams[i], NULL, &opts[i], NULL);
> if (ret < 0)
> goto fail;
> }
> diff --git a/fftools/cmdutils.h b/fftools/cmdutils.h
> index 2125f791d0..14340dff7d 100644
> --- a/fftools/cmdutils.h
> +++ b/fftools/cmdutils.h
> @@ -371,11 +371,13 @@ int check_stream_specifier(AVFormatContext *s, AVStream *st, const char *spec);
> * @param codec The particular codec for which the options should be filtered.
> * If null, the default one is looked up according to the codec id.
> * @param dst a pointer to the created dictionary
> + * @param opts_used if non-NULL, every option stored in dst is also stored here,
> + * with specifiers preserved
> * @return a non-negative number on success, a negative error code on failure
> */
> int filter_codec_opts(const AVDictionary *opts, enum AVCodecID codec_id,
> AVFormatContext *s, AVStream *st, const AVCodec *codec,
> - AVDictionary **dst);
> + AVDictionary **dst, AVDictionary **opts_used);
>
> /**
> * Setup AVCodecContext options for avformat_find_stream_info().
> diff --git a/fftools/ffmpeg.c b/fftools/ffmpeg.c
> index 88ce3007e8..61593c842f 100644
> --- a/fftools/ffmpeg.c
> +++ b/fftools/ffmpeg.c
> @@ -473,6 +473,56 @@ const FrameData *packet_data_c(AVPacket *pkt)
> return ret < 0 ? NULL : (const FrameData*)pkt->opaque_ref->data;
> }
>
> +int check_avoptions_used(const AVDictionary *opts, const AVDictionary *opts_used,
> + void *logctx, int decode)
> +{
> + const AVClass *class = avcodec_get_class();
> + const AVClass *fclass = avformat_get_class();
> +
> + const int flag = decode ? AV_OPT_FLAG_DECODING_PARAM :
> + AV_OPT_FLAG_ENCODING_PARAM;
> + const AVDictionaryEntry *e = NULL;
> +
> + while ((e = av_dict_iterate(opts, e))) {
> + const AVOption *option, *foption;
> + char *optname, *p;
> +
> + if (av_dict_get(opts_used, e->key, NULL, 0))
> + continue;
> +
> + optname = av_strdup(e->key);
> + if (!optname)
> + return AVERROR(ENOMEM);
> +
> + p = strchr(optname, ':');
> + if (p)
> + *p = 0;
> +
> + option = av_opt_find(&class, optname, NULL, 0,
> + AV_OPT_SEARCH_CHILDREN | AV_OPT_SEARCH_FAKE_OBJ);
> + foption = av_opt_find(&fclass, optname, NULL, 0,
> + AV_OPT_SEARCH_CHILDREN | AV_OPT_SEARCH_FAKE_OBJ);
> + av_freep(&optname);
> + if (!option || foption)
> + continue;
> +
> + if (!(option->flags & flag)) {
> + av_log(logctx, AV_LOG_ERROR, "Codec AVOption %s (%s) is not a %s "
> + "option.\n", option->name, option->help ? option->help : "",
> + decode ? "decoding" : "encoding");
> + return AVERROR(EINVAL);
> + }
> +
> + av_log(logctx, AV_LOG_WARNING, "Codec AVOption %s (%s) has not been used "
> + "for any stream. The most likely reason is either wrong type "
> + "(e.g. a video option with no video streams) or that it is a "
> + "private option of some decoder which was not actually used "
> + "for any stream.\n", option->name, option->help ? option->help : "");
> + }
I commented the same thing for the earlier version of this patch, you
should keep using e->key instead of option->name in the messages.
Regards,
Marton
> +
> + return 0;
> +}
> +
> void update_benchmark(const char *fmt, ...)
> {
> if (do_benchmark_all) {
> diff --git a/fftools/ffmpeg.h b/fftools/ffmpeg.h
> index fe75706afd..8e849bbedc 100644
> --- a/fftools/ffmpeg.h
> +++ b/fftools/ffmpeg.h
> @@ -710,8 +710,10 @@ void term_exit(void);
>
> void show_usage(void);
>
> +int check_avoptions_used(const AVDictionary *opts, const AVDictionary *opts_used,
> + void *logctx, int decode);
> +
> int assert_file_overwrite(const char *filename);
> -AVDictionary *strip_specifiers(const AVDictionary *dict);
> int find_codec(void *logctx, const char *name,
> enum AVMediaType type, int encoder, const AVCodec **codec);
> int parse_and_set_vsync(const char *arg, int *vsync_var, int file_idx, int st_idx, int is_global);
> diff --git a/fftools/ffmpeg_demux.c b/fftools/ffmpeg_demux.c
> index 1ca8d804ae..3762d589e3 100644
> --- a/fftools/ffmpeg_demux.c
> +++ b/fftools/ffmpeg_demux.c
> @@ -1207,7 +1207,7 @@ static DemuxStream *demux_stream_alloc(Demuxer *d, AVStream *st)
> return ds;
> }
>
> -static int ist_add(const OptionsContext *o, Demuxer *d, AVStream *st)
> +static int ist_add(const OptionsContext *o, Demuxer *d, AVStream *st, AVDictionary **opts_used)
> {
> AVFormatContext *ic = d->f.ctx;
> AVCodecParameters *par = st->codecpar;
> @@ -1334,7 +1334,7 @@ static int ist_add(const OptionsContext *o, Demuxer *d, AVStream *st)
>
> if (ist->dec) {
> ret = filter_codec_opts(o->g->codec_opts, ist->st->codecpar->codec_id,
> - ic, st, ist->dec, &ds->decoder_opts);
> + ic, st, ist->dec, &ds->decoder_opts, opts_used);
> if (ret < 0)
> return ret;
> }
> @@ -1553,8 +1553,7 @@ int ifile_open(const OptionsContext *o, const char *filename, Scheduler *sch)
> const AVInputFormat *file_iformat = NULL;
> int err, i, ret = 0;
> int64_t timestamp;
> - AVDictionary *unused_opts = NULL;
> - const AVDictionaryEntry *e = NULL;
> + AVDictionary *opts_used = NULL;
> const char* video_codec_name = NULL;
> const char* audio_codec_name = NULL;
> const char* subtitle_codec_name = NULL;
> @@ -1826,48 +1825,21 @@ int ifile_open(const OptionsContext *o, const char *filename, Scheduler *sch)
>
> /* Add all the streams from the given input file to the demuxer */
> for (int i = 0; i < ic->nb_streams; i++) {
> - ret = ist_add(o, d, ic->streams[i]);
> - if (ret < 0)
> + ret = ist_add(o, d, ic->streams[i], &opts_used);
> + if (ret < 0) {
> + av_dict_free(&opts_used);
> return ret;
> + }
> }
>
> /* dump the file content */
> av_dump_format(ic, f->index, filename, 0);
>
> /* check if all codec options have been used */
> - unused_opts = strip_specifiers(o->g->codec_opts);
> - for (i = 0; i < f->nb_streams; i++) {
> - DemuxStream *ds = ds_from_ist(f->streams[i]);
> - e = NULL;
> - while ((e = av_dict_iterate(ds->decoder_opts, e)))
> - av_dict_set(&unused_opts, e->key, NULL, 0);
> - }
> -
> - e = NULL;
> - while ((e = av_dict_iterate(unused_opts, e))) {
> - const AVClass *class = avcodec_get_class();
> - const AVOption *option = av_opt_find(&class, e->key, NULL, 0,
> - AV_OPT_SEARCH_CHILDREN | AV_OPT_SEARCH_FAKE_OBJ);
> - const AVClass *fclass = avformat_get_class();
> - const AVOption *foption = av_opt_find(&fclass, e->key, NULL, 0,
> - AV_OPT_SEARCH_CHILDREN | AV_OPT_SEARCH_FAKE_OBJ);
> - if (!option || foption)
> - continue;
> -
> -
> - if (!(option->flags & AV_OPT_FLAG_DECODING_PARAM)) {
> - av_log(d, AV_LOG_ERROR, "Codec AVOption %s (%s) is not a decoding "
> - "option.\n", e->key, option->help ? option->help : "");
> - return AVERROR(EINVAL);
> - }
> -
> - av_log(d, AV_LOG_WARNING, "Codec AVOption %s (%s) has not been used "
> - "for any stream. The most likely reason is either wrong type "
> - "(e.g. a video option with no video streams) or that it is a "
> - "private option of some decoder which was not actually used "
> - "for any stream.\n", e->key, option->help ? option->help : "");
> - }
> - av_dict_free(&unused_opts);
> + ret = check_avoptions_used(o->g->codec_opts, opts_used, d, 1);
> + av_dict_free(&opts_used);
> + if (ret < 0)
> + return ret;
>
> for (i = 0; i < o->dump_attachment.nb_opt; i++) {
> int j;
> diff --git a/fftools/ffmpeg_mux.c b/fftools/ffmpeg_mux.c
> index a1583edd61..055e2f3678 100644
> --- a/fftools/ffmpeg_mux.c
> +++ b/fftools/ffmpeg_mux.c
> @@ -865,6 +865,7 @@ void of_free(OutputFile **pof)
> av_freep(&mux->sch_stream_idx);
>
> av_dict_free(&mux->opts);
> + av_dict_free(&mux->enc_opts_used);
>
> av_packet_free(&mux->sq_pkt);
>
> diff --git a/fftools/ffmpeg_mux.h b/fftools/ffmpeg_mux.h
> index 1e9ea35412..1c1b407484 100644
> --- a/fftools/ffmpeg_mux.h
> +++ b/fftools/ffmpeg_mux.h
> @@ -99,6 +99,9 @@ typedef struct Muxer {
>
> AVDictionary *opts;
>
> + // used to validate that all encoder avoptions have been actually used
> + AVDictionary *enc_opts_used;
> +
> /* filesize limit expressed in bytes */
> int64_t limit_filesize;
> atomic_int_least64_t last_filesize;
> diff --git a/fftools/ffmpeg_mux_init.c b/fftools/ffmpeg_mux_init.c
> index 1f19d9283a..b1250d668f 100644
> --- a/fftools/ffmpeg_mux_init.c
> +++ b/fftools/ffmpeg_mux_init.c
> @@ -1160,7 +1160,8 @@ static int ost_add(Muxer *mux, const OptionsContext *o, enum AVMediaType type,
> const char *enc_time_base = NULL;
>
> ret = filter_codec_opts(o->g->codec_opts, enc->codec_id,
> - oc, st, enc->codec, &ost->encoder_opts);
> + oc, st, enc->codec, &ost->encoder_opts,
> + &mux->enc_opts_used);
> if (ret < 0)
> return ret;
>
> @@ -1265,7 +1266,8 @@ static int ost_add(Muxer *mux, const OptionsContext *o, enum AVMediaType type,
> }
> } else {
> ret = filter_codec_opts(o->g->codec_opts, AV_CODEC_ID_NONE, oc, st,
> - NULL, &ost->encoder_opts);
> + NULL, &ost->encoder_opts,
> + &mux->enc_opts_used);
> if (ret < 0)
> return ret;
> }
> @@ -3118,53 +3120,6 @@ static int process_forced_keyframes(Muxer *mux, const OptionsContext *o)
> return 0;
> }
>
> -static int validate_enc_avopt(Muxer *mux, const AVDictionary *codec_avopt)
> -{
> - const AVClass *class = avcodec_get_class();
> - const AVClass *fclass = avformat_get_class();
> - const OutputFile *of = &mux->of;
> -
> - AVDictionary *unused_opts;
> - const AVDictionaryEntry *e;
> -
> - unused_opts = strip_specifiers(codec_avopt);
> - for (int i = 0; i < of->nb_streams; i++) {
> - e = NULL;
> - while ((e = av_dict_iterate(of->streams[i]->encoder_opts, e)))
> - av_dict_set(&unused_opts, e->key, NULL, 0);
> - }
> -
> - e = NULL;
> - while ((e = av_dict_iterate(unused_opts, e))) {
> - const AVOption *option = av_opt_find(&class, e->key, NULL, 0,
> - AV_OPT_SEARCH_CHILDREN | AV_OPT_SEARCH_FAKE_OBJ);
> - const AVOption *foption = av_opt_find(&fclass, e->key, NULL, 0,
> - AV_OPT_SEARCH_CHILDREN | AV_OPT_SEARCH_FAKE_OBJ);
> - if (!option || foption)
> - continue;
> -
> - if (!(option->flags & AV_OPT_FLAG_ENCODING_PARAM)) {
> - av_log(mux, AV_LOG_ERROR, "Codec AVOption %s (%s) is not an "
> - "encoding option.\n", e->key, option->help ? option->help : "");
> - av_dict_free(&unused_opts);
> - return AVERROR(EINVAL);
> - }
> -
> - // gop_timecode is injected by generic code but not always used
> - if (!strcmp(e->key, "gop_timecode"))
> - continue;
> -
> - av_log(mux, AV_LOG_WARNING, "Codec AVOption %s (%s) has not been used "
> - "for any stream. The most likely reason is either wrong type "
> - "(e.g. a video option with no video streams) or that it is a "
> - "private option of some encoder which was not actually used for "
> - "any stream.\n", e->key, option->help ? option->help : "");
> - }
> - av_dict_free(&unused_opts);
> -
> - return 0;
> -}
> -
> static const char *output_file_item_name(void *obj)
> {
> const Muxer *mux = obj;
> @@ -3272,7 +3227,8 @@ int of_open(const OptionsContext *o, const char *filename, Scheduler *sch)
> return err;
>
> /* check if all codec options have been used */
> - err = validate_enc_avopt(mux, o->g->codec_opts);
> + err = check_avoptions_used(o->g->codec_opts, mux->enc_opts_used, mux, 0);
> + av_dict_free(&mux->enc_opts_used);
> if (err < 0)
> return err;
>
> diff --git a/fftools/ffmpeg_opt.c b/fftools/ffmpeg_opt.c
> index 910e4a336b..b256fc9372 100644
> --- a/fftools/ffmpeg_opt.c
> +++ b/fftools/ffmpeg_opt.c
> @@ -151,24 +151,6 @@ static int show_hwaccels(void *optctx, const char *opt, const char *arg)
> return 0;
> }
>
> -/* return a copy of the input with the stream specifiers removed from the keys */
> -AVDictionary *strip_specifiers(const AVDictionary *dict)
> -{
> - const AVDictionaryEntry *e = NULL;
> - AVDictionary *ret = NULL;
> -
> - while ((e = av_dict_iterate(dict, e))) {
> - char *p = strchr(e->key, ':');
> -
> - if (p)
> - *p = 0;
> - av_dict_set(&ret, e->key, e->value, 0);
> - if (p)
> - *p = ':';
> - }
> - return ret;
> -}
> -
> const char *opt_match_per_type_str(const SpecifierOptList *sol,
> char mediatype)
> {
> diff --git a/fftools/ffplay.c b/fftools/ffplay.c
> index 1d0511b254..30719e9417 100644
> --- a/fftools/ffplay.c
> +++ b/fftools/ffplay.c
> @@ -2673,7 +2673,7 @@ static int stream_component_open(VideoState *is, int stream_index)
> avctx->flags2 |= AV_CODEC_FLAG2_FAST;
>
> ret = filter_codec_opts(codec_opts, avctx->codec_id, ic,
> - ic->streams[stream_index], codec, &opts);
> + ic->streams[stream_index], codec, &opts, NULL);
> if (ret < 0)
> goto fail;
>
> diff --git a/fftools/ffprobe.c b/fftools/ffprobe.c
> index d7ba980ff9..e7e27d31ba 100644
> --- a/fftools/ffprobe.c
> +++ b/fftools/ffprobe.c
> @@ -3927,7 +3927,7 @@ static int open_input_file(InputFile *ifile, const char *filename,
> AVDictionary *opts;
>
> err = filter_codec_opts(codec_opts, stream->codecpar->codec_id,
> - fmt_ctx, stream, codec, &opts);
> + fmt_ctx, stream, codec, &opts, NULL);
> if (err < 0)
> exit(1);
>
> --
> 2.43.0
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".
>
More information about the ffmpeg-devel
mailing list