[virglrenderer-devel] [PATCH] blitter: Set fbo sRGB state according to the destination sourface type

[Gert Wollny]

Am Freitag, den 20.07.2018, 14:14 -0700 schrieb Robert Tarasov:
> Looks ok for me. Couple questions:
> 
> 1. Is there any reason to have "glEnable(GL_FRAMEBUFFER_SRGB);"
> inside loop?
You're right, I'll prepare an update for the patch to move this out of
the loop.  

> 2. Would it be better to restore old GL_FRAMEBUFFER_SRGB state
> instead of disabling it?
This framebuffer is only ever used from within this function, so it
should actually be enough to just set it correctly before it is used
and re-setting the state at the end is superfluous.

Best, 
Gert 

> 
> On Thu, Jul 19, 2018 at 5:03 AM Gert Wollny <gert.wollny at collabora.co
> m> wrote:
> > On an GL host set the sRGB framebuffer state explicitly to make
> > virgl
> > behave like on a GLES host.
> > 
> > This does not correct the handing of sRGB completely, because the
> > state
> > GL_FRAMEBUFFER_SRGB is not properly transmitted to the host. As a
> > result
> > the piglits  "blit texture linear_to_srgb * * *" flip.
> > Tests thatset "enable" failed before and pass now, and tests that
> > set "disable"
> > now fail.
> > 
> > Fixes on GL host:
> >   dEQP-GLES3.functional.fbo.blit.conversion.rgb8_to_srgb8_alpha8
> >   dEQP-GLES3.functional.fbo.blit.default_framebuffer.srgb8_alpha8
> > 
> > Signed-off-by: Gert Wollny <gert.wollny at collabora.com>
> > ---
> > 
> > Since this doesn't correct the piglits, only flips some of them, I
> > thought I 
> > might add a comment about the problems fixing this, I'm actually
> > not sure
> > whether handling GL_FRAMEBUFFER_SRGB can really be fixed in a
> > reliable way 
> > considering the approach how framebuffers and blits are
> > implemented: On one 
> > hand, the state GL_FRAMEBUFFER_SRGB is currently not transmitted to
> > the host,
> > but even if it would be, one still would have to guess to which
> > framebuffer it
> > refers, because the guest never sends information about which
> > framebuffers are
> > currently bound: This is handled within the mesa code, and Gallium
> > isn't
> > notified about changes in binding.
> > 
> > On the other hand, if GL_FRAMEBUFFER_SRGB is disabled, but an sRGB
> > surface is 
> > bound to the framebuffer then the framebuffer's state is updated by
> > attaching 
> > a non-sRGB surface that is also created on the host as a texture
> > view. Then the
> > changed state is then send to the host via
> > VIRGL_CCMD_SET_FRAMEBUFFER_STATE, 
> > but the command doesn't have any information about *which*
> > framebuffer's state 
> > must be set, since Gallium doesn't know this and virglrenderer
> > always sets the
> > state of the current sub-context's default fb.
> > 
> > Now, when virglrenderer enters vrend_renderer_blit_int it
> > references the 
> > original textures with there original formats, and when
> > GL_FRAMEBUFFER_SRGB is 
> > disabled things go wrong. 
> > 
> > The only approach for guessing the right state I could think of is,
> > to change 
> > Gallium to send the changed GL_FRAMEBUFFER_SRGB state, and when a
> > blit comes up, 
> > to check whether the target texture is sRGB has a texture view that
> > has the 
> > corresponding linear format and has otherwise the same parameters. 
> > 
> > Thanks for any comments and for reviewing the patch, 
> > Gert
> > 
> >  src/vrend_blitter.c  | 19 ++++++++++++++++++-
> >  src/vrend_renderer.c |  9 ++++++++-
> >  src/vrend_renderer.h |  3 ++-
> >  3 files changed, 28 insertions(+), 3 deletions(-)
> > 
> > diff --git a/src/vrend_blitter.c b/src/vrend_blitter.c
> > index 68b5bab..1439c9b 100644
> > --- a/src/vrend_blitter.c
> > +++ b/src/vrend_blitter.c
> > @@ -686,7 +686,8 @@ static void calc_dst_deltas_from_src(const
> > struct pipe_blit_info *info,
> >  void vrend_renderer_blit_gl(UNUSED struct vrend_context *ctx,
> >                              struct vrend_resource *src_res,
> >                              struct vrend_resource *dst_res,
> > -                            const struct pipe_blit_info *info)
> > +                            const struct pipe_blit_info *info,
> > +                            bool use_gles)
> >  {
> >     struct vrend_blitter_ctx *blit_ctx = &vrend_blit_ctx;
> >     GLuint buffers;
> > @@ -787,6 +788,8 @@ void vrend_renderer_blit_gl(UNUSED struct
> > vrend_context *ctx,
> >                        to_gl_swizzle(src_entry->swizzle[3]));
> >     }
> > 
> > +   if (!use_gles && util_format_is_srgb(src_res->base.format))
> > +      glTexParameteri(src_res->target, GL_TEXTURE_SRGB_DECODE_EXT,
> > GL_DECODE_EXT);
> > 
> >     glTexParameteri(src_res->target, GL_TEXTURE_WRAP_S,
> > GL_CLAMP_TO_EDGE);
> >     glTexParameteri(src_res->target, GL_TEXTURE_WRAP_T,
> > GL_CLAMP_TO_EDGE);
> > @@ -820,6 +823,13 @@ void vrend_renderer_blit_gl(UNUSED struct
> > vrend_context *ctx,
> >        glBindFramebuffer(GL_FRAMEBUFFER_EXT, blit_ctx->fb_id);
> >        vrend_fb_bind_texture(dst_res, 0, info->dst.level, layer);
> > 
> > +      if (!use_gles) {
> > +         if (util_format_is_srgb(dst_res->base.format))
> > +            glEnable(GL_FRAMEBUFFER_SRGB);
> > +         else
> > +            glDisable(GL_FRAMEBUFFER_SRGB);
> > +      }
> > +
> >        buffers = GL_COLOR_ATTACHMENT0_EXT;
> >        glDrawBuffers(1, &buffers);
> >        blitter_set_texcoords(blit_ctx, src_res, info->src.level,
> > @@ -829,4 +839,11 @@ void vrend_renderer_blit_gl(UNUSED struct
> > vrend_context *ctx,
> >        glBufferData(GL_ARRAY_BUFFER, sizeof(blit_ctx->vertices),
> > blit_ctx->vertices, GL_STATIC_DRAW);
> >        glDrawArrays(GL_TRIANGLE_FAN, 0, 4);
> >     }
> > +   if (!use_gles) {
> > +      if (util_format_is_srgb(dst_res->base.format))
> > +         glDisable(GL_FRAMEBUFFER_SRGB);
> > +
> > +      if (util_format_is_srgb(src_res->base.format))
> > +         glTexParameteri(src_res->target,
> > GL_TEXTURE_SRGB_DECODE_EXT, GL_DECODE_EXT);
> > +   }
> >  }
> > diff --git a/src/vrend_renderer.c b/src/vrend_renderer.c
> > index e357bae..8b3e513 100644
> > --- a/src/vrend_renderer.c
> > +++ b/src/vrend_renderer.c
> > @@ -6571,7 +6571,7 @@ static void vrend_renderer_blit_int(struct
> > vrend_context *ctx,
> >        use_gl = !vrend_formats_can_blit(tex_conv_table, src_res-
> > >base.format, dst_res->base.format);
> > 
> >     if (use_gl) {
> > -      vrend_renderer_blit_gl(ctx, src_res, dst_res, info);
> > +      vrend_renderer_blit_gl(ctx, src_res, dst_res, info,
> > vrend_state.use_gles);
> >        vrend_clicbs->make_current(0, ctx->sub->gl_context);
> >        return;
> >     }
> > @@ -6690,6 +6690,13 @@ static void vrend_renderer_blit_int(struct
> > vrend_context *ctx,
> >        vrend_fb_bind_texture(dst_res, 0, info->dst.level, info-
> > >dst.box.z + i);
> >        glBindFramebuffer(GL_DRAW_FRAMEBUFFER, ctx->sub-
> > >blit_fb_ids[1]);
> > 
> > +      if (!vrend_state.use_gles) {
> > +         if (util_format_is_srgb(dst_res->base.format))
> > +            glEnable(GL_FRAMEBUFFER_SRGB);
> > +         else
> > +            glDisable(GL_FRAMEBUFFER_SRGB);
> > +      }
> > +
> >        glBindFramebuffer(GL_READ_FRAMEBUFFER, intermediate_fbo);
> > 
> >        glBlitFramebuffer(info->src.box.x,
> > diff --git a/src/vrend_renderer.h b/src/vrend_renderer.h
> > index 28e2819..c873c8c 100644
> > --- a/src/vrend_renderer.h
> > +++ b/src/vrend_renderer.h
> > @@ -388,7 +388,8 @@ bool vrend_formats_can_blit(struct
> > vrend_format_table *table, enum virgl_formats
> >  void vrend_renderer_blit_gl(struct vrend_context *ctx,
> >                              struct vrend_resource *src_res,
> >                              struct vrend_resource *dst_res,
> > -                            const struct pipe_blit_info *info);
> > +                            const struct pipe_blit_info *info,
> > +                            bool use_gles);
> > 
> >  void vrend_renderer_reset(void);
> >  int vrend_renderer_get_poll_fd(void);