diff mbox series

[FFmpeg-devel,RFC] User-defined default enc/dec/mux/dem/etc

Message ID 84557813-39af-2393-ccb1-6479318e50ab@mail.de
State New
Headers show
Series [FFmpeg-devel,RFC] User-defined default enc/dec/mux/dem/etc | expand

Checks

Context Check Description
yinshiyou/configure_loongarch64 warning Failed to apply patch
andriy/commit_msg_armv7_RPi4 warning The first line of the commit message must start with a context terminated by a colon and a space, for example "lavu/opt: " or "doc: ".
andriy/make_armv7_RPi4 success Make finished
andriy/make_fate_armv7_RPi4 success Make fate finished
andriy/commit_msg_x86 warning The first line of the commit message must start with a context terminated by a colon and a space, for example "lavu/opt: " or "doc: ".
andriy/make_x86 success Make finished
andriy/make_fate_x86 success Make fate finished

Commit Message

Thilo Borgmann June 7, 2022, 1:25 p.m. UTC
Hi,

currently, if -c:{a,v} is not given, the list of all codecs is successively searched until the first enc/dec is found.

If I have two or more enc/dec's for the same codec ID (like by having libx264, libfdk-aac, etc..) it will still be the first in the list that is found that is used if no -c:{v,a} is given. Having a list of user-defined default enc/dec's avoids having the user to always specify their favorite via -c:{v,a}.

This patch creates a (redundant) list of default codecs user-defined via configure options. This list is then searched before the complete list of codecs is searched so that all user defaults will be found first. An alternative would be messing with the order of codecs during configure and creation of lavc/codec_list.c to have the defaults found first - which I think is not a good idea. Maybe do something else entirely instead...?

For the case you have hwaccel's for the codec ID in question, a default specified currently uses the hwaccel-enc/-dec even if no "-hwaccel something" is given on the command line. What would we want to happen? Stick to use only if given like auto hwaccel decoding?

The patch is of course dirty/wip and my shell-Fu is for sure not sufficient to our standards. Anyways, might also be useful to have this for mux/demux, if this is not a horrible idea for some reason to start with?

Any comments appreciated before I'd forge it into an actual patch!

Thanks,
Thilo
From c32f4e9ce13685690543d2d295a5edcc39b74899 Mon Sep 17 00:00:00 2001
From: Thilo Borgmann <thilo.borgmann@mail.de>
Date: Tue, 7 Jun 2022 11:33:28 +0200
Subject: [PATCH] defaults

---
 configure              | 48 ++++++++++++++++++++++++++++++++++++++++++
 libavcodec/allcodecs.c | 34 ++++++++++++++++++++++++++++++
 libavcodec/codec.h     | 11 ++++++++++
 3 files changed, 93 insertions(+)

Comments

Nicolas George June 8, 2022, 6:51 a.m. UTC | #1
Thilo Borgman (12022-06-07):
> Hi,
> 
> currently, if -c:{a,v} is not given, the list of all codecs is successively searched until the first enc/dec is found.
> 
> If I have two or more enc/dec's for the same codec ID (like by having libx264, libfdk-aac, etc..) it will still be the first in the list that is found that is used if no -c:{v,a} is given. Having a list of user-defined default enc/dec's avoids having the user to always specify their favorite via -c:{v,a}.
> 
> This patch creates a (redundant) list of default codecs user-defined via configure options. This list is then searched before the complete list of codecs is searched so that all user defaults will be found first. An alternative would be messing with the order of codecs during configure and creation of lavc/codec_list.c to have the defaults found first - which I think is not a good idea. Maybe do something else entirely instead...?
> 
> For the case you have hwaccel's for the codec ID in question, a default specified currently uses the hwaccel-enc/-dec even if no "-hwaccel something" is given on the command line. What would we want to happen? Stick to use only if given like auto hwaccel decoding?
> 
> The patch is of course dirty/wip and my shell-Fu is for sure not sufficient to our standards. Anyways, might also be useful to have this for mux/demux, if this is not a horrible idea for some reason to start with?
> 
> Any comments appreciated before I'd forge it into an actual patch!

From a user support perspective, I think it is not a good idea at all:
it makes it harder to spot problems with users commands when some of the
effects are hidden in the huge configure line.

Even from a usability perspective I thin it is not good: just because a
BOFH or a quirky package maintainer chose to select different defaults,
many examples, many stackoverflow answers stop working.

Better teach users to write explicit settings. They can write shell
scripts to make it permanent.

Regards,
diff mbox series

Patch

diff --git a/configure b/configure
index 196873c4aa..6053c59078 100755
--- a/configure
+++ b/configure
@@ -4046,6 +4046,19 @@  done
 
 enable $ARCH_EXT_LIST
 
+ENCODER_DEFAULT_LIST=
+DECODER_DEFAULT_LIST=
+
+default_encoder(){
+    enabled $1 &&
+    ENCODER_DEFAULT_LIST="$ENCODER_DEFAULT_LIST $1"
+}
+
+default_decoder(){
+    enabled $1 &&
+    DECODER_DEFAULT_LIST="$DECODER_DEFAULT_LIST $1"
+}
+
 die_unknown(){
     echo "Unknown option \"$1\"."
     echo "See $0 --help for available options."
@@ -4156,6 +4169,16 @@  for opt do
             [ "$list" = "" ] && warn "Option $opt did not match anything"
             test $action = enable && warn_if_gets_disabled $list
             $action $list
+        ;;
+         --default-*=*)
+            eval $(echo "${opt%%=*}" | sed 's/--/action=/;s/-/ thing=/')
+            is_in "${thing}s" $COMPONENT_LIST || die_unknown "$opt"
+            eval list=\$$(toupper $thing)_LIST
+            name=$(echo "${optval}" | sed "s/,/_${thing}|/g")_${thing}
+            list=$(filter "$name" $list)
+            [ "$list" = "" ] && warn "Option $opt did not match anything"
+            warn_if_gets_disabled $list
+            ${action}_${thing} $list
         ;;
         --enable-yasm|--disable-yasm)
             warn "The ${opt} option is only provided for compatibility and will be\n"\
@@ -7597,6 +7620,30 @@  if test -n "$WARNINGS"; then
     enabled fatal_warnings && exit 1
 fi
 
+# remove disabled defaults from list
+check_encoder_default_list(){
+    ENCODER_DEFAULT_LIST=""
+    for def in "$@"; do
+        enabled $def && ENCODER_DEFAULT_LIST="$ENCODER_DEFAULT_LIST $def"
+    done
+}
+
+check_decoder_default_list(){
+    DECODER_DEFAULT_LIST=""
+    for def in "$@"; do
+        enabled $def && DECODER_DEFAULT_LIST="$DECODER_DEFAULT_LIST $def"
+    done
+}
+
+check_encoder_default_list $ENCODER_DEFAULT_LIST
+check_decoder_default_list $DECODER_DEFAULT_LIST
+
+CODEC_DEFAULT_LIST="
+    $ENCODER_DEFAULT_LIST
+    $DECODER_DEFAULT_LIST
+"
+echo "CODEC_DEFAULT_LIST=" $CODEC_DEFAULT_LIST
+
 test -e Makefile || echo "include $source_path/Makefile" > Makefile
 
 esc(){
@@ -7883,6 +7930,7 @@  print_enabled_components(){
 
 print_enabled_components libavfilter/filter_list.c AVFilter filter_list $FILTER_LIST
 print_enabled_components libavcodec/codec_list.c FFCodec codec_list $CODEC_LIST
+print_enabled_components libavcodec/codec_default_list.c FFCodec codec_default_list $CODEC_DEFAULT_LIST
 print_enabled_components libavcodec/parser_list.c AVCodecParser parser_list $PARSER_LIST
 print_enabled_components libavcodec/bsf_list.c FFBitStreamFilter bitstream_filters $BSF_LIST
 print_enabled_components libavformat/demuxer_list.c AVInputFormat demuxer_list $DEMUXER_LIST
diff --git a/libavcodec/allcodecs.c b/libavcodec/allcodecs.c
index c47133aa18..1a86ecf353 100644
--- a/libavcodec/allcodecs.c
+++ b/libavcodec/allcodecs.c
@@ -866,8 +866,14 @@  const FFCodec * codec_list[] = {
     NULL,
     NULL
 };
+const FFCodec * codec_default_list[] = {
+    NULL,
+    NULL,
+    NULL
+};
 #else
 #include "libavcodec/codec_list.c"
+#include "libavcodec/codec_default_list.c"
 #endif
 
 static AVOnce av_codec_static_init = AV_ONCE_INIT;
@@ -893,6 +899,21 @@  const AVCodec *av_codec_iterate(void **opaque)
     return NULL;
 }
 
+const AVCodec *av_codec_iterate2(const void **list, void **opaque)
+{
+    uintptr_t i = (uintptr_t)*opaque;
+    const FFCodec **codecs = (const FFCodec**)list;
+    const FFCodec *c = codecs[i];
+
+    ff_thread_once(&av_codec_static_init, av_codec_init_static);
+
+    if (c) {
+        *opaque = (void*)(i + 1);
+        return &c->p;
+    }
+    return NULL;
+}
+
 static enum AVCodecID remap_deprecated_codec_id(enum AVCodecID id)
 {
     switch(id){
@@ -909,6 +930,19 @@  static const AVCodec *find_codec(enum AVCodecID id, int (*x)(const AVCodec *))
 
     id = remap_deprecated_codec_id(id);
 
+    while ((p = av_codec_iterate2((const void**)codec_default_list, &i))) {
+        if (!x(p))
+            continue;
+        if (p->id == id) {
+            if (p->capabilities & AV_CODEC_CAP_EXPERIMENTAL && !experimental) {
+                experimental = p;
+            } else
+                return p;
+        }
+    }
+
+    i = 0;
+
     while ((p = av_codec_iterate(&i))) {
         if (!x(p))
             continue;
diff --git a/libavcodec/codec.h b/libavcodec/codec.h
index 03e8be90a2..f9cb40bda4 100644
--- a/libavcodec/codec.h
+++ b/libavcodec/codec.h
@@ -257,6 +257,17 @@  typedef struct AVCodec {
  */
 const AVCodec *av_codec_iterate(void **opaque);
 
+/**
+ * Iterate over all registered codecs in a given array of codecs.
+ *
+ * @param opaque a pointer where libavcodec will store the iteration state. Must
+ *               point to NULL to start the iteration.
+ *
+ * @return the next default codec or NULL when the iteration is
+ *         finished
+ */
+const AVCodec *av_codec_iterate2(const void **list, void **opaque);
+
 /**
  * Find a registered decoder with a matching codec ID.
  *