softworkz .:
>
>
>> -----Original Message-----
>> From: ffmpeg-devel <ffmpeg-devel-boun...@ffmpeg.org> On Behalf Of Andreas
>> Rheinhardt
>> Sent: Sonntag, 1. Juni 2025 12:40
>> To: ffmpeg-devel@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* ?
>
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)
>
>
>>> 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.
> 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. 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.
>
>
> 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.
>
>
>
> 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.
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.
>
> 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.
>
- Andreas
_______________________________________________
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".
