Re: winex11: Use GLXFBConfig for the pixel format list

[Vitaliy Margolen]

Chris Robinson wrote:
> This greatly simplifies some of the wgl code by keeping around a single 
> customized fbconfig array, instead of holding a static, custom-typedef array 
> which left many functions to query glx for the fbconfig list anyway (which in 
> turn caused many needless, potentially problematic allocations and 
> deallocations of memory).
>
> This doesn't change the behavior of the code, but it makes it simpler for 
> future enhancements (such as supporting extra fbconfigs for on-screen or 
> off-screen use), and easier maintanence by removing a lot of duplicate code.

Please don't send compressed patches. It's not that huge to gzip it.
> -
>    WineGLFBConfigsListSize = 1;
> -
>    WineGLDisplayablePixelFormatListSize = 1;

If you hard-coding the list size to 1 it's not a list anymore, but a
single element.
> @@ -670,7 +718,7 @@ static int ConvertAttribWGLtoGLX(const i
>        switch (pop) {
>        case WGL_TYPE_COLORINDEX_ARB: pop = GLX_COLOR_INDEX_BIT; isColor = 1; break ;
>        case WGL_TYPE_RGBA_ARB: pop = GLX_RGBA_BIT; break ;
> -
>  case WGL_TYPE_RGBA_FLOAT_ATI: pop = GLX_RGBA_FLOAT_ATI_BIT; break ;
> -
>  case WGL_TYPE_RGBA_FLOAT_ARB: pop = GLX_RGBA_FLOAT_BIT; break ;
>  default:
>    ERR("unexpected PixelType(%x)\n", pop);
>    pop = 0;

This code seems like unrelated to the description. Please send it as a
separate patch.
> @@ -681,12 +729,7 @@ static int ConvertAttribWGLtoGLX(const i
>  case WGL_SUPPORT_GDI_ARB:
>    pop = iWGLAttr[++cur];
> -
>  /* We only support a limited number of formats which are all renderable by X (similar to GDI).
> -
>   * Ignore this attribute to prevent us from not finding a match due to the limited
> -
>   * amount of formats supported right now. This option could be matched to GLX_X_RENDERABLE
> -
>   * but the issue is that when a program asks for no GDI support, there's no format we can return
> -
>   * as all our supported formats are renderable by X.
> -
>   */
> -
>  PUSH2(oGLXAttr, GLX_X_RENDERABLE, pop);
>  TRACE("pAttr[%d] = WGL_SUPPORT_GDI_ARB: %d\n", cur, pop);
>  break;

Your description said nothing about altering this behavior stated in
comments.
Same for the PUSH, and lots of other changes. It seems to me that you
have at least 3-5 separate patches here. Please split them and send them
separately.
> @@ -1037,13 +987,13 @@ int X11DRV_ChoosePixelFormat(X11DRV_PDEV
>      /* When we pass all the checks we have found a matching format :) */
>      ret = 1;
>      TRACE("Successfully found a matching mode, returning index: %d\n", ret);
> +choose_exit:
> - ;
> }
>
> -choose_exit:
>    if(!ret)
>      TRACE("No matching mode was found returning 0\n");

Why do you need this change?
> @@ -1519,11 +1458,13 @@ BOOL X11DRV_wglMakeCurrent(X11DRV_PDEVIC
>               * We are certain that the drawable and context are compatible as we only allow compatible formats.
>               */
>              TRACE(" Creating GLX Context\n");
> -
>        ctx->ctx = pglXCreateContext(gdi_display, ctx->vis, NULL, type == OBJ_MEMDC ? False : True);
> -
>        create_glx_context(physDev, ctx, NULL);
>        TRACE(" created a delayed OpenGL context (%p)\n", ctx->ctx);
>    }
>    TRACE(" make current for dis %p, drawable %p, ctx %p\n", gdi_display, (void*) drawable, ctx->ctx);
> -
>    X11DRV_expect_error(gdi_display, error_catcher, NULL);
>    ret = pglXMakeCurrent(gdi_display, drawable, ctx->ctx);
> -
>    X11DRV_expect_error(gdi_display, NULL, NULL);
>    NtCurrentTeb()->glContext = ctx;
>    if(ret)
>    {

Can you explain this one a bit? Your error_catcher() does nothing,
except ignoring the error. Yet here you assuming that calls succeeded
and continue on, assigning the context to a thread which won't have the
context assigned.

Vitaliy.