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

[Eduardo Lima Mitev]

On 10/16/2015 06:33 AM, Jason Ekstrand wrote:
> 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.
>

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

> --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
>>
>