| Message ID | 1590364226-11567-2-git-send-email-lance.lmwang@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel,v7,1/3] avfilter/graphdump: support for the graph2dot function | 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-25): > From: Limin Wang <lance.lmwang@gmail.com> > > Signed-off-by: Limin Wang <lance.lmwang@gmail.com> > --- > doc/indevs.texi | 19 +++++++++++++++++-- > libavdevice/lavfi.c | 8 +++++--- > 2 files changed, 22 insertions(+), 5 deletions(-) Either we case about backwards compatibility, and you cannot change the type of the existing option at all, or we do not, and it is not necessary to add another option. I move we do not care much about compatibility: the syntax was never documented, except "1" should work. Then accept "1" and any valid key-value string. Regards,
On Fri, 5 Jun 2020, Nicolas George wrote: > lance.lmwang@gmail.com (12020-05-25): >> From: Limin Wang <lance.lmwang@gmail.com> >> >> Signed-off-by: Limin Wang <lance.lmwang@gmail.com> >> --- >> doc/indevs.texi | 19 +++++++++++++++++-- >> libavdevice/lavfi.c | 8 +++++--- >> 2 files changed, 22 insertions(+), 5 deletions(-) > > Either we case about backwards compatibility, and you cannot change the > type of the existing option at all, or we do not, and it is not > necessary to add another option. I think it always depended on the fallout, we were never blindly strict about this. > > I move we do not care much about compatibility: the syntax was never > documented, except "1" should work. Then accept "1" and any valid > key-value string. I suggested the separate option for the options because otherwise you can't do dumpgraph without options specified, so you cant make the code call avfilter_dump_graph(graph, NULL). "1" as a special string might be OK until some deprecation period but not for all eternity if the options string wants to be an options list and if it must be parseable as an AVDictionary. That would be the same inconsistency which we tried to avoid all over the codebase. E.g. this option looks like a dictionary, except "1" is also accepted... Regards, Marton
On Fri, Jun 05, 2020 at 07:10:08PM +0200, Marton Balint wrote: > > > On Fri, 5 Jun 2020, Nicolas George wrote: > > > lance.lmwang@gmail.com (12020-05-25): > > > From: Limin Wang <lance.lmwang@gmail.com> > > > > > > Signed-off-by: Limin Wang <lance.lmwang@gmail.com> > > > --- > > > doc/indevs.texi | 19 +++++++++++++++++-- > > > libavdevice/lavfi.c | 8 +++++--- > > > 2 files changed, 22 insertions(+), 5 deletions(-) > > > > Either we case about backwards compatibility, and you cannot change the > > type of the existing option at all, or we do not, and it is not > > necessary to add another option. > > I think it always depended on the fallout, we were never blindly strict > about this. > > > > > I move we do not care much about compatibility: the syntax was never > > documented, except "1" should work. Then accept "1" and any valid > > key-value string. > > I suggested the separate option for the options because otherwise you can't > do dumpgraph without options specified, so you cant make the code call > avfilter_dump_graph(graph, NULL). > > "1" as a special string might be OK until some deprecation period but not > for all eternity if the options string wants to be an options list and if it > must be parseable as an AVDictionary. That would be the same inconsistency > which we tried to avoid all over the codebase. E.g. this option looks like a > dictionary, except "1" is also accepted... When I'm try to understand how to use the option, no document, so I had to study the code, it's string type, but no code to use it, so I think it's boolean type. I guess most of user will have the same feeling. So I prefer to Marton's suggestion with separate option. > > Regards, > Marton > _______________________________________________ > 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".
On Fri, Jun 05, 2020 at 07:10:08PM +0200, Marton Balint wrote: > > > On Fri, 5 Jun 2020, Nicolas George wrote: > > > lance.lmwang@gmail.com (12020-05-25): > > > From: Limin Wang <lance.lmwang@gmail.com> > > > > > > Signed-off-by: Limin Wang <lance.lmwang@gmail.com> > > > --- > > > doc/indevs.texi | 19 +++++++++++++++++-- > > > libavdevice/lavfi.c | 8 +++++--- > > > 2 files changed, 22 insertions(+), 5 deletions(-) > > > > Either we case about backwards compatibility, and you cannot change the > > type of the existing option at all, or we do not, and it is not > > necessary to add another option. > > I think it always depended on the fallout, we were never blindly strict > about this. > > > > > I move we do not care much about compatibility: the syntax was never > > documented, except "1" should work. Then accept "1" and any valid > > key-value string. > > I suggested the separate option for the options because otherwise you can't > do dumpgraph without options specified, so you cant make the code call > avfilter_dump_graph(graph, NULL). > > "1" as a special string might be OK until some deprecation period but not > for all eternity if the options string wants to be an options list and if it > must be parseable as an AVDictionary. That would be the same inconsistency > which we tried to avoid all over the codebase. E.g. this option looks like a > dictionary, except "1" is also accepted... Nicolas, are you agree with Marton's opinion? If you're agree with that, then I'll update with the other patch by your comments. > > Regards, > Marton > _______________________________________________ > 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".
Marton Balint (12020-06-05): > I think it always depended on the fallout, we were never blindly strict > about this. I agree. And in this case, this option is quite obscure, and the function behind it even more. > I suggested the separate option for the options because otherwise you can't > do dumpgraph without options specified, so you cant make the code call > avfilter_dump_graph(graph, NULL). We can avfilter_dump_graph(graph, ""), which is equivalent enough. > "1" as a special string might be OK until some deprecation period but not > for all eternity if the options string wants to be an options list and if it > must be parseable as an AVDictionary. That would be the same inconsistency > which we tried to avoid all over the codebase. E.g. this option looks like a > dictionary, except "1" is also accepted... We are talking about if (options[0] == '1' && !options[1]) options = ""; at the beginning of an obscure function, not documented, only there to avoid breaking existing scripts. If that stays for all eternity, we can live with that, I think. Regards,
On Mon, 8 Jun 2020, Nicolas George wrote: > Marton Balint (12020-06-05): >> I think it always depended on the fallout, we were never blindly strict >> about this. > > I agree. And in this case, this option is quite obscure, and the > function behind it even more. > >> I suggested the separate option for the options because otherwise you can't >> do dumpgraph without options specified, so you cant make the code call >> avfilter_dump_graph(graph, NULL). > > We can avfilter_dump_graph(graph, ""), which is equivalent enough. I am still uneasy about this, but fine. Regards, Marton > >> "1" as a special string might be OK until some deprecation period but not >> for all eternity if the options string wants to be an options list and if it >> must be parseable as an AVDictionary. That would be the same inconsistency >> which we tried to avoid all over the codebase. E.g. this option looks like a >> dictionary, except "1" is also accepted... > > We are talking about > > if (options[0] == '1' && !options[1]) > options = ""; > > at the beginning of an obscure function, not documented, only there to > avoid breaking existing scripts. If that stays for all eternity, we can > live with that, I think.
diff --git a/doc/indevs.texi b/doc/indevs.texi index 6f5afaf..b4dffc4 100644 --- a/doc/indevs.texi +++ b/doc/indevs.texi @@ -973,8 +973,17 @@ Set the filename of the filtergraph to be read and sent to the other filters. Syntax of the filtergraph is the same as the one specified by the option @var{graph}. -@item dumpgraph -Dump graph to stderr. +@item dumpgraph (@emph{bool}) +Dump filter graph with ASCII style to stderr default. +By default, it is disable. + +@item dumpgraph_opts @var{options} +Set the options of dump filter graph + +it is a ':'-separated list of @var{key=value} pairs. + +Set the graph with graphviz DOT format by @var{fmt=dot|DOT}, +set the filename of filtergraph to output by @var{filename=path}. @end table @@ -988,6 +997,12 @@ ffplay -f lavfi -graph "color=c=pink [out0]" dummy @end example @item +dump the filter graph with graphviz DOT output format to ./test.tmp +@example +ffplay -dumpgraph 1 -dumpgraph_opts fmt=dot:filename=./test.tmp -f lavfi color=c=pink +@end example + +@item As the previous example, but use filename for specifying the graph description, and omit the "out0" label: @example diff --git a/libavdevice/lavfi.c b/libavdevice/lavfi.c index c949ff7..9cdfbaa 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; + char *dump_graph_opts; AVFilterGraph *graph; AVFilterContext **sinks; int *sink_stream_map; @@ -57,6 +57,7 @@ typedef struct { AVFrame *decoded_frame; int nb_sinks; AVPacket subcc_packet; + int dump_graph; } LavfiContext; static int *create_all_formats(int n) @@ -301,7 +302,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, lavfi->dump_graph_opts); if (dump != NULL) { fputs(dump, stderr); fflush(stderr); @@ -493,7 +494,8 @@ 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, {.str = 0}, 0, 1, DEC }, + { "dumpgraph_opts", "set the options of dump graph", OFFSET(dump_graph_opts), AV_OPT_TYPE_STRING, {.str = NULL}, 0, 0, DEC }, { NULL }, };