[FFmpeg-devel] [PATCH v9 0/4] print_graphs: Complete Filtergraph Printing

[softworkz .]

> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces at ffmpeg.org> On Behalf Of
> Andreas Rheinhardt
> Sent: Mittwoch, 2. April 2025 09:25
> To: ffmpeg-devel at ffmpeg.org
> Subject: Re: [FFmpeg-devel] [PATCH v9 0/4] print_graphs: Complete
> Filtergraph Printing
> 
> softworkz .:
> >
> >
> >> -----Original Message-----
> >> From: ffmpegagent <ffmpegagent at gmail.com>
> >> Sent: Sonntag, 23. März 2025 04:42
> >> To: ffmpeg-devel at ffmpeg.org
> >> Cc: softworkz <softworkz at hotmail.com>
> >> Subject: [PATCH v9 0/4] print_graphs: Complete Filtergraph Printing
> >>
> >> Due to the additional work on graph visualization (see
> >> https://lists.ffmpeg.org/pipermail/ffmpeg-devel/2025-
> March/341296.html),
> >> I
> >> have removed the two graph printing commits from this patchset rather
> >> than
> >> adding to it, as to avoid wasting the effort already spent on
> reviewing.
> >>
> >> So, now it's a graph printing patchset without graph printing :-)
> >>
> >> All other commits are unchanged from the previous version. I'm also
> >> resubmitting to give it a final run of the tests on various
> platforms.
> >>
> >>
> >> V9
> >> ==
> >>
> >>  * Fix outdir creation for out-of-tree builds
> >>    (thanks, Michael)
> >>
> >> softworkz (4):
> >>   fftools/textformat: Extract and generalize textformat api from
> >>     ffprobe.c
> >>   fftools/ffprobe: Change to use textformat api
> >>   fftools/ffprobe: Rename writer_print_section_* and WriterContext
> >>   fftools/ffprobe: Rename AVTextFormatContext variables (w => tfc)
> >>
> >>  fftools/Makefile                   |   15 +-
> >>  fftools/ffprobe.c                  | 2296 +++++---------------------
> --
> >>  fftools/textformat/avtextformat.c  |  672 ++++++++
> >>  fftools/textformat/avtextformat.h  |  171 +++
> >>  fftools/textformat/avtextwriters.h |   68 +
> >>  fftools/textformat/tf_compact.c    |  282 ++++
> >>  fftools/textformat/tf_default.c    |  145 ++
> >>  fftools/textformat/tf_flat.c       |  174 +++
> >>  fftools/textformat/tf_ini.c        |  160 ++
> >>  fftools/textformat/tf_json.c       |  215 +++
> >>  fftools/textformat/tf_xml.c        |  221 +++
> >>  fftools/textformat/tw_avio.c       |  129 ++
> >>  fftools/textformat/tw_buffer.c     |   92 ++
> >>  fftools/textformat/tw_stdout.c     |   82 +
> >>  14 files changed, 2776 insertions(+), 1946 deletions(-)
> >>  create mode 100644 fftools/textformat/avtextformat.c
> >>  create mode 100644 fftools/textformat/avtextformat.h
> >>  create mode 100644 fftools/textformat/avtextwriters.h
> >>  create mode 100644 fftools/textformat/tf_compact.c
> >>  create mode 100644 fftools/textformat/tf_default.c
> >>  create mode 100644 fftools/textformat/tf_flat.c
> >>  create mode 100644 fftools/textformat/tf_ini.c
> >>  create mode 100644 fftools/textformat/tf_json.c
> >>  create mode 100644 fftools/textformat/tf_xml.c
> >>  create mode 100644 fftools/textformat/tw_avio.c
> >>  create mode 100644 fftools/textformat/tw_buffer.c
> >>  create mode 100644 fftools/textformat/tw_stdout.c
> >>
> >>
> >> base-commit: 0b097ed9f141f57e2b91f0704c721a9eff0204c0
> >> Published-As: https://github.com/ffstaging/FFmpeg/releases/tag/pr-
> >> ffstaging-52%2Fsoftworkz%2Fsubmit_print_graphs5-v9
> >> Fetch-It-Via: git fetch https://github.com/ffstaging/FFmpeg pr-
> >> ffstaging-52/softworkz/submit_print_graphs5-v9
> >> Pull-Request: https://github.com/ffstaging/FFmpeg/pull/52
> >>
> >> Range-diff vs v8:
> >>
> >>  1:  f379c77e88 = 1:  f379c77e88 fftools/textformat: Extract and
> >> generalize textformat api from ffprobe.c
> >>  2:  67bc38ac96 ! 2:  5ebe2a6c20 fftools/ffprobe: Change to use
> >> textformat api
> >>      @@ fftools/Makefile: OBJS-ffmpeg +=                  \
> >>        OBJS-ffplay += fftools/ffplay_renderer.o
> >>
> >>        define DOFFTOOL
> >>      +@@ fftools/Makefile: ifdef HAVE_GNU_WINDRES
> >>      + OBJS-$(1) += fftools/fftoolsres.o
> >>      + endif
> >>      + $(1)$(PROGSSUF)_g$(EXESUF): $$(OBJS-$(1))
> >>      +-$$(OBJS-$(1)): | fftools
> >>      ++$$(OBJS-$(1)): | fftools fftools/textformat
> >>      + $$(OBJS-$(1)): CFLAGS  += $(CFLAGS-$(1))
> >>      + $(1)$(PROGSSUF)_g$(EXESUF): LDFLAGS += $(LDFLAGS-$(1))
> >>      + $(1)$(PROGSSUF)_g$(EXESUF): FF_EXTRALIBS += $(EXTRALIBS-$(1))
> >>      +@@ fftools/Makefile: all: $(AVPROGS)
> >>      +
> >>      + fftools/ffprobe.o fftools/cmdutils.o: libavutil/ffversion.h |
> >> fftools
> >>      + OUTDIRS += fftools
> >>      ++OUTDIRS += fftools/textformat
> >>      +
> >>      + ifdef AVPROGS
> >>      + install: install-progs install-data
> >>
> >>        ## fftools/ffprobe.c ##
> >>       @@
> >>  3:  52c41fdcd0 = 3:  41dab9d31a fftools/ffprobe: Rename
> >> writer_print_section_* and WriterContext
> >>  4:  e85abafb4d = 4:  ae472da3e3 fftools/ffprobe: Rename
> >> AVTextFormatContext variables (w => tfc)
> >>
> >> --
> >> ffmpeg-codebot
> >
> >
> > Does anyone need more time to review this? Please let me know.
> > Anyway, I'll wait until next Monday at least.
> >
> What is the reason for the +800 net line diff? Is it just license
> headers in new files or did you actually add something new? (If so, this
> should be mentioned in the commit message.)
> 
> - Andreas


Hi Andreas,

12 new files (10 c, 2 h), with license+includes of about 30 makes 360 lines.

Previous static functions have been turned into an API, so the definitions get duplicated in header files.

The componentization of output writing is new. What had been previously just an if statement about whether to write to stdout or to a file is now turned into "AVTextWriter" components (tw_stdout.c, tw_avio.c and tw_buffer.c) which have an AVClass, a priv_class and their own "AVTextWriterContext".

The latter ("bufferwriter", tw_buffer.c) is new in functional means - but trivial. It prints to an AVBPrint.

So, besides general reusability and a few details, the bufferwriter is the only functional piece that is new. 
I'm not sure whether that needs specific mention?

Thanks,
sw