> -----Original Message----- > From: ffmpeg-devel <ffmpeg-devel-boun...@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-boun...@ffmpeg.org> On Behalf Of > Marton > > > Balint > > > Sent: Sonntag, 6. April 2025 23:05 > > > To: FFmpeg development discussions and patches <ffmpeg- > de...@ffmpeg.org> > > > Subject: Re: [FFmpeg-devel] SW's Patchsets Overview > > > > > > > > > > > > On Wed, 2 Apr 2025, softworkz . wrote: > > > > > > > > > > > > > > >> -----Original Message----- > > > >> From: ffmpeg-devel <ffmpeg-devel-boun...@ffmpeg.org> On Behalf Of > > > Marton > > > >> Balint > > > >> Sent: Mittwoch, 2. April 2025 21:45 > > > >> To: FFmpeg development discussions and patches <ffmpeg- > > > de...@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 _______________________________________________ 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".
