[Libva] [PATCH] x11: check x pixmap is still alive before DRI2DestroyDrawable

[Xiang, Haihao]

On Wed, 2014-11-26 at 13:03 +0000, Lionel Landwerlin wrote:
> Calling vaTerminate() after the pixmaps, given to vaPutSurface(), have
> been destroyed results in errors coming from the X server.
> 
> These errors happens because LibVA tries to destroy the DRI2Drawables
> related to the XPixmaps passed to vaPutSurface(). VA_DRI2Error() has
> some code to catch these errors and ignore them. The problem is that
> LibX11 doesn't trigger the extension error handlers unless you're
> waiting for a response using _XReply(), and there is no defined answer
> to a DRI2DestroyDrawable message in the DRI2 protocol.
> 
> The attached patch tries to work around that issue by checking whether
> the XPixmap is still alive before proceeding with DRI2DestroyDrawable.
> 
> Signed-off-by: Lionel Landwerlin <lionel.g.landwerlin at intel.com>
> ---
>  va/x11/va_dri2.c | 57 ++++++++++++++++++++++++++++++++++++++++++++------------
>  1 file changed, 45 insertions(+), 12 deletions(-)
> 
> diff --git a/va/x11/va_dri2.c b/va/x11/va_dri2.c
> index 1c91bca..da354eb 100644
> --- a/va/x11/va_dri2.c
> +++ b/va/x11/va_dri2.c
> @@ -70,18 +70,30 @@ static XEXT_GENERATE_FIND_DISPLAY (DRI2FindDisplay, va_dri2Info,
>  				   &va_dri2ExtensionHooks, 
>  				   0, NULL)
>  
> +static CARD32 _va_resource_x_error_drawable = 0;
> +static Bool   _va_resource_x_error_matched = False;
> +
> +#define VA_EnterResourceError(drawable)                 \
> +  do {                                                  \
> +    _va_resource_x_error_drawable = (drawable);         \
> +    _va_resource_x_error_matched = False;               \
> +  } while (0)
> +
> +#define VA_LeaveResourceError()                 \
> +  do {                                          \
> +    _va_resource_x_error_drawable = 0;          \
> +  } while (0)
> +
> +#define VA_ResourceErrorMatched()               \
> +  (_va_resource_x_error_matched)
> +
>  static int
>  VA_DRI2Error(Display *dpy, xError *err, XExtCodes *codes, int *ret_code)
>  {
> -    /*
> -     * If the X drawable was destroyed before the VA drawable, the DRI2 drawable
> -     * will be gone by the time we call VA_DRI2DestroyDrawable(). So, simply
> -     * ignore BadDrawable errors in that case.
> -     */
> -    if (err->majorCode == codes->major_opcode &&
> -        err->errorCode == BadDrawable &&
> -        err->minorCode == X_DRI2DestroyDrawable)
> -	return True;
> +    if (_va_resource_x_error_drawable == err->resourceID) {
> +      _va_resource_x_error_matched = True;
> +      return True;
> +    }
>  
>      return False;
>  }
> @@ -229,11 +241,29 @@ void VA_DRI2DestroyDrawable(Display *dpy, XID drawable)
>  {
>      XExtDisplayInfo *info = DRI2FindDisplay(dpy);
>      xDRI2DestroyDrawableReq *req;
> +    unsigned int attachement = 0; // FRONT_LEFT
> +    VA_DRI2Buffer *buffers;
>  
>      XextSimpleCheckExtension (dpy, info, va_dri2ExtensionName);
>  
>      XSync(dpy, False);
>  
> +    /*
> +     * We have no way of catching DRI2DestroyDrawable errors because
> +     * this message doesn't have a defined answer. So we test whether
> +     * the drawable is still alive by sending DRIGetBuffers first and
> +     * checking whether we get an error.
> +     */
> +    VA_EnterResourceError(drawable);
> +    buffers = VA_DRI2GetBuffers(dpy, drawable,
> +                                NULL, NULL,
> +                                &attachement, 1, NULL);
> +    VA_LeaveResourceError();
> +    if (buffers)
> +      XFree(buffers);
> +    if (VA_ResourceErrorMatched())
> +      return;
> +

It is possible that the drawable is destroyed after VA_DRI2GetBuffers()
in another thread.

>      LockDisplay(dpy);
>      GetReq(DRI2DestroyDrawable, req);
>      req->reqType = info->codes->major_opcode;
> @@ -274,9 +304,12 @@ VA_DRI2Buffer *VA_DRI2GetBuffers(Display *dpy, XID drawable,
>  	return NULL;
>      }
>  
> -    *width = rep.width;
> -    *height = rep.height;
> -    *outCount = rep.count;
> +    if (width)
> +      *width = rep.width;
> +    if (height)
> +      *height = rep.height;
> +    if (outCount)
> +      *outCount = rep.count;
>  
>      buffers = Xmalloc(rep.count * sizeof buffers[0]);
>      if (buffers == NULL) {