[PATCH] Avoid unnecessarily re-allocating texture buffer when the size hasn't changed.

[Kristian Høgsberg]

On Thu, May 23, 2013 at 01:18:29PM -0700, Sinclair Yeh wrote:
> v2:
> Fixed the wrong comparison
> 
> v1:
> Depending on specific DRI driver implementation, glTexImage2D() with data
> set to NULL may or may not re-allocate the texture buffer each time it is
> called.  Unintended consequences happen if later glTexSubImage2D() is called
> to only update a sub-region of the texture buffer.
> 
> I've explored moving glTexImage2D() from gl_renderer_attach() and simply
> mark the texture dirty, but the current implemention seems cleaner because
> I won't have to worry about calling ensure_textures() and re-assigning
> gs->shader unnecessarily.
> ---
>  src/gl-renderer.c |   32 ++++++++++++++++++++------------
>  1 files changed, 20 insertions(+), 12 deletions(-)
> 
> diff --git a/src/gl-renderer.c b/src/gl-renderer.c
> index be74eba..c508825 100644
> --- a/src/gl-renderer.c
> +++ b/src/gl-renderer.c
> @@ -68,6 +68,7 @@ struct gl_surface_state {
>  
>  	struct weston_buffer_reference buffer_ref;
>  	int pitch; /* in pixels */
> +	int height;
>  };
>  
>  struct gl_renderer {
> @@ -1213,18 +1214,24 @@ gl_renderer_attach(struct weston_surface *es, struct wl_buffer *buffer)
>  	}
>  
>  	if (wl_buffer_is_shm(buffer)) {
> -		gs->pitch = wl_shm_buffer_get_stride(buffer) / 4;
> -		gs->target = GL_TEXTURE_2D;
> -
> -		ensure_textures(gs, 1);
> -		glBindTexture(GL_TEXTURE_2D, gs->textures[0]);
> -		glTexImage2D(GL_TEXTURE_2D, 0, GL_BGRA_EXT,
> -			     gs->pitch, buffer->height, 0,
> -			     GL_BGRA_EXT, GL_UNSIGNED_BYTE, NULL);
> -		if (wl_shm_buffer_get_format(buffer) == WL_SHM_FORMAT_XRGB8888)
> -			gs->shader = &gr->texture_shader_rgbx;
> -		else
> -			gs->shader = &gr->texture_shader_rgba;
> +		/* Only allocate a texture if it doesn't match existing one */
> +		if (((wl_shm_buffer_get_stride(buffer) / 4) != gs->pitch) ||
> +		    (buffer->height != gs->height)) {

I would prefer a little fewer parentheses here - only the () required
by if are necessary.  Nitpicks aside, we need to also call
glTexImage2D in case we're switching from a egl buffer to a shm
buffer.  If we don't the texture will remain backed by the EGLImage
from the previous frame and the glTexSubImage2D in flush_damage will
write into that buffer.

The best way to check if we're coming from a EGLImage buffer is to
see if gs->num_images is > 0, which I'd just add to the if statement.

> +			gs->pitch = wl_shm_buffer_get_stride(buffer) / 4;
> +			gs->height = buffer->height;
> +			gs->target = GL_TEXTURE_2D;
> +
> +			ensure_textures(gs, 1);
> +			glBindTexture(GL_TEXTURE_2D, gs->textures[0]);
> +			glTexImage2D(GL_TEXTURE_2D, 0, GL_BGRA_EXT,
> +				     gs->pitch, buffer->height, 0,
> +				     GL_BGRA_EXT, GL_UNSIGNED_BYTE, NULL);
> +			if (wl_shm_buffer_get_format(buffer) ==
> +			    WL_SHM_FORMAT_XRGB8888)
> +				gs->shader = &gr->texture_shader_rgbx;
> +			else
> +				gs->shader = &gr->texture_shader_rgba;

We need to always do the gs->shader assignment - a buffer can change
pixel without changing size and we need to change the shader in that
case.

> +		}
>  	} else if (gr->query_buffer(gr->egl_display, buffer,
>  				    EGL_TEXTURE_FORMAT, &format)) {
>  		for (i = 0; i < gs->num_images; i++)
> @@ -1279,6 +1286,7 @@ gl_renderer_attach(struct weston_surface *es, struct wl_buffer *buffer)
>  		}
>  
>  		gs->pitch = buffer->width;
> +		gs->height = buffer->height;
>  	} else {
>  		weston_log("unhandled buffer type!\n");
>  		weston_buffer_reference(&gs->buffer_ref, NULL);
> -- 
> 1.7.7.6
> 
> _______________________________________________
> wayland-devel mailing list
> wayland-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/wayland-devel