diff mbox series

[FFmpeg-devel,v3,2/2] avdevice/lavf: change the dumpgraph option to bool type

Message ID 1589900073-4183-2-git-send-email-lance.lmwang@gmail.com
State New
Headers show
Series [FFmpeg-devel,v3,1/2] fftools: add global option to dump filter graph to stderr | expand

Checks

Context Check Description
andriy/default pending
andriy/make success Make finished
andriy/make_fate success Make fate finished

Commit Message

Lance Wang May 19, 2020, 2:54 p.m. UTC 
From: Limin Wang <lance.lmwang@gmail.com>

dumpgraph option currently uses string types, but actually only requires bool type

Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
---
 doc/indevs.texi     | 2 +-
 libavdevice/lavfi.c | 6 +++---
 2 files changed, 4 insertions(+), 4 deletions(-)

Comments

Nicolas George May 19, 2020, 2:58 p.m. UTC | #1 
lance.lmwang@gmail.com (12020-05-19):
> From: Limin Wang <lance.lmwang@gmail.com>
> 
> dumpgraph option currently uses string types, but actually only requires bool type

Originally, the second parameter to avfilter_graph_dump() was supposed
to be dump options. None was ever added, and I later came to realize
that using a string for that was more of the same bad idea.

I have no objection to making this bit obsolete, but please do it
knowingly.

Regards,
Nicolas George May 19, 2020, 3:02 p.m. UTC | #2 
Nicolas George (12020-05-19):
> Originally, the second parameter to avfilter_graph_dump() was supposed
> to be dump options. None was ever added, and I later came to realize
> that using a string for that was more of the same bad idea.
> 
> I have no objection to making this bit obsolete, but please do it
> knowingly.

Again, forgot something before sending:

The idea was to be able to output other kinds of graph representation.
The current one is a very clumsy attempt at ASCII-art. But we could have
added:

	avfilter_graph_dump(graph, "fmt=dot");

to get output in Graphviz DOT format, basically importing
tools/graph2dot.c into libavfilter/graphdump.c. Finally doing that may
be a better idea than obsoleting the option.

And of course, other formats are possible too.

Regards,
Lance Wang May 19, 2020, 3:26 p.m. UTC | #3 
On Tue, May 19, 2020 at 05:02:42PM +0200, Nicolas George wrote:
> Nicolas George (12020-05-19):
> > Originally, the second parameter to avfilter_graph_dump() was supposed
> > to be dump options. None was ever added, and I later came to realize
> > that using a string for that was more of the same bad idea.
> > 
> > I have no objection to making this bit obsolete, but please do it
> > knowingly.
> 
> Again, forgot something before sending:
> 
> The idea was to be able to output other kinds of graph representation.
> The current one is a very clumsy attempt at ASCII-art. But we could have
> added:
> 
> 	avfilter_graph_dump(graph, "fmt=dot");
> 
> to get output in Graphviz DOT format, basically importing
> tools/graph2dot.c into libavfilter/graphdump.c. Finally doing that may
> be a better idea than obsoleting the option.

Thanks for sharing the background, now the document isn't very clear how to
use the option, after checking, nobody use it, so I think it's better to 
claim it as bool. I can take a look how to convert graph to dot format if nobody
will support it.

> 
> And of course, other formats are possible too.
> 
> Regards,
> 
> -- 
>   Nicolas George



> _______________________________________________
> 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".
Nicolas George May 19, 2020, 3:31 p.m. UTC | #4 
lance.lmwang@gmail.com (12020-05-19):
> Thanks for sharing the background, now the document isn't very clear how to
> use the option, after checking, nobody use it, so I think it's better to 
> claim it as bool. I can take a look how to convert graph to dot format if nobody
> will support it.

Since nothing uses it yet, you are free to do what you want with it. But
if you just treat the options string as bool, you cannot use it to
select the output format.

If you intend to also implement the DOT output, then it is better to
really treat it like an options string, probably using
parse_key_value_pair() (why do we have two versions of this is beyond
me) or similar.

Regards,
diff mbox series

Patch

diff --git a/doc/indevs.texi b/doc/indevs.texi
index 6f5afaf..0ce61f2 100644
--- a/doc/indevs.texi
+++ b/doc/indevs.texi
@@ -974,7 +974,7 @@  filters. Syntax of the filtergraph is the same as the one specified by
 the option @var{graph}.
 
 @item dumpgraph
-Dump graph to stderr.
+Dump graph to stderr. Boolean value, by default disabled.
 
 @end table
 
diff --git a/libavdevice/lavfi.c b/libavdevice/lavfi.c
index c949ff7..57c3070 100644
--- a/libavdevice/lavfi.c
+++ b/libavdevice/lavfi.c
@@ -47,7 +47,7 @@  typedef struct {
     AVClass *class;          ///< class for private options
     char          *graph_str;
     char          *graph_filename;
-    char          *dump_graph;
+    int dump_graph;
     AVFilterGraph *graph;
     AVFilterContext **sinks;
     int *sink_stream_map;
@@ -301,7 +301,7 @@  av_cold static int lavfi_read_header(AVFormatContext *avctx)
         goto end;
 
     if (lavfi->dump_graph) {
-        char *dump = avfilter_graph_dump(lavfi->graph, lavfi->dump_graph);
+        char *dump = avfilter_graph_dump(lavfi->graph, NULL);
         if (dump != NULL) {
             fputs(dump, stderr);
             fflush(stderr);
@@ -493,7 +493,7 @@  static int lavfi_read_packet(AVFormatContext *avctx, AVPacket *pkt)
 static const AVOption options[] = {
     { "graph",     "set libavfilter graph", OFFSET(graph_str),  AV_OPT_TYPE_STRING, {.str = NULL}, 0, 0, DEC },
     { "graph_file","set libavfilter graph filename", OFFSET(graph_filename), AV_OPT_TYPE_STRING, {.str = NULL}, 0, 0, DEC},
-    { "dumpgraph", "dump graph to stderr",  OFFSET(dump_graph), AV_OPT_TYPE_STRING, {.str = NULL}, 0, 0, DEC },
+    { "dumpgraph", "dump graph to stderr",  OFFSET(dump_graph), AV_OPT_TYPE_BOOL, {.i64 = 0}, 0, 1, DEC },
     { NULL },
 };