[FFmpeg-devel] [PATCH v6 6/8] fftools/ffmpeg_graphprint: Add options for filtergraph printing

[Soft Works]

> -----Original Message-----
> From: Stefano Sabatini <stefasab at gmail.com>
> Sent: Dienstag, 11. März 2025 00:03
> To: FFmpeg development discussions and patches <ffmpeg-devel at ffmpeg.org>
> Cc: softworkz <softworkz at hotmail.com>
> Subject: Re: [FFmpeg-devel] [PATCH v6 6/8] fftools/ffmpeg_graphprint:
> Add options for filtergraph printing


Hi Stefano,

I've made all of your changes unless mentioned below, ran patcheck, found two or three more bits, but a lot of its output is not applicable I suppose.


Here are some notes:


> > +    [SECTION_ID_HWFRAMESCONTEXT] =    { SECTION_ID_HWFRAMESCONTEXT, "HwFramesContext", 0, { SECTION_ID_ERROR, -1 },  },
> > +
> > +    [SECTION_ID_ERROR] =              { SECTION_ID_ERROR, "Error", 0, { -1 } },
> 
> > +    [SECTION_ID_LOGS] =               { SECTION_ID_LOGS, "Log", AV_TEXTFORMAT_SECTION_FLAG_IS_ARRAY, { SECTION_ID_LOG, -1 } },
> > +    [SECTION_ID_LOG] =                { SECTION_ID_LOG, "LogEntry", 0, { -1 },  },
> 
> Why not Logs/Log? It's OK but I'd keep ID and names in synch to
> simplify reading/editing.

The last three ones weren't even used - removed.


> > +static void print_hwdevicecontext(AVTextFormatContext *w, const AVHWDeviceContext *hw_device_context)
> > +{
> > +    avtext_print_section_header(w, NULL, SECTION_ID_HWDEViCECONTEXT);
> > +
> 
> > +    print_int("HasHwDeviceContext", 1);
> > +    print_str("DeviceType", av_hwdevice_get_type_name(hw_device_context->type));
> 
> 
> Is this even needed? If missing I'd expect type to be none. Or not?


av_hwdevice_get_type_name does not return "none" but NULL for the "none" enum id.

The reasoning was slightly different, though: Typically, you deserialize JSON (or XML) into an object model. If the DeviceType is not present in the target object model (missing enum member), the cannot be deserialized which means that it gets lost - including the information that there was a device context in place. The HasHwDeviceContext (and similar members) prevents that by separating presence and type information.

But I have reduced these that now and also dropped the extra section for hw_device_context.


> > +        print_str("SwPixelFormatAlias", pixdescSw->alias);
> > +    }
> > +
> 
> > +    print_int("Width", hw_frames_context->width);
> > +    print_int("Height", hw_frames_context->height);
> 
> is this meaningful in case of no context? Or should we rather skip them?

What do you mean by "no context"?. The function is only entered if a hwframes context exists.
In that case it's interesting to know width and height of the frames context as it may differ from the presentation frame size.



> > +        ////case AVMEDIA_TYPE_SUBTITLE:
> > +        ////    print_str("Format",  av_x_if_null(av_get_subtitle_fmt_name(link->format), "?"));
> > +        ////    print_int("Width", link->w);
> > +        ////    print_int("Height", link->h);
> > +        ////    print_q("TimeBase", link->time_base, '/');
> > +        ////    break;
> 
> I guess this does not exist yet right?

This does exist and is working, just not in ffmpeg yet (https://github.com/ffstaging/FFmpeg/pull/18)
But it's only av_get_subtitle_fmt_name(), so I left just that line commented. (I can also remove it if it should)



> > +    avtext_print_section_header(w, NULL, SECTION_ID_FILTER);
> > +
> > +    print_str("Name", filter->name);
> > +
> > +    if (filter->filter) {
> 
> > +        print_str("Name2", filter->filter->name);
> 
> something more descriptive such as "class name"? 

I've made it more clear now. There's a filter_id and a filter_name, no more name2/3.



> > +        print_str("DestName", link->dst->name);
> > +        print_str("DestPadName", avfilter_pad_get_name(link->dstpad, 0));
> > +        print_str("SourceName", link->src->name);
> 
> possibly unrelated, but I'm a bit surprised by the asymmetry

In case you meant why there's no SourcePadName in this snippet, that's because the source is the filter under which these ouputs are shown. I have changed that now, so there's pad_index, pad_name, dest_filter_id and dest_pad_name for outputs and similar for inputs.

Generally, there's asymmetry in quite a number of cases. What might explain it somewhat is that those graphs have a fixed direction.



> > +    if ((ret = avtext_context_open(&tctx, text_formatter, wctx, w_args, sections, FF_ARRAY_ELEMS(sections), 0, 0, 0, 0, -1, NULL)) >= 0) {
> 
> > +        avtext_print_section_header(tctx, NULL, SECTION_ID_ROOT);
> > +        avtext_print_section_header(tctx, NULL, SECTION_ID_FILTERGRAPHS);
> > +        avtext_print_section_header(tctx, NULL, SECTION_ID_FILTERGRAPH);
> > +
> > +        av_bprint_clear(target_buf);
> 
> If I understand this is printing the sections and then discarding the
> generated buffer, right? Maybe add a not to explain why this is done

Done:

// Due to the threading model each graph needs to print itself into a buffer
// from its own thread. The actual printing happens short before cleanup in ffmpeg.c
// where all grahps are assembled together. To make this work, we need to put the
// formatting context into the same state like it would be when printing all at once,
// so here we print the section headers and clear the buffer to get into the right state.



> > +        if (print_graphs) {
> > +            printf("%s", target_buf.str);
> 
> > +            av_log(NULL, AV_LOG_INFO, "%s    %c", target_buf.str, '\n');
> 
> why it not hardcoding the newline in the message?


I can't remember, not sure which or whether there was a good reason.



> > +    { "print_graphs_format", OPT_TYPE_STRING, 0,
> > +        { &print_graphs_format },
> 
> > +      "set the output printing format (available formats are: default, compact, csv, flat, ini, json, xml)", "format" },
> 
> non blocking but it would be good to avoid the hardcode in a case a
> new format is added (or we'll skip update)


There's again the problem of static initialization order. While I'm sure that this could be worked out in some tricky way, I wonder whether that's worth the effort. When was the last time that a new text format has been added? And when some new format gets added, it will need to be "hard-coded" in the doc files and at various other places anyway.



> About the output style, this is using FooBar style against foo_bar
> employed by ffprobe itself. I'm not against this, but I wonder if this
> was considered and if using a more consistent style across tools is
> helpful.

Back then, I hadn't thought much about it. I just did what I thought is natural for JSON and XML documents. 
Even though it will cause me some headaches, your point is undeniably valid, so I've reworked everything to snake case and also adjusted the naming of some elements for better clarity.


Thanks again for your reviews,
best wishes,
sw