[FFmpeg-devel] [PATCH] avutil/log: Replace addresses in log output with simple ids

[Soft Works]

> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces at ffmpeg.org> On Behalf Of Marvin
> Scholz
> Sent: Donnerstag, 6. März 2025 21:02
> To: FFmpeg development discussions and patches <ffmpeg-devel at ffmpeg.org>
> Subject: Re: [FFmpeg-devel] [PATCH] avutil/log: Replace addresses in log
> output with simple ids
> 
> 
> 
> On 6 Mar 2025, at 20:27, Soft Works wrote:
> 
> >> -----Original Message-----
> >> From: ffmpeg-devel <ffmpeg-devel-bounces at ffmpeg.org> On Behalf Of
> Marvin
> >> Scholz
> >> Sent: Donnerstag, 6. März 2025 19:59
> >> To: FFmpeg development discussions and patches <ffmpeg-
> devel at ffmpeg.org>
> >> Subject: Re: [FFmpeg-devel] [PATCH] avutil/log: Replace addresses in
> log
> >> output with simple ids
> >>
> >>
> >>
> >> On 6 Mar 2025, at 19:16, Soft Works wrote:
> >>
> >>>> -----Original Message-----
> >>>> From: ffmpeg-devel <ffmpeg-devel-bounces at ffmpeg.org> On Behalf Of
> >> Marvin
> >>>> Scholz
> >>>> Sent: Donnerstag, 6. März 2025 18:49
> >>>> To: FFmpeg development discussions and patches <ffmpeg-
> >> devel at ffmpeg.org>
> >>>> Subject: Re: [FFmpeg-devel] [PATCH] avutil/log: Replace addresses
> in
> >> log
> >>>> output with simple ids
> >>>>
> >>>>
> >>>>
> >>>> On 6 Mar 2025, at 18:44, Soft Works wrote:
> >>>>
> >>>>>> -----Original Message-----
> >>>>>> From: ffmpeg-devel <ffmpeg-devel-bounces at ffmpeg.org> On Behalf Of
> >>>> Marvin
> >>>>>> Scholz
> >>>>>> Sent: Donnerstag, 6. März 2025 18:38
> >>>>>> To: FFmpeg development discussions and patches <ffmpeg-
> >>>> devel at ffmpeg.org>
> >>>>>> Subject: Re: [FFmpeg-devel] [PATCH] avutil/log: Replace addresses
> >> in
> >>>> log
> >>>>>> output with simple ids
> >>>>>>
> >>>>>>
> >>>>>>
> >>>>>> On 6 Mar 2025, at 18:02, Soft Works wrote:
> >>>>>>
> >>>>>>>> -----Original Message-----
> >>>>>>>> From: ffmpeg-devel <ffmpeg-devel-bounces at ffmpeg.org> On Behalf
> Of
> >>>>>>>> Nicolas George
> >>>>>>>> Sent: Donnerstag, 6. März 2025 11:09
> >>>>>>>> To: FFmpeg development discussions and patches <ffmpeg-
> >>>>>> devel at ffmpeg.org>
> >>>>>>>> Subject: Re: [FFmpeg-devel] [PATCH] avutil/log: Replace
> addresses
> >
> > [..]
> >
> >>>>>>
> >>>>>> First of all I want to say I like the idea of having cleaner
> logs,
> >>>>>> but...
> >>>>>>
> >>>>>> IMHO "complex" logging formatting should be handled by fftools
> >>>>>> especially if
> >>>>>> they need global state. Even though thats not the case right now,
> >> but
> >>>>>> just
> >>>>>> like Nicolas I also would prefer to not add even more global
> state
> >>>> for
> >>>>>> logging
> >>>>>> to the library...
> >>>>>>
> >>>>>> All the fancy log formatting should be done in a log callback in
> >> the
> >>>>>> fftools and the default library logging callback should just be a
> >>>> very
> >>>>>> basic
> >>>>>> one, is my opinion on this.
> >>>>>
> >>>>> That's all fine and probably reasonable. But is it fair to block a
> >>>> small change because some major rework would be desired at some
> >> point?
> >>>>>
> >>>>> When that change will be made, it will of course move out this
> >> little
> >>>> change as well.
> >>>>>
> >>>>> But are you really saying that this small change cannot be made
> >>>> because you don't like the general way of the current
> implementation?
> >>>>>
> >>>>
> >>>> Just to be clear, I am not blocking this, just wanted to give my
> >>>> perspective on the topic.
> >>>> So if others think its fine and want it, lets go for it.
> >>>
> >>> Thanks - and sorry for my retardation, I just realized why it's bad
> to
> >> do it this way with regards to library usage. I'll add a callback so
> >> that fftools can do the prefix formatting.
> >>>
> >>> For the idea of moving all the formatting to fftools, wouldn't this
> be
> >> a major breakage for library consumers who are used to getting the
> log
> >> output formatted like now?
> >>>
> >>
> >> Yeah, one of the reasons why I did not really do any work on this yet
> as
> >> I am really
> >> not sure whats the best way to go about this to not be too surprising
> >> for existing
> >> library users...
> >
> > I think, people _do_ want the formatting even when using the libs
> directly, so while separating formatting from the plain logging might
> make sense, it would probably still need to be in avutil - otherwise
> most consumers would probably be annoyed and have to copy the formatting
> code from fftools to their own code base (or similar) - which wouldn't
> be a win for anybody.
> 
> Right, I do think it might be useful to have helpers to make log
> formatting easier for library users (especially because as the final
> consumer you can then decide where to put some eventual state needed),
> and maybe also some to make it easier to obtain logging details from
> AVClass as this is right now somewhat hard to get right and I had to
> check the source code last time to figure out how its done.
> 
> > I see why the global state is bad with regards to this patchset's
> feature (and V3 will remedy), but avoidance of global state would be
> much easier and also make more sense if there was some kind of "local
> state" as a replacement, so that people could for example have separate
> log outputs when performing separate and independent operations.
> > I suppose that's not easy to achieve with the current architecture?
> 
> Yeah, I think the end goal should be to have a way to not have a global
> log callback, however that would require quite a bit of
> redesign. 

Alright I thought so, just questionmarked because - with my patterns of thinking being dominated by object-oriented logic - I've been sometimes surprised by approaches in procedural languages to achieve similar things.

> A sort of hacky way right now to do this is use of thread local storage
> and then "demultiplex" the messages in the log callback
> based on that. However that only works as long as the message does not
> come from a "child" thread internally spawned by FFmpeg.

That's probably only feasible in very specific cases where you (as the consumer) know that it works.

The absolute minimum of an approach I can think of would be something like a "context_id", but it would need to (more or less) "auto-propagate" from context to context for all contexts that are being created - which sounds like a major change.


So, down to earth, for V3 of the patchset I have added a callback (av_log_formatprefix_callback) and a function to register (av_log_set_formatprefix_callback()) to log.c and the actual formatting (plus global state) is done in fftools now.

I realize that this is a much better way. Thanks for the feedback.

sw