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

[Iago Toral]

On Mon, 2015-06-22 at 12:35 -0700, Anuj Phogat wrote:
> On Sun, Jun 21, 2015 at 11:25 PM, Iago Toral <itoral at igalia.com> wrote:
> > 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.
> >
> Thanks for explaining the issue. With much tighter checks available in
> _mesa_format_matches_format_and_type(), I agree that this code can
> be deleted. I got no piglit failures when this code is deleted and the driver
> is forced to use _mesa_readpixels().
> 
> >> > 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.
> >
> After deleting the redundant checks in _mesa_readpixels_needs_slow_path(),
> _mesa_need_signed_unsigned_int_conversion() need not to be global.
> I'll make it a local function and squash with my meta PBO changes.
> 
> Do you want to send out the patch deleting the checks or I'll send it out later?

I can take care of that, the function is also called from one place in
gallium with a slightly different usage pattern, so I want to make sure
that these checks are not relevant to the gallium path either. I'll do
some tests and send the patch if I don't see any problems.

Iago

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