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

[Ander Conselvan de Oliveira]

On 06/04/2013 08:06 AM, Kristian Høgsberg wrote:
> On Tue, May 28, 2013 at 05:28:49PM -0700, Sinclair Yeh wrote:
>> v3:
>> * Removed unnecessary parentheses
>> * Added check for switching from EGL image to SHM buffer
>> * Moved shader assignment out of IF condition
>>
>> 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 |   31 +++++++++++++++++++++++--------
>>   1 files changed, 23 insertions(+), 8 deletions(-)
>>
>> diff --git a/src/gl-renderer.c b/src/gl-renderer.c
>> index be74eba..aa8f581 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,31 @@ 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;
>> +		/* Only allocate a texture if it doesn't match existing one.
>> + 		 * If gs->num_images is not 0, then a switch from DRM allocated
>> + 		 * buffer to a SHM buffer is happening, and we need to allocate
>> + 		 * a new texture buffer.
>> + 		 */
>> +		if (wl_shm_buffer_get_stride(buffer) / 4 != gs->pitch ||
>> +		    buffer->height != gs->height ||
>> +		    gs->num_images > 0) {
>> +			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);
>
> Looking better now, but we still need to make sure we do a full upload
> before we repaint.  If a client is clever about it's resizing and only
> posts a partial damage request after attaching the new buffer, we end
> up reallocating the texture storage but not uploading the full
> contents.
>
> However, weird behavior #1: This shouldn't happen with any of the
> clients I have (they all post full damage on resize), yet, I still see
> flicker when resizing.
>
> Weird behavior #2: The fix should be something like this:
>
> +			pixman_region32_union_rect(&gs->texture_damage,
> +						   &gs->texture_damage,
> +						   0, 0,
> +						   es->geometry.width,
> +						   es->geometry.height);
>
> after the glTexImage2D call, to make sure we upload all of the
> texture.  Instead of always uploading the contents (which would upload
> twice: immediately and then when we flush damage), we add the new
> surface rectangle to texture_damage.  However, when I add that, the
> window contents just sometimes completely disappears...

That happens when reducing the size. The new buffer dimensions are not 
propagated to the surface until the call to surface->configure() that 
comes after the attach call. In that case, you add a rectangle to 
texture_damage that is bigger than the texture itself. When Weston tries 
to upload this rectangle with glTexSubImage2D(), the check for the 
rectangle being inside of the texture fails and nothing is uploaded.

I guess it would make sense to just make texture damage equal to the 
size of the texture. But there's a problem that texture_damage is in 
surface coordinates, so we either convert the buffer dimensions to that 
at this point, or change it to be in actual buffer coordinates.

Cheers,
Ander

> This is what's holding up the patch - I don't have a lot of time to
> look into it right now, but it is something I want to get in this week
> so any help is apprecaited.
>
> Kristian
>
>> +		}
>>
>> -		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)
>> +		if (wl_shm_buffer_get_format(buffer) ==
>> +		    WL_SHM_FORMAT_XRGB8888)
>>   			gs->shader = &gr->texture_shader_rgbx;
>>   		else
>>   			gs->shader = &gr->texture_shader_rgba;
>> +
>>   	} else if (gr->query_buffer(gr->egl_display, buffer,
>>   				    EGL_TEXTURE_FORMAT, &format)) {
>>   		for (i = 0; i < gs->num_images; i++)
>> @@ -1279,6 +1293,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
> _______________________________________________
> wayland-devel mailing list
> wayland-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/wayland-devel
>