[Mesa-dev] [PATCH v2] mesa: Enable GL_RGB/GL_RGBA in GLES3 glGetInternalformativ

[Chad Versace]

On 01/09/2015 02:11 PM, michael.w.mason at intel.com wrote:
> From: Mike Mason <michael.w.mason at intel.com>

The substance of this patch looks good to me. I have a few formatting nitpicks
though.
 
> Removes commit 7894278 changes and moves fix to _mesa_GetInternalformativ().
> The original commit enabled the GL_RGB and GL_RGBA unsized internal formats
> as valid for render buffers in GLES3, but this is incorrect. They should
> have only been enabled for GetInternalformativ()
> 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=88079
> ---
>  src/mesa/main/fbobject.c    |  6 ------
>  src/mesa/main/formatquery.c | 12 +++++++++++-
>  2 files changed, 11 insertions(+), 7 deletions(-)
> 
> diff --git a/src/mesa/main/fbobject.c b/src/mesa/main/fbobject.c
> index 43b0886..f059750 100644
> --- a/src/mesa/main/fbobject.c
> +++ b/src/mesa/main/fbobject.c
> @@ -1430,9 +1430,6 @@ _mesa_base_fbo_format(struct gl_context *ctx, GLenum internalFormat)
>     case GL_RGB8:
>        return GL_RGB;
>     case GL_RGB:
> -      if (_mesa_is_gles3(ctx))
> -         return GL_RGB;
> -      /* fallthrough */
>     case GL_R3_G3_B2:
>     case GL_RGB4:
>     case GL_RGB5:
> @@ -1447,9 +1444,6 @@ _mesa_base_fbo_format(struct gl_context *ctx, GLenum internalFormat)
>     case GL_RGBA8:
>        return GL_RGBA;
>     case GL_RGBA:
> -      if (_mesa_is_gles3(ctx))
> -         return GL_RGBA;
> -      /* fallthrough */
>     case GL_RGBA2:
>     case GL_RGBA12:
>     case GL_RGBA16:
> diff --git a/src/mesa/main/formatquery.c b/src/mesa/main/formatquery.c
> index f6274fe..e8877c6 100644
> --- a/src/mesa/main/formatquery.c
> +++ b/src/mesa/main/formatquery.c
> @@ -89,8 +89,18 @@ _mesa_GetInternalformativ(GLenum target, GLenum internalformat, GLenum pname,
>      *     "If the <internalformat> parameter to GetInternalformativ is not
>      *     color-, depth- or stencil-renderable, then an INVALID_ENUM error is
>      *     generated."
> +    *
> +    * Page 243 of the GLES 3.0.4 spec says this for GetInternalformativ:
> +    *     "internalformat must be color-renderable, depth-renderable or 
> +    *     stencilrenderable (as defined in section 4.4.4)."
> +    * Section 4.4.4 on page 212 of the same spec says:
> +    *     "An internal format is color-renderable if it is one of the
> +    *     formats from table 3.13 noted as color-renderable or if it
> +    *     is unsized format RGBA or RGB."
> +    * Therefore, we must accept GL_RGB and GL_RGBA here.
>      */

Please add some newline padding aroundthe spec quotation. The usual style
throughout Mesa, and elsewhere in this function, is:

/* [The spec says]
 *     [empty line]
 *     [spec quote] 
 * [empty line]
 * [more comments, if needed]
 */

The extra newlines make it easier to quickly visually parse the comments.

Also, git complained about trailing whitespace in the hunk above. Here's the warning:

---- -8<- ----
Applying: mesa: Enable GL_RGB/GL_RGBA in GLES3 glGetInternalformativ
/home/chadv/proj/mesa/work/.git/rebase-apply/patch:40: trailing whitespace.
    *     "internalformat must be color-renderable, depth-renderable or 
warning: 1 line adds whitespace errors.
---- ->8- ----

If you don't see these warnings yourself, you can enable them by enabling
the pre-commit hook distributed with git.

    $ cd ~/mesa/.git/hooks
    $ mv pre-commit.sample pre-commit

Thanks for this bufgix!


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 884 bytes
Desc: OpenPGP digital signature
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20150112/e98e9d79/attachment.sig>