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

[Iago Toral]

On Fri, 2015-06-19 at 13:32 -0700, Anuj Phogat wrote:
> 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?

I have no concerns about the _mesa_need_signed_unsigned_int_conversion
function that you add in a later patch for your PBO work, my concern is
related to the fact that you are assuming that the checks that you need
in the PBO path are the same that we have in
_mesa_readpixels_needs_slow_path, so you make both the same when I think
they are trying to address different things.

In your PBO code, you can't handle signed/unsigned integer conversions,
so you need to detect that and fall back to another path. That should be
fine I guess and the function _mesa_need_signed_unsigned_int_conversion
does what you need, so no problems there.

However, in _mesa_readpixels_needs_slow_path I think we don't want to
just do integer checking. The purpose of the function is to tell whether
we can use memcpy to copy pixels from the framebuffer to the dst, and if
we have types with different signs, *whether they are integer or not*,
we can't, so limiting the check only to integer types does not look
right to me. The key aspect here is that what this function needs to
check is not specific to integer types, even if the current code only
seems to check things when the framebuffer has an integer format.

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

Right, that was not a good example, but forget about integer types, what
if the framebuffer is something like MESA_FORMAT_R8G8B8A8_UNORM and our
dst format/type is GL_RGBA/GL_BYTE? These are not integer types but we
can't memcpy anyway because the framebuffer is unsigned and the dst is
signed so a conversion is needed.

Of course, the current code in this function only cares about the
framebuffer being an integer format, but for the reasons I explain
above, I think that is wrong in this case, I think this code should be
either eliminated (and rely in the much tighter checks in
_mesa_format_matches_format_and_type) or expanded to not be specific to
integer formats, and your patch to this function conflicts with both
scenarios.

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

Yes, hopefully I explained them more clearly this time around, but
notice that the concerns I have with this patch do not affect your PBO
changes, you can still have _mesa_need_signed_unsigned_int_conversion
and you can still use that function from your PBO path, my concerns are
solely with your changes to _mesa_readpixels_needs_slow_path.

Iago

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