On 26/01/18 11:31, Antia Puentes wrote:
On 25/01/18 18:56, Roland Scheidegger wrote:
Am 25.01.2018 um 17:56 schrieb Roland Scheidegger:
Am 25.01.2018 um 16:30 schrieb Michel Dänzer:
On 2018-01-24 05:38 PM, Juan A. Suarez Romero wrote:
This fixes KHR-GL45.internalformat.renderbuffer.rgb9_e5.
---
src/mesa/main/fbobject.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/src/mesa/main/fbobject.c b/src/mesa/main/fbobject.c
index d23916d1ad7..c72204e11a0 100644
--- a/src/mesa/main/fbobject.c
+++ b/src/mesa/main/fbobject.c
@@ -1976,6 +1976,9 @@ _mesa_base_fbo_format(const struct
gl_context *ctx, GLenum internalFormat)
ctx->Extensions.ARB_texture_float) ||
_mesa_is_gles3(ctx) /* EXT_color_buffer_float */ )
? GL_RGBA : 0;
+ case GL_RGB9_E5:
+ return (_mesa_is_desktop_gl(ctx) &&
ctx->Extensions.EXT_texture_shared_exponent)
+ ? GL_RGB: 0;
case GL_ALPHA16F_ARB:
case GL_ALPHA32F_ARB:
return ctx->API == API_OPENGL_COMPAT &&
Unfortunately, this broke the "spec@arb_internalformat_query2@samples
and num_sample_counts pname checks" piglit tests with radeonsi and
llvmpipe, see below.
Any idea what might need to be done in Gallium to fix this?
32 bit failing case: pname = GL_NUM_SAMPLE_COUNTS, target =
GL_RENDERBUFFER, internalformat = GL_RGB9_E5, params[0] =
(1,GL_TRUE), supported=1
32 bit failing case: pname = GL_NUM_SAMPLE_COUNTS, target =
GL_TEXTURE_2D_MULTISAMPLE, internalformat = GL_RGB9_E5, params[0] =
(1,GL_TRUE), supported=1
32 bit failing case: pname = GL_NUM_SAMPLE_COUNTS, target =
GL_TEXTURE_2D_MULTISAMPLE_ARRAY, internalformat = GL_RGB9_E5,
params[0] = (1,GL_TRUE), supported=1
64 bit failing case: pname = GL_NUM_SAMPLE_COUNTS, target =
GL_RENDERBUFFER, internalformat = GL_RGB9_E5, params[0] =
(1,GL_TRUE), supported=1
64 bit failing case: pname = GL_NUM_SAMPLE_COUNTS, target =
GL_TEXTURE_2D_MULTISAMPLE, internalformat = GL_RGB9_E5, params[0] =
(1,GL_TRUE), supported=1
64 bit failing case: pname = GL_NUM_SAMPLE_COUNTS, target =
GL_TEXTURE_2D_MULTISAMPLE_ARRAY, internalformat = GL_RGB9_E5,
params[0] = (1,GL_TRUE), supported=1
PIGLIT: {"subtest": {"GL_NUM_SAMPLE_COUNTS" : "fail"}}
32 bit failing case: pname = GL_SAMPLES, target =
GL_RENDERBUFFER, internalformat = GL_RGB9_E5, params[0] =
(1,GL_TRUE), supported=1
32 bit failing case: pname = GL_SAMPLES, target =
GL_TEXTURE_2D_MULTISAMPLE, internalformat = GL_RGB9_E5, params[0] =
(1,GL_TRUE), supported=1
32 bit failing case: pname = GL_SAMPLES, target =
GL_TEXTURE_2D_MULTISAMPLE_ARRAY, internalformat = GL_RGB9_E5,
params[0] = (1,GL_TRUE), supported=1
64 bit failing case: pname = GL_SAMPLES, target =
GL_RENDERBUFFER, internalformat = GL_RGB9_E5, params[0] =
(1,GL_TRUE), supported=1
64 bit failing case: pname = GL_SAMPLES, target =
GL_TEXTURE_2D_MULTISAMPLE, internalformat = GL_RGB9_E5, params[0] =
(1,GL_TRUE), supported=1
64 bit failing case: pname = GL_SAMPLES, target =
GL_TEXTURE_2D_MULTISAMPLE_ARRAY, internalformat = GL_RGB9_E5,
params[0] = (1,GL_TRUE), supported=1
PIGLIT: {"subtest": {"GL_SAMPLES" : "fail"}}
PIGLIT: {"result": "fail" }
Purely coincidentally, I was trying to clean up the formatquery code
recently (should help some failures with r600 too), and I think these
cleanups would fix it.
Basically outright say "no" to target/pname combinations which don't
make sense rather than trying to find a format suitable for another
target and then asking the driver for the nonsense combination, plus
some other small bits like not validating things again (sometimes, a
third time...).
Albeit it will cause some breakage with the piglit test, which I
believe
is a test error, but that might be open for debate...
(For TEXTURE_BUFFER and the internalformat size/type queries, do you
return valid values or unsupported? The problem here is ARB_tbo says
you
can't get these values via the equivalent GetTexLevelParameter queries,
whereas with GL 3.1 you can. And internalformat_query2 says it returns
"the same information" as GetTexLevelParameter, albeit it's not
entirely
true in any case since the equivalent of the internalformat stencil
type
doesn't even exist. My stance would be that valid values should be
reported even without GL 3.1, but the piglit test thinks differently.)
Err, actually this won't fix it I suppose - because rgb9e5 now is a
valid fbo format. Was that commit really correct? It does not make sense
to me, rgb9e5 cannot be a fbo/renderable format. Or was this just
working around issues in formatquery.c (which I try to address with this
patch)?
I believe the commit is wrong, _mesa_base_fbo_format_ is used to
validate the internalformat
passed to RenderbufferStorage, which in the OpenGL 4.6 is said that
needs to be:
An INVALID_ENUM error is generated if internalformat is not one of the
color-renderable, depth-renderable, or stencil-renderable formats
defined in
section 9.4.
and that's exactly the error returned by the "renderbuffer_storage"
function if _mesa_base_fbo_format
returns 0.
RGB9_E5 is not renderable, that is stated in the same specification
(Bug 9338). Juan, I believe the CTS test is
wrong and this commit should be reverted.
This was discussed in
https://github.com/KhronosGroup/OpenGL-API/issues/32 and the conclusion
is that the format
is defined as color-renderable when EXT_texture_shared_exponent is
exposed, it would fail later during the framebuffer
completeness check for implementations which do not support rendering to
it (FRAMEBUFFER_UNSUPPORTED).
If there are no further comments, I would reapply this patch by the end
of the week,
adding the following wording to the commit message:
RGB9_E5 should be accepted by RenderbufferStorage if the
EXT_texture_shared_exponent is exposed. It is left to the
implementations to return GL_FRAMEBUFFER_UNSUPPORTED_EXT
when checking the framebuffer completeness if they do not
support rendering in this format.
Discussed in:
https://github.com/KhronosGroup/OpenGL-API/issues/32
I have sent a patch to Piglit to take the clarified behaviour into
account in the ARB_internalformat_query(2) tests.
Sorry for the noise.
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
