bugGnash - The GNU Flash player - Bugs: bug #22799, gnash 0.8.2...

 
 

bug #22799: gnash 0.8.2 agg_detect_pixel_format should not check endianess

Submitted by:  Nico Coesel <ncoesel>
Submitted on:  Tue 01 Apr 2008 11:30:25 AM UTC  
 
Category: gui-fbSeverity: 3 - Normal
Release: NoneStatus: Fixed
Privacy: PublicAssigned to: Udo Giacomozzi <udog>
Open/Closed: Closed

Add a New Comment(Rich Markup)
   

You are not logged in

Please log in, so followups can be emailed to you.

 

(Jump to the original submission Jump to the original submission)

Thu 10 Apr 2008 01:00:33 PM UTC, comment #21:

Color channel order, bit position, frame buffer format, AGG format... there are many places where endianess can matter. From my understanding this should apply:

- the hardware framebuffer (the real memory as seen by the graphics chip) can be in any order. RGB, RGBA, BGRA, ABGR, ARGB, ...
=> Gnash doesn't care much about this (just like Nico said)

- the graphics chip probably is able to switch between different orders and this is probably decided by the framebuffer driver (depending on OS and host endianess, probably)
=> Gnash doesn't care either

- the framebuffer driver describes the hardware format (like "red channel is 8 bits wide and starts af bit offset zero") and it does so probably respecting the host endianess! Like the same graphics card would report different format values on differen endian hosts. Could also be that it matters how data is transferred between gfx and main memory. In either case, when the framebuffer says "green at bit offset 8" it means that the host needs to to a 8 bit shift on a 32 bit value.
=> this is important for Gnash/AGG!

- for 15,16 bit pixel formats the AGG library uses bit shifting, which means the resulting memory depends on the host endianess and so the values reported by the frame buffer are okay.
=> that's why the new Gnash patch does NOT translate these bits anymore for this mode

- for 24,32 bit pixel formats the AGG library uses an array instead of bit shifting. This means that the resulting memory will always be the same, no matter what the host endianess is!
=> Gnash needs to translate (swap) the bit positions so that the order is still correct. If you like, you can say it's a bug in AGG and Gnash has a workaround for it.

- the "frame buffer format" reported in Gnash debug output is the one that is choosen to configure AGG. It doesn't necessarily tell you what the physical memory layout is.

- 64 bit formats will still need to be swapped because AGG still uses an array to address the single channels:

http://www.antigrain.com/__code/include/agg_pixfmt_rgba.h.html#pixfmt_rgba64

> what does the RGB24 pixel formatted framebuffer look like on a big endian host?


There is no single answer to this question. Depends on your point of view. From the CPU point of view it is:

byte0 byte1 byte2 byte3 byte4 byte5 ... ...
B G R B G R ... ...

But if you take a 32 bit integer for the pixel then you will use the same shifting values on both little and big endian hosts.

Closing this bug item. Discussion can be continued, if you like...

Udo Giacomozzi <udog>
Project MemberIn charge of this item.
Thu 10 Apr 2008 08:40:15 AM UTC, comment #20:

I've had a small e-mail conversation with Geert Uytterhoeven (one of the creators of the framebuffer device) on this topic to clear things up. What he told me comes down to that the framebuffer layout can be anything depending on the actual hardware.

For as long as the colors are on 8 bit boundaries the colors can be mapped by using the bit shift and masks provided by the framebuffer device so endianess is not a problem as long as the framebuffer driver provides the proper mapping. When it comes to RGB values which are not on 8 bit boundaries (like 565, 555 coding) then endianess comes into play and hardware support to swap bytes is needed.

Nico Coesel <ncoesel>
Thu 10 Apr 2008 08:25:21 AM UTC, comment #19:

48bits and 64bits pixel are already implemented in AGG.

OK, the last question before closing this item. what does the RGB24 pixel formatted framebuffer look like on a big endian host?

On my little endian host, it looks like this:

byte0 byte1 byte2 byte3 byte4 byte5 ... ...
R G B R G B ... ...

Zou Lunkai <zoulunkai>
Wed 02 Apr 2008 02:53:28 PM UTC, comment #18:

32 bits ought to be enough for anybody. ;)

48 bits would still be a 3 channel color, and AGG will probably still use an array for that (so endianess is still meaningful!). It's not the channel data type that matters, it's that an array is used to access the pixel data.

However, it makes no sense to bother about these modes unless AGG has implemented them :)

All other points were already clear to me. Tests appreciated....

Udo Giacomozzi <udog>
Project MemberIn charge of this item.
Wed 02 Apr 2008 02:17:34 PM UTC, comment #17:

Another two points:

(1)when thinking about rgb/bgr 24 or 48 bits pixel format used in agg, it does not make sense to talk about the endianess of the rendering buffer.

(2)when thinking about 48 bits pixel format and 64 bits pixel format supported in agg, the rendering buffer is even not necessarily a bytes array. It's a uint16s array! endianess is meaningless again.

Zou Lunkai <zoulunkai>
Wed 02 Apr 2008 01:50:32 PM UTC, comment #16:

Udo: Confirmed

(1)AGG assumes the bytes order of the rendering buffer in both 24 and 32 bits mode(see agg_pixfmt_rgb.h and agg_pixfmt_rgba.h). The bytes order of the rendering buffer is configurable. RGBA32(the predefined term in agg) always means this bytes order: R G B A R G B A ...... of the bytes array(the rendering buffer). How your gfx card interpret this byte order is another topic(eg. you may think this byte array as a ABGR32 frame buffer, and each pixel is a long integral).

(2)For 16/15 bits pixel mode, agg is endianess INSENSITIVE(see agg_pixfmt_rgb_packed.h), because no byte indexing is used.

BTW, when I talk about 'RGBA32' in this topic, I always mean the term used in AGG, not a general word in general context.

Zou Lunkai <zoulunkai>
Wed 02 Apr 2008 12:30:07 PM UTC, comment #15:

>It seems AGG is reversing the byte order depending on the endian mode with a comment it is for the PPC.


A quick search for "PPC" did not find any matches in my AGG code and the only occurrences for "endian" are in the font handling. Anyway, the byte order is hardcoded in agg_color_rgba.h:

struct order_rgb { enum rgb_e { R=0, G=1, B=2, rgb_tag }; }; //----order_rgb

..and the value is the index in the byte array.

Whatever... I just updated CVS HEAD.. See if this works for you.

Udo Giacomozzi <udog>
Project MemberIn charge of this item.
Wed 02 Apr 2008 11:50:46 AM UTC, comment #14:

The output is produced with my patch.

I took a quick look at the AGG code (case insensitive search for 'endian'). It seems AGG is reversing the byte order depending on the endian mode with a comment it is for the PPC. Furthermore it seems there is some endianess dependancy in AGG in the font handling.

My guess is that AGG is reversing the RGBA order while it shouldn't do that. Gnash is reversing the byte order again so 2 reversals make the byte order effectively unchanged.

Nico Coesel <ncoesel>
Wed 02 Apr 2008 11:10:40 AM UTC, comment #13:

Indeed, my assumption was correct.

See premultiply() in http://www.antigrain.com/__code/include/agg_pixfmt_rgba.h.html

and
http://www.antigrain.com/__code/include/agg_color_rgba.h.html

It accesses the memory as an 8 bit integer array.

In 16 bpp mode however it uses bit shifting:
http://www.antigrain.com/__code/include/agg_pixfmt_rgb_packed.h.html

So we do need to check the host endianess but only for 32bpp color modes. I'll provide a patch in a few moments...

Udo Giacomozzi <udog>
Project MemberIn charge of this item.
Wed 02 Apr 2008 10:06:08 AM UTC, comment #12:

>784] 04:07:24: DEBUG: red channel: 11 / 5
>784] 04:07:24: DEBUG: green channel: 5 / 6
>784] 04:07:24: DEBUG: blue channel: 0 / 5
>784] 04:07:24: DEBUG: Total bits per pixel: 16
>784] 04:07:24: DEBUG: framebuffer pixel format is RGB565
>(big-endian host)


Is this with or without your patch? You said that without your patch Gnash doesn't recognize the pixel format..

>You cannot assume the framebuffer to be an array of bytes.


Well the framebuffer is esentially only an array of bytes - I just get a chunk of memory. Still, it's basically possible that the framebuffer driver configures the gfx card to interpret the memory data in different ways based on the CPU endianess. I never heard about this but I see no reason why it should not be that way.

Anyway, what matters is the bit order reported by the framebuffer interface.

>The example below makes it more clear why application shouldn't bother with endianess of the framebuffer.


Don't forget that I have to bother about the low level pixel rendering too, ie. AGG itself. If AGG is endianess sensitive then Gnash is responsible to configure AGG correctly, no matter what happens at hardware level in the framebuffer.

Currently it seems that AGG is endianess sensitive for 24/32bpp modes but not for 15/16 bpp modes. I know that 16 bpp modes are implemented using bit shifting (so that would match my assumption). I need some more time to investigate 32bpp modes, since they are implemented differently and AGG is a huge thing with lots of optimizations.....

Udo Giacomozzi <udog>
Project MemberIn charge of this item.
Wed 02 Apr 2008 08:18:01 AM UTC, comment #11:

No, that is not right. You cannot assume the framebuffer to be an array of bytes. A framebuffer consists of pixels. When the pixels are assembled usually a 16 bit or 32 bit type is used. The order in the memory depends on the endianess of the processor.

So RGBA may be stored as RGBA of ABGR. The example below makes it more clear why application shouldn't bother with endianess of the framebuffer.

Lets say you have a 30bpp framebuffer with 10 bit RGB values. In an unsigned long (which is 32 bits on virtually every platform BTW) a pixel looks like:

xxrrrrrr rrrrgggg ggggggbb bbbbbbbb

Depending on the endianess, the pixel will be stored in the framebuffer as follows:

xxrrrrrr rrrrgggg ggggggbb bbbbbbbb

or

bbbbbbbb ggggggbb rrrrgggg xxrrrrrr

The last byte ordering cannot be created using the bit shift/mask parameters provided by the framebuffer device driver. This makes it clear that the framebuffer pixels have to be created using a type which is multiple bytes in size (16, 24, 32 bit or even larger) using the host's endianess. The framebuffer hardware will read the data using the correct endianess ofcourse.

Nico Coesel <ncoesel>
Wed 02 Apr 2008 07:41:38 AM UTC, comment #10:

> No, you shouldn't think about addresses if you want to write
> endian insensitive code.


hmm, I understand your example.

I believe in AGG, A "RGBA32 pixel" is a four-bytes-thing. It's not long or int type.

If you define an RGBA32 frame buffer, pixels are assumed to be stored like this:

R G B A R G B A R G B A ......

endianess doesn't make sense if you ONLY think about addresses instead of any data types longer then 8bits. frame buffer is just a bytes container to me, there's no predefined data types for it.

Zou Lunkai <zoulunkai>
Wed 02 Apr 2008 07:17:48 AM UTC, comment #9:

No, you shouldn't think about addresses if you want to write endian insensitive code.

Lets create an RGBA pixel in 2 ways:

First the wrong way (endian sensitive):

unsigned long rgba;
unsigned char *p;
unsigned char r,g,b,a;

p=(unsigned char *) rgba;
p[3]=r;
p[2]=g;
p[1]=b;
p[0]=a;

Now the right way (endian insensitive):

unsigned long rgba;
unsigned char r,g,b,a;

rgba= r<<24 | g<<16 | b<<8 | a;

The last example uses bit shifts to put the RGBA values at the proper location.

Nico Coesel <ncoesel>
Wed 02 Apr 2008 01:48:15 AM UTC, comment #8:

>The only thing a framebuffer application must deal with is bits >per pixel and the offset for the color channels. If the colors >show wrong then this must be solved in the framebuffer driver.


Agree. Conceptually(by looking at the input parameters), agg_detect_pixel_format(...) doesn't need to consider the endianess of the host.

No matter what endianess the host is, an RGBA32 pixel should always organized like this in memory(assumming 8bits per unit):
address X: R
address X+1: G
address X+2: B
address X+3: A

right?

Zou Lunkai <zoulunkai>
Tue 01 Apr 2008 02:08:53 PM UTC, comment #7:

When using 16bpp modes, the endianess of the video memory must always match the endianess of the processor. Otherwise the bytes will be swapped liked I showed before. This cannot be fixed by the bit offsets and bit masks provided by the framebuffer. So the endianess must be resolved by the hardware. The LCD controller in the AU1100 SoC is therefore capable of swapping the RGB bits in several ways. I'm very sure I got it right because every X windows application I run on this device -including gtk-gnash- shows the right colors.

The only thing a framebuffer application must deal with is bits per pixel and the offset for the color channels. If the colors show wrong then this must be solved in the framebuffer driver.

Here is the gnash output you requested:
gnash -v -vb -j 320 -k 234 -r 1 motion_guide.swf
784] 04:07:24: DEBUG: Verbose output turned on
784] 04:07:24: DEBUG: Setting height to 234
784] 04:07:24: DEBUG: Could not open /dev/input/event0: No such file or director
y
784] 04:07:24: DEBUG: You won't have any pointing input device, sorry.
784] 04:07:24: DEBUG: Could not open /dev/input/event0: No such file or director
y
784] 04:07:24: DEBUG: You won't have any keyboard input device, sorry.
784] 04:07:24: DEBUG: Framebuffer device uses 599040 bytes of memory.
784] 04:07:24: DEBUG: Video mode: 320x234 with 16 bits per pixel.
784] 04:07:24: DEBUG: Double buffering enabled
784] 04:07:24: DEBUG: red channel: 11 / 5
784] 04:07:24: DEBUG: green channel: 5 / 6
784] 04:07:24: DEBUG: blue channel: 0 / 5
784] 04:07:24: DEBUG: Total bits per pixel: 16
784] 04:07:24: DEBUG: framebuffer pixel format is RGB565 (big-endian host)
784] 04:07:24: DEBUG: initialized AGG buffer <0x2b804008>, 599040 bytes, 320x234
, rowsize is 640 bytes
784] 04:07:24: DEBUG: Original TTY NO = 1
784] 04:07:24: DEBUG: VT 1 ready

Nico Coesel <ncoesel>
Tue 01 Apr 2008 01:43:29 PM UTC, comment #6:

>I'm using a framebuffer device in 565RGB mode (Alchemy AU1100 system on chip).


Please post full Gnash verbose output. It provides important information for me (like the reported bit order, for example).

>Gnash tells met the pixel format is unsupported without my patch.


Then it might just not yet be implemented and I can fix that once I see what information Gnash has (ie. the stuff printed in the debug log).

>For instance when using 565 RGB format:
>"rrrrrggg gggbbbbb" would be transformed into "gggbbbbb rrrrrggg" when the bytes are swapped due to a different endian mode so this would be badly broken.


You are referring on how the bits are arranged in video memory. The problem is however, that the pixel value is calculated at once, to a 16 bit integer using bit shifting.

Example:

unsiged short int foo = 1 << 10;
unsigned char* ptr = &foo;
printf("%d %d\n", ptr[0], ptr[1]);

...will print different values depending on your CPU endianess. The gfx card sees the memory like the printf() function, but in AGG it is always calculated the same way (unless you teach it to swap the bytes, like we do currently).

The goal is to get the same bit mask in the video memory regardless of what CPU is used.

I guess the way AGG calculates 15/16 bit pixels is some way immune to the CPU endianess (unlike 32 bit modes as we know). But to be sure I need the verbose output from the backend...

>I can't find the thread you are refering to; do you have a direct link to it?


http://search.gmane.org/?query=ppc+blue&group=gmane.comp.gnu.gnash.devel

Udo Giacomozzi <udog>
Project MemberIn charge of this item.
Tue 01 Apr 2008 01:09:24 PM UTC, comment #5:

I'm using a framebuffer device in 565RGB mode (Alchemy AU1100 system on chip). Gnash tells met the pixel format is unsupported without my patch. I cannot use 32bpp modes.

It is possible to fix 32bpp modes by re-arranging the bytes, but this is not possible with 15bpp or 16bpp modes because the split-point is half way of a color value.

For instance when using 565 RGB format:
"rrrrrggg gggbbbbb" would be transformed into "gggbbbbb rrrrrggg" when the bytes are swapped due to a different endian mode so this would be badly broken.

I can't find the thread you are refering to; do you have a direct link to it?

Nico Coesel <ncoesel>
Tue 01 Apr 2008 12:35:04 PM UTC, comment #4:

Please check all of the following and tell us:

- what color mode is your graphics card set on? (fbset should tell)

- what color mode is Gnash using without your patch? (use -v option to see that, all Gnash+AGG output is useful)

- can you try a 32 bit gfx mode? AGG treats packed 16 bit and 32 bit pixel modes slightly differently, so there might be the problem.

Note: all tests without your patches, please.

Udo Giacomozzi <udog>
Project MemberIn charge of this item.
Tue 01 Apr 2008 12:15:31 PM UTC, comment #3:

> The framebuffer device is endian transparent by design


The device endianess is transparent but the pixel calculations are not (R/G/B/A to 32 bit value conversion).

After all this function has been introduced when Gnash+FB showed wrong results on a big endian machine. The endian check solved that problem.

FYI, I'm talking about the Gnash-dev thread "PPC blue video" (Aug 2007).

So, better describe your hardware so we can analyze the problem.

Udo Giacomozzi <udog>
Project MemberIn charge of this item.
Tue 01 Apr 2008 11:58:31 AM UTC, comment #2:

Okay, but how does this affect AGG pixel format detection on the X server? Have you tested the GTK GUI with this change?

Bastiaan Jacques <bjacques>
Project Member
Tue 01 Apr 2008 11:43:26 AM UTC, comment #1:

The title should ofcourse say "gnash 0.8.2 agg_detect_pixel_format should NOT check endianess"

Nico Coesel <ncoesel>
Tue 01 Apr 2008 11:30:25 AM UTC, original submission:

The function agg_detect_pixel_format in backend/render_handler.cpp at line 2174 should not check for endianess. The framebuffer device is endian transparent by design so there is no need to swap rgb positions.

I had to remove the test in order to get fb-gnash working on an RGB 565 system in big-endian mode. Consider this tested :-)

I've attached a patch for easy inclusion.

Nico Coesel <ncoesel>

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attach File(s):
   
   
Comment:
   

Attached Files

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -unavailable- added by zoulunkai (Posted a comment)
  • -unavailable- added by udog (Posted a comment)
  • -unavailable- added by bjacques (Posted a comment)
  • -unavailable- added by ncoesel (Submitted the item)
  •  

    Do you think this task is very important?
    If so, you can click here to add your encouragement to it.
    This task has 0 encouragements so far.

    Only logged-in users can vote.

     

    Please enter the title of George Orwell's famous dystopian book (it's a date):

     

     

    Follow 6 latest changes.

    Date Changed By Updated Field Previous Value => Replaced By
    Thu 10 Apr 2008 01:00:32 PM UTCudogStatusReady For Test=>Fixed
      Open/ClosedOpen=>Closed
    Wed 02 Apr 2008 12:30:07 PM UTCudogStatusNone=>Ready For Test
    Tue 01 Apr 2008 12:15:31 PM UTCudogSummarygnash 0.8.2 agg_detect_pixel_format should check endianess=>gnash 0.8.2 agg_detect_pixel_format should not check endianess
    Tue 01 Apr 2008 11:58:31 AM UTCbjacquesAssigned toNone=>udog
    Tue 01 Apr 2008 11:30:25 AM UTCncoeselAttached File-=>Added render_handler_agg.cpp.patch, #15369

    Back to the top


    Powered by Savane 3.1-cleanup1