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 |
Context | Check | Description |
---|---|---|
andriy/default | pending | |
andriy/make | success | Make finished |
andriy/make_fate | success | Make fate finished |
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 (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,
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".
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 --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 }, };