[Mesa-dev] [PATCH 2/2] glx: Remove (unused, broken) fastImageUnpack fast path

[Brian Paul]

On 06/01/2011 11:44 AM, Adam Jackson wrote:
> Signed-off-by: Adam Jackson<ajax at redhat.com>
> ---
>   src/glx/glxclient.h    |    8 ----
>   src/glx/indirect_glx.c |    3 -
>   src/glx/renderpix.c    |   98 +++++++++++++++++++-----------------------------
>   3 files changed, 39 insertions(+), 70 deletions(-)
>
> diff --git a/src/glx/glxclient.h b/src/glx/glxclient.h
> index 2b6966f..a443f78 100644
> --- a/src/glx/glxclient.h
> +++ b/src/glx/glxclient.h
> @@ -310,14 +310,6 @@ struct glx_context
>      /*@} */
>
>       /**
> -     * This is \c GL_TRUE if the pixel unpack modes are such that an image
> -     * can be unpacked from the clients memory by just copying.  It may
> -     * still be true that the server will have to do some work.  This
> -     * just promises that a straight copy will fetch the correct bytes.
> -     */
> -   GLboolean fastImageUnpack;
> -
> -    /**
>        * Fill newImage with the unpacked form of \c oldImage getting it
>        * ready for transport to the server.
>        */
> diff --git a/src/glx/indirect_glx.c b/src/glx/indirect_glx.c
> index 1870ede..b4f16c7 100644
> --- a/src/glx/indirect_glx.c
> +++ b/src/glx/indirect_glx.c
> @@ -404,10 +404,7 @@ indirect_create_context(struct glx_screen *psc,
>
>      /*
>       ** PERFORMANCE NOTE: A mode dependent fill image can speed things up.
> -    ** Other code uses the fastImageUnpack bit, but it is never set
> -    ** to GL_TRUE.
>       */
> -   gc->fastImageUnpack = GL_FALSE;
>      gc->fillImage = __glFillImage;
>      gc->pc = gc->buf;
>      gc->bufEnd = gc->buf + bufSize;
> diff --git a/src/glx/renderpix.c b/src/glx/renderpix.c
> index 8234bbe..b54f115 100644
> --- a/src/glx/renderpix.c
> +++ b/src/glx/renderpix.c
> @@ -76,10 +76,6 @@
>    * Modify this function so that \c NULL images are sent using
>    * \c __glXSendLargeChunk instead of __glXSendLargeCommand.  Doing this
>    * will eliminate the need to allocate a buffer for that case.
> - *
> - * \bugs
> - * The \c fastImageUnpack path, which is thankfully never used, is completely
> - * broken.
>    */
>   void
>   __glXSendLargeImage(struct glx_context * gc, GLint compsize, GLint dim,
> @@ -87,48 +83,38 @@ __glXSendLargeImage(struct glx_context * gc, GLint compsize, GLint dim,
>                       GLenum format, GLenum type, const GLvoid * src,
>                       GLubyte * pc, GLubyte * modes)
>   {
> -   if (!gc->fastImageUnpack || (src == NULL)) {
> -      /* Allocate a temporary holding buffer */
> -      GLubyte *buf = (GLubyte *) Xmalloc(compsize);
> -      if (!buf) {
> -         __glXSetError(gc, GL_OUT_OF_MEMORY);
> -         return;
> -      }
> +    /* Allocate a temporary holding buffer */
> +    GLubyte *buf = (GLubyte *) Xmalloc(compsize);
> +    if (!buf) {
> +	__glXSetError(gc, GL_OUT_OF_MEMORY);
> +	return;
> +    }
>
> -      /* Apply pixel store unpack modes to copy data into buf */
> -      if (src != NULL) {
> -         (*gc->fillImage) (gc, dim, width, height, depth, format, type,
> -                           src, buf, modes);
> -      }
> -      else {
> -         if (dim<  3) {
> -            (void) memcpy(modes, __glXDefaultPixelStore + 4, 20);
> -         }
> -         else {
> -            (void) memcpy(modes, __glXDefaultPixelStore + 0, 36);
> -         }
> -      }
> +    /* Apply pixel store unpack modes to copy data into buf */
> +    if (src != NULL) {
> +	(*gc->fillImage) (gc, dim, width, height, depth, format, type,
> +			  src, buf, modes);
> +    }
> +    else {
> +	if (dim<  3) {
> +	    (void) memcpy(modes, __glXDefaultPixelStore + 4, 20);

Independent of this patch, __glXDefaultPixelStore + 4 seems like a 
magic number kind of thing.  There's macros for this value and others 
in indirect.c (grep default_pixel_store_2D) which should probably be 
used here.  Feel like taking care of that?


> +	}
> +	else {
> +	    (void) memcpy(modes, __glXDefaultPixelStore + 0, 36);
> +	}
> +    }
>
> -      /* Send large command */
> -      __glXSendLargeCommand(gc, gc->pc, pc - gc->pc, buf, compsize);
> +    /* Send large command */
> +    __glXSendLargeCommand(gc, gc->pc, pc - gc->pc, buf, compsize);
>
> -      /* Free buffer */
> -      Xfree((char *) buf);
> -   }
> -   else {
> -      /* Just send the data straight as is */
> -      __glXSendLargeCommand(gc, gc->pc, pc - gc->pc, pc, compsize);
> -   }
> +    /* Free buffer */
> +    Xfree((char *) buf);
>   }
>
>   /************************************************************************/
>
>   /**
>    * Implement GLX protocol for \c glSeparableFilter2D.
> - *
> - * \bugs
> - * The \c fastImageUnpack path, which is thankfully never used, is completely
> - * broken.
>    */
>   void
>   __indirect_glSeparableFilter2D(GLenum target, GLenum internalformat,
> @@ -177,6 +163,7 @@ __indirect_glSeparableFilter2D(GLenum target, GLenum internalformat,
>         __GLX_END(0);
>      }
>      else {
> +      GLubyte *buf;
>         const GLint bufsize = image1len + image2len;
>
>         /* Use GLXRenderLarge protocol to send command */
> @@ -190,29 +177,22 @@ __indirect_glSeparableFilter2D(GLenum target, GLenum internalformat,
>         __GLX_PUT_LONG(20, type);
>         pc += hdrlen;
>
> -      if (!gc->fastImageUnpack) {
> -         /* Allocate a temporary holding buffer */
> -         GLubyte *buf = (GLubyte *) Xmalloc(bufsize);
> -         if (!buf) {
> -            __glXSetError(gc, GL_OUT_OF_MEMORY);
> -            return;
> -         }
> -         (*gc->fillImage) (gc, 1, width, 1, 1, format, type, row, buf,
> -                           pixelHeaderPC);
> +      /* Allocate a temporary holding buffer */
> +      buf = (GLubyte *) Xmalloc(bufsize);
> +      if (!buf) {
> +         __glXSetError(gc, GL_OUT_OF_MEMORY);
> +         return;
> +      }
> +      (*gc->fillImage) (gc, 1, width, 1, 1, format, type, row, buf,
> +                        pixelHeaderPC);
>
> -         (*gc->fillImage) (gc, 1, height, 1, 1, format, type, column,
> -                           buf + image1len, pixelHeaderPC);
> +      (*gc->fillImage) (gc, 1, height, 1, 1, format, type, column,
> +                        buf + image1len, pixelHeaderPC);
>
> -         /* Send large command */
> -         __glXSendLargeCommand(gc, gc->pc, (GLint) (pc - gc->pc), buf,
> -                               bufsize);
> -         /* Free buffer */
> -         Xfree((char *) buf);
> -      }
> -      else {
> -         /* Just send the data straight as is */
> -         __glXSendLargeCommand(gc, gc->pc, (GLint) (pc - gc->pc), pc,
> -                               bufsize);
> -      }
> +      /* Send large command */
> +      __glXSendLargeCommand(gc, gc->pc, (GLint) (pc - gc->pc), buf,
> +                            bufsize);
> +      /* Free buffer */
> +      Xfree((char *) buf);
>      }
>   }

Reviewed-by: Brian Paul <brianp at vmware.com>