Re: [FFmpeg-devel] [PATCH 1/3] fftools/ffmpeg: rewrite checking whether codec AVOptions have been used

[Marton Balint <cus@passwd.hu>]

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@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
>
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".