[Mesa-dev] [PATCH] mesa: Improve handling of GL_BGRA format in es3 format_and_type checks
Jason Ekstrand
jason at jlekstrand.net
Mon Oct 19 16:14:23 PDT 2015
On Mon, Oct 19, 2015 at 2:52 PM, Eduardo Lima Mitev <elima at 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 <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?
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 at lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
>>
>
More information about the mesa-dev
mailing list