[FFmpeg-devel] [PATCH 1/5] avutil/pixdesc: Remove always-false checks

[FFmpeg-devel] [PATCH 1/5] avutil/pixdesc: Remove always-false checks

[Andreas Rheinhardt]

ff_check_pixfmt_descriptors() was added in commit
20e99a9c10cdbe9ad659dce5bdec569d744f8219. At this time,
the values of enum AVPixelFormat were not contiguous;
instead there was a jump from 111 to 291 (or from 115
to 295 depending upon AV_PIX_FMT_ABI_GIT_MASTER).
ff_check_pixfmt_descriptors() accounts for this
by skipping empty descriptors. Yet this issue no longer
exists: There are no holes.

The check for said holes makes GCC believe that the name
can be NULL; because it is used as argument corresponding to
%s in a log statement, it therefore emits a warning
(since d75c4693fef51e8f0a1b88798530f4c5147ea906). Therefore
this commit simply removes these checks.

Also move the checks for name before the log statement.

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
---
 libavutil/pixdesc.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/libavutil/pixdesc.c b/libavutil/pixdesc.c
index 3ac44614a7..c42a0242c5 100644
--- a/libavutil/pixdesc.c
+++ b/libavutil/pixdesc.c
@@ -2922,13 +2922,11 @@ void ff_check_pixfmt_descriptors(void){
         int linesize[4] = {0,0,0,0};
         uint16_t tmp[2];
 
-        if (!d->name && !d->nb_components && !d->log2_chroma_w && !d->log2_chroma_h && !d->flags)
-            continue;
+        av_assert0(d->name && d->name[0]);
         av_log(NULL, AV_LOG_INFO, "Checking: %s\n", d->name);
         av_assert0(d->log2_chroma_w <= 3);
         av_assert0(d->log2_chroma_h <= 3);
         av_assert0(d->nb_components <= 4);
-        av_assert0(d->name && d->name[0]);
         av_assert2(av_get_pix_fmt(d->name) == i);
 
         for (j=0; j<FF_ARRAY_ELEMS(d->comp); j++) {
-- 
2.34.1

_______________________________________________
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 2/5] avutil/pixdesc: Avoid direct access to pix fmt desc array

[Andreas Rheinhardt]

Instead use av_pix_fmt_desc_next(). It is still possible
to check its return values by comparing it with the
(currently) expected values and the code does so.

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
---
 libavutil/pixdesc.c | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

diff --git a/libavutil/pixdesc.c b/libavutil/pixdesc.c
index c42a0242c5..f6755f41df 100644
--- a/libavutil/pixdesc.c
+++ b/libavutil/pixdesc.c
@@ -2913,10 +2913,10 @@ int av_pix_fmt_count_planes(enum AVPixelFormat pix_fmt)
 }
 
 void ff_check_pixfmt_descriptors(void){
-    int i, j;
+    const AVPixFmtDescriptor *d, *last = NULL;
+    int i;
 
-    for (i=0; i<FF_ARRAY_ELEMS(av_pix_fmt_descriptors); i++) {
-        const AVPixFmtDescriptor *d = &av_pix_fmt_descriptors[i];
+    for (i = AV_PIX_FMT_NONE, d = NULL; i++, d = av_pix_fmt_desc_next(d);) {
         uint8_t fill[4][8+6+3] = {{0}};
         uint8_t *data[4] = {fill[0], fill[1], fill[2], fill[3]};
         int linesize[4] = {0,0,0,0};
@@ -2927,9 +2927,15 @@ void ff_check_pixfmt_descriptors(void){
         av_assert0(d->log2_chroma_w <= 3);
         av_assert0(d->log2_chroma_h <= 3);
         av_assert0(d->nb_components <= 4);
-        av_assert2(av_get_pix_fmt(d->name) == i);
+        av_assert2(av_get_pix_fmt(d->name) == av_pix_fmt_desc_get_id(d));
+
+        /* The following two checks as well as the one after the loop
+         * would need to be changed if we changed the way the descriptors
+         * are stored. */
+        av_assert0(i == av_pix_fmt_desc_get_id(d));
+        av_assert0(!last || last + 1 == d);
 
-        for (j=0; j<FF_ARRAY_ELEMS(d->comp); j++) {
+        for (int j = 0; j < FF_ARRAY_ELEMS(d->comp); j++) {
             const AVComponentDescriptor *c = &d->comp[j];
             if(j>=d->nb_components) {
                 av_assert0(!c->plane && !c->step && !c->offset && !c->shift && !c->depth);
@@ -2948,6 +2954,7 @@ void ff_check_pixfmt_descriptors(void){
             av_write_image_line(tmp, data, linesize, d, 0, 0, j, 2);
         }
     }
+    av_assert0(i == AV_PIX_FMT_NB);
 }
 
 
-- 
2.34.1

_______________________________________________
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 2/5] avutil/pixdesc: Avoid direct access to pix fmt desc array

[Anton Khirnov]

Quoting Andreas Rheinhardt (2022-09-26 21:58:55)
> Instead use av_pix_fmt_desc_next(). It is still possible
> to check its return values by comparing it with the
> (currently) expected values and the code does so.
> 
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
> ---
>  libavutil/pixdesc.c | 17 ++++++++++++-----
>  1 file changed, 12 insertions(+), 5 deletions(-)
> 
> diff --git a/libavutil/pixdesc.c b/libavutil/pixdesc.c
> index c42a0242c5..f6755f41df 100644
> --- a/libavutil/pixdesc.c
> +++ b/libavutil/pixdesc.c
> @@ -2913,10 +2913,10 @@ int av_pix_fmt_count_planes(enum AVPixelFormat pix_fmt)
>  }
>  
>  void ff_check_pixfmt_descriptors(void){
> -    int i, j;
> +    const AVPixFmtDescriptor *d, *last = NULL;
> +    int i;
>  
> -    for (i=0; i<FF_ARRAY_ELEMS(av_pix_fmt_descriptors); i++) {
> -        const AVPixFmtDescriptor *d = &av_pix_fmt_descriptors[i];
> +    for (i = AV_PIX_FMT_NONE, d = NULL; i++, d = av_pix_fmt_desc_next(d);) {
>          uint8_t fill[4][8+6+3] = {{0}};
>          uint8_t *data[4] = {fill[0], fill[1], fill[2], fill[3]};
>          int linesize[4] = {0,0,0,0};
> @@ -2927,9 +2927,15 @@ void ff_check_pixfmt_descriptors(void){
>          av_assert0(d->log2_chroma_w <= 3);
>          av_assert0(d->log2_chroma_h <= 3);
>          av_assert0(d->nb_components <= 4);
> -        av_assert2(av_get_pix_fmt(d->name) == i);
> +        av_assert2(av_get_pix_fmt(d->name) == av_pix_fmt_desc_get_id(d));
> +
> +        /* The following two checks as well as the one after the loop
> +         * would need to be changed if we changed the way the descriptors
> +         * are stored. */
> +        av_assert0(i == av_pix_fmt_desc_get_id(d));
> +        av_assert0(!last || last + 1 == d);

Don't see last being set.

-- 
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 2/5] avutil/pixdesc: Avoid direct access to pix fmt desc array

[Andreas Rheinhardt]

Anton Khirnov:
> Quoting Andreas Rheinhardt (2022-09-26 21:58:55)
>> Instead use av_pix_fmt_desc_next(). It is still possible
>> to check its return values by comparing it with the
>> (currently) expected values and the code does so.
>>
>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
>> ---
>>  libavutil/pixdesc.c | 17 ++++++++++++-----
>>  1 file changed, 12 insertions(+), 5 deletions(-)
>>
>> diff --git a/libavutil/pixdesc.c b/libavutil/pixdesc.c
>> index c42a0242c5..f6755f41df 100644
>> --- a/libavutil/pixdesc.c
>> +++ b/libavutil/pixdesc.c
>> @@ -2913,10 +2913,10 @@ int av_pix_fmt_count_planes(enum AVPixelFormat pix_fmt)
>>  }
>>  
>>  void ff_check_pixfmt_descriptors(void){
>> -    int i, j;
>> +    const AVPixFmtDescriptor *d, *last = NULL;
>> +    int i;
>>  
>> -    for (i=0; i<FF_ARRAY_ELEMS(av_pix_fmt_descriptors); i++) {
>> -        const AVPixFmtDescriptor *d = &av_pix_fmt_descriptors[i];
>> +    for (i = AV_PIX_FMT_NONE, d = NULL; i++, d = av_pix_fmt_desc_next(d);) {
>>          uint8_t fill[4][8+6+3] = {{0}};
>>          uint8_t *data[4] = {fill[0], fill[1], fill[2], fill[3]};
>>          int linesize[4] = {0,0,0,0};
>> @@ -2927,9 +2927,15 @@ void ff_check_pixfmt_descriptors(void){
>>          av_assert0(d->log2_chroma_w <= 3);
>>          av_assert0(d->log2_chroma_h <= 3);
>>          av_assert0(d->nb_components <= 4);
>> -        av_assert2(av_get_pix_fmt(d->name) == i);
>> +        av_assert2(av_get_pix_fmt(d->name) == av_pix_fmt_desc_get_id(d));
>> +
>> +        /* The following two checks as well as the one after the loop
>> +         * would need to be changed if we changed the way the descriptors
>> +         * are stored. */
>> +        av_assert0(i == av_pix_fmt_desc_get_id(d));
>> +        av_assert0(!last || last + 1 == d);
> 
> Don't see last being set.
> 

Correct. Will fix.

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

[FFmpeg-devel] [PATCH 3/5] avutil/pixdesc: Move ff_check_pixfmt_descriptors() to its only user

[Andreas Rheinhardt]

Namely to lavu/tests/pixelutils.c. This way, this function will
not be included into actual binaries any more.

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
---
 libavutil/internal.h         |  2 --
 libavutil/pixdesc.c          | 48 ---------------------------------
 libavutil/tests/pixelutils.c | 52 +++++++++++++++++++++++++++++++++++-
 3 files changed, 51 insertions(+), 51 deletions(-)

diff --git a/libavutil/internal.h b/libavutil/internal.h
index c9e30bc5e9..454c59aa50 100644
--- a/libavutil/internal.h
+++ b/libavutil/internal.h
@@ -187,6 +187,4 @@ static av_always_inline av_const int avpriv_mirror(int x, int w)
     return x;
 }
 
-void ff_check_pixfmt_descriptors(void);
-
 #endif /* AVUTIL_INTERNAL_H */
diff --git a/libavutil/pixdesc.c b/libavutil/pixdesc.c
index f6755f41df..ca3e204a0b 100644
--- a/libavutil/pixdesc.c
+++ b/libavutil/pixdesc.c
@@ -22,12 +22,10 @@
 #include <stdio.h>
 #include <string.h>
 
-#include "avassert.h"
 #include "avstring.h"
 #include "common.h"
 #include "pixfmt.h"
 #include "pixdesc.h"
-#include "internal.h"
 #include "intreadwrite.h"
 
 void av_read_image_line2(void *dst,
@@ -2912,52 +2910,6 @@ int av_pix_fmt_count_planes(enum AVPixelFormat pix_fmt)
     return ret;
 }
 
-void ff_check_pixfmt_descriptors(void){
-    const AVPixFmtDescriptor *d, *last = NULL;
-    int i;
-
-    for (i = AV_PIX_FMT_NONE, d = NULL; i++, d = av_pix_fmt_desc_next(d);) {
-        uint8_t fill[4][8+6+3] = {{0}};
-        uint8_t *data[4] = {fill[0], fill[1], fill[2], fill[3]};
-        int linesize[4] = {0,0,0,0};
-        uint16_t tmp[2];
-
-        av_assert0(d->name && d->name[0]);
-        av_log(NULL, AV_LOG_INFO, "Checking: %s\n", d->name);
-        av_assert0(d->log2_chroma_w <= 3);
-        av_assert0(d->log2_chroma_h <= 3);
-        av_assert0(d->nb_components <= 4);
-        av_assert2(av_get_pix_fmt(d->name) == av_pix_fmt_desc_get_id(d));
-
-        /* The following two checks as well as the one after the loop
-         * would need to be changed if we changed the way the descriptors
-         * are stored. */
-        av_assert0(i == av_pix_fmt_desc_get_id(d));
-        av_assert0(!last || last + 1 == d);
-
-        for (int j = 0; j < FF_ARRAY_ELEMS(d->comp); j++) {
-            const AVComponentDescriptor *c = &d->comp[j];
-            if(j>=d->nb_components) {
-                av_assert0(!c->plane && !c->step && !c->offset && !c->shift && !c->depth);
-                continue;
-            }
-            if (d->flags & AV_PIX_FMT_FLAG_BITSTREAM) {
-                av_assert0(c->step >= c->depth);
-            } else {
-                av_assert0(8*c->step >= c->depth);
-            }
-            if (d->flags & AV_PIX_FMT_FLAG_BAYER)
-                continue;
-            av_read_image_line(tmp, (void*)data, linesize, d, 0, 0, j, 2, 0);
-            av_assert0(tmp[0] == 0 && tmp[1] == 0);
-            tmp[0] = tmp[1] = (1ULL << c->depth) - 1;
-            av_write_image_line(tmp, data, linesize, d, 0, 0, j, 2);
-        }
-    }
-    av_assert0(i == AV_PIX_FMT_NB);
-}
-
-
 enum AVPixelFormat av_pix_fmt_swap_endianness(enum AVPixelFormat pix_fmt)
 {
     const AVPixFmtDescriptor *desc = av_pix_fmt_desc_get(pix_fmt);
diff --git a/libavutil/tests/pixelutils.c b/libavutil/tests/pixelutils.c
index 927c8d9217..6e5e8cf738 100644
--- a/libavutil/tests/pixelutils.c
+++ b/libavutil/tests/pixelutils.c
@@ -18,15 +18,65 @@
 
 #include <stdio.h>
 
+#include "libavutil/avassert.h"
 #include "libavutil/internal.h"
+#include "libavutil/log.h"
 #include "libavutil/mem.h"
+#include "libavutil/pixdesc.h"
 #include "libavutil/pixelutils.c"
+#include "libavutil/pixfmt.h"
 
 #define W1 320
 #define H1 240
 #define W2 640
 #define H2 480
 
+static void check_pixfmt_descriptors(void)
+{
+    const AVPixFmtDescriptor *d, *last = NULL;
+    int i;
+
+    for (i = AV_PIX_FMT_NONE, d = NULL; i++, d = av_pix_fmt_desc_next(d);) {
+        uint8_t fill[4][8 + 6 + 3] = {{ 0 }};
+        uint8_t *data[4] = { fill[0], fill[1], fill[2], fill[3] };
+        int linesize[4] = { 0, 0, 0, 0 };
+        uint16_t tmp[2];
+
+        av_assert0(d->name && d->name[0]);
+        av_log(NULL, AV_LOG_INFO, "Checking: %s\n", d->name);
+        av_assert0(d->log2_chroma_w <= 3);
+        av_assert0(d->log2_chroma_h <= 3);
+        av_assert0(d->nb_components <= 4);
+        av_assert2(av_get_pix_fmt(d->name) == av_pix_fmt_desc_get_id(d));
+
+        /* The following two checks as well as the one after the loop
+         * would need to be changed if we changed the way the descriptors
+         * are stored. */
+        av_assert0(i == av_pix_fmt_desc_get_id(d));
+        av_assert0(!last || last + 1 == d);
+
+        for (int j = 0; j < FF_ARRAY_ELEMS(d->comp); j++) {
+            const AVComponentDescriptor *c = &d->comp[j];
+            if (j >= d->nb_components) {
+                av_assert0(!c->plane && !c->step && !c->offset && !c->shift && !c->depth);
+                continue;
+            }
+            if (d->flags & AV_PIX_FMT_FLAG_BITSTREAM) {
+                av_assert0(c->step >= c->depth);
+            } else {
+                av_assert0(8*c->step >= c->depth);
+            }
+            if (d->flags & AV_PIX_FMT_FLAG_BAYER)
+                continue;
+            av_read_image_line(tmp, (void*)data, linesize, d, 0, 0, j, 2, 0);
+            av_assert0(tmp[0] == 0 && tmp[1] == 0);
+            tmp[0] = tmp[1] = (1ULL << c->depth) - 1;
+            av_write_image_line(tmp, data, linesize, d, 0, 0, j, 2);
+        }
+    }
+    av_assert0(i == AV_PIX_FMT_NB);
+}
+
 static int run_single_test(const char *test,
                            const uint8_t *block1, ptrdiff_t stride1,
                            const uint8_t *block2, ptrdiff_t stride2,
@@ -87,7 +137,7 @@ int main(void)
         goto end;
     }
 
-    ff_check_pixfmt_descriptors();
+    check_pixfmt_descriptors();
 
 #define RANDOM_INIT(buf, size) do {             \
     int k;                                      \
-- 
2.34.1

_______________________________________________
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 4/5] avutil/tests/pixelutils: Use av_assert0 instead for test tools

[Andreas Rheinhardt]

These are test tools, so they should be picky.

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
---
 libavutil/tests/pixelutils.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/libavutil/tests/pixelutils.c b/libavutil/tests/pixelutils.c
index 6e5e8cf738..2d9c4edc47 100644
--- a/libavutil/tests/pixelutils.c
+++ b/libavutil/tests/pixelutils.c
@@ -47,7 +47,7 @@ static void check_pixfmt_descriptors(void)
         av_assert0(d->log2_chroma_w <= 3);
         av_assert0(d->log2_chroma_h <= 3);
         av_assert0(d->nb_components <= 4);
-        av_assert2(av_get_pix_fmt(d->name) == av_pix_fmt_desc_get_id(d));
+        av_assert0(av_get_pix_fmt(d->name) == av_pix_fmt_desc_get_id(d));
 
         /* The following two checks as well as the one after the loop
          * would need to be changed if we changed the way the descriptors
-- 
2.34.1

_______________________________________________
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 5/5] avutil/tests/pixelutils: Test that all non-hw pix fmts have components

[Andreas Rheinhardt]

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
---
 libavutil/tests/pixelutils.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/libavutil/tests/pixelutils.c b/libavutil/tests/pixelutils.c
index 2d9c4edc47..548ecb8801 100644
--- a/libavutil/tests/pixelutils.c
+++ b/libavutil/tests/pixelutils.c
@@ -47,6 +47,7 @@ static void check_pixfmt_descriptors(void)
         av_assert0(d->log2_chroma_w <= 3);
         av_assert0(d->log2_chroma_h <= 3);
         av_assert0(d->nb_components <= 4);
+        av_assert0(d->nb_components || (d->flags & AV_PIX_FMT_FLAG_HWACCEL));
         av_assert0(av_get_pix_fmt(d->name) == av_pix_fmt_desc_get_id(d));
 
         /* The following two checks as well as the one after the loop
-- 
2.34.1

_______________________________________________
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 1/5] avutil/pixdesc: Remove always-false checks

[Andreas Rheinhardt]

Andreas Rheinhardt:
> ff_check_pixfmt_descriptors() was added in commit
> 20e99a9c10cdbe9ad659dce5bdec569d744f8219. At this time,
> the values of enum AVPixelFormat were not contiguous;
> instead there was a jump from 111 to 291 (or from 115
> to 295 depending upon AV_PIX_FMT_ABI_GIT_MASTER).
> ff_check_pixfmt_descriptors() accounts for this
> by skipping empty descriptors. Yet this issue no longer
> exists: There are no holes.
> 
> The check for said holes makes GCC believe that the name
> can be NULL; because it is used as argument corresponding to
> %s in a log statement, it therefore emits a warning
> (since d75c4693fef51e8f0a1b88798530f4c5147ea906). Therefore
> this commit simply removes these checks.
> 
> Also move the checks for name before the log statement.
> 
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
> ---
>  libavutil/pixdesc.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/libavutil/pixdesc.c b/libavutil/pixdesc.c
> index 3ac44614a7..c42a0242c5 100644
> --- a/libavutil/pixdesc.c
> +++ b/libavutil/pixdesc.c
> @@ -2922,13 +2922,11 @@ void ff_check_pixfmt_descriptors(void){
>          int linesize[4] = {0,0,0,0};
>          uint16_t tmp[2];
>  
> -        if (!d->name && !d->nb_components && !d->log2_chroma_w && !d->log2_chroma_h && !d->flags)
> -            continue;
> +        av_assert0(d->name && d->name[0]);
>          av_log(NULL, AV_LOG_INFO, "Checking: %s\n", d->name);
>          av_assert0(d->log2_chroma_w <= 3);
>          av_assert0(d->log2_chroma_h <= 3);
>          av_assert0(d->nb_components <= 4);
> -        av_assert0(d->name && d->name[0]);
>          av_assert2(av_get_pix_fmt(d->name) == i);
>  
>          for (j=0; j<FF_ARRAY_ELEMS(d->comp); j++) {

Will apply this patchset (with the issue pointed out by Anton fixed)
tomorrow unless there are objections.

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