[Mesa-dev] [PATCH] Make single-buffered GLES representation internally consistent

[Ilia Mirkin]

On Mon, Jun 27, 2016 at 4:17 PM, Gurchetan Singh
<gurchetansingh at chromium.org> wrote:
> There a few places in the code where clearing and reading are done on incorrect
> buffers for GLES contexts.  See comments for details.  This fixes 75 GLES3
> dEQP tests on the surfaceless platform with no regressions.
>
> v2:  Corrected unclear comment
> ---
>  src/mesa/main/buffers.c | 14 ++++++++++++--
>  src/mesa/main/clear.c   |  8 ++++++++
>  src/mesa/main/get.c     |  9 +++++++++
>  3 files changed, 29 insertions(+), 2 deletions(-)
>
> diff --git a/src/mesa/main/buffers.c b/src/mesa/main/buffers.c
> index e8aedde..86696b8 100644
> --- a/src/mesa/main/buffers.c
> +++ b/src/mesa/main/buffers.c
> @@ -173,12 +173,22 @@ draw_buffer_enum_to_bitmask(const struct gl_context *ctx, GLenum buffer)
>   * return -1 for an invalid buffer.
>   */
>  static gl_buffer_index
> -read_buffer_enum_to_index(GLenum buffer)
> +read_buffer_enum_to_index(const struct gl_context *ctx, GLenum buffer)
>  {
>     switch (buffer) {
>        case GL_FRONT:
>           return BUFFER_FRONT_LEFT;
>        case GL_BACK:
> +         if (_mesa_is_gles(ctx)) {
> +            /* In draw_buffer_enum_to_bitmask, when GLES contexts draw to
> +             * GL_BACK with a single-buffered configuration, we actually end
> +             * up drawing to the sole front buffer in our internal
> +             * representation.  For consistency, we must read from that
> +             * front left buffer too.
> +             */
> +            if (!ctx->DrawBuffer->Visual.doubleBufferMode)
> +               return BUFFER_FRONT_LEFT;
> +         }
>           return BUFFER_BACK_LEFT;
>        case GL_RIGHT:
>           return BUFFER_FRONT_RIGHT;
> @@ -724,7 +734,7 @@ read_buffer(struct gl_context *ctx, struct gl_framebuffer *fb,
>        if (_mesa_is_gles3(ctx) && !is_legal_es3_readbuffer_enum(buffer))
>           srcBuffer = -1;
>        else
> -         srcBuffer = read_buffer_enum_to_index(buffer);
> +         srcBuffer = read_buffer_enum_to_index(ctx, buffer);
>
>        if (srcBuffer == -1) {
>           _mesa_error(ctx, GL_INVALID_ENUM,
> diff --git a/src/mesa/main/clear.c b/src/mesa/main/clear.c
> index 35b912c..a1bb36e 100644
> --- a/src/mesa/main/clear.c
> +++ b/src/mesa/main/clear.c
> @@ -267,6 +267,14 @@ make_color_buffer_mask(struct gl_context *ctx, GLint drawbuffer)
>           mask |= BUFFER_BIT_FRONT_RIGHT;
>        break;
>     case GL_BACK:
> +      /* For GLES contexts with a single buffered configuration, we actually
> +       * only have a front renderbuffer, so any clear calls to GL_BACK should
> +       * affect that buffer. See draw_buffer_enum_to_bitmask for details.
> +       */
> +      if (_mesa_is_gles(ctx))
> +         if (!ctx->DrawBuffer->Visual.doubleBufferMode)
> +            if (att[BUFFER_FRONT_LEFT].Renderbuffer)
> +               mask |= BUFFER_BIT_FRONT_LEFT;
>        if (att[BUFFER_BACK_LEFT].Renderbuffer)
>           mask |= BUFFER_BIT_BACK_LEFT;
>        if (att[BUFFER_BACK_RIGHT].Renderbuffer)
> diff --git a/src/mesa/main/get.c b/src/mesa/main/get.c
> index 6ffa99c..ec6bc3e 100644
> --- a/src/mesa/main/get.c
> +++ b/src/mesa/main/get.c
> @@ -627,6 +627,15 @@ find_custom_value(struct gl_context *ctx, const struct value_desc *d, union valu
>        break;
>
>     case GL_READ_BUFFER:
> +      /* In _mesa_initialize_window_framebuffer, for single-buffered visuals,
> +       * the ColorReadBuffer is set to be GL_FRONT, even with GLES contexts.
> +       * When calling read_buffer, we verify we are reading from GL_BACK in
> +       * is_legal_es3_readbuffer_enum.  But the default is incorrect, and
> +       * certain dEQP tests check this.  So fix it here.
> +       */
> +      if (_mesa_is_gles(ctx))
> +         if (ctx->ReadBuffer->ColorReadBuffer == GL_FRONT)
> +            ctx->ReadBuffer->ColorReadBuffer = GL_BACK;

Why is this OK to do? Shouldn't these just get not get set that way in
the first place? Writing values when doing a random glGet() seems like
a recipe for confusion and hard-to-reproduce bugs.

  -ilia