[PATCH] Avoid unnecessarily re-allocating texture buffer when the size hasn't changed.
Kristian Høgsberg
hoegsberg at gmail.com
Thu Jun 6 22:09:08 PDT 2013
On Fri, Jun 07, 2013 at 12:25:20AM -0400, Kristian Høgsberg wrote:
> 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.
Looking at this again, I think the current approach is fine. I was
hoping to store the texture_damage in buffer coordinates since that's
more natural for the glTexSubImage2D loop. But since we have to union
in the surface damage in flush_damage, we would have to loop though
the rects in the surface damage, scale them up and union them one by
one. That's a lot more work and likely less efficient than just
unioning the two regions.
> > >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