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

[Matteo Bruni]

2016-02-24 17:18 GMT+01:00 Ken Thomases <ken at codeweavers.com>:
> 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.

Yes, it looks like the current code doesn't handle a number of
use-cases from WGL_ARB_render_texture, at least all the cube map stuff
and mipmapping (actually WGL_MIPMAP_TEXTURE_ARB is completely ignored,
without any FIXME :/). That is orthogonal to this patch though.

> 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.

Just historical reasons I think.

>> 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();

Unrelated to your patch but those two probably could be fetched from
the current struct wgl_context (NtCurrentTeb()->glContext) instead,
avoiding a couple of GLX calls.

>> -        /* 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);

Nitpick, I don't think "upload" is entirely correct here. "Copy" would
be fine but I'd just drop the comment altogether.

>>         /* Switch back to the original drawable and upload the pbuffer-texture */
>>         pglXMakeCurrent(gdi_display, prev_drawable, prev_context);

That comment is wrong, nothing is uploaded here. I'd get rid of it too.

> 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.

It does make some sense. The current implementation is pretty limited
but it should work fine for the 2D + no mipmapping use case.
FWIW there is an almost equivalent GLX extension called
GLX_EXT_texture_from_pixmap but that, as the name implies, only works
with GLXPixmap drawables. One could imagine to implement
WGL_ARB_render_texture with that + GLXPixmap instead of GLXPbuffer
drawables but I don't see it happening any time soon...

Anyway, the patch looks fine to me. I'm going to sign this off as soon
as I manage to give it a quick smoke test (which has been "any minute
now" for the last few hours -_-).
If you want to resend the patch with the couple of comment changes I
suggested that's cool. If you also feel like writing some more patches
to address the other issues with this code, by all means!