[Date Prev][Date Next] [Thread Prev][Thread Next] [Date Index] [Thread Index]

Bug#930756: marked as done (unblock: movit/1.6.2-2)

Your message dated Thu, 20 Jun 2019 12:20:36 +0200
with message-id <b8a5f448-9555-4a68-71a4-f29001d082f9@debian.org>
and subject line Re: Bug#930756: unblock: movit/1.6.2-2
has caused the Debian Bug report #930756,
regarding unblock: movit/1.6.2-2
to be marked as done.

This means that you claim that the problem has been dealt with.
If this is not the case it is now your responsibility to reopen the
Bug report if necessary, and/or fix the problem forthwith.

(NB: If you are a system administrator and have no idea what this
message is talking about, this may indicate a serious mail system
misconfiguration somewhere. Please contact owner@bugs.debian.org
immediately.)


-- 
930756: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=930756
Debian Bug Tracking System
Contact owner@bugs.debian.org with problems
--- Begin Message --- 
Package: release.debian.org
Severity: normal
User: release.debian.org@packages.debian.org
Usertags: unblock

Please unblock package movit

It contains a single, focused fix for a severity=important bug that I found,
backported from upstream git.

It's a bit unclear to me whether “corrupted display” would count as appropriate
for buster unblocks, but I thought it might go under the “crashes etc.” heading
(it's technically undefined behavior with a thread race, but I don't think
there are any real GL drivers where this actually causes _crashes_ per se).
If not, please let me know.

From the upstream commit message; especially the part about ABI stability at
the end should be relevant:

    Fix an issue where temporary textures could be reused too early by a different thread.
    
    When an EffectChain is done rendering (ie., has submitted all of the GL
    rendering commands that it needs to), it releases all of the temporary
    textures it's used back to a common freelist.
    
    However, if another thread needs a texture of the same size and format,
    it could be picking it off of the freelist before the GPU has actually
    completed rendering the first thread's command list, and start uploading
    into it. This is undefined behavior in OpenGL, and can create garbled
    output depending on timing and driver. (I've seen this on at least the
    classic Intel Mesa driver.)
    
    Fix by setting fences, so that getting a texture from the freelist
    will have an explicit ordering versus the previous work. This increases the
    size of ResourcePool::TextureFormat, but it is only ever used in a private
    std::map. std::map is node-based (it has to, since the C++ standard requires
    iterators to it to be stable), and thus, sizeof(TextureFormat) does not factor
    into sizeof(ResourcePool), and thus, there is no ABI break. Verified by
    checking on libstdc++.

diff -Nru movit-1.6.2/debian/changelog movit-1.6.2/debian/changelog
--- movit-1.6.2/debian/changelog	2018-03-18 16:25:30.000000000 +0100
+++ movit-1.6.2/debian/changelog	2019-06-15 20:08:45.000000000 +0200
@@ -1,3 +1,11 @@
+movit (1.6.2-2) unstable; urgency=high
+
+  * fix-temporary-texture-race-issue.diff: New patch backported from upstream
+    git, fixes a threading issue where a thread could start reusing a temporary
+    texture while it's still being used in rendering. (Closes: #930570)
+
+ -- Steinar H. Gunderson <sesse@debian.org>  Sat, 15 Jun 2019 20:08:45 +0200
+
 movit (1.6.2-1) unstable; urgency=medium
 
   * New upstream release.
diff -Nru movit-1.6.2/debian/patches/fix-temporary-texture-race-issue.diff movit-1.6.2/debian/patches/fix-temporary-texture-race-issue.diff
--- movit-1.6.2/debian/patches/fix-temporary-texture-race-issue.diff	1970-01-01 01:00:00.000000000 +0100
+++ movit-1.6.2/debian/patches/fix-temporary-texture-race-issue.diff	2019-06-15 20:08:37.000000000 +0200
@@ -0,0 +1,50 @@
+Index: movit-1.6.2/resource_pool.cpp
+===================================================================
+--- movit-1.6.2.orig/resource_pool.cpp
++++ movit-1.6.2/resource_pool.cpp
+@@ -42,6 +42,7 @@ ResourcePool::~ResourcePool()
+ 	for (GLuint free_texture_num : texture_freelist) {
+ 		assert(texture_formats.count(free_texture_num) != 0);
+ 		texture_freelist_bytes -= estimate_texture_size(texture_formats[free_texture_num]);
++		glDeleteSync(texture_formats[free_texture_num].no_reuse_before);
+ 		texture_formats.erase(free_texture_num);
+ 		glDeleteTextures(1, &free_texture_num);
+ 		check_error();
+@@ -337,7 +338,10 @@ GLuint ResourcePool::create_2d_texture(G
+ 		    format_it->second.height == height) {
+ 			texture_freelist_bytes -= estimate_texture_size(format_it->second);
+ 			texture_freelist.erase(freelist_it);
++			GLsync sync = format_it->second.no_reuse_before;
+ 			pthread_mutex_unlock(&lock);
++			glWaitSync(sync, 0, GL_TIMEOUT_IGNORED);
++			glDeleteSync(sync);
+ 			return texture_num;
+ 		}
+ 	}
+@@ -449,12 +453,14 @@ void ResourcePool::release_2d_texture(GL
+ 	texture_freelist.push_front(texture_num);
+ 	assert(texture_formats.count(texture_num) != 0);
+ 	texture_freelist_bytes += estimate_texture_size(texture_formats[texture_num]);
++	texture_formats[texture_num].no_reuse_before = glFenceSync(GL_SYNC_GPU_COMMANDS_COMPLETE, 0);
+ 
+ 	while (texture_freelist_bytes > texture_freelist_max_bytes) {
+ 		GLuint free_texture_num = texture_freelist.back();
+ 		texture_freelist.pop_back();
+ 		assert(texture_formats.count(free_texture_num) != 0);
+ 		texture_freelist_bytes -= estimate_texture_size(texture_formats[free_texture_num]);
++		glDeleteSync(texture_formats[free_texture_num].no_reuse_before);
+ 		texture_formats.erase(free_texture_num);
+ 		glDeleteTextures(1, &free_texture_num);
+ 		check_error();
+Index: movit-1.6.2/resource_pool.h
+===================================================================
+--- movit-1.6.2.orig/resource_pool.h
++++ movit-1.6.2/resource_pool.h
+@@ -211,6 +211,7 @@ private:
+ 	struct Texture2D {
+ 		GLint internal_format;
+ 		GLsizei width, height;
++		GLsync no_reuse_before = nullptr;
+ 	};
+ 
+ 	// A mapping from texture number to format details. This is filled if the
diff -Nru movit-1.6.2/debian/patches/series movit-1.6.2/debian/patches/series
--- movit-1.6.2/debian/patches/series	1970-01-01 01:00:00.000000000 +0100
+++ movit-1.6.2/debian/patches/series	2019-06-15 20:08:08.000000000 +0200
@@ -0,0 +1 @@
+fix-temporary-texture-race-issue.diff

unblock movit/1.6.2-2

-- System Information:
Debian Release: 10.0
  APT prefers testing-proposed-updates
  APT policy: (500, 'testing-proposed-updates'), (500, 'testing-debug'), (500, 'testing'), (500, 'stable'), (1, 'experimental')
Architecture: amd64 (x86_64)
Foreign Architectures: i386

Kernel: Linux 5.1.11 (SMP w/40 CPU cores)
Locale: LANG=en_DK.UTF-8, LC_CTYPE=en_DK.UTF-8 (charmap=UTF-8), LANGUAGE=en_NO:en_US:en_GB:en (charmap=UTF-8)
Shell: /bin/sh linked to /bin/dash
Init: systemd (via /run/systemd/system)

--- End Message ---
--- Begin Message --- 
Hi Steinar,

On 20-06-2019 00:38, Steinar H. Gunderson wrote:
> unblock movit/1.6.2-2

Unblocked, thanks.

Paul

Attachment: signature.asc
Description: OpenPGP digital signature


--- End Message ---
Reply to: