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

[Baptiste Coudurier]

Michael Niedermayer wrote:
> On Thu, Jan 29, 2009 at 03:20:10PM -0800, Baptiste Coudurier wrote:
>> Michael Niedermayer wrote:
>>> On Thu, Jan 29, 2009 at 02:36:53PM -0800, Baptiste Coudurier wrote:
>>> [...]
>>>>>> 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_.
>>> i dont mind if we move the byte based RGBA, ... into the enum and
>>> move the RGB32 stuff to the #defines with the next major bump
>>> (it would break ABI not API)
>>> assuming someone posts a patch ...
>>>
>>> iam not in favor of droping the native 32bit word formats nor do i agree
>>> that this is just sws problem, it really is not.
>>> a grep in mplayer confirms this also, the 32bit word formats are used
>>> the byte based formats are not.
>> Well, mplayer is one thing, I know that several people already asked how
>> to access raw data from AVFrame.
> 
> these people will keep asking no matter what you do with the pixel formats
> you arent expecting them to grok the stuff you wrote about rgb15/16 below
> do you?

At least it is obvious to me. IMHO it is more obvious than groking arch
dependant description.

We all know people are not careful about arch dependant code and
concepts. I care about usually and simplicity.

> The current stuff is well explained in the header so unless someone fails
> to find that he shouldnt have a problem, if he does fail to find it so
> will he if something else is writen i there

Last time I tried to add support for RGBA32 of whatever variant
quicktime use, I was so confused about these arch dependant things and
these arch dependant defines that I gave up.

I hated the guy who decided that buf[0] for RGBA what not 'R' for
whatever reason.

>> If you don't want to use libswscale because it's GPL for example or you
>> want to write your own conversion routine or filter, it is definitely
>> more simple and obvious to have arch independant representation in
>> uint8_t buffer.
> 
> Iam not aware of anyone who tried to access rgb15/16 as bytes in a serious
> application and succeeded. 
> swscale, the old scaler as well as mplayer use 16bit scalers or SIMD
>

I did try to mess with RGB555 in .mov, and I gave up both in libswscale
and imgconvert because of the arch dependant mess in pixfmt.

We don't agree here.

> also the c code in sws is 99% LGPL, fixing the rest up shouldnt be that
> hard, and the c code should be several times faster than byte based code.

>>> also what is your oppinion on the 15/16bit formats, they dont have any
>>> sane byte based representation.
>> Im not too familiar with them, IMHO we should be consistant that is,
>> still considering uint8_t *buf
> 
> well we are consistent now, considering everything as native words where
> possible.

Well not consistent between arch considering the same uint8_t buffer,
but sorry, this really is strange for me.

RGB24 pixfmt is representing the uint8_t buffer, RGBA variants aren't, I
know this is related to internal use of the buffer.

> changing it to uint8_t is to some extend bikeshed.
> and fact is all code accesses 15/16 bits using native endian 16bit word
> (possibly SIMD) having the internal pixfmt reprsent something that means
> something entirely different to code on big and little endian is not
> simplifying things at all.
> 
> of course if you are just arguing that we should define the enum pixfmts
> based on bytes and then have #ifdef WORDS_BIGENDIAN #defines
> mapping them to more practical things, i dont really object to that
> it is conceptually more correct for example when passing the pixfmt value
> over a network ...
> but then we have even more things depending on WORDS_BIGENDIAN

In any way, IMHO having WORDS_BIGENDIAN in installed header, is _not_
possible and acceptable. API should be endian-independant.

>> 'R' is (buf[0] >> 3) & 0x1f and 'B' is (buf[1] >> 1) & 0x1f, if RGB5551,
>> 'R' is (buf[0] >> 2) & 0x1f and 'B' is buf[1] & 0x1f, if RGB1555
> 
> with this system we would need
> 8 formats for 15bit alone
> 2x due RGB vs BGR
> 2x due to big vs. little
> 2x due to 5551 vs 1555

Well, I only see 4 needed pixfmt:
RGB1555, RGB5551, BGR5551, BGR1555.

Because this is only what is stored as uncompressed rgb in _files_.
(memcpy)

After this, libswscale choose to do whatever it wants with these 4
pixfmt based on which arch it was compiled for.

> besides you skiped explaining how green is stored, this is where it becomes
> more clear why its not a good idea to use bytes.

Well, IMHO this is easy,
RGB5551: rrrrrgggggbbbbb1
        |   0    |   1   |

Here I give you the schema for the doxygen ;)

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