OGL: Only specify precision for sampler2DMSArray when it is defined

[Pokechu22]

See https://bugs.dolphin-emu.org/issues/13198 (where I wrote a bit about the history of the relevant dolphin code, and sampler2DMSArray in the GLES specs (though I haven't checked the main GL specs)) / https://gitlab.freedesktop.org/mesa/mesa/-/issues/8405. This change is untested, and the way we handle enabling extensions is a bit of a mess.

I'm also not sure if we should be disabling MSAA in some other way. Currently we say it's always enabled for ES 3.1 and ES 3.2, which seems odd. (Relatedly, though we check the max samples, we don't use that value and instead hardcode a list of them, though this might be related to needing an active context?)

Currently we don't check for support for multisample storage in OGLTexture (though I believe that existed in the past):

dolphin/Source/Core/VideoBackends/OGL/OGLTexture.cpp

Lines 130 to 144 in 560a239

	GLenum gl_internal_format = GetGLInternalFormatForTextureFormat(m_config.format, true);
	if (tex_config.IsMultisampled())
	{
	if (g_ogl_config.bSupportsTextureStorage)
	glTexStorage3DMultisample(target, tex_config.samples, gl_internal_format, m_config.width,
	m_config.height, m_config.layers, GL_FALSE);
	else
	glTexImage3DMultisample(target, tex_config.samples, gl_internal_format, m_config.width,
	m_config.height, m_config.layers, GL_FALSE);
	}
	else if (g_ogl_config.bSupportsTextureStorage)
	{
	glTexStorage3D(target, m_config.levels, gl_internal_format, m_config.width, m_config.height,
	m_config.layers);
	}

[phire]

though we check the max samples

Hmm... That only seems to be correct for OpenGL ES.
The documentation for Desktop OpenGL doesn't list GL_MAX_SAMPLES as a valid query for glGet. Though, I really suspect this is more of a bug in the spec: Why define GL_MAX_SAMPLES and reference it as a limit if it's not valid to query it?

Do we need to implement the method actually mentioned in the docs (querying GL_MAX_DEPTH_TEXTURE_SAMPLES GL_MAX_COLOR_TEXTURE_SAMPLES, GL_MAX_INTEGER_SAMPLES and taking the lowest)

though this might be related to needing an active context?

Other backends actually create a context just so they can fill out things like AA modes. Wouldn't be stupid to do the same here.

Currently we don't check for support for multisample storage in OGLTexture (though I believe that existed in the past):

Potentially worth fixing. But if you make sure AAModes only exposes supported modes, then VideoConfig::VerifyValidity() should stop any code ever trying to create a multisampled texture.

[phire]

Other things I noticed while skimming though OpenGL documentation:

There is a newer method for querying MSAA support starting from OpenGL 4.2, that allows you to query supported samples per type and internal format. glGetInternalformat. It also adds GL_SAMPLES, which returns a list of supported sample counts (though I really doubt any GPU, ignoring flipper, actually offers a non-power of two sample count)

Apparently if an implementation advertises support for MSAA, it must support at least 4xMSAA.

[Pokechu22]

The fifoci differences are due to 5.0-18820 randomly having a different result for xenoblade-menu, and aren't caused by this PR.

[alyssarosenzweig]

👍 on the PR

[alyssarosenzweig]

As for MSAA 8x, indeed not all drivers support it (Panfrost does not advertise this support for technical reasons, even though the hardware is capable of it). Would be nice to get that fixed as well but I don't think that blocks this change set (fixing the invalid shading language under ES3.1, which is orthogonal to fixing the sample count detection on any GL/ES).

[alyssarosenzweig]

+1 on the PR

(Did I miss why this is only draft? It looks good to me, with my graphics driver eyes and not necessarily my Dolphin eyes.)

[Pokechu22]

(Did I miss why this is only draft? It looks good to me, with my graphics driver eyes and not necessarily my Dolphin eyes.)

There's 3 parts that I'm not completely happy with:

• I think bSupportsOESMultisampleTextureStorage should be an enum instead, including checking GL_ARB_texture_storage_multisample or GL_OES_texture_storage_multisample_2d_array, but I'm not 100% sure what values that would take (in particular for when it's built-in on GLES 3.2 and desktop OGL).
• I still want to change the code in OGLTexture to handle the unsupported case better (or at least add some asserts)
• I generally don't like the way ProgramShaderCache::CreateHeader is implemented (I feel like concatenation would be a lot cleaner) - though I'll probably change that in a separate PR.

But most of these are small things that should be easy to fix, and not a problem with the main idea behind the PR.

[Pokechu22]

Oh, I never addressed this, but I'll be adding the glGetInternalformativ check in a separate PR after this one is merged. I've confirmed that it gives correct results on my machine, but refactoring will be needed to actually call it at the right time (doing it in PopulateConfig doesn't work as the UI doesn't get updated properly).

[stuartcamerondeakin]

Thanks so much for this change, finally got this branch building and running from EmulationStation on the Orange Pi 5 Ubuntu branch, I've posted a brief write-up for others to follow using that distribution with RetroPie until this gets merged. Joshua-Riek/ubuntu-rockchip#103

[alyssarosenzweig]

PR is both Reviewed-by: Alyssa Rosenzweig <alyssa@rosenzweig.io> and Tested-by: Alyssa Rosenzweig <alyssa@rosenzweig.io>. This avoids regressing Dolphin on Asahi when jumping from GLES3.0 to GLES3.1.

I know I have no authority to approve PRs in Dolphin but approving this one! 👅

[AdmiralCurtiss]

@Pokechu22 What's the status here?

[Pokechu22]

Both this and #11699 should be good to go (the latter could use another review for the most recent commits).