[FFmpeg-devel] [PATCH 3/3] Add assert that the avcodec lockisheld when initializing static VLC tables.

[Don Moir]

----- Original Message ----- 
From: "Don Moir" <donmoir at comcast.net>
To: "FFmpeg development discussions and patches" <ffmpeg-devel at ffmpeg.org>
Sent: Monday, December 03, 2012 8:09 AM
Subject: Re: [FFmpeg-devel] [PATCH 3/3] Add assert that the avcodec lockisheld when initializing static VLC tables.


>>> Do I need a bug report for this ?
>
>> If you get an assert failure, yes, you should submit a bug report.
>
> Now that I know I can trap av_log thru AV_LOG_FATAL I will have something to report if it happens.
>
> Mostly I turn off all calls made by av_log because its just not useful outside of debugging and missed the point of AV_LOG_FATAL 
> which is useful.

av_set_log_level (AV_LOG_FATAL) still allows pretty much every log level. While I can check the level it bothers me that in some 
cases I can get like 1000 calls over a minutes time that I have no use for.

I had mentioned in the past to allow NULL for av_log_callback and that was put in av_vlog but I wish the check for NULL was also at 
the top of av_log just to do nothing. The point being short-circuit av_log as soon as possible. Then would still need allowable 
av_assert0 replacement for the av_log call which default would be av_log.

Am I nit-picking too much ? I don't think so in the sense why perform actions that consume time and are not needed.

Existing av_log function:

void av_log(void* avcl, int level, const char *fmt, ...)
{
    AVClass* avc = avcl ? *(AVClass **) avcl : NULL;
    va_list vl;
    va_start(vl, fmt);
    if (avc && avc->version >= (50 << 16 | 15 << 8 | 2) &&
        avc->log_level_offset_offset && level >= AV_LOG_FATAL)
        level += *(int *) (((uint8_t *) avcl) + avc->log_level_offset_offset);
    av_vlog(avcl, level, fmt, vl);
    va_end(vl);
}

This modified av_log logic is technically more efficient then the current av_log even with the addtion of the check for 
av_log_callback. It removes the double check
for a avcl/avc null value and removes call to av_vlog. So point here is to short-circuit av_log as soon as possible if not needed 
because it is called way to often.

void av_log(void* avcl, int level, const char *fmt, ...)
{
    if (av_log_callback) {
        va_list vl;
        va_start(vl, fmt);
        if (avcl) {
           AVClass* avc = *(AVClass **) avcl ;
           if (avc->version  >= (50 << 16 | 15 << 8 | 2) &&
              avc->log_level_offset_offset && level >= AV_LOG_FATAL)
                 level += *(int *) (((uint8_t *) avcl) +  avc->log_level_offset_offset);
        }
        // prevent reduntant check for av_log_callback and call to av_vlog
        av_log_callback(avcl, level, fmt, vl);
        va_end(vl);
    }
}

For av_assert0 it would need to allow a replacement for the av_log call much like a replacement for the av_log_callback is allowed. 
The default would just be av_log.

#define av_assert0(cond) do {                                           \
    if (!(cond)) {
-       av_log(NULL, AV_LOG_FATAL, "Assertion %s failed at %s:%d\n",    \
+      av_fatal_callback(NULL, AV_LOG_FATAL, "Assertion %s failed at %s:%d\n",    \
               AV_STRINGIFY(cond), __FILE__, __LINE__);                 \
        abort();                                                        \
    }                                                                   \
} while (0)

All pretty frekin simple with no real code impact. Needs function to set av_fatal_callback.