[PATCH] Avoid unnecessarily re-allocating texture buffer when the size hasn't changed.
Kristian Høgsberg
hoegsberg at gmail.com
Thu Jun 6 21:25:20 PDT 2013
On Wed, Jun 05, 2013 at 11:47:40AM +0300, Ander Conselvan de Oliveira wrote:
> 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
Yup, you're right, thanks for looking into this. Sinclair updated the
patch to add the texture damage from the buffer size, by dividing down
by the surface scale. I think it might make more sense just keeping
the texture_damage region in buffer coordinates, but for now, it's all
working here.
> >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
> >
>
More information about the wayland-devel
mailing list