[Mesa-dev] [PATCH 5/5] meta: Add a meta implementation of GL_ARB_clear_texture

[Neil Roberts]

Ian Romanick <idr at freedesktop.org> writes:

> Every driver in Mesa supports GL_EXT_framebuffer_object and
> GL_EXT_framebuffer_blit. […]
>
> So, I don't think this check adds any value.

Ok.

>> +   /* This probably won't work with images that have a border */
>> +   if (texImage->Border != 0)
>> +      return false;
>
> The border is stripped.  Even if Border != 0, it's stored as if Border
> == 0.  This check can get dropped too.

I looked into this a bit more and found that if the border wasn't
stripped and texImage->Border == 1 then neither the meta implementation
nor the texstore implementation would work correctly because the
coordinates passed glClearTexSubImage are relative to the image inside
the border (ie, -1,-1 is the bottom left of the border). Therefore the
meta implemention would need to offset the coordinates by the border
size when setting the scissor and the texstore implemenation would need
to do the same thing when passing the coordinates to the texture map
function. If we wanted to make this work then we would need these two
patches:

https://github.com/bpeel/mesa/commit/3a3108710ada329065cdee085ee73
https://github.com/bpeel/mesa/commit/6a6b63428f1700b3e580f87322c74

However it looks like only the swrast driver doesn't strip the border so
maybe it would be better to just remember to never enable the extension
there (or just not care that border textures would be broken) rather
than cluttering up the code unnecessarily.

While testing that I think I found a bug with using FBOs for a texture
with a border because the size of the renderbuffer is set to the size of
the texture without the border. This makes the swrast driver use an
incorrect stride and it renders garbage.

With that in mind, I wonder if I should remove all mentions to
texImage->Border in the rest of the patches too?

> I think I'd structure the previous code as:
>
>     success = true;
>     for (i = 0; i < depth; i++) {
>        if (!...) {
>           success = false;
>           break;
>        }
>     }

Ok, fair enough.

Thanks for the review.

- Neil