[FFmpeg-devel] [PATCH 2/2] swscale/input: Avoid calls to av_pix_fmt_desc_get()

[Andreas Rheinhardt]

Michael Niedermayer:
> On Fri, Sep 09, 2022 at 08:15:22PM +0200, Andreas Rheinhardt wrote:
>> Michael Niedermayer:
>>> On Thu, Sep 08, 2022 at 11:44:51PM +0200, Andreas Rheinhardt wrote:
>>>> Michael Niedermayer:
>>>>> On Thu, Sep 08, 2022 at 09:38:51PM +0200, Andreas Rheinhardt wrote:
>>>>>> Michael Niedermayer:
>>> [...]
>>>>> To me if i look at the evolution
>>>>> of isBE() / code checking BE-ness it become more messy over time
>>>>>
>>>>> I think it would be interresting to think about if we can make
>>>>> av_pix_fmt_desc_get(compile time constant) work at compile time.
>>>>> or if we maybe can return to a simpler implementation
>>>>>
>>>>
>>>> We could put the av_pix_fmt_descriptors array into an internal header
>>>> and use something like
>>>>
>>>> static av_always_inline const AVPixFmtDescriptor
>>>> *ff_pix_fmt_descriptor_get(enum AVPixelFormat fmt)
>>>> {
>>>>     if (av_builtin_constant_p(fmt))
>>>>         return &av_pix_fmt_descriptors[fmt];
>>>>     return av_pix_fmt_desc_get(fmt);
>>>> }
>>>
>>> yes thats what i was thinking of too.
>>>
>>
>> Seems like Anton is away for a week or so. I am sure he has an opinion
>> on the above approach. I think we will wait for him or shall I apply the
>> patches as they are given that they do not block any later alternative
>> solution?
> 
> please dont apply code like "IS_BE(BE_LE)" 
> iam sure it makes sense to you today but it requires an additional step
> for the reader to understand
> simplest is a seperate endianness and isbe variable. on the wrapper
> that should be less code too but quite possibly you see a better and
> cleaner way
> 

I actually thought that my solution is superior to the one you seem to
have in mind; after all, the parameter for isbe is redundant: It can
also be derived from the pixfmt-name.

> 
>> (There is one thing I already don't like about the alternative solution:
>> It relies on av_builtin_constant_p, which not every compiler supports.)
> 
> Thats why i didnt suggets to use av_builtin_constant_p() i was hoping you
> saw a better way to achieve it.
> 

Well, without av_builtin_constant_p() the only other way for this would
be to add two systems to get the information, one for compile-time
constants and one for not-constants. Ensuring that the former is only
used for compile-time constants will be a challenge, to say the least.

> But this is a problem which occurs more than once in the codebase.
> mapping some identifer to some value just depending on the identifer,
> something that is compile time constant, yet it calls to a function to
> do a lookup
> 

Another idea from the top of my head:
- We could provide some of the info contained in the descriptors via
separate defines/enums that parallel the actual enum, like
enum AVPixelFormatFlags {
    AV_PIX_FMT_YUV420P_FLAGS = AV_PIX_FMT_FLAG_PLANAR,
    AV_PIX_FMT_YUYV422_FLAGS = 0,
...
};
The list in pixdesc.c would then use these constants instead of defining
the flags twice (to avoid inconsistencies). These constants can be used
from macros via fmt ## _FLAGS, avoiding the av_builtin_constant_p()
issue. This could even be made public if we add a big warning that new
flags may be added in the future. Other such enums for other values may
be added, too, but honestly I don't really like this approach.

We could also use make an xmacro out of the list in lavu/pixdesc.c and
use this xmacro to query the values. E.g. isBE() would then in effect
become one big switch:
isBE(fmt)
{
    switch (fmt) {
    case AV_PIX_FMT_YUV420P: return 0;
....
    }
}
This could be made to handle non-compile-time constants as well (by
having a default that uses av_pix_fmt_desc_get()), but it has a
downside: There would be runtime checks for whether we are in the
default branch or not. So once again this function should not be used
with non-compile time constants.

- Andreas