| Message ID | AM7PR03MB6660568B4F4BD2E325944A8E8FFE9@AM7PR03MB6660.eurprd03.prod.outlook.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [FFmpeg-devel,1/6] avfilter/internal: Replace AVFilterPad.needs_writable by flags | 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 (12021-08-17): > This can be enabled/disabled on a per-filter basis by setting > the new internal flags FF_FILTER_FLAG_FREE_(IN|OUT)PADS and > on a per-pad basis by setting the AVFILTERPAD_FLAG_FREE_NAME flag. > > Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com> > --- > I decided to combine both approaches: It has the advantage that > the marginal extra code for a filter all of whose inputs'/outputs' > names need to be freed is zero while making it easy to handle filters > that have some inputs/outputs whose names need to be freed. It had the drawback that the information about the nature of the pads ends up at two places of the code that are not close to each other. It is not good for maintenance and readability. I think a boolean flag to ff_append_...pad() would do the job much more elegantly. What do you think about it? Regards,
Nicolas George: > Andreas Rheinhardt (12021-08-17): >> This can be enabled/disabled on a per-filter basis by setting >> the new internal flags FF_FILTER_FLAG_FREE_(IN|OUT)PADS and >> on a per-pad basis by setting the AVFILTERPAD_FLAG_FREE_NAME flag. >> >> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com> >> --- >> I decided to combine both approaches: It has the advantage that >> the marginal extra code for a filter all of whose inputs'/outputs' >> names need to be freed is zero while making it easy to handle filters >> that have some inputs/outputs whose names need to be freed. > > It had the drawback that the information about the nature of the pads > ends up at two places of the code that are not close to each other. It > is not good for maintenance and readability. > > I think a boolean flag to ff_append_...pad() would do the job much more > elegantly. What do you think about it? > Well, this has the problem that it adds code in many places, whereas static flags for the filter just need code in two places (with zero marginal cost for making another filter use this feature, whereas a flag to ff_append_... has a nonzero cost even to users not making use of this feature). I also like that it uses a static flag for what is essentially a static property. - Andreas
Andreas Rheinhardt (12021-08-17): > Well, this has the problem that it adds code in many places, whereas > static flags for the filter just need code in two places (with zero > marginal cost for making another filter use this feature, whereas a flag > to ff_append_... has a nonzero cost even to users not making use of this > feature). I also like that it uses a static flag for what is essentially > a static property. I do not understand your reasoning. If you have one case that is the majority by a wide margin, then it makes sense to make it the default and require the other case to be explicit. But that is not what is happening here. A quick count indicate that among filters adding pads dynamically, there are two using allocated names for one using static names. Two-to-one is not a huge majority, it is one of the cases where it makes more sense to write it explicitly on both cases. Furthermore, it is two-to-one for dynamically allocated, which means the default should be to free(), and the flags should be FF_FILTER_FLAG_DO_NOT_FREE_(IN|OUT)PADS, which would make the logic quite ugly. And honestly, we are talking adding ", 1" or ", 0" to each ff_insert_(in|out)pad() call. Just the name of the flag requires eight times as much characters in the source code. Regards,
Nicolas George: > Andreas Rheinhardt (12021-08-17): >> Well, this has the problem that it adds code in many places, whereas >> static flags for the filter just need code in two places (with zero >> marginal cost for making another filter use this feature, whereas a flag >> to ff_append_... has a nonzero cost even to users not making use of this >> feature). I also like that it uses a static flag for what is essentially >> a static property. > > I do not understand your reasoning. If you have one case that is the > majority by a wide margin, then it makes sense to make it the default > and require the other case to be explicit. But that is not what is > happening here. > > A quick count indicate that among filters adding pads dynamically, there > are two using allocated names for one using static names. Two-to-one is > not a huge majority, it is one of the cases where it makes more sense to > write it explicitly on both cases. > > Furthermore, it is two-to-one for dynamically allocated, which means the > default should be to free(), and the flags should be > FF_FILTER_FLAG_DO_NOT_FREE_(IN|OUT)PADS, which would make the logic > quite ugly. > > And honestly, we are talking adding ", 1" or ", 0" to each > ff_insert_(in|out)pad() call. Just the name of the flag requires eight > times as much characters in the source code. > You are distinguishing between filters with allocated input/output names and those with static input/output names among those filters with dynamic inputs/outputs and don't see a wide margin between allocated and static; I am distinguishing between those filters that have both allocated and static input (or output) names and those that don't have this mix (among inputs or among outputs) and I see an overwhelming majority of the latter, as the former group has only two elements (actually, one could make the former group empty, by making the inpads using static names static). So the easiest is to use a static flag for the common default case and to treat the others by other means (either completely separately as in the first version of this patch or by setting the flags manually for them). - Andreas
Andreas Rheinhardt (12021-08-21): > You are distinguishing between filters with allocated input/output names > and those with static input/output names among those filters with > dynamic inputs/outputs and don't see a wide margin between allocated and > static; I am distinguishing between those filters that have both > allocated and static input (or output) names and those that don't have > this mix (among inputs or among outputs) and I see an overwhelming > majority of the latter, as the former group has only two elements > (actually, one could make the former group empty, by making the inpads > using static names static). So the easiest is to use a static flag for > the common default case and to treat the others by other means (either > completely separately as in the first version of this patch or by > setting the flags manually for them). I am sorry, but the fact that you need a whole paragraph full of nested conditions to explain your point makes it rather unconvincing. You are looking at it from the point of view of the person doing the refactoring. That means you have the whole issue very much in mind. You have to consider the points of view of somebody who writes new code and somebody who reads the existing code or a proposed patch. For somebody who writes new code, explaining "the third argument is 1 if the name is dynamically allocated, 0 if not" is much simpler than explaining flags that set other flags. For somebody who reviews the code, the fact that the information is at the same place is a huge benefit. Otherwise, we must stop reading the code linearly to go check if the flag was properly set. And in this instance, even the framework is simpler if you just add a third argument to append_in/out_pad(). If you are not convinced, we can try the experiment with an innocent bystander: pick somebody neutral, explain your proposal from scratch, I explain my proposal from scratch, and we see what they think. Regards,
diff --git a/libavfilter/avfilter.c b/libavfilter/avfilter.c index c3382036d2..48727201f6 100644 --- a/libavfilter/avfilter.c +++ b/libavfilter/avfilter.c @@ -123,8 +123,11 @@ static int insert_pad(unsigned *count, AVFilterPad **pads, *pads = newpads; if (newlinks) *links = newlinks; - if (!newpads || !newlinks) + if (!newpads || !newlinks) { + if (newpad->flags & AVFILTERPAD_FLAG_FREE_NAME) + av_freep(&newpad->name); return AVERROR(ENOMEM); + } memcpy(*pads + idx, newpad, sizeof(AVFilterPad)); (*links)[idx] = NULL; @@ -136,11 +139,17 @@ static int insert_pad(unsigned *count, AVFilterPad **pads, int ff_insert_inpad(AVFilterContext *f, AVFilterPad *p) { + if (f->filter->flags_internal & FF_FILTER_FLAG_FREE_INPADS) + p->flags |= AVFILTERPAD_FLAG_FREE_NAME; + return insert_pad(&f->nb_inputs, &f->input_pads, &f->inputs, p); } int ff_insert_outpad(AVFilterContext *f, AVFilterPad *p) { + if (f->filter->flags_internal & FF_FILTER_FLAG_FREE_OUTPADS) + p->flags |= AVFILTERPAD_FLAG_FREE_NAME; + return insert_pad(&f->nb_outputs, &f->output_pads, &f->outputs, p); } @@ -741,9 +750,13 @@ void avfilter_free(AVFilterContext *filter) for (i = 0; i < filter->nb_inputs; i++) { free_link(filter->inputs[i]); + if (filter->input_pads[i].flags & AVFILTERPAD_FLAG_FREE_NAME) + av_freep(&filter->input_pads[i].name); } for (i = 0; i < filter->nb_outputs; i++) { free_link(filter->outputs[i]); + if (filter->output_pads[i].flags & AVFILTERPAD_FLAG_FREE_NAME) + av_freep(&filter->output_pads[i].name); } if (filter->filter->priv_class) diff --git a/libavfilter/internal.h b/libavfilter/internal.h index 8fe17a5433..0d0335bd1c 100644 --- a/libavfilter/internal.h +++ b/libavfilter/internal.h @@ -68,6 +68,11 @@ struct AVFilterPad { */ #define AVFILTERPAD_FLAG_NEEDS_WRITABLE (1 << 0) + /** + * The pad's name is allocated and should be freed generically. + */ +#define AVFILTERPAD_FLAG_FREE_NAME (1 << 1) + /** * A combination of AVFILTERPAD_FLAG_* flags. */ @@ -227,6 +232,11 @@ void ff_tlog_link(void *ctx, AVFilterLink *link, int end); /** * Append a new input/output pad to the filter's list of such pads. + * + * If the underlying AVFilter has the FF_FILTER_FLAG_FREE_INPADS + * set, the AVFILTERPAD_FLAG_FREE_NAME flag will be set for new inpads, + * ensuring that it will be freed generically (even on insertion error). + * Similarly for outpads. */ int ff_insert_inpad (AVFilterContext *f, AVFilterPad *p); int ff_insert_outpad(AVFilterContext *f, AVFilterPad *p); @@ -317,6 +327,16 @@ void ff_filter_graph_remove_filter(AVFilterGraph *graph, AVFilterContext *filter */ #define FF_FILTER_FLAG_HWFRAME_AWARE (1 << 0) +/** + * The names of all input pads are allocated and should be freed generically. + */ + #define FF_FILTER_FLAG_FREE_INPADS (1 << 1) + +/** + * The names of all output pads are allocated and should be freed generically. + */ + #define FF_FILTER_FLAG_FREE_OUTPADS (1 << 2) + /** * Run one round of processing on a filter graph. */
This can be enabled/disabled on a per-filter basis by setting the new internal flags FF_FILTER_FLAG_FREE_(IN|OUT)PADS and on a per-pad basis by setting the AVFILTERPAD_FLAG_FREE_NAME flag. Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com> --- I decided to combine both approaches: It has the advantage that the marginal extra code for a filter all of whose inputs'/outputs' names need to be freed is zero while making it easy to handle filters that have some inputs/outputs whose names need to be freed. libavfilter/avfilter.c | 15 ++++++++++++++- libavfilter/internal.h | 20 ++++++++++++++++++++ 2 files changed, 34 insertions(+), 1 deletion(-)