<div dir="ltr"><div>Hi Ilia,</div><div><br></div><div>The changes for get.c where prompted by the es3fIntegerStateQueryTests (see modules/gles3/functional/es3fIntegerStateQueryTests.cpp in the dEQP tree).  Specifically, these few lines:</div><div><br></div><div>>> const GLint validInitialValues[] = {GL_BACK, GL_NONE};</div><div>>> m_verifier->verifyIntegerAnyOf(m_testCtx, GL_READ_BUFFER, validInitialValues, DE_LENGTH_OF_ARRAY(validInitialValues));</div><div>>> expectError(GL_NO_ERROR);</div><div><br></div><div>We initially set ColorReadBuffer to GL_FRONT in _mesa_initialize_window_framebuffer for single-buffered configs.  </div><div><br></div><div>We could also make sure the context is single-buffered in get.c to further avoid bugs.  Let me know if that works for you and I'll send a modified patch.</div><div><br></div><div>I do agree it is a bit hacky ... I'd definitely be interested in alternative solutions.</div></div><div class="gmail_extra"><br><div class="gmail_quote">On Mon, Jun 27, 2016 at 1:45 PM, Ilia Mirkin <span dir="ltr"><<a href="mailto:imirkin@alum.mit.edu" target="_blank">imirkin@alum.mit.edu</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5">On Mon, Jun 27, 2016 at 4:17 PM, Gurchetan Singh<br>
<<a href="mailto:gurchetansingh@chromium.org">gurchetansingh@chromium.org</a>> wrote:<br>
> There a few places in the code where clearing and reading are done on incorrect<br>
> buffers for GLES contexts.  See comments for details.  This fixes 75 GLES3<br>
> dEQP tests on the surfaceless platform with no regressions.<br>
><br>
> v2:  Corrected unclear comment<br>
> ---<br>
>  src/mesa/main/buffers.c | 14 ++++++++++++--<br>
>  src/mesa/main/clear.c   |  8 ++++++++<br>
>  src/mesa/main/get.c     |  9 +++++++++<br>
>  3 files changed, 29 insertions(+), 2 deletions(-)<br>
><br>
> diff --git a/src/mesa/main/buffers.c b/src/mesa/main/buffers.c<br>
> index e8aedde..86696b8 100644<br>
> --- a/src/mesa/main/buffers.c<br>
> +++ b/src/mesa/main/buffers.c<br>
> @@ -173,12 +173,22 @@ draw_buffer_enum_to_bitmask(const struct gl_context *ctx, GLenum buffer)<br>
>   * return -1 for an invalid buffer.<br>
>   */<br>
>  static gl_buffer_index<br>
> -read_buffer_enum_to_index(GLenum buffer)<br>
> +read_buffer_enum_to_index(const struct gl_context *ctx, GLenum buffer)<br>
>  {<br>
>     switch (buffer) {<br>
>        case GL_FRONT:<br>
>           return BUFFER_FRONT_LEFT;<br>
>        case GL_BACK:<br>
> +         if (_mesa_is_gles(ctx)) {<br>
> +            /* In draw_buffer_enum_to_bitmask, when GLES contexts draw to<br>
> +             * GL_BACK with a single-buffered configuration, we actually end<br>
> +             * up drawing to the sole front buffer in our internal<br>
> +             * representation.  For consistency, we must read from that<br>
> +             * front left buffer too.<br>
> +             */<br>
> +            if (!ctx->DrawBuffer->Visual.doubleBufferMode)<br>
> +               return BUFFER_FRONT_LEFT;<br>
> +         }<br>
>           return BUFFER_BACK_LEFT;<br>
>        case GL_RIGHT:<br>
>           return BUFFER_FRONT_RIGHT;<br>
> @@ -724,7 +734,7 @@ read_buffer(struct gl_context *ctx, struct gl_framebuffer *fb,<br>
>        if (_mesa_is_gles3(ctx) && !is_legal_es3_readbuffer_enum(buffer))<br>
>           srcBuffer = -1;<br>
>        else<br>
> -         srcBuffer = read_buffer_enum_to_index(buffer);<br>
> +         srcBuffer = read_buffer_enum_to_index(ctx, buffer);<br>
><br>
>        if (srcBuffer == -1) {<br>
>           _mesa_error(ctx, GL_INVALID_ENUM,<br>
> diff --git a/src/mesa/main/clear.c b/src/mesa/main/clear.c<br>
> index 35b912c..a1bb36e 100644<br>
> --- a/src/mesa/main/clear.c<br>
> +++ b/src/mesa/main/clear.c<br>
> @@ -267,6 +267,14 @@ make_color_buffer_mask(struct gl_context *ctx, GLint drawbuffer)<br>
>           mask |= BUFFER_BIT_FRONT_RIGHT;<br>
>        break;<br>
>     case GL_BACK:<br>
> +      /* For GLES contexts with a single buffered configuration, we actually<br>
> +       * only have a front renderbuffer, so any clear calls to GL_BACK should<br>
> +       * affect that buffer. See draw_buffer_enum_to_bitmask for details.<br>
> +       */<br>
> +      if (_mesa_is_gles(ctx))<br>
> +         if (!ctx->DrawBuffer->Visual.doubleBufferMode)<br>
> +            if (att[BUFFER_FRONT_LEFT].Renderbuffer)<br>
> +               mask |= BUFFER_BIT_FRONT_LEFT;<br>
>        if (att[BUFFER_BACK_LEFT].Renderbuffer)<br>
>           mask |= BUFFER_BIT_BACK_LEFT;<br>
>        if (att[BUFFER_BACK_RIGHT].Renderbuffer)<br>
> diff --git a/src/mesa/main/get.c b/src/mesa/main/get.c<br>
> index 6ffa99c..ec6bc3e 100644<br>
> --- a/src/mesa/main/get.c<br>
> +++ b/src/mesa/main/get.c<br>
> @@ -627,6 +627,15 @@ find_custom_value(struct gl_context *ctx, const struct value_desc *d, union valu<br>
>        break;<br>
><br>
>     case GL_READ_BUFFER:<br>
> +      /* In _mesa_initialize_window_framebuffer, for single-buffered visuals,<br>
> +       * the ColorReadBuffer is set to be GL_FRONT, even with GLES contexts.<br>
> +       * When calling read_buffer, we verify we are reading from GL_BACK in<br>
> +       * is_legal_es3_readbuffer_enum.  But the default is incorrect, and<br>
> +       * certain dEQP tests check this.  So fix it here.<br>
> +       */<br>
> +      if (_mesa_is_gles(ctx))<br>
> +         if (ctx->ReadBuffer->ColorReadBuffer == GL_FRONT)<br>
> +            ctx->ReadBuffer->ColorReadBuffer = GL_BACK;<br>
<br>
</div></div>Why is this OK to do? Shouldn't these just get not get set that way in<br>
the first place? Writing values when doing a random glGet() seems like<br>
a recipe for confusion and hard-to-reproduce bugs.<br>
<span class="HOEnZb"><font color="#888888"><br>
  -ilia<br>
</font></span></blockquote></div><br></div>