| Summary: | glDrawPixels with GL_COLOR_INDEX never returns. | ||
|---|---|---|---|
| Product: | Mesa | Reporter: | Juha-Pekka Heikkilä <juhapekka.heikkila> |
| Component: | Mesa core | Assignee: | mesa-dev |
| Status: | RESOLVED FIXED | QA Contact: | mesa-dev |
| Severity: | normal | ||
| Priority: | medium | CC: | imirkin, itoral, jason |
| Version: | git | ||
| Hardware: | Other | ||
| OS: | All | ||
| Whiteboard: | |||
| i915 platform: | i915 features: | ||
|
Description
Juha-Pekka Heikkilä
2015-04-28 11:10:03 UTC
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! |
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.