winex11: Use GLXFBConfig for the pixel format list

Vitaliy Margolen wine-devel at kievinfo.com
Mon Jan 1 02:47:41 CST 2007


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.



More information about the wine-devel mailing list