On Wed, Oct 14, 2015 at 6:56 PM, Eduardo Lima Mitev <el...@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 > _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev