Bug 89068 - glTexImage2D regression by texstore_rgba switch to _mesa_format_convert
Summary: glTexImage2D regression by texstore_rgba switch to _mesa_format_convert
Status: RESOLVED FIXED
Alias: None
Product: Mesa
Classification: Unclassified
Component: Mesa core (show other bugs)
Version: git
Hardware: Other All
: medium normal
Assignee: mesa-dev
QA Contact: mesa-dev
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-02-10 19:33 UTC by Brad King
Modified: 2015-07-06 12:49 UTC (History)
2 users (show)

See Also:
i915 platform:
i915 features:


Attachments
VTK test output image before change (19.13 KB, image/png)
2015-02-10 19:34 UTC, Brad King
Details
VTK test output image after change (14.23 KB, image/png)
2015-02-10 19:34 UTC, Brad King
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Brad King 2015-02-10 19:33:35 UTC
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.
Comment 1 Brad King 2015-02-10 19:34:24 UTC
Created attachment 113324 [details]
VTK test output image before change
Comment 2 Brad King 2015-02-10 19:34:48 UTC
Created attachment 113325 [details]
VTK test output image after change
Comment 3 Iago Toral 2015-02-11 07:41:31 UTC
Thanks for reporting this, I'll have a look.
Comment 4 Iago Toral 2015-02-11 07:52:28 UTC
(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?
Comment 5 Iago Toral 2015-02-11 09:19:01 UTC
(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?
Comment 6 Brad King 2015-02-11 14:03:53 UTC
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
Comment 7 Iago Toral 2015-02-12 10:08:37 UTC
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.
Comment 8 Jason Ekstrand 2015-02-12 14:17:02 UTC
(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.
Comment 9 Iago Toral 2015-02-13 07:03:47 UTC
(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.
Comment 10 Iago Toral 2015-02-13 09:25:34 UTC
Sent a patch for review to mesa-dev:
http://lists.freedesktop.org/archives/mesa-dev/2015-February/076851.html
Comment 11 Iago Toral 2015-02-13 13:10:42 UTC
Also, patch that adds a piglit test for this  to avoid new regressions here:
http://lists.freedesktop.org/archives/piglit/2015-February/014490.html
Comment 12 Brad King 2015-02-13 14:03:14 UTC
(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!
Comment 13 Emil Velikov 2015-07-06 12:49:53 UTC
The patch referenced is present in mesa 10.5.0 onwards.
Thanks for the report and confirmation Brad.


bug/show.html.tmpl processed on Mar 30, 2017 at 22:35:30.
(provided by the Example extension).