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

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

[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.
---
 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
 
-- 
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".

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

[Anton Khirnov]

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.

-- 
Anton Khirnov
_______________________________________________
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".

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

[Lynne via ffmpeg-devel]

[-- Attachment #1.1.1.1: Type: text/plain, Size: 1333 bytes --]

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.

[-- Attachment #1.1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 624 bytes --]

[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 236 bytes --]

[-- Attachment #2: Type: text/plain, Size: 251 bytes --]

_______________________________________________
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".

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

[Anton Khirnov]

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.

-- 
Anton Khirnov
_______________________________________________
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".

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

[Andreas Rheinhardt]

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

_______________________________________________
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".

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

[Lynne via ffmpeg-devel]

[-- Attachment #1.1.1.1: Type: text/plain, Size: 3574 bytes --]

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.

[-- Attachment #1.1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 624 bytes --]

[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 236 bytes --]

[-- Attachment #2: Type: text/plain, Size: 251 bytes --]

_______________________________________________
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".

Re: [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

_______________________________________________
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".

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

[Lynne via ffmpeg-devel]

[-- Attachment #1.1.1.1: Type: text/plain, Size: 4306 bytes --]

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.

[-- Attachment #1.1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 624 bytes --]

[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 236 bytes --]

[-- Attachment #2: Type: text/plain, Size: 251 bytes --]

_______________________________________________
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".

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

[Zhao Zhili]

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

_______________________________________________
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".

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

[Lynne]

[-- Attachment #1: Type: text/plain, Size: 17 bytes --]

Patch attached.


[-- Attachment #2: 0001-lavu-tx-stop-using-av_log-NULL.patch --]
[-- Type: text/x-diff, Size: 2319 bytes --]

From 2813dcb5b885bdf0c3f78f8aead43f4b11149a70 Mon Sep 17 00:00:00 2001
From: Lynne <dev@lynne.ee>
Date: Wed, 24 May 2023 21:57:25 +0200
Subject: [PATCH] lavu/tx: stop using av_log(NULL, )

---
 libavutil/tx.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/libavutil/tx.c b/libavutil/tx.c
index e25abf998f..34fbe3f6c7 100644
--- a/libavutil/tx.c
+++ b/libavutil/tx.c
@@ -29,6 +29,12 @@
      ((x) == AV_TX_DOUBLE_ ## type) || \
      ((x) == AV_TX_INT32_ ## type))
 
+static 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)
 {
@@ -631,7 +637,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, AV_LOG_DEBUG, "%s\n", bp.str);
+    av_log((void *)&tx_class, AV_LOG_DEBUG, "%s\n", bp.str);
 }
 
 static void print_tx_structure(AVTXContext *s, int depth)
@@ -639,7 +645,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);
 
@@ -798,10 +804,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_DEBUG, "%s\n", bp.str);
+    av_log((void *)&tx_class, AV_LOG_DEBUG, "%s\n", bp.str);
 
     for (int i = 0; i < nb_cd_matches; i++) {
-        av_log(NULL, AV_LOG_DEBUG, "    %i: ", i + 1);
+        av_log((void *)&tx_class, AV_LOG_DEBUG, "    %i: ", i + 1);
         print_cd_info(cd_matches[i].cd, cd_matches[i].prio, 0, 1);
     }
 #endif
@@ -909,7 +915,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.40.0


[-- Attachment #3: Type: text/plain, Size: 251 bytes --]

_______________________________________________
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".

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

[Paul B Mahol]

On 5/24/23, Lynne <dev@lynne.ee> wrote:
> Patch attached.
>
>

Probably fine.
_______________________________________________
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".

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

[Leo Izen]

On 5/24/23 16:35, Lynne wrote:
> Patch attached.
> 

+    av_log((void *)&tx_class, AV_LOG_DEBUG, "%s\n", bp.str);

The type of the first argument to av_log should be AVClass **, but this 
only appears to be AVClass *. See libavutil/log.c line 428.

- Leo Izen
_______________________________________________
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".

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

[Lynne]

May 24, 2023, 23:24 by leo.izen@gmail.com:

> On 5/24/23 16:35, Lynne wrote:
>
>> Patch attached.
>>
>
> +    av_log((void *)&tx_class, AV_LOG_DEBUG, "%s\n", bp.str);
>
> The type of the first argument to av_log should be AVClass **, but this only appears to be AVClass *. See libavutil/log.c line 428.
>
> - Leo Izen
>

Right, thanks, changed to:

> static const AVClass tx_class = {
>     .class_name                = "tx",
>     .item_name                 = av_default_item_name,
>     .version                   = LIBAVUTIL_VERSION_INT,
> };
>
> static const struct {
>     const AVClass *tx_class;
> } tx_log = {
>     &tx_class,
> };
Will push this tomorrow.
_______________________________________________
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".

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

[James Almer]

On 5/24/2023 9:32 PM, Lynne wrote:
> May 24, 2023, 23:24 by leo.izen@gmail.com:
> 
>> On 5/24/23 16:35, Lynne wrote:
>>
>>> Patch attached.
>>>
>>
>> +    av_log((void *)&tx_class, AV_LOG_DEBUG, "%s\n", bp.str);
>>
>> The type of the first argument to av_log should be AVClass **, but this only appears to be AVClass *. See libavutil/log.c line 428.
>>
>> - Leo Izen
>>
> 
> Right, thanks, changed to:
> 
>> static const AVClass tx_class = {
>>       .class_name                = "tx",
>>       .item_name                 = av_default_item_name,
>>       .version                   = LIBAVUTIL_VERSION_INT,
>> };
>>
>> static const struct {
>>       const AVClass *tx_class;
>> } tx_log = {
>>       &tx_class,
>> };
> Will push this tomorrow.

Can't add an AVClass* field to AVTXContext and set it to &tx_class 
during init?
_______________________________________________
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".

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

[Lynne]

May 28, 2023, 03:07 by jamrial@gmail.com:

> On 5/24/2023 9:32 PM, Lynne wrote:
>
>> May 24, 2023, 23:24 by leo.izen@gmail.com:
>>
>>> On 5/24/23 16:35, Lynne wrote:
>>>
>>>> Patch attached.
>>>>
>>>
>>> +    av_log((void *)&tx_class, AV_LOG_DEBUG, "%s\n", bp.str);
>>>
>>> The type of the first argument to av_log should be AVClass **, but this only appears to be AVClass *. See libavutil/log.c line 428.
>>>
>>> - Leo Izen
>>>
>>
>> Right, thanks, changed to:
>>
>>> static const AVClass tx_class = {
>>>       .class_name                = "tx",
>>>       .item_name                 = av_default_item_name,
>>>       .version                   = LIBAVUTIL_VERSION_INT,
>>> };
>>>
>>> static const struct {
>>>       const AVClass *tx_class;
>>> } tx_log = {
>>>       &tx_class,
>>> };
>>>
>> Will push this tomorrow.
>>
>
> Can't add an AVClass* field to AVTXContext and set it to &tx_class during init?
>

The struct is accessed from asm, didn't really want to fix all the loads
for something which only runs at init, and only if !CONFIG_SMALL.
_______________________________________________
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".

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

[Anton Khirnov]

Quoting Lynne (2023-05-28 04:48:00)
> May 28, 2023, 03:07 by jamrial@gmail.com:
> 
> > On 5/24/2023 9:32 PM, Lynne wrote:
> >
> >> May 24, 2023, 23:24 by leo.izen@gmail.com:
> >>
> >>> On 5/24/23 16:35, Lynne wrote:
> >>>
> >>>> Patch attached.
> >>>>
> >>>
> >>> +    av_log((void *)&tx_class, AV_LOG_DEBUG, "%s\n", bp.str);
> >>>
> >>> The type of the first argument to av_log should be AVClass **, but this only appears to be AVClass *. See libavutil/log.c line 428.
> >>>
> >>> - Leo Izen
> >>>
> >>
> >> Right, thanks, changed to:
> >>
> >>> static const AVClass tx_class = {
> >>>       .class_name                = "tx",
> >>>       .item_name                 = av_default_item_name,
> >>>       .version                   = LIBAVUTIL_VERSION_INT,
> >>> };
> >>>
> >>> static const struct {
> >>>       const AVClass *tx_class;
> >>> } tx_log = {
> >>>       &tx_class,
> >>> };
> >>>
> >> Will push this tomorrow.
> >>
> >
> > Can't add an AVClass* field to AVTXContext and set it to &tx_class during init?
> >
> 
> The struct is accessed from asm, didn't really want to fix all the loads
> for something which only runs at init, and only if !CONFIG_SMALL.

That's largely missing the point of having a non-NULL logging context,
which is that the log callback has access to the context. It's private
in this case, but AVOptions could still be used to retrieve things like
per-context opaque.

There have also been discussions recently about a per-context logging
callback being highly useful for some cases.

-- 
Anton Khirnov
_______________________________________________
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".