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

[Anuj Phogat]

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?

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