[cairo] CAIRO_FORMAT_ARGB32 format [ was: Re: "data" in cairo_image_surface_get_data ()

[Baz]

On Nov 30, 2007 3:58 AM, Nguyen Vu Hung <vuhung16plus at gmail.com> wrote:
> 2007/11/29, Baz <brian.ewins at gmail.com>:
> > Just to spell things out - this is what the previous post implied:
> >      for (i = 0; i < h; i++) {
> >        ppixel = data + i * wpl;
> >        rowptr = (U32 *)(data_cairo + i * w * 4);
> >        for ( j = 0; j < w ; j++) {
> >            rowptr[j] = ppixel[j] >> 8;
> >        }
> >     }
> >
> Did you missed something?
> data_cairo, rowptr are pointers of U8*,  but you've casted it to (U32 *)?

I am casting it deliberately, so that I am writing the data a word at
a time. Obviously you need to change the declaration of rowptr for
that to compile.

>
> rowptr = (U32 *)(data_cairo + i * w * 4);
>
> Or did you mean I have to use
>
> U32* rowptr instead of U8* rowptr?

Yes.

>
>  rowptr[j] = ppixel[j] >> 8; shifts ppixel 8 bits to the right? But
> can I make sure that the the last 8 bits ( alpha chanel ) are OK?

I explained this all a couple of mails back. The PIX format is RGBX,
in MSB->LSB order. Cairo's RGB24 is XRGB, in MSB->LSB order. Hence if
you have a U32 containing a PIX pixel, (ie ppixel[j]) containing, say,
0xaabbcc00, then (ppixel[j] >> 8) is 0x00aabbcc - which is the correct
value for the cairo pixel.

The alpha bits are *unused* in both PIX and RGB24. You don't care what's in it.

>
> When I did so ( declare U32 *rowptr; and loop ), the got an output PNG
> file that has all of its pixels are black :).

If the code you posted below is close to what you used, there are
multiple bugs in it that could have caused this problem.

>
> Is that has something doing with CAIRO_FORMAT_RGB24 in
> cairo_image_surface_create_for_data ?
>

No.

>
> > ... if you go back and reread what I said in the previous mail you
> > should see that this loop can be replaced with two memcpy()s, one for
> > the big-endian case and one for the little-endian case.
> >
> It is somewhat tricky and for me, I found it is hard to understand. I
> also want to improve the readability of my code so that the next
> maintainer will read it easy.
>
> Here is my code:
>
>                     U32 wpl = pixGetWpl(pixCut);
>                     U32 w = pixGetWidth(pixCut);
>                     U32 h = pixGetHeight(pixCut);
>
>                     cairo_surface_t* tmp_surface;
>                     U32* data_cairo;

^ this is unnecessary. I'd leave it unsigned char *. It also makes
your pointer arithmetic incorrect.

>
>                     int stride = wpl * 4;
>
>                     data_cairo = (unsigned char *) malloc(w * 4 * h );
>
>                     unsigned char* rowptr;
>
>                     for (i = 0; i < h; i++) {
>                       ppixel = data + i * wpl;
>                       rowptr = (U8 *) (data_cairo + i * w * 4);

^U8 is wrong here, because of what you do next. Also, because you
declared data_cairo as U32, the number you're adding to it is a number
of words, not bytes. So you meant (data_cairo + i * w) - think about
what's going on on that previous line with ppixel. If you used this
code with data_cairo being a U32, you were writing the image data in
the wrong place.

That first article on pointer arithmetic maybe didn't make it clear
enough. This:
ptr + 4
is exactly the same as this:
&(ptr[4])

... no matter what type of pointer 'ptr' is.

>                       for ( j = 0; j < w; j++) {
>                         rowptr[j] = ppixel[j] >> 8;

^ this will write the wrong data, repeatedly. Suppose rowptr is
looking at these bytes:
xrgbxrgbxrgbxrgb

and the PIX pixels all contain 'RGBX'. Then that line repeatedly tries
to assign XRGB to a U8, which is too small to contain it. IIRC the
value stored in the byte will be just 'B'. So after one iteration the
the cairo row will look like:
Brgbxrgbxrgbxrgb

The second time around, rowptr[1] doesn't point to the second cairo
pixel, but the second /byte/ because you used U8. So now the row will
look like:
BBgbxrgbxrgbxrgb

and after assigning the values from all 4 pixels in this row:
BBBBxrgbxrgbxrgb

>                       }
>                     }
>
>                     tmp_surface = cairo_image_surface_create_for_data (data_cairo,
>                                                                        CAIRO_FORMAT_RGB24,
>                                                                        w, h,
>                                                                        stride);
>

Please try:

U32 wpl = pixGetWpl(pixCut);
U32 w = pixGetWidth(pixCut);
U32 h = pixGetHeight(pixCut);

// as I said in the last mail: since you're not buffer sharing, don't
bother with create_for_data
cairo_surface_t* tmp_surface = cairo_image_surface_create
(CAIRO_FORMAT_RGB24, w, h);
// data_cairo is a pointer to bytes.
unsigned char* data_cairo = cairo_image_surface_get_data (tmp_surface);
// stride is the width of a row of the cairo surface, in bytes.
int stride = cairo_image_surface_get_stride (tmp_surface);

// U32 is the size of an RGB24 pixel.
U32* rowptr;

for (i = 0; i < h; i++) {
  ppixel = data + i * wpl;
  // in the previous mail I was trying to say that you should use stride when
  // dealing with the size of a row of the cairo surface. Note that data_cairo
  // is in bytes and that stride is a number of bytes.
  rowptr = (U32 *) (data_cairo + i * stride);
  for ( j = 0; j < w; j++) {
     // rowptr is a pointer to a U32, so its big enough to hold the value being
     // assigned here. it also means that rowptr[j] refers to the j'th pixel of
     // the row; if rowptr was U8 it would just refer to the j'th byte.
     rowptr[j] = ppixel[j] >> 8;
  }
}


>
> --
>
> Best Regards,
> Nguyen Hung Vu
> vuhung16plus{remove}@gmail.dot.com
> An inquisitive look at Harajuku
> http://www.flickr.com/photos/vuhung/sets/72157600109218238/
>