[FFmpeg-devel] [PATCH v2 1/4] avfilter/graphdump: implement options parsing

[Soft Works]

> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces at ffmpeg.org> On Behalf Of
> Niklas Haas
> Sent: Donnerstag, 27. Februar 2025 18:53
> To: ffmpeg-devel at ffmpeg.org
> Cc: Niklas Haas <git at haasn.dev>
> Subject: Re: [FFmpeg-devel] [PATCH v2 1/4] avfilter/graphdump: implement
> options parsing
> 
> On Tue, 18 Feb 2025 13:46:00 +0100 Niklas Haas <ffmpeg at haasn.xyz>
> wrote:
> > From: Niklas Haas <git at haasn.dev>
> >
> > And use it to make the output format configurable.
> >
> > I also added a "none" option to explicitly disable the behavior (in
> > case a previous command line argument was used to enable it). This has
> > the added benefit of giving the default "pretty" format the ID 1,
> > allowing `-dumpgraph 1` to continue working as expected.
> 
> Any further comments about this series? If not, I intend to merge after the
> weekend is over.
> 
> I decided to focus on this approach instead of the one proposed by Soft Works
> in the other thread because I think it is overall cleaner and less invasive.

Is that so...?


Hi Niklas,

I hadn't looked at yours after you had said:

> That seems amazing and exactly what I need.
> >
> > I can resubmit it, it's the most comprehensive implementation in this regard.
> 
> Yes, please resubmit it. I will review it.

Now I looked at it and I have some notes and questions:


1. Import of libavfilter/filters.h in fftools

I had this as well in my v1, but even though it's working fine, I'm just citing Andreas:

> That's an internal header which must not be used by fftools.


2. Clean and non-invasive?

I'd like to note that my v1 was fully self-contained in fftools without making any change to avfilter. The latest version adds 10 lines of code (one new public function) to avfilter and doesn't touch this lib otherwise - everything is isolated in fftools. 
(It's just clumsy and ugly due to the writers duplication but I'm working on that as you might have seen).


3. Your adding Fields to AVFilterPad

...for the sole purpose of the graphdump feature. Is that really more clean and less invasive than adding a simple function (avfilter_link_get_hw_frames_ctx()) to the public API of libavfilter?


4. FilterPad Labels (#2)

What's the purpose of those labels? The code mentions "tracking", but it's not clear to me why this would require an extra string label.
Each filter already has a unique ID assigned. Each filter also has [0...n] inputs and outputs. This means that you can build an identifier like 

<filter-id>_<in/out>_<pad index>


5. ffmpeg_filter.c

    avfilter_inout_free(&inputs);
    avfilter_inout_free(&outputs);

Why are you adding this?


6. Overview

Before I get to the last question, here's a comparison. I don't want to be unfair, so let me know when something is missing..

+------------------------------------------------+-----------+-------+
|                                                | softworkz | haasn |
+------------------------------------------------+-----------+-------+
| Prints all available filtegraph information    |     X     |       |
+------------------------------------------------+-----------+-------+
| Prints hw frames context information           |     X     |       |
| (the most valuable info for some)              |           |       |
+------------------------------------------------+-----------+-------+
| Prints each graph just once                    |     X     |       |
+------------------------------------------------+-----------+-------+
| Acquires the graph data in the right moment    |     X     |       |
| (right after parsing, many details are missing |           |       |
| until the first frames have passed through)    |           |       |
+------------------------------------------------+-----------+-------+
| Prints both, simple and complex graphs         |     X     |       |
+------------------------------------------------+-----------+-------+
| Prints all graphs at once (single document)    |     X     |       |
+------------------------------------------------+-----------+-------+
| Can output to file                             |     X     |       |
+------------------------------------------------+-----------+-------+
| Prints to stdout, not via av_log()             |     X     |       |
+------------------------------------------------+-----------+-------+
| Supports output formatting in standardized     |     X     |       |
| and machine-readable formats                   |           |       |
| (soon all ffprobe formats + YAML)              |           |       |
+------------------------------------------------+-----------+-------+
| Mature and proven in production for 7 years    |     X     |       |
+------------------------------------------------+-----------+-------+
| Doesn't need internal changes to avfilter      |     X     |       |
+------------------------------------------------+-----------+-------+
| Can print ASCII graphics                       |           |   X   |
+------------------------------------------------+-----------+-------+

So, the last question is - which one do you think should be merged?


Best wishes,
sw