diff mbox series

[FFmpeg-devel,v7,2/3] avdevice/lavfi: support the dumpgraph with options

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
Related show

Checks

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

Commit Message

Limin Wang May 24, 2020, 11:50 p.m. UTC
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(-)

Comments

Nicolas George June 5, 2020, 3:13 p.m. UTC | #1
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,
Marton Balint June 5, 2020, 5:10 p.m. UTC | #2
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
Limin Wang June 5, 2020, 10:54 p.m. UTC | #3
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".
Limin Wang June 8, 2020, 2:20 p.m. UTC | #4
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".
Nicolas George June 8, 2020, 3:36 p.m. UTC | #5
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,
Marton Balint June 14, 2020, 2:35 p.m. UTC | #6
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 mbox series

Patch

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 },
 };