[Mesa-dev] [PATCH v3] meta/GetTexSubImage: Account for GL_PACK_SKIP_IMAGES on compressed textures

[Eduardo Lima Mitev]

On 11/15/2016 11:03 AM, Nicolai Hähnle wrote:
> On 15.11.2016 09:27, Eduardo Lima Mitev wrote:
>> On 11/15/2016 12:25 AM, Kenneth Graunke wrote:
>>> From: Eduardo Lima Mitev <elima at igalia.com>
>>>
>>> This option was being ignored when packing compressed 3D and cube
>>> textures.
>>>
>>> Fixes CTS test (on gen8+):
>>> * GL45-CTS.gtf32.GL3Tests.packed_pixels.packed_pixels_pixelstore
>>>
>>> v2: Drop API checks.
>>> v3 (Ken): Just apply the existing code in more cases.
>>> ---
>>>  src/mesa/drivers/common/meta.c | 20 +++++++++++++++++---
>>>  1 file changed, 17 insertions(+), 3 deletions(-)
>>>
>>> Hey Eduardo,
>>>
>>> It looks like the existing code already tries to handle SkipImages - but
>>> we weren't applying it for 3D and cubemap textures.  I found a spec
>>> quote
>>> in the narrative for GetTexImage that indicates we need to do it for
>>> those
>>> as well.
>>>
>>> What do you think of this version?  I preserved your authorship as I
>>> wanted
>>> you to get the credit for this bugfix - I just typed it up so that I
>>> could
>>> make sure it actually fixed the test, and since I had it typed up, I
>>> figured
>>> I'd send it out to save you some time...
>>>
>>
>> Hi Kenneth,
>>
>> Great, this is the correct solution. I tried to make my original patch
>> work with _mesa_image_address3d() for all targets, as yours, but I
>> missed resetting SkipPixels and SkipRows for TEXTURE_3D, so it was
>> regressing some subcases.
> 
> For what it's worth, there's also _mesa_image_address which takes a
> dimensions parameter and automatically applies / doesn't apply
> SkipImages. But since you do manual fixups of SkipPixels/SkipRows
> anyway, it might not be the right thing here.
>

Yes, we could use _mesa_image_address() directly, but I think it is more
legible to call mesa_image_address3d(), because it is consistent with
the purpose of this code-path and the comment in it, being for
dimensions >2.

Thanks,
Eduardo

> Cheers,
> Nicolai
> 
>>
>> Patch is:
>>
>> Reviewed-by: Eduardo Lima Mitev <elima at igalia.com>
>>
>> I don't mind you taking authorship. Knowing the correct solution is way
>> more useful to me :). If you want, take the authorship, put my R-b and
>> push; otherwise let me know and I'll put your R-b and push it myself.
>>
>> Thanks!
>>
>> Eduardo
>>
>>>  --Ken
>>>
>>> diff --git a/src/mesa/drivers/common/meta.c
>>> b/src/mesa/drivers/common/meta.c
>>> index 5ab1e6c..99c85cf 100644
>>> --- a/src/mesa/drivers/common/meta.c
>>> +++ b/src/mesa/drivers/common/meta.c
>>> @@ -3243,8 +3243,20 @@ _mesa_meta_GetTexSubImage(struct gl_context *ctx,
>>>
>>>        for (slice = 0; slice < depth; slice++) {
>>>           void *dst;
>>> -         if (texImage->TexObject->Target == GL_TEXTURE_2D_ARRAY
>>> -             || texImage->TexObject->Target ==
>>> GL_TEXTURE_CUBE_MAP_ARRAY) {
>>> +         /* Section 8.11.4 (Texture Image Queries) of the GL 4.5 spec
>>> says:
>>> +          *
>>> +          *    "For three-dimensional, two-dimensional array, cube
>>> map array,
>>> +          *     and cube map textures pixel storage operations are
>>> applied as
>>> +          *     if the image were two-dimensional, except that the
>>> additional
>>> +          *     pixel storage state values PACK_IMAGE_HEIGHT and
>>> +          *     PACK_SKIP_IMAGES are applied. The correspondence of
>>> texels to
>>> +          *     memory locations is as defined for TexImage3D in
>>> section 8.5."
>>> +          */
>>> +         switch (texImage->TexObject->Target) {
>>> +         case GL_TEXTURE_3D:
>>> +         case GL_TEXTURE_2D_ARRAY:
>>> +         case GL_TEXTURE_CUBE_MAP:
>>> +         case GL_TEXTURE_CUBE_MAP_ARRAY: {
>>>              /* Setup pixel packing.  SkipPixels and SkipRows will be
>>> applied
>>>               * in the decompress_texture_image() function's call to
>>>               * glReadPixels but we need to compute the dest slice's
>>> address
>>> @@ -3255,9 +3267,11 @@ _mesa_meta_GetTexSubImage(struct gl_context *ctx,
>>>              packing.SkipRows = 0;
>>>              dst = _mesa_image_address3d(&packing, pixels, width,
>>> height,
>>>                                          format, type, slice, 0, 0);
>>> +            break;
>>>           }
>>> -         else {
>>> +         default:
>>>              dst = pixels;
>>> +            break;
>>>           }
>>>           result = decompress_texture_image(ctx, texImage, slice,
>>>                                             xoffset, yoffset, width,
>>> height,
>>>
>>
>> _______________________________________________
>> mesa-dev mailing list
>> mesa-dev at lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>