[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: Montag, 2. Juni 2025 00:08


> >>> - 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* ?
> >
> 
> The type would be void*. AVClass* is wrong (AVClass** would be correct).
> There are lots of functions with a logcontext parameter (see git grep -E
> 'void \*log_?ctx' *.h)

Alright, thanks.

> 
> >
> >
> >>>   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.
> 
> FATE is mostly useless here, given that graphprint is not covered by it.

It wasn't about graphprint. At that time, graphprint wasn't
even included. It was about the classic output formats, and
since some of them are used for output in FATE tests, it should
be a valid proof that the issues I had seen are no longer there.
Yet, I have run FATE on Linux only. 
Unfortunately, Patchwork takes always only the first patch when
sent as multiple attachments, so we can't see whether FATE is 
finishing well on all the builders.


> 
> > 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?
> 
> They are updated generically, via the option system. Making them const
> would lead to compiler warnings from the "{ &print_graphs_format },"
> line in the options definition. 

Why? The chars are const, but not the pointer.

I don't see a warning either (neither MSVC nor GCC)

> Furthermore, the compiler would put
> these variables into .rodata if they were made const, which would lead
> to crashes if the user sets the relevant options.

To be clear, what I meant is changing

extern char *print_graphs_file = NULL;

to

extern const char *print_graphs_file = NULL;




> > 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.
> 
> I'll of course wait for Stefano, but already want to add that nothing is
> more versatile than a callback.

By saying "versatile", I meant that the API is usable for
many cases, but it should also reduce the burden of
re-implementing common features. Specifically, in this 
case it would be more convenient for API consumers
when they wouldn't need to care about command line 
parsing and populating the dictionaries for sections
with the fields that should be printed.
Ideally, the API would allow both ways.


> > v2-0010-fftools-textformat-avtextformat-Move-avtext_print
> >
> > avtext_print_integers() allows to print binary data in
> > a tabular layout and various formats.
> 
> I don't see it that way. E.g. it forces you to write the offset at the
> start of each line, it forces the layout of the offset upon you. One
> could of course add yet more arguments to support more use cases, but
> then it would be ever more of a monster.

I think it would make sense to skim through the usages to
see what's actually required at the moment and based on that,
we can determine the best way to go forward, like a better 
signature and feature set.

PS: I'm CCing Stefano

Thanks
sw