[Mesa-dev] [PATCH] mesa: Improve handling of GL_BGRA format in es3 format_and_type checks
Eduardo Lima Mitev
elima at igalia.com
Mon Oct 19 14:52:24 PDT 2015
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?
>
> 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