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

[Iago Toral]

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.

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.

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.

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