[virglrenderer-devel] [PATCH] blitter: Set fbo sRGB state according to the destination sourface type
Robert Tarasov
tutankhamen at chromium.org
Tue Jul 24 20:58:27 UTC 2018
Awesome!
On Sun, Jul 22, 2018 at 12:03 AM Gert Wollny <gert.wollny at collabora.com>
wrote:
> 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);
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/virglrenderer-devel/attachments/20180724/6e4a5c16/attachment.html>
More information about the virglrenderer-devel
mailing list