When I run glDrawPixels(width, height, GL_COLOR_INDEX, GL_UNSIGNED_BYTE, data) code execution never return. Commit 84eb402c01962e3ff4b70c9948c85a61ed81678f changed the path so we end up in _mesa_format_from_format_and_type at src/mesa/main/glformats.c:2682 which does not seem to know anything about GL_COLOR_INDEX. On release build this function should return a value but doesn't and gets confused, on debug build I get assert _mesa_format_from_format_and_type: Assertion `!"Unsupported format"' failed. I made minimalistic test for this, one can get it from here https://github.com/juhapekka/CI_test.git There seems also exist test in Piglit which does similar and halts Piglit run. Piglit test is gl-1.0-drawpixels-color-index
This test asserted from the time it was written. The test failure does not necessarily indicate a regression. The test was introduced in piglit 2e7be059b1dc71eef0c09964ea393678618d898b, and references the "format overhaul". Also, Message-ID: <5525834F.4020504@vmware.com> on the piglit list seems to point at waffle, at least for nv. JP's application seems to rule that out, though.
Hi Mark, Juha: Some "special" formats, like GL_COLOR_INDEX, MESA_FORMAT_YCBCR or byte-swapped scenarios need special handling before we can call _mesa_format_convert (see the implementation of texstore_rgba() in texstore.c for example). The problem in this case is that we are not doing that and it seems that the old _mesa_unpack_color_span_float that was used here before the format conversion overhaul did handle at least GL_COLOR_INDEX (and byte-swapped scenarios too as far as I can see), so this is a regression. If the test was asserting before the overhaul that would explain that we did not catch it when we where in development, but it is a regression nonetheless. The solution is to add the same code we have in texstore.c to handle GL_COLOR_INDEX, which basically converts GL_COLOR_INDEX to GL_RGBA ubyte before calling _mesa_format_convert. We should probably also add the code we have there for byte-swapped formats. I think it could be useful to have a helper function for this so we don't have to replicate the code wherever we need to handle things like GL_COLOR_INDEX. Also, we should probably return something in _mesa_format_from_format_and_type even when we reach the assertion to make things less confusing in non-debug builds. Jason, what do you think?
(In reply to Iago Toral from comment #2) > Hi Mark, Juha: > > Some "special" formats, like GL_COLOR_INDEX, MESA_FORMAT_YCBCR or > byte-swapped scenarios need special handling before we can call > _mesa_format_convert (see the implementation of texstore_rgba() in > texstore.c for example). > > The problem in this case is that we are not doing that and it seems that the > old _mesa_unpack_color_span_float that was used here before the format > conversion overhaul did handle at least GL_COLOR_INDEX (and byte-swapped > scenarios too as far as I can see), so this is a regression. > > If the test was asserting before the overhaul that would explain that we did > not catch it when we where in development, but it is a regression > nonetheless. > > The solution is to add the same code we have in texstore.c to handle > GL_COLOR_INDEX, which basically converts GL_COLOR_INDEX to GL_RGBA ubyte > before calling _mesa_format_convert. We should probably also add the code we > have there for byte-swapped formats. I think it could be useful to have a > helper function for this so we don't have to replicate the code wherever we > need to handle things like GL_COLOR_INDEX. > > Also, we should probably return something in > _mesa_format_from_format_and_type even when we reach the assertion to make > things less confusing in non-debug builds. > > Jason, what do you think? Seems reasonable to me
(In reply to Jason Ekstrand from comment #3) > (In reply to Iago Toral from comment #2) > > Hi Mark, Juha: > > > > Some "special" formats, like GL_COLOR_INDEX, MESA_FORMAT_YCBCR or > > byte-swapped scenarios need special handling before we can call > > _mesa_format_convert (see the implementation of texstore_rgba() in > > texstore.c for example). > > > > The problem in this case is that we are not doing that and it seems that the > > old _mesa_unpack_color_span_float that was used here before the format > > conversion overhaul did handle at least GL_COLOR_INDEX (and byte-swapped > > scenarios too as far as I can see), so this is a regression. > > > > If the test was asserting before the overhaul that would explain that we did > > not catch it when we where in development, but it is a regression > > nonetheless. > > > > The solution is to add the same code we have in texstore.c to handle > > GL_COLOR_INDEX, which basically converts GL_COLOR_INDEX to GL_RGBA ubyte > > before calling _mesa_format_convert. We should probably also add the code we > > have there for byte-swapped formats. I think it could be useful to have a > > helper function for this so we don't have to replicate the code wherever we > > need to handle things like GL_COLOR_INDEX. > > > > Also, we should probably return something in > > _mesa_format_from_format_and_type even when we reach the assertion to make > > things less confusing in non-debug builds. > > > > Jason, what do you think? > > Seems reasonable to me Great. Juha, do you have plans to work on this? If not, I can give it a go next week.
Sent a couple of patches to the mailing list for review: http://lists.freedesktop.org/archives/mesa-dev/2015-May/083334.html http://lists.freedesktop.org/archives/mesa-dev/2015-May/083335.html The first one fixes the attached test program and also this piglit test: bin/gl-1.0-drawpixels-color-index -auto -fbo
Iago's patch fixed the issue, thanks!
*** Bug 89697 has been marked as a duplicate of this bug. ***
Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.