Re: [PATCH 2/5] winemac: Implement wglCreateContextAttribsARB.

[Ken Thomases]

On Jan 5, 2015, at 10:17 AM, Matteo Bruni <mbruni(a)codeweavers.com> wrote:
> +#if !defined(MAC_OS_X_VERSION_10_7) || MAC_OS_X_VERSION_MAX_ALLOWED < MAC_OS_X_VERSION_10_7
> +    if (core)
> +    {
> +        WARN("OS X version >= 10.7 is required to be able to create core contexts\n",
> +             err, CGLErrorString(err));

The "err, CGLErrorString(err)" arguments are superfluous.
> +        return FALSE;
> +    }
> +    if (core)
> +    {
> +        attribs[n++] = kCGLPFAOpenGLProfile;
> +        attribs[n++] = (int)kCGLOGLPVersion_3_2_Core;
> +    }

These lines won't compile when the SDK is earlier than 10.7.  So, they also need to be guarded with the appropriate #if.
> +            case WGL_CONTEXT_PROFILE_MASK_ARB:
> +                if ((value & ~VALID_PROFILE_BITS_MASK) || !value ||
> +                    (value & VALID_PROFILE_BITS_MASK) == VALID_PROFILE_BITS_MASK)

I think it would be better to simply do:

                if (value != WGL_CONTEXT_CORE_PROFILE_BIT_ARB &&
                    value != WGL_CONTEXT_COMPATIBILITY_PROFILE_BIT_ARB)

Or did I miss something about the logic you used?
> +                {
> +                    WARN("WGL_CONTEXT_PROFILE_MASK_ARB bits %#x invalid\n", value);
> +                    SetLastError(ERROR_INVALID_PROFILE_ARB);
> +                    return NULL;
> +                }
> +                profile = value;
> +                break;
> +    register_extension("WGL_ARB_create_context");
> +    register_extension("WGL_ARB_create_context_profile");
> +    opengl_funcs.ext.p_wglCreateContextAttribsARB = macdrv_wglCreateContextAttribsARB;

These should only be advertised when building against the 10.7 SDK or later.  So, this should be guarded by the SDK #if directive.

Also, given that it's possible to build against a 10.7+ SDK but target deployment back to 10.6, I'm tempted to say we should determine if core profiles are actually available and only advertise those extensions if so.  Basically, call CGLChoosePixelFormat() with the simplest set of attributes that specify the core profile and see if it succeeds.

-Ken

[PATCH 2/5] winemac: Implement wglCreateContextAttribsARB.

[Matteo Bruni]

2015-01-05 21:04 GMT+01:00 Ken Thomases <ken(a)codeweavers.com>:
> On Jan 5, 2015, at 10:17 AM, Matteo Bruni <mbruni(a)codeweavers.com> wrote:
> > +#if !defined(MAC_OS_X_VERSION_10_7) || MAC_OS_X_VERSION_MAX_ALLOWED < MAC_OS_X_VERSION_10_7
> > +    if (core)
> > +    {
> > +        WARN("OS X version >= 10.7 is required to be able to create core contexts\n",
> > +             err, CGLErrorString(err));
>
> The "err, CGLErrorString(err)" arguments are superfluous.
> > +        return FALSE;
> > +    }
> > +    if (core)
> > +    {
> > +        attribs[n++] = kCGLPFAOpenGLProfile;
> > +        attribs[n++] = (int)kCGLOGLPVersion_3_2_Core;
> > +    }
>
> These lines won't compile when the SDK is earlier than 10.7.  So, they also need to be guarded with the appropriate #if.
> > +            case WGL_CONTEXT_PROFILE_MASK_ARB:
> > +                if ((value & ~VALID_PROFILE_BITS_MASK) || !value ||
> > +                    (value & VALID_PROFILE_BITS_MASK) == VALID_PROFILE_BITS_MASK)
>
> I think it would be better to simply do:
>
> if (value != WGL_CONTEXT_CORE_PROFILE_BIT_ARB &&
>                     value != WGL_CONTEXT_COMPATIBILITY_PROFILE_BIT_ARB)
>
> Or did I miss something about the logic you used?

No, you're totally right. By following the WGL spec to the letter I
somehow lost sight of what I was actually doing...
> > +                {
> > +                    WARN("WGL_CONTEXT_PROFILE_MASK_ARB bits %#x invalid\n", value);
> > +                    SetLastError(ERROR_INVALID_PROFILE_ARB);
> > +                    return NULL;
> > +                }
> > +                profile = value;
> > +                break;
> > +    register_extension("WGL_ARB_create_context");
> > +    register_extension("WGL_ARB_create_context_profile");
> > +    opengl_funcs.ext.p_wglCreateContextAttribsARB = macdrv_wglCreateContextAttribsARB;
>
> These should only be advertised when building against the 10.7 SDK or later.  So, this should be guarded by the SDK #if directive.
>
> Also, given that it's possible to build against a 10.7+ SDK but target deployment back to 10.6, I'm tempted to say we should determine if core profiles are actually available and only advertise those extensions if so.  Basically, call CGLChoosePixelFormat() with the simplest set of attributes that specify the core profile and see if it succeeds.

Ah, I see. Sure I'll do that.
> -Ken

[PATCH 2/5] winemac: Implement wglCreateContextAttribsARB.

[Ken Thomases]

On Jan 5, 2015, at 2:04 PM, Ken Thomases <ken(a)codeweavers.com> wrote:
> On Jan 5, 2015, at 10:17 AM, Matteo Bruni <mbruni(a)codeweavers.com> wrote:
> > +    register_extension("WGL_ARB_create_context");
> > +    register_extension("WGL_ARB_create_context_profile");
> > +    opengl_funcs.ext.p_wglCreateContextAttribsARB = macdrv_wglCreateContextAttribsARB;
>
> These should only be advertised when building against the 10.7 SDK or later.  So, this should be guarded by the SDK #if directive.
>
> Also, given that it's possible to build against a 10.7+ SDK but target deployment back to 10.6, I'm tempted to say we should determine if core profiles are actually available and only advertise those extensions if so.  Basically, call CGLChoosePixelFormat() with the simplest set of attributes that specify the core profile and see if it succeeds.

Actually, after reviewing the WGL_ARB_create_context spec, I have changed my mind about this.  Those extensions and the entry point can still be advertised regardless of the SDK or the actual capabilities of the system.

However, wglCreateContextAttribsARB() should reject requests for anything which can't be supported because of the SDK or the capabilities of the system.

Probably the capabilities of the system with respect to core profile support should be tested once during initialization and recorded.

-Ken