From: Gareth H. <ga...@va...> - 2000-12-12 14:24:03
|
Here's a good one: I've been running glean on the r128 driver (everyone who's worked on glean deserves huge thanks!) and found that it was failing the alpha test. The test code is this: case ALPHA: if (state == ALWAYS_PASS) { glAlphaFunc(GL_GEQUAL, 0.0); glEnable(GL_ALPHA_TEST); } else if (state == ALWAYS_FAIL) { glAlphaFunc(GL_GREATER, 1.0); glEnable(GL_ALPHA_TEST); } else { glDisable(GL_ALPHA_TEST); } break; However, ctx->Color.AlphaRef never changes from zero. That is, glAlphaFunc(GL_GREATER, 1.0) results in a zero reference alpha which will thus update the fragment and cause the test to fail. I'm doing a commit now and will look into this after I'm done. -- Gareth |
From: Gareth H. <ga...@va...> - 2001-01-03 19:45:56
|
The ALWAYS_FAIL test of the color mask causes the DD_MULTIDRAW bit to be set in TriangleCaps (state.c:848). This causes a software fallback in most/all our drivers. However, the span functions get the white pixels from the test quad, and thus write them to the framebuffer. Several questions: 1) Why is DD_MULTIDRAW set? Perhaps I'm missing something... 2) If an all-GL_FALSE color mask should really hit a software path (and I'm hesitant to suggest it should), do we then have to apply the color mask test in the span functions? I'm confused as to why this is happening. It seems like a strange way to handle this case, especially with hardware that has a full RGBA8888 color mask. -- Gareth |
From: Keith W. <ke...@va...> - 2001-01-03 19:53:48
|
Gareth Hughes wrote: > > The ALWAYS_FAIL test of the color mask causes the DD_MULTIDRAW bit to be > set in TriangleCaps (state.c:848). This causes a software fallback in > most/all our drivers. > > However, the span functions get the white pixels from the test quad, and > thus write them to the framebuffer. Several questions: > > 1) Why is DD_MULTIDRAW set? Perhaps I'm missing something... > 2) If an all-GL_FALSE color mask should really hit a software path (and > I'm hesitant to suggest it should), do we then have to apply the color > mask test in the span functions? > > I'm confused as to why this is happening. It seems like a strange way > to handle this case, especially with hardware that has a full RGBA8888 > color mask. DD_MULTIDRAW is a wierd concept that's hard to pin down. I removed it in 3.5, and removed the usage of it in the mga driver. You would think it caught situations where you are expected to render to more than one color buffer, but it actually gets turned on more often than that. My suggestion is to remove references to it, and use the callbacks (DrawBuffer, ReadBuffer) to figure out if a fallback is required. Keith |
From: Gareth H. <ga...@va...> - 2001-01-04 10:59:04
|
Keith Whitwell wrote: > > DD_MULTIDRAW is a wierd concept that's hard to pin down. I removed it in 3.5, > and removed the usage of it in the mga driver. You would think it caught > situations where you are expected to render to more than one color buffer, but > it actually gets turned on more often than that. > > My suggestion is to remove references to it, and use the callbacks > (DrawBuffer, ReadBuffer) to figure out if a fallback is required. We were already doing that, and so removing DD_MULTIDRAW from ALL_FALLBACK in _tris.c fixes the glean paths test. Thanks for the tip. -- Gareth |
From: Brian P. <br...@va...> - 2000-12-12 14:55:37
|
Gareth Hughes wrote: > > Here's a good one: > > I've been running glean on the r128 driver (everyone who's worked on > glean deserves huge thanks!) and found that it was failing the alpha > test. The test code is this: > > case ALPHA: > if (state == ALWAYS_PASS) { > glAlphaFunc(GL_GEQUAL, 0.0); > glEnable(GL_ALPHA_TEST); > } > else if (state == ALWAYS_FAIL) { > glAlphaFunc(GL_GREATER, 1.0); > glEnable(GL_ALPHA_TEST); > } > else { > glDisable(GL_ALPHA_TEST); > } > break; > > However, ctx->Color.AlphaRef never changes from zero. That is, > glAlphaFunc(GL_GREATER, 1.0) results in a zero reference alpha which > will thus update the fragment and cause the test to fail. > > I'm doing a commit now and will look into this after I'm done. Hmmm, very strange. I'm anxious to learn what the problem is. -Brian |
From: Gareth H. <ga...@va...> - 2000-12-12 15:42:02
|
Brian Paul wrote: > > Hmmm, very strange. I'm anxious to learn what the problem is. Okay, a misleading error message had me confused. It's failing on the first test, which is an always-pass one (the message seemed to indicate it was the always-fail case). Thus, the reference alpha is as expected. It's getting pixel values around 0.97-0.99 or so, and is looking for 1.00. Have to chase this down. Sorry for the confusion... -- Gareth |
From: Brian P. <br...@va...> - 2000-12-12 16:32:20
|
Gareth Hughes wrote: > > Brian Paul wrote: > > > > Hmmm, very strange. I'm anxious to learn what the problem is. > > Okay, a misleading error message had me confused. It's failing on the > first test, which is an always-pass one (the message seemed to indicate > it was the always-fail case). Thus, the reference alpha is as expected. > > It's getting pixel values around 0.97-0.99 or so, and is looking for > 1.00. Have to chase this down. I'll repeat the reply I sent to Glean-dev here for the benefit of the other DRI developers: Gareth Hughes wrote: > > The paths test is failing at 16bpp due to rounding errors. An RGB565 > pixel of 0xffff is being converted to approximately (0.97, 0.98, 0.97), > which doesn't equal (1.0, 1.0, 1.0) and thus fails this test. > > Brian, what would you like to do here? Perhaps test for a pixel value > greater than some threshold? No, I think this is a genuine driver bug. I purposely wrote the test so that only 100% white is sent through OpenGL. 100% white should be in the framebuffer, and 100% white is what you should get from glReadPixels. In drv/r128/r128_span.c the 16bpp pixel read macro is: #define READ_RGBA( rgba, _x, _y ) \ do { \ GLushort p = *(GLushort *)(read_buf + _x*2 + _y*pitch); \ rgba[0] = (p >> 8) & 0xf8; \ rgba[1] = (p >> 3) & 0xfc; \ rgba[2] = (p << 3) & 0xf8; \ rgba[3] = 0xff; \ } while (0) If the pixel value is 0xffff you're going to return red=0xf8, green=0xfc, blue=0xf8. It should be red=green=blue=0xff. This was wrong in the tdfx driver for a while too. The simple, but slow solution is this (from the tdfx driver): #define READ_RGBA( rgba, _x, _y ) \ do { \ GLushort p = *(GLushort *)(read_buf + _x*2 + _y*pitch); \ rgba[0] = (((p >> 11) & 0x1f) * 255) / 31; \ rgba[1] = (((p >> 5) & 0x3f) * 255) / 63; \ rgba[2] = (((p >> 0) & 0x1f) * 255) / 31; \ rgba[3] = 0xff; \ } while (0) The integer multiply and divide could be replaced by a table lookup or a bit-twidling solution. -Brian |
From: Stephen C. T. <sc...@re...> - 2000-12-12 17:39:01
|
Hi, On Tue, Dec 12, 2000 at 09:35:50AM -0700, Brian Paul wrote: > > #define READ_RGBA( rgba, _x, _y ) \ > do { \ > GLushort p = *(GLushort *)(read_buf + _x*2 + _y*pitch); \ > rgba[0] = (((p >> 11) & 0x1f) * 255) / 31; \ > rgba[1] = (((p >> 5) & 0x3f) * 255) / 63; \ > rgba[2] = (((p >> 0) & 0x1f) * 255) / 31; \ > rgba[3] = 0xff; \ > } while (0) > > The integer multiply and divide could be replaced by a table lookup > or a bit-twidling solution. Integer multiply is fast on modern CPUs, and current gccs will replace all integer divide-by-constant operations with an equivalent using shifts plus a multiply (thanks to the wonders of integer modulo arithmetic). Cheers, Stephen |
From: Nathan H. <na...@ma...> - 2000-12-12 20:09:18
|
On Tue, Dec 12, 2000 at 09:35:50AM -0700, Brian Paul wrote: > > If the pixel value is 0xffff you're going to return red=0xf8, green=0xfc, > blue=0xf8. It should be red=green=blue=0xff. > > This was wrong in the tdfx driver for a while too. The simple, but > slow solution is this (from the tdfx driver): > > #define READ_RGBA( rgba, _x, _y ) \ > do { \ > GLushort p = *(GLushort *)(read_buf + _x*2 + _y*pitch); \ > rgba[0] = (((p >> 11) & 0x1f) * 255) / 31; \ > rgba[1] = (((p >> 5) & 0x3f) * 255) / 63; \ > rgba[2] = (((p >> 0) & 0x1f) * 255) / 31; \ > rgba[3] = 0xff; \ > } while (0) > > The integer multiply and divide could be replaced by a table lookup > or a bit-twidling solution. The bit twiddling solution is fairly easy. x * 255 / 31 = (x * 256 - x) / 31 x / 31 = x * ((1 << 16) / 31) + 32) >> 16) therefore x * 255 / 31 = (x * 256 - x) * ((1 << 16) / 31) + 32) >> 16) which is 1 shift, 1 subtract, 1 multiply This is just off the top of my head. I'll have breakfast and write a test jig to prove it's correct for x in [0..255]. |
From: Sam L. <her...@lo...> - 2000-12-12 22:11:26
|
Brian Paul wrote: > > Gareth Hughes wrote: > > > > Brian Paul wrote: > > > > > > Hmmm, very strange. I'm anxious to learn what the problem is. > > > > Okay, a misleading error message had me confused. It's failing on the > > first test, which is an always-pass one (the message seemed to indicate > > it was the always-fail case). Thus, the reference alpha is as expected. > > > > It's getting pixel values around 0.97-0.99 or so, and is looking for > > 1.00. Have to chase this down. > > I'll repeat the reply I sent to Glean-dev here for the benefit of > the other DRI developers: > > Gareth Hughes wrote: > > > > The paths test is failing at 16bpp due to rounding errors. An RGB565 > > pixel of 0xffff is being converted to approximately (0.97, 0.98, 0.97), > > which doesn't equal (1.0, 1.0, 1.0) and thus fails this test. > > > > Brian, what would you like to do here? Perhaps test for a pixel value > > greater than some threshold? > > No, I think this is a genuine driver bug. > > I purposely wrote the test so that only 100% white is sent through > OpenGL. 100% white should be in the framebuffer, and 100% white is > what you should get from glReadPixels. > > In drv/r128/r128_span.c the 16bpp pixel read macro is: > > #define READ_RGBA( rgba, _x, _y ) \ > do { \ > GLushort p = *(GLushort *)(read_buf + _x*2 + _y*pitch); \ > rgba[0] = (p >> 8) & 0xf8; \ > rgba[1] = (p >> 3) & 0xfc; \ > rgba[2] = (p << 3) & 0xf8; \ > rgba[3] = 0xff; \ > } while (0) > > If the pixel value is 0xffff you're going to return red=0xf8, green=0xfc, > blue=0xf8. It should be red=green=blue=0xff. > > This was wrong in the tdfx driver for a while too. The simple, but > slow solution is this (from the tdfx driver): > > #define READ_RGBA( rgba, _x, _y ) \ > do { \ > GLushort p = *(GLushort *)(read_buf + _x*2 + _y*pitch); \ > rgba[0] = (((p >> 11) & 0x1f) * 255) / 31; \ > rgba[1] = (((p >> 5) & 0x3f) * 255) / 63; \ > rgba[2] = (((p >> 0) & 0x1f) * 255) / 31; \ > rgba[3] = 0xff; \ > } while (0) > > The integer multiply and divide could be replaced by a table lookup > or a bit-twidling solution. We had the same problem in the SDL code. Here's the fix we used: /* * This makes sure that the result is mapped to the * interval [0..255], and the maximum value for each * component is 255. This is important to make sure * that white is indeed reported as (255, 255, 255), * and that opaque alpha is 255. * This only works for RGB bit fields at least 4 bit * wide, which is almost always the case. */ unsigned rv, gv, bv, av; rv = (pixel & fmt->Rmask) >> fmt->Rshift; *r = (rv << fmt->Rloss) + (rv >> (8 - fmt->Rloss)); gv = (pixel & fmt->Gmask) >> fmt->Gshift; *g = (gv << fmt->Gloss) + (gv >> (8 - fmt->Gloss)); bv = (pixel & fmt->Bmask) >> fmt->Bshift; *b = (bv << fmt->Bloss) + (bv >> (8 - fmt->Bloss)); if(fmt->Amask) { av = (pixel & fmt->Amask) >> fmt->Ashift; *a = (av << fmt->Aloss) + (av >> (8 - fmt->Aloss)); } else *a = SDL_ALPHA_OPAQUE; In this case: Rmask = 0x0000f800; Rshift = 11; Rloss = 3; Gmask = 0x000007e0; Gshift = 5; Gloss = 2; Bmask = 0x0000001f; Bshift = 0; Bloss = 3; It can be optimized quite a bit for this particular case. See ya! -- -Sam Lantinga, Lead Programmer, Loki Software, Inc. |
From: Keith W. <ke...@va...> - 2000-12-12 17:18:47
|
Gareth Hughes wrote: > > Brian Paul wrote: > > > > Hmmm, very strange. I'm anxious to learn what the problem is. > > Okay, a misleading error message had me confused. It's failing on the > first test, which is an always-pass one (the message seemed to indicate > it was the always-fail case). Thus, the reference alpha is as expected. > > It's getting pixel values around 0.97-0.99 or so, and is looking for > 1.00. Have to chase this down. > > Sorry for the confusion... One thing to look for is use of the IEEE_ONE macro to assign to floats. I just found this in 3.5 -- that macro actually defines IEEE_NEARLY_ONE, or something like .996 -- perfect for its orignal use in converting float colors to ubytes, but we've maybe used it some other places as well? Keith |