On 10/19/2015 11:49 PM, Eduardo Lima Mitev wrote: > On 10/16/2015 06:33 AM, Jason Ekstrand wrote: >> 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. >> > > I have been analyzing this again, specifically the code-paths you > mentions. You are right that more thought is needed to avoid further > regressions. I have experimented with a new patch that handles GL_BGRA > in those other places where it is currently ignored, and it seems save > to me. But... > >> 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). > > I fully agree that in this case neither dEQP nor piglit are enough to > land a patch that modifies the returned value of _mesa_base_tex_format() > from GL_RGBA to GL_BGRA. We need to check if there are piglit tests that > covers against the changes to these other code-paths, as well as a test > that checks that a GL_BGRA texture actually works. > > So, I propose the following, in order to move on with the bug: > > 1) Move forward with the piglit test that checks API errors related with > GL_BGRA (I sent a revised version [1]), but remove the checks that will > fail current master (format=GL_BGRA_EXT, internalFormat=GL_RGBA). > > 2) I find a slot of time to produce a new patch for Mesa that would > consider all possible implications more thoroughly. This would include > writing more tests to piglit, if needed. > > This way we could fix the bug now, since the piglit API error test I > wrote is enough to prevent this particular regression in the future (I > verified that). > > WDYT? > > Eduardo >
[1] http://lists.freedesktop.org/archives/piglit/2015-October/017561.html >> --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 > _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
