| 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 | expand |
| 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 |
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
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 --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));
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(-)