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

[Lynne]

On 26/07/2024 11:03, Andreas Rheinhardt wrote:
> 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).

There was humor in my response, maybe you didn't see it.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: OpenPGP_0xA2FEA5F03F034464.asc
Type: application/pgp-keys
Size: 624 bytes
Desc: OpenPGP public key
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20240726/8b2d5485/attachment.key>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: OpenPGP_signature.asc
Type: application/pgp-signature
Size: 236 bytes
Desc: OpenPGP digital signature
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20240726/8b2d5485/attachment.sig>