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