Fallout from https://bugzilla.mozilla.org/show_bug.cgi?id=383668#c9 As bug 383668 was fixed Windows only we still either have to fix gfxImageFrame::GetAlphaData, perhaps as suggested in attachment 270623 [details] [diff] [review], or remove GetAlphaData altogether and its usage in (OS/2's and Windows' nsWindow). I haven't tested in a while but I guess the testcase attachment 267723 [details] otherwise still crashes OS/2.

Get/SetAlphaData are no longer applicable in the Cairo based builds. See bug 367281 – Remove SetImageData/SetAlphaData from gfxImageFrame. Instead the OS2 code should instead of using GetAlphaData, use GetImageData, and then test the 'A' byte of each Cairo pixel (which is ARGB, of which A is the highest byte). Note, A1 is no longer A1 anymore, but also handled as A8. This could be fixed fairly easy. Pass to CreateTransparencyMask, the mImageData pointer as received from GetImageData. In CreateTransparencyMask, make pSrc a PRUint32* and then replace 'if (*pSrc++ < 128)' with 'if (*pSrc++ < 0x800000)', etc...

The OS/2 Cursor code really needs updating to work with Cairo -> Changed summary to reflect this. 24 The bitmap has a maximum of 2^24 colors. The bmciColors member is NULL, and each 3-byte sequence in the bitmap array represents the relative intensities of red, green, and blue, respectively, of a pixel. So, we need to convert the 32bit Cairo pixels (for both RGB24 and ARGB32) to the 3 bytes R,G,B. From Cairo, cairo-os2-surface.c: 305 /* copy the first three bytes for each pixel but skip over the fourth */ 306 for (ulPixels = 0; ulPixels < surface->bitmap_info.cx * surface->bitmap_info.cy; ulPixels++) 307 { 308 /* copy BGR from source buffer */ 309 *pchPixBuf++ = *pchPixSource++; 310 *pchPixBuf++ = *pchPixSource++; 311 *pchPixBuf++ = *pchPixSource++; 312 pchPixSource++; /* jump over alpha channel in source buffer */ 313 } Note that Windows can handle 32-bit BMP's, but OS/2 apparently not (I can't find any documentation about BMP's in OS/2).

Note, bug 389729 only wants to remove non-Cairo code (which doesn't work anymore as the image decoders only work in Cairo) after all OS/2-Cairo issues have been solved.

This shows how Cairo pixels can be converted into OS/2 bitmap data (RGB and XOR/AND masks). It will probably not compile and not work in reality, as I don't have access to an OS/2 system to it out. But the logic should be fine.

Peter, can you take this from through the compilation, testing, reviewing process? SetAlphaData will disappear soon, and the OS/2 code won't even compile anymore.

Revised testcase, the one from bug 383668 doesn't work any longer because the Google server addresses have changed.

Comment on attachment 277752 [details] [diff] [review] Proof-of-concept patch Alfred, thanks a lot for taking a stab at this! There are some minor problems to get it to compile: >+ PRInt32* pSrc = (PRUint32*)aImageData; pSrc should be PRUint32 otherwise the cast doesn't work. >+ HBITMAP CreateBGR24(PRUint8* aImageData, PRUint32 aWidth, PRUint32 aHeight); This definition in nsWindow.h should also be CreateBMP24. Then I tried it out. The testcase I just uploaded has the problem with the black background and the pointer is upside down (on OS/2 the y coordinates are reversed). I haven't looked at the code in detail yet, probably at least the latter problem is easily solved by reversing the for loop that counts the rows.

(In reply to comment #7) > (From update of attachment 277752 [details] [diff] [review]) > Alfred, thanks a lot for taking a stab at this! There are some minor problems > to get it to compile: > > >+ PRInt32* pSrc = (PRUint32*)aImageData; > > pSrc should be PRUint32 otherwise the cast doesn't work. Ah, that was supposed to be a signed (PRInt32*) buffer for a test further down. But that test doesn't work. Trying to debug the testcase I see that pSrc bytes are 0xffffffff for white, 0xff000000 for black, and 0x0 for transparent. But if I test for *pSrc++ == 0 then I get an upward white hand pointer and a downward black pointer. I also see that none of the cursors on http://e-vertise.com/warpzilla/OS2_Pointer_Test.html work...

Actually the test should have been if (*pSrc++ >= 0) (Negative values have high bit set and can then be considered not-transparent (like the white and black above), but positive values (like 0x0 and 0x00010101) are transparent. As for the reversal, in Windows one could also pass a negative height to indicate a top-down image. Otherwise one has to reverse both the image and the masks.

OK, with the >= test I get transparency most of the time when I should. To construct the bitmaps upside down, a negative height for the bitmapheader structure doesn't work (it's defined as ULONG and when I try anyway the cursors are not drawn at all). Instead, the loop in CreateBMP24 has to start with for (PRUint32 row = aHeight; row > 0; --row) { PRUint8* pDst = bmp + bpr * (row - 1); and I think to get the transparency reversed the correct thing might be to add pAnd += aHeight * abpr / 4 - 1; before the loop and then count pAnd down instead of up at the end of the loop. But trying the more complex, scaled graphics (especially the last three on http://e-vertise.com/warpzilla/OS2_Pointer_Test.html) the algorithm doesn't work well. Transparency is only drawn partly, it looks like scaling introduces bad effects. So while the cursors are drawn they don't look as nice as in Gecko 1.8.x... Will try to find out more tomorrow.

As I am not pretending to have working patches, here is proposal for the alpha mask construction in the bottom-up fashion: // init the XOR/AND bitmap to produce either black or transparent pixels memset(mono, 0x00, cbData * 2); switch (format) { // make the AND mask the inverse of the 8-bit alpha data case gfxIFormats::BGR_A1: case gfxIFormats::RGB_A1: case gfxIFormats::RGB_A8: case gfxIFormats::BGR_A8: { PRInt32* pSrc = (PRInt32*)aImageData; for (PRUint32 row = aHeight; row > 0; --row) { PRUint8* pDst = mono + abpr * (row - 1); PRUint8 mask = 0x80; for (PRUint32 col = aWidth; col > 0; --col) { // Alpha byte is highest byte, and we only check highest bit of alpha byte // So if the signed value is positive, the highest bit is 0, and therefor deemed as transparant if (*pSrc++ >= 0) dst |= mask; mask >>= 1; if (!mask) { // We've a complete mask byte, write it out *pDst++ = dst; mask = 0x80; } } // Write the last incomplete mask byte if (mask != 0x80) { *pDst++ = dst; mask = 0x80; } } break; } } Notes: * the reversal only needs in the vertical direction. Counting pAnd down will also reverse horizontally (for each DWORD). * we need to clear first the whole XOR/AND mask, as the new routines doesn't write whole rows. * simplified mask collection to one byte per go, instead of one DWORD. * it will not look as nice as on other platforms, as the cursor doesn't support alpha-blending. * scaling? when testing that page on Windows, there is no scaling involved? So, may be the above routine works better?

Thanks Alfred. Will try that suggestion tonight. The cursors are scaled because OS/2 mouse pointers are 32x32px. I have understood that as a given since https://bugzilla.mozilla.org/show_bug.cgi?id=286306#c12 and if I understand the comment in the original patch (attachment 180421 [details] [diff] [review]) correctly, it is the system that automatically scales them. Probably linearly, resulting in bad quality... Perhaps one should use some cairo function to scale it before passing to SetCursor() to get higher quality, but that's outside the scope of this bug.

See also bug 386229, as the Windows version is also not completely right right now. This was found out using your testcase! The innerloop can slighter tighther: for (PRUint32 row = aHeight; row > 0; --row) { PRUint8* pDst = mono + abpr * (row - 1); PRUint8 mask = 0x80; for (PRUint32 col = aWidth; col > 0; --col) { // Test sign as highest byte contains alpha, so negative means non-transparent. if (*pSrc++ < 0) *pDst |= mask; mask >>= 1; if (!mask) { pDst++; mask = 0x80; } } }

Actually, the first patch with the fixes from comment 7, comment 8,and comment 9 was closer to the truth. With it the transparency was upside down but at least there was transparency in the testcase. With the newest ones I get black everywhere in the pointer outside the main shape except a few vertical stripes of 1px width. And I don't understand at all why that is. I tried to construct the mask myself, pixel by pixel in a very simple manner without relying on memsets: for (PRUint32 row = aHeight; row > 0; --row) { // pointers to current pixel in XOR and AND masks PRUint8* pXOR = mono + abpr * (row - 1); PRUint8* pAND = mono + abpr * (row - 1) + cbData; for (PRUint32 col = aWidth; col > 0; --col) { // Test sign as highest byte contains alpha, so negative means // non-transparent. if (*pSrc++ < 0) { // set to white *pXOR = 1; *pAND = 0; } else { // set to transparent *pXOR = 0; // already done by the memset above *pAND = 1; } } } Using printfs I verified that the mono data buffer has all pixels set correctly, but I get the same display effect... I think there is something about bitmap creation from the buffer that I don't understand.

Try this, may be a better starting point to get this working in OS/2. Both Create* functions now work bottom-up, and the CreateTransparency now writes the AND bits in the right location (I forgot to offset the destination pointer by cbData). Also use Calloc to alloc and clear both masks in one go. For info about XOR/AND see: http://www.fileformat.info/format/os2bmp/egff.htm XOR mask should remain zero, and AND contain 1's for transparent pixels. The code is more aligned with the Windows version, where I am sure that it works.

Thanks, Alfred. Your newest patch works. :-) Yes, I had seen that XOR=0 and AND=1 should give transparent. That's why I don't understand why my simple approach that did exactly that didn't work. But your does, so I don't care any more... In the meantime I decided that I don't like the name CreateRGB24 as a function name, so I renamed it to CreateBitmapRGB which sounds more like what it does. And I added an extra comment about the two formats that don't require handling in the switch of CreateTransparencyMask any more.

Alfred did the work.

mkaply review reminder. :-)

Comment on attachment 278438 [details] [diff] [review] Final patch I thought I had reviewed this. sorry.

Thanks for the review, Mike. And especially thanks for your big help with the patch, Alfred! Checked into trunk.