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 _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".