[FFmpeg-devel] [PATCH 01/11] fftools/graph/graphprint: Fix races when initializing graphprint

[softworkz .]

> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces at ffmpeg.org> On Behalf Of Andreas
> Rheinhardt
> Sent: Sonntag, 1. Juni 2025 12:40
> To: ffmpeg-devel at ffmpeg.org
> Subject: Re: [FFmpeg-devel] [PATCH 01/11] fftools/graph/graphprint: Fix races
> when initializing graphprint


Hi Andreas,

thanks for v2.


> > - Why do you want to remove the ResourceManager AVClass?
> 
> Because I think callers should provide their own logcontext

Do we have any precedent cases for this? What would be the 
parameter type for passing it - AVClass* ?



> >   It wasn't unused. Now the prefix is gone for log entries in
> >   decompress_gzip()
> 
> True, this patch is just wrong (in case of decompression errors,
> av_log() will treat ResourceManagerContext as a logging context,
> although it is no more; this would lead to segfaults if AVDictionary is
> already set, i.e. when initializing the second resource).

Yes, I had seen it crashing once as well when moving debugger 
execution to an av_log line during the second execution.
All good now!

> >   Actually, all av_log() calls should include the resman_ctx
> >   Seems this has been forgotten (well..by me)

Thanks for fixing!


> > - For the registered_formatters initialization:
> >   I used to have initialization order issues when I had tried
> >   with static initialization. That's the reason for those functions
> >   Probably you've done it differently as it seems to work so far
> 
> What issues?

Here's my comment from an earlier review:

----

> > +static const AVTextFormatter *registered_formatters[7+1];
> 
> maybe use a const here, also I'd be more happy if we had a more
> dynamic registration system to avoid the hardcoded bits

While trying this, I remembered that I had tried this before already, but it causes trouble with static initialization order. Probably that's the reason why it has been like this before already, I'm not sure?

----

What I had seen: The registered_formatters array was initialized before 
static initialization of all its members had been done.
It happened at a rather early stage, so it is well possible that it is 
resolved by changes that were made meanwhile. 
FATE tests are passing, so it's probably fine, now.




Here's my full review:

v2-0001-fftools-graph-graphprint-Fix-races-when-initializ

LGTM, I wasn't aware that av_strtok() is destructive,
maybe the doc text should mention it. s param being 
non-const might give a hint, but it's still a bit 
unexpected for anybody who doesn't know all the APIs
by heart.

Now that it's no longer updated, should print_graphs_format
be made const? Same for print_graphs_file?
Could also be in a future patch, though.


v2-0002-fftools-textformat-avtextformat-Separate-mutable-

I am aware of the mixed mutability in the section
definitions. The general idea of separation and making
the section definitions immutable is right of course.

Yet, the general goal is to build a versatile API for
other use cases, so we should not make changes based
on "x is currently used by a only, not by b, so make
it local to a".

The ability to limit the fields that should be output
for each section is an essential feature that should
be generally available for all usages, that's why it
should rather go in the opposite direction - i.e. 
from FFprobe (where it's specific atm) to the API.
I haven't come to do that yet.


We should also hear what Stefano will say about it,
as far as I understood he sees it similar, but I 
might be wrong.



v2-0003-fftools-textformat-avtextformat-Avoid-relocations

LGTM


v2-0004-fftools-resources-resman-Don-t-alloc-ResourceMana

LGTM


v2-0005-fftools-resources-resman-Use-assert-for-always-fa

LGTM


v2-0006-fftools-resources-resman-Use-proper-logcontext

LGTM, thanks!


v2-0007-fftools-textformat-avtextformat-Fix-races-when-in

LGTM


v2-0008-fftools-ffprobe-Factor-writing-common-side-data-t

LGTM


v2-0009-fftools-ffprobe-Fix-indentation

LGTM


v2-0010-fftools-textformat-avtextformat-Move-avtext_print

avtext_print_integers() allows to print binary data in 
a tabular layout and various formats.

It is generally useful and not specific to FFprobe's
usage. AFAIR, Stefano had seen it the same way.


v2-0011-fftools-graph-graphprint-Remove-redundant-avio_fl

LGTM


In summary

v2-0002-fftools-textformat-avtextformat-Separate-mutable-
v2-0010-fftools-textformat-avtextformat-Move-avtext_print

Should wait for Stefano to comment.


All others are good to go.


Thanks
sw