see emails from Tim Rowley: Tim Rowley wrote: > > > The following message is a courtesy copy of an article > > that has been posted to netscape.public.mozilla.patches as well. > > > > This fixes ConvertRGBToRGB24 so that it no longer ignores the > > colorspace shifts. Along with the companion patch posted to > > mozilla.patches, this fixes the gtk frontend to properly display > > images on big-endian machines. > > > > --- color.cpp-orig Wed Dec 2 09:01:52 1998 > > +++ color.cpp Wed Dec 2 09:15:18 1998 > > @@ -486,14 +486,17 @@ > > { > > uint8 XP_HUGE *out = (uint8 XP_HUGE *) vout + (3 * x_offset); > > const uint8 *end = sp + num*3; > > - /* XXX this is a hack because it ignores the shifts */ > > + NI_ColorSpace *color_space = ic->image->header.color_space; > > + uint32 rindex = 2-((color_space->bit_alloc.rgb.red_shift) >> 3); > > + uint32 gindex = 2-((color_space->bit_alloc.rgb.green_shift) >> 3); > > + uint32 bindex = 2-((color_space->bit_alloc.rgb.blue_shift) >> 3); > > > > if (!mask) > > { > > while (sp < end) { > > - out[2] = sp[0]; > > - out[1] = sp[1]; > > - out[0] = sp[2]; > > + out[rindex] = sp[0]; > > + out[gindex] = sp[1]; > > + out[bindex] = sp[2]; > > out+=3; > > sp+=3; > > } > > @@ -501,9 +504,9 @@ > > while (sp < end) { > > if (*mask++) > > { > > - out[2] = sp[0]; > > - out[1] = sp[1]; > > - out[0] = sp[2]; > > + out[rindex] = sp[0]; > > + out[gindex] = sp[1]; > > + out[bindex] = sp[2]; > > } > > out+=3; > > sp+=3; > Ramiro tested the patch and it doesn't work. Follow up on the original problem. -pn

Perhaps this will clarify things: Current state of affairs: libimg: ConvertRGBToRGB24 is a gross hack that only converts to the Win32 color channel ordering win32: Receives channels in proper order mac: Doesn't use ConvertRGBToRGB24? x11 (gtk): Uses ConvertRGBtoRGB24 and then another (grosser) hack to change channel ordering into what X11 expects. This code does not work on big-endian machines. Situation after applying this patch _AND_ the gtk patch: libimg: ConvertRGBToRGB24 respects requested channel ordering win32: Receives channels in proper order mac: Doesn't use ConvertRGBToRGB24? x11 (gtk): Receives channels in proper order My testing was done on a Sparc running Solaris.

Any progress on applying these patches or getting me better feedback so I can fix them, if needed? I only have big-endian unix machines to test on here, so you have to tell me if it isn't working under some circumstances.

tor: Are you suggesting a different path for wintel, solaris, etc? If the patch is applied to mozilla/modules/libimg/src/color.cpp and requires an additional patch to gtk to work properly, won't platforms that don't use the gtk be broken? What am I missing here?

No, this patch should be applied to the global tree. The new version of this function will be functionally equivalent to the current version when running on the Win32 platform. The current version assumes Win32 colorspace shifts, while the fixed version will obtain the values at runtime. The gtk patch is needed to remove a function which converts the Win32 format image it currently receives into an X11 byte ordering. As for the mac platform, I'm guessing that it doesn't use this function. Is there anyone at netscape who could confirm or deny this?

This problem needs to be attacked another way. Putting some additional smarts in the imglib so it knows if we need RGB or BGR so it can get it right the first time seems right to me. The current fix is fine for big endian machines but will slow down everyone else and/or give the wrong color shift. The code affecting this problem is scattered all over the place, and the imglib will be going through drastic changes. I'll readdress this problem after the dust has settled.

It appears that I haven't been making myself clear - the patch that I purpose does exactly what you say, providing imglib with the smarts to put the channels in the right order. If you look at the other Convert*To* functions in that file, you'll see that all of them are already doing this. RGBToRGB24 seems to be an exception for some reason. The patch I offer should be endian agnostic as far as I can tell by just eyeballing it - the machines I use for mozilla development here are all big endian. If you're worried about a possible performance problem, you could special case some frequent channel orderings (win32, mac, etc) and drop through to the code I suggest if it doesn't match one of those.

all well and fine except your patch doesn't work when tested on other machines. I don't have time to track it down now. If someone can do some testing, great. have at it.

What architecture/OS combination(s) did you see it fail on? I'm perfectly willing to accept that there might be some problems with the patch, but I need at least some hints as to what's happening so I can fix it.

sorry I have some fires to put out now. later.

I have seen it fail on NT. Ramiro had it fail on Linux/pc. Me pointing out all the changes you'd need to make on your patch on code that either is on platforms you can't test or will be drastically changing in the near future seems like an empty exercize for both of us. I appreciate you pointing out the basic problem. I opened this bug to make sure the issue is not forgotten. There are plenty of other problems to address in the current code base. Adopt another issue...

Setting all current Open/Normal to M4.

[QA Assigning to Self]

reassignin to myself. i can look into this problem some more...

I believe this is fixed, could someone please double check?

The current rgb code is working fine for me on solaris. Ill investigate more later. marking m6.

marking m9

ramiro: I'd like to look at this issue for all platforms. -pn

ok, here you go. thanks. reassign to pnunn@netscape.com

*** Bug 6648 has been marked as a duplicate of this bug. ***

Putting on [Perf] radar

Pushing out milestone. Other priorities have precedence. -pn

CC self.

Latering the bug.

Code-level issue; rubber-stamping as Verified/Later. Frankly, I'm not familiar with the issue in question, and am not sure of the implication of it being latered. sfraser/ramiro/et al, could you please add your comments --- or re-open if you have evidence that this is critical functionality? Thanks!

LATER is deprecated.

->default owner

-> dcone

Bulk moving P1-P5 un-milestoned bugs to future.

Neither this function nor this file exist anymore... Marking worksforme, I guess.