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

[Michael Niedermayer]

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.

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.


> 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

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Republics decline into democracies and democracies degenerate into
despotisms. -- Aristotle
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20090129/df003d17/attachment.pgp>