diff mbox

[FFmpeg-devel,1/2] Move av_show_help_children() to avutil/opt

Message ID 20161104201636.22631-2-michael@niedermayer.cc
State New
Headers show

Commit Message

Michael Niedermayer Nov. 4, 2016, 8:16 p.m. UTC
Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
---
 cmdutils.c      | 20 ++++----------------
 cmdutils.h      |  5 -----
 ffmpeg_opt.c    | 10 +++++-----
 ffplay.c        |  8 ++++----
 ffprobe.c       |  2 +-
 libavutil/opt.c | 12 ++++++++++++
 libavutil/opt.h |  7 +++++++
 7 files changed, 33 insertions(+), 31 deletions(-)

Comments

Andreas Cadhalpun Nov. 7, 2016, 7:05 p.m. UTC | #1
On 04.11.2016 21:16, Michael Niedermayer wrote:
> diff --git a/libavutil/opt.c b/libavutil/opt.c
> index cd16bd1..1b8dae2 100644
> --- a/libavutil/opt.c
> +++ b/libavutil/opt.c
> @@ -1992,3 +1992,15 @@ int av_opt_serialize(void *obj, int opt_flags, int flags, char **buffer,
>      av_bprint_finalize(&bprint, buffer);
>      return 0;
>  }
> +
> +void av_show_help_children(const AVClass *class, int flags)
> +{
> +    const AVClass *child = NULL;
> +    if (class->option) {
> +        av_opt_show2(&class, NULL, flags, 0);
> +        printf("\n");

The libraries must not use printf directly, because unlike av_log it
can't be intercepted by API users. This applies also to the second patch.

On 04.11.2016 21:16, Michael Niedermayer wrote:
> This patch-set is also required for moving ffserver into a separate
> repository as it too uses these functions and they otherwise would need to
> be duplicated.

I'm not sure moving ffserver to a separate repository is very useful.
Its problem is that it uses private functions, which has to be fixed anyway.
However, moving it to a separate repository makes it impossible to test it
with FATE in order to prevent regressions.

Best regards,
Andreas
James Almer Nov. 7, 2016, 7:26 p.m. UTC | #2
On 11/7/2016 4:05 PM, Andreas Cadhalpun wrote:
> On 04.11.2016 21:16, Michael Niedermayer wrote:
>> diff --git a/libavutil/opt.c b/libavutil/opt.c
>> index cd16bd1..1b8dae2 100644
>> --- a/libavutil/opt.c
>> +++ b/libavutil/opt.c
>> @@ -1992,3 +1992,15 @@ int av_opt_serialize(void *obj, int opt_flags, int flags, char **buffer,
>>      av_bprint_finalize(&bprint, buffer);
>>      return 0;
>>  }
>> +
>> +void av_show_help_children(const AVClass *class, int flags)
>> +{
>> +    const AVClass *child = NULL;
>> +    if (class->option) {
>> +        av_opt_show2(&class, NULL, flags, 0);
>> +        printf("\n");
> 
> The libraries must not use printf directly, because unlike av_log it
> can't be intercepted by API users. This applies also to the second patch.
> 
> On 04.11.2016 21:16, Michael Niedermayer wrote:
>> This patch-set is also required for moving ffserver into a separate
>> repository as it too uses these functions and they otherwise would need to
>> be duplicated.
> 
> I'm not sure moving ffserver to a separate repository is very useful.
> Its problem is that it uses private functions, which has to be fixed anyway.
> However, moving it to a separate repository makes it impossible to test it
> with FATE in order to prevent regressions.

ffserver is being dropped from the project, as announced in the news page.

Reynaldo however wants to have a working copy somewhere so he's moving it to
an external repo and for that he's making it work without internal API, with
some help from Michael it seems.

> 
> Best regards,
> Andreas
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
Andreas Cadhalpun Nov. 7, 2016, 9:34 p.m. UTC | #3
On 07.11.2016 20:26, James Almer wrote:
> ffserver is being dropped from the project, as announced in the news page.
> 
> Reynaldo however wants to have a working copy somewhere so he's moving it to
> an external repo and for that he's making it work without internal API, with
> some help from Michael it seems.

Isn't the causality the other way around?
Because ffserver uses internal API it was decided to remove it.
If it gets fixed, there is no point in removing it, or is there?

Best regards,
Andreas
James Almer Nov. 7, 2016, 9:41 p.m. UTC | #4
On 11/7/2016 6:34 PM, Andreas Cadhalpun wrote:
> On 07.11.2016 20:26, James Almer wrote:
>> ffserver is being dropped from the project, as announced in the news page.
>>
>> Reynaldo however wants to have a working copy somewhere so he's moving it to
>> an external repo and for that he's making it work without internal API, with
>> some help from Michael it seems.
> 
> Isn't the causality the other way around?
> Because ffserver uses internal API it was decided to remove it.
> If it gets fixed, there is no point in removing it, or is there?

Removing ffserver was decided for several reasons, as mentioned in the news entry,
with one of them being the internal API usage. That stuff being fixed now is the
choice of a dev after the removal decision was made, since it was in his interests
to keep a working copy somewhere else.
Michael Niedermayer Nov. 7, 2016, 10 p.m. UTC | #5
On Mon, Nov 07, 2016 at 10:34:47PM +0100, Andreas Cadhalpun wrote:
> On 07.11.2016 20:26, James Almer wrote:
> > ffserver is being dropped from the project, as announced in the news page.
> > 
> > Reynaldo however wants to have a working copy somewhere so he's moving it to
> > an external repo and for that he's making it work without internal API, with
> > some help from Michael it seems.
> 
> Isn't the causality the other way around?
> Because ffserver uses internal API it was decided to remove it.
> If it gets fixed, there is no point in removing it, or is there?

+1

[...]
Reynaldo H. Verdejo Pinochet Nov. 7, 2016, 10:21 p.m. UTC | #6
Hi

On 11/07/2016 01:34 PM, Andreas Cadhalpun wrote:
> [...]
> Isn't the causality the other way around?
> Because ffserver uses internal API it was decided to remove it.
> If it gets fixed, there is no point in removing it, or is there?
>[..]

I see no reason to remove it if it works properly.

Bests,
diff mbox

Patch

diff --git a/cmdutils.c b/cmdutils.c
index 44fe64c..9f58b14 100644
--- a/cmdutils.c
+++ b/cmdutils.c
@@ -202,18 +202,6 @@  void show_help_options(const OptionDef *options, const char *msg, int req_flags,
     printf("\n");
 }
 
-void show_help_children(const AVClass *class, int flags)
-{
-    const AVClass *child = NULL;
-    if (class->option) {
-        av_opt_show2(&class, NULL, flags, 0);
-        printf("\n");
-    }
-
-    while (child = av_opt_child_class_next(class, child))
-        show_help_children(child, flags);
-}
-
 static const OptionDef *find_option(const OptionDef *po, const char *name)
 {
     const char *p = strchr(name, ':');
@@ -1421,7 +1409,7 @@  static void print_codec(const AVCodec *c)
                           0, GET_CH_LAYOUT_DESC);
 
     if (c->priv_class) {
-        show_help_children(c->priv_class,
+        av_show_help_children(c->priv_class,
                            AV_OPT_FLAG_ENCODING_PARAM |
                            AV_OPT_FLAG_DECODING_PARAM);
     }
@@ -1805,7 +1793,7 @@  static void show_help_demuxer(const char *name)
         printf("    Common extensions: %s.\n", fmt->extensions);
 
     if (fmt->priv_class)
-        show_help_children(fmt->priv_class, AV_OPT_FLAG_DECODING_PARAM);
+        av_show_help_children(fmt->priv_class, AV_OPT_FLAG_DECODING_PARAM);
 }
 
 static void show_help_muxer(const char *name)
@@ -1838,7 +1826,7 @@  static void show_help_muxer(const char *name)
     }
 
     if (fmt->priv_class)
-        show_help_children(fmt->priv_class, AV_OPT_FLAG_ENCODING_PARAM);
+        av_show_help_children(fmt->priv_class, AV_OPT_FLAG_ENCODING_PARAM);
 }
 
 #if CONFIG_AVFILTER
@@ -1886,7 +1874,7 @@  static void show_help_filter(const char *name)
         printf("        none (sink filter)\n");
 
     if (f->priv_class)
-        show_help_children(f->priv_class, AV_OPT_FLAG_VIDEO_PARAM | AV_OPT_FLAG_FILTERING_PARAM |
+        av_show_help_children(f->priv_class, AV_OPT_FLAG_VIDEO_PARAM | AV_OPT_FLAG_FILTERING_PARAM |
                                           AV_OPT_FLAG_AUDIO_PARAM);
     if (f->flags & AVFILTER_FLAG_SUPPORT_TIMELINE)
         printf("This filter has support for timeline through the 'enable' option.\n");
diff --git a/cmdutils.h b/cmdutils.h
index e75d8d3..38589cd 100644
--- a/cmdutils.h
+++ b/cmdutils.h
@@ -206,11 +206,6 @@  typedef struct OptionDef {
 void show_help_options(const OptionDef *options, const char *msg, int req_flags,
                        int rej_flags, int alt_flags);
 
-/**
- * Show help for all options with given flags in class and all its
- * children.
- */
-void show_help_children(const AVClass *class, int flags);
 
 /**
  * Per-fftool specific help handler. Implemented in each
diff --git a/ffmpeg_opt.c b/ffmpeg_opt.c
index 6346ecf..343453f 100644
--- a/ffmpeg_opt.c
+++ b/ffmpeg_opt.c
@@ -3101,13 +3101,13 @@  void show_help_default(const char *opt, const char *arg)
 
     if (show_avoptions) {
         int flags = AV_OPT_FLAG_DECODING_PARAM | AV_OPT_FLAG_ENCODING_PARAM;
-        show_help_children(avcodec_get_class(), flags);
-        show_help_children(avformat_get_class(), flags);
+        av_show_help_children(avcodec_get_class(), flags);
+        av_show_help_children(avformat_get_class(), flags);
 #if CONFIG_SWSCALE
-        show_help_children(sws_get_class(), flags);
+        av_show_help_children(sws_get_class(), flags);
 #endif
-        show_help_children(swr_get_class(), AV_OPT_FLAG_AUDIO_PARAM);
-        show_help_children(avfilter_get_class(), AV_OPT_FLAG_VIDEO_PARAM | AV_OPT_FLAG_AUDIO_PARAM | AV_OPT_FLAG_FILTERING_PARAM);
+        av_show_help_children(swr_get_class(), AV_OPT_FLAG_AUDIO_PARAM);
+        av_show_help_children(avfilter_get_class(), AV_OPT_FLAG_VIDEO_PARAM | AV_OPT_FLAG_AUDIO_PARAM | AV_OPT_FLAG_FILTERING_PARAM);
     }
 }
 
diff --git a/ffplay.c b/ffplay.c
index 79dc768..6b83ba2 100644
--- a/ffplay.c
+++ b/ffplay.c
@@ -3617,12 +3617,12 @@  void show_help_default(const char *opt, const char *arg)
     show_help_options(options, "Main options:", 0, OPT_EXPERT, 0);
     show_help_options(options, "Advanced options:", OPT_EXPERT, 0, 0);
     printf("\n");
-    show_help_children(avcodec_get_class(), AV_OPT_FLAG_DECODING_PARAM);
-    show_help_children(avformat_get_class(), AV_OPT_FLAG_DECODING_PARAM);
+    av_show_help_children(avcodec_get_class(), AV_OPT_FLAG_DECODING_PARAM);
+    av_show_help_children(avformat_get_class(), AV_OPT_FLAG_DECODING_PARAM);
 #if !CONFIG_AVFILTER
-    show_help_children(sws_get_class(), AV_OPT_FLAG_ENCODING_PARAM);
+    av_show_help_children(sws_get_class(), AV_OPT_FLAG_ENCODING_PARAM);
 #else
-    show_help_children(avfilter_get_class(), AV_OPT_FLAG_FILTERING_PARAM);
+    av_show_help_children(avfilter_get_class(), AV_OPT_FLAG_FILTERING_PARAM);
 #endif
     printf("\nWhile playing:\n"
            "q, ESC              quit\n"
diff --git a/ffprobe.c b/ffprobe.c
index a2980b3..6492f6e 100644
--- a/ffprobe.c
+++ b/ffprobe.c
@@ -2979,7 +2979,7 @@  void show_help_default(const char *opt, const char *arg)
     show_help_options(options, "Main options:", 0, 0, 0);
     printf("\n");
 
-    show_help_children(avformat_get_class(), AV_OPT_FLAG_DECODING_PARAM);
+    av_show_help_children(avformat_get_class(), AV_OPT_FLAG_DECODING_PARAM);
 }
 
 /**
diff --git a/libavutil/opt.c b/libavutil/opt.c
index cd16bd1..1b8dae2 100644
--- a/libavutil/opt.c
+++ b/libavutil/opt.c
@@ -1992,3 +1992,15 @@  int av_opt_serialize(void *obj, int opt_flags, int flags, char **buffer,
     av_bprint_finalize(&bprint, buffer);
     return 0;
 }
+
+void av_show_help_children(const AVClass *class, int flags)
+{
+    const AVClass *child = NULL;
+    if (class->option) {
+        av_opt_show2(&class, NULL, flags, 0);
+        printf("\n");
+    }
+
+    while (child = av_opt_child_class_next(class, child))
+        av_show_help_children(child, flags);
+}
diff --git a/libavutil/opt.h b/libavutil/opt.h
index 9430b98..2379662 100644
--- a/libavutil/opt.h
+++ b/libavutil/opt.h
@@ -858,6 +858,13 @@  int av_opt_is_set_to_default_by_name(void *obj, const char *name, int search_fla
  */
 int av_opt_serialize(void *obj, int opt_flags, int flags, char **buffer,
                      const char key_val_sep, const char pairs_sep);
+
+/**
+ * Show help for all options with given flags in class and all its
+ * children.
+ */
+void av_show_help_children(const AVClass *class, int flags);
+
 /**
  * @}
  */