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 <el...@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 <el...@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@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
