[virglrenderer-devel] [PATCH] blitter: Set fbo sRGB state according to the destination sourface type
Robert Tarasov
tutankhamen at chromium.org
Fri Jul 20 21:14:46 UTC 2018
Looks ok for me. Couple questions:
1. Is there any reason to have "glEnable(GL_FRAMEBUFFER_SRGB);" inside loop?
2. Would it be better to restore old GL_FRAMEBUFFER_SRGB state instead of
disabling it?
On Thu, Jul 19, 2018 at 5:03 AM Gert Wollny <gert.wollny at collabora.com>
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);
> --
> 2.17.1
>
> _______________________________________________
> virglrenderer-devel mailing list
> virglrenderer-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/virglrenderer-devel
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/virglrenderer-devel/attachments/20180720/4bcd0ed5/attachment.html>
More information about the virglrenderer-devel
mailing list