From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from ffbox0-bg.mplayerhq.hu (ffbox0-bg.ffmpeg.org [79.124.17.100]) by master.gitmailbox.com (Postfix) with ESMTP id 2802F4B0F0 for ; Thu, 27 Jun 2024 18:36:43 +0000 (UTC) Received: from [127.0.1.1] (localhost [127.0.0.1]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTP id 04F9168D51F; Thu, 27 Jun 2024 21:36:41 +0300 (EEST) Received: from iq.passwd.hu (iq.passwd.hu [217.27.212.140]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTP id E5B8C68D09E for ; Thu, 27 Jun 2024 21:36:34 +0300 (EEST) Received: from localhost (localhost [127.0.0.1]) by iq.passwd.hu (Postfix) with ESMTP id 5D85FEA817 for ; Thu, 27 Jun 2024 20:36:34 +0200 (CEST) X-Virus-Scanned: amavisd-new at passwd.hu Received: from iq.passwd.hu ([127.0.0.1]) by localhost (iq.passwd.hu [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id HOFMW5gHK8BI for ; Thu, 27 Jun 2024 20:36:31 +0200 (CEST) Received: from iq (iq [217.27.212.140]) by iq.passwd.hu (Postfix) with ESMTPS id D6FB8EA807 for ; Thu, 27 Jun 2024 20:36:30 +0200 (CEST) Date: Thu, 27 Jun 2024 20:36:30 +0200 (CEST) From: Marton Balint To: FFmpeg development discussions and patches In-Reply-To: <20240627084402.32474-1-anton@khirnov.net> Message-ID: <0ae39634-57e7-67e3-47b6-f63a56d062d6@passwd.hu> References: <20240627084402.32474-1-anton@khirnov.net> MIME-Version: 1.0 Subject: Re: [FFmpeg-devel] [PATCH 1/3] fftools/ffmpeg: rewrite checking whether codec AVOptions have been used X-BeenThere: ffmpeg-devel@ffmpeg.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: FFmpeg development discussions and patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Reply-To: FFmpeg development discussions and patches Content-Transfer-Encoding: 7bit Content-Type: text/plain; charset="us-ascii"; Format="flowed" Errors-To: ffmpeg-devel-bounces@ffmpeg.org Sender: "ffmpeg-devel" Archived-At: List-Archive: List-Post: 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".