[FFmpeg-devel] WORDS_BIGENDIAN used in installed header avutil.h

[Baptiste Coudurier]

Michael Niedermayer wrote:
> On Thu, Jan 29, 2009 at 11:10:05AM -0800, Baptiste Coudurier wrote:
>> Michael Niedermayer wrote:
>>> On Thu, Jan 29, 2009 at 01:41:42PM +0100, Diego Biurrun wrote:
>>>> On Thu, Jan 29, 2009 at 12:21:52PM +0100, Michael Niedermayer wrote:
>>>>> On Wed, Jan 28, 2009 at 03:16:46PM -0800, Baptiste Coudurier wrote:
>>>>>> Diego Biurrun wrote:
>>>>>>> Houston, we have a problem.  The installed header avutil.h uses
>>>>>>> WORDS_BIGENDIAN, which is #defined in config.h, which is not installed...
>>>>>>>
>>>>>> first, pix_fmt stuff should be moved to another header.
>>>>>>
>>>>>> second, Im pretty sure, these redefines for RGBA/BGRA/32 whatever can be
>>>>>> avoided if dealt within libswscale when choosing func pointer.
>>>>> they are part of the public API and they are intentionally part of it.
>>>>> They are usefull and used, for example in codecs.
>> Used _internally_ in codecs.
> 
> and swscale yes
> they couldt be used outside because of that bug here ...
> 
> 
>>>>> IMHO we shouldnt be butchering the API
>>>> Ummm, clearly only half of it is part of the API, since the
>>>> '#ifdef WORDS_BIGENDIAN' amounts to a '#if 0'...
>>> That may be the implementation but certainly not the API.
>>> Not every bug in the implementation is part of the API
>> You can add pixfmt.h and include it in avutil.h, this do not break API.
> 
> no and iam not against that at all but it doesnt solve the problem with
> WORDS_BIGENDIAN
> 
> 
>>>> So the goal of the API - different semantics on big-endian systems - is
>>>> not achieved.
>>> something like
>>>
>>> grep WORDS_BIGENDIAN config.h && echo '#define WORDS_BIGENDIAN 1' > lib/avutil.h
>>> cat avutil.h >> lib/avutil.h
>>>
>>> would solve it
>>> of course that can be done cleaner its just to show it can be solved
>>> trivially without breaking the API
>>>
>> Well changing the documentation can be done, since behaviour was always
>> wrong, nobody complained so far ? This lightens that practically nobody
>> noticed.
>>
>> You remove the ifdef WORDS_BIGENDIAN case and keep:
>>
>> #define PIX_FMT_RGBA PIX_FMT_BGR32
>> #define PIX_FMT_BGRA PIX_FMT_RGB32
>> #define PIX_FMT_ARGB PIX_FMT_BGR32_1
>> #define PIX_FMT_ABGR PIX_FMT_RGB32_1
>>
>> Then you add checks in libswscale, 
> 
> iam against this, the names BGRA,RGBA,ARGB,ABGR have been choosen that way
> because that is how the bytes are ordered, this matches the other byte based
> variants like the 24bit variants
> the naming is consistant and i am against making it inconsistant because
> there was a bug.

Yes the name are consistant, I really not suggesting to make inconsistant.

> The defines exist so the pix fmts can be easily refered to based on byte
> ordering as well as natively read 32bit word ordering. This is usefull and
> is used. Also it is consistent, droping stuff will not make it better you
> always have byte based 24bit and word based 16bit because neither can be
> repressented the other way.

It is inconsitant because it differs from one arch to the other.

It is confusing because pix_fmt would not refer to how bytes are order
in uint8_* and the latter is definitely more _intuitive_ and logic.

considering "uint8_t *buf"
if your pixfmt is RGBA you expect buf[0] to be 'R' on _all_
architectures, I don't see how you could imagine something else.

Other people have said that in the past on mls.

I claim this is useless because if libswscale check for PIX_FMT as
RGBA888 it nows _internally_ for what arch it has been compiled, and can
choose the correct routine itself.

>> considering both values since the
>> goal is to deprecate all _1 (horrible names)
> 
> You are free to suggest better names for the _1 cases, we can surely
> rename them in the next major bump

The best names are RGBA8888, BGRA8888, ARGB8888, ABGR8888, IMHO.
Now RGBA, BGRA, ARGB, ABGR are ok too.

And this is what should be as external API, there is _no_ sense if
hacking pixfmt externally.

If libswscale wants to optimize routines based on its internal
implementation (using uint64_t or uint32_t) to deal with data, it's
_its_ problem, not users which should see what is _obvious_.

-- 
Baptiste COUDURIER                              GnuPG Key Id: 0x5C1ABAAA
Key fingerprint                 8D77134D20CC9220201FC5DB0AC9325C5C1ABAAA
checking for life_signs in -lkenny... no
FFmpeg maintainer                                  http://www.ffmpeg.org