Re: [PATCH 1/2] opengl: don't create a tmp context for glCopyTexImage2D
On Feb 23, 2016, at 11:37 AM, Miklós Máté <mtmkls(a)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. 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.
- /* 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. 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
On 02/24/2016 05:18 PM, Ken Thomases wrote:
On Feb 23, 2016, at 11:37 AM, Miklós Máté <mtmkls(a)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
On Feb 24, 2016, at 12:19 PM, Miklós Máté <mtmkls(a)gmail.com> wrote:
On 02/24/2016 05:18 PM, Ken Thomases wrote:
- 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.
Oops, right. Sorry about that. -Ken
2016-02-24 17:18 GMT+01:00 Ken Thomases <ken(a)codeweavers.com>:
On Feb 23, 2016, at 11:37 AM, Miklós Máté <mtmkls(a)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!
Ah, I think I see now why this is using a temporary context: there is no guarantee that the current GL context is compatible with the pbuffer. That means a glXMakeCurrent() for the pbuffer and the current context might fail thus a temporary context is probably necessary in the general case after all. There is no reason to create a new context each time wglBindTexImageARB is called though. You could store the temporary GL context into struct wgl_pbuffer and reuse it for all the following calls with the same pbuffer (as long as the current context stays the same - if it changes the temporary context needs to be recreated so that it's shared with the "new" current context). The temporary context will have to be destroyed when the (WGL) pbuffer is destroyed. When you're resending please change the patch subject to something like "winex11: Don't create a temporary context each time X11DRV_wglBindTexImageARB is called."
On 02/25/2016 01:23 PM, Matteo Bruni wrote:
Ah, I think I see now why this is using a temporary context: there is no guarantee that the current GL context is compatible with the pbuffer. That means a glXMakeCurrent() for the pbuffer and the current context might fail thus a temporary context is probably necessary in the general case after all.
There is no reason to create a new context each time wglBindTexImageARB is called though. You could store the temporary GL context into struct wgl_pbuffer and reuse it for all the following calls with the same pbuffer (as long as the current context stays the same - if it changes the temporary context needs to be recreated so that it's shared with the "new" current context). The temporary context will have to be destroyed when the (WGL) pbuffer is destroyed.
When you're resending please change the patch subject to something like "winex11: Don't create a temporary context each time X11DRV_wglBindTexImageARB is called." Actually, GL contexts are quite flexible at working with different draw buffers, but you're right, it might be incompatible (however, this is extremely unlikely). I was afraid that going back to using a tmp context would re-introduce the MSAA problem, but it seems that was fixed by my patch to Mesa, and not this one :) I should have tested it better. I'll post the new patch shortly.
MM
participants (3)
-
Ken Thomases -
Matteo Bruni -
Miklós Máté