[Xprint] x11perf support for printer devices

Roland Mainz roland.mainz at nrubsig.org
Sun Apr 24 00:31:34 EDT 2005


Julien Lafon wrote:
> I have spend some time to evaluate print job generation performance
> and added support for printer devices to x11perf. It would be nice if
> Roland or AlanC could comment on the patch here before I attach it to
> a bugzilla rfe.

OK.. some comments on the patch:
-- snip --
> Index: Imakefile
> ===================================================================
> RCS file: /cvs/xorg/xc/programs/x11perf/Imakefile,v
> retrieving revision 1.4
> diff -u -r1.4 Imakefile
> --- Imakefile   11 Aug 2004 08:05:31 -0000      1.4
> +++ Imakefile   22 Apr 2005 21:40:21 -0000
> @@ -12,19 +12,24 @@
>  #if HasShm
>          SHMDEFS = -DMITSHM
>  #endif
> -#if BuildRenderLibrary
> +#if xBuildRenderLibrary
>      XRENDERDEFS = -DXRENDER
>      XRENDERDEPS = $(DEPXRENDERLIB)
>      XRENDERLIBS = $(XRENDERLIB)
>      XRENDERINCS = $(XRENDERINCLUDES)
>  #endif
> -#if BuildXftLibrary
> +#if xBuildXftLibrary

Uhm... is this debugging code or what ?

[snip]
> Index: do_blt.c
> ===================================================================
> RCS file: /cvs/xorg/xc/programs/x11perf/do_blt.c,v
> retrieving revision 1.2
> diff -u -r1.2 do_blt.c
> --- do_blt.c    23 Apr 2004 19:54:38 -0000      1.2
> +++ do_blt.c    22 Apr 2005 21:40:23 -0000
> @@ -36,29 +36,29 @@
>  #define NegMod(x, y) ((y) - (((-x)-1) % (7)) - 1)
>  
>  static void 
> -InitBltLines(void)
> +InitBltLines(XParms xp)
>  {
>      int i, x, y;
>  
>      points[0].x = points[0].y = y = 0;
>      for (i = 1; i != NUMPOINTS/2; i++) {    
>         if (i & 1) {
> -           points[i].x = WIDTH-1;
> +           points[i].x = xp->test_width-1;
>         } else {
>             points[i].x = 0;
>         }
> -       y += HEIGHT / (NUMPOINTS/2);
> +       y += xp->test_height / (NUMPOINTS/2);
>         points[i].y = y;
>      }
>      
>      x = 0;
>      for (i = NUMPOINTS/2; i!= NUMPOINTS; i++) {
>         if (i & 1) {
> -           points[i].y = HEIGHT-1;
> +           points[i].y = xp->test_height-1;
>         } else {
>             points[i].y = 0;
>         }
> -       x += WIDTH / (NUMPOINTS/2);
> +       x += xp->test_width / (NUMPOINTS/2);
>         points[i].x = x;
>      }
>  }
> @@ -66,7 +66,7 @@
>  int 
>  InitScroll(XParms xp, Parms p, int reps)
>  {
> -    InitBltLines();
> +    InitBltLines(xp);
>      XDrawLines(xp->d, xp->w, xp->fggc, points, NUMPOINTS, CoordModeOrigin);
>      return reps;
>  }
> @@ -93,18 +93,18 @@
>         XCopyArea(xp->d, xp->w, xp->w, xp->fggc, x, y + delta,
>             size, size, x, y);
>         y += size;
> -       if (y + size + delta > HEIGHT) {
> +       if (y + size + delta > xp->test_height) {
>             yorg += delta;
> -           if (yorg >= size || yorg + size + delta > HEIGHT) {
> +           if (yorg >= size || yorg + size + delta > xp->test_height) {
>                 yorg = 0;
>                 xorg++;
> -               if (xorg >= size || xorg + size > WIDTH) {
> +               if (xorg >= size || xorg + size > xp->test_width) {
>                     xorg = 0;
>                 }
>             }
>             y = yorg;
>             x += size;
> -           if (x + size > WIDTH) {
> +           if (x + size > xp->test_width) {
>                 x = xorg;
>             }
>         }
> @@ -140,8 +140,8 @@
>      xinc = (size & ~3) + 1;
>      yinc = xinc + 3;
>  
> -    width = (WIDTH - size) & ~31;
> -    height = (HEIGHT - size) & ~31;
> +    width = (xp->test_width - size) & ~31;
> +    height = (xp->test_height - size) & ~31;
>      
>      x1 = 0;
>      y1 = 0;
> @@ -199,10 +199,10 @@
>      (void) InitCopyWin(xp, p, reps);
>  
>      /* Create pixmap to write stuff into, and initialize it */
> -    pix = XCreatePixmap(xp->d, xp->w, WIDTH, HEIGHT, xp->vinfo.depth);
> +    pix = XCreatePixmap(xp->d, xp->w, xp->test_width, xp->test_height, xp->vinfo.depth);
>      pixgc = XCreateGC(xp->d, pix, 0, 0);
>      /* need a gc with GXcopy cos pixmaps contain junk on creation. mmm */
> -    XCopyArea(xp->d, xp->w, pix, pixgc, 0, 0, WIDTH, HEIGHT, 0, 0);
> +    XCopyArea(xp->d, xp->w, pix, pixgc, 0, 0, xp->test_width, xp->test_height, 0, 0);
>      XFreeGC(xp->d, pixgc);
>      return reps;
>  }
> @@ -212,9 +212,12 @@
>  {
>      (void) InitCopyWin(xp, p, reps);
>  
> +#if 1
> +p->font=1;

Another piece of debug code ?

>      /* Create image to stuff bits into */
> -    image = XGetImage(xp->d, xp->w, 0, 0, WIDTH, HEIGHT, xp->planemask,
> +    image = XGetImage(xp->d, xp->w, 0, 0, xp->test_width, xp->test_height, xp->planemask,
>                       p->font==0?ZPixmap:XYPixmap);
> +#endif
>      if(image==0){
>         printf("XGetImage failed\n");
>         return False;
> @@ -523,18 +526,18 @@
>      XGCValues   gcv;
>      GC         pixgc;
>  
> -    InitBltLines();
> +    InitBltLines(xp);
>      InitCopyLocations(xp, p, reps);
>  
>      /* Create pixmap to write stuff into, and initialize it */
> -    pix = XCreatePixmap(xp->d, xp->w, WIDTH, HEIGHT, 
> +    pix = XCreatePixmap(xp->d, xp->w, xp->test_width, xp->test_height, 
>             p->font==0 ? 1 : xp->vinfo.depth);
>      gcv.graphics_exposures = False;
>      gcv.foreground = 0;
>      gcv.background = 1;
>      pixgc = XCreateGC(xp->d, pix, 
>                 GCForeground | GCBackground | GCGraphicsExposures, &gcv);
> -    XFillRectangle(xp->d, pix, pixgc, 0, 0, WIDTH, HEIGHT);
> +    XFillRectangle(xp->d, pix, pixgc, 0, 0, xp->test_width, xp->test_height);
>      gcv.foreground = 1;
>      gcv.background = 0;
>      XChangeGC(xp->d, pixgc, GCForeground | GCBackground, &gcv);
[snip]
> Index: do_tests.c
> ===================================================================
> RCS file: /cvs/xorg/xc/programs/x11perf/do_tests.c,v
> retrieving revision 1.3
> diff -u -r1.3 do_tests.c
> --- do_tests.c  6 Aug 2004 23:42:11 -0000       1.3
> +++ do_tests.c  22 Apr 2005 21:40:28 -0000
> @@ -925,85 +925,85 @@
>    {"-ftext",    "Char in 80-char line (6x13)", NULL,
>                 InitText, DoText, ClearTextWin, EndText,
>                 V1_2FEATURE, ROP, 0,
> -               {80, False, "6x13", NULL}},
> +               {80, False, "-misc-fixed-medium-r-semicondensed--*-120-*-*-c-0-iso8859-1,6x13", NULL}},

Why are you replacing the XLFD names here ?

>    {"-f8text",    "Char in 70-char line (8x13)", NULL,
>                 InitText, DoText, ClearTextWin, EndText,
>                 V1_3FEATURE, ROP, 0,
> -               {70, False, "8x13", NULL}},
> +               {70, False, "-misc-fixed-medium-r-normal--*-120-*-*-c-0-iso8859-1,8x13", NULL}},
>    {"-f9text",    "Char in 60-char line (9x15)", NULL,
>                 InitText, DoText, ClearTextWin, EndText,
>                 V1_3FEATURE, ROP, 0,
> -               {60, False, "9x15", NULL}},
> +               {60, False, "-misc-fixed-medium-r-normal--*-140-*-*-c-0-iso8859-1,9x15", NULL}},
>    {"-f14text16",  "Char16 in 40-char line (k14)", NULL,
>                 InitText16, DoText16, ClearTextWin, EndText16,
>                 V1_3FEATURE, ROP, 0,
>                 {40, False,
> -             "-misc-fixed-medium-r-normal--14-130-75-75-c-140-jisx0208.1983-*",
> +             "-misc-fixed-medium-r-normal--*-130-*-*-c-0-jisx0208.1983-*,-misc-fixed-medium-r-normal--14-130-75-75-c-140-jisx0208.1983-*",

Shouldn't this
"-misc-fixed-medium-r-normal--*-130-*-*-c-0-jisx0208.1983-*,k14" ?

>                 NULL}},
>    {"-f24text16",  "Char16 in 23-char line (k24)", NULL,
>                 InitText16, DoText16, ClearTextWin, EndText16,
>                 V1_3FEATURE, ROP, 0,
>                 {23, False, 
> -             "-jis-fixed-medium-r-normal--24-230-75-75-c-240-jisx0208.1983-*",
> +             "-jis-fixed-medium-r-normal--*-230-*-*-c-0-jisx0208.1983-*,-jis-fixed-medium-r-normal--24-230-75-75-c-240-jisx0208.1983-*",
>                 NULL}},

See above... "...,k24" may be better....

>    {"-tr10text", "Char in 80-char line (TR 10)", NULL,
>                 InitText, DoText, ClearTextWin, EndText,
>                 V1_2FEATURE, ROP, 0,
>                 {80, False, 
> -               "-adobe-times-medium-r-normal--10-100-75-75-p-54-iso8859-1", 
> +               "-adobe-times-medium-r-normal--*-100-*-*-p-0-iso8859-1,-adobe-times-medium-r-normal--10-100-75-75-p-54-iso8859-1", 
>                 NULL}},

... and "...,tr10" ...

>    {"-tr24text", "Char in 30-char line (TR 24)", NULL,
>                 InitText, DoText, ClearTextWin, EndText,
>                 V1_2FEATURE, ROP, 0,
>                 {30, False, 
> -               "-adobe-times-medium-r-normal--24-240-75-75-p-124-iso8859-1",
> +               "-adobe-times-medium-r-normal--*-240-*-*-p-0-iso8859-1,-adobe-times-medium-r-normal--24-240-75-75-p-124-iso8859-1",
>                 NULL}},

... and "...,tr24" ...

>    {"-polytext", "Char in 20/40/20 line (6x13, TR 10)", NULL,
>                 InitText, DoPolyText, ClearTextWin, EndText,
>                 V1_2FEATURE, ROP, 0,
>                 {80, True, "6x13", 
> -               "-adobe-times-medium-r-normal--10-100-75-75-p-54-iso8859-1"}},
> +               "-adobe-times-medium-r-normal--*-100-*-*-p-0-iso8859-1,6x13"}},
>    {"-polytext16", "Char16 in 7/14/7 line (k14, k24)", NULL,
>                 InitText16, DoPolyText16, ClearTextWin, EndText16,
>                 V1_3FEATURE, ROP, 0,
>                 {28, True, 
> -            "-misc-fixed-medium-r-normal--14-130-75-75-c-140-jisx0208.1983-*",
> -             "-jis-fixed-medium-r-normal--24-230-75-75-c-240-jisx0208.1983-*"}},
> +            "-misc-fixed-medium-r-normal--*-130-*-*-c-0-jisx0208.1983-*,-misc-fixed-medium-r-normal--14-130-75-75-c-140-jisx0208.1983-*",
> +             "-jis-fixed-medium-r-normal--*-230-*-*-c-0-jisx0208.1983-*,-jis-fixed-medium-r-normal--24-230-75-75-c-240-jisx0208.1983-*"}},

dito.

>    {"-fitext",   "Char in 80-char image line (6x13)", NULL,
>                 InitText, DoImageText, ClearTextWin, EndText,
>                 V1_2FEATURE, PLANEMASK, 0,
> -               {80, False, "6x13", NULL}},
> +               {80, False, "-misc-fixed-medium-r-semicondensed--*-120-*-*-c-0-iso8859-1,6x13", NULL}},
>    {"-f8itext",   "Char in 70-char image line (8x13)", NULL,
>                 InitText, DoImageText, ClearTextWin, EndText,
>                 V1_3FEATURE, PLANEMASK, 0,
> -               {70, False, "8x13", NULL}},
> +               {70, False, "-misc-fixed-medium-r-normal--*-120-*-*-c-0-iso8859-1,8x13", NULL}},
>    {"-f9itext",   "Char in 60-char image line (9x15)", NULL,
>                 InitText, DoImageText, ClearTextWin, EndText,
>                 V1_3FEATURE, PLANEMASK, 0,
> -               {60, False, "9x15", NULL}},
> +               {60, False, "-misc-fixed-medium-r-normal--*-140-*-*-c-0-iso8859-1,9x15", NULL}},
>    {"-f14itext16", "Char16 in 40-char image line (k14)", NULL,
>                 InitText16, DoImageText16, ClearTextWin, EndText16,
>                 V1_3FEATURE, PLANEMASK, 0,
>                 {40, False,
> -             "-misc-fixed-medium-r-normal--14-130-75-75-c-140-jisx0208.1983-*",
> +             "-misc-fixed-medium-r-normal--*-130-*-*-c-0-jisx0208.1983-*,-misc-fixed-medium-r-normal--14-130-75-75-c-140-jisx0208.1983-*",
>                 NULL}},
>    {"-f24itext16", "Char16 in 23-char image line (k24)", NULL,
>                 InitText16, DoImageText16, ClearTextWin, EndText16,
>                 V1_3FEATURE, PLANEMASK, 0,
>                 {23, False, 
> -             "-jis-fixed-medium-r-normal--24-230-75-75-c-240-jisx0208.1983-*",
> +             "-jis-fixed-medium-r-normal--*-230-*-*-c-0-jisx0208.1983-*,-jis-fixed-medium-r-normal--24-230-75-75-c-240-jisx0208.1983-*",
>                 NULL}},
>    {"-tr10itext", "Char in 80-char image line (TR 10)", NULL,
>                 InitText, DoImageText, ClearTextWin, EndText,
>                 V1_2FEATURE, PLANEMASK, 0,
>                 {80, False, 
> -               "-adobe-times-medium-r-normal--10-100-75-75-p-54-iso8859-1", 
> +               "-adobe-times-medium-r-normal--*-100-*-*-p-0-iso8859-1,-adobe-times-medium-r-normal--10-100-75-75-p-54-iso8859-1", 
>                 NULL}},
>    {"-tr24itext", "Char in 30-char image line (TR 24)", NULL,
>                 InitText, DoImageText, ClearTextWin, EndText,
>                 V1_2FEATURE, PLANEMASK, 0,
>                 {30, False, 
> -               "-adobe-times-medium-r-normal--24-240-75-75-p-124-iso8859-1",
> +               "-adobe-times-medium-r-normal--*-240-*-*-p-0-iso8859-1,-adobe-times-medium-r-normal--24-240-75-75-p-124-iso8859-1",
>                 NULL}},
>  #ifdef XFT
>    {"-aa10text", "Char in 80-char aa line (Charter 10)", NULL,
> Index: do_text.c
> ===================================================================
> RCS file: /cvs/xorg/xc/programs/x11perf/do_text.c,v
> retrieving revision 1.2
> diff -u -r1.2 do_text.c
> --- do_text.c   23 Apr 2004 19:54:38 -0000      1.2
> +++ do_text.c   22 Apr 2005 21:40:29 -0000
> @@ -35,6 +35,24 @@
>  #define XPOS 20
>  #define SEGS 3
>  
> +static
> +XFontStruct *LoadQueryFontList(Display *d, char *list)
> +{
> +    char *s;
> +    XFontStruct *f;
> +    list = strdup(list);
> +    s = strtok(list, ",");

Ewwww... hopeflly the code does not use |strtok()| from a calling
function (|strtok()| uses a libc-internal global variable which prevents
the nested usage of this function). |strtok_r()| may be better here,
however that one is not available on all platforms.
Your choice - either replace |strok()| with |strtok_r()| here and
somehow add a compile-time check whether |strtok_r()| is available or
add a small comment which warns about that nested usage may be harmfull
(and check whether there are any other consumers in the x11perf
codebase).

> +    while(s)
> +    {
> +        f = XLoadQueryFont(d, s);
> +       if (f) {
> +           free(list);
> +           return f;
> +       }
> +        s = strtok(NULL, ",");
> +    }
> +    return NULL;
> +}
>  
>  int 
>  InitText(XParms xp, Parms p, int reps)
> @@ -43,7 +61,7 @@
>      char       ch;
>      XGCValues   gcv;
>  
> -    font = XLoadQueryFont(xp->d, p->font);
> +    font = LoadQueryFontList(xp->d, p->font);
>      if (font == NULL) {
>         printf("Could not load font '%s', benchmark omitted\n", p->font);
>         return 0;
> @@ -51,15 +69,15 @@
>  
>      bfont = NULL;
>      if (p->bfont != NULL) {
> -       bfont = XLoadQueryFont(xp->d, p->bfont);
> +       bfont = LoadQueryFontList(xp->d, p->bfont);
>         if (bfont == NULL) {
>             printf("Could not load font '%s', benchmark omitted\n", p->bfont);
>             return 0;
>         }
>      }
>  
> -    ypos = XPOS;
>      height = (font->max_bounds.ascent + font->max_bounds.descent) + 1;
> +    ypos = XPOS+height;
>      if (bfont != NULL) {
>         int     h = (bfont->max_bounds.ascent + bfont->max_bounds.descent) + 1;
>         if (h > height)
> @@ -129,7 +147,7 @@
>      int                        rows, columns, totalChars, ch;
>      int                        brows, bcolumns = 0, btotalChars = 0, bch = 0;
>  
> -    font = XLoadQueryFont(xp->d, p->font);
> +    font = LoadQueryFontList(xp->d, p->font);
>      if (font == NULL) {
>         printf("Could not load font '%s', benchmark omitted\n", p->font);
>         return 0;
> @@ -142,7 +160,7 @@
>  
>      bfont = NULL;
>      if (p->bfont != NULL) {
> -       bfont = XLoadQueryFont(xp->d, p->bfont);
> +       bfont = LoadQueryFontList(xp->d, p->bfont);
>         if (bfont == NULL) {
>             printf("Could not load font '%s', benchmark omitted\n", p->bfont);
>             return 0;
> @@ -154,8 +172,8 @@
>         if (brows > totalLines) totalLines = brows;
>      }
>  
> -    ypos = XPOS;
>      height = (font->max_bounds.ascent + font->max_bounds.descent) + 1;
> +    ypos = XPOS+height;
>      if (bfont != NULL) {
>         int     h = (bfont->max_bounds.ascent + bfont->max_bounds.descent) + 1;
>         if (h > height)
> @@ -230,9 +248,9 @@
>         XDrawString(
>             xp->d, xp->w, xp->fggc, XPOS, ypos, charBuf[line], charsPerLine);
>         ypos += height;
> -       if (ypos > HEIGHT - height) {
> +       if (ypos > xp->test_height - height) {
>             /* Wraparound to top of window */
> -           ypos = XPOS;
> +           ypos = XPOS+height;

Why is this change needed ?

[snip]
> Index: do_windows.c
> ===================================================================
> RCS file: /cvs/xorg/xc/programs/x11perf/do_windows.c,v
> retrieving revision 1.2
> diff -u -r1.2 do_windows.c
> --- do_windows.c        23 Apr 2004 19:54:38 -0000      1.2
> +++ do_windows.c        22 Apr 2005 21:40:30 -0000
> @@ -50,8 +50,8 @@
>  
>      ComputeSizes(xp, p);
>  
> -    parentcolumns = WIDTH / parentwidth;
> -    parentrows = HEIGHT / parentheight;
> +    parentcolumns = xp->test_width / parentwidth;
> +    parentrows = xp->test_height / parentheight;
>      parentwindows = parentcolumns * parentrows; /* Max reps we can fit */
>  
>      if (parentwindows > reps) {
> @@ -243,7 +243,7 @@
>             0, xp->foreground, xp->foreground);
>  #else   
>      isolate = XCreateSimpleWindow(
> -           xp->d, xp->w, 0, 0, WIDTH, HEIGHT,
> +           xp->d, xp->w, 0, 0, xp->test_width, xp->test_height,
>             0, xp->background, xp->background);

What about the x/y coordinates ? Should they really be 0,0 in this case
?
 
>      ComputeSizes(xp, p);
> Index: x11perf.c
> ===================================================================
> RCS file: /cvs/xorg/xc/programs/x11perf/x11perf.c,v
> retrieving revision 1.2
> diff -u -r1.2 x11perf.c
> --- x11perf.c   23 Apr 2004 19:54:38 -0000      1.2
> +++ x11perf.c   22 Apr 2005 21:40:33 -0000
> @@ -43,7 +43,9 @@
>  /* Only for working on ``fake'' servers, for hardware that doesn't exist */
>  static Bool     drawToFakeServer = False;
>  static Pixmap   tileToQuery     = None;
> -static char *displayName;
> +static char *displayName,
> +            *printerName,
> +            *printFilename;
>  int    abortTest;
>  
>  typedef struct _RopNames { char        *name; int  rop; } RopNameRec, *RopNamePtr;
> @@ -393,6 +395,125 @@
>      return(d);
>  }
>  
> +#ifdef XPRINT
> +static
> +void Open_Printer(char *printer_name, char *print_file)
> +{
> +    long           dpi_x              = 0L,
> +                   dpi_y              = 0L;   
> +    XPPrinterList  plist              = NULL;  /* list of printers */
> +    int            plist_count;                /* number of entries in |plist|-array */
> +    unsigned short dummy;
> +    XRectangle     winrect;
> +
> +    plist = XpuGetPrinterList(printer_name, &plist_count);
> +
> +    if (!plist) {
> +       fprintf(stderr, "%s:  no printers found for printer spec \"%s\".\n",
> +               program_name, NULLSTR(printer_name));
> +       exit(1);

Ewww... three-space intendation ? Did you copy that code from "glxgears"
? Please use four if possible unless the matching code uses something
else consistently (this is AFAIK only glxgears).

> +    }
> +
> +    printer_name = plist[0].name;
> +
> +    Log(("Using printer '%s'\n", printer_name));
> +
> +    if (XpuGetPrinter(printer_name, &xparms.d, &xparms.pcontext) != 1) {
> +       fprintf(stderr, "%s: Cannot open printer '%s'\n", program_name, printer_name);
> +       exit(1);

You can use |exit(EXIT_FAILURE| here (that's plain ANSI-C89).

> +    }
> +
> +    if (XpQueryExtension(xparms.d, &xparms.xp_event_base, &xparms.xp_error_base) == False) {
> +       fprintf(stderr, "%s: XpQueryExtension() failed.\n", program_name);
> +       XpuClosePrinterDisplay(xparms.d, xparms.pcontext);
> +       exit(1);

dito.

> +    }
> +
> +    /* Listen to XP(Start|End)(Job|Doc|Page)Notify events).
> +     * This is mantatory as Xp(Start|End)(Job|Doc|Page) functions are _not_ 
> +     * syncronous !!
> +     * Not waiting for such events may cause that subsequent data may be 
> +     * destroyed/corrupted!!
> +     */
> +    XpSelectInput(xparms.d, xparms.pcontext, XPPrintMask);
> +
> +    /* Set job title */
> +    XpuSetJobTitle(xparms.d, xparms.pcontext, "x11perf/Xprint");
> +
> +    /* Set print context
> +     * Note that this modifies the available fonts, including builtin printer prints.
> +     * All XListFonts()/XLoadFont() stuff should be done _after_ setting the print 
> +     * context to obtain the proper fonts.
> +     */ 
> +    XpSetContext(xparms.d, xparms.pcontext);
> +
> +    /* Get default printer reolution */   
> +    if (XpuGetResolution(xparms.d, xparms.pcontext, &dpi_x, &dpi_y) != 1) {
> +       fprintf(stderr, "%s: No default resolution for printer '%s'.\n",
> +       program_name, printer_name);
> +       XpuClosePrinterDisplay(xparms.d, xparms.pcontext);
> +       exit(1);

dito.

> +    }
> +
> +    if (print_file) {
> +       Log(("starting job (to file '%s').\n", print_file));
> +       xparms.printtofile_handle = XpuStartJobToFile(xparms.d, xparms.pcontext, print_file);
> +       if( !xparms.printtofile_handle ) {
> +          fprintf(stderr, "%s: Error: %s while trying to print to file.\n", 
> +                  program_name, strerror(errno));
> +          XpuClosePrinterDisplay(xparms.d, xparms.pcontext);
> +          exit(1);

dito.

> +       }
> +
> +       XpuWaitForPrintNotify(xparms.d, xparms.xp_event_base, XPStartJobNotify);
> +    }
> +    else
> +    {
> +       Log(("starting job.\n"));
> +       XpuStartJobToSpooler(xparms.d);    
> +       XpuWaitForPrintNotify(xparms.d, xparms.xp_event_base, XPStartJobNotify);
> +    }
> +
> +    xparms.screen = XpGetScreenOfContext(xparms.d, xparms.pcontext);

I remember that x11perf uses |DefaultScreen()| very often... please
check whether it's always using the correct screen (that will bite
people when there is more than one active print driver per server (e.g.
Xprt with PostScript+PCL+SVG screens)).

> +
> +    /* Obtain some info about page geometry */
> +    XpGetPageDimensions(xparms.d, xparms.pcontext, &dummy, &dummy, &winrect);
> +
> +    if (xparms.test_width == 0 &&
> +        xparms.test_height == 0) {
> +        xparms.test_x      = winrect.x;
> +        xparms.test_y      = winrect.y;
> +        xparms.test_width  = winrect.width;
> +        xparms.test_height = winrect.height;

Please check whether all window creations really use |test_x|/|test_y|.

[snip]
>  #ifdef SIGNALRETURNSINT
>  static int
> @@ -428,6 +549,8 @@
>      static char *help_message[] = {
>  "where options include:",
>  "    -display <host:display>   the X server to contact",
> +"    -printer <name>           name of printer to work on",
> +"    -printfile <filename>     output file for print job",
>  "    -sync                     do the tests in synchronous mode",
>  "    -pack                     pack rectangles right next to each other",

What about "-width" and "-height" options here ?

>  "    -repeat <n>               do tests <n> times (default = 5)",
> @@ -790,6 +913,11 @@
>      double  time, totalTime;
>      int     reps;
>      int     j;
> +    
> +    if (xp->pcontext) {
> +        XpStartPage(xp->d, xp->w);
> +        XpuWaitForPrintNotify(xp->d, xp->xp_event_base, XPStartPageNotify);      
> +    }
>  
>      xp->planemask = pm;
>      CreatePerfGCs(xp, func, pm);
> @@ -808,7 +936,7 @@
>          */
>         if(reps == 0){
>             DestroyPerfGCs(xp);
> -           return;
> +           goto done;
>         }
>         /* Create clip windows if requested */
>         CreateClipWindows(xp, test->clips);
> @@ -836,6 +964,12 @@
>      printf ("\n");
>      fflush(stdout);
>      DestroyPerfGCs(xp);
> +
> +done:
> +    if (xp->pcontext) {
> +        XpEndPage(xp->d);
> +        XpuWaitForPrintNotify(xp->d, xp->xp_event_base, XPEndPageNotify);
> +    }
>  } /* ProcessTest */
>  
>  #define Strstr strstr
> @@ -868,6 +1002,8 @@
>      xparms.pack = False;
>      xparms.save_under = False;
>      xparms.backing_store = NotUseful;
> +    xparms.test_width  = 0; /* set to 0 means: use default */
> +    xparms.test_height = 0; /* set to 0 means: use default */
>  
>      /* Count number of tests */
>      ForEachTest(numTests);
> @@ -878,7 +1014,18 @@
>      displayName = Get_Display_Name (&argc, argv);
>      xparms.version = GetVersion(&argc, argv);
>      for (i = 1; i != argc; i++) {
> -       if (strcmp (argv[i], "-all") == 0) {
> +       if (strcmp (argv[i], "-printer") == 0) {
> +           i++;
> +           if (argc <= i)
> +               usage ();
> +           printerName = argv[i];
> +       } else if (strcmp (argv[i], "-printfile") == 0) {
> +           i++;
> +           if (argc <= i)
> +               usage ();
> +           printFilename = argv[i];
> +       } else         
> +        if (strcmp (argv[i], "-all") == 0) {
>             ForEachTest (j)
>                 doit[j] = test[j].versions & xparms.version;
>             foundOne = True;
> @@ -1109,8 +1256,26 @@
>  
>      if (!foundOne)
>         usage ();
> -    xparms.d = Open_Display (displayName);
> -    screen = DefaultScreen(xparms.d);
> +    if (displayName && printerName) {
> +       fprintf (stderr, "%s:  You cannot specify -display and -printer at the name time\n",
> +                program_name);
> +       exit(1);
> +    }
> +    if (printerName) {
> +        Open_Printer(printerName, printFilename);
> +    }
> +    else
> +    {
> +        xparms.d = Open_Display (displayName);
> +        xparms.screen = DefaultScreenOfDisplay(xparms.d);

Please use |XDefaultScreenOfDisplay| instead of |DefaultScreenOfDisplay|
(|DefaultScreenOfDisplay| is a macro which uses semi-private fields in
the Xlib structures and this behaviour should be avoided... we're no
longer in 1980 where we can count the MIPS value of a CPU with the
fingers of one hand... :) ...

> +    }
> +
> +    screen = XScreenNumberOfScreen(xparms.screen);   
> +    if (xparms.test_width == 0 &&
> +        xparms.test_height == 0) {
> +        xparms.test_width  = 1200;
> +        xparms.test_height = 1200;
> +    }
>  
>      /* get visual info of default visual */
>      vmask = VisualIDMask | VisualScreenMask;
> @@ -1212,20 +1377,27 @@
>         AllocateColor(xparms.d, background, WhitePixel(xparms.d, screen));
>      xparms.ddbackground =
>         AllocateColor(xparms.d, ddbackground, WhitePixel(xparms.d, screen));
> -    window_x = 2;
> -    if (DisplayWidth(xparms.d, screen) < WIDTH + window_x + 1)
> -       window_x = -1;
> -    window_y = 2;
> -    if (DisplayHeight(xparms.d, screen) < HEIGHT + window_y + 1)
> -       window_y = -1;
> -    xparms.w = CreatePerfWindow(&xparms, window_x, window_y, WIDTH, HEIGHT);
> -    HSx = WIDTH-1;
> -    if (window_x + 1 + WIDTH > DisplayWidth(xparms.d, screen))
> +    if (xparms.pcontext) {
> +        window_x = xparms.test_x;
> +        window_y = xparms.test_y - (20+5); /* -room_for_status_window */
> +    }
> +    else
> +    {
> +        window_x = 2;
> +        if (DisplayWidth(xparms.d, screen) < xparms.test_width + window_x + 1)
> +            window_x = -1;
> +        window_y = 2;
> +        if (DisplayHeight(xparms.d, screen) < xparms.test_height + window_y + 1)
> +            window_y = -1;
> +    }
> +    xparms.w = CreatePerfWindow(&xparms, window_x, window_y, xparms.test_width, xparms.test_height);
> +    HSx = xparms.test_width-1;
> +    if (window_x + 1 + xparms.test_width > DisplayWidth(xparms.d, screen))
>         HSx = DisplayWidth(xparms.d, screen) - (1 + window_x + 1);
> -    HSy = HEIGHT-1;
> -    if (window_y + 1 + HEIGHT > DisplayHeight(xparms.d, screen))
> +    HSy = xparms.test_height-1;
> +    if (window_y + 1 + xparms.test_height > DisplayHeight(xparms.d, screen))
>         HSy = DisplayHeight(xparms.d, screen) - (1 + window_y + 1);
> -    status = CreatePerfWindow(&xparms, window_x, HEIGHT+5, WIDTH, 20);
> +    status = CreatePerfWindow(&xparms, window_x, xparms.test_height+5, xparms.test_width, 20);

Shouldn't this be |window_y+xparms.test_height+5| instead of
|xparms.test_height+5| =

>      tgcv.foreground = 
>         AllocateColor(xparms.d, "black", BlackPixel(xparms.d, screen));
>      tgcv.background = 
> @@ -1241,7 +1413,7 @@
>         software cursor machines it will slow graphics performance.  On
>         all current MIT-derived servers it will slow window 
>         creation/configuration performance. */
> -    XWarpPointer(xparms.d, None, status, 0, 0, 0, 0, WIDTH+32, 20+32);
> +    XWarpPointer(xparms.d, None, status, 0, 0, 0, 0, xparms.test_width+32, 20+32);
>  
>      /* Figure out how long to call HardwareSync, so we can adjust for that
>         in our total elapsed time */
> @@ -1320,7 +1492,31 @@
>      /* Restore ScreenSaver to original state. */
>      XSetScreenSaver(xparms.d, ssTimeout, ssInterval, ssPreferBlanking,
>         ssAllowExposures);
> -    XCloseDisplay(xparms.d);
> +
> +   if (xparms.pcontext) {
> +        /* End the print job - the final results are sent by the X print
> +         * server to the spooler sub system.
> +         */
> +        XpEndJob(xparms.d);
> +        XpuWaitForPrintNotify(xparms.d, xparms.xp_event_base, XPEndJobNotify);    
> +        Log(("end job.\n"));
> +
> +        if (printFilename) {
> +            if (XpuWaitForPrintFileChild(xparms.printtofile_handle) != XPGetDocFinished) {
> +                fprintf(stderr, "%s: Error while printing to file.\n", program_name);
> +                XpuClosePrinterDisplay(xparms.d, xparms.pcontext);
> +                exit(1);

|EXIT_FAILURE| ...

[snip]
> Index: x11perf.h
> ===================================================================
> RCS file: /cvs/xorg/xc/programs/x11perf/x11perf.h,v
> retrieving revision 1.3
> diff -u -r1.3 x11perf.h
> --- x11perf.h   6 Aug 2004 23:42:11 -0000       1.3
> +++ x11perf.h   22 Apr 2005 21:40:34 -0000
> @@ -34,15 +34,21 @@
>  #if defined(XlibSpecificationRelease) && XlibSpecificationRelease >= 5
>  #include <X11/Xfuncs.h>
>  #endif
> +#ifdef XPRINT
> +#include <X11/XprintUtil/xprintutil.h>
> +#endif
> +
>  #include <stddef.h>
>  
> +/* Turn a NULL pointer string into an empty string */
> +#define NULLSTR(x) (((x)!=NULL)?(x):(""))
> +#define Log(x) { printf x; }
> +#define Msg(x) { printf x; }

Both |Log()| and |Msg()| should be made conditional or replaced with
|fprintf(stderr, ...| in the x11perf cases unless there is more usage of
them in the code...

[snip]
-- snip --

Other nits:
- The status window of "x11perf" seems to use the "fixed" font.
- The idea of making the test window width/height depending on the page
size may be debateable as this may alter the test results of some DDX
when the default page size/resolution differs. Either a "-width" and
"-height" option may be usefull - or a "-pagesize" option...

----

Bye,
Roland

-- 
  __ .  . __
 (o.\ \/ /.o) roland.mainz at nrubsig.org
  \__\/\/__/  MPEG specialist, C&&JAVA&&Sun&&Unix programmer
  /O /==\ O\  TEL +49 641 7950090
 (;O/ \/ \O;)


More information about the Xprint mailing list