[Mesa-dev] [PATCH] mesa: change state query return value for RGB565

[Chad Versace]

+jekstrand

On 08/12/2016 01:36 PM, Haixia Shi wrote:
> The GL_BGR and GL_UNSIGNED_SHORT_5_6_5_REV are not defined anywhere in
> OpenGL ES 3.2 (or earlier) specification, and there are no known extensions
> in the Khronos registry that would add these enums as valid responses for
> glGetIntegerv(GL_IMPLEMENTATION_COLOR_READ_TYPE) and
> glGetIntegerv(GL_IMPLEMENTATION_COLOR_READ_FORMAT) queries.
>
> TEST=dEQP-GLES3.functional.state_query.integers.*

Does this patch fix all the tests in that group? Just a few tests? Exactly one?

> Signed-off-by: Haixia Shi <hshi at chromium.org>
> Cc: Chad Versace <chadversary at chromium.org>
> Cc: Stéphane Marchesin <marcheu at chromium.org>
>
> Change-Id: I81bbc8ccdc7e125edaeae443baf6fa8fdefcc6b6
> ---
>  src/mesa/main/framebuffer.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/src/mesa/main/framebuffer.c b/src/mesa/main/framebuffer.c
> index 38bd680..f024f5e 100644
> --- a/src/mesa/main/framebuffer.c
> +++ b/src/mesa/main/framebuffer.c
> @@ -857,7 +857,7 @@ _mesa_get_color_read_format(struct gl_context *ctx)
>        if (format == MESA_FORMAT_B8G8R8A8_UNORM)
>           return GL_BGRA;
>        else if (format == MESA_FORMAT_B5G6R5_UNORM)
> -         return GL_BGR;
> +         return GL_RGB;
>        else if (format == MESA_FORMAT_R_UNORM8)
>           return GL_RED;
>
> @@ -892,7 +892,7 @@ _mesa_get_color_read_type(struct gl_context *ctx)
>        const GLenum data_type = _mesa_get_format_datatype(format);
>
>        if (format == MESA_FORMAT_B5G6R5_UNORM)
> -         return GL_UNSIGNED_SHORT_5_6_5_REV;
> +         return GL_UNSIGNED_SHORT_5_6_5;
>
>        switch (data_type) {
>        case GL_SIGNED_NORMALIZED:

I'm convinced that the original code and hshi's patch are both correct because, as defined by the GL spec, the bit layout of

     GL_BGR + GL_UNSIGNED_SHORT_5_6_5_REV
and

     GL_RGB + GL_UNSIGNED_SHORT_5_6_5

are identical, which is

     msb -> lsb     lsb -> msb
     R6 G6 B5    == B5 G6 R5    == ISL_FORMAT_B5G6R5_UNORM

I *really* want to see the commit message briefly state that the patch doesn't change the bit
layout returned by the query. Because that's critical to the patch's correctness, but not
obvious at all.

I'm not 100% confident in my conclusion, though. I would like to see a 2nd opinion on mesa-dev, preferably by jekstrand who is the Format Layout King.