On 10/08/2015 12:58 AM, Jason Ekstrand wrote:
> The EXT_texture_format_BGRA8888 extension (which mesa supports
> unconditionally) adds a new format and internal format called GL_BGRA_EXT.
> Previously, this was not really handled at all in
> _mesa_ex3_error_check_format_and_type. When the checks were tightened in
> commit f15a7f3c, we accidentally tightened things too far and GL_BGRA_EXT
> would always cause an error to be thrown.
>
> There were two primary issues here. First, is that
> _mesa_es3_effective_internal_format_for_format_and_type didn't handle the
> GL_BGRA_EXT format. Second is that it blindly uses _mesa_base_tex_format
> which returns GL_RGBA for GL_BGRA_EXT. This commit fixes both of these
> issues as well as adds explicit checks that GL_BGRA_EXT is only ever used
> with GL_BGRA_EXT and GL_UNSIGNED_BYTE.
>
> Signed-off-by: Jason Ekstrand <jason.ekstr...@intel.com>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=92265
> Cc: Eduardo Lima Mitev <el...@igalia.com>
> ---
> src/mesa/main/glformats.c | 21 +++++++++++++++++++--
> 1 file changed, 19 insertions(+), 2 deletions(-)
>
> diff --git a/src/mesa/main/glformats.c b/src/mesa/main/glformats.c
> index 7dab33c..843ec02 100644
> --- a/src/mesa/main/glformats.c
> +++ b/src/mesa/main/glformats.c
> @@ -2678,6 +2678,7 @@
> _mesa_es3_effective_internal_format_for_format_and_type(GLenum format,
> * internal formats, they do not correspond to GL constants, so the
> base
> * format is returned instead.
> */
> + case GL_BGRA_EXT:
> case GL_LUMINANCE_ALPHA:
> case GL_LUMINANCE:
> case GL_ALPHA:
> @@ -2797,8 +2798,19 @@ _mesa_es3_error_check_format_and_type(const struct
> gl_context *ctx,
> if (effectiveInternalFormat == GL_NONE)
> return GL_INVALID_OPERATION;
>
> - GLenum baseInternalFormat =
> - _mesa_base_tex_format(ctx, effectiveInternalFormat);
> + 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 doe 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;
What's wrong with having _mesa_base_tex_format() return GL_BGRA_EXT for
format GL_BGRA_EXT?
Looking through all usages of _mesa_base_tex_format(), I fail to see a
case where returning GL_BGRA_EXT instead of GL_RGBA would cause a
different behavior.
As I understand the EXT_texture_format_BGRA8888 extension [1],
GL_BGRA_EXT becomes a valid base format at the same level of the others
in the table, which is what _mesa_base_tex_format() claims to return
(quoting the function doc)
"\return the corresponding \u base internal format (GL_ALPHA,
GL_LUMINANCE, GL_DEPTH_STENCIL, GL_LUMANCE_ALPHA, GL_INTENSITY, GL_RGB,
or GL_RGBA), or -1 if invalid enum."
I also tested dEQP and piglit with this modification and saw no regressions.
If we modify _mesa_base_tex_format() to return GL_BGRA_EXT for
GL_BGRA_EXT format, we simplify this code and avoid adding the specific
check inside the code that resolves the effective internal format.
Sorry to chime in late. I just returned from holidays.
> + } else {
> + baseInternalFormat =
> + _mesa_base_tex_format(ctx, effectiveInternalFormat);
> + }
>
> if (internalFormat != baseInternalFormat)
> return GL_INVALID_OPERATION;
> @@ -2807,6 +2819,11 @@ _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)
> + return GL_INVALID_OPERATION;
> + break;
> +
> case GL_RGBA:
> switch (type) {
> case GL_UNSIGNED_BYTE:
>
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
