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 <imir...@alum.mit.edu> wrote:
> On Mon, Jun 27, 2016 at 4:17 PM, Gurchetan Singh
> <gurchetansi...@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
>
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
