[FFmpeg-devel,v2] avfilter/formats: optimize ff_all_formats

This is a micro-optimization. Saving almost 200 reallocations makes it
worth a try.
---
fix commit message typo: relocations -> reallocations

 libavfilter/formats.c | 35 +++++++++++++++++++++--------------
 1 file changed, 21 insertions(+), 14 deletions(-)

lör 2019-12-07 klockan 01:06 +0800 skrev Zhao Zhili:
> This is a micro-optimization. Saving almost 200 reallocations makes
> it
> worth a try.
> ---
> fix commit message typo: relocations -> reallocations
> 
>  libavfilter/formats.c | 35 +++++++++++++++++++++--------------
>  1 file changed, 21 insertions(+), 14 deletions(-)
> 
> diff --git a/libavfilter/formats.c b/libavfilter/formats.c
> index 33c64668a0..1af7a1cedd 100644
> --- a/libavfilter/formats.c
> +++ b/libavfilter/formats.c
> @@ -348,23 +348,30 @@ int
> ff_add_channel_layout(AVFilterChannelLayouts **l, uint64_t
> channel_layout)
>  
>  AVFilterFormats *ff_all_formats(enum AVMediaType type)
>  {
> -    AVFilterFormats *ret = NULL;
> +    AVFilterFormats *ret;
> +    int i, count;
>  
> -    if (type == AVMEDIA_TYPE_VIDEO) {
> -        const AVPixFmtDescriptor *desc = NULL;
> -        while ((desc = av_pix_fmt_desc_next(desc))) {
> -            if (ff_add_format(&ret, av_pix_fmt_desc_get_id(desc)) <
> 0)
> -                return NULL;
> -        }
> -    } else if (type == AVMEDIA_TYPE_AUDIO) {
> -        enum AVSampleFormat fmt = 0;
> -        while (av_get_sample_fmt_name(fmt)) {
> -            if (ff_add_format(&ret, fmt) < 0)
> -                return NULL;
> -            fmt++;
> -        }
> +    if (type == AVMEDIA_TYPE_VIDEO)
> +        count = AV_PIX_FMT_NB;
> +    else if (type == AVMEDIA_TYPE_AUDIO)
> +        count = AV_SAMPLE_FMT_NB;
> +    else
> +        return NULL;
> +
> +    ret = av_mallocz(sizeof(*ret));
> +    if (!ret)
> +        return NULL;
> +
> +    ret->nb_formats = count;
> +    ret->formats = av_malloc_array(count, sizeof(*ret->formats));
> +    if (!ret->formats) {
> +        av_free(ret);
> +        return NULL;
>      }
>  
> +    for (i = 0; i < count; i++)
> +        ret->formats[i] = i;

As far as I can tell this is OK, and it passes FATE. But it just looks
very very wrong. Why does this function even exist if all it
effectively does is return the integers from 0..count-1?

/Tomas

> On Dec 10, 2019, at 4:20 AM, Tomas Härdin <tjoppen@acc.umu.se> wrote:
> 
> lör 2019-12-07 klockan 01:06 +0800 skrev Zhao Zhili:
>> This is a micro-optimization. Saving almost 200 reallocations makes
>> it
>> worth a try.
>> ---
>> fix commit message typo: relocations -> reallocations
>> 
>> libavfilter/formats.c | 35 +++++++++++++++++++++--------------
>> 1 file changed, 21 insertions(+), 14 deletions(-)
>> 
>> diff --git a/libavfilter/formats.c b/libavfilter/formats.c
>> index 33c64668a0..1af7a1cedd 100644
>> --- a/libavfilter/formats.c
>> +++ b/libavfilter/formats.c
>> @@ -348,23 +348,30 @@ int
>> ff_add_channel_layout(AVFilterChannelLayouts **l, uint64_t
>> channel_layout)
>> 
>> AVFilterFormats *ff_all_formats(enum AVMediaType type)
>> {
>> -    AVFilterFormats *ret = NULL;
>> +    AVFilterFormats *ret;
>> +    int i, count;
>> 
>> -    if (type == AVMEDIA_TYPE_VIDEO) {
>> -        const AVPixFmtDescriptor *desc = NULL;
>> -        while ((desc = av_pix_fmt_desc_next(desc))) {
>> -            if (ff_add_format(&ret, av_pix_fmt_desc_get_id(desc)) <
>> 0)
>> -                return NULL;
>> -        }
>> -    } else if (type == AVMEDIA_TYPE_AUDIO) {
>> -        enum AVSampleFormat fmt = 0;
>> -        while (av_get_sample_fmt_name(fmt)) {
>> -            if (ff_add_format(&ret, fmt) < 0)
>> -                return NULL;
>> -            fmt++;
>> -        }
>> +    if (type == AVMEDIA_TYPE_VIDEO)
>> +        count = AV_PIX_FMT_NB;
>> +    else if (type == AVMEDIA_TYPE_AUDIO)
>> +        count = AV_SAMPLE_FMT_NB;
>> +    else
>> +        return NULL;
>> +
>> +    ret = av_mallocz(sizeof(*ret));
>> +    if (!ret)
>> +        return NULL;
>> +
>> +    ret->nb_formats = count;
>> +    ret->formats = av_malloc_array(count, sizeof(*ret->formats));
>> +    if (!ret->formats) {
>> +        av_free(ret);
>> +        return NULL;
>>     }
>> 
>> +    for (i = 0; i < count; i++)
>> +        ret->formats[i] = i;
> 
> As far as I can tell this is OK, and it passes FATE. But it just looks
> very very wrong. Why does this function even exist if all it
> effectively does is return the integers from 0..count-1?

The function is there since the first libavfilter commit. I guess it’s for forward compatibility.

I don’t know whether a ‘all_formats’ flag works or not.

> 
> /Tomas
> 
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org <mailto:ffmpeg-devel@ffmpeg.org>
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel <https://ffmpeg.org/mailman/listinfo/ffmpeg-devel>
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org <mailto:ffmpeg-devel-request@ffmpeg.org> with subject "unsubscribe".

tis 2019-12-10 klockan 22:14 +0800 skrev zhilizhao:
> > On Dec 10, 2019, at 4:20 AM, Tomas Härdin <tjoppen@acc.umu.se> wrote:
> > 
> > lör 2019-12-07 klockan 01:06 +0800 skrev Zhao Zhili:
> > > This is a micro-optimization. Saving almost 200 reallocations makes
> > > it
> > > worth a try.
> > > ---
> > > fix commit message typo: relocations -> reallocations
> > > 
> > > libavfilter/formats.c | 35 +++++++++++++++++++++--------------
> > > 1 file changed, 21 insertions(+), 14 deletions(-)
> > > 
> > > diff --git a/libavfilter/formats.c b/libavfilter/formats.c
> > > index 33c64668a0..1af7a1cedd 100644
> > > --- a/libavfilter/formats.c
> > > +++ b/libavfilter/formats.c
> > > @@ -348,23 +348,30 @@ int
> > > ff_add_channel_layout(AVFilterChannelLayouts **l, uint64_t
> > > channel_layout)
> > > 
> > > AVFilterFormats *ff_all_formats(enum AVMediaType type)
> > > {
> > > -    AVFilterFormats *ret = NULL;
> > > +    AVFilterFormats *ret;
> > > +    int i, count;
> > > 
> > > -    if (type == AVMEDIA_TYPE_VIDEO) {
> > > -        const AVPixFmtDescriptor *desc = NULL;
> > > -        while ((desc = av_pix_fmt_desc_next(desc))) {
> > > -            if (ff_add_format(&ret, av_pix_fmt_desc_get_id(desc)) <
> > > 0)
> > > -                return NULL;
> > > -        }
> > > -    } else if (type == AVMEDIA_TYPE_AUDIO) {
> > > -        enum AVSampleFormat fmt = 0;
> > > -        while (av_get_sample_fmt_name(fmt)) {
> > > -            if (ff_add_format(&ret, fmt) < 0)
> > > -                return NULL;
> > > -            fmt++;
> > > -        }
> > > +    if (type == AVMEDIA_TYPE_VIDEO)
> > > +        count = AV_PIX_FMT_NB;
> > > +    else if (type == AVMEDIA_TYPE_AUDIO)
> > > +        count = AV_SAMPLE_FMT_NB;
> > > +    else
> > > +        return NULL;
> > > +
> > > +    ret = av_mallocz(sizeof(*ret));
> > > +    if (!ret)
> > > +        return NULL;
> > > +
> > > +    ret->nb_formats = count;
> > > +    ret->formats = av_malloc_array(count, sizeof(*ret->formats));
> > > +    if (!ret->formats) {
> > > +        av_free(ret);
> > > +        return NULL;
> > >     }
> > > 
> > > +    for (i = 0; i < count; i++)
> > > +        ret->formats[i] = i;
> > 
> > As far as I can tell this is OK, and it passes FATE. But it just looks
> > very very wrong. Why does this function even exist if all it
> > effectively does is return the integers from 0..count-1?
> 
> The function is there since the first libavfilter commit. I guess it’s for forward compatibility.
> 
> I don’t know whether a ‘all_formats’ flag works or not.

Perhaps someone with more insight into lavfi wants to comment?

/Tomas

Tomas Härdin (12019-12-15):
> Perhaps someone with more insight into lavfi wants to comment?

I intend to look at it, but it have been a hectic week here.

Regards,

Tomas Härdin (12019-12-09):
> As far as I can tell this is OK, and it passes FATE. But it just looks
> very very wrong. Why does this function even exist if all it
> effectively does is return the integers from 0..count-1?

For some time, there were gaps in the list of pixel formats, the task of
this function was precisely there to avoid the gaps and keep only the
actual pixel formats.

I am not sure about this change. It is technically valid, but it may
cause problems later.

There is an intermediate solution: keep the loop, but allocate only
once (simplified):

        while ((desc = ...))
            count++;
        ret->formats = av_malloc_array(count, sizeof(*ret->formats));
        count = 0;
        while ((desc = ...))
            ret->formats[count] = ...;

What do other developers think about it, with these explanations?

Regards,

On Mon, Dec 23, 2019 at 04:11:53PM +0100, Nicolas George wrote:
> Tomas Härdin (12019-12-09):
> > As far as I can tell this is OK, and it passes FATE. But it just looks
> > very very wrong. Why does this function even exist if all it
> > effectively does is return the integers from 0..count-1?
> 
> For some time, there were gaps in the list of pixel formats, the task of
> this function was precisely there to avoid the gaps and keep only the
> actual pixel formats.
> 
> I am not sure about this change. It is technically valid, but it may
> cause problems later.
> 
> There is an intermediate solution: keep the loop, but allocate only
> once (simplified):
> 
>         while ((desc = ...))
>             count++;
>         ret->formats = av_malloc_array(count, sizeof(*ret->formats));
>         count = 0;
>         while ((desc = ...))
>             ret->formats[count] = ...;
> 
> What do other developers think about it, with these explanations?

The count once then allocate feels more robust than assuming on !gaps

Thanks


[...]

> On Dec 24, 2019, at 4:32 AM, Michael Niedermayer <michael@niedermayer.cc> wrote:
> 
> On Mon, Dec 23, 2019 at 04:11:53PM +0100, Nicolas George wrote:
>> Tomas Härdin (12019-12-09):
>>> As far as I can tell this is OK, and it passes FATE. But it just looks
>>> very very wrong. Why does this function even exist if all it
>>> effectively does is return the integers from 0..count-1?
>> 
>> For some time, there were gaps in the list of pixel formats, the task of
>> this function was precisely there to avoid the gaps and keep only the
>> actual pixel formats.
>> 
>> I am not sure about this change. It is technically valid, but it may
>> cause problems later.
>> 
>> There is an intermediate solution: keep the loop, but allocate only
>> once (simplified):
>> 
>>        while ((desc = ...))
>>            count++;
>>        ret->formats = av_malloc_array(count, sizeof(*ret->formats));
>>        count = 0;
>>        while ((desc = ...))
>>            ret->formats[count] = ...;
>> 
>> What do other developers think about it, with these explanations?

I tried this solution at first. The following reasons make me thought AV_PIX_FMT cannot have gaps.
Of course, can libavfilter depends on that is another question. I can switch to the robust solution
if you prefer.

1. AV_PIX_FMT_NB depends on !gaps, unless we calculate and set it manually

2. The reason to deprecated VAAPI in this way (ABI incompatible) is AV_PIX_FMT cannot have gaps

#if FF_API_VAAPI
    /** @name Deprecated pixel formats */
    /**@{*/
    AV_PIX_FMT_VAAPI_MOCO, ///< HW acceleration through VA API at motion compensation entry-point, Picture.data[3] contains a vaapi_render_state struct which contains macroblocks as well as various fields extracted from headers
    AV_PIX_FMT_VAAPI_IDCT, ///< HW acceleration through VA API at IDCT entry-point, Picture.data[3] contains a vaapi_render_state struct which contains fields extracted from headers
    AV_PIX_FMT_VAAPI_VLD,  ///< HW decoding through VA API, Picture.data[3] contains a VASurfaceID
    /**@}*/
    AV_PIX_FMT_VAAPI = AV_PIX_FMT_VAAPI_VLD,
#else
    /**
     *  Hardware acceleration through VA-API, data[3] contains a
     *  VASurfaceID.
     */
    AV_PIX_FMT_VAAPI,
#endif


> 
> The count once then allocate feels more robust than assuming on !gaps
> 
> Thanks
> 
> 
> [...]
> 
> -- 
> Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
> 
> When you are offended at any man's fault, turn to yourself and study your
> own failings. Then you will forget your anger. -- Epictetus
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org <mailto:ffmpeg-devel@ffmpeg.org>
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel <https://ffmpeg.org/mailman/listinfo/ffmpeg-devel>
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org <mailto:ffmpeg-devel-request@ffmpeg.org> with subject "unsubscribe".

zhilizhao (12019-12-24):
> 1. AV_PIX_FMT_NB depends on !gaps, unless we calculate and set it manually

NB is not really the "number of".

> 2. The reason to deprecated VAAPI in this way (ABI incompatible) is
> AV_PIX_FMT cannot have gaps

No, it was because we WANTED AV_PIX_FMT to not have gaps. It had some in
the past, there may be reasons to have anew in the future.

But I just realized: there is a much simpler reason that makes this
particular approach impossible:

AV_PIX_FMT_NB         ///< number of pixel formats, DO NOT USE THIS if you want to link with shared libav*
AV_SAMPLE_FMT_NB           ///< Number of sample formats. DO NOT USE if linking dynamically

you cannot use AV_PIX_FMT_NB or AV_SAMPLE_FMT_NB in lavfi, only in lavu.

Regards,

Hi Nicolas,

> On Dec 24, 2019, at 6:50 PM, Nicolas George <george@nsup.org> wrote:
> 
> zhilizhao (12019-12-24):
>> 1. AV_PIX_FMT_NB depends on !gaps, unless we calculate and set it manually
> 
> NB is not really the "number of".
> 
>> 2. The reason to deprecated VAAPI in this way (ABI incompatible) is
>> AV_PIX_FMT cannot have gaps
> 
> No, it was because we WANTED AV_PIX_FMT to not have gaps. It had some in
> the past, there may be reasons to have anew in the future.
> 
> But I just realized: there is a much simpler reason that makes this
> particular approach impossible:
> 
> AV_PIX_FMT_NB         ///< number of pixel formats, DO NOT USE THIS if you want to link with shared libav*
> AV_SAMPLE_FMT_NB           ///< Number of sample formats. DO NOT USE if linking dynamically
> 
> you cannot use AV_PIX_FMT_NB or AV_SAMPLE_FMT_NB in lavfi, only in lavu.

I get the idea. However, if lavi is built agains version A of libavutils
and use version B at runtime, it’s not guaranteed to work with or without
access to AV_PIX_FMT_NB:

1. For major version bump, AV_PIX_FMT_XXX may have different values
in different versions of libavutils

2. For minor version bump, if there is new AV_PIX_FMT_XXX appends to
AVPixelFormat, it would be safe to use a smaller AV_PIX_FMT_NB_old.
(if we don’t know what the new format is, we don’t know how to deal with it)

IMO the requirement of link with shared libavutils:

1. For libavutils: never change old entries of AVPixelFormat, append new
entry to the tail

2. On the client side of libavutils: access the subset of known formats, and careful
about iterate overflow

PS: It’s not clear the comments are ffmpeg-user oriented or libavutils-user oriented.
libavcodec, libavfilter, and libavdevice have access to AV_PIX_FMT_NB.

Can you help me figure out these contradictions?

> 
> Regards,
> 
> -- 
>  Nicolas George
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

zhilizhao (12019-12-24):
> I get the idea. However, if lavi is built agains version A of libavutils
> and use version B at runtime, it’s not guaranteed to work with or without
> access to AV_PIX_FMT_NB:

In principle, it is.

> 1. For major version bump, AV_PIX_FMT_XXX may have different values
> in different versions of libavutils

Major bumps are precisely the time to introduce unavoidable
incompatibilities. But we will not make a major bump when adding a new
pixel format.

> 2. For minor version bump, if there is new AV_PIX_FMT_XXX appends to
> AVPixelFormat, it would be safe to use a smaller AV_PIX_FMT_NB_old.

It may be safe, but it would make ff_all_formats() return not all
formats, which is not acceptable.

> (if we don’t know what the new format is, we don’t know how to deal with it)

If we don't know what the new format is but it is relevant, then we
don't use ff_all_formats(). Look at the filters that use it: mostly
filters that do not depend on the frame data, only the frame properties.

> libavcodec, libavfilter, and libavdevice have access to AV_PIX_FMT_NB.
> 
> Can you help me figure out these contradictions?

They are bugs, they need to be fixed.

The rule is: only use AV_SOMEGHING_NB in the same library.

Regards,