[Mesa-dev] [PATCH 02/14] meta: Fix transfer operations check in meta pbo path for readpixels

[Anuj Phogat]

On Fri, Jun 19, 2015 at 4:48 PM, Anuj Phogat <anuj.phogat at gmail.com> wrote:
> On Thu, Jun 18, 2015 at 5:26 AM, Iago Toral <itoral at igalia.com> wrote:
>> On Tue, 2015-06-16 at 11:15 -0700, Anuj Phogat wrote:
>>> Without this patch, arb_color_buffer_float-readpixels test fails, when
>>> forced to use meta pbo path.
>>>
>>> Signed-off-by: Anuj Phogat <anuj.phogat at gmail.com>
>>> Cc: <mesa-stable at lists.freedesktop.org>
>>> ---
>>>  src/mesa/drivers/common/meta_tex_subimage.c | 10 ++++++----
>>>  1 file changed, 6 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/src/mesa/drivers/common/meta_tex_subimage.c b/src/mesa/drivers/common/meta_tex_subimage.c
>>> index d2474f5..00364f8 100644
>>> --- a/src/mesa/drivers/common/meta_tex_subimage.c
>>> +++ b/src/mesa/drivers/common/meta_tex_subimage.c
>>> @@ -273,12 +273,14 @@ _mesa_meta_pbo_GetTexSubImage(struct gl_context *ctx, GLuint dims,
>>>         format == GL_COLOR_INDEX)
>>>        return false;
>>>
>>> -   if (ctx->_ImageTransferState)
>>> -      return false;
>>> -
>>> -
>>
>> That test uses glReadPixels so it should call this with tex_image set to
>> NULL and it should flow through the if you have below. The call to
>> _mesa_get_readpixels_transfer_ops that you add below looks like it does
>> what we want for a pixel read from a framebuffer instead of simply
>> checking ctx->_ImageTransferState directly. I suppose this is what fixes
>> the test, right?
>>
> Right. Using _mesa_get_readpixels_transfer_ops() in place of simple
> ctx->_ImageTransferState check takes care of setting IMAGE_CLAMP_BIT
> in  ctx->_ImageTransferState when GL_CLAMP_READ_COLOR is set.
> I'll add some explanation in commit message.
>
>> The patch also removes the ctx->_ImageTransferState check for the case
>> where we are reading from a real texture (tex_image != NULL), that seems
>> unrelated to fixing arb_color_buffer_float-readpixels... Looking at the
>> texture read code from getteximage.c it seems like this should be fine
>> since that file does not seem to use that field for anything either, so
>> I guess the check might not valid in this case.
>>
> After another look I realized that glGetTexImage() do need transfer
> operations check to fall back to software path. It's the lack of piglit
> tests that I couldn't catch it. Thanks for noticing it. I'll send out a v2.
>
Sent out v2 with a comment in the code and additional details as
commit message.

For glGetTexImage I verified that software path doesn't use
ctx->_ImageTransferState.

>> I think it would be nice if you updated the changelog to explain these
>> things.
>>
>> Iago
>>
>>> +   /* Don't use meta path for readpixels in below conditions. */
>>>     if (!tex_image) {
>>>        rb = ctx->ReadBuffer->_ColorReadBuffer;
>>> +
>>> +      if (_mesa_get_readpixels_transfer_ops(ctx, rb->Format, format,
>>> +                                            type, GL_FALSE))
>>> +         return false;
>>> +
>>>        if (_mesa_need_rgb_to_luminance_conversion(rb->Format, format))
>>>           return false;
>>>     }
>>
>>