Re: [PATCH] winex11: Don't create a temporary context each time X11DRV_wglBindTexImageARB is called.

[Ken Thomases]

On Feb 26, 2016, at 5:29 PM, Miklós Máté  wrote:
> @@ -2320,6 +2322,7 @@ static BOOL X11DRV_wglDestroyPbufferARB( struct wgl_pbuffer *object )
>     TRACE("(%p)\n", object);
> pglXDestroyPbuffer(gdi_display, object->drawable);
> - pglXDestroyContext(gdi_display, object->tmp_context);

I think you need to check if object->tmp_context is non-zero before calling glXDestroyContext() to avoid a GLXBadContext error.
> -
>    if (!object->tmp_context) {
> -
>        object->tmp_context = pglXCreateNewContext(gdi_display, object->fmt->fbconfig, object->fmt->render_type, prev_context, True);
> -
>        object->prev_context = prev_context;
> -
>    } else if (object->prev_context != prev_context) {
> -
>        pglXDestroyContext(gdi_display, object->tmp_context);
> -
>        object->tmp_context = pglXCreateNewContext(gdi_display, object->fmt->fbconfig, object->fmt->render_type, prev_context, True);
> -
>        object->prev_context = prev_context;
> -
>    }

Those two branches are mostly the same.  This could be reduced to:

if (!object->tmp_context || object->prev_context != prev_context) {
            if (object->tmp_context)
                pglXDestroyContext(gdi_display, object->tmp_context);
            object->tmp_context = pglXCreateNewContext(gdi_display, object->fmt->fbconfig, object->fmt->render_type, prev_context, True);
            object->prev_context = prev_context;
        }

Is it necessary, when a context is destroyed, to go through all wgl_pbuffer structs looking for any whose prev_context matches the one being destroyed and clear it?  Otherwise, you risk having a stale reference that could be reused for a different context and not realize you need to recreate tmp_context.  Of course, enumerating all of them in a thread-safe manner is going to require a lot of added infrastructure and complexity.

-Ken

[PATCH] winex11: Don't create a temporary context each time X11DRV_wglBindTexImageARB is called.

[Matteo Bruni]

2016-02-29 3:59 GMT+01:00 Ken Thomases :
> On Feb 26, 2016, at 5:29 PM, Miklós Máté  wrote:
> > @@ -2320,6 +2322,7 @@ static BOOL X11DRV_wglDestroyPbufferARB( struct wgl_pbuffer *object )
> >     TRACE("(%p)\n", object);
> > pglXDestroyPbuffer(gdi_display, object->drawable);
> > - pglXDestroyContext(gdi_display, object->tmp_context);
>
> I think you need to check if object->tmp_context is non-zero before calling glXDestroyContext() to avoid a GLXBadContext error.
> > -
> >    if (!object->tmp_context) {
> > -
> >        object->tmp_context = pglXCreateNewContext(gdi_display, object->fmt->fbconfig, object->fmt->render_type, prev_context, True);
> > -
> >        object->prev_context = prev_context;
> > -
> >    } else if (object->prev_context != prev_context) {
> > -
> >        pglXDestroyContext(gdi_display, object->tmp_context);
> > -
> >        object->tmp_context = pglXCreateNewContext(gdi_display, object->fmt->fbconfig, object->fmt->render_type, prev_context, True);
> > -
> >        object->prev_context = prev_context;
> > -
> >    }
>
> Those two branches are mostly the same.  This could be reduced to:
>     if (!object->tmp_context || object->prev_context != prev_context) {
>         if (object->tmp_context)
>             pglXDestroyContext(gdi_display, object->tmp_context);
>         object->tmp_context = pglXCreateNewContext(gdi_display, object->fmt->fbconfig, object->fmt->render_type, prev_context, True);
>         object->prev_context = prev_context;
>     }
>
>
> Is it necessary, when a context is destroyed, to go through all wgl_pbuffer structs looking for any whose prev_context matches the one being destroyed and clear it?  Otherwise, you risk having a stale reference that could be reused for a different context and not realize you need to recreate tmp_context.  Of course, enumerating all of them in a thread-safe manner is going to require a lot of added infrastructure and complexity.

Good point, that's necessary in principle and it shouldn't be too hard
to do by keeping a wgl_pbuffers list (similarly to context_list for
wgl_context, you should probably reuse the same context_section
critical section for synchronization).

The other points brought up by Ken are valid too of course.

[PATCH] winex11: Don't create a temporary context each time X11DRV_wglBindTexImageARB is called.

[Miklós Máté]

On 02/29/2016 06:10 PM, Matteo Bruni wrote:
> 2016-02-29 3:59 GMT+01:00 Ken Thomases :
> > On Feb 26, 2016, at 5:29 PM, Miklós Máté  wrote:
> > > @@ -2320,6 +2322,7 @@ static BOOL X11DRV_wglDestroyPbufferARB( struct wgl_pbuffer *object )
> > >      TRACE("(%p)\n", object);
> > >  pglXDestroyPbuffer(gdi_display, object->drawable);
> > > - pglXDestroyContext(gdi_display, object->tmp_context);
> >
> > I think you need to check if object->tmp_context is non-zero before calling glXDestroyContext() to avoid a GLXBadContext error.

Mesa handles ctx=0 by doing nothing. I guess the blob drivers do the same.
> > > -
> > >    if (!object->tmp_context) {
> > > -
> > >        object->tmp_context = pglXCreateNewContext(gdi_display, object->fmt->fbconfig, object->fmt->render_type, prev_context, True);
> > > -
> > >        object->prev_context = prev_context;
> > > -
> > >    } else if (object->prev_context != prev_context) {
> > > -
> > >        pglXDestroyContext(gdi_display, object->tmp_context);
> > > -
> > >        object->tmp_context = pglXCreateNewContext(gdi_display, object->fmt->fbconfig, object->fmt->render_type, prev_context, True);
> > > -
> > >        object->prev_context = prev_context;
> > > -
> > >    }
> >
> > Those two branches are mostly the same.  This could be reduced to:
> >      if (!object->tmp_context || object->prev_context != prev_context) {
> >          if (object->tmp_context)
> >              pglXDestroyContext(gdi_display, object->tmp_context);
> >          object->tmp_context = pglXCreateNewContext(gdi_display, object->fmt->fbconfig, object->fmt->render_type, prev_context, True);
> >          object->prev_context = prev_context;
> >      }

I don't think this improves readability, but whatever.
> > Is it necessary, when a context is destroyed, to go through all wgl_pbuffer structs looking for any whose prev_context matches the one being destroyed and clear it?  Otherwise, you risk having a stale reference that could be reused for a different context and not realize you need to recreate tmp_context.  Of course, enumerating all of them in a thread-safe manner is going to require a lot of added infrastructure and complexity.
>
> Good point, that's necessary in principle and it shouldn't be too hard
> to do by keeping a wgl_pbuffers list (similarly to context_list for
> wgl_context, you should probably reuse the same context_section
> critical section for synchronization).
>
> The other points brought up by Ken are valid too of course.

If a context is destroyed, and a new context is created, what are the 
chances that the two pointers end up being the same?

MM

[PATCH] winex11: Don't create a temporary context each time X11DRV_wglBindTexImageARB is called.

[Matteo Bruni]

2016-02-29 23:20 GMT+01:00 Miklós Máté :
> On 02/29/2016 06:10 PM, Matteo Bruni wrote:
> > 2016-02-29 3:59 GMT+01:00 Ken Thomases :
> > > On Feb 26, 2016, at 5:29 PM, Miklós Máté  wrote:
> > > > @@ -2320,6 +2322,7 @@ static BOOL X11DRV_wglDestroyPbufferARB( struct
> > > > wgl_pbuffer *object )
> > > >      TRACE("(%p)\n", object);
> > > >  pglXDestroyPbuffer(gdi_display, object->drawable);
> > > > - pglXDestroyContext(gdi_display, object->tmp_context);
> > >
> > > I think you need to check if object->tmp_context is non-zero before
> > > calling glXDestroyContext() to avoid a GLXBadContext error.
>
> Mesa handles ctx=0 by doing nothing. I guess the blob drivers do the same.

Probably, but AFAIU the spec is vague enough to allow the error.
> > > > -
> > > >    if (!object->tmp_context) {
> > > > -
> > > >        object->tmp_context = pglXCreateNewContext(gdi_display,
> > > >
> > > > object->fmt->fbconfig, object->fmt->render_type, prev_context, True);
> > > > -
> > > >        object->prev_context = prev_context;
> > > > -
> > > >    } else if (object->prev_context != prev_context) {
> > > > -
> > > >        pglXDestroyContext(gdi_display, object->tmp_context);
> > > > -
> > > >        object->tmp_context = pglXCreateNewContext(gdi_display,
> > > >
> > > > object->fmt->fbconfig, object->fmt->render_type, prev_context, True);
> > > > -
> > > >        object->prev_context = prev_context;
> > > > -
> > > >    }
> > >
> > > Those two branches are mostly the same.  This could be reduced to:
> > >      if (!object->tmp_context || object->prev_context !=
> > >
> > > prev_context) {
> > >              if (object->tmp_context)
> > >                  pglXDestroyContext(gdi_display, object->tmp_context);
> > >              object->tmp_context = pglXCreateNewContext(gdi_display,
> > > object->fmt->fbconfig, object->fmt->render_type, prev_context, True);
> > >              object->prev_context = prev_context;
> > >          }
>
> I don't think this improves readability, but whatever.

I prefer Ken's version but it's certainly fine if you don't take this
suggestion.
> > > Is it necessary, when a context is destroyed, to go through all
> > > wgl_pbuffer structs looking for any whose prev_context matches the one being
> > > destroyed and clear it?  Otherwise, you risk having a stale reference that
> > > could be reused for a different context and not realize you need to recreate
> > > tmp_context.  Of course, enumerating all of them in a thread-safe manner is
> > > going to require a lot of added infrastructure and complexity.
> >
> > Good point, that's necessary in principle and it shouldn't be too hard
> > to do by keeping a wgl_pbuffers list (similarly to context_list for
> > wgl_context, you should probably reuse the same context_section
> > critical section for synchronization).
> >
> > The other points brought up by Ken are valid too of course.
>
> If a context is destroyed, and a new context is created, what are the
> chances that the two pointers end up being the same?

Definitely not impossible, when you take into account how memory
allocation usually works. Is keeping track of the wgl_pbuffer structs
more complicated than it looks?