Re: [PATCH 4/5] wined3d: Add a setting for the maximum OpenGL version to use.

[Henri Verbeet]

On 6 July 2015 at 22:20, Matteo Bruni  wrote:
>      if (context_debug_output_enabled(gl_info))
> -
>        ctx_flags = WGL_CONTEXT_DEBUG_BIT_ARB;
> -
>    ctx_attribs[ctx_attrib_idx++] = WGL_CONTEXT_MAJOR_VERSION_ARB;
> -
>    ctx_attribs[ctx_attrib_idx++] = gl_info->selected_gl_version >> 16;
> -
>    ctx_attribs[ctx_attrib_idx++] = WGL_CONTEXT_MINOR_VERSION_ARB;
> -
>    ctx_attribs[ctx_attrib_idx++] = gl_info->selected_gl_version & 0xffff;
> -
>    if (gl_info->selected_gl_version >= MAKEDWORD_VERSION(3, 2))
> -
>        ctx_flags |= WGL_CONTEXT_FORWARD_COMPATIBLE_BIT_ARB;
> -
>    if (ctx_flags)
>    {
>        ctx_attribs[ctx_attrib_idx++] = WGL_CONTEXT_FLAGS_ARB;
> -
>        ctx_attribs[ctx_attrib_idx++] = WGL_CONTEXT_DEBUG_BIT_ARB;
> -
>        ctx_attribs[ctx_attrib_idx++] = ctx_flags;
>    }
>    ctx_attribs[ctx_attrib_idx] = 0;
>
>    if (!(ctx = gl_info->p_wglCreateContextAttribsARB(hdc, share_ctx, ctx_attribs)))
>    {
> -
>        ERR("Failed to create a WGL context.\n");
> -
>        context_release(ret);
> -
>        goto out;
> -
>        if (ctx_flags & WGL_CONTEXT_FORWARD_COMPATIBLE_BIT_ARB)
> -
>        {
> -
>            ctx_attribs[ctx_attrib_idx - 1] &= ~WGL_CONTEXT_FORWARD_COMPATIBLE_BIT_ARB;
> -
>            if (!(ctx = gl_info->p_wglCreateContextAttribsARB(hdc, share_ctx, ctx_attribs)))
> -
>            {
> -
>                ERR("Failed to create a WGL context.\n");
> -
>                context_release(ret);
> -
>                goto out;
> -
>            }
> -
>        }
>    }
> }

I think this is starting to become complex enough that we'd probably
benefit from a helper function to be able to share some of this with
wined3d_adapter_init().
> - glGetIntegerv(GL_CONTEXT_PROFILE_MASK, &context_profile);
> - if (glGetError() != GL_NO_ERROR)
> -
>    context_profile = 0;

Should this perhaps just check for
gl_info->p_wglCreateContextAttribsARB instead of eating errors?
> - for (i = 0; i < ARRAY_SIZE(supported_gl_versions); ++i)
> - {
> -
>    if (supported_gl_versions[i] <= wined3d_settings.max_gl_version)
> -
>    {
> -
>        if (supported_gl_versions[i] != wined3d_settings.max_gl_version)
> -
>            ERR_(winediag)("Requested GL version %u.%u which is unsupported, trying version %u.%u instead.\n",
> -
>                    wined3d_settings.max_gl_version >> 16, wined3d_settings.max_gl_version & 0xffff,
> -
>                    supported_gl_versions[i] >> 16, supported_gl_versions[i] & 0xffff);
> -
>        break;
> -
>    }
> - }
> - if (i == ARRAY_SIZE(supported_gl_versions))
> - {
> -
>    ERR_(winediag)("Requested invalid GL version %u.%u.\n",
> -
>            wined3d_settings.max_gl_version >> 16, wined3d_settings.max_gl_version & 0xffff);
> -
>    i = ARRAY_SIZE(supported_gl_versions) - 1;
> - }

This is a bit different than what I had in mind. I think
max_gl_version should just be a maximum, and e.g. specifying 4.4
should just result in 3.2 at the moment. I think the winediag message
should just happen if the registry setting is set, regardless of its
actual value. (Except possibly if it's equal to the default value.)
I.e. we also want to know if someone sets e.g. 3.2 while the default
is currently 1.0 when reporting a bug.
> -
>    if (!get_config_key(hkey, appkey, "MaxGLVersion", buffer, size))
> -
>    {
> -
>        unsigned int major, minor;
> -
> -
>        if (sscanf(buffer, "%u.%u", &major, &minor) == 2)
> -
>            wined3d_settings.max_gl_version = MAKEDWORD_VERSION(major, minor);
> -
>        else
> -
>            ERR_(winediag)("Couldn't parse the requested maximum OpenGL version.\n");
> -
>    }

I'd make this a DWORD, if nothing else for consistency with
MaxShaderModelVS etc.
Perhaps "MaxVersionGL" would also more consistent for the naming, I'm not sure.

[PATCH 4/5] wined3d: Add a setting for the maximum OpenGL version to use.

[Matteo Bruni]

2015-07-07 16:58 GMT+02:00 Henri Verbeet :
> On 6 July 2015 at 22:20, Matteo Bruni  wrote:
> >      if (context_debug_output_enabled(gl_info))
> > -
> >        ctx_flags = WGL_CONTEXT_DEBUG_BIT_ARB;
> > -
> >    ctx_attribs[ctx_attrib_idx++] = WGL_CONTEXT_MAJOR_VERSION_ARB;
> > -
> >    ctx_attribs[ctx_attrib_idx++] = gl_info->selected_gl_version >> 16;
> > -
> >    ctx_attribs[ctx_attrib_idx++] = WGL_CONTEXT_MINOR_VERSION_ARB;
> > -
> >    ctx_attribs[ctx_attrib_idx++] = gl_info->selected_gl_version & 0xffff;
> > -
> >    if (gl_info->selected_gl_version >= MAKEDWORD_VERSION(3, 2))
> > -
> >        ctx_flags |= WGL_CONTEXT_FORWARD_COMPATIBLE_BIT_ARB;
> > -
> >    if (ctx_flags)
> >    {
> >        ctx_attribs[ctx_attrib_idx++] = WGL_CONTEXT_FLAGS_ARB;
> > -
> >        ctx_attribs[ctx_attrib_idx++] = WGL_CONTEXT_DEBUG_BIT_ARB;
> > -
> >        ctx_attribs[ctx_attrib_idx++] = ctx_flags;
> >    }
> >    ctx_attribs[ctx_attrib_idx] = 0;
> >
> >    if (!(ctx = gl_info->p_wglCreateContextAttribsARB(hdc, share_ctx, ctx_attribs)))
> >    {
> > -
> >        ERR("Failed to create a WGL context.\n");
> > -
> >        context_release(ret);
> > -
> >        goto out;
> > -
> >        if (ctx_flags & WGL_CONTEXT_FORWARD_COMPATIBLE_BIT_ARB)
> > -
> >        {
> > -
> >            ctx_attribs[ctx_attrib_idx - 1] &= ~WGL_CONTEXT_FORWARD_COMPATIBLE_BIT_ARB;
> > -
> >            if (!(ctx = gl_info->p_wglCreateContextAttribsARB(hdc, share_ctx, ctx_attribs)))
> > -
> >            {
> > -
> >                ERR("Failed to create a WGL context.\n");
> > -
> >                context_release(ret);
> > -
> >                goto out;
> > -
> >            }
> > -
> >        }
> >    }
> > }
>
> I think this is starting to become complex enough that we'd probably
> benefit from a helper function to be able to share some of this with
> wined3d_adapter_init().
> > - glGetIntegerv(GL_CONTEXT_PROFILE_MASK, &context_profile);
> > - if (glGetError() != GL_NO_ERROR)
> > -
> >    context_profile = 0;
>
> Should this perhaps just check for
> gl_info->p_wglCreateContextAttribsARB instead of eating errors?

You mean just check whether wglCreateContextAttribsARB succeeded for a
GL version >= 3.2? That should work, although it might be a bit
awkward (but not necessarily more than the current code).
> > - for (i = 0; i < ARRAY_SIZE(supported_gl_versions); ++i)
> > - {
> > -
> >    if (supported_gl_versions[i] <= wined3d_settings.max_gl_version)
> > -
> >    {
> > -
> >        if (supported_gl_versions[i] != wined3d_settings.max_gl_version)
> > -
> >            ERR_(winediag)("Requested GL version %u.%u which is unsupported, trying version %u.%u instead.\n",
> > -
> >                    wined3d_settings.max_gl_version >> 16, wined3d_settings.max_gl_version & 0xffff,
> > -
> >                    supported_gl_versions[i] >> 16, supported_gl_versions[i] & 0xffff);
> > -
> >        break;
> > -
> >    }
> > - }
> > - if (i == ARRAY_SIZE(supported_gl_versions))
> > - {
> > -
> >    ERR_(winediag)("Requested invalid GL version %u.%u.\n",
> > -
> >            wined3d_settings.max_gl_version >> 16, wined3d_settings.max_gl_version & 0xffff);
> > -
> >    i = ARRAY_SIZE(supported_gl_versions) - 1;
> > - }
>
> This is a bit different than what I had in mind. I think
> max_gl_version should just be a maximum, and e.g. specifying 4.4
> should just result in 3.2 at the moment. I think the winediag message
> should just happen if the registry setting is set, regardless of its
> actual value. (Except possibly if it's equal to the default value.)
> I.e. we also want to know if someone sets e.g. 3.2 while the default
> is currently 1.0 when reporting a bug.

Yeah, I was unsure here and I like your suggestion.
> > -
> >    if (!get_config_key(hkey, appkey, "MaxGLVersion", buffer, size))
> > -
> >    {
> > -
> >        unsigned int major, minor;
> > -
> > -
> >        if (sscanf(buffer, "%u.%u", &major, &minor) == 2)
> > -
> >            wined3d_settings.max_gl_version = MAKEDWORD_VERSION(major, minor);
> > -
> >        else
> > -
> >            ERR_(winediag)("Couldn't parse the requested maximum OpenGL version.\n");
> > -
> >    }
>
> I'd make this a DWORD, if nothing else for consistency with
> MaxShaderModelVS etc.

Hmm, how would you expect the (major, minor) version to be encoded in
the registry DWORD? Same as MAKEDWORD_VERSION()?
> Perhaps "MaxVersionGL" would also more consistent for the naming, I'm not sure.

I'm not sure that's better overall but I'm not attached to
"MaxGLVersion" either. I'll go with "MaxVersionGL" unless I'll find it
to particularly hurt my eyes ;)

[PATCH 4/5] wined3d: Add a setting for the maximum OpenGL version to use.

[Henri Verbeet]

On 7 July 2015 at 18:37, Matteo Bruni  wrote:
> 2015-07-07 16:58 GMT+02:00 Henri Verbeet :
> > > - glGetIntegerv(GL_CONTEXT_PROFILE_MASK, &context_profile);
> > > - if (glGetError() != GL_NO_ERROR)
> > > -
> > >    context_profile = 0;
> >
> > Should this perhaps just check for
> > gl_info->p_wglCreateContextAttribsARB instead of eating errors?
>
> You mean just check whether wglCreateContextAttribsARB succeeded for a
> GL version >= 3.2? That should work, although it might be a bit
> awkward (but not necessarily more than the current code).

I actually meant "if (gl_info->p_wglCreateContextAttribsARB)", but
that doesn't work because the query is only supported on 3.2 or later
contexts. But we could probably just check "gl_version" against 3.2
instead.
> > I'd make this a DWORD, if nothing else for consistency with
> > MaxShaderModelVS etc.
>
> Hmm, how would you expect the (major, minor) version to be encoded in
> the registry DWORD? Same as MAKEDWORD_VERSION()?

We could, but there are of course other options as well. (E.g.
0x00030002, 0x0302, 0x32, 302, 32.) We could also have separate
entries for the major and minor numbers, although I suppose that's
awkward if one is set but not the other.

[PATCH 4/5] wined3d: Add a setting for the maximum OpenGL version to use.

[Matteo Bruni]

2015-07-07 19:12 GMT+02:00 Henri Verbeet :
> On 7 July 2015 at 18:37, Matteo Bruni  wrote:
> > 2015-07-07 16:58 GMT+02:00 Henri Verbeet :
> > > > - glGetIntegerv(GL_CONTEXT_PROFILE_MASK, &context_profile);
> > > > - if (glGetError() != GL_NO_ERROR)
> > > > -
> > > >    context_profile = 0;
> > >
> > > Should this perhaps just check for
> > > gl_info->p_wglCreateContextAttribsARB instead of eating errors?
> >
> > You mean just check whether wglCreateContextAttribsARB succeeded for a
> > GL version >= 3.2? That should work, although it might be a bit
> > awkward (but not necessarily more than the current code).
>
> I actually meant "if (gl_info->p_wglCreateContextAttribsARB)", but
> that doesn't work because the query is only supported on 3.2 or later
> contexts. But we could probably just check "gl_version" against 3.2
> instead.

Yeah, that's probably easier.
> > > I'd make this a DWORD, if nothing else for consistency with
> > > MaxShaderModelVS etc.
> >
> > Hmm, how would you expect the (major, minor) version to be encoded in
> > the registry DWORD? Same as MAKEDWORD_VERSION()?
>
> We could, but there are of course other options as well. (E.g.
> 0x00030002, 0x0302, 0x32, 302, 32.) We could also have separate
> entries for the major and minor numbers, although I suppose that's
> awkward if one is set but not the other.

It's a bit ugly in any way. I'll go with the MAKEDWORD_VERSION() style
first and see how it looks.

[PATCH 4/5] wined3d: Add a setting for the maximum OpenGL version to use.

[Matteo Bruni]

2015-07-07 19:43 GMT+02:00 Matteo Bruni :
> 2015-07-07 19:12 GMT+02:00 Henri Verbeet :
> > On 7 July 2015 at 18:37, Matteo Bruni  wrote:
> > > 2015-07-07 16:58 GMT+02:00 Henri Verbeet :
> > > > > - glGetIntegerv(GL_CONTEXT_PROFILE_MASK, &context_profile);
> > > > > - if (glGetError() != GL_NO_ERROR)
> > > > > -
> > > > >    context_profile = 0;
> > > >
> > > > Should this perhaps just check for
> > > > gl_info->p_wglCreateContextAttribsARB instead of eating errors?
> > >
> > > You mean just check whether wglCreateContextAttribsARB succeeded for a
> > > GL version >= 3.2? That should work, although it might be a bit
> > > awkward (but not necessarily more than the current code).
> >
> > I actually meant "if (gl_info->p_wglCreateContextAttribsARB)", but
> > that doesn't work because the query is only supported on 3.2 or later
> > contexts. But we could probably just check "gl_version" against 3.2
> > instead.
>
> Yeah, that's probably easier.

Actually no, we can't, I just remembered. Linux binary drivers offer
compatibility contexts and will return the latest supported GL version
even if you create a 1.0 context. We don't want to enable the wined3d
3.2 codepath there.

[PATCH 4/5] wined3d: Add a setting for the maximum OpenGL version to use.

[Henri Verbeet]

On 7 July 2015 at 23:43, Matteo Bruni  wrote:
> Actually no, we can't, I just remembered. Linux binary drivers offer
> compatibility contexts and will return the latest supported GL version
> even if you create a 1.0 context. We don't want to enable the wined3d
> 3.2 codepath there.

I don't think that should make a difference, they still shouldn't set
the core profile bit in that case. (But if they do, the current patch
would be just as broken.)

[PATCH 4/5] wined3d: Add a setting for the maximum OpenGL version to use.

[Matteo Bruni]

2015-07-08 9:17 GMT+02:00 Henri Verbeet :
> On 7 July 2015 at 23:43, Matteo Bruni  wrote:
> > Actually no, we can't, I just remembered. Linux binary drivers offer
> > compatibility contexts and will return the latest supported GL version
> > even if you create a 1.0 context. We don't want to enable the wined3d
> > 3.2 codepath there.
>
> I don't think that should make a difference, they still shouldn't set
> the core profile bit in that case. (But if they do, the current patch
> would be just as broken.)

They don't. I thought you were suggesting to remove the
GL_CONTEXT_PROFILE_MASK query entirely but now I see you were only
talking about the glGetError() part. Updated patch coming in a bit.