[Mesa-dev] [PATCH 03/14] mesa: Fix conditions to test signed, unsigned integer format

[Anuj Phogat]

On Thu, Jun 18, 2015 at 7:09 AM, Iago Toral <itoral at igalia.com> wrote:
> On Tue, 2015-06-16 at 11:15 -0700, Anuj Phogat wrote:
>> Signed-off-by: Anuj Phogat <anuj.phogat at gmail.com>
>> Cc: <mesa-stable at lists.freedesktop.org>
>> ---
>>  src/mesa/main/readpix.c | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/src/mesa/main/readpix.c b/src/mesa/main/readpix.c
>> index caa2648..a9416ef 100644
>> --- a/src/mesa/main/readpix.c
>> +++ b/src/mesa/main/readpix.c
>> @@ -160,10 +160,12 @@ _mesa_readpixels_needs_slow_path(const struct gl_context *ctx, GLenum format,
>>        srcType = _mesa_get_format_datatype(rb->Format);
>>
>>        if ((srcType == GL_INT &&
>> +           _mesa_is_enum_format_integer(format) &&
>>             (type == GL_UNSIGNED_INT ||
>>              type == GL_UNSIGNED_SHORT ||
>>              type == GL_UNSIGNED_BYTE)) ||
>>            (srcType == GL_UNSIGNED_INT &&
>> +           _mesa_is_enum_format_integer(format) &&
>>             (type == GL_INT ||
>>              type == GL_SHORT ||
>>              type == GL_BYTE))) {
>
> As far as I understand this code we are trying to see if we can use
> memcpy to directly copy the contents of the framebuffer to the
> destination buffer. In that case, as long as the src/dst types have
> different sign we can't just use memcpy, right? In fact it looks like we
> might need to expand the checks to include the cases where srcType is
> GL_(UNSIGNED_)SHORT and GL_(UNSIGNED_)BYTE as well.
>
srcType returned by _mesa_get_format_datatype() is one of:
GL_UNSIGNED_NORMALIZED
GL_SIGNED_NORMALIZED
GL_UNSIGNED_INT
GL_INT
GL_FLOAT
So, the suggested checks for srcType are not required.

> That said, I think this code is not necessary with the call to
> _mesa_format_matches_format_and_type that we do immediately after this,
> since that will check that the framebuffer format exactly matches the
> destination format anyway, which is a much tighter check than this. In
> fact, a quick piglit run without these checks does not seem to break any
> tests on i965. Gallium uses these two functions in a slightly different
> way in st_cb_readpixels.c though, so I wonder if their use case requires
> these checks to exist in this function anyway.
>
> Iago
>