[Mesa-dev] [PATCH] mesa: Improve handling of GL_BGRA format in es3 format_and_type checks

[Jason Ekstrand]

On Wed, Oct 14, 2015 at 6:56 PM, Eduardo Lima Mitev <elima at igalia.com> wrote:
> We recently added support for GL_BGRA internal format when validating
> combination of format+type+internal_format in Tex(Sub)ImageXD calls
> (to fix https://bugs.freedesktop.org/show_bug.cgi?id=92265).
>
> However, the current implementation handles it as a special case when
> obtaining the effective internal format, treating GL_BGRA as if its
> base format is GL_RGBA execpt for the case of validation.
>
> This causes Mesa to accept a combination like:
> internalFormat = GL_BGRA_EXT, format = GL_RGBA, type = GL_UNSIGNED_BYTE as
> valid arguments to TexImage2D, when it is actually an invalid combination
> per EXT_texture_format_BGRA8888
> <https://www.khronos.org/registry/gles/extensions/EXT/EXT_texture_format_BGRA8888.txt>
>
> This patch makes _mesa_base_tex_format() return GL_BGRA_EXT as base format of
> GL_BGRA_EXT internal format, which is consistent with the extension
> spec. As a result, the code for handling GL_BGRA during validation gets
> simplified.
> ---
>  src/mesa/main/glformats.c | 21 +++++++--------------
>  1 file changed, 7 insertions(+), 14 deletions(-)
>
> diff --git a/src/mesa/main/glformats.c b/src/mesa/main/glformats.c
> index faa6382..e0192fe 100644
> --- a/src/mesa/main/glformats.c
> +++ b/src/mesa/main/glformats.c
> @@ -2148,6 +2148,9 @@ _mesa_es_error_check_format_and_type(GLenum format, GLenum type,
>   *
>   * \return the corresponding \u base internal format (GL_ALPHA, GL_LUMINANCE,
>   * GL_LUMANCE_ALPHA, GL_INTENSITY, GL_RGB, or GL_RGBA), or -1 if invalid enum.
> + * When profile is GLES, it will also return GL_BGRA as base format of
> + * GL_BGRA internal format, as specified by extension
> + * EXT_texture_format_BGRA8888.
>   *
>   * This is the format which is used during texture application (i.e. the
>   * texture format and env mode determine the arithmetic used.
> @@ -2215,7 +2218,7 @@ _mesa_base_tex_format(const struct gl_context *ctx, GLint internalFormat)
>     if (_mesa_is_gles(ctx)) {
>        switch (internalFormat) {
>        case GL_BGRA:
> -         return GL_RGBA;
> +         return GL_BGRA_EXT;

I don't think we can just up-and-change this.  It does get used some
places that don't expect GL_BGRA_EXT.  For instance, in
copytexture_error_check (teximage.c:2265) we call
_mesa_base_tex_format to get the base tex format for the renderbuffer
or texture and then pass that into _mesa_base_format_component_count()
which doesn't  know about GL_BGRA_EXT and so returns -1.  Also, in
gallium in st_format.c:1982 they have a line to treat BGRA as RGBA
which I'm guessing is to hack around mesa doing the same.  I think we
probably can make this change, but it's going to take some more care.

Given that we completely broke Weston and KWin with this, I don't
think "It passes piglit" is a particularly convincing argument for
this one.  Some code-searching would be good and we probably need a
test that actually tests that the format works (not just API errors).
--Jason

>        default:
>           ; /* fallthrough */
>        }
> @@ -2799,18 +2802,8 @@ _mesa_es3_error_check_format_and_type(const struct gl_context *ctx,
>           return GL_INVALID_OPERATION;
>
>        GLenum baseInternalFormat;
> -      if (internalFormat == GL_BGRA_EXT) {
> -         /* Unfortunately, _mesa_base_tex_format returns a base format of
> -          * GL_RGBA for GL_BGRA_EXT.  This makes perfect sense if you're
> -          * asking the question, "what channels does this format have?"
> -          * However, if we're trying to determine if two internal formats
> -          * match in the ES3 sense, we actually want GL_BGRA.
> -          */
> -         baseInternalFormat = GL_BGRA_EXT;
> -      } else {
> -         baseInternalFormat =
> -            _mesa_base_tex_format(ctx, effectiveInternalFormat);
> -      }
> +      baseInternalFormat =
> +         _mesa_base_tex_format(ctx, effectiveInternalFormat);
>
>        if (internalFormat != baseInternalFormat)
>           return GL_INVALID_OPERATION;
> @@ -2820,7 +2813,7 @@ _mesa_es3_error_check_format_and_type(const struct gl_context *ctx,
>
>     switch (format) {
>     case GL_BGRA_EXT:
> -      if (type != GL_UNSIGNED_BYTE || internalFormat != GL_BGRA)
> +      if (type != GL_UNSIGNED_BYTE || internalFormat != format)
>           return GL_INVALID_OPERATION;
>        break;
>
> --
> 2.5.3
>