Re: [Mesa-dev] [PATCH v2 05/29] mesa/main: clean up ES2_compatibility check
On Mon, 3 Dec 2018 at 17:47, Erik Faye-Lund
wrote:
>
> On Mon, 2018-12-03 at 17:24 +, Emil Velikov wrote:
> > Hi Erik,
> >
> > On Fri, 23 Nov 2018 at 10:54, Erik Faye-Lund
> > 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
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH v2 05/29] mesa/main: clean up ES2_compatibility check
On Mon, 2018-12-03 at 17:24 +, Emil Velikov wrote:
> Hi Erik,
>
> On Fri, 23 Nov 2018 at 10:54, Erik Faye-Lund
> 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.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH v2 05/29] mesa/main: clean up ES2_compatibility check
Hi Erik, On Fri, 23 Nov 2018 at 10:54, Erik Faye-Lund 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. -Emil ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH v2 05/29] mesa/main: clean up ES2_compatibility check
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.
Signed-off-by: Erik Faye-Lund
---
src/mesa/main/glformats.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/mesa/main/glformats.c b/src/mesa/main/glformats.c
index a47263a6525..d837bfc1aca 100644
--- a/src/mesa/main/glformats.c
+++ b/src/mesa/main/glformats.c
@@ -2325,7 +2325,7 @@ _mesa_base_tex_format(const struct gl_context *ctx, GLint
internalFormat)
}
}
- if (ctx->Extensions.ARB_ES2_compatibility) {
+ if (_mesa_has_ARB_ES2_compatibility(ctx) || _mesa_is_gles(ctx)) {
switch (internalFormat) {
case GL_RGB565:
return GL_RGB;
--
2.19.1
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
