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

[Anuj Phogat]

On Thu, Jun 18, 2015 at 11:41 PM, Iago Toral <itoral at igalia.com> wrote:
> On Thu, 2015-06-18 at 09:19 -0700, Anuj Phogat wrote:
>> 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.
>
> Oh, right, although I think that does not invalidate my point: can we
> memcpy from a GL_UNSIGNED_NORMALIZED to a format with type GL_FLOAT or
> GL_SIGNED_NORMALIZED? It does not look like these checks here are
> thorough.
>
Helper function _mesa_need_signed_unsigned_int_conversion() is
meant to do the checks only for integer formats. May be add another
function to do the missing checks for other formats?

> In any case, that's beyond the point of your patch. Talking specifically
> about your patch: can we memcpy, for example, from a _signed_ integer
> format like MESA_FORMAT_R_SINT8 to an _unsigned_ format (integer or
> not)? I don't think we can, in which case your patch would not look
> correct to me.
>
Reading integer format to a non integer format is not allowed in
glReadPixels. That's why those cases are not relevant here and
we just check for integer formats. From ext_texture_integer:
"INVALID_OPERATON is generated by ReadPixels if <format> is
an integer format and the color buffer is not an integer format, or
 if <format> is not an integer format and the color buffer is an
 integer format."

> Also, as I said in my previous e-mail, I feel like these checks here do
> not add anything, at least when this function is called from
> readpixels_can_use_memcpy, since even if we return true here, we will
> later call _mesa_format_matches_format_and_type and that would check
> that the formats match anyway before going through the memcpy path. Even
> the other use of this function in Gallium would call
> _mesa_format_matches_format_and_type before it calls this... That's why
> I think we probably want to remove these checks from this function and
> rely on _mesa_format_matches_format_and_type exclusively to check
> allowed formats and types.
>
It does seem like calling _mesa_need_signed_unsigned_int_conversion()
is redundant in case of readpixels_can_use_memcpy(). Thanks for noticing
it. Feel free to submit a patch to remove the redundant check.

But,  I still need _mesa_need_signed_unsigned_int_conversion() and changes
in this patch for one of my later patches fixing meta PBO path for glReadPixels.
Do you still have any concerns about this patch?

>> > 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
>> >
>>
>
>