Re: [PATCH v2 1/6] wined3d: Fix querying texture-related limits on core profile.
On 7 February 2017 at 22:08, Matteo Bruni <mbruni(a)codeweavers.com> wrote:
diff --git a/dlls/wined3d/directx.c b/dlls/wined3d/directx.c index c47052c..bfb018e 100644 --- a/dlls/wined3d/directx.c +++ b/dlls/wined3d/directx.c @@ -3405,24 +3405,35 @@ static void wined3d_adapter_init_limits(struct wined3d_gl_info *gl_info) } if (gl_info->supported[ARB_MULTITEXTURE]) { - gl_info->gl_ops.gl.p_glGetIntegerv(GL_MAX_TEXTURE_UNITS_ARB, &gl_max); - gl_info->limits.textures = min(MAX_TEXTURES, gl_max); - TRACE("Max textures: %d.\n", gl_info->limits.textures); + if (gl_info->supported[WINED3D_GL_LEGACY_CONTEXT]) + { + gl_info->gl_ops.gl.p_glGetIntegerv(GL_MAX_TEXTURE_UNITS_ARB, &gl_max); + gl_info->limits.textures = min(MAX_TEXTURES, gl_max); + TRACE("Max textures: %d.\n", gl_info->limits.textures);
- if (gl_info->supported[ARB_FRAGMENT_PROGRAM]) + if (gl_info->supported[ARB_FRAGMENT_PROGRAM]) + { + gl_info->gl_ops.gl.p_glGetIntegerv(GL_MAX_TEXTURE_COORDS_ARB, &gl_max); + gl_info->limits.texture_coords = min(MAX_TEXTURES, gl_max); + } + else + { + gl_info->limits.texture_coords = max(gl_info->limits.texture_coords, gl_max); + } + TRACE("Max texture coords: %d.\n", gl_info->limits.texture_coords); + } + + if (gl_info->supported[ARB_FRAGMENT_PROGRAM] || gl_info->supported[ARB_FRAGMENT_SHADER]) { GLint tmp; - gl_info->gl_ops.gl.p_glGetIntegerv(GL_MAX_TEXTURE_COORDS_ARB, &gl_max); - gl_info->limits.texture_coords = min(MAX_TEXTURES, gl_max); + gl_info->gl_ops.gl.p_glGetIntegerv(GL_MAX_TEXTURE_IMAGE_UNITS_ARB, &tmp); gl_info->limits.fragment_samplers = min(MAX_FRAGMENT_SAMPLERS, tmp); } else { - gl_info->limits.texture_coords = max(gl_info->limits.texture_coords, gl_max); gl_info->limits.fragment_samplers = max(gl_info->limits.fragment_samplers, gl_max); } - TRACE("Max texture coords: %d.\n", gl_info->limits.texture_coords); TRACE("Max fragment samplers: %d.\n", gl_info->limits.fragment_samplers);
if (gl_info->supported[ARB_VERTEX_SHADER]) This still doesn't really look logical to me.
2017-02-08 16:05 GMT+01:00 Henri Verbeet <hverbeet(a)gmail.com>:
On 7 February 2017 at 22:08, Matteo Bruni <mbruni(a)codeweavers.com> wrote:
diff --git a/dlls/wined3d/directx.c b/dlls/wined3d/directx.c index c47052c..bfb018e 100644 --- a/dlls/wined3d/directx.c +++ b/dlls/wined3d/directx.c @@ -3405,24 +3405,35 @@ static void wined3d_adapter_init_limits(struct wined3d_gl_info *gl_info) } if (gl_info->supported[ARB_MULTITEXTURE]) { - gl_info->gl_ops.gl.p_glGetIntegerv(GL_MAX_TEXTURE_UNITS_ARB, &gl_max); - gl_info->limits.textures = min(MAX_TEXTURES, gl_max); - TRACE("Max textures: %d.\n", gl_info->limits.textures); + if (gl_info->supported[WINED3D_GL_LEGACY_CONTEXT]) + { + gl_info->gl_ops.gl.p_glGetIntegerv(GL_MAX_TEXTURE_UNITS_ARB, &gl_max); + gl_info->limits.textures = min(MAX_TEXTURES, gl_max); + TRACE("Max textures: %d.\n", gl_info->limits.textures);
- if (gl_info->supported[ARB_FRAGMENT_PROGRAM]) + if (gl_info->supported[ARB_FRAGMENT_PROGRAM]) + { + gl_info->gl_ops.gl.p_glGetIntegerv(GL_MAX_TEXTURE_COORDS_ARB, &gl_max); + gl_info->limits.texture_coords = min(MAX_TEXTURES, gl_max); + } + else + { + gl_info->limits.texture_coords = max(gl_info->limits.texture_coords, gl_max); + } + TRACE("Max texture coords: %d.\n", gl_info->limits.texture_coords); + } + + if (gl_info->supported[ARB_FRAGMENT_PROGRAM] || gl_info->supported[ARB_FRAGMENT_SHADER]) { GLint tmp; - gl_info->gl_ops.gl.p_glGetIntegerv(GL_MAX_TEXTURE_COORDS_ARB, &gl_max); - gl_info->limits.texture_coords = min(MAX_TEXTURES, gl_max); + gl_info->gl_ops.gl.p_glGetIntegerv(GL_MAX_TEXTURE_IMAGE_UNITS_ARB, &tmp); gl_info->limits.fragment_samplers = min(MAX_FRAGMENT_SAMPLERS, tmp); } else { - gl_info->limits.texture_coords = max(gl_info->limits.texture_coords, gl_max); gl_info->limits.fragment_samplers = max(gl_info->limits.fragment_samplers, gl_max); } - TRACE("Max texture coords: %d.\n", gl_info->limits.texture_coords); TRACE("Max fragment samplers: %d.\n", gl_info->limits.fragment_samplers);
if (gl_info->supported[ARB_VERTEX_SHADER]) This still doesn't really look logical to me.
Okay, I'm still missing something here (probably I'm just being dense) so let me try to explain the general idea first. I want to avoid querying GL_MAX_TEXTURE_UNITS_ARB and GL_MAX_TEXTURE_COORDS_ARB on core contexts, those are old FFP pnames removed from core profile. The corresponding gl_info->limits fields "textures" and "texture_coords" indeed are only used in wined3d for GL FFP-related functionality. On the other hand, I still want to lookup the value of GL_MAX_TEXTURE_IMAGE_UNITS. That was introduced by both ARB_fragment_program and ARB_fragment_shader; since ARB_fragment_program usually isn't exposed on core context (but ARB_fragment_shader obviously is), keying that part on the availability of either of those extensions should do the right thing. Is there something wrong in this reasoning or is it about the actual patch?
On 8 February 2017 at 22:53, Matteo Bruni <matteo.mystral(a)gmail.com> wrote:
2017-02-08 16:05 GMT+01:00 Henri Verbeet <hverbeet(a)gmail.com>:
On 7 February 2017 at 22:08, Matteo Bruni <mbruni(a)codeweavers.com> wrote:
diff --git a/dlls/wined3d/directx.c b/dlls/wined3d/directx.c index c47052c..bfb018e 100644 --- a/dlls/wined3d/directx.c +++ b/dlls/wined3d/directx.c @@ -3405,24 +3405,35 @@ static void wined3d_adapter_init_limits(struct wined3d_gl_info *gl_info) } if (gl_info->supported[ARB_MULTITEXTURE]) { - gl_info->gl_ops.gl.p_glGetIntegerv(GL_MAX_TEXTURE_UNITS_ARB, &gl_max); - gl_info->limits.textures = min(MAX_TEXTURES, gl_max); - TRACE("Max textures: %d.\n", gl_info->limits.textures); + if (gl_info->supported[WINED3D_GL_LEGACY_CONTEXT]) + { + gl_info->gl_ops.gl.p_glGetIntegerv(GL_MAX_TEXTURE_UNITS_ARB, &gl_max); + gl_info->limits.textures = min(MAX_TEXTURES, gl_max); + TRACE("Max textures: %d.\n", gl_info->limits.textures);
- if (gl_info->supported[ARB_FRAGMENT_PROGRAM]) + if (gl_info->supported[ARB_FRAGMENT_PROGRAM]) + { + gl_info->gl_ops.gl.p_glGetIntegerv(GL_MAX_TEXTURE_COORDS_ARB, &gl_max); + gl_info->limits.texture_coords = min(MAX_TEXTURES, gl_max); + } + else + { + gl_info->limits.texture_coords = max(gl_info->limits.texture_coords, gl_max); + } + TRACE("Max texture coords: %d.\n", gl_info->limits.texture_coords); + } + + if (gl_info->supported[ARB_FRAGMENT_PROGRAM] || gl_info->supported[ARB_FRAGMENT_SHADER]) { GLint tmp; - gl_info->gl_ops.gl.p_glGetIntegerv(GL_MAX_TEXTURE_COORDS_ARB, &gl_max); - gl_info->limits.texture_coords = min(MAX_TEXTURES, gl_max); + gl_info->gl_ops.gl.p_glGetIntegerv(GL_MAX_TEXTURE_IMAGE_UNITS_ARB, &tmp); gl_info->limits.fragment_samplers = min(MAX_FRAGMENT_SAMPLERS, tmp); } else { - gl_info->limits.texture_coords = max(gl_info->limits.texture_coords, gl_max); gl_info->limits.fragment_samplers = max(gl_info->limits.fragment_samplers, gl_max); } - TRACE("Max texture coords: %d.\n", gl_info->limits.texture_coords); TRACE("Max fragment samplers: %d.\n", gl_info->limits.fragment_samplers);
if (gl_info->supported[ARB_VERTEX_SHADER]) This still doesn't really look logical to me.
Okay, I'm still missing something here (probably I'm just being dense) so let me try to explain the general idea first.
I want to avoid querying GL_MAX_TEXTURE_UNITS_ARB and GL_MAX_TEXTURE_COORDS_ARB on core contexts, those are old FFP pnames removed from core profile. The corresponding gl_info->limits fields "textures" and "texture_coords" indeed are only used in wined3d for GL FFP-related functionality. On the other hand, I still want to lookup the value of GL_MAX_TEXTURE_IMAGE_UNITS. That was introduced by both ARB_fragment_program and ARB_fragment_shader; since ARB_fragment_program usually isn't exposed on core context (but ARB_fragment_shader obviously is), keying that part on the availability of either of those extensions should do the right thing.
Is there something wrong in this reasoning or is it about the actual patch?
I think the use of "gl_max" in the else branch for the fragment sampler limit in particular is fragile. That's not entirely new, but I do think the patch makes it worse. Arguably the texture coordinate and (fixed-function) texture unit limits should be zero on core contexts. In the bigger picture though, perhaps it's time to make some of these limits internal to the specific fixed function and shader backend implementations, and only expose the common limits through struct wined3d_d3d_limits, to the extent they aren't already.
participants (2)
-
Henri Verbeet -
Matteo Bruni