[FFmpeg-devel] [PATCH] Add AVOptions for libopenh264 encoder message log level and log function.

[wm4]

On Wed, 2 Sep 2015 20:42:25 +0000
Gregory J Wolfe <gregory.wolfe at kodakalaris.com> wrote:

> > -----Original Message-----
> > From: ffmpeg-devel [mailto:ffmpeg-devel-bounces at ffmpeg.org] On Behalf
> > Of wm4
> > Sent: Wednesday, September 02, 2015 2:28 PM
> > To: ffmpeg-devel at ffmpeg.org
> > Subject: Re: [FFmpeg-devel] [PATCH] Add AVOptions for libopenh264
> > encoder message log level and log function.
> > 
> > On Wed, 2 Sep 2015 17:36:30 +0000
> > Gregory J Wolfe <gregory.wolfe at kodakalaris.com> wrote:
> > 
> > > > -----Original Message-----
> > > > From: ffmpeg-devel [mailto:ffmpeg-devel-bounces at ffmpeg.org] On
> > > > Behalf Of Paul B Mahol
> > > > Sent: Wednesday, September 02, 2015 1:15 PM
> > > > To: FFmpeg development discussions and patches
> > > > Subject: Re: [FFmpeg-devel] [PATCH] Add AVOptions for libopenh264
> > > > encoder message log level and log function.
> > > >
> > > > Dana 2. 9. 2015. 19:10 osoba "Gregory J. Wolfe" <
> > > > gregory.wolfe at kodakalaris.com> napisala je:
> > > > >
> > > > > By default, the libopenh264 encoder logs error and warning
> > > > > messages to stderr.  However, libopenh264 also provides a
> > > > > SetOption() function that allows the client to set the message
> > > > > logging level (e.g., error, warning,
> > > > > debug) and a client-supplied message logging function (whose
> > > > > signature must conform to libopenh264 type WelsTraceCallback).
> > > > >
> > > > > To enable this from the FFmpeg level, file libopenh264enc.c has
> > > > > been modified to add AVOption entries OPTION_TRACEL_LEVEL (log
> > > > > level) and OPTION_TRACE_CALLBACK (log function).  The names are
> > > > > derived from the enumerator values ENCODER_OPTION_TRACE_LEVEL
> > and
> > > > > ENCODER_OPTION_TRACE_CALLBACK which are passed to SetOption().
> > > > While
> > > > > they are somewhat verbose, they are very likely unique across
> > encoders.
> > > > >
> > > > > Some FFmpeg command line examples.  To disable all libopenh264
> > > > messages:
> > > > >
> > > > >   ffmpeg ... -OPTION_TRACE_LEVEL quiet
> > > > >
> > > > > To enable only error messages:
> > > > >
> > > > >   ffmpeg ... -OPTION_TRACE_LEVEL error
> > > > >
> > > > > The OPTION_TRACE_CALLBACK option can obviously not be used from
> > > > > the command line since it requires the address of a function.
> > > > > When used programmatically, the function address must be converted
> > > > > to a string representation of the function address as a signed 64-bit
> > integer.
> > > > >
> > > > > The function signature defined by libopenh264 type
> > > > > WelsTraceCallback can be found in source file
> > > > > codec/api/svc/codec_api.h.  The default implementation used by
> > > > > libopenh264 is function welsStderrTrace(), and can be found in source
> > file codec/common/source/welsCodecTrace.cpp.
> > > >
> > > > Why this doesnt make use of av_log functions?
> > >
> > > The libopenh264 library requires its own specific signature for the
> > > log function that you give to it.  If the client so desires, he/she
> > > can create a message logging function with the required libopenh264
> > > signature that then invokes av_log.  Note that libopenh264 already
> > > formats its messages using its own prefix strings, similar to what FFmpeg
> > does.
> > >
> > > >
> > > > CAPS LOCK OPTIONS ARE UNACCEPTABLE
> > > >
> > 
> > +1
> > 
> > > I'm guessing that you're objecting to the command line switch values
> > > like -OPTION_TRACE_LEVEL.  I did not want to use something like
> > > loglevel since it is already used by FFmpeg.  libopenh264 uses its own
> > > enumerator values for its message logging level, so I simply tried to
> > > come up with something that would not conflict with any existing
> > > AVOptions.  Do the AVOption keywords have to be unique across all
> > > formats/encoders/decoders?  Please supply some guidance as to
> > > requirements/restrictions and I will be happy to update the patch.
> > 
> > I don't see the slightest point in this. You could setup a callback which in turn
> > calls av_log(). This would be quite sane. It'd just work, and the user doesn't
> > have to set an obscure encoder-specific option to get expected behavior. I'm
> > not sure what you're objecting here. Is it maybe that the libopenh264
> > callback doesn't have a log level parameter?
> > 
> > In addition to being extremely roundabout to the user (as opposed to using
> > av_log), OPTION_TRACE_CALLBACK is also missing documentation.
> > What signature does it have?
> 
> I think I see what you're getting at.  But first, some quick comments in response:
> 
> - Admittedly, my first attempt addressed, at only the very lowest level,
> the minimum amount needed to be able to control messages from
> libopenh264, and was not well integrated with FFmpeg w.r.t. av_log.
> 
> - Regarding documentation, I did state in the git commit message:
> 
> ... a client-supplied message logging function (whose signature must conform
> to libopenh264 type WelsTraceCallback) ....  The function signature defined by
> libopenh264 type WelsTraceCallback can be found in source file
> codec/api/svc/codec_api.h.  The default implementation used by
> libopenh264 is function welsStderrTrace(), and can be found in source
> file codec/common/source/welsCodecTrace.cpp.

Yeah, but no user potentially interested in getting libopenh264 log
output is going to find the commit just to learn how to use it.

> OK, time to move forward.  Based on the discussion so far, here's what I
> propose as a revision:
> 
> (1) Eliminate OPTION_TRACE_LEVEL and OPTION_TRACE_CALLBACK.
> 
> (2) Intelligently map the FFmpeg log levels to their equivalents
> (more or less) in libopenh264, so that a change to the FFmpeg log level
> has the corresponding effect on libopenh264 logging.
> 
> (3) Add code to libopenh264enc.c that implements and sets up the needed
> libopenh264 logging function, which itself uses av_log.
> 
> These changes would then fully integrate the libopenh264 message logging
> Into FFmpeg without the client having to do anything.
> 
> Feedback?

Sounds perfect.

Note that API users may or may not do their own filtering using a
custom av_log callback, rather than relying on av_log's filtering.