Re: [FFmpeg-devel] SW's Patchsets Overview

[Michael Niedermayer <michael@niedermayer.cc>]

[-- Attachment #1.1: Type: text/plain, Size: 8458 bytes --]

On Tue, Apr 08, 2025 at 10:45:38PM +0000, softworkz . wrote:
> 
> 
> > -----Original Message-----
> > From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> > Michael Niedermayer
> > Sent: Mittwoch, 9. April 2025 00:25
> > To: FFmpeg development discussions and patches <ffmpeg-devel@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@ffmpeg.org> On Behalf Of
> > Marton
> > > > Balint
> > > > Sent: Sonntag, 6. April 2025 23:05
> > > > To: FFmpeg development discussions and patches <ffmpeg-
> > devel@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@ffmpeg.org> On Behalf Of
> > > > Marton
> > > > >> Balint
> > > > >> Sent: Mittwoch, 2. April 2025 21:45
> > > > >> To: FFmpeg development discussions and patches <ffmpeg-
> > > > devel@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 forgot about item_name(), i think i need more sleep
but i think the instance / item name could be richer than it is

like maybe "h264 Dec 1920x1080 <hash of extradata/sps>"
this wouldnt change between runs
its not a true counter though


> 
> 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...

[...]


-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

When you are offended at any man's fault, turn to yourself and study your
own failings. Then you will forget your anger. -- Epictetus

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 251 bytes --]

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".