[PATCH 1/2] opengl: don't create a tmp context for glCopyTexImage2D

[Miklós Máté]

On 02/24/2016 05:18 PM, Ken Thomases wrote:
> On Feb 23, 2016, at 11:37 AM, Miklós Máté <mtmkls at gmail.com> wrote:
>> It works fine without the tmp context, and Mesa's wgl state tracker doesn't
>> do that either. Theoretically wglBindTexImageARB is supposed to be a fast
>> render-to-texture method, and creating a new context is anything but fast.
> I don't know the history of this code, but there are comments and a FIXME strongly suggesting that, even as is, it doesn't completely work.  So, you need to do a thorough investigation for why the code was written this way.
That's because 1D and CUBE are not supported. KotOR only uses 2D, so 
it's fine. Also note that wglBindTexImageARB was originally intended to 
be zero-copy, but I don't think it's possible to implement it that way 
in glX.

>
> I note that use_render_texture_emulation is always true, these days, so maybe things have changed.  I'm not sure why that variable exists.
>
> Also see notes below.
>
>> diff --git a/dlls/winex11.drv/opengl.c b/dlls/winex11.drv/opengl.c
>> index 99befde..871cfc1 100644
>> --- a/dlls/winex11.drv/opengl.c
>> +++ b/dlls/winex11.drv/opengl.c
>> @@ -2922,10 +2922,8 @@ static BOOL X11DRV_wglBindTexImageARB( struct wgl_pbuffer *object, int iBuffer )
>>
>>      if (use_render_texture_emulation) {
>>          static BOOL initialized = FALSE;
>> -        int prev_binded_texture = 0;
>>          GLXContext prev_context;
>>          Drawable prev_drawable;
>> -        GLXContext tmp_context;
>>
>>          prev_context = pglXGetCurrentContext();
>>          prev_drawable = pglXGetCurrentDrawable();
>> @@ -2940,21 +2938,15 @@ static BOOL X11DRV_wglBindTexImageARB( struct wgl_pbuffer *object, int iBuffer )
>>          }
>>
>>          TRACE("drawable=%lx, context=%p\n", object->drawable, prev_context);
>> -        tmp_context = pglXCreateNewContext(gdi_display, object->fmt->fbconfig, object->fmt->render_type, prev_context, True);
>> -
>> -        opengl_funcs.gl.p_glGetIntegerv(object->texture_bind_target, &prev_binded_texture);
>>
>>          /* Switch to our pbuffer */
>> -        pglXMakeCurrent(gdi_display, object->drawable, tmp_context);
>> +        pglXMakeCurrent(gdi_display, object->drawable, prev_context);
> prev_context is precisely the current context.  So, there should be no need to make it current.
The name of that glX function is a bit misleading. It binds a new 
context and a new drawable at once. This time only the drawable is new, 
because we need to switch to the pbuffer to copy out its contents into a 
texture.

>
>> -        /* Make sure that the prev_binded_texture is set as the current texture state isn't shared between contexts.
>> -         * After that upload the pbuffer texture data. */
>> -        opengl_funcs.gl.p_glBindTexture(object->texture_target, prev_binded_texture);
>> +        /* Upload the pbuffer texture data. */
>>          opengl_funcs.gl.p_glCopyTexImage2D(object->texture_target, 0, object->use_render_texture, 0, 0, object->width, object->height, 0);
>>
>>          /* Switch back to the original drawable and upload the pbuffer-texture */
>>          pglXMakeCurrent(gdi_display, prev_drawable, prev_context);
> It shouldn't be necessary to "restore" prev_context as the current context, since you never switched to a temporary context.  The prev_context and prev_drawable variables should go away entirely.
This one changes back to the original drawable (i.e. the screen), the 
context stays the same.

MM

>
> It seems to me that this entire function boils down to little more than a wrapper around glCopyTexImage2D().  I'm not familiar enough with wglBindTexImageARB() to know if that makes sense.
>
> -Ken
>