My goal was not to return a positive value in case of error but a positive value in case the two parameters can be merged. I wanted to do that because basically the functions ff_merge_formats, etc are invoked to verify if the two formats are compatible. However if they are compatible the intersection of the formats is allocated and it is returned BUT the pointer is lost (so the memory leak). Given we are using these functions as they were returning a boolean I thought it could be worthy to return a boolean (I could not find any boolean-ish type in libav so I used an int instead, using 0 for false and 1 for true).
I am afraid I did not made my intention clear at all... sorry about that Federico On Mon, Feb 23, 2015 at 8:17 PM, Vittorio Giovara < [email protected]> wrote: > On Mon, Feb 23, 2015 at 5:55 PM, <[email protected]> wrote: > > From: Federico Tomassetti <[email protected]> > > > > ff_mergeable_formats is as a wrapper around ff_merge_formats aiming to > avoid memory leaks > > currently present > > That's a very interesting solution to the problem, and that section of > libavfilter is quite scary. > The only change that is needed in my opinion is that this function > (and the others from the subsequent patches) is the fact that you are > returning a positive value in case of error, while throughout the code > we always use a negative value for most cases. Moreover there is only > an error possible in those functions so it's pretty safe to return > AVERROR(ENOMEM) there. > > Vittorio > > > Bug-Id: CID 1224283 > > --- > > libavfilter/avfiltergraph.c | 6 +++--- > > libavfilter/formats.c | 13 +++++++++++++ > > libavfilter/formats.h | 5 +++++ > > 3 files changed, 21 insertions(+), 3 deletions(-) > > > > diff --git a/libavfilter/avfiltergraph.c b/libavfilter/avfiltergraph.c > > index 0fc385c..e274deb 100644 > > --- a/libavfilter/avfiltergraph.c > > +++ b/libavfilter/avfiltergraph.c > > @@ -300,7 +300,7 @@ static int query_formats(AVFilterGraph *graph, > AVClass *log_ctx) > > continue; > > > > if (link->in_formats != link->out_formats && > > - !ff_merge_formats(link->in_formats, > > + !ff_mergeable_formats(link->in_formats, > > link->out_formats)) > > convert_needed = 1; > > if (link->type == AVMEDIA_TYPE_AUDIO) { > > @@ -366,8 +366,8 @@ static int query_formats(AVFilterGraph *graph, > AVClass *log_ctx) > > convert->filter->query_formats(convert); > > inlink = convert->inputs[0]; > > outlink = convert->outputs[0]; > > - if (!ff_merge_formats( inlink->in_formats, > inlink->out_formats) || > > - !ff_merge_formats(outlink->in_formats, > outlink->out_formats)) > > + if (!ff_mergeable_formats( inlink->in_formats, > inlink->out_formats) || > > + !ff_mergeable_formats(outlink->in_formats, > outlink->out_formats)) > > ret |= AVERROR(ENOSYS); > > if (inlink->type == AVMEDIA_TYPE_AUDIO && > > (!ff_merge_samplerates(inlink->in_samplerates, > > diff --git a/libavfilter/formats.c b/libavfilter/formats.c > > index ea61ed2..9ccf112 100644 > > --- a/libavfilter/formats.c > > +++ b/libavfilter/formats.c > > @@ -96,6 +96,19 @@ fail: > > return NULL; > > } > > > > +int ff_mergeable_formats(AVFilterFormats *a, AVFilterFormats *b) > > +{ > > + AVFilterFormats* merged_formats = ff_merge_formats(a, b); > > + if (merged_formats) { > > + av_freep(&merged_formats->refs); > > + av_freep(&merged_formats->formats); > > + av_freep(&merged_formats); > > + return 1; > > + } else { > > + return 0; > > + } > > +} > > + > > AVFilterFormats *ff_merge_samplerates(AVFilterFormats *a, > > AVFilterFormats *b) > > { > > diff --git a/libavfilter/formats.h b/libavfilter/formats.h > > index 2e44792..2fd9595 100644 > > --- a/libavfilter/formats.h > > +++ b/libavfilter/formats.h > > @@ -173,6 +173,11 @@ AVFilterFormats *ff_planar_sample_fmts(void); > > AVFilterFormats *ff_merge_formats(AVFilterFormats *a, AVFilterFormats > *b); > > > > /** > > + * Return true if the intersection of the formats of a and b is not > empty. > > + */ > > +int ff_mergeable_formats(AVFilterFormats *a, AVFilterFormats *b); > > + > > +/** > > * Add *ref as a new reference to formats. > > * That is the pointers will point like in the ascii art below: > > * ________ > > -- > > 2.2.2 > > > > _______________________________________________ > > libav-devel mailing list > > [email protected] > > https://lists.libav.org/mailman/listinfo/libav-devel > > > > -- > Vittorio > _______________________________________________ > libav-devel mailing list > [email protected] > https://lists.libav.org/mailman/listinfo/libav-devel > -- Website at http://www.federico-tomassetti.it _______________________________________________ libav-devel mailing list [email protected] https://lists.libav.org/mailman/listinfo/libav-devel
