[FFmpeg-devel] SW's Patchsets Overview

[softworkz .]

> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces at ffmpeg.org> On Behalf Of
> Michael Niedermayer
> Sent: Mittwoch, 9. April 2025 00:25
> To: FFmpeg development discussions and patches <ffmpeg-devel at ffmpeg.org>
> Subject: Re: [FFmpeg-devel] SW's Patchsets Overview
> 
> Hi
> 
> On Sun, Apr 06, 2025 at 09:12:00PM +0000, softworkz . wrote:
> >
> >
> > > -----Original Message-----
> > > From: ffmpeg-devel <ffmpeg-devel-bounces at ffmpeg.org> On Behalf Of
> Marton
> > > Balint
> > > Sent: Sonntag, 6. April 2025 23:05
> > > To: FFmpeg development discussions and patches <ffmpeg-
> devel at ffmpeg.org>
> > > Subject: Re: [FFmpeg-devel] SW's Patchsets Overview
> > >
> > >
> > >
> > > On Wed, 2 Apr 2025, softworkz . wrote:
> > >
> > > >
> > > >
> > > >> -----Original Message-----
> > > >> From: ffmpeg-devel <ffmpeg-devel-bounces at ffmpeg.org> On Behalf Of
> > > Marton
> > > >> Balint
> > > >> Sent: Mittwoch, 2. April 2025 21:45
> > > >> To: FFmpeg development discussions and patches <ffmpeg-
> > > devel at ffmpeg.org>
> > > >> Subject: Re: [FFmpeg-devel] SW's Patchsets Overview
> > > >>
> > > >>
> > > >>
> > > >> On Wed, 2 Apr 2025, softworkz . wrote:
> > > >>
> > > >>> Hello everybody,
> > > >>>
> > > >>> with freshly gained push access rights, I want to act
> responsibly
> > > and
> > > >>> carefully, and also avoid unexpected surprises so I'm not going
> to
> > > >> rush
> > > >>> things. Due to that change, I thought it might be good to post
> an
> > > >>> overview of the patchsets I am intending to push in the near
> future:
> > > >>
> > > >> Thanks for the heads up.
> > > >>
> > > >> [...]
> > > >>
> > > >>> avutil/log: Replace addresses in log output with simple ids
> > > >>>
> > > >>> GitHub:    https://github.com/ffstaging/FFmpeg/pull/59
> > > >>> Patchwork:
> > > >> https://patchwork.ffmpeg.org/project/ffmpeg/list/?series=14094
> > > >>
> > > >
> > > > Hi Marton,
> > > >
> > > > thanks a lot for looking at the patchset.
> > > >
> > > >> To be honest, I don't like this at all. You duplicate a lot of
> code
> > > from
> > > >> avutil/log, and the implementation has quite a few problems, some
> of
> > > >> them not really fixable.
> > > >
> > > > Originally, this was a patch against avutil/log. Nicolas objected
> that
> > > > it was adding global state and Hendrik (and Nicolas) suggested
> that I
> > > > should to this in fftools only - outside of the libs, in a was
> that
> > > > fftools get their own logging implementation - with the potential
> of
> > > > being able to do other things in the future that wouldn't make
> sense
> > > in
> > > > the lib code. Letting fftools have their own logging
> implementation of
> > > > can of course only start from a copy in order to retain existing
> > > > behavior. On top of that I applied that little change then.
> > > >
> > > >
> > > >> - creating object IDs in the order the objects log something
> (what if
> > > >> they do not? What if it depends on loglevel?)
> > > >> - tracking object IDs based on their address - objects are
> > > >>    allocated and removed at runtime, it is possible that an
> address
> > > will be
> > > >>    re-used for a different object later on
> > > >
> > > > The Ids are not meant to have much more value than the addresses
> > > > currently shown - with an important difference: They are short and
> > > > remain the same on repeated execution. Plus: they are counted by
> > > > AVClass, that give a little additional value, but since they are
> just
> > > > "indexing" the addresses, they are in fact prone to the same
> > > > shortcomings like the addresses themselves, meaning that a re-
> > > assignment
> > > > might give you the same id for something different and also
> different
> > > > addresses (in consequence the IDs as well) can reference the same
> > > thing
> > > > (e.g. with buffer refs).
> > > >
> > > >> - linear search of addresses. A long ffmpeg process can
> constantly
> > > >> create
> > > >>    objects during runtime, eventually completely depleting the
> pool
> > > and
> > > >>    causing an extensive search for all future logs.
> > > >
> > > > I have considered that case. There is a hard limit from when on no
> IDs
> > > > are assigned anymore (all zeros).
> > > >
> > > >
> > > >> So overall I don't think it's worth pursuing this, especially
> since
> > > most
> > > >> users won't care neither about the ID, nor about the address...
> > > >
> > > > Let me give two examples of where I find it useful to have those
> IDs:
> > > >
> > > > On startup decoders can be initialized multiple times, like first
> for
> > > > probing and then for transcoding. Or when there are multiple
> streams
> > > of
> > > > the same type (codec), the log messages can be confusing when the
> log
> > > > output from several identical ones gets mixed up. Being able to
> see
> > > > "which is which" is quite of value at times.
> > > >
> > > > HW Device context can also get initialized multiple times and
> knowing
> > > > which one has shut down already and which hasn't - is helpful.
> Also,
> > > in
> > > > case of complex filtergraphs with multiple derived and reverse-
> derived
> > > > hw contexts, one can quickly get lost in understanding the logs.
> > > >
> > > >
> > > > That being said - I don't want to insist on those IDs. We could
> also
> > > > just hide the addresses (activatable by a log flag) and I'd still
> be
> > > > happy about being able to do logfile diffs in the future without
> > > trouble
> > > > 😊
> > > >
> > > > In that case, the change could also be made just in avutil/log.
> > > Probably
> > > > also depends on what the consensus would be regarding the value of
> > > > fftools having their own logging implementation - or rather not?
> > > >
> > > > I'm open for either direction.
> > >
> > > I think a log flag to completely hide the addresses makes sense, and
> can
> > > be implemented cleanly and reliably in avutil/log. I can totally
> support
> > > that.
> > >
> > > Thanks,
> > > Marton
> > > _______________________________________________
> >
> > Hi Marton,
> >
> >
> > thanks a lot for your reply. As nobody has voiced in favor of the
> simple ID replacement, I'm fine to go that way.
> >
> > There's one point remaining though: The intention was to hide the
> addresses by default and allow to enable them via flag. Two earlier
> commenters were seconding that, a third one not explicitly objecting. It
> was only said that it must be possible to enable it again by flag.
> 
> In the long run maybe some
>     int instance_name_offset;
> in AVClass that points to a place in each instance where the name is
> stored
> This can then be set when the objects are allocated or initialized and
> could
> also be overridden by the user application, storing other names

Do you mean this as a generalized alternative to the item_name function like used here https://github.com/ffstaging/FFmpeg/pull/61/files#diff-8622a9ccb5f7c11364d8cf5be7ee49928bf3d9c007774a3b138f5e4af9046d84R140-R150 for logging e.g. "D3D11VA" instead of just "AVHWDeviceContext"?

I had thought about an instance counter - which would be more reliable than inferring from the pointer-to-pointer address - but the problem is that there's no global base method for creating    AVClass instances where this could be implemented...

sw