[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