On Mon, Oct 19, 2015 at 2:52 PM, Eduardo Lima Mitev <el...@igalia.com> wrote: > 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?
I'm fine with pushing an API test that fails. Let's not weaaken the piglit test just because the driver is broken. Other than that, sounds fine to me. --Jason >> 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
