> -----Original Message-----
> From: Stefano Sabatini <stefa...@gmail.com>
> Sent: Dienstag, 11. März 2025 00:03
> To: FFmpeg development discussions and patches <ffmpeg-devel@ffmpeg.org>
> Cc: softworkz <softwo...@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
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
