diff mbox series

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

Message ID 20240726064216.893951-1-dev@lynne.ee
State New
Headers show
Series [FFmpeg-devel] lavu/tx: stop using av_log(NULL, ) | expand

Checks

Context Check Description
andriy/make_x86 success Make finished
andriy/make_fate_x86 fail Make fate failed

Commit Message

Lynne July 26, 2024, 6:42 a.m. UTC 
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.
---
 libavutil/tx.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

Comments

Anton Khirnov July 26, 2024, 7:47 a.m. UTC | #1 
Quoting Lynne via ffmpeg-devel (2024-07-26 08:42:11)
> 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.

No, it just hides the issue for the time being. I am against this patch,
just add a proper AVClass. AVTXContext is entirely opaque, so it should
definitely be feasible.
Andreas Rheinhardt July 26, 2024, 8:22 a.m. UTC | #2 
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.

> ---
>  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).

- Andreas
Lynne July 26, 2024, 8:30 a.m. UTC | #3 
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.
Lynne July 26, 2024, 8:33 a.m. UTC | #4 
On 26/07/2024 09:47, Anton Khirnov wrote:
> Quoting Lynne via ffmpeg-devel (2024-07-26 08:42:11)
>> 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.
> 
> No, it just hides the issue for the time being.

If this means "it may get broken eventually, its not forbidden 
anywhere", then IMO we should just codify the current behavior such that 
it won't, unless there's some use-case you can think of.

> I am against this patch, just add a proper AVClass. AVTXContext is
> entirely opaque, so it should definitely be feasible.
I'd like to avoid adding a pointer and allocating it if it can't be 
helped. And properly integrating each context into the AVClass system as 
a child of the parent context.

If you think a NULL av_log is valid (you implied that a year ago), then 
I'm more than happy to drop this patch.
Zhao Zhili July 26, 2024, 8:37 a.m. UTC | #5 
> On Jul 26, 2024, at 14:42, Lynne via ffmpeg-devel <ffmpeg-devel@ffmpeg.org> wrote:
> 
> 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.
> ---
> 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);

Isn’t the first argument of av_log is a pointer to pointer to AVClass, not pointer to AVClass?

> }
> 
> 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
> 
> -- 
> 2.45.2.753.g447d99e1c3b
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
Anton Khirnov July 26, 2024, 8:43 a.m. UTC | #6 
Quoting Lynne via ffmpeg-devel (2024-07-26 10:33:24)
> On 26/07/2024 09:47, Anton Khirnov wrote:
> > Quoting Lynne via ffmpeg-devel (2024-07-26 08:42:11)
> >> 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.
> > 
> > No, it just hides the issue for the time being.
> 
> If this means "it may get broken eventually, its not forbidden 
> anywhere", then IMO we should just codify the current behavior such that 
> it won't, unless there's some use-case you can think of.

It means "you're offloading fixing this issue properly on someone else",
which is a bad thing in my book. In my understanding of the API, it _is_
forbidden and UB.

> > I am against this patch, just add a proper AVClass. AVTXContext is
> > entirely opaque, so it should definitely be feasible.
> I'd like to avoid adding a pointer and allocating it if it can't be 
> helped. And properly integrating each context into the AVClass system as 
> a child of the parent context.

Consider the possibility that if your code depends on fixed layout of a
big and complex struct, then your code is...suboptimal. If it's about
offsets for SIMD, you could either
* generate them at build time
* move the things used by SIMD into a smaller struct embedded in the
  main context

> If you think a NULL av_log is valid (you implied that a year ago), then 
> I'm more than happy to drop this patch.

I didn't and I don't. To the contrary, I consider av_log(NULL) a
mispattern that we should be rid of.
Andreas Rheinhardt July 26, 2024, 9:03 a.m. UTC | #7 
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
Lynne July 26, 2024, 11:42 a.m. UTC | #8 
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.
diff mbox series

Patch

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