[Mesa-dev] [Bug 84566] Unify the format conversion code

[bugzilla-daemon at freedesktop.org]

https://bugs.freedesktop.org/show_bug.cgi?id=84566

--- Comment #6 from Jason Ekstrand <jason at jlekstrand.net> ---
(In reply to Iago Toral from comment #5)
> Jason, in your initial implementation of the master function you have this
> assert:
> 
> if (src_array_format.as_uint && dst_array_format.as_uint) {
>    assert(src_array_format.normalized == dst_array_format.normalized);
>    (...)
>    _mesa_swizzle_and_convert(dst, dst_gl_type, dst_array_format.num_channels,
>                              src, src_gl_type, src_array_format.num_channels,
>                              src2dst, normalized, width);
> }
> 
> I think this assertion is wrong, since it prevents conversion from float
> array types (which are being generated with normalized=false) to normalized
> integer types. Something that is explicitly supported in
> _mesa_swizzle_and_convert. I am hitting this assertion while converting from
> RGB FLOAT to RGBA UBYTE for example.
> 
> Looking at the implementation of texstore_swizzle, which uses
> _mesa_swizzle_and_convert (and is the path that the original code takes to
> do this conversion in my example), I don't find this kind of checks. What it
> does is this:
> 
> is_array = _mesa_format_to_array(dstFormat, &dst_type, &dst_components,
>                                  rgba2dst, &normalized);
> (...)
> normalized |= !_mesa_is_enum_format_integer(srcFormat);
> 
> This will make it so that normalized is always set to true for float src
> formats like RGB FLOAT. 
> 
> So assuming that this code is correct I think we would want to do the same
> in the master function: remove the assert and set normalized to true if
> either src or dst are normalized or if src is a float type.

Yes, I believe that is correct.  While I implemented the master function, it
didn't get much testing.  I'ts mostly just a prototype, so bugs like that
aren't surprising.

> There is another issue, and that is how we decide if a (glformat,gldatatype)
> combination is normalized or not when creating an array format from it.
> There are a bunch of functions in glformats.c I could use, like:
> 
> GLboolean _mesa_is_enum_format_unorm(GLenum format)
> GLboolean _mesa_is_enum_format_snorm(GLenum format)
> GLboolean _mesa_is_enum_format_integer(GLenum format)
> ...
> 
> But these do not consider the data type, only the format, so for example
> _mesa_is_enum_format_unorm(GL_RGB) will always return true, even if we have
> a GL_FLOAT data type. This is the same thing that _mesa_swizzle_and_convert
> does when doing:
> 
> normalized |= !_mesa_is_enum_format_integer(srcFormat);

This should be correct.  The way you detect things on the GL side is usually
that the GL_RGBA type formats are normalized while the GL_RGBA_INTEGER type
formats are not.

The only issue here is when you should clamp floating-point.  What we probably
want is a boolean "clamp" parameter that indicates that the input should be
clamped to [0, 1] before converting.  Whether a float-float conversion should
be clampped depends on where the conversion is being done.

> but it is inconsistent with your definition of array formats, since all
> array formats generated in format_info.c have a normalized setting of 0.

The generation is quite possibly wrong there.  Again, the branches I pushed
were very thrown together and not a full, tested, implementation.

> I could also avoid the problem if I simply use I just use
> _mesa_is_enum_format_integer(format) when mapping a (glformat,gldatatype) to
> a mesa_array_format, like texstore_swizzle does, but as I mentioned, that
> will create float array formats with normalized set to true, which is
> inconsistent with the array_formats you are generating, so I am not sure
> about the best path to take here.

I think that's perfectly reasonable if we fix the generation format_info code.

-- 
You are receiving this mail because:
You are the QA Contact for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20141008/2743a52b/attachment.html>