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

[Erik Faye-Lund]

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.