Re: [PATCH v2 1/6] wined3d: Fix querying texture-related limits on core profile.

[Henri Verbeet]

On 7 February 2017 at 22:08, Matteo Bruni  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.

[PATCH v2 1/6] wined3d: Fix querying texture-related limits on core profile.

[Matteo Bruni]

2017-02-08 16:05 GMT+01:00 Henri Verbeet :
> On 7 February 2017 at 22:08, Matteo Bruni  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?

[PATCH v2 1/6] wined3d: Fix querying texture-related limits on core profile.

[Henri Verbeet]

On 8 February 2017 at 22:53, Matteo Bruni  wrote:
> 2017-02-08 16:05 GMT+01:00 Henri Verbeet :
> > On 7 February 2017 at 22:08, Matteo Bruni  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.