[Mesa-dev] [PATCH v5 0/3] Add and enable extension EXT_sRGB_write_control

[Ilia Mirkin]

On Tue, Nov 6, 2018 at 10:27 AM Gert Wollny <gert.wollny at collabora.com> wrote:
>
> Am Dienstag, den 06.11.2018, 09:29 -0500 schrieb Ilia Mirkin:
> > On Tue, Nov 6, 2018 at 5:59 AM Gert Wollny <gert.wollny at collabora.com
> > > wrote:
> > >
> > >
> > > I'm not so sure that things break, at least we have all the dEQP
> > > master tests that are enabled by the available extension (level
> > > GLES 3.2+) passing on virgl, which would indicate that what is done
> > > in mesa and virglrender is sufficiently correct for running a gles
> > > 3.2 environment on top of a gles 3.2 host also without
> > > EXT_sRGB_write_control
> > > (obviously also not using this functionbality in the guest).
> >
> > Well, have a look at the code, and see if you can get it to trigger
> > with a SRGB format. I suspect there's just no test that hits that
> > particular scenario. Or perhaps you're 100% fallback-path driven on
> > virgl?
> Ther is no fallback, when the guest uses
> glEnable/Disable(GL_FRAMEBUFFER_EXT) and the host doesn't support it,
> and there it also fails. This is to be expected and only shows that the
> guest shouldn't expose EXT_framebuffer_sRGB solely based on available
> formats.

I mean in the code, there's a "goto fallback" in various situations,
which does the operation on the CPU. If you're 100% in that path, then
you won't have issues.

>
> > In that case, that particular code is fine, but I didn't do a
> > full audit of the codebase. These tests would presumably be failing
> > already, since the issue is that you *don't* have
> > EXT_sRGB_write_control on the host and are trying to implement SRGB
> > formats.
> That was my guess too. I think if that code would do something wrong
> without the guest explicitely en/disabling GL_FRAMEBUFFER_SRGB we would
> have seen it.

sRGB issues can be subtle. I doubt there's strong test coverage of all
the various cases, and visually scenes just appear "dark", not
"totally broken". I'd encourage you to trace the code and try to
construct a test case that hits the path I pointed out as problematic.
(iirc the blit in st_TexSubImage, which uses a normalized dst format,
but I don't have the code in front of me)

>
> [...]
> > > I can now also understand your point of view that
> > > EXT_sRGB_write_control and EXT_framebuffer_sRGB constitute the same
> > > functionality, but then there is still some flag missing that
> > > indicates for GLES that sRGB formats are available, but write
> > > control isn't,
> > > and using only a texture format test to identify whether
> > > EXT_framebuffer_sRGB is supported is not sufficint for the scenario
> > > above (which brings me back to actually using a CAP to indicate
> > > whether the "hardware" supports sRGB write control.).
> >
> > I'd go the other way -- have a cap that shows that you CANT support
> > surface formats differing from the resource format. The current
> > implication is that this is always supported. There's a handful of
> > such anti-CAP's, and given that this is a violation of the existing
> > gallium interface (where the surface format != resource format is
> > generally supported), I think that's the way to go.
> Hmm, currently mesa exposes GLES 3.0 based on EXT_framebuffer_sRGB,
> which is more then is needed, and EXT_texture_sRGB is not enough, so I
> need one more flag that reflect the GLES requirement of format support
> without the need to support the format switching, probably in gl_const,
> because this is what is passed to mesa/compute_version_e2(...).

I'm not as intimately familiar with GLES extensions / features, but
would EXT_sRGB be the extension that covers that support? Mesa does
not presently implement this extension, but if it's the right one,
perhaps adding an implementation of that, and then doing something
like

EXT_sRGB = is_format_supported(SRGB)
EXT_framebuffer_sRGB / ARB_framebuffer_sRGB / EXT_sRGB_write_control =
is_format_supported(SRGB) &&
!PIPE_CAP_REQUIRE_SURFACE_RESOURCE_FORMAT_MATCH
GLES3 = EXT_sRGB (& others, obviously)

And that PIPE_CAP would then in turn only ever be "enabled" by virgl
when running on a host without EXT_sRGB_write_control (or equivalent)
support.

Cheers,

  -ilia