Michel Dänzer <[email protected]> writes: > On 24.10.2014 07:09, Andreas Hartmetz wrote: >> >> commit bbcc084afc1396d3df42516baafea5839c95a488 >> Author: Andreas Hartmetz <[email protected]> >> Date: Sat Oct 4 18:13:04 2014 +0200 >> >> glamor: Don't free memory we are going to use. >> >> glamor_color_convert_to_bits() returns its second argument on >> success, NULL on error, and need_free_bits already makes sure that >> "bits" aliasing converted_bits is freed in the success case. >> Looks like the memory leak that was supposed to be fixed in >> 6e50bfa706cd3ab884c933bf1f17c221a6208aa4 only occurred in the error >> case. >> >> diff --git a/glamor/glamor_pixmap.c b/glamor/glamor_pixmap.c >> index 355fe4b..7c9bf26 100644 >> --- a/glamor/glamor_pixmap.c >> +++ b/glamor/glamor_pixmap.c >> @@ -774,8 +774,8 @@ _glamor_upload_bits_to_pixmap_texture(PixmapPtr >> pixmap, GLenum format, >> return FALSE; >> bits = glamor_color_convert_to_bits(bits, converted_bits, w, h, >> stride, no_alpha, revert, >> swap_rb); >> - free(converted_bits); >> if (bits == NULL) { >> + free(converted_bits); >> ErrorF("Failed to convert pixmap no_alpha %d," >> "revert mode %d, swap mode %d\n", no_alpha, revert, >> swap_rb); >> return FALSE; >> >> I've already, a week or two ago, sent this to Michel a who reviewed it >> (no idea how to forward that, guess I'll need another review from >> whoever volunteers) and OK'd it. > > You could have just submitted the patch with my > > Reviewed-by: Michel Dänzer <[email protected]> > > to this list and to Keith Packard (Cc'd, though that probably won't be > visible in my post as distributed by the list), as I suggested. :)
Looks like there's a path which will leak these bits still -- the fast path doesn't reach the call to free the bits. Can either of you review this patch and I'll merge both in?
From a0c5169f3199e915bcfcf8e880893271cbf11472 Mon Sep 17 00:00:00 2001 From: Keith Packard <[email protected]> Date: Fri, 24 Oct 2014 11:56:23 -0700 Subject: [PATCH] glamor: Free converted bits in _glamor_upload_bits_to_pixmap_texture fast path When uploading bits to a texture which need reformatting to match a supported GL format, a temporary buffer is allocated to hold the reformatted bits. This gets freed in the general path, but is not freed in the fast path because that includes an early return before the call to free. This patch removes the early return and places the general case under an 'else' block, so that both paths reach the call to free. Signed-off-by: Keith Packard <[email protected]> --- glamor/glamor_pixmap.c | 73 +++++++++++++++++++++++++------------------------- 1 file changed, 36 insertions(+), 37 deletions(-) diff --git a/glamor/glamor_pixmap.c b/glamor/glamor_pixmap.c index 355fe4b..af2e945 100644 --- a/glamor/glamor_pixmap.c +++ b/glamor/glamor_pixmap.c @@ -808,45 +808,44 @@ _glamor_upload_bits_to_pixmap_texture(PixmapPtr pixmap, GLenum format, format, type, x + fbo_x_off, y + fbo_y_off, w, h, bits, pbo); - return TRUE; + } else { + ptexcoords = texcoords_inv; + + pixmap_priv_get_dest_scale(pixmap_priv, &dst_xscale, &dst_yscale); + glamor_set_normalize_vcoords(pixmap_priv, dst_xscale, + dst_yscale, + x, y, + x + w, y + h, + vertices); + /* Slow path, we need to flip y or wire alpha to 1. */ + glamor_make_current(glamor_priv); + glVertexAttribPointer(GLAMOR_VERTEX_POS, 2, GL_FLOAT, + GL_FALSE, 2 * sizeof(float), vertices); + glEnableVertexAttribArray(GLAMOR_VERTEX_POS); + glVertexAttribPointer(GLAMOR_VERTEX_SOURCE, 2, GL_FLOAT, + GL_FALSE, 2 * sizeof(float), ptexcoords); + glEnableVertexAttribArray(GLAMOR_VERTEX_SOURCE); + + glamor_set_destination_pixmap_priv_nc(pixmap_priv); + __glamor_upload_pixmap_to_texture(pixmap, &tex, + format, type, 0, 0, w, h, bits, pbo); + glActiveTexture(GL_TEXTURE0); + glBindTexture(GL_TEXTURE_2D, tex); + + glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MIN_FILTER, GL_NEAREST); + glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MAG_FILTER, GL_NEAREST); + glUseProgram(glamor_priv->finish_access_prog[no_alpha]); + glUniform1i(glamor_priv->finish_access_revert[no_alpha], revert); + glUniform1i(glamor_priv->finish_access_swap_rb[no_alpha], swap_rb); + + glDrawArrays(GL_TRIANGLE_FAN, 0, 4); + + glDisableVertexAttribArray(GLAMOR_VERTEX_POS); + glDisableVertexAttribArray(GLAMOR_VERTEX_SOURCE); + glDeleteTextures(1, &tex); + glBindFramebuffer(GL_FRAMEBUFFER, 0); } - ptexcoords = texcoords_inv; - - pixmap_priv_get_dest_scale(pixmap_priv, &dst_xscale, &dst_yscale); - glamor_set_normalize_vcoords(pixmap_priv, dst_xscale, - dst_yscale, - x, y, - x + w, y + h, - vertices); - /* Slow path, we need to flip y or wire alpha to 1. */ - glamor_make_current(glamor_priv); - glVertexAttribPointer(GLAMOR_VERTEX_POS, 2, GL_FLOAT, - GL_FALSE, 2 * sizeof(float), vertices); - glEnableVertexAttribArray(GLAMOR_VERTEX_POS); - glVertexAttribPointer(GLAMOR_VERTEX_SOURCE, 2, GL_FLOAT, - GL_FALSE, 2 * sizeof(float), ptexcoords); - glEnableVertexAttribArray(GLAMOR_VERTEX_SOURCE); - - glamor_set_destination_pixmap_priv_nc(pixmap_priv); - __glamor_upload_pixmap_to_texture(pixmap, &tex, - format, type, 0, 0, w, h, bits, pbo); - glActiveTexture(GL_TEXTURE0); - glBindTexture(GL_TEXTURE_2D, tex); - - glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MIN_FILTER, GL_NEAREST); - glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MAG_FILTER, GL_NEAREST); - glUseProgram(glamor_priv->finish_access_prog[no_alpha]); - glUniform1i(glamor_priv->finish_access_revert[no_alpha], revert); - glUniform1i(glamor_priv->finish_access_swap_rb[no_alpha], swap_rb); - - glDrawArrays(GL_TRIANGLE_FAN, 0, 4); - - glDisableVertexAttribArray(GLAMOR_VERTEX_POS); - glDisableVertexAttribArray(GLAMOR_VERTEX_SOURCE); - glDeleteTextures(1, &tex); - glBindFramebuffer(GL_FRAMEBUFFER, 0); - if (need_free_bits) free(bits); return TRUE; -- 2.1.1
-- [email protected]
pgpeQ493QcRJO.pgp
Description: PGP signature
_______________________________________________ [email protected]: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
