[Mesa-dev] [PATCH v2 05/29] mesa/main: clean up ES2_compatibility check

[Emil Velikov]

On Mon, 3 Dec 2018 at 17:47, Erik Faye-Lund
<erik.faye-lund at collabora.com> wrote:
>
> On Mon, 2018-12-03 at 17:24 +0000, Emil Velikov wrote:
> > Hi Erik,
> >
> > On Fri, 23 Nov 2018 at 10:54, Erik Faye-Lund
> > <erik.faye-lund at collabora.com> wrote:
> > > This makes the logic a little bit easier to follow; this is
> > > *either*
> > > about ES2 compatibility *or* about gles. GL_RGB565 was added
> > > already in
> > > OpenGL ES 1.0.
> > >
> > AFAICT GL_RGB565 was introduced in OpenGLES1 with
> > GL_OES_framebuffer_object.
> >
> > Every driver supports that extension, so
> > _mesa_has_OES_framebuffer_object is an overkill.
> > Esp. since src/mesa/main/fbobject.c doesn't do it - although it that
> > one could use _mesa_has_ARB_ES2_compatibility().
> >
> > fbobject.c could be tweaked another day.
> >
>
> Hmmpf. I already pushed the series, but you're right. Seems I confused
> GL_RGB565 and GL_RGB + GL_UNSIGNED_SHORT_5_6_5, of which the latter was
> there from GLES 1.0.
>
> I supposed you're right, though; this won't misbehave on any current
> drivers. But it would be nice to clean it up somehow. Perhaps I should
> add a comment as a follow-up, something like this?
>
> ---8<---
> diff --git a/src/mesa/main/glformats.c b/src/mesa/main/glformats.c
> index 7506c238232..79d2a9739d7 100644
> --- a/src/mesa/main/glformats.c
> +++ b/src/mesa/main/glformats.c
> @@ -2313,6 +2313,8 @@ _mesa_base_tex_format(const struct gl_context
> *ctx, GLint internalFormat)
>     }
>
>     if (_mesa_has_ARB_ES2_compatibility(ctx) || _mesa_is_gles(ctx)) {
> +      /* GL_RGB565 requires OES_framebuffer_object on GLES 1.x, but
> all drivers
> +       * support that extension, so let's drop the complication */
>        switch (internalFormat) {
>        case GL_RGB565:
>           return GL_RGB;
> ---8<---
>
> Otherwise, I also don't think it's *too* bad to do this, just for
> clarity:
>
> ---8<---
> diff --git a/src/mesa/main/glformats.c b/src/mesa/main/glformats.c
> index 7506c238232..ea73068d025 100644
> --- a/src/mesa/main/glformats.c
> +++ b/src/mesa/main/glformats.c
> @@ -2312,7 +2312,9 @@ _mesa_base_tex_format(const struct gl_context
> *ctx, GLint internalFormat)
>        }
>     }
>
> -   if (_mesa_has_ARB_ES2_compatibility(ctx) || _mesa_is_gles(ctx)) {
> +   if (_mesa_has_ARB_ES2_compatibility(ctx) ||
> +       _mesa_has_OES_framebuffer_object(ctx) ||
> +       ctx->API == API_OPENGLES2) {
>        switch (internalFormat) {
>        case GL_RGB565:
>           return GL_RGB;
> ---8<---
>
> ..but yeah, I suppose we need some more guards in fbobject.c as well.
>
Personally, both looks good. For whichever you want to go with:

Reviewed-by: Emil Velikov <emil.velikov at collabora.com>

-Emil