Re: Patch: Xdamage support for Vino

[Mark McLoughlin <markmc redhat com>]

Hi Federico,

On Wed, 2004-05-26 at 20:22, Federico Mena Quintero wrote:
> Hi, Mark,
> 
> Here is a little patch to add Xdamage support to Vino.  With this, and
> if your X server supports the DAMAGE extension, Vino won't have to poll
> the screen periodically.  Instead, it will just get XDamageNotify events
> and update acoordingly.

	Thanks much for doing this - that's one thing to knock of the TODO list
:-)

>  #
>  # Check for XTest extension
>  #
> -XTEST_LIBS=
> -AC_CHECK_HEADER(X11/extensions/XTest.h, [
> -    AC_CHECK_LIB(Xtst, XTestQueryExtension, [
> -      AC_DEFINE(HAVE_XTEST)
> -      XTEST_LIBS="-lXtst"],, $X_LIBS)
> -  ])
> -AC_SUBST(XTEST_LIBS)
> +#XTEST_LIBS=
> +#AC_CHECK_HEADER(X11/extensions/XTest.h, [
> +#    AC_CHECK_LIB(Xtst, XTestQueryExtension, [
> +#      AC_DEFINE(HAVE_XTEST)
> +#      XTEST_LIBS="-lXtst"],, $X_LIBS)
> +#  ])
> +#AC_SUBST(XTEST_LIBS)

	Was the XTest detection not working for you?
  
> +#ifdef HAVE_DAMAGE
> +  Damage           xdamage;
> +  int              xdamage_event_base;

	Storing the calculated damage notify event number (i.e. event_base +
XDamageNotify) would be better since that's the only thing we use it
for.

> +  int              xdamage_error_base;

	You don't use this, I wouldn't bother keeping it around.

> +update_xrectangle (VinoFB *vfb, XRectangle *xrect)
> +{
> +  VinoFBPrivate *priv;
> +  gboolean use_shm;
> +  XShmSegmentInfo shm_info;
> +  XImage *image;
> +
> +  priv = vfb->priv;
> +
> +  use_shm = vino_fb_create_image (vfb, &image, &shm_info,
> +				  xrect->width, xrect->height,
> +				  priv->fb_image->bits_per_pixel);
> +  if (!image)
> +    {
> +      g_warning (G_STRLOC ": Could not create XImage; buffer will be out of sync");
> +      return;
> +    }
> +
> +  if (vino_fb_get_image (vfb, priv->root_window, image, &shm_info, use_shm,
> +			 xrect->x, xrect->y, xrect->width, xrect->height))
> +    copy_image_to_fb_image (vfb, image, xrect->x, xrect->y, xrect->width, xrect->height);
> +  else
> +    g_warning (G_STRLOC ": Could not fetch the XImage; buffer will be out of sync");
> +
> +  vino_fb_destroy_image (vfb, image, &shm_info, use_shm);
> +}

	It really worries me that we have to create an image (which is bounded
only be the screen size) for each damaged rectangle. Each one we have to
do:

  1) Allocate an XImage structure
  2) Allocate the number of shared memory pages required to hold the
     image (with 32 bits per pixel, on i386, a page holds 32x32 pixels)
  3) Map those pages into the processes address space
  4) Round trip to the X server to get it to map the segment (roundtip
     because we sync with the server)
  5) Round trip to get the server to copy the pixels
  6) Round trip to the server to get it to unmap the segment[1]
  7) Unmap the segment, which causes the pages to be de-allocated
  8) Free the XImage

	Now, given that we're going to have lots and lots of damage rectangles,
this is a hell of a lot churn on the VM. However, I don't have a clear
picture of how bad that is compared to the obvious alternative of
copying across the pixels in tiles - i.e doing (5) repeatedly - to a
pre-allocated shared image.

	Uggh, why on earth is there no XShmGetSubImage() ?

	One other option we could try is to use a single shared pixmap
(XShmCreatePixmap) for our copy of the framebuffer and use CopyArea to
update the damaged areas. That sounds feasible, right? The only downside
to that is that the shm and non-shm code paths would be very different,
but I'd nearly be tempted to dump the non-shm support ...


> +  region = XFixesCreateRegion (xdisplay, xrect, 1);
> +  XDamageSubtract (xdisplay, priv->xdamage, region, None);
> +  XFixesDestroyRegion (xdisplay, region);

	We shouldn't create a region for each rectangle, but create a single
region at init time and do XSetRegion()/SDamageSubtract() for each
rectangle.

> +  gdk_window_add_filter (priv->root_window, root_window_filter_cb, vfb);

	You're not removing the filter anywhere.

Cheers,
Mark.

[1] - Ouch! I've just realised we're *not* actually doing an
XShmDetach(), which means the server never unmapped the segment. Should
be fixed in CVS now ...