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

[Gurchetan Singh]

Hi Ilia,

The changes for get.c where prompted by the es3fIntegerStateQueryTests (see
modules/gles3/functional/es3fIntegerStateQueryTests.cpp in the dEQP tree).
Specifically, these few lines:

>> const GLint validInitialValues[] = {GL_BACK, GL_NONE};
>> m_verifier->verifyIntegerAnyOf(m_testCtx, GL_READ_BUFFER,
validInitialValues, DE_LENGTH_OF_ARRAY(validInitialValues));
>> expectError(GL_NO_ERROR);

We initially set ColorReadBuffer to GL_FRONT in
_mesa_initialize_window_framebuffer for single-buffered configs.

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.

I do agree it is a bit hacky ... I'd definitely be interested in
alternative solutions.

On Mon, Jun 27, 2016 at 1:45 PM, Ilia Mirkin <imirkin at alum.mit.edu> wrote:

> 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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20160627/c89a03a9/attachment-0001.html>