VTK's nightly testing regressed due to a mesa update that bisects to: commit 8ec6534b266549cdc2798e2523bf6753924f6cde Author: Iago Toral Quiroga <itoral@igalia.com> Date: Wed Oct 15 13:42:11 2014 +0200 mesa: Use _mesa_format_convert to implement texstore_rgba. Notice that _mesa_format_convert does not handle byte-swapping scenarios, GL_COLOR_INDEX or MESA_FORMAT_YCBCR(_REV), so these must be handled separately. Also, remove all the code that goes unused after using _mesa_format_convert. Reviewed-by: Jason Ekstrand <jason.ekstrand@intel.com> The call in question from VTK is: glTexImage2D( GL_TEXTURE_2D, 0, GL_RGBA8, imageMemorySize[0], imageMemorySize[1], 0, GL_RGBA, GL_UNSIGNED_SHORT, static_cast<unsigned short *>(image) ); Unfortunately I do not have a deep understanding of the relevant code in VTK or Mesa. I will attach example test outputs from before and after the change. Somehow the background color tints the foreground image. In texstore_rgba, the code path taken prior to this change was: } else if (_mesa_get_format_max_bits(dstFormat) <= 8 && !_mesa_is_format_signed(dstFormat)) { return store_ubyte_texture(ctx, dims, baseInternalFormat, dstFormat, dstRowStride, dstSlices, srcWidth, srcHeight, srcDepth, srcFormat, srcType, srcAddr, srcPacking); and later in _mesa_pack_ubyte_rgba_rect: if (srcRowStride == width * 4 * sizeof(GLubyte) && dstRowStride == _mesa_format_row_stride(format, width)) { /* do whole image at once */ _mesa_pack_ubyte_rgba_row(format, width * height, (const GLubyte (*)[4]) src, dst); } After the change, _mesa_format_convert is called but none of the coe paths invoking _mesa_pack_ubyte_rgba_row is taken. Instead the path using compute_src2dst_component_mapping is taken and the result is different.
Created attachment 113324 [details] VTK test output image before change
Created attachment 113325 [details] VTK test output image after change
Thanks for reporting this, I'll have a look.
(In reply to Brad King from comment #0) (...) > After the change, _mesa_format_convert is called but none of the coe paths > invoking _mesa_pack_ubyte_rgba_row is taken. Instead the path using > compute_src2dst_component_mapping is taken and the result is different. Brad, it would be super helpful for me to understand what is going on if you could run a gdb session and print: a) the values of the parameters passed to compute_src2dst_component_mapping _after_ it has been called (they all are 4-element arrays, the last one is an out parameter so that is why I need the values after the call has been done) b) The following input parameters for the call to _mesa_swizzle_and_convert right after: src_type, dst_type, dst_num_channels, src_num_channels, normalized, width. Could you do this?
(In reply to Iago Toral from comment #4) > (In reply to Brad King from comment #0) > (...) > > After the change, _mesa_format_convert is called but none of the coe paths > > invoking _mesa_pack_ubyte_rgba_row is taken. Instead the path using > > compute_src2dst_component_mapping is taken and the result is different. The kind of effect you get looks as if you were getting alpha values < 1.0 after the texture upload and then blending against the background. If the texture has a solid alpha channel that should not happen..., however, I wrote a small program to do this same kind of texture upload and it works fine for me, it only blends against the background if I pass alpha values < 65535 in the input, otherwise it won't blend, which is the expected behavior... so I am not sure about what is going on in your case. For reference, this is the output I get in a GDB session for that texture upload (on an Intel GPU): > Brad, it would be super helpful for me to understand what is going on if you > could run a gdb session and print: > > a) the values of the parameters passed to compute_src2dst_component_mapping > _after_ it has been called (they all are 4-element arrays, the last one is > an out parameter so that is why I need the values after the call has been > done) (gdb) p src2rgba $1 = "\000\001\002\003" (gdb) p rgba2dst $2 = "\000\001\002\003" (gdb) p rebase_swizzle $3 = (uint8_t *) 0x0 (gdb) p src2dst $4 = "\000\001\002\003" These are the expected values for my Intel hardware. > b) The following input parameters for the call to _mesa_swizzle_and_convert > right after: src_type, dst_type, dst_num_channels, src_num_channels, > normalized, width. (gdb) p src_type $5 = MESA_ARRAY_FORMAT_TYPE_USHORT (gdb) p src_num_channels $6 = 4 (gdb) p dst_type $7 = MESA_ARRAY_FORMAT_TYPE_UBYTE (gdb) p dst_num_channels $8 = 4 (gdb) p normalized $9 = true Again, these are the expected values. > Could you do this? And also, what GPU are you running on?
Thanks for taking a look! I've just run a debug session with commit 1e02f2ba checked out. > what GPU are you running on? I'm building software-only: ./autogen.sh --prefix="..." \ --disable-dri --disable-egl --disable-shared-glapi \ --enable-xlib-glx --enable-gallium-osmesa --with-gallium-drivers=swrast \ --enable-gallium-llvm=yes LLVM_CONFIG=/usr/bin/llvm-config-3.3 \ --enable-llvm-shared-libs --enable-debug > a) the values of the parameters passed to compute_src2dst_component_mapping > _after_ it has been called (they all are 4-element arrays, the last one is > an out parameter so that is why I need the values after the call has been > done) (gdb) p src2rgba $1 = "\000\001\002\003" (gdb) p rgba2dst $2 = "\000\001\002\003" (gdb) p rebase_swizzle $3 = (uint8_t *) 0x0 (gdb) p src2dst $4 = "\000\001\002\003" > b) The following input parameters for the call to _mesa_swizzle_and_convert > right after: src_type, dst_type, dst_num_channels, src_num_channels, > normalized, width. $5 = MESA_ARRAY_FORMAT_TYPE_USHORT (gdb) p src_num_channels $6 = 4 (gdb) p dst_type $7 = MESA_ARRAY_FORMAT_TYPE_UBYTE (gdb) p dst_num_channels $8 = 4 (gdb) p normalized $9 = true (gdb) p width $10 = 256
After testing with a binary provided by the reporter and comparing with the old code it looks like the problem comes from the fact that they are using transferOps for color scaling (which is effectively scaling 50% alpha values to 100%)... and we are not dealing with that in texstore_rgba. If we need to apply transferOps (we can call _mesa_texstore_needs_transfer_ops to check this) then we have to convert from the src format to RGBA_FLOAT, so we can call calling mesa_apply_rgba_transfer_ops(), then convert the result from RGBA_FLOAT to the dst format... I think we should do this after we have handled swaping scenarios and before the current call to _mesa_format_convert. Jason, if you agree with this I can write a patch.
(In reply to Iago Toral from comment #7) > After testing with a binary provided by the reporter and comparing with the > old code it looks like the problem comes from the fact that they are using > transferOps for color scaling (which is effectively scaling 50% alpha values > to 100%)... and we are not dealing with that in texstore_rgba. > > If we need to apply transferOps (we can call > _mesa_texstore_needs_transfer_ops to check this) then we have to convert > from the src format to RGBA_FLOAT, so we can call calling > mesa_apply_rgba_transfer_ops(), then convert the result from RGBA_FLOAT to > the dst format... > > I think we should do this after we have handled swaping scenarios and before > the current call to _mesa_format_convert. Sound good to me. I'm kind of surprised piglit didn't catch that.
(In reply to Jason Ekstrand from comment #8) > (In reply to Iago Toral from comment #7) > > After testing with a binary provided by the reporter and comparing with the > > old code it looks like the problem comes from the fact that they are using > > transferOps for color scaling (which is effectively scaling 50% alpha values > > to 100%)... and we are not dealing with that in texstore_rgba. > > > > If we need to apply transferOps (we can call > > _mesa_texstore_needs_transfer_ops to check this) then we have to convert > > from the src format to RGBA_FLOAT, so we can call calling > > mesa_apply_rgba_transfer_ops(), then convert the result from RGBA_FLOAT to > > the dst format... > > > > I think we should do this after we have handled swaping scenarios and before > > the current call to _mesa_format_convert. > > Sound good to me. I'm kind of surprised piglit didn't catch that. Yeah, me too. I'll try to write a pilgit test for this too.
Sent a patch for review to mesa-dev: http://lists.freedesktop.org/archives/mesa-dev/2015-February/076851.html
Also, patch that adds a piglit test for this to avoid new regressions here: http://lists.freedesktop.org/archives/piglit/2015-February/014490.html
(In reply to Iago Toral from comment #10) > Sent a patch for review to mesa-dev: > http://lists.freedesktop.org/archives/mesa-dev/2015-February/076851.html I've confirmed that fixes all the VTK test regressions. Thanks!
The patch referenced is present in mesa 10.5.0 onwards. Thanks for the report and confirmation Brad.
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.