diff mbox series

[FFmpeg-devel,01/10] fftools/cmdutils: Use avfilter_pad_count() for AVFilter's number of pads

Message ID AM7PR03MB666047448111C24F9EC280BC8FFC9@AM7PR03MB6660.eurprd03.prod.outlook.com
State Accepted
Headers show
Series [FFmpeg-devel,01/10] fftools/cmdutils: Use avfilter_pad_count() for AVFilter's number of pads
Related show

Checks

Context Check Description
andriy/x86_make success Make finished
andriy/x86_make_fate success Make fate finished
andriy/PPC64_make success Make finished
andriy/PPC64_make_fate success Make fate finished

Commit Message

Andreas Rheinhardt Aug. 15, 2021, 9:29 a.m. UTC
Besides being nicer code this also has the advantage of not making
assumptions about the internal implementation: While it is documented
that the AVFilter.inputs and AVFilter.outputs arrays are terminated
by a zeroed sentinel, one is not allowed to infer that one can just
check avfilter_pad_get_name(padarray, i) to see whether one has reached
the sentinel:
It could be that the pointer to the string is contained
in a different structure than AVFilterPad that needs to be accessed
first: return pad->struct->string.
It could be that for small strings an internal buffer in
AVFilterPad is used (to avoid a relocation) whereas for longer strings
an external string is used; this is useful to avoid relocations:
return pad->string_ptr ? pad->string_ptr : pad->interal_string
Or it could be that the name has a default value:
return pad->name ? pad->name : "default"
(This actually made sense for us because the name of most of our
AVFilterPads is just "default"; doing so would save lots of relocations.)

The only thing one is allowed to infer from the existence of the
sentinel is that one is allowed to use avfilter_pad_count() to get
the number of pads. Therefore it is used.

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
---
Unchanged since last time.

 fftools/cmdutils.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

Comments

Andreas Rheinhardt Aug. 20, 2021, 7:29 a.m. UTC | #1
Andreas Rheinhardt:
> Besides being nicer code this also has the advantage of not making
> assumptions about the internal implementation: While it is documented
> that the AVFilter.inputs and AVFilter.outputs arrays are terminated
> by a zeroed sentinel, one is not allowed to infer that one can just
> check avfilter_pad_get_name(padarray, i) to see whether one has reached
> the sentinel:
> It could be that the pointer to the string is contained
> in a different structure than AVFilterPad that needs to be accessed
> first: return pad->struct->string.
> It could be that for small strings an internal buffer in
> AVFilterPad is used (to avoid a relocation) whereas for longer strings
> an external string is used; this is useful to avoid relocations:
> return pad->string_ptr ? pad->string_ptr : pad->interal_string
> Or it could be that the name has a default value:
> return pad->name ? pad->name : "default"
> (This actually made sense for us because the name of most of our
> AVFilterPads is just "default"; doing so would save lots of relocations.)
> 
> The only thing one is allowed to infer from the existence of the
> sentinel is that one is allowed to use avfilter_pad_count() to get
> the number of pads. Therefore it is used.
> 
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
> ---
> Unchanged since last time.
> 
>  fftools/cmdutils.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/fftools/cmdutils.c b/fftools/cmdutils.c
> index 6d0bcd6085..96d38803df 100644
> --- a/fftools/cmdutils.c
> +++ b/fftools/cmdutils.c
> @@ -1707,12 +1707,14 @@ int show_filters(void *optctx, const char *opt, const char *arg)
>      while ((filter = av_filter_iterate(&opaque))) {
>          descr_cur = descr;
>          for (i = 0; i < 2; i++) {
> +            unsigned nb_pads;
>              if (i) {
>                  *(descr_cur++) = '-';
>                  *(descr_cur++) = '>';
>              }
>              pad = i ? filter->outputs : filter->inputs;
> -            for (j = 0; pad && avfilter_pad_get_name(pad, j); j++) {
> +            nb_pads = avfilter_pad_count(pad);
> +            for (j = 0; j < nb_pads; j++) {
>                  if (descr_cur >= descr + sizeof(descr) - 4)
>                      break;
>                  *(descr_cur++) = get_media_type_char(avfilter_pad_get_type(pad, j));
> 
Ping for this set.

- Andreas
Nicolas George Aug. 20, 2021, 9:10 a.m. UTC | #2
Andreas Rheinhardt (12021-08-15):
> Besides being nicer code this also has the advantage of not making
> assumptions about the internal implementation: While it is documented
> that the AVFilter.inputs and AVFilter.outputs arrays are terminated
> by a zeroed sentinel, one is not allowed to infer that one can just
> check avfilter_pad_get_name(padarray, i) to see whether one has reached
> the sentinel:
> It could be that the pointer to the string is contained
> in a different structure than AVFilterPad that needs to be accessed
> first: return pad->struct->string.
> It could be that for small strings an internal buffer in
> AVFilterPad is used (to avoid a relocation) whereas for longer strings
> an external string is used; this is useful to avoid relocations:
> return pad->string_ptr ? pad->string_ptr : pad->interal_string
> Or it could be that the name has a default value:
> return pad->name ? pad->name : "default"
> (This actually made sense for us because the name of most of our
> AVFilterPads is just "default"; doing so would save lots of relocations.)
> 
> The only thing one is allowed to infer from the existence of the
> sentinel is that one is allowed to use avfilter_pad_count() to get
> the number of pads. Therefore it is used.
> 
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
> ---
> Unchanged since last time.
> 
>  fftools/cmdutils.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)


It means this bit of code will be inefficient for a few commits that
will be pushed at the same time. I am ok with it. I do not maintain
this, but LGTM.


> diff --git a/fftools/cmdutils.c b/fftools/cmdutils.c
> index 6d0bcd6085..96d38803df 100644
> --- a/fftools/cmdutils.c
> +++ b/fftools/cmdutils.c
> @@ -1707,12 +1707,14 @@ int show_filters(void *optctx, const char *opt, const char *arg)
>      while ((filter = av_filter_iterate(&opaque))) {
>          descr_cur = descr;
>          for (i = 0; i < 2; i++) {
> +            unsigned nb_pads;
>              if (i) {
>                  *(descr_cur++) = '-';
>                  *(descr_cur++) = '>';
>              }
>              pad = i ? filter->outputs : filter->inputs;
> -            for (j = 0; pad && avfilter_pad_get_name(pad, j); j++) {
> +            nb_pads = avfilter_pad_count(pad);
> +            for (j = 0; j < nb_pads; j++) {
>                  if (descr_cur >= descr + sizeof(descr) - 4)
>                      break;
>                  *(descr_cur++) = get_media_type_char(avfilter_pad_get_type(pad, j));

Regards,
diff mbox series

Patch

diff --git a/fftools/cmdutils.c b/fftools/cmdutils.c
index 6d0bcd6085..96d38803df 100644
--- a/fftools/cmdutils.c
+++ b/fftools/cmdutils.c
@@ -1707,12 +1707,14 @@  int show_filters(void *optctx, const char *opt, const char *arg)
     while ((filter = av_filter_iterate(&opaque))) {
         descr_cur = descr;
         for (i = 0; i < 2; i++) {
+            unsigned nb_pads;
             if (i) {
                 *(descr_cur++) = '-';
                 *(descr_cur++) = '>';
             }
             pad = i ? filter->outputs : filter->inputs;
-            for (j = 0; pad && avfilter_pad_get_name(pad, j); j++) {
+            nb_pads = avfilter_pad_count(pad);
+            for (j = 0; j < nb_pads; j++) {
                 if (descr_cur >= descr + sizeof(descr) - 4)
                     break;
                 *(descr_cur++) = get_media_type_char(avfilter_pad_get_type(pad, j));