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

[Don Moir]

> On Mon, Dec 03, 2012 at 10:34:33AM -0500, Don Moir wrote:
>> 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.
>
> Well, in the sense that we are (by your numbers) talking about
> 1000 per minute at maybe 20 instructions, or around
> 0.00001 % of CPU time I'd say yes.
> However I notice two API concerns
> 1) Why do we apply log_level_offset_offset only to av_log but
> not av_vlog? This is also not documented. Is this really intentional?
> 2) Why are we handling the log level only in av_log_default_callback
> and not in common code like av_vlog?
>
>> #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.
>
> I'd prefer to not increase the size of our already unmanageable
> API as long as the only argument is a nearly non-measurable amount
> of CPU usage :-)

I tend not to think about the minor impact one function might have and assume a collection of optimizations in various functions 
will have an impact.

The current av_log is just bad coding.

You make an assignment to avc when it might not be necessary and then test avcl / avc twice.

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);
}

change to:

void av_log(void* avcl, int level, const char *fmt, ...)
{
    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);
    }
    av_vlog(avcl, level, fmt, vl);
    va_end(vl);
}