[FFmpeg-devel,5/6] avfilter/formats: Simplify cleanup for ff_merge_* functions

Now that the output's refs-array is only allocated once, it is NULL in
any error case and therefore needn't be freed at all. Furthermore, it is
unnecessary to av_freep(&ptr) when ptr == NULL.

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
---
 libavfilter/formats.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

Andreas Rheinhardt (12020-08-08):
> Now that the output's refs-array is only allocated once, it is NULL in
> any error case and therefore needn't be freed at all. Furthermore, it is
> unnecessary to av_freep(&ptr) when ptr == NULL.
> 
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
> ---
>  libavfilter/formats.c | 9 +++------
>  1 file changed, 3 insertions(+), 6 deletions(-)
> 
> diff --git a/libavfilter/formats.c b/libavfilter/formats.c
> index ae44006dfe..e452fd0408 100644
> --- a/libavfilter/formats.c
> +++ b/libavfilter/formats.c
> @@ -141,10 +141,9 @@ AVFilterFormats *ff_merge_formats(AVFilterFormats *a, AVFilterFormats *b,
>      return ret;
>  fail:
>      if (ret) {

> -        av_freep(&ret->refs);

It seems fragile. We could easily unmake the change that guarantees that
refs is NULL, or use fail for another exit later. Maybe add an
av_assert1()?

>          av_freep(&ret->formats);

> +        av_freep(&ret);
>      }
> -    av_freep(&ret);

This one ok, of course.

>      return NULL;
>  }
>  
> @@ -168,10 +167,9 @@ AVFilterFormats *ff_merge_samplerates(AVFilterFormats *a,
>      return ret;
>  fail:
>      if (ret) {
> -        av_freep(&ret->refs);
>          av_freep(&ret->formats);
> +        av_freep(&ret);
>      }
> -    av_freep(&ret);
>      return NULL;
>  }
>  
> @@ -261,10 +259,9 @@ AVFilterChannelLayouts *ff_merge_channel_layouts(AVFilterChannelLayouts *a,
>  
>  fail:
>      if (ret) {
> -        av_freep(&ret->refs);
>          av_freep(&ret->channel_layouts);
> +        av_freep(&ret);
>      }
> -    av_freep(&ret);
>      return NULL;
>  }

Regards,

Nicolas George:
> Andreas Rheinhardt (12020-08-08):
>> Now that the output's refs-array is only allocated once, it is NULL in
>> any error case and therefore needn't be freed at all. Furthermore, it is
>> unnecessary to av_freep(&ptr) when ptr == NULL.
>>
>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
>> ---
>>  libavfilter/formats.c | 9 +++------
>>  1 file changed, 3 insertions(+), 6 deletions(-)
>>
>> diff --git a/libavfilter/formats.c b/libavfilter/formats.c
>> index ae44006dfe..e452fd0408 100644
>> --- a/libavfilter/formats.c
>> +++ b/libavfilter/formats.c
>> @@ -141,10 +141,9 @@ AVFilterFormats *ff_merge_formats(AVFilterFormats *a, AVFilterFormats *b,
>>      return ret;
>>  fail:
>>      if (ret) {
> 
>> -        av_freep(&ret->refs);
> 
> It seems fragile. We could easily unmake the change that guarantees that
> refs is NULL, or use fail for another exit later. Maybe add an
> av_assert1()?
> 
Fixed locally.

- Andreas

PS: Is the order of the formats/channel_layouts arrays actually important?

Andreas Rheinhardt (12020-08-08):
> PS: Is the order of the formats/channel_layouts arrays actually important?

IIRC, there are parts of the code that selects best formats and put them
in the front of the list. I need to dig back into all this to refresh my
memory, it is complex and it will need to be simplified before it can be
extended for subtitles or data packets.

Regards,

Nicolas George:
> Andreas Rheinhardt (12020-08-08):
>> PS: Is the order of the formats/channel_layouts arrays actually important?
> 
> IIRC, there are parts of the code that selects best formats and put them
> in the front of the list. I need to dig back into all this to refresh my
> memory, it is complex and it will need to be simplified before it can be
> extended for subtitles or data packets.
> 

I was thinking about this part of the MERGE_FORMATS macro:

        for (i = 0; i < a->nb; i++)
            for (j = 0; j < b->nb; j++)
                if (a->fmts[i] == b->fmts[j]) {
                    if(k >= FFMIN(a->nb, b->nb)){
                        av_log(NULL, AV_LOG_ERROR, "Duplicate formats in
%s detected\n", __FUNCTION__);
                        av_free(ret->fmts);
                        av_free(ret);
                        return NULL;
                    }
                    ret->fmts[k++] = a->fmts[i];
                }

Said check is not sufficient to ensure that there are no duplicates in
ret->fmts, of course. It has been added in the merge commit 1cbf7fb4345
as an additional safety check to ensure that one does not write beyond
the end of ret->fmts (it has FFMIN(a->nb, b->nb) entries). Yet this goal
could also be achieved in a simpler way: If a->nb <= b->nb, one could
simply rewrite the above to

        for (i = 0; i < a->nb; i++)
            for (j = 0; j < b->nb; j++)
                if (a->fmts[i] == b->fmts[j]) {
                    ret->fmts[k++] = a->fmts[i];
                    break;
                }

This ensures that every entry of a will end up at most once in ret. And
if one were simply allowed to switch a and b, one could ensure a->nb <=
b->nb easily. ff_merge_channel_layouts() already does something similar.

As a next step one could then even reuse the a->fmts array for ret->fmts
(or simply reuse a altogether); this is possible because k <= i at the
beginning of every iteration of the outer loop.

Notice that if there is a total ordering of all the formats and if both
a->fmts and b->fmts are ordered according to this ordering, then
ret->fmts will also ordered in that way (regardless of whether one swaps
or not).

- Andreas

Andreas Rheinhardt (12020-08-08):
> I was thinking about this part of the MERGE_FORMATS macro:
> 
>         for (i = 0; i < a->nb; i++)
>             for (j = 0; j < b->nb; j++)
>                 if (a->fmts[i] == b->fmts[j]) {
>                     if(k >= FFMIN(a->nb, b->nb)){
>                         av_log(NULL, AV_LOG_ERROR, "Duplicate formats in
> %s detected\n", __FUNCTION__);
>                         av_free(ret->fmts);
>                         av_free(ret);
>                         return NULL;
>                     }
>                     ret->fmts[k++] = a->fmts[i];
>                 }
> 
> Said check is not sufficient to ensure that there are no duplicates in
> ret->fmts, of course. It has been added in the merge commit 1cbf7fb4345
> as an additional safety check to ensure that one does not write beyond
> the end of ret->fmts (it has FFMIN(a->nb, b->nb) entries). Yet this goal
> could also be achieved in a simpler way: If a->nb <= b->nb, one could
> simply rewrite the above to

This is only a protection against internal bugs, since lists should not
contain duplicates in the first place. As such it should have been an
assert. If we no longer need it, it would probably be better to make a
conditional complete test for duplicates somewhere.

>         for (i = 0; i < a->nb; i++)
>             for (j = 0; j < b->nb; j++)
>                 if (a->fmts[i] == b->fmts[j]) {
>                     ret->fmts[k++] = a->fmts[i];
>                     break;
>                 }
> 
> This ensures that every entry of a will end up at most once in ret. And
> if one were simply allowed to switch a and b, one could ensure a->nb <=
> b->nb easily. ff_merge_channel_layouts() already does something similar.

I noticed something similar when refreshing my memory about this.
Indeed, breaking out of the loop when a match is found would make the
same guarantee, and a nice optimization for a quadratic algorithm.

Now, I can confirm, the order of formats in the array is not relevant
for the rest of the negotiation.

> As a next step one could then even reuse the a->fmts array for ret->fmts
> (or simply reuse a altogether); this is possible because k <= i at the
> beginning of every iteration of the outer loop.

Indeed.

> Notice that if there is a total ordering of all the formats and if both

These are mostly numbers, we can choose to use the numeric ordering or a
variant.

> a->fmts and b->fmts are ordered according to this ordering, then
> ret->fmts will also ordered in that way (regardless of whether one swaps
> or not).

Even better: if the lists are ordered, then we can switch from a
quadratic algorithm to a linear one. Good idea.

Regards,

Nicolas George:
> Andreas Rheinhardt (12020-08-08):
>> I was thinking about this part of the MERGE_FORMATS macro:
>>
>>         for (i = 0; i < a->nb; i++)
>>             for (j = 0; j < b->nb; j++)
>>                 if (a->fmts[i] == b->fmts[j]) {
>>                     if(k >= FFMIN(a->nb, b->nb)){
>>                         av_log(NULL, AV_LOG_ERROR, "Duplicate formats in
>> %s detected\n", __FUNCTION__);
>>                         av_free(ret->fmts);
>>                         av_free(ret);
>>                         return NULL;
>>                     }
>>                     ret->fmts[k++] = a->fmts[i];
>>                 }
>>
>> Said check is not sufficient to ensure that there are no duplicates in
>> ret->fmts, of course. It has been added in the merge commit 1cbf7fb4345
>> as an additional safety check to ensure that one does not write beyond
>> the end of ret->fmts (it has FFMIN(a->nb, b->nb) entries). Yet this goal
>> could also be achieved in a simpler way: If a->nb <= b->nb, one could
>> simply rewrite the above to
> 
> This is only a protection against internal bugs, since lists should not
> contain duplicates in the first place. As such it should have been an
> assert. If we no longer need it, it would probably be better to make a
> conditional complete test for duplicates somewhere.
> 
>>         for (i = 0; i < a->nb; i++)
>>             for (j = 0; j < b->nb; j++)
>>                 if (a->fmts[i] == b->fmts[j]) {
>>                     ret->fmts[k++] = a->fmts[i];
>>                     break;
>>                 }
>>
>> This ensures that every entry of a will end up at most once in ret. And
>> if one were simply allowed to switch a and b, one could ensure a->nb <=
>> b->nb easily. ff_merge_channel_layouts() already does something similar.
> 
> I noticed something similar when refreshing my memory about this.
> Indeed, breaking out of the loop when a match is found would make the
> same guarantee, and a nice optimization for a quadratic algorithm.
> 
> Now, I can confirm, the order of formats in the array is not relevant
> for the rest of the negotiation.
> 

Sure about that? The filter-framerate-up test fails (the output pixel
format is now YUV9 instead of I420) when I switch a and b in
MERGE_FORMATS if b->nb < a->nb. What you said seems to only apply to
sample rates, not format negotiations.

One can of course still avoid the allocations, if one is either willing
to keep the bigger of the two formats buffers or if one reallocates the
buffer to the correct (usually even smaller) size afterwards. I'll go
with the latter.

- Andreas

Andreas Rheinhardt (12020-08-13):
> Sure about that? The filter-framerate-up test fails (the output pixel
> format is now YUV9 instead of I420) when I switch a and b in
> MERGE_FORMATS if b->nb < a->nb. What you said seems to only apply to
> sample rates, not format negotiations.

Well, there is one aspect of the order that matters: if no stronger
criterion applies and still several formats are possible, the first one
will be selected.

For most cases, it does not matter, because the input data will
constrain the type, or possibly the output codec. But here, the input is
testsrc2 and the output is framecrc, they support many formats.

It could be a good idea to define some kind of "best" pixel formats when
there is choice like that, but there is no urgency.

More importantly, these test should be fixed to specify -pix_fmt and
disable automatic conversions. I will try to have a look.

Regards,