[FFmpeg-devel] [PATCH] lavu/tx: stop using av_log(NULL, )

[Andreas Rheinhardt]

Lynne via ffmpeg-devel:
> On 26/07/2024 10:22, Andreas Rheinhardt wrote:
>> Lynne via ffmpeg-devel:
>>> Its not feasible to add an AVClass in the main context, as
>>> it would waste space, as the main context is recursive, and
>>> every bit of assembly would need to be changed.
>>>
>>> While its true that on paper av_log has access to the main
>>> context, that functionality is not used as no options are
>>> available for setting. No options will be exposed either,
>>> and it makes no sense.
>>>
>>> mpv has recently started warning if a NULL AVClass is used
>>> as an FFmpeg bug. While I don't fully agree nor disagree with
>>> this, this is a simple patch which fixes the issue.
>>
>> Really?
>> https://github.com/mpv-player/mpv/commit/54d0763b92f3d8239aa2258f2193eebdc74a91ef
>> is 13 years old and the check would only warn if a logcontext with NULL
>> AVClass* is used.
> 
> Odd, something started triggering the check on my system.
> 
>>>   libavutil/tx.c | 16 +++++++++++-----
>>>   1 file changed, 11 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/libavutil/tx.c b/libavutil/tx.c
>>> index 0aae4c7cf7..136d10c374 100644
>>> --- a/libavutil/tx.c
>>> +++ b/libavutil/tx.c
>>> @@ -30,6 +30,12 @@
>>>        ((x) == AV_TX_DOUBLE_ ## type) || \
>>>        ((x) == AV_TX_INT32_ ## type))
>>>   +static const AVClass tx_class = {
>>> +    .class_name                = "tx",
>>> +    .item_name                 = av_default_item_name,
>>> +    .version                   = LIBAVUTIL_VERSION_INT,
>>> +};
>>> +
>>>   /* Calculates the modular multiplicative inverse */
>>>   static av_always_inline int mulinv(int n, int m)
>>>   {
>>> @@ -645,7 +651,7 @@ static void print_cd_info(const FFTXCodelet *cd,
>>> int prio, int len, int print_pr
>>>       if (print_prio)
>>>           av_bprintf(&bp, ", prio: %i", prio);
>>>   -    av_log(NULL, log_level, "%s\n", bp.str);
>>> +    av_log((void *)&tx_class, log_level, "%s\n", bp.str);
>>>   }
>>>     static void print_tx_structure(AVTXContext *s, int depth)
>>> @@ -653,7 +659,7 @@ static void print_tx_structure(AVTXContext *s,
>>> int depth)
>>>       const FFTXCodelet *cd = s->cd_self;
>>>         for (int i = 0; i <= depth; i++)
>>> -        av_log(NULL, AV_LOG_DEBUG, "    ");
>>> +        av_log((void *)&tx_class, AV_LOG_DEBUG, "    ");
>>>         print_cd_info(cd, cd->prio, s->len, 0, AV_LOG_DEBUG);
>>>   @@ -818,10 +824,10 @@ av_cold int ff_tx_init_subtx(AVTXContext *s,
>>> enum AVTXType type,
>>>       AV_QSORT(cd_matches, nb_cd_matches, TXCodeletMatch, cmp_matches);
>>>     #if !CONFIG_SMALL
>>> -    av_log(NULL, AV_LOG_TRACE, "%s\n", bp.str);
>>> +    av_log((void *)&tx_class, AV_LOG_TRACE, "%s\n", bp.str);
>>>         for (int i = 0; i < nb_cd_matches; i++) {
>>> -        av_log(NULL, AV_LOG_TRACE, "    %i: ", i + 1);
>>> +        av_log((void *)&tx_class, AV_LOG_TRACE, "    %i: ", i + 1);
>>>           print_cd_info(cd_matches[i].cd, cd_matches[i].prio, 0, 1,
>>> AV_LOG_TRACE);
>>>       }
>>>   #endif
>>> @@ -931,7 +937,7 @@ av_cold int av_tx_init(AVTXContext **ctx,
>>> av_tx_fn *tx, enum AVTXType type,
>>>       *tx  = tmp.fn[0];
>>>     #if !CONFIG_SMALL
>>> -    av_log(NULL, AV_LOG_DEBUG, "Transform tree:\n");
>>> +    av_log((void *)&tx_class, AV_LOG_DEBUG, "Transform tree:\n");
>>>       print_tx_structure(*ctx, 0);
>>>   #endif
>>>   
>>
>> Did you ever test this? av_log() expects a pointer to an AVClass-enabled
>> struct, not a pointer to an AVClass. This will crash (it will interpret
>> AVClass.class_name as pointer to an AVClass) when the log is active
>> (when loglevel is high enough).
> 
> No, I trusted that I did test it when I submitted it a year ago.
> 

You have been informed of this last year:
https://patchwork.ffmpeg.org/project/ffmpeg/patch/NWETFS_--3-9@lynne.ee/
Then as now there are lots of FATE failures with this patch (as
patchwork shows).

- Andreas