[PATCH] dix/dispatch: DoGetImage: Use a single buffer

Aaron Plattner aplattner at nvidia.com
Mon Mar 31 11:11:48 PDT 2014


On 03/29/14 02:53, Daniel Kurtz wrote:
> DoGetImage chops up an image into IMAGE_BUFSIZE strips.
> This predates commit [0] (a pull from XFree86 to X.Org), at which time
> IMAGE_BUFSIZE was 8k.  Back then the immediate buffers were
> allocated with ALLOCATE_LOCAL(), so it made sense to try to keep them
> small since at least on some systems, these buffers were allocated on the
> stack.
>
> [0] commit ded6147bfb5d75ff1e67c858040a628b61bc17d1
> Author: Kaleb Keithley <kaleb at freedesktop.org>
> Date:   Fri Nov 14 15:54:54 2003 +0000
>
>      R6.6 is the Xorg base-line
>
> 8k would fetch an 1024x864 monochrome screen w/ 32-bit word in just 14
> buffers.  But, this was soon deemed too small, and the buffer size was
> increased to 64 kB in commit [1], although it was still using
> ALLOCATE_LOCAL().
>
> [1] commit d568221710959cf7d783e6ff0fb80fb43a231124
> Author: Kaleb Keithley <kaleb at freedesktop.org>
> Date:   Fri Nov 14 16:49:22 2003 +0000
>
>      XFree86 4.3.0.1
>
> Eventually people realized that alloca() had no error detection, and could
> overrun the stack, so and ALLOCATE_LOCAL was killed, replaced by a
> succession of heap allocators starting with commit [2]:
>    xalloc() -> xcalloc -> calloc().
>
> [2] commit 6e4f5cf83f35ffebb51633ab30b1826e63e37223
> Author: Ben Byer <bbyer at bbyer.local>
> Date:   Mon Nov 5 05:53:34 2007 -0800
>
>      changing ALLOCATE_LOCAL to xalloc to prevent stack overflow
>
> Since we are now allocating from the heap, there should no longer be
> necessary, or desirable to chop up the DoGetImage() buffer.
>
> In theory, we *could* still chop it up as a fallback if the big buffer
> allocation fails.  But, if the system is that close to running out of heap
> space, it is might be best to just fail quickly.

Can't the max image size be multiple gigabytes?  That seems a little 
large to describe as "close" to running out of heap space.

-- Aaron

> When copying from a EXA pixmap, this can save a bit of overhead from multiple
> EXA prepare/finish access calls.
>
> Signed-off-by: Daniel Kurtz <djkurtz at chromium.org>
> ---
>   dix/dispatch.c     | 109 +++++++++++++++++------------------------------------
>   include/servermd.h |   8 ----
>   2 files changed, 35 insertions(+), 82 deletions(-)
>
> diff --git a/dix/dispatch.c b/dix/dispatch.c
> index 4f830f7..a0b1bc0 100644
> --- a/dix/dispatch.c
> +++ b/dix/dispatch.c
> @@ -1977,8 +1977,7 @@ DoGetImage(ClientPtr client, int format, Drawable drawable,
>              Mask planemask)
>   {
>       DrawablePtr pDraw, pBoundingDraw;
> -    int nlines, linesPerBuf, rc;
> -    int linesDone;
> +    int rc;
>
>       /* coordinates relative to the bounding drawable */
>       int relx, rely;
> @@ -2072,31 +2071,7 @@ DoGetImage(ClientPtr client, int format, Drawable drawable,
>
>       }
>
> -    xgi.length = length;
> -
> -    xgi.length = bytes_to_int32(xgi.length);
> -    if (widthBytesLine == 0 || height == 0)
> -        linesPerBuf = 0;
> -    else if (widthBytesLine >= IMAGE_BUFSIZE)
> -        linesPerBuf = 1;
> -    else {
> -        linesPerBuf = IMAGE_BUFSIZE / widthBytesLine;
> -        if (linesPerBuf > height)
> -            linesPerBuf = height;
> -    }
> -    length = linesPerBuf * widthBytesLine;
> -    if (linesPerBuf < height) {
> -        /* we have to make sure intermediate buffers don't need padding */
> -        while ((linesPerBuf > 1) &&
> -               (length & ((1L << LOG2_BYTES_PER_SCANLINE_PAD) - 1))) {
> -            linesPerBuf--;
> -            length -= widthBytesLine;
> -        }
> -        while (length & ((1L << LOG2_BYTES_PER_SCANLINE_PAD) - 1)) {
> -            linesPerBuf++;
> -            length += widthBytesLine;
> -        }
> -    }
> +    xgi.length = bytes_to_int32(length);
>       if (!(pBuf = calloc(1, length)))
>           return BadAlloc;
>       WriteReplyToClient(client, sizeof(xGetImageReply), &xgi);
> @@ -2108,60 +2083,46 @@ DoGetImage(ClientPtr client, int format, Drawable drawable,
>           }
>       }
>
> -    if (linesPerBuf == 0) {
> +    if (length == 0) {
>           /* nothing to do */
>       }
>       else if (format == ZPixmap) {
> -        linesDone = 0;
> -        while (height - linesDone > 0) {
> -            nlines = min(linesPerBuf, height - linesDone);
> -            (*pDraw->pScreen->GetImage) (pDraw,
> -                                         x,
> -                                         y + linesDone,
> -                                         width,
> -                                         nlines,
> -                                         format, planemask, (void *) pBuf);
> -            if (pVisibleRegion)
> -                XaceCensorImage(client, pVisibleRegion, widthBytesLine,
> -                                pDraw, x, y + linesDone, width,
> -                                nlines, format, pBuf);
> -
> -            /* Note that this is NOT a call to WriteSwappedDataToClient,
> -               as we do NOT byte swap */
> -            ReformatImage(pBuf, (int) (nlines * widthBytesLine),
> -                          BitsPerPixel(pDraw->depth), ClientOrder(client));
> -
> -            WriteToClient(client, (int) (nlines * widthBytesLine), pBuf);
> -            linesDone += nlines;
> -        }
> +        (*pDraw->pScreen->GetImage) (pDraw,
> +                                     x,
> +                                     y,
> +                                     width,
> +                                     height,
> +                                     format, planemask, (void *) pBuf);
> +        if (pVisibleRegion)
> +            XaceCensorImage(client, pVisibleRegion, widthBytesLine,
> +                            pDraw, x, y, width, height, format, pBuf);
> +
> +        /* Note that this is NOT a call to WriteSwappedDataToClient,
> +           as we do NOT byte swap */
> +        ReformatImage(pBuf, length,
> +                      BitsPerPixel(pDraw->depth), ClientOrder(client));
> +
> +        WriteToClient(client, length, pBuf);
>       }
>       else {                      /* XYPixmap */
> -
> +        int plane_size = height * widthBytesLine;
>           for (; plane; plane >>= 1) {
>               if (planemask & plane) {
> -                linesDone = 0;
> -                while (height - linesDone > 0) {
> -                    nlines = min(linesPerBuf, height - linesDone);
> -                    (*pDraw->pScreen->GetImage) (pDraw,
> -                                                 x,
> -                                                 y + linesDone,
> -                                                 width,
> -                                                 nlines,
> -                                                 format, plane, (void *) pBuf);
> -                    if (pVisibleRegion)
> -                        XaceCensorImage(client, pVisibleRegion,
> -                                        widthBytesLine,
> -                                        pDraw, x, y + linesDone, width,
> -                                        nlines, format, pBuf);
> -
> -                    /* Note: NOT a call to WriteSwappedDataToClient,
> -                       as we do NOT byte swap */
> -                    ReformatImage(pBuf, (int) (nlines * widthBytesLine),
> -                                  1, ClientOrder(client));
> -
> -                    WriteToClient(client, (int)(nlines * widthBytesLine), pBuf);
> -                    linesDone += nlines;
> -                }
> +                (*pDraw->pScreen->GetImage) (pDraw,
> +                                             x,
> +                                             y,
> +                                             width,
> +                                             height,
> +                                             format, plane, (void *) pBuf);
> +                if (pVisibleRegion)
> +                    XaceCensorImage(client, pVisibleRegion, widthBytesLine,
> +                                    pDraw, x, y, width, height, format, pBuf);
> +
> +                /* Note: NOT a call to WriteSwappedDataToClient,
> +                   as we do NOT byte swap */
> +                ReformatImage(pBuf, plane_size, 1, ClientOrder(client));
> +
> +                WriteToClient(client, plane_size, pBuf);
>               }
>           }
>       }
> diff --git a/include/servermd.h b/include/servermd.h
> index 11f6c10..42a5e74 100644
> --- a/include/servermd.h
> +++ b/include/servermd.h
> @@ -300,14 +300,6 @@ SOFTWARE.
>
>   #endif                          /* __aarch64__ */
>
> -/* size of buffer to use with GetImage, measured in bytes. There's obviously
> - * a trade-off between the amount of heap used and the number of times the
> - * ddx routine has to be called.
> - */
> -#ifndef IMAGE_BUFSIZE
> -#define IMAGE_BUFSIZE          (64*1024)
> -#endif
> -
>   /* pad scanline to a longword */
>   #ifndef BITMAP_SCANLINE_UNIT
>   #define BITMAP_SCANLINE_UNIT   32
> --
> 1.9.1.423.g4596e3a



More information about the xorg-devel mailing list