Does CairoRenderContext::make_image() work on big endian systems?

[psychon]

Hi,

I just passed by and saw this code:

piet/piet-cairo/src/lib.rs

Lines 270 to 299 in 1568b6d

	ImageFormat::Rgb => {
	for x in 0..width {
	data[dst_off + x * 4 + 0] = buf[src_off + x * 3 + 2];
	data[dst_off + x * 4 + 1] = buf[src_off + x * 3 + 1];
	data[dst_off + x * 4 + 2] = buf[src_off + x * 3 + 0];
	}
	}
	ImageFormat::RgbaPremul => {
	// It's annoying that Cairo exposes only ARGB. Ah well. Let's
	// hope that LLVM generates pretty good code for this.
	// TODO: consider adding BgraPremul format.
	for x in 0..width {
	data[dst_off + x * 4 + 0] = buf[src_off + x * 4 + 2];
	data[dst_off + x * 4 + 1] = buf[src_off + x * 4 + 1];
	data[dst_off + x * 4 + 2] = buf[src_off + x * 4 + 0];
	data[dst_off + x * 4 + 3] = buf[src_off + x * 4 + 3];
	}
	}
	ImageFormat::RgbaSeparate => {
	fn premul(x: u8, a: u8) -> u8 {
	let y = (x as u16) * (a as u16);
	((y + (y >> 8) + 0x80) >> 8) as u8
	}
	for x in 0..width {
	let a = buf[src_off + x * 4 + 3];
	data[dst_off + x * 4 + 0] = premul(buf[src_off + x * 4 + 2], a);
	data[dst_off + x * 4 + 1] = premul(buf[src_off + x * 4 + 1], a);
	data[dst_off + x * 4 + 2] = premul(buf[src_off + x * 4 + 0], a);
	data[dst_off + x * 4 + 3] = a;
	}

This code is wrong. I do not understand it completely, but the swapping of the order of channels (stuff like data[foo + 0] = buf[bar + 2]) looks very much like a byte order conversion.
From https://www.cairographics.org/manual/cairo-Image-Surfaces.html#cairo-format-t, CAIRO_FORMAT_RGB24 is described as:

each pixel is a 32-bit quantity, with the upper 8 bits unused. Red, Green, and Blue are stored in the remaining 24 bits in that order. (Since 1.0)

This means that in C, if you have a pixel that starts at address ptr, the blue channel can be read via uint8_t blue = *((uint32_t*)ptr) & 0xff. This reads an uint32_t / u32 in the native endianness and then gets the lowest byte from that. On little endian, the least significant byte is stored first, so here this is equivalent to uint8_t blue = *ptr. On big-endian however, this ends up doing uint8_t blue = *(ptr + 3);.

I hope this is understandable.

Perhaps you could also clarify in https://docs.rs/piet/0.1.0/piet/enum.ImageFormat.html that the format is meant to be as "with first byte for the red channel, second byte for green, and third byte for blue" or something like that (to tell it apart from cairo's use of a "32 byte quantity" with its endianness).

Slightly unrelated:

piet/piet-cairo/src/lib.rs

Line 280 in 1568b6d

// TODO: consider adding BgraPremul format.

I am not shure how changing the order of the channels would help here. Right now you copy from offsets (2,1,0,3). After swapping the order of channels, you end up with (3,0,2,1). That's not really an improvement. Perhaps ABGR is meant by the comment? That would match cairo's ARGB order on little endian systems... well, no... now you got me really confused. Anyway, something is up with that TODO.

Edit: Random code link to underline what I mean: https://sources.debian.org/src/gtk+2.0/2.24.32-4/gdk/gdkcairo.c/?hl=211#L238
At the time of writing, this has the following code:

#if G_BYTE_ORDER == G_LITTLE_ENDIAN
	      q[0] = p[2];
	      q[1] = p[1];
	      q[2] = p[0];
#else	  
	      q[1] = p[0];
	      q[2] = p[1];
	      q[3] = p[2];
#endif

and 20 lines later in the file is the ARGB case:

#define MULT(d,c,a,t) G_STMT_START { t = c * a + 0x80; d = ((t >> 8) + t) >> 8; } G_STMT_END

	  while (p < end)
	    {
#if G_BYTE_ORDER == G_LITTLE_ENDIAN
	      MULT(q[0], p[2], p[3], t1);
	      MULT(q[1], p[1], p[3], t2);
	      MULT(q[2], p[0], p[3], t3);
	      q[3] = p[3];
#else	  
	      q[0] = p[3];
	      MULT(q[1], p[0], p[3], t1);
	      MULT(q[2], p[1], p[3], t2);
	      MULT(q[3], p[2], p[3], t3);
#endif
	      
	      p += 4;
	      q += 4;
	    }
	  
#undef MULT

[cmyr]

This does look like an oversight. I'm not sure what the TODO is about. Thanks for all the detail. :)

[raphlinus]

It's not a simple endian conversion, there are four choices of byte order for RGBA pixels even on little-endian systems, and you do see all four in use: A in the high or low byte, and either R or B at the other end. It's possible the code or comments could be cleaned up, but I'm not seeing a real problem here.

Regarding whether this works on big-endian systems, it's possible there's a problem, but it's a little tricky to investigate, we'd have to find out what order Cairo has, it's not a simple representation question. I've looked into testing when we've had endianness-specific code before (see projectfluent/fluent-langneg-rs#8 for an example of the latter), but it's not easy to get one's hands on a testing environment.

[psychon]

we'd have to find out what order Cairo has

Sure, no problem. Here is a git repo doing just that: https://github.com/psychon/cairo-endian-test
It builds on Travis CI as amd64 (little endian) and s390x (big endian): https://travis-ci.org/github/psychon/cairo-endian-test/builds/705057283

Little endian output (first are the individual bytes for that pixel and then comes the pixel as uint32_t):

   red: 0x00 0x00 0xff 0xff = 0xffff0000
 green: 0x00 0xff 0x00 0xff = 0xff00ff00
  blue: 0xff 0x00 0x00 0xff = 0xff0000ff
 blank: 0x00 0x00 0x00 0xff = 0xff000000

Big endian output:

   red: 0xff 0xff 0x00 0x00 = 0xffff0000
 green: 0xff 0x00 0xff 0x00 = 0xff00ff00
  blue: 0xff 0x00 0x00 0xff = 0xff0000ff
 blank: 0xff 0x00 0x00 0x00 = 0xff000000

As you can see, the uint32_t version stays the same, but the individual bytes are swapped.
For piet, I would suggest something like the following so that you do not end up with endian-specific versions of the code:

let (r, g, b, a) = whatever_bytes_you_want_to_write_as_u8s();
let (r, g, b, a) = (u32::from(r), u32::from(g), u32::from(b), u32::from(a));
let pixel: u32 = a << 24 | r << 16 | g << 8 | b << 0;
data[dst_off + x * 4..dst_off + x*4 + 4].copy_from_slice(pixel.to_ne_bytes()); // Cairo's pixel are in native endianness, hence to_ne_bytes()

[cmyr]

@psychon do you have any interest in making a PR?

[psychon]

Not really, sorry. I never even build piet on my computer and have not even checked how to build & test this.
I came from druid (never submitted a PR there either) and was looking for a way to get the cairo::Context or cairo::Surface out (luckily there was none - just what I wanted). While scrolling through the code, I noticed this byte order issue and wanted to leave a comment. Ever since I learnt how the byte ordering works in cairo, I became sensitive to this kind of issue.

[cmyr]

no worries, thanks for opening this! a fix is going to be pretty low priority, but i'm sure it will happen on a long enough time scale. 😉